Skip to content

Commit 13d5294

Browse files
fix(runtime): POST /api/v1/packages parses the manifest version leg instead of installing anything it is handed (#19326)
Fixes #19120 Clause-②: no (narrowing) `POST /api/v1/packages` installed a manifest with no `version` and answered `201`, while the published declaration for that door has always required one. The door now parses that leg and refuses loudly. ## What changed `PackageInstallRequestSchema` binds `manifest: ManifestSchema`, and `ManifestSchema` declares `version` required with a semantic grammar. The door parsed nothing at all — `const manifest = body.manifest || body` went straight to `installPackage`, with an id check as the only gate on the way. The install branch now asks the declaration **by reference** and refuses with `400` / `VALIDATION_ERROR` plus a prescription naming the key and its shape. ``` ManifestSchema.shape.version.safeParse(manifest?.version) ``` Three properties of that spelling are load-bearing: - **By reference, not by copy.** The `PATCH /packages/:id` door further down the same file keeps its own hand-written semver regex. A second copy here would be the third judgment of one key on one surface. Because this door asks the declaration, whatever the open version-grammar canon question settles on reaches it with no edit to this file. - **No new error code.** `deps.error(msg, 400)` derives `VALIDATION_ERROR` from `standardErrorCodeForHttpStatus(400)`, the standard catalog's member for 400 — the same dialect the repeated-query-parameter refusal on this route already answers in. Nothing in `packages/spec` moves, and nothing in it was touched. - **Ordered ahead of the duplicate-id 409.** A request-shape refusal must not depend on server state. Placed after the 409, one and the same under-specified body would answer `400` or `409` according to whether that id happened to be installed already — two answers to one authoring mistake. The id gate above it still wins, because with no id there is nothing to name in the sentence this gate prints. ## The scope fence held — the five classes are SEPARABLE The card is graded on the `version` leg alone. The declaration's residual docblock records five classes this door answers `201` to; the dispatch asked whether enforcing `version` forces the other four along. **It does not.** The structural reading: the five are produced at three different levels — a per-key field schema in `ManifestSchema.shape` (this leg), the union arms' strict close (unknown keys, bare-form options), and this handler's own `=== true` / `=== 'true'` comparisons (the string-typed options). The single call that would close four at once is `PackageInstallBodySchema.safeParse(body)`, and that call is pointedly not made. `ManifestSchema.shape.version` is an independent field schema that parses one value with no reference to any other key or to the envelope. Measured at this head, after the fix, driving the real door (one-shot probe, not a committed test): | class | case | status | installed | |---|---|---|---| | 1a **version** | wrapped, no `version` | **400** VALIDATION_ERROR | no | | 1a **version** | bare, no `version` | **400** VALIDATION_ERROR | no | | 1a **version** | off-spec grammar | **400** VALIDATION_ERROR | no | | 1b type | wrapped, no `type` | 201 | yes | | 1b type | bare, no `type` | 201 | yes | | 2 unknown keys | unknown key, wrapped form | 201 | yes | | 2 unknown keys | unknown key, inside the manifest | 201 | yes | | 2 unknown keys | unknown key, bare form | 201 | yes | | 3 string-typed | `enableOnInstall: 'false'` | 201 (enabled) | yes | | 3 string-typed | `overwrite: 'true'` on the body | 201 | yes | | 4 bare-form options | `settings` on the bare form | 201 | yes | | 5 whitespace id | id of spaces only | 400 (pre-existing) | no | Only the `version` rows moved. The other four classes are untouched in both directions, and **no committed test asserts anything about them** — pinning them as `201` would freeze four known residuals as intended behaviour and turn the card that closes one of them red for doing its job. Note for whoever takes the residual next: the docblock's clause 1 reads "a manifest missing `type` and/or `version`", and after this PR only the `type` half of it is still true. That text lives in `packages/spec`, which is the `domain:spec` seat's — flagged in the report, not edited here. ## Tests `packages/runtime/src/domains/packages-install-manifest-version.test.ts`, 20 cases: - **§0** asserts each fixture off-spec **by the declaration** before §1 holds the door to it. If the version-grammar canon moves, §0 goes red first and names what happened, instead of §1 failing for a reason a reader has to reconstruct. - **§1** pins both halves a refusal owes: `400` + `VALIDATION_ERROR` + `success: false` on the wire, **and** that neither install writer was called. A door that answered `400` and installed anyway satisfies a status assertion alone. - **§2** is the positive control, on both body forms and both install limbs, asserting the manifest reaches `installPackage` unchanged — this is a gate, not a normaliser. - **§3** pins the ordering, with the 409 itself as the control that the 409 was not simply removed. **One fixture repaired outside the new file.** `packages/runtime/src/domain-handler-registry.test.ts`'s duplicate-id case drove the door with `{ id: 'pkg-a', name: 'A' }` — a body `ManifestSchema` has never admitted. Its subject is the 409 guard and the `?overwrite=true` bypass, not manifest completeness, so it gets the `version` it never carried. The repair is owed whatever order the new gate sits in: that case's `?overwrite=true` limb asserts `201`, which an under-specified manifest must never reach, so no placement of the gate leaves the fixture valid. ## Verification Final head `0ccf7296d4`, after `git merge origin/main`. - `pnpm --filter @objectstack/runtime test` — **269 files / 3725 passed**, 1 skipped, exit 0 - `pnpm --filter @objectstack/runtime typecheck` — exit 0 - `pnpm --filter '@objectstack/runtime...' build` — exit 0 - **58 of 58** derived gate families run, **0 NOT-MEASURED, 0 UNRUN** (`dispatch-gates.mjs --ran`, exit codes captured before any pipe). `check:dual-build-cjs-loads` answered `PREREQUISITE NOT MET` on the pre-merge run and measures green here once its build closure existed. - `pnpm lint` — the **full unnarrowed repo union** (`eslint . --no-inline-config`), exit 0, at this head. Not a narrowed run, so no narrowing is claimed. - `check-adr-0087-registration` reads the changeset as `[BREAKING+clause-②-narrowing]` carrying `not-required (no-migration-prescription)`. ## Changeset `minor`, declared breaking. Not `major`: the launch-window guard ships breaking changes as minor while the stack versions in lockstep. The ADR-0087 disposition is `not-required (no-migration-prescription)` — nothing authorable is removed, renamed or reshaped, so `objectstack migrate meta` has nothing to reach; there is no old spelling that maps to a new one, only a key the declaration already required. ## Acceptance notes - **The residual docblock is now half-stale** and needs the `domain:spec` seat: clause 1 of `PackageInstallBodySchema`'s docblock records `type` and/or `version` as one `201` class, and only `type` remains. Reported, not edited — `packages/spec` is not this card's to touch. - **The four other residual classes** stay open and each needs its own reading, per triage. Measured unchanged above. - **The `PATCH /packages/:id` door keeps a hand-copied semver regex.** Correct today and not a defect, so not filed — but it is the copy that has to be found by hand if the version-grammar canon moves, while this door follows the declaration on its own. Noted for whoever takes that canon card. --- _Generated by [Claude Code](https://claude.ai/code/session_01QCdUBjM47SxioST9z5Zwdf)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent df0c856 commit 13d5294

4 files changed

Lines changed: 439 additions & 2 deletions

File tree

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
---
2+
'@objectstack/runtime': minor
3+
---
4+
5+
fix(runtime): `POST /api/v1/packages` parses the manifest's `version` leg instead of installing anything it is handed (#19120)
6+
7+
Clause-②: no (narrowing)
8+
9+
**BREAKING for callers of the install door** — a manifest with no `version`, or
10+
one whose `version` does not match the declared semantic grammar, is now refused
11+
`400` / `VALIDATION_ERROR`. It used to install and answer `201`.
12+
13+
The accept set only shrinks back to what the published declaration has always
14+
said. `PackageInstallRequestSchema` binds `manifest: ManifestSchema`, and
15+
`ManifestSchema` declares `version` required with a semantic grammar. The door
16+
parsed nothing at all: `const manifest = body.manifest || body` went straight to
17+
`installPackage`, with an id check as the only gate on the way. That is
18+
«declared ≠ enforced» on a published API contract — and because the install
19+
landed silently, an author could install metadata the platform's own CLI build
20+
step (`os plugin build`) would have refused outright.
21+
22+
The gate asks the declaration **by reference** — `ManifestSchema.shape.version`
23+
— rather than keeping a copy of the grammar. The version-grammar canon is an
24+
open question on its own card; whichever way it is settled, this door follows it
25+
with no further edit.
26+
27+
**What is not affected.** Boot-time and in-process installs reach
28+
`SchemaRegistry.installPackage` / `ObjectQL.registerApp` directly and never pass
29+
through this branch, so nothing about how a package is loaded from disk or
30+
registered by a plugin changes. A well-formed manifest installs exactly as
31+
before, on both body forms (wrapped and bare) and on both install limbs (the
32+
protocol primitive and the bare-registry fallback).
33+
34+
**Scope — the `version` leg alone.** The declaration's own docblock records five
35+
classes this door answers `201` to while the schema refuses them. This change
36+
closes one: `version`. A missing `type`, unknown keys on either body form, a
37+
string-typed `enableOnInstall` / `overwrite`, and install options spelled on the
38+
bare form are each left exactly as they were — measured after the change, all
39+
four still answer `201`. Each is its own reading and its own card.
40+
41+
**If you are refused.** Give the manifest the `version` the schema has always
42+
required — `version: "1.0.0"`, three dot-separated numbers. The refusal names
43+
the key and shows the shape, so the prescription arrives with the `400` rather
44+
than in a changelog.
45+
46+
<!-- adr-0087: not-required (no-migration-prescription) Nothing authorable is removed, renamed or reshaped: no spec key, no export, no stored row. `objectstack migrate meta` has nothing to reach, because there is no old spelling that maps to a new one — a caller supplies a key the declaration already required. The refusal itself carries the remedy. -->

‎packages/runtime/src/domain-handler-registry.test.ts‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -579,9 +579,19 @@ describe('HttpDispatcher extracted domains (PR-5: packages)', () => {
579579
it('POST /packages rejects a duplicate id with 409 unless ?overwrite=true (data-loss footgun guard)', async () => {
580580
const objectql = qlWithRegistry({ getPackage: vi.fn().mockReturnValue({ id: 'pkg-a' }) });
581581
const dispatcher = withPkgCaller(makeDispatcher({ objectql }));
582-
const dup = await dispatcher.dispatch('POST', '/packages', { id: 'pkg-a', name: 'A' }, {}, {} as any);
582+
// [#19120] `version` added to a fixture that never carried one. The
583+
// subject here is the duplicate-id guard and its `?overwrite=true`
584+
// bypass, ⛔ not manifest completeness — but `ManifestSchema` has always
585+
// declared `version` required, so this body was never a legal input to
586+
// the door it drives. Since the install door started parsing that leg,
587+
// the fixture's own defect is what the case would report.
588+
// ⚠️ The repair is owed whatever order the new gate sits in: the
589+
// `forced` limb asserts `201`, which an under-specified manifest must
590+
// never reach — so no placement of that gate leaves this fixture valid.
591+
const manifest = { id: 'pkg-a', name: 'A', version: '1.0.0' };
592+
const dup = await dispatcher.dispatch('POST', '/packages', manifest, {}, {} as any);
583593
expect(dup.response?.status).toBe(409);
584-
const forced = await dispatcher.dispatch('POST', '/packages', { id: 'pkg-a', name: 'A' }, { overwrite: 'true' }, {} as any);
594+
const forced = await dispatcher.dispatch('POST', '/packages', manifest, { overwrite: 'true' }, {} as any);
585595
expect(forced.response?.status).toBe(201);
586596
});
587597

0 commit comments

Comments
 (0)