Terraphim AI documentation

PR Evaluation: 4 Open Pull Requests

Date: 2026-05-16 Evaluator: Claude (quality-coordinator role) Remotes: origin/main and gitea/main at ccb517b2d

Executive Summary

| PR | Title | Verdict | Risk | |----|-------|---------|------| | #1505 | Ranking regression gate (WIG-1) | APPROVE with comments | Medium | | #1504 | Phase 1 robot mode tests | APPROVE | Low | | #1502 | Unsafe ptr::read replacement | CONDITIONAL APPROVE | Medium | | #1501 | TrackerConfig api_key redaction | CONDITIONAL APPROVE | Low |

PR #1505 — Ranking Regression Gate (WIG-1 Lead Measure)

Branch: task/1454-ranking-regression-gate → main Changes: +1036 / -215 in 16 files References: Closes #1454 (open)

Research Phase Evaluation

1. Problem Restatement and Scope

The PR implements a deterministic regression gate for the terraphim_service ranking pipeline. The original issue identified a gap: ranking-relevant changes (BM25F scoring, thesaurus cache, etc.) land without a gate, and degradation only surfaces via downstream user complaints. The solution adds fixture corpora, committed top-N snapshots, and a Kendall-tau CI gate.

IN SCOPE: Fixture corpus definition, snapshot format, Kendall-tau threshold (0.95), CI gate wiring, snapshot update mechanism. OUT OF SCOPE: Changes to the actual ranking algorithm, snapshot diff visualisations beyond file diff.

2. System Elements

| Component | Role | |-----------|------| | crates/terraphim_service/tests/ranking_regression_test.rs | New test module — loads corpora, computes rankings, compares Kendall-tau | | crates/terraphim_service/fixtures/ranking_snapshots/*.json | 6 new fixture files (3 corpora × 2: corpus + snapshot) | | .github/workflows/ci-pr.yml | Adds ranking-regression-gate CI job | | .github/PULL_REQUEST_TEMPLATE.md | New template with "Ranking change ACK" section |

3. Constraints and Implications

  • Determinism required: Tests must produce identical results across runs. Uses sort_documents and QueryScorer — if these have non-deterministic elements (hash maps, parallel sort), the gate will flap.
  • Snapshot immutability: The UPDATE_RANKING_SNAPSHOTS=1 flag bypasses the gate intentionally. The PR template requires explicit ACK, but nothing enforces this mechanically.
  • CI timeout (5 min): Test must complete within 5 minutes. With 3 corpora × 5 queries each, loading JSON and scoring 60 documents should be fast, but the CI job has sudo chown -R and rm -rf target steps that add overhead.
  • Self-hosted runner: The CI uses [self-hosted, bigbox] — the sudo chown and cache key terraphim-ai suggest this is the same machine as the ADF host. The rm -rf target could interfere with concurrent CI runs.

4. Risks and Assumptions

| Risk | Assessment | |------|------------| | Non-deterministic scoring (e.g., parallel sort, HashMap iteration) causes flapping | MEDIUM — not verified in code review | | UPDATE_RANKING_SNAPSHOTS=1 can be set by attacker in PR to bypass gate | LOW — gate runs before snapshot update; requires local run + PR approval | | Snapshot files bloat repo (each corpus has 2 JSON files, 127+ entries each) | LOW — 6 files, manageable | | sudo chown + rm -rf target in CI step interferes with other jobs | MEDIUM — affects concurrent self-hosted jobs on same runner |

Design Phase Evaluation

Target Behaviour

The gate should: (a) load 3 corpora, (b) run 5 queries each, (c) assert Kendall-tau >= 0.95 against committed snapshots, (d) fail CI on regression, (e) allow intentional updates via env var.

Implementation Quality

Strengths:

  • Clean separation: test module + fixture files + CI job
  • UPDATE_RANKING_SNAPSHOTS=1 is a sensible escape hatch
  • PR template "Ranking change ACK" checkbox provides human accountability
  • Kendall-tau with 0.95 threshold is a reasonable, well-explained choice

Concerns:

  1. CI sudo rm -rf target step (line 393-394 of diff): Deletes the entire build cache before the test. On a self-hosted runner shared with other jobs, this forces full rebuilds for subsequent jobs. Should instead clean only target/ranking* or use cargo's test-specific target directory.

  2. CI sudo chown step (lines 385-389): Runs on every invocation. If the runner already runs as the correct user, this is unnecessary. If it doesn't, permissions may have been set intentionally.

  3. Non-determinism risk in QueryScorer: The test uses terraphim_types::score::sort_documents — if this uses parallel sorting or HashMap iteration, results may vary across runs or machines, causing CI flapping.

  4. Issue #1454 is still open: The PR claims to close #1454, but the issue is still open on Gitea. Either it should be closed (the PR implements the acceptance criteria) or the PR body should say "Closes #1454" not "Fixes #1454" if there's remaining work.

  5. 3 corpora but original spec said 3 corpus types: Confirmed consistent — the implementation has default (30 docs), engineering (BM25F), and system_operator (BM25Plus) which matches the issue description.

Recommendation

APPROVE with comments. The implementation is solid overall. Request the following comments be addressed:

  1. Remove sudo rm -rf target or scope it to target/ranking*
  2. Document whether sort_documents is guaranteed deterministic
  3. Verify issue #1454 should be closed by this PR

PR #1504 — Phase 1 Robot Mode Test Suite

Branch: task/1473-phase1-test-suite → main Changes: +112 / -0 in 2 files References: Closes #1473 (open)

Research Phase Evaluation

1. Problem Restatement and Scope

Task 1.6 of the Phase 1 spec requires unit and integration tests for robot mode features that were implemented in Tasks 1.1–1.5 but never tested. The gap: no test coverage for RobotResponse serialisation, ForgivingParser, exit codes, token budget truncation, or command alias expansion.

2. System Elements

| Component | Role | |-----------|------| | crates/terraphim_agent/src/robot/exit_codes.rs | Adds from_code round-trip test and unit tests for all 8 variants | | crates/terraphim_agent/tests/phase1_robot_mode_tests.rs | New test file: Table format test + RobotResponse all-formats serialisation tests |

3. Constraints

  • Tests must pass on current cargo test -p terraphim_agent suite (14 phase1 + 6 exit_code = 20 tests)
  • No changes to source code — purely additive test code
  • No CI workflow changes required

Design Phase Evaluation

Strengths:

  • Minimal, focused change — exactly what a test PR should look like
  • Round-trip test test_exit_code_from_code_round_trip covers all 8 ExitCode variants
  • test_robot_response_serialized_in_all_formats covers JSON/JSONL/minimal/table formats
  • All existing tests still pass (14 + 6 = 20)

Concerns:

  1. Issue #1473 is still open: The acceptance criteria in the issue list 5 test categories, and the PR body claims all are covered, but the issue is still open.

  2. What happened to the other 3 test categories? The issue lists: (a) ForgivingParser, (b) command alias expansion, (c) token-budget truncation, (d) exit code mapping, (e) RobotResponse serialisation. The PR covers (d) and (e), but (a), (b), (c) are not addressed — they are described as "existing inline tests" in the PR body, but issue #1473 marks them "Not Started". This needs clarification.

Recommendation

APPROVE. The tests added are correct and valuable. However, the PR claims to fully address #1473 but appears to only cover 2 of the 5 listed acceptance criteria. Clarify whether the other 3 criteria are covered elsewhere or whether #1473 should remain open.

PR #1502 — Replace Unsafe ptr::read in Multi-Agent Examples

Branch: task/1497-fix-unsafe-ptr-read-examples → main Changes: +36 / -212 in 6 files References: "Refs terraphim/terraphim-ai#1497" (misleading — see below)

Research Phase Evaluation

1. Critical Issue: Title Does Not Match Problem

Issue #1497 is titled: "[Security] Findings 2026-05-16 — P0 Cloudflare token exposure" PR #1502 is titled: "Fix #1497: replace unsafe ptr::read with arc_memory_only() in multi_agent examples"

The actual security finding #1497 contains 5 items:

  • P0 — Cloudflare DNS API tokens hardcoded in world-readable file (the primary finding)
  • P1 — LLM Proxy on all interfaces with SSRF vector
  • P2 — Unsafe ptr::read on static storage reference (what this PR actually fixes)
  • P2 — Unmaintained rustls-pemfile 1.0.4
  • P3 — Multiple unmaintained crates

This PR only addresses the P2 "unsafe ptr::read" sub-finding. The P0 (Cloudflare tokens) and P1 (LLM proxy) remain unresolved. The PR title implies it fixes the security finding when it only addresses a small fraction of it.

2. System Elements

| File | Change | |------|--------| | crates/terraphim_multi_agent/examples/agent_workflow_patterns.rs | 3 unsafe blocks removed | | crates/terraphim_multi_agent/examples/enhanced_atomic_server_example.rs | 1 unsafe block removed | | crates/terraphim_multi_agent/examples/knowledge_graph_integration.rs | 1 unsafe block removed | | crates/terraphim_multi_agent/examples/multi_agent_coordination.rs | 1 unsafe block removed | | crates/terraphim_multi_agent/examples/simple_validation.rs | 1 unsafe block removed | | crates/terraphim_multi_agent/examples/workflow_patterns_working.rs | 1 unsafe block removed |

3. Technical Analysis

The replacement pattern is:

// BEFORE (unsafe)
let storage_copy = unsafe { ptr::read(storage_ref) };

// AFTER (safe)
let persistence = DeviceStorage::arc_memory_only()
    .await
    .map_err(|e: DeviceStorageError| ...)?;
let persistence = persistence.clone(); // or Arc::clone(&persistence)

Concerns:

  1. DeviceStorage::arc_memory_only() is async: The replacement changes the function signature — async fn must be called with .await. The callers in the example code may need to become async fn main() or the initialization may need to be done differently.

  2. The map_err in an example binary: The replacement adds .map_err(|e: DeviceStorageError| ...)? — need to verify the error type name is correct and matches what's imported.

  3. Drop behaviour: The original ptr::read created a bitwise copy that was never dropped (it was a raw bitwise copy on a 'static reference). The new code uses Arc::clone or .clone() which properly handles reference counting. This is the correct fix.

  4. The simple_validation.rs change: Looking at the diff stats (-14 lines), this is a small change, likely removing just the unsafe block and the associated storage copy logic.

Design Phase Evaluation

Strengths:

  • Net negative code (good for safety): -212 / +36
  • DeviceStorage::arc_memory_only() is the correct safe API
  • Examples are non-production code, so this is low-risk
  • Addresses a P2 finding from the security report

Concerns:

  1. PR title is misleading: Should be "Fix #1497 P2: replace unsafe ptr::read..." not "Fix #1497: replace unsafe ptr::read..."
  2. Issue #1497 is still open: The P0 Cloudflare token exposure is not resolved by this PR. If this PR closes #1497, the P0 remains open. If #1497 stays open, this PR's "Refs" reference is fine.
  3. P0 Cloudflare tokens still exposed: The Caddyfile_auth file with the Cloudflare tokens needs remediation.

Recommendation

CONDITIONAL APPROVE. The technical fix is correct. However:

  1. Rename the PR title to clarify it only addresses the P2 sub-finding of #1497
  2. Confirm whether issue #1497 should be closed (the P2 is fixed) or remain open (P0 still unresolved)
  3. Verify the async .await pattern works correctly in all 6 example entry points

PR #1501 — TrackerConfig api_key Redaction

Branch: task/1300-mask-secrets-debug-display-v2 → main Changes: +249 / -5 in 3 files References: Completes #1300 (closed)

Research Phase Evaluation

1. Problem Restatement

Issue #1300 (now closed) required masking API keys and tokens in Debug output of orchestrator config structs. The original issue proposed a Secret<T> newtype approach. This PR implements a simpler alternative: remove derive(Debug) and write a manual Debug impl that hardcodes "***REDACTED***" for the api_key field.

2. System Elements

| File | Change | |------|--------| | crates/terraphim_orchestrator/src/config.rs | Remove derive(Debug) from TrackerConfig, add manual Debug impl | | clippy-results.txt | New file — unclear purpose | | reports/spec-validation-20260516.md | New file — unclear purpose |

3. What Issue #1300 Actually Required

The original issue acceptance criteria:

Given a config struct containing a non-empty api_key field When formatted with {:?} or {} Then the output contains "[REDACTED]" in place of the key value And config.api_key.reveal() returns the original string

The PR implements the redaction but:

  • Does not implement Secret<T> newtype
  • Does not implement a reveal() accessor
  • The api_key field remains String, not Secret<String>

This is a simpler implementation than specified, but it achieves the core security goal (keys don't appear in Debug output).

Design Phase Evaluation

Strengths:

  • Correctly implements api_key redaction in Debug output
  • TrackerConfig now safe from accidental secret exposure in tracing/panics
  • Part 3 of a systematic effort (#1498 covered GiteaOutputConfig + WebhookConfig)
  • Test test_tracker_config_api_key_redacted_in_debug verifies the fix

Concerns:

  1. Extra files in diff: clippy-results.txt and reports/spec-validation-20260516.md are unrelated artifacts in the PR. The diff stats show they are part of this PR. If they are documentation/reports, they should be reviewed. If they are accidental inclusions, they should be removed.

  2. Approach diverges from issue #1300 specification: The issue described Secret<T> newtype with reveal(). The PR uses manual Debug impl. This is simpler but less extensible. Since #1300 is already closed, this is acceptable, but the divergence should be noted.

  3. Test plan in PR body has unchecked boxes: The test plan shows [ ] checkboxes but the actual CI has presumably passed. Either the checkboxes should be checked or the test plan should be marked as verified.

Recommendation

CONDITIONAL APPROVE. The redaction is implemented correctly. Request clarification on:

  1. What are clippy-results.txt and reports/spec-validation-20260516.md? Are they intentional or accidental?
  2. If intentional, do they need review?

Cross-PR Observations

1. Issue Hygiene

All 4 PRs reference issues that are either still open (#1454, #1473) or were closed before the PR was merged (#1300 closed 2026-05-16 19:40, PR #1501 created 2026-05-16 19:05). This is acceptable but the mismatch between issue state and PR intent should be cleaned up.

2. CI Workflow Modifications

PR #1505 is the only PR that modifies CI. The sudo chown and rm -rf target patterns are concerning for a self-hosted shared runner. Consider:

  • Using a dedicated ranking test cache key
  • Removing the broad rm -rf target in favour of targeted clean-up

3. Security Findings Tracking

PR #1502 addresses only a P2 sub-finding of #1497 (the P0 Cloudflare token exposure). This is fine, but the PR title should accurately reflect what it fixes to avoid misleading anyone scanning for "what did we fix for the P0?".

4. Test Coverage

PR #1504 claims to fully address #1473 but covers only 2 of 5 acceptance criteria. The other 3 ("existing inline tests") need verification.

Summary Recommendation

| PR | Decision | Key Action Required | |----|----------|---------------------| | #1505 | APPROVE with comments | Address CI rm -rf target scope, verify determinism | | #1504 | APPROVE | Clarify #1473 scope — only 2/5 criteria addressed | | #1502 | CONDITIONAL APPROVE | Rename title to reflect P2-only fix; confirm #1497 issue state | | #1501 | CONDITIONAL APPROVE | Clarify purpose of clippy-results.txt and reports/spec-validation-20260516.md |

Platform-Specific Installation Guide Quickwit Integration Guide