From 9dd2ba94bca3f58b944b25e4b1102c9b19abd44f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 07:37:03 +0000 Subject: [PATCH 1/2] fix(metadata-protocol, service-storage): report the refused writes three catch blocks swallowed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three tier-1 DARK sites from the #12981 swallow-family worklist (batch 7), across two packages. Control flow is unchanged at every one of them, and none of them is silent any more. metadata-protocol — reassignOrphanedMetadata. ADR-0070 D5's orphan-adoption loop dropped a refused sys_metadata update whole. The return reports `success: reassigned.length > 0`, so an adoption in which 99 of 100 orphans were refused answered `{ success: true, reassignedCount: 1 }` while the 99 stayed orphans with nothing recording that they had been tried. The loop now counts refusals and states the degradation ONCE after the loop at console.error, naming the count, the target package, the driver's sentence and the fix. `error` and not this file's usual console.warn, by the AGENTS.md question: the system keeps looking normal while something it claims to have persisted did not land. Same verdict recordPackageCommit already reaches in this file, and the inverse of clientFacingRowFailureText's recorded reason for choosing warn. The response shape is untouched. service-storage — two functional sites at the tail of start(), both warn. The settings-namespace binding caught two outcomes and named one: absence (a bare kernel, correctly silent) is now resolved on its own line, and a binding that fails with the service PRESENT is reported, because start() otherwise completes into a healthy-looking boot whose storage settings screen is wired to nothing. The storage/test probe cleanup swallowed its own failure; the result beside it reports the PROBE's failure, a different one, so a stray probe key accrued per failed test with the only record of its name lost. Both are warn on the merits: neither is a durability degradation, and AGENTS.md is explicit that escalating these is what makes error unreadable. No sink type is changed at any of the three sites — PluginContext.logger declares error non-optional, and metadata-protocol reports on console — so no published sink shape is widened. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs --- .changeset/durability-swallow-batch-7.md | 58 +++++ ...otocol.reassign-orphans-durability.test.ts | 238 ++++++++++++++++++ packages/metadata-protocol/src/protocol.ts | 53 +++- ...e-plugin.durability-swallow-repair.test.ts | 218 ++++++++++++++++ .../src/storage-service-plugin.ts | 74 +++++- 5 files changed, 633 insertions(+), 8 deletions(-) create mode 100644 .changeset/durability-swallow-batch-7.md create mode 100644 packages/metadata-protocol/src/protocol.reassign-orphans-durability.test.ts create mode 100644 packages/services/service-storage/src/storage-service-plugin.durability-swallow-repair.test.ts diff --git a/.changeset/durability-swallow-batch-7.md b/.changeset/durability-swallow-batch-7.md new file mode 100644 index 0000000000..5c265fe97d --- /dev/null +++ b/.changeset/durability-swallow-batch-7.md @@ -0,0 +1,58 @@ +--- +'@objectstack/metadata-protocol': minor +'@objectstack/service-storage': minor +--- + +Report the refused writes three `catch { }` sites swallowed (#12981 batch 7) + +Three tier-1 DARK sites from the #12981 swallow-family worklist, across two +packages. Control flow is unchanged at every one of them — none of these +failures should abort the operation it sits inside — but none of them is silent +any more. + +**`metadata-protocol` — `reassignOrphanedMetadata` (the durability one).** +ADR-0070 D5's orphan-adoption loop dropped a refused `sys_metadata` update +whole: not logged, not rethrown, not carried on the response. The return line +reports `success: reassigned.length > 0`, so an adoption in which 99 of 100 +orphans were refused answered `{ success: true, reassignedCount: 1 }` — a +response identical in shape to a healthy run with one orphan to move — while +the 99 stayed orphans, with nothing retrying them and no record that they had +been tried. The loop now counts refusals and states the degradation **once** +after the loop at `console.error`, naming the count, the target package, the +driver's own sentence and the fix. `error` and not this file's usual +`console.warn`, by the AGENTS.md question this turns on: the system keeps +looking normal while something it claims to have persisted did not land. It is +the verdict `recordPackageCommit` in the same file already reaches on the same +sink, and the inverse of the one `clientFacingRowFailureText` records for its +`console.warn` (there the row reports `success: false` and the counters +reconcile; here neither holds). The response shape is untouched — no +`failedCount` was added. + +**`service-storage` — two sites at the tail of `StorageServicePlugin.start()`, +both functional, both `warn`.** + +- The settings-namespace binding ended in `catch { }` with a comment naming + only one of the two outcomes it caught. The settings service being **absent** + (a bare kernel, where nothing ever claimed the admin UI could swap adapters) + is now resolved on its own line and stays correctly silent; a binding that + **fails with the service present** is reported, because `start()` otherwise + completes into a healthy-looking boot whose storage settings screen is wired + to nothing — an operator's adapter or credential change is saved and never + applied. +- The `storage/test` probe cleanup swallowed its own failure in + `catch { /* ignore */ }`. The result returned beside it reports the *probe's* + failure, which is a different failure: one stray `__objectstack_probe__/…` + key accrued per failed test and the only record of its name died with the + frame. The refused cleanup now names the key it left behind. + +Both `service-storage` sites are `warn` on the merits, not by default: neither +is a durability degradation. Storage keeps serving from the adapter the +plugin's own options built, and the leaked probe object is inert content no +record references — AGENTS.md is explicit that escalating these is what makes +`error` unreadable. No sink type is changed at any of the three sites: the two +`service-storage` reports go to `PluginContext.logger`, whose `error` is +already non-optional, and `metadata-protocol` reports on `console`. + +Each repaired seam is pinned by a test that fails if it goes quiet again, plus +absence-asserting controls — declared as controls — so a seam that reports +unconditionally cannot pass. diff --git a/packages/metadata-protocol/src/protocol.reassign-orphans-durability.test.ts b/packages/metadata-protocol/src/protocol.reassign-orphans-durability.test.ts new file mode 100644 index 0000000000..f17fcc35a1 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.reassign-orphans-durability.test.ts @@ -0,0 +1,238 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#12981 batch 7] A refused orphan rebind in `reassignOrphanedMetadata` must + * not be silent. + * + * ADR-0070 D5's adoption loop walked every package-less `sys_metadata` row and + * rebound it to a target base. Its `catch` was bare: + * + * } catch { + * /* skip a row that fails to update; report only what moved *\/ + * } + * + * and the return line below it reports `success: reassigned.length > 0`. Put + * together, an adoption in which 99 of 100 orphans were REFUSED answered + * `{ success: true, reassignedCount: 1 }` — a response byte-identical in shape + * to a healthy run that had exactly one orphan to move. The 99 stayed orphans, + * nothing retried them, and no line anywhere recorded that they had been tried. + * That is the AGENTS.md durability shape exactly: the system keeps looking + * normal while something it claims to have persisted did not land. + * + * ## What this file pins, and what it deliberately does NOT + * + * ONLY the silence changes. The loop must still skip the refused row and adopt + * the rest — aborting the adoption over one unwritable row would strand the + * rows that CAN move — and the response shape is untouched, because adding a + * `failedCount` is a contract change this card does not carry. Both halves are + * asserted below rather than assumed. + * + * ## The level, stated so it can be argued with + * + * `console.error`, not `console.warn`, and not by default: `console.warn` is + * this file's overwhelming idiom (51 sites) and `clientFacingRowFailureText` + * records the discriminator in prose — it chose `warn` "deliberately — nothing + * claimed to be persisted was silently dropped (the row reports + * `success: false` and the counters reconcile)". Here NEITHER holds. The + * matching precedent is in this same file: `recordPackageCommit` already + * answers `console.error` for a refused `sys_metadata_commit` write under a + * publish that reports success. + * + * ## ONE line, not one per row + * + * AGENTS.md → "Degradation log levels" requires the report be stated once, at + * the first occurrence, not once per failed write — and a `sys_metadata` write + * that is refused is refused for every row, so the per-row spelling would print + * one line per orphan in the environment. The count is pinned, not just the + * presence. + * + * ⚠️ Two cases below are CONTROLS, not pins: they assert an ABSENCE against a + * seam that logged nothing at all before this repair, so they stay green in + * both directions by construction and are not evidence in an ablation. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +// [#5619] The producer's OWN write-verb dispatch decision, so this double +// cannot accept an `update` shape ObjectQL refuses. From +// `@objectstack/metadata-core` and NOT `@objectstack/objectql` — objectql +// depends on THIS package, so that import would close a cycle turbo rejects. +import { assertEngineUpdateDispatch } from '@objectstack/metadata-core'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +interface MetaRow { + id: string; + type: string; + name: string; + organization_id: string | null; + package_id: string | null; +} + +/** + * Engine double over `sys_metadata` with a per-id refusal injector on `update`. + * + * The injection is keyed by ROW ID rather than being a global switch, because + * the defect's dangerous case is the PARTIAL one: an adoption where some rows + * move and some do not is the run that answers `success: true` while leaving + * orphans behind. A double that could only fail everything could not express + * it. + */ +function makeEngine(rows: MetaRow[]) { + const store = new Map(rows.map((r) => [r.id, { ...r }])); + const refuse = new Set(); + let updateAttempts = 0; + + const engine = { + async find(table: string, opts?: { where?: Record }) { + if (table !== 'sys_metadata') return []; + const where = opts?.where ?? {}; + return [...store.values()].filter((r) => { + const or = (where as { $or?: Array> }).$or; + if (!or) return true; + return or.some((c) => + Object.entries(c).every(([k, v]) => (r as unknown as Record)[k] === v), + ); + }); + }, + async update(_table: string, data: Record, opts: { where: Record }) { + assertEngineUpdateDispatch(data, opts); + updateAttempts += 1; + const id = String(opts.where.id); + if (refuse.has(id)) throw new Error(`write refused for ${id}: permission denied on sys_metadata`); + const row = store.get(id); + if (!row) return { id: null }; + Object.assign(row, data); + return { id }; + }, + }; + + return { + engine, + store, + refuseIds: (...ids: string[]) => ids.forEach((i) => refuse.add(i)), + updateAttempts: () => updateAttempts, + }; +} + +const orphan = (id: string): MetaRow => ({ + id, + type: 'object', + name: `obj_${id}`, + organization_id: null, + package_id: null, +}); + +/** The one sentence fragment an operator greps for. */ +const HEADLINE = 'orphaned metadata row(s) were NOT rebound'; + +afterEach(() => { + vi.restoreAllMocks(); +}); + +function spyConsole() { + return { + error: vi.spyOn(console, 'error').mockImplementation(() => {}), + warn: vi.spyOn(console, 'warn').mockImplementation(() => {}), + }; +} + +describe('reassignOrphanedMetadata: a refused rebind is reported (#12981)', () => { + // ⚠️ CONTROL, not a pin. Before the repair this seam logged nothing at any + // level, so "a healthy adoption says nothing" was already true — it stays + // green in BOTH directions and is not ablation evidence. It is here so the + // pins below cannot pass on a seam that reports unconditionally. + it('CONTROL: an adoption in which every row rebinds reports nothing', async () => { + const spy = spyConsole(); + const stub = makeEngine([orphan('a'), orphan('b')]); + const protocol = new ObjectStackProtocolImplementation(stub.engine as never); + + const res = await protocol.reassignOrphanedMetadata({ targetPackageId: 'app.base' }); + + expect(res.reassignedCount).toBe(2); + expect(res.success).toBe(true); + expect(stub.updateAttempts()).toBe(2); + expect(spy.error).not.toHaveBeenCalled(); + expect(spy.warn).not.toHaveBeenCalled(); + }); + + it('reports a PARTIAL adoption — the run that still answers success: true', async () => { + const spy = spyConsole(); + const stub = makeEngine([orphan('a'), orphan('b'), orphan('c')]); + stub.refuseIds('b', 'c'); + const protocol = new ObjectStackProtocolImplementation(stub.engine as never); + + const res = await protocol.reassignOrphanedMetadata({ targetPackageId: 'app.base' }); + + // Proof the writes were really attempted and really threw — otherwise + // every assertion below is about an adoption that never reached the seam. + expect(stub.updateAttempts()).toBe(3); + expect(stub.store.get('b')!.package_id).toBeNull(); + expect(stub.store.get('c')!.package_id).toBeNull(); + + // ⛔ The response is UNCHANGED: this is the shape that reads healthy. + expect(res.success).toBe(true); + expect(res.reassignedCount).toBe(1); + expect(res.reassigned).toEqual([{ type: 'object', name: 'obj_a' }]); + + // …and it is no longer the only thing that happened. + expect(spy.error).toHaveBeenCalledTimes(1); + const line = String(spy.error.mock.calls[0][0]); + expect(line).toContain(HEADLINE); + expect(line).toContain('2 of 3'); + expect(line).toContain('app.base'); + // The consequence and the fix, which AGENTS.md requires of this level. + expect(line).toContain('STILL orphans'); + expect(line).toContain('Fix: restore write access'); + // The driver's own sentence, so the operator is not left guessing why. + expect(line).toContain('permission denied on sys_metadata'); + }); + + it('reports a TOTAL refusal, where the response already says success: false', async () => { + const spy = spyConsole(); + const stub = makeEngine([orphan('a'), orphan('b')]); + stub.refuseIds('a', 'b'); + const protocol = new ObjectStackProtocolImplementation(stub.engine as never); + + const res = await protocol.reassignOrphanedMetadata({ targetPackageId: 'app.base' }); + + expect(res.success).toBe(false); + expect(res.reassignedCount).toBe(0); + expect(spy.error).toHaveBeenCalledTimes(1); + expect(String(spy.error.mock.calls[0][0])).toContain('2 of 2'); + }); + + it('states the degradation ONCE, not once per refused row', async () => { + const spy = spyConsole(); + const ids = ['a', 'b', 'c', 'd', 'e', 'f']; + const stub = makeEngine(ids.map(orphan)); + stub.refuseIds(...ids); + const protocol = new ObjectStackProtocolImplementation(stub.engine as never); + + await protocol.reassignOrphanedMetadata({ targetPackageId: 'app.base' }); + + // Six refused writes, ONE operator-facing line — AGENTS.md's "say it + // once, at the first degradation, not once per failed write". + expect(stub.updateAttempts()).toBe(6); + expect(spy.error).toHaveBeenCalledTimes(1); + expect(String(spy.error.mock.calls[0][0])).toContain('6 of 6'); + }); + + // ⚠️ CONTROL, not a pin — an INVARIANCE assertion. The pre-repair code + // returns exactly this too, so it stays green in both directions. It is + // here because the repair would be wrong if it changed control flow. + it('CONTROL: a refused row does not abort the rows that can still move', async () => { + spyConsole(); + const stub = makeEngine([orphan('a'), orphan('b'), orphan('c')]); + stub.refuseIds('a'); + const protocol = new ObjectStackProtocolImplementation(stub.engine as never); + + const res = await protocol.reassignOrphanedMetadata({ targetPackageId: 'app.base' }); + + expect(res.reassignedCount).toBe(2); + expect(stub.store.get('b')!.package_id).toBe('app.base'); + expect(stub.store.get('c')!.package_id).toBe('app.base'); + // The response shape is untouched: no `failedCount` was added. + expect(Object.keys(res).sort()).toEqual( + ['reassigned', 'reassignedCount', 'success', 'targetPackageId'].sort(), + ); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 7e3ddb6d97..03be73cf57 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -17815,6 +17815,28 @@ export class ObjectStackProtocolImplementation implements ); const reassigned: Array<{ type: string; name: string }> = []; + // [#12981] The refusal channel this loop used to lack entirely. + // + // The catch below is the card's defining shape, and this method is the + // sharpest instance of it in the file: a refused `update` was dropped + // whole -- not logged, not rethrown, not carried on the response -- and + // the return then reported `success: reassigned.length > 0`. So an + // adoption where 99 of 100 orphans were refused answered + // `{success: true, reassignedCount: 1}`, byte-identical in shape to a + // healthy run with one orphan to move, and the 99 stayed orphans with + // nothing anywhere recording that they had been tried. Nothing retries + // them and no later boot reconstructs the attempt. + // + // A COUNTER plus one report AFTER the loop, deliberately, and not a + // `console.error` inside the catch: AGENTS.md -> "Degradation log + // levels" says an operator-facing degradation is stated ONCE, at the + // first occurrence, not once per failed write -- and a refused + // `sys_metadata` write is refused for every row, so the per-row + // spelling would print one line per orphan in the environment. This is + // the same shape #12923's shared refusal accumulator takes at the other + // repaired seams of this family. + let refusedCount = 0; + let firstRefusal = ''; for (const row of orphans) { try { await this.engine.update( @@ -17823,10 +17845,37 @@ export class ObjectStackProtocolImplementation implements { where: { id: row.id } }, ); reassigned.push({ type: row.type, name: row.name }); - } catch { - /* skip a row that fails to update; report only what moved */ + } catch (e: any) { + // Control flow is UNCHANGED: a row that cannot be rebound must + // not abort the adoption of the rows that can. Only the + // silence changes. + refusedCount += 1; + if (firstRefusal === '') firstRefusal = e?.message ?? String(e); } } + if (refusedCount > 0) { + // `error` and not `warn`, by the one question AGENTS.md turns this + // on -- after the degradation the system still looks normal from + // the outside while something it claims to have persisted did not + // land. It is the same verdict, on the same sink, that + // `recordPackageCommit` in this file already reaches for the + // `sys_metadata_commit` write, and the inverse of the one + // `clientFacingRowFailureText` records for its `console.warn` + // (there the row reports `success: false` and the counters + // reconcile, so nothing was silently dropped; here neither holds). + console.error( + `[Protocol] reassignOrphanedMetadata: ${refusedCount} of ${orphans.length} orphaned ` + + `metadata row(s) were NOT rebound to package '${request.targetPackageId}' -- the ` + + `update was REFUSED. The call still answers reassignedCount=${reassigned.length}` + + `${reassigned.length > 0 ? ' with success: true' : ''}, so nothing looks broken, but ` + + 'those rows are STILL orphans: they keep `package_id` null or the `sys_metadata` ' + + 'sentinel, this environment has NOT converged on the package-first model (ADR-0070 ' + + 'D5 completes when an environment has no orphans), and nothing retries them. ' + + `First refusal: ${firstRefusal}. Fix: restore write access to \`sys_metadata\` for ` + + 'the system context and run the adoption again -- it is idempotent, rows already ' + + 'bound to a real package are left untouched.', + ); + } return { success: reassigned.length > 0, reassignedCount: reassigned.length, diff --git a/packages/services/service-storage/src/storage-service-plugin.durability-swallow-repair.test.ts b/packages/services/service-storage/src/storage-service-plugin.durability-swallow-repair.test.ts new file mode 100644 index 0000000000..f395a1e7ea --- /dev/null +++ b/packages/services/service-storage/src/storage-service-plugin.durability-swallow-repair.test.ts @@ -0,0 +1,218 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#12981 batch 7] The two silent `catch` blocks at the tail of + * `StorageServicePlugin.start()`. + * + * ## Site A — the settings-namespace binding + * + * One `try` wrapped the whole "bind to the `storage` settings namespace" block + * and ended in `catch { // settings service not present }`. The comment named + * one outcome; the catch caught two: + * + * - the settings service is ABSENT (a bare kernel) — nothing ever claimed the + * admin UI could swap adapters, so silence is correct and reporting it + * would put a line on every bare-kernel boot; + * - the settings service is PRESENT and the binding FAILED part-way — + * `subscribe` or `registerAction` threw. `start()` then completes, the + * kernel reports a healthy boot, and the storage settings screen is wired + * to nothing: an operator's adapter change is saved and never applied. + * + * Resolving `getService('settings')` on its own line splits them, so the + * remaining catch covers exactly the second. + * + * ## Site B — the `storage/test` probe cleanup + * + * `try { await proxy.delete(probeKey); } catch { /* ignore *\/ }`. The `return` + * beside it reports the PROBE's failure — a different failure. The cleanup's + * own failure reached nobody, so one stray `__objectstack_probe__/…` key + * accrued per failed test and the only record of its name (minted per call from + * a timestamp plus a random suffix) died with the frame. + * + * ## The level at BOTH sites: `warn`, and decided on the merits + * + * Neither is a durability degradation. Site A is textbook FUNCTIONAL under + * AGENTS.md — "a capability is not enabled… the next person to use the missing + * thing finds out" — and storage itself keeps serving from the adapter the + * plugin's own options built. Site B is housekeeping that did not happen; the + * leaked object is inert probe content no record references. AGENTS.md is + * explicit that escalating these is what makes `error` unreadable. + * + * ⚠️ Two cases below are CONTROLS, not pins: they assert an ABSENCE against + * seams that logged nothing at all before this repair, so they stay green in + * both directions by construction and are not ablation evidence. + */ + +import { describe, it, expect } from 'vitest'; +import { promises as fs } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import type { IStorageService } from '@objectstack/spec/contracts'; +import { StorageServicePlugin } from './storage-service-plugin.js'; + +function makeCtx() { + const services = new Map(); + const hooks: Array<() => Promise | void> = []; + const logs: { info: string[]; warn: string[]; error: string[] } = { info: [], warn: [], error: [] }; + const ctx: any = { + logger: { + info: (m: string) => { logs.info.push(String(m)); }, + warn: (m: string) => { logs.warn.push(String(m)); }, + error: (m: string) => { logs.error.push(String(m)); }, + }, + _logs: logs, + registerService: (name: string, svc: any) => { services.set(name, svc); }, + getService: (name: string): T => { + const s = services.get(name); + if (!s) throw new Error(`service '${name}' not registered`); + return s as T; + }, + hook: (event: string, fn: () => Promise | void) => { + if (event === 'kernel:ready') hooks.push(fn); + }, + _flushReady: async () => { for (const h of hooks) await h(); }, + }; + return ctx; +} + +/** + * A settings service whose `registerAction` can be made to throw. + * + * `registerAction` and not `getNamespace`, deliberately: `applySettings` has + * its own inner catch that already warns, so a failure there was never part of + * this family. `registerAction` is reached only through the OUTER try, which is + * the seam under test. + */ +function makeFakeSettings(opts: { failRegisterAction?: Error } = {}) { + const actions = new Map Promise>(); + return { + createClient: (_ns: string) => ({}), + getNamespace: async (_ns: string) => ({ values: {} }), + subscribe: (_ns: string, _fn: () => void) => {}, + registerAction: (ns: string, id: string, fn: (input: any) => Promise) => { + if (opts.failRegisterAction) throw opts.failRegisterAction; + actions.set(`${ns}/${id}`, fn); + }, + _runAction: async (ns: string, id: string, input: any) => { + const fn = actions.get(`${ns}/${id}`); + if (!fn) throw new Error(`no action ${ns}/${id}`); + return await fn(input); + }, + }; +} + +/** An adapter that refuses the verbs a test names, and counts what was tried. */ +function makeBrokenAdapter(refuse: { upload?: boolean; delete?: boolean }) { + const tried = { upload: 0, delete: 0 }; + const adapter = { + async upload(_k: string, _b: Buffer, _o?: unknown) { + tried.upload += 1; + if (refuse.upload) throw new Error('upload refused: bucket is read-only'); + return { key: _k } as never; + }, + async download(_k: string) { return Buffer.from('probe', 'utf-8'); }, + async delete(_k: string) { + tried.delete += 1; + if (refuse.delete) throw new Error('delete refused: bucket is read-only'); + }, + async exists(_k: string) { return true; }, + async list() { return { items: [] } as never; }, + async getUrl(k: string) { return `mem://${k}`; }, + }; + return { adapter: adapter as unknown as IStorageService, tried }; +} + +async function bootedPlugin(settings: unknown) { + const dir = await fs.mkdtemp(join(tmpdir(), 'oss-b7-')); + const plugin = new StorageServicePlugin({ + adapter: 'local', + local: { rootDir: dir }, + registerRoutes: false, + }); + const ctx = makeCtx(); + if (settings) ctx.registerService('settings', settings); + await plugin.init(ctx); + await plugin.start(ctx); + await ctx._flushReady(); + return { plugin, ctx, dir }; +} + +const BINDING_HEADLINE = 'settings namespace binding FAILED'; +const CLEANUP_HEADLINE = 'was NOT removed'; + +describe('StorageServicePlugin site A: settings binding (#12981)', () => { + // ⚠️ CONTROL, not a pin. The pre-repair code was silent here too, so this + // stays green in BOTH directions. It exists so the pin below cannot pass on + // a seam that reports unconditionally — and it pins the DIRECTION of the + // split: absence must stay silent. + it('CONTROL: a bare kernel with no settings service stays silent, and still starts', async () => { + const { ctx } = await bootedPlugin(null); + + expect(ctx._logs.warn.join('\n')).not.toContain(BINDING_HEADLINE); + expect(ctx._logs.error).toEqual([]); + // Storage itself is up: the absence really is only functional. + expect(ctx.getService('storage')).toBeTruthy(); + }); + + it('a binding that FAILS with the service PRESENT is reported, and names what stops working', async () => { + const boom = new Error('settings action registry is sealed'); + const settings = makeFakeSettings({ failRegisterAction: boom }); + + const { ctx } = await bootedPlugin(settings); + + const warned = ctx._logs.warn.filter((l: string) => l.includes(BINDING_HEADLINE)); + expect(warned).toHaveLength(1); + // The driver's own sentence, so the operator is not left guessing. + expect(warned[0]).toContain('settings action registry is sealed'); + // The consequence, concretely — and that the boot still looks healthy. + expect(warned[0]).toContain('will be saved but NOT applied'); + // The fix, which is the other half AGENTS.md requires of a degradation line. + expect(warned[0]).toContain('bindToSettings: false'); + // ⛔ Control flow unchanged: a failed binding must not fail the boot. + expect(ctx.getService('storage')).toBeTruthy(); + expect(ctx._logs.error).toEqual([]); + }); +}); + +describe('StorageServicePlugin site B: storage/test probe cleanup (#12981)', () => { + it('a refused probe cleanup is reported, and names the stray key', async () => { + const settings = makeFakeSettings(); + const { ctx } = await bootedPlugin(settings); + const broken = makeBrokenAdapter({ upload: true, delete: true }); + (ctx.getService('storage') as any).swap(broken.adapter); + + const result = await settings._runAction('storage', 'test', { values: {} }); + + // Proof the seam was really reached: the probe upload threw and the + // cleanup delete was really attempted and really refused. + expect(broken.tried.upload).toBe(1); + expect(broken.tried.delete).toBe(1); + + const warned = ctx._logs.warn.filter((l: string) => l.includes(CLEANUP_HEADLINE)); + expect(warned).toHaveLength(1); + expect(warned[0]).toContain('__objectstack_probe__/'); + expect(warned[0]).toContain('delete refused: bucket is read-only'); + + // ⛔ The probe RESULT is untouched — the operator still reads the original + // failure, not the cleanup's. + expect(result.ok).toBe(false); + expect(result.severity).toBe('error'); + expect(result.message).toBe('upload refused: bucket is read-only'); + }); + + // ⚠️ CONTROL, not a pin — green in both directions, because the pre-repair + // code also said nothing when the cleanup succeeded. It is here so the pin + // above cannot pass on a seam that warns on every failed probe. + it('CONTROL: a probe failure whose cleanup SUCCEEDS reports nothing on that channel', async () => { + const settings = makeFakeSettings(); + const { ctx } = await bootedPlugin(settings); + const broken = makeBrokenAdapter({ upload: true, delete: false }); + (ctx.getService('storage') as any).swap(broken.adapter); + + const result = await settings._runAction('storage', 'test', { values: {} }); + + expect(broken.tried.delete).toBe(1); + expect(result.ok).toBe(false); + expect(ctx._logs.warn.join('\n')).not.toContain(CLEANUP_HEADLINE); + }); +}); diff --git a/packages/services/service-storage/src/storage-service-plugin.ts b/packages/services/service-storage/src/storage-service-plugin.ts index 96f307ad68..a41fcaf607 100644 --- a/packages/services/service-storage/src/storage-service-plugin.ts +++ b/packages/services/service-storage/src/storage-service-plugin.ts @@ -458,9 +458,24 @@ export class StorageServicePlugin implements Plugin { // Allows the admin UI to swap adapters / credentials without // restart. Env-locked fields still win at the resolver layer. if (this.options.bindToSettings === false) return; + // [#12981] The settings service being ABSENT and the settings binding + // FAILING are two different outcomes, and one `try` used to spell them + // identically -- `catch { }` with a comment that named only the first. + // Resolving the service on its own line leaves the catch below covering + // exactly the second. + let settings: any; + try { + settings = ctx.getService('settings'); + } catch { + // A DECLARED absence, and silence is correct here: a bare kernel + // registers no `settings` service, so nothing ever claimed the admin + // UI could swap adapters at runtime, and the manifest fallback handler + // stays. Reporting it would put a line on every bare-kernel boot -- + // the "warn nobody reads" that makes the real one below unreadable. + return; + } + if (!settings || typeof settings.createClient !== 'function') return; try { - const settings = ctx.getService('settings'); - if (!settings || typeof settings.createClient !== 'function') return; const applySettings = async () => { if (!this.storage) return; @@ -554,15 +569,62 @@ export class StorageServicePlugin implements Plugin { message: `Storage round-trip succeeded (adapter=${adapter}).`, }; } catch (err: any) { - // Best-effort cleanup - try { await (proxy as IStorageService).delete(probeKey); } catch { /* ignore */ } + // Best-effort cleanup. [#12981] The cleanup's OWN failure used to + // be swallowed by `catch { /* ignore */ }`. The `return` below + // reports the PROBE's failure, which is a different failure: the + // operator is told the round-trip did not work and is told + // nothing at all about the probe object that is still sitting in + // the bucket. One stray key accrues per failed test, with the + // only record of its name -- `probeKey` is minted per call from a + // timestamp and a random suffix -- lost when this frame returns. + // + // `warn`, not `error`: nothing the system claims to a caller as + // persisted was lost. This is housekeeping that did not happen, + // and the AGENTS.md rule is explicit that escalating a + // non-durability degradation is how `error` stops being read. + try { + await (proxy as IStorageService).delete(probeKey); + } catch (cleanupErr: any) { + ctx.logger.warn( + `StorageServicePlugin: the storage/test probe object '${probeKey}' was NOT removed — ` + + `the cleanup delete was refused (${cleanupErr?.message ?? cleanupErr}). The probe ` + + 'result returned below is unaffected and still reports the original failure. ' + + 'This object is inert probe content that no record references and nothing ' + + 'retries or later collects; delete it by hand if stray keys under ' + + '`__objectstack_probe__/` matter in this bucket.', + ); + } return { ok: false, severity: 'error', message: err?.message ?? String(err) }; } }); ctx.logger.info('StorageServicePlugin: registered settings action storage/test'); } - } catch { - // settings service not present — manifest fallback handler stays + } catch (err: any) { + // [#12981] Reached only when the settings service IS present and the + // binding above failed part-way -- `subscribe` or `registerAction` + // threw, or `applySettings`'s first call did. The plugin then finishes + // `start()` and the kernel reports a healthy boot, while the storage + // settings screen is wired to nothing: an operator's adapter or + // credential change is accepted by the form and never reaches this + // service, and the `storage/test` button answers "no action". + // + // `warn` and not `error`, by the AGENTS.md question: this is a + // FUNCTIONAL degradation, not a durability one. Nothing claimed to be + // persisted was dropped -- the adapter this plugin constructed keeps + // serving every upload and download, and the next person to use the + // missing capability finds out. Escalating it would train readers to + // skim `error`, which is what made the founding incident's `warn` + // unreadable. + ctx.logger.warn( + 'StorageServicePlugin: the `storage` settings namespace binding FAILED — ' + + (err?.message ?? String(err)) + + '. The settings service IS present, so this is not a bare kernel: adapter and ' + + 'credential changes made in the admin UI will be saved but NOT applied to this ' + + 'service, and the live `storage/test` probe may be unregistered. Storage itself is ' + + 'unaffected — the adapter built from this plugin\'s own options keeps serving. Fix: ' + + 'restart after repairing the settings service, or pass `bindToSettings: false` to ' + + 'make the static configuration deliberate.', + ); } }); } From 88127211c746261291f85a4bd38bc9525ce7ac84 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 08:14:00 +0000 Subject: [PATCH 2/2] chore: satisfy the gates the batch-7 diff moved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three gate verdicts, each repaired at its own cause rather than at a ledger: - check:slot-lookup — splitting the settings lookup from its declaration is the rule's own FOURTH erasure shape (`let x: any; try { x = getService(…) }`). Named the slice this plugin actually uses as StorageSettingsSlot, which also retires the pre-existing `getService` there, so the file's count falls 6 -> 5. The gate's verdict line asked for the DOWNWARD ratchet by name: "erasure count fell 6 -> 5 — ratchet DOWN: run `pnpm check:slot-lookup --update` and commit the baseline." One line in the baseline, shrink only. Two `as any` service lookups in the new test are typed at their source. - check:objectql-double-limit — the new `find` double filtered on a `where` it only half implemented, so the control probe could not seat it (UNJUDGED). It now REFUSES both the `where` combinators and the `limit` it does not implement, which is the convention the gate's own remedy names and the stronger of the two: every case here adopts env-wide orphans, so the producer passes `{}` and no bound. - tsc — `ctx.getService(…)` on the `any`-typed fake ctx is a type argument on an untyped call (TS2347). The storage service is typed once, at the harness, by plain assignment: neither a type argument nor an `as any`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs --- ...otocol.reassign-orphans-durability.test.ts | 25 ++++++++++++------ ...e-plugin.durability-swallow-repair.test.ts | 16 ++++++++---- .../src/storage-service-plugin.ts | 26 +++++++++++++++++-- scripts/slot-lookup-baseline.json | 2 +- 4 files changed, 53 insertions(+), 16 deletions(-) diff --git a/packages/metadata-protocol/src/protocol.reassign-orphans-durability.test.ts b/packages/metadata-protocol/src/protocol.reassign-orphans-durability.test.ts index f17fcc35a1..6e415e6b3c 100644 --- a/packages/metadata-protocol/src/protocol.reassign-orphans-durability.test.ts +++ b/packages/metadata-protocol/src/protocol.reassign-orphans-durability.test.ts @@ -82,16 +82,25 @@ function makeEngine(rows: MetaRow[]) { let updateAttempts = 0; const engine = { - async find(table: string, opts?: { where?: Record }) { + async find(table: string, opts?: { where?: Record; limit?: number }) { if (table !== 'sys_metadata') return []; + // This double implements NEITHER a `where` combinator NOR a bound, + // and REFUSES both rather than answering them silently. Every case + // in this file adopts env-wide orphans, so the producer passes + // `{ where: {} }` and no `limit`; the org-scoped `$or` branch and + // paging belong to tests that do not exist yet. A double looser + // than the engine it stands in for converts a green suite into no + // suite at all (#4434) — and the reason to refuse rather than + // approximate is that the approximation is invisible on the day the + // producer starts using the shape. const where = opts?.where ?? {}; - return [...store.values()].filter((r) => { - const or = (where as { $or?: Array> }).$or; - if (!or) return true; - return or.some((c) => - Object.entries(c).every(([k, v]) => (r as unknown as Record)[k] === v), - ); - }); + if (Object.keys(where).length > 0) { + throw new Error(`fake engine: unsupported where ${JSON.stringify(where)}`); + } + if (opts?.limit !== undefined) { + throw new Error('fake engine: unsupported `limit` — this double holds no bound'); + } + return [...store.values()]; }, async update(_table: string, data: Record, opts: { where: Record }) { assertEngineUpdateDispatch(data, opts); diff --git a/packages/services/service-storage/src/storage-service-plugin.durability-swallow-repair.test.ts b/packages/services/service-storage/src/storage-service-plugin.durability-swallow-repair.test.ts index f395a1e7ea..d191546357 100644 --- a/packages/services/service-storage/src/storage-service-plugin.durability-swallow-repair.test.ts +++ b/packages/services/service-storage/src/storage-service-plugin.durability-swallow-repair.test.ts @@ -49,6 +49,7 @@ import { join } from 'node:path'; import { tmpdir } from 'node:os'; import type { IStorageService } from '@objectstack/spec/contracts'; import { StorageServicePlugin } from './storage-service-plugin.js'; +import type { SwappableStorageService } from './swappable-storage-service.js'; function makeCtx() { const services = new Map(); @@ -134,7 +135,12 @@ async function bootedPlugin(settings: unknown) { await plugin.init(ctx); await plugin.start(ctx); await ctx._flushReady(); - return { plugin, ctx, dir }; + // Typed HERE rather than at each call site: the fake ctx is `any`, so + // `ctx.getService(…)` is a type argument on an untyped call (TS2347) and + // `as any` would be a slot-lookup erasure (#4251). A plain typed assignment + // is neither. + const storage: SwappableStorageService = ctx.getService('storage'); + return { plugin, ctx, dir, storage }; } const BINDING_HEADLINE = 'settings namespace binding FAILED'; @@ -177,9 +183,9 @@ describe('StorageServicePlugin site A: settings binding (#12981)', () => { describe('StorageServicePlugin site B: storage/test probe cleanup (#12981)', () => { it('a refused probe cleanup is reported, and names the stray key', async () => { const settings = makeFakeSettings(); - const { ctx } = await bootedPlugin(settings); + const { ctx, storage } = await bootedPlugin(settings); const broken = makeBrokenAdapter({ upload: true, delete: true }); - (ctx.getService('storage') as any).swap(broken.adapter); + storage.swap(broken.adapter); const result = await settings._runAction('storage', 'test', { values: {} }); @@ -205,9 +211,9 @@ describe('StorageServicePlugin site B: storage/test probe cleanup (#12981)', () // above cannot pass on a seam that warns on every failed probe. it('CONTROL: a probe failure whose cleanup SUCCEEDS reports nothing on that channel', async () => { const settings = makeFakeSettings(); - const { ctx } = await bootedPlugin(settings); + const { ctx, storage } = await bootedPlugin(settings); const broken = makeBrokenAdapter({ upload: true, delete: false }); - (ctx.getService('storage') as any).swap(broken.adapter); + storage.swap(broken.adapter); const result = await settings._runAction('storage', 'test', { values: {} }); diff --git a/packages/services/service-storage/src/storage-service-plugin.ts b/packages/services/service-storage/src/storage-service-plugin.ts index a41fcaf607..51f623729c 100644 --- a/packages/services/service-storage/src/storage-service-plugin.ts +++ b/packages/services/service-storage/src/storage-service-plugin.ts @@ -463,9 +463,9 @@ export class StorageServicePlugin implements Plugin { // identically -- `catch { }` with a comment that named only the first. // Resolving the service on its own line leaves the catch below covering // exactly the second. - let settings: any; + let settings: StorageSettingsSlot | undefined; try { - settings = ctx.getService('settings'); + settings = ctx.getService('settings'); } catch { // A DECLARED absence, and silence is correct here: a bare kernel // registers no `settings` service, so nothing ever claimed the admin @@ -630,6 +630,28 @@ export class StorageServicePlugin implements Plugin { } } +/** + * [#12981] The slice of the `settings` service `start()` uses. + * + * Named rather than erased to `any` because splitting the lookup from the + * declaration is exactly `check:slot-lookup`'s FOURTH erasure shape (#4251): + * `let x: any; try { x = ctx.getService('…'); }` erases the contract in a + * position none of the rule's other selectors reach. Everything past + * `createClient` is feature-detected at its call site, so it is declared + * OPTIONAL here rather than assumed — the declaration says what the code + * actually requires, which is the property that makes it worth writing. + */ +interface StorageSettingsSlot { + createClient(namespace: string): unknown; + getNamespace(namespace: string): Promise<{ values: Record }>; + subscribe?(namespace: string, listener: () => void): void; + registerAction?( + namespace: string, + id: string, + handler: (input: any) => Promise, + ): void; +} + /** * Look up the host's MetricsRegistry from the service registry, with * the canonical fallback chain (explicit override → registered service diff --git a/scripts/slot-lookup-baseline.json b/scripts/slot-lookup-baseline.json index 1c5c897a2c..bdca43723c 100644 --- a/scripts/slot-lookup-baseline.json +++ b/scripts/slot-lookup-baseline.json @@ -20,7 +20,7 @@ "packages/services/service-realtime/src/realtime-service-plugin.ts": 1, "packages/services/service-settings/src/settings-service-plugin.ts": 2, "packages/services/service-sms/src/sms-plugin.ts": 1, - "packages/services/service-storage/src/storage-service-plugin.ts": 6, + "packages/services/service-storage/src/storage-service-plugin.ts": 5, "packages/triggers/trigger-record-change/src/formula-context.test.ts": 2, "packages/triggers/trigger-record-change/src/multilookup-context.test.ts": 2, "packages/triggers/trigger-record-change/src/record-change-integration.test.ts": 11