Skip to content

Duplicate migration numbers never affect a score: the rules are gated to SQLite/D1 at weight 0 #66

Description

@serge-ivo

Rewritten 2026-08-11 against main @ d1c5314. The original body proposed six contract/data-integrity checks as a new category. Verifying them against the code: one already ships and is switched off, one is plausible but unspecified, and four are not statically decidable without a config surface that does not exist yet. This issue is now scoped to the shippable core — making the migration rules that already exist actually run and actually count. The undecidable items are preserved verbatim under Not yet specifiable with the blocker for each.

Problem

Migration discipline is the one contract/data-integrity rule VCQA can already prove from static evidence — and almost no scanned project ever sees it. A repo can ship db/migrations/0003_add_users.sql and db/migrations/0003_add_orgs.sql (ambiguous apply order, the classic "works on my machine, corrupts staging" bug) and get a clean VCQA report with no finding at all.

Two independent gates suppress it:

  1. The component detector that opens the gate is narrower than the runner behind it, so common migration layouts never trigger it.
  2. Even when the gate opens, the check carries weight 0, so #65's delivered scoring contract renders every finding as advisory with no score impact.

What already ships

cli/src/runners/sqlite-d1.ts:330-372 already implements three migration rules — the original body's "DB migrations and checked-in schema/snapshot are consistent" bullet, minus the snapshot half:

// ── Migration discipline ──
const dirs = migrationDirs(cwd, workspace);
  • migration-unnumbered (sqlite-d1.ts:346, warning) — "Some migrations in <dir> are not numbered — ordering depends on filename sort, which is fragile"
  • migration-duplicate (sqlite-d1.ts:355, error) — "Duplicate migration number(s) N in <dir> — apply order is ambiguous"
  • migration-unsafe-drop (sqlite-d1.ts:367, warning) — "DROP TABLE without IF EXISTS in <f> — a partially-applied migration set cannot be re-run"

migrationDirs (sqlite-d1.ts:177-195) reads migrations/, db/migrations/ and drizzle/migrations/ at the repo root and in every workspace package. None of these three rules touches SQLite or D1 semantics — they are filename and SQL-text rules that hold for Postgres, MySQL, Drizzle, Prisma and Rails-style migration sets equally.

Gate 1 — the detector only recognises one of the three layouts the runner reads

cli/src/detect.ts:43-50 adds the sqlite-d1 component from a migration directory in exactly one place:

const mig = join(dir, "migrations");
if (!found.has("sqlite-d1") && existsSync(mig)) {
    if (readdirSync(mig).some((f) => f.endsWith(".sql"))) found.add("sqlite-d1");

The only other path to the component is a wrangler config containing d1_databases (detect.ts:37). So a project whose migrations live in db/migrations/ or drizzle/migrations/ — both of which migrationDirs knows how to read — never gets the component, and cli/src/core.ts:217-238 skips the whole check centrally with scoreMode: "not-applicable". The migration code never executes.

Gate 2 — weight 0 makes every finding advisory

schema/src/check-meta.ts:490-497:

"sqlite-d1": {
    category: "Security",
    priority: "critical",
    weight: 0,
    appliesTo: { component: ["sqlite-d1"] },

cli/src/core.ts:378 maps that to advisory:

if (details.synthetic || meta.weight === 0 || declaredScoreMode === "available-unscored") return "available-unscored";

and cli/src/score.ts:19-20 drops it from the composite. A duplicate migration number is emitted at severity: "error" and moves the score by zero points.

What to do

Ordered cheapest-first; each step ships on its own.

1. Widen the component detector to match the runner (one function, no taxonomy change).
In detect.ts:43, iterate the same three directory names migrationDirs uses instead of just migrations. This alone makes the three existing rules fire on Drizzle and db/migrations layouts. No weight change, no new check, no downstream doc churn. Ships today.

2. Extract the migration rules into their own stack-blind check.
Move sqlite-d1.ts:177-195 and sqlite-d1.ts:330-372 into cli/src/runners/migrations.ts exporting runMigrations(cwd, workspace, inventory). Register it in the allRunners array in cli/src/core.ts. Add a migrations entry to CHECK_META with no appliesTo — per the stack-gating rule in cli/CLAUDE.md:128-137 a check is either gated or stack-blind, and these rules mention no framework. Remove the migration block and the migrations detail key from sqlite-d1.ts; keep migrationCount out of its scoring denominator (sqlite-d1.ts:380) so the SQL-injection score is no longer diluted by migration file count.

3. Give it real weight and rebalance.
cli/src/check-meta.test.ts:57-60 asserts weights sum to exactly 100, so the new weight must come out of an existing check. Which one is a maintainer call — see Open questions.

4. Extend the rule set only after 1–3 are green. Candidates, in confidence order: migration files edited after being applied (git-history based, needs a decision on whether the analyzer may shell out to git log); numbering gaps; DROP COLUMN / NOT NULL without a default in a non-newest migration.

Alternatives considered and rejected

  • Just give sqlite-d1 a nonzero weight. Rejected: that also makes the injection, N+1 and SELECT * rules score-bearing, which #65 deliberately settled as advisory for stack-gated checks, and it leaves the migration rules gated behind SQLite detection — the actual complaint here.
  • Add a new Contracts category for this check. Rejected: the category name is currently hardcoded in five places (see CHECK_META is not the only taxonomy: four hardcoded copies leave 12 checks on no category page #61), one of which is an allowlist assertion at cli/src/check-meta.test.ts:51. A new category for a single check is not worth five edits. Put migrations in an existing category and let CHECK_META is not the only taxonomy: four hardcoded copies leave 12 checks on no category page #61 decide where it finally lives.
  • Parse the SQL with a real parser instead of regex. Rejected for this scope: all three shipping rules are filename-level or single-token text rules and already produce true positives. A parser is the right answer for the schema-drift comparison in step 4, not for numbering.
  • Keep everything inside sqlite-d1 and special-case the gate. Rejected: cli/CLAUDE.md:128-137 explicitly rejects diffs where a runner re-implements stack gating. The gate must stay central in core.ts.

Acceptance criteria

  • A project with db/migrations/0003_a.sql and db/migrations/0003_b.sql and no wrangler config reports migration-duplicate at severity: "error", and that finding lowers the composite score.
  • The same project's report shows sqlite-d1 as not-applicable (no SQLite/D1 detected) while migrations runs — the two are independently gated.
  • A project with no migration directory anywhere reports migrations as skipped/not-applicable with zero issues, not F/0 (#52's contract).
  • cli/src/check-meta.test.ts "weights sum to 100" still passes.
  • cli/src/cli.test.ts:56 and :290 still pass — they assert report.checks.length === Object.keys(CHECK_META).length + SYNTHETIC_CHECK_COUNT, so registry and metadata must move together.
  • Fixtures cover: numbered-contiguous (clean), duplicate number, unnumbered mixed with numbered, DROP TABLE without IF EXISTS, and a workspace package with its own drizzle/migrations. Follow the makeProject tmpdir pattern in cli/src/runners/sqlite-d1.test.ts:10-19.

Not yet specifiable

These four came from the original body. Each is undecidable from static evidence alone without project-declared configuration that VCQA cannot currently accept. Do not implement them as heuristics — a wrong answer on an authz rule is worse than no answer. They are recorded here so the intent is not lost.

The missing surface for all four is #30 (per-analyzer settings passed through AnalyzerContext and recorded in the report). Today .vcqa.json only supports enabled and ignore per check.

Proposal Why it is not decidable today Config it would need
Route handler schemas match exported client contracts / generated SDKs Requires knowing which module is the server contract and which is the client, across arbitrary frameworks and monorepo boundaries. No convention is common enough to infer. Declared route roots, client SDK entry points, and the codegen command that is supposed to keep them in sync.
Tenant/account/user scoping present on repository queries "Scoped" is project-specific: the column may be tenant_id, org_id, account, or implicit in a base repository class. Absence of a literal is not absence of scoping. The tenant column name(s), the repository/data-access module paths, and an allowlist of deliberately global queries.
Mutating endpoints enforce authz and validation before writes Needs a call graph from handler entry to write, plus knowledge of which function is the authz check. Middleware-based authz is invisible at the handler. The project's authz helper name(s)/import paths and its middleware registration point.
Webhook/event payloads versioned and validated No shared convention for what "versioned" looks like (envelope field, URL path, header). Declared event schema location and versioning convention.

The fifth original bullet — "API responses are runtime-validated at boundaries where external data enters" — is closer to decidable but still unspecified. cli/src/runners/best-practices.ts:581 already detects the presence of a validation library (zod/joi/yup/class-validator/ajv/superstruct/valibot) in dependencies; nothing checks whether it is applied at a boundary. Turning "boundary" into a testable definition is the open design work, not the detection. It needs its own issue with a written definition before it is picked up.

Constraints the implementer should know

  • Stack-gating rule (cli/CLAUDE.md:128-137): no stack.framework === ... branch inside a stack-blind runner. Gate in CHECK_META.appliesTo or not at all.
  • Trunk-based (cli/CLAUDE.md:21-45): commit to main, no PR.
  • Schema is upstream: CHECK_META lives in vibecodeqa/schema, and cli/src/check-meta.ts is a re-export shim. A metadata change needs a schema release before the CLI can consume it.
  • Fixtures must not name downstream repos. cli/src/downstream-fixture-guard.test.ts:23-40 fails the build if production scanner code contains downstream repo-specific names. This issue originated from a PAGS observation; do not carry PAGS paths into fixtures.
  • #73 (closed) still governs: do not close a scanner-fix issue until the fix is published to npm and validated through the published CLI.

Downstream — what else must move

Adding a check moves the public check count, which is currently inconsistent across the org. After step 2 the registry goes 38 → 39 and CHECK_META 37 → 38.

  • cli/CLAUDE.md:155 ("37 canonical checks across 7 categories") and the weight table at :163-170.
  • vibecodeqa/docs/docs/checks.md — the inventory table plus the "Last verified against 0.54.4 on 2026-08-08" callout at line 9.
  • The 11 marketing files still claiming 34 (vibecodeqa/index.html, scan.html, skills.html, tools.html, stacks/flutter.html, compare/*.html, docs/zensical.toml), plus app/src/App.tsx:122, mcp/README.md:58, ops/ARCHITECTURE.md:140.

The full count-drift inventory and the fix for it are tracked in #61 — that issue is where the count should become derived rather than typed. Do not open a separate count issue.

Open questions for the maintainer

  1. Where does migrations' weight come from? Weights must sum to 100. Reasonable donors: Quality's 30 (it has 16 members, five of which are worth 1 point each), or Security's 16. A duplicate migration number is a data-integrity bug, which argues for Security; the code currently files sqlite-d1 under Security already.
  2. Category placement — Security (matches sqlite-d1 today) or a new bucket introduced by CHECK_META is not the only taxonomy: four hardcoded copies leave 12 checks on no category page #61. Recommend Security now, revisit under CHECK_META is not the only taxonomy: four hardcoded copies leave 12 checks on no category page #61.
  3. May the analyzer shell out to git log for the "edited after applied" rule in step 4? cli/src/runners/git-hygiene.ts already reads git state, so there is precedent, but it makes the rule non-deterministic in shallow CI clones.

Related

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    analyzer-platformAnalyzer engine, registry, contracts, and normalized resultsenhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions