diff --git a/packages/python-sdk-memwal/memwal/client.py b/packages/python-sdk-memwal/memwal/client.py index eb32853e..2d7601d6 100644 --- a/packages/python-sdk-memwal/memwal/client.py +++ b/packages/python-sdk-memwal/memwal/client.py @@ -614,6 +614,7 @@ async def analyze( text: str, namespace: Optional[str] = None, occurred_at: Optional[Union[str, datetime]] = None, + extract_with_critique: bool = False, ) -> AnalyzeResult: """Analyze conversation text and return as soon as facts are accepted. @@ -645,6 +646,15 @@ async def analyze( (no ``now()`` fallback). The resolved date lives only inside the encrypted fact text + embedding; there is no server-readable metadata column for it (Architecture A). + extract_with_critique: Opt into the two-pass extraction + critique. When ``True``, the server runs a + second LLM pass that critiques and corrects the + first-pass extracted facts before any fact is embedded + or stored. The critic sees the same ``occurred_at`` + anchor and related-memories context the first pass + saw. **Doubles the per-analyze LLM call count** — + default ``False`` preserves the single-pass write + path. Wire field is omitted when ``False``. Returns: :class:`AnalyzeResult` with extracted ``facts`` + per-fact @@ -657,6 +667,11 @@ async def analyze( wire_occurred_at = _occurred_at_to_wire(occurred_at) if wire_occurred_at is not None: body["occurred_at"] = wire_occurred_at + # Only include the critique flag when explicitly True; matches + # the server's `#[serde(default)]` shape so the wire payload is + # byte-identical for existing callers. + if extract_with_critique: + body["extract_with_critique"] = True data = await self._signed_request( "POST", "/api/analyze", @@ -690,6 +705,7 @@ async def analyze_and_wait( namespace: Optional[str] = None, opts: Optional[RememberBulkOptions] = None, occurred_at: Optional[Union[str, datetime]] = None, + extract_with_critique: bool = False, ) -> AnalyzeWaitResult: """Analyze + wait for every extracted fact to finish persisting. @@ -698,11 +714,16 @@ async def analyze_and_wait( result combines the analyze fact list with the bulk-style settled per-job results. - ``occurred_at`` carries the same temporal-anchor semantics as - :meth:`analyze` — see that method's docstring for details. + ``occurred_at`` and ``extract_with_critique`` carry the same + semantics as :meth:`analyze` — see that method's docstring. """ - accepted = await self.analyze(text, namespace, occurred_at=occurred_at) + accepted = await self.analyze( + text, + namespace, + occurred_at=occurred_at, + extract_with_critique=extract_with_critique, + ) completed = await self.wait_for_remember_jobs(accepted.job_ids, opts) return AnalyzeWaitResult( results=completed.results, @@ -1360,9 +1381,17 @@ def analyze( text: str, namespace: Optional[str] = None, occurred_at: Optional[Union[str, datetime]] = None, + extract_with_critique: bool = False, ) -> AnalyzeResult: """Synchronous version of :meth:`MemWal.analyze`.""" - return self._run(self._inner.analyze(text, namespace, occurred_at=occurred_at)) + return self._run( + self._inner.analyze( + text, + namespace, + occurred_at=occurred_at, + extract_with_critique=extract_with_critique, + ) + ) def analyze_and_wait( self, @@ -1370,10 +1399,17 @@ def analyze_and_wait( namespace: Optional[str] = None, opts: Optional[RememberBulkOptions] = None, occurred_at: Optional[Union[str, datetime]] = None, + extract_with_critique: bool = False, ) -> AnalyzeWaitResult: """Synchronous version of :meth:`MemWal.analyze_and_wait`.""" return self._run( - self._inner.analyze_and_wait(text, namespace, opts, occurred_at=occurred_at) + self._inner.analyze_and_wait( + text, + namespace, + opts, + occurred_at=occurred_at, + extract_with_critique=extract_with_critique, + ) ) def embed(self, text: str) -> EmbedResult: diff --git a/packages/python-sdk-memwal/tests/test_client.py b/packages/python-sdk-memwal/tests/test_client.py index 833a2bac..31a7996a 100644 --- a/packages/python-sdk-memwal/tests/test_client.py +++ b/packages/python-sdk-memwal/tests/test_client.py @@ -592,6 +592,43 @@ async def test_analyze_naive_string_raises( occurred_at="2023-05-25T17:50:00", # no Z, no offset ) + @respx.mock + async def test_analyze_with_extract_with_critique_true_sends_flag( + self, memwal_client: MemWal + ) -> None: + """When ``extract_with_critique=True`` the wire body must + carry ``extract_with_critique: true`` so the server routes + through the two-pass critique path.""" + mock_seal_session_prereqs() + route = respx.post(f"{_TEST_SERVER}/api/analyze").mock( + return_value=httpx.Response(200, json={"facts": [], "total": 0, "owner": ""}) + ) + + await memwal_client.analyze( + "I moved last Friday", + extract_with_critique=True, + ) + + body = json.loads(route.calls[0].request.content) + assert body["extract_with_critique"] is True + + @respx.mock + async def test_analyze_with_extract_with_critique_false_omits_flag( + self, memwal_client: MemWal + ) -> None: + """Default ``False`` must omit the field from the wire body so + existing callers see byte-identical payloads (the server's + ``#[serde(default)]`` resolves the absent field to ``false``).""" + mock_seal_session_prereqs() + route = respx.post(f"{_TEST_SERVER}/api/analyze").mock( + return_value=httpx.Response(200, json={"facts": [], "total": 0, "owner": ""}) + ) + + await memwal_client.analyze("I moved last Friday") + + body = json.loads(route.calls[0].request.content) + assert "extract_with_critique" not in body + class TestRestore: @respx.mock diff --git a/packages/sdk/src/memwal.ts b/packages/sdk/src/memwal.ts index c9c9cadc..24ce58f4 100644 --- a/packages/sdk/src/memwal.ts +++ b/packages/sdk/src/memwal.ts @@ -736,6 +736,10 @@ export class MemWal { }; const wireOccurredAt = occurredAtToWire(options.occurredAt); if (wireOccurredAt !== undefined) body.occurred_at = wireOccurredAt; + // Only include the critique flag when explicitly true; default + // off matches the server's `#[serde(default)]` shape and keeps + // the wire payload byte-identical for existing callers. + if (options.extractWithCritique === true) body.extract_with_critique = true; return this.signedRequest("POST", "/api/analyze", body, [200, 202]); } diff --git a/packages/sdk/src/types.ts b/packages/sdk/src/types.ts index 442ae1da..89f166fd 100644 --- a/packages/sdk/src/types.ts +++ b/packages/sdk/src/types.ts @@ -183,6 +183,19 @@ export interface AnalyzeOptions { * no server-readable metadata column for it (Architecture A). */ occurredAt?: string | Date; + /** + * Opt-in two-pass extraction with self-critique. When `true`, the + * server runs a second LLM pass that critiques and corrects the + * first-pass extracted facts before any fact is embedded or + * stored. The critic sees the same `occurredAt` anchor and + * related-memories context the first pass saw. + * + * **Cost:** doubles the per-analyze LLM call count. Default + * `false` (or undefined) preserves the single-pass write path — + * only flip when the measured benefit justifies the cost. Wire + * field is omitted when `false`/`undefined`. + */ + extractWithCritique?: boolean; } /** A fact extracted by analyze() and accepted for background storage. */ diff --git a/services/server/benchmarks/core/client.py b/services/server/benchmarks/core/client.py index 0a9d0b23..07e06231 100644 --- a/services/server/benchmarks/core/client.py +++ b/services/server/benchmarks/core/client.py @@ -155,6 +155,7 @@ def analyze( text: str, namespace: str, occurred_at: str | None = None, + extract_with_critique: bool = False, ) -> AnalyzeResult: """ POST /api/analyze — feed conversation text, extract and store memories. @@ -175,6 +176,13 @@ def analyze( *inside the extracted fact text* — making time-anchored facts retrievable. Omit (or pass None) when no anchor is available; the server does NOT default to now() — silence is honest. + + `extract_with_critique` flips the server into the + two-pass critique path: first pass via the normal + extract.v6 extractor, second pass via the critique.v1 + critic that verifies and corrects the first-pass facts. + Doubles the per-analyze LLM call count when set. Default + False preserves the single-pass write path. """ body: dict = { "text": text, @@ -182,6 +190,8 @@ def analyze( } if occurred_at is not None: body["occurred_at"] = occurred_at + if extract_with_critique: + body["extract_with_critique"] = True data = self._post("/api/analyze", body) # Response shape varies between server versions: # - Synchronous (reference branch): {facts, total, owner} diff --git a/services/server/benchmarks/run.py b/services/server/benchmarks/run.py index a563733f..477222b0 100644 --- a/services/server/benchmarks/run.py +++ b/services/server/benchmarks/run.py @@ -156,6 +156,17 @@ def stage_ingest( start = time.time() concurrency = config.get("benchmarks", {}).get("concurrency", 10) + # when true, every analyze call routes through the two-pass + # critique path on the server. Doubles per-analyze LLM cost; we only + # flip it for the critique-stack run, not the baseline. + extract_with_critique = config.get("benchmarks", {}).get( + "extract_with_critique", False + ) + if extract_with_critique: + print( + " Two-pass critique extractor ENABLED — expect ~2x " + "ingestion wall time vs the single-pass baseline." + ) # Build tasks grouped by conversation. Each conversation's chunks are # processed SERIALLY (to mirror real-time message ordering and avoid @@ -222,7 +233,14 @@ def ingest_one_conversation(task): # pass occurred_at (RFC 3339 UTC string or None) # to the server. When present, the server uses it as the # temporal anchor for the extractor prompt. - result = client.analyze(text, namespace, occurred_at=occurred_at) + # extract_with_critique flips the server into the + # two-pass critique path; default False = single-pass. + result = client.analyze( + text, + namespace, + occurred_at=occurred_at, + extract_with_critique=extract_with_critique, + ) memory_ids = [fact.get("id", "") for fact in result.facts if fact.get("id")] if memory_ids: key = session_map_key(conv_id, session_id) @@ -438,6 +456,12 @@ def process_query(query): "mode": mode, "judge_model": config.get("judge", {}).get("model", ""), "answer_model": config.get("answer", {}).get("model", ""), + # pin which write-path the ingest ran through so + # later deltas are attributable. Recorded even when False + # so baseline runs are explicit about it. + "extract_with_critique": config.get("benchmarks", {}).get( + "extract_with_critique", False + ), }, metrics_overall=_dict_to_category_metrics(overall), metrics_by_category={cat: _dict_to_category_metrics(m) for cat, m in by_category.items()}, @@ -574,6 +598,12 @@ def build_parser() -> argparse.ArgumentParser: ing = sub.add_parser("ingest", help="Ingest benchmark conversations into Walrus Memory") ing.add_argument("benchmark", choices=list(BENCHMARKS.keys())) ing.add_argument("--run-id", default=None) + ing.add_argument( + "--extract-with-critique", + action="store_true", + help="Route every ingest call through the two-pass critique extractor. " + "Doubles per-analyze LLM cost — only flip when measuring the critique stack.", + ) # eval ev = sub.add_parser("eval", help="Evaluate retrieval with a single preset") @@ -596,6 +626,12 @@ def build_parser() -> argparse.ArgumentParser: full.add_argument("--mode", choices=["retrieval", "e2e"], default="e2e") full.add_argument("--run-id", default=None, help="Reuse an existing ingestion") full.add_argument("--skip-ingest", action="store_true", help="Skip ingestion stage (assumes run-id already ingested)") + full.add_argument( + "--extract-with-critique", + action="store_true", + help="Route every ingest call through the two-pass critique extractor. " + "Doubles per-analyze LLM cost — only flip when measuring the critique stack.", + ) # report rpt = sub.add_parser("report", help="View results") @@ -641,6 +677,14 @@ def main(): config = load_config() server_cfg = config["server"] + # Surface CLI flags into the config dict so stage_* helpers (which + # don't take args directly) can read them. Only command-level flags + # that influence ingestion / eval go here — top-level flags like + # --verbose are handled separately. + benchmarks_cfg = config.setdefault("benchmarks", {}) + if getattr(args, "extract_with_critique", False): + benchmarks_cfg["extract_with_critique"] = True + client = MemWalClient( server_url=server_cfg["url"], delegate_key_hex=server_cfg["delegate_key"], @@ -690,7 +734,11 @@ def main(): "metadata. Upgrade the server to a build that exposes these fields." ) sys.exit(1) - print(f" prompt versions: extract={prompt_versions['extract']} ask={prompt_versions['ask']}") + critique_ver = prompt_versions.get("critique", "n/a") + print( + f" prompt versions: extract={prompt_versions['extract']} " + f"critique={critique_ver} ask={prompt_versions['ask']}" + ) # Stash on `config` so stage_eval picks it up without a signature change. config["_server_prompt_versions"] = prompt_versions except SystemExit: diff --git a/services/server/src/routes/admin.rs b/services/server/src/routes/admin.rs index d5b55ac8..d07387a3 100644 --- a/services/server/src/routes/admin.rs +++ b/services/server/src/routes/admin.rs @@ -129,6 +129,8 @@ pub async fn health(State(state): State>) -> Json // and ask (`/api/ask`) — no separate config to drift. prompt_versions: PromptVersions { extract: crate::services::extractor::FACT_EXTRACTION_PROMPT_VERSION.to_string(), + critique: crate::services::extractor::FACT_EXTRACTION_CRITIQUE_PROMPT_VERSION + .to_string(), ask: ASK_SYSTEM_PROMPT_VERSION.to_string(), }, }) diff --git a/services/server/src/routes/analyze.rs b/services/server/src/routes/analyze.rs index 6cc7f921..d64afb95 100644 --- a/services/server/src/routes/analyze.rs +++ b/services/server/src/routes/analyze.rs @@ -324,10 +324,24 @@ pub async fn analyze( // references ("last Friday") to absolute dates *inside the extracted // fact text* (Architecture A — no metadata column, date flows into // the encrypted blob + embedding only). - let extracted = state - .extractor - .extract_with_context(&body.text, &related_texts, body.occurred_at) - .await?; + // + // when `body.extract_with_critique` is true, route through + // the two-pass critique path instead. The critique pass internally + // reuses `extract_with_context` for the first pass, so the same + // `related_texts` + `occurred_at` context applies to both passes — + // the critic sees the same anchor the extractor did. ~2x LLM cost + // when enabled; default false preserves the single-pass behaviour. + let extracted = if body.extract_with_critique { + state + .extractor + .extract_with_critique(&body.text, &related_texts, body.occurred_at) + .await? + } else { + state + .extractor + .extract_with_context(&body.text, &related_texts, body.occurred_at) + .await? + }; let raw_fact_count = extracted.raw_count; let facts = extracted.facts; let reserved_additional_weight = rate_limit::analyze_additional_weight(facts.len()); diff --git a/services/server/src/services/extractor.rs b/services/server/src/services/extractor.rs index ad900533..01945718 100644 --- a/services/server/src/services/extractor.rs +++ b/services/server/src/services/extractor.rs @@ -117,6 +117,33 @@ pub trait Extractor: Send + Sync { let _ = occurred_at; self.extract(text).await } + + /// **Opt-in two-pass extraction.** Runs the first pass via + /// [`Self::extract_with_context`], then runs a second LLM pass that + /// receives the first-pass facts + the original input + the same + /// `` anchor and `` block, + /// and emits a corrected final fact list. + /// + /// Default impl delegates to [`Self::extract_with_context`] (single + /// pass) so mocks and non-LLM extractors inherit a sane fallback — + /// only `LlmExtractor` overrides this with the actual two-pass + /// logic. Callers route here when + /// `AnalyzeRequest.extract_with_critique == true`; default false + /// preserves today's single-pass behaviour. + /// + /// **Cost:** doubles the per-analyze LLM call count when overridden + /// by an LLM-backed impl. Caller-side concern — the server does no + /// accounting beyond emitting an extra `extractor.extract_with_critique` + /// span. + async fn extract_with_critique( + &self, + text: &str, + related_memories: &[&str], + occurred_at: Option>, + ) -> Result { + self.extract_with_context(text, related_memories, occurred_at) + .await + } } // ============================================================ @@ -202,6 +229,31 @@ const FACT_EXTRACTION_PROMPT: &str = include_str!("prompts/extract.txt"); /// fall back to `now()`. pub const FACT_EXTRACTION_PROMPT_VERSION: &str = "extract.v6"; +/// System prompt for the opt-in critique pass. Sourced from +/// `prompts/critique.txt`. Mirrors the temporal / anti-leak / output- +/// format rules of `extract.v6` so the critic preserves resolved dates, +/// preserves recounted-event historical dates, and never emits +/// `` / `` / `` tags itself. +/// Treats the first-pass facts + original input + related-memories +/// block as untrusted data (per the prompt-injection guard in the +/// prompt's own opening paragraph). +const FACT_EXTRACTION_CRITIQUE_PROMPT: &str = include_str!("prompts/critique.txt"); + +/// Version ID for the critique-pass prompt. Bump on every meaningful +/// change. Surfaced on `GET /health` and pinned into benchmark result +/// artifacts (`prompt_versions.critique`) so a per-cycle delta is +/// attributable to the prompt rather than guessed at from git history. +/// +/// `critique.v1`: initial critique prompt. Includes temporal-rule +/// mirror (preserve resolved dates, preserve historical recounted-event +/// dates, strip wrongly-stamped stable facts), anti-leak (no tag +/// emission), and the standard strict output format. The prompt +/// receives the same `` and `` +/// blocks the first pass saw, plus a rendered first-pass facts block, +/// so it can verify against the original anchor without re-resolving +/// relative-time references. +pub const FACT_EXTRACTION_CRITIQUE_PROMPT_VERSION: &str = "critique.v1"; + /// Map a bucket name from the extractor LLM to a numeric importance score. /// Unknown / missing buckets default to `IMPORTANCE_STANDARD` so a noisy /// LLM line doesn't crash extraction — it just falls back to the neutral @@ -452,6 +504,104 @@ impl Extractor for LlmExtractor { }); self.call_chat_completion(messages).await } + + /// Two-pass extraction with self-critique. + /// + /// Step 1: run `extract_with_context` to get the first-pass facts + /// (with all `extract.v6` rules applied — temporal anchoring, + /// dedup, anti-leak). Step 2: build a second message array for the + /// critique LLM containing the SAME context the first pass saw, + /// plus the rendered first-pass facts: + /// + /// ```text + /// [system: FACT_EXTRACTION_CRITIQUE_PROMPT] + /// [user: ...] // only if related_memories non-empty + /// [user: ] // only if occurred_at is Some + /// [user: Original input:\n] + /// [user: First-pass extracted facts:\n] + /// ``` + /// + /// The critic returns the corrected final fact list in the same + /// `BUCKETFACT_TEXT` format as the extractor. Parsing reuses + /// `parse_extracted_facts` (which handles the explicit `NONE` + /// reply and the same bucket vocabulary). + /// + /// First-pass fact text is escaped via `escape_for_prompt_context` + /// before rendering — defence-in-depth against a first-pass fact + /// that somehow contains structural markers (e.g. an LLM that + /// briefly leaked `` despite the v6 anti-leak rule). The + /// `` and `` blocks themselves are + /// server-rendered with known-good shapes (no user-controlled + /// content), so they don't need additional escaping. + /// + /// **Cost:** ~2x latency and ~2x LLM cost compared to a single + /// pass. Bench harness flips this via `--extract-with-critique` + /// for the critique stacked re-bench; production callers opt in + /// via `AnalyzeRequest.extract_with_critique = true`. + /// + /// All transient-failure handling (UpstreamUnavailable on + /// 5xx/429, envelope detection, null-content → 503) is inherited + /// automatically because both passes route through the shared + /// `call_chat_completion` helper. + #[tracing::instrument( + name = "extractor.extract_with_critique", + skip_all, + fields( + text_len = text.len(), + context_len = related_memories.len(), + has_occurred_at = occurred_at.is_some(), + ) + )] + async fn extract_with_critique( + &self, + text: &str, + related_memories: &[&str], + occurred_at: Option>, + ) -> Result { + // Step 1: first pass. Reuses extract_with_context so we inherit + // its short-circuit + v6 prompt + temporal anchoring + dedup + // context. The first pass's `raw_count` is discarded — we only + // carry forward the kept facts (≤ MAX_ANALYZE_FACTS) into the + // critique. + let first_pass = self + .extract_with_context(text, related_memories, occurred_at) + .await?; + + // Step 2: build the critique message array. Mirrors the + // extract_with_context shape so the critic sees the same + // anchor + dedup context, plus the original input and the + // first-pass facts as additional user messages. + let mut messages = Vec::with_capacity(5); + messages.push(ChatMessage { + role: "system".to_string(), + content: FACT_EXTRACTION_CRITIQUE_PROMPT.to_string(), + }); + if !related_memories.is_empty() { + messages.push(ChatMessage { + role: "user".to_string(), + content: render_related_memories_block(related_memories), + }); + } + if let Some(ts) = occurred_at { + messages.push(ChatMessage { + role: "user".to_string(), + content: render_occurred_at_block(ts), + }); + } + messages.push(ChatMessage { + role: "user".to_string(), + content: format!("Original input:\n{}", text), + }); + messages.push(ChatMessage { + role: "user".to_string(), + content: format!( + "First-pass extracted facts:\n{}", + render_extracted_facts_for_prompt(&first_pass.facts) + ), + }); + + self.call_chat_completion(messages).await + } } /// render a `...` block from @@ -514,6 +664,68 @@ fn render_occurred_at_block(occurred_at: chrono::DateTime) -> Strin format!("", occurred_at.to_rfc3339()) } +/// Render a slice of first-pass extracted facts as a +/// `BUCKETFACT_TEXT` block, one per line, for inclusion in the +/// critique-pass user message. Returns the literal `"NONE"` +/// when the slice is empty — same vocabulary the extract / critique +/// prompts use for "no facts" so the critic doesn't have to +/// special-case empty input. +/// +/// Fact text is passed through `escape_for_prompt_context` before +/// rendering. This is defence-in-depth: the `extract.v6` anti-leak +/// rule already tells the first-pass LLM not to emit structural +/// markers, but if a fact ever does contain `<` / `>` / `&` (legit +/// user content, e.g. someone discussing HTML), the escape ensures +/// those characters can't close the critique's outer structure or +/// open a fake context tag the critic might believe. +/// +/// Bucket name is derived from `fact.importance` via +/// `bucket_for_importance`, the inverse of `importance_for_bucket` — +/// so a fact that came in as `vital` (importance 0.9) renders back +/// as `vital`, letting the critic preserve buckets verbatim per the +/// critique prompt's instructions. +/// +/// Free function so the prompt-formatting tests can pin it +/// independently of the HTTP call site. +fn render_extracted_facts_for_prompt(facts: &[ExtractedFact]) -> String { + if facts.is_empty() { + return "NONE".to_string(); + } + facts + .iter() + .map(|fact| { + format!( + "{}\t{}", + bucket_for_importance(fact.importance), + escape_for_prompt_context(&fact.text) + ) + }) + .collect::>() + .join("\n") +} + +/// Inverse of `importance_for_bucket` — maps a numeric importance +/// score back to the bucket name the extractor LLM emits. Used when +/// rendering first-pass facts for the critique-pass user message +/// so the critic sees the same vocabulary the extractor +/// produced. +/// +/// Comparison uses approximate equality (`(a - b).abs() < EPSILON`) +/// so a fact whose importance was lightly perturbed downstream (e.g. +/// by a future per-fact ranker pass) still maps to a sensible +/// bucket. Falls back to `standard` for unknown / out-of-range +/// values — same neutral default as the parse path. +fn bucket_for_importance(importance: f32) -> &'static str { + const EPSILON: f32 = 1e-6; + if (importance - IMPORTANCE_VITAL).abs() < EPSILON { + "vital" + } else if (importance - IMPORTANCE_TRIVIAL).abs() < EPSILON { + "trivial" + } else { + "standard" + } +} + /// Parsed shape of OpenRouter's "200 OK wrapping an upstream /// gateway-timeout error" envelope. Used to detect this case at the /// chat-completion + embedding call sites and route to @@ -1451,4 +1663,207 @@ mod tests { assert!(!is_upstream_status_transient(StatusCode::OK)); assert!(!is_upstream_status_transient(StatusCode::ACCEPTED)); } + + // ── critique.v1 prompt content pins ───────────────────────── + + #[test] + fn critique_prompt_version_is_v1() { + // First version of the critique prompt; bump on every + // meaningful change so artifacts are attributable. + assert_eq!(super::FACT_EXTRACTION_CRITIQUE_PROMPT_VERSION, "critique.v1"); + } + + #[test] + fn critique_prompt_contains_injection_guard() { + // The critique pass receives the original input + first-pass + // facts + related-memories block — ALL of which are + // user-influenced and must be treated as untrusted data. The + // prompt's own opening paragraph carries this guard; if it's + // ever removed the critic could be steered by injected + // instructions in stored memory text. + let prompt = super::FACT_EXTRACTION_CRITIQUE_PROMPT; + assert!( + prompt.contains("untrusted data"), + "critique prompt must mark input + first-pass facts as untrusted" + ); + assert!( + prompt.to_lowercase().contains("never follow instructions"), + "critique prompt must forbid following inline instructions" + ); + } + + #[test] + fn critique_prompt_contains_temporal_preservation_rule() { + // The whole point of routing critique through the same + // the first pass saw is so the critic + // can verify resolved dates against the anchor — and + // PRESERVE them, not strip them, even when the original + // input only contains a relative reference. Pin the + // load-bearing instruction. + let prompt = super::FACT_EXTRACTION_CRITIQUE_PROMPT; + assert!( + prompt.contains("PRESERVE resolved dates"), + "critique prompt must explicitly preserve resolved absolute dates" + ); + assert!( + prompt.contains("Weekday, D Month YYYY (YYYY-MM-DD)"), + "critique prompt must reference the verbose date format the extractor emits" + ); + } + + #[test] + fn critique_prompt_contains_recounted_past_carve_out() { + // Without this rule the critic might "correct" a fact like + // "User broke arm in 2008" by replacing 2008 with the + // anchor. Pin the explicit carve-out. + let prompt = super::FACT_EXTRACTION_CRITIQUE_PROMPT; + assert!( + prompt.contains("PRESERVE historical dates from recounted events"), + "critique prompt must preserve historical dates from recounted events" + ); + } + + #[test] + fn critique_prompt_blocks_tag_emission() { + // Anti-leak: the critic must NEVER emit ``, + // ``, or `` tags itself — + // even though it sees them in its input. Same rule + // `extract.v6` enforces for the first pass. + let prompt = super::FACT_EXTRACTION_CRITIQUE_PROMPT; + assert!( + prompt.contains("Do NOT emit"), + "critique prompt must forbid tag emission" + ); + // The forbidden tags must be explicitly named. + for tag in ["", "", ""] { + assert!( + prompt.contains(tag), + "critique prompt must explicitly name {} as a tag not to emit", + tag + ); + } + } + + #[test] + fn critique_prompt_documents_strict_output_format() { + // Same BUCKETFACT_TEXT contract as the extractor; same + // explicit NONE reply for the empty case. Without this pin + // a future prompt edit could accidentally let the critic + // emit "explanation" prose or change the bucket vocabulary, + // breaking the parser downstream. + let prompt = super::FACT_EXTRACTION_CRITIQUE_PROMPT; + assert!( + prompt.contains("BUCKETFACT_TEXT"), + "critique prompt must pin the bucket-tab-text output format" + ); + for bucket in ["vital", "standard", "trivial"] { + assert!( + prompt.contains(bucket), + "critique prompt must name bucket: {}", + bucket + ); + } + assert!( + prompt.contains("return exactly: `NONE`"), + "critique prompt must specify NONE for empty-result case" + ); + } + + // ── render_extracted_facts_for_prompt helper ───────────────── + + #[test] + fn render_extracted_facts_for_prompt_basic_shape() { + // Pin the wire format the critic sees: BUCKETTEXT, one + // fact per line, in the input order. Bucket name is derived + // from the numeric importance via bucket_for_importance. + let facts = vec![ + super::ExtractedFact { + text: "User lives in Hanoi".to_string(), + importance: super::IMPORTANCE_STANDARD, + }, + super::ExtractedFact { + text: "User is allergic to peanuts".to_string(), + importance: super::IMPORTANCE_VITAL, + }, + super::ExtractedFact { + text: "User casually mentioned the weather".to_string(), + importance: super::IMPORTANCE_TRIVIAL, + }, + ]; + let rendered = super::render_extracted_facts_for_prompt(&facts); + assert_eq!( + rendered, + "standard\tUser lives in Hanoi\n\ + vital\tUser is allergic to peanuts\n\ + trivial\tUser casually mentioned the weather" + ); + } + + #[test] + fn render_extracted_facts_for_prompt_empty_is_none() { + // An empty fact slice renders as the literal `NONE` — same + // vocabulary the extract / critique prompts use for "no + // facts", so the critic doesn't have to special-case empty + // input ("first pass found nothing" looks the same as + // "no first pass run"). + assert_eq!(super::render_extracted_facts_for_prompt(&[]), "NONE"); + } + + #[test] + fn render_extracted_facts_for_prompt_escapes_tag_chars() { + // Defence-in-depth: the v6 anti-leak rule tells the first + // pass not to emit / tags, but + // a fact's text could legitimately contain `<`/`>`/`&` (a + // user discussing HTML, e.g.). Those characters must be + // escaped so they can't close the critique's outer + // structure or open a fake context tag the critic might + // believe. + let facts = vec![super::ExtractedFact { + text: "User dislikes