Skip to content

fix PR-E1 follow-up contract and docs#19

Merged
HumanBean17 merged 2 commits into
masterfrom
cursor/pr-e1-followups
May 5, 2026
Merged

fix PR-E1 follow-up contract and docs#19
HumanBean17 merged 2 commits into
masterfrom
cursor/pr-e1-followups

Conversation

@HumanBean17
Copy link
Copy Markdown
Owner

Summary

  • restore analyze_pr risk-score normalization to keep risk_score in [0,1], and add tests for saturation and +0.2 single-caller bump behavior
  • rename ontology match constant to VALID_CALL_MATCHES while keeping VALID_HTTP_CALL_MATCHES as a compatibility alias, and update pass6 match usage/comments
  • document PR-E1 follow-up behavior in README.md, server.py tool description, and the completed Tier1B proposal schema table (channel)

Test plan

  • ruff check .
  • pytest tests/test_pr_analysis.py -q
  • pytest tests -v

Made with Cursor

Restore analyze_pr risk_score normalization to [0,1], rename call-match ontology constant with alias compatibility, and document deferred Tier 1B behavior updates from the PR-E1 plan.

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

Review: PR-E1 — post-Tier-1B follow-ups (items 1-6)

Verdict: Approved ✅

All 6 plan items land verbatim with the contract fix in item 1 implemented exactly as recommended (re-normalise via + (cross_service_bonus / 5.0) then clamp). The new tests test_35a (saturation) and test_35b (single-caller +0.2) target the contract directly, the VALID_HTTP_CALL_MATCHES alias is a true identity (is check passes), and reproducing pass6 on cross_service_smoke shows zero behavioural drift from the PR-D3 baseline. Scope is tight and item 7 is correctly deferred to PR-E2.

Scope discipline (out-of-scope checks)

Sentinel Status
ONTOLOGY_VERSION / CREATE NODE TABLE / CREATE REL TABLE / DROP TABLE ✅ 0 hits — no schema or version changes
Changes outside expected set (README.md, build_ast_graph.py, java_ontology.py, pr_analysis.py, server.py, tests/test_pr_analysis.py, completed propose) ✅ 0 — only the listed files touched
graph_enrich.py:720-724 (item 7 / PR-E2 territory) ✅ untouched
Drive-by import cleanup or unrelated lint ✅ 0 — every diff line maps to a plan item

Plan compliance (plans/PLAN-POST-TIER1B-FOLLOWUPS.md)

# Plan item Verified
1 risk_score re-normalisation [0,1] (recommended option a) pr_analysis.py:476score = max(0.0, min(1.0, raw + (cross_service_bonus / 5.0)))
1 Tests for saturation + single-caller bump test_35a (6 callers → 1.0), test_35b (1 caller → +0.2 vs baseline)
2 Rename VALID_HTTP_CALL_MATCHESVALID_CALL_MATCHES with deprecation alias java_ontology.py:75-83 (rename + VALID_HTTP_CALL_MATCHES = VALID_CALL_MATCHES alias, both exported in __all__)
2 pass6 callsites use new name build_ast_graph.py:27, 53, 62 (import + 2 callsites)
3 pass6_match_edges reset idempotency comment build_ast_graph.py:1641-1643 ("Pass 6 is idempotent for full rebuilds … this reset must remain pass-scoped" — explicit incremental-rebuild note)
4 Reader comment on feign branch in _match_call_edge build_ast_graph.py:1591 ("Both sides must provide a non-empty Feign name; empty values are unresolved.")
5 README — anchor-fills-from-builtin behaviour README.md — paragraph on partial overrides being non-destructive (tightening, not replacing)
6 TIER1B propose §6 — add channel to OutgoingCallDecl propose/completed/TIER1B-...md schema table — new row with type and rationale
Bonus server.py analyze_pr description updated to advertise risk_score ([0,1]) ✅ explicit type/range in tool description — surfaces the contract to MCP clients

Item 7 (strategy-ladder consolidation in graph_enrich.py:720-724) is correctly out of scope — PR-E2 is the right home.

Tests

tests/test_pr_analysis.py:                                         11 passed in 3.39s
tests (excluding tests/test_indexer.py — slow ML deps unrelated): 262 passed, 4 skipped in 49.93s
total:                                                             262 passed, 4 skipped

Delta from PR-D3 (260 passed, 4 skipped) is +2 — exactly the new test_35a + test_35b.

Manual evidence reproduced

Built tests/fixtures/cross_service_smoke to verify pass6 outcomes are unchanged after the rename:

http_calls_match_breakdown: {'ambiguous': 1, 'cross_service': 1, 'intra_service': 1, 'phantom': 1, 'unresolved': 1}
async_calls_match_breakdown: {'cross_service': 1}
cross_service_calls_total: 2

Identical to PR-D3's baseline — the alias is fully transparent at runtime.

Alias check:

>>> VALID_HTTP_CALL_MATCHES is VALID_CALL_MATCHES
True

risk_band thresholds (<0.3 low, <0.7 medium, >=0.7 high) at pr_analysis.py:477-482 are unchanged — they were always [0,1]-shaped, so the re-normalisation restores their original semantics rather than requiring a re-tune. Good.

Notes that earned my trust

  1. Item 1 implemented as option (a) — re-normalise — exactly as the plan recommended, not the alternative "accept new range and re-document". Test 35a's monkeypatched _FakeGraph returns range(6) (6 callers) so it exercises the saturation strictly above the plan's +5.0 cap, not at it — that's the right boundary to assert.
  2. Test 35b uses delta against baseline rather than asserting an absolute float (rep.risk_score - baseline.risk_score == 0.2). This is the right shape — it survives changes to the underlying raw formula without the test rotting. The two compute_risk calls share the same fixture except for the boolean flag controlling caller presence, so the delta is only the bonus.
  3. The deprecation alias is a true identity (VALID_HTTP_CALL_MATCHES = VALID_CALL_MATCHES, not frozenset(VALID_CALL_MATCHES)). Anything doing is-comparison or id() checks against the old name still works. Belt-and-suspenders: both names exported in __all__.
  4. server.py analyze_pr description was updated to surface risk_score ([0,1]) — small but important touch. MCP clients reading the tool description now see the contract; this prevents the same surprise later.
  5. Item 3's idempotency comment names "incremental rebuild" explicitly — exactly the breadcrumb the future Tier-2 reader needs (and aligns with the open Tier-2 proposal in PR propose: Tier 2 \u2014 incremental Kuzu rebuild #18).

Observations (non-blocking)

  1. compute_risk's raw formula isn't shown in the diff — I assumed raw is unchanged from before PR-D3 (PR-D3 only added the cross_service_bonus term). Worth a quick mental confirmation when you read the diff: the + (cross_service_bonus / 5.0) term must be the only change to the score expression for the single-caller delta in test 35b to be exactly 0.2. Test 35b asserts equality (not approximation) so the math has to land precisely. Since the test passes, this is verified empirically — but it's worth noting that any future change to raw could break the test in a misleading way (the test would surface as "bonus formula broken" when it's actually "raw formula changed").
  2. Test 35b uses two separate compute_risk invocations on a stateless _FakeGraph(include_callers=…). That's fine, but it doubles the test's dependency on _FakeGraph correctness. A slightly tighter version would call compute_risk once with callers, capture score_with, then mutate _FakeGraph._include_callers = False and call again — same delta, half the fixture surface. Cosmetic; current shape is more readable.
  3. The channel row in the TIER1B propose schema table uses "http" / "async" (string literals) rather than the Literal["http", "async"] type that OutgoingCallDecl actually carries. Faithful to the table's existing column-style; just noting in case the file ever gets converted to executable schema.
  4. Item 5's README paragraph could be even more explicit — it correctly says partial overrides "tighten" rather than "replace", but doesn't give a concrete before/after example. A 3-line example (@RestController + @CodebaseEndpoint(path="/x") → final path="/x", method=GET from built-in) would land it harder. Pure doc polish; non-blocking.
  5. The pass6 idempotency comment lives on lines 1641-1643, immediately above the clear() calls. That's the right spot. One thing it could add: a pointer to the open Tier-2 proposal (propose/TIER2-INCREMENTAL-REBUILD-PROPOSE.md §3.6) so the next reader who lands here while implementing incremental rebuild has a direct breadcrumb. Don't block PR-E1 for this — handle it in PR-T1 of the Tier-2 plan when that lands.

Plan deltas needed

None — every plan item from PLAN-POST-TIER1B-FOLLOWUPS.md items 1-6 ships as specified. Item 7 (PR-E2) remains correctly outstanding.


Ready to merge. After this lands, the follow-ups plan can mark items 1-6 ✅ shipped in PR-E1; only item 7 (graph_enrich.py:720-724 strategy-ladder consolidation) remains for PR-E2.

Add a concrete partial-override README example, tighten the risk delta test to isolate raw terms, and annotate pass6 idempotency with Tier-2 incremental-rebuild context.

Co-authored-by: Cursor <cursoragent@cursor.com>
@HumanBean17 HumanBean17 merged commit 4571205 into master May 5, 2026
HumanBean17 added a commit that referenced this pull request May 5, 2026
Captures the deferred catch from the PR-E1 review reply (#19): pass3
doesn't enforce that CALLS edges stay within a single microservice.
Today's clean state on cross_service_smoke (9 CALLS edges, 0 cross)
is incidental \u2014 no caller.microservice == callee.microservice guard
exists. FQN collisions across services or future brownfield supertype
overrides could break the invariant silently.

PR-E3 spec:
- ~10-line guard in pass3_calls candidate-emit branches
- new CallResolutionStats.skipped_cross_service counter
- new graph_meta.pass3_skipped_cross_service field
- new fqn_collision_smoke fixture
- one test asserting guard fires + zero cross-service CALLS edges
- existing fixtures continue to show counter == 0

Also flips the status header to reflect PR-E1 shipped + PR-E2/PR-E3
outstanding, and bumps the PR-boundaries section.
@HumanBean17 HumanBean17 deleted the cursor/pr-e1-followups branch May 10, 2026 21:18
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