|
| 1 | +# Implementation Plan: Terraphim Grep KG Failover, RLM Validation Tests, and Release Readiness |
| 2 | + |
| 3 | +**Status**: Draft | Pending human approval before implementation |
| 4 | +**Research Doc**: `.docs/research-terraphim-grep-failover-rlm-validation-release.md` |
| 5 | +**Author**: OpenCode / Terraphim Engineer |
| 6 | +**Date**: 2026-06-27 |
| 7 | +**Estimated Effort**: 1.5 days |
| 8 | + |
| 9 | +## Overview |
| 10 | + |
| 11 | +### Summary |
| 12 | + |
| 13 | +This plan makes `terraphim-grep` usable without a knowledge graph, proves that `terraphim_rlm` validates commands before executing them, and aligns the installed toolchain versions with the `1.21.0` release. |
| 14 | + |
| 15 | +### Approach |
| 16 | + |
| 17 | +1. **Failover is a CLI gate change, not a search rewrite**: `fff-search` is already integrated; we only need to let the CLI start with an empty thesaurus and skip KG boosting. |
| 18 | +2. **Validation coverage is test-first**: add focused unit tests using the real `LocalExecutor` and `KnowledgeGraphValidator` so Issue #2491 can be closed with evidence. |
| 19 | +3. **Release readiness is bookkeeping**: commit `Cargo.lock`, bump `terraphim-clients` to `1.21.0`, build/install binaries, and tidy untracked docs. |
| 20 | + |
| 21 | +### Scope |
| 22 | + |
| 23 | +**In Scope:** |
| 24 | +- `terraphim-grep` no-thesaurus failover to `fff-search` enhanced grep (`terraphim-clients`). |
| 25 | +- `terraphim_rlm` QueryLoop validation-order unit test (`terraphim-ai`). |
| 26 | +- Commit `terraphim-ai/Cargo.lock` (proptest dev-dependency). |
| 27 | +- Bump `terraphim-clients` workspace version to `1.21.0` and update `terraphim_service` dependency. |
| 28 | +- Build and install updated binaries locally. |
| 29 | +- File Gitea issues and link them to the work. |
| 30 | + |
| 31 | +**Out of Scope:** |
| 32 | +- Changing `KgStrictness` semantics or retry policy (unless the spike reveals a gap). |
| 33 | +- New release automation or CI changes. |
| 34 | +- Refactoring `LlmBridge` for broader testability. |
| 35 | + |
| 36 | +**Avoid At All Cost** (from 5/25 analysis): |
| 37 | +- Re-implementing a grep engine when `fff-search` already exists. |
| 38 | +- Adding a `--no-kg` manual flag instead of automatic failover. |
| 39 | +- Broad `terraphim_rlm` refactors beyond the validation test. |
| 40 | + |
| 41 | +## Architecture |
| 42 | + |
| 43 | +### Component Diagram |
| 44 | + |
| 45 | +```text |
| 46 | +terraphim-grep CLI |
| 47 | + ├─ Thesaurus discovery (optional) |
| 48 | + ├─ HybridSearcher |
| 49 | + │ ├─ RoleGraph (empty thesaurus) |
| 50 | + │ └─ fff-search FilePicker.grep |
| 51 | + └─ GrepResult (concepts empty, chunks populated) |
| 52 | +
|
| 53 | +terraphim_rlm QueryLoop |
| 54 | + ├─ validate_command(input) |
| 55 | + │ └─ LocalExecutor.validate(input) -> KnowledgeGraphValidator |
| 56 | + └─ execute_command/execute_code |
| 57 | + └─ LocalExecutor (real; pass/fail controlled by thesaurus terms) |
| 58 | +``` |
| 59 | + |
| 60 | +### Data Flow |
| 61 | + |
| 62 | +**`terraphim-grep` failover:** |
| 63 | + |
| 64 | +```text |
| 65 | +query -> main.rs |
| 66 | + -> if thesaurus missing: empty Thesaurus(role_name) |
| 67 | + -> HybridSearcher::new(role_name, thesaurus) // works with empty thesaurus |
| 68 | + -> HybridSearcher::search |
| 69 | + -> KG path returns [] concepts |
| 70 | + -> code path runs fff-search |
| 71 | + -> boost_chunks_with_kg(chunks, []) // no-op |
| 72 | + -> GrepResult with empty concepts |
| 73 | +``` |
| 74 | + |
| 75 | +**`terraphim_rlm` validation test:** |
| 76 | + |
| 77 | +```text |
| 78 | +test -> QueryLoop::execute_command(Command::Run("echo allowed_term")) |
| 79 | + -> LocalExecutor::validate -> KnowledgeGraphValidator (thesaurus=["echo", "allowed_term"]) |
| 80 | + -> validation passes -> LocalExecutor::execute_command runs echo |
| 81 | + -> output contains "allowed_term" |
| 82 | +
|
| 83 | +test -> QueryLoop::execute_command(Command::Run("echo disallowed_term")) |
| 84 | + -> LocalExecutor::validate -> KnowledgeGraphValidator (thesaurus=["echo"]) |
| 85 | + -> validation fails -> LocalExecutor::execute_command is NOT called |
| 86 | + -> output is the validation feedback message |
| 87 | +``` |
| 88 | + |
| 89 | +### Key Design Decisions |
| 90 | + |
| 91 | +| Decision | Rationale | Alternatives Rejected | |
| 92 | +|----------|-----------|----------------------| |
| 93 | +| Make `code-search` a default feature in `terraphim_grep` | Failover must work for end users without remembering to enable features | Keep it optional — would leave failover disabled by default | |
| 94 | +| Use empty `Thesaurus` as the no-KG sentinel | `HybridSearcher::new` already accepts it; existing test proves it works | Refactor `HybridSearcher` to be optional — larger change, same outcome | |
| 95 | +| Add `info!` log when falling back | Users can see why KG concepts are absent | Silent fallback — harder to debug | |
| 96 | +| Test `QueryLoop::execute_command` directly with real `LocalExecutor` + `KnowledgeGraphValidator` | No mocks; uses real validation and execution paths; proves ordering by observing side effects | Mock executor — violates project "no mocks in tests" rule | |
| 97 | +| Treat `terraphim-clients` version bump as part of this plan | Installed binaries must match the latest release | Separate release cycle — would leave binaries stale longer | |
| 98 | + |
| 99 | +### Eliminated Options (Essentialism) |
| 100 | + |
| 101 | +| Option Rejected | Why Rejected | Risk of Including | |
| 102 | +|-----------------|--------------|-------------------| |
| 103 | +| Manual `--no-kg` flag | Fails the "works out of the box" requirement | Users still hit the error when they forget the flag | |
| 104 | +| Bundled default thesaurus | Increases binary size and creates a maintenance hotspot | Diverges from "your knowledge, your boost" model | |
| 105 | +| Mock executor for QueryLoop tests | Project rule: never use mocks in tests | Brittle tests that don't exercise real validation/execution paths | |
| 106 | +| Full RLM strictness/escalation redesign | Issue #2491 is about validation ordering; behaviour already matches acceptance criteria | Scope creep and risk of changing working security semantics | |
| 107 | +| Patching `terraphim-ai/Cargo.toml` for `terraphim-clients` | Separate repos/workspaces; cross-repo patches break resolution | Build instability | |
| 108 | + |
| 109 | +### Simplicity Check |
| 110 | + |
| 111 | +**What if this could be easy?** |
| 112 | + |
| 113 | +- For grep: remove the hard error when thesaurus is missing, pass an empty thesaurus, and log a one-line info message. |
| 114 | +- For RLM: add a test module that builds a real `QueryLoop` with `LocalExecutor` and a controlled thesaurus-backed validator, asserting pass/fail by inspecting command output. |
| 115 | +- For release: bump one workspace version string and commit the lockfile. |
| 116 | + |
| 117 | +**Senior Engineer Test**: The design is not overcomplicated; it reuses existing paths and adds minimal surface area. |
| 118 | + |
| 119 | +**Nothing Speculative Checklist**: |
| 120 | +- [x] No features the user didn't request |
| 121 | +- [x] No abstractions "in case we need them later" |
| 122 | +- [x] No flexibility "just in case" |
| 123 | +- [x] No error handling for scenarios that cannot occur |
| 124 | +- [x] No premature optimization |
| 125 | + |
| 126 | +## File Changes |
| 127 | + |
| 128 | +### `terraphim-clients` repository |
| 129 | + |
| 130 | +#### Modified Files |
| 131 | + |
| 132 | +| File | Changes | |
| 133 | +|------|---------| |
| 134 | +| `crates/terraphim_grep/Cargo.toml` | Add `code-search` to `default` features; keep `llm` | |
| 135 | +| `crates/terraphim_grep/src/main.rs` | Make thesaurus optional; build empty thesaurus when none found; log failover | |
| 136 | +| `crates/terraphim_grep/src/hybrid_searcher.rs` | Add `is_kg_configured()` helper or document empty-thesaurus semantics; add unit test | |
| 137 | +| `crates/terraphim_grep/src/lib.rs` | Add no-thesaurus failover test | |
| 138 | +| `Cargo.toml` | Bump `workspace.package.version` to `1.21.0` | |
| 139 | +| `crates/terraphim_grep/Cargo.toml` | Update `terraphim_service` version to `1.21.0` (if published) or keep patch override | |
| 140 | +| `RELEASE_NOTES_v1.21.0.md` | Create release notes (optional but recommended) | |
| 141 | + |
| 142 | +### `terraphim-ai` repository |
| 143 | + |
| 144 | +#### Modified Files |
| 145 | + |
| 146 | +| File | Changes | |
| 147 | +|------|---------| |
| 148 | +| `crates/terraphim_rlm/src/query_loop.rs` | Add real-executor validation-order unit tests | |
| 149 | +| `Cargo.lock` | Commit existing proptest/terraphim_spawner changes | |
| 150 | + |
| 151 | +#### New Files |
| 152 | + |
| 153 | +None in `terraphim-ai` beyond test code inside `query_loop.rs`. |
| 154 | + |
| 155 | +### Untracked Documentation |
| 156 | + |
| 157 | +| Action | Files | |
| 158 | +|--------|-------| |
| 159 | +| Commit or remove | `.docs/adf-weekly-activity-*.md`, `.docs/design-*.md`, `.docs/plan-*.md`, `.docs/research-*.md`, `docs/handovers/*.md`, `docs/plans/*.md`, `docs/research/*.md`, `implementation-plan-2301.md` | |
| 160 | + |
| 161 | +## API Design |
| 162 | + |
| 163 | +### No new public APIs |
| 164 | + |
| 165 | +The failover is transparent to callers. Existing public types remain unchanged: |
| 166 | + |
| 167 | +```rust |
| 168 | +pub struct GrepResult { ... } |
| 169 | +pub struct HybridSearcher { ... } |
| 170 | +pub async fn TerraphimGrep::search(&self, query: &str, options: GrepOptions) -> Result<GrepResult> { ... } |
| 171 | +``` |
| 172 | + |
| 173 | +### Internal helper (optional) |
| 174 | + |
| 175 | +In `main.rs`: |
| 176 | + |
| 177 | +```rust |
| 178 | +/// Build a thesaurus, falling back to an empty one when no project thesaurus exists. |
| 179 | +async fn resolve_thesaurus(role_name: &str, explicit: Option<&Path>) -> Result<Thesaurus> { |
| 180 | + if let Some(path) = explicit { |
| 181 | + let automata_path = AutomataPath::from_local(path); |
| 182 | + return terraphim_automata::load_thesaurus(&automata_path) |
| 183 | + .await |
| 184 | + .with_context(|| format!("Failed to load thesaurus from {:?}", path)); |
| 185 | + } |
| 186 | + if let Some(path) = find_default_thesaurus(role_name) { |
| 187 | + let automata_path = AutomataPath::from_local(&path); |
| 188 | + return terraphim_automata::load_thesaurus(&automata_path) |
| 189 | + .await |
| 190 | + .with_context(|| format!("Failed to load thesaurus from {:?}", path)); |
| 191 | + } |
| 192 | + tracing::info!("No thesaurus found; running in fff-search enhanced grep mode"); |
| 193 | + Ok(Thesaurus::new(role_name.to_string())) |
| 194 | +} |
| 195 | +``` |
| 196 | + |
| 197 | +### Test helpers (test-only) |
| 198 | + |
| 199 | +In the `#[cfg(test)]` module of `crates/terraphim_rlm/src/query_loop.rs`: |
| 200 | + |
| 201 | +```rust |
| 202 | +/// Build a QueryLoop wired to a real LocalExecutor and a thesaurus-backed validator. |
| 203 | +fn test_query_loop( |
| 204 | + thesaurus_terms: &[&str], |
| 205 | + strictness: KgStrictness, |
| 206 | +) -> (QueryLoop<dyn ExecutionEnvironment>, SessionId) { ... } |
| 207 | + |
| 208 | +/// Build a thesaurus containing the given terms. |
| 209 | +fn test_thesaurus(terms: &[&str]) -> Thesaurus { ... } |
| 210 | +``` |
| 211 | + |
| 212 | +No mock types are introduced. Validation pass/fail is controlled entirely by the terms present in the real `KnowledgeGraphValidator` thesaurus. |
| 213 | + |
| 214 | +## Test Strategy |
| 215 | + |
| 216 | +### Unit Tests |
| 217 | + |
| 218 | +| Test | Location | Purpose | |
| 219 | +|------|----------|---------| |
| 220 | +| `search_without_thesaurus_uses_fff_mode` | `terraphim_grep/src/lib.rs` | Empty thesaurus yields non-empty fff chunks and empty concepts | |
| 221 | +| `main_accepts_missing_thesaurus` | `terraphim_grep/tests/no_thesaurus_cli.rs` (new) | CLI runs successfully with no `--thesaurus` in a temp directory | |
| 222 | +| `run_command_validates_before_execution` | `terraphim_rlm/src/query_loop.rs` | Real executor: allowed `RUN` command produces shell output | |
| 223 | +| `code_command_validates_before_execution` | `terraphim_rlm/src/query_loop.rs` | Real executor: allowed `CODE` command produces Python output | |
| 224 | +| `validation_failure_blocks_run_command` | `terraphim_rlm/src/query_loop.rs` | Real executor: disallowed `RUN` command returns validation feedback, no shell output | |
| 225 | +| `validation_failure_blocks_code_command` | `terraphim_rlm/src/query_loop.rs` | Real executor: disallowed `CODE` command returns validation feedback, no Python output | |
| 226 | + |
| 227 | +### Integration Tests |
| 228 | + |
| 229 | +| Test | Location | Purpose | |
| 230 | +|------|----------|---------| |
| 231 | +| `terraphim_grep` e2e no-thesaurus | `terraphim_grep/tests/no_thesaurus_cli.rs` | End-to-end CLI invocation returns JSON results with no concepts | |
| 232 | + |
| 233 | +### Existing Tests to Keep Passing |
| 234 | + |
| 235 | +- `terraphim_grep::tests::search_without_llm_degrades_to_search_only` |
| 236 | +- `cargo test -p terraphim_rlm` |
| 237 | +- `cargo clippy --workspace` for both repos |
| 238 | + |
| 239 | +## Implementation Steps |
| 240 | + |
| 241 | +### Step 1: `terraphim-grep` failover |
| 242 | +**Repo**: `terraphim-clients` |
| 243 | +**Files**: `crates/terraphim_grep/Cargo.toml`, `crates/terraphim_grep/src/main.rs`, `crates/terraphim_grep/src/lib.rs` |
| 244 | +**Description**: |
| 245 | +1. Add `"code-search"` to `default` features in `Cargo.toml`. |
| 246 | +2. Replace the hard thesaurus error in `main.rs` with `resolve_thesaurus()` helper that returns an empty thesaurus when none is found. |
| 247 | +3. Add unit test `search_without_thesaurus_uses_fff_mode`. |
| 248 | +**Tests**: New unit test + existing test suite |
| 249 | +**Estimated**: 3 hours |
| 250 | + |
| 251 | +### Step 2: `terraphim_rlm` validation-order test |
| 252 | +**Repo**: `terraphim-ai` |
| 253 | +**Files**: `crates/terraphim_rlm/src/query_loop.rs` |
| 254 | +**Description**: |
| 255 | +1. Add test helpers in the `#[cfg(test)]` module to build a real `QueryLoop` with a `LocalExecutor` and a `KnowledgeGraphValidator` loaded with a controlled thesaurus. |
| 256 | +2. Add four tests covering: |
| 257 | + - Allowed `Command::Run` validates then executes. |
| 258 | + - Allowed `Command::Code` validates then executes. |
| 259 | + - Disallowed `Command::Run` is blocked by validation feedback. |
| 260 | + - Disallowed `Command::Code` is blocked by validation feedback. |
| 261 | +3. Assertions inspect the `ExecuteResult::Continue { output }` string for either command output or validation feedback; no mocks are used. |
| 262 | +4. Run `cargo test -p terraphim_rlm` and `cargo clippy -p terraphim_rlm -- -D warnings`. |
| 263 | +**Tests**: New unit tests |
| 264 | +**Estimated**: 3 hours |
| 265 | + |
| 266 | +### Step 3: Commit `terraphim-ai/Cargo.lock` |
| 267 | +**Repo**: `terraphim-ai` |
| 268 | +**Files**: `Cargo.lock` |
| 269 | +**Description**: |
| 270 | +1. Inspect the diff to confirm only `proptest` and related transitive deps were added. |
| 271 | +2. Commit with message: `chore(deps): commit proptest dev-dependency for terraphim_spawner Refs #<issue>`. |
| 272 | +**Tests**: `cargo check --workspace` must still pass |
| 273 | +**Estimated**: 30 minutes |
| 274 | + |
| 275 | +### Step 4: Version-align `terraphim-clients` |
| 276 | +**Repo**: `terraphim-clients` |
| 277 | +**Files**: `Cargo.toml`, `crates/terraphim_grep/Cargo.toml`, `crates/terraphim_agent/Cargo.toml`, etc. |
| 278 | +**Description**: |
| 279 | +1. Bump `workspace.package.version` from `1.20.5` to `1.21.0`. |
| 280 | +2. Update `terraphim_service` dependency version in `terraphim_grep/Cargo.toml` to `1.21.0` if available on the registry; otherwise keep the workspace patch override and document it. |
| 281 | +3. Optionally create `RELEASE_NOTES_v1.21.0.md`. |
| 282 | +**Tests**: `cargo check --workspace` passes |
| 283 | +**Estimated**: 1 hour |
| 284 | + |
| 285 | +### Step 5: Build and install binaries |
| 286 | +**Repo**: both |
| 287 | +**Description**: |
| 288 | +1. `cargo install --path terraphim-clients/crates/terraphim_grep --force` |
| 289 | +2. `cargo install --path terraphim-clients/crates/terraphim_agent --force` |
| 290 | +3. `cargo install --path terraphim-ai/crates/terraphim_rlm --force` |
| 291 | +4. Verify versions: `terraphim-grep --version`, `terraphim-agent --version`, `terraphim-rlm --version`. |
| 292 | +**Tests**: Manual version checks |
| 293 | +**Estimated**: 1 hour |
| 294 | + |
| 295 | +### Step 6: Documentation cleanup |
| 296 | +**Repo**: `terraphim-ai` |
| 297 | +**Description**: |
| 298 | +1. Review untracked `.docs/`, `docs/handovers/`, `docs/plans/`, `docs/research/` files. |
| 299 | +2. Commit design/research artefacts that are still relevant; remove obsolete drafts. |
| 300 | +**Tests**: `git status --short` should show only intentional changes |
| 301 | +**Estimated**: 1 hour |
| 302 | + |
| 303 | +### Step 7: Gitea issue/PR workflow |
| 304 | +**Repo**: both |
| 305 | +**Description**: |
| 306 | +1. Create Gitea issues for Step 1, Step 2, and release alignment. |
| 307 | +2. Link Step 2 to Issue #2491. |
| 308 | +3. Open PRs with `Refs #<issue>` commits. |
| 309 | +4. Close issues after merge and binary verification. |
| 310 | +**Estimated**: 1 hour |
| 311 | + |
| 312 | +## Rollback Plan |
| 313 | + |
| 314 | +If issues are discovered: |
| 315 | + |
| 316 | +1. Revert the `code-search` default feature change in `terraphim_grep/Cargo.toml`. |
| 317 | +2. Revert `main.rs` thesaurus resolution to the previous mandatory path. |
| 318 | +3. Remove new test modules (safe — additive only). |
| 319 | +4. Restore previous `Cargo.lock` from git if needed. |
| 320 | +5. Re-install previous binaries with `cargo install --version 1.20.5 terraphim_grep terraphim_agent`. |
| 321 | + |
| 322 | +No database migrations or stateful changes are involved. |
| 323 | + |
| 324 | +## Dependencies |
| 325 | + |
| 326 | +### New Dependencies |
| 327 | + |
| 328 | +None. |
| 329 | + |
| 330 | +### Feature Changes |
| 331 | + |
| 332 | +| Crate | Feature | Change | |
| 333 | +|-------|---------|--------| |
| 334 | +| `terraphim_grep` | `code-search` | Move from optional to default | |
| 335 | +| `terraphim_grep` | `llm` | Remains default | |
| 336 | + |
| 337 | +### Version Updates |
| 338 | + |
| 339 | +| Crate/Workspace | From | To | Reason | |
| 340 | +|-----------------|------|-----|--------| |
| 341 | +| `terraphim-clients` workspace | `1.20.5` | `1.21.0` | Align with `terraphim-ai` release | |
| 342 | +| `terraphim_service` (grep dep) | `1.20.4/1.20.5` | `1.21.0` | Version parity | |
| 343 | + |
| 344 | +## Performance Considerations |
| 345 | + |
| 346 | +### Expected Performance |
| 347 | + |
| 348 | +| Metric | Target | Measurement | |
| 349 | +|--------|--------|-------------| |
| 350 | +| `terraphim-grep` no-thesaurus startup | < 500 ms | Manual timing on repo root | |
| 351 | +| `terraphim-grep` binary size delta | < +10 % | `ls -la target/release/terraphim-grep` before/after | |
| 352 | +| `terraphim_rlm` test duration | < 5 s for new tests | `cargo test -p terraphim_rlm` | |
| 353 | + |
| 354 | +### Benchmarks to Add |
| 355 | + |
| 356 | +None required; existing `hybrid_search` bench remains valid. |
| 357 | + |
| 358 | +## Open Items |
| 359 | + |
| 360 | +| Item | Status | Owner | |
| 361 | +|------|--------|-------| |
| 362 | +| Confirm `terraphim_service 1.21.0` is published on Gitea registry | Pending | Release owner | |
| 363 | +| Decide whether to keep or delete each untracked doc file | Pending | Project owner | |
| 364 | +| Approve making `code-search` a default feature | Pending | Human reviewer | |
| 365 | + |
| 366 | +## Approval |
| 367 | + |
| 368 | +- [ ] Technical review complete |
| 369 | +- [ ] Test strategy approved |
| 370 | +- [ ] Performance targets agreed |
| 371 | +- [ ] Human approval received |
| 372 | + |
| 373 | +## Cross-Repository Notes |
| 374 | + |
| 375 | +| Change | Repository | Branch naming | |
| 376 | +|--------|------------|---------------| |
| 377 | +| Grep failover | `terraphim-clients` | `task/<idx>-grep-kg-failover` | |
| 378 | +| RLM validation test | `terraphim-ai` | `task/<idx>-rlm-validation-test` | |
| 379 | +| Cargo.lock + docs | `terraphim-ai` | `task/<idx>-release-hygiene` | |
| 380 | +| Version bump clients | `terraphim-clients` | `task/<idx>-bump-1.21.0` | |
| 381 | + |
| 382 | +Push order (per Remote Sync Protocol): |
| 383 | + |
| 384 | +1. Push functional branches to `origin` first. |
| 385 | +2. After merge, push `main` to `origin`, then to `gitea`. |
| 386 | +3. Verify `git diff origin/main gitea/main --stat` is empty. |
0 commit comments