fix(mt#4944): Target the JSONB member so knowledge search --sources returns results

[minsky-ai]

Summary

knowledge search --sources <x> builds filters: { sourceName: params.sources }, and PostgresVectorStorage renders a filter key as a bare column name — but sourceName is a member of knowledge_embeddings.metadata, not a column. Postgres folds the unquoted identifier to sourcename and raises 42703, and the command's catch turns that throw into { chunks: [], degraded: true }.

Reproduced against prod rather than inferred:

SELECT count(*) FROM knowledge_embeddings WHERE sourceName = 'minsky-design'
ERROR:  42703: column "sourcename" does not exist

Found by mt#4937's audit of the shared vector-search path, which was looking for a recall defect and established that one is latent. This is the different, live defect in the one caller that passes filters at all.

Correction found while implementing — this fix is necessary and NOT sufficient

Verifying the end-to-end command showed that knowledge.search returns { chunks: [], backend: "none", degraded: true } for every query — filtered and unfiltered:

knowledge_search { query: "minsky architecture", sources: ["minsky-design"] } → backend: "none", degraded: true
knowledge_search { query: "minsky architecture" }                            → backend: "none", degraded: true

That is a different, prior defect: registerKnowledgeCommands(targetRegistry, deps?) reads its vector storage from deps, and the only production caller (src/adapters/shared/commands/index.ts:135) invokes it with no arguments. So the command has never reached the vector store at all, and the 42703 sits behind that. Filed as mt#4946 — the mt#2508 caller-direction wiring defect in its purest form.

This does not weaken the fix here. The 42703 is verified by direct SQL against the real schema and would fire on every --sources query the moment the wiring lands; mt#4944 merging first is what keeps that from happening. But it does mean the task's own criterion 1 is not satisfiable by this PR, and it has been amended in the spec to say so rather than left to read as covered.

[sc1-deferred: mt#4946]

Key changes

1. buildFilterConditions gains a dotted key form. metadata.sourceName renders metadata->>$n with the member bound as a parameter, so only the column half is ever SQL text and rawIdentifier still guards it. A key with more than one dot is refused rather than guessed at — nested access needs #>> and a path array, and there is no caller for it.

This stays domain-agnostic per ADR-013 — "it filters by whatever column is named" — widening "column" to "column or JSONB member". No knowledge-specific concept enters that layer.

2. Array values do set membership. { k: ["a","b"] } now renders k IN ($1,$2) instead of binding an array to a scalar =, which matches nothing and reports no error — the same silent-zero shape. Empty array emits no predicate, symmetric with the existing *Exclude branch. The caller's z.array(z.string()) signature is unchanged: planning ruled out narrowing it to a scalar, since that is a command-parameter contract change with four consumers and unrelated to the defect.

3. The catch carries its reason. KnowledgeSearchResponse gains optional degraded / degradedReason / backend, populated via getLoggableErrorSummary — not getErrorMessage, because a DrizzleQueryError's .message is Failed query: … and the actual PG error (here the 42703) lives on .cause. A reason that omits the diagnosis satisfies the criterion's letter and none of its point.

Planning narrowed the fix shape, and ADR-013 is why

Criterion 2 arrived as a free three-way menu (JSONB predicate / domain post-filter / promoted column). ADR-013 decides it: its prohibition — "do not denormalize the mutable field into the shared index" — is scoped to a mutable field copied from another table's source of truth, and it explicitly preserves the store's capability. sourceName is written at index time from the document itself with no second row to drift from, so none of the three scope conditions bind.

Coordination

Touches src/adapters/shared/commands/knowledge/index.ts at the same catch line PR #3412 rewrites. That PR is mergeable_state: dirty and parked on an operator decision about its unreviewable 313-file diff, so waiting would block a live defect indefinitely. The conflict is now nearly content-identical: #3412 converts that line to getLoggableErrorSummary, which this PR also does, for its own reason.

Testing

Execution evidence:

bun test --preload ./tests/setup.ts packages/domain/src/storage/vector/postgres-vector-storage.test.ts
 23 pass / 0 fail / 64 expect() calls   (was 17 before this change)

Across all four affected suites, including sql-generation-proof.test.ts — which reaches this module only through a dynamic import and which the related-test selector therefore cannot see (mt#4945, filed for that gap and applied here by running it explicitly):

 65 pass
 0 fail
 228 expect() calls
Ran 65 tests across 4 files. [200.00ms]

Related-test gate on all three source files: 335 / 17 / 207 tests, all passing. lint:strict (--max-warnings=0) 0 errors 0 warnings; typecheck 0 errors across 8 projects; format clean.

SC1 — [sc1-deferred: mt#4946]. Not satisfiable here; see the correction section above. The evidence this task can produce is below.
SC2 — the dotted form, narrowed to ADR-013's default at planning, mapping recorded in the spec's ## Planning Audit (READY).
SC3 — the IN branch; signature deliberately unchanged.
SC4 — degradedReason on the response, via getLoggableErrorSummary so it carries the PG error rather than Drizzle's wrapper.

AT2 / AT3 — the predicate executed live, read-only, against prod. The fixed shape against the same table that raises 42703 for the old one:

SELECT count(*) FROM knowledge_embeddings WHERE metadata->>'sourceName' IN ('minsky-design')
 → 111

SELECT count(*) FROM knowledge_embeddings WHERE metadata->>'sourceName' IN ('definitely-not-a-source')
 → 0        (no error — an empty result that is genuinely empty)

111 is the full corpus, the expected count: knowledge_embeddings holds 111 rows carrying exactly one distinct sourceName. The two cases are distinguishable at the SQL layer, which is the layer this PR changes.

Negative control: reverted BOTH new mechanisms — filterTarget's dotted form and the array IN branch — and re-ran.

 17 pass
 6 fail
(fail) JSONB member targets > a dotted key targets a JSONB member, with the member name BOUND not interpolated
(fail) JSONB member targets > a key with more than one dot, or an empty member, is refused rather than guessed at
(fail) array set membership > an array value renders IN, not a scalar equality against an array
(fail) array set membership > set membership composes with a JSONB member target — the real knowledge shape
(fail) array set membership > a single-element array still uses IN, so one and many behave the same way
(fail) array set membership > an empty array emits no predicate, symmetric with the *Exclude branch

6 of 6 new tests red, all 17 pre-existing green — the control fires on exactly the new properties and nothing else. Restored before commit. Per mt#4512 this was a full revert of both mechanisms, not just the line I believed was load-bearing.

One error typecheck caught, recorded rather than quietly fixed: I declared backend as "vector" | "none" from memory; the emitted value is "embeddings". Four type errors across two projects. Corrected after reading the call sites.

Deploy verification

All four changed files are deploy surface — isDeploySurfaceFile returns true for each, run over the actual changed-file list rather than recalled. After merge I will run deployment_wait-for-latest with notBefore set to the merge timestamp and expectCommitSha set to the merge SHA, read buildIdentity rather than treating SUCCESS alone as proof, and correlate the deploy workflow run against the merge SHA when identity comes back indeterminate (expected for an image-source service). A tool or auth flake is a blocker to retry, not a licence to defer.

I will not claim a working knowledge search --sources post-deploy — mt#4946 is what makes that observable, and asserting it here would be the deploy-SUCCESS-equals-feature-works error this repo has already paid for.

🤖 Generated with Claude Code

https://claude.ai/code/session_01F1LhVjJvmY8oX8KxUK9vaA

[minsky-reviewer]

Minsky Reviewer Status

Verdict: APPROVED — no blocking findings
Review: View review
Model: openai/gpt-5 | Tokens: 201K prompt, 6K completion | Duration: 93s
Mode: normal

Commands

• /review — request a fresh review

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

---

Targeted fix looks solid: the JSONB-member dotted filter and array-IN rendering address the root causes, and the knowledge command now carries a degraded reason on errors. Tests assert the new SQL shapes and dispatch behavior. I found one non-blocking gap: when vector storage is unavailable the degraded response omits a degradedReason, reducing diagnosability; consider adding a brief reason for parity. Live behavior (SC1) cannot be verified from the diff, but SC2–SC4 are met via code and tests. No blocking issues; approve to merge.

Findings

• [NON-BLOCKING] src/adapters/shared/commands/knowledge/index.ts:62 — Degraded response when vector storage is unavailable lacks a degradedReason, reducing diagnosability
  At src/adapters/shared/commands/knowledge/index.ts:62-82, when vectorSearch is falsy the command returns a degraded response with backend: "none" and degraded: true but does not include a degradedReason. The PR adds degradedReason elsewhere to distinguish broken bindings from empty corpora, but this branch remains reason-less. For consistency and diagnosability, consider populating degradedReason here (e.g., "vector storage unavailable"). This isn't covered by SC4's catch-path but will help surfaces and tests distinguish configuration/runtime unavailability from a genuinely empty corpus.

Spec verification

Criterion	Status	Evidence
1. minsky knowledge search --sources minsky-design returns a non-empty result set against the live corpus, and --sources <nonexistent> returns an empty one WITHOUT an error being logged — the two cases are distinguishable in the output.	Unverifiable	Live-run behavior against production cannot be verified from the diff. The code change routes the filter correctly (src/adapters/shared/commands/knowledge/index.ts:218-227) and adds degraded signaling in the catch, but confirming non-empty/empty outputs requires a live environment the reviewer cannot access.
2. The filter is expressed against where sourceName actually lives. ... So option (a) — a metadata->>'sourceName' predicate on the store's own row — is the DEFAULT ...	Met	src/adapters/shared/commands/knowledge/index.ts:218-227 changes the filter to { "metadata.sourceName": params.sources }. packages/domain/src/storage/vector/postgres-vector-storage.ts:101-136 adds filterTarget() to render dotted keys as metadata->>$n; tests assert this shape (postgres-vector-storage.test.ts:77-102).
3. Array-valued filters do set membership (IN / = ANY), not scalar equality. ... do NOT resolve this by changing the caller to pass a scalar.	Met	packages/domain/src/storage/vector/postgres-vector-storage.ts:65-90 adds the array-handling branch rendering IN (...) and skipping empty arrays. Tests cover both column and JSONB-member cases (postgres-vector-storage.test.ts:104-153). Caller signature remains z.array(z.string()) (src/adapters/shared/commands/knowledge/index.ts:39-47).
4. The swallowing catch in knowledge/index.ts no longer renders a query failure as an empty success. A failure surfaces as degraded: true WITH the reason reaching the caller.	Met	Catch now uses getLoggableErrorSummary(error) and returns degraded: true, backend: "none", and degradedReason on failure (src/adapters/shared/commands/knowledge/index.ts:268-287). Additionally, the success path sets backend: "embeddings" and degraded: false (:249-266).

Documentation impact

• no-update-needed — I checked docs/cli for knowledge command references and did not find a doc that promises specific response fields for knowledge.search; this PR changes internal filter rendering and adds optional degraded fields already reflected in types. I did not sweep the entire docs tree; no CLI flags or command contracts changed. If a dedicated knowledge command doc exists elsewhere, it may benefit from mentioning degradedReason for diagnosability, but existing prose is not falsified by this change.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

---

Verified the follow-up commit addresses the prior NON-BLOCKING concern by adding a degradedReason to the early "vector storage not available" path. The main fixes — targeting metadata.sourceName via the dotted key, set-membership rendering for arrays, and using getLoggableErrorSummary with degradedReason in the catch — remain intact. I found no new defects introduced by the latest changes, and the amended spec’s constraints are respected (SC2–SC4 met; SC1 deferred to mt#4946). Approving to merge.

Spec verification

Criterion	Status	Evidence
1. minsky knowledge search --sources minsky-design returns a non-empty result set against the live corpus, and --sources <nonexistent> returns an empty one WITHOUT an error being logged — the two cases are distinguishable in the output. AMENDED: not satisfiable here; evidence limited to predicate rendering/tests and direct-SQL verification.	Not Met	Per the amended criterion, the end-to-end command cannot succeed until mt#4946 wires a real vector store into registerKnowledgeCommands. This PR adds degradedReason for both the early-return and catch paths (src/adapters/shared/commands/knowledge/index.ts:151-172, 297-331), but it does not alter the production wiring. The failure is intentionally deferred to mt#4946; this PR provides only the necessary lower-layer fixes and observability.
2. The filter is expressed against where sourceName actually lives (JSONB member in metadata).	Met	src/adapters/shared/commands/knowledge/index.ts:216 changes filters to use the dotted key { "metadata.sourceName": params.sources }. The storage layer supports this dotted-key JSONB-member form via filterTarget/buildFilterConditions (packages/domain/src/storage/vector/postgres-vector-storage.ts:49-108), rendering metadata->>$n with the member name bound as a parameter.
3. Array-valued filters do set membership (IN / = ANY), not scalar equality, and the caller signature remains an array.	Met	packages/domain/src/storage/vector/postgres-vector-storage.ts:74-89 implements the array IN branch (${filterTarget(key)} IN (...)) and explicitly treats empty arrays as no predicate. The caller continues to pass params.sources (an array) unchanged (src/adapters/shared/commands/knowledge/index.ts:216-223).
4. The swallowing catch no longer renders a query failure as an empty success; a failure surfaces as degraded: true with a reason reaching the caller.	Met	src/adapters/shared/commands/knowledge/index.ts:297-331 switches to getLoggableErrorSummary(error) and includes degradedReason in the returned payload. Additionally, the earlier “vector storage not available” degraded path now also carries a degradedReason (lines 151-172), ensuring both common failure shapes are distinguishable from an empty corpus.
1. minsky knowledge search --sources minsky-design returns a non-empty result set against the live corpus, and --sources <nonexistent> returns an empty one WITHOUT an error being logged — the two cases are distinguishable in the output. AMENDED: not satisfiable here; evidence limited to predicate rendering/tests and direct-SQL verification.	N/A	The spec explicitly amends SC1 as not satisfiable by this task (blocked by mt#4946). This PR confines itself to necessary lower-layer fixes and observability. End-to-end verification is deferred and owned by mt#4946 per the amended criterion text; therefore SC1 is not applicable to this PR’s diff.

Documentation impact

• no-update-needed — Bugfix to knowledge search filter construction and degraded-path observability; no new commands or flags added. Response now includes optional degradedReason alongside existing degraded/backend when failures occur, matching the task spec’s requirement for reasons to surface. No user-facing docs in this repo describe the internal error-shape of knowledge.search, and the CLI behavior (returning results vs degraded) is unchanged except for improved diagnostics.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

---

Re-verified the latest diff against the amended spec. The prior NON-BLOCKING concern about the early degraded path lacking a reason is addressed — degradedReason now accompanies both the no-provider early return and the catch path. The core fixes remain intact: dotted-key support targets JSONB members with the member name bound as a parameter, array-valued filters render as IN (including composition with JSONB targets and empty-array elision), and the knowledge command now passes "metadata.sourceName" for --sources. Unit tests exercise the new behaviors. I found no new critical defects introduced by this commit. SC2–SC4 are met; SC1 is correctly deferred to mt#4946 per the spec amendment. Approving to merge.

Spec verification

Criterion	Status	Evidence
1. minsky knowledge search --sources minsky-design returns a non-empty result set against the live corpus, and --sources <nonexistent> returns an empty one WITHOUT an error being logged — the two cases are distinguishable in the output. AMENDED: not satisfiable here due to mt#4946; evidence limited to predicate rendering and direct-SQL shape.	Not Met	Per the amended spec, end-to-end command verification is deferred to mt#4946. This PR updates the filter target and array handling but does not (and cannot) make the command reach the vector storage in production. Evidence this PR does provide: src/adapters/shared/commands/knowledge/index.ts:213-222 renders filters: { "metadata.sourceName": params.sources }; unit tests execute the generated predicates without 42703 (see packages/domain/src/storage/vector/postgres-vector-storage.test.ts:104-142, JSONB member tests). Follow-up required in mt#4946 to satisfy this criterion with a live run.
2. The filter is expressed against where sourceName actually lives (metadata JSONB member), per ADR-013 narrowing.	Met	src/adapters/shared/commands/knowledge/index.ts:213-222 switches from { sourceName: params.sources } to { "metadata.sourceName": params.sources }. The translation logic in packages/domain/src/storage/vector/postgres-vector-storage.ts:65-102 adds dotted-key support via filterTarget, rendering metadata->>$n with the member name bound as a parameter. Tests cover this at packages/domain/src/storage/vector/postgres-vector-storage.test.ts:104-131.
3. Array-valued filters do set membership (IN/= ANY), not scalar equality, without narrowing the caller’s array signature.	Met	packages/domain/src/storage/vector/postgres-vector-storage.ts:90-102 adds the array IN branch (and mirrors it for the *Exclude branch at :72-86). Tests assert IN rendering and composition with JSONB members, including empty-array elision: packages/domain/src/storage/vector/postgres-vector-storage.test.ts:144-191.
4. The swallowing catch no longer renders a query failure as an empty success; a degraded result carries a reason to the caller.	Met	src/adapters/shared/commands/knowledge/index.ts:313-329 switches to getLoggableErrorSummary(error) and returns { degraded: true, degradedReason, backend: "none" }. The early no-provider path now also includes a degradedReason (:147-170).

Documentation impact

• no-update-needed — Bugfix to filter rendering and error propagation. No user-facing CLI flags or semantics changed; --sources begins to work once mt#4946 wires the provider, but this PR alone does not change documented behavior. The optional backend/degraded/degradedReason fields are aligned with what the command already emitted and are typed here; no docs reference these internal response fields.