diff --git a/.changeset/qa-http-adapter-mount-discovery.md b/.changeset/qa-http-adapter-mount-discovery.md new file mode 100644 index 0000000000..91f05bb943 --- /dev/null +++ b/.changeset/qa-http-adapter-mount-discovery.md @@ -0,0 +1,38 @@ +--- +"@objectstack/core": patch +--- + +fix(qa): `HttpTestAdapter` resolves the Data Protocol mount from the server's `/discovery`, and falls back to the convention loudly (#7983) + +The record-shaped `os test` action types (`create_record`, `read_record`, +`update_record`, `delete_record`, `query_records`) built their URLs from the +**defaults** of `RestApiConfigSchema.apiPath` and +`CrudEndpointsConfigSchema.dataPrefix`, because the adapter is handed an origin +and nothing else. A deployment that moved the mount got a 404 that reads like the +suite author's own URL mistake rather than a platform limitation. + +The adapter now asks the server, following the `getRoute` precedent in +`@objectstack/client`: **one memoised `GET {apiBase}/discovery` per run** (`os +test` builds one adapter for the whole run), addressing whatever `routes.data` +advertises, with the schema-derived convention as the fallback. Measured on a +booted stack (REST route generator + dispatcher bridge), before and after: + +| deployment | before | after | +|---|---|---| +| stock | created | created | +| `crud.dataPrefix: '/objects'` | `HTTP Error 404` | created | +| `api.apiPath: '/api/2026-01'` | `HTTP Error 404` | `HTTP Error 404`, now naming the mount | + +The `apiPath` row is **not** closed, and the reason is structural: `apiPath` +moves the base that `/discovery` is itself mounted under, so the document that +would name the new mount sits behind the prefix that is missing. The one +discovery document at a fixed path does not rescue it — `/.well-known/objectstack` +advertises the **dispatcher's** `${prefix}/data`, measured as `/api/v1/data` +under all three configs above — so it is deliberately not probed: trusting it +would attach a false provenance ("discovery told us") to the same 404. + +Instead that case degrades loudly. Falling back to the convention prints a +warning naming the mount it will address, the probe that failed and the remedy, +and every 404/405 from a record action now carries the mount it addressed and +where that mount came from. `api_call` is unchanged, issues no probe, and remains +the escape hatch for a host the probe cannot reach. diff --git a/content/docs/deployment/cli.mdx b/content/docs/deployment/cli.mdx index 3d2fd4ed93..36a0a77f9a 100644 --- a/content/docs/deployment/cli.mdx +++ b/content/docs/deployment/cli.mdx @@ -1054,11 +1054,22 @@ success forever. Pass **`--fail-on-empty`** to opt into the strict reading, wher an empty match exits 1 (#7848). The **record-shaped** action types — `create_record`, `read_record`, -`update_record`, `delete_record`, `query_records` — address the Data Protocol at -its default mount (`/api/v1/data`, i.e. `{apiPath}{crud.dataPrefix}`). A -deployment that moves that mount by setting `api.apiPath` or `crud.dataPrefix` is -out of reach for them; write those steps as `api_call`, which takes the path you -give it. `run_script` has no adapter branch at all and fails by name. +`update_record`, `delete_record`, `query_records` — **ask the server where the +Data Protocol is mounted** instead of assuming it. Once per run, `os test` +fetches `{apiBase}/discovery` and addresses whatever `routes.data` advertises, +so a deployment that moves the mount with `crud.dataPrefix` is reached without +you telling it anything. When the probe cannot answer, the run falls back to the +convention `{apiPath}{crud.dataPrefix}` (`/api/v1/data`) and **says so**: a +warning naming the mount it will address and the probe that failed, and the same +statement appended to every 404 a record step gets — so a wrong mount reads as a +wrong mount, not as your own URL mistake. + +One case survives that fallback by construction: setting **`api.apiPath`** moves +the discovery document itself out from under the probe, and no fixed-path +document reports the REST mount (`/.well-known/objectstack` advertises the +dispatcher's own prefix, not this one). Against such a host, write those steps as +`api_call`, which takes the path you give it. `run_script` has no adapter branch +at all and fails by name. #### `os doctor` diff --git a/docs/qa/platform-checklist/areas/cli.json b/docs/qa/platform-checklist/areas/cli.json index ac5fa89ce8..a1adc61bfb 100644 --- a/docs/qa/platform-checklist/areas/cli.json +++ b/docs/qa/platform-checklist/areas/cli.json @@ -433,7 +433,7 @@ "title": "os test: a Quality Protocol suite is validated at LOAD, executed against a booted app, and its verdict is the exit code — capture/interpolation thread state, an unevaluable assertion FAILS", "since": "v17", "status": "active", - "revision": 2, + "revision": 3, "priority": "P1", "surface": "cli", "personas": ["operator (local shell)", "suite author (writes qa/*.test.json)"], @@ -447,7 +447,7 @@ ], "knownGaps": [ "`run_script` has no adapter branch and fails by name (`Unsupported action type in HttpAdapter: run_script`) — the variant sweep records ONE refusal and that is the honest verdict, not a fixture gap to work around. The other seven members execute since #7848; the five record-shaped ones did not until then (see `negative`), so a sweep transcript predating that fix shows five 404s and is not comparable", - "the record action types address the Data Protocol at its DEFAULT mount only (`{apiPath}{crud.dataPrefix}` = `/api/v1/data`). A deployment that sets `api.apiPath` or `crud.dataPrefix` moves the mount out from under them; the fixture boot uses stock config, so this does not bite here — a run against a re-prefixed host must write those steps as `api_call`", + "the record action types resolve the Data Protocol mount from the server's own `/discovery` (`routes.data`) since #7983, so a deployment that sets `crud.dataPrefix` IS reached; what remains out of reach is `api.apiPath`, which moves the discovery document itself out from under the probe (measured: `{apiBase}/discovery` 404s, and `/.well-known/objectstack` reports the DISPATCHER's `/api/v1/data`, not the REST mount). Against such a host the adapter falls back to the convention and says so — it names the mount it addressed, the probe that failed and the remedy, on the warning AND on every 404 — and those steps must still be written as `api_call`. The fixture boot is stock, so neither case bites here", "no scenario SELECTION exists: `os test` has exactly two flags (--url, --token), `scenario.tags` filters nothing and `scenario.requires` is never checked (packages/spec/liveness/qa.json rows `tags`/`requires`), so the whole glob always runs and a suite cannot declare a precondition it will be skipped for" ] }, @@ -541,7 +541,7 @@ "source": [ "packages/cli/src/commands/test.ts (the shipped `os test`: the #7363 lazy segment-directed glob with its prune list, `loadTestSuite`'s #6247 boundary parse, the per-scenario report, and the exit 0/1 summary)", "packages/core/src/qa/runner.ts (scenario sequencing, `capture` + `{{var}}` interpolation, the assertion operators, setup/teardown semantics, the #7256 unevaluable-`contains` fix)", - "packages/core/src/qa/http-adapter.ts (the action-type switch — its case labels ARE the enum values; the record routes derive their prefix from RestApiConfigSchema + CrudEndpointsConfigSchema rather than spelling `/api/v1/data` out, #7848)", + "packages/core/src/qa/http-adapter.ts (the action-type switch — its case labels ARE the enum values; the record routes take their prefix from the one memoised `/discovery` probe per run, falling back to the RestApiConfigSchema + CrudEndpointsConfigSchema convention with a diagnostic that names the mount, #7848 / #7983)", "packages/spec/src/qa/testing.zod.ts (TestSuiteSchema — the shape enforced at load; TestActionTypeSchema pinned above)", "packages/spec/liveness/qa.json (the ADR-0049 ledger whose existence this item is coverage.json's mapping for — its dead `tags`/`requires` rows are why no scenario selection exists)", "packages/cli/test/qa-suite-schema-load.test.ts, packages/cli/test/resolve-glob-lazy-walk.test.ts, packages/core/src/qa/runner.test.ts (the three unit pins — cited so a run knows what is already covered, NOT a substitute for driving a booted app)", @@ -550,7 +550,8 @@ ], "history": [ { "revision": 1, "date": "2026-08-11", "change": "new item: the `qa` capability's coverage.json mapping, authored rather than waived (#7347 triage ruling). `os test` is a shipped, documented CLI command, so the honest mapping is a surface:cli item that authors a real qa/*.test.json suite and drives it against a booted app — the fixture suite examples/app-showcase/qa/platform-smoke.test.json lands with this item and is the repo's first Quality Protocol suite. Every clause was measured on showcase before it was written: the green path, capture/interpolation, the #6247 load refusal, the #7256 unevaluable-contains failure, teardown-after-failure, the 8-member action-type sweep and the #7363 glob. `since: v17` records the release in which the surface became GOVERNED (liveness ledger seeded + TestSuiteSchema enforced at the load site, #6247 / PR #7255); the command itself predates it. No `automated` entry: the three unit pins cover pieces, none of them proves a suite reaches a real server", "ref": "#7347" }, - {"revision": 2, "date": "2026-08-12", "change": "the adapter was repaired, which this item's own `negative` clause declared to be a revision rather than a silent green (#7848). Item 1: the five record-shaped action types now round-trip against a stock server — the `${baseUrl}/api/data/:object` literal became a prefix DERIVED from the two schemas RestServer itself resolves from (RestApiConfigSchema `apiPath ?? {basePath}/{version}` + CrudEndpointsConfigSchema.dataPrefix), and `update_record` PATCHes where it used to PUT a route that has no PUT sibling. Re-measured on a booted showcase, one scenario per member with NO shared setup so no member's verdict is inferred from a sibling: 7 of 8 execute and assert, `run_script` still refuses by name. The `negative` clause is inverted accordingly — a 404 from a record action type is the regression now — and the knownGap it rested on is replaced by the narrower one that survives: the record types address the DEFAULT mount only, so a host that moves it with `api.apiPath`/`crud.dataPrefix` still needs `api_call`. Item 2: a zero-match glob still exits 0, deliberately (a repo that legitimately ships no suites must not start failing CI), but the posture is now DECLARED — stated in `--help`, opt out with the new `--fail-on-empty`, and `Found N test suites.` is emitted on EVERY run including `Found 0 test suites.`, which is the line this item's first acceptance clause already asks a run record to quote", "ref": "#7848"} + {"revision": 2, "date": "2026-08-12", "change": "the adapter was repaired, which this item's own `negative` clause declared to be a revision rather than a silent green (#7848). Item 1: the five record-shaped action types now round-trip against a stock server — the `${baseUrl}/api/data/:object` literal became a prefix DERIVED from the two schemas RestServer itself resolves from (RestApiConfigSchema `apiPath ?? {basePath}/{version}` + CrudEndpointsConfigSchema.dataPrefix), and `update_record` PATCHes where it used to PUT a route that has no PUT sibling. Re-measured on a booted showcase, one scenario per member with NO shared setup so no member's verdict is inferred from a sibling: 7 of 8 execute and assert, `run_script` still refuses by name. The `negative` clause is inverted accordingly — a 404 from a record action type is the regression now — and the knownGap it rested on is replaced by the narrower one that survives: the record types address the DEFAULT mount only, so a host that moves it with `api.apiPath`/`crud.dataPrefix` still needs `api_call`. Item 2: a zero-match glob still exits 0, deliberately (a repo that legitimately ships no suites must not start failing CI), but the posture is now DECLARED — stated in `--help`, opt out with the new `--fail-on-empty`, and `Found N test suites.` is emitted on EVERY run including `Found 0 test suites.`, which is the line this item's first acceptance clause already asks a run record to quote", "ref": "#7848"}, + {"revision": 3, "date": "2026-08-17", "change": "the record action types stopped ASSUMING the mount (#7983). They now resolve it from the server: one memoised `GET {apiBase}/discovery` per run, addressing whatever `routes.data` advertises, with the RestApiConfigSchema + CrudEndpointsConfigSchema convention as the fallback — the `@objectstack/client` `getRoute` pattern, copied rather than re-invented. Measured on a booted stack (REST generator + dispatcher bridge) before and after, three configs: stock stays green; `crud.dataPrefix: '/objects'` went from `HTTP Error 404` to a created record, so that row of the gap is CLOSED; `api.apiPath: '/api/2026-01'` still 404s and is NOT closed — `apiPath` moves the discovery document itself, and the one fixed-path document (`/.well-known/objectstack`) advertises the dispatcher's `/api/v1/data` under all three configs, so trusting it would attach a false provenance to the same 404. That row is narrowed instead: the fallback is announced (a warning naming the mount, the failed probe and the remedy) and every 404 from a record action now carries the mount it addressed and where that mount came from, so the failure can no longer read as the suite author's own URL mistake. `api_call` is unchanged and probes nothing — it remains the escape hatch for a host the probe cannot reach", "ref": "#7983"} ] }, { diff --git a/packages/core/src/qa/http-adapter.test.ts b/packages/core/src/qa/http-adapter.test.ts index fd6331e8a4..81053d9211 100644 --- a/packages/core/src/qa/http-adapter.test.ts +++ b/packages/core/src/qa/http-adapter.test.ts @@ -25,11 +25,19 @@ * own URL rather than a platform defect. * * What these tests pin is the URL and the VERB per action type, against a - * captured `fetch` — the wire statement, without needing a server. The base - * path is asserted against the spec schemas the server itself resolves from - * (`RestApiConfigSchema` + `CrudEndpointsConfigSchema`), never against a second - * copy of the literal `/api/v1/data`: a pin that hard-codes the string it is - * guarding goes green the day the schema moves and the adapter does not. + * captured `fetch` — the wire statement, without needing a server. + * + * PIN (#7983) — the mount is now RESOLVED from the server, not assumed. + * + * The adapter asks `{apiBase}/discovery` once per run and addresses whatever + * `routes.data` advertises; the schema-derived convention is the fallback, and + * taking it is announced loudly (Route & surface ownership §3). So the record + * URLs below are pinned against a discovery document that deliberately + * advertises a NON-default mount — a pin written against the literal + * `/api/v1/data` would pass while the probe was ignored entirely, and would go + * red for #9292's reasons rather than this adapter's. The convention pins keep + * asserting the schemas (`RestApiConfigSchema` + `CrudEndpointsConfigSchema`), + * never a second copy of the literal they produce. */ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; @@ -39,13 +47,27 @@ import { HttpTestAdapter } from './http-adapter.js'; const BASE_URL = 'http://localhost:3000'; +/** What `RestServer.getApiBasePath()` evaluates. */ +const EXPECTED_API_BASE = (() => { + const api = RestApiConfigSchema.parse({}); + return api.apiPath ?? `${api.basePath}/${api.version}`; +})(); + /** What `RestServer` composes: `getApiBasePath()` + `crud.dataPrefix`. */ const EXPECTED_DATA_PATH = (() => { - const api = RestApiConfigSchema.parse({}); const crud = CrudEndpointsConfigSchema.parse({}); - return `${api.apiPath ?? `${api.basePath}/${api.version}`}${crud.dataPrefix}`; + return `${EXPECTED_API_BASE}${crud.dataPrefix}`; })(); +/** Where `registerDiscoveryEndpoints` mounts the document the adapter probes. */ +const EXPECTED_DISCOVERY_URL = `${BASE_URL}${EXPECTED_API_BASE}/discovery`; + +/** + * The mount this suite's server advertises — deliberately NOT the convention, + * so every record-URL assertion below fails if the probe is ignored. + */ +const ADVERTISED_DATA_PATH = '/api/v1/objects'; + interface Call { url: string; method: string; @@ -55,6 +77,10 @@ interface Call { let calls: Call[]; let fetchMock: ReturnType; +let warnings: string[]; + +/** What the discovery probe gets back; per-test overridable. */ +let discoveryReply: () => Response | Promise; function jsonResponse(body: unknown, status = 200): Response { return new Response(JSON.stringify(body), { @@ -63,8 +89,15 @@ function jsonResponse(body: unknown, status = 200): Response { }); } +/** A discovery document shaped like the one `@objectstack/rest` answers. */ +function discoveryDocument(routes: Record): Response { + return jsonResponse({ version: 'v1', apiName: 'ObjectStack', routes }); +} + beforeEach(() => { calls = []; + warnings = []; + discoveryReply = () => discoveryDocument({ data: ADVERTISED_DATA_PATH, metadata: '/api/v1/meta' }); // `input` is deliberately `unknown`: this package's tsc program has no DOM lib, // so `RequestInfo` does not resolve here — and the assertions want the URL as a // string anyway. @@ -75,49 +108,61 @@ beforeEach(() => { body: typeof init?.body === 'string' ? JSON.parse(init.body) : undefined, headers: (init?.headers ?? {}) as Record, }); + if (String(input) === EXPECTED_DISCOVERY_URL) return discoveryReply(); return jsonResponse({ ok: true }); }); vi.stubGlobal('fetch', fetchMock); + vi.spyOn(console, 'warn').mockImplementation((...args: unknown[]) => { + warnings.push(args.map(String).join(' ')); + }); }); afterEach(() => { vi.unstubAllGlobals(); + vi.restoreAllMocks(); }); function action(type: QA.TestActionType, target: string, payload?: Record): QA.TestAction { return { type, target, ...(payload ? { payload } : {}) } as QA.TestAction; } +/** Every call the adapter made that was not the one-per-run discovery probe. */ +function actionCalls(): Call[] { + return calls.filter((c) => c.url !== EXPECTED_DISCOVERY_URL); +} + async function run(a: QA.TestAction): Promise { const adapter = new HttpTestAdapter(BASE_URL); await adapter.execute(a, {}); - expect(calls).toHaveLength(1); - return calls[0]; + const acted = actionCalls(); + expect(acted).toHaveLength(1); + return acted[0]; } describe('[#7848] HttpTestAdapter record action types reach the served route', () => { - it('derives the data path from the schemas the server resolves from', () => { + it('derives the convention from the schemas the server resolves from', () => { // Not a tautology: this is the one assertion that would fail if the // adapter went back to writing the prefix out by hand. expect(EXPECTED_DATA_PATH).toBe('/api/v1/data'); + expect(EXPECTED_API_BASE).toBe('/api/v1'); }); it('create_record POSTs the collection URL', async () => { const call = await run(action('create_record', 'crm_account', { name: 'Acme' })); - expect(call.url).toBe(`${BASE_URL}${EXPECTED_DATA_PATH}/crm_account`); + expect(call.url).toBe(`${BASE_URL}${ADVERTISED_DATA_PATH}/crm_account`); expect(call.method).toBe('POST'); expect(call.body).toEqual({ name: 'Acme' }); }); it('read_record GETs the record URL', async () => { const call = await run(action('read_record', 'crm_account', { id: 'rec_1' })); - expect(call.url).toBe(`${BASE_URL}${EXPECTED_DATA_PATH}/crm_account/rec_1`); + expect(call.url).toBe(`${BASE_URL}${ADVERTISED_DATA_PATH}/crm_account/rec_1`); expect(call.method).toBe('GET'); }); it('update_record PATCHes the record URL — the route has no PUT sibling', async () => { const call = await run(action('update_record', 'crm_account', { id: 'rec_1', name: 'Acme II' })); - expect(call.url).toBe(`${BASE_URL}${EXPECTED_DATA_PATH}/crm_account/rec_1`); + expect(call.url).toBe(`${BASE_URL}${ADVERTISED_DATA_PATH}/crm_account/rec_1`); expect(call.method).toBe('PATCH'); // `id` addressed the record; the body is the field patch, not a column write. expect(call.body).toEqual({ name: 'Acme II' }); @@ -125,20 +170,20 @@ describe('[#7848] HttpTestAdapter record action types reach the served route', ( it('delete_record DELETEs the record URL', async () => { const call = await run(action('delete_record', 'crm_account', { id: 'rec_1' })); - expect(call.url).toBe(`${BASE_URL}${EXPECTED_DATA_PATH}/crm_account/rec_1`); + expect(call.url).toBe(`${BASE_URL}${ADVERTISED_DATA_PATH}/crm_account/rec_1`); expect(call.method).toBe('DELETE'); }); it('query_records POSTs the QueryAST to the collection query URL', async () => { const call = await run(action('query_records', 'crm_account', { filters: [['name', '=', 'Acme']] })); - expect(call.url).toBe(`${BASE_URL}${EXPECTED_DATA_PATH}/crm_account/query`); + expect(call.url).toBe(`${BASE_URL}${ADVERTISED_DATA_PATH}/crm_account/query`); expect(call.method).toBe('POST'); expect(call.body).toEqual({ filters: [['name', '=', 'Acme']] }); }); it('percent-encodes the record id so an id with a slash cannot forge a path', async () => { const call = await run(action('read_record', 'crm_account', { id: 'a/b c' })); - expect(call.url).toBe(`${BASE_URL}${EXPECTED_DATA_PATH}/crm_account/a%2Fb%20c`); + expect(call.url).toBe(`${BASE_URL}${ADVERTISED_DATA_PATH}/crm_account/a%2Fb%20c`); }); it('no record action type addresses the old unversioned `/api/data` path', async () => { @@ -146,16 +191,162 @@ describe('[#7848] HttpTestAdapter record action types reach the served route', ( calls = []; const adapter = new HttpTestAdapter(BASE_URL); await adapter.execute(action(type, 'crm_account', { id: 'rec_1' }), {}); - expect(calls[0].url.startsWith(`${BASE_URL}/api/data/`)).toBe(false); + expect(actionCalls()[0].url.startsWith(`${BASE_URL}/api/data/`)).toBe(false); } }); }); +describe('[#7983] the data mount is resolved from the server, once per run', () => { + it('probes the discovery document at the base path the server mounts it under', async () => { + await run(action('create_record', 'crm_account', { name: 'Acme' })); + const probes = calls.filter((c) => c.url === EXPECTED_DISCOVERY_URL); + expect(probes).toHaveLength(1); + expect(probes[0].method).toBe('GET'); + }); + + it('addresses whatever routes.data advertises — a re-prefixed mount is reached', async () => { + discoveryReply = () => discoveryDocument({ data: '/api/2026-01/records' }); + const call = await run(action('create_record', 'crm_account', { name: 'Acme' })); + expect(call.url).toBe(`${BASE_URL}/api/2026-01/records/crm_account`); + }); + + it('probes ONCE per adapter, however many record actions run', async () => { + const adapter = new HttpTestAdapter(BASE_URL); + await adapter.execute(action('create_record', 'crm_account', { name: 'Acme' }), {}); + await adapter.execute(action('read_record', 'crm_account', { id: 'rec_1' }), {}); + await adapter.execute(action('query_records', 'crm_account', {}), {}); + expect(calls.filter((c) => c.url === EXPECTED_DISCOVERY_URL)).toHaveLength(1); + expect(actionCalls()).toHaveLength(3); + }); + + it('probes once even when record actions start concurrently', async () => { + const adapter = new HttpTestAdapter(BASE_URL); + await Promise.all([ + adapter.execute(action('create_record', 'crm_account', { name: 'A' }), {}), + adapter.execute(action('create_record', 'crm_account', { name: 'B' }), {}), + adapter.execute(action('read_record', 'crm_account', { id: 'rec_1' }), {}), + ]); + expect(calls.filter((c) => c.url === EXPECTED_DISCOVERY_URL)).toHaveLength(1); + }); + + it('sends the bearer token on the probe — a host that gates discovery still answers', async () => { + const adapter = new HttpTestAdapter(BASE_URL, 'tok_123'); + await adapter.execute(action('read_record', 'crm_account', { id: 'rec_1' }), {}); + const probe = calls.find((c) => c.url === EXPECTED_DISCOVERY_URL)!; + expect(probe.headers['Authorization']).toBe('Bearer tok_123'); + }); + + it('reads the dispatcher bridge\'s `{ data: … }` envelope as well as the bare document', async () => { + discoveryReply = () => jsonResponse({ data: { routes: { data: '/api/v1/things' } } }); + const call = await run(action('read_record', 'crm_account', { id: 'rec_1' })); + expect(call.url).toBe(`${BASE_URL}/api/v1/things/crm_account/rec_1`); + }); + + it('says nothing when the probe answered — no false alarm on a stock host', async () => { + await run(action('create_record', 'crm_account', { name: 'Acme' })); + expect(warnings).toEqual([]); + }); +}); + +describe('[#7983] falling back to the convention degrades LOUDLY', () => { + /** Each row is a way the probe fails to settle the mount. */ + const failures: Array<[label: string, reply: () => Response, evidence: RegExp]> = [ + ['discovery is not mounted (404)', () => jsonResponse({ error: 'Not found' }, 404), /answered 404/], + ['discovery is disabled (503)', () => jsonResponse({ error: 'nope' }, 503), /answered 503/], + ['the document carries no routes.data', () => discoveryDocument({ metadata: '/api/v1/meta' }), /carried no routes\.data/], + ['routes.data is not a usable path', () => discoveryDocument({ data: '' }), /carried no routes\.data/], + ]; + + for (const [label, reply, evidence] of failures) { + it(`falls back and names the mount when ${label}`, async () => { + discoveryReply = reply; + const call = await run(action('create_record', 'crm_account', { name: 'Acme' })); + expect(call.url).toBe(`${BASE_URL}${EXPECTED_DATA_PATH}/crm_account`); + expect(warnings).toHaveLength(1); + // The mount it resolved… + expect(warnings[0]).toContain(`${BASE_URL}${EXPECTED_DATA_PATH}`); + // …why it resolved that way… + expect(warnings[0]).toMatch(evidence); + expect(warnings[0]).toContain(EXPECTED_DISCOVERY_URL); + // …and the remedy, which is the part a bare 404 never carries. + expect(warnings[0]).toContain('api_call'); + expect(warnings[0]).toContain('api.apiPath'); + }); + } + + it('falls back when the host cannot be reached at all', async () => { + discoveryReply = () => { throw new Error('ECONNREFUSED'); }; + const call = await run(action('read_record', 'crm_account', { id: 'rec_1' })); + expect(call.url).toBe(`${BASE_URL}${EXPECTED_DATA_PATH}/crm_account/rec_1`); + expect(warnings[0]).toContain('ECONNREFUSED'); + }); + + it('warns ONCE per run, not once per action', async () => { + discoveryReply = () => jsonResponse({ error: 'Not found' }, 404); + const adapter = new HttpTestAdapter(BASE_URL); + await adapter.execute(action('create_record', 'crm_account', { name: 'Acme' }), {}); + await adapter.execute(action('read_record', 'crm_account', { id: 'rec_1' }), {}); + expect(warnings).toHaveLength(1); + }); + + it('⛔ never probes /.well-known/objectstack — it advertises the dispatcher prefix, not the REST mount', async () => { + discoveryReply = () => jsonResponse({ error: 'Not found' }, 404); + await run(action('create_record', 'crm_account', { name: 'Acme' })); + expect(calls.some((c) => c.url.includes('/.well-known/'))).toBe(false); + }); +}); + +describe('[#7983] a failed record action carries the mount it addressed', () => { + it('appends the mount and its provenance to a 404', async () => { + discoveryReply = () => jsonResponse({ error: 'Not found' }, 404); + fetchMock.mockImplementation(async (input: unknown) => { + if (String(input) === EXPECTED_DISCOVERY_URL) return jsonResponse({ error: 'Not found' }, 404); + return jsonResponse({ error: 'Not found' }, 404); + }); + const adapter = new HttpTestAdapter(BASE_URL); + await expect(adapter.execute(action('create_record', 'crm_account', { name: 'Acme' }), {})).rejects.toThrow( + /HTTP Error 404: .*record actions addressed http:\/\/localhost:3000\/api\/v1\/data \(data mount assumed by convention/, + ); + }); + + it('names discovery as the source when discovery answered', async () => { + fetchMock.mockImplementation(async (input: unknown) => { + if (String(input) === EXPECTED_DISCOVERY_URL) { + return discoveryDocument({ data: ADVERTISED_DATA_PATH }); + } + return jsonResponse({ error: 'Not found' }, 404); + }); + const adapter = new HttpTestAdapter(BASE_URL); + await expect(adapter.execute(action('read_record', 'crm_account', { id: 'rec_1' }), {})).rejects.toThrow( + /record actions addressed http:\/\/localhost:3000\/api\/v1\/objects \(data mount resolved from discovery/, + ); + }); + + it('leaves a non-routing failure undecorated — the server\'s own message stands alone', async () => { + fetchMock.mockImplementation(async (input: unknown) => { + if (String(input) === EXPECTED_DISCOVERY_URL) { + return discoveryDocument({ data: ADVERTISED_DATA_PATH }); + } + return jsonResponse({ error: 'name is required' }, 422); + }); + const adapter = new HttpTestAdapter(BASE_URL); + const err = await adapter + .execute(action('create_record', 'crm_account', {}), {}) + .then(() => undefined, (e: Error) => e); + expect(err?.message).toBe('HTTP Error 422: {"error":"name is required"}'); + }); +}); + describe('[#7848] the three non-record action types are unchanged', () => { it('api_call resolves a relative target against the base URL', async () => { - const call = await run(action('api_call', '/api/v1/discovery', { method: 'GET' })); - expect(call.url).toBe(`${BASE_URL}/api/v1/discovery`); - expect(call.method).toBe('GET'); + // Not routed through `run()`: this target IS the discovery path, and the + // helper filters that URL out as the probe. `api_call` never probes, so + // the single recorded call is the action's own — which is the point. + const adapter = new HttpTestAdapter(BASE_URL); + await adapter.execute(action('api_call', `${EXPECTED_API_BASE}/discovery`, { method: 'GET' }), {}); + expect(calls).toHaveLength(1); + expect(calls[0].url).toBe(EXPECTED_DISCOVERY_URL); + expect(calls[0].method).toBe('GET'); }); it('api_call leaves an absolute target alone', async () => { @@ -163,6 +354,22 @@ describe('[#7848] the three non-record action types are unchanged', () => { expect(call.url).toBe('http://example.test/health'); }); + it('[#7983] api_call issues NO discovery probe — it takes the path it is given', async () => { + const adapter = new HttpTestAdapter(BASE_URL); + await adapter.execute(action('api_call', '/api/2026-01/data/crm_account', { method: 'GET' }), {}); + expect(calls).toHaveLength(1); + expect(calls[0].url).toBe(`${BASE_URL}/api/2026-01/data/crm_account`); + }); + + it('[#7983] an api_call failure is not decorated with a mount it never used', async () => { + fetchMock.mockImplementation(async () => jsonResponse({ error: 'Not found' }, 404)); + const adapter = new HttpTestAdapter(BASE_URL); + const err = await adapter + .execute(action('api_call', '/nope', { method: 'GET' }), {}) + .then(() => undefined, (e: Error) => e); + expect(err?.message).toBe('HTTP Error 404: {"error":"Not found"}'); + }); + it('wait resolves without touching the network', async () => { const adapter = new HttpTestAdapter(BASE_URL); const result = await adapter.execute(action('wait', 'n/a', { duration: 1 }), {}); @@ -185,7 +392,8 @@ describe('[#7848] auth and impersonation headers still ride along', () => { { type: 'create_record', target: 'crm_account', payload: { name: 'Acme' }, user: 'alice' } as QA.TestAction, {}, ); - expect(calls[0].headers['Authorization']).toBe('Bearer tok_123'); - expect(calls[0].headers['X-Run-As']).toBe('alice'); + const call = actionCalls()[0]; + expect(call.headers['Authorization']).toBe('Bearer tok_123'); + expect(call.headers['X-Run-As']).toBe('alice'); }); }); diff --git a/packages/core/src/qa/http-adapter.ts b/packages/core/src/qa/http-adapter.ts index 0e5c5cbe0c..c1daa841f9 100644 --- a/packages/core/src/qa/http-adapter.ts +++ b/packages/core/src/qa/http-adapter.ts @@ -4,11 +4,12 @@ import * as QA from '@objectstack/spec/qa'; import { RestApiConfigSchema, CrudEndpointsConfigSchema } from '@objectstack/spec/api'; import { TestExecutionAdapter } from './adapter.js'; -/** Memoised {@link defaultDataPath} — the schemas are `lazySchema`, so build them once. */ -let dataPathCache: string | undefined; +/** Memoised {@link conventionMounts} — the schemas are `lazySchema`, so build them once. */ +let conventionCache: { apiBase: string; dataPath: string } | undefined; /** - * The path prefix a stock ObjectStack server serves the Data Protocol under. + * The paths a STOCK ObjectStack server serves the API base and the Data + * Protocol under, derived from the two schemas `RestServer` resolves from. * * ## [#7848] Why this is derived and not written down * @@ -28,32 +29,172 @@ let dataPathCache: string | undefined; * - `CrudEndpointsConfigSchema.dataPrefix` — what `RestServer` appends to it * to get `dataPath` (`/data`). * - * Defaults only: this adapter is handed an origin, not a deployment's config, - * so a host that overrides `api.apiPath` or `crud.dataPrefix` is still out of - * reach here (tracked separately — the `api_call` action type is the escape - * hatch until then). What the derivation buys is that the DEFAULT can never - * again disagree with the schema that declares it. + * These are the CONVENTION: what the schemas default to, which is what a + * deployment serves until it says otherwise. `HttpTestAdapter` is handed an + * origin and nothing else, so the convention is also all it can know before it + * asks the server — which is what {@link HttpTestAdapter.resolveDataMount} + * does (#7983). The `/discovery` path is derived here for the same reason the + * data path is: `RestServer.registerDiscoveryEndpoints` mounts it at + * `${getApiBasePath()}/discovery`, so a second literal would be a second place + * to forget. */ -function defaultDataPath(): string { - if (dataPathCache === undefined) { +function conventionMounts(): { apiBase: string; dataPath: string } { + if (conventionCache === undefined) { const api = RestApiConfigSchema.parse({}); const crud = CrudEndpointsConfigSchema.parse({}); - dataPathCache = `${api.apiPath ?? `${api.basePath}/${api.version}`}${crud.dataPrefix}`; + const apiBase = api.apiPath ?? `${api.basePath}/${api.version}`; + conventionCache = { apiBase, dataPath: `${apiBase}${crud.dataPrefix}` }; } - return dataPathCache; + return conventionCache; +} + +/** + * Where this adapter decided the Data Protocol is mounted, and on what evidence. + * + * `why` is not decoration — it is the whole difference between a 404 that reads + * as the suite author's own URL mistake and one that names the mount it used + * and where that mount came from (Route & surface ownership §3). + */ +interface ResolvedDataMount { + /** The path prefix record actions address, e.g. `/api/v1/data`. */ + readonly path: string; + /** `discovery` = the server told us; `convention` = the schemas' defaults. */ + readonly source: 'discovery' | 'convention'; + /** One clause naming the probe and its outcome. */ + readonly why: string; } export class HttpTestAdapter implements TestExecutionAdapter { + /** + * The single discovery probe of a run, memoised as the in-flight promise so + * concurrent record actions share one request rather than racing N. + * + * `os test` builds ONE adapter for the whole run (`packages/cli/src/commands/ + * test.ts`) and hands it to every suite, so instance scope IS run scope. + */ + private mountPromise?: Promise; + constructor(private baseUrl: string, private authToken?: string) {} - /** `{baseUrl}{apiBasePath}{dataPrefix}/{object}` — the collection URL. */ - private collectionUrl(objectName: string): string { - return `${this.baseUrl}${defaultDataPath()}/${encodeURIComponent(objectName)}`; + /** The resolved data mount; probes at most once per adapter. */ + private dataMount(): Promise { + if (this.mountPromise === undefined) { + this.mountPromise = this.resolveDataMount(); + } + return this.mountPromise; + } + + /** + * Ask the server where it serves the Data Protocol, and fall back to the + * convention — loudly — when it cannot say. + * + * ## [#7983] What the probe recovers, measured rather than assumed + * + * `@objectstack/client` answers the same question through discovery + * (`getRoute`, `packages/client/src/index.ts`), and this follows it: prefer + * the server's own `routes.data`, fall back to the convention. Measured on a + * booted stack (REST generator + dispatcher bridge, three configs): + * + * | deployment | `{apiBase}/discovery` | serves | + * |--------------------------------|-----------------------|---------------| + * | stock | 200 `/api/v1/data` | `/api/v1/data`| + * | `crud.dataPrefix: '/objects'` | 200 `/api/v1/objects` | `/api/v1/objects` | + * | `api.apiPath: '/api/2026-01'` | **404** | `/api/2026-01/data` | + * + * So the probe closes the `dataPrefix` row exactly: `RestServer`'s discovery + * handler substitutes the configured prefix into `routes.data`, and reading + * it is strictly better than recomputing it here. The `apiPath` row it cannot + * close, and the reason is structural rather than an oversight — `apiPath` + * moves the base that discovery itself is mounted under, so the document that + * would name the new mount is behind the very prefix we are missing. + * + * ⛔ And the one discovery document at a FIXED path does not rescue it: + * `/.well-known/objectstack` is mounted at the site root by the dispatcher + * bridge, but its `routes.data` is the DISPATCHER's own `${prefix}/data` — + * measured as `/api/v1/data` under all three configs above, including the two + * where the server serves elsewhere. Falling back to it would turn "we could + * not resolve the mount" into "discovery told us `/api/v1/data`": the same + * 404, now with a false provenance attached. Not probed, deliberately. + * + * Hence: one probe, then a diagnostic that NAMES the mount, the evidence and + * the remedy. `api_call` takes the path it is given and is unaffected either + * way — it stays the escape hatch for a host this cannot reach. + */ + private async resolveDataMount(): Promise { + const { apiBase, dataPath } = conventionMounts(); + const probeUrl = `${this.baseUrl}${apiBase}/discovery`; + let why: string; + + try { + const headers: Record = {}; + if (this.authToken) { + headers['Authorization'] = `Bearer ${this.authToken}`; + } + const response = await fetch(probeUrl, { method: 'GET', headers }); + if (response.ok) { + const body = await response.json(); + // `@objectstack/rest` answers the document bare; the dispatcher bridge + // wraps it as `{ data: … }`. Pick the object that actually carries + // `routes` rather than `body.data || body` — the document's own + // `routes.data` key makes the looser test ambiguous to read here. + const doc = (body && typeof body === 'object' && 'routes' in body) + ? (body as { routes?: Record }) + : ((body as { data?: { routes?: Record } } | null)?.data); + const advertised = doc?.routes?.data; + if (typeof advertised === 'string' && advertised.length > 0) { + return { + path: advertised, + source: 'discovery', + why: `GET ${probeUrl} advertised routes.data`, + }; + } + why = `GET ${probeUrl} answered ${response.status} but carried no routes.data`; + } else { + why = `GET ${probeUrl} answered ${response.status}`; + } + } catch (error) { + why = `GET ${probeUrl} could not be reached (${(error as Error).message})`; + } + + const mount: ResolvedDataMount = { path: dataPath, source: 'convention', why }; + // Absence must be loud (Route & surface ownership §3): state the mount that + // will be addressed, the evidence for it, and what to do about a host this + // cannot reach — never leave the bare 404 to be diagnosed. + console.warn( + `[HttpTestAdapter] Data Protocol mount NOT resolved from discovery. ` + + `Record actions (create_record, read_record, update_record, delete_record, query_records) ` + + `will address ${this.baseUrl}${dataPath}, the convention declared by RestApiConfigSchema ` + + `(apiPath ?? basePath/version) + CrudEndpointsConfigSchema.dataPrefix. Evidence: ${why}. ` + + `A deployment that sets crud.dataPrefix is normally recovered by this probe; one that sets ` + + `api.apiPath moves discovery itself out from under it, and no fixed-path document reports ` + + `the REST mount — write those steps as api_call, which takes the path you give it.`, + ); + return mount; + } + + /** `{baseUrl}{dataMount}/{object}` — the collection URL. */ + private collectionUrl(mount: ResolvedDataMount, objectName: string): string { + return `${this.baseUrl}${mount.path}/${encodeURIComponent(objectName)}`; } /** `{collection}/{id}` — the single-record URL. */ - private recordUrl(objectName: string, id: unknown): string { - return `${this.collectionUrl(objectName)}/${encodeURIComponent(String(id))}`; + private recordUrl(mount: ResolvedDataMount, objectName: string, id: unknown): string { + return `${this.collectionUrl(mount, objectName)}/${encodeURIComponent(String(id))}`; + } + + /** + * The provenance clause appended to a failed record action's error. + * + * The card this closes is about a 404 that reads like the author's own URL + * mistake; the mount is the one fact that distinguishes the two, so it rides + * on the failure itself rather than only on a warning printed earlier in the + * transcript. + */ + private mountNote(mount: ResolvedDataMount): string { + const how = mount.source === 'discovery' + ? 'resolved from discovery' + : 'assumed by convention — the server was not able to state it'; + return `record actions addressed ${this.baseUrl}${mount.path} (data mount ${how}: ${mount.why})`; } async execute(action: QA.TestAction, _context: Record): Promise { @@ -90,65 +231,74 @@ export class HttpTestAdapter implements TestExecutionAdapter { } private async createRecord(objectName: string, data: Record, headers: Record) { - const response = await fetch(this.collectionUrl(objectName), { + const mount = await this.dataMount(); + const response = await fetch(this.collectionUrl(mount, objectName), { method: 'POST', headers, body: JSON.stringify(data) }); - return this.handleResponse(response); + return this.handleResponse(response, this.mountNote(mount)); } private async updateRecord(objectName: string, data: Record, headers: Record) { const { id, ...fields } = data; if (!id) throw new Error('Update record requires id in payload'); - // PATCH, not PUT: `PATCH {apiPath}/data/:object/:id` is the route the server + // PATCH, not PUT: `PATCH {dataMount}/:object/:id` is the route the server // registers, and there is no PUT sibling — the old verb 404'd even once the // path was right (#7848). The body is the field patch, so `id` is peeled off // rather than posted back as a column write. - const response = await fetch(this.recordUrl(objectName, id), { + const mount = await this.dataMount(); + const response = await fetch(this.recordUrl(mount, objectName, id), { method: 'PATCH', headers, body: JSON.stringify(fields) }); - return this.handleResponse(response); + return this.handleResponse(response, this.mountNote(mount)); } private async deleteRecord(objectName: string, data: Record, headers: Record) { const id = data.id; if (!id) throw new Error('Delete record requires id in payload'); - const response = await fetch(this.recordUrl(objectName, id), { + const mount = await this.dataMount(); + const response = await fetch(this.recordUrl(mount, objectName, id), { method: 'DELETE', headers }); - return this.handleResponse(response); + return this.handleResponse(response, this.mountNote(mount)); } private async readRecord(objectName: string, data: Record, headers: Record) { const id = data.id; if (!id) throw new Error('Read record requires id in payload'); - const response = await fetch(this.recordUrl(objectName, id), { + const mount = await this.dataMount(); + const response = await fetch(this.recordUrl(mount, objectName, id), { method: 'GET', headers }); - return this.handleResponse(response); + return this.handleResponse(response, this.mountNote(mount)); } private async queryRecords(objectName: string, data: Record, headers: Record) { - // `POST {apiPath}/data/:object/query` — the spec-shape advanced query + // `POST {dataMount}/:object/query` — the spec-shape advanced query // (QueryAST in the body), the same route `client.data.query()` posts to. - const response = await fetch(`${this.collectionUrl(objectName)}/query`, { + const mount = await this.dataMount(); + const response = await fetch(`${this.collectionUrl(mount, objectName)}/query`, { method: 'POST', headers, body: JSON.stringify(data) }); - return this.handleResponse(response); + return this.handleResponse(response, this.mountNote(mount)); } private async rawApiCall(endpoint: string, data: Record, headers: Record) { + // Deliberately does NOT consult the resolved mount — `api_call` takes the + // path it is given, which is what makes it the escape hatch for a host + // the discovery probe cannot reach (#7983). It probes nothing either: a + // suite written entirely in `api_call` steps issues no discovery request. const method = (data.method as string) || 'GET'; const body = data.body ? JSON.stringify(data.body) : undefined; const url = endpoint.startsWith('http') ? endpoint : `${this.baseUrl}${endpoint}`; - + const response = await fetch(url, { method, headers, @@ -157,10 +307,23 @@ export class HttpTestAdapter implements TestExecutionAdapter { return this.handleResponse(response); } - private async handleResponse(response: Response) { + private async handleResponse(response: Response, mountNote?: string) { if (!response.ok) { const text = await response.text(); - throw new Error(`HTTP Error ${response.status}: ${text}`); + // The `HTTP Error : ` prefix is unchanged — a suite author + // (and every transcript predating #7983) reads the same first clause; + // the mount provenance is APPENDED, never substituted. + // + // Appended on 404/405 only: those are the statuses a wrong mount + // produces (nothing registered at the path / registered under another + // verb), and they are exactly the ones this card calls unreadable. A + // 400 or 422 reached the right route and got a real answer from it — + // decorating those with the mount would bury the server's own message + // under a URL the author has no reason to doubt. + const wrongUrlShaped = response.status === 404 || response.status === 405; + throw new Error( + `HTTP Error ${response.status}: ${text}${wrongUrlShaped && mountNote ? ` — ${mountNote}` : ''}`, + ); } const contentType = response.headers.get('content-type'); if (contentType && contentType.includes('application/json')) {