test: cover plan, tool_call, ext, and protocol_level regression risks - #4
Conversation
Adds unit tests for Plan / PlanEntry in both v1 and v2 covering: - snake_case wire format for PlanEntryPriority and PlanEntryStatus - PlanEntry round-trip with optional _meta omitted - Plan preserving _meta when set - Plan tolerating outer-shape errors on `entries` (null / string / object / number) via DefaultOnError - Plan skipping individual malformed entries via VecSkipError while keeping valid ones - the required-field contract on `entries` (missing key still errors) The resilient deserialization was introduced in agentclientprotocol#1006 but had no direct coverage on the protocol-level Plan type, which is a core session/update payload. Co-authored-by: QuantuM <qumusai@proton.me>
Adds unit tests for tool_call.rs in both v1 and v2 covering the most load-bearing logic in this file, which previously had zero direct tests: - defaults for ToolKind / ToolCallStatus skip serialization - ToolKind unknown variants decode to Other via #[serde(other)] - ToolCallStatus has no catch-all (documented via the update test) - ToolCall round-trip preserves every populated field - ToolCall::update only replaces present fields (others untouched) - ToolCall::update replaces collections rather than extending them - TryFrom<ToolCallUpdate> for ToolCall requires title and surfaces a stable error message + InvalidParams code that clients display - TryFrom fills defaults for missing optional fields - From<ToolCall> for ToolCallUpdate preserves all fields (round trip with TryFrom rebuilds the original) - ToolCallUpdate flattens fields onto the wire next to toolCallId - ToolCallContent uses snake_case discriminator on "type" with content / diff / terminal variants - malformed elements in content/locations are skipped per the DefaultOnError<VecSkipError> guarantees (resilient deserialization) - ToolCallUpdateFields tolerates unknown kind/status values - Diff::new leaves old_text unset and omits it from the wire - From<ContentBlock> wraps in Content variant; From<Diff> -> Diff These behaviors directly affect agent ↔ client communication for tool execution — the largest blast radius surface in the protocol — so locking them down prevents silent regressions in update merging or shape-tolerant parsing. Co-authored-by: QuantuM <qumusai@proton.me>
…fication ext.rs (v1 + v2) — previously no direct tests: - ExtRequest::new preserves the method name verbatim from any Into<Arc<str>>, including reusing an existing Arc<str> without reallocation. Protects the fix from agentclientprotocol#883 (preserve '_' prefix) at the type level even after the Side/decode_request layer moved into the SDK in agentclientprotocol#1009. - ExtRequest, ExtNotification, and ExtResponse all serialize as the raw params payload only — the method name must never leak into the serialized JSON, otherwise custom RPC handlers would receive unexpected fields. - ExtResponse::from(Arc<RawValue>) is exercised. protocol_level.rs (v1 + v2) — also previously untested: - CANCEL_REQUEST_METHOD_NAME stays "$/cancel_request" and is re-exported via PROTOCOL_LEVEL_METHOD_NAMES; this is a wire-format contract a casual rename would break. - ProtocolLevelNotification::method() returns the right name for the cancel variant. - CancelRequestNotification round-trips with and without _meta and works for every RequestId shape (null / number / string). Co-authored-by: QuantuM <qumusai@proton.me>
- collapse a long let-binding onto one line (rustfmt)
- drop unneeded raw-string hashes around `raw("42")` (clippy
needless_raw_string_hashes)
No behavior change.
Co-authored-by: QuantuM <qumusai@proton.me>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_91a5f40e-c0f7-492d-9ce3-fa8fb215f183) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_2902aa92-200b-4b8d-bdc4-3d4cb34be7b3) |
There was a problem hiding this comment.
Pull request overview
This PR adds direct unit-test coverage for several protocol “hot path” schema modules in both v1 and v2, primarily locking in serde wire-format behavior and update/merge semantics to reduce regression risk.
Changes:
- Add comprehensive
#[cfg(test)]modules forplan,tool_call,ext, andprotocol_levelin bothsrc/v1/andsrc/v2/. - Assert resilience behaviors (
DefaultOnError+VecSkipError), default-skipping, flattening, and conversion/update invariants. - Reformat the “Key commands” table in
AGENTS.md.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/v1/plan.rs | Adds tests for resilient entries parsing, snake_case enums, and _meta serialization behavior. |
| src/v2/plan.rs | Mirrors v1 plan tests for v2 types/wire format. |
| src/v1/tool_call.rs | Adds extensive tests covering default omission, update semantics, flattening, resilient list parsing, and conversions. |
| src/v2/tool_call.rs | Mirrors v1 tool_call tests for v2 types/wire format and invariants. |
| src/v1/ext.rs | Adds tests ensuring extension payloads serialize transparently and method names don’t leak. |
| src/v2/ext.rs | Mirrors v1 ext tests for v2 types/wire format. |
| src/v1/protocol_level.rs | Adds cancel-request wire-format tests (currently missing required feature gating). |
| src/v2/protocol_level.rs | Mirrors v1 protocol_level tests (currently missing required feature gating). |
| AGENTS.md | Reflows the key-commands table for readability. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
Risky behavior now covered
This PR closes meaningful regression gaps in four files that previously had zero direct unit tests (only indirect coverage through v1↔v2 conversion round-trips). All four sit on hot paths in the protocol:
plan.rs(v1 + v2)Plan::entriesresilient deserialization (introduced in feat(rust-only): better tolerate malformed optional fields in deserialization agentclientprotocol/agent-client-protocol#1006) had no direct test on the protocol-level type. New tests prove:PlanEntrydoes not poison a whole plan update (DefaultOnError<VecSkipError<_, SkipListener>>).entries(null / string / object / number) collapse to an empty vec.entrieskey is still required if missing (intentional, documented in the test).PlanEntryPriority/PlanEntryStatussnake_case wire format is locked in._metaround-trips when set and is omitted when unset.tool_call.rs(v1 + v2)The largest blast-radius file in the schema crate (674 lines, 0 tests). New coverage:
ToolKind::Other/ToolCallStatus::Pendingdefaults are skipped from the wire (is_default).ToolKind#[serde(other)]decodes unknown variants toOther— protects forward compatibility with future spec revisions.ToolCallStatushas noothercatch-all (locked in via the update-fields test whereDefaultOnErrorcollapses unknown statuses toNone).ToolCall::update()merge semantics: only present fields overwrite; collections (content/locations) are replaced, not extended — this is easy to silently break.TryFrom<ToolCallUpdate> for ToolCallrequirestitleand surfaces a stable, actionable error message +InvalidParamscode.From<ToolCall> for ToolCallUpdatepreserves every field, includingraw_input/raw_output— theToolCall → ToolCallUpdate → ToolCallround-trip is asserted.ToolCallUpdateflattens fields next totoolCallIdon the wire.ToolCallContentdiscriminator istypewith snake_casecontent/diff/terminal.content/locations(mirrorsPlan::entries).ext.rs(v1 + v2)ExtRequest::newpreserves the method name verbatim from anyInto<Arc<str>>and reuses an existingArc<str>without reallocating. Protects the fix from fix(rpc): preserve '_' prefix for extension methods and reject empty ext agentclientprotocol/agent-client-protocol#883 at the type level even after theSide/decode_requestmachinery moved into the SDK in feat(rust-only): Remove unused RPC message schema types (schema.json unchanged) agentclientprotocol/agent-client-protocol#1009.ExtRequest,ExtNotification, andExtResponseall serialize as the raw params payload only — the method name must never leak into the JSON, otherwise custom RPC handlers would receive unexpected fields.protocol_level.rs(v1 + v2)CANCEL_REQUEST_METHOD_NAMEstays"$/cancel_request"and is re-exported viaPROTOCOL_LEVEL_METHOD_NAMES. Renaming this is a wire-format break.ProtocolLevelNotification::method()returns the right name for the cancel variant.CancelRequestNotificationround-trips with and without_metaand for everyRequestIdshape (null / number / string).Test files added/updated
src/v1/plan.rs— newtestsmodule (5 tests)src/v2/plan.rs— newtestsmodule (5 tests)src/v1/tool_call.rs— newtestsmodule (16 tests)src/v2/tool_call.rs— newtestsmodule (16 tests)src/v1/ext.rs— newtestsmodule (4 tests)src/v2/ext.rs— newtestsmodule (4 tests)src/v1/protocol_level.rs— newtestsmodule (5 tests)src/v2/protocol_level.rs— newtestsmodule (5 tests)Why these tests materially reduce regression risk
All four files contain logic that is invisible at compile time but corrupts wire-protocol behavior if broken:
DefaultOnError<VecSkipError<_, SkipListener>>) is purely a serde annotation. Dropping or rewriting these attributes silently flips the schema from forgiving to strict, breaking any client that sends a slightly-malformed list element — exactly the case the pattern was introduced to handle (feat(rust-only): better tolerate malformed optional fields in deserialization agentclientprotocol/agent-client-protocol#1006).ToolCall::updatemerge semantics are easy to "simplify" in ways that change behavior (e.g. extending vs replacing collections, treatingNoneas clear-the-field). A regression here corrupts every tool-call stream.TryFrom<ToolCallUpdate>requiringtitleis the contract every agent relies on when reconstructing a missed tool-call event from a later update.#[serde(skip)]onmethod,#[serde(transparent)]on the struct) is invisible to type-checking. Any change that lets the method name leak into params breaks downstream extension handlers — exactly the regression fix(rpc): preserve '_' prefix for extension methods and reject empty ext agentclientprotocol/agent-client-protocol#883 fixed.$/cancel_requestmethod name is a wire-format string with no compile-time validation. The test makes accidental renames a test failure rather than a runtime mystery.Validation
cargo test --all-features: 320 passed (was 260 before this PR — 60 new tests).cargo test --lib --no-default-features: 66 passed.cargo test --lib --features unstable_protocol_v2: 124 passed.cargo clippy --all-features --all-targets -- -D warnings: clean.cargo fmt --check: clean.npm run spellcheck: clean.npx prettier --check: only flagsAGENTS.md, which is pre-existing and unrelated to this PR (introduced in 2691825).Tests are deterministic, hermetic (no I/O, no time, no thread-locals), and follow the existing
#[cfg(test)] mod tests+serde_json::to_value/from_valueround-trip convention used elsewhere in the crate.Summary by cubic
Add unit tests for v1 and v2 protocol hot paths (
plan,tool_call,ext,protocol_level) to lock in wire format and update behavior, reducing regression risk.Covers resilient list parsing, default-skipping for
ToolKind/ToolCallStatus,ToolCall::updatereplacing collections,TryFrom<ToolCallUpdate>requiringtitlewithInvalidParams,From<ToolCall>preserving fields, transparent extension RPC payloads, and the stable$/cancel_requestmethod withRequestIdround-trips; also tidiesAGENTS.mdkey commands table formatting.Written for commit 3c07b86. Summary will update on new commits.
Note
Add regression test coverage for plan, tool_call, ext, and protocol_level modules
#[cfg(test)]modules toplan,tool_call,ext, andprotocol_levelin both v1 and v2, covering serialization, deserialization, round-trips, and edge cases.TryFromconversions.CANCEL_REQUEST_METHOD_NAMEconstant value andRequestIdshape handling (null, number, string).From/TryFromconversion correctness.Macroscope summarized 3c07b86.
Note
Low Risk
Test-only changes with no runtime or schema logic edits; risk is limited to CI time and possible future test maintenance if serde attributes change intentionally.
Overview
Adds ~60 new unit tests in v1 and v2 for
plan,tool_call,ext, andprotocol_level—modules that previously had little or no direct coverage. Production types are unchanged; tests assert serde wire behavior and in-memory update/conversion rules.Coverage locks in forgiving list parsing on
Plan::entriesandToolCallcontent/locations, default omission forToolKind/ToolCallStatus,ToolCall::update(partial field merge, collections replaced not extended),TryFrom<ToolCallUpdate>requiringtitlewithInvalidParams, extension types serializing params only (method name must not leak), and the stable$/cancel_requestmethod plusCancelRequestNotification/RequestIdround-trips.AGENTS.mdonly reformats the key-commands table for readability.Reviewed by Cursor Bugbot for commit 3c07b86. Bugbot is set up for automated code reviews on this repo. Configure here.