diff --git a/README.md b/README.md index 989ba75..daefed1 100644 --- a/README.md +++ b/README.md @@ -221,7 +221,7 @@ repo it is numbered against. #### Issues ``` -atgc issue create --title '…' # file an issue against this checkout's repo +atgc issue create --title '…' --body '…' # file one against this checkout's repo atgc issue list # your issues on this repo (--all, --author, --state) atgc issue view # one issue, read live from its author's PDS atgc issue comment # comment (--body/--body-file) @@ -236,6 +236,12 @@ separate newest-wins log. So filing one needs no permission on the repo, and closing somebody else's issue on a repo you own writes a record into *your* PDS rather than theirs. +A body is required even though the lexicon marks it optional: tangled.org's +ingester refuses an issue whose body is empty and drops the record without +retry, so a title-only issue would sit in your PDS, federate, and never +appear. `atgc issue create` refuses it up front instead, and `issue edit` +will not empty one either. + Two limits worth knowing before you reach for them. `issue list` is one account's issues and says so on stderr every time — "every issue on this repo, whoever filed it" needs the appview index, which atgc does not read for issues diff --git a/TODO.md b/TODO.md index c373570..c26d53f 100644 --- a/TODO.md +++ b/TODO.md @@ -1121,6 +1121,16 @@ and borrows `pr`'s conventions rather than inventing new ones beside them. - [x] `issue create` — a record in your own PDS naming the repo by its DID, so it needs no permission on the repo, exactly as `pr create` does not. `--body-file` (and `-` for stdin) rather than an `$EDITOR` spawn +- [x] A body is *required*, though `issue.json` marks it optional. The + appview's `Issue.Validate` refuses `issue body is empty` and its + ingester logs `failed to ingest record, dropping it without retry`, so + a title-only issue is accepted by the PDS, federates, and never appears + — the same silent shape as a comment in the deprecated collection, + arrived at from the other direction. `issue create` refuses it before + the write and `issue edit` will not empty one, since an update the + appview drops leaves it showing a paragraph the PDS no longer holds. + Pulls are exempt from the equivalent check when the patch is a + format-patch, which is why `pr create` needs no such rule - [x] `issue view` — by record key or at:// URI, read live from the author's PDS. No branch fallback: an issue record names no branch, so there would be nothing to guess from diff --git a/docs/architecture.md b/docs/architecture.md index 6f9142a..6f4160d 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -44,6 +44,12 @@ enumerates. Its **state** is a separate `sh.tangled.repo.issue.state` record and the newest wins, exactly as a pull's is. Its **number** is the appview's too. +Where the lexicon and the appview disagree, the appview decides what exists. +`sh.tangled.repo.issue` marks `body` optional; tangled.org's ingester refuses +a record whose body is empty and drops it without retry. A schema is what a +PDS will accept, not what the network will show, and atgc refuses at the +narrower of the two. + Comments are where the two converge in the record itself: an issue comment and a pull comment are both `sh.tangled.feed.comment`, told apart by what their `subject` points at. The per-collection comment records the lexicons still diff --git a/src/cmd/issue/mod.rs b/src/cmd/issue/mod.rs index 8c5444c..d5e9a62 100644 --- a/src/cmd/issue/mod.rs +++ b/src/cmd/issue/mod.rs @@ -43,10 +43,13 @@ pub(crate) enum Command { /// exactly as `pr create` does not. The repo is the one this checkout's /// `origin` points at unless `--remote` says otherwise. /// - /// There is no editor. Use `--body-file -` and a heredoc for a long one. + /// A body is required, though the lexicon says otherwise: tangled.org's + /// ingester drops an issue whose body is empty, so a title-only record + /// would federate and never appear. There is no editor — use + /// `--body-file -` and a heredoc for a long one. /// /// Examples: - /// atgc issue create --title 'pr list hangs on a big repo' + /// atgc issue create --title 'pr list hangs' --body 'on a 40k-commit repo' /// atgc issue create --title 'typo in README' --body 'second paragraph' /// atgc issue create --title 'long one' --body-file - <<'EOF' #[command(verbatim_doc_comment)] diff --git a/src/cmd/issue/write.rs b/src/cmd/issue/write.rs index 175c53b..591e188 100644 --- a/src/cmd/issue/write.rs +++ b/src/cmd/issue/write.rs @@ -156,11 +156,19 @@ pub(super) struct CommentedJson { /// composes with a heredoc, which an editor does not. /// /// The nested `Option` is the whole point. `None` is "not asked for" and -/// `Some(None)` is "asked for, and the answer is no body" — the distinction -/// that lets `issue edit --body ''` clear a body instead of being a no-op. +/// `Some(None)` is "asked for, and the answer is no body" — which is the +/// distinction that lets [`edit`] *refuse* an emptying. Collapse the two and +/// `--body ''` becomes indistinguishable from not passing `--body` at all, +/// so the refusal below could not be made and the request would be silently +/// ignored instead. See [`empty_body_refusal`] for why it is a refusal. fn new_body(body: Option, body_file: Option) -> Result>> { let text = match (body, body_file) { - (Some(_), Some(_)) => bail!("pass --body or --body-file, not both"), + (Some(_), Some(_)) => { + return Err(crate::exit::fail( + crate::exit::Exit::Usage, + "pass --body or --body-file, not both", + )); + } (Some(text), None) => text, (None, Some(path)) if path == "-" => { use std::io::Read; @@ -187,15 +195,49 @@ fn new_body(body: Option, body_file: Option) -> Result, body_file: Option) -> Result { match new_body(body, body_file)? { Some(Some(text)) => Ok(text), - Some(None) => bail!("a comment needs a body; that one is empty"), - None => bail!( + Some(None) => Err(crate::exit::fail( + crate::exit::Exit::Usage, + "a comment needs a body; that one is empty", + )), + None => Err(crate::exit::fail( + crate::exit::Exit::Usage, "nothing to say — pass --body, or --body-file (with `-` for stdin)\n\ atgc does not open an editor: it is built to run unattended, so `--body-file -` \ - and a heredoc are the long-body path" - ), + and a heredoc are the long-body path", + )), } } +/// An issue body is required, whatever the lexicon says, and this is the +/// refusal. +/// +/// `sh.tangled.repo.issue` marks `body` optional and tangled.org does not +/// honour that: the appview's `Issue.Validate` refuses `body is empty`, and +/// its ingester turns a validation failure into `failed to ingest record, +/// dropping it without retry`. A title-only issue is therefore accepted by +/// the PDS, federates, and never appears anywhere — the same silent shape +/// [`crate::lexicon::tangled::LEGACY_ISSUE_COMMENT_NSID`] describes for a +/// comment in the wrong collection, arrived at from the other direction. +/// +/// Refused here rather than warned about, because the record cannot be +/// unpublished once written and nothing inside the process can observe that +/// it went nowhere. `what` names which half is refusing: filing an issue +/// without a body, or emptying one that has it. +fn empty_body_refusal(what: &str) -> anyhow::Error { + crate::exit::fail( + crate::exit::Exit::Usage, + format!( + "{what}\n\ + the lexicon marks an issue's body optional and tangled.org does not: its \ + ingester refuses a record whose body is empty (`issue body is empty`) and drops \ + it without retry, so the record would sit in your PDS, federate, and never \ + appear\n\ + atgc does not open an editor: `--body-file -` and a heredoc are the long-body \ + path" + ), + ) +} + /// A body, described rather than printed. An issue body is arbitrarily long /// and the point of the line is that it changed, not what it now says. fn describe_body(body: Option<&str>) -> String { @@ -214,7 +256,7 @@ pub(crate) struct CreateArgs { /// Issue title #[arg(short, long)] pub title: String, - /// Issue body + /// Issue body; required, whatever the lexicon says #[arg(short, long)] pub body: Option, /// Read the body from a file; `-` reads stdin @@ -247,11 +289,18 @@ pub(crate) async fn create(args: CreateArgs) -> Result<()> { let title = args.title.trim().to_string(); if title.is_empty() { - bail!("an issue needs a title; --title cannot be empty"); + return Err(crate::exit::fail( + crate::exit::Exit::Usage, + "an issue needs a title; --title cannot be empty", + )); } // Read before touching the network: a missing file is the likeliest // mistake, and finding out after three round trips is worse. - let body = new_body(args.body, args.body_file)?.flatten(); + let Some(body) = new_body(args.body, args.body_file)?.flatten() else { + return Err(empty_body_refusal( + "an issue needs a body; pass --body, or --body-file (with `-` for stdin)", + )); + }; let remote_url = git::remote_url(&args.remote)?; let repo = resolve::repo_ref(&remote_url).await?; @@ -263,13 +312,13 @@ pub(crate) async fn create(args: CreateArgs) -> Result<()> { title: title.clone(), repo_did: repo.did.clone(), url: read::issues_url(&repo.did), - body_bytes: body.as_deref().map(str::len).unwrap_or(0), + body_bytes: body.len(), }; if !args.json { println!("repo: {}", repo.did); println!("title: {title}"); - println!("body: {}", describe_body(body.as_deref())); + println!("body: {}", describe_body(Some(&body))); } let scope_check = @@ -293,7 +342,7 @@ pub(crate) async fn create(args: CreateArgs) -> Result<()> { repo: Did::new(repo.did.clone().into()) .map_err(|e| anyhow::anyhow!("{} is not a valid repo DID: {e}", repo.did))?, title: title.into(), - body: body.map(Into::into), + body: Some(body.into()), created_at: Datetime::now(), // An issue atgc opens carries nothing the struct does not name: the // web UI is what sets mentions, references and image blobs, and each @@ -986,6 +1035,20 @@ mod tests { assert!(err.contains("editor"), "{err}"); } + /// The refusal an empty issue body earns has to name the appview's own + /// error, because that string is the whole evidence for a rule the + /// lexicon contradicts — a reader who checks `issue.json` finds `body` + /// optional and would otherwise conclude atgc invented the requirement. + #[test] + fn the_empty_body_refusal_cites_the_appview_and_exits_two() { + let err = super::empty_body_refusal("an issue needs a body"); + assert_eq!(crate::exit::classify(&err), crate::exit::Exit::Usage); + let message = err.to_string(); + assert!(message.contains("an issue needs a body"), "{message}"); + assert!(message.contains("issue body is empty"), "{message}"); + assert!(message.contains("federate, and never appear"), "{message}"); + } + /// A body is reported by size rather than echoed: it is arbitrarily long /// and the line's job is to say that it moved. #[test] diff --git a/tests/issue_flows.rs b/tests/issue_flows.rs index 9afd2a9..476a652 100644 --- a/tests/issue_flows.rs +++ b/tests/issue_flows.rs @@ -31,9 +31,20 @@ use support::{ALICE, BOB, FEED_COMMENT_NSID, ISSUE_NSID, ISSUE_STATE_NSID, REPO_ const OTHER_REPO: &str = "did:plc:oooooooooooooooooooooooo"; /// File an issue and hand back its record key. +/// +/// A body every time, because tangled.org requires one — see +/// [`an_issue_with_no_body_is_refused_before_anything_leaves_the_machine`]. fn open_issue(world: &Scenario, title: &str) -> String { let created = world - .run(&["issue", "create", "--title", title, "--json"]) + .run(&[ + "issue", + "create", + "--title", + title, + "--body", + "what happened", + "--json", + ]) .success() .json(); created["rkey"] @@ -105,7 +116,17 @@ fn an_issue_is_written_by_and_into_the_selected_account() { let world = Scenario::new("issue-create-identity"); world - .run_as(BOB, &["issue", "create", "--title", "bob's issue"]) + .run_as( + BOB, + &[ + "issue", + "create", + "--title", + "bob's issue", + "--body", + "it broke", + ], + ) .success(); assert!(world.issues(ALICE).is_empty(), "it landed in the wrong PDS"); @@ -136,6 +157,8 @@ fn a_dry_run_creates_nothing_and_mints_no_identifiers() { "create", "--title", "not sent", + "--body", + "nor this", "--dry-run", "--json", ]) @@ -151,6 +174,45 @@ fn a_dry_run_creates_nothing_and_mints_no_identifiers() { assert!(world.issues(ALICE).is_empty(), "a dry run wrote a record"); } +/// **An issue needs a body, whatever the lexicon says.** +/// +/// `sh.tangled.repo.issue` marks `body` optional; tangled.org's appview does +/// not honour that. `Issue.Validate` refuses `issue body is empty` and the +/// ingester logs `failed to ingest record, dropping it without retry`, so a +/// title-only issue is accepted by the PDS, federates, and never appears — +/// the same silent shape as a comment in the deprecated collection, reached +/// from the other side. It has to be refused before the write, because after +/// the write there is nothing to observe and nothing to undo. +/// +/// Exit `2`: the command line does not make sense, and no retry or re-login +/// changes that. +#[test] +fn an_issue_with_no_body_is_refused_before_anything_leaves_the_machine() { + let world = Scenario::new("issue-create-bodyless"); + + for args in [ + vec!["issue", "create", "--title", "title only"], + vec!["issue", "create", "--title", "title only", "--body", ""], + vec![ + "issue", + "create", + "--title", + "title only", + "--body", + " \n ", + ], + // A dry run is refused too: it is meant to answer "would this work", + // and this would not. + vec!["issue", "create", "--title", "title only", "--dry-run"], + ] { + world.run(&args).refused_with(2, "issue body is empty"); + } + assert!( + world.issues(ALICE).is_empty(), + "a bodyless issue reached the PDS" + ); +} + // --------------------------------------------------------------------------- // close and reopen // --------------------------------------------------------------------------- @@ -230,7 +292,17 @@ fn closing_appends_a_state_record_and_reopening_appends_another() { fn a_repo_owner_closes_someone_elses_issue_from_their_own_pds() { let world = Scenario::new("issue-close-as-owner"); world - .run_as(BOB, &["issue", "create", "--title", "bob reports a bug"]) + .run_as( + BOB, + &[ + "issue", + "create", + "--title", + "bob reports a bug", + "--body", + "steps", + ], + ) .success(); let (rkey, _) = only_issue(&world, BOB); let uri = format!("at://{BOB}/{ISSUE_NSID}/{rkey}"); @@ -509,7 +581,14 @@ fn a_state_is_known_on_your_own_repo_and_unsettled_off_it() { world .run_as( BOB, - &["issue", "create", "--title", "bob's, on alice's repo"], + &[ + "issue", + "create", + "--title", + "bob's, on alice's repo", + "--body", + "steps", + ], ) .success(); let listed = world diff --git a/tests/support/mod.rs b/tests/support/mod.rs index d3ebc8d..365da76 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -455,6 +455,26 @@ impl Run { self } + /// The same, and the exit status it carried. + /// + /// Worth a second assertion because the status is half the contract: + /// docs/output.md makes the numbers a public interface, and a refusal + /// that exits `1` says only that something went wrong. [`Run::refused`] + /// passes against every one of them, so a call site that loses its + /// classification is invisible to it. + pub fn refused_with(self, exit: i32, needle: &str) -> Self { + let refused = self.refused(needle); + assert_eq!( + refused.code, + Some(exit), + "`atgc {}` exited {:?}, not {exit}\n--- stderr ---\n{}", + refused.args, + refused.code, + refused.stderr, + ); + refused + } + /// stdout parsed as the one JSON object a `--json` command prints. pub fn json(&self) -> serde_json::Value { serde_json::from_str(&self.stdout).unwrap_or_else(|e| {