feat(edda-serve): differentiate HTTP error codes (GH-379)

[fagemx]

Summary

• Add ServiceUnavailable (503) and NotImplemented (501) variants to AppError enum
• Classify open_ledger() errors at the choke point via classify_open_error() helper:
  • "not an edda workspace" → 404 (was 500)
  • "database is locked" → 503 with Retry-After: 1 header (was 500)
• Chronicle-not-enabled guards in analytics/metrics now return 501 (was 500)
• Missing snapshot event in reconstruct_snapshot returns 404 (was 500)
• Remove unused use anyhow::Context imports from 10 handler modules

Test plan

• classify_open_error unit tests: workspace-not-found → NotFound, database-locked → ServiceUnavailable, unknown → Internal
• Integration test: /api/status on bare directory returns 404
• Integration test: /api/recap without chronicle returns 501
• ServiceUnavailable response includes Retry-After: 1 header
• All 112 existing tests still pass (zero regressions)
• cargo clippy and cargo fmt --check clean

Closes #379

🤖 Generated with Claude Code

[fagemx]

Code Review: PR #384

Summary

Properly differentiates HTTP error codes for open_ledger() failures and chronicle-not-enabled guards. Previously these all returned 500; now they return semantically correct 404, 501, and 503. Clean and well-scoped change.

Findings

Blockers

None.

Suggestions

1. error.rs:72-77 — The ServiceUnavailable special case for Retry-After header is checked after constructing (status, code) but before the default return. This works but creates an asymmetry in the response flow — consider moving the header injection into the match arm or using a builder pattern to keep the flow uniform. Minor readability nit, not blocking.

2. error.rs:88 — classify_open_error uses format!("{err:#}") (alternate/pretty format) for string matching. This includes the full error chain. If a future inner error accidentally contains "not an edda workspace" or "database is locked" in its chain, it could misclassify. Low risk given current codebase, but worth noting.

Four-Point Check

Check	Status	Notes
Scope	✅	Matches GH-379 exactly: differentiates 404/501/503 from 500, removes unused .context() imports.
Reality	✅	All types, variants, and APIs are real and verified against edda-ledger and axum APIs.
Testing	✅	3 unit tests for classify_open_error, integration tests for 404 on bare dir and 501 without chronicle, plus Retry-After header test.
YAGNI	✅	No over-engineering; router_no_chronicle test helper is justified for 501 testing.

Verdict

LGTM

---

Reviewed by edda AI

[fagemx]

Code Review: PR #384 (Round 1)

Summary

This PR adds ServiceUnavailable (503) and NotImplemented (501) variants to AppError, introduces classify_open_error() to route ledger open failures to appropriate HTTP status codes, and cleans up unused use anyhow::Context imports. The implementation is clean and well-tested.

Key Findings

Critical Issues (P0)

None.

High Priority (P1)

1. policy.rs:211 bypasses open_ledger() choke point — post_approval_check calls Ledger::open(&state.repo_root).context(...) directly instead of state.open_ledger(). This means the new classify_open_error logic is NOT applied, so this endpoint will still return 500 for "not an edda workspace" or "database is locked" errors. Should be migrated to state.open_ledger()? for consistency with every other handler.
   • File: crates/edda-serve/src/api/policy.rs:211
   • Also has leftover use anyhow::Context (line 3) that would become unused after the fix.

Testing Review

Coverage

• classify_open_error: 3 unit tests covering all branches (NotFound, ServiceUnavailable, Internal) ✅
• Integration: /api/status on bare directory returns 404 ✅
• Integration: /api/recap without chronicle returns 501 ✅
• Integration: ServiceUnavailable response includes Retry-After: 1 header ✅
• All 112 existing tests pass, zero regressions ✅

Convention Compliance

• Tests follow project patterns (#[test], #[tokio::test], real fixtures via tempfile) ✅
• thiserror for error types ✅
• No unwrap in library code ✅

Testing Verdict: Adequate

Verdict: Changes Requested

Fixing P1 issue and will re-review.

---

Round 1 of automated review-fix loop

[fagemx]

Code Review: PR #384 (Round 2) — LGTM 🎉

All P0 and P1 issues have been resolved.

Summary

This PR correctly differentiates HTTP error codes for ledger failures in edda-serve:

• New AppError variants: ServiceUnavailable (503) with Retry-After: 1 header, NotImplemented (501)
• classify_open_error() choke point: Routes "not an edda workspace" to 404, "database is locked" to 503, and unknown errors to 500
• open_ledger() signature change: Returns Result<Ledger, AppError> instead of anyhow::Result<Ledger>, applying classification at the single choke point in AppState
• Chronicle-not-enabled guards: Correctly return 501 instead of 500
• Missing snapshot event: Correctly returns 404 instead of 500
• Cleanup: Removed unused use anyhow::Context from all handler modules

Round 1 fix applied: policy.rs was bypassing open_ledger() — now routed through the choke point like every other handler.

Test Coverage

• 3 unit tests for classify_open_error (all branches)
• Integration tests for 404 on non-workspace, 501 without chronicle, Retry-After header
• All 112 tests pass with zero regressions

Verdict: LGTM ✅

No critical or high-priority issues remaining. This PR is ready for merge.

---

Completed after 2 round(s) of automated review-fix loop