fix: close the two bugs deferred from the engine hunt - #112
Conversation
Distill embedding reconcile (data integrity). A node's row and its embedding are written in separate transactions, so a transient embedding failure after the row commits left a node with no vector; the idempotent re-distill skip then made the miss permanent, hiding a real fact from semantic search. Distill now re-embeds any existing node missing its vector at the start of the pass (a no-op once all are embedded), through a new store.hasEmbedding and an embedTextForNode that mirrors each create path's embed text. API timestamp validation (security). The since/until/before query params are now validated as ISO-8601 at the boundary and rejected with a fixed 400, so a malformed value never reaches Postgres as an uncastable timestamp whose raw error (engine, column type, reflected input) the classifier would echo to the client. Two new tests (reconcile re-embeds a vector-less node; a bad timestamp param is a fixed 400 with no leaked DB error). typecheck, lint, root test (fail 0), smoke:packed, and benchmark-drift all green. Patch changeset (core + web). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8022d9cc67
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const t = Date.parse(raw); | ||
| if (Number.isNaN(t)) throw new ApiError(400, `${name} must be an ISO-8601 timestamp`); | ||
| return new Date(t).toISOString(); |
There was a problem hiding this comment.
Date.parse is deliberately permissive, so malformed/non-ISO inputs such as since=June%201,%202024 are accepted, and impossible dates such as since=2024-02-30 are silently normalized to March 1 instead of receiving the documented fixed 400. This means the new boundary does not actually enforce the advertised ISO-8601 contract for /api/metrics or /api/runs; use a strict ISO shape and calendar-date validation before canonicalizing.
Useful? React with 👍 / 👎.
| if (!(await this.store.hasEmbedding(node.id, node.kind))) { | ||
| await this.embedNode(node.id, node.kind, this.embedTextForNode(node)); |
There was a problem hiding this comment.
When two requests or server instances re-distill the same evidence while a node has no vector, both can observe hasEmbedding as false and then insert an embedding. The embedding schema has no unique (node_id, node_kind) constraint, so both writes succeed; nearestNodes applies its limit before deduplicating IDs, allowing duplicate rows to consume semantic-search candidate slots indefinitely. Enforce uniqueness and use an atomic insert/upsert (or otherwise serialize this check/write).
Useful? React with 👍 / 👎.
# Conflicts: # packages/core/src/distill.test.ts # packages/core/src/marrow.ts # packages/web/src/server.test.ts
1 participant
Completes the two defects deferred from #111 (the engine bug hunt), each with a regression test.
1. Distill embedding reconcile (data integrity)
A node's row and its embedding vector are written in separate transactions. A transient embedding failure after the row commits left a real node with no vector — and the idempotent re-distill skip (
seen.has(key)) made the miss permanent, so the fact was invisible to semantic search forever (mitigated only by the keyword fallback and graph neighbors).Fix: at the start of a distill pass, re-embed any existing node for that evidence that is missing its vector, via a new
store.hasEmbeddingand anembedTextForNodethat mirrors each create path's embed text. It's a no-op once everything is embedded, and a re-distill of the same source now repairs a past failure.2. API timestamp validation (security / info disclosure)
The error classifier echoes messages matching
/invalid /verbatim. A malformedsince/until/beforequery param reached Postgres as an uncastable timestamp, whose raw error (invalid input syntax for type timestamp: "..."— engine, column type, reflected input) then got reflected to the client.Fix: validate those params as ISO-8601 at the boundary (
parseIsoParam), rejecting a bad value with a fixed400and a canonical ISO string on success, so the raw DB error is never produced. The classifier is left untouched (core's own product-voice messages still surface correctly).Verification
Two new tests: a vector-less node gets re-embedded on the next distill; a bad timestamp param is a fixed 400 with no leaked DB error.
pnpm typecheck,pnpm lint, rootpnpm test(ℹ fail 0; core 361, web 117),pnpm smoke:packed, andcheck-benchmark-driftall green. Patch changeset (core + web).With this, every finding from the engine hunt (#111 plus these two) is fixed.
🤖 Generated with Claude Code