From 768963eb2c29138420079f6b3b83237fc66776fc Mon Sep 17 00:00:00 2001 From: Vladimir Rogojin Date: Sat, 30 May 2026 14:19:44 +0200 Subject: [PATCH] fix(uxf)(audit-333-h3): surface mergePkg per-token skips; add strict mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before this fix, `UxfPackage.merge` (→ `mergePkg`) silently dropped any tokenId whose per-token resolver threw. A single malformed sibling element inside a Rule 4 synthetic-rebuild made a legitimately-received token vanish from the merged manifest with only a `logger.warn`. On the receive path this manifested as token loss from view: the caller had no machine-readable signal that anything had gone wrong. Fix: - `UxfPackage.merge()` now returns `{ skipped: MergeSkip[] }`. Each `MergeSkip` records the tokenId, the original error, the target's prior root (if any), and the source's incoming root that failed to incorporate. Callers can react: replay, retry, surface to UI, or quarantine. - `opts.strict: true` aggregates skipped tokens into a `UxfError('MERGE_PARTIAL_FAILURE')` thrown BEFORE the atomic apply phase — target state is unchanged on the throw. Use on receive- path JOINs where any silent drop is unacceptable; leave default-off for opportunistic peer JOINs where partial coverage is fine. - `opts.onSkip` fires once per skipped tokenId for callers that want telemetry visibility without strict-mode failure. Throws inside onSkip are caught and logged — observability must not change merge semantics. - New error code `MERGE_PARTIAL_FAILURE` on UxfErrorCode. The thrown error also carries a non-typed `skipped` field with the structured skip list so callers needing machine-readable details can read it via `(err as { skipped }).skipped`. - Back-compat at the internal `mergePkg` boundary: legacy positional `(target, source, verifiedProofs)` callers keep working via a Set-vs-opts shape probe. Public callers (4 in tree: PaymentsModule, profile-token-storage-provider, profile/consolidation, flush-scheduler) ignore the returned `{ skipped }` and continue to use merge as void — behavior unchanged for them. Tests: 8 new H3 regression tests covering empty-skipped baseline, captured-skip on resolver throw, MergeSkip metadata fields, strict-mode hard fail, strict-mode atomicity invariant (target unchanged on throw), strict-mode happy path, onSkip callback fires once per skip, and onSkip throw does not change merge semantics. All 451 existing uxf tests pass unchanged. 8163 total unit tests pass. Refs: #333 (H3). Stacked on #351 (H2) → #349 (H1) → #348 (C3) → #347 (C2) → #346 (C1). --- .../uxf/h3-mergepkg-skipped-tokens.test.ts | 296 ++++++++++++++++++ uxf/UxfPackage.ts | 160 +++++++++- uxf/errors.ts | 10 +- 3 files changed, 452 insertions(+), 14 deletions(-) create mode 100644 tests/unit/uxf/h3-mergepkg-skipped-tokens.test.ts diff --git a/tests/unit/uxf/h3-mergepkg-skipped-tokens.test.ts b/tests/unit/uxf/h3-mergepkg-skipped-tokens.test.ts new file mode 100644 index 00000000..5d3d79ac --- /dev/null +++ b/tests/unit/uxf/h3-mergepkg-skipped-tokens.test.ts @@ -0,0 +1,296 @@ +/** + * Tests for Audit #333 H3 — UxfPackage.mergePkg silent-drop fix. + * + * Background + * ---------- + * Before this fix, a per-token resolver throw inside `mergePkg` + * (e.g., `computeElementHash` rejecting a malformed child during a + * Rule 4 synthetic rebuild) was silently dropped with only a + * `logger.warn`. The affected tokenId vanished from the merged + * manifest with no observable signal to the caller. On the receive + * path this manifested as token loss from view: a legitimately- + * received token whose sibling element was malformed disappeared + * entirely instead of being flagged. + * + * Fix + * --- + * - `UxfPackage.merge()` now returns `{ skipped: MergeSkip[] }`. + * Each `MergeSkip` records the tokenId, error, target's prior + * root (if any), and the source's incoming root that we failed + * to incorporate. + * - `opts.strict: true` aggregates skipped tokens into a + * `UxfError('MERGE_PARTIAL_FAILURE')` thrown BEFORE the atomic + * apply phase — target state is unchanged on the throw. + * - `opts.onSkip` fires once per skipped tokenId for callers that + * want telemetry visibility without strict-mode failure. + * - Back-compat: callers passing `verifiedProofs` directly as the + * positional third arg of internal mergePkg still work; the + * public `.merge()` API was always opts-bag-based. + * + * These tests use vi.mock to control `resolveTokenRoot`'s behavior + * so the contract is exercised without depending on the natural + * trigger conditions (which require constructing a malformed Rule 4 + * synthetic rebuild scenario). + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { UxfPackage } from '../../../uxf/UxfPackage.js'; +import { UxfError } from '../../../uxf/errors.js'; +import { + TOKEN_A, + TOKEN_B, + TOKEN_C, +} from '../../fixtures/uxf-mock-tokens.js'; +import * as tokenJoin from '../../../uxf/token-join.js'; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +function tokenId(token: Record): string { + return ( + (token.genesis as { data: { tokenId: string } }).data.tokenId + ).toLowerCase(); +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +describe('Audit #333 H3 — mergePkg surfaces per-token skips', () => { + let resolveSpy: ReturnType; + + beforeEach(() => { + resolveSpy = vi.spyOn(tokenJoin, 'resolveTokenRoot'); + }); + + afterEach(() => { + resolveSpy.mockRestore(); + }); + + describe('default (non-strict) mode', () => { + it('returns an empty `skipped` array when every resolver succeeds', () => { + const pkg1 = UxfPackage.create(); + pkg1.ingest(TOKEN_A); + + const pkg2 = UxfPackage.create(); + pkg2.ingest(TOKEN_B); + + const result = pkg1.merge(pkg2); + expect(result.skipped).toEqual([]); + // Sanity: both tokens landed in the merged manifest. + expect(pkg1.hasToken(tokenId(TOKEN_A))).toBe(true); + expect(pkg1.hasToken(tokenId(TOKEN_B))).toBe(true); + }); + + it('captures a per-token resolver throw in `skipped` (was silently dropped pre-fix)', () => { + // Build two packages that BOTH carry TOKEN_A. Tamper pkg2's + // manifest entry for TOKEN_A so it points to a different (but + // syntactically valid) rootHash — this forces the resolver to + // fire on the divergent pair instead of taking the + // `existingRoot === incomingRoot` fast path. Then force the + // resolver to throw via vi.spyOn. + const pkg1 = UxfPackage.create(); + pkg1.ingest(TOKEN_A); + + const pkg2 = UxfPackage.create(); + pkg2.ingest(TOKEN_A); + pkg2.ingest(TOKEN_C); + + const targetTokenId = tokenId(TOKEN_A); + // Tamper pkg2 to force a divergent manifest entry for TOKEN_A. + (pkg2.packageData.manifest.tokens as Map).set( + targetTokenId, + '55'.repeat(32), + ); + + resolveSpy.mockImplementation((params: { tokenId: string }) => { + if (params.tokenId === targetTokenId) { + throw new Error('synthetic resolver fault'); + } + throw new Error( + `test bug: unexpected resolver call for tokenId=${params.tokenId}`, + ); + }); + + const result = pkg1.merge(pkg2); + expect(result.skipped).toHaveLength(1); + expect(result.skipped[0].tokenId).toBe(targetTokenId); + expect(result.skipped[0].error.message).toBe('synthetic resolver fault'); + // Pre-fix the affected token vanished; now it's preserved at the + // target's PRIOR root (we DID NOT incorporate the incoming root + // because we couldn't resolve, but we also did NOT drop the + // entry we already had). + expect(pkg1.hasToken(targetTokenId)).toBe(true); + // TOKEN_C (disjoint tokenId) successfully merged. + expect(pkg1.hasToken(tokenId(TOKEN_C))).toBe(true); + }); + + it('records the target-prior and source-incoming hashes in MergeSkip', () => { + const pkg1 = UxfPackage.create(); + pkg1.ingest(TOKEN_A); + const pkg2 = UxfPackage.create(); + pkg2.ingest(TOKEN_A); + + // Force divergence: tamper pkg2's manifest entry for TOKEN_A so + // the resolver fires. Easiest: just inject a different rootHash. + // We do this by directly mutating pkg2's manifest map post- + // ingest. The pool stays valid; only the manifest pointer is + // changed. + const tokenAId = tokenId(TOKEN_A); + const data = pkg2.packageData; + const realRoot = data.manifest.tokens.get(tokenAId)!; + const fakeRoot = ('00'.repeat(32)) as string; + (data.manifest.tokens as Map).set(tokenAId, fakeRoot); + + resolveSpy.mockImplementation((params: { tokenId: string }) => { + if (params.tokenId === tokenAId) { + throw new Error('forced fault'); + } + throw new Error(`unexpected tokenId=${params.tokenId}`); + }); + + const result = pkg1.merge(pkg2); + expect(result.skipped).toHaveLength(1); + const skip = result.skipped[0]; + expect(skip.tokenId).toBe(tokenAId); + expect(skip.sourceIncoming).toBe(fakeRoot); + expect(skip.targetExisting).toBe(realRoot); + }); + }); + + describe('strict mode', () => { + it('throws UxfError(MERGE_PARTIAL_FAILURE) when any per-token resolver throws', () => { + const pkg1 = UxfPackage.create(); + pkg1.ingest(TOKEN_A); + const pkg2 = UxfPackage.create(); + pkg2.ingest(TOKEN_A); + + const tokenAId = tokenId(TOKEN_A); + // Tamper to force divergence. + const data = pkg2.packageData; + (data.manifest.tokens as Map).set( + tokenAId, + '11'.repeat(32), + ); + + resolveSpy.mockImplementation(() => { + throw new Error('forced strict-mode fault'); + }); + + let thrown: unknown = null; + try { + pkg1.merge(pkg2, { strict: true }); + } catch (e) { + thrown = e; + } + expect(thrown).toBeInstanceOf(UxfError); + expect((thrown as UxfError).code).toBe('MERGE_PARTIAL_FAILURE'); + // Structured skip list is attached to the error for caller use. + const skipped = (thrown as unknown as { + skipped: Array<{ tokenId: string; error: Error }>; + }).skipped; + expect(skipped).toHaveLength(1); + expect(skipped[0].tokenId).toBe(tokenAId); + }); + + it('leaves target unchanged on strict-mode throw (atomic-failure invariant)', () => { + const pkg1 = UxfPackage.create(); + pkg1.ingest(TOKEN_A); + pkg1.ingest(TOKEN_B); + const tokenAId = tokenId(TOKEN_A); + const tokenBId = tokenId(TOKEN_B); + const tokenCId = tokenId(TOKEN_C); + const pkg2 = UxfPackage.create(); + pkg2.ingest(TOKEN_A); + pkg2.ingest(TOKEN_C); + + // Force divergence on TOKEN_A so the resolver fires; the strict + // throw should prevent TOKEN_C from landing. + (pkg2.packageData.manifest.tokens as Map).set( + tokenAId, + '22'.repeat(32), + ); + + resolveSpy.mockImplementation((params: { tokenId: string }) => { + if (params.tokenId === tokenAId) { + throw new Error('strict-mode atomicity probe'); + } + throw new Error(`unexpected tokenId=${params.tokenId}`); + }); + + expect(() => pkg1.merge(pkg2, { strict: true })).toThrow(UxfError); + + // pkg1 still has TOKEN_A + TOKEN_B from its original ingest; it + // did NOT acquire TOKEN_C because strict mode aborted before the + // atomic apply phase. + expect(pkg1.hasToken(tokenAId)).toBe(true); + expect(pkg1.hasToken(tokenBId)).toBe(true); + expect(pkg1.hasToken(tokenCId)).toBe(false); + }); + + it('does NOT throw under strict mode when no resolver fails', () => { + const pkg1 = UxfPackage.create(); + pkg1.ingest(TOKEN_A); + const pkg2 = UxfPackage.create(); + pkg2.ingest(TOKEN_B); + // Disjoint tokenIds — no resolver call. + + const result = pkg1.merge(pkg2, { strict: true }); + expect(result.skipped).toEqual([]); + }); + }); + + describe('onSkip callback', () => { + it('fires once per skipped tokenId', () => { + const pkg1 = UxfPackage.create(); + pkg1.ingest(TOKEN_A); + const pkg2 = UxfPackage.create(); + pkg2.ingest(TOKEN_A); + + const tokenAId = tokenId(TOKEN_A); + (pkg2.packageData.manifest.tokens as Map).set( + tokenAId, + '33'.repeat(32), + ); + + resolveSpy.mockImplementation(() => { + throw new Error('callback test fault'); + }); + + const observed: Array<{ tokenId: string; error: Error }> = []; + pkg1.merge(pkg2, { + onSkip: (event) => observed.push(event), + }); + + expect(observed).toHaveLength(1); + expect(observed[0].tokenId).toBe(tokenAId); + expect(observed[0].error.message).toBe('callback test fault'); + }); + + it('does NOT change merge semantics when onSkip itself throws', () => { + const pkg1 = UxfPackage.create(); + pkg1.ingest(TOKEN_A); + const pkg2 = UxfPackage.create(); + pkg2.ingest(TOKEN_A); + + const tokenAId = tokenId(TOKEN_A); + (pkg2.packageData.manifest.tokens as Map).set( + tokenAId, + '44'.repeat(32), + ); + + resolveSpy.mockImplementation(() => { + throw new Error('callback-throws-during-merge'); + }); + + const result = pkg1.merge(pkg2, { + onSkip: () => { + throw new Error('observability-side fault'); + }, + }); + // Merge still completed (no strict mode), skipped is reported. + expect(result.skipped).toHaveLength(1); + }); + }); +}); diff --git a/uxf/UxfPackage.ts b/uxf/UxfPackage.ts index 12125a6b..72e29899 100644 --- a/uxf/UxfPackage.ts +++ b/uxf/UxfPackage.ts @@ -249,9 +249,41 @@ export class UxfPackage { * omitted, falls back to the conservative pre-G.3 `divergent` * resolution for any pairwise hash mismatch. */ - merge(other: UxfPackage, opts?: { verifiedProofs?: ReadonlySet }): this { - mergePkg(this.data, other.data, opts?.verifiedProofs); - return this; + merge( + other: UxfPackage, + opts?: { + readonly verifiedProofs?: ReadonlySet; + /** + * Audit #333 H3 — strict mode. When `true`, mergePkg throws a + * `UxfError('MERGE_PARTIAL_FAILURE')` summarising every per-token + * resolver failure instead of silently dropping the affected + * tokens. The default (`false`) preserves the existing "good + * tokens survive; bad ones are skipped" contract but now + * returns the skipped set so the caller can react. + * + * Use `strict: true` on the recipient receive path where any + * silent drop is observable as token loss; leave default-off + * for opportunistic peer JOIN merges where partial coverage + * is acceptable. + */ + readonly strict?: boolean; + /** + * Audit #333 H3 — per-token error callback. Fires once per + * skipped tokenId BEFORE strict-mode aggregation. Useful for + * telemetry / operator visibility surfaces that want each + * failure surfaced individually. + */ + readonly onSkip?: (event: { + readonly tokenId: string; + readonly error: Error; + }) => void; + }, + ): { readonly skipped: ReadonlyArray } { + return mergePkg(this.data, other.data, { + verifiedProofs: opts?.verifiedProofs, + strict: opts?.strict, + onSkip: opts?.onSkip, + }); } /** @@ -725,11 +757,59 @@ export function removeToken(pkg: UxfPackageData, tokenId: string): void { * compatibility partition and pick the majority class. Leave the * refactor — just documenting. */ +/** + * Audit #333 H3 — per-token merge skip record. + * + * One entry per source tokenId whose resolver threw during + * {@link mergePkg}. The token is NOT present in the merged manifest; + * `targetExisting` records what the target already had (if anything) + * so the caller can decide whether the skip is recoverable (e.g., the + * target's existing root is still good). + */ +export interface MergeSkip { + readonly tokenId: string; + readonly error: Error; + /** target.manifest.tokens.get(tokenId) BEFORE the merge attempt. */ + readonly targetExisting: ContentHash | undefined; + /** source.manifest.tokens.get(tokenId) that we failed to incorporate. */ + readonly sourceIncoming: ContentHash; +} + +/** Internal merge options bag — surfaced through {@link UxfPackage.merge}. */ +interface MergePkgOpts { + readonly verifiedProofs?: ReadonlySet; + readonly strict?: boolean; + readonly onSkip?: (event: { + readonly tokenId: string; + readonly error: Error; + }) => void; +} + +interface MergePkgResult { + readonly skipped: ReadonlyArray; +} + function mergePkg( target: UxfPackageData, source: UxfPackageData, - verifiedProofs?: ReadonlySet, -): void { + opts?: MergePkgOpts | ReadonlySet, +): MergePkgResult { + // Back-compat: legacy callers passed `verifiedProofs` directly as the + // third positional argument. Normalise both shapes into the opts bag. + // (ReadonlySet does not narrow through `instanceof Set` cleanly under + // strict-mode TS — fall through an explicit cast.) + let normalisedOpts: MergePkgOpts; + if (opts === undefined) { + normalisedOpts = {}; + } else if (opts instanceof Set) { + normalisedOpts = { verifiedProofs: opts as unknown as ReadonlySet }; + } else { + normalisedOpts = opts as MergePkgOpts; + } + const verifiedProofs = normalisedOpts.verifiedProofs; + const strict = normalisedOpts.strict === true; + const onSkip = normalisedOpts.onSkip; + const mutablePool = target.pool as Map; const mutableManifest = target.manifest.tokens as Map; @@ -770,6 +850,13 @@ function mergePkg( const stagedManifestWrites = new Map(); const stagedSyntheticInserts = new Map(); + // Audit #333 H3 — collect per-token failures so the caller can + // observe them. Pre-fix the only signal was a logger.warn, and the + // failed tokens silently vanished from the merged manifest. Now the + // result carries an explicit `skipped` array; strict mode aggregates + // them into a hard throw. + const skipped: MergeSkip[] = []; + for (const [tokenId, incomingRoot] of source.manifest.tokens) { try { const existingRoot = mutableManifest.get(tokenId); @@ -798,20 +885,64 @@ function mergePkg( } stagedManifestWrites.set(tokenId, outcome.rootHash); } catch (err) { - // One poisoned tokenId must not abort the whole merge. - // Skip this entry, log it for telemetry / operator review, - // and continue. Target state for this tokenId stays at its - // pre-merge value (whatever `mutableManifest.get(tokenId)` - // was — possibly undefined, i.e. the entry is simply - // absent from the merged manifest). - const message = err instanceof Error ? err.message : String(err); + // Audit #333 H3: a per-token resolver failure used to vanish + // with only a logger.warn — token loss from the receive path's + // point of view. Now we ALSO record the skip in the result so + // the caller can react (telemetry, retry, operator surface, + // or hard-fail in strict mode). + const error = err instanceof Error ? err : new Error(String(err)); logger.warn( 'UxfPackage', - `mergePkg: skipping tokenId ${tokenId} — resolver threw: ${message}`, + `mergePkg: skipping tokenId ${tokenId} — resolver threw: ${error.message}`, ); + const existingRoot = mutableManifest.get(tokenId); + const skipRecord: MergeSkip = { + tokenId, + error, + targetExisting: existingRoot, + sourceIncoming: incomingRoot, + }; + skipped.push(skipRecord); + if (onSkip) { + try { + onSkip({ tokenId, error }); + } catch (cbErr) { + // Best-effort: onSkip is observability. A callback throw + // must not change merge semantics, so we swallow and log. + logger.warn( + 'UxfPackage', + `mergePkg: onSkip callback threw for tokenId=${tokenId} (ignored): ` + + `${cbErr instanceof Error ? cbErr.message : String(cbErr)}`, + ); + } + } } } + // Audit #333 H3 — strict-mode aggregation. + // + // BEFORE the atomic apply phase so target state is untouched on the + // throw. The aggregated error preserves each per-token cause for + // operator triage. + if (strict && skipped.length > 0) { + const summary = skipped + .slice(0, 5) + .map((s) => `${s.tokenId.slice(0, 16)}…: ${s.error.message}`) + .join('; '); + const suffix = skipped.length > 5 ? ` (and ${skipped.length - 5} more)` : ''; + const aggregate = new UxfError( + 'MERGE_PARTIAL_FAILURE', + `mergePkg(strict): ${skipped.length} per-token resolver failure(s); ` + + `target unchanged. ${summary}${suffix}`, + ); + // Stash the structured skip list on the error for callers that + // want machine-readable details. We extend the error rather than + // bloating UxfError's type so the public type stays stable. + (aggregate as unknown as { skipped: ReadonlyArray }).skipped = + skipped; + throw aggregate; + } + // ---- Phase 3: atomic apply ---- // // Synchronous Map.set calls. No I/O, no throws in this block — @@ -848,6 +979,9 @@ function mergePkg( // Update envelope timestamp (target.envelope as { updatedAt: number }).updatedAt = Math.floor(Date.now() / 1000); + + // Audit #333 H3 — surface the per-token skip list to the caller. + return { skipped }; } /** diff --git a/uxf/errors.ts b/uxf/errors.ts index 23d08afb..cbcbefaf 100644 --- a/uxf/errors.ts +++ b/uxf/errors.ts @@ -15,7 +15,15 @@ export type UxfErrorCode = | 'INVALID_PACKAGE' | 'INVALID_INPUT' | 'LIMIT_EXCEEDED' - | 'NOT_IMPLEMENTED'; + | 'NOT_IMPLEMENTED' + /** + * Audit #333 H3 — surfaced by `UxfPackage.merge({ strict: true })` + * when one or more per-token resolvers throw. Pre-fix the failures + * silently disappeared with only a `logger.warn`. The error + * carries a `skipped: MergeSkip[]` field (machine-readable) so + * callers can decide how to react. + */ + | 'MERGE_PARTIAL_FAILURE'; /** * Structured error for all UXF operations.