Move miette to the edges; use thiserror in quire/mod.rs internals
quire/mod.rs was library code that leaked miette's Result alias,
bail!/ensure! macros, and IntoDiagnostic into its internals. These
belong only at CLI/binary boundaries.
- Add Error::InvalidRepoName to the top-level typed error enum
- Replace ensure! calls in validate_name with explicit if/return Err(...)
- Replace .into_diagnostic().context() in from_path with .map_err(|_| ...)
- Drop the miette import entirely; return crate::Result throughout
- Update AGENTS.md to codify the library vs. edge boundary rule
CLI command handlers (commands/*.rs) and binary entry points
(main.rs, quire-ci) are unaffected — they remain the correct home
for bail!, ensure!, IntoDiagnostic, and miette::Result.
diff --git a/AGENTS.md b/AGENTS.md
index bc837d9..7758f33 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -12,10 +12,20 @@ The backlog shifts between sessions and even within a session — tasks get reor
## Error handling
-Prefer `bail!` and `ensure!` from miette over constructing errors manually with
-`miette::miette!()` and `.ok_or_else()`. Use `bail!` for early returns and
-`ensure!` for precondition checks. `IntoDiagnostic` (via `.into_diagnostic()`)
-is the right bridge for non-miette error types.
+**Library code** (anything that isn't a binary entry point or CLI command handler)
+must use typed error enums derived with `thiserror`. Return `Result<T, SomeError>`
+— never `miette::Result`. Do not use `bail!`, `ensure!`, or `IntoDiagnostic` in
+library code; construct errors explicitly.
+
+**CLI and binary entry points** (`main.rs`, `commands/`) are the miette boundary.
+Use `bail!` and `ensure!` for ad-hoc checks. Use `?` to propagate typed errors
+upward — it converts them to `miette::Report` automatically because every typed
+error in this codebase implements `miette::Diagnostic`. Use `IntoDiagnostic` (via
+`.into_diagnostic()`) only to bridge non-miette, non-`Diagnostic` error types like
+`std::io::Error` from third-party calls.
+
+`miette::Context` (`.context()`, `.with_context()`) is available in CLI code to
+add human-readable narrative around propagated errors.
## Before committing
diff --git a/quire-server/src/error.rs b/quire-server/src/error.rs
index 58f1798..87181a7 100644
--- a/quire-server/src/error.rs
+++ b/quire-server/src/error.rs
@@ -12,6 +12,9 @@ pub enum Error {
#[error("config not found: {0}")]
ConfigNotFound(String),
+ #[error("{0}")]
+ InvalidRepoName(String),
+
#[error(transparent)]
#[diagnostic(transparent)]
Fennel(#[from] Box<FennelError>),
diff --git a/quire-server/src/quire/mod.rs b/quire-server/src/quire/mod.rs
index 589b26e..035297c 100644
--- a/quire-server/src/quire/mod.rs
+++ b/quire-server/src/quire/mod.rs
@@ -2,12 +2,10 @@ use std::collections::HashMap;
use std::path::{Path, PathBuf};
use std::sync::{Arc, Mutex, OnceLock};
-use miette::{Context, IntoDiagnostic, Result, ensure};
-
pub mod web;
use crate::ci::{Ci, Executor, Runs};
-use crate::{Error, Result as AppResult};
+use crate::{Error, Result};
pub use quire_core::telemetry::SentryConfig;
use quire_core::fennel::Fennel;
@@ -87,13 +85,12 @@ impl Repo {
/// Verifies the path falls under `base` and passes name validation.
/// Used by hooks that receive `GIT_DIR` from git.
pub fn from_path(repos_base: &Path, path: &Path) -> Result<Self> {
- let relative = path
- .strip_prefix(repos_base)
- .into_diagnostic()
- .context(format!(
+ let relative = path.strip_prefix(repos_base).map_err(|_| {
+ Error::InvalidRepoName(format!(
"path is not under repos directory: {}",
path.display()
- ))?;
+ ))
+ })?;
let name = relative.to_string_lossy();
Self::validate_name(&name)?;
Ok(Self {
@@ -103,26 +100,40 @@ impl Repo {
}
fn validate_name(name: &str) -> Result<()> {
- ensure!(!name.is_empty(), "repository name cannot be empty");
- ensure!(!name.contains(".."), "invalid repository name: {name}");
- ensure!(
- name.ends_with(".git"),
- "repository name must end in .git: {name}"
- );
- ensure!(!name.contains("//"), "invalid repository name: {name}");
+ if name.is_empty() {
+ return Err(Error::InvalidRepoName(
+ "repository name cannot be empty".to_string(),
+ ));
+ }
+ if name.contains("..") {
+ return Err(Error::InvalidRepoName(format!(
+ "invalid repository name: {name}"
+ )));
+ }
+ if !name.ends_with(".git") {
+ return Err(Error::InvalidRepoName(format!(
+ "repository name must end in .git: {name}"
+ )));
+ }
+ if name.contains("//") {
+ return Err(Error::InvalidRepoName(format!(
+ "invalid repository name: {name}"
+ )));
+ }
let segments = name.split('/').collect::<Vec<_>>();
- ensure!(
- segments.len() <= 2,
- "repository name allows at most one level of grouping: {name}"
- );
+ if segments.len() > 2 {
+ return Err(Error::InvalidRepoName(format!(
+ "repository name allows at most one level of grouping: {name}"
+ )));
+ }
for seg in &segments {
- ensure!(!seg.is_empty(), "invalid repository name: {name}");
- ensure!(
- *seg != "." && *seg != ".." && *seg != ".git",
- "invalid repository name: {name}"
- );
+ if seg.is_empty() || *seg == "." || *seg == ".." || *seg == ".git" {
+ return Err(Error::InvalidRepoName(format!(
+ "invalid repository name: {name}"
+ )));
+ }
}
Ok(())
@@ -266,7 +277,7 @@ impl Quire {
///
/// Re-reads on every call. Cheap at current call volume; revisit if
/// `quire serve` ends up loading per-request.
- pub fn global_config(&self) -> AppResult<GlobalConfig> {
+ pub fn global_config(&self) -> Result<GlobalConfig> {
let config_path = self.config_path();
if !config_path.exists() {
return Err(Error::ConfigNotFound(config_path.display().to_string()));