Rust SDK: PR #1367 review follow-ups#1382
Merged
Merged
Conversation
Three fix-before-done items:
* Repair broken intra-doc links in tool.rs ToolHandler doc and related
sites so cargo doc --no-deps works on default features (without
--all-features). Tool and define_tool were both feature-gated under
derive, causing rustdoc::broken_intra_doc_links to fail. Switched
to fully-qualified paths (Tool) and plain backticks (define_tool,
schema_for).
* Convert SessionConfig::to_wire and ResumeSessionConfig::to_wire into
consuming into_wire(self) -> (Wire, SessionConfigRuntime). The new
SessionConfigRuntime bundle holds the trait-object handlers, tool
handlers map, session-fs provider, and slash commands drained out of
the config. Eliminates the deep Vec<Tool> / HashMap<String, Value>
clone the old &self-based shape required, and makes the
.take()-after-to_wire ordering hazard a compile-time impossibility.
The duplicate-tool-handler check moves into into_wire as part of the
drain.
* Drop Serialize and Deserialize derives from SessionConfig and
ResumeSessionConfig. Since every handler field had #[serde(skip)],
serde round-tripping silently dropped all runtime behavior -- a
footgun. The wire payload is now produced exclusively via into_wire
on the dedicated wire::Session{Create,Resume}Wire structs. Also
dropped the now-redundant env_value_mode field (the wire struct
hard-codes "direct"). Two bucket_b coverage tests moved into the
in-crate unit-test module since the wire types are pub(crate).
Nice-to-haves:
* Session struct rustdoc: get_messages -> get_events (the canonical
Phase A name).
* Simplify the Client::start connection-token resolution block (drop
the shadowing inner block + redundant retyped let).
* Tighten Tool::with_parameters to panic on non-object input via the
existing tool_parameters helper, matching the loud-on-misuse
semantics of the rest of the tool module instead of silently
producing an empty parameter map.
* Fix README typo: SystemMessageTransform replaces the duplicate
ToolHandler in the handler-trait list.
* Trim the unreachable tool_name.is_empty() branch in the
ExternalToolRequested dispatch path (the outer guard already
short-circuits when the tools map lookup fails).
* Move Tool::handler from pub to pub(crate) and add a Tool::handler()
accessor so external callers can no longer overwrite an
already-attached handler by direct field assignment.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR applies the follow-ups from Rust SDK API cleanup (#1367) to harden the Rust SDK’s public surface and wire serialization path, while fixing rustdoc breakages on default features and removing a few remaining footguns.
Changes:
- Reworked session config wire-building to
into_wire(self) -> Result<(Wire, SessionConfigRuntime), Error>, eliminating ordering hazards and deep cloning while centralizing duplicate tool-handler validation. - Removed
Serialize/DeserializefromSessionConfigandResumeSessionConfigto prevent silent dropping of runtime handler fields; shifted wire-shape assertions into in-crate unit tests. - Tightened tool API ergonomics and docs:
Tool::with_parametersnow panics on non-object schema input,Tool::handleris no longer publicly settable, and rustdoc/README links were fixed for default-feature builds.
Show a summary per file
| File | Description |
|---|---|
| rust/tests/session_test.rs | Drops integration-test assertions that relied on serde wire-shape for configs; keeps token-redaction Debug coverage. |
| rust/src/types.rs | Implements into_wire + SessionConfigRuntime, removes serde derives from configs, enforces tool handler encapsulation, and updates tests accordingly. |
| rust/src/tool.rs | Fixes rustdoc links for default features and routes parameter-schema normalization through tool_parameters (panic-on-misuse). |
| rust/src/session.rs | Adopts into_wire runtime bundle, removes now-unreachable tool-name empty branch, and keeps handler extraction correct. |
| rust/src/lib.rs | Simplifies connection-token resolution logic in Client::start. |
| rust/README.md | Fixes handler-trait list typo and removes feature-gated rustdoc link to define_tool. |
Copilot's findings
- Files reviewed: 6/6 changed files
- Comments generated: 0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Local review of #1367 (Rust SDK API cleanup) flagged 9 follow-ups: 3 fix-before-done items (broken doc build, an ordering hazard, and a silent-handler-drop footgun) plus 6 nice-to-haves. This PR addresses all 9 in a single pass so the API is in shape before the next round of dependents pin against it.
Fix-before-done
cargo doc --no-depsis no longer broken on default features.tool.rsToolHandlerdoc was using[Tool]and[define_tool]intra-doc links that fail rustdoc's broken-link lint whenderiveis off. Switched to fully-qualified paths forTooland plain backticks fordefine_tool/schema_for. Same sweep applied to the README and a couple oftypes.rsitems that linked topub(crate)symbols.SessionConfig::to_wire/ResumeSessionConfig::to_wireare now consuminginto_wire(self) -> Result<(Wire, SessionConfigRuntime), Error>. The newSessionConfigRuntimestruct holds the trait-object handlers, tool-handler map, session-fs provider, and slash commands drained out of the config. This eliminates the deepVec<Tool>/HashMap<String, Value>clone the old&self-based shape required, and makes the.take()-after-to_wireordering hazard a compile-time impossibility. The duplicate-tool-handler check moved intointo_wireas part of the drain.Serialize/Deserializedropped fromSessionConfigandResumeSessionConfig. Every handler field had#[serde(skip)], so serde round-tripping silently dropped all runtime behavior. The wire payload is now produced exclusively viainto_wireon thewire::Session{Create,Resume}Wirestructs. Also dropped the now-redundantenv_value_modefield on the configs (the wire struct hard-codes"direct"). Two*_serializes_bucket_b_fieldsintegration tests moved into the in-crate unit-test module since the wire types arepub(crate).Nice-to-haves
Sessionrustdoc:get_messages->get_events(the Phase A canonical name).Client::startconnection-token resolution block - dropped the shadow block,let _ = &mut options;, and the redundant retyped let.Tool::with_parametersnow panics on non-object input via the existingtool_parametershelper instead of silently producing an empty parameter map. Matches the loud-on-misuse pattern of the rest of thetoolmodule; in-tree call sites usingjson!({...})are unaffected.ToolHandlerreplaced withSystemMessageTransformin the handler-trait list.tool_name.is_empty()branch in theExternalToolRequesteddispatch path (the outer guard already short-circuits when the tools map lookup fails).Tool::handlermoved frompubtopub(crate)and aTool::handler()read-only accessor added. External callers can no longer overwrite an already-attached handler via direct field assignment; mutation goes throughTool::with_handler.Public API impact
Three breaking changes worth calling out (all latent-bug or encapsulation fixes; SDK is pre-1.0):
SessionConfig/ResumeSessionConfigloseSerialize+Deserialize(the path was always broken).Tool::with_parameterspanics on non-object instead of silently dropping (real callers usejson!({...})and are unaffected).Tool::handlerfield becomespub(crate); newTool::handler()accessor is the read replacement.Everything else is implementation-detail-only (
to_wirewaspub(crate), doc/typo fixes, dead-branch removal).Validation
cargo check --tests --all-featurescargo doc --no-deps(default features) - confirms the fix-before-done #1 fixcargo doc --no-deps --all-featurescargo test --lib --features test-support(128 passed)cargo clippy --all-targets --all-features -- -D warningscargo fmt --checkFixes: #1380