fix: send UpdateResponse to clients when update results in no state change (#1734)

Co-authored-by: Claude <noreply@anthropic.com>

Author: sanity

Cargo.lock

@@ -5,7 +5,8 @@ members = [
 	"apps/freenet-ping/app",
 	"apps/freenet-ping/types",
 	"apps/freenet-ping/contracts/ping",
-	"tests/test-contract-integration"
+	"tests/test-contract-integration",
+	"tests/test-contract-update-nochange"
 ]
 
 [workspace.dependencies]

Cargo.toml

@@ -0,0 +1,68 @@
+# Pre-Commit Hook with Claude Test Detection
+
+## Overview
+
+The pre-commit hook now includes Claude-powered detection of disabled tests to prevent accidentally committing code with disabled tests.
+
+## How It Works
+
+The hook performs the following checks:
+1. **Rust formatting** - Ensures code follows standard formatting
+2. **Clippy linting** - Catches common Rust mistakes and anti-patterns
+3. **TODO-MUST-FIX detection** - Blocks commits with TODO-MUST-FIX comments
+4. **Disabled test detection** - Uses Claude to analyze the diff and detect disabled tests
+
+## Disabled Test Detection
+
+Claude will detect various patterns indicating disabled tests:
+- Rust: `#[ignore]`, `#[ignore = "reason"]`
+- JavaScript/TypeScript: `it.skip()`, `describe.skip()`, `test.skip()`
+- Python: `@pytest.mark.skip`, `@unittest.skip`
+- Commented out test functions
+- Any other language-specific test disabling patterns
+
+When disabled tests are detected, the hook will:
+1. Block the commit
+2. Show exactly where the disabled tests were found
+3. Provide guidance on how to proceed
+
+## Example Output
+
+```bash
+Checking for disabled tests with Claude...
+✗ Claude detected disabled tests in the commit
+Disabled tests found at:
+test_example.rs:6-9 - test marked with #[ignore] attribute
+Tests should not be disabled in commits. If a test must be temporarily disabled:
+1. Add a TODO-MUST-FIX comment explaining why
+2. Fix the underlying issue before committing
+3. Or exclude the test changes from this commit
+```
+
+## Handling Disabled Tests
+
+If you need to temporarily disable a test:
+
+1. **Add TODO-MUST-FIX comment**: This will also block the commit, forcing you to address it
+   ```rust
+   // TODO-MUST-FIX: This test hangs during startup
+   #[ignore = "See TODO-MUST-FIX above"]
+   fn test_broken() { }
+   ```
+
+2. **Fix the test**: The preferred solution is to fix the underlying issue
+
+3. **Exclude from commit**: Use `git add -p` to selectively stage changes, excluding the disabled test
+
+## Requirements
+
+- Claude CLI must be installed at `~/.claude/local/claude`
+- The hook will gracefully skip Claude checks if the CLI is not available
+
+## Troubleshooting
+
+- If Claude checks are failing unexpectedly, check that the Claude CLI is working:
+  ```bash
+  ~/.claude/local/claude --help
+  ```
+- The hook won't fail the commit if Claude itself has an error (only if disabled tests are found)

PRE_COMMIT_HOOK_GUIDE.md

@@ -434,26 +434,28 @@ async fn process_open_request(
                             ?data,
                             "Starting update op",
                         );
-                        let new_state = match op_manager
+                        let update_response = op_manager
                             .notify_contract_handler(ContractHandlerEvent::UpdateQuery {
                                 key,
                                 data,
                                 related_contracts: related_contracts.clone(),
                             })
-                            .await
-                        {
-                            Ok(ContractHandlerEvent::UpdateResponse {
+                            .await?;
+
+                        let new_state = match update_response {
+                            ContractHandlerEvent::UpdateResponse {
                                 new_value: Ok(new_val),
-                            }) => Ok(new_val),
-                            Ok(ContractHandlerEvent::UpdateResponse {
+                            } => Ok(new_val),
+                            ContractHandlerEvent::UpdateResponse {
                                 new_value: Err(err),
-                            }) => Err(OpError::from(err)),
-                            Ok(ContractHandlerEvent::UpdateNoChange { key }) => {
-                                tracing::debug!(%key, "update with no change, do not start op");
-                                return Ok(None);
+                            } => Err(OpError::from(err)),
+                            ContractHandlerEvent::UpdateNoChange { key } => {
+                                // This should not happen anymore since we now return UpdateResponse
+                                // from the contract handler even for NoChange cases
+                                tracing::warn!(%key, "Unexpected UpdateNoChange event - this should have been converted to UpdateResponse");
+                                return Err(OpError::UnexpectedOpState.into());
                             }
-                            Err(err) => Err(err.into()),
-                            Ok(_) => Err(OpError::UnexpectedOpState),
+                            _ => return Err(OpError::UnexpectedOpState.into()),
                         }
                         .inspect_err(|err| tracing::error!(%key, "update query failed: {}", err))?;
 

crates/core/src/client_events/mod.rs

@@ -552,7 +552,25 @@ async fn process_host_response(
                         HostResponse::Ok => "HostResponse::Ok",
                         _ => "Unknown",
                     };
-                    tracing::debug!(response = %res, response_type, cli_id = %id, "sending response");
+
+                    // Enhanced logging for UPDATE responses
+                    match &res {
+                        HostResponse::ContractResponse(ContractResponse::UpdateResponse {
+                            key,
+                            summary,
+                        }) => {
+                            tracing::debug!(
+                                "Processing UpdateResponse for WebSocket delivery - client: {}, key: {}, summary length: {}",
+                                id,
+                                key,
+                                summary.size()
+                            );
+                        }
+                        _ => {
+                            tracing::debug!(response = %res, response_type, cli_id = %id, "sending response");
+                        }
+                    }
+
                     match res {
                         HostResponse::ContractResponse(ContractResponse::GetResponse {
                             key,
@@ -572,14 +590,64 @@ async fn process_host_response(
                     Err(err)
                 }
             };
+            // Log when UPDATE response is about to be sent over WebSocket
+            let is_update_response = match &result {
+                Ok(HostResponse::ContractResponse(ContractResponse::UpdateResponse {
+                    key,
+                    ..
+                })) => {
+                    tracing::debug!(
+                        "About to serialize UpdateResponse for WebSocket delivery - client: {}, key: {}",
+                        client_id,
+                        key
+                    );
+                    Some(*key)
+                }
+                _ => None,
+            };
+
             let serialized_res = match encoding_protoc {
                 EncodingProtocol::Flatbuffers => match result {
                     Ok(res) => res.into_fbs_bytes()?,
                     Err(err) => err.into_fbs_bytes()?,
                 },
                 EncodingProtocol::Native => bincode::serialize(&result)?,
             };
-            tx.send(Message::Binary(serialized_res)).await?;
+
+            // Log serialization completion for UPDATE responses
+            if let Some(key) = is_update_response {
+                tracing::debug!(
+                    "Serialized UpdateResponse for WebSocket delivery - client: {}, key: {}, size: {} bytes",
+                    client_id,
+                    key,
+                    serialized_res.len()
+                );
+            }
+
+            let send_result = tx.send(Message::Binary(serialized_res)).await;
+
+            // Log WebSocket send result for UPDATE responses
+            if let Some(key) = is_update_response {
+                match &send_result {
+                    Ok(()) => {
+                        tracing::debug!(
+                            "Successfully sent UpdateResponse over WebSocket to client {} for key {}",
+                            client_id,
+                            key
+                        );
+                    }
+                    Err(err) => {
+                        tracing::error!(
+                            "Failed to send UpdateResponse over WebSocket to client {} for key {}: {:?}",
+                            client_id,
+                            key,
+                            err
+                        );
+                    }
+                }
+            }
+
+            send_result?;
             Ok(None)
         }
         Some(HostCallbackResult::SubscriptionChannel { key, id, callback }) => {
@@ -627,20 +695,88 @@ impl ClientEventsProxy for WebSocketProxy {
         result: Result<HostResponse, ClientError>,
     ) -> BoxFuture<Result<(), ClientError>> {
         async move {
+            // Log UPDATE responses specifically
+            match &result {
+                Ok(HostResponse::ContractResponse(freenet_stdlib::client_api::ContractResponse::UpdateResponse { key, summary })) => {
+                    tracing::debug!(
+                        "WebSocket send() called with UpdateResponse for client {} - key: {}, summary length: {}",
+                        id,
+                        key,
+                        summary.size()
+                    );
+                }
+                Ok(other_response) => {
+                    tracing::debug!("WebSocket send() called with response for client {}: {:?}", id, other_response);
+                }
+                Err(error) => {
+                    tracing::debug!("WebSocket send() called with error for client {}: {:?}", id, error);
+                }
+            }
+
             if let Some(ch) = self.response_channels.remove(&id) {
+                // Log success/failure of sending UPDATE responses
+                if let Ok(HostResponse::ContractResponse(freenet_stdlib::client_api::ContractResponse::UpdateResponse { key, .. })) = &result {
+                    tracing::debug!(
+                        "Found WebSocket channel for client {}, sending UpdateResponse for key {}",
+                        id,
+                        key
+                    );
+                }
+
+                // Check if this is an UPDATE response and extract key before moving result
+                let update_key = match &result {
+                    Ok(HostResponse::ContractResponse(freenet_stdlib::client_api::ContractResponse::UpdateResponse { key, .. })) => Some(*key),
+                    _ => None
+                };
+
                 let should_rm = result
                     .as_ref()
                     .map_err(|err| matches!(err.kind(), ErrorKind::Disconnect))
                     .err()
                     .unwrap_or(false);
-                if ch.send(HostCallbackResult::Result { id, result }).is_ok() && !should_rm {
+
+                let send_result = ch.send(HostCallbackResult::Result { id, result });
+
+                // Log UPDATE response send result
+                if let Some(key) = update_key {
+                    match send_result.is_ok() {
+                        true => {
+                            tracing::debug!(
+                                "Successfully sent UpdateResponse to client {} for key {}",
+                                id,
+                                key
+                            );
+                        }
+                        false => {
+                            tracing::error!(
+                                "Failed to send UpdateResponse to client {} for key {} - channel send failed",
+                                id,
+                                key
+                            );
+                        }
+                    }
+                }
+
+                if send_result.is_ok() && !should_rm {
                     // still alive connection, keep it
                     self.response_channels.insert(id, ch);
                 } else {
                     tracing::info!("dropped connection to client #{id}");
                 }
             } else {
-                tracing::warn!("client: {id} not found");
+                // Log when client is not found for UPDATE responses
+                match &result {
+                    Ok(HostResponse::ContractResponse(freenet_stdlib::client_api::ContractResponse::UpdateResponse { key, .. })) => {
+                        tracing::error!(
+                            "Client {} not found in WebSocket response channels when trying to send UpdateResponse for key {}",
+                            id,
+                            key
+                        );
+                    }
+                    _ => {
+                        tracing::warn!("client: {id} not found");
+                    }
+                }
             }
             Ok(())
         }

crates/core/src/client_events/websocket.rs

@@ -144,7 +144,29 @@ where
                     .await;
 
                 let event_result = match update_result {
-                    Ok(UpsertResult::NoChange) => ContractHandlerEvent::UpdateNoChange { key },
+                    Ok(UpsertResult::NoChange) => {
+                        tracing::info!(%key, "UPDATE resulted in NoChange, fetching current state to return UpdateResponse");
+                        // When there's no change, we still need to return the current state
+                        // so the client gets a proper response
+                        match contract_handler.executor().fetch_contract(key, false).await {
+                            Ok((Some(current_state), _)) => {
+                                tracing::info!(%key, "Successfully fetched current state for NoChange update");
+                                ContractHandlerEvent::UpdateResponse {
+                                    new_value: Ok(current_state),
+                                }
+                            }
+                            Ok((None, _)) => {
+                                tracing::warn!(%key, "No state found when fetching for NoChange update");
+                                // Fallback to the old behavior if we can't fetch the state
+                                ContractHandlerEvent::UpdateNoChange { key }
+                            }
+                            Err(err) => {
+                                tracing::error!(%key, %err, "Error fetching state for NoChange update");
+                                // Fallback to the old behavior if we can't fetch the state
+                                ContractHandlerEvent::UpdateNoChange { key }
+                            }
+                        }
+                    }
                     Ok(UpsertResult::Updated(state)) => ContractHandlerEvent::UpdateResponse {
                         new_value: Ok(state),
                     },