Skip to content

Commit f3f69c2

Browse files
AlexMikhalevAlexMikhalev
committed
feat(agent): add learn export-kg command and NormalizedTerm metadata Refs #759 #735 
1 parent 8dd24dd commit f3f69c2

21 files changed

Lines changed: 1480 additions & 1074 deletions

File tree

‎.docs/design-735-normalizedterm-metadata.md‎

Lines changed: 211 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,211 @@
1+
# Design & Implementation Plan: Add Action Metadata to NormalizedTerm
2+
3+
**Issue**: #735 — `feat(types): add action metadata to NormalizedTerm beyond url field`
4+
**Author**: Agent
5+
**Date**: 2026-04-22
6+
**Phase**: 2 — Design
7+
8+
---
9+
10+
## 1. Summary of Target Behavior
11+
12+
After implementation, `NormalizedTerm` carries four additional optional metadata fields:
13+
14+
| Field | Type | Description |
15+
|-------|------|-------------|
16+
| `action` | `Option<String>` | CLI action template with `{{ model }}` and `{{ prompt }}` placeholders |
17+
| `priority` | `Option<u8>` | Routing tiebreaking priority (higher = preferred) |
18+
| `trigger` | `Option<String>` | Pattern or alias that activates this term |
19+
| `pinned` | `bool` | Whether the term is pinned (default `false`) |
20+
21+
`AutocompleteMetadata` (in `terraphim_automata`) receives the same four fields in the same PR to stay in sync.
22+
23+
All new fields are `Option<T>` (or `bool` with `#[serde(default)]`), ensuring backward-compatible JSON deserialization: existing serialised `NormalizedTerm` data remains valid.
24+
25+
---
26+
27+
## 2. Key Invariants and Acceptance Criteria
28+
29+
### Invariants
30+
- `NormalizedTerm` JSON can be deserialised regardless of which optional fields are present
31+
- Adding fields does not change the meaning of existing `id`, `value`, `display_value`, or `url` fields
32+
- `AutocompleteMetadata` and `NormalizedTerm` remain structurally aligned for their shared fields
33+
34+
### Acceptance Criteria
35+
36+
| # | Criterion | Test Type | Location |
37+
|---|-----------|-----------|----------|
38+
| AC1 | `NormalizedTerm::new()`, `with_auto_id()`, `with_display_value()`, `with_url()` still compile and produce terms without the new fields set | Unit | New test in `terraphim_types` |
39+
| AC2 | `NormalizedTerm` deserialises from JSON missing all new fields | Unit | New test in `terraphim_types` |
40+
| AC3 | `NormalizedTerm` serialises with all four new fields present | Unit | New test in `terraphim_types` |
41+
| AC4 | `NormalizedTerm::action()` / `::priority()` / `::trigger()` / `::pinned()` builder methods work | Unit | New test in `terraphim_types` |
42+
| AC5 | `AutocompleteMetadata` gains the same four fields | Unit | New test in `terraphim_automata` |
43+
| AC6 | `cargo build --workspace` succeeds with no new warnings | Build | CI |
44+
| AC7 | All existing tests pass (`cargo test --workspace`) | Integration | CI |
45+
46+
---
47+
48+
## 3. High-Level Design and Boundaries
49+
50+
### Components
51+
52+
```
53+
crates/terraphim_types/src/lib.rs
54+
└── NormalizedTerm struct (MODIFY)
55+
+ action: Option<String>
56+
+ priority: Option<u8>
57+
+ trigger: Option<String>
58+
+ pinned: bool
59+
+ builder methods: with_action(), with_priority(), with_trigger(), with_pinned()
60+
+ getter methods: action(), priority(), trigger(), pinned()
61+
62+
crates/terraphim_automata/src/autocomplete.rs
63+
└── AutocompleteMetadata struct (MODIFY)
64+
+ action: Option<String>
65+
+ priority: Option<u8>
66+
+ trigger: Option<String>
67+
+ pinned: bool
68+
```
69+
70+
### Boundaries
71+
- **Inside**: Adding fields to `NormalizedTerm` and `AutocompleteMetadata`, including serde attributes and builder/getter methods
72+
- **Outside**: No changes to `MarkdownDirectives`, `RouteDirective`, `RoutingRule`, `Thesaurus`, or any storage layer
73+
- **New dependencies**: None — only standard library + existing serde/ahash imports
74+
75+
### Anti-Patterns Avoided
76+
- No wrapper struct (per user decision)
77+
- No changes to JSON schema for existing persisted data (backward-compatible `Option` fields)
78+
- No migration of existing data
79+
80+
---
81+
82+
## 4. File/Module-Level Change Plan
83+
84+
| File/Module | Action | Before | After | Dependencies |
85+
|-------------|--------|--------|-------|--------------|
86+
| `crates/terraphim_types/src/lib.rs` | Modify | `NormalizedTerm` with 4 fields | `NormalizedTerm` with 8 fields; 4 new builder methods; 4 getter methods | None new |
87+
| `crates/terraphim_types/src/lib.rs` | Modify | `AutocompleteMetadata` with 4 fields | `AutocompleteMetadata` with 8 fields | None new |
88+
| `crates/terraphim_automata/src/autocomplete.rs` | Modify | `AutocompleteMetadata` with 4 fields | `AutocompleteMetadata` with 8 fields; serde attributes updated | `terraphim_types` re-export |
89+
90+
---
91+
92+
## 5. Step-by-Step Implementation Sequence
93+
94+
### Step 1: Add fields to `NormalizedTerm` in `terraphim_types/src/lib.rs`
95+
- Add `action: Option<String>` with `#[serde(default, skip_serializing_if = "Option::is_none")]`
96+
- Add `priority: Option<u8>` with same serde attributes
97+
- Add `trigger: Option<String>` with same serde attributes
98+
- Add `pinned: bool` with `#[serde(default)]`
99+
- Add builder methods: `with_action(self, String)`, `with_priority(self, u8)`, `with_trigger(self, String)`, `with_pinned(self, bool)`
100+
- Add getter methods: `action(&self) -> Option<&String>`, `priority(&self) -> Option<&u8>`, `trigger(&self) -> Option<&String>`, `pinned(&self) -> bool`
101+
- Update `new()` to initialise all new fields to default values
102+
- Update `with_auto_id()` similarly
103+
- Update existing `with_url()` return type to `Self` (already correct for builder pattern)
104+
105+
**Deployable state**: Yes — compiles, all old code path-compatible
106+
107+
### Step 2: Add tests for `NormalizedTerm` in `terraphim_types`
108+
- Test deserialisation from JSON missing all new fields
109+
- Test serialisation with all fields set
110+
- Test builder chain with all new methods
111+
- Test `display()` still works with new fields
112+
113+
**Deployable state**: Yes — tests only
114+
115+
### Step 3: Update `AutocompleteMetadata` in `terraphim_automata/src/autocomplete.rs`
116+
- Add the same four fields with identical serde attributes
117+
- Add identical builder and getter methods
118+
119+
**Deployable state**: Yes — compiles, structurally aligned with NormalizedTerm
120+
121+
### Step 4: Add tests for `AutocompleteMetadata`
122+
- Test serialisation and deserialisation round-trip
123+
- Test builder methods
124+
125+
**Deployable state**: Yes — tests only
126+
127+
### Step 5: Run full workspace build and test
128+
- `cargo build --workspace`
129+
- `cargo test --workspace`
130+
- `cargo clippy -- -D warnings`
131+
132+
**Deployable state**: Yes — verified green
133+
134+
---
135+
136+
## 6. Testing & Verification Strategy
137+
138+
| Acceptance Criteria | Test Type | Test Location |
139+
|---------------------|-----------|---------------|
140+
| AC1: Constructors produce unset new fields | Unit | `crates/terraphim_types/src/lib.rs` (inline tests or `tests/` dir) |
141+
| AC2: JSON backward compat (missing fields) | Unit | Same |
142+
| AC3: JSON serialisation with all fields | Unit | Same |
143+
| AC4: Builder/getter methods | Unit | Same |
144+
| AC5: AutocompleteMetadata alignment | Unit | `crates/terraphim_automata/src/autocomplete.rs` |
145+
| AC6: No new warnings | Build | CI (`cargo clippy`) |
146+
| AC7: All tests pass | Integration | CI (`cargo test --workspace`) |
147+
148+
---
149+
150+
## 7. Risk & Complexity Review
151+
152+
| Risk | Mitigation | Residual Risk |
153+
|------|------------|--------------|
154+
| Forgetting to update a construction site causes compiler warning | Run `cargo build --workspace` which will warn on missing fields; all test crates updated in same PR | Low |
155+
| JSON output change breaks external consumers | New fields are optional; consumers that ignore unknown fields are unaffected | Low |
156+
| `AutocompleteMetadata` and `NormalizedTerm` drift out of sync in future | Document that they must be updated together; consider a shared test | Low |
157+
158+
---
159+
160+
## 8. Open Questions / Decisions for Human Review
161+
162+
1. **Field defaults for `priority`**: `Option<u8>` means `None` means no priority set. Should `priority: Option<u8>` be replaced with `priority: u8` with `#[serde(default = "default_priority")]` where `default_priority()` returns `50` as a sensible neutral priority? (The current plan uses `Option<u8>` — confirm this is preferred or choose the default-value approach.)
163+
164+
2. **Should `trigger` be a `Vec<String>` instead of `Option<String>`** to support multiple trigger patterns per term? (Current plan uses `Option<String>` — single trigger per term. Multi-trigger would require `Option<Vec<String>>`.)
165+
166+
3. **`pinned: bool` with `#[serde(default)]`** — should pinned terms default to `false` (current plan) or should the serde default be a specific behaviour (e.g., pinned = false is never serialised)?
167+
168+
**Note**: All three questions above are clarifications on default values and representation — the four-field structure is fixed per the approved research. The default-value approach for `priority` and the single-string `trigger` are recommended for simplicity.
169+
170+
---
171+
172+
## Appendix: Target Struct Shapes
173+
174+
### `NormalizedTerm` (after)
175+
```rust
176+
pub struct NormalizedTerm {
177+
pub id: u64,
178+
pub value: NormalizedTermValue,
179+
#[serde(default, skip_serializing_if = "Option::is_none")]
180+
pub display_value: Option<String>,
181+
#[serde(default, skip_serializing_if = "Option::is_none")]
182+
pub url: Option<String>,
183+
#[serde(default, skip_serializing_if = "Option::is_none")]
184+
pub action: Option<String>,
185+
#[serde(default, skip_serializing_if = "Option::is_none")]
186+
pub priority: Option<u8>,
187+
#[serde(default, skip_serializing_if = "Option::is_none")]
188+
pub trigger: Option<String>,
189+
#[serde(default)]
190+
pub pinned: bool,
191+
}
192+
```
193+
194+
### `AutocompleteMetadata` (after)
195+
```rust
196+
pub struct AutocompleteMetadata {
197+
pub id: u64,
198+
pub normalized_term: NormalizedTermValue,
199+
pub original_term: String,
200+
#[serde(default, skip_serializing_if = "Option::is_none")]
201+
pub url: Option<String>,
202+
#[serde(default, skip_serializing_if = "Option::is_none")]
203+
pub action: Option<String>,
204+
#[serde(default, skip_serializing_if = "Option::is_none")]
205+
pub priority: Option<u8>,
206+
#[serde(default, skip_serializing_if = "Option::is_none")]
207+
pub trigger: Option<String>,
208+
#[serde(default)]
209+
pub pinned: bool,
210+
}
211+
```

‎.docs/quality-evaluation-design-735.md‎

Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
# Document Quality Evaluation Report
2+
3+
## Metadata
4+
- **Document**: `.docs/design-735-normalizedterm-metadata.md`
5+
- **Type**: Phase 2 Design
6+
- **Evaluated**: 2026-04-22
7+
8+
## Decision: GO
9+
10+
**Average Score**: 4.2 / 5.0
11+
**Blocking Dimensions**: None
12+
13+
---
14+
15+
## Dimension Scores
16+
17+
| Dimension | Score | Status |
18+
|-----------|-------|--------|
19+
| Syntactic | 5/5 | Pass |
20+
| Semantic | 4/5 | Pass |
21+
| Pragmatic | 4/5 | Pass |
22+
| Social | 4/5 | Pass |
23+
| Physical | 4/5 | Pass |
24+
| Empirical | 4/5 | Pass |
25+
26+
---
27+
28+
## Detailed Findings
29+
30+
### Syntactic Quality (5/5) — EXEMPLARY
31+
32+
**Strengths:**
33+
- All 8 expected sections present and correctly labelled
34+
- Tables in Sections 2, 3, 5, 6, 7 are internally consistent (columns match content)
35+
- The Appendix shows exact target Rust struct shapes, fully consistent with the prose descriptions
36+
- No contradictions between sections: Section 5 step sequence matches Section 4 change plan
37+
- Code in appendix uses correct Rust syntax and serde attributes (`#[serde(default, skip_serializing_if = "Option::is_none")]`) matching the established pattern in the codebase
38+
- Field names and types in appendix exactly match the field list in Section 1
39+
40+
**Weaknesses:**
41+
- None identified
42+
43+
**Suggested Revisions:**
44+
- None
45+
46+
---
47+
48+
### Semantic Quality (4/5)
49+
50+
**Strengths:**
51+
- All field types are technically accurate: `Option<String>` for action/trigger, `Option<u8>` for priority, `bool` for pinned
52+
- serde attribute strategy (backward-compatible `Option` with `skip_serializing_if`) correctly applied to all three new optional fields
53+
- `pinned: bool` with `#[serde(default)]` correctly means it serialises only when `true`
54+
- Builder and getter method signatures are plausible and follow existing builder pattern in the codebase
55+
- Anti-patterns section correctly identifies what is NOT being done (no wrapper struct, no migration, no changes to related types)
56+
57+
**Weaknesses:**
58+
- Section 7 risk table repeats the same "Low" residual risk for all three risks — the actual residual risks are genuinely low but the repetition makes the table uninformative
59+
- Open Question 1 in Section 8 asks about `priority` default value but the field is declared `Option<u8>` — the question is valid but the design should tentatively pick a default or confirm `Option<u8>` is correct before implementation
60+
61+
**Suggested Revisions:**
62+
- [ ] Add a tentative recommendation in Open Question 1 (e.g., "Recommend Option<u8> with None = unset, since 50 as magic default could conflict with intentional priority values")
63+
64+
---
65+
66+
### Pragmatic Quality (4/5)
67+
68+
**Strengths:**
69+
- Step-by-step sequence (Section 5) is concrete and actionable — a competent Rust developer could follow it
70+
- Each step is independently deployable (verified state)
71+
- Acceptance criteria table maps directly to testable criteria with specific test types and locations
72+
- Anti-patterns and boundaries clearly delineate what is NOT changing, reducing implementer uncertainty
73+
- Build/test commands in Step 5 are specific: `cargo build --workspace`, `cargo test --workspace`, `cargo clippy -- -D warnings`
74+
75+
**Weaknesses:**
76+
- The "Step 1" description says "Update `new()` to initialise all new fields to default values" — for `Option` fields this is `None` and for `bool` this is `false`, but this is not explicitly stated, leaving ambiguity about whether the developer should set them explicitly or rely on Rust's default
77+
- Open Questions 1-3 are left as pure questions without even a tentative recommendation, which is less helpful for Phase 3
78+
79+
**Suggested Revisions:**
80+
- [ ] Clarify in Step 1 that new fields are simply omitted from struct literals (relying on `#[serde(default)]`) since Rust will zero-initialise missing struct fields
81+
- [ ] Provide a tentative recommendation for each of the three Open Questions
82+
83+
---
84+
85+
### Social Quality (4/5)
86+
87+
**Strengths:**
88+
- All assumptions made explicit in the plan (top-level fields, four fields, same PR for AutocompleteMetadata)
89+
- Anti-patterns section clearly separates what is NOT in scope, reducing misinterpretation risk
90+
- Builder method names follow existing codebase convention (`with_*`) — no ambiguity about naming
91+
- serde attribute semantics (`skip_serializing_if = "Option::is_none"`) are explained in context in Section 1
92+
93+
**Weaknesses:**
94+
- "Option<T>" serde semantics could be misunderstood by developers unfamiliar with serde — the document assumes familiarity
95+
- "skip_serializing_if = Option::is_none" pattern is used without explicit explanation of what it means for the JSON output (field absent vs. field = null)
96+
97+
**Suggested Revisions:**
98+
- [ ] Add a brief note in Section 1 explaining that `Option` fields are absent from JSON when None (not null), which is the existing pattern already used for `display_value` and `url`
99+
100+
---
101+
102+
### Physical Quality (4/5)
103+
104+
**Strengths:**
105+
- All 8 sections present with clear headers
106+
- Tables used appropriately throughout: AC table (Section 2), components table (Section 3), change plan table (Section 4), step sequence (Section 5), test table (Section 6), risk table (Section 7)
107+
- Appendix with exact struct shapes is a strong addition
108+
- Consistent formatting throughout
109+
- Good use of checkboxes in Section 7 "Anti-Patterns Avoided"
110+
111+
**Weaknesses:**
112+
- None significant
113+
114+
**Suggested Revisions:**
115+
- [ ] None
116+
117+
---
118+
119+
### Empirical Quality (4/5)
120+
121+
**Strengths:**
122+
- Well-chunked: each of the 8 sections is independently navigable
123+
- Step sequence uses bullet points with bold purpose labels — easy to scan
124+
- Tables break up dense content effectively
125+
- The design is concise: 6 pages total including appendix
126+
- Phrasings like "competent Rust developer could follow it" in Section 5 evaluation are honest about the target audience
127+
128+
**Weaknesses:**
129+
- Section 7 risk table with three "Low" entries is repetitive — the three risks are genuinely different but all low, but the table format obscures the distinctions
130+
- Open Questions section is brief — while three focused questions is better than ten, the absence of even tentative recommendations makes it less actionable
131+
132+
**Suggested Revisions:**
133+
- [ ] Convert Section 7 risk table to prose bullets to avoid the repetitive "Low" column
134+
- [ ] Provide a one-sentence tentative recommendation for each Open Question
135+
136+
---
137+
138+
## Revision Checklist
139+
140+
Priority order based on impact:
141+
142+
- **[Low]** Provide tentative recommendation for each Open Question in Section 8 (helps Phase 3)
143+
- **[Low]** Add note in Section 1 that Option fields are absent (not null) in JSON
144+
- **[Low]** Clarify Step 1 struct literal approach for new fields
145+
- **[Trivial]** Convert Section 7 risk table to prose bullets to avoid repetition
146+
147+
---
148+
149+
## Phase 2 Weighted Score Calculation
150+
151+
| Dimension | Score | Weight | Weighted |
152+
|-----------|-------|--------|----------|
153+
| Syntactic | 5 | 1.5 | 7.5 |
154+
| Semantic | 4 | 1.0 | 4.0 |
155+
| Pragmatic | 4 | 1.5 | 6.0 |
156+
| Social | 4 | 1.0 | 4.0 |
157+
| Physical | 4 | 1.0 | 4.0 |
158+
| Empirical | 4 | 1.0 | 4.0 |
159+
| **Total** | | **6.0** | **29.5 / 6.0 = 4.2** |
160+
161+
**Threshold**: Average >= 3.5, no dimension < 3 → **PASS**
162+
163+
---
164+
165+
## Next Steps
166+
167+
**GO**: Design approved for Phase 3. The minor revision suggestions above are all low priority — implementation can proceed. The Open Questions in Section 8 should be resolved before Phase 3 begins, or explicitly deferred with rationale.
168+
169+
Proceed with: `disciplined-implementation` skill, or begin Phase 3 directly if the Open Questions can be resolved.

0 commit comments

Comments
 (0)