From 9b4adef9bbb205ef27c161724dd2fe863254ffd2 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 5 Oct 2026 11:32:46 +0000 Subject: [PATCH 1/2] fix(core): validation errors name what was validated Every StackValidationError opened with "Content validation failed", including refusals of arguments that have nothing to do with content (migrateAll(TypeId), grantType(TypeId), a versioned filter.baseId, an empty associate([]), a duplicate in amendAssociations()). StackValidationError now takes its header. Content keeps "Content validation failed"; defineType()'s schema checks read "Schema validation failed"; argument checks read "Invalid arguments". create() and mutate() report option errors ahead of content errors so each refusal carries one subject. associate()/dissociate() and grantAccess()/revokeAccess() check their list before wrapping it as edits, so errors name `associations` or `permissions` rather than the `changes` of the verb they delegate to. deserializeError() keeps the header a server sent. Closes #406 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01B7z8BEjS2hwDbLyZTaopo4 --- .changeset/validation-error-headers.md | 7 + packages/conformance-fixtures/src/index.ts | 8 +- packages/core/src/errors.ts | 18 ++- packages/core/src/query-validation.ts | 42 ++++- packages/core/src/record-id.ts | 17 +- packages/core/src/scoped-stack.ts | 5 + packages/core/src/stack.ts | 80 ++++++---- packages/core/src/wire-body.ts | 6 +- packages/core/tests/element-keys.test.ts | 8 +- .../core/tests/validation-headers.test.ts | 149 ++++++++++++++++++ packages/wire-types/src/index.ts | 4 +- packages/wire-types/tests/errors.test.ts | 6 + 12 files changed, 291 insertions(+), 59 deletions(-) create mode 100644 .changeset/validation-error-headers.md create mode 100644 packages/core/tests/validation-headers.test.ts diff --git a/.changeset/validation-error-headers.md b/.changeset/validation-error-headers.md new file mode 100644 index 00000000..1bbe0b50 --- /dev/null +++ b/.changeset/validation-error-headers.md @@ -0,0 +1,7 @@ +--- +'@haverstack/core': patch +'@haverstack/wire-types': patch +'@haverstack/conformance-fixtures': patch +--- + +`StackValidationError`'s message header names what was validated: record content keeps "Content validation failed", a type schema reads "Schema validation failed", and argument checks read "Invalid arguments". `associate()`/`dissociate()` and `grantAccess()`/`revokeAccess()` report list errors under `associations`/`permissions`, the parameter the caller passed, rather than `changes`. `deserializeError()` keeps the header a server sent. diff --git a/packages/conformance-fixtures/src/index.ts b/packages/conformance-fixtures/src/index.ts index c93d0d1f..cc4013e6 100644 --- a/packages/conformance-fixtures/src/index.ts +++ b/packages/conformance-fixtures/src/index.ts @@ -2243,7 +2243,7 @@ export const installRequestFixtures: ConformanceFixture< responseBody: { error: { code: 'validation', - message: 'Content validation failed', + message: 'Invalid arguments', details: [ { path: 'types[0].id', @@ -2719,7 +2719,7 @@ export const errorResponseFixtures: ConformanceFixture[] = [ responseBody: { error: { code: 'validation', - message: 'Content validation failed', + message: 'Invalid arguments', details: [ { path: 'permissions[0]', @@ -2755,7 +2755,7 @@ export const errorResponseFixtures: ConformanceFixture[] = [ responseBody: { error: { code: 'validation', - message: 'Content validation failed', + message: 'Invalid arguments', details: [ { path: 'permissions[0]', @@ -3156,7 +3156,7 @@ export const errorResponseFixtures: ConformanceFixture[] = [ responseBody: { error: { code: 'validation', - message: 'Content validation failed', + message: 'Invalid arguments', details: [ { path: 'changes[0].association.label', diff --git a/packages/core/src/errors.ts b/packages/core/src/errors.ts index c006ba6b..57747571 100644 --- a/packages/core/src/errors.ts +++ b/packages/core/src/errors.ts @@ -59,13 +59,23 @@ export abstract class StackError extends Error { abstract readonly code: StackErrorCode; } +export const CONTENT_INVALID = 'Content validation failed'; +export const ARGUMENTS_INVALID = 'Invalid arguments'; +export const SCHEMA_INVALID = 'Schema validation failed'; + +/** + * `header` names what was validated — record content, or the arguments a + * call was given — so a refusal of `associate([])` doesn't read as a + * content failure. Only the message varies: `code` is the contract. + */ export class StackValidationError extends StackError { static readonly code = 'validation' as const; override readonly code = StackValidationError.code; - constructor(public readonly errors: ValidationError[]) { - super( - `Content validation failed:\n` + errors.map((e) => ` ${e.path}: ${e.message}`).join('\n'), - ); + constructor( + public readonly errors: ValidationError[], + header: string = CONTENT_INVALID, + ) { + super(`${header}:\n` + errors.map((e) => ` ${e.path}: ${e.message}`).join('\n')); this.name = 'StackValidationError'; } } diff --git a/packages/core/src/query-validation.ts b/packages/core/src/query-validation.ts index b9ee3ea7..e3cba413 100644 --- a/packages/core/src/query-validation.ts +++ b/packages/core/src/query-validation.ts @@ -17,7 +17,7 @@ * See docs/spec/data-model.md § Capability-gated filters. */ -import { StackBadRequestError, StackValidationError } from './errors.js'; +import { ARGUMENTS_INVALID, StackBadRequestError, StackValidationError } from './errors.js'; import { familyIdProblem } from './schema.js'; import { associationEqual, isAuthorityAssociation } from './record-changes.js'; import { CONTENT_SEGMENT_METACHARACTERS, SEGMENT_METACHARACTER_RE } from './validate.js'; @@ -525,9 +525,10 @@ export function assertAssociationEdits( half: 'data' | 'authority', ): asserts changes is AssociationEdit[] { if (!Array.isArray(changes) || changes.length === 0) { - throw new StackValidationError([ - { path: 'changes', message: `${surface} names at least one change.` }, - ]); + throw new StackValidationError( + [{ path: 'changes', message: `${surface} names at least one change.` }], + ARGUMENTS_INVALID, + ); } const errors: ValidationError[] = []; changes.forEach((raw: unknown, i) => { @@ -550,7 +551,7 @@ export function assertAssociationEdits( } errors.push(...validateAssociation(edit.association as Association, `${path}.association`)); }); - if (errors.length > 0) throw new StackValidationError(errors); + if (errors.length > 0) throw new StackValidationError(errors, ARGUMENTS_INVALID); const associations = (changes as AssociationEdit[]).map((c) => c.association); if (half === 'data') assertDataAssociations(associations, surface); @@ -565,7 +566,34 @@ export function assertAssociationEdits( }); } }); - if (duplicates.length > 0) throw new StackValidationError(duplicates); + if (duplicates.length > 0) throw new StackValidationError(duplicates, ARGUMENTS_INVALID); +} + +/** + * assertAssociationEdits() for the verbs taking a bare association list — + * associate(), grantAccess() and their inverses — asked before the list is + * wrapped as edits, so each problem is reported under `param`, the name the + * caller passed it as, rather than the wrapped list's `changes`. + */ +export function assertAssociationList( + associations: unknown, + surface: string, + param: string, +): asserts associations is Association[] { + if (!Array.isArray(associations) || associations.length === 0) { + throw new StackValidationError( + [{ path: param, message: `${surface} names at least one association.` }], + ARGUMENTS_INVALID, + ); + } + const errors = associations.flatMap((raw: unknown, i) => + typeof raw === 'object' && raw !== null && !Array.isArray(raw) + ? [] + : [{ path: `${param}[${i}]`, message: `${param}[${i}] must be an object.` }], + ); + if (errors.length > 0) throw new StackValidationError(errors, ARGUMENTS_INVALID); + const listErrors = validateAssociations(associations as Association[], param); + if (listErrors.length > 0) throw new StackValidationError(listErrors, ARGUMENTS_INVALID); } /** @@ -607,7 +635,7 @@ export function assertValidBaseIdFilter(filter: { baseId?: string | string[] } | .map((b) => familyIdProblem(b, 'filter.baseId')) .filter((m): m is string => m !== null) .map((message) => ({ path: 'filter.baseId', message })); - if (errors.length > 0) throw new StackValidationError(errors); + if (errors.length > 0) throw new StackValidationError(errors, ARGUMENTS_INVALID); } /** diff --git a/packages/core/src/record-id.ts b/packages/core/src/record-id.ts index 35d22034..ede66333 100644 --- a/packages/core/src/record-id.ts +++ b/packages/core/src/record-id.ts @@ -10,7 +10,7 @@ */ import { isValidIdFormat, idTimestamp, MAX_ID_TIMESTAMP } from './id.js'; -import { StackBadRequestError, StackValidationError } from './errors.js'; +import { ARGUMENTS_INVALID, StackBadRequestError, StackValidationError } from './errors.js'; import type { ValidationError } from './validate.js'; // ------------------------------------------------------- @@ -134,11 +134,14 @@ export function validateIdTimestampSkew( if (toleranceMs === null) return; const skew = Math.abs(referenceMs - idTimestamp(id)); if (skew > toleranceMs) { - throw new StackValidationError([ - { - path: 'id', - message: `ID "${id}" timestamp disagrees with ${referenceLabel} by more than the allowed clock-skew tolerance (${toleranceMs}ms).`, - }, - ]); + throw new StackValidationError( + [ + { + path: 'id', + message: `ID "${id}" timestamp disagrees with ${referenceLabel} by more than the allowed clock-skew tolerance (${toleranceMs}ms).`, + }, + ], + ARGUMENTS_INVALID, + ); } } diff --git a/packages/core/src/scoped-stack.ts b/packages/core/src/scoped-stack.ts index 96e18406..2724e4b4 100644 --- a/packages/core/src/scoped-stack.ts +++ b/packages/core/src/scoped-stack.ts @@ -74,6 +74,7 @@ import { } from './errors.js'; import { assertAssociationEdits, + assertAssociationList, assertAuthorityAssociations, assertDataAssociations, assertSortCapability, @@ -1350,6 +1351,7 @@ export class ScopedStack implements StackClient { * read so it cannot depend on who is asking. */ async associate(id: RecordId, associations: DataAssociation[]): Promise { + assertAssociationList(associations, 'associate()', 'associations'); return this.amendAssociations( id, associations.map((association) => ({ op: 'add', association })), @@ -1359,6 +1361,7 @@ export class ScopedStack implements StackClient { /** See associate() — the same write gate, the same kind refusal. */ async dissociate(id: RecordId, associations: DataAssociation[]): Promise { + assertAssociationList(associations, 'dissociate()', 'associations'); return this.amendAssociations( id, associations.map((association) => ({ op: 'remove', association })), @@ -1394,6 +1397,7 @@ export class ScopedStack implements StackClient { * See docs/spec/access-control.md § Record-level permissions. */ async grantAccess(id: RecordId, permissions: AuthorityAssociation[]): Promise { + assertAssociationList(permissions, 'grantAccess()', 'permissions'); return this.amendAccess( id, permissions.map((association) => ({ op: 'add', association })), @@ -1403,6 +1407,7 @@ export class ScopedStack implements StackClient { /** Withdraw elements of who reaches a record — see grantAccess(). */ async revokeAccess(id: RecordId, permissions: AuthorityAssociation[]): Promise { + assertAssociationList(permissions, 'revokeAccess()', 'permissions'); return this.amendAccess( id, permissions.map((association) => ({ op: 'remove', association })), diff --git a/packages/core/src/stack.ts b/packages/core/src/stack.ts index e4dce78d..c562ebc6 100644 --- a/packages/core/src/stack.ts +++ b/packages/core/src/stack.ts @@ -95,6 +95,8 @@ import { StackSchemaDriftError, StackValidationError, StackVersionConflictError, + ARGUMENTS_INVALID, + SCHEMA_INVALID, } from './errors.js'; import { assertQueryCapabilities, @@ -106,6 +108,7 @@ import { assertValidSort, normalizeSort, assertAssociationEdits, + assertAssociationList, assertAuthorityAssociations, assertDataAssociations, filtersContent, @@ -778,7 +781,7 @@ export class Stack implements StackClient { // JSON that no compiler has seen. const shapeErrors = validateSchemaShape(schema); if (shapeErrors.length > 0) { - throw new StackValidationError(shapeErrors); + throw new StackValidationError(shapeErrors, SCHEMA_INVALID); } const nameErrors = [ @@ -786,7 +789,7 @@ export class Stack implements StackClient { ...validateSchemaFieldNames(schema), ]; if (nameErrors.length > 0) { - throw new StackValidationError(nameErrors); + throw new StackValidationError(nameErrors, SCHEMA_INVALID); } const schemaHash = await hashSchema(schema); @@ -922,7 +925,9 @@ export class Stack implements StackClient { ): Promise<{ migrated: number; skipped: number }> { this.assertOpen(); const problem = familyIdProblem(baseId, 'migrateAll'); - if (problem) throw new StackValidationError([{ path: 'baseId', message: problem }]); + if (problem) { + throw new StackValidationError([{ path: 'baseId', message: problem }], ARGUMENTS_INVALID); + } const types = await this.adapter.listTypes(); const familyTypeIds = types.filter((t) => t.baseId === baseId).map((t) => t.id); @@ -1033,13 +1038,7 @@ export class Stack implements StackClient { assertDataAssociations(opts.associations ?? [], 'associations'); assertAuthorityAssociations(opts.permissions ?? [], 'permissions'); - const errors = [ - ...validateReservedKeys(content), - ...validateContentKeys(content), - ...validateContent(content, type.schema), - ...validateGrantee(typeId, content), - ...validateGrantBaseId(typeId, content), - ...validateInstall(typeId, content), + const argumentErrors = [ ...validateAssociations(opts.permissions, 'permissions'), ...validatePermissions(opts.permissions), ...validateAssociations(opts.associations), @@ -1050,8 +1049,19 @@ export class Stack implements StackClient { // on its own is caught too: defaulted-createdAt is now, which a // backdated updatedAt alone would still precede. if (opts.updatedAt !== undefined && updatedAt.getTime() < createdAt.getTime()) { - errors.push({ path: 'updatedAt', message: 'updatedAt cannot precede createdAt.' }); + argumentErrors.push({ path: 'updatedAt', message: 'updatedAt cannot precede createdAt.' }); + } + if (argumentErrors.length > 0) { + throw new StackValidationError(argumentErrors, ARGUMENTS_INVALID); } + const errors = [ + ...validateReservedKeys(content), + ...validateContentKeys(content), + ...validateContent(content, type.schema), + ...validateGrantee(typeId, content), + ...validateGrantBaseId(typeId, content), + ...validateInstall(typeId, content), + ]; if (errors.length > 0) { throw new StackValidationError(errors); } @@ -1354,19 +1364,22 @@ export class Stack implements StackClient { if (associations) assertDataAssociations(associations, 'associations'); if (permissions) assertAuthorityAssociations(permissions, 'permissions'); - const errors = [ - ...(contentPatch - ? [ - ...validateReservedKeys(contentPatch), - ...validatePatchValues(contentPatch), - ...validateContentKeys(contentPatch), - ] - : []), + const argumentErrors = [ ...(permissions ? [...validateAssociations(permissions, 'permissions'), ...validatePermissions(permissions)] : []), ...(associations ? validateAssociations(associations) : []), ]; + if (argumentErrors.length > 0) { + throw new StackValidationError(argumentErrors, ARGUMENTS_INVALID); + } + const errors = contentPatch + ? [ + ...validateReservedKeys(contentPatch), + ...validatePatchValues(contentPatch), + ...validateContentKeys(contentPatch), + ] + : []; if (errors.length > 0) throw new StackValidationError(errors); await this.checkAttachmentAssociationPointers(associations, existing.associations); @@ -1462,6 +1475,7 @@ export class Stack implements StackClient { associations: DataAssociation[], opts: ActorOptions = {}, ): Promise { + assertAssociationList(associations, 'associate()', 'associations'); return this.amendAssociations( id, associations.map((association) => ({ op: 'add', association })), @@ -1482,6 +1496,7 @@ export class Stack implements StackClient { associations: DataAssociation[], opts: ActorOptions = {}, ): Promise { + assertAssociationList(associations, 'dissociate()', 'associations'); return this.amendAssociations( id, associations.map((association) => ({ op: 'remove', association })), @@ -1542,6 +1557,7 @@ export class Stack implements StackClient { permissions: AuthorityAssociation[], opts: ActorOptions = {}, ): Promise { + assertAssociationList(permissions, 'grantAccess()', 'permissions'); return this.amendAccess( id, permissions.map((association) => ({ op: 'add', association })), @@ -1560,6 +1576,7 @@ export class Stack implements StackClient { permissions: AuthorityAssociation[], opts: ActorOptions = {}, ): Promise { + assertAssociationList(permissions, 'revokeAccess()', 'permissions'); return this.amendAccess( id, permissions.map((association) => ({ op: 'remove', association })), @@ -1604,7 +1621,7 @@ export class Stack implements StackClient { */ private assertPermissionSet(next: AuthorityAssociation[]): void { const errors = validatePermissions(next); - if (errors.length > 0) throw new StackValidationError(errors); + if (errors.length > 0) throw new StackValidationError(errors, ARGUMENTS_INVALID); } /** @@ -2315,12 +2332,15 @@ export class Stack implements StackClient { // succeeds does confirm the record it names, but only to a caller who // already has file access for those bytes. // See the anti-oracle rule in docs/spec/attachments.md. - throw new StackValidationError([ - { - path: 'attachmentRecordId', - message: 'attachmentRecordId must name an `_attachment` record for this fileId', - }, - ]); + throw new StackValidationError( + [ + { + path: 'attachmentRecordId', + message: 'attachmentRecordId must name an `_attachment` record for this fileId', + }, + ], + ARGUMENTS_INVALID, + ); }); } @@ -2824,7 +2844,9 @@ export class Stack implements StackClient { this.assertOpen(); validateGrantTarget(grant.grantee); const problem = familyIdProblem(baseId, 'revokeType'); - if (problem) throw new StackValidationError([{ path: 'baseId', message: problem }]); + if (problem) { + throw new StackValidationError([{ path: 'baseId', message: problem }], ARGUMENTS_INVALID); + } const familyId = baseId; const actionSet = new Set(grant.actions); const all = await loadGrantRecords((q) => this.query(q)); @@ -3048,7 +3070,7 @@ export class Stack implements StackClient { } } }); - if (errors.length > 0) throw new StackValidationError(errors); + if (errors.length > 0) throw new StackValidationError(errors, ARGUMENTS_INVALID); for (const r of manifest.requests) this.checkGrantValid(r.baseId, r.actions); } @@ -3196,7 +3218,7 @@ export class Stack implements StackClient { }); }); if (errors.length > 0) { - throw new StackValidationError(errors); + throw new StackValidationError(errors, ARGUMENTS_INVALID); } } diff --git a/packages/core/src/wire-body.ts b/packages/core/src/wire-body.ts index f3ba89c2..d3d885ca 100644 --- a/packages/core/src/wire-body.ts +++ b/packages/core/src/wire-body.ts @@ -17,7 +17,7 @@ * See docs/spec/wire-format.md § Unrecognized input. */ -import { StackBadRequestError, StackValidationError } from './errors.js'; +import { ARGUMENTS_INVALID, StackBadRequestError, StackValidationError } from './errors.js'; import type { DefineTypeOptions } from './stack.js'; import { assertKnownKeys, validateAssociation } from './query-validation.js'; import type { AppManifest } from './install.js'; @@ -35,7 +35,7 @@ export function requireBody(body: unknown, label: string): Record 0) throw new StackValidationError(errors); + if (errors.length > 0) throw new StackValidationError(errors, ARGUMENTS_INVALID); return { op: edit.op, association } as AssociationEdit; }); } diff --git a/packages/core/tests/element-keys.test.ts b/packages/core/tests/element-keys.test.ts index e310e5fe..c169ffc6 100644 --- a/packages/core/tests/element-keys.test.ts +++ b/packages/core/tests/element-keys.test.ts @@ -39,7 +39,7 @@ describe('an association element carries only the keys its kind defines', () => new StackBadRequestError(message), ); await expect(stack.associate(recordId, [bad])).rejects.toThrow( - new StackBadRequestError('Unknown key in changes[0].association: scope'), + new StackBadRequestError(message), ); }); } @@ -58,7 +58,7 @@ describe('an association element carries only the keys its kind defines', () => target: { kind: 'entity', entityId: 'did:key:a', ns: 'atproto' }, } as DataAssociation; await expect(stack.associate(recordId, [a])).rejects.toThrow( - new StackBadRequestError('Unknown key in changes[0].association.target: ns'), + new StackBadRequestError('Unknown key in associations[0].target: ns'), ); }); @@ -80,7 +80,7 @@ describe('a permission element carries only the keys its kind and grantee define test('an unknown key on the grantee is refused', async () => { const p = read({ kind: 'entity', entityId: 'did:key:a', scope: 'all' }); await expect(stack.grantAccess(recordId, [p])).rejects.toThrow( - new StackBadRequestError('Unknown key in changes[0].association.grantee: scope'), + new StackBadRequestError('Unknown key in permissions[0].grantee: scope'), ); await expect(stack.mutate(recordId, { permissions: [p] })).rejects.toThrow( new StackBadRequestError('Unknown key in permissions[0].grantee: scope'), @@ -95,7 +95,7 @@ describe('a permission element carries only the keys its kind and grantee define test('an unknown key on the element itself is refused', async () => { const anyone = extra({ kind: 'anyone', label: 'read' } as AuthorityAssociation, 'until'); await expect(stack.grantAccess(recordId, [anyone])).rejects.toThrow( - new StackBadRequestError('Unknown key in changes[0].association: until'), + new StackBadRequestError('Unknown key in permissions[0]: until'), ); }); }); diff --git a/packages/core/tests/validation-headers.test.ts b/packages/core/tests/validation-headers.test.ts new file mode 100644 index 00000000..5c943091 --- /dev/null +++ b/packages/core/tests/validation-headers.test.ts @@ -0,0 +1,149 @@ +import { describe, test, expect, beforeEach } from 'vitest'; +import { Stack } from '../src/stack.js'; +import type { StackClient } from '../src/stack.js'; +import { StackValidationError } from '../src/errors.js'; +import { MemoryAdapter } from '../src/testing.js'; +import type { DataAssociation, TypeGrant } from '../src/types.js'; + +const NOTE = 'com.example.test/note@1'; +const OWNER = 'did:key:owner'; +const TAG: DataAssociation = { kind: 'tag', label: 'starred' }; +const GRANT: TypeGrant = { + grantee: { kind: 'entity', entityId: 'did:key:a' }, + actions: ['read-any'], +}; + +let stack: Stack; +let recordId: string; + +beforeEach(async () => { + stack = await Stack.open(await MemoryAdapter.open({ ownerEntityId: OWNER, timezone: 'UTC' })); + await stack.defineType({ id: NOTE, name: 'Note', schema: { text: { kind: 'text' } } }); + recordId = (await stack.create(NOTE, { text: 'x' })).id; +}); + +/** The refusal's header — its message's first line — and the path of each line under it. */ +const refusal = async (p: Promise): Promise<{ header: string; paths: string[] }> => { + const err = await p.then( + () => { + throw new Error('expected a refusal'); + }, + (e: unknown) => e, + ); + expect(err).toBeInstanceOf(StackValidationError); + const { message, errors } = err as StackValidationError; + return { header: message.split('\n')[0]!, paths: errors.map((e) => e.path) }; +}; + +describe('a content refusal says content was validated', () => { + test('create() with content the schema refuses', async () => { + expect(await refusal(stack.create(NOTE, { text: 1 }))).toEqual({ + header: 'Content validation failed:', + paths: ['text'], + }); + }); + + test('mutate() with a patch the schema refuses', async () => { + expect(await refusal(stack.mutate(recordId, { contentPatch: { text: 1 } }))).toEqual({ + header: 'Content validation failed:', + paths: ['text'], + }); + }); +}); + +describe('an argument refusal says the arguments were invalid, under their own names', () => { + test('defineType() names the schema', async () => { + const bad = { id: 'com.example.test/bad@1', name: 'Bad', schema: { x: { kind: 'nope' } } }; + expect((await refusal(stack.defineType(bad as never))).header).toBe( + 'Schema validation failed:', + ); + }); + + test('create() with an option the schema never sees', async () => { + expect(await refusal(stack.create(NOTE, { text: 'y' }, { associations: [TAG, TAG] }))).toEqual({ + header: 'Invalid arguments:', + paths: ['associations[1]'], + }); + expect( + await refusal(stack.create(NOTE, { text: 'y' }, { createdAt: new Date('nope') })), + ).toEqual({ header: 'Invalid arguments:', paths: ['createdAt'] }); + }); + + test('mutate() with a duplicate association', async () => { + expect(await refusal(stack.mutate(recordId, { associations: [TAG, TAG] }))).toEqual({ + header: 'Invalid arguments:', + paths: ['associations[1]'], + }); + }); + + test('migrateAll() and revokeType() given a TypeId', async () => { + expect(await refusal(stack.migrateAll(NOTE as never))).toEqual({ + header: 'Invalid arguments:', + paths: ['baseId'], + }); + expect(await refusal(stack.revokeType(NOTE as never, GRANT))).toEqual({ + header: 'Invalid arguments:', + paths: ['baseId'], + }); + }); + + test('grantType() given a TypeId', async () => { + expect(await refusal(stack.grantType(NOTE as never, GRANT))).toEqual({ + header: 'Invalid arguments:', + paths: ['baseId'], + }); + }); + + test('query() with a versioned filter.baseId', async () => { + expect(await refusal(stack.query({ filter: { baseId: NOTE } }))).toEqual({ + header: 'Invalid arguments:', + paths: ['filter.baseId'], + }); + }); + + test('amendAssociations() reports under changes', async () => { + expect(await refusal(stack.amendAssociations(recordId, []))).toEqual({ + header: 'Invalid arguments:', + paths: ['changes'], + }); + const add = { op: 'add', association: TAG } as const; + expect(await refusal(stack.amendAssociations(recordId, [add, add]))).toEqual({ + header: 'Invalid arguments:', + paths: ['changes[1]'], + }); + }); + + for (const [name, client] of [ + ['Stack', () => stack], + ['ScopedStack', () => stack.asEntity(OWNER)], + ] as [string, () => StackClient][]) { + describe(`${name}: the list verbs report under the list they were passed`, () => { + test('associate() and dissociate() under associations', async () => { + for (const verb of ['associate', 'dissociate'] as const) { + expect(await refusal(client()[verb](recordId, []))).toEqual({ + header: 'Invalid arguments:', + paths: ['associations'], + }); + expect(await refusal(client()[verb](recordId, [TAG, TAG]))).toEqual({ + header: 'Invalid arguments:', + paths: ['associations[1]'], + }); + } + }); + + test('grantAccess() and revokeAccess() under permissions', async () => { + const anyone = { kind: 'anyone', label: 'write' } as never; + for (const verb of ['grantAccess', 'revokeAccess'] as const) { + expect(await refusal(client()[verb](recordId, []))).toEqual({ + header: 'Invalid arguments:', + paths: ['permissions'], + }); + expect(await refusal(client()[verb](recordId, [anyone]))).toEqual({ + header: 'Invalid arguments:', + paths: ['permissions[0].label'], + }); + } + }); + }); + } +}); diff --git a/packages/wire-types/src/index.ts b/packages/wire-types/src/index.ts index 87590488..fa484c02 100644 --- a/packages/wire-types/src/index.ts +++ b/packages/wire-types/src/index.ts @@ -389,7 +389,9 @@ export function deserializeError(body: WireError): Error { const { code, message, details, versionConflict, schemaDrift } = body.error; switch (code) { case 'validation': - return new StackValidationError(details ?? []); + // The header is the message's first line, kept so a reconstructed + // error still says what the server validated. + return new StackValidationError(details ?? [], message.split('\n')[0].replace(/:$/, '')); case 'permission': return new StackPermissionError(message); case 'not_found': diff --git a/packages/wire-types/tests/errors.test.ts b/packages/wire-types/tests/errors.test.ts index b9eea52c..87a31fc6 100644 --- a/packages/wire-types/tests/errors.test.ts +++ b/packages/wire-types/tests/errors.test.ts @@ -101,6 +101,12 @@ describe('error round trip', () => { } }); + it('preserves the header saying what a validation error validated', () => { + const err = new StackValidationError([{ path: 'baseId', message: 'bad' }], 'Invalid arguments'); + const rebuilt = deserializeError(serializeError(err)!.body); + expect(rebuilt.message).toBe(err.message); + }); + it('preserves the fields an ifVersion retry loop needs', () => { const rebuilt = deserializeError( serializeError(new StackVersionConflictError('mismatch', '1hk153x0a00b', 3, 5))!.body, From 6516435bab8f79203070f9eb00973a15ccc6217d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 01:17:02 +0000 Subject: [PATCH 2/2] fix(core): keep one refusal per call and refuse wrong-surface lists first - create()/mutate() report argument and content problems together again; a mixed set reads "Invalid arguments", content alone keeps its header. - assertAssociationList() runs the surface check before counting duplicates, matching assertAssociationEdits(), so associate() given a duplicated permission is still a bad_request, not a validation error. - Changeset is minor: message text and errors[].path are observable. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012Juw9shSNkSZVNbXopt3Z3 --- .changeset/validation-error-headers.md | 6 +-- packages/core/src/query-validation.ts | 12 +++++- packages/core/src/scoped-stack.ts | 8 ++-- packages/core/src/stack.ts | 39 +++++++++++-------- .../core/tests/validation-headers.test.ts | 28 ++++++++++++- 5 files changed, 67 insertions(+), 26 deletions(-) diff --git a/.changeset/validation-error-headers.md b/.changeset/validation-error-headers.md index 1bbe0b50..0af2c0e5 100644 --- a/.changeset/validation-error-headers.md +++ b/.changeset/validation-error-headers.md @@ -1,7 +1,7 @@ --- -'@haverstack/core': patch -'@haverstack/wire-types': patch -'@haverstack/conformance-fixtures': patch +'@haverstack/core': minor +'@haverstack/wire-types': minor +'@haverstack/conformance-fixtures': minor --- `StackValidationError`'s message header names what was validated: record content keeps "Content validation failed", a type schema reads "Schema validation failed", and argument checks read "Invalid arguments". `associate()`/`dissociate()` and `grantAccess()`/`revokeAccess()` report list errors under `associations`/`permissions`, the parameter the caller passed, rather than `changes`. `deserializeError()` keeps the header a server sent. diff --git a/packages/core/src/query-validation.ts b/packages/core/src/query-validation.ts index e3cba413..c4031d98 100644 --- a/packages/core/src/query-validation.ts +++ b/packages/core/src/query-validation.ts @@ -573,12 +573,15 @@ export function assertAssociationEdits( * assertAssociationEdits() for the verbs taking a bare association list — * associate(), grantAccess() and their inverses — asked before the list is * wrapped as edits, so each problem is reported under `param`, the name the - * caller passed it as, rather than the wrapped list's `changes`. + * caller passed it as, rather than the wrapped list's `changes`. The checks + * run in the same order, so a wrong-surface element is refused as such + * before its duplicates are counted. */ export function assertAssociationList( associations: unknown, surface: string, param: string, + half: 'data' | 'authority', ): asserts associations is Association[] { if (!Array.isArray(associations) || associations.length === 0) { throw new StackValidationError( @@ -592,7 +595,12 @@ export function assertAssociationList( : [{ path: `${param}[${i}]`, message: `${param}[${i}] must be an object.` }], ); if (errors.length > 0) throw new StackValidationError(errors, ARGUMENTS_INVALID); - const listErrors = validateAssociations(associations as Association[], param); + const list = associations as Association[]; + const shapeErrors = list.flatMap((a, i) => validateAssociation(a, `${param}[${i}]`)); + if (shapeErrors.length > 0) throw new StackValidationError(shapeErrors, ARGUMENTS_INVALID); + if (half === 'data') assertDataAssociations(list, surface); + else assertAuthorityAssociations(list, surface); + const listErrors = validateAssociations(list, param); if (listErrors.length > 0) throw new StackValidationError(listErrors, ARGUMENTS_INVALID); } diff --git a/packages/core/src/scoped-stack.ts b/packages/core/src/scoped-stack.ts index 2724e4b4..a58f390d 100644 --- a/packages/core/src/scoped-stack.ts +++ b/packages/core/src/scoped-stack.ts @@ -1351,7 +1351,7 @@ export class ScopedStack implements StackClient { * read so it cannot depend on who is asking. */ async associate(id: RecordId, associations: DataAssociation[]): Promise { - assertAssociationList(associations, 'associate()', 'associations'); + assertAssociationList(associations, 'associate()', 'associations', 'data'); return this.amendAssociations( id, associations.map((association) => ({ op: 'add', association })), @@ -1361,7 +1361,7 @@ export class ScopedStack implements StackClient { /** See associate() — the same write gate, the same kind refusal. */ async dissociate(id: RecordId, associations: DataAssociation[]): Promise { - assertAssociationList(associations, 'dissociate()', 'associations'); + assertAssociationList(associations, 'dissociate()', 'associations', 'data'); return this.amendAssociations( id, associations.map((association) => ({ op: 'remove', association })), @@ -1397,7 +1397,7 @@ export class ScopedStack implements StackClient { * See docs/spec/access-control.md § Record-level permissions. */ async grantAccess(id: RecordId, permissions: AuthorityAssociation[]): Promise { - assertAssociationList(permissions, 'grantAccess()', 'permissions'); + assertAssociationList(permissions, 'grantAccess()', 'permissions', 'authority'); return this.amendAccess( id, permissions.map((association) => ({ op: 'add', association })), @@ -1407,7 +1407,7 @@ export class ScopedStack implements StackClient { /** Withdraw elements of who reaches a record — see grantAccess(). */ async revokeAccess(id: RecordId, permissions: AuthorityAssociation[]): Promise { - assertAssociationList(permissions, 'revokeAccess()', 'permissions'); + assertAssociationList(permissions, 'revokeAccess()', 'permissions', 'authority'); return this.amendAccess( id, permissions.map((association) => ({ op: 'remove', association })), diff --git a/packages/core/src/stack.ts b/packages/core/src/stack.ts index c562ebc6..f43216e0 100644 --- a/packages/core/src/stack.ts +++ b/packages/core/src/stack.ts @@ -578,6 +578,21 @@ export interface StackClient { subscribe(handler: (change: RecordChange) => void, opts?: SubscribeOptions): Promise; } +/** + * One refusal for every problem a call carries, content and arguments alike, + * so a caller fixes them in one round trip. Content is itself an argument, + * so a mixed set reads "Invalid arguments"; content alone keeps its header. + */ +function throwValidation( + contentErrors: ValidationError[], + argumentErrors: ValidationError[], +): void { + if (argumentErrors.length > 0) { + throw new StackValidationError([...argumentErrors, ...contentErrors], ARGUMENTS_INVALID); + } + if (contentErrors.length > 0) throw new StackValidationError(contentErrors); +} + // ------------------------------------------------------- // Stack class // ------------------------------------------------------- @@ -1051,10 +1066,7 @@ export class Stack implements StackClient { if (opts.updatedAt !== undefined && updatedAt.getTime() < createdAt.getTime()) { argumentErrors.push({ path: 'updatedAt', message: 'updatedAt cannot precede createdAt.' }); } - if (argumentErrors.length > 0) { - throw new StackValidationError(argumentErrors, ARGUMENTS_INVALID); - } - const errors = [ + const contentErrors = [ ...validateReservedKeys(content), ...validateContentKeys(content), ...validateContent(content, type.schema), @@ -1062,9 +1074,7 @@ export class Stack implements StackClient { ...validateGrantBaseId(typeId, content), ...validateInstall(typeId, content), ]; - if (errors.length > 0) { - throw new StackValidationError(errors); - } + throwValidation(contentErrors, argumentErrors); assertContentSize(content, this.capabilities.limits.contentBytes, 'Content'); @@ -1370,17 +1380,14 @@ export class Stack implements StackClient { : []), ...(associations ? validateAssociations(associations) : []), ]; - if (argumentErrors.length > 0) { - throw new StackValidationError(argumentErrors, ARGUMENTS_INVALID); - } - const errors = contentPatch + const patchErrors = contentPatch ? [ ...validateReservedKeys(contentPatch), ...validatePatchValues(contentPatch), ...validateContentKeys(contentPatch), ] : []; - if (errors.length > 0) throw new StackValidationError(errors); + throwValidation(patchErrors, argumentErrors); await this.checkAttachmentAssociationPointers(associations, existing.associations); @@ -1475,7 +1482,7 @@ export class Stack implements StackClient { associations: DataAssociation[], opts: ActorOptions = {}, ): Promise { - assertAssociationList(associations, 'associate()', 'associations'); + assertAssociationList(associations, 'associate()', 'associations', 'data'); return this.amendAssociations( id, associations.map((association) => ({ op: 'add', association })), @@ -1496,7 +1503,7 @@ export class Stack implements StackClient { associations: DataAssociation[], opts: ActorOptions = {}, ): Promise { - assertAssociationList(associations, 'dissociate()', 'associations'); + assertAssociationList(associations, 'dissociate()', 'associations', 'data'); return this.amendAssociations( id, associations.map((association) => ({ op: 'remove', association })), @@ -1557,7 +1564,7 @@ export class Stack implements StackClient { permissions: AuthorityAssociation[], opts: ActorOptions = {}, ): Promise { - assertAssociationList(permissions, 'grantAccess()', 'permissions'); + assertAssociationList(permissions, 'grantAccess()', 'permissions', 'authority'); return this.amendAccess( id, permissions.map((association) => ({ op: 'add', association })), @@ -1576,7 +1583,7 @@ export class Stack implements StackClient { permissions: AuthorityAssociation[], opts: ActorOptions = {}, ): Promise { - assertAssociationList(permissions, 'revokeAccess()', 'permissions'); + assertAssociationList(permissions, 'revokeAccess()', 'permissions', 'authority'); return this.amendAccess( id, permissions.map((association) => ({ op: 'remove', association })), diff --git a/packages/core/tests/validation-headers.test.ts b/packages/core/tests/validation-headers.test.ts index 5c943091..0b9ab059 100644 --- a/packages/core/tests/validation-headers.test.ts +++ b/packages/core/tests/validation-headers.test.ts @@ -1,7 +1,7 @@ import { describe, test, expect, beforeEach } from 'vitest'; import { Stack } from '../src/stack.js'; import type { StackClient } from '../src/stack.js'; -import { StackValidationError } from '../src/errors.js'; +import { StackBadRequestError, StackValidationError } from '../src/errors.js'; import { MemoryAdapter } from '../src/testing.js'; import type { DataAssociation, TypeGrant } from '../src/types.js'; @@ -69,6 +69,18 @@ describe('an argument refusal says the arguments were invalid, under their own n ).toEqual({ header: 'Invalid arguments:', paths: ['createdAt'] }); }); + test('create() and mutate() report argument and content problems in one refusal', async () => { + expect(await refusal(stack.create(NOTE, { text: 1 }, { associations: [TAG, TAG] }))).toEqual({ + header: 'Invalid arguments:', + paths: ['associations[1]', 'text'], + }); + expect( + await refusal( + stack.mutate(recordId, { associations: [TAG, TAG], contentPatch: { text: undefined } }), + ), + ).toEqual({ header: 'Invalid arguments:', paths: ['associations[1]', 'text'] }); + }); + test('mutate() with a duplicate association', async () => { expect(await refusal(stack.mutate(recordId, { associations: [TAG, TAG] }))).toEqual({ header: 'Invalid arguments:', @@ -131,6 +143,20 @@ describe('an argument refusal says the arguments were invalid, under their own n } }); + test('a wrong-surface element is refused as such, even when duplicated', async () => { + const read = { kind: 'anyone', label: 'read' } as never; + for (const verb of ['associate', 'dissociate'] as const) { + await expect(client()[verb](recordId, [read, read])).rejects.toBeInstanceOf( + StackBadRequestError, + ); + } + for (const verb of ['grantAccess', 'revokeAccess'] as const) { + await expect(client()[verb](recordId, [TAG, TAG] as never)).rejects.toBeInstanceOf( + StackBadRequestError, + ); + } + }); + test('grantAccess() and revokeAccess() under permissions', async () => { const anyone = { kind: 'anyone', label: 'write' } as never; for (const verb of ['grantAccess', 'revokeAccess'] as const) {