Two more from Macroscope review: - `ORDER BY COALESCE(...)` throws on a source down to one recency column. SQLite requires at least two arguments, so the pre-017 schema the column filter exists to support was exactly the case that aborted. Emit the column directly when only one survives the filter. - `seedDevDatabase` accepted non-positive limits. Both are bound into `LIMIT ?`, where SQLite reads a negative as "no limit" — a caller bypassing the CLI flags silently got a full-table copy instead of the bounded one the options contract promises. Rejected before the target is touched, alongside the existing upper-bound check. Both regression tests confirmed failing without their fix.
Showing the changes to scripts (2 of 2 files). Show all
Unified · Split
| @@ -571,6 +571,40 @@ describe("seedDevDatabase", () => { · 570 unmodified lines | ||
| 571 | 571 | ); |
| 572 | 572 | }); |
| 573 | 573 | |
| 574 | + // SQLite reads a negative LIMIT as "no limit", so an unchecked value turns a | |
| 575 | + // capped copy into a full-table one rather than failing. | |
| 576 | + for (const limits of [ | |
| 577 | + { threadLimit: 0, activityLimit: 10 }, | |
| 578 | + { threadLimit: -1, activityLimit: 10 }, | |
| 579 | + { threadLimit: 5, activityLimit: 0 }, | |
| 580 | + { threadLimit: 5, activityLimit: -1 }, | |
| 581 | + ]) { | |
| 582 | + it(`refuses threadLimit ${String(limits.threadLimit)} / activityLimit ${String(limits.activityLimit)}`, () => { | |
| 583 | + withDatabases(({ source, target }) => { | |
| 584 | + assert.throws( | |
| 585 | + () => | |
| 586 | + seedDevDatabase({ | |
| 587 | + sourceDbPath: source, | |
| 588 | + targetDbPath: target, | |
| 589 | + ...limits, | |
| 590 | + seededAt: SEEDED_AT, | |
| 591 | + }), | |
| 592 | + DevSeedError, | |
| 593 | + ); | |
| 594 | + | |
| 595 | + // Rejected before the destructive transaction opened. | |
| 596 | + const projects = query<{ project_id: string }>( | |
| 597 | + target, | |
| 598 | + "SELECT project_id FROM projection_projects", | |
| 599 | + ); | |
| 600 | + assert.deepStrictEqual( | |
| 601 | + projects.map((row) => row.project_id), | |
| 602 | + ["stale"], | |
| 603 | + ); | |
| 604 | + }); | |
| 605 | + }); | |
| 606 | + } | |
| 607 | + | |
| 574 | 608 | // A caller that bypasses the CLI flag would otherwise discover the ceiling |
| 575 | 609 | // only after the target had been emptied and the copy rolled back. |
| 576 | 610 | it("refuses a thread limit above the ceiling before touching the target", () => { |
| @@ -599,6 +633,75 @@ describe("seedDevDatabase", () => { · 22 unmodified lines | ||
| 599 | 633 | }); |
| 600 | 634 | }); |
| 601 | 635 | |
| 636 | + // Down to one recency column, the ordering expression cannot be a COALESCE: | |
| 637 | + // SQLite requires at least two arguments and throws otherwise, which would | |
| 638 | + // abort the seed on exactly the old schema the column filter exists to serve. | |
| 639 | + it("reads a source with only one recency column", () => { | |
| 640 | + const directory = NodeFS.mkdtempSync(NodePath.join(NodeOS.tmpdir(), "t3-dev-seed-1col-")); | |
| 641 | + const source = NodePath.join(directory, "source.sqlite"); | |
| 642 | + const target = NodePath.join(directory, "target.sqlite"); | |
| 643 | + try { | |
| 644 | + const database = new NodeSqlite.DatabaseSync(source); | |
| 645 | + // created_at alone: no updated_at, no archived_at, no latest_user_message_at. | |
| 646 | + database.exec(`CREATE TABLE projection_threads ( | |
| 647 | + thread_id TEXT PRIMARY KEY, project_id TEXT NOT NULL, title TEXT NOT NULL, | |
| 648 | + created_at TEXT NOT NULL, deleted_at TEXT)`); | |
| 649 | + // Matches the target's project columns; only the thread recency columns | |
| 650 | + // are being varied here. | |
| 651 | + database.exec(`CREATE TABLE projection_projects ( | |
| 652 | + project_id TEXT PRIMARY KEY, title TEXT NOT NULL, workspace_root TEXT NOT NULL, | |
| 653 | + scripts_json TEXT NOT NULL, created_at TEXT NOT NULL, updated_at TEXT NOT NULL, | |
| 654 | + deleted_at TEXT)`); | |
| 655 | + database.exec(`CREATE TABLE projection_state (projector TEXT PRIMARY KEY, | |
| 656 | + last_applied_sequence INTEGER NOT NULL, updated_at TEXT NOT NULL)`); | |
| 657 | + database | |
| 658 | + .prepare(`INSERT INTO projection_projects VALUES ('p1','P','/repo','[]',?,?,NULL)`) | |
| 659 | + .run(SEEDED_AT, SEEDED_AT); | |
| 660 | + for (const thread of [ | |
| 661 | + { id: "old", at: "2026-07-10T00:00:00.000Z" }, | |
| 662 | + { id: "new", at: "2026-07-20T00:00:00.000Z" }, | |
| 663 | + ]) { | |
| 664 | + database | |
| 665 | + .prepare(`INSERT INTO projection_threads VALUES (?,?,?,?,NULL)`) | |
| 666 | + .run(thread.id, "p1", thread.id, thread.at); | |
| 667 | + } | |
| 668 | + database.close(); | |
| 669 | + | |
| 670 | + // The target mirrors the source's thread columns: this test varies the | |
| 671 | + // recency columns, and a target demanding one the source lacks would fail | |
| 672 | + // on the NOT NULL rather than on the ordering expression under test. | |
| 673 | + const targetDatabase = new NodeSqlite.DatabaseSync(target); | |
| 674 | + targetDatabase.exec(`CREATE TABLE projection_threads ( | |
| 675 | + thread_id TEXT PRIMARY KEY, project_id TEXT NOT NULL, title TEXT NOT NULL, | |
| 676 | + created_at TEXT NOT NULL, deleted_at TEXT)`); | |
| 677 | + targetDatabase.exec(`CREATE TABLE projection_projects ( | |
| 678 | + project_id TEXT PRIMARY KEY, title TEXT NOT NULL, workspace_root TEXT NOT NULL, | |
| 679 | + scripts_json TEXT NOT NULL, created_at TEXT NOT NULL, updated_at TEXT NOT NULL, | |
| 680 | + deleted_at TEXT)`); | |
| 681 | + targetDatabase.exec(`CREATE TABLE projection_state (projector TEXT PRIMARY KEY, | |
| 682 | + last_applied_sequence INTEGER NOT NULL, updated_at TEXT NOT NULL)`); | |
| 683 | + targetDatabase.close(); | |
| 684 | + | |
| 685 | + const summary = seedDevDatabase({ | |
| 686 | + sourceDbPath: source, | |
| 687 | + targetDbPath: target, | |
| 688 | + threadLimit: 1, | |
| 689 | + activityLimit: 10, | |
| 690 | + seededAt: SEEDED_AT, | |
| 691 | + }); | |
| 692 | + | |
| 693 | + assert.equal(summary.threads, 1); | |
| 694 | + // Ordering still applies: the newer thread is the one kept. | |
| 695 | + const titles = query<{ title: string }>(target, "SELECT title FROM projection_threads"); | |
| 696 | + assert.deepStrictEqual( | |
| 697 | + titles.map((row) => row.title), | |
| 698 | + ["new"], | |
| 699 | + ); | |
| 700 | + } finally { | |
| 701 | + NodeFS.rmSync(directory, { recursive: true, force: true }); | |
| 702 | + } | |
| 703 | + }); | |
| 704 | + | |
| 602 | 705 | it("reports a source with nothing to copy", () => { |
| 603 | 706 | withDatabases( |
| 604 | 707 | ({ source, target }) => { |
| @@ -321,6 +321,21 @@ export function seedDevDatabase(options: DevSeedOptions): DevSeedSummary { · 320 unmodified lines | ||
| 321 | 321 | "Each thread costs one SQLite bound variable, and that is the engine's limit.", |
| 322 | 322 | ); |
| 323 | 323 | } |
| 324 | + // Both limits are bound straight into `LIMIT ?`, where SQLite reads a | |
| 325 | + // negative value as "no limit" — so a caller bypassing the CLI flags turns a | |
| 326 | + // capped copy into a full-table one, silently. The flags already reject this; | |
| 327 | + // enforcing it here keeps the options contract true of the function itself. | |
| 328 | + for (const [name, value] of [ | |
| 329 | + ["threadLimit", options.threadLimit], | |
| 330 | + ["activityLimit", options.activityLimit], | |
| 331 | + ] as const) { | |
| 332 | + if (!Number.isSafeInteger(value) || value < 1) { | |
| 333 | + throw new DevSeedError( | |
| 334 | + `${name} must be a positive integer, got ${String(value)}`, | |
| 335 | + "SQLite reads a negative LIMIT as unbounded, which would copy the whole table.", | |
| 336 | + ); | |
| 337 | + } | |
| 338 | + } | |
| 324 | 339 | |
| 325 | 340 | let source: NodeSqlite.DatabaseSync; |
| 326 | 341 | try { |
| @@ -368,6 +383,14 @@ export function seedDevDatabase(options: DevSeedOptions): DevSeedSummary { · 41 unmodified lines | ||
| 368 | 383 | const recencyColumns = ["latest_user_message_at", "updated_at", "created_at"].filter((column) => |
| 369 | 384 | hasColumn(source, "projection_threads", column), |
| 370 | 385 | ); |
| 386 | + // SQLite's COALESCE requires at least two arguments, so a source down to a | |
| 387 | + // single recency column — the pre-migration-017 case this filter exists to | |
| 388 | + // support — would throw rather than degrade. `created_at` is NOT NULL from | |
| 389 | + // migration 005 on, so there is always at least one. | |
| 390 | + const recencyOrder = | |
| 391 | + recencyColumns.length > 1 | |
| 392 | + ? `COALESCE(${recencyColumns.join(", ")})` | |
| 393 | + : (recencyColumns[0] ?? "rowid"); | |
| 371 | 394 | const activeFilters = ["deleted_at", "archived_at"] |
| 372 | 395 | .filter((column) => hasColumn(source, "projection_threads", column)) |
| 373 | 396 | .map((column) => `${column} IS NULL`); |
| @@ -376,7 +399,7 @@ export function seedDevDatabase(options: DevSeedOptions): DevSeedSummary { · 2 unmodified lines | ||
| 376 | 399 | .prepare( |
| 377 | 400 | `SELECT thread_id FROM projection_threads |
| 378 | 401 | ${activeFilters.length > 0 ? `WHERE ${activeFilters.join(" AND ")}` : ""} |
| 379 | − ORDER BY COALESCE(${recencyColumns.join(", ")}) DESC | |
| 402 | + ORDER BY ${recencyOrder} DESC | |
| 380 | 403 | LIMIT ?`, |
| 381 | 404 | ) |
| 382 | 405 | .all(options.threadLimit) as Array<{ thread_id: string }> |