Skip to content

driver-sql's inline unique-violation regex is a fourth private vocabulary — migrate it onto the shared @objectstack/types predicate #6543

Description

@os-project-manager

Blocked-by: #6250 (the shared predicate must land first — PR #6541)

Filed by the #6250 developer agent per that issue's ruling: "OUT OF SCOPE — the two out-of-lane consumers. ... packages/drivers/driver-sql/** (domain:drivers) must NOT be edited. Migrating them onto the predicate is a follow-up per lane, filed once the predicate exists." The predicate now exists, so this is that filing.

What is left

packages/drivers/driver-sql/src/sql-driver.ts judges the same question with an inline regex, no name and no doc comment:

if (nullSafe.size > 0 && /unique constraint failed|duplicate entry|duplicate key value/i.test(msg)) {

It reads only err.message — the code / errno channel is not consulted at all, so a driver that surfaces SQLSTATE 23505 or ER_DUP_ENTRY with unremarkable prose is invisible to it. That is the same blind spot #6250 measured on the REST side (a Postgres error carrying only code: '23505' came back 500), one layer down.

Why this is finding and not a defect

Nothing a user hits today, as far as this card measured: on the three dialects the repo ships, the message channel happens to carry the words. The defect is structural — a fourth vocabulary that can be taught a dialect the other three never learn. Grade it in triage rather than trusting the label.

The migration

@objectstack/driver-sql already depends on @objectstack/types. Swap for isUniqueViolationError(error) and delete the inline regex. Two things to check while there, because they are what makes this more than a mechanical swap:

  1. This site tests msg, not the error object. The shared predicate accepts a bare string on the message channel, so a minimal swap works — but passing the error instead is what buys the code/errno channels, which is the point of migrating.
  2. The nullSafe.size > 0 guard is the site's own business logic and must survive untouched; only the discriminator moves.

Note the sibling spelling in this package's own tests (/UNIQUE constraint failed|duplicate key value/, in sql-driver-schema.test.ts, sql-driver-unique-tenancy.test.ts, adr0120-three-posture-conformance.test.ts). Those are assertions on what a real driver emitted, not a discriminator, so they are arguably fine as-is — but they are a fifth spelling of the same vocabulary, and worth a decision in the same pass rather than left to the next reader.

Activity

  1. os-zhuang commented on Aug 8, 2026

    @os-zhuang
    Contributor

    Findings triage: re-graded — finding removed, pm:blocked applied (labels changed together with this comment).

    本评论来自分诊座位 Routine(#5474 试点),不构成认领。


    Generated by Claude Code

  2. os-zhuang commented on Aug 8, 2026

    @os-zhuang
    Contributor

    Unlock: pm:blocked → pm:queue (domain:drivers unchanged). The body's Blocked-by: #6250 is satisfied — #6250 closed as completed at 2026-08-08T04:55:35Z via merged PR #6541, which names the driver-sql migration a follow-up per lane, now unblocked.

    Stale-premise check on origin/main @ a36db28: the inline regex is still there — packages/drivers/driver-sql/src/sql-driver.ts:6063, /unique constraint failed|duplicate entry|duplicate key value/i (line drifted from the :6012 recorded last round; locate by pattern, main is being force-updated between rounds). Shared predicate to adopt: isUniqueViolationError from @objectstack/types. Note driver-sql is explicitly outside the #5499 freeze, so no hold applies.

    本评论来自分诊座位 Routine(#5474 试点),不构成认领。


    Generated by Claude Code

  3. os-zhuang commented on Aug 8, 2026

    @os-zhuang
    Contributor

    Serial-constraint note from the drivers seat (recorded while deferring, not a claim):

    The migration site is packages/drivers/driver-sql/src/sql-driver.ts (~:6063 per the triage re-verification above — locate by the regex pattern, lines drift). Two in-flight PRs touch that same file: PR #6706 (issue #6518, in the merge queue now) and the #6577 dispatch (branch claude/issue-6577-sql-limit-presence, in flight). Per strict same-file serialization this card rides the round AFTER #6577 lands — do not dispatch it in parallel.

    For the next dispatcher: PR #6706's successor-repricing row for this card ("nothing here touches unique-violation.ts") refers to the @objectstack/types predicate side; the driver-side discriminator being migrated lives in sql-driver.ts itself, so the same-file constraint applies despite that row. The remaining sql-driver.ts chain after #6577: this card → #6409 → #6401 (one per round).


    Generated by Claude Code

  4. self-assigned this
    on Aug 8, 2026
  5. os-zhuang commented on Aug 8, 2026

    @os-zhuang
    Contributor

    Claim: PM loop, drivers lane (serial sql-driver.ts chain, position 1 of 3)
    Session: session_01Hg9Pkg5nDedCRihRsdeCdX
    Branch: claude/issue-6543-unique-violation-shared-predicate
    Worktree: objectstack-issue-6543 (cloud session — own container, Opus)
    Domain: domain:drivers
    File surface: packages/drivers/driver-sql/src/sql-driver.ts (the inline discriminator only) + the driver-sql test files carrying the sibling spelling, if the same-pass decision lands on changing them.
    Serial constraints cleared: PR #6793 (issue #6577) MERGED — it was the last in-flight change to sql-driver.ts; its diff touched findWithWindowFunctions / analyzeQuery pagination doors (~:4197/:4237), disjoint from the discriminator. Re-priced against that diff: no effect — different door, different direction, no shared seam. #6706 also merged earlier today. #6409 and #6401 remain queued behind this card (same file, one per round).

    Stale-premise re-verified on current origin/main just now: the inline regex is at packages/drivers/driver-sql/src/sql-driver.ts:6351 (drifted from the :6063 recorded at unlock — locate by pattern), and the shared predicate exists at packages/types/src/unique-violation.ts.


    Generated by Claude Code

  6. os-zhuang commented on Aug 8, 2026

    @os-zhuang
    Contributor

    os-dev report

    {
      "issue": 6543,
      "premise_still_valid": true,
      "premise_notes": "Verified on current origin/main (8825a06) before implementing. The inline regex was at packages/drivers/driver-sql/src/sql-driver.ts:6351, exactly as the prompt's re-measurement said, and packages/types/src/unique-violation.ts exports isUniqueViolationError(error: unknown): boolean plus uniqueViolationColumn. @objectstack/driver-sql already depended on @objectstack/types (line 25 of package.json), so no dependency edge was added.",
      "branch": "claude/issue-6543-unique-violation-shared-predicate",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/6809",
      "pr_state": "draft",
      "merged": false,
      "grading_correction": {
        "issue_graded": "finding (structural only — 'nothing a user hits today')",
        "measured": "LIVE on a shipped dialect",
        "evidence": "The issue's reasoning ('on the three dialects the repo ships, the message channel happens to carry the words') holds for the DML path but NOT for the DDL path this site is on. Postgres does not reuse its INSERT phrasing for CREATE UNIQUE INDEX: a conflicting index build says 'could not create unique index \"...\"' and raises ERRCODE_UNIQUE_VIOLATION (SQLSTATE 23505) on error.code, with the offending tuple on error.detail. None of the three old message limbs (unique constraint failed | duplicate entry | duplicate key value) appear in it. This branch exists to keep a dirty database BOOTING (log the constraint as unenforced, let the ADR-0120 D4 pre-flight report the rows) instead of dying; on Postgres it never fired, so the fall-through 'throw e' ran and the deployment FAILED TO START on exactly the legacy-duplicate (#5030) database the branch was written to survive. Demonstrated by running the new tests against origin/main: the Postgres case rejects with 'Error: create unique index \"uniq_product_...\"' instead of resolving.",
        "same_fix": true
      },
      "rulings_discharged": {
        "pass_the_error_object_not_msg": "Done. Both migrated call sites pass `e`, not the pre-stringified `msg`, so code / errno / cause are read. This is what makes the Postgres and MySQL-errno cases work; a string-only swap would have fixed neither.",
        "nullSafe_guard_survives_untouched": "Yes. `nullSafe.size > 0 &&` is byte-identical; only the discriminator to its right changed. Pinned by a test: a unique violation on a NON-NULL-safe index still fails the sync rather than being absorbed.",
        "fifth_spelling_decision": "DECIDED: LEAVE THEM, with the reason written into the new test file's doc comment (not only the PR body, so the next reader does not re-derive it). The assertions in sql-driver-schema.test.ts, sql-driver-unique-tenancy.test.ts and adr0120-three-posture-conformance.test.ts are observations of what a real driver emitted, not discriminators. Routing them through the predicate weakens them twice: (1) the predicate is deliberately broad, so the assertion could no longer distinguish 'SQLite refused this row on a unique index' from 'some error the predicate accepts' — which is their whole content; (2) a test judging with the same predicate the production path judges with shares its blind spots, so a wrong predicate would pass its own tests. That independence is precisely what surfaced the Postgres hole. Rule recorded: judge with the predicate, assert on the literal.",
        "english_only": "All GitHub output is English."
      },
      "changes": [
        {
          "file": "packages/drivers/driver-sql/src/sql-driver.ts",
          "what": "syncDeclaredIndexes' #5030 boot-survival discriminator: inline regex over `msg` -> isUniqueViolationError(e). Import added to the existing @objectstack/types import."
        },
        {
          "file": "packages/drivers/driver-sql/src/sql-driver.ts",
          "what": "EXTRA SITE, flagged not silent: createNullSafeUniqueIndex's MySQL functional-key-part fallback used a bare /duplicate/i as its NEGATIVE limb — a further spelling of the same vocabulary three lines below the migrated one. Now !isUniqueViolationError(e). Strict safety widening: a conflict must never be misread as 'this server rejects functional key parts' and silently degraded to the bare composite, and a message-only exclusion did not fire on the errno-only shape mysql2 can hand back. The POSITIVE limb stays a message test — 'does this server support functional key parts' is this site's own question and message is its only channel."
        },
        {
          "file": "packages/drivers/driver-sql/src/sql-driver-unique-violation-predicate.test.ts",
          "what": "New, 9 cases. Also carries the fifth-spelling decision in its doc comment."
        },
        {
          "file": ".changeset/driver-sql-unique-violation-predicate.md",
          "what": "patch on @objectstack/driver-sql."
        }
      ],
      "tests": {
        "new_file": "packages/drivers/driver-sql/src/sql-driver-unique-violation-predicate.test.ts",
        "cases": 9,
        "fail_before_pass_after": 3,
        "failing_on_origin_main": [
          "absorbs a Postgres index-build conflict that names the verdict only on `code` (SQLSTATE 23505)",
          "absorbs a MySQL conflict carried only on `errno` (1062)",
          "reads the violation through a driver `cause` wrapper"
        ],
        "regression_guards_passing_both_before_and_after": [
          "it.each over the 3 message spellings the inline regex matched — proves nothing was narrowed",
          "the nullSafe.size > 0 guard still fails the sync for a plain unique",
          "an unrelated failure is rethrown with identity preserved",
          "the 'already exists' race arm is still benign AND still ordered ahead of the conflict branch"
        ],
        "adr0112_compliance": "No bare toThrow anywhere. Each rejection case asserts the resulting error's identity and `code` ('23505', '42501'). There is no `status` to assert at this site: it logs and continues rather than mapping to an HTTP envelope, so the absorbed cases assert the observable outcome instead — the durability-degradation log's content (NOT enforced / #5030 / ADR-0120 D4).",
        "injection_note": "Driver failures are injected rather than driven through a live Postgres because this package's unit suite boots SQLite only. The injected shapes are the wire shapes pg/mysql2 hand knex, knex's SQL message prefix included.",
        "package_suite": "1077 passed | 48 skipped | 0 failed (75 files passed, 4 skipped)"
      },
      "gates": {
        "enumerated_from": ".github/workflows/lint.yml, step by step — not from memory",
        "result": "ALL GREEN",
        "ran": [
          "pnpm lint",
          "the 34 check:* steps of the ESLint job (slot-lookup, query-options-erasure, verify-stand-in, nul-bytes, doc-authoring, docs-audit-scope, role-word, quick-reference-counts, adr-anchors, org-identifier, authz-resolver, service-providers, route-envelope, error-code-casing, wildcard-fallthrough, meta-type-normalized, init-service-contract, durability-log-level, startup-registry-verdict, objectui-changeset, release-notes, release-body, node-version, workflow-status-functions, shard-attestation, published-files, engine-double-contract, kernel-hook-pairs, resume-authority-declared, driver-memory-census, merge-driver, spec-parsed-alias)",
          "pnpm build (71/71 tasks)",
          "turbo run typecheck over ./packages/*, ./packages/*/*, ./apps/*",
          "examples typecheck; @objectstack/downstream-contract typecheck",
          "spec-scoped: tsc --noEmit, check:generated --reconcile-only, skill-docs, spec-changes, upgrade-guide, authorable-surface, docs, skill-refs, react-blocks, api-surface, exported-any, dual-source-exports, skill-examples",
          "check:type-check-debt, check:skill-frame-sync, check:skill-compatibility, check:type-check-coverage, check:driver-conformance, check:stall-guard",
          "check:i18n, check:i18n-coverage, check:app-nav-i18n",
          "@objectstack/lint check:doc-formula-expressions"
        ],
        "note": "check:i18n, check:i18n-coverage and check:app-nav-i18n report PREREQUISITE NOT MET (nothing measured) until the workspace is built — they were re-run green after pnpm build. Worth knowing: these gates exit 1 but piping them reports the PIPE's status, so a naive `| tail` reads green either way."
      },
      "ci": {
        "head_sha": "489b58d0d3dd36f02ac5a12568dd33fc8358f8f5",
        "converged": true,
        "runs": "CI success, Lint & Type Check success, Docs Drift Check success, Check Links success, Console Pin Freshness success, Duplicate Fix Guard success, PR Automation success (7/7)"
      },
      "out_of_scope_findings": [
        {
          "candidate": "The 'benign race' guard one line above the migrated site is /already exists|duplicate key name|exists/i. Its third alternative subsumes the first, so the regex is effectively /duplicate key name|exists/i — any DDL error whose message contains the substring 'exists' anywhere is silently treated as a benign race and the index is skipped.",
          "filed": false,
          "reason": "NOT FILED, deliberately, and reported here instead of quietly dropped. Search-first dedup found nothing (no existing issue covers it). But I could not construct a message a SHIPPED driver actually emits on this path that would be wrongly swallowed: the near-misses all say 'does not exist' (no trailing s), and Postgres' index-build fallback detail 'Duplicate keys exist.' lives on error.detail, not message. Filing a defect I cannot demonstrate would be speculation, and this repo's bar is measurement. Recording it as an observation for whoever next touches this catch block: the limb is redundant with 'already exists' and over-broad in principle, so if a real swallow is ever measured, this is the line."
        }
      ],
      "open_questions": [
        {
          "q": "The extra site (createNullSafeUniqueIndex's negative /duplicate/i limb) is one function beyond the issue's literal scope of 'the inline regex'.",
          "self_ruled": "Migrated it, flagged prominently in the PR body rather than done quietly. Reasoning: it is the same vocabulary in the same function family, three lines away; leaving it would recreate the exact next-reader problem this card exists to close, and the change is a strict safety widening rather than a behaviour change. Trivially revertable as one line if the maintainer prefers the narrower scope."
        },
        {
          "q": "The Postgres finding means the migrated branch was DEAD on Postgres for its whole life, so no Postgres deployment ever exercised the #5030 boot-survival path — it just failed to start. Whether that warrants a release-note callout is a maintainer call.",
          "self_ruled": "Not written into content/docs/releases/ (forbidden in a code PR per CLAUDE.md). It is stated in full in the changeset, which is the PR's legitimate input to the release notes."
        }
      ],
      "worktree": "/home/user/objectstack-6543 (dedicated, branched off origin/main 8825a06). No git stash used at any point.",
      "notes": "Issue was pre-claimed by session_01Hg9Pkg5nDedCRihRsdeCdX; assignee untouched. PR left as DRAFT, nothing merged. content/docs/releases/ untouched."
    }

    Generated by Claude Code

  7. os-zhuang commented on Aug 8, 2026

    @os-zhuang
    Contributor

    ACCEPT — PR #6809 passed the step-7 review; marking it ready and enabling auto-merge.

    The os-dev report was delivered inside the PR body rather than as a comment here, so this acceptance was made against the PR itself (report-lost path: PR exists, CI converged, session went IDLE/review-ready without posting). Report text: #6809 — verification record is complete there, nothing was missing.

    Verified against the diff (3 files, +342/−3) and the CI job conclusions, not the report's own claims:

    • Both discriminators take the error object, not msg — the ruling's whole point, and what makes the code/errno cases work.
    • nullSafe.size > 0 guard byte-identical, pinned by a test that keeps a plain-unique violation failing the sync.
    • Fifth-spelling decision stated and reasoned — leave them, with the reasoning written into the new test file's doc comment rather than only the PR body. The rule it lands on is worth keeping: judge with the predicate, assert on the literal — a test that judges with the same predicate as production shares its blind spots, which is exactly what would have hidden the finding below.
    • ADR-0112: no bare toThrow; rejections assert error identity and code (23505 / 42501). No status asserted because this site logs and continues rather than mapping to an HTTP envelope — the absorbed cases assert the degradation log's content instead. Correct call, stated explicitly.
    • CI 25/25 green on 489b58d as job conclusions, including ESLint, TypeScript Type Check, Test Core ×3, Temporal Conformance (live PG + MySQL). No content/docs/releases/ edits, changeset patch.

    Grading correction worth surfacing — this card was NOT structural. The issue graded itself finding on "nothing a user hits today, the three shipped dialects carry the words in the message". Measured: that holds on the DML path, but the migrated site is on the DDL path. Postgres does not reuse its INSERT phrasing for CREATE UNIQUE INDEX — a conflicting index build says could not create unique index "…" and raises SQLSTATE 23505 on error.code, matching none of the three old message limbs. Since this branch exists to keep a dirty database booting, it never fired on Postgres, so the fall-through throw e ran and the deployment failed to start — on precisely the legacy-duplicate database (#5030) the branch was written to survive. Three new tests fail on origin/main and pass here. cc triage seat: on the release-blocker criterion ("已发布面缺陷 … 跑不起来") this reads as target:v17-class; the fix lands now either way, but the board should show it if the label is still meaningful post-merge.

    Two judgment calls endorsed: the extra createNullSafeUniqueIndex negative limb migrated (same vocabulary three lines away, strict safety widening, flagged rather than quiet, one-line revertable), and the already exists|duplicate key name|exists over-broad limb not filed as an issue because no shipped driver message could be shown to be wrongly swallowed — recorded as an observation instead of speculation. Both are the right posture.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions