Skip to content

Commit d9342f1

Browse files
committed
fix(runtime,spec,client): the install door persists the state it returns, and the contract says what it does not cover
Patch round for the at-tier contract review's four FAIL items on #18058. F1 — `POST /api/v1/packages` wrote `setPackageDisabled(..., true)` and never `false`, so a re-install that answered `enabled: true` left the durable state file still listing the id as disabled. `SchemaRegistry.installPackage` reads that file at boot, so the package came back DISABLED one restart later with nothing red in between. The durable write now happens on both arms, and four cases pin the re-install path the previous test could not reach. F2 — the two runtime door drives cited as the bare form's evidence post bodies this declaration REFUSES (no `type`; the second has no `version` either), and the fixture claiming to transcribe one had `type: 'app'` added. They are now pinned as the residual, refused, with a control that completing them turns them green. The docblock and the changeset state all five residual classes. F3 — `PackageInstallRequestSchema` is `z.object`, so the wrapped branch is STRIP mode, not closed. The sentence is corrected rather than the schema: a strict wrapped branch would refuse bodies the door answers 201 to. F4 — `packages/client/src/readme-install-example.test.ts` reads the README, extracts the literal a reader copies, wraps it as the SDK does and parses it, so the acceptance fixture can no longer drift back to the refused text silently. Claude-Session: https://claude.ai/code/session_01LvwGppdonww4zGLWZo5rho Co-authored-by: Claude <noreply@anthropic.com>
1 parent f30e8fb commit d9342f1

6 files changed

Lines changed: 415 additions & 28 deletions

File tree

‎.changeset/18058-install-door-contract-rebind.md‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,18 @@ Clause-②: yes (widening)
1515
- `PackageApiContracts.installPackage.path` — `'/api/v1/packages/install'` → `'/api/v1/packages'`. A caller that read the constant to build a URL was building one nothing serves; a caller that hard-coded the old string gets a `404` today and should send `POST /api/v1/packages`. The method (`POST`) is unchanged and is what distinguishes this entry from `listPackages`.
1616
- `PackageApiContracts.installPackage.input` — `PackageInstallRequestSchema` → the new `PackageInstallBodySchema`. The wrapped schema is still exported and still parses the wrapped form; the new export is a union that also parses a bare manifest.
1717
- `PackageInstallRequestSchema` gains **`overwrite?: boolean`**. This is a declaration of behaviour that already shipped: the door reads `overwrite` from the body (or `?overwrite=true`) to opt back in to replacing an already-installed id instead of answering `409 Conflict`, the first-party SDK sends it, and no schema declared it — so any parse at that door would have silently stripped it and turned a deliberate re-install into a conflict.
18-
- **`PackageInstallBodySchema`** / `PackageInstallBody` / `PackageInstallBodyParsed` are new. The door reads `body.manifest || body`, and first-party callers really do post a bare manifest as the whole body, so the contract declares both forms as a union of two whole closed declarations — never a tolerant shape. The bare form carries no install options: `settings`, `enableOnInstall` and `overwrite` are not manifest keys and the manifest surface is closed, so a bare-form caller reaches `overwrite` through the query string alone.
18+
- **`PackageInstallBodySchema`** / `PackageInstallBody` / `PackageInstallBodyParsed` are new. The door reads `body.manifest || body`, and first-party callers really do post a bare manifest as the whole body, so the contract declares both forms as a union — every parse is a full parse of one coherent form, never a tolerant shape. The two branches are disjoint, but only the BARE one is CLOSED: `PackageInstallRequestSchema` is a plain `z.object`, so an unknown key on the wrapped form is DROPPED (`{ manifest, bogus: 1 }` parses and `bogus` is gone) while the same key on a bare manifest is refused by name. That asymmetry matches the door, which reads four keys off the wrapper and ignores the rest — closing the wrapped branch would refuse bodies the door answers `201` to. The bare form carries no install options: `settings`, `enableOnInstall` and `overwrite` are not manifest keys and the manifest surface is closed, so a bare-form caller reaches `overwrite` through the query string alone.
1919

2020
**What moved at the runtime**
2121

2222
`POST /api/v1/packages` now honours `enableOnInstall: false` in the wrapped body: the package installs `disabled`, through the same registry flip and durable state write `PATCH /packages/:id/disable` uses, so a restart does not re-enable what the caller switched off. `true` and absent install enabled, which is the declared default. Previously the key was declared in three schemas, sent by the SDK, and read by no handler at all.
2323

24+
The durable write happens on **both** arms, not just the disable. `POST /packages` is a create that an already-installed id reaches through `overwrite`, and `DELETE /packages/:id` does not clear this record either, so an install could answer `201` with `enabled: true` while the state file still listed the id as disabled — and `SchemaRegistry.installPackage` reads that file at boot, re-installing the package DISABLED one restart later with nothing red in between. Every install now persists the state it returned.
25+
26+
**What the declaration does NOT cover — the measured residual**
27+
28+
This is a subset description of the live door, deliberately, and it is recorded rather than implied. Measured through `HttpDispatcher.handlePackages`, the door also answers `201` to: a manifest missing `type` and/or `version` (both of the runtime's own door drives post one); unknown keys on either form (refused by name on the bare branch, dropped on the wrapped one, `201` either way); a string-typed `enableOnInstall` / `overwrite`, which is compared against `true`/`false`/`'true'` and therefore treated as absent — `enableOnInstall: 'false'` installs ENABLED; and install options spelled on the bare form, which are ignored. In the opposite direction the door answers `400` to a whitespace-only `id` this declaration admits. `ManifestSchema` is not relaxed to close any of that.
29+
2430
**Documentation**
2531

2632
`packages/client`'s README install example could not parse against the manifest contract — no `id`, no `type`, and a `label` key the closed manifest surface refuses by name — and the live door answered it `400 Package id is required`. It is now a manifest that parses, and the example names the `overwrite` opt-in beside it.
Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#18058] The published README's install example, parsed against the contract
5+
* the door it is copied into actually enforces.
6+
*
7+
* ## Why this file exists — the fixture that could not fail
8+
*
9+
* #18058 fixed `packages/client/README.md`: its `client.packages.install(…)`
10+
* example was a body the live door answers `400 Package id is required` to —
11+
* no `id`, no `type`, and a `label` key the closed manifest surface refuses by
12+
* name. The fix was pinned in `packages/spec/src/api/package-api.test.ts`, but
13+
* pinned as a HAND-TRANSCRIBED literal: reverting the README to its old text
14+
* left that suite 56/56 green, because nothing in the repository read the
15+
* README at all. An acceptance fixture that can drift back to the refused text
16+
* with nothing turning red is not an acceptance fixture — it is a copy of one.
17+
*
18+
* So this test reads the README itself, extracts the object literal a reader
19+
* would copy, wraps it exactly as `index.ts`'s `packages.install` wraps it on
20+
* the wire, and parses that. Revert the README and this file goes red; edit
21+
* the example into something the door refuses and it goes red; and the
22+
* pre-#18058 literal is pinned as REFUSED beside it so a schema relaxation
23+
* cannot make the whole thing vacuous.
24+
*
25+
* ⛔ The extraction is deliberately loud: a README that no longer contains an
26+
* `install(` call with an object literal FAILS here rather than silently
27+
* testing nothing. "Could not run" is a failure, not a skip.
28+
*/
29+
30+
import { describe, it, expect } from 'vitest';
31+
import { readFileSync } from 'node:fs';
32+
import { fileURLToPath } from 'node:url';
33+
import { PackageInstallBodySchema } from '@objectstack/spec/api';
34+
35+
/** This package's own published README — `packages/client/README.md`. */
36+
const README = readFileSync(fileURLToPath(new URL('../README.md', import.meta.url)), 'utf8');
37+
38+
/**
39+
* The `// Package Management` block, bounded by the next top-level comment
40+
* group in the same fence. Scoping matters: `install(` also appears in the
41+
* overwrite line right below the example, and that one passes an identifier.
42+
*/
43+
function packageManagementBlock(source: string): string {
44+
const start = source.indexOf('// Package Management');
45+
if (start < 0) throw new Error('packages/client/README.md no longer has a `// Package Management` block');
46+
const rest = source.slice(start + 1);
47+
const end = rest.indexOf('\n\n');
48+
return end < 0 ? rest : rest.slice(0, end);
49+
}
50+
51+
/**
52+
* The first object literal passed to `client.packages.install(` — brace-matched
53+
* rather than regex-matched, so a nested object in the example cannot truncate
54+
* it. Throws when the call or the literal is gone.
55+
*/
56+
function firstInstallObjectLiteral(block: string): string {
57+
const call = block.indexOf('client.packages.install({');
58+
if (call < 0) {
59+
throw new Error('the README\'s Package Management block no longer calls `client.packages.install({ … })` with an object literal');
60+
}
61+
const open = block.indexOf('{', call);
62+
let depth = 0;
63+
for (let i = open; i < block.length; i += 1) {
64+
if (block[i] === '{') depth += 1;
65+
else if (block[i] === '}') {
66+
depth -= 1;
67+
if (depth === 0) return block.slice(open, i + 1);
68+
}
69+
}
70+
throw new Error('the README\'s install example has an unbalanced object literal');
71+
}
72+
73+
const LITERAL = firstInstallObjectLiteral(packageManagementBlock(README));
74+
75+
/** The literal as a value. It is ordinary JS — single quotes, bare keys. */
76+
const README_MANIFEST = new Function(`return (${LITERAL});`)() as Record<string, unknown>;
77+
78+
/**
79+
* `index.ts`'s `packages.install` body, spelled out: `manifest`, `settings`,
80+
* `enableOnInstall`, and `overwrite` only when the caller passed one. This is
81+
* the body that leaves the process, which is what the door parses.
82+
*/
83+
const asTheSdkSends = (
84+
manifest: unknown,
85+
options?: { settings?: Record<string, unknown>; enableOnInstall?: boolean; overwrite?: boolean },
86+
) => ({
87+
manifest,
88+
settings: options?.settings,
89+
enableOnInstall: options?.enableOnInstall,
90+
...(options?.overwrite !== undefined ? { overwrite: options.overwrite } : {}),
91+
});
92+
93+
describe('#18058 — the README install example is parsed, not transcribed', () => {
94+
it('the extraction really found a manifest — this file is not testing an empty object', () => {
95+
// The non-vacuity guard for the extractor itself: the three keys the
96+
// pre-#18058 text was missing are exactly the ones a reader needs.
97+
expect(Object.keys(README_MANIFEST).length).toBeGreaterThanOrEqual(3);
98+
expect(README_MANIFEST).toHaveProperty('id');
99+
expect(LITERAL.startsWith('{')).toBe(true);
100+
});
101+
102+
it('parses green, wrapped exactly as `packages.install` wraps it on the wire', () => {
103+
const verdict = PackageInstallBodySchema.safeParse(asTheSdkSends(README_MANIFEST));
104+
expect(verdict.error?.issues ?? []).toEqual([]);
105+
expect(verdict.success).toBe(true);
106+
});
107+
108+
it('parses green BARE too — `body.manifest || body` is how the door reads it', () => {
109+
expect(PackageInstallBodySchema.safeParse(README_MANIFEST).success).toBe(true);
110+
});
111+
112+
it('the overwrite opt-in beside it parses too', () => {
113+
expect(PackageInstallBodySchema.safeParse(asTheSdkSends(README_MANIFEST, { overwrite: true })).success).toBe(true);
114+
});
115+
116+
it('⛔ the pre-#18058 text is still REFUSED — a schema relaxation cannot make this vacuous', () => {
117+
// No `id`, no `type`, and `label` is not a manifest key. If this ever turns
118+
// green the manifest contract was relaxed, not the example fixed.
119+
const asShipped = { name: 'vendor_plugin', label: 'Vendor Plugin', version: '1.0.0' };
120+
expect(PackageInstallBodySchema.safeParse(asTheSdkSends(asShipped)).success).toBe(false);
121+
expect(PackageInstallBodySchema.safeParse(asShipped).success).toBe(false);
122+
});
123+
});

‎packages/runtime/src/domains/packages-install-enable-on-install.test.ts‎

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,3 +167,118 @@ describe('#18058 — the install door honours `enableOnInstall`', () => {
167167
expect(persistedDisabled().has('com.acme.bare')).toBe(false);
168168
});
169169
});
170+
171+
/**
172+
* [#18058 F1] The DURABLE half of the declared default, on the path the cases
173+
* above structurally cannot reach: an id that is ALREADY INSTALLED.
174+
*
175+
* ## The defect these pin shut
176+
*
177+
* The handler wrote `setPackageDisabled(env, id, true)` and never `false`, so
178+
* the three records this door is held to only agreed while every id was fresh:
179+
*
180+
* ```text
181+
* POST {manifest, overwrite:true, enableOnInstall:false} → 201, disabled, state file lists the id
182+
* POST {manifest, overwrite:true} (flag absent) → 201, enabled:true, registry true,
183+
* and the state file STILL lists it
184+
* ```
185+
*
186+
* Nothing is red at that moment. The loss surfaces one restart later:
187+
* `SchemaRegistry.installPackage` reads `initialDisabledPackageIds`, finds the
188+
* id, and re-installs the package DISABLED — a door that answered correctly on
189+
* the wire and wrongly on disk. `PATCH /packages/:id/enable` has always made
190+
* exactly the `false` call these cases demand; the install door did not.
191+
*
192+
* ⚠️ The fix is deliberately UNCONDITIONAL rather than scoped to the overwrite
193+
* path: `DELETE /packages/:id` does not clear the durable disable either, so a
194+
* disable → uninstall → re-install lands on a FRESH registry id whose durable
195+
* record still says disabled. Persisting the state the door actually returned,
196+
* every time, is one call that cannot be out of step with the row.
197+
*/
198+
describe('#18058 — a re-install persists the state it RETURNS, not just a disable', () => {
199+
let registry: SchemaRegistry;
200+
let dispatcher: HttpDispatcher;
201+
202+
beforeEach(() => {
203+
registry = freshRegistry();
204+
dispatcher = makeDoor(registry);
205+
});
206+
207+
/** Install once with `enableOnInstall: false`, and prove the disable really landed. */
208+
const installDisabled = async (id: string, namespace: string) => {
209+
const first = await install(dispatcher, {
210+
manifest: manifest(id, namespace),
211+
overwrite: true,
212+
enableOnInstall: false,
213+
});
214+
expect(first.response?.status).toBe(201);
215+
expect(first.response?.body?.data?.enabled, 'precondition: the first install really disabled it').toBe(false);
216+
expect(persistedDisabled().has(id), 'precondition: the disable really reached disk').toBe(true);
217+
};
218+
219+
it('re-installing with the flag ABSENT clears the durable disable — the declared default is `true`', async () => {
220+
const id = 'com.acme.reinstall.absent';
221+
await installDisabled(id, 'reinstallabsent');
222+
223+
const again = await install(dispatcher, { manifest: manifest(id, 'reinstallabsent'), overwrite: true });
224+
225+
expect(again.response?.status).toBe(201);
226+
expect(again.response?.body?.data?.enabled, 'the row this door RETURNED').toBe(true);
227+
expect(registry.getPackage(id)?.enabled, 'the registry the next read serves from').toBe(true);
228+
// ⭐ The line this F item exists for: without it the next boot reads the
229+
// stale id out of `initialDisabledPackageIds` and installs it DISABLED.
230+
expect(
231+
persistedDisabled().has(id),
232+
'the durable state — a re-install that answered `enabled` must not leave `disabled` on disk',
233+
).toBe(false);
234+
});
235+
236+
it('re-installing with `enableOnInstall: true` clears the durable disable', async () => {
237+
const id = 'com.acme.reinstall.true';
238+
await installDisabled(id, 'reinstalltrue');
239+
240+
const again = await install(dispatcher, {
241+
manifest: manifest(id, 'reinstalltrue'),
242+
overwrite: true,
243+
enableOnInstall: true,
244+
});
245+
246+
expect(again.response?.status).toBe(201);
247+
expect(again.response?.body?.data?.enabled).toBe(true);
248+
expect(registry.getPackage(id)?.enabled).toBe(true);
249+
expect(persistedDisabled().has(id)).toBe(false);
250+
});
251+
252+
it('a BARE re-install clears it too — that form cannot ask for `false`, so it installs enabled', async () => {
253+
const id = 'com.acme.reinstall.bare';
254+
await installDisabled(id, 'reinstallbare');
255+
256+
// No wrapper, so `enableOnInstall` is not a declared key here at all and
257+
// the door installs at the default. The durable record must follow.
258+
const again = await install(dispatcher, manifest(id, 'reinstallbare'));
259+
260+
expect(again.response?.status).toBe(409, 'the bare form reaches overwrite through the query string alone');
261+
262+
const forced = await dispatcher.handlePackages(
263+
'', 'POST', manifest(id, 'reinstallbare'), { overwrite: 'true' }, PKG_ADMIN(),
264+
);
265+
expect(forced.response?.status).toBe(201);
266+
expect(forced.response?.body?.data?.enabled).toBe(true);
267+
expect(persistedDisabled().has(id)).toBe(false);
268+
});
269+
270+
it('⛔ the disable direction is UNCHANGED — a re-install asking for `false` still persists it', async () => {
271+
const id = 'com.acme.reinstall.stays-off';
272+
await installDisabled(id, 'reinstallstaysoff');
273+
274+
const again = await install(dispatcher, {
275+
manifest: manifest(id, 'reinstallstaysoff'),
276+
overwrite: true,
277+
enableOnInstall: false,
278+
});
279+
280+
expect(again.response?.status).toBe(201);
281+
expect(again.response?.body?.data?.enabled).toBe(false);
282+
expect(persistedDisabled().has(id)).toBe(true);
283+
});
284+
});

0 commit comments

Comments
 (0)