Skip to content

refactor: consolidate strategy ladder for PR-E2#20

Merged
HumanBean17 merged 2 commits into
masterfrom
cursor/pr-e2-strategy-ladder
May 5, 2026
Merged

refactor: consolidate strategy ladder for PR-E2#20
HumanBean17 merged 2 commits into
masterfrom
cursor/pr-e2-strategy-ladder

Conversation

@HumanBean17
Copy link
Copy Markdown
Owner

Summary

  • Consolidate the annotation/spel/constant_ref route-path ladder in graph_enrich.py behind a single helper used by layer-A route hint extraction.
  • Add a sentinel regression test that asserts graph_enrich.py contains exactly one strategy-ladder pattern to prevent future drift.
  • Mark item 7 as shipped in plans/PLAN-POST-TIER1B-FOLLOWUPS.md for PR-E2 tracking.

Test plan

  • ruff check graph_enrich.py tests/test_call_edge_matching.py
  • pytest tests/test_call_edge_matching.py -v
  • pytest tests -q

Plan reference

  • plans/PLAN-POST-TIER1B-FOLLOWUPS.md (PR-E2 — Strategy-ladder consolidation)

Made with Cursor

Remove the duplicated annotation/spel/constant_ref branching in graph_enrich and add a sentinel regression test so the resolver ladder stays unified going forward.

Co-authored-by: Cursor <cursoragent@cursor.com>
@HumanBean17
Copy link
Copy Markdown
Owner Author

Review: PR-E2 — Strategy Ladder Consolidation

Verdict: Approved ✅

Tight, surgical refactor exactly as planned. The annotation/spel/constant_ref ladder is now extracted into one tiny helper (_route_path_atom, 7 lines) and the four enums + three confidence values live in exactly one place. Plan-mandated risk check (rebuild bank-chat-system, diff EXPOSES rows) confirms zero behavioural drift — same 11 edges, same (annotation:9, constant_ref:1, spel:1) distribution, byte-identical attribute hashes.

Scope discipline (out-of-scope checks)

Sentinel Status
ONTOLOGY_VERSION / SCHEMA_VERSION ✅ 0 hits
CREATE NODE TABLE / CREATE REL TABLE / DROP TABLE ✅ 0 hits
@mcp.tool (no new tool registrations) ✅ 0 hits
skipped_cross_service / fqn_collision (PR-E3 territory) ✅ 0 hits
Files touched ✅ exactly 3: graph_enrich.py (+13/-8), the plan, the test
Other spel/constant_ref literals in graph_enrich.py ✅ 0 outside the helper (lines 789/792/794 only)
Drive-by lint / unrelated edits ✅ none

Plan compliance (vs plans/PLAN-POST-TIER1B-FOLLOWUPS.md §PR-E2)

# Step from plan Verified
1 Extract _choose_route_strategy helper returning (path, strategy, confidence, anchor) ✅ shipped as _route_path_atom at graph_enrich.py:788-794 (4-tuple shape matches the spec exactly, name is fine)
2 Replace inline ladder in _http_paths_from_ann_ref graph_enrich.py:840-853 now calls the helper twice (loop body + default fallback)
3 Preserve confidence values 1.0 / 0.85 / 0.7 ✅ verbatim
4 Default fallback equivalent to ("", "annotation", 1.0, True) _route_path_atom("", "string") traces to that exact 4-tuple — branch returns raw_value, "annotation", 1.0, True because "" not in "${"
5 Sentinel test asserts only one ladder occurrence tests/test_call_edge_matching.py::test_graph_enrich_has_single_route_strategy_ladder — regex matches the helper's docstring comment exactly once
6 Mark plan item 7 as shipped ✅ done in the diff

Tests

263 passed, 4 skipped in 45.47s

+1 vs PR-E1 baseline (262), as expected (the new sentinel test). Focused run: tests/test_call_edge_matching.py 10 passed (was 9).

Manual evidence reproduced (plan-mandated risk check)

Built bank-chat-system on master and on the PR branch, then queried EXPOSES rows directly:

MASTER: count=11 by_strategy={'annotation': 9, 'constant_ref': 1, 'spel': 1}
MASTER: rows_hash=c5d3d1c44600b231
PR-E2 : count=11 by_strategy={'annotation': 9, 'constant_ref': 1, 'spel': 1}
PR-E2 : rows_hash=c5d3d1c44600b231

Byte-identical hashes over (path, strategy, confidence, framework) tuples. All three ladder branches are exercised by the fixture, so this is a real parity check, not a vacuous one. pass4 summary line is also identical: emitted=11, exposes=11, routes_resolved_pct=81.8, by_framework={'spring_mvc': 9, 'kafka': 2}.

Notes that earned my trust

  • Helper signature returns bool for "anchor" instead of recomputing it at the call site. The previous inline code had three separate False/True literals scattered across branches; centralising them removes the chance of a future diff getting one out of sync.
  • _HTTP_ROUTE_KINDS introduced as a module-level frozenset right above the helper even though it's not used in this PR. Tiny piece of forward-compat hygiene that hints at the same constant getting used in _http_paths_from_ann_decl too — exactly the kind of thing the plan called out as "Phase 2 cleanup". Not flagged because it's unreferenced and harmless.
  • Default-fallback case routed through the helper instead of being open-coded. The naive refactor would have left out.append(("", "annotation", 1.0, True)) literal at the bottom; routing it through _route_path_atom("", "string") means future changes to the "empty path is annotation" semantic only need editing in one place.
  • Sentinel regex is a regex over file content, not over AST/imports. That's intentional and correct — the threat model is "someone copy-pastes the three-step comment block into another helper", which a string-level check catches but a structural check would miss.

Observations (non-blocking)

  1. Helper name _route_path_atom ≠ plan name _choose_route_strategy. The new name is arguably better (it returns a path atom, not just a strategy) but a one-line plan delta would close the doc-vs-code drift. Suggest updating the plan section header rather than renaming the function.
  2. The value_kind == "string" branch swallows two semantically distinct cases (empty "" and a real literal) into the same "annotation" strategy via the default fallback. This matches the prior behaviour exactly, so it's not a regression — but if a future PR ever wants to distinguish "no value present" from "value present and resolved", the helper will need a third value_kind. Worth a one-line comment if you're touching the helper anyway.
  3. No companion sentinel guards the _async_topics_from_ann_ref cousin (line ~870ish if memory serves). If an identical ladder exists for async topics, it's still inline and the consolidation only solved half the problem. Not blocking PR-E2, but worth a quick grep -n "spel" graph_enrich.py next time you're in the file. (Quick check while reviewing: only the helper's three lines mention spel/constant_ref, so async paths must use a different shape — confirmed not a regression.)
  4. Sentinel regex uses .* (greedy, single-line) which is fine for the current file but would silently miss a duplicate that spans a newline. If you ever want to harden it, re.findall(r"annotation.*spel.*constant_ref", source, re.DOTALL) would catch multi-line duplicates too. Trade-off: false positives go up.

Plan deltas needed

Only the cosmetic helper-name doc fix in observation #1. No semantic deltas.


Ready to merge. Next: PR-E3 (intra-JVM CALLS invariant guard) per plans/PLAN-POST-TIER1B-FOLLOWUPS.md §item-8 — that's the last follow-up before the plan moves to plans/completed/.

Document the `_route_path_atom` naming in the PR-E2 plan and clarify empty-string vs no-value semantics in the helper comment for future refactors.

Co-authored-by: Cursor <cursoragent@cursor.com>
@HumanBean17 HumanBean17 merged commit d8a0d12 into master May 5, 2026
@HumanBean17 HumanBean17 deleted the cursor/pr-e2-strategy-ladder branch May 10, 2026 21:18
HumanBean17 added a commit that referenced this pull request May 16, 2026
* update completed propose and plan docs to shipped status

Align status lines in propose/completed/ and plans/completed/ with
landed work; note PR-E2 strategy-ladder consolidation as deferred.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix post-tier1b plan status — PR-E2 shipped in #20

Correct earlier housekeeping note; strategy-ladder consolidation
landed with _route_path_atom and sentinel test. Mark all three
follow-up PRs shipped in the boundaries section.

Co-authored-by: Cursor <cursoragent@cursor.com>

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant