fix: remove optionality of server_peer in component handling functions

[Mossaka]

This allows the load-component and unload-component to notify the client that the list of tools are changed

[thomastaylor312]

I'm fine with the change, but I'd like to understand why. Is Peer always required or are there times people wouldn't use it?

The requested change is in the log lines while we're here. As they stand they'll trigger clippy and should probably be fields instead anyway

[thomastaylor312]

Is there a use case still for having this be optional? I'm not sure I follow why we are removing the optionality here based on the PR description as we could still notify before

[Mossaka]

server_peer is always None as far as I understand. I remember we used to have a set_peer function but it was removed, and thus this variable is never Some(...), causing the peer.notify_tool_list_changed() to never be invoked.

I am not sure why we could still notify before though...

[Copilot]

Pull Request Overview

Removes the optionality of server_peer parameter in component handling functions to ensure tool list change notifications are consistently sent to clients when components are loaded or unloaded.

• Removes Option<> wrapper from server_peer parameter in handle_tools_call, handle_load_component, and handle_unload_component functions
• Updates the McpServer struct to remove the optional peer field and pass the peer from the request context instead
• Improves logging format to use structured logging with key-value pairs

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/main.rs	Removes optional peer field from McpServer and passes peer from request context
crates/mcp-server/src/tools.rs	Changes server_peer parameter from Optional to required
crates/mcp-server/src/components.rs	Removes Option wrapper and updates notification logic to always send notifications
crates/wassette/src/lib.rs	Updates logging format to structured logging
tests/transport_integration_test.rs	Adds comprehensive integration test for tool list notifications

[Copilot]

[nitpick] The logging is verbose with three separate log statements for a single notification operation. Consider consolidating into fewer statements or using a single log with conditional messaging based on success/failure."

Note: See the diff below for a potential fix:

@@ -60,18 +60,21 @@
 
             let contents = vec![Content::text(status_text)];
 
-            info!(
-                component_id = %id,
-                "Notifying server peer about tool list change after loading component"
-            );
             // Notify the server peer about the tool list change
-            if let Err(e) = server_peer.notify_tool_list_changed().await {
-                error!(error = %e, "Failed to send tool list change notification");
-            } else {
-                info!(
-                    component_id = %id,
-                    "Sent tool list changed notification after loading component"
-                );
+            match server_peer.notify_tool_list_changed().await {
+                Ok(_) => {
+                    info!(
+                        component_id = %id,
+                        "Tool list change notification sent to server peer after loading component"
+                    );
+                }
+                Err(e) => {
+                    error!(
+                        component_id = %id,
+                        error = %e,
+                        "Failed to send tool list change notification to server peer after loading component"
+                    );
+                }
             }
 
             Ok(CallToolResult {

[Copilot]

[nitpick] Hard-coded sleep duration makes the test slower and potentially flaky. Consider using a more robust approach like polling the process status or checking for readiness indicators."

Suggested change

	// Wait for the server to become ready by reading stdout for a readiness message, with a timeout
	let mut ready = false;
	let readiness_timeout = Duration::from_secs(5);
	let mut stdout_line = String::new();
	let readiness_result = timeout(readiness_timeout, async {
	loop {
	stdout_line.clear();
	let n = stdout.read_line(&mut stdout_line).await?;
	if n == 0 {
	// EOF reached
	break;
	}
	// Adjust the readiness message as appropriate for your server
	if stdout_line.contains("Listening") || stdout_line.contains("Server started") {
	ready = true;
	break;
	}
	}
	Ok::<(), anyhow::Error>(())
	}).await;
	if !ready {
	let mut stderr_output = String::new();
	let _ = stderr.read_line(&mut stderr_output).await;
	return Err(anyhow::anyhow!(
	"Server did not become ready in time. Last stdout: '{}', stderr: '{}'",
	stdout_line,
	stderr_output
	));
	}

[Mossaka]

@thomastaylor312 I know that you've requested changes. I will merge this PR for now to unblock our release schedule and if there are remaining items, I can work on in a new PR.

[asw101]

Merging per @Mossaka

[thomastaylor312]

@Mossaka my only blocking change you addressed anyway