fix(#5542): de-brittle SSE-contract RFC anchors — cite by symbol, not line number - #5569
Conversation
The RFC (docs/rfcs/session-sse-contract-v1.md) cited hardcoded absolute line numbers like `api/routes.py:12345-12346` and `api/routes.py:16177`, and tests/test_issue4812_session_sse_contract_rfc.py validated those exact lines against source. Any PR that shifted routes.py lines (e.g. #5543, #5534) broke the test — a chronic brittle-failure class (#5542). Root fix: anchor by SYMBOL, never by line number. RFC: - Strip every `api/routes.py:<NNNN>` / `<NNNN>-<NNNN>` and `api/streaming.py:<NNNN>` line-number suffix; keep the symbol name (route string, function/constant names) + the file, e.g. "_handle_session_events_stream() in api/routes.py". - Update the inventory note to state names are the stable anchors and the RFC deliberately avoids line numbers so a source-layout shift can't invalidate the doc or its contract test. Test: - Rewrite the two line-anchor tests to assert (a) each cited symbol still exists in api/routes.py and (b) the RFC names that symbol — dropping the `api/routes.py:<line>` regex bounds-checks entirely. Invariant preserved: "the RFC's cited symbols are real and named in the doc", NOT "the cited line numbers are exact". - Add test_rfc_uses_no_hardcoded_routes_line_numbers as a regression guard that fails if any `*.py:<line>` anchor is reintroduced into the RFC. Proven de-brittled: prepending 31 lines to api/routes.py (scratch, reverted) leaves all 34 tests green. Refs #5542, #5513
|
| Filename | Overview |
|---|---|
| docs/rfcs/session-sse-contract-v1.md | All line-number anchors removed and replaced with symbol/file-level references; the rationale is self-documenting in the updated prose. |
| tests/test_issue4812_session_sse_contract_rfc.py | Two tests refactored to symbol-based probes; new regression guard test added to prevent line-number reintroduction; logic is correct and the regex guard covers all cited file types. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[RFC cites symbol/route name] --> B{Symbol exists in api/routes.py?}
B -- yes --> C{RFC prose names the symbol?}
B -- no --> FAIL1[Test FAIL: symbol removed from source]
C -- yes --> D{Any *.py:line anchors in RFC?}
C -- no --> FAIL2[Test FAIL: RFC dropped symbol name]
D -- none --> PASS[All 3 tests PASS]
D -- found --> FAIL3[Regression guard FAIL: line number re-introduced]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[RFC cites symbol/route name] --> B{Symbol exists in api/routes.py?}
B -- yes --> C{RFC prose names the symbol?}
B -- no --> FAIL1[Test FAIL: symbol removed from source]
C -- yes --> D{Any *.py:line anchors in RFC?}
C -- no --> FAIL2[Test FAIL: RFC dropped symbol name]
D -- none --> PASS[All 3 tests PASS]
D -- found --> FAIL3[Regression guard FAIL: line number re-introduced]
Reviews (2): Last reviewed commit: "Merge remote-tracking branch 'origin/mas..." | Re-trigger Greptile
| ("_parse_run_journal_after_seq", "def _parse_run_journal_after_seq"), | ||
| ("_runner_event_id", "def _runner_event_id"), | ||
| ("_replay_run_journal", "def _replay_run_journal"), | ||
| ("_sse_with_id", "_sse_with_id"), |
There was a problem hiding this comment.
_sse_with_id probe is inconsistent with the other four checks
The four sibling entries all use "def <symbol>" as source_probe, so they verify a definition exists in api/routes.py. The _sse_with_id entry uses just "_sse_with_id", which also matches call sites, string literals, or comments — meaning the check still passes if the helper is removed but its name survives in a comment or import string. If this is intentional (e.g., _sse_with_id is defined elsewhere and only called from routes.py), a brief inline comment explaining that would prevent future maintainers from "fixing" it to "def _sse_with_id" and introducing a spurious failure.
Summary
De-brittles the session-SSE-contract RFC's source anchors so a
api/routes.pyline-shift can never again break an unrelated PR's CI.The problem (#5542): the RFC (
docs/rfcs/session-sse-contract-v1.md, merged via #5513) pinned source anchors asapi/routes.py:NNNNline numbers, andtests/test_issue4812_session_sse_contract_rfc.pyasserted those exact lines contain the cited symbols. Any PR that inserts/removes lines above a pinned anchor shifts the numbers and red-fails the RFC test on a branch that's otherwise clean — confirmed collateral on #5534 (CORS, +16 lines) and again on #5553 (the #5532 P0 fix, +46 lines). Line numbers on this codebase drift constantly (release-driven), so this recurred on essentially every routes.py PR.The fix (root cause): cite by symbol / route name, verified by existence, never by line number.
/api/sessions/events, the handler_handle_session_events_stream, and the run-journal symbols (_parse_run_journal_event_id,_parse_run_journal_after_seq,_runner_event_id,_replay_run_journal,_sse_with_id) by name — no*.py:NNNNanchors remain (0 left, verified).test_rfc_cites_current_global_endpoint_sourceandtest_rfc_run_journal_anchors_land_on_real_sourcenow assert each cited symbol (a) still exists inapi/routes.pyand (b) is named in the RFC prose — a real accuracy check that's immune to line-shifts.test_rfc_uses_no_hardcoded_routes_line_numbers: fails if anyone ever re-introduces a*.py:<line>anchor into the RFC, so the brittleness can't come back.This preserves the original tests' intent (the RFC's source references must be accurate, not rotted) while removing the line-number coupling that caused the churn.
Tests
tests/test_issue4812_session_sse_contract_rfc.py— 34 pass. The two former line-anchor tests are now symbol-based; the anti-regression guard confirms the RFC carries zero hardcoded source line numbers.Note on ordering vs #5553
#5553 (the #5532 P0 fix) currently realigns these same RFC line numbers to survive its +46-line shift. Whichever merges first: if this lands first, #5553 should drop its now-obsolete RFC-anchor realignment (the anchors no longer exist); if #5553 lands first, this cleanly removes the line numbers it realigned. No code conflict either way — both only touch the RFC's citation style.
Closes #5542.