Skip to content

Phase 1: synthetic vault data - #2

Merged
MachineLearning-Nerd merged 9 commits into
mainfrom
phase-1-vault-data
Aug 25, 2026
Merged

MachineLearning-Nerd merged 9 commits into
mainfrom
phase-1-vault-data

Conversation

@MachineLearning-Nerd

Copy link
Copy Markdown
Owner

What

Synthetic support-ticket dataset behind an internal storage module: STRICT SQLite schema with typed column-sensitivity labels, a deterministic 62-row seed (fixed-seed PRNG) containing one canary row and one below-threshold small-cell group (APAC/2026-W33, k=2), and a closure-encapsulated node:sqlite database whose only exports are narrow scalar queries.

Why

Phase 1 of the build plan: the vault is the enforcement boundary everything later builds on. Two invariants are locked by tests here: no raw-row accessor is exported from the storage module (module-surface test), and no exported query accepts a caller-chosen probe against a sensitive column — a parameterized substring/equality check would be a boolean oracle that leaks row contents bit by bit, so canary presence is checked by a parameterless hasCanaryRow().

Boundary notes for later phases: seed.js is itself a raw-row source and must never be imported by the agent-facing tool layer (only openVaultDatabase); groupSize exists for suppression logic and tests.

Evidence

  • 12/12 tests green, typecheck + lint clean; all tests deterministic and LLM-free
  • Seed pinned to a literal fixture row + length so cross-process drift fails the suite
  • Internal review (2 passes) findings all fixed in 92910d9 and 2b9fe61: sensitive-column query oracle closed, seeding rollback added, module-export surface locked, k>=3 asserted over the full week x region cross product, DDL verified via pragma_table_info, unused database location parameter removed

Copilot AI lite review requested due to automatic review settings August 25, 2026 06:22
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Phase 1: Add synthetic vault dataset with sealed SQLite accessors

✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Add STRICT SQLite ticket schema with per-column sensitivity labeling.
• Seed a deterministic 62-row synthetic dataset with a canary and one k=2 small cell.
• Expose a closure-encapsulated vault DB with only narrow scalar queries, enforced by tests.
Diagram

graph TD
  T["src/vault/vault.test.ts"] --> D["src/vault/database.ts"] --> S["src/vault/schema.ts"] --> Q[("SQLite :memory:")]
  D --> R["src/vault/seed.ts"]
  T --> S
  T --> R

  subgraph Legend
    direction LR
    _test["Test file"] ~~~ _mod["Module/file"] ~~~ _db[("SQLite database")]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Ship a prebuilt SQLite fixture file
  • ➕ Faster startup; no per-test seeding cost
  • ➕ Schema+data are literally identical across runs without relying on PRNG correctness
  • ➖ Harder to review/modify in code review (binary/opaque diffs)
  • ➖ Adds file management complexity and potential for accidental distribution of raw rows beyond the vault boundary
2. Use a higher-level ORM/query builder
  • ➕ Stronger compile-time query construction; potential ergonomics as queries grow
  • ➕ Easier migrations if schema evolves significantly
  • ➖ Larger dependency surface in a boundary-critical module
  • ➖ May make it easier to accidentally add row-returning accessors or richer probing APIs
3. Expose a generic "countWhere(column, value)" API with allowlist
  • ➕ More flexible for later phases; fewer bespoke methods
  • ➖ Even with allowlists, increases risk of reintroducing boolean-oracle style probes on sensitive fields
  • ➖ Harder to enforce invariants via a simple export-surface test

Recommendation: Keep the current approach (sealed DatabaseSync handle + narrowly-scoped scalar methods + deterministic seed) because it optimizes for boundary safety and testable invariants in Phase 1. If performance becomes a concern later, consider a prebuilt fixture DB, but only if it can remain vault-internal and still preserve the “no raw rows” and “no sensitive probes” guarantees.

Files changed (4) +283 / -0

Enhancement (3) +177 / -0
database.tsAdd sealed in-memory SQLite vault with narrow scalar queries +63/-0

Add sealed in-memory SQLite vault with narrow scalar queries

• Implements openVaultDatabase() that builds a STRICT SQLite database in-memory, seeds it transactionally (with rollback on failure), and returns only scalar query methods. Encapsulates the DatabaseSync handle in a closure and avoids any exported API that would enable caller-chosen probing of sensitive columns.

src/vault/database.ts

schema.tsDefine ticket schema DDL and typed column sensitivity labels +41/-0

Define ticket schema DDL and typed column sensitivity labels

• Introduces a strict tickets table DDL and a Sensitivity model that classifies columns as sensitive, safe dimensions, or metric sources. Exposes derived column lists for validation and downstream policy logic.

src/vault/schema.ts

seed.tsAdd deterministic synthetic ticket seed with canary and k<3 small-cell case +73/-0

Add deterministic synthetic ticket seed with canary and k<3 small-cell case

• Adds a typed TicketRow model and deterministic mulberry32 PRNG seeding for stable cross-run fixtures. Generates week/region bulk groups with k>=3, one explicit APAC small-cell group with size 2, and a single instruction-shaped canary row for boundary-leak detection.

src/vault/seed.ts

Tests (1) +106 / -0
vault.test.tsTest schema labels, seed determinism, canary presence, and export surface +106/-0

Test schema labels, seed determinism, canary presence, and export surface

• Adds Vitest coverage to ensure every column is labeled, the DDL matches labeled columns plus internal id, and the seed is deterministic and pinned to a fixture row/length. Validates canary and small-cell invariants, and locks down that neither the returned DB object nor the module exports expose raw-row accessors or sensitive-column probe APIs.

src/vault/vault.test.ts

@qodo-code-review

qodo-code-review Bot commented Aug 25, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Unclosed prepared statements ✗ Dismissed 🐞 Bug ☼ Reliability
Description
openVaultDatabase() creates StatementSync objects (insert and per-call count queries) and never
closes them, relying on GC timing; this can keep the SQLite connection in a delayed-close/zombie
state and leak statement resources under repeated calls. This is especially risky because
DatabaseSync.close() wraps sqlite3_close_v2(), which defers deallocation until statements are
finalized, so close() may not actually free resources deterministically.
Code

src/vault/database.ts[R20-24]

+  const insert = db.prepare(
+    `INSERT INTO tickets
+       (customer_id, email, phone, free_text, week, region, category, resolution_hours)
+     VALUES (?, ?, ?, ?, ?, ?, ?, ?)`,
+  );
Evidence
The PR prepares statements via db.prepare() but never calls StatementSync.close()/dispose. Because
DatabaseSync.close() is a wrapper around sqlite3_close_v2(), unfinalized statements can keep the DB
in a deferred-cleanup state until GC finalizes them, making resource cleanup non-deterministic.

src/vault/database.ts[20-24]
src/vault/database.ts[46-49]
src/vault/database.ts[51-62]
🌐 Node’s sqlite docs state database.close() is a wrapper around sqlite3_close_v2(), which is designed to work with GC’d host languages and can defer cleanup.
🌐 SQLite docs explain sqlite3_close_v2() returns OK even with unfinalized statements, but marks the connection as a zombie and deallocates it only after statements are finalized.

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`openVaultDatabase()` prepares SQLite statements but never closes them. This can cause resource leaks and non-deterministic cleanup because `database.close()` wraps `sqlite3_close_v2()` and will defer final cleanup until all statements are finalized.

### Issue Context
- The insert statement is prepared once and is never explicitly closed.
- The `count()` helper prepares a new statement on every call and also never closes it.
- Deterministic teardown matters for tests, repeated open/close cycles, and long-running processes.

### Fix Focus Areas
- src/vault/database.ts[16-62]

### Implementation notes
- Close the `insert` statement after seeding (e.g., in a `finally` after COMMIT/ROLLBACK).
- Avoid preparing new statements inside `count()` without closing them. Options:
 - Prepare the few fixed statements once (rowCount, canary check, groupSize) and close them inside `close()`.
 - Or, inside `count()`, do `const stmt = db.prepare(sql); try { ... } finally { stmt.close(); }`.
- Ensure `close()` closes any cached statements before closing the database.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Mutable sensitivity exports ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
COLUMN_SENSITIVITY and the derived arrays (SENSITIVE_COLUMNS/SAFE_DIMENSIONS/METRIC_SOURCE_COLUMNS)
are exported as mutable objects, so any consumer can mutate them at runtime and silently change
classification behavior. This makes later enforcement logic brittle because a single accidental
mutation can re-label columns without type errors or test failures in other suites.
Code

src/vault/schema.ts[R6-8]

+export const COLUMN_SENSITIVITY = {
+  customer_id: "sensitive",
+  email: "sensitive",
Evidence
The PR exports plain object/array values for sensitivity classification. These are not frozen, so
they remain mutable at runtime, and any importing module could alter them (which would affect
enforcement logic that consumes these exports).

src/vault/schema.ts[6-15]
src/vault/schema.ts[25-27]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Exported classification constants are mutable at runtime. Consumers can mutate `COLUMN_SENSITIVITY` or push/splice the exported arrays, potentially altering enforcement decisions.

### Issue Context
TypeScript’s `as const` is compile-time only; it does not freeze objects/arrays. Because these are exported bindings, runtime mutation will affect all importers.

### Fix Focus Areas
- src/vault/schema.ts[6-27]

### Implementation notes
- Freeze the exported object and arrays:
 - `export const COLUMN_SENSITIVITY = Object.freeze({ ... } as const);`
 - `export const SENSITIVE_COLUMNS = Object.freeze(columnsWith('sensitive')) as readonly TicketColumn[];` (same for others)
- Consider exporting readonly types to reinforce immutability in callers.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Web pages:
  +7 more
Review mode: ⚖️ Balanced: This introduces security-sensitive vault boundaries, schema/seeding invariants, and database lifecycle/query logic across several files; it warrants a careful single-pass review, but is not dense enough to justify redundant extended passes.

Grey Divider

Tip of the day
💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread src/vault/database.ts Outdated
Comment thread src/vault/schema.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The changes implement the described vault boundary with deterministic seeding and tests; review notes are limited to minor maintainability/performance refinements.

Pull request overview

Adds a synthetic “vault” support-ticket dataset backed by an in-memory node:sqlite database, with a strict schema and deterministic seeding, and locks the intended safety boundary via tests (narrow query-only module surface; no exported raw-row access or parameterized probes on sensitive columns).

Changes:

  • Introduces a STRICT SQLite schema plus typed per-column sensitivity labeling.
  • Implements deterministic seeded row generation including a single canary row and one below-k small-cell group.
  • Adds a closure-encapsulated SQLite database wrapper that exposes only scalar queries, with tests that enforce the module surface and invariants.
File summaries
File Description
src/vault/vault.test.ts Adds tests for schema labeling/DDL, deterministic seeding, and locked-down database/module exports.
src/vault/seed.ts Implements deterministic synthetic ticket row generation, including canary and small-cell fixtures.
src/vault/schema.ts Defines column sensitivity labels and the STRICT tickets table DDL.
src/vault/database.ts Adds openVaultDatabase() that seeds the dataset and exposes only scalar queries (row count, canary presence, group size).
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/vault/seed.ts
Comment thread src/vault/database.ts Outdated
@MachineLearning-Nerd

Copy link
Copy Markdown
Owner Author

Both Qodo findings addressed in 7a31ce4:

1. Unclosed prepared statements — fixed the valid part, declining the rest with reason. The per-call db.prepare() inside the count() helper was real statement churn and is gone: the three read statements are now prepared once and reused for the life of the handle. The prescribed StatementSync.close() is not implementable on this project's pinned runtime — it landed in Node 26 (the cited docs are v26.7.0); this repo pins engines >= 24 and CI runs Node 24, where the API does not exist (verified: typeof stmt.close === 'undefined'). With statement churn removed, the remaining exposure is a handful of statements finalized at GC on short-lived in-memory handles.

2. Mutable sensitivity exports — fixed, and extended. COLUMN_SENSITIVITY and the derived column arrays are now Object.freezed with readonly return types (verified pre-fix that runtime mutation actually succeeded). Applied the same hardening to the seed constants (CANARY, SMALL_CELL, WEEKS, REGIONS, CATEGORIES) since enforcement logic will read those too, and added a test asserting every exported classification constant is runtime-frozen.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The changes define the security enforcement boundary that all later phases depend on, so final sign-off should come from a human reviewer despite no blocking issues being found.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The change establishes the security enforcement boundary that all later phases build on, so despite being clean and well-tested it warrants final human sign-off.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@MachineLearning-Nerd
MachineLearning-Nerd merged commit cf42e03 into main Aug 25, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants