From 8fed600356dafed71a93e62b981cbde9a31ec74c Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Fri, 28 Aug 2026 12:33:13 -0400 Subject: [PATCH] fix(pr)!: read the repo owner's status records before writing state A merge performed by the repo owner lives in the owner's PDS, which the status walk never read, so the author closing that pull walked past the merged guard and hid the merge. The owner is resolved through the appview, and an owner that will not resolve is refused rather than assumed absent. Change-Id: I5df7fdb9dbe0d95f965a7f9298376481e1ae3c8f --- plan/pull-requests.md | 30 ++++++++ src/clients/tangled/resolve.rs | 59 ++++++++++++++ src/cmd/pr/write.rs | 48 ++++++++++++ tests/pr_flows.rs | 136 +++++++++++++++++++++++++++++++++ tests/support/http.rs | 9 +++ tests/support/mod.rs | 9 +++ tests/support/services.rs | 19 +++++ tests/support/world.rs | 7 ++ 8 files changed, 317 insertions(+) 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(()), -- 2.51.2