fix(metadata-protocol): classify a failed index build from the ERROR, not its message (#6699) (#6845)

`classifyIndexFailure` carried a fifth private unique-violation vocabulary —
the one #6250's inventory missed, because it lives in a package none of the
other four touched — and answered from the message channel only.

The first arm now delegates to `@objectstack/types`' `isUniqueViolationError`,
and `probeThenReplaceIndex` passes it the caught ERROR OBJECT rather than
`err.message`. A string-only swap would have compiled unchanged and kept the
defect: the `code` / `errno` / `cause` channels are the point of the shared
predicate.

A conflict reported on `code`/`errno` with unhelpful prose (SQLite's
`SQLITE_CONSTRAINT_UNIQUE`, MySQL's `ER_DUP_ENTRY`/1062, Postgres' `23505`, or
one step down `cause`) was classified `failed`; it is now `conflict`, which is
the verdict that produces ADR-0120 D4's report. Every message-channel verdict
is unchanged.

Preserved deliberately: the arm order (duplicate wording judged BEFORE dialect
wording, since MySQL's duplicate error mentions the key and some drivers wrap
both facts in one string), and the dialect arm as this module's own
message-based vocabulary — the shared predicate answers the first arm only.


Claude-Session: https://claude.ai/code/session_01W6bLax4KMrSfnE1ydFU8Dw

Co-authored-by: Claude <noreply@anthropic.com>

Author: os-zhuang

.changeset/classify-index-failure-error-channel.md

@@ -0,0 +1,40 @@
+---
+"@objectstack/metadata-protocol": patch
+---
+
+fix(metadata-protocol): classify a failed index build from the ERROR, not its message (#6699)
+
+`classifyIndexFailure` — the function both runtime partial-index migrations in
+this package classify a failed `CREATE UNIQUE INDEX` with — carried its own
+private unique-violation vocabulary and answered from the **message channel
+only**. That made it the fifth such copy in the repo, and the one #6250's
+inventory missed: it lives in a package none of the other four touched, so it
+was never in that table and none of the queued follow-ups covered it.
+
+The first arm now delegates to `@objectstack/types`' `isUniqueViolationError`
+(#6250 / PR #6541) — the one named answer to "is this a unique-constraint
+violation?" — and `probeThenReplaceIndex` passes it the **caught error object**
+instead of `err.message`. A string-only swap would have compiled unchanged and
+kept the defect: the point of the shared predicate is the `code` / `errno` /
+`cause` channels, which unwrapping the message throws away.
+
+**What changes at runtime.** A driver that reports the conflict on `code` or
+`errno` while giving unhelpful prose — SQLite's `SQLITE_CONSTRAINT_UNIQUE`,
+MySQL's `ER_DUP_ENTRY` / errno `1062`, Postgres' SQLSTATE `23505`, or the
+condition one step down `error.cause` behind a pooled wrapper's `Write failed`
+— was classified `failed`. It is now `conflict`, which is the verdict that
+produces the report ADR-0120 D4 requires: the key that is not enforced, the
+query that lists the offending rows, and the pointer at `os migrate plan`.
+Every message-channel verdict is unchanged — the shared predicate's message
+limb covers all three shipped dialects' prose.
+
+**Two things deliberately preserved.** The arm order still checks the
+duplicate-row question BEFORE the dialect question, because MySQL's duplicate
+error mentions the key and some drivers wrap both facts in one string; and the
+dialect arm (`unsupported`) is still this module's own message-based
+vocabulary, since the shared predicate answers the first arm only and has no
+opinion about dialect support.
+
+`classifyIndexFailure`'s parameter widens from `string` to `unknown`, so every
+existing string call still compiles and is judged exactly as before. Callers
+holding a caught error should pass it directly rather than `err.message`.

packages/metadata-protocol/src/migrations/partial-index-probe.test.ts

@@ -157,6 +157,97 @@ describe('probe-first partial index replacement (#6418)', () => {
         expect(classifyIndexFailure('disk I/O error')).toBe('failed');
     });
 
+    /**
+     * #6699 — the substance of the migration onto `@objectstack/types`'
+     * `isUniqueViolationError`: the conflict is judged on the channels the
+     * driver actually wrote it to, not on prose alone.
+     *
+     * Every message below is deliberately USELESS — none carries a word any
+     * unique-violation vocabulary's message limb looks for — so each case can
+     * pass ONLY by reading `code` / `errno`. The second assertion in each case
+     * proves that: run the same prose through the classifier on its own and the
+     * verdict is `failed`, which is what this module answered for all of them
+     * while it carried its own message-only regex.
+     */
+    it.each([
+        ['a SQLite extended result code', { code: 'SQLITE_CONSTRAINT_UNIQUE' }],
+        ["mysql2's symbolic name", { code: 'ER_DUP_ENTRY' }],
+        ['a bare MySQL errno', { errno: 1062 }],
+        ['a Postgres SQLSTATE', { code: '23505' }],
+    ])('reads a conflict off %s when the message says nothing (#6699)', (_label, channels) => {
+        const error = Object.assign(new Error('insert failed'), channels);
+        expect(classifyIndexFailure(error)).toBe('conflict');
+        expect(classifyIndexFailure(error.message)).toBe('failed');
+    });
+
+    it('follows a pooled wrapper down to the cause (#6699)', () => {
+        // Pool and query-builder layers re-throw with the original attached, so
+        // the only copy of the condition is one step down.
+        const wrapped = Object.assign(new Error('Write failed'), {
+            cause: Object.assign(new Error('insert failed'), { code: '23505' }),
+        });
+        expect(classifyIndexFailure(wrapped)).toBe('conflict');
+        expect(classifyIndexFailure(wrapped.message)).toBe('failed');
+    });
+
+    it('keeps the data verdict ahead of the dialect verdict on the OBJECT channel too (#6699)', () => {
+        // The arm order, re-pinned where widening the input could have broken
+        // it: a duplicate reported on `code`, wrapped by a layer whose prose is
+        // a dialect refusal. Judged on the message alone this is `unsupported`
+        // — "this database cannot build this index" for a real data conflict,
+        // exactly the misreport the ordering exists to prevent.
+        const both = Object.assign(new Error('near "WHERE": syntax error'), {
+            code: 'SQLITE_CONSTRAINT_UNIQUE',
+        });
+        expect(classifyIndexFailure(both)).toBe('conflict');
+        expect(classifyIndexFailure(both.message)).toBe('unsupported');
+        // …and on a single string carrying both facts, unchanged since #6418.
+        expect(
+            classifyIndexFailure('near "WHERE": syntax error — duplicate key value violates unique constraint'),
+        ).toBe('conflict');
+    });
+
+    it('a dialect refusal carrying its own code is still `unsupported` (#6699)', () => {
+        // Widening the input from `string` to the error object must not blind
+        // the second arm: MySQL's parse error has `code` and `errno` too, and
+        // neither is a unique-violation signal, so the verdict has to come from
+        // the message exactly as before.
+        const parseError = Object.assign(
+            new Error("You have an error in your SQL syntax ... near 'WHERE state'"),
+            { code: 'ER_PARSE_ERROR', errno: 1064 },
+        );
+        expect(classifyIndexFailure(parseError)).toBe('unsupported');
+        const io = Object.assign(new Error('disk I/O error'), { code: 'SQLITE_IOERR', errno: 10 });
+        expect(classifyIndexFailure(io)).toBe('failed');
+    });
+
+    it('the probe hands the ERROR to the classifier, not its message (#6699)', async () => {
+        // The threading pin, and the only test here that can see it: every
+        // assertion above still passes if `probeThenReplaceIndex` keeps
+        // unwrapping `err.message` before classifying. This one cannot — the
+        // verdict exists nowhere but on `code`.
+        const codeOnly: IndexExec = async (sql: string) => {
+            if (sql.startsWith('CREATE')) {
+                throw Object.assign(new Error('insert failed'), { code: 'SQLITE_CONSTRAINT_UNIQUE' });
+            }
+            return db.exec(sql);
+        };
+
+        const outcome = await probeThenReplaceIndex(codeOnly, {
+            indexName: REAL,
+            probeIndexName: PROBE,
+            buildSql,
+        });
+
+        expect(outcome.status).toBe('conflict');
+        expect(outcome.failedAt).toBe('probe');
+        // `detail` is unchanged — still the driver's own prose, for the operator.
+        expect(outcome.detail).toBe('insert failed');
+        // The probe is what failed, so the previous index is untouched.
+        expect(indexDdl(REAL)).toEqual(EXISTING_DDL);
+        expect(indexDdl(PROBE)).toBeUndefined();
+    });
+
     it('logProblem prefers error(), falls back to warn(), and tolerates neither', () => {
         const full = { warn: vi.fn(), error: vi.fn() };
         logProblem(full, 'msg', 'detail');

packages/metadata-protocol/src/migrations/partial-index-probe.ts

@@ -37,6 +37,8 @@
  * a classified status plus the driver's own text and stays out of the way.
  */
 
+import { isUniqueViolationError } from '@objectstack/types';
+
 /** Raw-SQL seam. Mirrors `ensureOverlayIndex`: `raw()` first, `execute()` second. */
 export type IndexExec = (sql: string) => Promise<unknown>;
 
@@ -86,7 +88,26 @@ export type PartialIndexStatus =
     | 'failed';
 
 /**
- * Classify a failed `CREATE UNIQUE INDEX`.
+ * The text the DIALECT arm judges, from a thrown value of any shape.
+ *
+ * `message` first, because that is the channel a driver writes its refusal on
+ * and the only one this arm has ever read; `String()` only as the last resort
+ * — which is what a bare string resolves to unchanged, so a caller holding
+ * nothing but prose is judged exactly as before.
+ */
+function indexFailureText(error: unknown): string {
+    if (typeof error === 'string') return error;
+    if (typeof error === 'object' && error !== null) {
+        const { message } = error as { message?: unknown };
+        if (typeof message === 'string') return message;
+    }
+    return String(error);
+}
+
+/**
+ * Classify a failed `CREATE UNIQUE INDEX`, from the thrown ERROR (#6699).
+ *
+ * ## The two arms, and why the order is load-bearing
  *
  * Duplicate-row wording is checked BEFORE dialect wording: MySQL's duplicate
  * error mentions the key, and some drivers wrap both facts in one string, so
@@ -98,12 +119,35 @@ export type PartialIndexStatus =
  * split: no partial indexes at all, and (before 8.0.13 / on MariaDB) no
  * functional key parts for `COALESCE` parts. Both leave the same outcome — the
  * previous index stays — so one verdict is enough.
+ *
+ * ## Why the first arm is not this module's own regex any more
+ *
+ * "Is this a unique-constraint violation?" is one question, and #6250 gave it
+ * one named answer — `isUniqueViolationError` in `@objectstack/types`. This
+ * function carried a **fifth** private vocabulary for it (#6699, missed by that
+ * inventory because it lives in a package none of the other four touched), and
+ * the copy was strictly weaker in the way that inventory was about: it read the
+ * **message channel only**. A driver that reports the conflict on `code` /
+ * `errno` — SQLite's `SQLITE_CONSTRAINT_UNIQUE`, MySQL's `ER_DUP_ENTRY` /
+ * errno `1062`, Postgres' SQLSTATE `23505` — while giving unhelpful prose
+ * (`insert failed`, a pooled wrapper's `Write failed`, or the condition one step
+ * down `error.cause`) was classified `failed` here, where the shared predicate
+ * answers `true`. Same shape as the hole that made every MySQL conflict a 500
+ * in `mapDataError` before #6541.
+ *
+ * The predicate answers the FIRST arm only. It has no opinion about dialect
+ * support, so the second arm stays this module's own — and stays second.
+ *
+ * ⚠️ Pass the **error**, not `err.message`. A string still works (the predicate
+ * reads it on the message channel, and so does {@link indexFailureText}), but a
+ * caller that unwraps first throws away the `code` / `errno` / `cause` channels
+ * that are the whole reason this reads the object.
  */
-export function classifyIndexFailure(message: string): PartialIndexStatus {
-    if (/unique constraint failed|duplicate entry|duplicate key value|violates unique/i.test(message)) {
+export function classifyIndexFailure(error: unknown): PartialIndexStatus {
+    if (isUniqueViolationError(error)) {
         return 'conflict';
     }
-    if (/partial|where clause|near "where"|near 'where'|functional|syntax/i.test(message)) {
+    if (/partial|where clause|near "where"|near 'where'|functional|syntax/i.test(indexFailureText(error))) {
         return 'unsupported';
     }
     return 'failed';
@@ -169,9 +213,14 @@ export async function probeThenReplaceIndex(
     try {
         await exec(buildSql(probeIndexName));
     } catch (err: unknown) {
+        // `detail` is the OPERATOR-facing text and stays the driver's own prose.
+        // The VERDICT is taken from the error object itself, so a conflict
+        // reported on `code` / `errno` / `cause` with unhelpful prose is still
+        // classified as one (#6699) — unwrapping first is exactly what the
+        // migration onto the shared predicate exists to stop.
         const detail = err instanceof Error ? err.message : String(err);
         await dropIndexQuietly(exec, probeIndexName);
-        return { status: classifyIndexFailure(detail), detail, failedAt: 'probe' };
+        return { status: classifyIndexFailure(err), detail, failedAt: 'probe' };
     }
     await dropIndexQuietly(exec, probeIndexName);
 

packages/metadata-protocol/src/migrations/view-definition-active-index.test.ts

@@ -469,6 +469,14 @@ describe('sys_view_definition active-row uniqueness (#5839) on a NULL-safe key (
         // MariaDB's refusal of a functional key part, which #6417 introduces.
         expect(classifyIndexFailure('Functional index on a column is not supported')).toBe('unsupported');
         expect(classifyIndexFailure('disk I/O error')).toBe('failed');
+        // #6699: the same verdict off the `code` channel, with prose that
+        // carries no signal at all. Asserted through THIS module's re-export
+        // (the public `@objectstack/metadata-protocol` surface), because that is
+        // the export the classifier's own home is reached by — the full
+        // channel matrix lives in `partial-index-probe.test.ts`.
+        expect(
+            classifyIndexFailure(Object.assign(new Error('insert failed'), { code: 'ER_DUP_ENTRY' })),
+        ).toBe('conflict');
     });
 
     it('buildActiveIndexSql scopes rows AND spells the key NULL-safe', () => {