diff --git a/CHANGELOG.md b/CHANGELOG.md index a33322f..cd51e73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added +- MCP `add_pattern` and `update_pattern` accept an optional `language` argument that opts the + pattern into the structural retrieval gate, warn-and-proceed for unknown tokens. (#63) - `lore status` inserts a new `Languages:` line between `Sources:` and `Last commit:` reporting a per-language source-count breakdown plus an `undeclared` bucket; the same data is exposed as `languages_declared` / `languages_undeclared` on the MCP `lore_status` tool's metadata fence for diff --git a/ROADMAP.md b/ROADMAP.md index 8e9d219..5ef0f7c 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -8,14 +8,6 @@ refactor validation, debug, and continuous dogfooding. Builds on `default_trace_dir()` from the etcetera refactor (PR #52) and the three-list RRF pipeline from language detection (PR #50). See `docs/brainstorms/2026-05-14-track-2-observability-requirements.md`. -- [ ] Accept `language` on the `add_pattern` / `update_pattern` / `append_to_pattern` MCP tools — - the language table now covers 27 entries, but the MCP authoring path silently drops any - `language` argument because the input schema does not list it. Agents that create patterns via - MCP cannot declare a language at all; only manually-authored markdown picks up the structural - retrieval gate. Add the field to each tool's `inputSchema`, plumb it through the pattern-write - path so the resulting file carries `language:` frontmatter, and validate tokens against - `is_known_token` so unknown tokens hit the same R12 warn-and-proceed path that ingest already - uses. ## Future @@ -61,6 +53,17 @@ ## Completed +- [x] Accept `language` on the `add_pattern` and `update_pattern` MCP tools — `inputSchema` now + declares the field (`oneOf [string, array]`), the handler coerces scalar input to an + array at the boundary, and ingest renders a canonical `language: [...]` flow-list line into + the file's frontmatter. `update_pattern` mirrors `tags`'s three-way semantics (omit preserves, + `[]` clears, non-empty replaces) so body-only rewrites do not silently de-language a pattern. + Unknown tokens warn-and-proceed: every offending token reaches the caller via + `WriteResult.language_warnings` and the response's `lore-metadata` fence, including on the + inbox-branch short-circuit (which previously skipped indexing and silently dropped the + advisory). `append_to_pattern` deliberately does not accept `language` — appends are body-only + by definition, and Decision 3's regression pin in the schema tests fails fast if a future PR + adds the field. See `docs/plans/2026-05-19-001-feat-mcp-language-arg-plan.md`. - [x] Hook-driven Bash language inference reads every signal — both `command` and `description` now contribute to language detection (fixing the prior bug where a populated `description` silently shadowed the `command`), and path-prefixed invocations like `./gradlew`, diff --git a/docs/plans/2026-05-19-001-feat-mcp-language-arg-plan.md b/docs/plans/2026-05-19-001-feat-mcp-language-arg-plan.md new file mode 100644 index 0000000..2e88dba --- /dev/null +++ b/docs/plans/2026-05-19-001-feat-mcp-language-arg-plan.md @@ -0,0 +1,600 @@ +--- +title: "feat: Accept language on MCP pattern-authoring tools" +type: feat +status: active +created: 2026-05-19 +origin: tmp/mcp-language-arg-ce-plan-prompt.md +pr: https://github.com/attila/lore/pull/63 +--- + +# feat: Accept `language` on `add_pattern` / `update_pattern` / `append_to_pattern` + +## Summary + +The MCP pattern-authoring tools silently drop a `language` argument today because their +`inputSchema`s do not declare the field. Agents that create or modify patterns via MCP cannot opt +patterns into the structural retrieval gate (PR #50); only hand-authored markdown frontmatter does. +This plan closes the gap on the MCP surface only: extend the two relevant tools' input schemas, +validate tokens through the same `is_known_token` predicate the ingest path uses, write canonical +`language:` frontmatter into the pattern file, and surface unknown-token advisories on both stderr +and the tool response's metadata fence. + +The implementation rides on existing infrastructure — `parse_frontmatter_language_list` already +validates tokens and emits `MalformedLanguageEntry` advisories during chunking, and +`index_single_file` already aggregates them. The new work is at the MCP layer: schema, coercion, +frontmatter rendering, and advisory propagation into `WriteResult`. + +--- + +## Problem Frame + +PR #50 introduced the `language:` frontmatter field and the `language_json` column that drives the +U5 structural retrieval gate. PR #61 expanded the canonical `LANGUAGES` table to 27 entries. The +agent-authoring surface, however, was left behind: + +- `add_pattern`, `update_pattern`, and `append_to_pattern` have no `language` field on their + `inputSchema`. +- The handlers in `src/server.rs` do not read a `language` argument, and `ingest::add_pattern` / + `update_pattern`'s `build_file_content` only renders `tags:` frontmatter, never `language:`. +- An agent that writes a Rust pattern via `add_pattern` produces a file with no `language:` + frontmatter, which means the resulting chunk lands on the FTS-fallback path (R10), not the + structural retrieval gate (R12) — the exact gap PR #50 was built to close. + +The user-visible failure mode: MCP-authored patterns silently underperform on language-scoped +queries compared to hand-authored ones, with no agent-visible signal that the argument was dropped. + +--- + +## Scope + +### In scope + +- Add a `language` field to the `inputSchema` of `add_pattern` and `update_pattern`. Accept both + string and array-of-strings input. +- Plumb the value through `ingest::add_pattern` / `update_pattern` so `build_file_content` renders a + canonical `language:` line into the file's frontmatter. +- Validate every passed token against `crate::engine::is_known_token`. Unknown tokens warn but + proceed (R12). +- Surface unknown-token advisories on stderr (matches the existing ingest log line) and on the tool + response's `lore-metadata` fence as a `language_warnings` array. +- Update tool descriptions to document the new field and the unknown-token policy. +- CHANGELOG entry; move the ROADMAP item from Up Next to Completed in the same diff. + +### Deferred to Follow-Up Work + +- Surfacing `language:` on the `list_patterns` MCP output (the `Pattern` row would need to carry the + parsed token list). Tracked by the table-expansion follow-up. +- A `language_json` column read on the search-side metadata fence (today the column is only exposed + via `lore_status`). + +### Outside this product's identity + +- **No new MCP tool.** The work is additive on three existing schemas. +- **No engine code change.** `is_known_token`, `LANGUAGES`, and `parse_frontmatter_language_list` + stay as-is. +- **No language inference from body content.** The field is purely declarative; we do not + heuristically guess a language from prose, code fences, or filenames passed via the title. +- **No DB schema migration.** `language_json` was populated by the chunking path on every write + since PR #50; this work only changes what the chunking path sees in the on-disk frontmatter. +- **No change to `append_to_pattern`'s contract.** Append is body-only by definition; see Key + Technical Decisions §3. + +--- + +## Key Technical Decisions + +### 1. Input shape: `oneOf` scalar or array, handler coerces + +The frontmatter parser (`parse_frontmatter_language_list`) already accepts three YAML shapes — +scalar, flow list, block list. The MCP `inputSchema` will mirror this with JSON Schema `oneOf`: + +```json +"language": { + "oneOf": [ + { "type": "string" }, + { "type": "array", "items": { "type": "string" } } + ], + "description": "Optional language declaration..." +} +``` + +The handler coerces a scalar input to a one-element `Vec` before further processing. +Rationale: + +- Mirrors the YAML surface the manual authoring path supports — agents that round-trip a file + through search → update will not need to reshape. +- The single-language case (the dominant case for new patterns) stays ergonomic — agents pass + `"language": "rust"`, not `"language": ["rust"]`. +- Coercion happens once at the boundary; the rest of the pipeline (validation, frontmatter + rendering) sees a uniform `&[String]`. + +Rejected: array-only schema with no scalar. The cost of the slightly more permissive shape is a +five-line coercion in the handler; the upside is parity with the YAML surface and friendlier +ergonomics for the dominant case. + +### 2. `update_pattern` language semantics: three-way matching `tags` + +`update_pattern`'s `tags` argument already establishes a three-way semantics for frontmatter list +fields (`src/server.rs:875-885`, `src/ingest.rs:1253-1294`): + +- **absent** → preserve the existing frontmatter list +- **`[]`** → clear +- **`[...]`** → replace wholesale + +The `language` argument adopts the same three-way semantics for the same reason: an agent that calls +`update_pattern` to rewrite the body without re-supplying `language` should not silently de-language +the pattern (the analogue of the de-universalisation footgun that motivated `tags`'s +preserve-on-`None` semantics). + +Rejected: + +- **Merge (union with existing)**: cute but surprising — an agent that intentionally narrows + `[swift, objectivec]` to `[swift]` cannot. Replace is the more honest primitive; agents that want + merge can list-patterns first. +- **Set-if-absent**: silently ignoring a passed argument when the field is already set creates the + same "dropped argument" failure mode this plan exists to fix. + +### 3. `append_to_pattern` does not accept `language` + +`append_to_pattern` is body-only by definition: it appends a heading and body to the existing file +without touching frontmatter. The right place to change frontmatter is `update_pattern`. Adding a +`language` argument to append would either: + +- Be a no-op the schema lies about (the schema says "accepted", the handler ignores it), or +- Quietly mutate frontmatter on what an agent thinks is a body-only operation (worse — same class of + footgun as the `tags`-on-update default-clear that already cost us once). + +Schema honesty beats surface symmetry. Agents that want to add a language declaration to an existing +pattern call `update_pattern` (which now accepts `language`) or edit the markdown directly. The +append tool's description gains a single line pointing at this. + +### 4. Unknown-token surfacing: stderr line + `language_warnings` on the metadata fence + +When `parse_frontmatter_language_list` is invoked on the newly-written file during +`index_single_file`, it already emits `MalformedLanguageEntry` advisories for unknown tokens; the +entry-bearing path already logs each entry to stderr inside `index_single_file`. This plan does not +duplicate that stderr line — it just makes sure the new advisories from MCP-authored writes flow +through the same code path. + +The new surface is on the tool response's `lore-metadata` fence. `WriteResult` gains a +`language_warnings: Vec` field (collected from the `IndexedFile`'s `malformed_language` +advisories, mapped to the bare lowercased tokens). The handler renders it into the metadata JSON as: + +```json +"language_warnings": ["objectiv-c"] +``` + +— an array of strings naming the offending tokens, lowercased, deduplicated, first-seen order. Empty +array (not `null`) when every token validated; field always present so agents can pattern-match on +the key. Field name matches the existing `warnings: []` convention used by the trace fence +(`src/server.rs:1227`). + +Rejected: + +- **stderr only**: invisible to agents using MCP, which is the surface this plan is fixing. The + whole point is that the authoring path needs an agent-observable signal. +- **Metadata fence only**: breaks consistency with the ingest path's stderr log, which operators + rely on when debugging language coverage from the CLI. +- **Richer objects (`{token, file_path}`)**: not actionable on the MCP side — the agent already + knows which tokens it just passed. Strings are enough. + +--- + +## High-Level Technical Design + +``` +Agent JSON-RPC call (language: "rust" or ["java","kotlin"]) + │ + ▼ +src/server.rs::handle_add / handle_update + • read args.language (Value) + • coerce scalar → Vec via parse_language_arg(arg) + • check_limit on serialised length (reuse check_tags_limit pattern) + │ + ▼ +ingest::add_pattern(..., language: Option<&[&str]>) +ingest::update_pattern(..., language: Option<&[&str]>) // three-way per Decision 2 + • build_file_content(title, body, tags, language) → "---\ntags: [...]\nlanguage: [rust]\n---\n..." + • std::fs::write(file_path, content) + │ + ▼ +index_single_file (unchanged) + • parse_frontmatter_language_list validates against is_known_token + • returns ChunkingAdvisories { malformed_language, ... } + │ + ▼ +WriteResult { ..., language_warnings: Vec } + │ + ▼ +Tool response prose + lore-metadata fence: + { "language_warnings": ["objectiv-c"], ... } +``` + +_Directional guidance for review, not implementation specification._ + +--- + +## Implementation Units + +### U1. Plumb `language` arg through `add_pattern` and `update_pattern` + +**Goal:** Accept the `language` argument on the two MCP tools, coerce input shape, and pass it +through to `ingest::add_pattern` / `update_pattern`. No frontmatter rendering yet — that's U2. + +**Dependencies:** None. + +**Files:** + +- `src/server.rs` — tool definitions (`add_pattern`, `update_pattern`) `inputSchema`s; `handle_add`, + `handle_update` arg extraction +- `src/ingest.rs` — `add_pattern`, `update_pattern` signatures (add `language: Option<&[&str]>` + parameter) + +**Approach:** + +- Add a `parse_language_arg(args: &Value) -> Result>, String>` helper in + `src/server.rs` that handles the three cases: absent (`None`), string (coerce to one-element vec), + array (collect strings). Reject non-string/non-array shapes with a structured error. For + `update_pattern`, the absent vs `[]` vs `[...]` distinction has to be preserved end-to-end — + return `Option>` so absent maps to `None`, `[]` maps to `Some(vec![])`, `[x]` maps to + `Some(vec!["x"])`. For `add_pattern`, an empty list is equivalent to absent (no semantic + difference on create) — fold both to `None` for that call site. +- Add a `MAX_LANGUAGE_BYTES` limit (mirror `MAX_TAGS_BYTES` shape; serialised JSON size cap) and a + `check_language_limit` helper that runs the same serialised-size check as `check_tags_limit`. +- Pass the coerced value into the ingest helpers as `Option<&[&str]>`. For now, the ingest helpers + accept the parameter but ignore it (U2 wires it into `build_file_content`). + +**Patterns to follow:** + +- `tags` extraction in `handle_add` (`src/server.rs:808-816`) for the array-of-string shape. +- `tags` three-way handling in `handle_update` (`src/server.rs:875-889`) for the `Option>` + lifetime dance. +- `check_tags_limit` (`src/server.rs:607-618`) for serialised-size validation. + +**Test scenarios:** + +- `add_pattern` with `"language": "rust"` → handler coerces to `vec!["rust"]` and passes through to + `ingest::add_pattern`. +- `add_pattern` with `"language": ["java", "kotlin"]` → array preserved. +- `add_pattern` with `"language": []` → folded to `None` (or `Some(vec![])`; either is fine as long + as the file ends up without a `language:` line in U2). +- `add_pattern` with no `language` key → `None`. +- `add_pattern` with `"language": 42` → handler returns the structured + `"language must be a string or array of strings"` error. +- `update_pattern` with no `language` key → `Option>` is `None` (preserves on update per + Decision 2). +- `update_pattern` with `"language": []` → `Some(vec![])` (clears on update per Decision 2). +- `update_pattern` with `"language": ["go"]` → `Some(vec!["go"])` (replaces on update per Decision + 2). +- Serialised `language` larger than `MAX_LANGUAGE_BYTES` → returns the limit-exceeded error before + reaching the ingest layer. +- The two existing snapshot/golden tests for `add_pattern` / `update_pattern` calls that omit + `language` still pass unchanged (parameter is `None`-defaulted, no behaviour change). + +Schema-shape assertions on `tool_definitions()` (the field is added here; U4 re-asserts these +alongside description-prose drift checks): + +- `add_pattern.inputSchema.properties.language.oneOf` exists with the two-branch `string` / + `array` shape. +- `add_pattern.inputSchema.required` does not contain `language`. +- `update_pattern.inputSchema.properties.language` matches the same `oneOf` shape. +- `update_pattern.inputSchema.required` does not contain `language`. +- `append_to_pattern.inputSchema.properties` does not contain `language` (negative regression guard + — Decision 3). + +**Verification:** Unit tests for `parse_language_arg` cover the shape matrix; existing MCP tool +integration tests still pass. + +--- + +### U2. Render `language:` frontmatter on write + +**Goal:** Extend `build_file_content` so it emits a canonical `language:` line into the frontmatter. +Wire the parameter from U1 through `add_pattern` / `update_pattern` so the file on disk reflects +what the MCP call requested. Implement the three-way `update_pattern` semantics from Decision 2. + +**Dependencies:** U1. + +**Files:** + +- `src/ingest.rs` — `build_file_content`; `update_pattern`'s preserve-on-`None` branch (mirror the + `tags` logic for `language`); `add_pattern`'s frontmatter rendering + +**Approach:** + +- `build_file_content(title, body, tags, language)` builds the frontmatter block in canonical order: + `tags:` first, then `language:`, both rendered as flow lists. A `language: [rust]` block emits + even for a single-element list — flow list matches what the parser already accepts and avoids the + scalar/list serialisation choice. Empty list does not render a line. +- `update_pattern`: when `language: None` is passed, read the existing file via + `parse_frontmatter_language_list` and pass the preserved list to `build_file_content`. When + `Some(vec![])`, render no `language:` line. When `Some([...])`, render the new list. Mirror the + existing `tags` preserve-or-replace branch precisely. +- `add_pattern`: pass the (possibly empty) list straight to `build_file_content`; no preserve case. + +**Patterns to follow:** + +- `update_pattern`'s `tags` three-way handling (`src/ingest.rs:1282-1294`) — the `preserved` / + `preserved_refs` lifetime dance is the precedent. +- `parse_frontmatter_tag_list` (`src/chunking.rs`) — the analogue parser for the language preserve + path. +- `parse_frontmatter_language_list` for the read path on preserve. + +**Test scenarios:** + +- `add_pattern` with `language: ["rust"]` writes a file whose first non-empty content is + `---\nlanguage: [rust]\n---\n\n# Title\n\nBody\n`. +- `add_pattern` with `language: ["java", "kotlin", "groovy"]` writes + `language: [java, kotlin, groovy]`. +- `add_pattern` with `language: None` (or empty) writes no `language:` line — file matches today's + output byte-for-byte (regression guard for the pre-`language` shape). +- `add_pattern` with both `tags: ["universal"]` and `language: ["rust"]` writes + `tags: [universal]\nlanguage: [rust]` in that order. +- `update_pattern` with `language: None` on a file declaring `language: [swift, objectivec]` writes + back `language: [swift, objectivec]` — the preserve case, the de-universalisation-class footgun + analogue. **This is the critical test for Decision 2.** +- `update_pattern` with `language: Some(vec![])` on a file declaring `language: [rust]` writes back + no `language:` line — the explicit-clear case. +- `update_pattern` with `language: Some(vec!["go"])` on a file declaring `language: [rust]` writes + back `language: [go]` — the replace case. +- After every successful `add_pattern` / `update_pattern` with `language: Some(["rust"])`, the + database row for that source has `language_json = '["rust"]'` (verified via direct DB read — this + is the end-to-end assertion mandated by the slice-shape-vs-pipeline-tests learning). + +**Verification:** File contents match the expected frontmatter shape; DB row's `language_json` +matches what the agent passed. + +--- + +### U3. Surface unknown-token advisories on `WriteResult` and metadata fence + +**Goal:** When any passed token fails `is_known_token`, surface the offending tokens on the tool +response's `lore-metadata` fence as `language_warnings: [...]`. Stderr advisories already flow +through `index_single_file` and need no new code. + +**Dependencies:** U2. + +**Files:** + +- `src/ingest.rs` — add `language_warnings: Vec` to `WriteResult`; populate from the + `IndexedFile`'s `malformed_language` field +- `src/server.rs` — render the field into the `lore-metadata` fence JSON in `handle_add` and + `handle_update` + +**Approach:** + +- After `index_single_file` returns, map `indexed.malformed_language` to a `Vec` of the bare + `token` field, dedup while preserving first-seen order, and assign to + `WriteResult::language_warnings`. Empty vec (not `None`) when every token validated. +- In `handle_add` and `handle_update`, add `"language_warnings": result.language_warnings` to the + metadata JSON. Always present, defaults to `[]`. +- **Inbox-branch path parity.** The inbox-branch short-circuit (`src/ingest.rs:1167-1184` for add, + `src/ingest.rs:1298-1316` for update) writes the file to a new branch and pushes without invoking + `index_single_file`, so it never sees the chunking parser's advisories. Closing this gap inside + this plan: before the short-circuit returns, call + `parse_frontmatter_language_list(&content, &filename)` directly on the about-to-be-written + content, dedup the malformed entries' tokens, populate `WriteResult.language_warnings`, and emit + the same `[lore]` stderr line per offending token that `index_single_file` does. The parser is + pure (`&str` in, advisories out — no DB, no embedder, no I/O) so the cost is roughly ten lines and + one new helper invocation per short-circuit site. Closing this here matters because inbox-branch + is the agent-submission contract; leaving it advisory-blind would silently re-introduce the + dropped-argument failure mode the plan exists to fix, just on a different code path. + +**Patterns to follow:** + +- `embedding_failures: Vec` flow on `IndexedFile` → `WriteResult` → metadata JSON (already wired + end-to-end and is the closest precedent). +- `warnings: []` field on the trace metadata fence (`src/server.rs:1227`) for the field-name + convention. + +**Test scenarios:** + +- `add_pattern` with `language: ["rust", "objectiv-c"]` → file gets `language: [rust, objectiv-c]`; + metadata fence includes `"language_warnings": ["objectiv-c"]`; stderr carries the `[lore]` + advisory line for `objectiv-c` (one line per offending token, already produced by + `index_single_file`). +- `add_pattern` with all-valid tokens → metadata fence includes `"language_warnings": []` (present, + empty). +- `add_pattern` with the same unknown token passed twice → `language_warnings` contains the token + once (dedup). +- `update_pattern` with `language: ["objectiv-c"]` replacing `language: [rust]` → file's frontmatter + updates; metadata fence includes the warning for `objectiv-c`. +- `update_pattern` with `language: None` on a file whose existing frontmatter contains an unknown + token from an earlier write → `language_warnings` contains the preserved unknown token + (re-validates on every write, matching ingest semantics). +- Inbox-branch path: `add_pattern` with `inbox_branch_prefix: Some(_)` and + `language: ["objectiv-c"]` → `WriteResult.language_warnings` contains `["objectiv-c"]` (advisory + fires via the direct `parse_frontmatter_language_list` call on the short-circuit path); stderr + carries the `[lore]` advisory line. +- Inbox-branch path: `update_pattern` with `inbox_branch_prefix: Some(_)` and + `language: ["rust", "objectiv-c"]` → `WriteResult.language_warnings` contains `["objectiv-c"]`; + the valid `rust` token does not appear. +- Inbox-branch path with all-valid tokens (`language: ["rust"]`) → `WriteResult.language_warnings` + is empty `[]` (the field is present, not omitted). + +**Verification:** Metadata fence carries the expected `language_warnings` array; stderr line is +unchanged from today's ingest advisory shape. + +--- + +### U4. UAT, tool descriptions, ROADMAP move, CHANGELOG + +**Goal:** Documentation, real-binary UAT, and the housekeeping that closes the ROADMAP entry. + +**Dependencies:** U1, U2, U3. + +**Files:** + +- `src/server.rs` — tool descriptions for `add_pattern`, `update_pattern` (document the new + `language` field and its semantics); `append_to_pattern`'s description gains one sentence pointing + at `update_pattern` for frontmatter changes +- `ROADMAP.md` — move the entry from `## Up Next` to `## Completed` +- `CHANGELOG.md` — single bullet per the two CHANGELOG conventions (user-facing only, one sentence + ending in `(#N)`) +- `docs/pattern-authoring-guide.md` (if it has a section on `language:`) — note that the MCP + authoring tools now accept the same field +- `tmp/mcp-language-arg-uat.md` — disposable per-PR UAT runbook (not committed; lives in `tmp/`) + +**Approach:** + +- Run the UAT through the real binary per the UAT-through-real-binary learning: build the release + binary, start `lore serve` against an isolated XDG environment, drive a JSON-RPC `add_pattern` + call with `language: "rust"`, confirm the on-disk file carries `language: [rust]`, run + `lore ingest`, then `lore status` and assert the `Languages:` line includes `rust: 1`. Repeat with + an unknown token to confirm both surfaces (stderr + metadata fence) fire. +- Cross-surface grep per multi-surface-consistency: `git grep -nF 'add_pattern'`, + `git grep -nF 'language'` (filtered to the surfaces this PR touches), + `git grep -nF 'update_pattern'` — confirm every doc reference quoting these tools matches the new + signature. +- ROADMAP move and CHANGELOG bullet land in the same commit/PR (per the project's + ROADMAP-update-in-feature-PR convention). + +**Patterns to follow:** + +- `docs/plans/2026-05-15-001-feat-language-in-status-plan.md` — the prior PR that extended an MCP + tool's metadata fence. Its CHANGELOG entry shape is the template. +- `docs/solutions/best-practices/uat-through-real-binary-catches-inference-path-bugs-2026-05-19.md` + — UAT discipline. +- `docs/solutions/best-practices/slice-shape-tests-are-not-pipeline-tests-2026-05-19.md` — why U2 + and U3 include end-to-end DB-state assertions, not just file-shape assertions. + +**Test scenarios:** + +Schema shape — `add_pattern`: + +- `tool_definitions()["add_pattern"].inputSchema.properties.language` exists. +- `language.oneOf` is a two-element array; first branch is `{type: "string"}`, second is + `{type: "array", items: {type: "string"}}`. +- `language` is NOT present in `inputSchema.required`. +- `inputSchema.required` still contains `title` and `body` (regression guard). +- The existing properties `title`, `body`, `tags`, and `include_metadata` are still present + (regression guard against accidental dropping). +- `language.description` is non-empty. + +Schema shape — `update_pattern`: + +- Same six assertions as above, with `source_file` substituted for `title` in the required-fields + regression guard. +- `language.description` substring-matches the three-way semantics vocabulary (e.g., `preserve`, + `clear`, `replace`) so the contract documentation cannot silently drift back to the de-language + footgun. + +Schema shape — `append_to_pattern` (the load-bearing Decision 3 regression pin): + +- `tool_definitions()["append_to_pattern"].inputSchema.properties` does NOT contain a `language` + key. +- The existing properties `source_file`, `heading`, `body`, and `include_metadata` are still + present. +- `tool_definitions()["append_to_pattern"].description` substring-matches `update_pattern` (the + pointer that tells agents where frontmatter changes belong). + +Tool description prose: + +- `add_pattern.description` substring-matches `language` and mentions the unknown-token advisory + behaviour (substring match on a stable noun such as `unknown` or `warn`). +- `update_pattern.description` substring-matches `language` and mentions the same advisory + behaviour. + +Top-level catalogue integrity: + +- The `tools/list` JSON-RPC response still contains all three tool names (`add_pattern`, + `update_pattern`, `append_to_pattern`) — regression guard against careless edits. + +Out of automated scope (reviewer + UAT runbook): + +- ROADMAP entry moved from `## Up Next` to `## Completed`, referencing this plan path. +- CHANGELOG bullet present and follows the project's two CHANGELOG rules (user-facing only, one + assertive-voice sentence ending in `(#N)`). +- `docs/pattern-authoring-guide.md` updated if it has a section quoting the MCP tool surface. + +**Verification:** all schema-shape and description-content assertions pass as unit tests against +`tool_definitions()`; UAT runbook passes end-to-end against the freshly-built binary; `cargo test` +green; `cargo clippy -- -D warnings` clean; ROADMAP entry sits under Completed referencing this plan +path. + +--- + +## Test Strategy + +Three layers, matching the slice-shape-vs-pipeline-tests learning: + +1. **Unit (per implementation unit, in `#[cfg(test)] mod tests`).** Argument coercion (U1), + frontmatter rendering (U2), advisory propagation (U3). Driven against + `KnowledgeDB::open(":memory:")` and `FakeEmbedder`. These are the fast feedback loop; they don't + prove the pipeline. + +2. **End-to-end via the MCP server (U2 and U3).** Drive the server through its JSON-RPC interface, + assert the file contents on disk **and** the resulting `language_json` value in the DB. + Slice-shape tests pass even when ingest is broken; the pipeline test catches the regression. + +3. **UAT through the real binary (U4).** Build, run, drive `lore serve` over the production stdio + transport from an isolated XDG environment, exercise both the happy path and the unknown-token + path. The PR is not ready for merge until this passes. + +The four feature-bearing units above (U1, U2, U3) each have explicit DB-state assertions wherever +the unit produces a database-observable effect. The unit-level scenarios are the contract; the UAT +runbook is the smoke. + +--- + +## Risk Analysis + +### R1. Existing `language:` frontmatter is silently overwritten on update + +If `update_pattern` is called without `language` against a file that declares +`language: [swift, objectivec]`, and the preserve path has a bug, the language line silently +disappears — same class of footgun as the `tags` de-universalisation incident that motivated +`tags`'s preserve-on-`None` behaviour. **Mitigation:** the critical preserve-case test in U2 pins +this exact scenario; the implementation mirrors `tags`'s precedent verbatim. + +### R2. Coercion path admits malformed input that crashes the chunking parser + +The frontmatter parser strips `,` and control characters as a defence; the MCP layer should not +duplicate that filter, but it should also not let in shapes the parser cannot handle. +**Mitigation:** the `parse_language_arg` helper accepts only `string` and `array` and +rejects every other JSON shape; the unit test matrix in U1 covers the reject cases. + +### R3. Existing snapshot tests for `add_pattern` file output break + +`build_file_content` is touched. Any existing snapshot test of "an `add_pattern` call with no +`language` produces this exact bytes" would fire as a failure if the frontmatter rendering changes +shape even when `language` is absent. **Mitigation:** the absent-language case in U2 explicitly pins +byte-for-byte equality with today's output (no `language:` line, no extra blank lines). The change +is purely additive on the `language: Some(...)` path. + +### R4. The inbox-branch path silently swallows the advisory + +The inbox-branch short-circuit (`src/ingest.rs:1167-1184` and `1298-1316`) writes a file and pushes +to a remote without running `index_single_file` locally, so the chunking-driven advisory never fires +for those writes. **Mitigation:** closed inside U3 — both short-circuits invoke +`parse_frontmatter_language_list(&content, &filename)` directly on the about-to-be-written content +and populate `WriteResult.language_warnings` from its `malformed_language` output before returning. +The same stderr line that `index_single_file` emits for unknown tokens fires on the short-circuit +path too, so CLI and inbox-branch observability stay aligned. The dedicated inbox-branch test +scenarios in U3 pin this. + +--- + +## Workflow Notes + +- All work happens inside the sibling worktree at `/srv/misc/Projects/lore/lore-mcp-language-arg/` + on branch `feat/mcp-language-arg`. +- The main checkout at `/srv/misc/Projects/lore/lore/` is not touched. +- Plan written to `docs/plans/2026-05-19-001-feat-mcp-language-arg-plan.md` inside the worktree. + +## References + +- ROADMAP entry under `## Up Next` (the `add_pattern`/`update_pattern`/`append_to_pattern` line) +- `docs/plans/2026-05-14-001-feat-language-detection-architecture-plan.md` — the architecture that + introduced `language:` frontmatter and `language_json` +- `docs/plans/2026-05-15-001-feat-language-in-status-plan.md` — the precedent MCP metadata-fence + extension this plan mirrors +- `docs/plans/2026-05-18-001-feat-language-table-expansion-plan.md` — the data-only PR that expanded + the canonical token table to 27 entries +- `src/server.rs` — tool definitions, handlers, `WriteResult` metadata rendering +- `src/ingest.rs` — `add_pattern`, `update_pattern`, `append_to_pattern`, `build_file_content`, + `WriteResult`, `IndexedFile` +- `src/chunking.rs` — `parse_frontmatter_language_list`, `MalformedLanguageEntry` +- `src/engine/languages.rs` — `is_known_token`, `LANGUAGES` +- `docs/solutions/best-practices/slice-shape-tests-are-not-pipeline-tests-2026-05-19.md` +- `docs/solutions/best-practices/uat-through-real-binary-catches-inference-path-bugs-2026-05-19.md` diff --git a/docs/solutions/best-practices/schema-and-description-prose-are-testable-surface-2026-05-19.md b/docs/solutions/best-practices/schema-and-description-prose-are-testable-surface-2026-05-19.md new file mode 100644 index 0000000..7732f5a --- /dev/null +++ b/docs/solutions/best-practices/schema-and-description-prose-are-testable-surface-2026-05-19.md @@ -0,0 +1,206 @@ +--- +title: "Schema JSON and tool description prose are testable surface, not docs" +date: 2026-05-19 +category: best-practices +module: mcp-server +problem_type: best_practice +component: tooling +severity: medium +applies_when: + - "Shipping a feature whose deliverable is a schema field, an MCP tool description, or any public contract object" + - "Scoping a plan unit that touches only documentation text on a contract surface (tool descriptions, JSON Schema, OpenAPI, GraphQL SDL)" + - "Tempted to annotate `Test expectation: none -- documentation` on a unit whose output is a contract artefact rather than behaviour" + - "Reviewing a PR that adds a field to a schema but no test that asserts the field's presence or shape" +tags: + - testing + - mcp + - schema + - contract-tests + - drift-guard + - regression-pin +--- + +# Schema JSON and tool description prose are testable surface, not docs + +## Context + +While planning PR #63 (`feat: accept language arg on add_pattern / update_pattern MCP tools`), the +final implementation unit (U4) was scoped as "tool descriptions + ROADMAP move + CHANGELOG bullet." +The plan annotated its test scenarios as `Test expectation: none -- documentation, manual UAT only`, +treating the schema descriptions as untestable docs. + +The user pushed back: _"I want automated unit tests for the feature implemented. provided we have +unit tests for the mcp interface, this is not foreign idea, right?"_ — and on follow-up: _"I want +full test coverage on all meaningful angles. unit tests are cheap, run them."_ + +That reframing pointed at the actual testable surface in U4: + +- The `language` property present on `add_pattern.inputSchema.properties` with the right `oneOf` + shape. +- The `language` property present on `update_pattern.inputSchema.properties` with the same shape. +- The `language` property **absent** from `append_to_pattern.inputSchema.properties` — load-bearing, + because a future "let's add it for symmetry" PR would silently undo a deliberate design decision. +- Each tool's `description` string mentions specific stable nouns (`language`, `update_pattern`, + `preserve`/`clear`/`replace`) that document the contract agents read when listing tools. +- The top-level `tools/list` catalogue still contains every expected tool name. + +Every one of these is a substring or structural assertion against `tool_definitions()`'s JSON +output. Cheap to write, fast to run, and they catch a specific regression class that nothing else in +the test suite covers. + +## Guidance + +When a unit's deliverable is a schema, a contract object, or a description block, the structural and +substring assertions on that artefact **are** the unit tests. Three concrete shapes: + +### 1. Pin the presence (and absence) of fields on the schema + +For an MCP tool catalogue exposed via `tool_definitions()`: + +```rust +#[test] +fn tool_definitions_add_pattern_has_language_oneof_shape() { + let tool = tool_schema("add_pattern"); + let lang = &tool["inputSchema"]["properties"]["language"]; + let one_of = lang["oneOf"].as_array().expect("language.oneOf array"); + assert_eq!(one_of.len(), 2); + assert_eq!(one_of[0]["type"], "string"); + assert_eq!(one_of[1]["type"], "array"); + assert_eq!(one_of[1]["items"]["type"], "string"); + assert!( + !required(&tool).contains(&"language"), + "must remain optional" + ); +} + +#[test] +fn tool_definitions_append_pattern_does_not_accept_language() { + // Load-bearing pin against a future "add language for symmetry" PR. + let tool = tool_schema("append_to_pattern"); + assert!(tool["inputSchema"]["properties"].get("language").is_none()); +} +``` + +The presence test catches accidental deletions or renames. The absence test catches additions that +would undo a deliberate design choice. **Both are required for genuinely contract-pinning +coverage.** + +### 2. Pin the description prose with substring matches on stable nouns + +The text agents read when expanding a schema in their tool registry is the contract documentation. +Drift in that text is drift in the contract: + +```rust +#[test] +fn update_pattern_description_mentions_language_and_three_way_semantics() { + let desc = tool_description("update_pattern"); + assert!(desc.contains("language"), "must mention `language`"); + for keyword in ["preserve", "clear", "replace"] { + assert!( + desc.contains(keyword), + "three-way semantics keyword `{keyword}` missing from description: {desc}" + ); + } +} +``` + +Key choice: assert on stable nouns the agent contract is keyed to (`language`, `update_pattern`, +`preserve`/`clear`/`replace`), not on the surrounding prose. Wording can drift; nouns can't drift +without changing the contract. + +### 3. Pin catalogue integrity with a names-only assertion + +```rust +#[test] +fn tools_list_contains_all_expected_tools() { + let names: Vec<&str> = tool_definitions() + .as_array() + .unwrap() + .iter() + .filter_map(|t| t["name"].as_str()) + .collect(); + assert_eq!( + names, + vec![ + "search_patterns", + "add_pattern", + "update_pattern", + "append_to_pattern", + "list_patterns", + "lore_status" + ] + ); +} +``` + +Cheap regression guard against accidentally dropping a tool from the catalogue. + +## Why This Matters + +The default mental model for "documentation deliverable" treats the artefact as inert text — read by +humans, untestable by machines. That mental model is wrong for schema JSON, tool descriptions, +OpenAPI spec text, GraphQL SDL comments, and any contract artefact agents (or downstream code) read. + +Three concrete failure modes the substring/structural tests catch: + +- **Decision 3 regression.** This PR's `append_to_pattern` deliberately does not accept `language` + (Decision 3 — schema honesty over surface symmetry). Without + `tool_definitions_append_pattern_does_not_accept_language`, a future symmetry-driven PR titled + "add language to append for consistency" would land green and silently undo the design. +- **De-language footgun via doc drift.** `update_pattern`'s `language` description documents the + three-way semantics (omit preserves, `[]` clears, non-empty replaces). If a future edit drops the + word `preserve`, agents reading the schema no longer learn the contract, and the de-language + footgun reappears in agent code rather than in the implementation. The substring-on-stable-nouns + pattern catches this. +- **Accidental property removal.** A careless schema edit could drop `tags` while adding `language`. + The sibling-property regression guard catches it before it ships. + +The cost is small: five substring or structural assertions per touched schema. The benefit is a +durable regression guard against the failure modes that don't have any other test surface. + +## When to Apply + +Apply when the unit's deliverable is **the contract text or schema itself**: + +- An MCP tool's `inputSchema`, `description`, or catalogue entry +- An OpenAPI / Swagger / GraphQL SDL contract field +- A JSON Schema or YAML schema file with downstream consumers +- A public type / interface signature with cross-package consumers +- A CLI `--help` string that downstream automation greps + +Skip when: + +- The text is internal-only prose (a `# Conventions` section in `AGENTS.md`, a comment block in a + source file) with no programmatic consumer +- The contract is already pinned by an OpenAPI / JSON Schema generator that runs in CI — drift shows + up at the generator boundary, not the consumer + +## Examples + +The U4 work in PR #63 ships eight unit tests against `tool_definitions()`: + +| Test | Pins | +| ------------------------------------------------------------------------- | ------------------------------------------------------------------------------- | +| `tool_definitions_add_pattern_has_language_oneof_shape` | `language` present + `oneOf` shape on `add_pattern` | +| `tool_definitions_update_pattern_has_language_oneof_shape` | Same on `update_pattern` | +| `tool_definitions_append_pattern_does_not_accept_language` | **`language` absent** on `append_to_pattern` (load-bearing) | +| `add_pattern_description_mentions_language_and_warn_behaviour` | Description mentions `language` + unknown-token policy | +| `update_pattern_description_mentions_language_and_three_way_semantics` | Description mentions `language` + `preserve`/`clear`/`replace` | +| `update_pattern_language_field_description_documents_three_way_semantics` | Per-field description echoes the three-way vocabulary | +| `append_pattern_description_points_at_update_pattern_for_language` | Append description redirects agents to `update_pattern` for frontmatter changes | +| `tools_list_returns_all_six_tools` (existing, extended) | Catalogue integrity | + +Total cost: ~80 lines of test code, all single-file, all `cargo test`-fast. Coverage gap that would +otherwise exist: every regression listed under **Why This Matters**. + +## Related + +- [`slice-shape-tests-are-not-pipeline-tests-2026-05-19.md`](slice-shape-tests-are-not-pipeline-tests-2026-05-19.md) + — adjacent lesson on the other end of the testing spectrum: shape tests on static data slices + don't prove pipeline integration. This doc says "do write the shape tests for contract surfaces"; + the slice-shape doc says "and don't stop there for behavioural surfaces." +- [`mcp-metadata-via-fenced-content-block-2026-04-07.md`](mcp-metadata-via-fenced-content-block-2026-04-07.md) + — the metadata fence design choice this PR extends with `language_warnings`. Same MCP-tool design + area. +- `docs/plans/2026-05-19-001-feat-mcp-language-arg-plan.md` — the plan whose U4 carries the + schema-shape and description-prose unit tests this lesson generalises from. diff --git a/docs/solutions/best-practices/sibling-code-paths-can-reintroduce-fixed-failure-modes-2026-05-19.md b/docs/solutions/best-practices/sibling-code-paths-can-reintroduce-fixed-failure-modes-2026-05-19.md new file mode 100644 index 0000000..813f972 --- /dev/null +++ b/docs/solutions/best-practices/sibling-code-paths-can-reintroduce-fixed-failure-modes-2026-05-19.md @@ -0,0 +1,187 @@ +--- +title: "Sibling code paths can silently reintroduce the failure mode a fix exists to address" +date: 2026-05-19 +category: best-practices +module: mcp-server +problem_type: best_practice +component: tooling +severity: high +applies_when: + - "Shipping a fix for a specific failure mode (dropped argument, silent error, lost advisory) on one of several sibling code paths" + - "Reviewing a plan whose risk section labels a related gap as 'known limitation, follow-up'" + - "A feature ships on the primary code path while a fast-path, short-circuit, or alternate-mode path is left out of scope" + - "Triaging residuals after planning to decide which belong in the current PR vs a follow-up" +tags: + - residual-triage + - feature-parity + - sibling-paths + - dropped-argument + - scope-discipline + - adversarial-review +--- + +# Sibling code paths can silently reintroduce the failure mode a fix exists to address + +## Context + +PR #63 added a `language` argument to the MCP pattern-authoring tools because the existing tools +silently dropped the argument — the dominant failure mode it exists to fix is _"agent passes +`language`, nothing happens, no signal."_ + +During planning, an adversarial review flagged that the **inbox-branch short-circuit** (used when +`config.inbox_branch_prefix` is set — the most common agent-submission path in production) bypasses +`index_single_file` entirely. That meant the chunking parser's unknown-language-token advisories +never fired for inbox-branch writes: `WriteResult.language_warnings` stayed empty even when the +caller passed an unknown token. The initial plan-time response was to label this as R4 — _known +limitation, follow-up PR._ + +The user redirected: _"I don't want followups, talk me through both residuals."_ On the technical +walkthrough side-by-side with the rest of the work, the conclusion became obvious: the inbox-branch +path is the **same failure mode the feature exists to fix**, just on a different code path. Leaving +it out would mean the dropped-argument bug _reappears at MCP runtime_ for the most common path — +silently, with no agent-observable signal. + +The fix folded into U3: each short-circuit (add, update, append) invokes +`parse_frontmatter_language_list` directly on the about-to-be-written content, the shared +`collect_language_warnings` helper emits one stderr line per unique unknown token, and +`WriteResult.language_warnings` matches what the local-write path would have produced. Closed in the +same diff. Integration tests in `tests/branch_push.rs` pin both paths. + +## Guidance + +When triaging a "follow-up" candidate after planning, ask one question: + +> Is the limitation I'm about to defer **the same shape** as the failure mode the current PR is +> fixing? + +If yes, it is not a follow-up. It is **incomplete scope**. Fold it into the current PR. + +Three concrete tests for "same shape": + +### 1. Identify the failure mode by its agent-observable signal + +What does the current PR's failure mode look like _from the consumer's seat_? In PR #63 it was: +_agent calls a tool with `language`, nothing happens, no signal in the response or stderr._ That +signal — silent argument drop — is the canonical form. Now scan every code path the consumer can +reach and ask: _would this signal also appear here, in the same shape?_ + +For PR #63: inbox-branch path → agent calls `add_pattern` with `language: ["objectiv-c"]`, +short-circuit pushes to a remote branch, returns `language_warnings: []`. Same canonical signal. +Same bug. + +### 2. Audit sibling code paths against the contract the fix establishes + +Whenever a fix establishes a new contract on a public surface (a metadata field that's "always +present", a stderr line that "always fires for unknown tokens"), find every code path that returns +through that surface and verify the contract holds. The trace is mechanical: + +| Path | Returns `WriteResult` | Invokes `index_single_file`? | Surfaces advisories? | +| ---------------------------------- | --------------------- | ---------------------------- | ------------------------- | +| `add_pattern` (local) | yes | yes | yes via parser advisories | +| `add_pattern` (inbox branch) | yes | **no** — short-circuits | **no — gap** | +| `update_pattern` (local) | yes | yes | yes | +| `update_pattern` (inbox branch) | yes | **no** — short-circuits | **no — gap** | +| `append_to_pattern` (local) | yes | yes | yes | +| `append_to_pattern` (inbox branch) | yes | **no** — short-circuits | **no — gap** | + +The grid makes the gap visible. Every "no" cell is a sibling-path failure-mode reappearance. + +### 3. Cost-check: is the fix cheap enough to fold in? + +Sibling-path fixes are often surprisingly cheap because the canonical implementation already exists +— the sibling path just needs to call the same helper. In PR #63: `parse_frontmatter_language_list` +is a pure function (`&str, &str` in, advisories out, no DB, no embedder, no I/O); each short-circuit +needed roughly 10 lines (parse + collect + threading through `WriteResult`). One shared helper +covered all three sites. + +If the fold-in is genuinely expensive (requires a schema change, a new dependency, a protocol +revision), then the follow-up label may be correct — but write a hazard-pin test for the current gap +so the next refactor surfaces it. See +[`composition-cascades-new-write-paths-can-be-silently-undone-2026-04-06.md`](composition-cascades-new-write-paths-can-be-silently-undone-2026-04-06.md) +for the hazard-pin pattern when the real fix is genuinely out of scope. + +## Why This Matters + +The plan-time framing of "known limitation, follow-up PR" is correct sometimes and seductive always. +It feels disciplined — _I noticed the gap, I called it out, I'll fix it later._ That framing hides +one specific failure mode: **the residual is the same failure mode the current PR exists to fix.** +When that's true, "follow-up" means _ship the PR that's supposed to close the bug while leaving the +bug open on the dominant path._ + +The cost asymmetry is what makes this worth catching: + +- **In the current PR:** ~10 lines per sibling path, one shared helper, one or two extra test + scenarios. The reviewer has the full context. The bug never ships. +- **In a follow-up PR:** a new branch, a new PR, a new review round, a new round of plan + documentation, and — between landing and the follow-up — agents in production hit the bug the + current PR claimed to fix. + +The deeper lesson is that **a fix on one code path establishes a new contract on the public surface, +not on that code path.** If `WriteResult.language_warnings` is "always present, always populated +with unknown tokens", then every `WriteResult`-producing code path owes that contract. The sibling +path didn't suddenly grow a bug; it was always broken — the new contract just made the brokenness +visible. + +## When to Apply + +Apply this check whenever: + +- The current PR fixes a specific class of failure mode (dropped argument, silent error, lost + advisory, missing observability) on a tool, endpoint, or public surface +- The fix establishes a new contract on the response shape (a field that's "always present", a log + line that "always fires", a side effect that "always happens") +- The system has fast-path, short-circuit, alternate-mode, or batch-mode code paths that produce the + same response shape via different internal logic +- An adversarial review or plan-time risk section labels a related gap as "follow-up" or "known + limitation" + +Skip when: + +- The "sibling path" is genuinely a different feature with a different contract +- The fold-in requires a schema change, protocol revision, or breaking change that genuinely belongs + in a separate PR +- The current PR is already large and the sibling-path fix can land as the literal next commit on + the same branch within a day + +## Examples + +### lore PR #63 — inbox-branch language advisory parity + +- **Primary fix:** `add_pattern` / `update_pattern` accept `language`, write canonical frontmatter, + surface unknown tokens via stderr + `WriteResult.language_warnings`. +- **Sibling-path gap (initially R4 → fold-in U3):** the inbox-branch short-circuit in each of the + three pattern-authoring functions skips `index_single_file`, so unknown-language tokens never + surface on the agent-submission path. Same canonical failure mode as the primary fix. +- **Fold-in:** ~10 lines per short-circuit, one shared `collect_language_warnings` helper, three + integration tests in `tests/branch_push.rs`. R4 deleted from the risk section; documented as + closed. + +### Generic template for the audit + +When reviewing a PR that fixes a specific failure mode on a public surface: + +``` +1. Name the failure mode by its agent-observable signal. +2. Enumerate every code path that returns through that surface. +3. For each path, ask: "if a consumer triggers the same input that the primary fix addresses, + does this path produce the same observable signal as the fixed primary path?" +4. If the answer is "no" — the sibling path silently reintroduces the failure mode. +5. Fold the fix into the current PR unless the fold-in is genuinely expensive; otherwise apply + the hazard-pin pattern from composition-cascades-….md. +``` + +## Related + +- [`composition-cascades-new-write-paths-can-be-silently-undone-2026-04-06.md`](composition-cascades-new-write-paths-can-be-silently-undone-2026-04-06.md) + — adjacent lesson on **inter-path** composition (a new path + an existing reconciliation pass). + This doc covers **intra-feature** parity (one feature shipped only on some of its sibling code + paths). Same family of "feature shape doesn't survive across all code paths"; different specific + shape. +- [`mcp-metadata-via-fenced-content-block-2026-04-07.md`](mcp-metadata-via-fenced-content-block-2026-04-07.md) + — the metadata-fence contract this PR's `language_warnings` extends. The new field's "always + present" property is the new contract whose sibling-path coverage this lesson is about. +- `docs/plans/2026-05-19-001-feat-mcp-language-arg-plan.md` — the plan whose R4 → U3 fold-in is the + concrete example. The Risk Analysis section before the rewrite labelled the inbox-branch gap as + "known limitation"; after the user's redirection it became "closed inside U3." +- `tests/branch_push.rs::inbox_add_pattern_collects_unknown_language_tokens` and the two sibling + tests around it — the integration tests that pin the closed gap. diff --git a/docs/solutions/design-patterns/preserve-branch-canonicalisation-asymmetry-2026-05-19.md b/docs/solutions/design-patterns/preserve-branch-canonicalisation-asymmetry-2026-05-19.md new file mode 100644 index 0000000..5c1dd5a --- /dev/null +++ b/docs/solutions/design-patterns/preserve-branch-canonicalisation-asymmetry-2026-05-19.md @@ -0,0 +1,227 @@ +--- +title: "Preserve-on-`None` branches inherit parser normalisation — pin the asymmetry, don't fight it" +date: 2026-05-19 +category: design-patterns +module: ingest +problem_type: design_pattern +component: tooling +severity: medium +applies_when: + - "Designing a three-way `Option<&[T]>` argument (absent preserves, empty clears, non-empty replaces) for a serialised list field" + - "Two sibling fields share the same three-way semantics but their parsers normalise input differently (lowercasing, NFC, percent-decoding)" + - "Reviewing a preserve branch that round-trips an existing on-disk value through a parser before re-rendering" + - "Auditing whether a body-only update silently canonicalises file content the user never explicitly changed" +tags: + - canonicalisation + - preserve-branch + - parser-asymmetry + - three-way-semantics + - intentional-asymmetry + - pin-the-contract +--- + +# Preserve-on-`None` branches inherit parser normalisation — pin the asymmetry, don't fight it + +## Context + +PR #63 added a `language: Option<&[&str]>` argument to `ingest::update_pattern` mirroring the +existing three-way semantics of `tags`: + +- `None` → preserve the existing frontmatter list (avoids the de-language footgun on body-only + rewrites) +- `Some(&[])` → clear +- `Some(&[...])` → replace wholesale + +Implementation copied the `tags` precedent: read the existing file, parse out the existing list via +`parse_frontmatter_language_list`, feed the parsed list into `build_file_content`'s re-renderer. + +A correctness reviewer flagged a subtle divergence: the language parser **lowercases** tokens at +read time (`src/chunking.rs::parse_frontmatter_language_list`), so a body-only `update_pattern` +against an existing `language: [Rust]` file rewrites it as `language: [rust]`. The `tags` parser is +case-preserving — its preserve branch is byte-stable, but the language preserve branch is not. + +Two possible reads of the divergence: + +- **As a bug** — the preserve semantics promise to keep the existing list verbatim; lowercase + rewriting violates that promise. +- **As intentional asymmetry** — the canonical form for language tokens _is_ lowercase (the + `LANGUAGES` table keys are lowercase, the DB has stored lowercased tokens since PR #50, the + retrieval gate matches lowercased tokens). The preserve branch making the file converge to the + canonical form is consistent behaviour, not divergent behaviour. + +The intentional-asymmetry read won. The asymmetry was pinned with a documentation comment in +`update_pattern` and a regression-pinning test +(`update_pattern_preserve_lowercases_existing_mixed_case_language`). A future change in either +parser's casing posture surfaces at the call site rather than passing silently. + +## Guidance + +When two sibling fields share three-way preserve/clear/replace semantics on a list, their preserve +branches re-render through their respective parsers. Whatever transformations the parsers apply +**will surface on the preserve path**. Three concrete rules: + +### 1. Compare parser normalisation between sibling fields explicitly + +Before assuming a preserve branch is byte-stable, audit the parser: + +``` +For each sibling field with preserve-on-`None` semantics: + 1. What does the parser return when called on the existing file? + 2. Is the return canonicalised (lowercased, NFC, sorted, deduped) or verbatim? + 3. If two sibling fields have different canonicalisation postures, the preserve branches + WILL diverge in their on-disk effect. +``` + +For PR #63: `parse_frontmatter_tag_list` is verbatim-with-quote-stripping. +`parse_frontmatter_language_list` is lowercase-with-quote-stripping-with-canonical-token-validation. +The two preserve branches were guaranteed to behave differently on mixed-case input from the moment +the language parser landed in PR #50 — the asymmetry just wasn't visible until `update_pattern` +started exercising it. + +### 2. Decide whether the asymmetry is a bug or intentional — then pin it + +The decision turns on whether the parser's normalisation **is** the canonical form for the field: + +- If the parser normalises because the field has a canonical form (lowercase for language tokens + matched against a canonical table; NFC for filenames; percent-decoded URLs), the preserve branch + converging to that form is intentional. The file shape converges to match what the DB / engine + already stores. **Pin the convergence as intentional with a regression test that names the + asymmetry.** +- If the parser normalises by accident (an over-eager trim, an unnecessary lowercase on a + case-sensitive field), the preserve branch silently mutates the user's content. **Fix the parser, + not the preserve branch.** + +The pin shape, when intentional: + +```rust +#[test] +fn update_pattern_preserve_lowercases_existing_mixed_case_language() { + // Intentional contract pin: the language parser lowercases at read time + // (the canonical form for `LANGUAGES`-table lookups), so a body-only + // update against an existing `language: [Rust]` file rewrites the line + // as `language: [rust]`. Asymmetric with `tags`'s case-preserving + // preserve branch by design — see the explanatory comment in + // `update_pattern` for the rationale. + // + // A future change to either parser's casing posture lands here. + let result = update_pattern(/* file with language: [Rust, Kotlin] */ ..); + let content = fs::read_to_string(...).unwrap(); + assert!(content.contains("language: [rust, kotlin]")); +} +``` + +The test does two things at once: it asserts the current behaviour is **what the rewrite intends**, +and it documents _why_ by naming the parser asymmetry and the canonical-form rationale. A future +parser change in either direction (language parser stops lowercasing, or tags parser starts) fails +this test and forces a conscious decision about whether the new behaviour is desired. + +### 3. Co-locate the explanation with the code, not just the test + +The user-visible behaviour is in `update_pattern`. A reader inspecting the preserve branch needs the +asymmetry rationale in-line, not buried in a test file three layers deeper. Add an in-tree comment +at the preserve branch: + +```rust +// Intentional asymmetry with `tags`'s preserve branch: the language +// parser lowercases tokens at read time, so a body-only update against +// an existing `language: [Rust]` file rewrites the line as +// `language: [rust]`. The DB has stored the lowercased form since the +// parser landed; the file now converges to match rather than drifting. +// `tags`'s preserve path keeps the original casing because the tags +// parser does not lowercase — language tokens are validated against a +// canonical table where lowercase IS the canonical form, tags are free-form. +// Pinned by `update_pattern_preserve_lowercases_existing_mixed_case_language`. +``` + +Comment + test together: a reader finds the _why_ at the code, and the _guard_ at the test. + +## Why This Matters + +The class of bug this guards against is _the future review where someone reads the asymmetry as a +regression_. Without the pin, that review path is: + +1. Reviewer reads the preserve branch, notices lowercase divergence from `tags`. +2. Reviewer flags as "inconsistent with `tags` precedent — should preserve verbatim." +3. A well-meaning fix re-introduces case-preservation for the language preserve branch. +4. Now the file's `language: [Rust]` no longer matches the DB's lowercased `["rust"]` after a + body-only update — and the retrieval gate's structural-match-on-DB-lowercased starts missing + patterns that look declared on disk. + +The pin breaks the cycle: the regression test names the asymmetry as intentional, so the +well-meaning fix surfaces as a failing assertion that forces a conscious "do we still want this +asymmetry?" conversation. The intentional asymmetry survives review cycles because it's documented +_as_ intentional, not implied. + +The deeper observation is that **parser normalisation is a leaky abstraction for preserve +semantics.** Preserve-branch behaviour is determined by the parser, not by the preserve branch's own +code. Pattern this means: review the parser before reviewing the preserve branch, and pin the +parser's contribution to the preserve branch's behaviour as a contract. + +## When to Apply + +Apply this practice when: + +- A new field gains preserve-on-`None` semantics on a serialised list +- A sibling field with the same semantics already exists, and you are tempted to "copy the pattern" +- The parser for the new field normalises (lowercase, NFC, sort, dedup) and the sibling parser does + not, or vice versa +- A correctness review flags the preserve branch as "inconsistent with the sibling field" — the + question to answer is _is the asymmetry intentional or accidental?_ + +Skip when: + +- Both sibling fields use parsers with identical normalisation posture (genuine symmetry) +- The new field has no canonical form distinct from the user's raw input — preserve really does mean + verbatim, the parser is a passthrough +- The field is so short-lived that pinning is overhead + +## Examples + +### lore PR #63 — `language` vs `tags` preserve-branch asymmetry + +| Field | Parser canonicalises? | Preserve branch behaviour | +| ---------- | ------------------------------------------------------------------ | --------------------------------------- | +| `tags` | No (`parse_frontmatter_tag_list` is verbatim-with-quote-stripping) | Byte-stable on preserve | +| `language` | Yes — lowercases against canonical `LANGUAGES` table | Converges to canonical form on preserve | + +- **Pin:** `update_pattern_preserve_lowercases_existing_mixed_case_language` in + `src/ingest.rs::tests` asserts that a file with `language: [Rust, Kotlin]` rewrites to + `language: [rust, kotlin]` after a body-only update. +- **In-tree comment:** the preserve branch in `update_pattern` carries a 6-line comment naming the + asymmetry, the rationale (canonical-form for `LANGUAGES` lookups, DB has stored lowercased since + PR #50), and the pinning test by name. +- **Reviewer expectation:** a future correctness review that flags the asymmetry surfaces the + in-tree comment first, then the pinning test — both name the rationale before the reviewer can + propose a "fix." + +### Generic template + +Whenever two sibling fields share three-way semantics on a list: + +``` +1. Read both parsers. Note any transformation each applies (case, NFC, trim, sort, dedup). +2. If the transformations differ, the preserve branches will diverge — find the divergence point. +3. Ask: "is the diverging field's parser transformation the canonical form?" + - Yes → intentional asymmetry. Pin with a regression test and an in-tree comment naming + the rationale. + - No → accidental asymmetry. Fix the parser to match the sibling, or accept the divergence + with a documented rationale. +4. If intentional: co-locate the rationale at the preserve branch (comment) and the pinning + test (regression guard). Both are required — the comment without the pin drifts; the pin + without the comment confuses future readers. +``` + +## Related + +- [`round-trip-discriminator-canonicalise-both-sides-2026-05-10.md`](round-trip-discriminator-canonicalise-both-sides-2026-05-10.md) + — adjacent design pattern on the _other_ direction of canonicalisation: discriminator equality + comparisons require **symmetric** canonicalisation on both input and stored sides. This doc says + "asymmetric canonicalisation can be intentional in preserve branches"; that doc says "asymmetric + canonicalisation in discriminators is a bug." Both apply to the same parser; the question is what + surface the parser feeds. +- `docs/plans/2026-05-19-001-feat-mcp-language-arg-plan.md` — the plan whose correctness review + surfaced this asymmetry. The Risk Analysis section's R1 (originally a SHOULD-FIX) was resolved by + pinning the intent rather than fixing it. +- `src/ingest.rs::update_pattern` preserve branch — the in-tree comment carrying the rationale. +- `src/ingest.rs::tests::update_pattern_preserve_lowercases_existing_mixed_case_language` — the + pinning test. diff --git a/src/ingest.rs b/src/ingest.rs index d73032e..7b5a2f3 100644 --- a/src/ingest.rs +++ b/src/ingest.rs @@ -201,6 +201,15 @@ pub struct WriteResult { pub chunks_indexed: usize, pub commit_status: CommitStatus, pub embedding_failures: usize, + /// Unknown-language-token advisories surfaced by the chunking parser + /// when the just-written file is reindexed (or, on the inbox-branch + /// short-circuit, when the parser is invoked directly on the + /// about-to-be-written content). One entry per unique offending token, + /// preserving first-seen order, lowercased to match the parser's + /// canonical form. Empty when every token validated against + /// [`crate::engine::is_known_token`]; never `None` so callers can + /// always render the field as an array on a `lore-metadata` fence. + pub language_warnings: Vec, } // --------------------------------------------------------------------------- @@ -1128,11 +1137,44 @@ pub fn ingest_single_file( // Write operations // --------------------------------------------------------------------------- +/// Collapse a list of [`MalformedLanguageEntry`] advisories into the +/// `language_warnings: Vec` shape that `WriteResult` exposes to +/// agents, and emit one stderr line per unique unknown token so the CLI +/// observability surface matches the metadata-fence surface. Order is +/// first-seen; duplicates are dropped. +/// +/// Shared between the normal write path (advisories from +/// [`index_single_file`]) and the inbox-branch short-circuit (advisories +/// from a direct [`crate::chunking::parse_frontmatter_language_list`] +/// call on the about-to-be-written content). Without this shared helper +/// the short-circuit would silently swallow the warning, leaving +/// agent-submitted patterns advisory-blind on what is the most common +/// agent submission path — the exact dropped-argument failure mode the +/// `language` argument exists to fix, on a different code path. +fn collect_language_warnings(entries: &[MalformedLanguageEntry], source_file: &str) -> Vec { + let mut out: Vec = Vec::new(); + for entry in entries { + if out.iter().any(|t| t == &entry.token) { + continue; + } + eprintln!( + "Warning: pattern {source_file}: unknown language token `{}`.", + entry.token + ); + out.push(entry.token.clone()); + } + out +} + /// Create a new pattern file, index it, and commit. /// /// When `inbox_branch_prefix` is `Some`, the file is committed to a /// per-submission branch and pushed to the remote instead of being written /// to disk and indexed locally. +#[allow(clippy::too_many_arguments)] // Canonical MCP write path; refactoring +// into a struct would obscure the call +// sites without making them easier to +// read. pub fn add_pattern( db: &KnowledgeDB, embedder: &dyn Embedder, @@ -1140,6 +1182,7 @@ pub fn add_pattern( title: &str, body: &str, tags: &[&str], + language: &[&str], inbox_branch_prefix: Option<&str>, ) -> anyhow::Result { // Sanitise the title at the canonical write boundary: trim surrounding @@ -1162,9 +1205,21 @@ pub fn add_pattern( // Validate slug doesn't contain path traversal components. validate_slug(&filename)?; - let content = build_file_content(title, body, tags); + let content = build_file_content(title, body, tags, language); if let Some(prefix) = inbox_branch_prefix { + // Inbox-branch parity: the short-circuit pushes the file to a remote + // branch without invoking `index_single_file`, so the chunking + // parser's malformed-language advisories are never fired by the + // ingest layer for this path. Invoke the parser directly on the + // about-to-be-written content so the warning reaches stderr and the + // returned `language_warnings` matches what the local-write path + // would have produced. Without this, agent-submitted patterns + // (the dominant inbox use case) would silently swallow unknown + // language tokens. + let (_, malformed) = crate::chunking::parse_frontmatter_language_list(&content, &filename); + let language_warnings = collect_language_warnings(&malformed, &filename); + let branch = git::commit_to_new_branch( knowledge_dir, prefix, @@ -1180,6 +1235,7 @@ pub fn add_pattern( chunks_indexed: 0, commit_status: CommitStatus::Pushed { branch }, embedding_failures: 0, + language_warnings, }); } @@ -1227,8 +1283,10 @@ pub fn add_pattern( let IndexedFile { chunks_indexed: chunks, embedding_failures, + malformed_language, .. } = index_single_file(db, embedder, knowledge_dir, &file_path, "heading")?; + let language_warnings = collect_language_warnings(&malformed_language, &filename); let commit_status = try_commit( knowledge_dir, @@ -1241,6 +1299,7 @@ pub fn add_pattern( chunks_indexed: chunks, commit_status, embedding_failures, + language_warnings, }) } @@ -1256,6 +1315,16 @@ pub fn add_pattern( /// drop every tag (including `universal`, de-universalising the pattern). /// - `Some(&[])` — explicitly clear all tags. /// - `Some(&[...])` — replace the tag list wholesale. +/// +/// `language` follows the same three-way semantics as `tags`. The +/// preserve-on-`None` branch reads the existing `language:` frontmatter +/// via [`crate::chunking::parse_frontmatter_language_list`] so a body-only +/// rewrite does not silently de-language a pattern — the analogue of the +/// de-universalisation footgun that motivated `tags`'s preserve semantics. +#[allow(clippy::too_many_arguments)] // Canonical MCP write path; refactoring +// into a struct would obscure the call +// sites without making them easier to +// read. pub fn update_pattern( db: &KnowledgeDB, embedder: &dyn Embedder, @@ -1263,6 +1332,7 @@ pub fn update_pattern( source_file: &str, body: &str, tags: Option<&[&str]>, + language: Option<&[&str]>, inbox_branch_prefix: Option<&str>, ) -> anyhow::Result { let file_path = knowledge_dir.join(source_file); @@ -1283,19 +1353,59 @@ pub fn update_pattern( // the body through `update_pattern` but forgets to pass `tags`, which // previously silently cleared every tag — including `universal`, which // would de-universalise a pinned pattern without any signal. - let preserved: Vec; - let preserved_refs: Vec<&str>; + let preserved_tags: Vec; + let preserved_tag_refs: Vec<&str>; let tags_to_apply: &[&str] = if let Some(t) = tags { t } else { - preserved = crate::chunking::parse_frontmatter_tag_list(&existing); - preserved_refs = preserved.iter().map(String::as_str).collect(); - &preserved_refs + preserved_tags = crate::chunking::parse_frontmatter_tag_list(&existing); + preserved_tag_refs = preserved_tags.iter().map(String::as_str).collect(); + &preserved_tag_refs + }; + + // Mirror of the `tags` preserve-on-`None` branch — see Decision 2 in the + // language-arg plan doc for the analogous footgun this guards against. + // `parse_frontmatter_language_list` validates against the canonical + // table; we discard the malformed-token advisories here because the + // post-write `index_single_file` re-parses the file and surfaces them + // through the normal advisory path. U3 closes the inbox-branch + // short-circuit's blind spot for that path. + // + // Intentional asymmetry with `tags`'s preserve branch: the language + // parser lowercases tokens at read time (see + // `crate::chunking::parse_frontmatter_language_list`), so a body-only + // `update_pattern` against an existing `language: [Rust]` file rewrites + // the line as `language: [rust]`. The DB has stored the lowercased form + // since PR #50; the file now converges to match rather than drifting. + // `tags`'s preserve path keeps the original casing because the tags + // parser does not lowercase — language tokens are validated against + // the canonical `LANGUAGES` table where lowercase is the canonical form, + // tags are free-form. Pinned by + // `update_pattern_preserve_lowercases_existing_mixed_case_language` so + // a future change to either parser's casing posture surfaces here. + let preserved_lang: Vec; + let preserved_lang_refs: Vec<&str>; + let language_to_apply: &[&str] = if let Some(l) = language { + l + } else { + let (tokens, _) = crate::chunking::parse_frontmatter_language_list(&existing, source_file); + preserved_lang = tokens; + preserved_lang_refs = preserved_lang.iter().map(String::as_str).collect(); + &preserved_lang_refs }; - let content = build_file_content(&title, body, tags_to_apply); + let content = build_file_content(&title, body, tags_to_apply, language_to_apply); if let Some(prefix) = inbox_branch_prefix { + // Inbox-branch parity: see [`add_pattern`]'s short-circuit for the + // same rationale — the short-circuit skips `index_single_file`, so + // the parser is invoked directly on the about-to-be-written content + // to keep stderr and metadata-fence advisories aligned with the + // local-write path. + let (_, malformed) = + crate::chunking::parse_frontmatter_language_list(&content, source_file); + let language_warnings = collect_language_warnings(&malformed, source_file); + let slug = file_stem(source_file); let branch = git::commit_to_new_branch( knowledge_dir, @@ -1312,6 +1422,7 @@ pub fn update_pattern( chunks_indexed: 0, commit_status: CommitStatus::Pushed { branch }, embedding_failures: 0, + language_warnings, }); } @@ -1320,8 +1431,10 @@ pub fn update_pattern( let IndexedFile { chunks_indexed: chunks, embedding_failures, + malformed_language, .. } = index_single_file(db, embedder, knowledge_dir, &canonical, "heading")?; + let language_warnings = collect_language_warnings(&malformed_language, source_file); let commit_status = try_commit( knowledge_dir, @@ -1334,6 +1447,7 @@ pub fn update_pattern( chunks_indexed: chunks, commit_status, embedding_failures, + language_warnings, }) } @@ -1374,6 +1488,14 @@ pub fn append_to_pattern( content.push('\n'); if let Some(prefix) = inbox_branch_prefix { + // Inbox-branch parity (append edition): the heading-and-body append + // preserves the existing frontmatter, but the existing frontmatter + // may already carry unknown language tokens. Surface them so the + // append-via-MCP path is no quieter than `add_pattern` / `update_pattern`. + let (_, malformed) = + crate::chunking::parse_frontmatter_language_list(&content, source_file); + let language_warnings = collect_language_warnings(&malformed, source_file); + let slug = file_stem(source_file); let branch = git::commit_to_new_branch( knowledge_dir, @@ -1390,6 +1512,7 @@ pub fn append_to_pattern( chunks_indexed: 0, commit_status: CommitStatus::Pushed { branch }, embedding_failures: 0, + language_warnings, }); } @@ -1398,8 +1521,10 @@ pub fn append_to_pattern( let IndexedFile { chunks_indexed: chunks, embedding_failures, + malformed_language, .. } = index_single_file(db, embedder, knowledge_dir, &canonical, "heading")?; + let language_warnings = collect_language_warnings(&malformed_language, source_file); let commit_status = try_commit( knowledge_dir, @@ -1412,6 +1537,7 @@ pub fn append_to_pattern( chunks_indexed: chunks, commit_status, embedding_failures, + language_warnings, }) } @@ -1881,12 +2007,27 @@ fn slugify(title: &str) -> String { .join("-") } -/// Build a markdown file from title, body, and optional frontmatter tags. -fn build_file_content(title: &str, body: &str, tags: &[&str]) -> String { +/// Build a markdown file from title, body, and optional frontmatter tags +/// and language tokens. +/// +/// Frontmatter ordering is fixed: `tags:` first, then `language:`. Both +/// render as flow lists for parser symmetry — single-element lists render +/// as `language: [rust]` rather than the scalar form so the on-disk shape +/// stays uniform regardless of how the caller passed the value. The +/// frontmatter block is omitted entirely when both fields are empty so the +/// pre-`language` write shape is preserved byte-for-byte. +fn build_file_content(title: &str, body: &str, tags: &[&str], language: &[&str]) -> String { let mut content = String::new(); - if !tags.is_empty() { + let has_tags = !tags.is_empty(); + let has_language = !language.is_empty(); + if has_tags || has_language { content.push_str("---\n"); - let _ = writeln!(content, "tags: [{}]", tags.join(", ")); + if has_tags { + let _ = writeln!(content, "tags: [{}]", tags.join(", ")); + } + if has_language { + let _ = writeln!(content, "language: [{}]", language.join(", ")); + } content.push_str("---\n\n"); } let _ = write!(content, "# {title}\n\n"); @@ -2335,6 +2476,7 @@ mod tests { "My Pattern", "Pattern body that is long enough for chunking.", &["design", "rust"], + &[], None, ) .unwrap(); @@ -2361,7 +2503,7 @@ mod tests { fs::write(dir.join("existing.md"), "# Existing\n").unwrap(); - let result = add_pattern(&db, &embedder, dir, "Existing", "body", &[], None); + let result = add_pattern(&db, &embedder, dir, "Existing", "body", &[], &[], None); assert!(result.is_err()); let msg = result.unwrap_err().to_string(); assert!(msg.contains("already exists"), "unexpected error: {msg}"); @@ -2386,7 +2528,7 @@ mod tests { fs::write(dir.join("api-notes.md"), "# API Notes\n").unwrap(); - let result = add_pattern(&db, &embedder, dir, "API: Notes", "body", &[], None); + let result = add_pattern(&db, &embedder, dir, "API: Notes", "body", &[], &[], None); assert!(result.is_err()); let msg = result.unwrap_err().to_string(); assert!(msg.contains("Slug \"api-notes\""), "missing slug: {msg}"); @@ -2418,7 +2560,7 @@ mod tests { ) .unwrap(); - let result = add_pattern(&db, &embedder, dir, "API Notes", "body", &[], None); + let result = add_pattern(&db, &embedder, dir, "API Notes", "body", &[], &[], None); assert!(result.is_err()); let msg = result.unwrap_err().to_string(); assert!( @@ -2444,7 +2586,7 @@ mod tests { fs::write(dir.join("café.md"), "# café\n").unwrap(); - let result = add_pattern(&db, &embedder, dir, "cafe\u{0301}", "body", &[], None); + let result = add_pattern(&db, &embedder, dir, "cafe\u{0301}", "body", &[], &[], None); assert!(result.is_err()); let msg = result.unwrap_err().to_string(); assert!( @@ -2464,7 +2606,16 @@ mod tests { let db = memory_db(); let embedder = FakeEmbedder::new(); - let result = add_pattern(&db, &embedder, dir, "\u{0301}\u{0301}", "body", &[], None); + let result = add_pattern( + &db, + &embedder, + dir, + "\u{0301}\u{0301}", + "body", + &[], + &[], + None, + ); assert!(result.is_err()); let msg = result.unwrap_err().to_string(); assert!( @@ -2482,7 +2633,7 @@ mod tests { let db = memory_db(); let embedder = FakeEmbedder::new(); - let result = add_pattern(&db, &embedder, dir, "", "body", &[], None); + let result = add_pattern(&db, &embedder, dir, "", "body", &[], &[], None); assert!(result.is_err()); let msg = result.unwrap_err().to_string(); assert!( @@ -2509,11 +2660,21 @@ mod tests { " My Pattern ", "body text", &[], + &[], None, ) .unwrap(); - let result = add_pattern(&db, &embedder, dir, "My Pattern", "other body", &[], None); + let result = add_pattern( + &db, + &embedder, + dir, + "My Pattern", + "other body", + &[], + &[], + None, + ); assert!(result.is_err()); let msg = result.unwrap_err().to_string(); assert!( @@ -2535,7 +2696,7 @@ mod tests { let db = memory_db(); let embedder = FakeEmbedder::new(); - let result = add_pattern(&db, &embedder, dir, "Hello\nWorld", "body", &[], None); + let result = add_pattern(&db, &embedder, dir, "Hello\nWorld", "body", &[], &[], None); assert!(result.is_err()); let msg = result.unwrap_err().to_string(); assert!( @@ -2553,7 +2714,7 @@ mod tests { let db = memory_db(); let embedder = FakeEmbedder::new(); - let result = add_pattern(&db, &embedder, dir, "Hello\rWorld", "body", &[], None); + let result = add_pattern(&db, &embedder, dir, "Hello\rWorld", "body", &[], &[], None); assert!(result.is_err()); let msg = result.unwrap_err().to_string(); assert!( @@ -2576,7 +2737,7 @@ mod tests { // Create a directory at the path `add_pattern` would target. fs::create_dir_all(dir.join("blocked.md")).unwrap(); - let result = add_pattern(&db, &embedder, dir, "Blocked", "body", &[], None); + let result = add_pattern(&db, &embedder, dir, "Blocked", "body", &[], &[], None); assert!(result.is_err()); let msg = result.unwrap_err().to_string(); // The fs::write that follows the discriminator may also fail with an @@ -2602,7 +2763,7 @@ mod tests { // Existing file's heading is NFD (`e` + combining acute). fs::write(dir.join("café.md"), "# cafe\u{0301}\n").unwrap(); - let result = add_pattern(&db, &embedder, dir, "café", "body", &[], None); + let result = add_pattern(&db, &embedder, dir, "café", "body", &[], &[], None); assert!(result.is_err()); let msg = result.unwrap_err().to_string(); assert!( @@ -2638,6 +2799,7 @@ mod tests { "Brand new body that is long enough for a chunk.", Some(&["updated"]), None, + None, ) .unwrap(); @@ -2678,6 +2840,7 @@ mod tests { "New body long enough for a chunk.", None, None, + None, ) .unwrap(); assert!(result.chunks_indexed >= 1); @@ -2721,6 +2884,7 @@ mod tests { "New body long enough.", Some(&[]), None, + None, ) .unwrap(); @@ -2731,6 +2895,451 @@ mod tests { ); } + // -- language frontmatter rendering (U2) ------------------------------- + // + // The U2 contract: build_file_content renders a canonical `language:` + // flow-list line, ordered after `tags:` so the two list fields share a + // stable visual order on disk; the absent case writes no language line; + // update_pattern threads the parameter through the same preserve / clear + // / replace branch as `tags`. Every test below also reads the resulting + // DB row's `language_json` so the on-disk shape lines up with what the + // chunking parser produces (the slice-shape-vs-pipeline-tests learning). + + /// Read the `language_json` column from the `patterns` row for a + /// source file. Returns the JSON string as stored — `None` when no + /// `language:` frontmatter was parsed; `Some("[\"rust\"]")` when one + /// was. Flattens the double-`Option` (`no row` vs `row with NULL`) that + /// `pattern_language_json_for_source` returns since these tests always + /// write the file before reading. + fn language_json_for(db: &KnowledgeDB, source_file: &str) -> Option { + db.pattern_language_json_for_source(source_file) + .unwrap() + .expect("pattern row must exist for source") + } + + #[test] + fn add_pattern_with_language_renders_flow_list_frontmatter() { + let tmp = tempdir().unwrap(); + let dir = tmp.path(); + let db = memory_db(); + let embedder = FakeEmbedder::new(); + + add_pattern( + &db, + &embedder, + dir, + "Rust Patterns", + "Body text that is long enough for a chunk.", + &[], + &["rust"], + None, + ) + .unwrap(); + + let content = fs::read_to_string(dir.join("rust-patterns.md")).unwrap(); + assert!( + content.starts_with("---\nlanguage: [rust]\n---\n\n# Rust Patterns\n"), + "expected canonical flow-list shape, got:\n{content}" + ); + // End-to-end: the DB row must agree with the on-disk shape so the + // structural retrieval gate fires. + assert_eq!( + language_json_for(&db, "rust-patterns.md").as_deref(), + Some("[\"rust\"]") + ); + } + + #[test] + fn add_pattern_with_language_array_preserves_order() { + let tmp = tempdir().unwrap(); + let dir = tmp.path(); + let db = memory_db(); + let embedder = FakeEmbedder::new(); + + add_pattern( + &db, + &embedder, + dir, + "JVM Patterns", + "Body text long enough for chunking.", + &[], + &["java", "kotlin", "groovy"], + None, + ) + .unwrap(); + + let content = fs::read_to_string(dir.join("jvm-patterns.md")).unwrap(); + assert!( + content.contains("language: [java, kotlin, groovy]"), + "expected ordered flow-list, got:\n{content}" + ); + assert_eq!( + language_json_for(&db, "jvm-patterns.md").as_deref(), + Some("[\"java\",\"kotlin\",\"groovy\"]") + ); + } + + #[test] + fn add_pattern_with_tags_and_language_orders_tags_then_language() { + // The on-disk frontmatter order is a public contract — agents that + // diff a file before/after an MCP call rely on the line order being + // stable. This pin catches accidental swaps. + let tmp = tempdir().unwrap(); + let dir = tmp.path(); + let db = memory_db(); + let embedder = FakeEmbedder::new(); + + add_pattern( + &db, + &embedder, + dir, + "Mixed", + "Body long enough for a chunk.", + &["universal"], + &["rust"], + None, + ) + .unwrap(); + + let content = fs::read_to_string(dir.join("mixed.md")).unwrap(); + let frontmatter_block = content + .split("---\n\n") + .next() + .expect("expected frontmatter"); + let tags_pos = frontmatter_block.find("tags:").expect("tags line present"); + let lang_pos = frontmatter_block + .find("language:") + .expect("language line present"); + assert!( + tags_pos < lang_pos, + "tags must precede language; got:\n{frontmatter_block}" + ); + } + + #[test] + fn add_pattern_without_language_writes_no_language_line() { + // Regression guard: the absent-language case must produce the same + // file shape as before `language:` existed, so existing patterns + // and the unparametrised happy path stay byte-stable. + let tmp = tempdir().unwrap(); + let dir = tmp.path(); + let db = memory_db(); + let embedder = FakeEmbedder::new(); + + add_pattern( + &db, + &embedder, + dir, + "Plain", + "Body long enough for a chunk.", + &[], + &[], + None, + ) + .unwrap(); + + let content = fs::read_to_string(dir.join("plain.md")).unwrap(); + assert!( + !content.contains("language:"), + "absent language must not render any frontmatter line, got:\n{content}" + ); + assert!( + !content.contains("---\n"), + "no tags + no language must produce no frontmatter block, got:\n{content}" + ); + } + + #[test] + fn update_pattern_preserve_language_when_arg_absent() { + // The critical Decision 2 test — body-only rewrites must not + // silently de-language a pattern. Mirrors + // `update_pattern_with_none_tags_preserves_existing_frontmatter_tags`. + let tmp = tempdir().unwrap(); + let dir = tmp.path(); + let db = memory_db(); + let embedder = FakeEmbedder::new(); + + fs::write( + dir.join("multi.md"), + "---\nlanguage: [swift, objectivec]\n---\n\n# Multi\n\nOriginal body long enough.\n", + ) + .unwrap(); + + update_pattern( + &db, + &embedder, + dir, + "multi.md", + "Replacement body long enough for chunking.", + None, + None, + None, + ) + .unwrap(); + + let content = fs::read_to_string(dir.join("multi.md")).unwrap(); + assert!( + content.contains("language: [swift, objectivec]"), + "preserve-on-`None` must keep the existing language list; got:\n{content}" + ); + assert_eq!( + language_json_for(&db, "multi.md").as_deref(), + Some("[\"swift\",\"objectivec\"]") + ); + } + + #[test] + fn update_pattern_clears_language_when_arg_is_empty_slice() { + let tmp = tempdir().unwrap(); + let dir = tmp.path(); + let db = memory_db(); + let embedder = FakeEmbedder::new(); + + fs::write( + dir.join("rust.md"), + "---\nlanguage: [rust]\n---\n\n# Rust\n\nOriginal body long enough.\n", + ) + .unwrap(); + + update_pattern( + &db, + &embedder, + dir, + "rust.md", + "Replacement body long enough.", + None, + Some(&[]), + None, + ) + .unwrap(); + + let content = fs::read_to_string(dir.join("rust.md")).unwrap(); + assert!( + !content.contains("language:"), + "explicit empty must remove the language line, got:\n{content}" + ); + assert_eq!(language_json_for(&db, "rust.md"), None); + } + + #[test] + fn update_pattern_replaces_language_when_arg_is_non_empty() { + let tmp = tempdir().unwrap(); + let dir = tmp.path(); + let db = memory_db(); + let embedder = FakeEmbedder::new(); + + fs::write( + dir.join("doc.md"), + "---\nlanguage: [rust]\n---\n\n# Doc\n\nOriginal body long enough.\n", + ) + .unwrap(); + + update_pattern( + &db, + &embedder, + dir, + "doc.md", + "Replacement body long enough.", + None, + Some(&["go"]), + None, + ) + .unwrap(); + + let content = fs::read_to_string(dir.join("doc.md")).unwrap(); + assert!( + content.contains("language: [go]"), + "non-empty arg must replace the language list, got:\n{content}" + ); + assert!( + !content.contains("rust"), + "old language must be gone, got:\n{content}" + ); + assert_eq!( + language_json_for(&db, "doc.md").as_deref(), + Some("[\"go\"]") + ); + } + + // -- language_warnings on WriteResult (U3) ----------------------------- + // + // The contract: every unknown language token that the chunking parser + // would have flagged also reaches the caller via `WriteResult` and a + // stderr line, regardless of which write path was taken — including the + // inbox-branch short-circuit, which historically skipped + // `index_single_file` entirely. Tests below pin both paths and the + // all-valid baseline (the field must still be present so MCP callers + // can render `language_warnings: []` rather than omit the key). + + #[test] + fn add_pattern_language_warnings_collects_unknown_tokens() { + let tmp = tempdir().unwrap(); + let dir = tmp.path(); + let db = memory_db(); + let embedder = FakeEmbedder::new(); + + let result = add_pattern( + &db, + &embedder, + dir, + "Mixed Validity", + "Body long enough for a chunk.", + &[], + &["rust", "objectiv-c", "rust", "objectiv-c"], + None, + ) + .unwrap(); + + // Dedup is first-seen order; the valid token is filtered out + // because `is_known_token("rust")` is true. + assert_eq!(result.language_warnings, vec!["objectiv-c".to_string()]); + } + + #[test] + fn add_pattern_language_warnings_empty_when_all_valid() { + let tmp = tempdir().unwrap(); + let dir = tmp.path(); + let db = memory_db(); + let embedder = FakeEmbedder::new(); + + let result = add_pattern( + &db, + &embedder, + dir, + "All Valid", + "Body long enough for a chunk.", + &[], + // `golang` is the canonical token (display name "Go"); see + // src/engine/languages.rs entry around line 137. + &["rust", "golang"], + None, + ) + .unwrap(); + + assert!( + result.language_warnings.is_empty(), + "all-valid input must produce no warnings, got: {:?}", + result.language_warnings + ); + } + + #[test] + fn update_pattern_language_warnings_propagate_on_replace() { + let tmp = tempdir().unwrap(); + let dir = tmp.path(); + let db = memory_db(); + let embedder = FakeEmbedder::new(); + + fs::write( + dir.join("doc.md"), + "---\nlanguage: [rust]\n---\n\n# Doc\n\nOriginal body long enough.\n", + ) + .unwrap(); + + let result = update_pattern( + &db, + &embedder, + dir, + "doc.md", + "Replacement body long enough.", + None, + Some(&["objectiv-c"]), + None, + ) + .unwrap(); + + assert_eq!(result.language_warnings, vec!["objectiv-c".to_string()]); + } + + #[test] + fn update_pattern_language_warnings_fire_on_preserved_unknown_tokens() { + // Preserve-on-`None` re-renders the existing language list. The + // chunking parser's advisory then fires for any unknown token that + // was already on disk, surfacing it on this write even though the + // call did not pass `language`. + let tmp = tempdir().unwrap(); + let dir = tmp.path(); + let db = memory_db(); + let embedder = FakeEmbedder::new(); + + fs::write( + dir.join("legacy.md"), + "---\nlanguage: [objectiv-c]\n---\n\n# Legacy\n\nOriginal body long enough.\n", + ) + .unwrap(); + + let result = update_pattern( + &db, + &embedder, + dir, + "legacy.md", + "Replacement body long enough.", + None, + None, + None, + ) + .unwrap(); + + assert_eq!(result.language_warnings, vec!["objectiv-c".to_string()]); + } + + #[test] + fn update_pattern_preserve_lowercases_existing_mixed_case_language() { + // Intentional contract pin (correctness review finding 1): + // the language parser lowercases tokens at read time, so a body-only + // `update_pattern` against an existing `language: [Rust]` file + // rewrites the line as `language: [rust]`. The DB has stored the + // lowercased form since PR #50; the file converges to match the + // canonical form on the first body-only update. Asymmetric with + // `tags`'s preserve branch by design — see the explanatory comment + // in `update_pattern` for the rationale. + // + // A future change to either the language or tag parser's casing + // posture lands here: if the language parser stops lowercasing, this + // test starts failing and the author can decide whether the new + // behaviour is desired. + let tmp = tempdir().unwrap(); + let dir = tmp.path(); + let db = memory_db(); + let embedder = FakeEmbedder::new(); + + fs::write( + dir.join("mixed.md"), + "---\nlanguage: [Rust, Kotlin]\n---\n\n# Mixed\n\nOriginal body long enough.\n", + ) + .unwrap(); + + update_pattern( + &db, + &embedder, + dir, + "mixed.md", + "Replacement body long enough.", + None, + None, + None, + ) + .unwrap(); + + let content = fs::read_to_string(dir.join("mixed.md")).unwrap(); + assert!( + content.contains("language: [rust, kotlin]"), + "preserve branch must canonicalise mixed-case tokens to the \ + parser's lowercase form, got:\n{content}" + ); + // DB state must match — it already did before the rewrite, since + // the parser lowercases on read. + assert_eq!( + language_json_for(&db, "mixed.md").as_deref(), + Some("[\"rust\",\"kotlin\"]") + ); + } + + // Note: inbox-branch parity tests for `language_warnings` live in + // `tests/branch_push.rs` alongside the other inbox-branch coverage + // because they need the `temp_env::with_vars` neutralisation plus the + // bare-remote scaffolding the integration test module already exposes. + // -- append_to_pattern ------------------------------------------------- #[test] @@ -3130,6 +3739,7 @@ mod tests { "Git Test", "Body text that is long enough for a chunk.", &["test"], + &[], None, ) .unwrap(); @@ -3172,6 +3782,7 @@ mod tests { "Inbox Pattern", "Body content long enough for chunking.", &[], + &[], Some("inbox/"), ); @@ -3204,6 +3815,7 @@ mod tests { "doc.md", "Replacement body that is long enough.", Some(&[]), + None, Some("inbox/"), ); @@ -3251,7 +3863,16 @@ mod tests { let db = memory_db(); let embedder = FakeEmbedder::new(); - let result = add_pattern(&db, &embedder, dir, "!@#$%^&*()", "body text", &[], None); + let result = add_pattern( + &db, + &embedder, + dir, + "!@#$%^&*()", + "body text", + &[], + &[], + None, + ); assert!(result.is_err()); let msg = result.unwrap_err().to_string(); assert!( @@ -3282,7 +3903,7 @@ mod tests { let db = memory_db(); let embedder = FakeEmbedder::new(); - let result = update_pattern(&db, &embedder, dir, &rel, "new body", Some(&[]), None); + let result = update_pattern(&db, &embedder, dir, &rel, "new body", Some(&[]), None, None); assert!(result.is_err()); let msg = result.unwrap_err().to_string(); assert!( diff --git a/src/server.rs b/src/server.rs index 09c23a7..75d7761 100644 --- a/src/server.rs +++ b/src/server.rs @@ -341,7 +341,9 @@ fn tool_definitions() -> Value { "Create a new pattern in the knowledge base. Use only when the user explicitly \ asks to save, record, or document a pattern. Creates a markdown file and indexes \ it; the change is committed to git when the knowledge base is a git repository, \ - otherwise the file is written without a commit.", + otherwise the file is written without a commit. Pass `language` to opt the \ + pattern into the structural retrieval gate (warn-and-proceed for unknown \ + tokens).", "inputSchema": { "type": "object", "properties": { @@ -366,12 +368,28 @@ fn tool_definitions() -> Value { docs/pattern-authoring-guide.md §\"When to use the universal tag\" \ and §\"Tool/command predicate (applies_when)\"." }, + "language": { + "oneOf": [ + { "type": "string" }, + { "type": "array", "items": { "type": "string" } } + ], + "description": + "Optional language declaration that opts the pattern into the \ + structural retrieval gate. Accepts a single token (`\"rust\"`) or \ + an array (`[\"java\", \"kotlin\"]`). Tokens are lowercased and \ + validated against the canonical language table; unknown tokens \ + warn-and-proceed (the pattern is still written, the offending \ + tokens are surfaced on stderr and in the response's \ + `lore-metadata` fence as `language_warnings`)." + }, "include_metadata": { "type": "boolean", "description": "When true, appends a `lore-metadata` fenced code block to the \ end of the response containing machine-readable JSON with the \ - written file path, chunk count, and commit status. Defaults to false.", + written file path, chunk count, commit status, and \ + `language_warnings` (array of unknown language tokens; empty when \ + every token validated). Defaults to false.", "default": false } }, @@ -384,7 +402,9 @@ fn tool_definitions() -> Value { "Replace the content of an existing pattern. Use only when the user explicitly \ asks to update or rewrite a pattern. Overwrites the file and re-indexes; the \ change is committed to git when the knowledge base is a git repository, \ - otherwise the file is written without a commit.", + otherwise the file is written without a commit. `tags` and `language` share \ + three-way semantics (preserve when omitted, clear with `[]`, replace with a \ + non-empty value); unknown language tokens warn-and-proceed.", "inputSchema": { "type": "object", "properties": { @@ -409,12 +429,29 @@ fn tool_definitions() -> Value { injection tier (see `add_pattern`'s `tags` description for the \ interaction with `applies_when` predicates)." }, + "language": { + "oneOf": [ + { "type": "string" }, + { "type": "array", "items": { "type": "string" } } + ], + "description": + "Optional language declaration. Three-way semantics mirror `tags`: \ + omit to preserve the existing frontmatter `language:` list (the \ + default that prevents the de-language footgun on body-only \ + rewrites); pass an empty array `[]` to clear; pass a non-empty \ + value (string or array) to replace the list wholesale. Tokens are \ + lowercased and validated against the canonical language table; \ + unknown tokens warn-and-proceed and surface on stderr and in the \ + response's `lore-metadata` fence as `language_warnings`." + }, "include_metadata": { "type": "boolean", "description": "When true, appends a `lore-metadata` fenced code block to the \ end of the response containing machine-readable JSON with the \ - updated file path, chunk count, and commit status. Defaults to false.", + updated file path, chunk count, commit status, and \ + `language_warnings` (array of unknown language tokens; empty when \ + every token validated). Defaults to false.", "default": false } }, @@ -427,7 +464,9 @@ fn tool_definitions() -> Value { "Add a new section to an existing pattern without replacing it. Use when the user \ wants to add examples, edge cases, or notes to an existing pattern. Appends a \ heading and body and re-indexes; the change is committed to git when the \ - knowledge base is a git repository, otherwise the file is written without a commit.", + knowledge base is a git repository, otherwise the file is written without a commit. \ + Does not accept a `language` argument — appends are body-only by definition; use \ + `update_pattern` to change the frontmatter `language:` declaration.", "inputSchema": { "type": "object", "properties": { @@ -584,6 +623,7 @@ const MAX_SOURCE_FILE_BYTES: usize = 512; const MAX_HEADING_BYTES: usize = 512; const MAX_BODY_BYTES: usize = 262_144; // 256 KB const MAX_TAGS_BYTES: usize = 8192; // 8 KB serialised JSON +const MAX_LANGUAGE_BYTES: usize = 8192; // 8 KB serialised JSON const MAX_TOP_K: u64 = 100; /// Return an error response if `value` exceeds `max_bytes`. @@ -617,6 +657,96 @@ fn check_tags_limit(req: &JsonRpcRequest, args: &Value) -> Option Option { + if let Some(lang_val) = args.get("language") { + let serialised = serde_json::to_string(lang_val).unwrap_or_default(); + if serialised.len() > MAX_LANGUAGE_BYTES { + return Some(error_response( + req, + &format!("language exceeds maximum serialised size of {MAX_LANGUAGE_BYTES} bytes"), + )); + } + } + None +} + +/// Parse the MCP `language` argument into a canonical `Option>`. +/// +/// The MCP `inputSchema` accepts `oneOf [string, array]`, mirroring +/// the YAML frontmatter shapes [`crate::chunking::parse_frontmatter_language_list`] +/// already accepts (scalar, flow list, block list). This helper folds both +/// JSON shapes into the same canonical shape downstream code consumes: +/// +/// - Absent (`None`): no `language` key. `update_pattern` interprets this as +/// "preserve existing frontmatter `language:`", mirroring the `tags` +/// three-way semantics that prevent the body-only-rewrite footgun. +/// `add_pattern` has no existing list to preserve, so it folds the absent +/// case into the no-render path. +/// - `Some(vec![])`: caller passed `[]`. `update_pattern` clears the +/// frontmatter `language:` line; `add_pattern` writes no `language:` line. +/// - `Some(vec!["rust"])` (scalar input) or `Some(vec!["java", "kotlin"])` +/// (array input): caller passed a value. Both shapes coerce to `Vec`. +/// +/// Tokens are stored verbatim (no lowercasing or validation here — the +/// frontmatter parser owns canonical lowercasing and `is_known_token` +/// validation, so the MCP layer does not duplicate that logic and risk +/// diverging from it). Non-string array entries and non-string/non-array +/// top-level shapes return a structured error. +fn parse_language_arg( + req: &JsonRpcRequest, + args: &Value, +) -> Result>, JsonRpcResponse> { + let Some(lang_val) = args.get("language") else { + return Ok(None); + }; + let raw: Vec = if let Some(s) = lang_val.as_str() { + vec![s.to_string()] + } else if let Some(arr) = lang_val.as_array() { + let mut out = Vec::with_capacity(arr.len()); + for item in arr { + let Some(s) = item.as_str() else { + return Err(error_response( + req, + "language array entries must be strings", + )); + }; + out.push(s.to_string()); + } + out + } else { + return Err(error_response( + req, + "language must be a string or array of strings", + )); + }; + // Reject tokens that would corrupt the on-disk flow-list shape or the + // chunking parser's view of the frontmatter. The YAML flow-list renderer + // joins tokens with ', ' inside `[...]`; an embedded `,`, `]`, or + // newline therefore would either truncate the list or split the token + // on parser read-back, producing a silent round-trip mismatch the agent + // cannot easily detect. Hard-erroring at the MCP boundary is preferable + // to letting the chunking parser's defensive filter drop the token + // post-split. + for token in &raw { + if let Some(c) = token + .chars() + .find(|c| matches!(c, ',' | '[' | ']') || c.is_control()) + { + return Err(error_response( + req, + &format!( + "language token {token:?} contains forbidden character {c:?}; \ + tokens must not contain commas, square brackets, or control characters" + ), + )); + } + } + Ok(Some(raw)) +} + // --------------------------------------------------------------------------- // Tool handlers // --------------------------------------------------------------------------- @@ -814,6 +944,19 @@ fn handle_add(req: &JsonRpcRequest, ctx: &ServerContext<'_>, args: &Value) -> Js if let Some(err) = check_tags_limit(req, args) { return err; } + if let Some(err) = check_language_limit(req, args) { + return err; + } + // Parse the language argument at the MCP boundary so a malformed call + // hits a structured error before any write lock is taken. The + // empty-array case is semantically identical to absent for + // `add_pattern` (no existing list to preserve), so both fold into an + // empty borrow for the ingest helper. + let language_owned = match parse_language_arg(req, args) { + Ok(v) => v.unwrap_or_default(), + Err(resp) => return resp, + }; + let language_refs: Vec<&str> = language_owned.iter().map(String::as_str).collect(); eprintln!("[lore] Add pattern: \"{title}\""); @@ -833,6 +976,7 @@ fn handle_add(req: &JsonRpcRequest, ctx: &ServerContext<'_>, args: &Value) -> Js title, body, &tags, + &language_refs, ctx.config.inbox_branch_prefix(), ) { Ok(result) => { @@ -843,6 +987,7 @@ fn handle_add(req: &JsonRpcRequest, ctx: &ServerContext<'_>, args: &Value) -> Js "chunks_indexed": result.chunks_indexed, "embedding_failures": result.embedding_failures, "commit_status": commit_status_metadata(&result.commit_status), + "language_warnings": result.language_warnings, }); let prose = format!( "Pattern \"{}\" saved to {} ({} chunks indexed{}{embed_note}).", @@ -887,6 +1032,20 @@ fn handle_update(req: &JsonRpcRequest, ctx: &ServerContext<'_>, args: &Value) -> if let Some(err) = check_tags_limit(req, args) { return err; } + if let Some(err) = check_language_limit(req, args) { + return err; + } + // `language` mirrors `tags`'s three-way semantics: absent preserves, + // `[]` clears, `[...]` replaces. The parsed shape carries that + // distinction through to `ingest::update_pattern` which routes it into + // the preserve / clear / replace branches. + let language_owned: Option> = match parse_language_arg(req, args) { + Ok(v) => v, + Err(resp) => return resp, + }; + let language_refs: Option> = language_owned + .as_ref() + .map(|v| v.iter().map(String::as_str).collect()); eprintln!("[lore] Update pattern: \"{source_file}\""); @@ -906,6 +1065,7 @@ fn handle_update(req: &JsonRpcRequest, ctx: &ServerContext<'_>, args: &Value) -> source_file, body, tags_owned.as_deref(), + language_refs.as_deref(), ctx.config.inbox_branch_prefix(), ) { Ok(result) => { @@ -916,6 +1076,7 @@ fn handle_update(req: &JsonRpcRequest, ctx: &ServerContext<'_>, args: &Value) -> "chunks_indexed": result.chunks_indexed, "embedding_failures": result.embedding_failures, "commit_status": commit_status_metadata(&result.commit_status), + "language_warnings": result.language_warnings, }); let prose = format!( "Pattern {} updated ({} chunks re-indexed{}{embed_note}).", @@ -979,6 +1140,7 @@ fn handle_append(req: &JsonRpcRequest, ctx: &ServerContext<'_>, args: &Value) -> "chunks_indexed": result.chunks_indexed, "embedding_failures": result.embedding_failures, "commit_status": commit_status_metadata(&result.commit_status), + "language_warnings": result.language_warnings, }); let prose = format!( "Section \"{}\" appended to {} ({} chunks re-indexed{}{embed_note}).", @@ -2476,6 +2638,389 @@ mod tests { ); } + // -- language arg parsing (U1) ----------------------------------------- + // + // Cover the four shape categories the MCP boundary must handle: + // - absent → `None` (`update_pattern` interprets this as preserve) + // - scalar string → `Some(vec![s])` + // - array of strings → `Some(vec)` + // - malformed (non-string scalar, array with non-string entries) → + // structured error before the write lock is taken + // + // The handlers in U1 parse and validate; U2 wires the parsed value + // through to `ingest::*`. These tests pin the parsing contract directly + // via `parse_language_arg`, exercise the boundary-error path through the + // `add_pattern` handler (so malformed input fails before any write lock + // is taken), and assert the catalogue's schema-shape contract in the + // `tool_definitions_*` tests that follow. + + fn dummy_request() -> JsonRpcRequest { + serde_json::from_str(r#"{"jsonrpc":"2.0","id":1,"method":"tools/call"}"#).unwrap() + } + + /// Resolve [`parse_language_arg`] to its OK value, failing the test with + /// a stringified version of the error response when the helper errors. + /// `JsonRpcResponse` does not implement `Debug`, so `unwrap()` is not + /// available; this helper routes through `serde_json::to_string` instead. + fn parse_language_ok(req: &JsonRpcRequest, args: &Value) -> Option> { + match parse_language_arg(req, args) { + Ok(v) => v, + Err(resp) => panic!( + "parse_language_arg returned an error: {}", + serde_json::to_string(&resp).unwrap_or_default() + ), + } + } + + /// Resolve [`parse_language_arg`] to its error response, failing the test + /// if it succeeded. + fn parse_language_err(req: &JsonRpcRequest, args: &Value) -> JsonRpcResponse { + match parse_language_arg(req, args) { + Ok(v) => panic!("parse_language_arg succeeded unexpectedly with: {v:?}"), + Err(resp) => resp, + } + } + + #[test] + fn parse_language_arg_absent_returns_none() { + let req = dummy_request(); + let args = json!({}); + assert_eq!(parse_language_ok(&req, &args), None); + } + + #[test] + fn parse_language_arg_scalar_coerces_to_single_element_vec() { + let req = dummy_request(); + let args = json!({ "language": "rust" }); + assert_eq!( + parse_language_ok(&req, &args), + Some(vec!["rust".to_string()]) + ); + } + + #[test] + fn parse_language_arg_array_preserves_order_and_duplicates() { + let req = dummy_request(); + let args = json!({ "language": ["java", "kotlin", "java"] }); + assert_eq!( + parse_language_ok(&req, &args), + Some(vec![ + "java".to_string(), + "kotlin".to_string(), + "java".to_string(), + ]) + ); + } + + #[test] + fn parse_language_arg_empty_array_returns_some_empty_vec() { + // The empty-array case is semantically distinct from absent on + // `update_pattern` (clear vs preserve). The helper must preserve the + // distinction; the caller folds them together for `add_pattern`. + let req = dummy_request(); + let args = json!({ "language": [] }); + assert_eq!(parse_language_ok(&req, &args), Some(Vec::new())); + } + + #[test] + fn parse_language_arg_rejects_non_string_scalar() { + let req = dummy_request(); + let args = json!({ "language": 42 }); + let err = parse_language_err(&req, &args); + let value = serde_json::to_value(&err).unwrap(); + let message = value["error"]["message"].as_str().unwrap(); + assert!( + message.contains("string or array of strings"), + "error must name the accepted shapes, got: {message}" + ); + } + + #[test] + fn parse_language_arg_rejects_array_with_non_string_entries() { + let req = dummy_request(); + let args = json!({ "language": ["rust", 7] }); + let err = parse_language_err(&req, &args); + let value = serde_json::to_value(&err).unwrap(); + let message = value["error"]["message"].as_str().unwrap(); + assert!( + message.contains("array entries must be strings"), + "error must name the entry constraint, got: {message}" + ); + } + + #[test] + fn parse_language_arg_rejects_token_containing_comma() { + // Correctness review finding 2: a token with an embedded comma + // would render as `language: [a,b]` and the chunking parser would + // split it into two distinct tokens, producing a round-trip + // mismatch the agent cannot easily detect. Hard-erroring at the + // boundary surfaces the mistake before any write. + let req = dummy_request(); + let args = json!({ "language": "objective-c, swift" }); + let err = parse_language_err(&req, &args); + let value = serde_json::to_value(&err).unwrap(); + let message = value["error"]["message"].as_str().unwrap(); + assert!( + message.contains("forbidden character") && message.contains("commas"), + "error must name the delimiter constraint, got: {message}" + ); + } + + #[test] + fn parse_language_arg_rejects_token_containing_brackets() { + let req = dummy_request(); + let args = json!({ "language": ["rust[" ] }); + parse_language_err(&req, &args); + } + + #[test] + fn parse_language_arg_rejects_token_containing_newline() { + let req = dummy_request(); + let args = json!({ "language": "rust\ngo" }); + parse_language_err(&req, &args); + } + + #[test] + fn add_pattern_rejects_malformed_language_before_write_lock() { + // Regression guard: a non-string/non-array `language` argument must + // hit a structured error before any write or lock acquisition. The + // assertion uses the response's error shape — if the handler were to + // ignore the argument it would succeed and create the pattern. + let h = TestHarness::new(); + let resp = h.request_value( + r#"{ + "jsonrpc":"2.0","id":31,"method":"tools/call", + "params":{ + "name":"add_pattern", + "arguments":{ + "title":"Test Malformed", + "body":"Body text that is long enough for a chunk.", + "language": 42 + } + } + }"#, + ); + assert!(!resp["error"].is_null(), "expected an error response"); + let message = resp["error"]["message"].as_str().unwrap(); + assert!( + message.contains("string or array of strings"), + "expected a structured shape error, got: {message}" + ); + } + + #[test] + fn add_pattern_rejects_language_exceeding_size_limit() { + let h = TestHarness::new(); + // Build an array of single-character tokens whose serialised JSON + // size exceeds the limit. Each `"a",` adds 4 bytes of serialised + // form; a comfortably-over-limit payload uses 3000 entries. + let huge: Vec<&str> = (0..3000).map(|_| "a").collect(); + let req_json = serde_json::json!({ + "jsonrpc":"2.0","id":32,"method":"tools/call", + "params":{ + "name":"add_pattern", + "arguments":{ + "title":"Test Limit", + "body":"Body text that is long enough for a chunk.", + "language": huge + } + } + }); + let resp = h.request_value(&req_json.to_string()); + let message = resp["error"]["message"].as_str().unwrap(); + assert!( + message.contains("language exceeds maximum"), + "expected the language limit error, got: {message}" + ); + } + + // -- tool_definitions schema shape (U1 contract pins) ----------------- + // + // The schemas are the MCP contract — these tests pin the `language` + // property's presence and shape on the two write tools that accept it, + // and (in U4) pin its absence on `append_to_pattern`. Drift in either + // direction silently reintroduces the dropped-argument failure mode the + // plan exists to fix. + + fn tool_schema(name: &str) -> Value { + let tools = tool_definitions(); + let arr = tools.as_array().expect("tool_definitions returns an array"); + arr.iter() + .find(|t| t["name"] == name) + .unwrap_or_else(|| panic!("tool {name} not found in tool_definitions")) + .clone() + } + + #[test] + fn tool_definitions_add_pattern_has_language_oneof_shape() { + let tool = tool_schema("add_pattern"); + let lang = &tool["inputSchema"]["properties"]["language"]; + assert!(lang.is_object(), "add_pattern must expose `language`"); + let one_of = lang["oneOf"].as_array().expect("language.oneOf array"); + assert_eq!(one_of.len(), 2, "language.oneOf must have two branches"); + assert_eq!(one_of[0]["type"], "string"); + assert_eq!(one_of[1]["type"], "array"); + assert_eq!(one_of[1]["items"]["type"], "string"); + assert!( + lang["description"].as_str().is_some_and(|s| !s.is_empty()), + "language.description must be non-empty" + ); + let required: Vec<&str> = tool["inputSchema"]["required"] + .as_array() + .unwrap() + .iter() + .filter_map(Value::as_str) + .collect(); + assert!( + !required.contains(&"language"), + "language must remain optional" + ); + // Regression guards on the original required fields and sibling + // properties so a careless edit cannot silently drop them. + assert!(required.contains(&"title") && required.contains(&"body")); + let props = &tool["inputSchema"]["properties"]; + for sibling in ["title", "body", "tags", "include_metadata"] { + assert!(props.get(sibling).is_some(), "{sibling} must stay present"); + } + } + + #[test] + fn tool_definitions_update_pattern_has_language_oneof_shape() { + let tool = tool_schema("update_pattern"); + let lang = &tool["inputSchema"]["properties"]["language"]; + assert!(lang.is_object(), "update_pattern must expose `language`"); + let one_of = lang["oneOf"].as_array().expect("language.oneOf array"); + assert_eq!(one_of.len(), 2); + assert_eq!(one_of[0]["type"], "string"); + assert_eq!(one_of[1]["type"], "array"); + assert_eq!(one_of[1]["items"]["type"], "string"); + let required: Vec<&str> = tool["inputSchema"]["required"] + .as_array() + .unwrap() + .iter() + .filter_map(Value::as_str) + .collect(); + assert!(!required.contains(&"language")); + assert!(required.contains(&"source_file") && required.contains(&"body")); + let props = &tool["inputSchema"]["properties"]; + for sibling in ["source_file", "body", "tags", "include_metadata"] { + assert!(props.get(sibling).is_some(), "{sibling} must stay present"); + } + } + + #[test] + fn tool_definitions_append_pattern_does_not_accept_language() { + // Load-bearing pin for Decision 3 (schema honesty over surface + // symmetry). A future symmetry-driven PR adding `language` to append + // would silently reintroduce the dropped-argument footgun on what + // an agent believes is a body-only operation. This test fails fast + // if that happens. + let tool = tool_schema("append_to_pattern"); + let props = &tool["inputSchema"]["properties"]; + assert!( + props.get("language").is_none(), + "append_to_pattern must not declare a `language` property; \ + frontmatter changes belong on update_pattern" + ); + for sibling in ["source_file", "heading", "body", "include_metadata"] { + assert!(props.get(sibling).is_some(), "{sibling} must stay present"); + } + } + + // -- tool description prose (U4 drift guards) -------------------------- + // + // The schema descriptions are the contract agents read when they list + // tools, so the keywords below are part of the public API surface. Each + // assertion below pins a specific failure mode by name: if a future edit + // drops the keyword the test calls out, the test fails, and the author + // either restores the keyword or the test's failure message names what + // changed and why. Substring matches keyed on stable nouns + // (`language`, `update_pattern`, `preserve` / `clear` / `replace`) keep + // the assertions resilient to innocent wording tweaks. + + /// Helper: fetch a tool's top-level `description` string. + fn tool_description(name: &str) -> String { + tool_schema(name)["description"] + .as_str() + .expect("description must be a string") + .to_string() + } + + #[test] + fn add_pattern_description_mentions_language_and_warn_behaviour() { + let desc = tool_description("add_pattern"); + assert!( + desc.contains("language"), + "add_pattern description must mention `language`, got: {desc}" + ); + assert!( + desc.contains("warn-and-proceed") || desc.contains("unknown"), + "add_pattern description must document the unknown-token \ + advisory behaviour, got: {desc}" + ); + } + + #[test] + fn update_pattern_description_mentions_language_and_three_way_semantics() { + let desc = tool_description("update_pattern"); + assert!( + desc.contains("language"), + "update_pattern description must mention `language`, got: {desc}" + ); + // The three-way semantics keywords are load-bearing: an agent + // reading the description must learn the preserve/clear/replace + // distinction without having to test the boundary themselves. + assert!( + desc.contains("preserve"), + "update_pattern description must document the preserve case, got: {desc}" + ); + assert!( + desc.contains("clear"), + "update_pattern description must document the clear case, got: {desc}" + ); + assert!( + desc.contains("replace"), + "update_pattern description must document the replace case, got: {desc}" + ); + } + + #[test] + fn append_pattern_description_points_at_update_pattern_for_language() { + // The description is how agents discover the right tool when they + // want to change frontmatter; the pointer is part of the schema + // contract per Decision 3 in the plan. + let desc = tool_description("append_to_pattern"); + assert!( + desc.contains("update_pattern"), + "append_to_pattern description must point at update_pattern for \ + frontmatter changes, got: {desc}" + ); + assert!( + desc.contains("language"), + "append_to_pattern description must mention `language` to flag \ + the missing field for the agent, got: {desc}" + ); + } + + #[test] + fn update_pattern_language_field_description_documents_three_way_semantics() { + // The per-field description is what agents see when they expand a + // schema in their tool registry. Pin the three keywords directly so + // the field-level contract cannot drift away from the tool-level + // contract. + let tool = tool_schema("update_pattern"); + let lang_desc = tool["inputSchema"]["properties"]["language"]["description"] + .as_str() + .expect("language.description must be a string"); + for keyword in ["preserve", "clear", "replace"] { + assert!( + lang_desc.contains(keyword), + "update_pattern.language.description must contain `{keyword}` \ + to document the three-way semantics, got: {lang_desc}" + ); + } + } + // -- lore_status ------------------------------------------------------- #[test] @@ -3046,6 +3591,51 @@ mod tests { metadata["file_path"].is_string(), "metadata should expose file_path" ); + // U3: the field is always present, even on the all-valid baseline, + // so agents can pattern-match on `language_warnings` without first + // checking for key existence. + let warnings = metadata["language_warnings"] + .as_array() + .expect("language_warnings must be an array on the metadata fence"); + assert!( + warnings.is_empty(), + "no language passed must yield an empty warnings array, got: {warnings:?}" + ); + } + + #[test] + fn add_pattern_metadata_fence_carries_language_warnings_for_unknown_tokens() { + // U3 contract: an `add_pattern` call carrying an unknown language + // token writes the file (warn-and-proceed) and surfaces the + // offending token through the metadata fence so MCP-side agents can + // detect the drift without scraping stderr. + let h = TestHarness::new(); + let resp = h.request_value( + r#"{ + "jsonrpc":"2.0","id":80,"method":"tools/call", + "params":{ + "name":"add_pattern", + "arguments":{ + "title":"Lang Warn", + "body":"Body text that is long enough for chunking.", + "language":["rust","objectiv-c"], + "include_metadata":true + } + } + }"#, + ); + + assert!(resp["error"].is_null()); + let metadata = metadata_from_response(&resp); + let warnings = metadata["language_warnings"] + .as_array() + .expect("language_warnings must be an array on the metadata fence"); + assert_eq!( + warnings.len(), + 1, + "expected exactly one warning for the single unknown token, got: {warnings:?}" + ); + assert_eq!(warnings[0], "objectiv-c"); } /// When the caller does NOT pass `include_metadata: true`, the write diff --git a/src/snapshots/lore__server__tests__tools_list_returns_all_six_tools.snap b/src/snapshots/lore__server__tests__tools_list_returns_all_six_tools.snap index 63e4d2e..3d5f2e3 100644 --- a/src/snapshots/lore__server__tests__tools_list_returns_all_six_tools.snap +++ b/src/snapshots/lore__server__tests__tools_list_returns_all_six_tools.snap @@ -33,7 +33,7 @@ expression: resp "name": "search_patterns" }, { - "description": "Create a new pattern in the knowledge base. Use only when the user explicitly asks to save, record, or document a pattern. Creates a markdown file and indexes it; the change is committed to git when the knowledge base is a git repository, otherwise the file is written without a commit.", + "description": "Create a new pattern in the knowledge base. Use only when the user explicitly asks to save, record, or document a pattern. Creates a markdown file and indexes it; the change is committed to git when the knowledge base is a git repository, otherwise the file is written without a commit. Pass `language` to opt the pattern into the structural retrieval gate (warn-and-proceed for unknown tokens).", "inputSchema": { "properties": { "body": { @@ -42,9 +42,23 @@ expression: resp }, "include_metadata": { "default": false, - "description": "When true, appends a `lore-metadata` fenced code block to the end of the response containing machine-readable JSON with the written file path, chunk count, and commit status. Defaults to false.", + "description": "When true, appends a `lore-metadata` fenced code block to the end of the response containing machine-readable JSON with the written file path, chunk count, commit status, and `language_warnings` (array of unknown language tokens; empty when every token validated). Defaults to false.", "type": "boolean" }, + "language": { + "description": "Optional language declaration that opts the pattern into the structural retrieval gate. Accepts a single token (`\"rust\"`) or an array (`[\"java\", \"kotlin\"]`). Tokens are lowercased and validated against the canonical language table; unknown tokens warn-and-proceed (the pattern is still written, the offending tokens are surfaced on stderr and in the response's `lore-metadata` fence as `language_warnings`).", + "oneOf": [ + { + "type": "string" + }, + { + "items": { + "type": "string" + }, + "type": "array" + } + ] + }, "tags": { "description": "Optional tags for categorisation. Passing `\"universal\"` opts the pattern into the always-on injection tier — its body is emitted at every SessionStart and bypasses PreToolUse deduplication, unless the body's frontmatter also declares an `applies_when` predicate (in which case the pattern is deferred from SessionStart and re-injects on matching PreToolUse calls instead). See docs/pattern-authoring-guide.md §\"When to use the universal tag\" and §\"Tool/command predicate (applies_when)\".", "items": { @@ -66,7 +80,7 @@ expression: resp "name": "add_pattern" }, { - "description": "Replace the content of an existing pattern. Use only when the user explicitly asks to update or rewrite a pattern. Overwrites the file and re-indexes; the change is committed to git when the knowledge base is a git repository, otherwise the file is written without a commit.", + "description": "Replace the content of an existing pattern. Use only when the user explicitly asks to update or rewrite a pattern. Overwrites the file and re-indexes; the change is committed to git when the knowledge base is a git repository, otherwise the file is written without a commit. `tags` and `language` share three-way semantics (preserve when omitted, clear with `[]`, replace with a non-empty value); unknown language tokens warn-and-proceed.", "inputSchema": { "properties": { "body": { @@ -75,9 +89,23 @@ expression: resp }, "include_metadata": { "default": false, - "description": "When true, appends a `lore-metadata` fenced code block to the end of the response containing machine-readable JSON with the updated file path, chunk count, and commit status. Defaults to false.", + "description": "When true, appends a `lore-metadata` fenced code block to the end of the response containing machine-readable JSON with the updated file path, chunk count, commit status, and `language_warnings` (array of unknown language tokens; empty when every token validated). Defaults to false.", "type": "boolean" }, + "language": { + "description": "Optional language declaration. Three-way semantics mirror `tags`: omit to preserve the existing frontmatter `language:` list (the default that prevents the de-language footgun on body-only rewrites); pass an empty array `[]` to clear; pass a non-empty value (string or array) to replace the list wholesale. Tokens are lowercased and validated against the canonical language table; unknown tokens warn-and-proceed and surface on stderr and in the response's `lore-metadata` fence as `language_warnings`.", + "oneOf": [ + { + "type": "string" + }, + { + "items": { + "type": "string" + }, + "type": "array" + } + ] + }, "source_file": { "description": "Relative path of the file to update (from search results)", "type": "string" @@ -99,7 +127,7 @@ expression: resp "name": "update_pattern" }, { - "description": "Add a new section to an existing pattern without replacing it. Use when the user wants to add examples, edge cases, or notes to an existing pattern. Appends a heading and body and re-indexes; the change is committed to git when the knowledge base is a git repository, otherwise the file is written without a commit.", + "description": "Add a new section to an existing pattern without replacing it. Use when the user wants to add examples, edge cases, or notes to an existing pattern. Appends a heading and body and re-indexes; the change is committed to git when the knowledge base is a git repository, otherwise the file is written without a commit. Does not accept a `language` argument — appends are body-only by definition; use `update_pattern` to change the frontmatter `language:` declaration.", "inputSchema": { "properties": { "body": { diff --git a/tests/branch_push.rs b/tests/branch_push.rs index 9e29215..fa46e9e 100644 --- a/tests/branch_push.rs +++ b/tests/branch_push.rs @@ -133,6 +133,7 @@ fn add_pattern_pushes_to_inbox_branch() { "Error Handling", "Use anyhow for application errors.\n", &["rust"], + &[], Some("inbox/"), ) .unwrap(); @@ -197,6 +198,7 @@ fn update_pattern_pushes_modified_file() { "testing.md", "New testing content with property-based tests.\n", Some(&["testing"]), + None, Some("inbox/"), ) .unwrap(); @@ -287,6 +289,7 @@ fn two_adds_create_independent_branches() { "Pattern Alpha", "Alpha content.\n", &[], + &[], Some("inbox/"), ) .unwrap(); @@ -298,6 +301,7 @@ fn two_adds_create_independent_branches() { "Pattern Beta", "Beta content.\n", &[], + &[], Some("inbox/"), ) .unwrap(); @@ -335,6 +339,7 @@ fn same_title_disambiguates_branch_name() { "My Pattern", "First version.\n", &[], + &[], Some("inbox/"), ) .unwrap(); @@ -346,6 +351,7 @@ fn same_title_disambiguates_branch_name() { "My Pattern", "Second version.\n", &[], + &[], Some("inbox/"), ) .unwrap(); @@ -378,6 +384,7 @@ fn no_git_config_preserves_default_behavior() { "Local Pattern", "Body content that is long enough for a chunk.\n", &["local"], + &[], None, ) .unwrap(); @@ -418,6 +425,7 @@ fn push_failure_is_hard_error() { "Will Fail", "This should fail on push.\n", &[], + &[], Some("inbox/"), ); @@ -428,3 +436,110 @@ fn push_failure_is_hard_error() { "error should mention push/remote, got: {msg}" ); } + +// --------------------------------------------------------------------------- +// Inbox-branch language_warnings parity (U3 / Residual 2) +// +// The inbox-branch short-circuit historically skipped `index_single_file`, +// so unknown-language-token advisories never reached the caller for the +// dominant agent-submission path. U3 closes that by invoking +// `parse_frontmatter_language_list` directly on the about-to-be-written +// content before the push. These integration tests pin both the warn case +// and the all-valid baseline against a real bare-remote setup. +// --------------------------------------------------------------------------- + +#[test] +fn inbox_add_pattern_collects_unknown_language_tokens() { + with_neutralised_git_env(|| { + let tmp = tempdir().unwrap(); + let dir = tmp.path(); + let _bare = setup_repo_with_remote(dir); + let embedder = FakeEmbedder::new(); + let db = open_db(dir, embedder.dimensions()); + + let result = ingest::add_pattern( + &db, + &embedder, + dir, + "Inbox Lang Warn", + "Body content long enough for chunking.\n", + &[], + &["objectiv-c"], + Some("inbox/"), + ) + .expect("inbox-branch add should succeed against a wired bare remote"); + + assert!(matches!(result.commit_status, CommitStatus::Pushed { .. })); + assert_eq!( + result.language_warnings, + vec!["objectiv-c".to_string()], + "inbox-branch path must surface unknown tokens via the direct \ + parser invocation; without it the dropped-argument failure mode \ + would silently reappear on the agent-submission path" + ); + }); +} + +#[test] +fn inbox_add_pattern_returns_empty_warnings_when_tokens_are_valid() { + with_neutralised_git_env(|| { + let tmp = tempdir().unwrap(); + let dir = tmp.path(); + let _bare = setup_repo_with_remote(dir); + let embedder = FakeEmbedder::new(); + let db = open_db(dir, embedder.dimensions()); + + let result = ingest::add_pattern( + &db, + &embedder, + dir, + "Inbox Clean", + "Body content long enough for chunking.\n", + &[], + &["rust"], + Some("inbox/"), + ) + .expect("inbox-branch add should succeed"); + + assert!( + result.language_warnings.is_empty(), + "all-valid input on the inbox-branch path must produce an empty \ + warnings vec (not omitted) so MCP callers can render the field \ + uniformly" + ); + }); +} + +#[test] +fn inbox_update_pattern_collects_unknown_language_tokens() { + with_neutralised_git_env(|| { + let tmp = tempdir().unwrap(); + let dir = tmp.path(); + let _bare = setup_repo_with_remote(dir); + let embedder = FakeEmbedder::new(); + let db = open_db(dir, embedder.dimensions()); + + // Seed an existing file on the working tree so `update_pattern`'s + // file-exists guard passes. + fs::write( + dir.join("doc.md"), + "# Doc\n\nOriginal body long enough for a chunk.\n", + ) + .unwrap(); + + let result = ingest::update_pattern( + &db, + &embedder, + dir, + "doc.md", + "Replacement body long enough.\n", + None, + Some(&["objectiv-c"]), + Some("inbox/"), + ) + .expect("inbox-branch update should succeed"); + + assert!(matches!(result.commit_status, CommitStatus::Pushed { .. })); + assert_eq!(result.language_warnings, vec!["objectiv-c".to_string()]); + }); +} diff --git a/tests/e2e.rs b/tests/e2e.rs index 8555e06..7bf3a8b 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -137,6 +137,7 @@ fn full_lifecycle() { Always include span context for distributed tracing.\n\ Log at warn level for recoverable errors, error level for unrecoverable.\n", &["observability", "rust"], + &[], None, ) .unwrap(); @@ -183,6 +184,7 @@ fn full_lifecycle() { Never use println for diagnostic output in production.\n", Some(&["observability", "production"]), None, + None, ) .unwrap(); diff --git a/tests/ollama_integration.rs b/tests/ollama_integration.rs index af4ded8..f352ac4 100644 --- a/tests/ollama_integration.rs +++ b/tests/ollama_integration.rs @@ -222,6 +222,7 @@ fn ollama_lifecycle() { Always include span context for distributed tracing.\n\ Log at warn level for recoverable errors, error level for unrecoverable.\n", &["observability", "rust"], + &[], None, ) .unwrap(); @@ -248,6 +249,7 @@ fn ollama_lifecycle() { Never use println for diagnostic output in production.\n", Some(&["observability", "production"]), None, + None, ) .unwrap();