[gandazgul]

Summary

• Classify agent-to-client JSON-RPC requests by method and ID before outbound response correlation.
• Return JSON-RPC -32601 for unsupported reverse request methods without removing a same-ID pending OpenAB request.
• Preserve numeric, negative, and string request IDs when replying.
• Add two focused regression tests for same-number collisions and non-u64 request IDs.

This extracts the protocol correctness fix requested in #1533. It does not include elicitation or presentation functionality.

Validation

• cargo check --workspace — passed.
• cargo clippy --workspace -- -D warnings — passed.
• ACP reader-loop tests — 6 passed; test count increased by 2, with no tests removed or replaced.
• cargo test --workspace -- --skip secrets::tests::resolve_exec_nonzero_exit — passed.
• cargo test --workspace — one failure in the unchanged secrets::tests::resolve_exec_nonzero_exit; the same test fails on a clean upstream/main checkout in this environment.
• cargo fmt --all -- --check — reports existing formatting differences; the same command fails on a clean upstream/main checkout.
• git diff --check — passed.

Discord discussion: https://discord.com/channels/1491295327620169908/1491365158868619404/1549133818089316454

Review Contract

Goal

Prevent an agent-to-client JSON-RPC request from being mistaken for an OpenAB request response and consuming the wrong pending response when their IDs collide.

Non-goals

• Add ACP elicitation support.
• Add any Discord form or presentation behavior.
• Handle new agent-to-client methods beyond the existing permission request.

Accepted Residual Risks

Unsupported agent-to-client methods receive JSON-RPC -32601 until OpenAB implements them. This is explicit protocol behavior and does not affect pending outbound requests.

Acceptance Criteria

• A frame with both method and ID is classified as an agent-to-client request before response correlation.
• A reverse request whose numeric ID matches an active OpenAB request does not remove or resolve that pending request.
• Existing session/request_permission requests continue to receive their current automatic response.
• Unsupported reverse requests receive -32601 with the original numeric, negative, or string ID.
• Ordinary numeric response frames continue to resolve and forward the matching pending response.

Follow-ups

Elicitation support and its presentation design remain separate work from #1533 and require the live-wire evidence requested there.

[gandazgul]

Addressed both round-one findings in c600812a:

• F1: JsonRpcId now preserves signed negative IDs, the full u64 range, and string IDs without narrowing casts. Matched responses forward the original ID unchanged.
• F2: Unsupported reverse requests now use a typed JsonRpcErrorResponse. Permission and error responses share one serialization/write/flush helper.

Added 3 regression tests, with no tests removed or replaced:

1. Full signed/unsigned/string ID round-tripping and fractional-ID rejection.
2. A u64::MAX reverse-request collision followed by successful pending-response correlation and subscriber forwarding.
3. Permission responses above i64::MAX.

Validation:

• cargo check --workspace — passed.
• cargo clippy --workspace -- -D warnings — passed.
• Focused new regression tests — passed. Mutation checks confirmed the ID-conversion and subscriber-forwarding assertions fail on incorrect implementations.
• cargo test --workspace — one unchanged failure: secrets::tests::resolve_exec_nonzero_exit; reproduced on untouched main.
• cargo fmt --all -- --check — still reports repository-wide formatting differences; reproduced on untouched main.
• git diff --check — passed.

[chaodu-obk]

Note

LGTM ✅ - Both round-1 findings are fully addressed in c600812a with regression tests that pin the fixes in place.

What This PR Does

Prevents an agent-to-client JSON-RPC request from being misread as a response to an outbound OpenAB request when their IDs collide, which previously consumed the wrong pending-response slot. This is the protocol-correctness portion extracted from #1533; no elicitation or presentation behavior is included.

How It Works

• The ACP reader loop classifies any frame carrying both method and id as an agent-to-client request before outbound response correlation. session/request_permission keeps its auto-response; every other reverse-request method receives a typed JSON-RPC -32601 error echoing the original ID.
• JsonRpcId is now Unsigned(u64) / Signed(i64) / String: the full unsigned 64-bit ID domain parses, negative IDs are preserved, and From<u64> has no narrowing cast. Pending-response correlation operates on as_u64(), and matched or stale frames forward the original ID unchanged via msg.id.clone().
• Both reply paths share one write_json_rpc_reply serialization/write/flush helper, and error replies use the typed JsonRpcErrorResponse (with error.data omitted when absent).

Round-1 Findings Resolution

F1 (round-1): JsonRpcId::Number(i64) narrows the accepted numeric ID domain: IDs in (i64::MAX, u64::MAX] now fail whole-frame deserialization and are silently dropped; From<u64> silently wraps.

✅ Addressed in c600812a: the enum gained an Unsigned(u64) variant tried first during untagged deserialization, Signed(i64) retains negatives, and From<u64> maps to Unsigned with no cast. Verified by json_rpc_ids_preserve_all_supported_integer_ranges (covers 0, i64::MAX, i64::MAX + 1, u64::MAX, -1, i64::MIN, string IDs, and rejects fractional IDs) plus a u64::MAX end-to-end reverse-request/response test and an above-i64::MAX permission-response test.

F2 (round-1): The -32601 reply is hand-rolled json! and the lock/write/flush block is duplicated; extract a typed error response or a shared write helper.

✅ Addressed in c600812a: JsonRpcErrorResponse::new mirrors JsonRpcResponse::new, and both the permission and error paths now call the shared write_json_rpc_reply helper. The raw json! literal and the duplicated write block are gone.

Findings

#	Severity	Finding	Location
1	🟢	Round-1 F1 resolved: full-range JsonRpcId with no narrowing casts, verified across the whole integer domain	crates/openab-core/src/acp/protocol.rs:6
2	🟢	Round-1 F2 resolved: typed JsonRpcErrorResponse plus shared write_json_rpc_reply helper	crates/openab-core/src/acp/protocol.rs:68
3	🟢	Subscriber forwarding clones the original ID instead of reconstructing it, removing a lossy round-trip	crates/openab-core/src/acp/connection.rs:310
4	🟢	New tests exercise boundary IDs end-to-end: u64::MAX collision then successful correlation, above-i64::MAX permission reply, and error.data omission	crates/openab-core/src/acp/connection.rs:1152

Baseline Check

• Round-2 head SHA: c600812a4682ae40e3e29db250b257b902b11da9 (one new commit over round-1 head cd9ef801)
• Base: main (declared and default); merge-base 50424ed461776fc4b817a85ee08052533f511d1e equals the base head, so no drift
• Diff vs base: 3 files, +317/-23; the incremental commit touches only protocol.rs and connection.rs
• Net-new since round-1: Unsigned(u64) ID variant, typed JsonRpcErrorResponse, shared write helper, Serialize for JsonRpcError with data skip, original-ID forwarding, and 3 regression tests (none removed or replaced)
• Verification note: reviewed from a local clone; CI check and validate are green on the head SHA. The review environment has no Rust toolchain, so the author's local test and mutation-check results were not re-run here; the assertions were verified by code reading.

What's Good (🟢)

• Remediation goes beyond the letter of the findings: boundary tests are end-to-end through the real reader loop, not just unit-level serde checks
• Fractional IDs (1.5) are explicitly rejected with a test, documenting the JSON-RPC "SHOULD NOT contain fractional parts" stance
• #[serde(skip_serializing_if = "Option::is_none")] on error.data keeps the wire format minimal, and the test asserts the key is absent

5. Three Reasons We Might Not Need This PR

1. The collision may be unobserved in practice - if current agents never issue reverse requests beyond session/request_permission, the mis-correlation window is narrow. Counterpoint: both sides start ID counters near zero, so collisions become likely as soon as any new reverse request appears.
2. The -32601 auto-reply hard-codes protocol behavior ahead of elicitation - answering a future session/request_input-style method with an error might make some agents abort a turn instead of degrading gracefully. Counterpoint: an unanswered request hangs the agent; an explicit error is the lesser evil and is JSON-RPC-correct.
3. It could land inside the eventual elicitation implementation - merging now creates one more intermediate state. Counterpoint: the correctness fix is independently testable and shrinks the future elicitation PR's review surface.