Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 115 additions & 2 deletions apps/cli/src/__tests__/lib/validate-type-spec.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,88 @@ describe('validateValueAgainstTypeSpec – oneOf with mixed schemas', () => {
oneOf: [{ const: 'block' }, { type: 'object', properties: { kind: { const: 'inline' } }, required: ['kind'] }],
};

test('falls back to generic message when variants are not all const', () => {
test('enumerates the accepted variant shapes when nothing matches', () => {
try {
validateValueAgainstTypeSpec('nope', mixedSchema, 'target');
throw new Error('Expected CliError to be thrown');
} catch (error) {
const cliError = error as CliError;
expect(cliError.message).toBe('target must match one of the allowed schema variants.');
expect(cliError.message).toBe(
'target must match one of: "block" | { kind: "inline" }. Received a string ("nope").',
);
}
});
});

// ---------------------------------------------------------------------------
// Tagged-union (target/at) actionable errors — the dominant LLM failure class.
// These exercise the REAL contract schemas so the assertions track what an
// agent actually sees. See customer error pivot: "insert:target …" and
// "create paragraph:at must match one of the allowed schema variants."
// ---------------------------------------------------------------------------

describe('validateValueAgainstTypeSpec – tagged-union actionable errors (real schemas)', () => {
const atSchema = CLI_OPERATION_METADATA['doc.create.paragraph'].params.find((p) => p.name === 'at')?.schema;
const insertTargetSchema = CLI_OPERATION_METADATA['doc.insert'].params.find((p) => p.name === 'target')?.schema;

if (!atSchema || !insertTargetSchema) {
throw new Error('metadata missing create.paragraph:at or insert:target schema');
}

test('flattened "at" (nodeId hoisted out of target) names the missing field', () => {
try {
validateValueAgainstTypeSpec({ kind: 'after', nodeId: 'ABC123' }, atSchema, 'at');
throw new Error('Expected CliError to be thrown');
} catch (error) {
// Was the misleading "at.nodeId is not allowed by schema."
expect((error as CliError).message).toBe('at.target is required.');
}
});

test('unknown "at" kind enumerates the valid kinds and echoes what was sent', () => {
try {
validateValueAgainstTypeSpec({ kind: 'sidebar' }, atSchema, 'at');
throw new Error('Expected CliError to be thrown');
} catch (error) {
// Was the misleading "at.target is required."
expect((error as CliError).message).toBe(
'at.kind must be one of: "documentStart", "documentEnd", "before", "after" (received "sidebar").',
);
}
});

test('bare string "at" enumerates the accepted shapes instead of the opaque union error', () => {
try {
validateValueAgainstTypeSpec('ABC123', atSchema, 'at');
throw new Error('Expected CliError to be thrown');
} catch (error) {
const message = (error as CliError).message;
expect(message).not.toContain('allowed schema variants');
expect(message).toContain('at must match one of:');
expect(message).toContain('kind: "before"');
expect(message).toContain('Received a string ("ABC123")');
}
});

test('insert target missing nodeType names the missing field', () => {
try {
validateValueAgainstTypeSpec({ kind: 'block', nodeId: 'ABC123' }, insertTargetSchema, 'target');
throw new Error('Expected CliError to be thrown');
} catch (error) {
const message = (error as CliError).message;
expect(message).not.toContain('allowed schema variants');
expect(message).toContain('nodeType');
}
});

test('bare string insert target enumerates the accepted shapes', () => {
try {
validateValueAgainstTypeSpec('ABC123', insertTargetSchema, 'target');
throw new Error('Expected CliError to be thrown');
} catch (error) {
const message = (error as CliError).message;
expect(message).not.toContain('allowed schema variants');
expect(message).toContain('target must match one of:');
}
});
});
Expand Down Expand Up @@ -108,6 +183,44 @@ describe('validateValueAgainstTypeSpec – repeated actionable oneOf errors', ()
});
});

// Without a shared const discriminator across object variants, the
// discriminator path cannot intercept — a repeated error must therefore surface
// through `selectRepeatedActionableOneOfError`. Pins that fallback so a future
// change to the discriminator gate can't silently make it unreachable.
describe('validateValueAgainstTypeSpec – repeated actionable oneOf errors (no discriminator)', () => {
const repeatedRequiredSchema: CliTypeSpec = {
oneOf: [
{
type: 'object',
properties: {
id: { type: 'string' },
name: { type: 'string' },
},
required: ['id'],
},
{
type: 'object',
properties: {
id: { type: 'string' },
label: { type: 'string' },
},
required: ['id'],
},
],
};

test('surfaces the repeated required-field error when no discriminator exists', () => {
try {
validateValueAgainstTypeSpec({ name: 'x' }, repeatedRequiredSchema, 'target');
throw new Error('Expected CliError to be thrown');
} catch (error) {
const cliError = error as CliError;
expect(cliError.message).toBe('target.id is required.');
expect((cliError.details as { selectedError?: string }).selectedError).toBe('target.id is required.');
}
});
});

describe('validateValueAgainstTypeSpec – enum branch', () => {
const enumSchema: CliTypeSpec = {
type: 'string',
Expand Down
113 changes: 112 additions & 1 deletion apps/cli/src/lib/operation-args.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,85 @@ function selectRepeatedActionableOneOfError(path: string, errors: string[]): str
return bestMessage;
}

/**
* Render a variant's shape compactly so a oneOf failure can name the accepted
* forms (e.g. `{ kind: "before", target }`) instead of an opaque "must match
* one of the allowed schema variants". Object properties show their `const`
* discriminator when present, and a trailing `?` when optional.
*/
function describeVariant(variant: CliTypeSpec): string {
if ('const' in variant) return JSON.stringify(variant.const);
if ('oneOf' in variant) return (variant.oneOf as CliTypeSpec[]).map(describeVariant).join(' | ');
if (variant.enum) return variant.enum.map((entry) => JSON.stringify(entry)).join(' | ');
if (variant.type === 'object') {
const properties = variant.properties ?? {};
const required = new Set(variant.required ?? []);
const keys = Object.keys(properties);
if (keys.length === 0) return 'object';
const parts = keys.map((key) => {
const prop = properties[key];
if (prop && 'const' in prop) return `${key}: ${JSON.stringify(prop.const)}`;
return required.has(key) ? key : `${key}?`;
});
return `{ ${parts.join(', ')} }`;
}
if (variant.type === 'array') return 'array';
if (variant.type === 'json') return 'object';
return variant.type ?? 'value';
}

/**
* The property key that carries a `const` in every object variant — the
* discriminator of a tagged union (e.g. `kind` for target/at, `op` for
* mutation steps). Returns null when there is no such shared key. Non-object
* variants (e.g. a bare string ref) are ignored so mixed unions still resolve.
*/
function getOneOfDiscriminator(variants: readonly CliTypeSpec[]): string | null {
const objectVariants = variants.filter(
(variant): variant is Extract<CliTypeSpec, { type: 'object' }> =>
!('const' in variant) && !('oneOf' in variant) && variant.type === 'object' && isRecord(variant.properties),
);
if (objectVariants.length < 2) return null;
for (const key of Object.keys(objectVariants[0].properties)) {
const sharedByAll = objectVariants.every((variant) => {
const prop = variant.properties[key];
return prop != null && 'const' in prop;
});
if (sharedByAll) return key;
}
return null;
}

/** The const value a variant pins for `key`, if any (its discriminator tag). */
function variantConstFor(variant: CliTypeSpec, key: string): unknown {
if ('const' in variant || 'oneOf' in variant || variant.type !== 'object') return undefined;
const prop = variant.properties?.[key];
return prop && 'const' in prop ? prop.const : undefined;
}

/**
* Truncate a serialized value to keep oneOf error messages bounded — a caller
* accidentally passing a multi-MB string as `target`/`at` shouldn't inflate
* logs or the LLM context window. Matches the truncation pattern used by the
* REPAIR_BLOCKED preview.
*/
function truncateForError(serialized: string, max = 64): string {
return serialized.length > max ? `${serialized.slice(0, max)}…` : serialized;
}

/** Human description of a received value, for oneOf error messages. */
function describeReceived(value: unknown): string {
if (value === null) return 'null';
if (Array.isArray(value)) return 'an array';
const valueType = typeof value;
if (valueType === 'object') {
const keys = Object.keys(value as Record<string, unknown>);
return keys.length > 0 ? `an object with keys [${keys.join(', ')}]` : 'an empty object';
}
if (valueType === 'string') return `a string (${truncateForError(JSON.stringify(value))})`;
return `a ${valueType} (${truncateForError(JSON.stringify(value))})`;
}

export function validateValueAgainstTypeSpec(value: unknown, schema: CliTypeSpec, path: string): void {
if ('const' in schema) {
if (value !== schema.const) {
Expand All @@ -166,12 +245,44 @@ export function validateValueAgainstTypeSpec(value: unknown, schema: CliTypeSpec
}
}

// Tagged-union path (target/at keyed by `kind`, mutation steps keyed by
// `op`): when the value carries the discriminator, surface the matching
// variant's specific failure ("at.target is required.") or the set of
// valid tags. This lets an LLM self-correct instead of retrying the same
// malformed shape against an opaque union error.
const discriminator = getOneOfDiscriminator(variants);
if (discriminator && isRecord(value) && value[discriminator] !== undefined) {
const received = value[discriminator];
const matched = variants.find((variant) => variantConstFor(variant, discriminator) === received);
if (matched) {
try {
validateValueAgainstTypeSpec(value, matched, path);
// Unreachable in practice: `matched` already failed in the outer
// variant loop above (deterministic same-input revalidation must
// throw again). Explicit return so a future fast-path refactor on
// this function can't silently let control fall through to the
// unmatched-tag throw below with a falsely-matched value.
return;
} catch (error) {
const message = error instanceof Error ? error.message : String(error);
throw new CliError('VALIDATION_ERROR', message, { errors, selectedError: message });
}
}
const allowedTags = variants
.map((variant) => variantConstFor(variant, discriminator))
.filter((tag) => tag !== undefined)
.map((tag) => JSON.stringify(tag));
const unmatchedTagMessage = `${path}.${discriminator} must be one of: ${allowedTags.join(', ')} (received ${truncateForError(JSON.stringify(received))}).`;
throw new CliError('VALIDATION_ERROR', unmatchedTagMessage, { errors, selectedError: unmatchedTagMessage });
}

const allowedValues = extractConstValues(variants);
const selectedError = selectRepeatedActionableOneOfError(path, errors);
const message =
allowedValues.length > 0
? `${path} must be one of: ${allowedValues.join(', ')}.`
: (selectedError ?? `${path} must match one of the allowed schema variants.`);
: (selectedError ??
`${path} must match one of: ${variants.map(describeVariant).join(' | ')}. Received ${describeReceived(value)}.`);
throw new CliError('VALIDATION_ERROR', message, { errors, selectedError });
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -998,6 +998,13 @@ export function executeTextRewrite(
tr.replaceWith(remap(change.docFrom), remap(change.docTo), content);
}
}
} else if (trimmedNew.length === 0) {
// Pure deletion after trimming: a non-empty replacement whose new text is
// fully contained in the old text's common prefix + suffix collapses to an
// empty delta (e.g. "best endeavours to:" → "endeavours to:" leaves
// trimmedNew === ""). Delete the removed range rather than building
// schema.text('') — ProseMirror rejects empty text nodes.
tr.delete(trimmedFrom, trimmedTo);
} else {
// 0 or 1 word change: replace just the trimmed range.
const content = buildTextWithTabs(editor.state.schema, trimmedNew, asProseMirrorMarks(marks));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,11 @@ describe('remap correctness: mapping.map called with compiled positions', () =>

expect(tr.mapping.map).toHaveBeenCalledWith(3);
expect(tr.mapping.map).toHaveBeenCalledWith(10);
expect(tr.replaceWith).toHaveBeenCalledWith(5, 12, expect.anything());
// An empty replacement collapses to a pure deletion: it must use tr.delete,
// not tr.replaceWith with an empty text node (ProseMirror rejects empty
// text nodes — the mocked schema previously hid that crash).
expect(tr.delete).toHaveBeenCalledWith(5, 12);
expect(tr.replaceWith).not.toHaveBeenCalled();
});

it('equal length replacement: maps positions through mapping', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,54 @@ describe('doc.replace multi-paragraph integration', () => {
expect(accepted).not.toContain('Smith address');
});

// Customer-reported crash ("Empty text nodes are not allowed"): a non-empty
// replacement whose new text is fully contained in the old text's prefix +
// suffix trims to an EMPTY delta. The single-change branch must delete the
// removed text rather than build schema.text('') (which ProseMirror rejects).
it('rewrites a replacement that trims to an empty delta as a deletion (executor)', () => {
editor = makeEditor(['the Company refers to: the following terms']);
const { step, target } = compileSingleRewrite(editor, 'refers to:', 'to:');
const tr = editor.state.tr;

const outcome = executeTextRewrite(editor, tr, target as any, step as any, tr.mapping as any);

expect(outcome).toEqual({ changed: true });
expect(tr.doc.textContent).toBe('the Company to: the following terms');
});

it('handles a tracked replace that trims to an empty delta (deletion only)', () => {
editor = makeEditor(['We will use our best endeavours to: deliver']);
const receipt = editor.doc.replace(
{
ref: getFirstMatchRef(editor, 'best endeavours to:'),
text: 'endeavours to:',
},
{ changeMode: 'tracked' },
);

expect(receipt.success).toBe(true);

// Accepted view: drop trackDelete marks, keep everything else.
const acceptedParts: string[] = [];
editor.state.doc.descendants((node: any) => {
if (!node.isText || !node.text) return;
const isDeleted = node.marks.some((mark: any) => mark.type.name === TrackDeleteMarkName);
if (!isDeleted) acceptedParts.push(node.text);
});

expect(acceptedParts.join('')).toBe('We will use our endeavours to: deliver');

// The trimmed-away prefix "best " must be represented as a tracked deletion.
const deletedTexts: string[] = [];
editor.state.doc.descendants((node: any) => {
if (!node.isText || !node.text) return;
if (node.marks.some((mark: any) => mark.type.name === TrackDeleteMarkName)) {
deletedTexts.push(node.text);
}
});
expect(deletedTexts.join('')).toContain('best');
});

it('SD-3044: tracked rewrite of long block preserves spacing across multiple equal anchors', () => {
editor = makeEditor([
'[insert] Pty Limited a company incorporated in Australia having its registered office at [insert] (ACN [insert])("Company")',
Expand Down
Loading