From 69d1fbca1df937c7e6cf7cdf9e63b1e457c237ba Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 19 Feb 2026 13:12:49 +0000 Subject: [PATCH 1/6] Select task FU-P13-T11: Preserve JSON-RPC numeric request ID fidelity in broker transport --- SPECS/INPROGRESS/next.md | 26 ++++++++++++-------------- SPECS/Workplan.md | 2 +- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/SPECS/INPROGRESS/next.md b/SPECS/INPROGRESS/next.md index 6ab8ffa4..7d3eb392 100644 --- a/SPECS/INPROGRESS/next.md +++ b/SPECS/INPROGRESS/next.md @@ -1,19 +1,17 @@ -# No Active Task +# Next Task: FU-P13-T11 — Preserve JSON-RPC numeric request ID fidelity in broker transport -## Recently Archived +**Priority:** P1 +**Phase:** Phase 13 follow-up (broker transport) +**Effort:** 2h +**Dependencies:** P13-T3 (✅ complete) +**Status:** Selected -- 2026-02-19 — FU-P13-T10: Implement explicit broker daemon entrypoint and operational CLI flows (PASS) -- 2026-02-19 — FU-P12-T3-1: Document unused `error_message` parameter in `MetricsCollector.record_response` (PASS) -- 2026-02-19 — FU-P12-T1-6: Uniform HTML escaping in `renderClientWidgets` (PASS) -- 2026-02-19 — FU-P12-T1-5: Cap `_clients` dict and prune `client_identities` to prevent unbounded growth (PASS) -- 2026-02-19 — FU-P12-T1-4: Make `IN FLIGHT` KPI reflect real in-flight requests in shared-metrics mode (PASS) -- 2026-02-19 — FU-P12-T3-2: Add `error_code` column to audit CSV export (PASS) +## Description -## Suggested Next Tasks +Remove lossy 20-bit integer ID masking in broker request remapping and implement a reversible per-session ID mapping for numeric IDs so all valid JSON-RPC IDs round-trip exactly. -- FU-P13-T11: Preserve JSON-RPC numeric request ID fidelity in broker transport (P1) -- FU-P13-T12: Enforce local Unix-socket security boundary for broker clients (P1) -- FU-P13-T13: Make broker startup transactional when transport bind/start fails (P1) -- FU-P13-T14: Complete interactive Xcode prompt verification and close P13-T5 (P1) +Currently `_process_client_line` applies `original_id & 0xFFFFF` to integer request IDs before encoding them into the broker composite ID. This silently truncates any integer outside the 20-bit range and aliases distinct IDs that share the same lower 20 bits. Negative IDs are also mangled. The fix replaces the bitmask with a per-session incrementing counter (already used for string IDs) and stores a reverse mapping so the response path can restore the exact original value in O(1). -Pending follow-up backlog from review: `4` open tasks. +## Next Step + +Run the PLAN command to generate the implementation-ready PRD. diff --git a/SPECS/Workplan.md b/SPECS/Workplan.md index 937efe0d..979e4686 100644 --- a/SPECS/Workplan.md +++ b/SPECS/Workplan.md @@ -2205,7 +2205,7 @@ Phase 9 Follow-up Backlog --- -#### ⬜️ FU-P13-T11: Preserve JSON-RPC numeric request ID fidelity in broker transport +#### ⬜️ FU-P13-T11: Preserve JSON-RPC numeric request ID fidelity in broker transport **INPROGRESS** - **Description:** Remove lossy 20-bit integer ID masking in broker request remapping and implement a reversible per-session ID mapping for numeric IDs so all valid JSON-RPC IDs round-trip exactly. - **Priority:** P1 - **Dependencies:** P13-T3 From 47a7dc90246c6f4ee1ada3d224d639ed60636fe9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 19 Feb 2026 13:14:06 +0000 Subject: [PATCH 2/6] Plan task FU-P13-T11: Preserve JSON-RPC numeric request ID fidelity in broker transport --- ...request_ID_fidelity_in_broker_transport.md | 145 ++++++++++++++++++ 1 file changed, 145 insertions(+) create mode 100644 SPECS/INPROGRESS/FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport.md diff --git a/SPECS/INPROGRESS/FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport.md b/SPECS/INPROGRESS/FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport.md new file mode 100644 index 00000000..6653d995 --- /dev/null +++ b/SPECS/INPROGRESS/FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport.md @@ -0,0 +1,145 @@ +# PRD: FU-P13-T11 — Preserve JSON-RPC numeric request ID fidelity in broker transport + +**Task ID:** FU-P13-T11 +**Phase:** Phase 13 Follow-up +**Priority:** P1 +**Status:** IN PROGRESS +**Created:** 2026-02-19 + +--- + +## 1. Problem Statement + +`UnixSocketServer._process_client_line` in `broker/transport.py` remaps integer request IDs using a lossy 20-bit bitmask: + +```python +int_id = original_id & _ID_MASK # _ID_MASK = 0xFFFFF (20 bits) +``` + +This causes three correctness failures: + +1. **Truncation**: IDs larger than 1,048,575 (2^20 − 1) silently lose upper bits. +2. **Aliasing**: Two distinct IDs whose lower 20 bits match produce the same `broker_id`, routing responses to the wrong pending future. +3. **Negative IDs**: Python's `& 0xFFFFF` on a negative int produces a positive value, making restoration impossible. + +The response restoration path compounds the problem by performing an O(n) linear scan of `string_id_map` to look up the original string ID and falling back to the truncated `int_local_id` for integers — an asymmetry that masks the bug in small-scale testing. + +--- + +## 2. Deliverables + +| Artifact | Description | +|----------|-------------| +| `src/mcpbridge_wrapper/broker/types.py` | Add `int_id_map`, `id_restore`, `_next_local_id` fields to `ClientSession` | +| `src/mcpbridge_wrapper/broker/transport.py` | Replace lossy mask with reversible per-session counter; O(1) restore; update docstring | +| `tests/unit/test_broker_transport.py` | Update broken tests; add large/negative/concurrent ID tests | + +--- + +## 3. Design + +### 3.1 New `ClientSession` fields + +```python +# Forward maps: original_id → local_seq (existing + new) +string_id_map: dict[str, int] # kept for API compatibility +int_id_map: dict[int, int] # NEW: original int → local_seq + +# Reverse map: local_seq → original_id (NEW, O(1) restore) +id_restore: dict[int, int | str] + +# Shared counter (NEW) +_next_local_id: int = 0 # field(default=0, repr=False) +``` + +A single `_next_local_id` counter is shared across string and integer allocations within a session, preventing cross-type collisions. + +### 3.2 ID allocation helper + +Add a module-level helper in `transport.py`: + +```python +def _alloc_local_id(session: ClientSession) -> int: + session._next_local_id += 1 + if session._next_local_id >= (1 << _SESSION_SHIFT): + session._next_local_id = 1 # wrap (keeps within 20 bits) + return session._next_local_id +``` + +### 3.3 Updated `_process_client_line` remapping + +**String IDs** (extended to populate `id_restore`): +```python +if original_id not in session.string_id_map: + local_int = _alloc_local_id(session) + session.string_id_map[original_id] = local_int + session.id_restore[local_int] = original_id +int_id = session.string_id_map[original_id] +``` + +**Integer IDs** (reversible mapping instead of bitmask): +```python +if original_id not in session.int_id_map: + local_int = _alloc_local_id(session) + session.int_id_map[original_id] = local_int + session.id_restore[local_int] = original_id +int_id = session.int_id_map[original_id] +``` + +### 3.4 Updated response restoration (O(1)) + +Replace the O(n) scan in both `route_upstream_response` and `_drain_session`: + +```python +int_local_id = broker_id & _ID_MASK +original_id: int | str = session.id_restore.get(int_local_id, int_local_id) +``` + +The fallback (`int_local_id`) handles legacy test fixtures that set `pending` directly without going through `_process_client_line`. + +--- + +## 4. Test Plan (test-first) + +### 4.1 Fix existing tests that break + +| Test | Breakage | Fix | +|------|----------|-----| +| `test_integer_id_is_remapped` | Hardcodes `broker_id = (session_id << SHIFT) \| 10` | Check `10 in session.int_id_map`; derive `broker_id` dynamically | +| `test_string_id_restored_from_map` | Sets `string_id_map["req-abc"] = 5` but not `id_restore[5]` | Also set `s.id_restore[5] = "req-abc"` | +| `test_drain_with_string_id_sends_string_in_error` | Same as above | Also set `session.id_restore[3] = "my-req"` | + +### 4.2 New tests + +| Test | Scenario | Assertion | +|------|----------|-----------| +| `test_large_integer_id_round_trips` | Send integer ID `2**21` (> 20-bit) | Response restored with exact original ID | +| `test_negative_integer_id_round_trips` | Send integer ID `-1` | Response restored with `-1` | +| `test_concurrent_int_ids_no_collision` | Send IDs `1` and `1 + 2**20` in same session | Both have distinct `broker_id`s and are independently routable | +| `test_int_and_string_id_no_collision` | Send integer `1` then string ID that gets alias `1` | Counter ensures different local_ints; no aliasing | +| `test_integer_id_reuses_existing_mapping` | Send same integer ID twice | Same `broker_id` used both times (no double allocation) | + +--- + +## 5. Acceptance Criteria + +- [ ] Integer IDs (including negative and > 20-bit) are returned unchanged to clients +- [ ] Distinct concurrent numeric IDs cannot collide within a session +- [ ] Existing string-ID routing behavior remains backward compatible +- [ ] Broker transport tests cover ID round-trip fidelity for int and string IDs +- [ ] `pytest` passes (all tests green) +- [ ] `ruff check src/` passes +- [ ] `mypy src/` passes + +--- + +## 6. Dependencies + +- P13-T3 (UnixSocketServer implementation) ✅ + +--- + +## 7. Out of Scope + +- Growing beyond 2^20 unique in-flight requests per session (counter wrap is acceptable) +- Persistence of session ID state across reconnects From 3e66e266a89e86d430ba06e1ff4a790d4e64d4cb Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 19 Feb 2026 13:20:20 +0000 Subject: [PATCH 3/6] Implement FU-P13-T11: reversible per-session integer ID mapping in broker transport Replace lossy 20-bit bitmask with a shared monotonic counter (_alloc_local_id) and reverse map (id_restore) so large, negative, and concurrent integer request IDs round-trip exactly. Update 3 existing tests and add 5 new tests covering these edge cases. O(1) restore replaces previous O(n) string_id_map scan. --- .../FU-P13-T11_Validation_Report.md | 46 +++++++ src/mcpbridge_wrapper/broker/transport.py | 72 ++++++---- src/mcpbridge_wrapper/broker/types.py | 8 +- tests/unit/test_broker_transport.py | 123 +++++++++++++++++- 4 files changed, 219 insertions(+), 30 deletions(-) create mode 100644 SPECS/INPROGRESS/FU-P13-T11_Validation_Report.md diff --git a/SPECS/INPROGRESS/FU-P13-T11_Validation_Report.md b/SPECS/INPROGRESS/FU-P13-T11_Validation_Report.md new file mode 100644 index 00000000..21a25387 --- /dev/null +++ b/SPECS/INPROGRESS/FU-P13-T11_Validation_Report.md @@ -0,0 +1,46 @@ +# Validation Report: FU-P13-T11 — Preserve JSON-RPC numeric request ID fidelity in broker transport + +**Date:** 2026-02-19 +**Verdict:** PASS + +--- + +## Changes Delivered + +| File | Change | +|------|--------| +| `src/mcpbridge_wrapper/broker/types.py` | Added `int_id_map`, `id_restore`, `_next_local_id` fields to `ClientSession` | +| `src/mcpbridge_wrapper/broker/transport.py` | Added `_alloc_local_id()` helper; replaced lossy `& _ID_MASK` bitmask with reversible per-session counter for integer IDs; replaced O(n) scan with O(1) `id_restore.get()` in `route_upstream_response` and `_drain_session`; updated module docstring | +| `tests/unit/test_broker_transport.py` | Fixed 3 existing tests; added 5 new `TestIntegerIDFidelity` tests | + +--- + +## Quality Gates + +| Gate | Result | +|------|--------| +| `pytest tests/unit/` | ✅ 526 passed, 9 skipped | +| `ruff check src/mcpbridge_wrapper/broker/` | ✅ All checks passed | +| `mypy src/mcpbridge_wrapper/broker/ --ignore-missing-imports` | ✅ Success: no issues found in 5 source files | + +--- + +## Acceptance Criteria + +- [x] Integer IDs (including negative and > 20-bit) are returned unchanged to clients + - Verified by `test_large_integer_id_round_trips` (ID = 2^21) and `test_negative_integer_id_round_trips` (ID = -1) +- [x] Distinct concurrent numeric IDs cannot collide within a session + - Verified by `test_concurrent_int_ids_no_collision` (IDs 1 and 1 + 2^20) +- [x] Existing string-ID routing behavior remains backward compatible + - All existing string-ID tests pass; `string_id_map` field retained +- [x] Broker transport tests cover ID round-trip fidelity for int and string IDs + - `TestIntegerIDFidelity` class added with 5 tests; `test_int_and_string_id_no_collision` covers cross-type safety + +--- + +## Implementation Notes + +- The `_alloc_local_id()` counter is **shared** across string and integer allocations, preventing cross-type collisions (e.g. integer `1` and a string ID cannot receive the same local alias). +- The `id_restore` reverse map enables O(1) restoration in both `route_upstream_response` and `_drain_session`, replacing the previous O(n) linear scans of `string_id_map`. +- The fallback in `id_restore.get(int_local_id, int_local_id)` preserves backward compatibility for legacy test fixtures that directly set `session.pending` without routing through `_process_client_line`. +- Counter wraps at `2^20 - 1` (skipping 0), preserving the 20-bit `broker_id` encoding invariant. diff --git a/src/mcpbridge_wrapper/broker/transport.py b/src/mcpbridge_wrapper/broker/transport.py index fa77636b..b40366e7 100644 --- a/src/mcpbridge_wrapper/broker/transport.py +++ b/src/mcpbridge_wrapper/broker/transport.py @@ -6,13 +6,22 @@ Request ID remapping -------------------- -Outgoing request IDs are namespaced to prevent collisions across clients: +Outgoing request IDs are namespaced to prevent collisions across clients. +Each session maintains a monotonic counter (``ClientSession._next_local_id``) +and two forward maps (``string_id_map``, ``int_id_map``) plus a unified +reverse map (``id_restore``) so every original ID round-trips exactly: - broker_id = (session_id << 20) | (original_id_int & 0xFFFFF) + local_seq = _alloc_local_id(session) # 1 … 2^20-1 + broker_id = (session_id << 20) | local_seq Responses from upstream carry broker_id; the server extracts -``client_id = broker_id >> 20``, restores ``original_id``, and routes -the response back to the correct ClientSession. +``client_id = broker_id >> 20``, restores ``original_id`` via +``session.id_restore[local_seq]`` in O(1), and routes the response back +to the correct ClientSession. + +This design preserves large (> 20-bit), negative, and concurrent integer IDs +without truncation or aliasing. (Replaces the lossy ``original_id & 0xFFFFF`` +mask from P13-T3; see FU-P13-T11.) JSON-RPC notifications (``id == null``) are broadcast to all active clients. @@ -40,6 +49,21 @@ _ID_MASK = (1 << _SESSION_SHIFT) - 1 # 0xFFFFF +def _alloc_local_id(session: ClientSession) -> int: # noqa: F821 + """Allocate the next local sequence ID within *session*'s 20-bit namespace. + + The counter is shared across string and integer ID allocations so that no + two original IDs of any type can receive the same local alias within a + single session. The counter wraps at ``2^_SESSION_SHIFT - 1`` (skipping + 0) rather than at ``2^_SESSION_SHIFT`` so that 0 is reserved for + notifications/null IDs. + """ + session._next_local_id += 1 + if session._next_local_id >= (1 << _SESSION_SHIFT): + session._next_local_id = 1 # wrap, skipping 0 + return session._next_local_id + + class UnixSocketServer: """Accepts and manages local client connections over a Unix domain socket. @@ -153,13 +177,10 @@ async def route_upstream_response(self, line: str) -> None: ) return - # Restore original request ID - original_id: int | str | None = int_local_id - # Check if the original ID was a string - for str_id, mapped_int in session.string_id_map.items(): - if mapped_int == int_local_id: - original_id = str_id - break + # Restore original request ID via O(1) reverse map. + # Fall back to int_local_id for sessions that pre-populated pending + # without going through _process_client_line (e.g. legacy test fixtures). + original_id: int | str | None = session.id_restore.get(int_local_id, int_local_id) # Rebuild the message with the original ID msg["id"] = original_id @@ -290,20 +311,23 @@ async def _process_client_line(self, session: ClientSession, line: str) -> None: ) return - # Remap the request ID + # Remap the request ID using a reversible per-session counter so all + # valid JSON-RPC IDs (large, negative, string) round-trip exactly. original_id = raw_id if isinstance(original_id, str): - # Assign a stable integer alias for string IDs + # Assign a stable local alias for string IDs. if original_id not in session.string_id_map: - # Use a simple incrementing counter within the session's lower bits - existing_ints = set(session.string_id_map.values()) - next_int = 1 - while next_int in existing_ints: - next_int += 1 - session.string_id_map[original_id] = next_int & _ID_MASK + local_int = _alloc_local_id(session) + session.string_id_map[original_id] = local_int + session.id_restore[local_int] = original_id int_id = session.string_id_map[original_id] elif isinstance(original_id, int): - int_id = original_id & _ID_MASK + # Assign a stable local alias for integer IDs (reversible; no bitmask). + if original_id not in session.int_id_map: + local_int = _alloc_local_id(session) + session.int_id_map[original_id] = local_int + session.id_restore[local_int] = original_id + int_id = session.int_id_map[original_id] else: await self._send_parse_error(session, original_id) return @@ -393,13 +417,9 @@ async def _drain_session(self, session: ClientSession) -> None: for broker_id, fut in list(session.pending.items()): if not fut.done(): fut.cancel() - # Determine original_id for the error response + # Restore original_id via O(1) reverse map. int_local_id = broker_id & _ID_MASK - original_id: int | str = int_local_id - for str_id, mapped_int in session.string_id_map.items(): - if mapped_int == int_local_id: - original_id = str_id - break + original_id: int | str = session.id_restore.get(int_local_id, int_local_id) await self._send_error(session, original_id, -32001, "Broker shutting down") session.pending.clear() diff --git a/src/mcpbridge_wrapper/broker/types.py b/src/mcpbridge_wrapper/broker/types.py index 7d0bff04..299fdada 100644 --- a/src/mcpbridge_wrapper/broker/types.py +++ b/src/mcpbridge_wrapper/broker/types.py @@ -73,8 +73,14 @@ class ClientSession: connected_at: float writer: asyncio.StreamWriter pending: dict[int, asyncio.Future] = field(default_factory=dict) - # Maps string original IDs to their integer broker IDs (for string-ID support) + # Maps string original IDs to their local integer alias (for string-ID support) string_id_map: dict[str, int] = field(default_factory=dict) + # Maps integer original IDs to their local integer alias (reversible; FU-P13-T11) + int_id_map: dict[int, int] = field(default_factory=dict) + # Reverse map: local_seq → original_id (int or str) for O(1) restoration + id_restore: dict[int, int | str] = field(default_factory=dict) + # Shared monotonic counter for allocating local alias IDs within this session + _next_local_id: int = field(default=0, repr=False) @dataclass diff --git a/tests/unit/test_broker_transport.py b/tests/unit/test_broker_transport.py index 7d6f901b..0272ea75 100644 --- a/tests/unit/test_broker_transport.py +++ b/tests/unit/test_broker_transport.py @@ -190,6 +190,7 @@ async def test_string_id_restored_from_map(self, tmp_path: Any) -> None: s = _make_session(session_id) # Simulate that "req-abc" was mapped to int alias 5 s.string_id_map["req-abc"] = 5 + s.id_restore[5] = "req-abc" broker_id = (session_id << _SESSION_SHIFT) | 5 loop = asyncio.get_event_loop() fut: asyncio.Future[str] = loop.create_future() @@ -238,9 +239,11 @@ async def test_integer_id_is_remapped(self, tmp_path: Any) -> None: request = json.dumps({"jsonrpc": "2.0", "id": 10, "method": "tools/list"}) await server._process_client_line(session, request) - expected_broker_id = (2 << _SESSION_SHIFT) | 10 - written = session.pending - assert expected_broker_id in written + # Verify a local alias was allocated for the integer ID + assert 10 in session.int_id_map + local_alias = session.int_id_map[10] + expected_broker_id = (2 << _SESSION_SHIFT) | local_alias + assert expected_broker_id in session.pending call_bytes: bytes = server._daemon._upstream.stdin.write.call_args[0][0] sent = json.loads(call_bytes.rstrip(b"\n")) @@ -668,6 +671,7 @@ async def test_drain_with_string_id_sends_string_in_error(self, tmp_path: Any) - server = _make_server(tmp_path) session = _make_session(1) session.string_id_map["my-req"] = 3 + session.id_restore[3] = "my-req" broker_id = (1 << _SESSION_SHIFT) | 3 loop = asyncio.get_event_loop() fut: asyncio.Future[str] = loop.create_future() @@ -697,3 +701,116 @@ async def test_route_response_already_done_future_skipped(self, tmp_path: Any) - response = json.dumps({"jsonrpc": "2.0", "id": broker_id, "result": {}}) # Should not raise InvalidStateError await server.route_upstream_response(response) + + +# --------------------------------------------------------------------------- +# FU-P13-T11 — Reversible per-session integer ID mapping +# --------------------------------------------------------------------------- + + +class TestIntegerIDFidelity: + """Verify that integer request IDs of all shapes round-trip exactly.""" + + @pytest.mark.asyncio + async def test_large_integer_id_round_trips(self, tmp_path: Any) -> None: + """An integer ID larger than 20 bits is preserved exactly.""" + server = _make_server(tmp_path) + session = _make_session(1) + server._sessions[1] = session + + large_id = 2**21 # 2,097,152 — exceeds 20-bit mask + request = json.dumps({"jsonrpc": "2.0", "id": large_id, "method": "tools/list"}) + await server._process_client_line(session, request) + + # Forward map should record it + assert large_id in session.int_id_map + local_alias = session.int_id_map[large_id] + broker_id = (1 << _SESSION_SHIFT) | local_alias + + # Simulate upstream response + resp = json.dumps({"jsonrpc": "2.0", "id": broker_id, "result": {}}) + await server.route_upstream_response(resp) + + call_bytes: bytes = session.writer.write.call_args[0][0] + decoded = json.loads(call_bytes.rstrip(b"\n")) + assert decoded["id"] == large_id + + @pytest.mark.asyncio + async def test_negative_integer_id_round_trips(self, tmp_path: Any) -> None: + """A negative integer ID is preserved exactly (not mangled by bitmask).""" + server = _make_server(tmp_path) + session = _make_session(1) + server._sessions[1] = session + + neg_id = -1 + request = json.dumps({"jsonrpc": "2.0", "id": neg_id, "method": "tools/list"}) + await server._process_client_line(session, request) + + assert neg_id in session.int_id_map + local_alias = session.int_id_map[neg_id] + broker_id = (1 << _SESSION_SHIFT) | local_alias + + resp = json.dumps({"jsonrpc": "2.0", "id": broker_id, "result": {}}) + await server.route_upstream_response(resp) + + call_bytes: bytes = session.writer.write.call_args[0][0] + decoded = json.loads(call_bytes.rstrip(b"\n")) + assert decoded["id"] == neg_id + + @pytest.mark.asyncio + async def test_concurrent_int_ids_no_collision(self, tmp_path: Any) -> None: + """Two integer IDs whose lower 20 bits match get distinct broker IDs.""" + server = _make_server(tmp_path) + session = _make_session(1) + server._sessions[1] = session + + id_a = 1 + id_b = 1 + (1 << _SESSION_SHIFT) # same lower 20 bits as 1 + + req_a = json.dumps({"jsonrpc": "2.0", "id": id_a, "method": "tools/list"}) + req_b = json.dumps({"jsonrpc": "2.0", "id": id_b, "method": "tools/list"}) + await server._process_client_line(session, req_a) + await server._process_client_line(session, req_b) + + alias_a = session.int_id_map[id_a] + alias_b = session.int_id_map[id_b] + assert alias_a != alias_b, "distinct original IDs must get distinct local aliases" + + broker_a = (1 << _SESSION_SHIFT) | alias_a + broker_b = (1 << _SESSION_SHIFT) | alias_b + assert broker_a != broker_b + + @pytest.mark.asyncio + async def test_integer_id_reuses_existing_mapping(self, tmp_path: Any) -> None: + """Sending the same integer ID twice reuses the same local alias.""" + server = _make_server(tmp_path) + session = _make_session(1) + server._sessions[1] = session + + request = json.dumps({"jsonrpc": "2.0", "id": 42, "method": "tools/list"}) + await server._process_client_line(session, request) + alias_first = session.int_id_map.get(42) + + await server._process_client_line(session, request) + alias_second = session.int_id_map.get(42) + + assert alias_first == alias_second + assert alias_first is not None + + @pytest.mark.asyncio + async def test_int_and_string_id_no_collision(self, tmp_path: Any) -> None: + """Integer 1 and a string ID do not receive the same local alias.""" + server = _make_server(tmp_path) + session = _make_session(1) + server._sessions[1] = session + + req_int = json.dumps({"jsonrpc": "2.0", "id": 1, "method": "tools/list"}) + req_str = json.dumps({"jsonrpc": "2.0", "id": "call-1", "method": "tools/list"}) + await server._process_client_line(session, req_int) + await server._process_client_line(session, req_str) + + int_alias = session.int_id_map.get(1) + str_alias = session.string_id_map.get("call-1") + assert int_alias is not None + assert str_alias is not None + assert int_alias != str_alias, "int and string IDs must not share a local alias" From adc9d7aa006c522f772d38f95c3c60dadf598d6f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 19 Feb 2026 13:22:06 +0000 Subject: [PATCH 4/6] Archive task FU-P13-T11: Preserve JSON-RPC numeric request ID fidelity in broker transport (PASS) --- ...request_ID_fidelity_in_broker_transport.md | 4 +++ .../FU-P13-T11_Validation_Report.md | 0 SPECS/ARCHIVE/INDEX.md | 4 ++- SPECS/INPROGRESS/next.md | 26 ++++++++++--------- SPECS/Workplan.md | 15 ++++++----- 5 files changed, 29 insertions(+), 20 deletions(-) rename SPECS/{INPROGRESS => ARCHIVE/FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport}/FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport.md (99%) rename SPECS/{INPROGRESS => ARCHIVE/FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport}/FU-P13-T11_Validation_Report.md (100%) diff --git a/SPECS/INPROGRESS/FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport.md b/SPECS/ARCHIVE/FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport/FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport.md similarity index 99% rename from SPECS/INPROGRESS/FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport.md rename to SPECS/ARCHIVE/FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport/FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport.md index 6653d995..17462f2c 100644 --- a/SPECS/INPROGRESS/FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport.md +++ b/SPECS/ARCHIVE/FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport/FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport.md @@ -143,3 +143,7 @@ The fallback (`int_local_id`) handles legacy test fixtures that set `pending` di - Growing beyond 2^20 unique in-flight requests per session (counter wrap is acceptable) - Persistence of session ID state across reconnects + +--- +**Archived:** 2026-02-19 +**Verdict:** PASS diff --git a/SPECS/INPROGRESS/FU-P13-T11_Validation_Report.md b/SPECS/ARCHIVE/FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport/FU-P13-T11_Validation_Report.md similarity index 100% rename from SPECS/INPROGRESS/FU-P13-T11_Validation_Report.md rename to SPECS/ARCHIVE/FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport/FU-P13-T11_Validation_Report.md diff --git a/SPECS/ARCHIVE/INDEX.md b/SPECS/ARCHIVE/INDEX.md index 5ecf2e12..ce6bf983 100644 --- a/SPECS/ARCHIVE/INDEX.md +++ b/SPECS/ARCHIVE/INDEX.md @@ -1,6 +1,6 @@ # mcpbridge-wrapper Tasks Archive -**Last Updated:** 2026-02-19 (FU-P13-T10_broker_daemon_entrypoint) +**Last Updated:** 2026-02-19 (FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport) ## Archived Tasks @@ -124,6 +124,7 @@ | FU-P12-T3-1 | [FU-P12-T3-1_Document_unused_error_message_parameter_in_MetricsCollector_record_response/](FU-P12-T3-1_Document_unused_error_message_parameter_in_MetricsCollector_record_response/) | 2026-02-19 | PASS | | FU-P12-T3-2 | [FU-P12-T3-2_Add_error_code_column_to_audit_CSV_export/](FU-P12-T3-2_Add_error_code_column_to_audit_CSV_export/) | 2026-02-19 | PASS | | FU-P13-T10 | [FU-P13-T10_broker_daemon_entrypoint/](FU-P13-T10_broker_daemon_entrypoint/) | 2026-02-19 | PASS | +| FU-P13-T11 | [FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport/](FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport/) | 2026-02-19 | PASS | ## Historical Artifacts @@ -378,3 +379,4 @@ | 2026-02-19 | FU-P12-T3-2 | Archived REVIEW_FU-P12-T3-2_error_code_csv_export report | | 2026-02-19 | FU-P13-T10 | Archived broker_daemon_entrypoint (PASS) | | 2026-02-19 | FU-P13-T10 | Archived REVIEW_FU-P13-T10_broker_daemon_entrypoint report | +| 2026-02-19 | FU-P13-T11 | Archived Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport (PASS) | diff --git a/SPECS/INPROGRESS/next.md b/SPECS/INPROGRESS/next.md index 7d3eb392..3b146948 100644 --- a/SPECS/INPROGRESS/next.md +++ b/SPECS/INPROGRESS/next.md @@ -1,17 +1,19 @@ -# Next Task: FU-P13-T11 — Preserve JSON-RPC numeric request ID fidelity in broker transport +# No Active Task -**Priority:** P1 -**Phase:** Phase 13 follow-up (broker transport) -**Effort:** 2h -**Dependencies:** P13-T3 (✅ complete) -**Status:** Selected +## Recently Archived -## Description +- 2026-02-19 — FU-P13-T11: Preserve JSON-RPC numeric request ID fidelity in broker transport (PASS) +- 2026-02-19 — FU-P13-T10: Implement explicit broker daemon entrypoint and operational CLI flows (PASS) +- 2026-02-19 — FU-P12-T3-1: Document unused `error_message` parameter in `MetricsCollector.record_response` (PASS) +- 2026-02-19 — FU-P12-T1-6: Uniform HTML escaping in `renderClientWidgets` (PASS) +- 2026-02-19 — FU-P12-T1-5: Cap `_clients` dict and prune `client_identities` to prevent unbounded growth (PASS) +- 2026-02-19 — FU-P12-T1-4: Make `IN FLIGHT` KPI reflect real in-flight requests in shared-metrics mode (PASS) +- 2026-02-19 — FU-P12-T3-2: Add `error_code` column to audit CSV export (PASS) -Remove lossy 20-bit integer ID masking in broker request remapping and implement a reversible per-session ID mapping for numeric IDs so all valid JSON-RPC IDs round-trip exactly. +## Suggested Next Tasks -Currently `_process_client_line` applies `original_id & 0xFFFFF` to integer request IDs before encoding them into the broker composite ID. This silently truncates any integer outside the 20-bit range and aliases distinct IDs that share the same lower 20 bits. Negative IDs are also mangled. The fix replaces the bitmask with a per-session incrementing counter (already used for string IDs) and stores a reverse mapping so the response path can restore the exact original value in O(1). +- FU-P13-T12: Enforce local Unix-socket security boundary for broker clients (P1) +- FU-P13-T13: Make broker startup transactional when transport bind/start fails (P1) +- FU-P13-T14: Complete interactive Xcode prompt verification and close P13-T5 (P1) -## Next Step - -Run the PLAN command to generate the implementation-ready PRD. +Pending follow-up backlog from review: `3` open tasks. diff --git a/SPECS/Workplan.md b/SPECS/Workplan.md index 979e4686..1d34f6e2 100644 --- a/SPECS/Workplan.md +++ b/SPECS/Workplan.md @@ -2205,20 +2205,21 @@ Phase 9 Follow-up Backlog --- -#### ⬜️ FU-P13-T11: Preserve JSON-RPC numeric request ID fidelity in broker transport **INPROGRESS** -- **Description:** Remove lossy 20-bit integer ID masking in broker request remapping and implement a reversible per-session ID mapping for numeric IDs so all valid JSON-RPC IDs round-trip exactly. +#### ✅ FU-P13-T11: Preserve JSON-RPC numeric request ID fidelity in broker transport +- **Status:** ✅ Completed (2026-02-19) +- **Description:** Replaced lossy 20-bit integer ID bitmask with a reversible per-session counter (`_alloc_local_id`) and reverse map (`id_restore`). Large, negative, and concurrent integer IDs now round-trip exactly. O(1) restore replaces previous O(n) scan. - **Priority:** P1 - **Dependencies:** P13-T3 - **Parallelizable:** yes - **Outputs/Artifacts:** - Updated `src/mcpbridge_wrapper/broker/transport.py` ID remap/restore strategy - - Updated `src/mcpbridge_wrapper/broker/types.py` session mapping fields (if required) + - Updated `src/mcpbridge_wrapper/broker/types.py` session mapping fields - New/updated unit tests for large, negative, and concurrent numeric IDs - **Acceptance Criteria:** - - [ ] Integer IDs (including negative and > 20-bit) are returned unchanged to clients - - [ ] Distinct concurrent numeric IDs cannot collide within a session - - [ ] Existing string-ID routing behavior remains backward compatible - - [ ] Broker transport tests cover ID round-trip fidelity for int and string IDs + - [x] Integer IDs (including negative and > 20-bit) are returned unchanged to clients + - [x] Distinct concurrent numeric IDs cannot collide within a session + - [x] Existing string-ID routing behavior remains backward compatible + - [x] Broker transport tests cover ID round-trip fidelity for int and string IDs --- From 14ff5c6df1a4dc54e5b9010a900fc35512e64bb3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 19 Feb 2026 13:22:58 +0000 Subject: [PATCH 5/6] Review FU-P13-T11: integer ID fidelity in broker transport --- .../REVIEW_FU-P13-T11_integer_id_fidelity.md | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 SPECS/INPROGRESS/REVIEW_FU-P13-T11_integer_id_fidelity.md diff --git a/SPECS/INPROGRESS/REVIEW_FU-P13-T11_integer_id_fidelity.md b/SPECS/INPROGRESS/REVIEW_FU-P13-T11_integer_id_fidelity.md new file mode 100644 index 00000000..50b0b485 --- /dev/null +++ b/SPECS/INPROGRESS/REVIEW_FU-P13-T11_integer_id_fidelity.md @@ -0,0 +1,60 @@ +## REVIEW REPORT — FU-P13-T11: Preserve JSON-RPC numeric request ID fidelity in broker transport + +**Scope:** commits 69d1fbc..adc9d7a (branch `claude/implement-flow-specs-Ry4XN`) +**Files:** 5 (types.py, transport.py, test_broker_transport.py, Workplan.md, INDEX.md + next.md) + +--- + +### Summary Verdict + +- [ ] Approve +- [x] Approve with comments +- [ ] Request changes +- [ ] Block + +--- + +### Critical Issues + +None. All correctness requirements from the task spec are met. + +--- + +### Secondary Issues + +**[Low] `# noqa: F821` comment left on `_alloc_local_id`** + +`transport.py:52` has `def _alloc_local_id(session: ClientSession) -> int: # noqa: F821` after ruff auto-fixed the type annotation. Since `ClientSession` is explicitly imported at the top of the module, `F821` (undefined name) can never fire here. The comment is harmless but misleading and should be removed in a future cleanup pass. + +**[Low] `_next_local_id` field appears in `ClientSession.__init__` signature** + +The field is declared as `field(default=0, repr=False)` which means `init=True` — callers can accidentally pass `_next_local_id=n` during construction. Using `field(default=0, init=False, repr=False)` would make it strictly internal. This is a minor API hygiene issue; the current form is still safe since tests don't exploit it. + +--- + +### Architectural Notes + +1. **Counter wrap**: `_alloc_local_id` wraps at `2^20 - 1` ≈ 1M IDs. A single MCP session sending > 1M distinct IDs would start recycling aliases. In practice this is not a concern for MCP workloads, but a session-level guard (e.g. capping `int_id_map` and `string_id_map`) would prevent memory growth for long-lived broker sessions. This is a separate concern from the bug fixed here. + +2. **Fallback in `id_restore.get()`**: The fallback `int_local_id` silently returns an integer for any pending entry that bypassed `_process_client_line`. This is intentional to preserve compatibility with test fixtures that set `pending` directly, but it means a programming error (failing to go through the proper path) would silently produce a wrong ID in production. An explicit log warning at DEBUG level when the key is missing could aid future debugging. + +3. **`string_id_map` kept for compatibility**: The existing `string_id_map` field is retained. It still serves as the forward map for string IDs, but the reverse lookup path now goes exclusively through `id_restore`. This creates mild redundancy (string IDs appear in both `string_id_map` and `id_restore`). A future cleanup could unify into a single forward map `id_map: dict[int | str, int]`. + +--- + +### Tests + +- 526 unit tests pass, 9 skipped — no regressions. +- 5 new tests in `TestIntegerIDFidelity` cover all stated acceptance criteria. +- `test_string_id_reuses_existing_alias` continues to pass, confirming backward compatibility for string IDs. +- The `test_drain_with_string_id_sends_string_in_error` test now explicitly populates `id_restore`, making the test setup more explicit and accurate. + +--- + +### Next Steps + +No blockers. The two Low findings can be addressed in a future cleanup task if desired: + +- FU-P13-T11-1 (optional): Remove `# noqa: F821` comment from `_alloc_local_id` and change `_next_local_id` to `field(default=0, init=False, repr=False)`. + +FOLLOW-UP is **not required** — the issues found are cosmetic/stylistic and do not affect correctness or safety. From 2be7a4a9dbf8ce95cb7ae3b4ce914c7f77071640 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 19 Feb 2026 17:57:38 +0000 Subject: [PATCH 6/6] Archive REVIEW_FU-P13-T11_integer_id_fidelity report --- SPECS/ARCHIVE/INDEX.md | 2 ++ .../_Historical}/REVIEW_FU-P13-T11_integer_id_fidelity.md | 0 2 files changed, 2 insertions(+) rename SPECS/{INPROGRESS => ARCHIVE/_Historical}/REVIEW_FU-P13-T11_integer_id_fidelity.md (100%) diff --git a/SPECS/ARCHIVE/INDEX.md b/SPECS/ARCHIVE/INDEX.md index ce6bf983..aa9db1b8 100644 --- a/SPECS/ARCHIVE/INDEX.md +++ b/SPECS/ARCHIVE/INDEX.md @@ -213,6 +213,7 @@ | [REVIEW_FU-P12-T3-1_error_message_docstring.md](_Historical/REVIEW_FU-P12-T3-1_error_message_docstring.md) | Review report for FU-P12-T3-1 | | [REVIEW_FU-P12-T3-2_error_code_csv_export.md](_Historical/REVIEW_FU-P12-T3-2_error_code_csv_export.md) | Review report for FU-P12-T3-2 | | [REVIEW_FU-P13-T10_broker_daemon_entrypoint.md](_Historical/REVIEW_FU-P13-T10_broker_daemon_entrypoint.md) | Review report for FU-P13-T10 | +| [REVIEW_FU-P13-T11_integer_id_fidelity.md](_Historical/REVIEW_FU-P13-T11_integer_id_fidelity.md) | Review report for FU-P13-T11 | ## Archive Log @@ -380,3 +381,4 @@ | 2026-02-19 | FU-P13-T10 | Archived broker_daemon_entrypoint (PASS) | | 2026-02-19 | FU-P13-T10 | Archived REVIEW_FU-P13-T10_broker_daemon_entrypoint report | | 2026-02-19 | FU-P13-T11 | Archived Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport (PASS) | +| 2026-02-19 | FU-P13-T11 | Archived REVIEW_FU-P13-T11_integer_id_fidelity report | diff --git a/SPECS/INPROGRESS/REVIEW_FU-P13-T11_integer_id_fidelity.md b/SPECS/ARCHIVE/_Historical/REVIEW_FU-P13-T11_integer_id_fidelity.md similarity index 100% rename from SPECS/INPROGRESS/REVIEW_FU-P13-T11_integer_id_fidelity.md rename to SPECS/ARCHIVE/_Historical/REVIEW_FU-P13-T11_integer_id_fidelity.md