Select task FU-P13-T11: Preserve JSON-RPC numeric request ID fidelity in broker transport

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

@@ -0,0 +1,149 @@
+# 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
+
+---
+**Archived:** 2026-02-19
+**Verdict:** PASS

SPECS/ARCHIVE/FU-P13-T11_Preserve_JSON-RPC_numeric_request_ID_fidelity_in_broker_transport/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.

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
 
@@ -212,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
 
@@ -378,3 +380,5 @@
 | 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) |
+| 2026-02-19 | FU-P13-T11 | Archived REVIEW_FU-P13-T11_integer_id_fidelity report |

SPECS/ARCHIVE/_Historical/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.

SPECS/INPROGRESS/next.md

@@ -2,6 +2,7 @@
 
 ## Recently Archived
 
+- 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)
@@ -11,9 +12,8 @@
 
 ## Suggested Next Tasks
 
-- 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)
 
-Pending follow-up backlog from review: `4` open tasks.
+Pending follow-up backlog from review: `3` open tasks.

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
-- **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
 
 ---