fix: improve agent request error handling with structured details

AGENTS.md

@@ -84,7 +84,7 @@ tests/
 - **Dispatcher deduplication** (`src/dispatcher.rs`): Three-set `DedupSets` tracks event lifecycle states (`in_flight`, `completed`, `permanently_failed`). Events are identified by dedup keys formatted as `{owner}/{repo}/{event_id}`, where `event_id` is the canonical form defined per trigger type in the architecture design (Appendix A: Trigger Reference) — e.g. `issue-42` for GitHub issue events, `pr-7-review-999` for PR reviews. The canonical `event_id` from `TriggerEvent.event_id` is used directly without stripping or transformation. `SharedDedupSets` (`Arc<RwLock<DedupSets>>`) provides thread-safe async access. An event is considered a duplicate if its key appears in *any* of the three sets. State transitions: `mark_in_flight` → `mark_completed` (success) or `mark_failed` (permanent failure); `remove_in_flight` allows retry on transient failures.
 - **Dedup persistence** (`src/dispatcher.rs`): `FailedEntry` struct records permanently failed events with `{key, timestamp, error}`. `PersistenceError` enum handles IO and JSON errors from file operations. `load_dedup_file` deserializes JSON files (returns `NotFound` for missing, `Json` for corrupted). `save_dedup_file` uses atomic writes — writes to `.json.tmp`, then `rename` to target — to prevent data corruption on crash. `DedupSets::persist_completed` creates the work directory if missing (`create_dir_all`) before saving the `completed` set to `completed.json`. `DedupSets::persist_failed` creates the work directory if missing before appending a `FailedEntry` to `failed.json` (load-append-save pattern; JSON arrays require full rewrite). Error logs include the target file path via a `path` field for diagnostics. `load_persistence` reads `completed.json` and `failed.json` from the work directory at startup, treating missing files as empty sets and logging warnings for corrupted ones. `in_flight` is always empty on load (transient state).
 - **Watermark persistence** (`src/dispatcher.rs`): `Watermark` struct records the last-processed delivery/event per repository with `{last_delivery_id: Option<String>, last_event_id: Option<String>, last_processed_at: DateTime<Utc>}`. `WatermarkStore` wraps a `HashMap<String, Watermark>` keyed by `"{owner}/{repo}"` (e.g., `"mintybasil/yoke"`). On successful workflow completion, `handle_dispatch` updates the watermark for the event's repository: `last_delivery_id` is set from `TriggerEvent.delivery_id` (GitHub: the `X-GitHub-Delivery` header UUID; GitLab: `None`), `last_event_id` is set from `TriggerEvent.event_id` (the canonical event identifier), and `last_processed_at` is set to `Utc::now()`. Watermarks are persisted to `watermark.json` in the work directory during `persist_state()` (graceful shutdown) using the same atomic `.tmp` + `rename` pattern as dedup persistence. Empty watermark stores skip file creation. `load_watermarks` reads `watermark.json` at startup, falling back to an empty default for missing or corrupted files. `new_watermark_store` creates an empty `Arc<RwLock<WatermarkStore>>` for use in tests. The `X-GitHub-Delivery` header is extracted in the server's webhook handler and passed as the `delivery_id` parameter to `WebhookHandler::handle_webhook`, which assigns it to `TriggerEvent.delivery_id` before dispatch.
-- **Hermes API harness** (`src/harness.rs`): `HermesClient` encapsulates a `reqwest::Client`, `base_url`, and `api_key` for making authenticated POST requests to the Hermes Agent API `/v1/responses` endpoint. `execute_step(instructions, input)` builds a `HermesRequest { instructions, input, store: true }` where `instructions` is `Option<String>` — when `None`, the field is omitted from the JSON payload, sends it via `POST {base_url}/v1/responses` with `Authorization: Bearer *** and parses the response into a `HermesResponse`. Response parsing filters `output` content blocks for `type == "output_text"` and joins their text with newlines. Non-2xx responses write the status code and body to a `.error` file and return `HarnessError::Api`. `HarnessError` has three variants: `Http` (network/request errors), `Api` (non-2xx status with status code and body), and `Io` (file write errors for `.error`). `execute_step_with_error_path` accepts an optional `Path` for the error file (used in tests). `HermesRequest`, `HermesResponse`, and `ContentBlock` derive `Serialize`/`Deserialize` for JSON round-tripping; `ContentBlock` uses `#[serde(rename = "type")]` for the `block_type` field.
+- **Hermes API harness** (`src/harness.rs`): `HermesClient` encapsulates a `reqwest::Client`, `base_url`, and `api_key` for making authenticated POST requests to the Hermes Agent API `/v1/responses` endpoint. `execute_step(instructions, input)` builds a `HermesRequest { instructions, input, store: true }` where `instructions` is `Option<String>` — when `None`, the field is omitted from the JSON payload, sends it via `POST {base_url}/v1/responses` with `Authorization: Bearer *** and parses the response into a `HermesResponse`. Response parsing filters `output` content blocks for `type == "output_text"` and joins their text with newlines. Non-2xx responses write the status code and body to a `.error` file and return `HarnessError::Api`. `HarnessError` has three variants: `Http` (network/request errors — structured with a `message` field that includes the URL, timeout/connect status, and full cause chain via `from_reqwest_error()`), `Api` (non-2xx status with status code and body), and `Io` (file write errors for `.error`). `execute_step_with_error_path` accepts an optional `Path` for the error file (used in tests). `HermesRequest`, `HermesResponse`, and `ContentBlock` derive `Serialize`/`Deserialize` for JSON round-tripping; `ContentBlock` uses `#[serde(rename = "type")]` for the `block_type` field.
 - **StepResult struct** (`src/harness.rs`): `StepResult` captures the output of a single agent step execution. Fields: `extracted_message` (the text from `output_text` content blocks), `raw_request` (the full JSON request body sent to the API), and `raw_response` (the full JSON response body received). Both `execute_step` and `execute_step_with_error_path` return `Result<StepResult, HarnessError>` instead of `Result<String, HarnessError>`, enabling audit logging of the full HTTP exchange per step.
 - **Workflow step audit files** (`src/file_log.rs`, called from `src/runner.rs`): `write_prompt_file(step_num, step_name, prompt, workspace_dir)` writes a rendered prompt template to `{workspace_dir}/{step_num:02}_{step_name}.prompt` *before* the API call. `write_request_log_file(step_num, step_name, request, workspace_dir)` writes just the request portion to `{workspace_dir}/{step_num:02}_{step_name}.log` *before* the API call, ensuring the request is always logged even if the API call fails. `write_log_file(step_num, step_name, request, response, extracted_message, workspace_dir)` overwrites the log file with the full HTTP exchange *after* a successful API call, with sections for `REQUEST:`, `RESPONSE:`, and `FINAL MESSAGE:`. All three functions create the workspace directory if it doesn't exist. `HermesClient::build_request_body(instructions, input)` serializes the request body for pre-call logging without sending it. File naming uses zero-padded two-digit step numbers (e.g., `00_Plan.log`, `01_Analyze.prompt`). File writing is performed in `WorkflowRunner::execute_step()` using an `AtomicUsize` step counter for zero-based numbering.
 - **Workspace directory** (`src/dispatcher.rs`): `workspace_dir(workdir, owner, repo, event_id)` constructs the per-event workspace path `{workdir}/{owner}/{repo}/{event_id}/` per the architecture design doc (Section 11). The dispatcher creates this directory before spawning the workflow task. Git orchestration is **config-driven**: when any matching workflow has `[git] clone = true`, the dispatcher performs a per-event shallow clone (`git clone --depth=1 --branch <branch>`) directly into the workspace directory via `git::shallow_clone()`. The clone URL has the auth token embedded via `embed_token_in_url()` for CLI-based authentication. The `worktree` feature has been removed — `GitConfig` now rejects `worktree = true` at deserialization time with a clear migration error. Branch resolution: the dispatcher prefers `TriggerEvent.branch` (from the webhook payload), falling back to the workflow's `git.default_branch`. The `clone` field defaults to `false` (opt-in). The actual prompt and log files are written by `WorkflowRunner::execute_step()` using real API request/response data.

src/harness.rs

@@ -7,6 +7,7 @@
 //! - Parses responses to extract `output_text` content blocks
 //! - Writes non-2xx error details to a `.error` file in the current directory
 
+use std::error::Error as StdError;
 use std::fs;
 use std::path::Path;
 
@@ -110,9 +111,18 @@ pub struct StepResult {
 /// Errors that can occur during harness operations.
 #[derive(Debug, Error)]
 pub enum HarnessError {
-    /// HTTP request failed (network or server error).
-    #[error("HTTP request failed: {0}")]
-    Http(#[from] reqwest::Error),
+    /// HTTP request failed (network error, timeout, or server error).
+    ///
+    /// Carries structured details so the error message includes *why* the
+    /// request failed (timeout, connection refused, DNS error, etc.) rather
+    /// than only the opaque `error sending request for url (...)` string that
+    /// `reqwest::Error` produces by default.
+    #[error("HTTP request failed: {message}")]
+    Http {
+        /// Human-readable summary of the failure, including the URL,
+        /// timeout/connect status, and the full cause chain.
+        message: String,
+    },
     /// The API returned a non-2xx status code.
     #[error("API error {status}: {body}")]
     Api {
@@ -126,6 +136,68 @@ pub enum HarnessError {
     Io(#[from] std::io::Error),
 }
 
+impl HarnessError {
+    /// Build a descriptive `HarnessError::Http` from a `reqwest::Error`.
+    ///
+    /// Extracts timeout/connect status and walks the cause chain to produce a
+    /// message that actually explains *why* the request failed, not just
+    /// *that* it failed.
+    fn from_reqwest_error(err: reqwest::Error, url: &str) -> Self {
+        let mut parts: Vec<String> = Vec::new();
+
+        // Timeout is the most actionable piece of information for operators —
+        // it tells them to either raise the timeout or check if the server is
+        // overloaded. Surface it prominently.
+        if err.is_timeout() {
+            parts.push("timeout reached".to_string());
+        }
+
+        // A connect error means the server was unreachable (connection
+        // refused, DNS failure, etc.) — distinct from a timeout or a server
+        // that returned an error status.
+        if err.is_connect() {
+            parts.push("connection failed".to_string());
+        }
+
+        // If we somehow got a status code back (e.g. from a redirect or the
+        // response was received but the body read failed), include it.
+        if let Some(status) = err.status() {
+            parts.push(format!("status {}", status.as_u16()));
+        }
+
+        // Walk the cause chain to get the real root cause (e.g. "dns error:
+        // failed to lookup address information", "connection refused", etc.).
+        // reqwest::Error implements std::error::Error, so `source()` gives us
+        // the next link in the chain.
+        let mut chain_sources: Vec<String> = Vec::new();
+        let mut source: Option<&dyn StdError> = err.source();
+        while let Some(s) = source {
+            let display = format!("{s}");
+            // Avoid duplicate entries in the chain
+            if !chain_sources.contains(&display) {
+                chain_sources.push(display);
+            }
+            source = s.source();
+        }
+
+        // Build the final message: "error sending request for url (X):
+        // [timeout reached] [connection failed] [status N]: cause1: cause2"
+        let mut message = format!("error sending request for url ({url})");
+
+        if !parts.is_empty() {
+            message.push_str(": ");
+            message.push_str(&parts.join(", "));
+        }
+
+        if !chain_sources.is_empty() {
+            message.push_str(": ");
+            message.push_str(&chain_sources.join(": "));
+        }
+
+        HarnessError::Http { message }
+    }
+}
+
 /// HTTP client for the Hermes Agent API.
 ///
 /// Encapsulates the base URL, API key, and `reqwest::Client` for making
@@ -217,10 +289,14 @@ impl HermesClient {
             .bearer_auth(&self.api_key)
             .json(&request)
             .send()
-            .await?;
+            .await
+            .map_err(|e| HarnessError::from_reqwest_error(e, &url))?;
 
         let status = response.status();
-        let raw_response = response.text().await?;
+        let raw_response = response
+            .text()
+            .await
+            .map_err(|e| HarnessError::from_reqwest_error(e, &url))?;
 
         if !status.is_success() {
             let error_path = error_path.unwrap_or(Path::new(".error"));
@@ -470,4 +546,37 @@ mod tests {
         ));
         assert!(format!("{io_err}").contains("file not found"));
     }
+
+    #[test]
+    fn test_harness_error_http_includes_url() {
+        let err = HarnessError::Http {
+            message: "error sending request for url (http://10.200.0.3:8500/v1/responses): timeout reached: operation timed out".to_string(),
+        };
+        let display = format!("{err}");
+        assert!(display.contains("HTTP request failed"));
+        assert!(display.contains("http://10.200.0.3:8500/v1/responses"));
+        assert!(display.contains("timeout reached"));
+        assert!(display.contains("operation timed out"));
+    }
+
+    #[test]
+    fn test_harness_error_http_connection_refused() {
+        let err = HarnessError::Http {
+            message: "error sending request for url (http://localhost:8500/v1/responses): connection failed: Connection refused (os error 111)".to_string(),
+        };
+        let display = format!("{err}");
+        assert!(display.contains("connection failed"));
+        assert!(display.contains("Connection refused"));
+    }
+
+    #[test]
+    fn test_harness_error_http_minimal() {
+        // Even with no extra details, the message should still be meaningful.
+        let err = HarnessError::Http {
+            message: "error sending request for url (http://example.com/v1/responses)".to_string(),
+        };
+        let display = format!("{err}");
+        assert!(display.contains("HTTP request failed"));
+        assert!(display.contains("http://example.com/v1/responses"));
+    }
 }

tests/harness_tests.rs

@@ -277,3 +277,24 @@ fn test_hermes_response_skips_non_message_items() {
     let response: HermesResponse = serde_json::from_str(json).unwrap();
     assert!(response.extract_text().is_empty());
 }
+
+/// Verify that HarnessError::Http includes the URL and cause details.
+///
+/// Regression test for issue #225: the error message was just
+/// "HTTP request failed: error sending request for url (...)" with no
+/// indication of *why* the request failed (timeout, connection refused,
+/// DNS error, etc.). The new structured variant must include the URL,
+/// timeout/connect status, and the cause chain.
+#[test]
+fn test_harness_http_error_includes_details() {
+    use yoke::harness::HarnessError;
+
+    let err = HarnessError::Http {
+        message: "error sending request for url (http://10.200.0.3:8500/v1/responses): timeout reached: operation timed out".to_string(),
+    };
+    let display = format!("{err}");
+    assert!(display.contains("HTTP request failed"));
+    assert!(display.contains("http://10.200.0.3:8500/v1/responses"));
+    assert!(display.contains("timeout reached"));
+    assert!(display.contains("operation timed out"));
+}