|
| 1 | +# Plan: HINTS-STRING-REMOVAL |
| 2 | + |
| 3 | +Status: **active (planning)**. This plan implements |
| 4 | +[`propose/HINTS-STRING-REMOVAL-PROPOSE.md`](../propose/HINTS-STRING-REMOVAL-PROPOSE.md) |
| 5 | +as a single PR. |
| 6 | + |
| 7 | +Depends on: none (all prerequisite work — `hints_structured` with `label` — is landed). |
| 8 | + |
| 9 | +## Goal |
| 10 | + |
| 11 | +- Remove the redundant `hints: list[str]` field from all five MCP tool output models. |
| 12 | +- Consolidate advisory text into a new `reason: str` field on `StructuredHint`. |
| 13 | +- Eliminate dual-emission maintenance burden and the parity test. |
| 14 | + |
| 15 | +## Principles (do not relitigate in review) |
| 16 | + |
| 17 | +- **Single hint mechanism**: `hints_structured` is the only hint field after this change. |
| 18 | +- **`reason` defaults to `""`**: backward-compatible; actionable hints may have an empty reason (they already carry `tool` + `args`). Non-actionable hints **should** carry a reason — enforced at test time, not by schema. |
| 19 | +- **Breaking change is allowed**: AGENTS.md and the proposal explicitly state breaking changes are always allowed; no deprecation cycle. |
| 20 | +- **No re-index, no ontology bump**: this is an output-only change. |
| 21 | + |
| 22 | +## PR breakdown - overview |
| 23 | + |
| 24 | +| PR | Scope | Ontology bump | Areas of concern | Test buckets | Independent of | |
| 25 | +| --- | --- | --- | --- | --- | --- | |
| 26 | +| PR-1 | Remove `hints` field, add `reason` to `StructuredHint`, remove string templates and parity test, update docs | none | `generate_hints` return type change ripples to all five tool handlers; test assertions migrating from `out.hints` to `out.hints_structured[i].reason`; `server.py` tool descriptions referencing `hints` | hint unit tests, parity removal, round-trip integration, docs consistency | n/a | |
| 27 | + |
| 28 | +Landing order: **PR-1 only** (single PR). |
| 29 | + |
| 30 | +## Resolved design decisions |
| 31 | + |
| 32 | +| Topic | Decision | |
| 33 | +| --- | --- | |
| 34 | +| `reason` default | `reason: str = ""` — avoids forcing a reason on every hint; non-actionable hints are expected to carry one (test-enforced). | |
| 35 | +| String template deletion | Delete entirely; git history preserves them. | |
| 36 | +| `generate_hints` return type | `list[_StructuredHint]` — no tuple wrapper. | |
| 37 | +| `_to_structured_hints` conversion | Must forward `reason` from internal `_StructuredHint` to public `StructuredHint`. | |
| 38 | +| `finalize_hint_list` | Delete — no longer needed. | |
| 39 | + |
| 40 | +--- |
| 41 | + |
| 42 | +# PR-1 — Remove string hints, add reason to StructuredHint |
| 43 | + |
| 44 | +## File-by-file changes |
| 45 | + |
| 46 | +### 1. `mcp_hints.py` |
| 47 | + |
| 48 | +- Add `reason: str = ""` field to `_StructuredHint` NamedTuple. |
| 49 | +- Remove `MCP_HINTS_FIELD_DESCRIPTION` constant. |
| 50 | +- Remove `finalize_hint_list` function. |
| 51 | +- Remove all `TPL_*` string template constants. |
| 52 | +- Change `generate_hints` signature: return `list[_StructuredHint]` instead of `tuple[list[str], list[_StructuredHint]]`. |
| 53 | +- Remove all `pairs` / `finalize_hint_list` calls throughout `generate_hints`; keep only `struct_pairs` logic. |
| 54 | +- For each hint that previously appended to `pairs`, extract the advisory text into a `reason=` kwarg on the corresponding `_StructuredHint`. |
| 55 | + - Example: the search weak-results hint currently does `pairs.append((PRIORITY_META, TPL_SEARCH_WEAK))` and `struct_pairs.append(_StructuredHint("find", ..., LABEL_WEAK_RESULTS))`. After change: `struct_pairs.append(_StructuredHint("find", ..., LABEL_WEAK_RESULTS, reason="results look weak — narrow the query or try find with a role filter"))`. |
| 56 | + - The `reason` text should be derived from the former template rendering (substitute any format variables as needed). |
| 57 | +- Remove `_hints` helper and `_filter_neighbors_dotkey_hints` function (both were string-hint-specific). |
| 58 | +- Remove `_FIND_SUCCESS_MAX_CHARS`, `_RESOLVE_HINT_MAX_CHARS`, `_NEIGHBORS_SUCCESS_MAX_CHARS` cap constants (they applied to rendered string length; structured hints use the cap from `finalize_structured_hints`). |
| 59 | +- Remove `_append_find_success_hint` and `_append_neighbors_success_hint` helpers. |
| 60 | +- Remove `find_success_hints` function (string-only); keep the structured-hint emission logic inline in `generate_hints`. |
| 61 | +- Remove `neighbors_success_hints` function (string-only); keep `_neighbors_success_structured_hints`. |
| 62 | +- Remove `neighbors_empty_hints` function (string-only); keep `_neighbors_empty_structured_hints`. |
| 63 | +- Remove `neighbors_calls_fanout_hints` and `neighbors_calls_meta_hints` (string-only); keep the structured meta hint logic inline in `generate_hints`. |
| 64 | +- Remove `_FIRST_NEIGHBORS_CALL_RE`, `_parse_first_traversal` — these were used to parse string templates into structured hint args; now unnecessary. |
| 65 | +- Remove `LABEL_*` constants that were only used in structured hints — keep them (they're still used for `_StructuredHint.label`). |
| 66 | + |
| 67 | +### 2. `mcp_v2.py` |
| 68 | + |
| 69 | +- Add `reason: str = ""` field to `StructuredHint` model. |
| 70 | +- Remove `hints: list[str]` field from `SearchOutput`, `FindOutput`, `DescribeOutput`, `NeighborsOutput`, `ResolveOutput`. |
| 71 | +- Remove `MCP_HINTS_FIELD_DESCRIPTION` import from `mcp_hints`. |
| 72 | +- Update `_to_structured_hints` to forward `reason`. |
| 73 | +- Update all five tool functions (`search_v2`, `find_v2`, `describe_v2`, `neighbors_v2`, `resolve_v2`): |
| 74 | + - Change `str_hints, raw_struct = generate_hints(...)` to `raw_struct = generate_hints(...)`. |
| 75 | + - Remove `hints=str_hints` from all `*Output(...)` constructor calls. |
| 76 | + - Remove `hints=[]` from all error-path `*Output(...)` constructor calls. |
| 77 | +- Remove all `hints=[]` from early-return error paths in `search_v2`, `find_v2`, `describe_v2`, `neighbors_v2`, `resolve_v2`, `_resolve_finalize_success`. |
| 78 | + |
| 79 | +### 3. `server.py` |
| 80 | + |
| 81 | +- Update MCP tool `_INSTRUCTIONS` / description strings that reference `"hints"`: |
| 82 | + - Line 344: change `hints` (advisory next-step strings) to `hints_structured` (advisory next-step objects with tool, args, actionable, label, reason). |
| 83 | + - Line 393: same. |
| 84 | + - Line 429: same. |
| 85 | + - Lines 463-464: same. |
| 86 | + - Line 549: same. |
| 87 | + |
| 88 | +### 4. `tests/test_mcp_hints.py` |
| 89 | + |
| 90 | +- Remove `test_structured_hints_parity_with_string_hints` (line 2589–2638). |
| 91 | +- Remove `_hints` helper function (line 41-43) — it returns string hints only. |
| 92 | +- Remove `_structural_neighbors_hints` helper (lines 426-433) — it filters string hints. |
| 93 | +- Migrate all tests that assert on `out.hints` (string form) to assert on `out.hints_structured` instead: |
| 94 | + - Tests checking for template presence in `out.hints` → check `any(h.reason and "<substring>" in h.reason for h in out.hints_structured)`. |
| 95 | + - Tests checking `out.hints == []` → check `out.hints_structured == []` (or equivalently, no hint with a specific tool). |
| 96 | + - Tests using `_hints(output_kind, payload)` → use `_struct(output_kind, payload)` instead. |
| 97 | +- Update `test_hints_clean_outputs_empty` to assert on `hints_structured`. |
| 98 | +- Remove string-template length tests (`test_hints_all_v4_templates_under_120_chars`, `test_hints_template_rendered_length_leq_120`) — these validated string template rendering; replace with a test that all `reason` strings are ≤ 120 chars. |
| 99 | +- Remove `test_hints_dedupe_collapses_identical_rendered_strings`, `test_hints_cap_drops_lowest_priority_over_five`, `test_hints_cap_same_priority_keeps_emission_order` — these tested `finalize_hint_list` which is deleted; the structured equivalents already exist (`test_structured_hints_dedup`, `test_structured_hints_cap_5`). |
| 100 | +- Update `_resolve_finalize_success` in `mcp_v2.py` to remove `hints=str_hints` from `model_copy(update=...)`. |
| 101 | + |
| 102 | +### 5. `docs/AGENT-GUIDE.md` |
| 103 | + |
| 104 | +- Update line 19 to remove reference to `hints` list and describe only `hints_structured`. |
| 105 | +- Document the `reason` field on structured hints. |
| 106 | + |
| 107 | +### 6. `README.md` |
| 108 | + |
| 109 | +- No changes needed (README does not detail the `hints` field explicitly; the five-tool table links to AGENT-GUIDE). |
| 110 | + |
| 111 | +## Tests for PR-1 |
| 112 | + |
| 113 | +1. `test_structured_hints_cap_5` — existing, unchanged. |
| 114 | +2. `test_structured_hints_dedup` — existing, unchanged. |
| 115 | +3. `test_structured_hint_round_trip` — existing, updated to remove `out.hints` assertions. |
| 116 | +4. `test_structured_hint_label_values` — existing, unchanged. |
| 117 | +5. All `test_hints_*` tests — migrated from `out.hints` to `out.hints_structured[*].reason`. |
| 118 | +6. `test_structured_hints_reason_content` — new: verify `reason` field carries expected text for key scenarios (describe type rollup, describe method overriders, search weak, find empty, resolve none, neighbors empty structural). |
| 119 | +7. `test_structured_hints_reason_char_cap` — new: verify all emitted `reason` strings are ≤ 120 chars. |
| 120 | +8. `test_no_string_hints_field` — new: verify `SearchOutput`, `FindOutput`, `DescribeOutput`, `NeighborsOutput`, `ResolveOutput` have no `hints` field. |
| 121 | + |
| 122 | +## Definition of done (PR-1) |
| 123 | + |
| 124 | +- `hints: list[str]` field absent from all five output models. |
| 125 | +- `reason: str` field present on `StructuredHint` and `_StructuredHint`. |
| 126 | +- `generate_hints` returns `list[_StructuredHint]`, not a tuple. |
| 127 | +- `MCP_HINTS_FIELD_DESCRIPTION`, all `TPL_*` constants, `finalize_hint_list` removed from `mcp_hints.py`. |
| 128 | +- `server.py` tool descriptions reference `hints_structured`, not `hints`. |
| 129 | +- Full test suite passes: `.venv/bin/python -m pytest tests -v`. |
| 130 | +- Ruff clean: `.venv/bin/ruff check .`. |
| 131 | +- No references to `hints: list[str]` in AGENT-GUIDE or server descriptions. |
| 132 | + |
| 133 | +## Implementation step list |
| 134 | + |
| 135 | +| # | Step | File(s) | Done when | |
| 136 | +| - | - | - | - | |
| 137 | +| 1 | Add `reason` to `_StructuredHint`, change `generate_hints` return type | `mcp_hints.py` | `generate_hints` returns `list[_StructuredHint]`; all callers updated in same step | |
| 138 | +| 2 | Remove `MCP_HINTS_FIELD_DESCRIPTION`, all `TPL_*`, `finalize_hint_list`, string-only helpers | `mcp_hints.py` | No string template constants or string-only functions remain | |
| 139 | +| 3 | Add `reason` to public `StructuredHint`, remove `hints` from all 5 output models | `mcp_v2.py` | All `*Output` models lack `hints` field; `_to_structured_hints` forwards `reason` | |
| 140 | +| 4 | Update all 5 tool functions to use new `generate_hints` signature | `mcp_v2.py` | No `str_hints` variable; no `hints=str_hints` in any constructor | |
| 141 | +| 5 | Update `server.py` tool descriptions | `server.py` | All descriptions say `hints_structured` | |
| 142 | +| 6 | Migrate test assertions from `out.hints` to `out.hints_structured` | `tests/test_mcp_hints.py` | All tests pass | |
| 143 | +| 7 | Remove parity test, string-only test helpers | `tests/test_mcp_hints.py` | `test_structured_hints_parity_with_string_hints` removed; `_hints` helper removed | |
| 144 | +| 8 | Update `docs/AGENT-GUIDE.md` | `docs/AGENT-GUIDE.md` | No `hints` list reference; `hints_structured` documented with `reason` | |
| 145 | +| 9 | Run full validation | all | `ruff check .` clean; `pytest tests -v` passes | |
| 146 | + |
| 147 | +--- |
| 148 | + |
| 149 | +# Cross-PR risks and mitigations |
| 150 | + |
| 151 | +| # | Risk | Severity | Mitigation | |
| 152 | +| --- | --- | --- | --- | |
| 153 | +| 1 | Downstream consumers expecting `hints` field | low | Breaking changes are always allowed per AGENTS.md; `hints_structured` has been present since #209 | |
| 154 | +| 2 | Reason text divergence from former templates | medium | Derive reason strings from the same template renderings at authoring time; verify via new `reason` content tests | |
| 155 | + |
| 156 | +# Out of scope |
| 157 | + |
| 158 | +- Changing hint trigger logic or priority tiers. |
| 159 | +- Adding new hint categories or triggers. |
| 160 | +- Modifying the `_StructuredHint.priority` field semantics. |
| 161 | +- Changing the `label` field behavior. |
| 162 | +- Adding `reason` to `EDGE_SCHEMA` or any graph-layer changes. |
| 163 | +- Index rebuild or ontology bump. |
| 164 | + |
| 165 | +# Whole-plan done definition |
| 166 | + |
| 167 | +1. `hints: list[str]` no longer exists on any output model. |
| 168 | +2. All advisory text lives in `reason` on `StructuredHint`. |
| 169 | +3. Test suite passes with zero references to the old `hints` field. |
| 170 | +4. `ruff check .` clean. |
| 171 | + |
| 172 | +# Tracking |
| 173 | + |
| 174 | +- `PR-1`: _pending_ |
0 commit comments