diff --git a/plan/output.md b/plan/output.md index 8e27995..55db628 100644 --- a/plan/output.md +++ b/plan/output.md @@ -28,6 +28,14 @@ say which they are. ## What it needs +- [ ] A knot answers `RepoNotFound` with **204 No Content** in some handlers + (`knotserver/xrpc/repo_tag.go` and its neighbours, read on 2026-08-26), + and `knot::xrpc` treats any 2xx as success — so that refusal arrives as + an empty body rather than as an error, and whatever reads it sees an + empty answer instead of a missing repo. Nothing in the tree calls those + particular methods through the authenticated funnel today, which is why + this is an entry and not a fix; the shape to watch for is a knot read + that comes back suspiciously empty - [ ] None of the four listings has an upper bound except `search`, whose 1000 is Bobbin's own ceiling. `--limit 0` refusing now reads as "0 is not in 1..=4294967295", which is accurate and ugly. A ceiling would fix @@ -37,15 +45,6 @@ say which they are. - [ ] `-v` has no messages yet: the ladder has room for a verbose rung between `step` and `debug`, and nothing emits at it. Add it when there is a line that wants it, not before -- [ ] The knot funnel in `clients/tangled/knot.rs` is the one left. `Refused` - is a typed error carrying the knot's HTTP status and its `AccessControl` - tag, and `exit::classify` only downcasts `Coded`, so every refusal - through it — `pr merge`, `stack merge`, `repo delete-branch` — still - exits `1` with the answer sitting in a field. Two shapes work and the - choice is real: give `Refused` an `exit` field and teach `classify` to - ask it, or convert at the call sites the way `repo/` now does. The - first is less code and widens what `classify` knows about; the second - keeps exit.rs's "one error carries the code" rule intact - [ ] The `"?"` sentinel is round-tripped through three modules while docs/output.md says unknown is `null`, never `"?"`. `State` is a proper enum, `label()` flattens `Unknown` to the string, `StackRow.state` and @@ -57,6 +56,30 @@ say which they are. ## Done +- [x] The knot funnel in `clients/tangled/knot.rs` was the one left, and it + is the one that turned out not to be a status question. `Refused` is a + typed error carrying the knot's HTTP status and its error tag, and + `exit::classify` only downcast `Coded`, so every refusal through it — + `pr merge`, `stack merge`, `repo delete-branch`, `api` — exited `1` + with the answer sitting unread in a field. + Neither shape this entry proposed survived contact. Converting at the + call sites would have been the same mapping written five times, and + *either* of them classifying by status would have been wrong, because a + knot does not spell one refusal with one status: read off + `knotserver/xrpc/` on 2026-08-26, `AccessControl` comes back **401** + from `pushableRepoDID` — the shared push-access guard every merge and + branch deletion passes through — **403** from the membership and + collaborator handlers, and **400** from `delete_repo`; `RepoNotFound` + comes back **404**, **400** and **204**. Classifying by number answers + "log in", "not as this account" and "fix your command line" for one + problem, depending on which handler the call reached. + So `Refused::exit` reads the *tag* for the handful whose meaning is + unambiguous and defers to `exit::from_status` for everything else, and + `classify` asks it — which it can, since it has always reached into the + chain for `reqwest::Error`. The one call site that already coded its + own answer, `stack merge`'s `AccessControl` sentence about push access, + still outranks it + - [x] Output channels — one rule, on every flag: stdout is the answer, everything else is on stderr. Warnings, notes and progress lines all go through `crate::term::say` at a level (`warn`/`note`/`step`/`debug`) and diff --git a/src/clients/tangled/knot.rs b/src/clients/tangled/knot.rs index 62a7f75..35b0482 100644 --- a/src/clients/tangled/knot.rs +++ b/src/clients/tangled/knot.rs @@ -128,6 +128,41 @@ pub struct Refused { pub detail: String, } +impl Refused { + /// The status atgc should exit with, from the knot's answer. + /// + /// **The tag first, because one refusal is not one status.** A knot + /// spells the same error with whatever number the handler that raised it + /// happened to pass, and the spread is not small (read off + /// `knotserver/xrpc/` in tangled.org/@tangled.sh/core on 2026-08-26): + /// + /// - `AccessControl` comes back **401** from `pushableRepoDID`, the + /// shared push-access guard every merge and branch deletion goes + /// through, **403** from the membership and collaborator handlers, and + /// **400** from `delete_repo`. + /// - `RepoNotFound` comes back **404**, **400**, and **204**. + /// + /// Classifying those by number would answer "log in", "not as this + /// account" and "fix your command line" for one problem, depending on + /// which handler the call reached. The tag is the part the knot is + /// consistent about, so it decides where it can, and everything else + /// falls through to [`crate::exit::from_status`], which is the one table + /// for what a status means. + /// + /// Only the tags whose meaning is unambiguous are listed. This is a + /// mapping of somebody else's vocabulary and it is worth no more than + /// that reading, so a tag not named here is left to its status rather + /// than guessed at. + pub fn exit(&self) -> crate::exit::Exit { + match self.tag.as_str() { + "AccessControl" => crate::exit::Exit::Denied, + "RepoNotFound" | "OwnerNotFound" | "RefNotFound" => crate::exit::Exit::NotFound, + "RepoExists" | "RecordExists" => crate::exit::Exit::Conflict, + _ => crate::exit::from_status(self.status), + } + } +} + impl std::fmt::Display for Refused { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let Refused { diff --git a/src/exit.rs b/src/exit.rs index fd4c744..c34276a 100644 --- a/src/exit.rs +++ b/src/exit.rs @@ -157,15 +157,36 @@ pub fn fail(exit: Exit, message: impl fmt::Display) -> anyhow::Error { /// The status this failure should exit with. /// -/// A [`fail`] anywhere in the chain wins. Failing that, a `reqwest::Error` -/// anywhere in it that reports a connect failure or a timeout means the host -/// never answered, which is [`Unreachable`](Exit::Unreachable) whether or not -/// anyone said so. Anything else is [`Failure`](Exit::Failure) — the status -/// every failure had before this module existed. +/// A [`fail`] anywhere in the chain wins, because somebody said so at the +/// call site and that outranks anything derived here. Failing that, a knot's +/// [`Refused`](crate::clients::tangled::knot::Refused) says what it earns — +/// from its error *tag* where the knot's own statuses for one refusal +/// disagree with each other, and from [`from_status`] otherwise. Failing +/// *that*, a `reqwest::Error` anywhere in the chain +/// reporting a connect failure or a timeout means the host never answered, +/// which is [`Unreachable`](Exit::Unreachable) whether or not anyone said so. +/// Anything else is [`Failure`](Exit::Failure) — the status every failure had +/// before this module existed. +/// +/// The knot arm is here rather than at the call sites, which was the other +/// way it could have gone. Every knot call in the tree goes through one +/// funnel and none of them wants a *different* answer from the table, so a +/// conversion per call site would be the same mapping written five times and +/// forgotten on the sixth — `pr merge`, `stack merge`, `repo delete-branch` +/// and `api` all exited `1` with the knot's status sitting unread in a field. +/// This is not a new kind of knowledge for this function either: it has +/// always reached into the chain for `reqwest::Error`. pub fn classify(err: &anyhow::Error) -> Exit { if let Some(coded) = err.downcast_ref::() { return coded.exit; } + if let Some(refused) = err + .chain() + .filter_map(|e| e.downcast_ref::()) + .next() + { + return refused.exit(); + } let unreachable = err .chain() .filter_map(|e| e.downcast_ref::()) @@ -345,6 +366,113 @@ mod tests { /// free. Driven through a real `reqwest::Error` — built by asking a /// client with a 1ms timeout for an unroutable TEST-NET-3 address /// (RFC 5737), so nothing leaves this machine. + /// A knot's refusal exits as its status says, not as an unclassified 1. + /// + /// The knot funnel was the last error in the tree carrying a status + /// nothing read. `pr merge` on a repo you have no push access to answered + /// `403 AccessControl` and exited `1`, which is the code for "the message + /// is the only description" — on a failure whose whole point is that it + /// is a permissions problem somebody else can fix. + #[test] + fn a_knot_refusal_exits_as_its_status() { + use crate::clients::tangled::knot::Refused; + let refused = |status: reqwest::StatusCode| { + classify(&anyhow::Error::new(Refused { + knot: "knot1.tangled.sh".to_string(), + nsid: "sh.tangled.repo.merge".to_string(), + status, + tag: "AccessControl".to_string(), + detail: "no push access".to_string(), + })) + }; + // One tag, three statuses, one answer. A knot spells `AccessControl` + // 401 from its shared push guard, 403 from the membership handlers + // and 400 from delete_repo, and "you do not have access" is the same + // problem in all three. + for status in [ + reqwest::StatusCode::UNAUTHORIZED, + reqwest::StatusCode::FORBIDDEN, + reqwest::StatusCode::BAD_REQUEST, + ] { + assert_eq!(refused(status), Exit::Denied, "{status}"); + } + } + + /// A tag the mapping does not name falls through to the status table. + /// + /// The table is the general answer and the tags are the exceptions to it, + /// not a replacement: a knot error this build has never heard of still + /// gets whatever its status earns. + #[test] + fn an_unmapped_tag_is_left_to_its_status() { + use crate::clients::tangled::knot::Refused; + let refused = |tag: &str, status: reqwest::StatusCode| { + classify(&anyhow::Error::new(Refused { + knot: "knot1.tangled.sh".to_string(), + nsid: "sh.tangled.repo.merge".to_string(), + status, + tag: tag.to_string(), + detail: "no".to_string(), + })) + }; + assert_eq!( + refused("Git", reqwest::StatusCode::CONFLICT), + Exit::Conflict + ); + assert_eq!( + refused( + "SomethingNewerThanThisBuild", + reqwest::StatusCode::NOT_FOUND + ), + Exit::NotFound + ); + // And a status the table has no answer for keeps the unclassified 1, + // which is what "the message is the only description" is for. + assert_eq!( + refused("Git", reqwest::StatusCode::INTERNAL_SERVER_ERROR), + Exit::Failure + ); + } + + /// And it survives the context a caller wraps it in. + /// + /// Every real refusal reaches `main` under at least one `.context`, so a + /// check that only looked at the outermost error would pass this module's + /// tests and fix nothing. + #[test] + fn a_wrapped_knot_refusal_still_exits_as_its_status() { + use anyhow::Context; + let err = Err::<(), _>(anyhow::Error::new(crate::clients::tangled::knot::Refused { + knot: "knot1.tangled.sh".to_string(), + nsid: "sh.tangled.repo.merge".to_string(), + status: reqwest::StatusCode::FORBIDDEN, + tag: String::new(), + detail: "no".to_string(), + })) + .context("merging pull 12") + .unwrap_err(); + assert_eq!(classify(&err), Exit::Denied); + } + + /// A `fail` still outranks it. `stack merge` turns an `AccessControl` + /// refusal into a sentence about push access and codes it itself; that + /// decision is made at a call site that knows more than the table does, + /// and must not be re-derived here. + #[test] + fn an_explicit_code_outranks_the_knots_status() { + let err = fail( + Exit::Usage, + anyhow::Error::new(crate::clients::tangled::knot::Refused { + knot: "knot1.tangled.sh".to_string(), + nsid: "sh.tangled.repo.merge".to_string(), + status: reqwest::StatusCode::FORBIDDEN, + tag: String::new(), + detail: "no".to_string(), + }), + ); + assert_eq!(classify(&err), Exit::Usage); + } + #[tokio::test] async fn a_host_that_never_answered_is_unreachable_untagged() { let slow = reqwest::Client::builder() diff --git a/tests/pr_flows.rs b/tests/pr_flows.rs index 3827e71..c1e0ca1 100644 --- a/tests/pr_flows.rs +++ b/tests/pr_flows.rs @@ -881,9 +881,14 @@ fn a_knot_that_denies_push_access_stops_the_merge_after_asking() { let uri = format!("at://{ALICE}/{PULL_NSID}/{rkey}"); world.with(|w| w.knot_push_allowed = Some(vec![ALICE.to_string()])); + // Exit 4, not the unclassified 1: the knot answered `AccessControl`, and + // a script that has to tell "you may not do this" from "something went + // wrong" reads the code rather than the sentence. The knot spells that + // one refusal 401 here, 403 from its membership handlers and 400 from + // `delete_repo`, so the *tag* is what decides — see `knot::Refused::exit`. world .run_as(BOB, &["pr", "merge", &uri]) - .refused("push access"); + .refused_with(4, "push access"); world.with(|w| { assert_eq!(