Skip to content

test: finish #92 — remove the real-DB-via-service tail from the fast suite - #96

Merged
jcfischer merged 3 commits into
mainfrom
fix/flaky-suite-finish-92
Jun 12, 2026
Merged

jcfischer merged 3 commits into
mainfrom
fix/flaky-suite-finish-92

Conversation

@jcfischer

@jcfischer jcfischer commented Jun 12, 2026 •

Copy link
Copy Markdown
Owner

Follow-up to #95 — finishes #92. Removes the remaining tests that implicitly resolved the live 854k-node workspace DB through service internals, so the fast pre-push suite is hermetic and load-immune.

DI-converted (kept on CI, deterministic)

  • tests/helpers/seeded-db.ts — shared seeded DB. The supertag tables the schema-service reads are built via the production migrations (migrateSupertagMetadataSchema/migrateSchemaConsolidation/migrateFieldValuesSchema) so the fixture can't drift; only the two indexer-owned tables (nodes, tag_applications) are declared locally. Seeds todo/meeting.
  • batch-operations.test.ts validation describe — the three payload-building tests create + clean up a seeded DB inline and pass _dbPathOverride (the 18s "exactly 50 nodes" boundary test now runs in ~ms); the two throw-before-DB cases build nothing.
  • batch-cli.test.ts executeBatchCreate describe — inject _dbPath likewise.

Gated behind RUN_INTEGRATION (run on CI + test:integration; self-skip on CI with no DB)

  • commands/create-validation.test.ts (CLI-subprocess validation)
  • commands/fields.test.ts — only the "Integration Tests (with real database)" describe; the TEST_DB-backed unit describes stay fast.
  • mcp/tools/__tests__/transcript.test.ts, tools.test.ts ("MCP Tools Integration" only — sibling "MCP Tools Unit Tests" stays), field-values.test.ts. In these last two, the module-level getDatabasePath()/existsSync are guarded behind the gate's RUN_INTEGRATION flag, so importing them in the fast suite does not resolve the workspace DB.
  • services/node-builder.test.ts — the four getSchemaRegistry/createNode describes; buildChildNodes (hermetic) stays fast.

Result

Fast suite: 0 fail, 55s at load 133 (before this work it blew past the pre-push hook's 360s cap at load ~60). No fast-suite test resolves the workspace DB any more — the real-DB suites are gated and their path-resolution is RUN_INTEGRATION-guarded.

DI was preferred where the service exposed an injection point (_dbPathOverride/_dbPath) so coverage stays on CI; gating was used for CLI-subprocess and registry/real-DB suites where a faithful fixture is disproportionate.

Closes #92.

…suite

Follow-up to #95. Eliminates the remaining tests that implicitly resolved the
live 854k-node workspace DB through service internals (and either ran for tens
of seconds or SQLITE_CANTOPEN-skipped on CI), so the fast pre-push suite is now
hermetic and load-immune.

DI (kept on CI, deterministic):
- tests/helpers/seeded-db.ts — shared schema-valid seeded DB (nodes + supertag
  tables, seeds todo/meeting).
- batch-operations.test.ts `validation` describe — inject _dbPathOverride into
  the tests that get past size validation into payload building (the 18s
  "exactly 50 nodes" boundary test now runs in ~0ms; drop SQLITE_CANTOPEN
  catches). The pure >50-throws tests stay as-is (they throw before any DB).
- batch-cli.test.ts `executeBatchCreate` describe — inject _dbPath likewise.

Gated behind RUN_INTEGRATION (run on CI via the workflow env + locally via
test:integration; self-skip on CI with no DB):
- commands/create-validation.test.ts (CLI subprocess validation)
- commands/fields.test.ts — only the "Integration Tests (with real database)"
  describe; the TEST_DB-backed unit describes stay in the fast suite.
- mcp/tools/__tests__/transcript.test.ts, tools.test.ts ("MCP Tools
  Integration" only — the sibling "MCP Tools Unit Tests" stays), field-values.
- services/node-builder.test.ts — the four getSchemaRegistry/createNode
  describes; buildChildNodes (hermetic) stays in the fast suite.

Result: fast suite 0 fail, 55s at load 133 (before: blew past the hook's 360s
cap at load ~60). No ungated real-DB/CANTOPEN tests remain in the fast glob.

Closes #92.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@jcfischer jcfischer left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Sage code review — changes-requested

3 finding(s): 1 blocker, 2 suggestion.

Performance

  • [suggestion] tests/batch-operations.test.ts:497 — Avoid unused DB setup
    The new beforeEach(() => { seeded = createSeededDb('batch-validation'); }); creates a SQLite database and temp directory for every validation test, including the should throw VALIDATION_ERROR when more than 50 nodes provided case that never uses seeded.dbPath. That adds avoidable synchronous fs and SQLite setup to tests that throw before DB access.
    Fix: Move createSeededDb into only the tests that pass _dbPathOverride: seeded.dbPath, or split those cases into a nested describe.

Maintainability

  • [suggestion] tests/helpers/seeded-db.ts:11 — Hand-maintained fixture schema may drift
    The helper says, "Schema mirrors the columns the node-builder / schema-service read," then hard-codes multiple CREATE TABLE blocks. This creates a second schema definition that has to be updated whenever the production schema readers change.
    Fix: Build this fixture through the same migration/schema setup path used by production tests, then seed only the required rows.

HonestOracle

  • [blocker] src/mcp/tools/__tests__/field-values.test.ts:12 — Workspace DB lookup remains ungated
    The description claims "no ungated SQLITE_CANTOPEN / resolveWorkspace(undefined) / getDatabasePath() tests remain" and that the fast suite is "hermetic," but this file still runs const dbPath = getDatabasePath(); before the new describeIntegration("tana_field_values MCP Tool", () => { gate. A fast-suite import can still resolve the workspace DB path even when the integration describe is skipped, so the stated hermetic boundary is false.

Posted by Sage on Codex CLI substrate.

@jcfischer jcfischer left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Sage code review — changes-requested

3 finding(s): 3 important; 1 lens(es) failed to run: CodeQuality.

CodeQuality — DID NOT RUN

Coverage incomplete; re-run before merge.

  • [important] (lens output):0 — CodeQuality: model deviated from JSON contract
    codex substrate timed out after 600000ms

HonestOracle

  • [important] PR description:9 — CI coverage claim is unsupported
    The description says "Gated behind RUN_INTEGRATION (run on CI + test:integration; self-skip on CI with no DB)", but the diff only swaps describe for describeIntegration and shows no CI or package-script change proving RUN_INTEGRATION is set. This can mislead reviewers into believing coverage is retained on CI when the evidence provided only shows tests being gated.

  • [important] PR description:18 — Fast-suite proof is missing
    The result claims "Fast suite: 0 fail, 55s at load 133" and "A content grep confirms no ungated SQLITE_CANTOPEN / resolveWorkspace(undefined) / getDatabasePath() tests remain", but no command, log, grep pattern, or fast-glob definition is included. Reviewers are asked to accept the hermetic/load-immune conclusion without reproducible evidence.


Posted by Sage on Codex CLI substrate.

- blocker (HonestOracle): getDatabasePath() ran at module load in
  field-values/tools mcp tests, breaking the hermetic claim. Guard both behind
  the gate's RUN_INTEGRATION flag so the fast suite never resolves the
  workspace DB path; the real-DB describes only run under integration anyway.
- perf: batch-operations validation no longer builds a seeded DB in a
  describe-level beforeEach for every test; the two throw-before-DB cases pay
  nothing — the three payload-building tests create+cleanup a seeded DB inline.
- maintainability: seeded-db builds the supertag tables via the production
  migrations (migrateSupertagMetadataSchema/SchemaConsolidation/FieldValues)
  instead of hand-rolled CREATE TABLEs, so the fixture can't drift from the
  real schema. Only the two indexer-owned tables remain declared locally.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@jcfischer jcfischer left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Sage code review — commented

3 finding(s): 3 suggestion.

Performance

  • [suggestion] tests/batch-cli.test.ts:291 — Avoid fixture setup for export check
    The suite-level setup beforeEach(() => { seeded = createSeededDb('batch-cli-create'); }); runs migrations and filesystem SQLite setup even for it('should export executeBatchCreate function', which only imports the function. That adds avoidable per-test I/O and allocation to the fast suite.
    Fix: Move createSeededDb into only the tests that pass _dbPath, or split the export test into a separate describe without the fixture.

Maintainability

  • [suggestion] tests/batch-operations.test.ts:532 — Extract seeded DB lifecycle
    The same fixture lifecycle repeats across three tests: const seeded = createSeededDb('batch-validation'); followed by finally { seeded.cleanup(); }. That duplication makes later setup changes easy to miss in one case.
    Fix: Use a small withSeededDb helper or local beforeEach/afterEach scoped to the DB-backed validation tests.

HonestOracle

  • [suggestion] tests/helpers/seeded-db.ts:14 — Indexer schema parity is asserted, not shown
    The comment says the locally declared tables are "matching src/db/indexer.ts", but the diff provides no cited source excerpt or invariant proving these hand-written nodes and tag_applications definitions match the indexer schema. That misleads readers into treating the fixture as source-parity evidence when it is still a manual duplicate.

Posted by Sage on Codex CLI substrate.

- withSeededDb(label, fn) wraps create+cleanup; the DB-backed batch tests use
  it so import-only / throw-before-DB tests in the same describe build no
  fixture (removes batch-cli's describe-level beforeEach and the repeated
  try/finally in batch-operations).
- soften seeded-db comment: the local nodes/tag_applications tables are a
  minimal subset to allow opening the DB, not a verified mirror of indexer.ts.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@jcfischer jcfischer left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Sage code review — commented

2 finding(s): 2 suggestion.

Maintainability

  • [suggestion] src/mcp/tools/__tests__/field-values.test.ts:11 — Deduplicate integration DB guard
    The same guard is copied here and in tools.test.ts: const dbPath = RUN_INTEGRATION ? getDatabasePath() : ""; plus const hasDatabase = RUN_INTEGRATION ? existsSync(dbPath) : false;. Keeping that hermetic import behavior in two test files means future gate changes require synchronized edits.
    Fix: Move the DB presence check into a shared helper in tests/helpers/integration-gate.

HonestOracle

  • [suggestion] PR description:22 — Issue closure is unsupported
    The description says "Closes #92", but the stdin contains no #92 acceptance criteria or issue text, so a downstream reader cannot verify that this diff actually completes the referenced issue rather than only part of it.

Posted by Sage on Codex CLI substrate.

@jcfischer

Copy link
Copy Markdown
Owner Author

Sage: effective pass (commented, 0 blockers / 0 majors). Blocker (module-level getDatabasePath breaking the hermetic claim) fixed via the RUN_INTEGRATION guard; nits addressed (withSeededDb helper, migration-based fixture, softened parity comment). CI green, CLEAN. Merging — closes #92.

@jcfischer
jcfischer merged commit 4b0dfd5 into main Jun 12, 2026
1 check passed
@jcfischer
jcfischer deleted the fix/flaky-suite-finish-92 branch June 12, 2026 22:26

@jcfischer jcfischer left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Sage code review — commented

1 finding(s): 1 suggestion.

HonestOracle

  • [suggestion] tests/helpers/seeded-db.ts:51 — Local schema mirror is unproven
    The comment says "Indexer-owned tables the node builder may read (mirror src/db/indexer.ts)", but the diff only hand-declares two tables and shows no comparison to src/db/indexer.ts. That sells a local subset as a mirror, which can mislead downstream readers about fixture parity.

Posted by Sage on Codex CLI substrate.

@jcfischer jcfischer left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Review — #96: finish #92, remove the real-DB-via-service tail from the fast suite

Solid follow-through on the #95 / #92 cleanup. The DI / gating split is sensible (DI where a _dbPath* injection point exists, gating where the test is CLI-subprocess or registry-driven), and the comment-level disclosure in seeded-db.ts about the fixture-vs-production schema is appreciated. A handful of minor observations:

Minor

  • tests/helpers/seeded-db.ts schema drift — the helper's own comment notes the nodes / tag_applications tables aren't a verified mirror; concretely they diverge in two ways:

    • nodes is missing node_type and done_at (both present in src/db/indexer.ts).
    • tag_applications drops the id INTEGER PRIMARY KEY AUTOINCREMENT and relaxes tag_id / tag_name from NOT NULL to nullable.

    Today nothing on the batchCreateNodes dry-run path INSERTs into either table, so the test green is honest. The risk is asymmetric: a future query that adds nodes.node_type or relies on tag_applications.tag_id NOT NULL would pass against the fixture and fail against production. Worth either tightening the columns to match exactly or naming the schema-drift risk in src/db/indexer.ts alongside the CREATE TABLE blocks so the next editor sees the cross-reference.

  • field-values.test.ts over-gates the export check — the outer describeIntegration("tana_field_values MCP Tool", …) now skips the inner describe("Unit Tests") whose only assertion is expect(typeof fieldValues).toBe("function"). That's the kind of pure unit test tools.test.ts deliberately keeps un-gated as a sibling describe. Trivial to lift out; very low-value test, but inconsistent with the pattern you established in tools.test.ts.

  • node-builder.test.ts — createNode() with database field types — this describe builds a fully hermetic temp DB via migrateSupertagMetadataSchema / migrateSchemaConsolidation per beforeEach. Gating it behind RUN_INTEGRATION removes it from the fast suite even though it has no live-workspace dependency (the one legitimately-non-hermetic test, "should fall back to registry when database does not exist", uses getSchemaRegistry() and could justify the gate on its own). If wall time of the migration calls is what's driving the gate, that'd be worth saying in a comment; otherwise the first three tests look like fair game for the fast suite.

Nits

  • The counter module-global in seeded-db.ts plus process.pid already disambiguates concurrent invocations, but the comment in withSeededDb ("guaranteeing cleanup afterwards") slightly oversells — a SIGKILL mid-test still leaves the tmp dir. Not worth changing, just noting the wording.
  • executeBatchCreate describe (lines 288–346) converted to withSeededDb, but the sibling batch create E2E integration describe (lines 452+) still hand-rolls a richer schema inline. Reasonable (the E2E schema is broader than the helper's seed), but if you ever want a uniform fixture, the helper could grow a withRichSeededDb variant.

Net

Approving in spirit — the PR meets its stated goal (fast suite hermetic, no resolution of the workspace DB, real-DB suites still gated and runnable via RUN_INTEGRATION=1). Schema-drift is the only thing I'd want a follow-up note on; everything else is taste.

@jcfischer jcfischer left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Review: #96 — finish hermetic-test conversion for the fast suite

Overview

Follow-up to #95 that closes #92 by removing the last fast-suite tests that implicitly opened the live 854k-node workspace DB. The split is sensible:

  • DI-converted where the service already exposed an injection point (_dbPathOverride / _dbPath): batch-operations validation, batch-cli executeBatchCreate.
  • Gated behind RUN_INTEGRATION=1 where a faithful fixture would be disproportionate: CLI-subprocess and registry/real-DB describes in commands/create-validation, commands/fields, mcp/tools/__tests__/{transcript,tools,field-values}, and the four getSchemaRegistry/createNode describes in services/node-builder.test.

The numbers in the description (360s pre-push timeout → 55s @ load 133) are the win.

What's good

  • Test fixture schema for the supertag-service tables is built via the production migrations (migrateSupertagMetadataSchema / migrateSchemaConsolidation / migrateFieldValuesSchema), so that part can't drift.
  • withSeededDb uses try/finally so the temp dir is removed even on assertion failure, and the previous SQLITE_CANTOPEN swallow-on-CI pattern is gone (errors now surface).
  • Gating is consistently done at the outer describe, so module-level getDatabasePath() / existsSync calls in field-values.test.ts and tools.test.ts are guarded with RUN_INTEGRATION ? … : "" — importing those files in the fast suite no longer resolves the workspace DB.
  • Per-test seeding in the validation describes (rather than beforeEach) means the size-validation tests that throw before reaching the DB don't pay for fixture setup. Nice.

Notes (non-blocking)

  1. tests/helpers/seeded-db.ts — nodes / tag_applications fixtures drift from src/db/indexer.ts. The fixture is missing node_type and done_at on nodes, the id PK + NOT NULL on tag_applications, and the production indexes. The docstring is honest that this is a minimal "open-the-DB" subset, but if buildNodePayloadFromDatabase (or anything else in the dry-run path) ever starts reading those columns, the fast suite will pass while production breaks. A small future improvement would be to extract initializeSchema() from TanaIndexer into a free function and call it here too — same single-source-of-truth pattern the supertag tables already use.

  2. _dbPath (CLI) vs _dbPathOverride (service) naming. Pre-existing; not introduced here, but seeded-db.ts's usage example shows both side by side. Worth a follow-up to settle on one name across the seam — picking it up now while the touch is fresh would be cheap.

  3. let counter = 0 at module scope in seeded-db.ts is fine for bun:test (single-process) and process.pid covers cross-process collisions, but worth a one-line comment noting the assumption in case the runner ever fans out to workers.

  4. The gating helper itself (describeIntegration = RUN_INTEGRATION ? describe : describe.skip) is well-scoped, but it means RUN_INTEGRATION is evaluated once at module load. That's the intended semantics — just flagging that toggling the env var inside a test would not flip the gate.

Risks

None blocking. The biggest correctness risk is fixture drift in (1) — it's explicitly acknowledged in the docstring, which is the right move. CI is green and the original goal (no fast-suite test resolves the workspace DB) is met.

LGTM.

@jcfischer jcfischer left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Review of #96 — finish #92 (post-merge)

Scope is well-bounded: convert real-DB-leaking fast-suite tests via DI (_dbPath / _dbPathOverride) where the service exposes an injection point, gate the rest behind RUN_INTEGRATION so CI keeps coverage while the pre-push fast suite stays hermetic. Three sage rounds clearly tightened the result (module-load getDatabasePath() guarded, describe-level beforeEach removed, schema built via real migrations).

Strengths

  • tests/helpers/seeded-db.ts builds supertag_metadata / supertag_fields / field_values via the production migrateSupertagMetadataSchema / migrateSchemaConsolidation / migrateFieldValuesSchema — the schema-service tables can't drift from prod.
  • withSeededDb(label, fn) correctly wraps createSeededDb + cleanup in try/finally, and the module-level counter plus process.pid in the temp-dir name avoid collisions across parallel bun-test workers and reused labels ('batch-validation', 'batch-cli-create').
  • The getSupertag query that the new fixture has to satisfy (LEFT JOIN nodes n ON n.id = m.tag_id + COALESCE(json_extract(n.raw_data, ...), '') NOT LIKE '%TRASH%') only reads id and raw_data from nodes, which the local subset provides — and with the seeded nodes table empty, the n.raw_data IS NULL branch is true, so the seeded todo/meeting rows pass the trash filter. The minimal subset is sufficient for the validation path it has to support.
  • The module-load RUN_INTEGRATION ? getDatabasePath() : "" pattern in field-values.test.ts and tools.test.ts is the right fix for the second commit's blocker — the fast-suite import is now genuinely path-resolution-free.

Nits (non-blocking; safe to address in a follow-up if at all)

  • src/mcp/tools/__tests__/field-values.test.ts:21 — the trivial Unit Tests describe (expect(typeof fieldValues).toBe("function")) is nested inside the gated describeIntegration("tana_field_values MCP Tool", …), so it no longer runs in the fast suite. tools.test.ts correctly hoists its MCP Tools Unit Tests describe to top-level — field-values.test.ts could do the same to keep the trivial export check in the fast suite. Coverage loss is negligible.
  • tests/helpers/seeded-db.ts:52-70 — the local nodes / tag_applications declarations are explicitly called out as "not a verified mirror of src/db/indexer.ts", which is fine for what the schema-service currently reads. If a future schema-service path joins on nodes.node_type or tag_applications.tag_id NOT NULL, the fixture will start returning silently-wrong results rather than failing loudly — worth keeping in mind when reviewing future PRs that touch unified-schema-service.ts or validateSupertagExists.
  • tests/helpers/seeded-db.ts:50 — migrateFieldValuesSchema(db) is included for fixture completeness, but none of the current PR's tests exercise field_values. Harmless; flagging only so a future reader doesn't infer a dependency that isn't there.

Net: the fast-suite hermetic-ization goal is met, DI was correctly preferred where injection points exist, and the trade-offs (minimal nodes/tag_applications subset, integration gating for CLI-subprocess/registry suites) are explicitly documented in the helper and PR description.

{"verdict":"approved","blockers":0,"majors":0,"nits":3}

@jcfischer jcfischer left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Test infrastructure review — finish #92 hermeticization

Net assessment: good direction, mostly clean execution. The fast suite is now genuinely hermetic (no more workspace-DB path resolution at module load thanks to the RUN_INTEGRATION ? getDatabasePath() : "" guards), the withSeededDb extraction is the right shape, and using the production migrations in seeded-db.ts to build the supertag tables eliminates schema drift. A few notes below.

Major

describeIntegration on createNode() with database field types is over-aggressive — moves hermetic tests out of the fast suite without need. src/services/node-builder.test.ts:331

The PR description states the rule: "DI was preferred where the service exposed an injection point; gating was used for CLI-subprocess and registry/real-DB suites where a faithful fixture is disproportionate." This describe is already on the DI side of that line — it builds its own temp DB in beforeEach via migrateSupertagMetadataSchema/migrateSchemaConsolidation, seeds it with a TestTag + three explicit-typed fields, and passes _dbPathOverride: dbPath to createNode. It never touches getDatabasePath() or the workspace DB and doesn't gate on hasSchema.

Wrapping it in describeIntegration therefore removes the T-1 / T-3 explicit-field-type coverage from the fast suite for no hermeticity reason — only because it shares a file with three other describes (validateSupertags(), buildNodePayload(), createNode()) that do depend on the registry. Consider switching this one back to plain describe(...); it's the only piece in the file that exercises the "explicit DB field types beat heuristic" path on every push, which is exactly the kind of thing you don't want silently regressing on CI-only.

Nits

Stale comment in seeded-db.ts contradicts the softened docstring. tests/helpers/seeded-db.ts:52

The module docstring (r3) was deliberately softened to "not a verified mirror of src/db/indexer.ts; they exist only so the builder can open the DB", but the inline comment above the local nodes / tag_applications CREATEs still claims "(mirror src/db/indexer.ts)". Worth aligning — the looser wording is the load-bearing one (these tables are a minimal subset, not a mirror).

Unused dbPath const after the gate flip. src/mcp/tools/__tests__/field-values.test.ts:14 and src/mcp/tools/__tests__/tools.test.ts:23

const dbPath = RUN_INTEGRATION ? getDatabasePath() : ""; is only referenced one line down inside existsSync(dbPath). Could be inlined as const hasDatabase = RUN_INTEGRATION && existsSync(getDatabasePath()); and lose two lines + the empty-string branch. Pure tidy.

Things I checked and liked

  • withSeededDb(label, fn) cleanly fixes the previous per-describe beforeEach cost — the throw-before-DB tests (>50 rejection, suggestion-on-validation-error) genuinely pay nothing now, and the inline pattern keeps the DB lifecycle visible at the call site rather than hidden in describe scope.
  • RUN_INTEGRATION is read once at module load (process.env.RUN_INTEGRATION === "1") and exported alongside describeIntegration, so the import-time guards (RUN_INTEGRATION ? getDatabasePath() : "") and the describe gating use the same source of truth — no chance of skew.
  • Migration order in createSeededDb is correct: migrateSupertagMetadataSchema (creates supertag_fields, supertag_parents) then migrateSchemaConsolidation (creates supertag_metadata) then migrateFieldValuesSchema. The INSERT INTO supertag_metadata afterwards works against the consolidated schema.
  • tag_applications / nodes schemas being a "minimal subset to allow opening the DB" rather than a verified mirror is now honestly documented — if the indexer adds a NOT NULL column those services don't read, this fixture is unaffected, and if they add one those services do read, the test will fail loudly with a clear SQL error. Right tradeoff for a fast-suite fixture.

@jcfischer jcfischer left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Review — #96: finish #92, hermetic fast suite (post-merge)

The DI-vs-gate split is sensible, the RUN_INTEGRATION ? getDatabasePath() : "" guard at module load is the right fix for the second-commit blocker (the fast-suite import now genuinely does not resolve the workspace DB), and building the supertag tables via the production migrations in seeded-db.ts removes the schema-drift surface for the parts that matter. Four observations below; one is worth a follow-up, the rest are nits.

Major

src/services/node-builder.test.ts:331 — createNode() with database field types is gated despite being hermetic. This describe builds its own temp DB in beforeEach via migrateSupertagMetadataSchema / migrateSchemaConsolidation, seeds it with a TestTag + three explicit-typed fields (date / url / reference), and passes _dbPathOverride: dbPath to createNode. It never calls getDatabasePath() and does not gate on hasSchema. By the PR's own stated rule ("DI was preferred where the service exposed an injection point; gating was used for CLI-subprocess and registry/real-DB suites"), this describe is on the DI side of the line — it shares a file with three other describes that do depend on the registry, which is why it got swept in. Wrapping it in describeIntegration removes the T-1 / T-3 explicit-field-type coverage ("explicit DB field types beat heuristic") from every push without a hermeticity reason. Worth flipping back to plain describe(...) in a follow-up.

Nits

src/mcp/tools/__tests__/field-values.test.ts:21 — Unit Tests describe is nested inside the gated wrapper, inconsistent with tools.test.ts. The trivial export check (expect(typeof fieldValues).toBe("function")) is inside describeIntegration("tana_field_values MCP Tool", …), so it no longer runs in the fast suite. The sibling file src/mcp/tools/__tests__/tools.test.ts:266 correctly hoists describe('MCP Tools Unit Tests', …) to top-level alongside the gated MCP Tools Integration. Same pattern would apply here. Coverage loss is negligible; the value is consistency with the established convention.

tests/helpers/seeded-db.ts:52 — inline comment contradicts the softened module docstring. The r3-softened module docstring deliberately reads "not a verified mirror of src/db/indexer.ts; they exist only so the builder can open the DB", but the inline comment above the local CREATEs still claims "(mirror src/db/indexer.ts)". Worth aligning — the looser wording is the load-bearing one. Concretely the local subset diverges in ways the docstring warns about but the inline comment denies: nodes is missing node_type and done_at; tag_applications drops id INTEGER PRIMARY KEY AUTOINCREMENT and relaxes tag_id / tag_name from NOT NULL to nullable. Today nothing on the batchCreateNodes dry-run path touches those columns, so the test green is honest, but a future query that adds nodes.node_type or relies on tag_applications.tag_id NOT NULL would pass against the fixture and fail in production. If TanaIndexer.initializeSchema() could be extracted into a free function and reused here, that'd close the gap the same way the supertag tables already do — otherwise just aligning the inline comment with the docstring is the cheap fix.

src/mcp/tools/__tests__/field-values.test.ts:14 and tools.test.ts:23 — unused intermediate dbPath const. const dbPath = RUN_INTEGRATION ? getDatabasePath() : ""; is only referenced one line down inside existsSync(dbPath). Could be inlined as const hasDatabase = RUN_INTEGRATION && existsSync(getDatabasePath()); and lose two lines plus the empty-string branch. Pure tidy.

Net

Fast-suite hermetic goal is met and the DI/gate split is right — only thing actually worth a follow-up is moving createNode() with database field types back into the fast suite; the rest is comment/style alignment.

@jcfischer jcfischer left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Test-hygiene PR; reviewed post-merge.

Scope & verification

  • DI route uses real injection points: _dbPath on batch.ts:42,65,143,225 and _dbPathOverride on batch-operations.ts:387,407,499. The seeded fixture is actually threaded through to the production path.
  • tests/helpers/seeded-db.ts builds the schema-service tables via the production migrations (migrateSupertagMetadataSchema/migrateSchemaConsolidation/migrateFieldValuesSchema exist in src/db/migrate.ts:97,203,308). Good — no drift surface. The local nodes/tag_applications declarations are honestly disclosed as a minimal subset, not a mirror, which matches what the builder needs to open without SQLITE_CANTOPEN.
  • RUN_INTEGRATION gating is wired correctly: package.json sets it for test:integration/test:full/precommit, .github/workflows/test.yml:29 sets it for CI. The fast suite genuinely skips the gated describes; CI keeps coverage.
  • HonestOracle's prior finding addressed: field-values.test.ts and tools.test.ts no longer resolve getDatabasePath() at module load — both are now RUN_INTEGRATION ? getDatabasePath() : "", so importing the file in the fast suite is hermetic.
  • node-builder.test.ts: outer describe only touches SCHEMA_CACHE_FILE (config path, not the workspace DB), so leaving it ungated is fine; buildChildNodes stays in the fast suite as the description claims.
  • Bonus: the migrated batch tests now assert real outcomes (e.g. toHaveLength(BATCH_CREATE_MAX_NODES)) instead of silently swallowing SQLITE_CANTOPEN — a fidelity improvement, not just a perf one.

Nits (non-blocking, optional follow-up)

  • batch-operations.test.ts validation: the two cases that assert on results[0].error for missing supertag / missing name fail validation before any DB call. Wrapping them in withSeededDb still creates+seeds a DB they don't read. Cheap, but inconsistent with the comment that says "only the tests that get past size validation… need a DB". Could drop the wrapper on those two.
  • withSeededDb is the recommended API; consider not exporting createSeededDb's cleanup field as part of the interface, or marking the bare createSeededDb as @internal, so future callers don't reintroduce manual cleanup paths.
  • seeded-db.ts seeds only todo/meeting. Fine for current callers; worth a one-line comment that adding new fixtures requires updating both this file and any test that needs them, so the next person doesn't quietly fall back to the live workspace DB if a seed is missing.

No blockers, no majors. LGTM.

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.

Flaky full-suite tests: resolveEntity + transcript timeouts under parallel load

1 participant