Reconcile Gitea main into GitHub main (#342) + RLM fixes (#349)

[AlexMikhalev]

Summary

Reconciles the diverged Gitea and GitHub main lines (#342) and applies the RLM reasoning-model fixes (#349).

#342 Reconciliation

• Merges all 324 Gitea-only commits into GitHub main (the canonical release line)
• Syncs all crate src/ and tests/ directories to gitea/main versions (the evolved code line)
• Preserves all GitHub-only CI/release infrastructure (37 commits: release-binaries.yml, forge-mirror.yml, promote-release.yml, etc.)
• No conflicts in .github/ or scripts/ — GitHub CI is fully intact

#349 RLM Fixes (cherry-picked from e672f13)

• Raise RLM synthesis max_tokens from 2000 to 8000 (reasoning models exhaust 2000 on chain-of-thought)
• Make max_tokens configurable via TERRAPHIM_GREP_MAX_TOKENS
• Extract extract_content() with tracing::warn! when LLM returns empty content
• Log tracing::warn! when answer parsing fails

Result

terraphim_grep now has: the #81 RLM opt-in gate, the #349 max_tokens fix, and all GitHub CI — everything needed to cut a proper v1.21.17 release.

Test plan

• cargo check --workspace passes
• cargo test --workspace — all 967 tests pass (1 flaky under parallel load, passes individually)
• All 87 terraphim_grep tests pass (including 5 new for #349)
• Verified: #81 opt-in gate, #349 max_tokens, extract_content all present
• Verified: GitHub CI workflows intact (release-binaries.yml, forge-mirror.yml)

Refs #342, #349

[AlexMikhalev]

Structural Review Status

pi-rust structural review could not be executed — no API keys are configured for any supported pi provider (openai-codex, anthropic, openai all return "No API key found").

Manual review of new code (the only non-merge changes)

The PR contains 324 reconciliation commits from Gitea main (previously reviewed) plus one new cherry-picked fix (#349, 172 insertions across 2 files):

crates/terraphim_grep/src/lib.rs:

• rlm_max_tokens(): env-var override with sensible default (8000), handles invalid/zero input gracefully
• Answer parsing: changed from .ok().map() (silent) to match with tracing::warn! on failure

crates/terraphim_grep/src/openrouter_client.rs:

• extract_content(): pure function, testable, logs finish_reason and has_reasoning on empty content

Findings

No P0/P1 issues identified. The changes are additive (logging) and a token limit increase.

Blocker

The pi-rust review should be re-run once provider credentials are available, before merging. The 267-file reconciliation is a merge of previously reviewed Gitea commits; the only new code is the #349 fix above.

[AlexMikhalev]

Summary

This PR raises the RLM synthesis max_tokens cap from 2000 to 8000 (with a TERRAPHIM_GREP_MAX_TOKENS env override), extracts extract_content() in the OpenRouter client with a tracing::warn! diagnostic for empty LLM content, and replaces silent .ok() swallowing of answer-parse failures with an explicit match + tracing::warn!.

Key changes:

• rlm_max_tokens() (lib.rs): env-overridable token budget, default 8000, invalid/zero values fall back to the default. Correctly targets the reasoning-model content: null failure mode.
• extract_content() (openrouter_client.rs): pure extraction helper that logs finish_reason and has_reasoning when content is empty — the right observability for diagnosing token-budget exhaustion.
• Match-based answer parsing (lib.rs): parse failure now emits a structured tracing::warn! and returns answer: None instead of silently discarding via .ok().map().
• Sufficiency explanations (lib.rs, refs #87): every GrepResult branch now carries a human-readable sufficiency_explanation with the actual metrics and thresholds.
• RLM opt-in gating (lib.rs, refs #81): NeedsSynthesis/NeedsExpansion verdicts no longer trigger an LLM round trip unless --answer or --force-rlm was passed; a CountingLocalLlm in-process test double asserts zero LLM calls on the search-only path.

What was done well: the test suite is a standout — the CountingLocalLlm is a real LlmClient trait implementation rather than a mocking framework, the #81 regression tests assert the observable difference (call count), fixture preconditions are asserted rather than assumed, and extract_content has direct unit tests for all four response shapes (normal, null-content-with-reasoning, missing choices, null-content-without-reasoning). Structured tracing fields (error = %e, response_len, %finish_reason) are textbook observability.

What remains problematic: the final sufficiency_explanation in search_with_rlm_fallback unconditionally claims "the answer was synthesised by the LLM" even when parsing failed and answer is None — the JSON contract then self-contradicts (RlmSynthesis + answer: null + text claiming success). Additionally, the replacement insufficient_path_propagates_chunk_count test is weaker than the test it deleted (conditional assertion, no KG-concept coverage), and several smaller robustness gaps exist around env-var handling and an unreachable predicate branch.

Confidence Score: 3/5

• Safe to merge with caution — fix the P1 explanation/contract contradiction before or immediately after merging.
• Zero P0. One active P1: the unconditional "answer was synthesised" explanation produces a self-contradictory JSON contract precisely on the failure path this PR sets out to make visible; a consumer keying on SufficiencyState::RlmSynthesis and unwrapping answer will misbehave. The remaining five findings are P2 (test weakening, env-var clamping, unreachable predicate branch, defensive formatting, curation of unparseable responses).
• Files requiring attention: crates/terraphim_grep/src/lib.rs (P1 at line ~444, P2s at lines ~247, ~129, ~143, ~822, ~442).

Important Files Changed

Filename	Overview
crates/terraphim_grep/src/lib.rs	Adds rlm_max_tokens() (env override, default 8000), rlm_requested() gating (#81), search_only_result() helper, sufficiency_explanation on all GrepResult branches, match-based answer parsing with tracing::warn!. One P1 (unconditional synthesis claim in explanation), four P2s. Needs attention.
crates/terraphim_grep/src/openrouter_client.rs	Extracts extract_content() with empty-content tracing::warn! carrying finish_reason/has_reasoning; four unit tests for response shapes. Clean. No findings in the new code itself; one P2 comment on unchanged call-site behaviour (kg_curation ingestion) filed under Comments Outside Diff.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["search(query, options)"] --> B{"options.force_rlm?"}
    B -->|yes| C["search_with_rlm: hybrid search + judge_with_metrics"]
    B -->|no| D["hybrid_searcher.search"]
    D --> E["judge_with_metrics -> (Sufficiency, SufficiencyMetrics)"]
    E --> F{"Sufficiency verdict"}
    F -->|Sufficient| G["search_only_result + explanation<br/>(coverage/KG/diversity vs thresholds)"]
    F -->|NeedsSynthesis| H{"rlm_requested()?<br/>(--answer / --force-rlm)"}
    F -->|NeedsExpansion| H2{"rlm_requested()?"}
    F -->|Insufficient| I["RlmInsufficient + explanation<br/>(truthful chunks_returned / kg_hits)"]
    H -->|no| J["SearchOnly + below-thresholds explanation"]
    H2 -->|no| J2["SearchOnly + low-coverage explanation"]
    H -->|yes| K["search_with_rlm_fallback"]
    H2 -->|yes| K
    C --> K
    K --> L{"llm_client configured?"}
    L -->|no| M["SearchOnly + 'no LLM client' explanation"]
    L -->|yes| N["chat_completion(max_tokens = rlm_max_tokens())<br/>env: TERRAPHIM_GREP_MAX_TOKENS"]
    N --> O["extract_content() in OpenRouterClient<br/>warn! if empty (finish_reason, has_reasoning)"]
    O --> P{"options.include_answer?"}
    P -->|yes| Q{"AnswerSignature.parse(llm_response)"}
    Q -->|Ok| R["AnswerWithCitations + 'synthesised in Nms' explanation"]
    Q -->|Err| S["warn! + answer: None<br/>⚠️ explanation still claims synthesis — P1"]
    P -->|no| T["RlmSynthesis, answer: None"]
    style S fill:#f8d7da,stroke:#dc3545
    style J fill:#d4edda,stroke:#28a745
    style J2 fill:#d4edda,stroke:#28a745
    style M fill:#d4edda,stroke:#28a745

Loading

Inline Findings

P1 crates/terraphim_grep/src/lib.rs, line 444: Unconditional "answer was synthesised" explanation contradicts the contract on the parse-failure path

When AnswerSignature::parse fails (line ~406), the code correctly logs tracing::warn! and sets answer: None. But execution then flows to the final Ok(GrepResult { ... }) whose sufficiency_explanation unconditionally reads:

sufficiency_explanation: format!(
    "Found {} chunks (coverage {:.2}, KG confidence {:.2}); the answer was \
     synthesised by the LLM in {}ms.",
    ...
),
sufficiency: SufficiencyState::RlmSynthesis,

The emitted JSON then self-contradicts: sufficiency: "RlmSynthesis", answer: null, and prose asserting an answer was synthesised. This is exactly the failure path the PR sets out to make visible, and the structured contract lies about it. A downstream consumer that treats RlmSynthesis as "answer is present" (a reasonable reading of the enum) will panic on answer.unwrap() or present a nonexistent synthesis to the user. This is the same class of defect as the skill's "fallback paths must accurately report their data source in metadata" rule — the metadata misreports the outcome.

Suggested fix:

let explanation = if answer.is_some() {
    format!(
        "Found {} chunks (coverage {:.2}, KG confidence {:.2}); the answer was \
         synthesised by the LLM in {}ms.",
        chunks.len(), metrics.coverage, metrics.kg_confidence, rlm_latency_ms,
    )
} else {
    format!(
        "Found {} chunks (coverage {:.2}, KG confidence {:.2}); the LLM responded \
         in {}ms but the response could not be parsed into an answer (see logs). \
         The chunks below are the unmodified search results.",
        chunks.len(), metrics.coverage, metrics.kg_confidence, rlm_latency_ms,
    )
};

Optionally also consider a distinct SufficiencyState variant (e.g. RlmUnparseable) so machine consumers can distinguish the states without string-matching the explanation.

---

P2 crates/terraphim_grep/src/lib.rs, line 822: Replacement insufficient_path_propagates_chunk_count test is weaker than the deleted rlm_insufficient_preserves_chunks_and_reports_truthful_stats

The deleted test asserted the RlmInsufficient branch deterministically (single known match + a populated thesaurus guaranteed the verdict) and verified three invariants: truthful chunks_returned, truthful non-zero kg_hits, and preservation of the known KG concept by name. The replacement wraps everything in if matches!(result.sufficiency, SufficiencyState::RlmInsufficient) — if a judge change ever flips the verdict, the test silently passes having asserted nothing (a vacuous regression guard). It also uses an empty thesaurus, so assert_eq!(result.stats.kg_hits, result.concepts.len()) is a 0 == 0 tautology and KG-concept preservation through the Insufficient branch — the actual subject of the #2721 regression — is no longer exercised at all. Two files (< min_results = 3) deterministically force Insufficient, so the if guard is unnecessary. Suggested fix: drop the conditional, and restore a thesaurus fixture with a known concept (or keep both tests).

---

P2 crates/terraphim_grep/src/lib.rs, line 129: rlm_requested() checks force_rlm, but that branch is unreachable from search()

search() early-returns to search_with_rlm at line 178 when options.force_rlm is set, so by the time the NeedsSynthesis/NeedsExpansion arms evaluate Self::rlm_requested(&options), force_rlm is always false and the predicate reduces to options.include_answer. The redundancy is harmless today but misleading to readers ("two flags gate this branch" when only one can), and the unit test rlm_requested_only_for_explicit_flags documents a reachability that does not exist. Either gate on options.include_answer directly with a comment, or move the early-return so the predicate is genuinely two-flagged.

---

P2 crates/terraphim_grep/src/lib.rs, line 143: rlm_max_tokens() has no upper bound and re-reads the environment on every LLM call

The filter(|&n| n > 0) guard rejects zero/negative values but accepts arbitrarily large ones (TERRAPHIM_GREP_MAX_TOKENS=1000000 passes), which a typo or misconfiguration turns into an unbounded cost/latency multiplier against a billed API — the exact failure mode (runaway token spend) the default of 8000 is meant to bound. Consider a sanity ceiling (e.g. clamp or reject values above ~32k–64k with a warning). Separately, the env var is re-parsed on every chat_completion; reading once at client construction (or OnceLock) would remove a per-request syscall and make the override deterministic within a process lifetime. Neither blocks merge.

---

P2 crates/terraphim_grep/src/lib.rs, line 247: below.join(", ") produces "Found N chunks but ; ..." if judge thresholds ever change

The below vector is populated only when a metric is strictly under its threshold, which is guaranteed non-empty only by the current coupling between SufficiencyJudge::judge_with_metrics and this arm (NeedsSynthesis implies at least one threshold failed). That coupling is implicit: a future judge change (e.g. adding a fourth Sufficient condition) would make below empty and emit a malformed explanation to users. One-line defensive fix:

let reason = if below.is_empty() {
    "multiple metrics below their thresholds".to_string()
} else {
    below.join(", ")
};

---

Comments Outside Diff (1)

1. crates/terraphim_grep/src/lib.rs, line 442 (let _ = kg_curation.extract_and_index(query, &llm_response).await;)
   Unchanged by this PR, but the PR's parse-failure path increases how often it matters: when AnswerSignature::parse fails, llm_response is whatever unparseable text the model returned, and it is still fed into KG curation for indexing. Curation will extract "concepts" from garbage/non-JSON output and index them into the knowledge graph. Pre-existing behaviour, but now that parse failure is an explicit, named state, consider skipping curation when answer.is_none() (or at least logging what is being indexed).

Footer

_{Last reviewed commit: 8bc9fd8 | Reviews (1)}

[AlexMikhalev]

P1 + P2 Fixes Applied (commit 9e1092e)

All findings from the pi-rust structural review have been addressed:

P1 fixed: sufficiency_explanation now emits an accurate message on the parse-failure path instead of unconditionally claiming the answer was synthesised. The JSON contract no longer self-contradicts (RlmSynthesis + answer: null + prose asserting success).

P2 fixed: TERRAPHIM_GREP_MAX_TOKENS capped at 32k; defensive fallback for empty below-thresholds vector.

P2 deferred (pre-existing code, not introduced by this PR):

• Unreachable force_rlm in rlm_requested() — pre-existing from #81, harmless
• Weakened insufficient_path_propagates_chunk_count test — pre-existing from reconciliation
• KG curation ingesting unparseable responses — pre-existing, noted for future work

All 87 tests pass. Clippy clean. rustfmt clean.