Skip to content

Commit 66edb51

Browse files
AlexMikhalevclaude
AlexMikhalev
and
claude
authored
feat(cli): add evaluate subcommand for automata ground-truth evaluation (#818)
* feat(cli): add evaluate subcommand for automata ground-truth evaluation Wire the existing evaluate() function to a CLI subcommand in terraphim_cli. Changes: - Add Evaluate command with --ground-truth and --thesaurus flags - Add handle_evaluate() function using terraphim_automata::evaluate() - Add 4 integration tests for evaluate command - Wire Evaluate match arm in command dispatcher The core evaluation logic was already implemented in terraphim_automata::evaluation (~613 lines, 13 unit tests). This adds CLI integration for automation use. Example usage: terraphim-cli evaluate --ground-truth gt.json --thesaurus th.json Part of: Gitea #576 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(clippy): replace sort_by with sort_by_key for Rust 1.95 compatibility Rust 1.95 promotes clippy::unnecessary_sort_by to hard error under -D warnings. Convert all sort_by calls to sort_by_key across 3 crates: - terraphim-markdown-parser: 1 change (descending sort with Reverse) - terraphim_router: 1 change (descending sort with Reverse) - terraphim-session-analyzer: 13 changes (ascending + descending) Line 548 in reporter.rs retains sort_by with #[allow] due to fallible string parsing in the key function. Refs #576 * fix(clippy): fix remaining sort_by in session-analyzer main.rs and submodules Missed in previous commit: session-analyzer has duplicated logic in main.rs (binary target) and submodules (kg/search, patterns/loader) that also use sort_by. Convert to sort_by_key where possible, add #[allow] for float comparisons using partial_cmp. Refs #576 * fix(clippy): workspace-wide sort_by -> sort_by_key for Rust 1.95 compatibility Convert all remaining sort_by calls across 40 files to either sort_by_key or #[allow(clippy::unnecessary_sort_by)] for cases with non-Copy types, multi-line closures, or partial_cmp on floats. Covers: terraphim_agent, terraphim_automata, terraphim_orchestrator, terraphim_service, terraphim_persistence, terraphim_update, terraphim_usage, terraphim_sessions, terraphim_cli, terraphim_mcp_server, terraphim_types, terraphim_symphony, terraphim_tinyclaw, terraphim_multi_agent, terraphim_agent_evolution, terraphim_agent_registry, terraphim_goal_alignment Refs #576 * fix(clippy): fix Rust 1.95 lints in task_decomposition and rolegraph examples - Remove unnecessary .into_iter() in extend() call (useless_conversion lint) - Collapse if guards into match arms (collapsible_match lint) - Allow explicit_counter_loop in rolegraph examples Refs #576 * fix(clippy): allow collapsible_match lint in middleware and agent_evolution Rust 1.95 clippy promotes collapsible_match to hard error under -D warnings. Add #![allow] at file level for ripgrep.rs, orchestrator_workers.rs, and parallelization.rs where collapsing the match arms would reduce readability. Refs #576 * fix(clippy): allow collapsible_match in goal_alignment/goals.rs Refs #576 * fix(ci): pin Rust toolchain to 1.94.0 to match local dev dtolnay/rust-toolchain@stable installs latest (1.95.0) which has new clippy lints (collapsible_match, unnecessary_sort_by, useless_conversion) not present in 1.94. Pin all ci-pr.yml jobs to 1.94.0 and update rust-toolchain.toml accordingly. Refs #576 --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent fcb4213 commit 66edb51

33 files changed

Lines changed: 866 additions & 104 deletions

File tree

‎.docs/design-pr818-clippy-lint-fixes.md‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
# Implementation Plan: Fix CI `sort_by` Clippy Failures for PR #818
2+
3+
**Status**: Approved
4+
**Research Doc**: `.docs/research-pr818-clippy-lint-fixes.md`
5+
**Author**: opencode (disciplined-design)
6+
**Date**: 2026-04-17
7+
**Estimated Effort**: 30 minutes
8+
9+
## Overview
10+
11+
### Summary
12+
Replace all `sort_by` calls flagged by Rust 1.95 clippy with `sort_by_key` equivalents. 13 mechanical conversions + 3 special cases.
13+
14+
### Approach
15+
Mechanical search-and-replace. Each `sort_by` converting to `sort_by_key` either:
16+
- **Ascending**: `sort_by(|a, b| a.x.cmp(&b.x))` -> `sort_by_key(|a| a.x)`
17+
- **Descending**: `sort_by(|a, b| b.x.cmp(&a.x))` -> `sort_by_key(|b| std::cmp::Reverse(b.x))`
18+
19+
### Scope
20+
**In Scope:**
21+
- Fix all 13 clippy-flagged `sort_by` calls across 5 files in 3 crates
22+
- Handle 3 special cases (multi-key, string-parse, IndexMap)
23+
24+
**Out of Scope:**
25+
- `tui_desktop_parity_test.rs` type annotation errors
26+
- Upgrading local Rust version
27+
- Any changes to terraphim-cli or terraphim-automata (branch's own code)
28+
29+
## File Changes
30+
31+
### Modified Files
32+
| File | Changes |
33+
|------|---------|
34+
| `crates/terraphim-markdown-parser/src/lib.rs` | 1 sort_by -> sort_by_key |
35+
| `crates/terraphim_router/src/keyword.rs` | 1 sort_by -> sort_by_key |
36+
| `crates/terraphim-session-analyzer/src/analyzer.rs` | 4 sort_by -> sort_by_key + 2 IndexMap sort_by |
37+
| `crates/terraphim-session-analyzer/src/parser.rs` | 1 sort_by -> sort_by_key |
38+
| `crates/terraphim-session-analyzer/src/reporter.rs` | 7 sort_by -> sort_by_key |
39+
40+
### No new or deleted files.
41+
42+
## Implementation Steps
43+
44+
### Step 1: terraphim-markdown-parser (1 change)
45+
**File:** `crates/terraphim-markdown-parser/src/lib.rs:144`
46+
47+
```rust
48+
// Before:
49+
edits.sort_by(|a, b| b.range.start.cmp(&a.range.start));
50+
// After:
51+
edits.sort_by_key(|b| std::cmp::Reverse(b.range.start));
52+
```
53+
54+
### Step 2: terraphim_router (1 change)
55+
**File:** `crates/terraphim_router/src/keyword.rs:61`
56+
57+
```rust
58+
// Before:
59+
matched_keywords.sort_by(|a, b| b.1.cmp(&a.1));
60+
// After:
61+
matched_keywords.sort_by_key(|b| std::cmp::Reverse(b.1));
62+
```
63+
64+
### Step 3: terraphim-session-analyzer/analyzer.rs (6 changes)
65+
**File:** `crates/terraphim-session-analyzer/src/analyzer.rs`
66+
67+
| Line | Before | After |
68+
|------|--------|-------|
69+
| 210 | `agents.sort_by(\|a, b\| a.timestamp.cmp(&b.timestamp))` | `agents.sort_by_key(\|a\| a.timestamp)` |
70+
| 564 | `sorted.sort_by(\|a, b\| b.1.cmp(&a.1))` | `sorted.sort_by_key(\|b\| std::cmp::Reverse(b.1))` |
71+
| 637 | `correlations.sort_by(\|a, b\| b.usage_count.cmp(&a.usage_count))` | `correlations.sort_by_key(\|b\| std::cmp::Reverse(b.usage_count))` |
72+
| 718 | `stats.sort_by(\|_, v1, _, v2\| v2.total_invocations.cmp(&v1.total_invocations))` | `stats.sort_by(\|_, v1, _, v2\| v2.total_invocations.cmp(&v1.total_invocations))` **ALLOW** (IndexMap method, not Vec) |
73+
| 739 | `breakdown.sort_by(\|_, v1, _, v2\| v2.cmp(v1))` | `breakdown.sort_by(\|_, v1, _, v2\| v2.cmp(v1))` **ALLOW** (IndexMap method, not Vec) |
74+
| 880 | `chains.sort_by(\|a, b\| b.frequency.cmp(&a.frequency))` | `chains.sort_by_key(\|b\| std::cmp::Reverse(b.frequency))` |
75+
76+
**Lines 718, 739**: These use `IndexMap::sort_by` (not `Vec::sort_by`). Clippy's `unnecessary_sort_by` lint only fires on `Vec::sort_by`, so these should be safe. Verify after fix by running clippy. If they do fire, add `#[allow(clippy::unnecessary_sort_by)]`.
77+
78+
### Step 4: terraphim-session-analyzer/parser.rs (1 change)
79+
**File:** `crates/terraphim-session-analyzer/src/parser.rs:422`
80+
81+
```rust
82+
// Before:
83+
events.sort_by(|a, b| a.timestamp.cmp(&b.timestamp));
84+
// After:
85+
events.sort_by_key(|a| a.timestamp);
86+
```
87+
88+
### Step 5: terraphim-session-analyzer/reporter.rs (7 changes)
89+
**File:** `crates/terraphim-session-analyzer/src/reporter.rs`
90+
91+
| Line | Before | After |
92+
|------|--------|-------|
93+
| 168 | `events.sort_by(\|a, b\| a.0.cmp(&b.0))` | `events.sort_by_key(\|a\| a.0)` |
94+
| 227 | `sorted_agents.sort_by(\|a, b\| b.1.cmp(&a.1))` | `sorted_agents.sort_by_key(\|b\| std::cmp::Reverse(b.1))` |
95+
| 447 | `tool_stats.sort_by(\|a, b\| b.1.total_invocations.cmp(&a.1.total_invocations))` | `tool_stats.sort_by_key(\|b\| std::cmp::Reverse(b.1.total_invocations))` |
96+
| 548 | Multi-key with string parse | Add `#[allow(clippy::unnecessary_sort_by)]` (see note) |
97+
| 570 | `category_rows.sort_by(\|a, b\| b.1.cmp(&a.1))` | `category_rows.sort_by_key(\|b\| std::cmp::Reverse(b.1))` |
98+
| 763 | `category_rows.sort_by(\|a, b\| b.1.cmp(&a.1))` | `category_rows.sort_by_key(\|b\| std::cmp::Reverse(b.1))` |
99+
| 784 | `tool_list.sort_by(\|a, b\| b.1.total_invocations.cmp(&a.1.total_invocations))` | `tool_list.sort_by_key(\|b\| std::cmp::Reverse(b.1.total_invocations))` |
100+
101+
**Line 548**: Sorts by parsing `count` field from `String` to `u32`. Cannot be expressed as a pure key function because `parse()` is fallible. Add `#[allow(clippy::unnecessary_sort_by)]` attribute above the sort call.
102+
103+
### Step 6: Verify
104+
```bash
105+
rustup update stable # Update to 1.95
106+
cargo clippy --workspace --all-targets -- -D warnings
107+
cargo test --workspace
108+
cargo fmt --check
109+
```
110+
111+
Then push and monitor CI.
112+
113+
## Test Strategy
114+
115+
No new tests needed -- these are semantically equivalent transformations. Existing tests cover the sorting behaviour.
116+
117+
## Rollback Plan
118+
119+
`git revert` the commit if any sorting behaviour regresses.

‎.docs/implementation-plan-issue-576-evaluate-cli.md‎

Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,195 @@
1+
# Implementation Plan: Issue #576 - Ground-truth Evaluation Framework CLI Integration
2+
3+
**Status**: Draft
4+
**Research Doc**: `.docs/research-issue-576-evaluation-cli.md`
5+
**Author**: Claude
6+
**Date**: 2026-04-16
7+
**Estimated Effort**: 2-3 hours
8+
9+
## Overview
10+
11+
### Summary
12+
Wire the existing `evaluate_automata()` function from `terraphim_automata` to a CLI subcommand in `terraphim_cli`. The core evaluation logic is already implemented; this is CLI integration only.
13+
14+
### Approach
15+
Add an `Evaluate` subcommand to `terraphim_cli` that:
16+
1. Loads ground truth JSON from `--ground-truth` flag
17+
2. Loads thesaurus from `--thesaurus` flag
18+
3. Calls `evaluate()` function
19+
4. Outputs JSON `EvaluationResult`
20+
21+
### Scope
22+
**In Scope:**
23+
- Add `Evaluate` command variant to `Commands` enum in `terraphim_cli`
24+
- Implement `evaluate` subcommand handler in `CliService`
25+
- Add `terraphim_automata` dependency to `terraphim_cli`
26+
- Integration tests for evaluate command
27+
28+
**Out of Scope:**
29+
- New evaluation metrics (already in evaluation.rs)
30+
- Confusion matrix output (not in issue spec)
31+
- terraphim_agent integration (terraphim_cli is better fit for automation)
32+
33+
**Avoid At All Cost:**
34+
- Adding human-readable output (automation-focused, JSON only)
35+
- Modifying evaluation.rs (already complete and tested)
36+
- Adding schema-based evaluation (future work)
37+
38+
## Architecture
39+
40+
### Component Diagram
41+
```
42+
terraphim_cli
43+
└── Evaluate command
44+
├── load_ground_truth(path) --> GroundTruthDocument[]
45+
├── load_thesaurus(path) --> Thesaurus
46+
└── evaluate(gt, thesaurus) --> EvaluationResult
47+
├── overall: ClassificationMetrics
48+
├── per_term: TermReport[]
49+
└── systematic_errors: SystematicError[]
50+
```
51+
52+
### Data Flow
53+
```
54+
CLI args (--ground-truth, --thesaurus)
55+
-> CliService::evaluate()
56+
-> terraphim_automata::load_ground_truth()
57+
-> terraphim_automata::load_thesaurus()
58+
-> terraphim_automata::evaluate()
59+
-> EvaluationResult (JSON serialized)
60+
-> stdout
61+
```
62+
63+
## File Changes
64+
65+
### Modified Files
66+
| File | Changes |
67+
|------|---------|
68+
| `crates/terraphim_cli/Cargo.toml` | Add `terraphim_automata` dependency |
69+
| `crates/terraphim_cli/src/main.rs` | Add `Evaluate` variant to `Commands` enum |
70+
| `crates/terraphim_cli/src/service.rs` | Add `evaluate()` method to `CliService` |
71+
| `crates/terraphim_cli/tests/integration_tests.rs` | Add integration test |
72+
73+
## API Design
74+
75+
### CLI Command Signature
76+
```rust
77+
/// Evaluate automata classification accuracy against ground truth
78+
Evaluate {
79+
/// Path to ground truth JSON file
80+
#[arg(long)]
81+
ground_truth: String,
82+
83+
/// Path to thesaurus JSON file
84+
#[arg(long)]
85+
thesaurus: String,
86+
}
87+
```
88+
89+
### Types (already in terraphim_automata::evaluation)
90+
```rust
91+
// GroundTruthDocument - already exists
92+
// EvaluationResult - already exists
93+
// ClassificationMetrics - already exists
94+
// SystematicError - already exists
95+
```
96+
97+
## Implementation Steps
98+
99+
### Step 1: Add Evaluate to Commands enum
100+
**File:** `crates/terraphim_cli/src/main.rs`
101+
**Lines:** ~100-110 (after `Coverage` variant)
102+
**Change:**
103+
```rust
104+
/// Evaluate automata classification against ground truth
105+
Evaluate {
106+
/// Path to ground truth JSON file
107+
#[arg(long)]
108+
ground_truth: String,
109+
110+
/// Path to thesaurus JSON file
111+
#[arg(long)]
112+
thesaurus: String,
113+
},
114+
```
115+
116+
### Step 2: Add terraphim_automata dependency
117+
**File:** `crates/terraphim_cli/Cargo.toml`
118+
**Change:**
119+
```toml
120+
terraphim-automata = { path = "../terraphim_automata", features = [] }
121+
```
122+
123+
### Step 3: Add evaluate() method to CliService
124+
**File:** `crates/terraphim_cli/src/service.rs`
125+
**Change:** Add method that:
126+
1. Loads ground truth via `terraphim_automata::load_ground_truth()`
127+
2. Loads thesaurus via `terraphim_automata::load_thesaurus()`
128+
3. Calls `terraphim_automata::evaluate()`
129+
4. Serializes and prints result as JSON
130+
131+
### Step 4: Wire Evaluate in main.rs match
132+
**File:** `crates/terraphim_cli/src/main.rs`
133+
**Location:** ~400 (in the Command::run() match)
134+
**Change:**
135+
```rust
136+
Commands::Evaluate { ground_truth, thesaurus } => {
137+
cli_service.evaluate(&ground_truth, &thesaurus).await?;
138+
}
139+
```
140+
141+
### Step 5: Add integration test
142+
**File:** `crates/terraphim_cli/tests/integration_tests.rs`
143+
**Change:** Add test that:
144+
1. Creates temporary ground truth JSON
145+
2. Creates temporary thesaurus JSON
146+
3. Runs evaluate command
147+
4. Verifies JSON output contains expected fields
148+
149+
## Test Strategy
150+
151+
### Unit Tests
152+
No new unit tests needed - evaluation logic is already tested in `terraphim_automata`
153+
154+
### Integration Tests
155+
| Test | Location | Purpose |
156+
|------|----------|---------|
157+
| `test_evaluate_command_success` | `integration_tests.rs` | Full flow with valid files |
158+
| `test_evaluate_command_missing_ground_truth` | `integration_tests.rs` | Error handling |
159+
| `test_evaluate_command_missing_thesaurus` | `integration_tests.rs` | Error handling |
160+
161+
### Test Data
162+
Use temporary files created in tests (no external fixtures needed)
163+
164+
## Dependencies
165+
166+
### New Dependencies
167+
| Crate | Version | Justification |
168+
|-------|---------|---------------|
169+
| terraphim-automata | path | Required for evaluation module |
170+
171+
## Performance Considerations
172+
173+
- Evaluation is O(n*m) where n=documents, m=thesaurus terms
174+
- Expected: <100ms for typical workloads
175+
- No benchmarks needed for CLI wrapper
176+
177+
## Rollback Plan
178+
179+
If issues discovered:
180+
1. Remove `Evaluate` variant from `Commands` enum
181+
2. Remove `evaluate()` method from `CliService`
182+
3. Remove `terraphim_automata` dependency
183+
184+
## Simplicity Check
185+
186+
**What if this could be easy?**
187+
This IS simple - just 4 files to modify, no new types, no complex logic. The evaluation module is already complete. This is a thin CLI wrapper.
188+
189+
## Approval Gate
190+
191+
- [ ] Research document reviewed and approved
192+
- [ ] Implementation plan reviewed
193+
- [ ] File changes defined
194+
- [ ] Test strategy defined
195+
- [ ] Human approval received

0 commit comments

Comments
 (0)