refactor: extract session_id extraction helper, DRY auth.rs

Extract duplicated session ID extraction + validation logic from SessionAuthMiddleware::call() and require_session() into two shared helper functions: extract_session_id and validate_and_build_identity. Removes ~60 lines of duplicated code while preserving behavior: - Both call sites now rely on the same extraction/validation path - User-Agent default remains empty string (existing behavior unchanged) - Error response format (401 with header/validation messages) unchanged Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-18 03:18:27 +07:00
parent 7d8487cefb
commit e32501ee59
2 changed files with 1037 additions and 56 deletions
@@ -115,6 +115,52 @@ pub struct SessionAuthMiddleware<S> {
    store: Arc<Store>,
 }
 // ---------------------------------------------------------------------------
 // Session ID helpers
 // ---------------------------------------------------------------------------
 /// Extract and validate `X-Session-Id` from request headers.
 ///
 /// Flow: read header -> validate non-empty -> return ID or a 401 error response.
 fn extract_session_id<ReqBody>(req: &Request<ReqBody>) -> Result<String, Response> {
    let session_id = req
        .headers()
        .get("X-Session-Id")
        .and_then(|v| v.to_str().ok())
        .map(|s| s.to_string());
    match session_id {
        Some(id) if !id.is_empty() => Ok(id),
        _ => Err((StatusCode::UNAUTHORIZED, "missing X-Session-Id header").into_response()),
    }
 }
 /// Validate session and build identity from request context.
 ///
 /// Flow: validate session in store -> extract User-Agent -> build SessionIdentity.
 fn validate_and_build_identity<ReqBody>(
    session_id: &str,
    store: &Store,
    req: &Request<ReqBody>,
 ) -> Result<SessionIdentity, Response> {
    match validate_session(session_id, store) {
        Ok(_session) => {
            let user_agent = req
                .headers()
                .get(header::USER_AGENT)
                .and_then(|v| v.to_str().ok())
                .unwrap_or("")
                .to_string();
            Ok(SessionIdentity::new(session_id.to_string(), user_agent))
        }
        Err(e) => Err((
            StatusCode::UNAUTHORIZED,
            format!("session validation failed: {e}"),
        )
            .into_response()),
    }
 }
 impl<S, ReqBody> Service<Request<ReqBody>> for SessionAuthMiddleware<S>
 where
    S: Service<Request<ReqBody>, Response = Response> + Send + 'static,
@@ -133,39 +179,17 @@ where
    fn call(&mut self, mut req: Request<ReqBody>) -> Self::Future {
        let store = Arc::clone(&self.store);
-        // Extract session id from header.
+        let session_id = match extract_session_id(&req) {
-        let session_id = req
+            Ok(id) => id,
-            .headers()
+            Err(resp) => return Box::pin(async move { Ok(resp) }),
            .get("X-Session-Id")
            .and_then(|v| v.to_str().ok())
            .map(|s| s.to_string());
        let session_id = match session_id {
            Some(id) if !id.is_empty() => id,
            _ => {
                let resp =
                    (StatusCode::UNAUTHORIZED, "missing X-Session-Id header").into_response();
                return Box::pin(async move { Ok(resp) });
            }
        };
-        // Validate session.
+        match validate_and_build_identity(&session_id, &store, &req) {
-        if let Err(e) = validate_session(&session_id, &store) {
+            Ok(identity) => {
-            let resp = (
+                req.extensions_mut().insert(identity);
-                StatusCode::UNAUTHORIZED,
+            }
-                format!("session validation failed: {e}"),
+            Err(resp) => return Box::pin(async move { Ok(resp) }),
-            )
+        };
                .into_response();
            return Box::pin(async move { Ok(resp) });
        }
        let user_agent = req
            .headers()
            .get(header::USER_AGENT)
            .and_then(|v| v.to_str().ok())
            .unwrap_or("")
            .to_string();
        let identity = SessionIdentity::new(session_id, user_agent);
        req.extensions_mut().insert(identity);
        let fut = self.inner.call(req);
        Box::pin(fut)
@@ -186,33 +210,15 @@ pub async fn require_session(
    mut req: Request<axum::body::Body>,
    next: axum::middleware::Next,
 ) -> Response {
-    let session_id = req
+    let session_id = match extract_session_id(&req) {
-        .headers()
+        Ok(id) => id,
-        .get("X-Session-Id")
+        Err(resp) => return resp,
        .and_then(|v| v.to_str().ok())
        .map(|s| s.to_string());
    let session_id = match session_id {
        Some(id) if !id.is_empty() => id,
        _ => {
            return (StatusCode::UNAUTHORIZED, "missing X-Session-Id header").into_response();
        }
    };
-    if let Err(e) = validate_session(&session_id, &store) {
+    let identity = match validate_and_build_identity(&session_id, &store, &req) {
-        return (
+        Ok(identity) => identity,
-            StatusCode::UNAUTHORIZED,
+        Err(resp) => return resp,
-            format!("session validation failed: {e}"),
+    };
        )
            .into_response();
    }
    let user_agent = req
        .headers()
        .get(header::USER_AGENT)
        .and_then(|v| v.to_str().ok())
        .unwrap_or("")
        .to_string();
    let identity = SessionIdentity::new(session_id, user_agent);
    req.extensions_mut().insert(identity);
    next.run(req).await
 }
@@ -0,0 +1,975 @@
 # DRY Refactor — High Priority Items Implementation Plan
 > **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
 **Goal:** Eliminate ~500 lines of duplicated code across 6 high-impact patterns, making the codebase more maintainable and reducing the surface area for bugs.
 **Architecture:** Each task is independent and can be implemented, tested, and committed separately. Tasks are ordered by risk/reward — highest impact, lowest risk first.
 **Tech Stack:** Rust, anyhow, serde, std::fs, tower (Service/Layer trait), std::sync::atomic
 ---
 ## Task 1: Atomic Write Helper — `zesdex-utils`
 **Problem:** 10 files across 3 crates (zesdex-cms persistence, zesdex-iam persistence, zesdex-entities) duplicate the same crash-safe write-to-then-rename pattern. Each has minor variations: tmp filename strategy, permission setting, log message.
 **Design:** Add a `write_json_atomic` helper to `zesdex-utils` that handles the common pattern. For the permission variant (oauth_repo.rs), expose a mode parameter. For the logging variant, the caller handles that.
 **Files:**
 - Create: `crates/zesdex-utils/src/atomic_write.rs`
 - Modify: `crates/zesdex-utils/src/lib.rs`
 - Modify: 10 caller files across 3 crates
 **Interfaces:**
 - Produces: `pub fn write_json_atomic<T: Serialize>(path: &Path, data: &T, mode: Option<u32>) -> Result<()>`
 ### Step 1: Create `atomic_write.rs` module
 ```rust
 //! Crash-safe atomic file write helper.
 //!
 //! Writes serializable data to a temp file, fsyncs, then renames into
 //! place to guarantee atomicity.  On Unix, an optional `mode` sets the
 //! permissions of the final file (e.g. `0o600` for OAuth tokens).
 use std::io::Write;
 use std::path::Path;
 use serde::Serialize;
 /// Atomically write serializable `data` to `path`.
 ///
 /// Flow: serialize → write to `path.tmp` → fsync → rename → fsync parent.
 /// If `mode` is `Some`, set permissions before rename (Unix only).
 ///
 /// Edge case: tmp file name uses `with_extension("tmp")` which replaces
 /// the existing extension — correct for `foo.json` → `foo.tmp`. For paths
 /// without an extension (unlikely in this codebase), appends `.tmp`.
 pub fn write_json_atomic<T: Serialize>(path: &Path, data: &T, mode: Option<u32>) -> anyhow::Result<()> {
    let tmp = path.with_extension("tmp");
    let bytes = serde_json::to_vec_pretty(data)?;
    {
        let mut f = std::fs::OpenOptions::new()
            .create(true)
            .truncate(true)
            .write(true)
            .open(&tmp)?;
        f.write_all(&bytes)?;
        f.sync_all()?;
    }
    if let Some(m) = mode {
        #[cfg(unix)]
        {
            use std::os::unix::fs::PermissionsExt;
            std::fs::set_permissions(&tmp, std::fs::Permissions::from_mode(m))?;
        }
        #[cfg(not(unix))]
        { let _ = m; }
    }
    std::fs::rename(&tmp, path)?;
    if let Some(parent) = path.parent() {
        let _ = std::fs::File::open(parent).and_then(|d| d.sync_all());
    }
    Ok(())
 }
 ```
 ### Step 2: Register module in `zesdex-utils/src/lib.rs`
 Add `pub mod atomic_write;` and `pub use atomic_write::write_json_atomic;`
 ### Step 3–10: Replace 10 call sites
 Each caller follows the same pattern — replace 10–20 lines with a single call. Below is the before/after for each file.
 **A) `crates/zesdex-cms/src/infrastructure/persistence/app_config_repo.rs:133-164`**
 Before (30 lines with OpenOptions + write_all + sync_all + rename + parent sync):
 ```rust
 fn save(&self, base_dir: &Path, config: &AppConfig) -> Result<()> {
    std::fs::create_dir_all(base_dir)
        .with_context(|| format!("failed to create base dir '{}'", base_dir.display()))?;
    let path = base_dir.join("app_config.json");
    let json = serde_json::to_string_pretty(config).context("failed to serialize app config")?;
    let tmp = base_dir.join("app_config.json.tmp");
    {
        let mut f = std::fs::OpenOptions::new()
            .create(true).truncate(true).write(true)
            .open(&tmp)
            .with_context(|| format!("failed to write temp file '{}'", tmp.display()))?;
        f.write_all(json.as_bytes())?;
        f.sync_all()?;
    }
    std::fs::rename(&tmp, &path).with_context(|| {
        format!("failed to rename '{}' -> '{}'", tmp.display(), path.display())
    })?;
    if let Some(parent) = path.parent() {
        if let Ok(d) = std::fs::File::open(parent) {
            let _ = d.sync_all();
        }
    }
    tracing::debug!("app_config saved to '{}'", path.display());
    Ok(())
 }
 ```
 After (7 lines):
 ```rust
 fn save(&self, base_dir: &Path, config: &AppConfig) -> Result<()> {
    std::fs::create_dir_all(base_dir)
        .with_context(|| format!("failed to create base dir '{}'", base_dir.display()))?;
    let path = base_dir.join("app_config.json");
    write_json_atomic(&path, config, None)
        .with_context(|| "failed to save app_config")?;
    tracing::debug!("app_config saved to '{}'", path.display());
    Ok(())
 }
 ```
 The `serde_json::to_string_pretty` call is now inside `write_json_atomic`, so its `.context("...")` goes away. The error context is slightly less specific per call site, but `write_json_atomic` itself maps errors via `?` and they'll propagate with the caller's context.
 **B) `crates/zesdex-cms/src/infrastructure/persistence/conversation_repo.rs:43-74`**
 Same pattern — replace the 30-line block with `write_json_atomic(&path, conversation, None)?`.
 **C) `crates/zesdex-cms/src/infrastructure/persistence/settings_repo.rs:55-87`**
 Replace with `write_json_atomic(&path, settings, None)?`.
 **D) `crates/zesdex-cms/src/infrastructure/persistence/memory_repo.rs:171-205`**
 This variant uses a UUID-based tmp name (`uuid::Uuid::new_v4()`) instead of a fixed `.tmp` suffix. **Goes away** — `write_json_atomic` uses `with_extension("tmp")`.
 Replace with `write_json_atomic(&path, memory, None)?`.
 Note: The UUID tmp name was an intentional safety measure (no name collision risk even on concurrent writes). `with_extension("tmp")` can still collide on truly concurrent saves to the same path, but the rename is atomic so at most one wins. Accept this trade-off for the DRY benefit.
 **E) `crates/zesdex-cms/src/infrastructure/persistence/rewind_blob_repo.rs:62-71`**
 This writes binary `data: &[u8]` (not serializable). The helper only handles `Serialize`. Two options:
 1. Keep as-is (it's short — 10 lines, 3 are unique)
 2. Create a separate `write_binary_atomic(path, data, mode)` function
 **Decision:** Leave as-is. Binary blob write is 10 lines and has a different signature (`&[u8]`, not `&impl Serialize`). Not worth abstracting.
 **F) `crates/zesdex-iam/src/infrastructure/persistence/oauth_repo.rs:29-48`**
 Adds `#[cfg(unix)]` chmod 0o600. Gets `mode: Some(0o600)`:
 ```rust
 fn save_token(&self, path: &Path, token: &OAuthToken) -> anyhow::Result<()> {
    if let Some(parent) = path.parent() {
        std::fs::create_dir_all(parent)?;
    }
    write_json_atomic(path, token, Some(0o600))?;
    Ok(())
 }
 ```
 **G) `crates/zesdex-iam/src/infrastructure/persistence/session_repo.rs:60-76`**
 Replace with `write_json_atomic(&path, session, None)?`.
 **H) `crates/zesdex-entities/src/domain/common/conversation.rs:83-95`**
 This uses `std::io::Result` not `anyhow::Result`. The helper returns `anyhow::Result`. Two options:
 1. Make helper generic over error type (too complex)
 2. Convert
 **Decision:** Convert caller to use `anyhow::Result`. The entity crate already depends on anyhow transitively (it's used by caller crates). Add `use anyhow::Context as _;` and wrap.
 ```rust
 pub fn save_conversation(&self, base_dir: &std::path::Path) -> anyhow::Result<()> {
    let dir = base_dir.join("sessions").join(&self.session_id);
    std::fs::create_dir_all(&dir)?;
    let path = dir.join("conversation.json");
    write_json_atomic(&path, self, None)?;
    Ok(())
 }
 ```
 **I) `crates/zesdex-entities/src/domain/auth/session.rs:70-82`**
 Same conversion as H:
 ```rust
 pub fn save(&self, base_dir: &Path) -> anyhow::Result<()> {
    let dir = self.session_dir(base_dir);
    std::fs::create_dir_all(&dir)?;
    let path = dir.join("session.json");
    write_json_atomic(&path, self, None)?;
    Ok(())
 }
 ```
 **J) `crates/zesdex-entities/src/domain/auth/session_lock.rs:72-88`**
 The stale-lock recovery path in `try_lock()`. Same conversion:
 ```rust
 let tmp = self.path.with_extension("lock.tmp");
 {
    let mut tmp_file = fs::OpenOptions::new()
        .create(true).truncate(true).write(true).open(&tmp)?;
    write!(tmp_file, "{}", self.pid)?;
    tmp_file.sync_all()?;
 }
 fs::rename(&tmp, &self.path)?;
 if let Some(parent) = self.path.parent() {
    let _ = fs::File::open(parent).and_then(|d| d.sync_all());
 }
 ```
 This writes a PID string, not JSON. `write_json_atomic` expects `Serialize`. **Keep as-is** — 10 lines, different serialization format (write! macro, not serde).
 ### Step 11: Build & test
 Run: `cargo build -p zesdex-utils && cargo test -p zesdex-utils`
 Run: `cargo build -p zesdex-cms -p zesdex-iam -p zesdex-entities`
 Run full test suite: `cargo test`
 ### Step 12: Commit
 ```bash
 git add -A
 git commit -m "refactor: extract write_json_atomic helper, DRY 7 call sites"
 ```
 ---
 ## Task 2: Consolidate `#![allow(clippy::cast_*)]` to crate roots
 **Problem:** 67 files across 8 crates have a `#![allow(clippy::cast_...)]` inner attribute. 6 of 8 crate roots already have it, making sub-file attrs redundant. 2 crate roots (`zesdex-entities`, `zesdex-utils`) lack it — need to add before removing sub-file attrs.
 **Strategy:** Remove inner `#![allow(clippy::cast_*)]` from every sub-file, leaving only the crate-root attribute. Use a script for the mechanical removal, then verify with `cargo build`.
 **Files:** ~65 files to edit (remove 6-line block from each), 2 files to add block to
 ### Step 1: Add to crate roots that lack it
 Add to `crates/zesdex-entities/src/lib.rs` (before `pub mod domain;`):
 ```rust
 #![allow(
    clippy::cast_possible_truncation,
    clippy::cast_sign_loss,
    clippy::cast_precision_loss,
    clippy::cast_possible_wrap
 )]
 ```
 Add same block to `crates/zesdex-utils/src/lib.rs`.
 ### Step 2–65: Remove from sub-files
 For each sub-file that has the inner allow block, remove the 6-line annotation block. **Do NOT remove `#[allow(...)]` (outer) on individual items — only `#![allow(...)]` (inner) at module level.**
 **File list** (65 files — grouped by crate to parallelize):
 **zesdex-cms** (18 sub-files — crate root lib.rs already has it):
 `infrastructure/persistence/app_config_repo.rs`, `conversation_repo.rs`, `edit_log_repo.rs`, `memory_repo.rs`, `mod.rs`, `settings_repo.rs`, `rewind_blob_repo.rs`
 `domain/app_config.rs`, `edit_log.rs`, `memory.rs`, `mod.rs`, `repository.rs`, `service.rs`, `settings.rs`
 `application/conversation_service.rs`, `memory_service.rs`, `mod.rs`, `settings_service.rs`
 `infrastructure/http/dto.rs`, `handlers.rs`, `mod.rs`
 `infrastructure/mod.rs`
 **zesdex-iam** (9 sub-files — lib.rs already has it):
 `application/oauth_service.rs`, `session_service.rs`
 `domain/oauth.rs`, `repository.rs`, `service.rs`
 `infrastructure/http/dto.rs`, `handlers.rs`, `oauth_loopback.rs`
 `infrastructure/persistence/oauth_repo.rs`, `session_repo.rs`
 **zesdex-backend** (12 sub-files — main.rs already has it):
 `app/bgbash/control.rs`, `app/mode/effort.rs`, `app/mode/rewind.rs`, `app/review/probe.rs`, `app/runtime/actions/mod.rs`, `app/state/types.rs`
 `tool/fs/edit.rs`, `tool/fs/read.rs`, `tool/lsp/mod.rs`
 `view/chat.rs`, `view/mod.rs`, `view/status.rs`
 **zesdex-entities** (9 sub-files — after adding to lib.rs):
 `domain/auth/session.rs`, `session_lock.rs`
 `domain/common/conversation.rs`, `message.rs`, `provider.rs`, `store.rs`, `tool_call.rs`, `tool_result.rs`, `usage.rs`
 **zesdex-ipc** (3 sub-files — lib.rs already has it):
 `frame.rs`, `protocol.rs`, `server.rs`
 **zesdex-middleware** (2 sub-files — lib.rs already has it):
 `auth.rs`, `cors.rs`
 **zesdex-infra** (4 sub-files — lib.rs already has it):
 `database.rs`, `jwt.rs`, `password.rs`, `state.rs`
 **zesdex-utils** (3 sub-files — after adding to lib.rs):
 `error.rs`, `pagination.rs`, `sanitize.rs`, `slug.rs`
 **Tip:** Use a bash loop for the mechanical removal (after verifying the first few manually):
 ```bash
 for f in $(grep -rl "#!\[allow" crates/ --include="*.rs" | grep -v lib.rs | grep -v main.rs | grep -v target); do
  # Remove 6-line clippy allow block (lines 1-6 or after doc comment)
  # Manual approach: sed -i '/^#!\[allow/,/^)/d' "$f"
  # But careful: only remove if it's the clippy::cast allow block
 done
 ```
 **Important:** Do NOT run a blind sed. Each file may have different structure (doc comments before the allow, etc.). Use a targeted approach:
 1. Search for `#![allow(clippy::cast_` 
 2. Verify it's the 4 cast lints
 3. Remove from `#![allow(` through `)]` inclusive
 ### Step 66: Build & verify
 Run: `cargo build 2>&1 | head -50`
 If any crate needs the allow and doesn't have it at root level, the cast lints will fire as warnings (denied as errors if `#[deny(clippy::...)]` is in play). Add the allow to that crate root.
 ### Step 67: Commit
 ```bash
 git add -A
 git commit -m "refactor: consolidate #![allow(clippy::cast_*)] to crate roots, remove from 65 sub-files"
 ```
 ---
 ## Task 3: Tower Service/Layer Boilerplate Macro — `zesdex-middleware`
 **Problem:** `auth.rs` and `rate_limit.rs` have byte-for-byte identical `where` clause, `type Response`, `type Error`, `type Future`, and `fn poll_ready`. The `call()` method differs (auth vs rate-limit logic).
 **Design:** Create a `impl_tower_middleware!` macro that generates the shared boilerplate.
 **Files:**
 - Modify: `crates/zesdex-middleware/src/lib.rs`
 - Modify: `crates/zesdex-middleware/src/auth.rs`
 - Modify: `crates/zesdex-middleware/src/rate_limit.rs`
 ### Step 1: Add macro to `lib.rs`
 ```rust
 /// Generate the boilerplate Tower `Service` impl for a middleware struct.
 ///
 /// Usage:
 /// ```ignore
 /// impl_tower_middleware!(MyMiddleware<S> [ inner: S, extra_field: Type ]);
 /// ```
 ///
 /// Expands to:
 /// - `type Response = S::Response`
 /// - `type Error = S::Error`
 /// - `type Future = Pin<Box<dyn Future<Output = Result<...>> + Send + 'static>>`
 /// - `fn poll_ready(&mut self, cx) { self.inner.poll_ready(cx) }`
 #[macro_export]
 macro_rules! impl_tower_middleware {
    ($name:ident<S $(, $extra:ident: $ty:ty)*>) => {
        impl<S, ReqBody> tower::Service<axum::http::Request<ReqBody>> for $name<S>
        where
            S: tower::Service<axum::http::Request<ReqBody>, Response = axum::response::Response>
                + Send + 'static,
            S::Future: Send + 'static,
            ReqBody: Send + 'static,
        {
            type Response = S::Response;
            type Error = S::Error;
            type Future = std::pin::Pin<
                Box<dyn std::future::Future<Output = Result<Self::Response, Self::Error>> + Send + 'static>,
            >;
            fn poll_ready(
                &mut self,
                cx: &mut std::task::Context<'_>,
            ) -> std::task::Poll<Result<(), Self::Error>> {
                self.inner.poll_ready(cx)
            }
        }
    };
 }
 ```
 ### Step 2: Apply to `auth.rs`
 Before (lines 124-137):
 ```rust
 impl<S, ReqBody> Service<Request<ReqBody>> for SessionAuthMiddleware<S>
 where
    S: Service<Request<ReqBody>, Response = Response> + Send + 'static,
    S::Future: Send + 'static,
    ReqBody: Send + 'static,
 {
    type Response = S::Response;
    type Error = S::Error;
    type Future =
        Pin<Box<dyn Future<Output = Result<Self::Response, Self::Error>> + Send + 'static>>;
    fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
        self.inner.poll_ready(cx)
    }
    fn call(&mut self, mut req: Request<ReqBody>) -> Self::Future {
        // ... 40 lines of actual logic
    }
 }
 ```
 After:
 ```rust
 impl_tower_middleware!(SessionAuthMiddleware<S>);
 impl<S, ReqBody> SessionAuthMiddleware<S>
 where
    S: Service<Request<ReqBody>, Response = Response> + Send + 'static,
    S::Future: Send + 'static,
    ReqBody: Send + 'static,
 {
    fn call(&mut self, mut req: Request<ReqBody>) -> Self::Future {
        // ... same 40 lines
    }
 }
 ```
 Wait — the macro generates the `impl<S, ReqBody> Service<Request<ReqBody>> for ...` block including `fn call`. We need to only use the macro for the boilerplate and keep `call()` free.
 **Revised approach:** The macro expands to the full `impl Service for ...` but only includes `poll_ready` and associated types, NOT `call`. The `call()` method remains in a separate `impl` block:
 ```rust
 // Generated by macro:
 impl<S, ReqBody> Service<Request<ReqBody>> for SessionAuthMiddleware<S>
 where ...
 {
    type Response = S::Response;
    type Error = S::Error;
    type Future = ...;
    fn poll_ready(...) { ... }
    // call() is NOT in the macro — must be written by hand in a separate
    // inherent impl block. Actually no — call() is required by the trait.
 }
 ```
 **Revised design:** Don't use a macro. Instead, extract a **trait** or simply accept the duplication — 15 lines of boilerplate across 2 files is acceptable. Alternative: use a **widget supertrait** or keep-as-is.
 **Decision:** Skip this task. The Tower Service boilerplate is only 15 lines duplicated once (2 files). The macro approach adds complexity without proportional benefit. The `where` clause in particular is fragile — tightening bounds (e.g., adding `ReqBody: Debug`) shouldn't need a macro change.
 **Note to implementer:** If a clean solution is found later (perhaps via a Tower helper crate or a proc-macro), it can be applied then. For now, mark this as `wontfix`.
 ---
 ## Task 4: Extract Session ID Helper — `auth.rs`
 **Problem:** Session ID extraction + validation + error response is duplicated verbatim at `auth.rs:143-174` and `auth.rs:195-222` (~30 lines × 2).
 **Files:**
 - Modify: `crates/zesdex-middleware/src/auth.rs`
 ### Step 1: Add helper function
 ```rust
 /// Extract and validate `X-Session-Id` from request headers.
 ///
 /// Flow: read header → validate non-empty → return ID or a 401 error response.
 fn extract_session_id(req: &Request<ReqBody>) -> Result<String, Response> {
    let session_id = req
        .headers()
        .get("X-Session-Id")
        .and_then(|v| v.to_str().ok())
        .map(|s| s.to_string());
    match session_id {
        Some(id) if !id.is_empty() => Ok(id),
        _ => Err((StatusCode::UNAUTHORIZED, "missing X-Session-Id header").into_response()),
    }
 }
 /// Validate session and build identity from request context.
 fn validate_and_build_identity(
    session_id: &str,
    store: &Store,
    req: &Request<ReqBody>,
 ) -> Result<SessionIdentity, Response> {
    match validate_session(session_id, store) {
        Ok(session) => {
            let user_agent = req
                .headers()
                .get("User-Agent")
                .and_then(|v| v.to_str().ok())
                .unwrap_or("unknown")
                .to_string();
            Ok(SessionIdentity::new(session_id.to_string(), user_agent))
        }
        Err(e) => Err((
            StatusCode::UNAUTHORIZED,
            format!("session validation failed: {e}"),
        )
            .into_response()),
    }
 }
 ```
 ### Step 2: Replace first call site (inside `SessionAuthMiddleware::call`, lines 143-174)
 Before:
 ```rust
 let session_id = req
    .headers()
    .get("X-Session-Id")
    .and_then(|v| v.to_str().ok())
    .map(|s| s.to_string());
 let session_id = match session_id {
    Some(id) if !id.is_empty() => id,
    _ => {
        let resp = (StatusCode::UNAUTHORIZED, "missing X-Session-Id header").into_response();
        return Box::pin(async move { Ok(resp) });
    }
 };
 let user_agent = req
    .headers()
    .get("User-Agent")
    .and_then(|v| v.to_str().ok())
    .unwrap_or("unknown")
    .to_string();
 match validate_session(&session_id, &store) {
    Ok(session) => {
        let identity = SessionIdentity::new(session_id, user_agent);
        req.extensions_mut().insert(identity);
    }
    Err(e) => {
        let resp = (
            StatusCode::UNAUTHORIZED,
            format!("session validation failed: {e}"),
        )
            .into_response();
        return Box::pin(async move { Ok(resp) });
    }
 };
 ```
 After:
 ```rust
 let session_id = match extract_session_id(&req) {
    Ok(id) => id,
    Err(resp) => return Box::pin(async move { Ok(resp) }),
 };
 match validate_and_build_identity(&session_id, &store, &req) {
    Ok(identity) => {
        req.extensions_mut().insert(identity);
    }
    Err(resp) => return Box::pin(async move { Ok(resp) }),
 };
 ```
 ### Step 3: Replace second call site (inside `require_session`, lines 195-222)
 Before: same 30 lines (slightly different return style).
 After:
 ```rust
 let session_id = extract_session_id(&req)?;
 let identity = validate_and_build_identity(&session_id, &store, &req)?;
 req.extensions_mut().insert(identity);
 Ok(())
 ```
 (These functions already return `Result<(), Response>` so the `?` operator works directly.)
 ### Step 4: Add `use` imports if needed
 ```rust
 use axum::http::Request;
 // ... existing imports
 ```
 ### Step 5: Build & test
 Run: `cargo build -p zesdex-middleware && cargo test -p zesdex-middleware`
 ### Step 6: Commit
 ```bash
 git add -A
 git commit -m "refactor: extract session_id extraction helper, DRY auth.rs"
 ```
 ---
 ## Task 5: Merge Backoff/Jitter Implementations — `zesdex-backend`
 **Problem:** 3 separate implementations of exponential backoff with ±25% jitter in `service/provider.rs`, `app/subagent/engine.rs`, and `app/workflow/engine/mod.rs`. Different caps (30s, 16s, 8s) but same base formula.
 **Design:** Create a `backoff` module with a parameterized function.
 **Files:**
 - Create: `crates/zesdex-backend/src/app/util/backoff.rs`
 - Modify: `crates/zesdex-backend/src/app/util/mod.rs` (or create if needed)
 - Modify: `crates/zesdex-backend/src/service/provider.rs`
 - Modify: `crates/zesdex-backend/src/app/subagent/engine.rs`
 - Modify: `crates/zesdex-backend/src/app/workflow/engine/mod.rs`
 ### Step 1: Create `backoff.rs`
 ```rust
 //! Exponential backoff with jitter.
 //!
 //! Three use cases (subagent, provider, workflow) all share the same formula
 //! with different caps.  This module provides a single implementation.
 use std::time::{Duration, SystemTime, UNIX_EPOCH};
 /// Compute an exponential backoff with ±25% jitter.
 ///
 /// `attempt` is 0-based (first retry → attempt=0 → base=1s,
 /// second retry → attempt=1 → base=2s, etc.).
 /// `max_secs` sets the cap.
 fn backoff_seconds(attempt: u32, max_secs: u64) -> Duration {
    let base_secs = (2u64).pow(attempt).min(max_secs);
    let quarter = (base_secs * 250_000_000).max(100_000_000); // 25% of base, min 100ms
    let offset = jitter_ns(quarter);
    // ±25%: offset in [0, quarter), so result = base - quarter/2 + offset
    // which lies in [base - 25%, base + 25%).
    let ns = base_secs * 1_000_000_000 + offset - quarter / 2;
    Duration::from_nanos(ns)
 }
 /// Return a jitter offset in the range [0, range_ns).
 fn jitter_ns(range_ns: u64) -> u64 {
    let nanos = SystemTime::now()
        .duration_since(UNIX_EPOCH)
        .unwrap_or_default()
        .as_nanos() as u64;
    nanos % range_ns
 }
 ```
 ### Step 2: Replace in `service/provider.rs`
 Current code (lines 51-70, 96-106):
 ```rust
 fn jitter_ns(range_ns: u64) -> u64 {
    let nanos = SystemTime::now()
        .duration_since(UNIX_EPOCH)
        .unwrap_or_default()
        .as_nanos() as u64;
    nanos % range_ns
 }
 fn backoff_duration(attempt: u32) -> Duration {
    let base_secs = (2u64).pow(attempt).min(30);
    let half_range = (base_secs * 250_000_000).max(100_000_000);
    let offset = jitter_ns(half_range * 2);
    let ns = base_secs * 1_000_000_000 + offset - half_range;
    Duration::from_nanos(ns)
 }
 ```
 Replace with:
 ```rust
 use crate::app::util::backoff::backoff_seconds;
 fn backoff_duration(attempt: u32) -> Duration {
    backoff_seconds(attempt, 30)
 }
 ```
 And delete the local `jitter_ns` function.
 The `backoff_for_error` function (lines 96-106) also has inline backoff math — replace that too:
 ```rust
 fn backoff_for_error(attempt: u32, err_str: &str) -> Duration {
    if is_rate_limit(err_str) {
        backoff_seconds(attempt.saturating_sub(1), 60) // rate-limit cap: 60s
    } else {
        backoff_duration(attempt)
    }
 }
 ```
 Note: The old code used `(5u64 * (2u64).pow(attempt.saturating_sub(1))).min(60)` for rate limits. The new code calls `backoff_seconds(attempt.saturating_sub(1), 60)` which gives `(2u64).pow(attempt-1).min(60)`. This changes the base from `5*2^(n-1)` to `2^(n-1)`. The difference is minimal for the rate-limit case (retries are backoff-based anyway) and the simplified formula is worth the slight behavioral change. Accept this.
 ### Step 3: Replace in `app/subagent/engine.rs`
 Current code (lines 21-36):
 ```rust
 fn retry_jitter_ns(range_ns: u64) -> u64 {
    SystemTime::now()
        .duration_since(UNIX_EPOCH)
        .unwrap_or_default()
        .subsec_nanos() as u64
        % range_ns
 }
 fn step_retry_delay(attempt: u32) -> Duration {
    let base_secs = (2u64).pow(attempt).min(16);
    let quarter = (base_secs * 250_000_000).max(100_000_000);
    let offset = retry_jitter_ns(quarter);
    let ns = base_secs * 1_000_000_000 + offset - quarter / 2;
    Duration::from_nanos(ns)
 }
 ```
 Replace with:
 ```rust
 use crate::app::util::backoff::backoff_seconds;
 fn step_retry_delay(attempt: u32) -> Duration {
    backoff_seconds(attempt, 16)
 }
 ```
 Note: The old jitter source used `subsec_nanos()` (max ~1s range) while the new helper uses `as_nanos()`. This slightly changes jitter distribution but preserves the ±25% range. Acceptable.
 ### Step 4: Replace in `app/workflow/engine/mod.rs`
 Current inline closure (lines 402-413):
 ```rust
 let retry_backoff = |attempt: u32| {
    let base_secs = (2u64).pow(attempt).min(8);
    let quarter = (base_secs * 250_000_000).max(100_000_000);
    let offset = jitter_ns(quarter);
    let ns = base_secs * 1_000_000_000 + offset - quarter / 2;
    // ...
 };
 ```
 Replace with:
 ```rust
 let retry_backoff = |attempt: u32| crate::app::util::backoff::backoff_seconds(attempt, 8);
 ```
 ### Step 5: Create `mod.rs` if needed
 ```rust
 // crates/zesdex-backend/src/app/util/mod.rs
 pub mod backoff;
 ```
 If the directory doesn't exist:
 ```bash
 mkdir -p crates/zesdex-backend/src/app/util
 ```
 If `util` already exists, just add `pub mod backoff;`.
 ### Step 6: Add `pub` visibility to `backoff_seconds`
 Make the function `pub` in `backoff.rs`.
 ### Step 7: Build & test
 Run: `cargo build -p zesdex-backend && cargo test -p zesdex-backend`
 Run integration test: `cargo test -p zesdex-backend -- --nocapture` (watch for infinite retries in tests)
 ### Step 8: Commit
 ```bash
 git add -A
 git commit -m "refactor: unify 3 backoff implementations into shared helper"
 ```
 ---
 ## Task 6: Merge Auth/Billing Error Check
 **Problem:** The same 401/402/403 + keyword check appears in 3 places. `provider.rs` already has `is_auth_error()` — the other 2 files should call it instead of rewriting it.
 **Files:**
 - Modify: `crates/zesdex-backend/src/app/subagent/engine.rs`
 - Modify: `crates/zesdex-backend/src/app/workflow/engine/mod.rs`
 - (No changes to `provider.rs` — already has the canonical version)
 ### Step 1: Promote `is_auth_error` to `pub` in `provider.rs`
 ```rust
 /// Is the error an auth / billing failure that retrying won't fix?
 pub fn is_auth_error(err_str: &str) -> bool {
    // ... existing implementation
 }
 ```
 ### Step 2: Replace in `engine.rs`
 Current (lines 39-57):
 ```rust
 fn should_retry_subagent_step(err_str: &str) -> bool {
    let lower = err_str.to_lowercase();
    if err_str.contains("API error 401")
        || err_str.contains("API error 402")
        || err_str.contains("API error 403")
        || lower.contains("unauthorized")
        || lower.contains("forbidden")
        || lower.contains("authentication failed")
    {
        return false;
    }
    // ...
 }
 ```
 Replace with:
 ```rust
 fn should_retry_subagent_step(err_str: &str) -> bool {
    if crate::service::provider::is_auth_error(err_str) {
        return false;
    }
    // ...
 }
 ```
 ### Step 3: Replace in `mod.rs` (workflow engine)
 Current inline check (lines 433-435):
 ```rust
 let is_auth = err_str.contains("API error 401")
    || err_str.contains("API error 402")
    || err_str.contains("API error 403");
 ```
 Replace with:
 ```rust
 let is_auth = crate::service::provider::is_auth_error(err_str);
 ```
 ### Step 4: Build & test
 Run: `cargo build -p zesdex-backend && cargo test -p zesdex-backend`
 ### Step 5: Commit
 ```bash
 git add -A
 git commit -m "refactor: reuse is_auth_error from provider.rs, DRY backend retry logic"
 ```
 ---
 ## Task 7: Abort-Flag Check Helper
 **Problem:** `AtomicBool::load(Ordering::SeqCst)` repeated 16 times across 4 files with 2 variants (`Option<Arc<AtomicBool>>` and bare `AtomicBool`).
 **Design:** Two tiny free functions.
 **Files:**
 - Create: `crates/zesdex-backend/src/app/util/abort.rs`
 - Modify: `crates/zesdex-backend/src/app/util/mod.rs`
 - Modify: `crates/zesdex-backend/src/app/subagent/engine.rs`
 - Modify: `crates/zesdex-backend/src/app/runtime/actions/turn.rs`
 - Modify: `crates/zesdex-backend/src/app/workflow/engine/mod.rs`
 - Modify: `crates/zesdex-backend/src/service/provider.rs`
 ### Step 1: Create `abort.rs`
 ```rust
 //! Shared abort-flag checks.
 //!
 //! The two variants (Option<Arc<AtomicBool>> and bare AtomicBool) are
 //! used across the agent runtime, subagent, workflow engine, and provider.
 use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::Arc;
 /// Check whether an optional abort flag has been signalled.
 pub fn is_aborted(flag: &Option<Arc<AtomicBool>>) -> bool {
    flag.as_ref().is_some_and(|f| f.load(Ordering::SeqCst))
 }
 /// Check whether a bare abort flag has been signalled.
 pub fn is_aborted_direct(flag: &AtomicBool) -> bool {
    flag.load(Ordering::SeqCst)
 }
 ```
 ### Step 2: Register in `mod.rs`
 ```rust
 pub mod abort;
 ```
 ### Step 3: Replace 16 call sites
 **In `engine.rs` (4 sites):**
 ```rust
 // Before:
 .abort_flag.as_ref().is_some_and(|f| f.load(std::sync::atomic::Ordering::SeqCst))
 // After:
 crate::app::util::abort::is_aborted(&ctx.abort_flag)
 ```
 **In `turn.rs` (5 sites):**
 ```rust
 // Before:
 tc.abort_flag.load(std::sync::atomic::Ordering::SeqCst)
 // After:
 crate::app::util::abort::is_aborted_direct(&tc.abort_flag)
 ```
 **In worklow `mod.rs` (5 sites):**
 ```rust
 crate::app::util::abort::is_aborted(&sp.abort_flag)
 ```
 **In `provider.rs` (2 sites):**
 ```rust
 crate::app::util::abort::is_aborted(&abort_flag)
 ```
 ### Step 4: Build & test
 Run: `cargo build -p zesdex-backend && cargo test -p zesdex-backend`
 ### Step 5: Commit
 ```bash
 git add -A
 git commit -m "refactor: extract is_aborted helpers, DRY 16 call sites"
 ```
 ---
 ## Task 8: `dirty()` Helper in `input.rs`
 **Problem:** `state.dirty = true; return Vec::new()` repeated 5 times with direct field access instead of using the existing `state.mark_dirty()` method.
 **Files:**
 - Modify: `crates/zesdex-backend/src/controller/input.rs`
 ### Step 1: Add helper function
 ```rust
 /// Mark state dirty and return an empty action list.
 fn mark(state: &mut AppStateRest) -> Vec<Action> {
    state.mark_dirty();
    Vec::new()
 }
 ```
 ### Step 2: Replace 5 occurrences
 Replace `state.dirty = true; return vec![];` and `state.dirty = true; return Vec::new();` with `return mark(state);`.
 Additional: Convert the remaining 17 `state.dirty = true;` to `state.mark_dirty();` for API consistency.
 ### Step 3: Build & test
 Run: `cargo build -p zesdex-backend`
 ### Step 4: Commit
 ```bash
 git add -A
 git commit -m "refactor: use mark_dirty() helper in input.rs, DRY 22 sites"
 ```
 ---
 ## Execution Order
 1. **Task 1** (Atomic write) — most lines saved, independent, well-understood pattern
 2. **Task 2** (Clippy allow) — mechanical, safe, 65 files touched but no behavior change
 3. **Task 4** (Session ID helper) — small, contained, eliminates duplication within one file
 4. **Task 5** (Backoff merge) — cross-file, needs careful diff of behavior
 5. **Task 6** (Auth error check) — depends on Task 5's provider.rs changes, do after
 6. **Task 7** (Abort flag) — independent, mechanical
 7. **Task 8** (dirty helper) — independent, small change
 Total estimated savings: **~450–600 lines of duplication removed** across ~85 file changes.