diff --git a/plan/pull-numbers.md b/plan/pull-numbers.md index 1bca35b..0f428d0 100644 --- a/plan/pull-numbers.md +++ b/plan/pull-numbers.md @@ -42,6 +42,22 @@ delete the day a lexicon carries the number. ## Done +- [x] A pasted pull URL is resolved against **the repo the URL names**. The + parser used to keep the `/pulls/23` and throw the `@owner/name` in front + of it away, and every caller then resolved the bare number against + `git remote get-url`, so `atgc pr diff + https://tangled.org/@bob/otherproject/pulls/23` printed the current + checkout's #23 instead — under the other pull's title, and with nothing + in a patch on a pipe to name a repo and give it away. `pr checkout` on + the same link `git am`ed the wrong patch, and `pr close`, `pr reopen` and + `pr edit` classify their own input and had the same hole. Resolving the + URL's own repo rather than refusing when it disagrees with the checkout + is the choice made: a link in a chat message is how somebody else's pull + request usually arrives, reading one should not require a clone of a repo + you may not have, and the repo is *written in the URL* — using what is + written is not the kind of guess `parse_pull_ref` refuses to make. A bare + number still means "in this repo", which is what `--remote` is for + - [x] A number is printed only when it has been confirmed against the record's own at-URI. `numbers_for_records` used to accept a *unique* title match without confirming, on the rule that a match is evidence diff --git a/src/clients/tangled/web/pulls.rs b/src/clients/tangled/web/pulls.rs index 0209fec..cdd5e78 100644 --- a/src/clients/tangled/web/pulls.rs +++ b/src/clients/tangled/web/pulls.rs @@ -38,22 +38,62 @@ use anyhow::{Context, Result}; use std::collections::{BTreeSet, HashMap}; use std::time::Duration; -/// The number out of a Tangled pull URL, so a pasted link works as well as -/// the number in it. `/pulls/23`, `/pulls/23/round/2` and a trailing slash -/// all mean pull 23. +/// What a pasted Tangled pull URL names: a number, and the repo it is a +/// number *in*. +/// +/// Both halves, because a pull number is unique only inside one repo and +/// `/pulls/23` on two repos is two different pull requests. This used to hand +/// back the number alone, and every caller then resolved it against whatever +/// repo the current checkout pointed at — so `atgc pr diff +/// https://tangled.org/@bob/otherproject/pulls/23` printed *your* #23's patch, +/// under a stranger's title, with nothing in the output naming a repo. The one +/// spelling that carries the repo was the one that threw it away. +#[derive(Debug, PartialEq, Eq)] +pub(crate) struct PullInUrl { + /// The appview base for the repo in front of `/pulls/`: scheme, host and + /// `@owner/name`, with no trailing slash. `https://` is filled in when the + /// URL was pasted without a scheme, which is how one arrives out of a chat + /// message rather than an address bar. + /// + /// `None` when there was no repo path at all — `https://tangled.org/pulls/23` + /// — since then the URL says no more about which repo is meant than a bare + /// `23` does, and the caller falls back to the checkout's remote. + pub(crate) repo_url: Option, + pub(crate) number: u32, +} + +/// The pull a Tangled URL names. `/pulls/23`, `/pulls/23/round/2` and a +/// trailing slash all mean pull 23. /// /// Crate-visible because [`crate::cmd::pr`]'s write half tells the same spellings /// apart under stricter rules — it will not read a handle as a pull's owner — /// and so classifies its own input rather than calling `review`'s own parser, but -/// there is no second way to find a number in a URL and it does not get one. -pub(crate) fn pull_number_from_url(url: &str) -> Option { - let mut segments = url.split('/').peekable(); - while let Some(segment) = segments.next() { - if segment == "pulls" { - return segments.next()?.parse().ok(); - } +/// there is no second way to find a pull in a URL and it does not get one. +pub(crate) fn pull_in_url(url: &str) -> Option { + let (before, after) = url.split_once("/pulls/")?; + let number = after.split('/').next()?.parse().ok()?; + Some(PullInUrl { + repo_url: repo_url_of(before), + number, + }) +} + +/// The appview base out of the part of a pull URL in front of `/pulls/`. +/// +/// A scheme is optional and supplied; a host on its own is not a repo and is +/// refused, so that a URL naming no repo falls back to the checkout rather +/// than sending the appview a request for `https://tangled.org/pulls/23`. +fn repo_url_of(before: &str) -> Option { + let trimmed = before.trim().trim_end_matches('/'); + let (scheme, rest) = match trimmed.split_once("://") { + Some((scheme, rest)) => (format!("{scheme}://"), rest), + None => ("https://".to_string(), trimmed), + }; + let (host, path) = rest.split_once('/')?; + if host.is_empty() || path.is_empty() { + return None; } - None + Some(format!("{scheme}{host}/{path}")) } /// Which pull record a page of the appview's HTML is about. @@ -1023,17 +1063,47 @@ mod tests { "/tests/fixtures/appview_pull_listing_excerpt.html" )); + /// A pasted URL names a pull *and the repo it is a pull of*, and both + /// halves come back. Dropping the repo is what made `pr diff ` print the current checkout's pull of the same number instead, so + /// the repo being carried is the assertion that matters here. #[test] - fn finds_the_pull_number_in_a_url() { + fn finds_the_pull_and_its_repo_in_a_url() { assert_eq!( - pull_number_from_url("https://tangled.org/permadeath.com/atgc/pulls/29"), - Some(29) + pull_in_url("https://tangled.org/permadeath.com/atgc/pulls/29"), + Some(PullInUrl { + repo_url: Some("https://tangled.org/permadeath.com/atgc".to_string()), + number: 29, + }) + ); + // The round sub-page is still pull 29 of the same repo. + assert_eq!( + pull_in_url("https://tangled.org/@bob/other/pulls/29/round/2"), + Some(PullInUrl { + repo_url: Some("https://tangled.org/@bob/other".to_string()), + number: 29, + }) ); - assert_eq!(pull_number_from_url("https://tangled.org/x/y/pulls/"), None); + // Pasted out of a chat message rather than an address bar: the scheme + // is supplied, because the base is about to be fetched. assert_eq!( - pull_number_from_url("https://tangled.org/x/y/issues/3"), - None + pull_in_url("tangled.org/@bob/other/pulls/29"), + Some(PullInUrl { + repo_url: Some("https://tangled.org/@bob/other".to_string()), + number: 29, + }) + ); + // A URL with no repo path says no more than a bare number does, so it + // carries no repo and the caller falls back to the checkout. + assert_eq!( + pull_in_url("https://tangled.org/pulls/29"), + Some(PullInUrl { + repo_url: None, + number: 29, + }) ); + assert_eq!(pull_in_url("https://tangled.org/x/y/pulls/"), None); + assert_eq!(pull_in_url("https://tangled.org/x/y/issues/3"), None); } /// The scrape's whole safety argument, against the page that breaks the /// naive version of it. diff --git a/src/cmd/pr/review.rs b/src/cmd/pr/review.rs index f157f70..a84b716 100644 --- a/src/cmd/pr/review.rs +++ b/src/cmd/pr/review.rs @@ -42,7 +42,7 @@ use crate::clients::git::run as git; use crate::clients::git::worktree; use crate::clients::tangled::resolve; -use crate::clients::tangled::web::pulls::{pull_number_from_url, pull_uri_from_appview}; +use crate::clients::tangled::web::pulls::{pull_in_url, pull_uri_from_appview}; use crate::config::account; use crate::lexicon::aturi::split_aturi; use crate::lexicon::identity; @@ -81,9 +81,18 @@ enum PullRef { Authored { author: String, rkey: String }, /// A record key alone. Whose it is has to be worked out. Bare(String), - /// The `/pulls/23` number from a Tangled URL. Not in the record — see - /// [`pull_uri_from_appview`]. - Number(u32), + /// The `/pulls/23` number from a Tangled URL, or typed bare. Not in the + /// record — see [`pull_uri_from_appview`]. + /// + /// `repo_url` is the repo the URL named, when it named one. A number is + /// unique only inside one repo, so a bare number has to borrow the + /// checkout's remote and a pasted URL must not: it already says which repo + /// it is a number in, and resolving it against the checkout is how + /// somebody else's `/pulls/23` becomes yours. + Number { + number: u32, + repo_url: Option, + }, } /// Classify what the user typed. Never guesses across kinds: a value that is @@ -123,18 +132,24 @@ fn parse_pull_ref(input: &str) -> Result { } if raw.starts_with("https://") || raw.starts_with("http://") { - return pull_number_from_url(raw) - .map(PullRef::Number) - .with_context(|| format!("{raw:?} is a URL with no /pulls/ in it")); + let found = pull_in_url(raw) + .with_context(|| format!("{raw:?} is a URL with no /pulls/ in it"))?; + return Ok(PullRef::Number { + number: found.number, + repo_url: found.repo_url, + }); } // Record keys are TIDs — thirteen characters of base32-sortable — so they // are never all digits, and a number is never mistaken for one. if raw.chars().all(|c| c.is_ascii_digit()) { - return raw + let number = raw .parse() - .map(PullRef::Number) - .with_context(|| format!("{raw:?} is too large to be a pull number")); + .with_context(|| format!("{raw:?} is too large to be a pull number"))?; + return Ok(PullRef::Number { + number, + repo_url: None, + }); } // `permadeath.com/3msg7wllcqc2b` — the author and the key, which is the @@ -303,7 +318,9 @@ pub(crate) async fn resolve_pull( PullRef::Full { did, rkey } => (did, rkey), PullRef::Authored { author, rkey } => (account::actor_did(&author).await?, rkey), PullRef::Bare(rkey) => (author_of_rkey(&rkey, author, remote).await?, rkey), - PullRef::Number(n) => pull_ref_from_number(n, remote).await?, + PullRef::Number { number, repo_url } => { + pull_ref_from_number(number, repo_url.as_deref(), remote).await? + } }; // The PDS to ask is the one in the *author's* DID document, not ours: a @@ -331,15 +348,34 @@ pub(crate) async fn resolve_pull( /// swap against, so having this fetch a copy first would buy a request nobody /// uses. /// -/// Which repo a number belongs to comes from the git remote, because a number -/// is only unique inside one — `#67` on two repos is two pulls. That is the -/// one thing the number form needs a checkout for, and why the commands that -/// take one grew a `--remote`: the record key and at-URI spellings still reach -/// a pull from anywhere. -pub(crate) async fn pull_ref_from_number(number: u32, remote: &str) -> Result<(String, String)> { - let remote_url = git::remote_url(remote)?; - let repo = resolve::repo_ref(&remote_url).await?; - pull_uri_from_appview(&repo.web_url, number).await +/// Which repo a number belongs to comes from the URL it was pasted out of +/// when there was one, and from the git remote otherwise — because a number is +/// only unique inside one repo, and `#67` on two repos is two pulls. That is +/// the one thing a *bare* number needs a checkout for, and why the commands +/// that take one grew a `--remote`: the record key and at-URI spellings reach +/// a pull from anywhere, and so, now, does a pasted URL. +/// +/// Resolving the URL's own repo rather than refusing when it disagrees with +/// the checkout is the choice made here, and it is the more useful of the two: +/// a link in a chat message is the commonest way somebody else's pull request +/// arrives, and reading it should not require standing in a clone of a repo +/// you may not have. It is not a guess either, which is the rule +/// [`parse_pull_ref`] keeps — the repo is written in the URL, and using what +/// is written is the opposite of inferring what was not. +pub(crate) async fn pull_ref_from_number( + number: u32, + repo_url: Option<&str>, + remote: &str, +) -> Result<(String, String)> { + let web_url = match repo_url { + Some(url) => url.to_string(), + None => { + let remote_url = git::remote_url(remote)?; + resolve::repo_ref(&remote_url).await?.web_url + } + }; + crate::logging::debug::log(format!("pull #{number} is being looked up on {web_url}")); + pull_uri_from_appview(&web_url, number).await } /// Whose record key is this? @@ -1456,6 +1492,7 @@ pub(crate) async fn checkout(args: CheckoutArgs) -> Result<()> { // round's patch before being trusted, refused if it disagrees, and the // lines printed below say whether a corroborated branch or a patch was // what actually got checked out. + if let Some(source) = pull.source_branch() && !args.patch { @@ -1734,15 +1771,30 @@ mod tests { rkey: "3msg7wllcqc2b".into() } ); - assert_eq!(parse_pull_ref("23").unwrap(), PullRef::Number(23)); + // A bare number names no repo, so it borrows the checkout's remote. + assert_eq!( + parse_pull_ref("23").unwrap(), + PullRef::Number { + number: 23, + repo_url: None + } + ); + // A URL does name one, and keeping it is what stops a stranger's link + // resolving against whatever repo happens to be underfoot. assert_eq!( parse_pull_ref("https://tangled.org/permadeath.com/atgc/pulls/23").unwrap(), - PullRef::Number(23) + PullRef::Number { + number: 23, + repo_url: Some("https://tangled.org/permadeath.com/atgc".into()) + } ); // The URL the appview redirects a pull to. assert_eq!( parse_pull_ref("https://tangled.org/permadeath.com/atgc/pulls/23/round/2").unwrap(), - PullRef::Number(23) + PullRef::Number { + number: 23, + repo_url: Some("https://tangled.org/permadeath.com/atgc".into()) + } ); assert_eq!( parse_pull_ref("permadeath.com/3msg7wllcqc2b").unwrap(), diff --git a/src/cmd/pr/write.rs b/src/cmd/pr/write.rs index ccac7d9..8539674 100644 --- a/src/cmd/pr/write.rs +++ b/src/cmd/pr/write.rs @@ -1350,7 +1350,16 @@ enum PullTarget { Local(PullRef), /// The appview's `/pulls/23`, which is not in the record and has to be /// translated — see [`crate::cmd::pr::review::pull_ref_from_number`]. - Number(u32), + /// + /// `repo_url` is the repo a pasted link named, and `None` for a number + /// typed bare, which has to borrow the checkout's remote instead. A number + /// means nothing without a repo, so a link that carries one must not be + /// resolved against whichever repo the caller happens to be standing in — + /// these are the verbs that *close* and *edit* pull requests. + Number { + number: u32, + repo_url: Option, + }, } /// Turn what a user typed into a pull reference, or explain why it cannot be @@ -1432,7 +1441,10 @@ fn classify_pull_ref(input: &str, acting_did: &str) -> Result { if input.bytes().all(|b| b.is_ascii_digit()) { return input .parse() - .map(PullTarget::Number) + .map(|number| PullTarget::Number { + number, + repo_url: None, + }) .map_err(|_| usage(format!("{input} is too large to be a pull number"))); } @@ -1440,8 +1452,11 @@ fn classify_pull_ref(input: &str, acting_did: &str) -> Result { // is as much an answer as the number in it, and is what the browser's // address bar hands over. if input.contains('/') { - return match crate::clients::tangled::web::pulls::pull_number_from_url(input) { - Some(n) => Ok(PullTarget::Number(n)), + return match crate::clients::tangled::web::pulls::pull_in_url(input) { + Some(found) => Ok(PullTarget::Number { + number: found.number, + repo_url: found.repo_url, + }), None => Err(usage(format!( "{input} is not a pull request reference. A tangled.org link works when it has \ a /pulls/ in it, and this one does not.\n\ @@ -1470,8 +1485,13 @@ fn classify_pull_ref(input: &str, acting_did: &str) -> Result { async fn resolve_pull_ref(input: &str, acting_did: &str, remote: &str) -> Result { match classify_pull_ref(input, acting_did)? { PullTarget::Local(target) => Ok(target), - PullTarget::Number(n) => { - let (author, rkey) = crate::cmd::pr::review::pull_ref_from_number(n, remote).await?; + PullTarget::Number { + number: n, + repo_url, + } => { + let (author, rkey) = + crate::cmd::pr::review::pull_ref_from_number(n, repo_url.as_deref(), remote) + .await?; crate::logging::debug::log(format!( "pull #{n} is at://{author}/{}/{rkey}", crate::lexicon::tangled::PULL_NSID @@ -2924,7 +2944,10 @@ mod tests { for (input, expected) in [("23", 23), ("1", 1), ("0042", 42), (" 7 ", 7)] { assert_eq!( classify_pull_ref(input, ME).unwrap(), - PullTarget::Number(expected), + PullTarget::Number { + number: expected, + repo_url: None, + }, "{input:?}" ); } @@ -2936,20 +2959,31 @@ mod tests { assert!(err.contains("too large"), "{err}"); } - /// A pasted link is the number in it, with or without the scheme — the - /// address bar gives you one and a chat message the other. + /// A pasted link is the number in it *and the repo it is a number in*, + /// with or without the scheme — the address bar gives you one and a chat + /// message the other. + /// + /// The repo half is the part that pins a bug: `pr close` and `pr edit` + /// write, and resolving a stranger's link against the current checkout is + /// how one of these verbs lands on the wrong pull request entirely. #[test] - fn a_tangled_url_is_the_number_in_it() { - for input in [ - "https://tangled.org/@permadeath.com/atgc/pulls/23", - "http://tangled.org/@permadeath.com/atgc/pulls/23", - "tangled.org/@permadeath.com/atgc/pulls/23", - // The round sub-page is still pull 23. - "https://tangled.org/@permadeath.com/atgc/pulls/23/round/1", + fn a_tangled_url_is_the_number_in_it_and_the_repo_it_is_in() { + for (input, scheme) in [ + ("https://tangled.org/@permadeath.com/atgc/pulls/23", "https"), + ("http://tangled.org/@permadeath.com/atgc/pulls/23", "http"), + ("tangled.org/@permadeath.com/atgc/pulls/23", "https"), + // The round sub-page is still pull 23 of the same repo. + ( + "https://tangled.org/@permadeath.com/atgc/pulls/23/round/1", + "https", + ), ] { assert_eq!( classify_pull_ref(input, ME).unwrap(), - PullTarget::Number(23), + PullTarget::Number { + number: 23, + repo_url: Some(format!("{scheme}://tangled.org/@permadeath.com/atgc")), + }, "{input:?}" ); } diff --git a/tests/pr_flows.rs b/tests/pr_flows.rs index e57ad48..263fc6e 100644 --- a/tests/pr_flows.rs +++ b/tests/pr_flows.rs @@ -1492,3 +1492,84 @@ fn a_listing_says_how_many_rows_the_limit_cut() { run.stderr ); } + +// --------------------------------------------------------------------------- +// reading somebody else's pull: diff and checkout +// --------------------------------------------------------------------------- + +/// A pasted pull URL is read against the repo *the URL names*, not against +/// whatever repo the current checkout points at. +/// +/// A pull number is allocated per repo, so `#23` on two repos is two pull +/// requests. The URL parser used to keep the number and throw the +/// `@owner/name` in front of it away, and the number was then resolved against +/// `git remote get-url origin`. Standing in your own checkout, `atgc pr diff +/// ` printed *your* #23's patch — and since a +/// patch on a pipe carries no repo anywhere in it, nothing said so. +#[test] +fn a_pasted_pull_url_names_the_repo_its_number_belongs_to() { + let world = Scenario::new("pr-diff-pasted-url"); + + // Two pulls numbered 23: one on the repo this checkout points at, one on + // a repo it has never heard of. Pre-rounds records, whose patch is inline, + // so that what is printed is decided by which record was read and by + // nothing else. + world.with(|w| { + w.plant( + ALICE, + PULL_NSID, + "3aaaaaaaaaaaa", + serde_json::json!({ + "title": "alice's twenty-third", + "targetRepo": REPO_DID, + "targetBranch": "main", + "patch": "--- a/alice.txt\n+++ b/alice.txt\n", + "createdAt": "2026-01-01T00:00:00Z", + }), + ); + w.plant( + BOB, + PULL_NSID, + "3bbbbbbbbbbbb", + serde_json::json!({ + "title": "bob's twenty-third", + "targetBranch": "main", + "patch": "--- a/bob.txt\n+++ b/bob.txt\n", + "createdAt": "2026-01-01T00:00:00Z", + }), + ); + // The page each repo's `/pulls/23` renders. The checkout's own repo is + // addressed by DID, which is what `resolve::repo_ref` makes of an + // `origin` pointing at a knot. + w.pull_pages.insert( + (REPO_DID.to_string(), 23), + format!("at://{ALICE}/{PULL_NSID}/3aaaaaaaaaaaa"), + ); + w.pull_pages.insert( + ("@bob.test/otherproject".to_string(), 23), + format!("at://{BOB}/{PULL_NSID}/3bbbbbbbbbbbb"), + ); + }); + + let url = format!("{}/@bob.test/otherproject/pulls/23", world.appview_url()); + let run = world.run(&["pr", "diff", &url]).success(); + assert!( + run.stdout.contains("bob.txt"), + "the link named Bob's repo and this is not Bob's patch\n--- stdout ---\n{}", + run.stdout + ); + assert!( + !run.stdout.contains("alice.txt"), + "the checkout's own #23 was printed for a link to another repo's\n--- stdout ---\n{}", + run.stdout + ); + + // And a bare number still means "in this repo", which is the whole reason + // these commands take a `--remote` at all. + let run = world.run(&["pr", "diff", "23"]).success(); + assert!( + run.stdout.contains("alice.txt"), + "a bare number stopped meaning this checkout's repo\n--- stdout ---\n{}", + run.stdout + ); +} diff --git a/tests/support/services.rs b/tests/support/services.rs index 398738a..ecb3e61 100644 --- a/tests/support/services.rs +++ b/tests/support/services.rs @@ -497,11 +497,40 @@ pub fn bobbin(world: &mut World, req: &Incoming) -> Reply { /// pretending it has a link it does not. Serving invented pages here would /// test the scraper, which `tests/fixtures/appview_pull_page_excerpt.html` /// already does against real markup. +/// An appview, which for these tests means one thing: the HTML page a pull +/// *number* names. +/// +/// The number is in no record and no XRPC response, so `pr diff 23` and every +/// write verb that takes a number resolve one by fetching `/pulls/` +/// and reading the `data-aturi` off it. What the page says is therefore what +/// decides which record a command acts on, and [`World::pull_pages`] holds one +/// answer per `(repo path, number)` pair rather than per number. pub fn appview(world: &mut World, req: &Incoming) -> Reply { note(world, "appview", &req.path, req, false); + if let Some((repo, number)) = req.path.rsplit_once("/pulls/") + && let Ok(number) = number.parse::() + && let Some(uri) = world.pull_pages.get(&(repo.to_string(), number)) + { + return Reply::Bytes(pull_page(uri).into_bytes()); + } Reply::not_found(format!("the mock appview has no page at /{}", req.path)) } +/// A pull's page, cut down to the one thing atgc reads out of it. +/// +/// The real page is up to 2.5 MB of rendered diff and this is four lines of +/// it, which is honest rather than lazy: a fixture is what proves the scrape +/// survives a real page (see `tests/fixtures/appview_pull_page_excerpt.html`), +/// and a mock's job here is only to give two repos different answers so a test +/// can tell which one was asked. +fn pull_page(uri: &str) -> String { + format!( + "\ + \ + " + ) +} + /// A knot: the merge check and the merge itself. /// /// It performs no git operation. What a merge means to the commands under diff --git a/tests/support/world.rs b/tests/support/world.rs index 7031735..4f36d9b 100644 --- a/tests/support/world.rs +++ b/tests/support/world.rs @@ -136,6 +136,17 @@ pub struct World { /// separate from [`World::repos`]: Bobbin is an index and can lag, and /// the commands that must not trust it are the ones worth testing. pub bobbin_pulls: Vec, + /// The pull pages the mock appview serves: `(repo path, number)` → the + /// at-URI that page's record-identity widget names. + /// + /// A pull *number* lives nowhere but the appview's own database, so a + /// number is only ever resolved by fetching the page it names and reading + /// the `data-aturi` off it. Keying on the repo path as well as the number + /// is the whole point: `#23` on two repos is two different pull requests, + /// and a mock that answered every path with one record could not tell a + /// command that resolved the right repo from one that resolved whichever + /// repo it was standing in. + pub pull_pages: BTreeMap<(String, u32), String>, /// A service that answers every call with this many bytes and no /// `Content-Length`, instead of whatever it was going to say. /// @@ -178,6 +189,7 @@ impl World { knot_delete: Ok(()), fail_next_batch_swap: false, bobbin_pulls: Vec::new(), + pull_pages: BTreeMap::new(), flood: None, } }