feat(project-engine-client): stateful mock core — store, resource ops, seeds (LLMO-5460)#1665
feat(project-engine-client): stateful mock core — store, resource ops, seeds (LLMO-5460)#1665aliciadriani wants to merge 15 commits into
Conversation
|
This PR will trigger a minor release when merged. |
Type-surface finding (surfaced by the new CI guard)The The mismatch:
Why it matters (footgun, not cosmetics): the client's public type ( Options:
Hard constraint (option 1): do the Parked for the #4 adoption (where real call sites make the cost and the exact shape-to-omit concrete; the transform is a non-trivial recursive mapped type). The client type itself lives in #1661. The type-test fixture moves in lockstep — whatever's decided, Tracked as an LLMO-5461 sub-task linked to the #4 adoption work. |
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Inline comments below carry the proposed fix for each finding from the review.
Process — Should Fix (not inline): the PR description is stale relative to the branch. The body says step 5 (Counterfact runner) and step 8 (E2E + CI wiring) are "Deferred to a follow-up PR," but this branch already contains run.js, the route handlers, __reset.js, the full E2E suite, and the new E2E (project-engine mock) CI job. The "Contents" list also omits context.js, run.js, the handlers, and the type-surface test. Please refresh the description to match the 24-file / +1579 diff so reviewers can size scope.
Fix: update the PR body — move steps 5 & 8 from "Deferred" to "Contents," and add context.js, run.js, the counterfact handlers, __reset.js, the E2E suite, and test/types/index.type-test.ts to the contents list.
| export function POST($) { | ||
| const { path, body, context } = $; | ||
| const created = context.ops.prompts.createMany( | ||
| { workspaceId: path.id, projectId: path.project_id }, | ||
| (body?.items ?? []).map((text) => ({ name: text })), | ||
| ); | ||
| const first = created[0] ?? {}; | ||
| return $.response[200].json({ id: first.id, name: first.name }); |
There was a problem hiding this comment.
Should Fix — this create handler is mounted on a path the spec and the consumer don't use.
The spec defines only delete on /aio/prompts (aio-delete-prompt-by-ids-v2) — there is no POST here. The real create is POST /aio/prompts/tagged (aio-create-prompts-with-tags) and the real read is POST /aio/prompts/by_tags (aio-list-prompts-by-tag-ids) — exactly what docs/mock-statefulness.md lists as the consumer's stateful prompt ops.
Consequences as written:
- The E2E
creates aio prompts (v2)case drives a non-specPOST, so there's no200response schema for Counterfact to validate the{ id, name }envelope against — the "response validation stays on" guarantee doesn't cover this handler. - When
spacecat-api-serviceruns against the mock, prompt create (tagged) and read (by_tags) hit Counterfact's stateless stubs, so the documented prompt write-then-read spine isn't actually stateful.
Fix: keep DELETE in this file, move create into a new aio/prompts/tagged.js, and add a aio/prompts/by_tags.js read handler (the store + ops already support both via createMany / list). Repoint the E2E case at /v2/.../aio/prompts/tagged.
// aio/prompts/tagged.js — POST create (operationId aio-create-prompts-with-tags)
export function POST($) {
const { path, body, context } = $;
const created = context.ops.prompts.createMany(
{ workspaceId: path.id, projectId: path.project_id },
(body?.items ?? []).map((text) => ({ name: text })),
);
const first = created[0] ?? {};
return $.response[200].json({ id: first.id, name: first.name });
}// aio/prompts/by_tags.js — POST list/read (operationId aio-list-prompts-by-tag-ids)
// shape the envelope to match the spec's 200 response for this op
export function POST($) {
const { path, context } = $;
const items = context.ops.prompts.list({ workspaceId: path.id, projectId: path.project_id });
return $.response[200].json({ items, page: 1, total: items.length });
}If this is intentionally deferred to the adoption PR, please scope it out in the description and drop the POST /aio/prompts row from the README's stateful-endpoints table, which currently advertises it as "create prompts."
There was a problem hiding this comment.
Resolved in cbe35f3 — the bogus POST /aio/prompts handler was removed and the correct paths were wired:
aio/prompts/tagged.js→POSTcreate (201,IDsWithStatsResponse)aio/prompts/by_tags.js→POSTlist (200,AIOPromptsWithStatusListResponse)aio/prompts.jsretains onlyDELETE→ 204
The E2E now drives tagged → by_tags with a real write-then-read assertion and response validation on, closing the false-coverage gap.
| { id: 'model-gpt4o', name: 'gpt-4o' }, | ||
| ], | ||
| [collectionKey('prompts', { workspaceId: WORKSPACE_ID, projectId: PROJECT_ID })]: [ | ||
| { id: 'prompt-1', text: 'What is the best running shoe?' }, |
There was a problem hiding this comment.
Nit — seed/created prompt field mismatch. Seed prompts use text, but prompts.createMany stores { name } and the create handler returns { id, name }. If a by_tags read handler is added (see the prompts.js comment), seeded vs. created prompts will have inconsistent shapes. Align the seed to name:
| { id: 'prompt-1', text: 'What is the best running shoe?' }, | |
| { id: 'prompt-1', name: 'What is the best running shoe?' }, |
There was a problem hiding this comment.
Resolved in cbe35f3 — seed prompts now use name (+ tags: []) matching the AIOPromptWithStatus shape, consistent with what createMany stores and by_tags returns.
| const id = entity.id ?? InMemoryStore.#generateId(); | ||
| /** @type {Entity} */ | ||
| const stored = { ...entity, id }; | ||
| this.#collection(name).set(id, stored); |
There was a problem hiding this comment.
Nit (optional, no change required). create with an explicit existing id silently overwrites via Map.set (a real API would 409). Fine for a mock; flagging only because seeds use fixed ids, so a seed-id collision would be swallowed.
If you want collisions to surface during dev, the minimal guard is:
if (entity.id && this.#collection(name).has(entity.id)) {
throw new Error(`duplicate id ${entity.id} in collection ${name}`);
}Otherwise leave as-is — overwrite-on-duplicate is acceptable mock behaviour.
There was a problem hiding this comment.
Added in 1f5c8cf — the guard throws on an explicit id collision:
if (entity.id && this.#collection(name).has(entity.id)) {
throw new Error(`duplicate id ${entity.id} in collection ${name}`);
}Seed-id collisions now surface immediately instead of being silently swallowed.
| { workspaceId: path.id, projectId: path.project_id }, | ||
| { model: { id: body.model_id }, prompts_count: 0 }, | ||
| ); | ||
| return $.response[200].json(created); |
There was a problem hiding this comment.
Nit (no change required). This POST returns 200 while the projects create returns 201. CI response-validation is on and green, so 200 is what the spec declares for add ai_model — the inconsistency is spec-driven, not a bug. Noting only for awareness.
There was a problem hiding this comment.
Acknowledged — spec-driven inconsistency, no action needed.
| } | ||
|
|
||
| /** DELETE — batch-delete prompts (body: { ids }). */ | ||
| export function DELETE($) { |
There was a problem hiding this comment.
Nit (no change required). prompts.removeMany (and ai_models.removeMany) compute a removed count that this handler discards — it always returns 204. The spec DELETE is 204, so this is correct; the count is exercised at the ops layer in stateful.test.js. Flagging only so the unused return value reads as deliberate.
There was a problem hiding this comment.
Acknowledged — the count is exercised at the ops layer; the handler deliberately discards it and always returns 204 per spec.
…-Data-Jwt header The generated `paths` mark `Auth-Data-Jwt` as a required per-operation header (faithful to the API contract), but the client injects that header at runtime via middleware (headers.set) and overwrites whatever a caller passes. So the typed surface was forcing every consumer to pass a value that's always ignored — a footgun (a "real" token passed there silently does nothing). Narrow the *exported* client type only: - Add a type-only mapped transform over the generated `paths` (ClientPaths) that removes `Auth-Data-Jwt` from each operation's header params. Auth-Data-Jwt is the sole header in the contract, so the emptied bag collapses to `never` — passing a header is now a compile error, not just optional, which actively closes the footgun. A genuine consumer header (if ever added) is preserved. - Type SerenityProjectEngineApiClient / the factory return against ClientPaths instead of raw Client<paths>. Generated types, the vendored spec, and the Pydantic models are untouched — they remain the honest API contract; only the client surface hides the header it supplies itself. Runtime (client.js, middleware) is unchanged. Validated with a throwaway tsc harness (not committed): a consumer calls an op without Auth-Data-Jwt (compiles), path params stay required, passing Auth-Data-Jwt is rejected, and the bogus-path drift canary still trips. Lint clean; unit suite 51 passing at 100% coverage (type-only change). Guard reconciliation (cross-PR): the type-surface guard fixture lives in #1665 (test/types/index.type-test.ts) and currently asserts the un-narrowed surface (it passes header: { 'Auth-Data-Jwt' }). That fixture must be updated when these land together at main — see the follow-up note on #1665. Relates to the #4 adoption, where consumer call sites drop the header argument. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Resolved — prompts stateful slice reconciled against the consumer and the spec (commit Follow-up to Should Fix #1 (the What was wrong. The stateful create handler was mounted on
Because the spec declares no The actual consumer flow (
Fix (option a — wire the correct handlers, repoint the E2E):
Validation. The E2E now exercises the real spec operations with response validation on, so the Spec quirk worth flagging. On |
…atching deployed transport (LLMO-5461) The wrapper authenticated by forwarding the raw IMS token as the `Auth-Data-Jwt` header. Against the only reachable Project Engine endpoint — the Adobe-hosted gateway — that 401s: the gateway authenticates `Authorization: Bearer <IMS>` and injects Semrush's native `Auth-Data-Jwt` server-side. This replicates the proven prod consumer, spacecat-api-service `src/support/serenity/rest-transport.js`, whose `buildHeaders()` sends `Authorization: Bearer` and whose comment notes the `Auth-Data-Jwt` branch was "deliberately removed". - client.js: authMiddleware now sets `Authorization: Bearer <token>` (was `Auth-Data-Jwt`); still no exchange/minting — the gateway exchanges the bearer server-side. - client.js: own the `/enterprise/projects/api` prefix like rest-transport — normalise baseUrl to its origin (drop any path/credentials) + the fixed prefix, and fail fast on an invalid baseUrl. Idempotent for callers that already include the prefix, so the unit tests and the #1665 mock e2e are unaffected. - index.d.ts: keep the type-narrowing pointed at `Auth-Data-Jwt`. The generated `paths` still mark it required, but the gateway injects it server-side, so a consumer must not pass it; dropping the narrowing would force them to. The new `Authorization` header is middleware-injected, outside the typed params, so it needs no narrowing. Comments/JSDoc updated to the gateway-exchange model. - tests: assert `Authorization: Bearer` (not `Auth-Data-Jwt`); add baseUrl normalisation + fail-fast coverage; reword the spec/title references. Gates: lint clean, 29 unit passing @ 100% coverage, throwaway tsc confirms the narrowing still holds (no-header call compiles; passing `Auth-Data-Jwt` is rejected). Contract basis: confirmed by Rainer Friederich's first-hand live test (`Authorization: Bearer` -> 200, `Auth-Data-Jwt` -> 401) plus the deployed rest-transport.js. NOT independently re-probed — our IMS identity is not provisioned on the Semrush side (the project's known no-live-access constraint), so a local probe 401s on every header. LLMO-5461 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Heads-up for the rebase onto #1661 (now that it carries the generation-time overlay 1. 2. The route prefix breaks when you switch specs. Counterfact honors a Swagger-2.0
Net diff to const SPEC = join(packageRoot, 'build', 'openapi3.json'); // was spec/projectengine_swagger_public.yaml
// ...ensure build/openapi3.json exists first (npm run generate)...
[findCounterfactBin(), SPEC, BASE_PATH, '--port', String(PORT), '--serve',
'--prefix', '/enterprise/projects/api', '--no-validate-request', '--no-update-check']Alternative (bigger change): keep feeding Swagger 2.0 so |
…ul mock Port the scaffold's generic, resource-agnostic InMemoryStore to JS + JSDoc (mocha + chai). This is the statefulness primitive for the Counterfact mock: collection-keyed CRUD plus seed/reset, with deep cloning on every read/write so a loaded seed snapshot is never mutated and reset() restores a pristine copy. Knows nothing about specific resources — the stateful handlers (projects, ai_models, prompts per the recommended first cut) plug their collections into it in a follow-up commit; non-stateful endpoints keep Counterfact's auto-generated schema response. 100% coverage; lint clean. LLMO-5460 (epic LLMO-5459) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e mock Confirm the spike against both consumers: spacecat-api-service is the only Project Engine consumer (llmo-data-retrieval-service makes zero calls), and the write-then-read spine is exactly projects, ai_models (per project), and prompts (aio, per project) — the recommended first cut. Documented as the AC floor in docs/mock-statefulness.md. - src/mock/stateful.js: the confirmed stateful set as pure operations over InMemoryStore (per-workspace/per-project collection scoping + the CRUD each group needs). No Counterfact or HTTP coupling, so it is unit-tested on its own; the runner adapts these into per-path handlers and maps results into spec-valid response envelopes. - src/mock/seeds.js: named seed sets (empty-workspace, workspace-with-data) keyed to the same collections, plus DEFAULT_SEED and SEED_IDS. Loaded via store.load(...); store.reset() restores between tests. 100% coverage; lint clean. LLMO-5460 (epic LLMO-5459) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Materializes the committed stateful handlers into a gitignored .counterfact scratch tree and launches Counterfact against the Project Engine spec. The shared in-memory store is wired in via a generated root Context, seeded by MOCK_SEED, with a __reset control route for E2E isolation. Validated end-to-end against a running server: projects list/create/get/ patch/delete, ai_models list/add, v2 aio prompts create, and __reset restoring seed state. Key runner details discovered against the live server: - Materialize the whole tree as .ts so Counterfact's transpiler emits .cjs with matching rewritten specifiers; .js sources emit CJS content under a "type": "module" package and fail to load. - Pass --serve explicitly so Counterfact does not default-enable codegen, which would append typed random() stubs onto our handlers (duplicate GET/POST). Transpile + load still run under --serve. - --no-validate-request: the spec marks Auth-Data-Jwt required but Node lowercases inbound header names, so validation 400s every request. run.js and the materialized handlers require a live server, so they are excluded from coverage; the unit-testable Context/store/ops keep 100%. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a Mock section covering MOCK_PORT/MOCK_SEED, the base URL, the stateful endpoints and the __reset control route, plus a note on how the runner materializes handlers. Drop the stale "later PR" marker now that the mock ships. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t the live mock Adds an env-gated (MOCK_E2E=1) E2E suite that boots the stateful Counterfact mock via src/mock/run.js and drives it through the real client: projects list/create/get/patch/delete, ai_models list/add, v2 aio prompts, and __reset isolation between cases. Self-managed lifecycle (spawn in before, process-group teardown in after). Kept deliberately out of the fast path: - file glob *.e2e.js (not *.test.js) so default `npm test` never loads it, preserving the unit suite's speed and 100% coverage with no live-server dep - `npm run test:e2e` runs it in isolation (--no-package so the package.json mocha spec/reporters don't bleed in) - a dedicated `E2E (project-engine mock)` CI job runs it, independent of the release gate - root eslint override allows devDependencies in *.e2e.js (the import rule's built-in test glob only covers *.test.js) Scoped to this package (client <-> mock contract); the api-service-level E2E rides with the adoption PR (LLMO-5461 #4, blocked on publish). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t + robust teardown Address review points on the live-mock E2E: - CI scope: add a cheap `changes` guard job (plain git merge-base diff, no third-party action) that the E2E job `needs`/`if`-gates, so Counterfact only boots when the project-engine-client package actually changed — not on every monorepo push. `on.paths` can't be used here: the workflow triggers on push, so a path filter would gate `test`/`release` too. - Ports: replace the hardcoded 4099 with an OS-assigned free port (MOCK_E2E_PORT still honoured for manual pinning) to avoid collisions across runs/jobs. - Teardown: after() now awaits the detached process group's exit (bounded) so the port is freed and no mock server is orphaned, including when before()/a test throws (mocha still runs after-hooks). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The typed client is this package's deliverable, but the public surface (hand-written src/index.d.ts) was unguarded: it can silently drift from the generated `paths` and degrade to `any`, and both the unit and E2E suites are type-blind (plain JS). Per repo convention every sibling package hand-writes its .d.ts and none emit from JSDoc, so rather than diverge to a checkJs build, add an additive consumer type-test: - test/types/index.type-test.ts: compile-only fixture asserting the factory returns Client<paths>, a generated operation stays typed, and bogus path/component keys are rejected (an @ts-expect-error canary that fails the build if the surface collapses to `any`). - tsconfig.types-test.json: tsc --noEmit, bundler resolution so `.js` import specifiers resolve to the .d.ts / generated .ts. - `npm run test:types`; pinned typescript devDep so a transitive-hoist change can't silently disable the guard. - CI: runs in the existing path-gated project-engine job (tsc-only, no server), so it only fires when the package or its generated types change. Verified the guard fails on real drift (client => any -> TS2578 unused-directive; factory return change -> TS2322) and is clean otherwise. Surfaced finding (tracked separately, not fixed here): the generated spec marks `Auth-Data-Jwt` as a required per-call header, so TS consumers must pass it even though the client injects it at runtime — a DX wart in the typed surface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ths (tagged/by_tags)
The stateful prompt handler was mounted on POST /aio/prompts, which the spec
defines as delete-only and no consumer calls — false coverage that also escaped
response validation (no 200 schema to check the envelope against). The real
spacecat-api-service prompt flow (rest-transport.js) is:
- create: POST /aio/prompts/tagged (body { prompts: { [tag]: [text] } })
- list: POST /aio/prompts/by_tags (body { tag_ids, page, ... })
- delete: DELETE /aio/prompts (body { ids })
Reconciled the spike's deferred call-site check against the actual consumer and
the spec:
- Remove the bogus POST from aio/prompts.js (keep DELETE → 204, the only op the
spec defines on that path).
- Add aio/prompts/tagged.js → 201 IDsWithStatsResponse { ids, existing_count }.
- Add aio/prompts/by_tags.js → 200 AIOPromptsWithStatusListResponse
{ items, page, total, unassigned }; empty tag_ids lists all, else OR-filter.
Created prompts carry a synthesized AIOTag so by_tags filtering is meaningful.
- Align the workspace-with-data prompt seed to AIOPromptWithStatus (name + tags,
was the stale `text` field).
- Repoint the E2E at tagged/by_tags with a real write-then-read assertion plus a
delete-reflects-in-list case.
- Update the README stateful-endpoints table.
Validated: npm test (51 passing, 100% coverage), npm run lint, npm run test:types,
npm run test:e2e (8 passing against the live mock with response validation on).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Switch SPEC from the raw Swagger 2.0 file to build/openapi3.json (the overlay-corrected OAS3 artifact produced by npm run generate). Two fixes follow from this: 1. Counterfact now sees CR1 (GET /v1/ai_models) and auto-generates a stub for it, closing the gap where that path was missing from the mock. 2. Counterfact honours Swagger 2.0 basePath as a serving prefix but ignores OAS3 servers[0].url. Add --prefix /enterprise/projects/api explicitly so all routes (stateful handlers + auto-generated stubs) are served under the correct base path and the real client resolves them correctly. Also add a fast-fail guard in materialize() that exits with a clear message when build/openapi3.json is absent, so developers get actionable feedback instead of a cryptic Counterfact error. README: update the Pipeline diagram and description to reflect the mock now reads the OAS3 artifact, document npm run generate as a prerequisite, and update the mock table entry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
aliciadriani
force-pushed
the
llmo-5460-project-engine-mock
branch
from
831bdf3 to
0ccd959
Compare
Seed entries use fixed ids. A seed-id collision via create() was silently swallowed via Map.set overwrite. Now throws on explicit id collision so the bug surfaces during dev instead of being masked. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Rebase + review round resolved. Rebase onto current `main` — the branch is now stacked cleanly on top of the squash-merged #1661 (`0465ab0`). Conflicts were mechanical (all from the foundation files now in `main`); resolved by keeping the overlay-inclusive `generate` script from `main` and the mock-runner additions from this branch.
Type-surface follow-up (same commit `0ccd959`): `test/types/index.type-test.ts` dropped the `Auth-Data-Jwt` header from the typed call — the overlay (CR2) already removes it from every operation in the generated surface, so passing it was a type error against the current `main` types. Inline nits (all in `cbe35f3` or `1f5c8cf`):
Process fix: PR description updated below to reflect the full 24-file / +1579 diff. Gates: `npm test` 98 passing 100% coverage · `npm run lint` clean · `npm run test:types` clean. |
…endencies Regenerated after adding sinon and typescript devDependencies to the project-engine-client package. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… cover duplicate-id guard - Add 'Generate corrected OAS3 artifact' step to the E2E job so the mock runner finds build/openapi3.json (gitignored) before it starts. - Add missing test for InMemoryStore.create duplicate-id guard to restore 100% coverage (lines 77-78 were uncovered, failing the coverage threshold). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
main merged js-yaml@4.2.0 (#1704); project-engine-client's workspace entry was missing js-yaml@4.2.0 + argparse@2.0.1 from the lock file. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
What
The stateful Project Engine Counterfact mock — LLMO-5460 (epic LLMO-5459). A generic in-memory store + the confirmed stateful resource ops, the live Counterfact runner that wires them into per-path handlers, named seeds, a test-only reset route, and an in-package E2E that drives the real client against a booted mock.
Stacked on #1661 → #1660. Base retargeted to `main` after #1661 merged.
Spike outcome (confirmed against both consumers)
Grepped the real consumers to set the AC floor (`docs/mock-statefulness.md`):
Contents
Validation
Follow-up (separate PR)
Adopting the mock into `spacecat-api-service`'s own E2E/local-dev (replacing its Project Engine calls against this mock) lands with the adoption PR — out of scope here, which delivers the mock and its self-contained E2E.
🤖 Generated with Claude Code