diff --git a/plan/pull-requests.md b/plan/pull-requests.md index b766c19..e547350 100644 --- a/plan/pull-requests.md +++ b/plan/pull-requests.md @@ -43,6 +43,12 @@ line of work sit beside it: publishing the branch a pull claims is answers "does this actor own it", not "who does", so resolving the owner from the repo DID is a lookup this path does not currently make. Decide where that comes from before adding a third walk +- [ ] **The same walk in `issue close`/`issue reopen` has not been checked.** + Issues carry the identical shape — a state record in the *acting* + account's PDS, honored from the author or the repo owner, newest wins — + and the pull version of it read the wrong set of accounts for long + enough to hide a merge. Nothing here says the issue one is right; it + says nobody has looked yet - [ ] `resolve` refuses `//` rather than resolving it. The knot serves that path, but answers it directly instead of redirecting, so there is no repo DID to read out of a `Location` @@ -80,6 +86,30 @@ line of work sit beside it: publishing the branch a pull claims is ## Done +- [x] **`pr close`/`pr reopen` read the repo owner's status records, not + just the author's and their own.** The `merged` refusal exists because + state is a log and the newest record wins, so a `closed` written after + a `merged` replaces it in every reader's view rather than sitting + beside it. The guard was right and the state it guarded on was not: + `list_statuses` walked the pull's author and the acting account, and a + merge performed by the repo owner — the ordinary way a contributor's + pull gets merged — lands in the *owner's* PDS. The author saw no status + record at all, fell through to `Open`, and atgc printed + `open -> closed` over somebody's merge. The quieter form of the same + blindness had `pr reopen` answer `already open; nothing written` and + exit `0` against a pull the owner had closed. + Getting the owner is the awkward part and there is exactly one route: a + repo's DID document names its knot and nothing else, and the repo + record that names an owner lives in that owner's PDS, which is the + thing being looked for. `resolve::owner_of` asks the appview, which + publishes the mapping as a `302` from `/` to + `//`. That is a new dependency on a service these commands + did not previously touch, and it is here because the alternative is + writing over merges. An owner that will not resolve is refused rather + than assumed absent, on the same reasoning as the page cap beside it — + both are absences that cannot be told apart from "nobody has acted". + The lookup is skipped when `standing_of` already found the acting + account to be the owner, and when the pull names no repo - [x] `pr create` names a pull after the commit it *starts* with, which is what `stack create` has always done — "a member is titled and described by its bottom commit". The flat verb took the tip instead, so a branch diff --git a/src/clients/tangled/resolve.rs b/src/clients/tangled/resolve.rs index d5ff034..1ae68cc 100644 --- a/src/clients/tangled/resolve.rs +++ b/src/clients/tangled/resolve.rs @@ -186,6 +186,65 @@ fn path_of(url: &str) -> &str { .map_or(url, |(_host, path)| path) } +/// Who owns the repo `repo_did`, as a DID, or `None` when nothing here can +/// say. +/// +/// **There is no PDS-only answer to this question.** A repo's DID document +/// names its knot and nothing else — no `alsoKnownAs`, no owner — and the +/// repo *record* that does name an owner lives in that owner's PDS, which is +/// the thing being looked for. `ownership::owns_repo` answers "does this +/// account own it", which is a different question and cannot be run over +/// every account there is. +/// +/// So this asks the appview, which keeps the mapping and publishes it as a +/// redirect: `GET /` answers `302` to +/// `//`. The handle then resolves to a DID the ordinary +/// way. That is one request against a service atgc otherwise only reads when +/// asked to, and it is here because the alternative is worse: see +/// `crate::cmd::pr::write`'s status walk, which without this cannot see a +/// merge performed by a repo owner and will happily write `closed` over it. +/// +/// `None` rather than an error for a redirect that does not come or does not +/// parse: the caller decides what an unanswerable owner means for what it +/// was about to do, and for at least one of them the answer is to refuse. +pub async fn owner_of(repo_did: &str) -> Result> { + let url = format!("{}/{repo_did}", crate::clients::endpoints::appview()); + crate::logging::debug::log(format!(">> GET {url} (owner of {repo_did})")); + let client = crate::clients::http::builder() + .redirect(reqwest::redirect::Policy::none()) + .build()?; + let resp = client.get(&url).send().await?; + let location = resp + .headers() + .get(reqwest::header::LOCATION) + .and_then(|l| l.to_str().ok()) + .map(str::to_string); + crate::logging::debug::log(format!("<< {} location: {location:?}", resp.status())); + let Some(location) = location else { + return Ok(None); + }; + // `//`, absolute or relative. The owner is the first + // segment, and it is a handle: the appview redirects to the human + // spelling, which is the whole reason this works. + let path = path_of(&location); + let Some(owner) = path.split('/').find(|seg| !seg.is_empty()) else { + return Ok(None); + }; + // Already a DID in the path is possible and needs no second lookup. + if owner.starts_with("did:") { + return Ok(Some(owner.to_string())); + } + match crate::clients::atproto::did::resolve_handle(owner).await { + Ok(did) => Ok(Some(did)), + Err(e) => { + crate::logging::debug::log(format!( + "{owner} is the owner of {repo_did} but does not resolve: {e:#}" + )); + Ok(None) + } + } +} + pub async fn repo_ref(remote_url: &str) -> Result { let url = normalize(remote_url); let (_, path) = url diff --git a/src/cmd/pr/write.rs b/src/cmd/pr/write.rs index 2375baf..43c259a 100644 --- a/src/cmd/pr/write.rs +++ b/src/cmd/pr/write.rs @@ -2012,6 +2012,54 @@ async fn set_state(args: StateArgs, wanted: PullState) -> Result<()> { statuses.extend(mine.statuses); complete &= mine.complete; } + // **And the repo owner's, which is the account this walk used to be + // blind to.** + // + // `standing_of` above names the two accounts Tangled honors — the pull's + // author and the target repo's owner — and this walk read only the + // first, plus whoever happens to be running the command. So an owner's + // records were invisible to the author, and the state below fell through + // to `Open` for want of a record that existed. + // + // That is not a cosmetic wrong answer. The `merged` refusal a few lines + // down exists because state is a log and the newest record wins, so a + // `closed` written after a `merged` *replaces* it in every reader's + // view. A pull merged by the repo owner — the ordinary way a + // contributor's pull gets merged — carries its `merged` record in the + // owner's PDS, so the author closing it walked straight past the guard + // and hid the merge. `an_owners_merge_is_invisible_to_the_authors_close` + // in `tests/pr_flows.rs` is that sequence. + // Skipped when this account *is* the owner: `standing_of` settled that + // above, and author-plus-acting has then already covered both honored + // writers. Skipped too when the pull names no repo, because then there + // is no owner for Tangled to honor and nothing more to read. + if standing != Standing::RepoOwner + && let Some(repo_did) = repo_did.as_deref() + { + match resolve::owner_of(repo_did).await? { + Some(owner) if owner != target.author && owner != acting => { + let theirs = list_statuses(&owner, &uri, &target.rkey).await?; + statuses.extend(theirs.statuses); + complete &= theirs.complete; + } + // The owner is the author, or is running this: already walked. + Some(_) => {} + // Refused rather than guessed, for the same reason the page cap + // below is. An account that was not read is an absence that + // reads exactly like "nobody has acted on this pull", and acting + // on it means writing over whatever is in there. + None => bail!( + "cannot tell who owns the repo this pull targets, so its current state \ + cannot be settled\n\ + refusing to {verb} it: a status record in the owner's repository — a \ + merge, most of all — is invisible from here, and the newest record is \ + the one every reader believes\n\ + the owner is published by the appview and it did not answer; {verb} it \ + through tangled.org, which reads its own index", + ), + } + } + // **A state read off a short walk is not a state.** Every decision below // turns on the *absence* of a record — "already closed, nothing written" // and the refusal that keeps a merge from being overwritten are both read diff --git a/tests/pr_flows.rs b/tests/pr_flows.rs index cb83a17..cb2a895 100644 --- a/tests/pr_flows.rs +++ b/tests/pr_flows.rs @@ -1771,3 +1771,139 @@ fn a_pull_with_no_status_record_is_open_and_an_unknown_one_is_absent() { ); }); } + +/// **A pull merged by the repo owner cannot be closed by its author.** +/// +/// `pr close` refuses to write over a `merged`, and the comment on that +/// refusal says why: state is a log of records and the newest wins, so a +/// `closed` written after a `merged` does not sit beside it, it replaces it +/// in every reader's view. +/// +/// The guard was right and the state it guarded on was not. Current state +/// came from `list_statuses`, which walked the pull author's PDS and the +/// acting account's — and a merge performed by the repo owner lands in the +/// *owner's* PDS, which is the ordinary way a contributor's pull gets +/// merged. The author saw no status record at all, `state_of` fell through +/// to `Open`, and the guard never fired: atgc printed `open -> closed` and +/// hid the merge. +/// +/// Alice owns the repo and merges; Bob wrote the pull and tries to close it. +#[test] +fn an_owners_merge_is_seen_by_the_authors_close_and_refuses_it() { + let world = Scenario::new("pr-close-hides-owners-merge"); + feature_branch(&world); + let rkey = world + .run_as(BOB, &["pr", "create", "--title", "bob's pull", "--json"]) + .success() + .json()["uri"] + .as_str() + .expect("the uri") + .rsplit('/') + .next() + .expect("a record key") + .to_string(); + let uri = format!("at://{BOB}/{PULL_NSID}/{rkey}"); + + // The owner merges. The `merged` status record is hers, in her PDS. + world.run(&["pr", "merge", &uri]).success(); + world.with(|w| { + assert_eq!( + w.collection(ALICE, PULL_STATUS_NSID).len(), + 1, + "the merge wrote no status record" + ); + assert!( + w.collection(BOB, PULL_STATUS_NSID).is_empty(), + "the merge status landed in the author's PDS" + ); + }); + + // The author now closes it. This must be refused: the pull is merged. + let run = world.run_as(BOB, &["pr", "close", &uri]); + assert_ne!( + run.code, + Some(0), + "the author closed a merged pull\n--- stdout ---\n{}\n--- stderr ---\n{}", + run.stdout, + run.stderr + ); + assert!( + run.stderr.contains("already merged"), + "refused for the wrong reason\n--- stderr ---\n{}", + run.stderr + ); +} + +/// The same blindness in its quieter form: a pull the repo owner closed, +/// reopened by its author. Before the owner's records were read, this +/// printed `already open; nothing written` and exited `0` — a success that +/// asserted a state atgc could not see, against a pull tangled.org showed as +/// closed. Now the reopen is a real write. +#[test] +fn an_author_can_reopen_what_the_repo_owner_closed() { + let world = Scenario::new("pr-reopen-after-owners-close"); + feature_branch(&world); + let rkey = world + .run_as(BOB, &["pr", "create", "--title", "bob's pull", "--json"]) + .success() + .json()["uri"] + .as_str() + .expect("the uri") + .rsplit('/') + .next() + .expect("a record key") + .to_string(); + let uri = format!("at://{BOB}/{PULL_NSID}/{rkey}"); + + world.run(&["pr", "close", &uri]).success(); + let run = world.run_as(BOB, &["pr", "reopen", &uri]).success(); + + assert!( + !run.stdout.contains("nothing written"), + "the reopen was a no-op against a pull the owner had closed\n--- stdout ---\n{}", + run.stdout + ); + assert_eq!( + world.with(|w| w.collection(BOB, PULL_STATUS_NSID).len()), + 1, + "the reopen wrote no status record" + ); +} + +/// An owner that cannot be resolved is refused rather than assumed absent. +/// +/// The appview publishes the repo-DID-to-owner mapping and nothing else +/// does, so when it does not answer, the set of accounts whose records +/// decide this pull's state is unknown. That is the same shape as the page +/// cap below it — an absence that cannot be told apart from "nobody acted" — +/// and it gets the same answer, because the write it would let through can +/// erase a merge. +#[test] +fn an_unresolvable_owner_refuses_the_write() { + let world = Scenario::new("pr-close-owner-unknown"); + feature_branch(&world); + let rkey = world + .run_as(BOB, &["pr", "create", "--title", "bob's pull", "--json"]) + .success() + .json()["uri"] + .as_str() + .expect("the uri") + .rsplit('/') + .next() + .expect("a record key") + .to_string(); + let uri = format!("at://{BOB}/{PULL_NSID}/{rkey}"); + + // The appview forgets who owns the repo. + world.with(|w| { + w.repo_owners.clear(); + }); + + world + .run_as(BOB, &["pr", "close", &uri]) + .refused("cannot tell who owns the repo"); + assert!( + world.with(|w| w.collection(BOB, PULL_STATUS_NSID).is_empty()), + "the refused run wrote a status record anyway" + ); +} diff --git a/tests/support/http.rs b/tests/support/http.rs index 880db00..fd340b2 100644 --- a/tests/support/http.rs +++ b/tests/support/http.rs @@ -70,6 +70,10 @@ pub enum Reply { /// This many bytes, chunked, with no `Content-Length` — a host that /// answers with more than atgc will read. See [`World::flood`]. Flood(usize), + /// A `302` to `location`. The appview publishes the repo-DID-to-owner + /// mapping this way and nowhere else, so a mock that cannot redirect + /// cannot answer the one question `resolve::owner_of` asks. + Redirect(String), } impl Reply { @@ -236,6 +240,11 @@ where .body(Full::new(Bytes::from(body.to_string())).boxed()) .expect("a response") } + Reply::Redirect(location) => Response::builder() + .status(StatusCode::FOUND) + .header(hyper::header::LOCATION, location) + .body(Full::new(Bytes::new()).boxed()) + .expect("a response"), // A 200 with a plausible content type, because the interesting // refusal is the one that happens before anything has been parsed // or the status has been looked at. diff --git a/tests/support/mod.rs b/tests/support/mod.rs index b3f18fe..ccd738b 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -182,6 +182,15 @@ impl Scenario { "createdAt": "2026-01-01T00:00:00Z", }), ); + // The same fact the appview publishes as a redirect from the + // repo's DID. Planted beside the record rather than derived from + // it, because the appview does not read the owner's PDS to + // answer — it answers from its own index, and a command that + // needs the owner has no other route to one. + w.repo_owners.insert( + REPO_DID.to_string(), + (alice.did.clone(), "demo".to_string()), + ); } let checkout = git::Checkout::new(root.path().join("checkout"), &knot, REPO_DID); diff --git a/tests/support/services.rs b/tests/support/services.rs index ecb3e61..994690b 100644 --- a/tests/support/services.rs +++ b/tests/support/services.rs @@ -513,6 +513,25 @@ pub fn appview(world: &mut World, req: &Incoming) -> Reply { { return Reply::Bytes(pull_page(uri).into_bytes()); } + // `/` redirects to `//`, which is how the + // real appview publishes who owns a repo and the only route + // `resolve::owner_of` has to it — a repo's DID document names its knot + // and nothing else. + let path = req.path.trim_matches('/'); + if path.starts_with("did:") + && let Some((owner, name)) = world.repo_owners.get(path) + { + // **The real appview redirects to the owner's *handle*; this uses + // their DID.** `resolve::owner_of` accepts either, and the harness + // has no way to answer a handle: `resolve_handle` is jacquard's and + // goes to DNS and `.well-known` on the real network, neither of + // which a loopback mock can stand in front of. So the half of that + // path these tests exercise is "the appview names an owner and the + // status walk reads their records", and the handle-to-DID hop is + // covered nowhere — worth knowing before trusting a green run here + // to mean the whole lookup works. + return Reply::Redirect(format!("/{owner}/{name}")); + } Reply::not_found(format!("the mock appview has no page at /{}", req.path)) } diff --git a/tests/support/world.rs b/tests/support/world.rs index 4f36d9b..a6d1240 100644 --- a/tests/support/world.rs +++ b/tests/support/world.rs @@ -147,6 +147,12 @@ pub struct World { /// command that resolved the right repo from one that resolved whichever /// repo it was standing in. pub pull_pages: BTreeMap<(String, u32), String>, + /// Repo DID to `(owner handle, repo name)`, which the mock appview + /// serves as the `302` the real one does. There is no other route from a + /// repo's DID to its owner — the DID document names only the knot — so a + /// command that has to read the owner's records depends on this mapping + /// existing. + pub repo_owners: BTreeMap, /// A service that answers every call with this many bytes and no /// `Content-Length`, instead of whatever it was going to say. /// @@ -172,6 +178,7 @@ impl World { dids: BTreeMap::new(), tokens: BTreeMap::new(), journal: Vec::new(), + repo_owners: BTreeMap::new(), // A clean merge unless a test says otherwise; a conflict is the // exceptional case and reads better as one. merge_check: Ok(()),