From a0bacb9f7aa53ae37335dab656d0bc5be36b55ec Mon Sep 17 00:00:00 2001 From: zeroklaw Date: Mon, 29 Jun 2026 20:44:09 +0000 Subject: [PATCH] fix: improve agent request error handling with structured details Replace the opaque HarnessError::Http(#[from] reqwest::Error) variant with a structured variant that captures the URL, timeout/connect status, and full cause chain. The previous error message was just 'HTTP request failed: error sending request for url (...)' with no indication of *why* the request failed. The new from_reqwest_error() helper walks the reqwest::Error source chain to surface the root cause (DNS failure, connection refused, timeout, etc.) and includes timeout/connect indicators. Closes #225 --- AGENTS.md | 2 +- src/harness.rs | 119 +++++++++++++++++++++++++++++++++++++++-- tests/harness_tests.rs | 21 ++++++++ 3 files changed, 136 insertions(+), 6 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a3dc8d3..4a6b7cd 100644 --- a/AGENTS.md +++ b/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>`) 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, last_event_id: Option, last_processed_at: DateTime}`. `WatermarkStore` wraps a `HashMap` 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>` 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` — 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` — 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` instead of `Result`, 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 `) 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. diff --git a/src/harness.rs b/src/harness.rs index dd932c6..22323e1 100644 --- a/src/harness.rs +++ b/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 = 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 = 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")); + } } diff --git a/tests/harness_tests.rs b/tests/harness_tests.rs index 90a5ed5..c17d767 100644 --- a/tests/harness_tests.rs +++ b/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")); +}