huskies: merge 604_story_service_module_conventions_and_first_extraction

2026-04-24 13:40:47 +00:00
parent 3521649cbf
commit 2f07365745
11 changed files with 1365 additions and 201 deletions
@@ -0,0 +1,29 @@
+# Future Service Module Extractions
+
+Recommended order for extracting remaining HTTP handlers into `service/<domain>/`
+modules, following the conventions in [service-modules.md](service-modules.md).
+
+## Recommended Order
+
+1. **`settings`** — small surface, few dependencies, good warm-up
+2. **`oauth`** — reads/writes token files; pure validation logic separates cleanly
+3. **`wizard`** — stateless generation logic is already mostly pure; thin I/O layer
+4. **`project`** — project scaffolding; wraps `io::fs::scaffold`, clean separation
+5. **`io`** (search/shell) — wraps `io::search` and `io::shell`; pure query-building separable
+6. **`anthropic`** — token-proxy handler; pure request-shaping + thin HTTP I/O
+7. **`stories`** (workflow) — CRDT-backed story ops; typed errors for 400/404/409/500
+8. **`events`** — SSE handler; mostly framework wiring, but event filtering is pure
+
+## Special Case: `ws`
+
+The WebSocket handler (`http/ws.rs`) is a **dedicated harder extraction** because
+it mixes multiple concerns (chat dispatch, permission forwarding, SSE bridging)
+and depends on long-lived async streams. Extract it last, after the above list
+is complete and the service module pattern is well-established.
+
+## Notes
+
+- Each extraction should link back to `docs/architecture/service-modules.md`
+  in the story description to maintain consistency.
+- The `agents` extraction (story 604) is the reference implementation every
+  future extraction should follow.
@@ -0,0 +1,191 @@
+# Service Module Conventions
+
+This document defines the layout, layering rules, and patterns for all service
+modules under `server/src/service/`. Every extraction from the HTTP handlers to
+a service module **must** follow these conventions.
+
+---
+
+## 1. Directory Layout
+
+```
+server/src/service/<domain>/
+    mod.rs      — public API, typed Error, orchestration, integration tests
+    io.rs       — every side-effectful call; the ONLY file that may touch the
+                  filesystem, spawn processes, or call external crates that do
+    <topic>.rs  — pure logic for a named concern within the domain; no I/O
+```
+
+### Rules
+
+- `<domain>` matches the HTTP handler filename (e.g. `agents`, `settings`,
+  `oauth`).
+- **No file named `logic.rs`** — use a descriptive domain name instead
+  (e.g. `selection.rs`, `token.rs`, `validation.rs`).
+- New topic files are added when a pure concern grows beyond ~50 lines or when
+  it has independent test coverage needs.
+
+---
+
+## 2. The Functional-Core / Imperative-Shell Rule
+
+```
+io.rs (imperative shell)  ←→  mod.rs (orchestrator)  ←→  <topic>.rs (functional core)
+```
+
+| Layer | Allowed | Forbidden |
+|-------|---------|-----------|
+| `<topic>.rs` | Pure Rust, data-transformation, branching logic, pattern matching | Any I/O |
+| `io.rs` | `std::fs`, `std::process`, `tokio::fs`, network calls, `SystemTime::now` | Business logic beyond a thin wrapper |
+| `mod.rs` | Calls into `io.rs` and `<topic>.rs`; owns the `Error` type | Direct I/O without going through `io.rs` |
+
+**Grep-enforceable check:** The following must NOT appear in any `service/<domain>/` file other than `io.rs`:
+
+- `std::fs`
+- `std::process`
+- `std::thread::sleep`
+- `tokio::fs`
+- `reqwest`
+- `SystemTime::now`
+
+---
+
+## 3. Error Type Pattern
+
+Each service domain declares its own typed error enum in `mod.rs`:
+
+```rust
+/// Errors returned by `service::agents` operations.
+#[derive(Debug)]
+pub enum Error {
+    ProjectRootNotConfigured,
+    AgentNotFound(String),
+    WorkItemNotFound(String),
+    WorktreeError(String),
+    ConfigError(String),
+    IoError(String),
+}
+
+impl std::fmt::Display for Error { ... }
+```
+
+HTTP handlers map service errors to **specific** HTTP status codes:
+
+| Error variant | HTTP status |
+|--------------|-------------|
+| `ProjectRootNotConfigured` | 400 Bad Request |
+| `AgentNotFound` | 404 Not Found |
+| `WorkItemNotFound` | 404 Not Found |
+| `WorktreeError` | 400 Bad Request |
+| `ConfigError` | 400 Bad Request |
+| `IoError` | 500 Internal Server Error |
+
+**No generic `bad_request` for everything** — distinguish 400 vs 404 vs 500.
+
+---
+
+## 4. Test Pattern
+
+### Pure topic files (`<topic>.rs`)
+
+```rust
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    // Unit tests MUST:
+    //   - Use no tempdir, tokio runtime, or filesystem
+    //   - Cover every branch of every public function
+    #[test]
+    fn filter_removes_archived_agents() { ... }
+}
+```
+
+### `io.rs`
+
+```rust
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use tempfile::TempDir;
+
+    // IO tests MAY use tempdirs and real filesystem.
+    // Keep them few and focused on the thin I/O wrapper contract.
+    #[test]
+    fn is_archived_returns_true_when_in_done() { ... }
+}
+```
+
+### `mod.rs`
+
+```rust
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    // Integration tests compose io + pure layers end-to-end.
+    // May use tempdirs. Keep the count small — they are integration-level.
+    #[tokio::test]
+    async fn list_agents_excludes_archived() { ... }
+}
+```
+
+---
+
+## 5. Dependency Injection Pattern
+
+Service functions take **only the dependencies they actually use**:
+
+```rust
+// Good — takes only what it needs
+pub async fn start_agent(
+    pool: &AgentPool,
+    project_root: &Path,
+    story_id: &str,
+    agent_name: Option<&str>,
+) -> Result<AgentInfo, Error> { ... }
+
+// Bad — takes the whole AppContext
+pub async fn start_agent(ctx: &AppContext, ...) -> Result<AgentInfo, Error> { ... }
+```
+
+Standard injected dependencies for `service::agents`:
+
+| Type | Purpose |
+|------|---------|
+| `&AgentPool` | Agent lifecycle operations |
+| `&Path` (`project_root`) | Filesystem operations scoped to the project |
+| `&WorkflowState` | In-memory test result cache |
+
+**The dependency set chosen for `agents` is the reference pattern for all future
+service module extractions.**
+
+---
+
+## 6. HTTP Handler Contract
+
+After extraction, HTTP handlers are thin adapters:
+
+```rust
+async fn start_agent(&self, payload: Json<StartAgentPayload>) -> OpenApiResult<...> {
+    let project_root = self.ctx.agents.get_project_root(&self.ctx.state)
+        .map_err(|e| bad_request(e))?;                  // extract from AppContext
+    let info = service::agents::start_agent(             // call service
+        &self.ctx.agents, &project_root, &payload.story_id, payload.agent_name.as_deref(),
+    ).await.map_err(map_service_error)?;                 // map typed error → HTTP
+    Ok(Json(AgentInfoResponse { ... }))                  // shape DTO
+}
+```
+
+Handlers must contain **no**:
+- `std::fs` / file reads
+- `std::process` invocations
+- Inline load-mutate-save sequences
+- Inline validation that belongs in the service layer
+
+---
+
+## 7. Follow-up Extractions
+
+See [future-extractions.md](future-extractions.md) for the recommended order
+and rationale for remaining extraction targets.