diff --git a/.changeset/d1-create-side-hatch-clause.md b/.changeset/d1-create-side-hatch-clause.md new file mode 100644 index 0000000000..b669811d94 --- /dev/null +++ b/.changeset/d1-create-side-hatch-clause.md @@ -0,0 +1,63 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): the ADR-0070 D1 refusal tells an operator with the hatch open that it does not reach package writability (#8361) + +`OS_METADATA_WRITABLE` unlocks a metadata **type**; it has never unlocked a +package's **writability**. #8146 wrote that sentence into both package-door +emitters in `SysMetadataRepository`, so a refusal emitted while the variable is +set says so instead of leaving the operator to guess. On the override side that +clause is reachable, and #8184 made it reachable on scoped kernels too. + +On the **create** side it was reachable from nowhere an author actually writes +from. `saveMetaItem`'s ADR-0070 D1 gate refuses on a strictly wider predicate +than the repository's package door — no "did the caller name a base" limb, no +registry limbs above it — so it threw first on every kernel, with its own +sentence, which had no hatch clause in it. Measured before the fix, with +`OS_METADATA_WRITABLE=permission` set and a runtime-only create aimed at a +read-only package: + +```text +[writable_package_required] Cannot save permission/runtime_reviewer: the package +'com.example.showcase' is read-only (provided by code or an installed app). +Switch to a writable package in the package selector, or create a new one, and retry. +``` + +Byte-identical with the hatch open and with it shut. The operator is told the +base is read-only — true — and never told that the variable they set a moment +ago cannot make it writable. Milder than the false prescription #8146 closed on +the override side (D1 never told anyone to set the variable), so nobody retried +forever; the missing half is guidance, which is why this ships as a diagnostic +fix. + +**What changed.** D1 now calls the repository's existing emitter, +`SysMetadataRepository.readOnlyBaseCreateError`, instead of spelling a second +sentence for the same condition — the create-side mirror of what #8184 did on +the override side, and the same one-emitter direction: two independently +authored refusals behind one condition is how a vocabulary drifts. The same +request now answers: + +```text +[writable_package_required] Cannot create permission/runtime_reviewer in package +'com.example.showcase': that package is read-only (provided by code or an installed +app), so it is not a writable base. Switch to a writable package in the package +selector, or create a new one, and retry. (OS_METADATA_WRITABLE is set for +'permission': it unlocks the metadata TYPE, not package writability, so it does not +make a read-only package a writable base.) +``` + +With the hatch **shut** the clause is absent and the sentence keeps the remedy +that is true there — the clause is selected, never appended. + +**No acceptance decision moves.** D1's predicate is untouched: every create it +refused it still refuses, with the same `WRITABLE_PACKAGE_REQUIRED` code, the +same 422, the same `packageId`, and the same ADR-0070 `docs` pointer; every +create it admitted — into a writable base, or naming no base at all — still +lands. Only the sentence the refusal carries changed. + +`readOnlyBaseCreateError` gained an optional trailing `name` so the delegated +sentence can keep naming the item the way D1's always did. Omitted, its output +is byte-identical to what shipped in #8146 — which is what the direct +`repository.put` callers (`promoteDraft`, `restoreVersion`, `revertCommit`) see, +and until this change they were the *only* callers reaching that clause at all. diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index e625005a99..460fe2b10a 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -11556,23 +11556,55 @@ export class ObjectStackProtocolImplementation implements // A `null` packageId is still accepted here (legacy org-overlay // destination); ADR-0070 D5 retires it once the surfaces always // resolve a base and the orphan migration has run. + // + // [#8361] THE SENTENCE IS THE REPOSITORY'S, the predicate is still + // D1's. `SysMetadataRepository.readOnlyBaseCreateError` is CALLED, not + // copied — the create-side mirror of what #8184 did four hundred lines + // above on the override side. + // + // ⛔ NOTHING ABOUT THE ACCEPTANCE SET MOVES. The `if` is byte-for-byte + // the predicate D1 has always used; only the error object it throws + // changes. Every create D1 refused it still refuses, every create it + // admitted it still admits — this card is about what the operator is + // TOLD, not about what is refused. + // + // WHY DELEGATE RATHER THAN TEACH D1 ITS OWN `hatchOpen` CLAUSE — the + // fork #8361 was filed on, and both of the reasons the one-emitter + // default might NOT have applied were measured here and did not hold: + // • "D1 fires before the repository exists in the call." True of the + // INSTANCE (`getOverlayRepo` is below), irrelevant to the emitter: + // it is `static`, the class is imported at the top of this file, + // and the override-side call at the top of this same method has + // been calling a static sibling since #8184. + // • "D1 carries its own `docs` pointer." Measured identical — both + // sites set `docs/adr/0070-package-first-authoring.md`, because D1 + // and this emitter implement the SAME decision. (That is what + // separates the create side from the override side, where the + // repository points at ADR-0010 and the fork was real.) + // The one true difference was that D1's sentence names the ITEM and + // the emitter's named only the type; the emitter took an optional + // `name` rather than the sentence being forked. + // + // `hatchOpen` is COMPUTED here, and must be — unlike the override site, + // which passes a literal `false` because reaching it proves the hatch + // is shut. Both directions are live at D1: a type with + // `allowRuntimeCreate` (e.g. `permission`) arrives with the hatch shut, + // and a type with neither channel (e.g. `job`) arrives ONLY because + // `OS_METADATA_WRITABLE` opened it — `isOverlayAllowed` folds the hatch + // in, so an open hatch carries the write past the code-only refusal + // straight to this gate. Read through this class's OWN memoised reader, + // the one that admission decision used, so `hatchOpen` cannot disagree + // with the limb that let the write through. if ( intent === 'runtime-only' && request.packageId != null && !this.isWritablePackage(request.packageId) ) { - // Surfaced verbatim as a console toast — keep the sentence - // user-actionable; the ADR pointer lives in `docs` below. - const err = new Error( - `[writable_package_required] Cannot save ${singularTypeForRepo}/${request.name}: ` - + `the package '${request.packageId}' is read-only (provided by code or an installed app). ` - + `Switch to a writable package in the package selector, or create a new one, and retry.`, + const envWritable = ObjectStackProtocolImplementation.envWritableTypes(); + const hatchOpen = envWritable.has(singularTypeForRepo) || envWritable.has(request.type); + throw SysMetadataRepository.readOnlyBaseCreateError( + singularTypeForRepo, request.packageId, hatchOpen, request.name, ); - (err as any).code = 'WRITABLE_PACKAGE_REQUIRED'; - (err as any).status = 422; - (err as any).packageId = request.packageId; - (err as any).docs = 'docs/adr/0070-package-first-authoring.md'; - throw err; } const orgId = request.organizationId ?? null; const repo = this.getOverlayRepo(orgId); diff --git a/packages/metadata-protocol/src/sys-metadata-repository.package-writability.test.ts b/packages/metadata-protocol/src/sys-metadata-repository.package-writability.test.ts index 1c423ec085..7230d13653 100644 --- a/packages/metadata-protocol/src/sys-metadata-repository.package-writability.test.ts +++ b/packages/metadata-protocol/src/sys-metadata-repository.package-writability.test.ts @@ -879,3 +879,324 @@ describe('#8184 — the scoped kernel answers the same code as the host-config k expect(metaRowsOf(engine)[0]).toMatchObject({ package_id: WRITABLE_PKG }); }, 30_000); }); + +/** + * [#8361] The CREATE side of the same shadowing — and the block that makes + * #8146's create-side hatch clause reachable from the surface it was written + * for. + * + * ## The defect, measured on `ffb090e6f7` before the fix + * + * #8146 gave BOTH package-door emitters a `hatchOpen` remedy selection. The + * override one is reachable (the block above, and #8184 made it reachable on + * scoped kernels too). The create one — `readOnlyBaseCreateError`, `422 + * WRITABLE_PACKAGE_REQUIRED` — was not reachable from `saveMetaItem` on ANY + * topology: the ADR-0070 D1 gate throws first, on a strictly WIDER predicate + * (no `namedBase` limb, no registry limbs above it), and D1 spelled its own + * sentence with no hatch clause in it. Probed on the merged ref, hatch OPEN, + * `permission/probe_reviewer` into `com.example.showcase`: + * + * [writable_package_required] Cannot save permission/probe_reviewer: the + * package 'com.example.showcase' is read-only (provided by code or an + * installed app). Switch to a writable package in the package selector, or + * create a new one, and retry. + * + * — identical with the hatch open and with it shut. An operator who has just + * set `OS_METADATA_WRITABLE` is told the base is read-only and never told that + * the variable they set does not reach the package dimension. Milder than the + * override side's false prescription (D1 never told them to set it), which is + * why this was a finding rather than a bug: the missing half is guidance. + * + * ## THE SECONDARY DELIVERABLE — where that clause WAS live + * + * Enumerated on the merged ref, not restated from the card. `assertAllowed` + * reaches `readOnlyBaseCreateError` only from `put`, with `intent: + * 'runtime-only'` and a non-empty `packageId` (`assertDeleteAllowed` passes no + * base, so the delete side can never reach it). The production `put` callers + * carrying those two facts are: + * + * • `saveMetaItem` (`protocol.ts` — SHADOWED by D1: every + * write that would reach the door was + * already thrown by the wider gate) + * • `SysMetadataRepository.promoteDraft` ← `publishMetaDraft` + * • `SysMetadataRepository.restoreVersion` ← `revertCommit` + * • `SysMetadataRepository.restoreVersion` ← `restoreMetaVersion` + * + * The last three pass the ROW's own binding, never a caller-named base, and + * none of them passes through D1 — so the clause was exercised only by + * republish/repair traffic and never by the authoring surface it was written + * for. `LayeredRepository` is not a fourth route: nothing in the repo composes + * a `SysMetadataRepository` into one outside a TSDoc example. That route is + * pinned below (`the direct put callers keep their sentence verbatim`). + * + * ## The shape, and why it is delegation rather than a second clause + * + * D1 now CALLS `SysMetadataRepository.readOnlyBaseCreateError` — the lane's + * one-emitter direction, the create-side mirror of #8184. The two reasons the + * card gave for why that default might not apply were measured and did not + * hold: the emitter is `static` (no repository instance needed, and the + * override site has been calling a static sibling from this same method since + * #8184), and both sites already carried the IDENTICAL `docs` pointer + * (`0070-package-first-authoring.md`) because both implement D1. The one real + * difference — D1's sentence names the item, the emitter's named only the type + * — was closed by widening the emitter with an optional trailing `name`, so + * the direct `put` callers' sentence stays byte-identical. + * + * ## ⛔ What this block is NOT + * + * Not a change to the acceptance set. D1's predicate is untouched; the cases + * below pin both directions of that (a create into a writable base and a + * package-less create still LAND; the read-only create is still REFUSED with + * the same code and status). A shape that starts admitting or refusing + * anything new has left the card. + * + * Read `the sentence is the repository's, not a copy of it` as the anti-fork + * pin: it compares `saveMetaItem`'s message to the emitter's own output rather + * than to a literal, so re-spelling the sentence at D1 goes red even if the + * re-spelling is word-perfect on the day it is written. + */ +describe('#8361 — the create-side hatch clause reaches saveMetaItem', () => { + /** Not artifact-backed under {@link boot}, so `saveMetaItem` intends `runtime-only`. */ + const runtimePermissionBody = { name: 'runtime_reviewer', label: 'Reviewer', objects: {} }; + /** + * `job` is `allowOrgOverride: false` AND `allowRuntimeCreate: false`, so the + * hatch is the ONLY thing that can carry this write as far as D1 — which is + * what makes it the honest test of "the clause is reachable", as opposed to + * `permission`, which reaches D1 either way. Spec-valid on purpose: the Zod + * gate runs BEFORE D1, and a body missing `schedule`/`handler` answers + * `INVALID_METADATA` / 422 — measured, while writing this block. + */ + const jobBody = { + name: 'nightly_sweep', + label: 'Nightly Sweep', + schedule: { type: 'interval', intervalMs: 60_000 }, + handler: 'nightlySweep', + }; + + /** @param environmentId `undefined` = host-config kernel; a string = per-env kernel. */ + function boot(environmentId?: string) { + const engine = makeFakeEngine() as unknown as Record; + (engine as { registry: Record }).registry = { + ...(engine.registry as Record), + registerItem: () => {}, + registerObject: () => {}, + listItems: () => [], + getItem: () => undefined, + // Nothing is artifact-backed here: every case in this block is a CREATE. + getArtifactItem: () => undefined, + }; + const protocol = new ObjectStackProtocolImplementation( + engine as never, + () => new Map(), + environmentId, + ) as unknown as { saveMetaItem(req: Record): Promise }; + return { engine, protocol }; + } + + const metaRowsOf = (engine: Record) => + Array.from((engine as unknown as { rows: Map }).rows.values()) + .filter((r) => r.__table === 'sys_metadata'); + + const save = ( + protocol: { saveMetaItem(req: Record): Promise }, + req: Record, + ) => protocol.saveMetaItem(req).then(() => null, (e: unknown) => e); + + const openHatch = (types: string) => { + process.env.OS_METADATA_WRITABLE = types; + resetEnvWritableMetadataTypes(); + ObjectStackProtocolImplementation.resetEnvWritableCache(); + }; + + /** The runtime-only create the whole block is about, varying only the base. */ + const createPermission = ( + protocol: { saveMetaItem(req: Record): Promise }, + packageId?: string, + ) => save(protocol, { + type: 'permission', name: 'runtime_reviewer', item: runtimePermissionBody, + ...(packageId !== undefined ? { packageId } : {}), + }); + + beforeEach(() => { + delete process.env.OS_METADATA_WRITABLE; + resetEnvWritableMetadataTypes(); + ObjectStackProtocolImplementation.resetEnvWritableCache(); + }); + afterEach(() => { + delete process.env.OS_METADATA_WRITABLE; + resetEnvWritableMetadataTypes(); + ObjectStackProtocolImplementation.resetEnvWritableCache(); + }); + + // ── the card: the clause is reachable, and only when it is TRUE ──────── + + it('with the hatch OPEN the refusal says the hatch does not reach the package dimension', async () => { + openHatch('permission'); + const { engine, protocol } = boot(); + const err = await createPermission(protocol, READ_ONLY_PKG) as { + code?: string; status?: number; packageId?: string; docs?: string; message?: string; + }; + + // The ADR-0112 envelope first — a message-only assertion cannot tell this + // refusal from the code-only one two limbs above it. + expect(err).toMatchObject({ + code: 'WRITABLE_PACKAGE_REQUIRED', + status: 422, + packageId: READ_ONLY_PKG, + docs: 'docs/adr/0070-package-first-authoring.md', + }); + // …then the sentence this card exists for. + const message = String(err.message); + expect(message).toContain("OS_METADATA_WRITABLE is set for 'permission'"); + expect(message).toContain('it unlocks the metadata TYPE, not package writability'); + // Refused, not reported: the diagnostic change persists nothing. + expect(metaRowsOf(engine)).toEqual([]); + }, 30_000); + + it('with the hatch CLOSED the same request does NOT mention it — the clause is selected, not appended', async () => { + // THE OVER-BROAD DIRECTION. A shape that emitted the repository sentence + // unconditionally passes every case above and fails here: with the hatch + // shut the clause is noise, and worse, it names a variable the operator + // never set. `permission` is `allowRuntimeCreate: true`, so this request + // reaches D1 with the hatch shut — the same door, one variable different. + const { engine, protocol } = boot(); + const err = await createPermission(protocol, READ_ONLY_PKG) as { + code?: string; status?: number; message?: string; + }; + + expect(err).toMatchObject({ code: 'WRITABLE_PACKAGE_REQUIRED', status: 422 }); + const message = String(err.message); + expect(message).not.toContain('OS_METADATA_WRITABLE'); + // …and the remedy that IS true here survives the selection. + expect(message).toContain('Switch to a writable package in the package selector'); + expect(metaRowsOf(engine)).toEqual([]); + }, 30_000); + + it('the item is still named — delegation costs the authoring surface no information', async () => { + // D1's own sentence said `Cannot save permission/runtime_reviewer`. The + // emitter's said only the type, because its `put` callers have no name at + // that seam. Delegating without this would have made the toast vaguer. + const { protocol } = boot(); + const err = await createPermission(protocol, READ_ONLY_PKG) as { message?: string }; + expect(String(err.message)).toContain('permission/runtime_reviewer'); + }, 30_000); + + it('the sentence is the repository\'s, not a copy of it', async () => { + // THE ANTI-FORK PIN. Compared against the emitter's OWN output rather than + // a literal: a re-spelling at D1 that is word-perfect on the day it lands + // still goes red here, which is the property "one condition, one emitter" + // actually needs. Both remedy directions, because the selection is what a + // copy would most easily get wrong. + const { protocol } = boot(); + + const shut = await createPermission(protocol, READ_ONLY_PKG) as { message?: string }; + expect(shut.message).toBe( + SysMetadataRepository + .readOnlyBaseCreateError('permission', READ_ONLY_PKG, false, 'runtime_reviewer') + .message, + ); + + openHatch('permission'); + const open = await createPermission(protocol, READ_ONLY_PKG) as { message?: string }; + expect(open.message).toBe( + SysMetadataRepository + .readOnlyBaseCreateError('permission', READ_ONLY_PKG, true, 'runtime_reviewer') + .message, + ); + expect(open.message).not.toBe(shut.message); + }, 30_000); + + // ── the hatch really is what carries the write to D1 ─────────────────── + + it('a type with NO create channel reaches D1 only through the hatch', async () => { + // Both halves in one case, because either alone is ambiguous. `job` has + // neither `allowOrgOverride` nor `allowRuntimeCreate`: shut, the code-only + // refusal answers first and D1 is never consulted (so a hatch clause there + // would be unreachable for this type); open, `isOverlayAllowed` folds the + // hatch in and the write lands on D1 — carrying the clause. + const shutBoot = boot(); + const shut = await save(shutBoot.protocol, { + type: 'job', name: 'nightly_sweep', item: jobBody, packageId: READ_ONLY_PKG, + }) as { code?: string; status?: number }; + expect(shut).toMatchObject({ code: 'NOT_CREATABLE', status: 403 }); + + openHatch('job'); + const openBoot = boot(); + const open = await save(openBoot.protocol, { + type: 'job', name: 'nightly_sweep', item: jobBody, packageId: READ_ONLY_PKG, + }) as { code?: string; status?: number; message?: string }; + expect(open).toMatchObject({ code: 'WRITABLE_PACKAGE_REQUIRED', status: 422 }); + expect(String(open.message)).toContain("OS_METADATA_WRITABLE is set for 'job'"); + }, 30_000); + + // ── ⛔ the acceptance set does not move ──────────────────────────────── + + it('a runtime-only create into a WRITABLE base still lands, hatch open or shut', async () => { + // THE SCOPE PIN. This card changes what the operator is TOLD; if it ever + // starts changing what is refused, it has left its own scope. Membership, + // never a row count: a guard upstream that stabilised the count would make + // a length assertion green with this change absent. + const shutBoot = boot(); + expect(await createPermission(shutBoot.protocol, WRITABLE_PKG)).toBeNull(); + expect(metaRowsOf(shutBoot.engine)).toContainEqual( + expect.objectContaining({ name: 'runtime_reviewer', package_id: WRITABLE_PKG }), + ); + + openHatch('permission'); + const openBoot = boot(); + expect(await createPermission(openBoot.protocol, WRITABLE_PKG)).toBeNull(); + expect(metaRowsOf(openBoot.engine)).toContainEqual( + expect.objectContaining({ name: 'runtime_reviewer', package_id: WRITABLE_PKG }), + ); + }, 30_000); + + it('a package-less runtime-only create still lands, bound to NO package', async () => { + // The other admitted direction: D1's `packageId != null` limb is untouched. + openHatch('permission'); + const { engine, protocol } = boot(); + expect(await createPermission(protocol)).toBeNull(); + expect(metaRowsOf(engine)).toContainEqual( + expect.objectContaining({ name: 'runtime_reviewer', package_id: null }), + ); + }, 30_000); + + // ── topology, and the callers that never pass through D1 ─────────────── + + it('both kernels answer the same thing — D1 sits below the environmentId branch', async () => { + // Compared to each other, not to a literal, for the reason #8184's block + // states: the property is "one condition keeps one vocabulary", and only a + // comparison can tell a shared move from a divergence. + openHatch('permission'); + const hostConfig = await createPermission(boot().protocol, READ_ONLY_PKG) as { + code?: string; status?: number; message?: string; + }; + const scoped = await createPermission(boot('env_alpha').protocol, READ_ONLY_PKG) as { + code?: string; status?: number; message?: string; + }; + + expect(scoped.code).toBe(hostConfig.code); + expect(scoped.status).toBe(hostConfig.status); + expect(scoped.message).toBe(hostConfig.message); + expect(String(scoped.message)).toContain("OS_METADATA_WRITABLE is set for 'permission'"); + }, 30_000); + + it('the direct put callers keep their sentence verbatim — the widening is opt-in', async () => { + // `promoteDraft` / `restoreVersion` / `revertCommit` reach the emitter with + // the row's own binding and no item name. Their sentence is what #8146 + // shipped and must not have moved: `name` is optional and last precisely so + // this stays byte-identical. + const engine = makeFakeEngine(); + const repo = new SysMetadataRepository({ + engine: engine as never, organizationId: null, orgLabel: 'env', + }); + const err = await putWith(repo, { + type: 'job', name: 'nightly_sweep', intent: 'runtime-only', packageId: READ_ONLY_PKG, + }) as { message?: string }; + + expect(err.message).toBe( + SysMetadataRepository.readOnlyBaseCreateError('job', READ_ONLY_PKG, false).message, + ); + // The name-less spelling, verbatim: no `job/nightly_sweep` at this seam. + expect(String(err.message)).toContain(`Cannot create job in package '${READ_ONLY_PKG}'`); + }, 30_000); +}); diff --git a/packages/metadata-protocol/src/sys-metadata-repository.ts b/packages/metadata-protocol/src/sys-metadata-repository.ts index 1ae7a05bac..3e6b5ef674 100644 --- a/packages/metadata-protocol/src/sys-metadata-repository.ts +++ b/packages/metadata-protocol/src/sys-metadata-repository.ts @@ -1218,10 +1218,39 @@ export class SysMetadataRepository implements MetadataRepository { * one vocabulary, stated at both enforcement points rather than re-spelled * differently at each. The message stays user-actionable because here the * prescription is true — a writable base really is what this write needs. + * + * @internal [#8361] `saveMetaItem`'s ADR-0070 D1 gate now CALLS this emitter + * instead of spelling a second sentence for the same condition — the CREATE + * side of what #8184 did on the override side, and the reason this is + * `static` rather than `private static`. ⛔ Do not re-privatise without + * deleting that call site. + * + * Why the duplicate had to go rather than gain a copy of the clause below: + * D1 refuses on a strictly WIDER predicate than this door (no `namedBase` + * limb, no registry limbs above it), so every `saveMetaItem` write that + * would have reached here had already been thrown by D1 — and D1's own + * sentence carried no hatch clause. #8146's clause was therefore live only + * for the direct `put` callers (`promoteDraft` / `restoreVersion` / + * `revertCommit`), which carry the row's OWN package binding and never pass + * through D1. Teaching D1 a SECOND copy of the clause would have put two + * independently-authored sentences behind one condition, which is exactly + * how the `NOT_OVERRIDABLE`-everywhere problem started. + * + * `name` is optional and positional-LAST on purpose. Omitted, the sentence + * is byte-identical to the one the `put` callers have always seen (this seam + * has no item name to offer them). Supplied by D1, it preserves the + * `type/name` D1's own sentence has always shown, so delegation costs the + * authoring surface no information. Widening the one emitter beats forking + * it. */ - private static readOnlyBaseCreateError(type: string, packageId: string, hatchOpen = false): Error { + static readOnlyBaseCreateError( + type: string, + packageId: string, + hatchOpen = false, + name?: string, + ): Error { const err: any = new Error( - `[writable_package_required] Cannot create ${type} in package '${packageId}': ` + `[writable_package_required] Cannot create ${name ? `${type}/${name}` : type} in package '${packageId}': ` + `that package is read-only (provided by code or an installed app), so it is not a writable base. ` + `Switch to a writable package in the package selector, or create a new one, and retry.` // [#8146] Said only when the hatch IS set, because otherwise it is noise.