From 516437e2e411205ab2ef873fe4429fab9cc5435d Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Sat, 15 Aug 2026 17:21:15 -0400 Subject: [PATCH] fix(issue): require a body, which tangled.org does and the lexicon does not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sh.tangled.repo.issue` marks `body` optional; the appview does not honour that. Its `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 silent shape this module already guards a comment against, arrived at from the other direction. Pulls are exempt from the same check when the patch is a format-patch, which is why `pr create` needs no such rule. Refused before the write rather than warned about, because the record cannot be unpublished and nothing inside the process can observe that it went nowhere. The refusal names the appview's own error string, since that is the whole evidence for a rule `issue.json` contradicts. The two argument refusals beside it — `--body` with `--body-file`, an empty comment — take an exit status in the same hunks; the rest of the family's statuses are a commit of their own. Co-Authored-By: Claude Opus 5 (1M context) Change-Id: I2294de80612f742fbf8ce33ea47bbc668965addc --- README.md | 8 +++- TODO.md | 10 +++++ docs/architecture.md | 6 +++ src/cmd/issue/mod.rs | 7 +++- src/cmd/issue/write.rs | 89 ++++++++++++++++++++++++++++++++++++------ tests/issue_flows.rs | 87 +++++++++++++++++++++++++++++++++++++++-- tests/support/mod.rs | 20 ++++++++++ 7 files changed, 207 insertions(+), 20 deletions(-) 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| { -- 2.51.2