diff --git a/src/cmd/pr/read/fixtures.rs b/src/cmd/pr/read/fixtures.rs new file mode 100644 index 0000000..143f403 --- /dev/null +++ b/src/cmd/pr/read/fixtures.rs @@ -0,0 +1,82 @@ +//! The captured pages the three modules' tests are checked against. +//! +//! One module rather than a copy per test module: these are real bytes off +//! real services, and the paragraphs saying what each capture holds and why +//! are the reason a reader can trust an assertion counting six pulls in it. +//! Duplicating them per file is how two copies of the same fixture come to +//! disagree about what they contain. + +use serde_json::Value; + +/// The repo `pr list` is run against by hand, and the one the PDS +/// fixture's records are mostly aimed at. +pub(super) const ATGC_REPO: &str = "did:plc:gspkabpde4kx47fj3bhiwrms"; + +/// A real page of `sh.tangled.repo.listPullsBy` off Bobbin: the item +/// envelope (uri, state, commentCount) wrapped around the PDS record. +pub(super) const BOBBIN_PAGE: &str = include_str!(concat!( + env!("CARGO_MANIFEST_DIR"), + "/tests/fixtures/bobbin_list_pulls_by.json" +)); + +/// A real `com.atproto.repo.listRecords` page of `sh.tangled.repo.pull` +/// off this account's PDS — the source the whole change is about. +/// +/// Eleven records taken verbatim out of a 52-record capture, chosen to +/// keep every trap the live collection contains: six aimed at +/// [`ATGC_REPO`] and five aimed at five *other* repos (a pull record +/// lives in its author's PDS whatever it targets, so this collection +/// mixes them and a listing that forgets to filter is wrong), records +/// with a `source` and records without, and two of the six atgc ones +/// carrying no status record at all, so their state is unknowable. +/// +/// ```text +/// curl 'https://amanita.us-east.host.bsky.network/xrpc/com.atproto.repo.listRecords\ +/// ?repo=did:plc:nlzmjyfv6loqtxyzvdcznwgf&collection=sh.tangled.repo.pull&limit=100' +/// ``` +pub(super) const PDS_PULLS: &str = include_str!(concat!( + env!("CARGO_MANIFEST_DIR"), + "/tests/fixtures/pds_pulls_page.json" +)); + +/// The matching real `sh.tangled.repo.pull.status` page. Same capture +/// command with `collection=sh.tangled.repo.pull.status`. Nine records, +/// covering eight of the eleven pulls above: eight merged and one closed. +pub(super) const PDS_STATUSES: &str = include_str!(concat!( + env!("CARGO_MANIFEST_DIR"), + "/tests/fixtures/pds_pull_statuses_page.json" +)); + +fn pds_records(fixture: &str) -> Vec { + serde_json::from_str::(fixture).unwrap()["records"] + .as_array() + .unwrap() + .clone() +} + +pub(super) fn pds_pulls() -> Vec { + pds_records(PDS_PULLS) +} + +pub(super) fn pds_statuses() -> Vec { + pds_records(PDS_STATUSES) +} + +/// A real pre-rounds record. Its listRecords envelope has the same +/// `uri` + `value` shape a Bobbin item does, minus the state fields, so +/// it stands in for an old item here. +pub(super) const OLD_ITEM: &str = include_str!(concat!( + env!("CARGO_MANIFEST_DIR"), + "/tests/fixtures/pull_old_record.json" +)); + +pub(super) fn bobbin_items() -> Vec { + serde_json::from_str::(BOBBIN_PAGE).unwrap()["items"] + .as_array() + .unwrap() + .clone() +} + +pub(super) fn old_item() -> Value { + serde_json::from_str(OLD_ITEM).unwrap() +} diff --git a/src/cmd/pr/read/labels.rs b/src/cmd/pr/read/labels.rs new file mode 100644 index 0000000..39607ef --- /dev/null +++ b/src/cmd/pr/read/labels.rs @@ -0,0 +1,501 @@ +//! What a row is called: a handle, a repo name, an appview number. +//! +//! Everything here turns an identifier a record carries — an author DID, a +//! target repo DID — into the string a listing prints for it, and into the +//! URL that string is punctuation for. None of it decides *which* pulls +//! there are, which is why it is not in [`super::sources`], and both of the +//! other two modules need it: the listings for their columns, and the +//! backfill for a repo's web root to scrape. +//! +//! Every lookup here is best-effort by construction. A label that will not +//! resolve costs a column its contents, never a row its place in the +//! listing, and the fallbacks say so one at a time. + +use super::sources::Listed; +use crate::term::column::ellipsize; +use futures_util::stream::{self, StreamExt}; +use std::collections::HashMap; + +/// How many identity lookups a listing may have in flight at once. +/// +/// The one number for both of a listing's label errands, and it is the +/// resolver's: the columns a page needs are per *unique* DID rather than per +/// row, and a listing is not entitled to open as many sockets as it happens +/// to have rows. See [`crate::clients::atproto::handles::CONCURRENCY`] for +/// the rest of the argument. +const LABEL_CONCURRENCY: usize = crate::clients::atproto::handles::CONCURRENCY; + +/// `@handle` per unique author DID on the page, falling back to the DID. +/// +/// The resolution is [`crate::clients::atproto::handles`], which every +/// listing shares and which remembers a DID for the length of the process; +/// what stays here is this table's own answer to a handle that will not +/// resolve, which is to print the DID rather than leave the column empty. +pub(super) async fn author_handles(items: &[Listed]) -> HashMap { + let dids = unique(items.iter().filter_map(|i| author_did(&i.value))); + let handles = crate::clients::atproto::handles::handles(dids.clone()).await; + dids.into_iter() + .map(|did| { + let label = match handles.get(&did) { + Some(h) => format!("@{h}"), + None => did.clone(), + }; + (did, label) + }) + .collect() +} + +/// [`RepoName`] per unique target repo DID on the page. +/// +/// The expensive half of the two: [`repo_name`] is up to three round trips of +/// its own (Bobbin, then the appview redirect, then the owner's handle), so a +/// page of `pr list --all` spanning ten repos was thirty serial requests +/// before the numbering sweep had started. +pub(super) async fn repo_names(items: &[Listed]) -> HashMap { + let dids = unique(items.iter().filter_map(|i| target_repo_did(&i.value))); + crate::logging::debug::log(format!( + "listing labels: {} repo name(s) for {} row(s), {LABEL_CONCURRENCY} at a time", + dids.len(), + items.len() + )); + stream::iter( + dids.into_iter() + .map(|did| async move { (did.clone(), repo_name(&did).await) }), + ) + .buffer_unordered(LABEL_CONCURRENCY) + .collect() + .await +} + +/// The distinct values of `keys`, in the order they first appear. +/// +/// Deduplication is the whole point: the loops this replaces skipped a DID +/// they had already resolved, and dropping that would turn one lookup per +/// author into one per row. Order is kept because it costs nothing and makes +/// the debug log read in page order. +fn unique(keys: impl Iterator) -> Vec { + let mut seen = std::collections::HashSet::new(); + keys.filter(|k| seen.insert(k.clone())).collect() +} + +/// The target repo's DID: value.target.repo in new records, or the at-uri +/// authority in old records' targetRepo (the repo owner's DID back then — +/// close enough for display). +pub(in crate::cmd::pr) fn target_repo_did(item: &serde_json::Value) -> Option { + let v = &item["value"]; + if let Some(did) = v["target"]["repo"].as_str() { + return Some(did.to_string()); + } + v["targetRepo"] + .as_str()? + .strip_prefix("at://")? + .split('/') + .next() + .map(String::from) +} + +/// The appview's `#` for every row it can place, across every repo on the +/// page. +/// +/// `pr list` numbers one repo's rows from one sweep of that repo's pull +/// listings. `status pr` spans repos, so it is that same sweep once per repo — +/// and the reason it did not have a number column, since a sweep costs about a +/// quarter of a megabyte and doing several in turn is a wait nobody asked for. +/// +/// So they go together. The repos on a page of `status pr` are independent +/// appview reads, and running them concurrently makes the column cost one +/// round trip rather than one per repo. What it cannot make cheaper is the +/// bytes, which is why this is per repo *on the page* and not per repo the +/// account has ever filed against. +/// +/// A repo whose label could not be resolved has no URL to read, and its rows +/// keep the blank column. So do the rows of a repo whose listings cannot be +/// reached: this decorates a listing that is already correct without it. +pub(super) async fn numbers_across_repos( + items: &[Listed], + repos: &HashMap, +) -> HashMap { + let mut sweeps = tokio::task::JoinSet::new(); + for (repo_did, rows) in rows_by_repo(items) { + let Some(web_url) = repos.get(&repo_did).and_then(|name| name.web_url.clone()) else { + crate::logging::debug::log(format!( + "pull numbers: no appview URL for {repo_did}, leaving {} row(s) blank", + rows.len() + )); + continue; + }; + sweeps.spawn(async move { + let rows: Vec<(&str, &str)> = rows + .iter() + .map(|(uri, title)| (uri.as_str(), title.as_str())) + .collect(); + crate::clients::tangled::web::pulls::numbers_for_records(&web_url, &rows).await + }); + } + + let mut numbers = HashMap::new(); + while let Some(swept) = sweeps.join_next().await { + if let Ok(found) = swept { + numbers.extend(found); + } + } + numbers +} + +/// The page's rows grouped by the repo whose listings can number them. +/// +/// A number is only unique inside a repo, so the grouping *is* the question: +/// two rows from two repos may both be #12 and neither is evidence about the +/// other. A row naming no target repo is dropped rather than swept against +/// somebody's guess at one. +fn rows_by_repo(items: &[Listed]) -> HashMap> { + let mut by_repo: HashMap> = HashMap::new(); + for item in items { + let Some(repo_did) = target_repo_did(&item.value) else { + continue; + }; + let title = item.value["value"]["title"].as_str().unwrap_or_default(); + by_repo + .entry(repo_did) + .or_default() + .push((item.uri.clone(), title.to_string())); + } + by_repo +} + +/// The appview `status pr` builds repo URLs against. +/// +/// A repo DID resolves to a knot and a PDS, and neither says which web front +/// end is showing it; Tangled's is the one atgc knows how to read pull +/// numbers off. The address itself, and its override, are +/// [`crate::clients::endpoints`]'s. +fn appview() -> String { + crate::clients::endpoints::appview() +} + +/// What `status pr` knows about one repo: what to print for it, and where its +/// pages are. +/// +/// The two travel together because they come from the same lookup. Both +/// sources answer with an owner and a name — Bobbin as an at-uri, the appview +/// as a redirect target — and `@owner/name` and +/// `https://tangled.org/owner/name` are that same pair, punctuated for a +/// person and for a URL. Deriving the second from the first is why numbering +/// `status pr` costs no lookup it was not already making. +pub(super) struct RepoName { + /// `@owner/name`, or a truncated DID when neither source could say. + pub(super) label: String, + /// The repo's appview root, absent exactly when the label is that DID: + /// nothing is known to build a URL out of, so nothing is guessed. + pub(super) web_url: Option, +} + +/// "owner-handle/name" for a repo DID, best-effort, falling back to a +/// truncated DID. +/// +/// Two lookups, because the first one inherits the lag `status pr` is built +/// around. Bobbin's `getRepoByRepoDid` is asked first and answers with an +/// at-uri that resolves to a handle. When it does not know the repo — which is +/// common here, since a PDS-sourced listing surfaces pulls against repos +/// Bobbin has not indexed — the web appview is asked instead, and it runs a +/// *different* index that is routinely ahead. Without the second lookup this +/// would trade a missing row for an unreadable one. +async fn repo_name(repo_did: &str) -> RepoName { + let named = match repo_label_from_bobbin(repo_did).await { + Some(label) => Some(label), + None => repo_label_from_appview(repo_did).await, + }; + match named { + Some(label) => RepoName { + web_url: Some(appview_url(&label)), + label, + }, + None => RepoName { + label: ellipsize(repo_did, 21), + web_url: None, + }, + } +} + +/// The appview root for a repo, from the label both lookups produce. +/// +/// `@permadeath.com/atgc` is `https://tangled.org/permadeath.com/atgc`: the +/// `@` is punctuation for a reader and is not in the path. Only ever called +/// with a resolved label, never with the truncated-DID fallback, which is a +/// display string with an ellipsis in it and not a repo anybody can fetch. +pub(super) fn appview_url(label: &str) -> String { + format!( + "{}/{}", + appview(), + label.trim_start_matches('@').trim_end_matches('/') + ) +} + +async fn repo_label_from_bobbin(repo_did: &str) -> Option { + let uri = crate::clients::tangled::bobbin::repo_uri(repo_did).await?; + // Old records carry an *owner* DID in this position, which is not a repo + // DID and so resolves to nothing here. + let (owner, name) = owner_and_name(&uri)?; + let owner_label = match crate::clients::atproto::handles::handle(owner).await { + Some(h) => format!("@{h}"), + None => owner.to_string(), + }; + Some(format!("{owner_label}/{name}")) +} + +/// `tangled.org/` 302s to `tangled.org//`. +/// +/// The same redirect-probe trick [`crate::clients::tangled::resolve::repo_ref`] uses to go from +/// a remote URL to a repo DID, run backwards, and against the one index in +/// the system that is reliably current. It costs one request and is only +/// reached when Bobbin has already declined. +pub(super) async fn repo_label_from_appview(repo_did: &str) -> Option { + let client = crate::clients::http::builder() + .redirect(reqwest::redirect::Policy::none()) + .build() + .ok()?; + let url = format!("{}/{repo_did}", appview()); + crate::logging::debug::log(format!(">> GET {url}")); + let resp = client.get(&url).send().await.ok()?; + let location = resp.headers().get(reqwest::header::LOCATION)?; + crate::logging::debug::log(format!("<< {} location: {location:?}", resp.status())); + label_from_location(location.to_str().ok()?) +} + +/// Read "owner/name" off a redirect target. +/// +/// The tail two segments are owner and name whether the `Location` is +/// absolute or relative, so both spellings are read the same way. Anything +/// else is declined rather than guessed at — in particular a redirect that +/// lands on another DID, which would render as a handle that is not one. +fn label_from_location(location: &str) -> Option { + let mut segments = location.trim_end_matches('/').rsplit('/'); + let name = segments.next()?; + let owner = segments.next()?; + if name.is_empty() || owner.is_empty() || owner.contains(':') || name.contains(':') { + return None; + } + Some(format!("@{owner}/{name}")) +} + +pub(super) fn author_did(item: &serde_json::Value) -> Option { + // at://did:plc:xyz/sh.tangled.repo.pull/rkey + item["uri"] + .as_str()? + .strip_prefix("at://")? + .split('/') + .next() + .map(String::from) +} +/// Split `at:///sh.tangled.repo/` into its owner and name. +/// The collection sits between them, hence the skip. +fn owner_and_name(uri: &str) -> Option<(&str, &str)> { + let mut parts = uri.strip_prefix("at://")?.split('/'); + Some((parts.next()?, parts.nth(1)?)) +} +#[cfg(test)] +mod tests { + use super::super::fixtures::{bobbin_items, old_item}; + use super::super::sources::{Listed, State}; + use super::{appview_url, rows_by_repo, unique}; + use super::{author_did, label_from_location, owner_and_name, target_repo_did}; + use serde_json::json; + + #[test] + fn reads_the_target_repo_from_a_live_item() { + let items = bobbin_items(); + for item in &items { + let did = target_repo_did(item).expect("live items carry a target"); + assert_eq!(did, item["value"]["target"]["repo"].as_str().unwrap()); + // A DID, not specifically a did:plc one. Repos are minted with + // plc DIDs today, but nothing in the code depends on that and + // the fixture should not make the suite depend on it either. + assert!(crate::lexicon::identity::is_did(&did), "got {did}"); + } + } + + /// Old records have no `target` at all — the target was an at-uri + /// pointing at the repo *record*, so its authority is the repo owner's + /// DID rather than the repo's. The code knows this and settles for it, + /// since it only ever feeds a display label. + #[test] + fn falls_back_to_the_at_uri_authority_for_old_items() { + let item = old_item(); + assert!(item["value"].get("target").is_none()); + assert_eq!( + target_repo_did(&item).as_deref(), + Some("did:plc:wshs7t2adsemcrrd4snkeqli") + ); + } + + #[test] + fn target_repo_did_gives_up_rather_than_guessing() { + for malformed in [ + json!({}), + json!({ "value": {} }), + // Present but the wrong type: as_str declines, so does this. + json!({ "value": { "targetRepo": 3 } }), + json!({ "value": { "target": { "repo": null } } }), + // An at-uri that is not one. + json!({ "value": { "targetRepo": "did:plc:abc/sh.tangled.repo/x" } }), + ] { + assert_eq!(target_repo_did(&malformed), None, "input: {malformed}"); + } + } + + #[test] + fn reads_the_author_from_the_record_uri() { + for item in &bobbin_items() { + assert_eq!( + author_did(item).as_deref(), + Some("did:plc:nlzmjyfv6loqtxyzvdcznwgf") + ); + } + assert_eq!( + author_did(&old_item()).as_deref(), + Some("did:plc:wshs7t2adsemcrrd4snkeqli") + ); + } + + #[test] + fn author_did_gives_up_rather_than_guessing() { + for malformed in [ + json!({}), + json!({ "uri": null }), + json!({ "uri": "https://tangled.org/permadeath.com/atgc" }), + ] { + assert_eq!(author_did(&malformed), None, "input: {malformed}"); + } + } + + #[test] + fn splits_a_repo_record_uri() { + assert_eq!( + owner_and_name("at://did:plc:nlzmjyfv6loqtxyzvdcznwgf/sh.tangled.repo/atgc"), + Some(("did:plc:nlzmjyfv6loqtxyzvdcznwgf", "atgc")) + ); + for bad in [ + "", + "did:plc:abc/sh.tangled.repo/atgc", + // Authority and collection, but the record key is missing. + "at://did:plc:abc/sh.tangled.repo", + ] { + assert_eq!(owner_and_name(bad), None, "input: {bad}"); + } + } + /// The observation this whole change is built on, as it stood on + /// The label both repo lookups produce, turned into the root the pull + /// listings hang off. The `@` is for a reader and is not in the path. + #[test] + fn builds_a_repo_url_from_the_label_it_already_had() { + assert_eq!( + appview_url("@permadeath.com/atgc"), + "https://tangled.org/permadeath.com/atgc" + ); + // Bobbin hands back a bare DID for an owner with no handle, and that + // is still a path tangled.org resolves. + assert_eq!( + appview_url("did:plc:abc/atgc"), + "https://tangled.org/did:plc:abc/atgc" + ); + } + + /// `status pr` spans repos and a number is only unique inside one, so the + /// rows are swept per repo. Two repos each having a pull that will come + /// back #12 is the case that makes one shared sweep wrong. + #[test] + fn groups_the_rows_by_the_repo_that_can_number_them() { + let row = |uri: &str, repo: &str, title: &str| Listed { + uri: uri.to_string(), + value: json!({ + "uri": uri, + "value": { "title": title, "target": { "repo": repo } }, + }), + state: State::Unknown, + comments: 0, + indexed: false, + }; + let items = vec![ + row( + "at://did:plc:a/sh.tangled.repo.pull/1", + "did:plc:one", + "Add repo list", + ), + row( + "at://did:plc:a/sh.tangled.repo.pull/2", + "did:plc:two", + "Add repo list", + ), + row( + "at://did:plc:a/sh.tangled.repo.pull/3", + "did:plc:one", + "Add pr status", + ), + ]; + let by_repo = rows_by_repo(&items); + assert_eq!(by_repo.len(), 2); + assert_eq!(by_repo["did:plc:one"].len(), 2); + assert_eq!( + by_repo["did:plc:two"], + vec![( + "at://did:plc:a/sh.tangled.repo.pull/2".to_string(), + "Add repo list".to_string() + )] + ); + } + + /// A row naming no target repo has no listing to be numbered from, and is + /// dropped rather than swept against another repo's. + #[test] + fn drops_a_row_that_names_no_repo() { + let orphan = Listed { + uri: "at://did:plc:a/sh.tangled.repo.pull/1".to_string(), + value: json!({ "value": { "title": "Add repo list" } }), + state: State::Unknown, + comments: 0, + indexed: false, + }; + assert!(rows_by_repo(&[orphan]).is_empty()); + } + + /// The reverse lookup that keeps `status pr` readable once it lists + /// repos Bobbin has never heard of. Checked against the real redirect: + /// `tangled.org/did:plc:gspkabpde4kx47fj3bhiwrms` 302s to + /// `https://tangled.org/permadeath.com/atgc`. + #[test] + fn reads_a_repo_label_off_the_appview_redirect() { + for location in [ + "https://tangled.org/permadeath.com/atgc", + "https://tangled.org/permadeath.com/atgc/", + "/permadeath.com/atgc", + ] { + assert_eq!( + label_from_location(location).as_deref(), + Some("@permadeath.com/atgc"), + "input: {location}" + ); + } + for bad in [ + "", + "/", + "/atgc", + // A hop that lands on a DID would render as a handle that is + // not one, which is worse than the truncated DID it falls back + // to. + "https://tangled.org/did:plc:gspkabpde4kx47fj3bhiwrms", + "https://tangled.org/permadeath.com/did:plc:abc", + ] { + assert_eq!(label_from_location(bad), None, "input: {bad}"); + } + } + + /// The dedupe the concurrent lookups inherited from the `for` loops they + /// replaced. Losing it would turn one lookup per author into one per row, + /// which is the opposite of the point. + #[test] + fn unique_keeps_first_appearance_order_and_drops_repeats() { + let keys = ["b", "a", "b", "c", "a"].iter().map(|s| s.to_string()); + assert_eq!(unique(keys), vec!["b", "a", "c"]); + assert!(unique(std::iter::empty()).is_empty()); + } +} diff --git a/src/cmd/pr/read/mod.rs b/src/cmd/pr/read/mod.rs new file mode 100644 index 0000000..db3f40f --- /dev/null +++ b/src/cmd/pr/read/mod.rs @@ -0,0 +1,1531 @@ +//! Listing pull requests: `atgc pr list`, `pr list --all` and `pr view`. +//! +//! The read half of [`crate::cmd::pr`]. Nothing in here authenticates: a +//! `sh.tangled.repo.pull` record is public, so is every +//! `com.atproto.repo.listRecords` call that fetches one, and so is Bobbin. A +//! session buys this half exactly one thing — a DID whose PDS is worth +//! reading — and its absence costs that and no more. +//! +//! What makes the half awkward is that a pull request has two homes and +//! neither is complete. The record lives in its **author's** PDS, so your own +//! pulls are one public request away and are visible the instant the write +//! returns; everyone else's are scattered across the PDSes of people nothing +//! enumerates, which is why answering "what pulls exist on this repo" needs +//! an index and why [`Source`] exists to name which sources a listing used. +//! So `pr list --all`, scoped to one account, can be answered off a PDS alone and +//! never be stale, while `pr list`, scoped to a repo, cannot — and on +//! 2026-08-06 Bobbin returned nothing at all for a repo whose author's PDS +//! held thirty pull records aimed at it. +//! +//! Which is why this is a directory rather than a file. It had reached 4,287 +//! lines, and the seam is not the verbs: `pr list`, `pr list --all` and +//! `pr view` all print different shapes of the same answer, and all three +//! spend most of their length working out what that answer is. +//! +//! - [`mod@sources`] decides what the set of pull requests *is*: which +//! sources were asked, how their answers are merged, what a disagreement +//! between them is evidence of, and which states are known rather than +//! guessed. It is the paragraph above, in code. +//! - [`mod@labels`] turns the identifiers in those records into the things a +//! row is called: an author's handle, a repo's `owner/name` and appview +//! root, the appview's `#`. Neither of the other two owns it — the +//! listing needs it for its columns, and the backfill needs a repo's web +//! root to scrape — so it is its own module rather than a helper inside +//! whichever one reached for it first. +//! - This file is what is left: the clap arguments, the two listing verbs, +//! `pr view`, and the `--json` shapes all three emit. +//! +//! The split is a move and nothing else. No function was renamed, no +//! signature changed, no printed string altered; the edits that are not a cut +//! and a paste are this paragraph, the two submodule headers, a handful of +//! `pub(super)`s on items that had been private to one file, and the +//! re-exports below, which exist so that every `crate::cmd::pr::read::…` +//! path other modules already use still resolves. + +mod labels; +mod sources; + +#[cfg(test)] +mod fixtures; + +// The read half's public face, unchanged by the split: every one of these +// was `read::` before the file became a directory and still is. +pub(super) use labels::target_repo_did; +pub(crate) use sources::{Reach, Source, pds_pulls}; +pub(in crate::cmd) use sources::{ + StackRow, branch_pull, branch_pull_record, for_branch, repo_rows, +}; + +use crate::clients::git::run as git; +use crate::clients::tangled::resolve; +use crate::lexicon::tangled::PullState; +use crate::term::column::{day, ellipsize}; +use anyhow::Result; +use labels::{RepoName, author_did, author_handles, numbers_across_repos, repo_names}; +use sources::{ + Ask, Empty, Scope, apply_state_filter, classify_empty, gather, gather_branch_pulls, + note_author_scoped, note_unknown_states, own_did, require_branch_pull, warn_stale, +}; + +/// How many rounds a pull has been through. Records written before the +/// rounds migration carry a single inline patch and no `rounds` array; they +/// are one round by definition, so a missing array counts as one rather than +/// as none. Crate-visible for [`crate::cmd::stack`]'s chain display. +pub(in crate::cmd) fn round_count(value: &serde_json::Value) -> usize { + value["rounds"].as_array().map(|r| r.len()).unwrap_or(1) +} + +/// The words `--state` accepts, on either pull listing. +/// +/// A `ValueEnum` rather than a `String` because the string took anything: +/// `--state opne` matched no row, printed "no opne pull requests", and read +/// as *there are none*. That is the failure this tree spent a release +/// arguing about — an answer given in the shape and with the confidence of a +/// real one — reproduced locally by a flag, and it costs one enum to make it +/// a refusal that names the four words instead. clap prints them in `--help` +/// as a side effect. +/// +/// The *input* side alone. What a record says a pull's state is stays +/// [`sources::State::Known`]'s unconstrained `String`, holding whatever word the PDS +/// or the index returned, for the reason spelled out there and in +/// [`PullState::from_token`]: a state Tangled adds after this build has to +/// list rather than crash. A filter is typed by a person and can be a typo; a +/// status record is somebody else's fact and cannot be. +/// +/// `All` filters nothing, which also makes it the only way to see a pull +/// whose state nothing available could settle: an unknown state matches no +/// filter, so every other value hides those rows (and says how many). +#[derive(clap::ValueEnum, Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum StateFilter { + Open, + Closed, + Merged, + /// Every state, including a pull whose state is unknown + All, +} + +impl StateFilter { + /// The label a row's state is compared against, or `None` for `all`: + /// exactly the shape [`apply_state_filter`] and [`classify_empty`] want, + /// where `None` means "keep everything". + fn label(self) -> Option<&'static str> { + match self { + StateFilter::Open => Some(PullState::Open.label()), + StateFilter::Closed => Some(PullState::Closed.label()), + StateFilter::Merged => Some(PullState::Merged.label()), + StateFilter::All => None, + } + } +} + +impl std::fmt::Display for StateFilter { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.label().unwrap_or("all")) + } +} + +#[derive(clap::Args, Debug)] +pub(crate) struct ListArgs { + /// Every repo instead of this one: your pulls wherever you filed them + /// + /// Widening is per author and not per repo, because the records are read + /// from a PDS: a pull lives in the PDS of whoever wrote it, so "every + /// repo" is answerable for one account and there is no listing of + /// everyone's pulls everywhere to ask for. It is you unless `--author` + /// names somebody else, and it needs no checkout. + #[arg(long)] + pub all: bool, + /// Show another account's pull requests (handle or DID); needs --all + #[arg(long, requires = "all")] + pub author: Option, + /// Filter by state + #[arg(long, default_value = "open")] + pub state: StateFilter, + /// Maximum number of PRs to show + /// A floor of 1, the same `--limit` `search` has carried since it + /// shipped. `--limit 0` used to parse and then print "no open pull + /// requests" — an empty listing that reads as *there are none*, which is + /// the stale-index failure this tree opted out of arriving from the + /// command line instead. It is now clap's own refusal, naming the range, + /// exit 2: the same treatment `--state opne` got and for the same reason. + #[arg(long, default_value_t = 30, value_parser = clap::value_parser!(u32).range(1..))] + pub limit: u32, + /// Git remote pointing at the repo. Ignored with --all, which is not + /// scoped to a checkout at all + #[arg(long, default_value = "origin")] + pub remote: String, + /// Where to read from: pds (default, your own pulls and never stale), + /// bobbin, web, auto for all three, or a comma-separated combination. + /// Both indexes are opt-in; ATGC_USE_BOBBIN=1 and ATGC_USE_WEB=1 add + /// one to the default, and ATGC_BOBBIN points at another instance + #[arg(long)] + pub source: Option, + /// Print a JSON array instead of a table, one object per pull; + /// colour, hyperlinks and ellipsizing are all off (see --help for + /// the field contract's stability note) + #[arg(long)] + pub json: bool, +} +// --------------------------------------------------------------------------- +// `--json` +// --------------------------------------------------------------------------- +// +// `pr list` and `pr view` are the two read commands an agent workflow leans +// on hardest — `pr list` to see what is there, `pr view` to act on one — so +// they are the pair that got `--json`, ahead of the wide listing, `stack view`, +// `repo view` and `repo list`, which stay text-only for now (see TODO.md). +// +// Both emit the *derived* view: the same state, round count and resolved +// author handle the human table or detail already compute, not the raw +// `sh.tangled.repo.pull` record. The record is deliberately not the answer +// here, for a reason specific to this lexicon rather than a general +// preference: a pull's state and appview number are not fields on the +// record at all (see docs/output.md, "What the record does and does +// not say") — they are exactly the two things `pr list` exists to resolve, +// so a `--json` that printed the record would omit the entire reason to run +// the command and hand the caller the same two-source merge this file +// exists to do, unauthenticated calls and all. A caller who wants the raw +// bytes anyway can already fetch them, unauthenticated, with +// `com.atproto.repo.getRecord` off the `uri` this prints. +// +// This diverges from `logs oauth --json` on purpose. That command re-emits the +// exact bytes it read (`entry.raw`) because the log *is* the record — a +// line of it already is the answer, and re-serializing would risk disagreeing +// with what `jq` was already parsing. Here the record is raw input to a +// computation the record itself cannot answer, so printing it back would not +// be honest in the same way; it would just be incomplete. +// +// Both builders below are pure functions of data their caller has already +// resolved (no I/O, no formatting decisions left to make), which is what +// lets a test pin their shape against a fixture record with no network and +// no mock server — see the tests at the end of this module. +// +// Stability: this project makes none of the guarantees a 1.0 CLI would (see +// README.md, "no stability guarantees"). Within that, these two shapes +// follow the same rule every other flag does — CONTRIBUTING.md's versioning +// section — a field can be added in a `feat`, but removing or renaming one +// is a breaking change and needs a `!`. Nothing here is guaranteed never to +// change; it is guaranteed to say so with a version bump when it does. + +/// `None` for the `"?"` [`sources::State::Unknown`] renders as, `Some` otherwise. +/// +/// Shared by every builder in the tool — [`crate::cmd::stack::read::view`]'s included — +/// so "unknown" has exactly one spelling in the output. The alternative — printing the literal string `"?"` a script +/// would have to special-case — is what the human column does and is a +/// display convention, not a value; JSON already has a way to say "we do +/// not know this", and using it is the whole point of typing the output. +pub(in crate::cmd) fn known_state(label: &str) -> Option { + (label != "?").then(|| label.to_string()) +} + +/// One row of `pr list --json`. +#[derive(serde::Serialize, Debug, PartialEq)] +pub(crate) struct PullRowJson { + pub uri: String, + /// The appview's `/pulls/` number, when [`crate::clients::tangled::web::pulls::numbers_for_records`] + /// could place this row. `null` rather than an empty column. + pub number: Option, + pub state: Option, + pub title: String, + pub author_did: Option, + /// Without the leading `@` the terminal column prints — a caller wanting + /// a mention back can add it; wanting the bare handle (to build a URL, to + /// pass to `--author`) is at least as common and this way needs no + /// stripping. + pub author_handle: Option, + /// RFC 3339, straight from the record's `createdAt`. `null` for the rare + /// pre-rounds record that carries `""` here — an empty string is not a + /// timestamp, and a script parsing this field should not have to + /// special-case one record era to find that out. + pub created_at: Option, + pub rounds: usize, + pub comments: u64, +} + +/// Build one [`PullRowJson`] from data `list` has already resolved. +/// +/// `envelope` is a merged item's `{"uri", "value"}` shape (a [`sources::Listed::value`] +/// or the record wrapped the same way `pds_pulls` returns it) — the same +/// value the text table reads every field of this from. `state` is +/// [`sources::State::label`]'s output, `number` is what the appview-number sweep +/// resolved for this row if anything, and `author_handle` is the same +/// lookup the text column prints, without its `@`. +pub(super) fn pull_row_json( + envelope: &serde_json::Value, + state: &str, + number: Option, + author_handle: Option<&str>, + comments: u64, +) -> PullRowJson { + let v = &envelope["value"]; + PullRowJson { + uri: envelope["uri"].as_str().unwrap_or_default().to_string(), + number, + state: known_state(state), + title: v["title"].as_str().unwrap_or("(untitled)").to_string(), + author_did: author_did(envelope), + author_handle: author_handle.map(str::to_string), + created_at: v["createdAt"] + .as_str() + .filter(|s| !s.is_empty()) + .map(str::to_string), + rounds: round_count(v), + comments, + } +} + +/// One row of `status pr --json`: a `pr list` row plus the repo it targets. +/// +/// Flattened rather than nested so the two listings really do print the same +/// fields for the same pull — a caller that reads `pr list --json` and then +/// widens to every repo should not have to move `.state` to `.pull.state` +/// on the way. What `status pr` adds is the column its table adds, and +/// nothing else. +#[derive(serde::Serialize, Debug, PartialEq)] +pub(crate) struct StatusRowJson { + #[serde(flatten)] + pub pull: PullRowJson, + /// The repo the pull targets. Present whenever the record names one at + /// all, resolved or not — it is the identifier both lookups below start + /// from, and the one a caller can act on. + pub repo_did: Option, + /// `owner/name`, without the `@` the table's column prints, and `null` + /// when neither Bobbin nor the appview could name the repo. The text + /// column falls back to a truncated DID there; that string has an + /// ellipsis in it and names nothing fetchable, so it is not a value. + /// `repo_did` still carries the identity, exactly as `author_did` + /// survives an unresolved `author_handle`. + pub repo: Option, + pub repo_url: Option, +} + +/// Build one [`StatusRowJson`] from data `status` has already resolved. +/// +/// `repo` is the same [`RepoName`] the text column reads, `None` for a +/// record naming no target repo at all. Pure, like the two builders above. +fn status_row_json( + envelope: &serde_json::Value, + state: &str, + number: Option, + author_handle: Option<&str>, + comments: u64, + repo_did: Option, + repo: Option<&RepoName>, +) -> StatusRowJson { + // `web_url` is `Some` exactly when the label came from a lookup rather + // than from the truncated-DID fallback — the invariant `RepoName`'s own + // doc states — so it is what decides whether there is a name here. + let named = repo.filter(|name| name.web_url.is_some()); + StatusRowJson { + pull: pull_row_json(envelope, state, number, author_handle, comments), + repo_did, + repo: named.map(|name| name.label.trim_start_matches('@').to_string()), + repo_url: named.and_then(|name| name.web_url.clone()), + } +} + +/// One round of `pr view --json`'s `rounds` array. +#[derive(serde::Serialize, Debug, PartialEq)] +pub(crate) struct RoundJson { + /// One-based, matching every `--round` flag `pr diff`, `pr checkout` and + /// `pr comment` take — not Tangled's own zero-based `/round/` URLs. + /// See docs/output.md, "Comments attach to a round". + pub index: usize, + pub created_at: Option, + pub patch_bytes: Option, +} + +/// One member of `pr view --json`'s `stack.members`, when the pull is +/// stacked. +#[derive(serde::Serialize, Debug, PartialEq)] +pub(crate) struct StackMemberJson { + /// 1-based, counted from the bottom of the stack — the same numbering + /// `atgc stack view` prints, even though both that command and this + /// array list members top first. + pub position: usize, + /// Not printed by the human view, which shows a member's state and title + /// only. Included here because an agent's next move on a stack member is + /// almost always to act on it by URI (`pr checkout`, `pr view `), + /// and the identifier is already in hand — leaving it out would just + /// mean a second command to go and scrape it back out. + pub uri: String, + pub state: Option, + pub title: String, +} + +/// `pr view --json`'s `stack` field, present exactly when the pull belongs +/// to one. +#[derive(serde::Serialize, Debug, PartialEq)] +pub(crate) struct StackJson { + pub total: usize, + /// Which member the surrounding object is describing, 1-based from the + /// bottom — `total` when it is the top. Naming a member now details that + /// member, so a consumer cannot assume the top the way it once could; + /// `null` only if the chain walk and the detail disagree. + pub position: Option, + /// Top first, matching the order the human view prints them in — and + /// including the pull this whole object is about, at its own position, + /// the same way the text loop does. + pub members: Vec, + /// A `dependentOn` link the listing used to build this view did not + /// contain. See [`crate::cmd::stack::Chain::missing_below`]. + pub missing_below: Option, +} + +/// `pr view --json`'s whole object. +#[derive(serde::Serialize, Debug, PartialEq)] +pub(crate) struct PullDetailJson { + pub uri: String, + pub state: Option, + pub title: String, + pub author_did: Option, + pub author_handle: Option, + pub created_at: Option, + pub comments: u64, + pub rounds: Vec, + /// `null` for an empty or whitespace-only body, matching the text view's + /// own "nothing worth printing" test. + pub body: Option, + /// The confirmed `/pulls/` page, and `null` when the appview has no + /// number for this pull yet — [`crate::clients::tangled::web::pulls::PullLink::numbered_url`]. + /// Never the repo's `/pulls` listing, which the text view prints with a + /// note saying it is not the pull; there is nowhere to put that note + /// here, so the field says nothing instead of saying it wrongly. + pub url: Option, + pub stack: Option, + /// The discussion, oldest first, or absent when it was not asked for. + /// + /// Distinct from `comments` above, which is the *count* the listing + /// envelope carries and is there whether or not anybody asked for the + /// bodies. `issue view --json` spells this field the same way. + #[serde(skip_serializing_if = "Option::is_none")] + pub thread: Option>, +} + +/// Build a [`PullDetailJson`] from data `view` has already resolved. +/// +/// `item` is the top pull's `{"uri", "value"}` shape, `state` is +/// [`sources::State::label`]'s output for it, `url` is the already-resolved `view:` +/// link when it names this pull and `None` when it does not, and `stack` is +/// `None` for a pull that stands alone. A pure function of already-resolved +/// inputs, same as [`pull_row_json`]. +pub(super) fn pull_detail_json( + item: &serde_json::Value, + state: &str, + author_handle: Option<&str>, + comments: u64, + url: Option<&str>, + stack: Option, + thread: Option>, +) -> PullDetailJson { + let v = &item["value"]; + let rounds = match v["rounds"].as_array() { + Some(rounds) => rounds + .iter() + .enumerate() + .map(|(i, round)| RoundJson { + index: i + 1, + created_at: round["createdAt"] + .as_str() + .filter(|s| !s.is_empty()) + .map(str::to_string), + patch_bytes: round["patchBlob"]["size"].as_u64(), + }) + .collect(), + // Old records: a single inline patch, no rounds array — the text + // view's own fallback, mirrored. + None => vec![RoundJson { + index: 1, + created_at: v["createdAt"] + .as_str() + .filter(|s| !s.is_empty()) + .map(str::to_string), + patch_bytes: v["patch"].as_str().map(|s| s.len() as u64), + }], + }; + PullDetailJson { + uri: item["uri"].as_str().unwrap_or_default().to_string(), + state: known_state(state), + title: v["title"].as_str().unwrap_or("(untitled)").to_string(), + author_did: author_did(item), + author_handle: author_handle.map(str::to_string), + created_at: v["createdAt"] + .as_str() + .filter(|s| !s.is_empty()) + .map(str::to_string), + comments, + rounds, + body: v["body"] + .as_str() + .map(str::trim) + .filter(|s| !s.is_empty()) + .map(str::to_string), + url: url.map(str::to_string), + stack, + thread, + } +} +/// This repo's pull requests, or — with `--all` — one account's across every +/// repo. +/// +/// One verb and a scope flag, rather than two verbs. The two listings print +/// nearly the same columns and differ in exactly one thing, which is which +/// pulls are in them; when that was carried by the choice between the words +/// `list` and `status`, it was carried by nothing a reader could see. A flag +/// named for the scope sits among the other filters, where the rest of the +/// question is already being asked. +/// +/// The asymmetry underneath is real and is why the flag is not simply "more +/// rows": this repo's pulls are gathered by repo and an account's are gathered +/// by author, because a pull record lives in its author's PDS whatever it +/// targets. See [`Scope`]. +pub(super) async fn list(args: ListArgs) -> Result<()> { + crate::term::jsonout::init(args.json); + match args.all { + true => across_repos(args).await, + false => in_this_repo(args).await, + } +} + +async fn in_this_repo(args: ListArgs) -> Result<()> { + let source = Source::parse_arg(args.source.as_deref())?; + let remote_url = git::remote_url(&args.remote)?; + let repo = resolve::repo_ref(&remote_url).await?; + let me = match source.uses_pds() { + true => own_did().await, + false => None, + }; + + let gathered = gather(Ask { + source, + scope: Scope::Repo, + did: me.as_deref(), + endpoint: "listPulls", + subject: &repo.did, + repo_filter: Some(&repo.did), + limit: args.limit, + reach: Reach::Screen(args.limit as usize), + }) + .await?; + + // Before the listing rather than after it: the warning describes what + // follows, and it is also the one part worth reading when the filter + // empties the list and the early return below fires. + warn_stale( + &gathered.missing, + gathered.pds_total, + Scope::Repo, + gathered.backfilled, + ); + + let filter = args.state.label(); + let unfiltered = gathered.items.len(); + let (mut items, unknown) = apply_state_filter(gathered.items, filter); + items.truncate(args.limit as usize); + + if items.is_empty() { + // `[]` on stdout either way — a script should never have to tell + // "no pulls" apart from "the process died before printing anything". + // The reason still goes out, on stderr rather than the stdout the + // prose version uses, so it is not lost, only moved off the stream + // something downstream is about to parse. + if args.json { + crate::term::say::note!( + Index, + "no pull requests to list ({})", + match classify_empty(filter, unfiltered) { + Empty::NotIndexed => + "none found; run without --json for why that might be index lag", + Empty::NoneInState => "none in the requested state", + } + ); + println!("[]"); + return Ok(()); + } + match classify_empty(filter, unfiltered) { + Empty::NotIndexed => { + // Three different claims, and printing the strongest one + // where only the weakest is earned is the whole bug this + // change is about. Without an index in the source set + // nothing here has looked at anybody else's pulls, so the + // honest sentence is about *you*, not about the repo. + match (me.is_some(), source.spans_authors()) { + (true, false) => { + println!("you have no pull requests on {}", repo.did); + println!( + "Read from your PDS alone, so this says nothing about anyone \ + else's. Add an index to look: --source bobbin (alpha, and \ + its ingest stalls) or --source web (tangled.org's own, scraped)." + ); + } + // Every source asked came back empty, and one of them + // cannot be behind. As close to "this repo has no + // pulls" as this tool gets. + (true, true) => { + println!("no pull requests found for {}", repo.did); + println!( + "Your PDS has none for it either, so this is probably right \ + rather than index lag." + ); + } + (false, _) => { + println!("no pull requests found for {}", repo.did); + println!( + "No account is selected, so your own PDS was not read, and an \ + index can be behind. Nothing here has ruled out there being some." + ); + } + } + } + Empty::NoneInState => println!("no {} pull requests", args.state), + } + println!( + "view: {}", + crate::term::hyperlink::url(&format!("{}/pulls", repo.web_url)) + ); + return Ok(()); + } + + note_author_scoped(source, me.is_some()); + + // One lookup per unique author DID, run together rather than in turn. + let handles = author_handles(&items).await; + + // Numbers are the appview's and are in no record, so they are a separate + // errand — one that reads three listing pages once for the whole page of + // rows rather than anything per row. A row it cannot place keeps the blank + // column it has always had. + let wanted: Vec<(&str, &str)> = items + .iter() + .map(|item| { + ( + item.uri.as_str(), + item.value["value"]["title"].as_str().unwrap_or_default(), + ) + }) + .collect(); + let numbers = + crate::clients::tangled::web::pulls::numbers_for_records(&repo.web_url, &wanted).await; + + // `--json` skips the whole table below: ellipsizing, column padding and + // hyperlink-wrapping are display decisions for a terminal, and every one + // of them would have to be undone by anything reading the output back — + // constraint 4 in the brief this shipped against says so explicitly, + // and this is that: no `ellipsize`, no `hyperlink::url`, no width math, + // unconditionally, not merely because stdout is a pipe. + if args.json { + let rows: Vec = items + .iter() + .map(|item| { + // `handles` falls back to the bare DID, unprefixed, when + // resolution failed — the text column is happy to print + // that in the author slot, but a JSON `author_handle` that + // silently held a DID would be indistinguishable from a + // real handle, so only a label that actually starts with + // `@` counts as one here. + let author_handle = author_did(&item.value) + .and_then(|d| handles.get(&d)) + .and_then(|h| h.strip_prefix('@')) + .map(str::to_string); + pull_row_json( + &item.value, + item.state.label(), + numbers.get(&item.uri).copied(), + author_handle.as_deref(), + item.comments, + ) + }) + .collect(); + crate::term::jsonout::emit(&rows)?; + if gathered.truncated { + crate::term::say::warning!( + Pds, + "stopped after {} pages of your pull records; older ones for this repo \ + may be missing", + crate::clients::atproto::pds::MAX_PAGES + ); + } + note_unknown_states(unknown); + return Ok(()); + } + + for item in &items { + let v = &item.value["value"]; + let state = item.state.label(); + let number = match numbers.get(&item.uri) { + Some(number) => format!("#{number}"), + None => String::new(), + }; + let title = ellipsize(v["title"].as_str().unwrap_or("(untitled)"), 54); + let author = author_did(&item.value) + .and_then(|d| handles.get(&d).cloned()) + .unwrap_or_else(|| "?".to_string()); + let rounds = round_count(v); + let comments = item.comments; + let date = day(v["createdAt"].as_str().unwrap_or("")); + println!( + "{number:<5} {state:<7} {title:<56} {author:<24} {date:<11} \ + rounds:{rounds} comments:{comments}" + ); + } + println!("\nview: {}/pulls", repo.web_url); + if gathered.truncated { + crate::term::say::warning!( + Pds, + "stopped after {} pages of your pull records; older ones for this repo \ + may be missing", + crate::clients::atproto::pds::MAX_PAGES + ); + } + note_unknown_states(unknown); + Ok(()) +} + +/// One account's pull requests across every repo. +/// +/// The listing where dropping Bobbin is not a compromise at all. Every record +/// this half wants lives in one PDS — the subject's — so a single +/// `listRecords` answers it completely, and no index can be behind on it. +/// Bobbin is still asked, because it is the only thing that knows about +/// status records written by a maintainer who is not the author, and about +/// comment counts. But it is now an enrichment: when it is stale or down, +/// this still lists every pull, marking as `?` only the states it genuinely +/// cannot see. +/// +/// Needs no checkout and no session: reading somebody's pulls wants a DID and +/// nothing else, so this path stays available to an account whose session has +/// expired, and from a directory that is not a git repo at all. +async fn across_repos(args: ListArgs) -> Result<()> { + let source = Source::parse_arg(args.source.as_deref())?; + // Reading someone's pulls needs no token, only a DID — so this path + // stays available for an account whose session has expired. + let did = match &args.author { + Some(author) => crate::config::account::actor_did(author).await?, + None => crate::config::account::select().await?.did, + }; + + let gathered = gather(Ask { + source, + scope: Scope::Author, + did: Some(&did), + endpoint: "listPullsBy", + subject: &did, + repo_filter: None, + limit: args.limit, + reach: Reach::Screen(args.limit as usize), + }) + .await?; + + warn_stale( + &gathered.missing, + gathered.pds_total, + Scope::Author, + gathered.backfilled, + ); + + let filter = args.state.label(); + let unfiltered = gathered.items.len(); + let (mut items, unknown) = apply_state_filter(gathered.items, filter); + items.truncate(args.limit as usize); + + if items.is_empty() { + let why = match classify_empty(filter, unfiltered) { + Empty::NoneInState => format!("no {} pull requests", args.state), + Empty::NotIndexed => format!("no pull requests for {did}"), + }; + // `[]` on stdout and the reason on stderr, the same split `pr list` + // makes: an empty listing is a result, not an absence of one. + if args.json { + crate::term::say::note!(Index, "{why}"); + println!("[]"); + return Ok(()); + } + println!("{why}"); + return Ok(()); + } + + // One lookup per unique repo DID, run together rather than in turn. + let repos = repo_names(&items).await; + + let numbers = numbers_across_repos(&items, &repos).await; + + if args.json { + // The table has no author column — every row is the one account's — + // so this handle is resolved for the JSON alone, once for the page. + // It is what keeps the row shape genuinely the same as `pr list + // --json`'s rather than the same minus a field. + let subject_handle = crate::clients::atproto::handles::handle(&did).await; + let rows: Vec = items + .iter() + .map(|item| { + let repo_did = target_repo_did(&item.value); + // Only the subject's own handle is in hand. A row filed by + // anyone else — which a Bobbin-sourced listing can carry — + // reads `null` rather than borrowing this one. + let author_handle = subject_handle + .as_deref() + .filter(|_| author_did(&item.value).as_deref() == Some(did.as_str())); + status_row_json( + &item.value, + item.state.label(), + numbers.get(&item.uri).copied(), + author_handle, + item.comments, + repo_did.clone(), + repo_did.as_deref().and_then(|d| repos.get(d)), + ) + }) + .collect(); + crate::term::jsonout::emit(&rows)?; + note_unknown_states(unknown); + return Ok(()); + } + + for item in &items { + let v = &item.value["value"]; + let state = item.state.label(); + let number = match numbers.get(&item.uri) { + Some(number) => format!("#{number}"), + None => String::new(), + }; + let title = ellipsize(v["title"].as_str().unwrap_or("(untitled)"), 54); + let repo = target_repo_did(&item.value) + .and_then(|d| repos.get(&d).map(|name| name.label.clone())) + .unwrap_or_else(|| "?".to_string()); + let rounds = round_count(v); + let comments = item.comments; + let date = day(v["createdAt"].as_str().unwrap_or("")); + println!( + "{number:<5} {state:<7} {title:<56} {repo:<28} {date:<11} \ + rounds:{rounds} comments:{comments}" + ); + } + note_unknown_states(unknown); + Ok(()) +} +#[derive(clap::Args, Debug)] +pub(crate) struct ViewArgs { + /// The pull request: its number, record key, at:// URI, or Tangled URL + /// + /// Defaults to the one this branch is about. + #[arg(value_name = "PULL")] + pub pull: Option, + /// The same, as a flag, for symmetry with the other verbs + #[arg(long, value_name = "PULL", conflicts_with = "pull")] + pub pr: Option, + /// Whose pull request it is, when a bare record key is ambiguous + #[arg(long, value_name = "HANDLE|DID")] + pub author: Option, + /// Git remote pointing at the repo + #[arg(long, default_value = "origin")] + pub remote: String, + /// Also open the repo's pulls page in the browser + #[arg(long)] + pub web: bool, + /// Also show the discussion (needs the index: --source bobbin) + /// + /// A comment is a record in its commenter's PDS and nothing enumerates + /// the people who have commented, so a thread can only come from the + /// appview's index, whose ingest stalls. + #[arg(long)] + pub comments: bool, + /// Where to read from: pds (default), bobbin, web, auto for all three, + /// or a comma-separated combination. Both indexes are opt-in; + /// ATGC_USE_BOBBIN=1 and ATGC_USE_WEB=1 add one to the default + #[arg(long)] + pub source: Option, + /// Print a JSON object instead of the human view; colour, hyperlinks + /// and day-truncated dates are all off (see --help for the field + /// contract's stability note) + #[arg(long)] + pub json: bool, +} + +impl ViewArgs { + /// The pull this names, whichever spelling was used. + /// + /// The positional and `--pr` are declared `conflicts_with` each other, so + /// clap has already refused naming one pull twice; this only picks + /// whichever of the two carries it. + pub(crate) fn pull(&self) -> Option<&str> { + self.pull.as_deref().or(self.pr.as_deref()) + } +} + +/// A pull request's detail view: a named pull from anywhere, or the current +/// branch's — and only that one. +/// +/// The command most damaged by index lag, because the pull it wants is +/// almost always the one just opened — the least likely to be indexed and +/// the one whose author's PDS is certainly yours. Reading the PDS first +/// makes `pr view` work immediately after `pr create` instead of hours later. +/// +/// The match is `source.branch` and nothing else. It used to fall back to +/// your newest pull on the repo, which sounds like a courtesy and is not +/// one: run from a second worktree it answers with whatever you opened most +/// recently, on any branch, and says nothing about having guessed. A wrong +/// answer that looks right is worse than no answer, the more so because the +/// `uri:` line it prints is what a person then feeds to `pr close`. +pub(crate) async fn view(args: ViewArgs) -> Result<()> { + crate::term::jsonout::init(args.json); + let source = Source::parse_arg(args.source.as_deref())?; + // Resolved up front so that a bad reference fails before any listing is + // fetched. `pr view 23` is the spelling people try first, because every + // sibling command (`diff`, `checkout`, `merge`, `edit`, `comment`) + // accepts it; being the one refusal in the family taught nobody anything. + let named = match args.pull() { + Some(reference) => Some( + crate::cmd::pr::review::resolve_pull( + Some(reference), + args.author.as_deref(), + &args.remote, + ) + .await?, + ), + None => None, + }; + let remote_url = git::remote_url(&args.remote)?; + let repo = resolve::repo_ref(&remote_url).await?; + let gathered = gather_branch_pulls(source, &repo).await?; + let items: Vec<&serde_json::Value> = gathered.items.iter().map(|i| &i.value).collect(); + + // A named pull is looked up in the listing so its state and chain can + // be shown; one the index has not caught up with is shown from the + // record alone, because the record in hand outranks the index that + // lacks it. Only the no-argument form reads the checked-out branch — + // naming a pull works from a detached HEAD. + let unlisted: serde_json::Value; + let item: &serde_json::Value = match &named { + Some(pull) => match items + .iter() + .copied() + .find(|i| i["uri"].as_str() == Some(pull.uri.as_str())) + { + Some(item) => item, + None => { + crate::term::say::note!( + Index, + "this pull is not in the repo listing yet (index lag, or another \ + repo's pull): showing the record by itself" + ); + unlisted = serde_json::json!({ "uri": pull.uri, "value": pull.value }); + &unlisted + } + }, + None => { + let branch = git::current_branch()?; + require_branch_pull(&gathered.items, &branch)? + } + }; + // A stacked branch matches every member of its stack — they all share + // `source.branch` — so the newest match is only a way into the chain. + // Before this, `pr view` on a stacked branch showed whichever member was + // created last, with nothing to say others existed: a wrong answer in + // the shape of a right one. With a chain, the top pull is the detail and + // the chain is printed below it. + // A chain that cannot be ordered — a fork or a loop in somebody's + // records, which the appview refuses to ingest but a PDS happily holds — + // must degrade this view, not destroy it. `stack view` is the command + // that refuses loudly; `pr view` says what it saw and shows the newest + // match flat, because "one damaged record on the branch" and "no answer + // about your pull at all" are very different sizes of problem. + let chain = + match crate::cmd::stack::chain_containing(&items, item["uri"].as_str().unwrap_or_default()) + { + Ok(chain) => chain, + Err(e) => { + crate::term::say::warning!( + Pds, + "this branch's pulls do not form an orderable stack: {e}\n\ + showing the newest match for the branch by itself" + ); + None + } + }; + // Only the branch match is promoted to the top. A *named* pull is not a + // way into the chain — it is the answer, and substituting the top for it + // was this command's own "wrong answer in the shape of a right one": + // every member of a stack reported the top's title, body, rounds and + // state under the number, rkey, at:// URI or URL of the member asked + // for, so two different members read back byte-identical. Nothing said + // a swap had happened; in `--json` only `url` disagreed with the + // question. The chain is still built either way — it is what the + // `stack:` block below is drawn from. + let item = detail_item(named.is_some(), chain.as_ref(), item); + // Where the pull being detailed sits in its chain, 1-based from the + // bottom, so both views can say which member this is rather than + // asserting it is the top. + let position = chain.as_ref().and_then(|chain| { + chain + .members + .iter() + .position(|m| m["uri"] == item["uri"]) + .map(|i| i + 1) + }); + let found = gathered + .items + .iter() + .find(|i| i.uri.as_str() == item["uri"].as_str().unwrap_or_default()); + + let v = &item["value"]; + let title = v["title"].as_str().unwrap_or("(untitled)"); + // One pull, one unresolved state: exactly the case the targeted web + // confirm exists for. Auto only — `--source pds` asked for no third + // parties and `--source bobbin` wants Bobbin's unpatched answer — and + // only after every record-borne source has come up empty. A miss keeps + // the `?`: the scrape is a witness, not a fourth source of truth. + let mut state = found.map_or("?", |i| i.state.label()).to_string(); + if state == "?" + && source.uses_web() + && let Some(confirmed) = crate::clients::tangled::web::backfill::state_of( + &repo.web_url, + item["uri"].as_str().unwrap_or_default(), + title, + ) + .await + { + state = confirmed.to_string(); + } + // Resolved once and shared by both the human and `--json` paths below, + // rather than the human column's `format!("@{h}")` baked in early: JSON + // wants the DID and the bare handle as two separate, unambiguous fields + // instead of one string that means different things depending on + // whether the lookup succeeded. + let author_handle = match author_did(item) { + Some(did) => crate::clients::atproto::handles::handle(&did).await, + None => None, + }; + let author = match (author_did(item), &author_handle) { + (Some(_), Some(h)) => format!("@{h}"), + (Some(did), None) => did, + (None, _) => "?".to_string(), + }; + let created = day(v["createdAt"].as_str().unwrap_or("")); + let comments = item["commentCount"].as_u64().unwrap_or(0); + let uri = item["uri"].as_str().unwrap_or("?"); + let thread = crate::cmd::read_thread(args.comments, source, uri).await; + // The number is the appview's and is in no record, so this is a lookup + // rather than a format — and one that falls back to the repo's listing, + // which is what the line said before it could ever be more specific. The + // fallback says so now: a `view:` line that quietly widened to the whole + // repo is the one link a reader cannot tell has widened. Resolved before + // either rendering, since `--json` needs it too. + let link = crate::clients::tangled::web::pulls::pull_url(&repo.web_url, uri, title).await; + + if args.json { + let stack = chain.as_ref().map(|chain| { + let total = chain.members.len(); + let members = chain + .members + .iter() + .enumerate() + .rev() + .map(|(i, member)| { + let member_uri = member["uri"].as_str().unwrap_or_default(); + let member_state = gathered + .items + .iter() + .find(|g| g.uri == member_uri) + .map_or("?", |g| g.state.label()); + StackMemberJson { + position: i + 1, + uri: member_uri.to_string(), + state: known_state(member_state), + title: member["value"]["title"] + .as_str() + .unwrap_or("(untitled)") + .to_string(), + } + }) + .collect(); + StackJson { + total, + position, + members, + missing_below: chain.missing_below.clone(), + } + }); + let detail = pull_detail_json( + item, + &state, + author_handle.as_deref(), + comments, + link.numbered_url(), + stack, + crate::cmd::thread_json(&thread).await, + ); + crate::term::jsonout::emit(&detail)?; + // On stderr in both modes, and for the same reason: it is the only + // thing that says why `url` is null and why `--web` is about to open + // a page the pull is merely somewhere on. + if let Some(note) = link.note() { + crate::term::say::note!(Index, "{note}"); + } + if args.web { + crate::term::noinput::open_in_browser(link.url()); + } + return Ok(()); + } + + println!("{state} {title}"); + println!("author: {author}"); + println!("created: {created}"); + println!("comments: {comments}"); + match v["rounds"].as_array() { + Some(rounds) => { + println!("rounds: {}", rounds.len()); + for (i, round) in rounds.iter().enumerate() { + let date = day(round["createdAt"].as_str().unwrap_or("?")); + match round["patchBlob"]["size"].as_u64() { + Some(size) => println!(" {}: {date}, patch {size} bytes", i + 1), + None => println!(" {}: {date}", i + 1), + } + } + } + // Old records: a single inline patch, no rounds array. + None => { + println!("rounds: 1"); + let size = v["patch"].as_str().map(str::len).unwrap_or(0); + println!(" 1: {created}, patch {size} bytes"); + } + } + if let Some(chain) = &chain { + let total = chain.members.len(); + let which = match position { + Some(p) if p == total => "this is the top".to_string(), + Some(p) => format!("this is {p} of {total}"), + // The detail is in the chain by construction; a miss would mean + // the walk and the item disagreed, and silence would be a lie. + None => "this pull's place in it is unclear".to_string(), + }; + println!("stack: {total} pulls; {which}: `atgc stack view` for the chain"); + for (i, member) in chain.members.iter().enumerate().rev() { + let member_uri = member["uri"].as_str().unwrap_or("?"); + let state = gathered + .items + .iter() + .find(|g| g.uri == member_uri) + .map_or("?", |g| g.state.label()); + // Two spaces or an arrow, so the rows stay in one column and the + // one being detailed is findable without counting. + let mark = match position == Some(i + 1) { + true => "> ", + false => " ", + }; + println!( + "{mark}{}/{total} {state:<7} {}", + i + 1, + member["value"]["title"].as_str().unwrap_or("(untitled)"), + ); + } + if let Some(missing) = &chain.missing_below { + println!(" ...continues below {missing}, which this listing does not contain"); + } + } + if let Some(body) = v["body"].as_str().filter(|b| !b.trim().is_empty()) { + println!("\n{}", body.trim_end()); + } + println!(); + println!("uri: {uri}"); + println!("view: {}", crate::term::hyperlink::url(link.url())); + crate::cmd::print_thread(&thread).await; + if let Some(note) = link.note() { + crate::term::say::note!(Index, "{note}"); + } + if args.web { + crate::term::noinput::open_in_browser(link.url()); + } + Ok(()) +} +/// The pull [`view`] details: the top of the chain when the branch was the +/// question, and the pull itself when one was named. +/// +/// Kept apart from [`view`] for the same reason [`for_branch`] is — "naming +/// a pull answers about that pull" is a rule a test should be able to hold +/// still, and observing it through [`view`] would take a network. `named` +/// is only whether the caller gave a reference; which record it resolved to +/// is already `matched`. +pub(super) fn detail_item<'a>( + named: bool, + chain: Option<&crate::cmd::stack::Chain<'a>>, + matched: &'a serde_json::Value, +) -> &'a serde_json::Value { + match (chain, named) { + (Some(chain), false) => chain.top(), + _ => matched, + } +} +#[cfg(test)] +mod tests { + use super::fixtures::{ATGC_REPO, bobbin_items, old_item}; + use super::labels::RepoName; + use super::{StackJson, StackMemberJson, pull_detail_json, pull_row_json}; + use super::{detail_item, ellipsize, known_state, round_count, status_row_json}; + use serde_json::{Value, json}; + + /// Live records all carry `rounds`; pre-rounds ones carry one inline + /// patch, which is one round however it is spelled. A zero here would + /// print "rounds:0" against a PR that plainly has a patch in it. + #[test] + fn counts_rounds_across_both_record_shapes() { + for item in &bobbin_items() { + let expected = item["value"]["rounds"].as_array().unwrap().len(); + assert_eq!(round_count(&item["value"]), expected); + } + let old = old_item(); + assert!(old["value"].get("rounds").is_none()); + assert_eq!(round_count(&old["value"]), 1); + } + + // ----------------------------------------------------------------------- + // `--json`: pure serializers, pinned against real records + // ----------------------------------------------------------------------- + + #[test] + fn known_state_turns_the_question_mark_into_null_and_nothing_else() { + assert_eq!(known_state("?"), None); + assert_eq!(known_state("open"), Some("open".to_string())); + assert_eq!(known_state("merged"), Some("merged".to_string())); + } + + /// The happy path: a live record with one round, a resolved number and a + /// resolved handle, pinned field by field against what + /// `bobbin_list_pulls_by.json`'s first item actually holds. + #[test] + fn pull_row_json_pulls_the_fields_the_table_already_computes() { + let item = &bobbin_items()[0]; + let row = pull_row_json(item, "merged", Some(42), Some("permadeath.com"), 3); + assert_eq!(row.uri, item["uri"].as_str().unwrap()); + assert_eq!(row.number, Some(42)); + assert_eq!(row.state.as_deref(), Some("merged")); + assert_eq!(row.title, "Include branch in image tag"); + assert_eq!( + row.author_did.as_deref(), + Some("did:plc:nlzmjyfv6loqtxyzvdcznwgf") + ); + assert_eq!(row.author_handle.as_deref(), Some("permadeath.com")); + assert_eq!(row.created_at.as_deref(), Some("2026-08-05T04:31:07+03:00")); + assert_eq!(row.rounds, 1); + assert_eq!(row.comments, 3); + } + + /// Everything unresolved comes back `null`, not the terminal column's + /// `"?"` or an empty string — the whole reason this type exists rather + /// than reusing the display strings. + #[test] + fn pull_row_json_unresolved_fields_are_null_not_placeholder_strings() { + let item = &bobbin_items()[0]; + let row = pull_row_json(item, "?", None, None, 0); + assert_eq!(row.state, None); + assert_eq!(row.number, None); + assert_eq!(row.author_handle, None); + // The DID is still there — only the *handle* was unresolved. + assert!(row.author_did.is_some()); + } + + /// A pre-rounds record: no `rounds` array and an empty `createdAt`, + /// which is real bytes off the network rather than an invented edge + /// case — see `pull_old_record.json`'s own doc comment. + #[test] + fn pull_row_json_old_record_has_no_created_at_and_counts_one_round() { + let item = old_item(); + let row = pull_row_json(&item, "closed", None, None, 0); + assert_eq!(row.created_at, None); + assert_eq!(row.rounds, 1); + } + + /// /// The exact key set a `jq` pipeline would see, pinned as a shape rather + /// than as the values inside it: a renamed field breaks a caller silently. + #[test] + fn pull_row_json_serializes_with_the_documented_field_names() { + let item = &bobbin_items()[0]; + let row = pull_row_json(item, "merged", Some(7), Some("permadeath.com"), 2); + let value = serde_json::to_value(&row).unwrap(); + let mut keys: Vec<&str> = value + .as_object() + .unwrap() + .keys() + .map(String::as_str) + .collect(); + keys.sort_unstable(); + assert_eq!( + keys, + vec![ + "author_did", + "author_handle", + "comments", + "created_at", + "number", + "rounds", + "state", + "title", + "uri", + ] + ); + } + + /// `status pr --json` is a `pr list --json` row with the repo columns + /// flattened in beside it, not nested under one — a caller moving from + /// one listing to the other reads `.state`, not `.pull.state`. + #[test] + fn status_row_json_flattens_the_pull_row_beside_the_repo() { + let item = &bobbin_items()[0]; + let repo = RepoName { + label: "@permadeath.com/atgc".to_string(), + web_url: Some("https://tangled.org/permadeath.com/atgc".to_string()), + }; + let row = status_row_json( + item, + "merged", + Some(7), + Some("permadeath.com"), + 2, + Some(ATGC_REPO.to_string()), + Some(&repo), + ); + let value = serde_json::to_value(&row).unwrap(); + let mut keys: Vec<&str> = value + .as_object() + .unwrap() + .keys() + .map(String::as_str) + .collect(); + keys.sort_unstable(); + assert_eq!( + keys, + vec![ + "author_did", + "author_handle", + "comments", + "created_at", + "number", + "repo", + "repo_did", + "repo_url", + "rounds", + "state", + "title", + "uri", + ] + ); + // No `@`, matching `author_handle` and every other JSON field that + // holds a name a URL is built from. + assert_eq!(value["repo"], "permadeath.com/atgc"); + assert_eq!(value["state"], "merged"); + } + + /// A repo neither lookup could name keeps its DID and reads `null` for + /// the name and the URL. The text column prints a truncated DID there — + /// a display string with an ellipsis in it, which is not a repo anybody + /// can fetch and so is not a value this may emit. + #[test] + fn status_row_json_unresolved_repo_is_null_not_a_truncated_did() { + let item = &bobbin_items()[0]; + let unresolved = RepoName { + label: ellipsize(ATGC_REPO, 21), + web_url: None, + }; + let row = status_row_json( + item, + "open", + None, + None, + 0, + Some(ATGC_REPO.to_string()), + Some(&unresolved), + ); + assert_eq!(row.repo, None); + assert_eq!(row.repo_url, None); + assert_eq!(row.repo_did.as_deref(), Some(ATGC_REPO)); + } + + /// The old-record fallback `pr view` prints as a single inline round: + /// one entry, its bytes taken from the inline `patch` string rather than + /// a `patchBlob`. + #[test] + fn pull_detail_json_old_record_falls_back_to_one_inline_round() { + let item = old_item(); + let detail = pull_detail_json( + &item, + "closed", + None, + 0, + Some("https://tangled.org/x/pulls/9"), + None, + None, + ); + assert_eq!(detail.rounds.len(), 1); + assert_eq!(detail.rounds[0].index, 1); + assert!(detail.rounds[0].patch_bytes.unwrap() > 0); + assert_eq!(detail.created_at, None); + assert_eq!(detail.stack, None); + } + + /// A live record's `rounds` array is walked one-for-one, one-based, with + /// the round's own blob size rather than anything recomputed. + #[test] + fn pull_detail_json_lists_every_round_from_the_record() { + let item = &bobbin_items()[0]; + let detail = pull_detail_json( + item, + "merged", + Some("permadeath.com"), + 1, + Some("https://tangled.org/x/pulls/1"), + None, + None, + ); + assert_eq!(detail.rounds.len(), 1); + assert_eq!(detail.rounds[0].index, 1); + assert_eq!(detail.rounds[0].patch_bytes, Some(5145)); + assert_eq!(detail.state.as_deref(), Some("merged")); + assert_eq!(detail.url.as_deref(), Some("https://tangled.org/x/pulls/1")); + assert_eq!( + detail.body.as_deref(), + Some("Co-Authored-By: Claude Fable 5 ") + ); + } + + /// A body that is present but only whitespace prints nothing in the + /// human view, and reads `null` here for the same reason. + #[test] + fn pull_detail_json_blank_body_is_null() { + let item = json!({ + "uri": "at://did:plc:me/sh.tangled.repo.pull/3xyz", + "value": { "title": "t", "body": " \n ", "createdAt": "2026-01-01T00:00:00Z" }, + }); + let detail = pull_detail_json( + &item, + "open", + None, + 0, + Some("https://x/pulls/1"), + None, + None, + ); + assert_eq!(detail.body, None); + } + + /// The `stack` field is carried through exactly as built — this only + /// pins that `pull_detail_json` does not touch it, since the assembly + /// itself lives in `view` next to the chain it walks. + #[test] + fn pull_detail_json_carries_a_stack_through_untouched() { + let item = &bobbin_items()[0]; + let stack = StackJson { + total: 2, + position: Some(2), + members: vec![StackMemberJson { + position: 2, + uri: "at://did:plc:me/sh.tangled.repo.pull/3top".to_string(), + state: Some("open".to_string()), + title: "top".to_string(), + }], + missing_below: Some("at://did:plc:me/sh.tangled.repo.pull/3below".to_string()), + }; + let detail = pull_detail_json( + item, + "open", + None, + 0, + Some("https://x/pulls/1"), + Some(stack), + None, + ); + let stack = detail.stack.expect("stack should be Some"); + assert_eq!(stack.total, 2); + assert_eq!(stack.position, Some(2)); + assert_eq!(stack.members.len(), 1); + assert_eq!(stack.members[0].position, 2); + assert_eq!( + stack.missing_below.as_deref(), + Some("at://did:plc:me/sh.tangled.repo.pull/3below") + ); + } + + /// Naming a member of a stack details *that* member. This is the whole + /// of the bug it replaces: the top was substituted for every named pull, + /// so a stack of eight answered eight different numbers, rkeys, at:// + /// URIs and URLs with one identical record, and nothing in the output + /// said so. + #[test] + fn naming_a_stack_member_details_that_member_not_the_top() { + let bottom = json!({ "uri": "at://did:plc:me/sh.tangled.repo.pull/3bot" }); + let top = json!({ "uri": "at://did:plc:me/sh.tangled.repo.pull/3top" }); + let chain = crate::cmd::stack::Chain { + members: vec![&bottom, &top], + missing_below: None, + }; + assert_eq!( + detail_item(true, Some(&chain), &bottom)["uri"], + bottom["uri"], + "a named pull is the answer, not a way into the chain" + ); + } + + /// The no-argument form keeps promoting to the top, which is what makes + /// the chain readable from a stacked branch at all: every member shares + /// `source.branch`, so the branch match alone is arbitrary. + #[test] + fn a_branch_match_is_still_promoted_to_the_top() { + let bottom = json!({ "uri": "at://did:plc:me/sh.tangled.repo.pull/3bot" }); + let top = json!({ "uri": "at://did:plc:me/sh.tangled.repo.pull/3top" }); + let chain = crate::cmd::stack::Chain { + members: vec![&bottom, &top], + missing_below: None, + }; + assert_eq!(detail_item(false, Some(&chain), &bottom)["uri"], top["uri"]); + } + + /// A pull that stands alone is itself either way — no chain, nothing to + /// promote to, and the named and branch paths must not diverge here. + #[test] + fn an_unstacked_pull_is_its_own_detail() { + let only = json!({ "uri": "at://did:plc:me/sh.tangled.repo.pull/3one" }); + assert_eq!(detail_item(true, None, &only)["uri"], only["uri"]); + assert_eq!(detail_item(false, None, &only)["uri"], only["uri"]); + } + + /// The exact key set `pr view --json` prints, including that `stack` is + /// present (as `null`) even for a pull that stands alone rather than + /// being omitted — a script checking `.stack == null` should not have + /// to also check `has("stack")`. + #[test] + fn pull_detail_json_serializes_with_the_documented_field_names() { + let item = &bobbin_items()[0]; + let detail = pull_detail_json( + item, + "merged", + None, + 0, + Some("https://x/pulls/1"), + None, + None, + ); + let value = serde_json::to_value(&detail).unwrap(); + let mut keys: Vec<&str> = value + .as_object() + .unwrap() + .keys() + .map(String::as_str) + .collect(); + keys.sort_unstable(); + assert_eq!( + keys, + vec![ + "author_did", + "author_handle", + "body", + "comments", + "created_at", + "rounds", + "stack", + "state", + "title", + "uri", + "url", + ] + ); + assert_eq!(value["stack"], Value::Null); + } +} diff --git a/src/cmd/pr/read.rs b/src/cmd/pr/read/sources.rs similarity index 52% rename from src/cmd/pr/read.rs rename to src/cmd/pr/read/sources.rs index 0f9ef4f..e4d868d 100644 --- a/src/cmd/pr/read.rs +++ b/src/cmd/pr/read/sources.rs @@ -1,21 +1,10 @@ -//! Listing pull requests: `atgc pr list`, `pr list --all` and `pr view`. +//! Source reconciliation: what the set of pull requests actually is. //! -//! The read half of [`crate::cmd::pr`]. Nothing in here authenticates: a -//! `sh.tangled.repo.pull` record is public, so is every -//! `com.atproto.repo.listRecords` call that fetches one, and so is Bobbin. A -//! session buys this half exactly one thing — a DID whose PDS is worth -//! reading — and its absence costs that and no more. -//! -//! What makes the half awkward is that a pull request has two homes and -//! neither is complete. The record lives in its **author's** PDS, so your own -//! pulls are one public request away and are visible the instant the write -//! returns; everyone else's are scattered across the PDSes of people nothing -//! enumerates, which is why answering "what pulls exist on this repo" needs -//! an index and why [`Source`] exists to name which sources a listing used. -//! So `pr list --all`, scoped to one account, can be answered off a PDS alone and -//! never be stale, while `pr list`, scoped to a repo, cannot — and on -//! 2026-08-06 Bobbin returned nothing at all for a repo whose author's PDS -//! held thirty pull records aimed at it. +//! The asymmetry [`super`]'s documentation opens with — a pull record lives +//! in its author's PDS, so one source is complete about one account and +//! another is the only thing that can speak for a repo — is this module's +//! whole subject, and the paragraph that used to sit at the top of the file +//! is about the code in here: //! //! Most of the code below is that problem rather than the printing. [`merge`] //! joins the two sources on the at-uri and keeps track of which one supplied @@ -24,25 +13,16 @@ //! that a pull whose status record nothing available can see is not reported //! as open, which would be a guess wearing a fact's clothes. +use super::labels::{appview_url, repo_label_from_appview}; use crate::clients::git::run as git; use crate::clients::tangled::resolve; use crate::lexicon::tangled::PullState; -use crate::term::column::{day, ellipsize}; use anyhow::Result; -use futures_util::stream::{self, StreamExt}; use jacquard::types::string::Datetime; use std::collections::HashMap; use std::collections::hash_map::Entry; use std::str::FromStr; -/// How many rounds a pull has been through. Records written before the -/// rounds migration carry a single inline patch and no `rounds` array; they -/// are one round by definition, so a missing array counts as one rather than -/// as none. Crate-visible for [`crate::cmd::stack`]'s chain display. -pub(in crate::cmd) fn round_count(value: &serde_json::Value) -> usize { - value["rounds"].as_array().map(|r| r.len()).unwrap_or(1) -} - // --------------------------------------------------------------------------- // Where a pull listing comes from // --------------------------------------------------------------------------- @@ -187,7 +167,7 @@ impl Source { self.bobbin } - fn uses_web(self) -> bool { + pub(super) fn uses_web(self) -> bool { self.web } @@ -197,7 +177,7 @@ impl Source { /// before it picks its wording. With no index in play the honest answer /// is "you have none", which is a much smaller claim than "there are /// none" and must not be printed as if it were the larger one. - fn spans_authors(self) -> bool { + pub(super) fn spans_authors(self) -> bool { self.bobbin || self.web } } @@ -288,7 +268,7 @@ impl FromStr for Source { /// A pull's state, and whether it is actually known. #[derive(Clone, Debug, PartialEq, Eq)] -enum State { +pub(super) enum State { /// Bobbin's answer, or a `sh.tangled.repo.pull.status` record the account /// whose PDS we read wrote itself. Known(String), @@ -306,7 +286,7 @@ enum State { } impl State { - fn label(&self) -> &str { + pub(super) fn label(&self) -> &str { match self { State::Known(s) => s, State::Unknown => "?", @@ -323,19 +303,19 @@ impl State { /// One pull request, however it was arrived at. #[derive(Clone, Debug)] -struct Listed { +pub(super) struct Listed { /// `at:///sh.tangled.repo.pull/`. The identity of a /// pull, and so the key the two sources are deduplicated on. - uri: String, + pub(super) uri: String, /// The record itself, in the shape both sources happen to agree on: PDS /// `listRecords` and Bobbin's item envelope both nest it under `value`. - value: serde_json::Value, - state: State, - comments: u64, + pub(super) value: serde_json::Value, + pub(super) state: State, + pub(super) comments: u64, /// Whether Bobbin returned this one. The only input to the staleness /// evidence, and the reason the merge keeps the flag rather than /// discarding provenance once the records are joined. - indexed: bool, + pub(super) indexed: bool, } impl Listed { @@ -356,7 +336,7 @@ impl Listed { /// Why a pull listing came back with nothing in it. #[derive(Debug, PartialEq, Eq)] -enum Empty { +pub(super) enum Empty { /// There are pulls, just none in the asked-for state. NoneInState, /// Neither source has a pull for this subject in any state. For a repo @@ -375,112 +355,12 @@ enum Empty { /// was forced by the staleness check rather than made for speed: comparing a /// `status=open` page against the PDS would report every merged pull as /// missing from the index. -fn classify_empty(filter: Option<&str>, unfiltered: usize) -> Empty { +pub(super) fn classify_empty(filter: Option<&str>, unfiltered: usize) -> Empty { match filter { Some(_) if unfiltered > 0 => Empty::NoneInState, _ => Empty::NotIndexed, } } - -/// Split `at:///sh.tangled.repo/` into its owner and name. -/// The collection sits between them, hence the skip. -fn owner_and_name(uri: &str) -> Option<(&str, &str)> { - let mut parts = uri.strip_prefix("at://")?.split('/'); - Some((parts.next()?, parts.nth(1)?)) -} - -/// The words `--state` accepts, on either pull listing. -/// -/// A `ValueEnum` rather than a `String` because the string took anything: -/// `--state opne` matched no row, printed "no opne pull requests", and read -/// as *there are none*. That is the failure this tree spent a release -/// arguing about — an answer given in the shape and with the confidence of a -/// real one — reproduced locally by a flag, and it costs one enum to make it -/// a refusal that names the four words instead. clap prints them in `--help` -/// as a side effect. -/// -/// The *input* side alone. What a record says a pull's state is stays -/// [`State::Known`]'s unconstrained `String`, holding whatever word the PDS -/// or the index returned, for the reason spelled out there and in -/// [`PullState::from_token`]: a state Tangled adds after this build has to -/// list rather than crash. A filter is typed by a person and can be a typo; a -/// status record is somebody else's fact and cannot be. -/// -/// `All` filters nothing, which also makes it the only way to see a pull -/// whose state nothing available could settle: an unknown state matches no -/// filter, so every other value hides those rows (and says how many). -#[derive(clap::ValueEnum, Clone, Copy, Debug, PartialEq, Eq)] -pub(crate) enum StateFilter { - Open, - Closed, - Merged, - /// Every state, including a pull whose state is unknown - All, -} - -impl StateFilter { - /// The label a row's state is compared against, or `None` for `all`: - /// exactly the shape [`apply_state_filter`] and [`classify_empty`] want, - /// where `None` means "keep everything". - fn label(self) -> Option<&'static str> { - match self { - StateFilter::Open => Some(PullState::Open.label()), - StateFilter::Closed => Some(PullState::Closed.label()), - StateFilter::Merged => Some(PullState::Merged.label()), - StateFilter::All => None, - } - } -} - -impl std::fmt::Display for StateFilter { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.write_str(self.label().unwrap_or("all")) - } -} - -#[derive(clap::Args, Debug)] -pub(crate) struct ListArgs { - /// Every repo instead of this one: your pulls wherever you filed them - /// - /// Widening is per author and not per repo, because the records are read - /// from a PDS: a pull lives in the PDS of whoever wrote it, so "every - /// repo" is answerable for one account and there is no listing of - /// everyone's pulls everywhere to ask for. It is you unless `--author` - /// names somebody else, and it needs no checkout. - #[arg(long)] - pub all: bool, - /// Show another account's pull requests (handle or DID); needs --all - #[arg(long, requires = "all")] - pub author: Option, - /// Filter by state - #[arg(long, default_value = "open")] - pub state: StateFilter, - /// Maximum number of PRs to show - /// A floor of 1, the same `--limit` `search` has carried since it - /// shipped. `--limit 0` used to parse and then print "no open pull - /// requests" — an empty listing that reads as *there are none*, which is - /// the stale-index failure this tree opted out of arriving from the - /// command line instead. It is now clap's own refusal, naming the range, - /// exit 2: the same treatment `--state opne` got and for the same reason. - #[arg(long, default_value_t = 30, value_parser = clap::value_parser!(u32).range(1..))] - pub limit: u32, - /// Git remote pointing at the repo. Ignored with --all, which is not - /// scoped to a checkout at all - #[arg(long, default_value = "origin")] - pub remote: String, - /// Where to read from: pds (default, your own pulls and never stale), - /// bobbin, web, auto for all three, or a comma-separated combination. - /// Both indexes are opt-in; ATGC_USE_BOBBIN=1 and ATGC_USE_WEB=1 add - /// one to the default, and ATGC_BOBBIN points at another instance - #[arg(long)] - pub source: Option, - /// Print a JSON array instead of a table, one object per pull; - /// colour, hyperlinks and ellipsizing are all off (see --help for - /// the field contract's stability note) - #[arg(long)] - pub json: bool, -} - // --------------------------------------------------------------------------- // The PDS side: an account's own records, no index involved // --------------------------------------------------------------------------- @@ -607,7 +487,7 @@ pub(crate) async fn pds_pulls( /// here can be absent but never wrong about what the account itself did. /// /// A status variant this build does not recognize is skipped rather than -/// allowed to win, which is the rule [`super::write`]'s `state_of` applies to +/// allowed to win, which is the rule [`crate::cmd::pr::write`]'s `state_of` applies to /// a whole collection at once — the two must agree, or `pr list` and /// `pr close` would read the same record differently. [`PullState::label`] /// also spells the three states exactly as Bobbin renders them, so a merged @@ -798,7 +678,7 @@ fn merge( /// A pull the PDS holds that Bobbin provably failed to return. #[derive(Debug, PartialEq, Eq)] -struct Missing { +pub(super) struct Missing { uri: String, created_at: String, } @@ -838,7 +718,7 @@ fn missing_from_bobbin(items: &[Listed]) -> Vec { /// What a listing is a listing *of*, which decides what its incompleteness /// means. #[derive(Clone, Copy, PartialEq, Eq, Debug)] -enum Scope { +pub(super) enum Scope { /// One repo, every author. Bobbin lagging means pulls are missing. Repo, /// One author, every repo. The PDS answers this completely on its own, so @@ -856,7 +736,7 @@ enum Scope { /// `backfilled` is how many other authors' pulls [`crate::clients::tangled::web::backfill`] managed /// to scrape back in. It changes what the incompleteness *means* — the gap is /// patched rather than open — so it changes the sentence. -fn warn_stale(missing: &[Missing], pds_total: usize, scope: Scope, backfilled: usize) { +pub(super) fn warn_stale(missing: &[Missing], pds_total: usize, scope: Scope, backfilled: usize) { let Some(newest) = missing.iter().max_by_key(|m| m.created_at.as_str()) else { return; }; @@ -910,7 +790,7 @@ fn warn_stale(missing: &[Missing], pds_total: usize, scope: Scope, backfilled: u /// merged` can show something that turns out to be open, so it is only /// defensible next to the note the caller prints — the count comes back for /// exactly that reason. -fn apply_state_filter(items: Vec, filter: Option<&str>) -> (Vec, usize) { +pub(super) fn apply_state_filter(items: Vec, filter: Option<&str>) -> (Vec, usize) { let Some(filter) = filter else { return (items, 0); }; @@ -940,7 +820,7 @@ fn apply_state_filter(items: Vec, filter: Option<&str>) -> (Vec, /// Only fires when no index was consulted, and only when there was a PDS to /// read — with no account selected the listing has a different problem and /// the empty-case message says so. -fn note_author_scoped(source: Source, had_account: bool) { +pub(super) fn note_author_scoped(source: Source, had_account: bool) { if source.spans_authors() || !had_account { return; } @@ -954,7 +834,7 @@ fn note_author_scoped(source: Source, had_account: bool) { } /// The footnote that keeps `--state` honest. -fn note_unknown_states(unknown: usize) { +pub(super) fn note_unknown_states(unknown: usize) { if unknown == 0 { return; } @@ -972,15 +852,15 @@ fn note_unknown_states(unknown: usize) { /// Everything a listing command needs, plus the evidence about how complete /// it is. Kept together because the two are only meaningful together: the /// items without the evidence is the old behaviour that lied by omission. -struct Gathered { - items: Vec, - missing: Vec, +pub(super) struct Gathered { + pub(super) items: Vec, + pub(super) missing: Vec, /// How many pull records the PDS side actually held, before merging. - pds_total: usize, + pub(super) pds_total: usize, /// The PDS walk hit its page cap with more to read. - truncated: bool, + pub(super) truncated: bool, /// How many pulls the [`crate::clients::tangled::web::backfill`] workaround scraped back in. - backfilled: usize, + pub(super) backfilled: usize, } // `states_complete` and `own_repo` used to ride along here so that @@ -996,24 +876,24 @@ struct Gathered { /// or `Option<&str>` in a row — `endpoint`, `subject`, `repo_filter` — and a /// call site that reads `"listPulls", &repo.did, Some(&repo.did), 100` gives /// no hint which is which. -struct Ask<'a> { - source: Source, - scope: Scope, +pub(super) struct Ask<'a> { + pub(super) source: Source, + pub(super) scope: Scope, /// Whose PDS to read, when there is one worth reading. - did: Option<&'a str>, + pub(super) did: Option<&'a str>, /// The Bobbin method: `listPulls` for a repo, `listPullsBy` for an author. - endpoint: &'a str, + pub(super) endpoint: &'a str, /// What that method is asked about — a repo DID or an account DID. - subject: &'a str, + pub(super) subject: &'a str, /// Keep only pull records aimed at this repo. `None` lists them all, /// which is what an author-scoped listing wants. - repo_filter: Option<&'a str>, + pub(super) repo_filter: Option<&'a str>, /// How many items to ask Bobbin for. - limit: u32, + pub(super) limit: u32, /// How much of the PDS collection to walk. Separate from `limit` because /// they were one number, which is how a caller that needed the whole /// collection came to ask for a screenful of it — see [`Reach`]. - reach: Reach, + pub(super) reach: Reach, } /// Read from whichever sources are in play and merge them. @@ -1027,7 +907,7 @@ struct Ask<'a> { /// asymmetry is the point of the change: an appview outage used to take every /// read command down with it, and now takes down only the part of the answer /// that genuinely depends on it. -async fn gather(ask: Ask<'_>) -> Result { +pub(super) async fn gather(ask: Ask<'_>) -> Result { let Ask { source, scope, @@ -1285,7 +1165,7 @@ fn append_backfilled(items: &mut Vec, extra: Vec) { /// because you are not logged in would be a regression — before this change /// the command needed no account at all. So a failure here quietly costs the /// PDS half and leaves Bobbin's answer, which is what you would have got. -async fn own_did() -> Option { +pub(super) async fn own_did() -> Option { match crate::config::account::select().await { Ok(selection) => Some(selection.did), Err(e) => { @@ -1350,1258 +1230,14 @@ pub(in crate::cmd) async fn repo_rows(source: Source, repo_did: &str) -> Result< .collect(); Ok(RepoRows { rows, truncated }) } - -// --------------------------------------------------------------------------- -// `--json` -// --------------------------------------------------------------------------- -// -// `pr list` and `pr view` are the two read commands an agent workflow leans -// on hardest — `pr list` to see what is there, `pr view` to act on one — so -// they are the pair that got `--json`, ahead of the wide listing, `stack view`, -// `repo view` and `repo list`, which stay text-only for now (see TODO.md). -// -// Both emit the *derived* view: the same state, round count and resolved -// author handle the human table or detail already compute, not the raw -// `sh.tangled.repo.pull` record. The record is deliberately not the answer -// here, for a reason specific to this lexicon rather than a general -// preference: a pull's state and appview number are not fields on the -// record at all (see docs/output.md, "What the record does and does -// not say") — they are exactly the two things `pr list` exists to resolve, -// so a `--json` that printed the record would omit the entire reason to run -// the command and hand the caller the same two-source merge this file -// exists to do, unauthenticated calls and all. A caller who wants the raw -// bytes anyway can already fetch them, unauthenticated, with -// `com.atproto.repo.getRecord` off the `uri` this prints. -// -// This diverges from `logs oauth --json` on purpose. That command re-emits the -// exact bytes it read (`entry.raw`) because the log *is* the record — a -// line of it already is the answer, and re-serializing would risk disagreeing -// with what `jq` was already parsing. Here the record is raw input to a -// computation the record itself cannot answer, so printing it back would not -// be honest in the same way; it would just be incomplete. -// -// Both builders below are pure functions of data their caller has already -// resolved (no I/O, no formatting decisions left to make), which is what -// lets a test pin their shape against a fixture record with no network and -// no mock server — see the tests at the end of this module. -// -// Stability: this project makes none of the guarantees a 1.0 CLI would (see -// README.md, "no stability guarantees"). Within that, these two shapes -// follow the same rule every other flag does — CONTRIBUTING.md's versioning -// section — a field can be added in a `feat`, but removing or renaming one -// is a breaking change and needs a `!`. Nothing here is guaranteed never to -// change; it is guaranteed to say so with a version bump when it does. - -/// `None` for the `"?"` [`State::Unknown`] renders as, `Some` otherwise. -/// -/// Shared by every builder in the tool — [`crate::cmd::stack::read::view`]'s included — -/// so "unknown" has exactly one spelling in the output. The alternative — printing the literal string `"?"` a script -/// would have to special-case — is what the human column does and is a -/// display convention, not a value; JSON already has a way to say "we do -/// not know this", and using it is the whole point of typing the output. -pub(in crate::cmd) fn known_state(label: &str) -> Option { - (label != "?").then(|| label.to_string()) -} - -/// One row of `pr list --json`. -#[derive(serde::Serialize, Debug, PartialEq)] -pub(crate) struct PullRowJson { - pub uri: String, - /// The appview's `/pulls/` number, when [`crate::clients::tangled::web::pulls::numbers_for_records`] - /// could place this row. `null` rather than an empty column. - pub number: Option, - pub state: Option, - pub title: String, - pub author_did: Option, - /// Without the leading `@` the terminal column prints — a caller wanting - /// a mention back can add it; wanting the bare handle (to build a URL, to - /// pass to `--author`) is at least as common and this way needs no - /// stripping. - pub author_handle: Option, - /// RFC 3339, straight from the record's `createdAt`. `null` for the rare - /// pre-rounds record that carries `""` here — an empty string is not a - /// timestamp, and a script parsing this field should not have to - /// special-case one record era to find that out. - pub created_at: Option, - pub rounds: usize, - pub comments: u64, -} - -/// Build one [`PullRowJson`] from data `list` has already resolved. -/// -/// `envelope` is a merged item's `{"uri", "value"}` shape (a [`Listed::value`] -/// or the record wrapped the same way `pds_pulls` returns it) — the same -/// value the text table reads every field of this from. `state` is -/// [`State::label`]'s output, `number` is what the appview-number sweep -/// resolved for this row if anything, and `author_handle` is the same -/// lookup the text column prints, without its `@`. -pub(super) fn pull_row_json( - envelope: &serde_json::Value, - state: &str, - number: Option, - author_handle: Option<&str>, - comments: u64, -) -> PullRowJson { - let v = &envelope["value"]; - PullRowJson { - uri: envelope["uri"].as_str().unwrap_or_default().to_string(), - number, - state: known_state(state), - title: v["title"].as_str().unwrap_or("(untitled)").to_string(), - author_did: author_did(envelope), - author_handle: author_handle.map(str::to_string), - created_at: v["createdAt"] - .as_str() - .filter(|s| !s.is_empty()) - .map(str::to_string), - rounds: round_count(v), - comments, - } -} - -/// One row of `status pr --json`: a `pr list` row plus the repo it targets. -/// -/// Flattened rather than nested so the two listings really do print the same -/// fields for the same pull — a caller that reads `pr list --json` and then -/// widens to every repo should not have to move `.state` to `.pull.state` -/// on the way. What `status pr` adds is the column its table adds, and -/// nothing else. -#[derive(serde::Serialize, Debug, PartialEq)] -pub(crate) struct StatusRowJson { - #[serde(flatten)] - pub pull: PullRowJson, - /// The repo the pull targets. Present whenever the record names one at - /// all, resolved or not — it is the identifier both lookups below start - /// from, and the one a caller can act on. - pub repo_did: Option, - /// `owner/name`, without the `@` the table's column prints, and `null` - /// when neither Bobbin nor the appview could name the repo. The text - /// column falls back to a truncated DID there; that string has an - /// ellipsis in it and names nothing fetchable, so it is not a value. - /// `repo_did` still carries the identity, exactly as `author_did` - /// survives an unresolved `author_handle`. - pub repo: Option, - pub repo_url: Option, -} - -/// Build one [`StatusRowJson`] from data `status` has already resolved. -/// -/// `repo` is the same [`RepoName`] the text column reads, `None` for a -/// record naming no target repo at all. Pure, like the two builders above. -fn status_row_json( - envelope: &serde_json::Value, - state: &str, - number: Option, - author_handle: Option<&str>, - comments: u64, - repo_did: Option, - repo: Option<&RepoName>, -) -> StatusRowJson { - // `web_url` is `Some` exactly when the label came from a lookup rather - // than from the truncated-DID fallback — the invariant `RepoName`'s own - // doc states — so it is what decides whether there is a name here. - let named = repo.filter(|name| name.web_url.is_some()); - StatusRowJson { - pull: pull_row_json(envelope, state, number, author_handle, comments), - repo_did, - repo: named.map(|name| name.label.trim_start_matches('@').to_string()), - repo_url: named.and_then(|name| name.web_url.clone()), - } -} - -/// One round of `pr view --json`'s `rounds` array. -#[derive(serde::Serialize, Debug, PartialEq)] -pub(crate) struct RoundJson { - /// One-based, matching every `--round` flag `pr diff`, `pr checkout` and - /// `pr comment` take — not Tangled's own zero-based `/round/` URLs. - /// See docs/output.md, "Comments attach to a round". - pub index: usize, - pub created_at: Option, - pub patch_bytes: Option, -} - -/// One member of `pr view --json`'s `stack.members`, when the pull is -/// stacked. -#[derive(serde::Serialize, Debug, PartialEq)] -pub(crate) struct StackMemberJson { - /// 1-based, counted from the bottom of the stack — the same numbering - /// `atgc stack view` prints, even though both that command and this - /// array list members top first. - pub position: usize, - /// Not printed by the human view, which shows a member's state and title - /// only. Included here because an agent's next move on a stack member is - /// almost always to act on it by URI (`pr checkout`, `pr view `), - /// and the identifier is already in hand — leaving it out would just - /// mean a second command to go and scrape it back out. - pub uri: String, - pub state: Option, - pub title: String, -} - -/// `pr view --json`'s `stack` field, present exactly when the pull belongs -/// to one. -#[derive(serde::Serialize, Debug, PartialEq)] -pub(crate) struct StackJson { - pub total: usize, - /// Which member the surrounding object is describing, 1-based from the - /// bottom — `total` when it is the top. Naming a member now details that - /// member, so a consumer cannot assume the top the way it once could; - /// `null` only if the chain walk and the detail disagree. - pub position: Option, - /// Top first, matching the order the human view prints them in — and - /// including the pull this whole object is about, at its own position, - /// the same way the text loop does. - pub members: Vec, - /// A `dependentOn` link the listing used to build this view did not - /// contain. See [`crate::cmd::stack::Chain::missing_below`]. - pub missing_below: Option, -} - -/// `pr view --json`'s whole object. -#[derive(serde::Serialize, Debug, PartialEq)] -pub(crate) struct PullDetailJson { - pub uri: String, - pub state: Option, - pub title: String, - pub author_did: Option, - pub author_handle: Option, - pub created_at: Option, - pub comments: u64, - pub rounds: Vec, - /// `null` for an empty or whitespace-only body, matching the text view's - /// own "nothing worth printing" test. - pub body: Option, - /// The confirmed `/pulls/` page, and `null` when the appview has no - /// number for this pull yet — [`crate::clients::tangled::web::pulls::PullLink::numbered_url`]. - /// Never the repo's `/pulls` listing, which the text view prints with a - /// note saying it is not the pull; there is nowhere to put that note - /// here, so the field says nothing instead of saying it wrongly. - pub url: Option, - pub stack: Option, - /// The discussion, oldest first, or absent when it was not asked for. - /// - /// Distinct from `comments` above, which is the *count* the listing - /// envelope carries and is there whether or not anybody asked for the - /// bodies. `issue view --json` spells this field the same way. - #[serde(skip_serializing_if = "Option::is_none")] - pub thread: Option>, -} - -/// Build a [`PullDetailJson`] from data `view` has already resolved. -/// -/// `item` is the top pull's `{"uri", "value"}` shape, `state` is -/// [`State::label`]'s output for it, `url` is the already-resolved `view:` -/// link when it names this pull and `None` when it does not, and `stack` is -/// `None` for a pull that stands alone. A pure function of already-resolved -/// inputs, same as [`pull_row_json`]. -pub(super) fn pull_detail_json( - item: &serde_json::Value, - state: &str, - author_handle: Option<&str>, - comments: u64, - url: Option<&str>, - stack: Option, - thread: Option>, -) -> PullDetailJson { - let v = &item["value"]; - let rounds = match v["rounds"].as_array() { - Some(rounds) => rounds - .iter() - .enumerate() - .map(|(i, round)| RoundJson { - index: i + 1, - created_at: round["createdAt"] - .as_str() - .filter(|s| !s.is_empty()) - .map(str::to_string), - patch_bytes: round["patchBlob"]["size"].as_u64(), - }) - .collect(), - // Old records: a single inline patch, no rounds array — the text - // view's own fallback, mirrored. - None => vec![RoundJson { - index: 1, - created_at: v["createdAt"] - .as_str() - .filter(|s| !s.is_empty()) - .map(str::to_string), - patch_bytes: v["patch"].as_str().map(|s| s.len() as u64), - }], - }; - PullDetailJson { - uri: item["uri"].as_str().unwrap_or_default().to_string(), - state: known_state(state), - title: v["title"].as_str().unwrap_or("(untitled)").to_string(), - author_did: author_did(item), - author_handle: author_handle.map(str::to_string), - created_at: v["createdAt"] - .as_str() - .filter(|s| !s.is_empty()) - .map(str::to_string), - comments, - rounds, - body: v["body"] - .as_str() - .map(str::trim) - .filter(|s| !s.is_empty()) - .map(str::to_string), - url: url.map(str::to_string), - stack, - thread, - } -} - -/// This repo's pull requests, or — with `--all` — one account's across every -/// repo. -/// -/// One verb and a scope flag, rather than two verbs. The two listings print -/// nearly the same columns and differ in exactly one thing, which is which -/// pulls are in them; when that was carried by the choice between the words -/// `list` and `status`, it was carried by nothing a reader could see. A flag -/// named for the scope sits among the other filters, where the rest of the -/// question is already being asked. -/// -/// The asymmetry underneath is real and is why the flag is not simply "more -/// rows": this repo's pulls are gathered by repo and an account's are gathered -/// by author, because a pull record lives in its author's PDS whatever it -/// targets. See [`Scope`]. -pub(super) async fn list(args: ListArgs) -> Result<()> { - crate::term::jsonout::init(args.json); - match args.all { - true => across_repos(args).await, - false => in_this_repo(args).await, - } -} - -async fn in_this_repo(args: ListArgs) -> Result<()> { - let source = Source::parse_arg(args.source.as_deref())?; - let remote_url = git::remote_url(&args.remote)?; - let repo = resolve::repo_ref(&remote_url).await?; - let me = match source.uses_pds() { - true => own_did().await, - false => None, - }; - - let gathered = gather(Ask { - source, - scope: Scope::Repo, - did: me.as_deref(), - endpoint: "listPulls", - subject: &repo.did, - repo_filter: Some(&repo.did), - limit: args.limit, - reach: Reach::Screen(args.limit as usize), - }) - .await?; - - // Before the listing rather than after it: the warning describes what - // follows, and it is also the one part worth reading when the filter - // empties the list and the early return below fires. - warn_stale( - &gathered.missing, - gathered.pds_total, - Scope::Repo, - gathered.backfilled, - ); - - let filter = args.state.label(); - let unfiltered = gathered.items.len(); - let (mut items, unknown) = apply_state_filter(gathered.items, filter); - items.truncate(args.limit as usize); - - if items.is_empty() { - // `[]` on stdout either way — a script should never have to tell - // "no pulls" apart from "the process died before printing anything". - // The reason still goes out, on stderr rather than the stdout the - // prose version uses, so it is not lost, only moved off the stream - // something downstream is about to parse. - if args.json { - crate::term::say::note!( - Index, - "no pull requests to list ({})", - match classify_empty(filter, unfiltered) { - Empty::NotIndexed => - "none found; run without --json for why that might be index lag", - Empty::NoneInState => "none in the requested state", - } - ); - println!("[]"); - return Ok(()); - } - match classify_empty(filter, unfiltered) { - Empty::NotIndexed => { - // Three different claims, and printing the strongest one - // where only the weakest is earned is the whole bug this - // change is about. Without an index in the source set - // nothing here has looked at anybody else's pulls, so the - // honest sentence is about *you*, not about the repo. - match (me.is_some(), source.spans_authors()) { - (true, false) => { - println!("you have no pull requests on {}", repo.did); - println!( - "Read from your PDS alone, so this says nothing about anyone \ - else's. Add an index to look: --source bobbin (alpha, and \ - its ingest stalls) or --source web (tangled.org's own, scraped)." - ); - } - // Every source asked came back empty, and one of them - // cannot be behind. As close to "this repo has no - // pulls" as this tool gets. - (true, true) => { - println!("no pull requests found for {}", repo.did); - println!( - "Your PDS has none for it either, so this is probably right \ - rather than index lag." - ); - } - (false, _) => { - println!("no pull requests found for {}", repo.did); - println!( - "No account is selected, so your own PDS was not read, and an \ - index can be behind. Nothing here has ruled out there being some." - ); - } - } - } - Empty::NoneInState => println!("no {} pull requests", args.state), - } - println!( - "view: {}", - crate::term::hyperlink::url(&format!("{}/pulls", repo.web_url)) - ); - return Ok(()); - } - - note_author_scoped(source, me.is_some()); - - // One lookup per unique author DID, run together rather than in turn. - let handles = author_handles(&items).await; - - // Numbers are the appview's and are in no record, so they are a separate - // errand — one that reads three listing pages once for the whole page of - // rows rather than anything per row. A row it cannot place keeps the blank - // column it has always had. - let wanted: Vec<(&str, &str)> = items - .iter() - .map(|item| { - ( - item.uri.as_str(), - item.value["value"]["title"].as_str().unwrap_or_default(), - ) - }) - .collect(); - let numbers = - crate::clients::tangled::web::pulls::numbers_for_records(&repo.web_url, &wanted).await; - - // `--json` skips the whole table below: ellipsizing, column padding and - // hyperlink-wrapping are display decisions for a terminal, and every one - // of them would have to be undone by anything reading the output back — - // constraint 4 in the brief this shipped against says so explicitly, - // and this is that: no `ellipsize`, no `hyperlink::url`, no width math, - // unconditionally, not merely because stdout is a pipe. - if args.json { - let rows: Vec = items - .iter() - .map(|item| { - // `handles` falls back to the bare DID, unprefixed, when - // resolution failed — the text column is happy to print - // that in the author slot, but a JSON `author_handle` that - // silently held a DID would be indistinguishable from a - // real handle, so only a label that actually starts with - // `@` counts as one here. - let author_handle = author_did(&item.value) - .and_then(|d| handles.get(&d)) - .and_then(|h| h.strip_prefix('@')) - .map(str::to_string); - pull_row_json( - &item.value, - item.state.label(), - numbers.get(&item.uri).copied(), - author_handle.as_deref(), - item.comments, - ) - }) - .collect(); - crate::term::jsonout::emit(&rows)?; - if gathered.truncated { - crate::term::say::warning!( - Pds, - "stopped after {} pages of your pull records; older ones for this repo \ - may be missing", - crate::clients::atproto::pds::MAX_PAGES - ); - } - note_unknown_states(unknown); - return Ok(()); - } - - for item in &items { - let v = &item.value["value"]; - let state = item.state.label(); - let number = match numbers.get(&item.uri) { - Some(number) => format!("#{number}"), - None => String::new(), - }; - let title = ellipsize(v["title"].as_str().unwrap_or("(untitled)"), 54); - let author = author_did(&item.value) - .and_then(|d| handles.get(&d).cloned()) - .unwrap_or_else(|| "?".to_string()); - let rounds = round_count(v); - let comments = item.comments; - let date = day(v["createdAt"].as_str().unwrap_or("")); - println!( - "{number:<5} {state:<7} {title:<56} {author:<24} {date:<11} \ - rounds:{rounds} comments:{comments}" - ); - } - println!("\nview: {}/pulls", repo.web_url); - if gathered.truncated { - crate::term::say::warning!( - Pds, - "stopped after {} pages of your pull records; older ones for this repo \ - may be missing", - crate::clients::atproto::pds::MAX_PAGES - ); - } - note_unknown_states(unknown); - Ok(()) -} - -/// One account's pull requests across every repo. -/// -/// The listing where dropping Bobbin is not a compromise at all. Every record -/// this half wants lives in one PDS — the subject's — so a single -/// `listRecords` answers it completely, and no index can be behind on it. -/// Bobbin is still asked, because it is the only thing that knows about -/// status records written by a maintainer who is not the author, and about -/// comment counts. But it is now an enrichment: when it is stale or down, -/// this still lists every pull, marking as `?` only the states it genuinely -/// cannot see. -/// -/// Needs no checkout and no session: reading somebody's pulls wants a DID and -/// nothing else, so this path stays available to an account whose session has -/// expired, and from a directory that is not a git repo at all. -async fn across_repos(args: ListArgs) -> Result<()> { - let source = Source::parse_arg(args.source.as_deref())?; - // Reading someone's pulls needs no token, only a DID — so this path - // stays available for an account whose session has expired. - let did = match &args.author { - Some(author) => crate::config::account::actor_did(author).await?, - None => crate::config::account::select().await?.did, - }; - - let gathered = gather(Ask { - source, - scope: Scope::Author, - did: Some(&did), - endpoint: "listPullsBy", - subject: &did, - repo_filter: None, - limit: args.limit, - reach: Reach::Screen(args.limit as usize), - }) - .await?; - - warn_stale( - &gathered.missing, - gathered.pds_total, - Scope::Author, - gathered.backfilled, - ); - - let filter = args.state.label(); - let unfiltered = gathered.items.len(); - let (mut items, unknown) = apply_state_filter(gathered.items, filter); - items.truncate(args.limit as usize); - - if items.is_empty() { - let why = match classify_empty(filter, unfiltered) { - Empty::NoneInState => format!("no {} pull requests", args.state), - Empty::NotIndexed => format!("no pull requests for {did}"), - }; - // `[]` on stdout and the reason on stderr, the same split `pr list` - // makes: an empty listing is a result, not an absence of one. - if args.json { - crate::term::say::note!(Index, "{why}"); - println!("[]"); - return Ok(()); - } - println!("{why}"); - return Ok(()); - } - - // One lookup per unique repo DID, run together rather than in turn. - let repos = repo_names(&items).await; - - let numbers = numbers_across_repos(&items, &repos).await; - - if args.json { - // The table has no author column — every row is the one account's — - // so this handle is resolved for the JSON alone, once for the page. - // It is what keeps the row shape genuinely the same as `pr list - // --json`'s rather than the same minus a field. - let subject_handle = crate::clients::atproto::handles::handle(&did).await; - let rows: Vec = items - .iter() - .map(|item| { - let repo_did = target_repo_did(&item.value); - // Only the subject's own handle is in hand. A row filed by - // anyone else — which a Bobbin-sourced listing can carry — - // reads `null` rather than borrowing this one. - let author_handle = subject_handle - .as_deref() - .filter(|_| author_did(&item.value).as_deref() == Some(did.as_str())); - status_row_json( - &item.value, - item.state.label(), - numbers.get(&item.uri).copied(), - author_handle, - item.comments, - repo_did.clone(), - repo_did.as_deref().and_then(|d| repos.get(d)), - ) - }) - .collect(); - crate::term::jsonout::emit(&rows)?; - note_unknown_states(unknown); - return Ok(()); - } - - for item in &items { - let v = &item.value["value"]; - let state = item.state.label(); - let number = match numbers.get(&item.uri) { - Some(number) => format!("#{number}"), - None => String::new(), - }; - let title = ellipsize(v["title"].as_str().unwrap_or("(untitled)"), 54); - let repo = target_repo_did(&item.value) - .and_then(|d| repos.get(&d).map(|name| name.label.clone())) - .unwrap_or_else(|| "?".to_string()); - let rounds = round_count(v); - let comments = item.comments; - let date = day(v["createdAt"].as_str().unwrap_or("")); - println!( - "{number:<5} {state:<7} {title:<56} {repo:<28} {date:<11} \ - rounds:{rounds} comments:{comments}" - ); - } - note_unknown_states(unknown); - Ok(()) -} - -/// How many identity lookups a listing may have in flight at once. -/// -/// The one number for both of a listing's label errands, and it is the -/// resolver's: the columns a page needs are per *unique* DID rather than per -/// row, and a listing is not entitled to open as many sockets as it happens -/// to have rows. See [`crate::clients::atproto::handles::CONCURRENCY`] for -/// the rest of the argument. -const LABEL_CONCURRENCY: usize = crate::clients::atproto::handles::CONCURRENCY; - -/// `@handle` per unique author DID on the page, falling back to the DID. -/// -/// The resolution is [`crate::clients::atproto::handles`], which every -/// listing shares and which remembers a DID for the length of the process; -/// what stays here is this table's own answer to a handle that will not -/// resolve, which is to print the DID rather than leave the column empty. -async fn author_handles(items: &[Listed]) -> HashMap { - let dids = unique(items.iter().filter_map(|i| author_did(&i.value))); - let handles = crate::clients::atproto::handles::handles(dids.clone()).await; - dids.into_iter() - .map(|did| { - let label = match handles.get(&did) { - Some(h) => format!("@{h}"), - None => did.clone(), - }; - (did, label) - }) - .collect() -} - -/// [`RepoName`] per unique target repo DID on the page. -/// -/// The expensive half of the two: [`repo_name`] is up to three round trips of -/// its own (Bobbin, then the appview redirect, then the owner's handle), so a -/// page of `pr list --all` spanning ten repos was thirty serial requests -/// before the numbering sweep had started. -async fn repo_names(items: &[Listed]) -> HashMap { - let dids = unique(items.iter().filter_map(|i| target_repo_did(&i.value))); - crate::logging::debug::log(format!( - "listing labels: {} repo name(s) for {} row(s), {LABEL_CONCURRENCY} at a time", - dids.len(), - items.len() - )); - stream::iter( - dids.into_iter() - .map(|did| async move { (did.clone(), repo_name(&did).await) }), - ) - .buffer_unordered(LABEL_CONCURRENCY) - .collect() - .await -} - -/// The distinct values of `keys`, in the order they first appear. -/// -/// Deduplication is the whole point: the loops this replaces skipped a DID -/// they had already resolved, and dropping that would turn one lookup per -/// author into one per row. Order is kept because it costs nothing and makes -/// the debug log read in page order. -fn unique(keys: impl Iterator) -> Vec { - let mut seen = std::collections::HashSet::new(); - keys.filter(|k| seen.insert(k.clone())).collect() -} - -/// The target repo's DID: value.target.repo in new records, or the at-uri -/// authority in old records' targetRepo (the repo owner's DID back then — -/// close enough for display). -pub(super) fn target_repo_did(item: &serde_json::Value) -> Option { - let v = &item["value"]; - if let Some(did) = v["target"]["repo"].as_str() { - return Some(did.to_string()); - } - v["targetRepo"] - .as_str()? - .strip_prefix("at://")? - .split('/') - .next() - .map(String::from) -} - -/// The appview's `#` for every row it can place, across every repo on the -/// page. -/// -/// `pr list` numbers one repo's rows from one sweep of that repo's pull -/// listings. `status pr` spans repos, so it is that same sweep once per repo — -/// and the reason it did not have a number column, since a sweep costs about a -/// quarter of a megabyte and doing several in turn is a wait nobody asked for. -/// -/// So they go together. The repos on a page of `status pr` are independent -/// appview reads, and running them concurrently makes the column cost one -/// round trip rather than one per repo. What it cannot make cheaper is the -/// bytes, which is why this is per repo *on the page* and not per repo the -/// account has ever filed against. -/// -/// A repo whose label could not be resolved has no URL to read, and its rows -/// keep the blank column. So do the rows of a repo whose listings cannot be -/// reached: this decorates a listing that is already correct without it. -async fn numbers_across_repos( - items: &[Listed], - repos: &HashMap, -) -> HashMap { - let mut sweeps = tokio::task::JoinSet::new(); - for (repo_did, rows) in rows_by_repo(items) { - let Some(web_url) = repos.get(&repo_did).and_then(|name| name.web_url.clone()) else { - crate::logging::debug::log(format!( - "pull numbers: no appview URL for {repo_did}, leaving {} row(s) blank", - rows.len() - )); - continue; - }; - sweeps.spawn(async move { - let rows: Vec<(&str, &str)> = rows - .iter() - .map(|(uri, title)| (uri.as_str(), title.as_str())) - .collect(); - crate::clients::tangled::web::pulls::numbers_for_records(&web_url, &rows).await - }); - } - - let mut numbers = HashMap::new(); - while let Some(swept) = sweeps.join_next().await { - if let Ok(found) = swept { - numbers.extend(found); - } - } - numbers -} - -/// The page's rows grouped by the repo whose listings can number them. -/// -/// A number is only unique inside a repo, so the grouping *is* the question: -/// two rows from two repos may both be #12 and neither is evidence about the -/// other. A row naming no target repo is dropped rather than swept against -/// somebody's guess at one. -fn rows_by_repo(items: &[Listed]) -> HashMap> { - let mut by_repo: HashMap> = HashMap::new(); - for item in items { - let Some(repo_did) = target_repo_did(&item.value) else { - continue; - }; - let title = item.value["value"]["title"].as_str().unwrap_or_default(); - by_repo - .entry(repo_did) - .or_default() - .push((item.uri.clone(), title.to_string())); - } - by_repo -} - -/// The appview `status pr` builds repo URLs against. -/// -/// A repo DID resolves to a knot and a PDS, and neither says which web front -/// end is showing it; Tangled's is the one atgc knows how to read pull -/// numbers off. The address itself, and its override, are -/// [`crate::clients::endpoints`]'s. -fn appview() -> String { - crate::clients::endpoints::appview() -} - -/// What `status pr` knows about one repo: what to print for it, and where its -/// pages are. -/// -/// The two travel together because they come from the same lookup. Both -/// sources answer with an owner and a name — Bobbin as an at-uri, the appview -/// as a redirect target — and `@owner/name` and -/// `https://tangled.org/owner/name` are that same pair, punctuated for a -/// person and for a URL. Deriving the second from the first is why numbering -/// `status pr` costs no lookup it was not already making. -struct RepoName { - /// `@owner/name`, or a truncated DID when neither source could say. - label: String, - /// The repo's appview root, absent exactly when the label is that DID: - /// nothing is known to build a URL out of, so nothing is guessed. - web_url: Option, -} - -/// "owner-handle/name" for a repo DID, best-effort, falling back to a -/// truncated DID. -/// -/// Two lookups, because the first one inherits the lag `status pr` is built -/// around. Bobbin's `getRepoByRepoDid` is asked first and answers with an -/// at-uri that resolves to a handle. When it does not know the repo — which is -/// common here, since a PDS-sourced listing surfaces pulls against repos -/// Bobbin has not indexed — the web appview is asked instead, and it runs a -/// *different* index that is routinely ahead. Without the second lookup this -/// would trade a missing row for an unreadable one. -async fn repo_name(repo_did: &str) -> RepoName { - let named = match repo_label_from_bobbin(repo_did).await { - Some(label) => Some(label), - None => repo_label_from_appview(repo_did).await, - }; - match named { - Some(label) => RepoName { - web_url: Some(appview_url(&label)), - label, - }, - None => RepoName { - label: ellipsize(repo_did, 21), - web_url: None, - }, - } -} - -/// The appview root for a repo, from the label both lookups produce. -/// -/// `@permadeath.com/atgc` is `https://tangled.org/permadeath.com/atgc`: the -/// `@` is punctuation for a reader and is not in the path. Only ever called -/// with a resolved label, never with the truncated-DID fallback, which is a -/// display string with an ellipsis in it and not a repo anybody can fetch. -fn appview_url(label: &str) -> String { - format!( - "{}/{}", - appview(), - label.trim_start_matches('@').trim_end_matches('/') - ) -} - -async fn repo_label_from_bobbin(repo_did: &str) -> Option { - let uri = crate::clients::tangled::bobbin::repo_uri(repo_did).await?; - // Old records carry an *owner* DID in this position, which is not a repo - // DID and so resolves to nothing here. - let (owner, name) = owner_and_name(&uri)?; - let owner_label = match crate::clients::atproto::handles::handle(owner).await { - Some(h) => format!("@{h}"), - None => owner.to_string(), - }; - Some(format!("{owner_label}/{name}")) -} - -/// `tangled.org/` 302s to `tangled.org//`. -/// -/// The same redirect-probe trick [`crate::clients::tangled::resolve::repo_ref`] uses to go from -/// a remote URL to a repo DID, run backwards, and against the one index in -/// the system that is reliably current. It costs one request and is only -/// reached when Bobbin has already declined. -async fn repo_label_from_appview(repo_did: &str) -> Option { - let client = crate::clients::http::builder() - .redirect(reqwest::redirect::Policy::none()) - .build() - .ok()?; - let url = format!("{}/{repo_did}", appview()); - crate::logging::debug::log(format!(">> GET {url}")); - let resp = client.get(&url).send().await.ok()?; - let location = resp.headers().get(reqwest::header::LOCATION)?; - crate::logging::debug::log(format!("<< {} location: {location:?}", resp.status())); - label_from_location(location.to_str().ok()?) -} - -/// Read "owner/name" off a redirect target. -/// -/// The tail two segments are owner and name whether the `Location` is -/// absolute or relative, so both spellings are read the same way. Anything -/// else is declined rather than guessed at — in particular a redirect that -/// lands on another DID, which would render as a handle that is not one. -fn label_from_location(location: &str) -> Option { - let mut segments = location.trim_end_matches('/').rsplit('/'); - let name = segments.next()?; - let owner = segments.next()?; - if name.is_empty() || owner.is_empty() || owner.contains(':') || name.contains(':') { - return None; - } - Some(format!("@{owner}/{name}")) -} - -fn author_did(item: &serde_json::Value) -> Option { - // at://did:plc:xyz/sh.tangled.repo.pull/rkey - item["uri"] - .as_str()? - .strip_prefix("at://")? - .split('/') - .next() - .map(String::from) -} - -#[derive(clap::Args, Debug)] -pub(crate) struct ViewArgs { - /// The pull request: its number, record key, at:// URI, or Tangled URL - /// - /// Defaults to the one this branch is about. - #[arg(value_name = "PULL")] - pub pull: Option, - /// The same, as a flag, for symmetry with the other verbs - #[arg(long, value_name = "PULL", conflicts_with = "pull")] - pub pr: Option, - /// Whose pull request it is, when a bare record key is ambiguous - #[arg(long, value_name = "HANDLE|DID")] - pub author: Option, - /// Git remote pointing at the repo - #[arg(long, default_value = "origin")] - pub remote: String, - /// Also open the repo's pulls page in the browser - #[arg(long)] - pub web: bool, - /// Also show the discussion (needs the index: --source bobbin) - /// - /// A comment is a record in its commenter's PDS and nothing enumerates - /// the people who have commented, so a thread can only come from the - /// appview's index, whose ingest stalls. - #[arg(long)] - pub comments: bool, - /// Where to read from: pds (default), bobbin, web, auto for all three, - /// or a comma-separated combination. Both indexes are opt-in; - /// ATGC_USE_BOBBIN=1 and ATGC_USE_WEB=1 add one to the default - #[arg(long)] - pub source: Option, - /// Print a JSON object instead of the human view; colour, hyperlinks - /// and day-truncated dates are all off (see --help for the field - /// contract's stability note) - #[arg(long)] - pub json: bool, -} - -impl ViewArgs { - /// The pull this names, whichever spelling was used. - /// - /// The positional and `--pr` are declared `conflicts_with` each other, so - /// clap has already refused naming one pull twice; this only picks - /// whichever of the two carries it. - pub(crate) fn pull(&self) -> Option<&str> { - self.pull.as_deref().or(self.pr.as_deref()) - } -} - -/// A pull request's detail view: a named pull from anywhere, or the current -/// branch's — and only that one. -/// -/// The command most damaged by index lag, because the pull it wants is -/// almost always the one just opened — the least likely to be indexed and -/// the one whose author's PDS is certainly yours. Reading the PDS first -/// makes `pr view` work immediately after `pr create` instead of hours later. -/// -/// The match is `source.branch` and nothing else. It used to fall back to -/// your newest pull on the repo, which sounds like a courtesy and is not -/// one: run from a second worktree it answers with whatever you opened most -/// recently, on any branch, and says nothing about having guessed. A wrong -/// answer that looks right is worse than no answer, the more so because the -/// `uri:` line it prints is what a person then feeds to `pr close`. -pub(crate) async fn view(args: ViewArgs) -> Result<()> { - crate::term::jsonout::init(args.json); - let source = Source::parse_arg(args.source.as_deref())?; - // Resolved up front so that a bad reference fails before any listing is - // fetched. `pr view 23` is the spelling people try first, because every - // sibling command (`diff`, `checkout`, `merge`, `edit`, `comment`) - // accepts it; being the one refusal in the family taught nobody anything. - let named = match args.pull() { - Some(reference) => Some( - crate::cmd::pr::review::resolve_pull( - Some(reference), - args.author.as_deref(), - &args.remote, - ) - .await?, - ), - None => None, - }; - let remote_url = git::remote_url(&args.remote)?; - let repo = resolve::repo_ref(&remote_url).await?; - let gathered = gather_branch_pulls(source, &repo).await?; - let items: Vec<&serde_json::Value> = gathered.items.iter().map(|i| &i.value).collect(); - - // A named pull is looked up in the listing so its state and chain can - // be shown; one the index has not caught up with is shown from the - // record alone, because the record in hand outranks the index that - // lacks it. Only the no-argument form reads the checked-out branch — - // naming a pull works from a detached HEAD. - let unlisted: serde_json::Value; - let item: &serde_json::Value = match &named { - Some(pull) => match items - .iter() - .copied() - .find(|i| i["uri"].as_str() == Some(pull.uri.as_str())) - { - Some(item) => item, - None => { - crate::term::say::note!( - Index, - "this pull is not in the repo listing yet (index lag, or another \ - repo's pull): showing the record by itself" - ); - unlisted = serde_json::json!({ "uri": pull.uri, "value": pull.value }); - &unlisted - } - }, - None => { - let branch = git::current_branch()?; - require_branch_pull(&gathered.items, &branch)? - } - }; - // A stacked branch matches every member of its stack — they all share - // `source.branch` — so the newest match is only a way into the chain. - // Before this, `pr view` on a stacked branch showed whichever member was - // created last, with nothing to say others existed: a wrong answer in - // the shape of a right one. With a chain, the top pull is the detail and - // the chain is printed below it. - // A chain that cannot be ordered — a fork or a loop in somebody's - // records, which the appview refuses to ingest but a PDS happily holds — - // must degrade this view, not destroy it. `stack view` is the command - // that refuses loudly; `pr view` says what it saw and shows the newest - // match flat, because "one damaged record on the branch" and "no answer - // about your pull at all" are very different sizes of problem. - let chain = - match crate::cmd::stack::chain_containing(&items, item["uri"].as_str().unwrap_or_default()) - { - Ok(chain) => chain, - Err(e) => { - crate::term::say::warning!( - Pds, - "this branch's pulls do not form an orderable stack: {e}\n\ - showing the newest match for the branch by itself" - ); - None - } - }; - // Only the branch match is promoted to the top. A *named* pull is not a - // way into the chain — it is the answer, and substituting the top for it - // was this command's own "wrong answer in the shape of a right one": - // every member of a stack reported the top's title, body, rounds and - // state under the number, rkey, at:// URI or URL of the member asked - // for, so two different members read back byte-identical. Nothing said - // a swap had happened; in `--json` only `url` disagreed with the - // question. The chain is still built either way — it is what the - // `stack:` block below is drawn from. - let item = detail_item(named.is_some(), chain.as_ref(), item); - // Where the pull being detailed sits in its chain, 1-based from the - // bottom, so both views can say which member this is rather than - // asserting it is the top. - let position = chain.as_ref().and_then(|chain| { - chain - .members - .iter() - .position(|m| m["uri"] == item["uri"]) - .map(|i| i + 1) - }); - let found = gathered - .items - .iter() - .find(|i| i.uri.as_str() == item["uri"].as_str().unwrap_or_default()); - - let v = &item["value"]; - let title = v["title"].as_str().unwrap_or("(untitled)"); - // One pull, one unresolved state: exactly the case the targeted web - // confirm exists for. Auto only — `--source pds` asked for no third - // parties and `--source bobbin` wants Bobbin's unpatched answer — and - // only after every record-borne source has come up empty. A miss keeps - // the `?`: the scrape is a witness, not a fourth source of truth. - let mut state = found.map_or("?", |i| i.state.label()).to_string(); - if state == "?" - && source.uses_web() - && let Some(confirmed) = crate::clients::tangled::web::backfill::state_of( - &repo.web_url, - item["uri"].as_str().unwrap_or_default(), - title, - ) - .await - { - state = confirmed.to_string(); - } - // Resolved once and shared by both the human and `--json` paths below, - // rather than the human column's `format!("@{h}")` baked in early: JSON - // wants the DID and the bare handle as two separate, unambiguous fields - // instead of one string that means different things depending on - // whether the lookup succeeded. - let author_handle = match author_did(item) { - Some(did) => crate::clients::atproto::handles::handle(&did).await, - None => None, - }; - let author = match (author_did(item), &author_handle) { - (Some(_), Some(h)) => format!("@{h}"), - (Some(did), None) => did, - (None, _) => "?".to_string(), - }; - let created = day(v["createdAt"].as_str().unwrap_or("")); - let comments = item["commentCount"].as_u64().unwrap_or(0); - let uri = item["uri"].as_str().unwrap_or("?"); - let thread = crate::cmd::read_thread(args.comments, source, uri).await; - // The number is the appview's and is in no record, so this is a lookup - // rather than a format — and one that falls back to the repo's listing, - // which is what the line said before it could ever be more specific. The - // fallback says so now: a `view:` line that quietly widened to the whole - // repo is the one link a reader cannot tell has widened. Resolved before - // either rendering, since `--json` needs it too. - let link = crate::clients::tangled::web::pulls::pull_url(&repo.web_url, uri, title).await; - - if args.json { - let stack = chain.as_ref().map(|chain| { - let total = chain.members.len(); - let members = chain - .members - .iter() - .enumerate() - .rev() - .map(|(i, member)| { - let member_uri = member["uri"].as_str().unwrap_or_default(); - let member_state = gathered - .items - .iter() - .find(|g| g.uri == member_uri) - .map_or("?", |g| g.state.label()); - StackMemberJson { - position: i + 1, - uri: member_uri.to_string(), - state: known_state(member_state), - title: member["value"]["title"] - .as_str() - .unwrap_or("(untitled)") - .to_string(), - } - }) - .collect(); - StackJson { - total, - position, - members, - missing_below: chain.missing_below.clone(), - } - }); - let detail = pull_detail_json( - item, - &state, - author_handle.as_deref(), - comments, - link.numbered_url(), - stack, - crate::cmd::thread_json(&thread).await, - ); - crate::term::jsonout::emit(&detail)?; - // On stderr in both modes, and for the same reason: it is the only - // thing that says why `url` is null and why `--web` is about to open - // a page the pull is merely somewhere on. - if let Some(note) = link.note() { - crate::term::say::note!(Index, "{note}"); - } - if args.web { - crate::term::noinput::open_in_browser(link.url()); - } - return Ok(()); - } - - println!("{state} {title}"); - println!("author: {author}"); - println!("created: {created}"); - println!("comments: {comments}"); - match v["rounds"].as_array() { - Some(rounds) => { - println!("rounds: {}", rounds.len()); - for (i, round) in rounds.iter().enumerate() { - let date = day(round["createdAt"].as_str().unwrap_or("?")); - match round["patchBlob"]["size"].as_u64() { - Some(size) => println!(" {}: {date}, patch {size} bytes", i + 1), - None => println!(" {}: {date}", i + 1), - } - } - } - // Old records: a single inline patch, no rounds array. - None => { - println!("rounds: 1"); - let size = v["patch"].as_str().map(str::len).unwrap_or(0); - println!(" 1: {created}, patch {size} bytes"); - } - } - if let Some(chain) = &chain { - let total = chain.members.len(); - let which = match position { - Some(p) if p == total => "this is the top".to_string(), - Some(p) => format!("this is {p} of {total}"), - // The detail is in the chain by construction; a miss would mean - // the walk and the item disagreed, and silence would be a lie. - None => "this pull's place in it is unclear".to_string(), - }; - println!("stack: {total} pulls; {which}: `atgc stack view` for the chain"); - for (i, member) in chain.members.iter().enumerate().rev() { - let member_uri = member["uri"].as_str().unwrap_or("?"); - let state = gathered - .items - .iter() - .find(|g| g.uri == member_uri) - .map_or("?", |g| g.state.label()); - // Two spaces or an arrow, so the rows stay in one column and the - // one being detailed is findable without counting. - let mark = match position == Some(i + 1) { - true => "> ", - false => " ", - }; - println!( - "{mark}{}/{total} {state:<7} {}", - i + 1, - member["value"]["title"].as_str().unwrap_or("(untitled)"), - ); - } - if let Some(missing) = &chain.missing_below { - println!(" ...continues below {missing}, which this listing does not contain"); - } - } - if let Some(body) = v["body"].as_str().filter(|b| !b.trim().is_empty()) { - println!("\n{}", body.trim_end()); - } - println!(); - println!("uri: {uri}"); - println!("view: {}", crate::term::hyperlink::url(link.url())); - crate::cmd::print_thread(&thread).await; - if let Some(note) = link.note() { - crate::term::say::note!(Index, "{note}"); - } - if args.web { - crate::term::noinput::open_in_browser(link.url()); - } - Ok(()) -} - /// Every pull aimed at `repo`, gathered the way the branch-matching commands /// need them: the account's own PDS first when `source` allows it, because /// the pull a branch names is almost always the one just opened — the least /// likely to be indexed and the one whose author's PDS is certainly yours. -async fn gather_branch_pulls(source: Source, repo: &resolve::RepoRef) -> Result { +pub(super) async fn gather_branch_pulls( + source: Source, + repo: &resolve::RepoRef, +) -> Result { // Resolved before the fetch, because it decides whose PDS to read. A // failure to select is not fatal — these are read-only commands, and // Bobbin alone may well have the pull. @@ -2629,7 +1265,10 @@ async fn gather_branch_pulls(source: Source, repo: &resolve::RepoRef) -> Result< /// none: pulls with no source that cannot match any branch, or an index that /// has not caught up. Shared by `pr view` and `browse --pr`, so the two /// commands agree on the match and on the explanation when it misses. -fn require_branch_pull<'a>(items: &'a [Listed], branch: &str) -> Result<&'a serde_json::Value> { +pub(super) fn require_branch_pull<'a>( + items: &'a [Listed], + branch: &str, +) -> Result<&'a serde_json::Value> { let Some(item) = for_branch(items.iter().map(|i| &i.value), branch) else { // `NotFound`: the branch is a perfectly good identifier that resolved // to no pull. Not `Usage` — the branch name came from the checkout @@ -2720,12 +1359,12 @@ pub(in crate::cmd) async fn branch_pull(remote: &str) -> Result { /// /// A pull with no `source` matches no branch, which is the whole point: it /// is not evidence about this branch and must not be returned as if it -/// were. Kept apart from [`view`] so that "does not fall back" is a thing a +/// were. Kept apart from [`super::view`] so that "does not fall back" is a thing a /// test can hold on to. /// /// On a stacked branch every member of the stack matches, since they all /// share `source.branch`; the newest is then only a way *into* the chain, -/// which is why [`view`] and [`crate::cmd::stack::read::view`] both follow this with +/// which is why [`super::view`] and [`crate::cmd::stack::read::view`] both follow this with /// [`crate::cmd::stack::chain_containing`]. pub(in crate::cmd) fn for_branch<'a>( items: impl Iterator, @@ -2733,26 +1372,6 @@ pub(in crate::cmd) fn for_branch<'a>( ) -> Option<&'a serde_json::Value> { newest(items.filter(|i| i["value"]["source"]["branch"].as_str() == Some(branch))) } - -/// The pull [`view`] details: the top of the chain when the branch was the -/// question, and the pull itself when one was named. -/// -/// Kept apart from [`view`] for the same reason [`for_branch`] is — "naming -/// a pull answers about that pull" is a rule a test should be able to hold -/// still, and observing it through [`view`] would take a network. `named` -/// is only whether the caller gave a reference; which record it resolved to -/// is already `matched`. -pub(super) fn detail_item<'a>( - named: bool, - chain: Option<&crate::cmd::stack::Chain<'a>>, - matched: &'a serde_json::Value, -) -> &'a serde_json::Value { - match (chain, named) { - (Some(chain), false) => chain.top(), - _ => matched, - } -} - /// The newest of `items` by `createdAt`, parsed rather than compared as a /// string. /// @@ -2780,194 +1399,18 @@ fn newest<'a>(items: impl Iterator) -> Option<&'a #[cfg(test)] mod tests { + use super::super::fixtures::{ATGC_REPO, old_item, pds_pulls, pds_statuses}; use super::latest_states; use super::past_the_floor; use super::require_branch_pull; - use super::target_repo_did; use super::targets_repo; use super::{Datetime, FromStr, Reach}; - use super::{Empty, Listed, Source, State, author_did, classify_empty, ellipsize}; - use super::{RepoName, StackJson, StackMemberJson, known_state, status_row_json}; - use super::{append_backfilled, merge, missing_from_bobbin, newest, owner_and_name}; - use super::{ - apply_state_filter, appview_url, detail_item, for_branch, label_from_location, - settle_own_open, unique, - }; - use super::{pull_detail_json, pull_row_json}; - use super::{round_count, rows_by_repo, should_backfill}; + use super::{Empty, Listed, Source, State, classify_empty}; + use super::{append_backfilled, merge, missing_from_bobbin, newest}; + use super::{apply_state_filter, for_branch, settle_own_open, should_backfill}; use serde_json::{Value, json}; use std::collections::HashMap; - /// The repo `pr list` is run against by hand, and the one the PDS - /// fixture's records are mostly aimed at. - const ATGC_REPO: &str = "did:plc:gspkabpde4kx47fj3bhiwrms"; - - /// A real page of `sh.tangled.repo.listPullsBy` off Bobbin: the item - /// envelope (uri, state, commentCount) wrapped around the PDS record. - const BOBBIN_PAGE: &str = include_str!(concat!( - env!("CARGO_MANIFEST_DIR"), - "/tests/fixtures/bobbin_list_pulls_by.json" - )); - - /// A real `com.atproto.repo.listRecords` page of `sh.tangled.repo.pull` - /// off this account's PDS — the source the whole change is about. - /// - /// Eleven records taken verbatim out of a 52-record capture, chosen to - /// keep every trap the live collection contains: six aimed at - /// [`ATGC_REPO`] and five aimed at five *other* repos (a pull record - /// lives in its author's PDS whatever it targets, so this collection - /// mixes them and a listing that forgets to filter is wrong), records - /// with a `source` and records without, and two of the six atgc ones - /// carrying no status record at all, so their state is unknowable. - /// - /// ```text - /// curl 'https://amanita.us-east.host.bsky.network/xrpc/com.atproto.repo.listRecords\ - /// ?repo=did:plc:nlzmjyfv6loqtxyzvdcznwgf&collection=sh.tangled.repo.pull&limit=100' - /// ``` - const PDS_PULLS: &str = include_str!(concat!( - env!("CARGO_MANIFEST_DIR"), - "/tests/fixtures/pds_pulls_page.json" - )); - - /// The matching real `sh.tangled.repo.pull.status` page. Same capture - /// command with `collection=sh.tangled.repo.pull.status`. Nine records, - /// covering eight of the eleven pulls above: eight merged and one closed. - const PDS_STATUSES: &str = include_str!(concat!( - env!("CARGO_MANIFEST_DIR"), - "/tests/fixtures/pds_pull_statuses_page.json" - )); - - fn pds_records(fixture: &str) -> Vec { - serde_json::from_str::(fixture).unwrap()["records"] - .as_array() - .unwrap() - .clone() - } - - fn pds_pulls() -> Vec { - pds_records(PDS_PULLS) - } - - fn pds_statuses() -> Vec { - pds_records(PDS_STATUSES) - } - - /// A real pre-rounds record. Its listRecords envelope has the same - /// `uri` + `value` shape a Bobbin item does, minus the state fields, so - /// it stands in for an old item here. - const OLD_ITEM: &str = include_str!(concat!( - env!("CARGO_MANIFEST_DIR"), - "/tests/fixtures/pull_old_record.json" - )); - - fn bobbin_items() -> Vec { - serde_json::from_str::(BOBBIN_PAGE).unwrap()["items"] - .as_array() - .unwrap() - .clone() - } - - fn old_item() -> Value { - serde_json::from_str(OLD_ITEM).unwrap() - } - - #[test] - fn reads_the_target_repo_from_a_live_item() { - let items = bobbin_items(); - for item in &items { - let did = target_repo_did(item).expect("live items carry a target"); - assert_eq!(did, item["value"]["target"]["repo"].as_str().unwrap()); - // A DID, not specifically a did:plc one. Repos are minted with - // plc DIDs today, but nothing in the code depends on that and - // the fixture should not make the suite depend on it either. - assert!(crate::lexicon::identity::is_did(&did), "got {did}"); - } - } - - /// Old records have no `target` at all — the target was an at-uri - /// pointing at the repo *record*, so its authority is the repo owner's - /// DID rather than the repo's. The code knows this and settles for it, - /// since it only ever feeds a display label. - #[test] - fn falls_back_to_the_at_uri_authority_for_old_items() { - let item = old_item(); - assert!(item["value"].get("target").is_none()); - assert_eq!( - target_repo_did(&item).as_deref(), - Some("did:plc:wshs7t2adsemcrrd4snkeqli") - ); - } - - #[test] - fn target_repo_did_gives_up_rather_than_guessing() { - for malformed in [ - json!({}), - json!({ "value": {} }), - // Present but the wrong type: as_str declines, so does this. - json!({ "value": { "targetRepo": 3 } }), - json!({ "value": { "target": { "repo": null } } }), - // An at-uri that is not one. - json!({ "value": { "targetRepo": "did:plc:abc/sh.tangled.repo/x" } }), - ] { - assert_eq!(target_repo_did(&malformed), None, "input: {malformed}"); - } - } - - #[test] - fn reads_the_author_from_the_record_uri() { - for item in &bobbin_items() { - assert_eq!( - author_did(item).as_deref(), - Some("did:plc:nlzmjyfv6loqtxyzvdcznwgf") - ); - } - assert_eq!( - author_did(&old_item()).as_deref(), - Some("did:plc:wshs7t2adsemcrrd4snkeqli") - ); - } - - #[test] - fn author_did_gives_up_rather_than_guessing() { - for malformed in [ - json!({}), - json!({ "uri": null }), - json!({ "uri": "https://tangled.org/permadeath.com/atgc" }), - ] { - assert_eq!(author_did(&malformed), None, "input: {malformed}"); - } - } - - #[test] - fn splits_a_repo_record_uri() { - assert_eq!( - owner_and_name("at://did:plc:nlzmjyfv6loqtxyzvdcznwgf/sh.tangled.repo/atgc"), - Some(("did:plc:nlzmjyfv6loqtxyzvdcznwgf", "atgc")) - ); - for bad in [ - "", - "did:plc:abc/sh.tangled.repo/atgc", - // Authority and collection, but the record key is missing. - "at://did:plc:abc/sh.tangled.repo", - ] { - assert_eq!(owner_and_name(bad), None, "input: {bad}"); - } - } - - /// Live records all carry `rounds`; pre-rounds ones carry one inline - /// patch, which is one round however it is spelled. A zero here would - /// print "rounds:0" against a PR that plainly has a patch in it. - #[test] - fn counts_rounds_across_both_record_shapes() { - for item in &bobbin_items() { - let expected = item["value"]["rounds"].as_array().unwrap().len(); - assert_eq!(round_count(&item["value"]), expected); - } - let old = old_item(); - assert!(old["value"].get("rounds").is_none()); - assert_eq!(round_count(&old["value"]), 1); - } - /// The distinction added in "Distinguish an empty pull list from a stale /// index": an empty filtered listing means little on its own, because /// Bobbin's index runs behind the web view's. Only a second, unfiltered @@ -3415,84 +1858,8 @@ mod tests { "a naive string sort puts these the other way round" ); } - // -- the staleness evidence ------------------------------------------ - /// The observation this whole change is built on, as it stood on - /// The label both repo lookups produce, turned into the root the pull - /// listings hang off. The `@` is for a reader and is not in the path. - #[test] - fn builds_a_repo_url_from_the_label_it_already_had() { - assert_eq!( - appview_url("@permadeath.com/atgc"), - "https://tangled.org/permadeath.com/atgc" - ); - // Bobbin hands back a bare DID for an owner with no handle, and that - // is still a path tangled.org resolves. - assert_eq!( - appview_url("did:plc:abc/atgc"), - "https://tangled.org/did:plc:abc/atgc" - ); - } - - /// `status pr` spans repos and a number is only unique inside one, so the - /// rows are swept per repo. Two repos each having a pull that will come - /// back #12 is the case that makes one shared sweep wrong. - #[test] - fn groups_the_rows_by_the_repo_that_can_number_them() { - let row = |uri: &str, repo: &str, title: &str| Listed { - uri: uri.to_string(), - value: json!({ - "uri": uri, - "value": { "title": title, "target": { "repo": repo } }, - }), - state: State::Unknown, - comments: 0, - indexed: false, - }; - let items = vec![ - row( - "at://did:plc:a/sh.tangled.repo.pull/1", - "did:plc:one", - "Add repo list", - ), - row( - "at://did:plc:a/sh.tangled.repo.pull/2", - "did:plc:two", - "Add repo list", - ), - row( - "at://did:plc:a/sh.tangled.repo.pull/3", - "did:plc:one", - "Add pr status", - ), - ]; - let by_repo = rows_by_repo(&items); - assert_eq!(by_repo.len(), 2); - assert_eq!(by_repo["did:plc:one"].len(), 2); - assert_eq!( - by_repo["did:plc:two"], - vec![( - "at://did:plc:a/sh.tangled.repo.pull/2".to_string(), - "Add repo list".to_string() - )] - ); - } - - /// A row naming no target repo has no listing to be numbered from, and is - /// dropped rather than swept against another repo's. - #[test] - fn drops_a_row_that_names_no_repo() { - let orphan = Listed { - uri: "at://did:plc:a/sh.tangled.repo.pull/1".to_string(), - value: json!({ "value": { "title": "Add repo list" } }), - state: State::Unknown, - comments: 0, - indexed: false, - }; - assert!(rows_by_repo(&[orphan]).is_empty()); - } - /// 2026-08-06: Bobbin returned zero pulls for the atgc repo while the /// author's PDS held thirty. Every one of them is evidence. #[test] @@ -3681,37 +2048,6 @@ mod tests { } } - /// The reverse lookup that keeps `status pr` readable once it lists - /// repos Bobbin has never heard of. Checked against the real redirect: - /// `tangled.org/did:plc:gspkabpde4kx47fj3bhiwrms` 302s to - /// `https://tangled.org/permadeath.com/atgc`. - #[test] - fn reads_a_repo_label_off_the_appview_redirect() { - for location in [ - "https://tangled.org/permadeath.com/atgc", - "https://tangled.org/permadeath.com/atgc/", - "/permadeath.com/atgc", - ] { - assert_eq!( - label_from_location(location).as_deref(), - Some("@permadeath.com/atgc"), - "input: {location}" - ); - } - for bad in [ - "", - "/", - "/atgc", - // A hop that lands on a DID would render as a handle that is - // not one, which is worse than the truncated DID it falls back - // to. - "https://tangled.org/did:plc:gspkabpde4kx47fj3bhiwrms", - "https://tangled.org/permadeath.com/did:plc:abc", - ] { - assert_eq!(label_from_location(bad), None, "input: {bad}"); - } - } - /// Only an index can see a pull this account did not write, which is /// what every "there are no pull requests" message has to know before /// choosing how big a claim to make. @@ -3751,16 +2087,6 @@ mod tests { ); } - /// The dedupe the concurrent lookups inherited from the `for` loops they - /// replaced. Losing it would turn one lookup per author into one per row, - /// which is the opposite of the point. - #[test] - fn unique_keeps_first_appearance_order_and_drops_repeats() { - let keys = ["b", "a", "b", "c", "a"].iter().map(|s| s.to_string()); - assert_eq!(unique(keys), vec!["b", "a", "c"]); - assert!(unique(std::iter::empty()).is_empty()); - } - /// The case a string max got wrong. /// /// `pr view` matches a branch against the merged listing, whose two @@ -3932,356 +2258,4 @@ mod tests { assert!(!items[0].indexed); assert_eq!(items[0].comments, 0); } - - // ----------------------------------------------------------------------- - // `--json`: pure serializers, pinned against real records - // ----------------------------------------------------------------------- - - #[test] - fn known_state_turns_the_question_mark_into_null_and_nothing_else() { - assert_eq!(known_state("?"), None); - assert_eq!(known_state("open"), Some("open".to_string())); - assert_eq!(known_state("merged"), Some("merged".to_string())); - } - - /// The happy path: a live record with one round, a resolved number and a - /// resolved handle, pinned field by field against what - /// `bobbin_list_pulls_by.json`'s first item actually holds. - #[test] - fn pull_row_json_pulls_the_fields_the_table_already_computes() { - let item = &bobbin_items()[0]; - let row = pull_row_json(item, "merged", Some(42), Some("permadeath.com"), 3); - assert_eq!(row.uri, item["uri"].as_str().unwrap()); - assert_eq!(row.number, Some(42)); - assert_eq!(row.state.as_deref(), Some("merged")); - assert_eq!(row.title, "Include branch in image tag"); - assert_eq!( - row.author_did.as_deref(), - Some("did:plc:nlzmjyfv6loqtxyzvdcznwgf") - ); - assert_eq!(row.author_handle.as_deref(), Some("permadeath.com")); - assert_eq!(row.created_at.as_deref(), Some("2026-08-05T04:31:07+03:00")); - assert_eq!(row.rounds, 1); - assert_eq!(row.comments, 3); - } - - /// Everything unresolved comes back `null`, not the terminal column's - /// `"?"` or an empty string — the whole reason this type exists rather - /// than reusing the display strings. - #[test] - fn pull_row_json_unresolved_fields_are_null_not_placeholder_strings() { - let item = &bobbin_items()[0]; - let row = pull_row_json(item, "?", None, None, 0); - assert_eq!(row.state, None); - assert_eq!(row.number, None); - assert_eq!(row.author_handle, None); - // The DID is still there — only the *handle* was unresolved. - assert!(row.author_did.is_some()); - } - - /// A pre-rounds record: no `rounds` array and an empty `createdAt`, - /// which is real bytes off the network rather than an invented edge - /// case — see `pull_old_record.json`'s own doc comment. - #[test] - fn pull_row_json_old_record_has_no_created_at_and_counts_one_round() { - let item = old_item(); - let row = pull_row_json(&item, "closed", None, None, 0); - assert_eq!(row.created_at, None); - assert_eq!(row.rounds, 1); - } - - /// /// The exact key set a `jq` pipeline would see, pinned as a shape rather - /// than as the values inside it: a renamed field breaks a caller silently. - #[test] - fn pull_row_json_serializes_with_the_documented_field_names() { - let item = &bobbin_items()[0]; - let row = pull_row_json(item, "merged", Some(7), Some("permadeath.com"), 2); - let value = serde_json::to_value(&row).unwrap(); - let mut keys: Vec<&str> = value - .as_object() - .unwrap() - .keys() - .map(String::as_str) - .collect(); - keys.sort_unstable(); - assert_eq!( - keys, - vec![ - "author_did", - "author_handle", - "comments", - "created_at", - "number", - "rounds", - "state", - "title", - "uri", - ] - ); - } - - /// `status pr --json` is a `pr list --json` row with the repo columns - /// flattened in beside it, not nested under one — a caller moving from - /// one listing to the other reads `.state`, not `.pull.state`. - #[test] - fn status_row_json_flattens_the_pull_row_beside_the_repo() { - let item = &bobbin_items()[0]; - let repo = RepoName { - label: "@permadeath.com/atgc".to_string(), - web_url: Some("https://tangled.org/permadeath.com/atgc".to_string()), - }; - let row = status_row_json( - item, - "merged", - Some(7), - Some("permadeath.com"), - 2, - Some(ATGC_REPO.to_string()), - Some(&repo), - ); - let value = serde_json::to_value(&row).unwrap(); - let mut keys: Vec<&str> = value - .as_object() - .unwrap() - .keys() - .map(String::as_str) - .collect(); - keys.sort_unstable(); - assert_eq!( - keys, - vec![ - "author_did", - "author_handle", - "comments", - "created_at", - "number", - "repo", - "repo_did", - "repo_url", - "rounds", - "state", - "title", - "uri", - ] - ); - // No `@`, matching `author_handle` and every other JSON field that - // holds a name a URL is built from. - assert_eq!(value["repo"], "permadeath.com/atgc"); - assert_eq!(value["state"], "merged"); - } - - /// A repo neither lookup could name keeps its DID and reads `null` for - /// the name and the URL. The text column prints a truncated DID there — - /// a display string with an ellipsis in it, which is not a repo anybody - /// can fetch and so is not a value this may emit. - #[test] - fn status_row_json_unresolved_repo_is_null_not_a_truncated_did() { - let item = &bobbin_items()[0]; - let unresolved = RepoName { - label: ellipsize(ATGC_REPO, 21), - web_url: None, - }; - let row = status_row_json( - item, - "open", - None, - None, - 0, - Some(ATGC_REPO.to_string()), - Some(&unresolved), - ); - assert_eq!(row.repo, None); - assert_eq!(row.repo_url, None); - assert_eq!(row.repo_did.as_deref(), Some(ATGC_REPO)); - } - - /// The old-record fallback `pr view` prints as a single inline round: - /// one entry, its bytes taken from the inline `patch` string rather than - /// a `patchBlob`. - #[test] - fn pull_detail_json_old_record_falls_back_to_one_inline_round() { - let item = old_item(); - let detail = pull_detail_json( - &item, - "closed", - None, - 0, - Some("https://tangled.org/x/pulls/9"), - None, - None, - ); - assert_eq!(detail.rounds.len(), 1); - assert_eq!(detail.rounds[0].index, 1); - assert!(detail.rounds[0].patch_bytes.unwrap() > 0); - assert_eq!(detail.created_at, None); - assert_eq!(detail.stack, None); - } - - /// A live record's `rounds` array is walked one-for-one, one-based, with - /// the round's own blob size rather than anything recomputed. - #[test] - fn pull_detail_json_lists_every_round_from_the_record() { - let item = &bobbin_items()[0]; - let detail = pull_detail_json( - item, - "merged", - Some("permadeath.com"), - 1, - Some("https://tangled.org/x/pulls/1"), - None, - None, - ); - assert_eq!(detail.rounds.len(), 1); - assert_eq!(detail.rounds[0].index, 1); - assert_eq!(detail.rounds[0].patch_bytes, Some(5145)); - assert_eq!(detail.state.as_deref(), Some("merged")); - assert_eq!(detail.url.as_deref(), Some("https://tangled.org/x/pulls/1")); - assert_eq!( - detail.body.as_deref(), - Some("Co-Authored-By: Claude Fable 5 ") - ); - } - - /// A body that is present but only whitespace prints nothing in the - /// human view, and reads `null` here for the same reason. - #[test] - fn pull_detail_json_blank_body_is_null() { - let item = json!({ - "uri": "at://did:plc:me/sh.tangled.repo.pull/3xyz", - "value": { "title": "t", "body": " \n ", "createdAt": "2026-01-01T00:00:00Z" }, - }); - let detail = pull_detail_json( - &item, - "open", - None, - 0, - Some("https://x/pulls/1"), - None, - None, - ); - assert_eq!(detail.body, None); - } - - /// The `stack` field is carried through exactly as built — this only - /// pins that `pull_detail_json` does not touch it, since the assembly - /// itself lives in `view` next to the chain it walks. - #[test] - fn pull_detail_json_carries_a_stack_through_untouched() { - let item = &bobbin_items()[0]; - let stack = StackJson { - total: 2, - position: Some(2), - members: vec![StackMemberJson { - position: 2, - uri: "at://did:plc:me/sh.tangled.repo.pull/3top".to_string(), - state: Some("open".to_string()), - title: "top".to_string(), - }], - missing_below: Some("at://did:plc:me/sh.tangled.repo.pull/3below".to_string()), - }; - let detail = pull_detail_json( - item, - "open", - None, - 0, - Some("https://x/pulls/1"), - Some(stack), - None, - ); - let stack = detail.stack.expect("stack should be Some"); - assert_eq!(stack.total, 2); - assert_eq!(stack.position, Some(2)); - assert_eq!(stack.members.len(), 1); - assert_eq!(stack.members[0].position, 2); - assert_eq!( - stack.missing_below.as_deref(), - Some("at://did:plc:me/sh.tangled.repo.pull/3below") - ); - } - - /// Naming a member of a stack details *that* member. This is the whole - /// of the bug it replaces: the top was substituted for every named pull, - /// so a stack of eight answered eight different numbers, rkeys, at:// - /// URIs and URLs with one identical record, and nothing in the output - /// said so. - #[test] - fn naming_a_stack_member_details_that_member_not_the_top() { - let bottom = json!({ "uri": "at://did:plc:me/sh.tangled.repo.pull/3bot" }); - let top = json!({ "uri": "at://did:plc:me/sh.tangled.repo.pull/3top" }); - let chain = crate::cmd::stack::Chain { - members: vec![&bottom, &top], - missing_below: None, - }; - assert_eq!( - detail_item(true, Some(&chain), &bottom)["uri"], - bottom["uri"], - "a named pull is the answer, not a way into the chain" - ); - } - - /// The no-argument form keeps promoting to the top, which is what makes - /// the chain readable from a stacked branch at all: every member shares - /// `source.branch`, so the branch match alone is arbitrary. - #[test] - fn a_branch_match_is_still_promoted_to_the_top() { - let bottom = json!({ "uri": "at://did:plc:me/sh.tangled.repo.pull/3bot" }); - let top = json!({ "uri": "at://did:plc:me/sh.tangled.repo.pull/3top" }); - let chain = crate::cmd::stack::Chain { - members: vec![&bottom, &top], - missing_below: None, - }; - assert_eq!(detail_item(false, Some(&chain), &bottom)["uri"], top["uri"]); - } - - /// A pull that stands alone is itself either way — no chain, nothing to - /// promote to, and the named and branch paths must not diverge here. - #[test] - fn an_unstacked_pull_is_its_own_detail() { - let only = json!({ "uri": "at://did:plc:me/sh.tangled.repo.pull/3one" }); - assert_eq!(detail_item(true, None, &only)["uri"], only["uri"]); - assert_eq!(detail_item(false, None, &only)["uri"], only["uri"]); - } - - /// The exact key set `pr view --json` prints, including that `stack` is - /// present (as `null`) even for a pull that stands alone rather than - /// being omitted — a script checking `.stack == null` should not have - /// to also check `has("stack")`. - #[test] - fn pull_detail_json_serializes_with_the_documented_field_names() { - let item = &bobbin_items()[0]; - let detail = pull_detail_json( - item, - "merged", - None, - 0, - Some("https://x/pulls/1"), - None, - None, - ); - let value = serde_json::to_value(&detail).unwrap(); - let mut keys: Vec<&str> = value - .as_object() - .unwrap() - .keys() - .map(String::as_str) - .collect(); - keys.sort_unstable(); - assert_eq!( - keys, - vec![ - "author_did", - "author_handle", - "body", - "comments", - "created_at", - "rounds", - "stack", - "state", - "title", - "uri", - "url", - ] - ); - assert_eq!(value["stack"], Value::Null); - } }