feat(knowledge-graph): structured dataset ingestion via CSV import - #533
Merged
Conversation
) Adds a dataset ingestion path into the Knowledge Graph that respects the shape of structured data (columns, types) instead of treating it as plain text, per the maintainer-approved plan in the issue's triage comments. Storage (decided in triage): relational sidecar, not graph-node explosion. Migration 0029_datasets.sql adds `datasets` + `dataset_rows` tables in the Neon graphPool; exactly one Dataset graph node (PluginEntity, system='dataset') is created per dataset for recall/citation linking, rows never become graph nodes. New KnowledgeGraph.{ingestDataset,listDatasets, getDataset,queryDatasetRows,deleteDataset} surface, implemented with full parity in both the Neon and in-memory backends (no silent no-op). Privacy-on-import (decided in triage): every imported row runs through the existing C0 regex PII-detector baseline (createBaselineDetector/maskPrompt, now re-exported from @omadia/plugin-privacy-guard) before being persisted -- the same masking pipeline that already protects free-text user prompts. Only string/date-typed columns are scanned; number/boolean columns are, by construction, non-free-text and scanning them risks corrupting legitimate data on a false-positive regex hit -- see datasetImport.ts's module doc for the full reasoning. Cost note: O(rows x string-columns) regex passes, CPU- bound; a future GLiNER (C1) sidecar hookup for this path is a natural follow-up if false-negative rate needs improving. Import surfaces: - POST /api/v1/datasets multipart CSV upload (src/routes/datasets.ts), ACL pattern mirrors /api/v1/memory (session-derived owner only). - Chat-attachment auto-ingest: CSV attachments now import as a queryable dataset instead of being silently truncated at the existing 20,000-char MAX_TEXT_CHARS cap in attachmentExtract.ts. Query: new query_dataset native tool (list_datasets/get_schema/query_rows) over a constrained filter+aggregate DSL -- never raw SQL from the model. Column names are still bound as SQL parameters (not interpolated) even after DSL validation, since a dataset's columns are themselves CSV-header data, not a trusted literal. Results always page/aggregate server-side. CSV-first v1 scope per the triage's own effort estimate (L for CSV, XL if XLSX/DB exports included) -- XLSX/DB-export ingestion is an explicit follow-up, not attempted here. Not included in this change, by deliberate scoping decision (see PR body): the web-ui/app/admin/ upload/schema/delete page. The full REST + tool + storage path is implemented and tested; the admin page needs its own separate typecheck/lint verification this change's gate (middleware only) doesn't cover. Docs: docs/CHANGELOG.md Unreleased entry + docs/middleware-agent-handoff.md Knowledge-Graph section, per AGENTS.md's doc-alongside-code rule.
inferColumnType (datasetImport.ts) typed any pure-digit CSV column as
'number' whenever every value matched /^-?\d+(?:\.\d+)?$/, including
zero-padded identifiers such as phone numbers ('0301234567') and postal
codes ('01234'). Number()-coercion of such a column silently drops the
leading zero (data corruption), and number-typed columns are excluded
from the mandatory C0 privacy scan by design, so a real phone number in
such a column was persisted un-redacted.
inferColumnType now also rejects 'number' for any column containing a
value matching /^0\d/ (leading zero, excluding a bare '0' or a '0.x'
decimal), falling back to 'string' instead. That routes the column
through the mandatory privacy scan like any other free-text column and
keeps the value intact when it isn't PII.
Also: validateDatasetQueryOptions (knowledgeGraph.ts) clamped an
explicit limit:0 to the 50-row default instead of 1, because
Math.trunc(0) || DEFAULT evaluates the falsy 0 as 'not provided'. Now
limit:0 clamps to the documented minimum of 1.
Also: fix the packages/plugin-privacy-guard workspace running after
packages/harness-orchestrator in package.json's build/dev/typecheck
scripts — a pre-existing ordering gap that this branch's new
harness-orchestrator -> plugin-privacy-guard dependency turned into a
hard build/typecheck failure on a clean checkout.
Adds regression coverage for both dataset-import fixes and the
limit-clamp fix.
…st (#430) Second fixup round on the #430 dataset-ingestion branch, addressing an adversarial cross-vendor review of commit 7909dbb. All five confirmed findings: 1. ACL identity bug - the chat-attachment CSV auto-ingest path (orchestrator.ts's ingestAttachments) wrote ownerOmadiaUserId from input.userId, which for a channel turn is the RAW channel-native id (Teams AAD oid via orchestratorDispatcher.ts), not the canonical omadiaUserId uuid the KG's ACL routes filter on. ChatTurnInput gains an optional channelIdentity field ({ channelKind, channelUserId }), populated only by createOrchestratorDispatcher for channel kinds the KG ChannelKind model covers (teams/slack/telegram); the CSV-import call site now resolves it via KnowledgeGraph.resolveOrCreateChannelIdentity before using it as the dataset owner, and declines the KG-import branch (falling back to the plain-text attachment path) for channel kinds it can't map (discord, whatsapp, the canvas channel's 'custom' userRef) rather than guessing. 2. Silent CSV truncation - parseCsv's per-cell MAX_CELL_CHARS cut had no signal. parseCsv/buildDatasetFromCsv/importCsvDataset now return a truncation: { truncatedCellCount, truncatedColumns } alongside privacyScan, surfaced in the POST /api/v1/datasets response and in the chat-ingest tool-result note. 3. Neon ILIKE wildcard escaping - the contains dataset filter now escapes %, _, and backslash in the filter value before wrapping it for ILIKE ... ESCAPE, matching the in-memory backend's literal substring .includes() semantics. 4. In-memory group-by unbounded - InMemoryKnowledgeGraph's grouped dataset query now caps at 200 groups (sorted by aggregate value descending, nulls last, for a deterministic truncation), matching NeonKnowledgeGraph's existing LIMIT 200. 5. Scope honesty - docs/middleware-agent-handoff.md gains the missing #3 (Dataset-Routen + query_dataset-Tool) and #8 cross-reference entries (AGENTS.md's route/tool doc-placement rule), plus a #13 roadmap bullet for the deferred admin UI. CHANGELOG documents the round-2 fixes and the scope correction (this addresses, not closes, #430 - see PR body). Verified: npm run typecheck && npm test, both clean on this branch tip (full suite: 4829 pass / 0 fail / 4 skipped - the 4 skips are the DATABASE_URL-gated live-Neon tests, unchanged from before this commit).
…memory (#430) matchesDatasetFilter (InMemoryKnowledgeGraph) compared eq/neq/contains filter values with no type coercion (value === filter.value), while NeonKnowledgeGraph's buildDatasetFilterClause already coerced filter.value to the target column's declared type before comparing. Concrete failing case: a number column amount storing 250 (a JS number) queried via query_dataset with {column:'amount', op:'eq', value:'250'} (a JSON string -- the tool's Zod schema allows this regardless of column type or op) matched on the Neon backend but silently returned totalMatched: 0 on the in-memory backend for the identical logical query. Fix mirrors Neon's coercion exactly: filter.value is coerced against the column's schema-declared type (Number(...) for a number column, String(...) otherwise) rather than against the filter value's own JS type or a single row's runtime value. contains now also coerces a non-string filter.value to a string before the substring check instead of rejecting it outright. Added a regression test in inMemoryKnowledgeGraph.test.ts reproducing the exact eq case above, plus the neq and contains mirrors. Also appends a docs/CHANGELOG.md entry per AGENTS.md's bugfix-documentation rule.
…set query ACL (#430) Round 5 adversarial-review fixup. Round 2's channel-identity resolution fix only covered the CSV-import path in ingestAttachments; QueryDatasetTool still read the raw turnContext.current()?.userId (a Teams AAD oid etc. for a channel turn) instead of the canonical omadiaUserId a channel-imported dataset was actually stored under, so list_datasets/get_schema/query_rows could never find a dataset a channel user had just imported. - Add resolveTurnOwnerIdentity(), extracting the resolve-or-fallback logic ingestAttachments already had into a single shared helper. - Add TurnContextValue.resolvedOmadiaUserId, populated once at both per-turn scope establishment sites (runTurn's turnContext.run and chatStream's turnContext.enter — the latter is what channel adapters actually call and previously never carried a resolved identity for dataset-ACL purposes at all). - Point QueryDatasetTool.handle and ingestAttachments at that single shared field instead of each re-deriving/reading it independently. - Add a regression test in queryDatasetTool.test.ts simulating a channel turn's import-vs-query identity round trip.
…mn-type inference (#430) LEADING_ZERO_RE only matched an unsigned leading zero (`/^0\d/`), so a signed zero-padded value like '-0123'/'-0456' still passed NUMBER_RE (which allows an optional leading '-') without tripping the guard. The column was mistyped 'number' (Number() drops the leading zero after the sign, corrupting the value) and skipped the mandatory privacy scan — same defect class as the already-fixed unsigned case, just missed for signed values. Widen the pattern to /^-?0\d/, which still excludes a bare '0'/'-0' or a '0.x'/'-0.x' decimal (followed by nothing or '.', not another digit). Add a regression test with signed zero-padded values proving the column types as 'string', the value round-trips with sign and leading zero intact, and the privacy scan actually runs on it.
…nvelope (#430) POST / was the only one of the five dataset route handlers with no try/catch around its core call (importCsvDataset). An unexpected thrown error (e.g. a transient Postgres error inside NeonKnowledgeGraph.ingestDataset) fell through to Express 5's default error handler and returned an HTML error page instead of the {code, message} JSON envelope the other four handlers already return via mapErrorToHttp. Wraps the handler's importCsvDataset call in the same try/catch + mapErrorToHttp pattern already used by GET /, GET /:id, GET /:id/rows, and DELETE /:id. The existing structured {ok: false, reason} not-ok / privacy-rejection return path is unaffected. Adds a regression test in datasetsRoute.test.ts using a graph whose ingestDataset throws, asserting the route returns a JSON {code, message} body rather than an unhandled rejection.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds structured dataset ingestion to the Knowledge Graph: CSV files (uploaded via a new REST surface, or attached in chat) are parsed, privacy-scanned, and persisted as queryable datasets instead of being treated as opaque text blobs.
KnowledgeGraphsurface —ingestDataset,listDatasets,getDataset,queryDatasetRows,deleteDataset— backed by a relational sidecar (datasets+dataset_rowstables, migration0029_datasets.sql). Individual rows never become graph nodes; exactly oneDatasetnode (PluginEntity,system='dataset') is created per dataset for recall/citation linking. Implemented with full parity in both@omadia/knowledge-graph-neon(parameterized SQL/JSONB) and@omadia/knowledge-graph-inmemory.POST /api/v1/datasets(multipart CSV upload),GET /api/v1/datasets,GET /api/v1/datasets/:id,GET /api/v1/datasets/:id/rows,DELETE /api/v1/datasets/:id— ACL pattern mirrors/api/v1/memory(session-derived owner only, no anonymous access).query_datasetnative tool:list_datasets/get_schema/query_rows, a constrained filter+aggregate DSL (never raw SQL from the model), always paginated/aggregated server-side.@omadia/plugin-privacy-guard) before persistence. On a detected hit, the entire import is rejected (naming only the offending column/row indexes, never the flagged value) — the scan never silently masks or fabricates a substitute value. See fixup history below for why.Fixup history (adversarial review, 7 rounds)
inferColumnTypeno longer types a column asnumberwhen any value has a leading zero (e.g.'0301234567') — such values are zero-padded identifiers, not numbers; typing them asnumberboth corrupted the value (Number()drops the leading zero) and skipped the mandatory privacy scan.omadiaUserId(viaresolveOrCreateChannelIdentity) instead of writing the raw channel-native id asownerOmadiaUserId; per-cell truncation (MAX_CELL_CHARS) is no longer silent (truncationstats surfaced);NeonKnowledgeGraph'scontainsfilter escapes SQLILIKEwildcards;InMemoryKnowledgeGraph's grouped dataset query caps at 200 groups, matching Neon'sLIMIT 200.InMemoryKnowledgeGraph's dataset filter comparisons (eq/neq/contains) now coerce values by the column's declared type, mirroringNeonKnowledgeGraph's SQL cast behavior — previously a query could silently returntotalMatched: 0on the in-memory backend for a logically-identical Neon query.string/datecell throughmaskPrompt()— built for masking spans inside free-text chat prompts — and persisted only the masked/substituted text. Verified this silently, permanently corrupted legitimate structured values ('€72,000'→'€10000','24.12.1987'→'01.01.1970', a real address → a fabricated one), since the free-text baseline has a high false-positive rate on formatted structured data. Redesigned to detect-only, reject-on-hit: nothing is ever auto-substituted; a detected identifying span (email/IBAN/phone/address/date-in-free-text) rejects the whole import with a column+row-index error instead.resolveTurnOwnerIdentity) and shared between the CSV-import path andQueryDatasetTool— previously a dataset imported via a channel turn (Teams/Slack/Telegram) could never be found again byquery_datasetfrom that same channel, because the query path read the raw channel-native id while the dataset was stored under the resolved canonical id./^0\d/); a signed zero-padded value (-0123) still slipped through asnumber. Widened to/^-?0\d/.POST /api/v1/datasetswas the only one of the five dataset route handlers with notry/catcharound its core call — an unexpected thrown error (e.g. a transient Postgres failure) fell through to Express's default HTML error page instead of the{code, message}JSON envelope every other dataset endpoint returns. Fixed to match the other four handlers.Test plan
npm --prefix middleware run typecheck— 0 errors across all workspaces.npm --prefix middleware run lint— 0 errors.npm --prefix middleware test— full suite green modulo pre-existing, unrelated full-suite-only test-pollution flakes (each confirmed passing in isolation across every round).datasetImport.test.ts,datasetsRoute.test.ts,inMemoryKnowledgeGraph.test.ts,neonDatasetFilterEscaping.test.ts,orchestratorCsvDatasetIdentity.test.ts,orchestratorDispatcher.test.ts,queryDatasetTool.test.ts.31 files changed, +3444/-8 across 7 commits.
Addresses #430 (CSV import/query path). The admin upload/schema/delete UI required by #430's own acceptance criteria is intentionally deferred — tracked as #532.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.