Skip to content

docs: fix error-handling.md to reflect shared ApiError in agentd-common#724

Open
geoffjay wants to merge 2 commits intomainfrom
docs/fix-error-handling-accuracy
Open

docs: fix error-handling.md to reflect shared ApiError in agentd-common#724
geoffjay wants to merge 2 commits intomainfrom
docs/fix-error-handling-accuracy

Conversation

@geoffjay
Copy link
Copy Markdown
Owner

Summary

  • Fixes inaccurate ApiError documentation — the existing docs described an outdated per-service pattern with 3 variants; the actual shared type in crates/common/src/error.rs has 7 variants with different signatures
  • Documents both usage patterns — re-export (notify, orchestrator, communicate, memory) vs. custom error type (ask, hook, monitor)
  • Removes the incorrect variant tableAgentNotRunning does not exist; orchestrator uses ApiError::Conflict
  • Fixes server init docs — replaces inline tracing_subscriber snippet with agentd_common::server::init_tracing()
  • Adds missing CORS docscors_layer() and AGENTD_CORS_ORIGINS were undocumented

Motivation

The refactor agent filed issues #720#723 against crates/common, flagging the crate for improvements. In reviewing the code for those issues I found the developer docs significantly diverged from reality — services were documented as defining their own ApiError when most re-export the shared type, and the variant list was stale.

Test plan

  • Verify all code snippets compile by cross-referencing with crates/common/src/error.rs, crates/common/src/server.rs, and service api.rs files
  • Confirm the two usage-pattern tabs render correctly in MkDocs Material (=== "...")
  • Check that links to source files resolve (../../crates/common/src/error.rs)

🤖 Generated with Claude Code

- Replace per-service ApiError pattern with accurate shared type from crates/common/src/error.rs
- Document all 7 variants (was showing only 3) with correct signatures
- Show two usage patterns: re-export (notify/orchestrator/communicate/memory) vs custom (ask/hook/monitor)
- Fix IntoResponse example to match actual implementation
- Replace inline init_tracing() snippet with agentd_common::server::init_tracing()
- Add cors_layer() documentation and AGENTD_CORS_ORIGINS env var
- Remove incorrect AgentNotRunning variant (orchestrator uses ApiError::Conflict)
@geoffjay geoffjay added the review-agent Used to invoke a review by an agent tracking this label label Mar 23, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.66%. Comparing base (a5867f0) to head (993333b).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #724   +/-   ##
=======================================
  Coverage   55.66%   55.66%           
=======================================
  Files         126      126           
  Lines       13759    13760    +1     
=======================================
+ Hits         7659     7660    +1     
  Misses       6100     6100           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@geoffjay
Copy link
Copy Markdown
Owner Author

👀 Conductor: Awaiting reviewer — 39 minutes with no review activity.

CI: PASSED — this PR is ready for review. No reviewer has commented or approved yet.

Nudge posted by conductor pipeline sync at 2026-03-23 17:01 UTC. Communicate service offline; using PR comment as fallback.

Copy link
Copy Markdown
Owner Author

@geoffjay geoffjay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: docs: fix error-handling.md to reflect shared ApiError in agentd-common

Stack position: docs/fix-error-handling-accuracy is directly on main
but git-spice reports (needs restack) — the branch is behind main and
must be rebased before the conductor can merge it.

The documentation is substantially correct and a clear improvement over what
it replaces. All stated facts verified against source. Two non-blocking
observations below.


Verification results

Claim Verified
7 ApiError variants with correct signatures ✅ matches crates/common/src/error.rs exactly
#[error(transparent)] on Internal
Status code table
IntoResponse snippet ✅ matches current production code (lines 69-80)
Re-export crates: notify, orchestrator, communicate, memory ✅ all four confirmed with pub use agentd_common::error::ApiError
Custom error crates: ask, hook, monitor ✅ all three have dedicated error.rs with custom ApiError
ask variant list and status codes ✅ (see note below)
init_tracing() call pattern ✅ matches crates/common/src/server.rs
cors_layer() and AGENTD_CORS_ORIGINS

Observation 1 — wrap is missing from the custom error type list

The crates/wrap/src/api.rs crate defines its own ApiError inline (line
372) rather than re-exporting the shared type:

// crates/wrap/src/api.rs
#[derive(Debug, thiserror::Error)]
pub enum ApiError {
    NotFound(String),   // takes a String — not the shared unit NotFound
    Internal(),
    // …
}

It belongs in the custom error list alongside ask, hook, and monitor,
but the docs currently say:

"This is the pattern used by ask, hook, and monitor."

Adding wrap to that sentence (and noting that its ApiError lives in
api.rs rather than a dedicated error.rs) would make the enumeration
accurate. This is non-blocking — the stated facts about ask/hook/monitor are
all correct, and wrap being omitted doesn't break anything — but a developer
checking a specific crate against the docs would find a gap.

Observation 2 — ask snippet silently omits InvalidRequest

The ask custom error snippet shows five variants. The actual
crates/ask/src/error.rs has a sixth: InvalidRequest(String) (HTTP 400).
Since the snippet is introduced with // crates/ask/src/error.rs and
pub enum ApiError { without a truncation marker (// …), it reads as the
complete definition. Adding a // … or including the missing variant would
prevent the docs from diverging from reality as ask evolves. Non-blocking.


Forward compatibility note (informational)

The IntoResponse snippet correctly documents the current code:

ApiError::Internal(e) => (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()),

PR #739 (pending) refactors into_response to use self.status_code(),
which will change this arm. Once #739 merges, the snippet will need a
one-line update. Not a blocker for this PR.


Action required before merge

Rebase onto main:

git-spice branch restack
git-spice branch submit

Once restacked the two observations above can be addressed in the same
commit or as a follow-up — neither blocks merge.

@geoffjay geoffjay added needs-restack Branch is behind its stack parent, needs git-spice restack and removed review-agent Used to invoke a review by an agent tracking this label labels Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-restack Branch is behind its stack parent, needs git-spice restack

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant