diff --git a/plan/pull-numbers.md b/plan/pull-numbers.md index a3babb1..1bca35b 100644 --- a/plan/pull-numbers.md +++ b/plan/pull-numbers.md @@ -42,6 +42,23 @@ delete the day a lexicon carries the number. ## Done +- [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 + when it is unique and the walk saw every pull there is to be unique + against. The second half of that rule was never enforced: uniqueness is + asserted over the rows the walk *saw*, and the record being placed need + not be among them. A pull the appview has not indexed yet has no number + at all, so the only row carrying its title belongs to something else — + and a complete walk makes that answer more confident, not less. + Seen live: `stack view` printed `#316` beside a record minted minutes + earlier, having taken the number from a different pull, closed moments + before, that shared its title. Completeness now decides only what may + be *checked*, never what may be printed; more rows read blank than + before and none of them read wrong, which is what a blank has always + meant here. The single-pull path was always right and is unchanged — + it treats the title as a shortlist and confirms every candidate + - [x] Tangled pull *numbers* (`/pulls/23`) are not resolvable by any *query*. The number is `pulls.pull_id`, allocated from a per-repo `repo_pull_seqs` counter inside the appview's own database, and no diff --git a/src/clients/tangled/web/pulls.rs b/src/clients/tangled/web/pulls.rs index d58e973..0209fec 100644 --- a/src/clients/tangled/web/pulls.rs +++ b/src/clients/tangled/web/pulls.rs @@ -12,9 +12,10 @@ //! with the number when it can be found and an honest fallback when it //! cannot. A link that says it is the listing rather than the pull is not a //! failure; a link that claims to be the pull and is not would be. -//! - [`numbers_for_records`] — the same question for a whole listing at once, -//! joined on title, budgeted, and refusing to guess when two pulls share -//! one. +//! - [`numbers_for_records`] — the same question for a whole listing at once: +//! the listings' titles pick which numbers are worth checking, a bounded +//! handful are checked against the records, and anything unchecked stays +//! blank. //! - [`pull_uri_from_appview`] — the other direction: a number to the at-uri //! it names. //! @@ -152,10 +153,11 @@ const MAX_LISTING_PAGES: u32 = 4; /// has not reached the end. /// /// Higher than [`MAX_LISTING_PAGES`], and for a different job. That budget -/// bounds a *search*; this one buys a *proof*: reaching the end of every -/// listing is what turns "this title appears once in what I read" into "this -/// title is unique", which is the whole basis on which a join may be printed -/// (see [`Coverage`]). +/// bounds a search for one pull; this one buys a *shortlist that is not +/// missing anything*. A number the walk never saw can never be confirmed, so +/// a repo whose listings run past this budget loses the numbers of its oldest +/// pulls outright (see [`Coverage`]). It no longer licenses printing anything +/// — nothing does but a confirmation — it only decides what may be checked. /// /// Sixteen pages is 480 pulls per state. It sounds extravagant beside four and /// is not, because the walk stops the moment the listings run dry: a repo with @@ -174,14 +176,23 @@ const MAX_JOIN_LISTING_PAGES: u32 = 16; /// cost a request or two and must never cost a visible wait. const NUMBER_LOOKUP_BUDGET: usize = 4; -/// How many `/pulls/` pages [`numbers_for_records`] will open to place the -/// rows its title join could not. Larger than [`NUMBER_LOOKUP_BUDGET`], -/// because it is spending pages on several rows rather than several guesses at -/// one, and because the pages go out together; small anyway, because a page -/// runs to 2.5 MB and a listing that has to open thirty of them has stopped -/// being a listing. Six covers the worst case this project has produced — four -/// pulls sharing the title "docs: sync atgc usage instructions from the atgc -/// repo" — with room for a second clash beside it. +/// How many `/pulls/` pages [`numbers_for_records`] will open to place its +/// rows — and so, since a confirmation is the only thing that may be printed, +/// how many numbers one listing can show at all. +/// +/// Larger than [`NUMBER_LOOKUP_BUDGET`], because it is spending pages on +/// several rows rather than several guesses at one, and because the pages go +/// out together; small anyway, because a page runs to 2.5 MB and a listing +/// that has to open thirty of them has stopped being a listing. +/// +/// This used to be a cleanup budget, spent only on the rows a title join could +/// not place, and six was chosen against the worst clash this project has +/// produced — four pulls sharing the title "docs: sync atgc usage instructions +/// from the atgc repo". Now that no join may be printed unconfirmed, six is +/// instead the ceiling on a whole column, which is a real loss of decoration +/// on a thirty-row `pr list` and the price of never printing another wrong +/// number. Raising it buys more numbers at megabytes each; the rows it does +/// reach are the newest ones, which is where a reader looks. const LIST_CONFIRM_BUDGET: usize = 6; /// How long to wait for the appview to notice a record atgc has just written. @@ -477,18 +488,22 @@ pub(crate) fn listing_url(web_url: &str, state: &str, offset: u32) -> String { /// How much of a repo's pull listing a walk actually saw. /// -/// The distinction the join rests on, and the one the walk used to keep to -/// itself. "This title appears once in what I read" only means "this title is -/// unique" when what I read was everything. +/// No longer a licence to print: a number reaches the column only by being +/// confirmed against the record's at-URI. What completeness buys is the +/// shortlist — under [`Coverage::Complete`] every number the appview has +/// handed out is available to be checked, and under [`Coverage::Partial`] the +/// right one may be past the edge of the walk and the row stays blank. #[derive(Clone, Copy, Debug, PartialEq, Eq)] enum Coverage { /// Every state's listing ran out inside the page budget, so the walk holds /// every pull this appview has given a number. A title seen once here is - /// unique on the repo, which is what makes a join sound rather than likely. + /// unique among *those* — which says nothing about the record being + /// placed, since a pull the appview has not indexed is on no listing at + /// all and its title's only match belongs to somebody else. Complete, - /// The page budget ran out first. Nothing can be concluded from a title - /// appearing once, because the walk stopped somewhere and the twin may be - /// on the other side of that. + /// The page budget ran out first, or a listing could not be read. The + /// shortlist may be missing the number a row actually wants, so a blank + /// here means "not found" rather than "not confirmed". Partial, } @@ -571,69 +586,107 @@ async fn walk_listings(web_url: &str) -> (Vec<(u32, String)>, Coverage) { /// `pr list` prints thirty rows and cannot pay [`number_for_record`]'s price /// per row — a pull's page is up to 2.5 MB, so confirming thirty of them would /// move tens of megabytes to decorate one listing. This pays for the three -/// listings once instead and joins the two sets on their titles, then spends a -/// bounded handful of pages on the rows that join could not place. +/// listings once instead, uses their titles to work out which numbers are +/// worth a page, and then spends a bounded handful of pages actually checking +/// them. /// -/// The join is unverified, so it is deliberately timid: a record gets a number -/// only when its title matches exactly one pull on the listings *and* no other -/// record on this page carries that title. Both halves are needed — four of -/// this project's own pulls are called "docs: sync atgc usage instructions -/// from the atgc repo", and guessing which is which would put a wrong number -/// against three of them. +/// # What may be printed /// -/// Those four are what the second pass is for. Sharing a title makes a row -/// impossible to *join* and no harder to *confirm*: the listings still state -/// which numbers are in the running, and opening one of their pages says -/// exactly which record is on it. That is a page per candidate, which is why -/// it runs on the leftovers rather than on the whole listing and why it stops -/// at [`LIST_CONFIRM_BUDGET`]. +/// A number is a fact about a *record*, and exactly one thing establishes +/// one: **a confirmation**. The pull's own page states its at-URI, so opening +/// `/pulls/` settles which record is number `n` — [`place_confirmed`]. +/// Everything else is a hypothesis, a hypothesis is not printed, and if the +/// budget does not reach one the column stays blank, which is what a blank +/// has always meant here. /// -/// # What may be printed +/// A title is not a key. It is lossy — the listing renders Markdown and +/// escapes HTML, so it does not equal the title in the record — and it +/// repeats: four of this project's own pulls are called "docs: sync atgc +/// usage instructions from the atgc repo". So [`join_on_title`] is kept, but +/// only as a shortlist: it says which numbers are worth opening, and never +/// which record a number belongs to. +/// +/// # Why a unique title on a complete walk is still not evidence /// -/// A number is a fact about a *record*, and only two things establish one: +/// This function used to print a join outright whenever the walk had reached +/// [`Coverage::Complete`], on the rule "a match is evidence when it is unique +/// *and* the walk saw every pull there is to be unique against". The second +/// half of that rule was never enforced, because it is not the check that was +/// written: uniqueness is asserted over the pulls the walk *saw*, and the +/// record being placed need not be among them. /// -/// 1. **A confirmation.** The pull's own page states its at-URI, so opening -/// `/pulls/` settles which record is number `n`. Authoritative, and it -/// outranks everything below — [`place_confirmed`]. -/// 2. **A join over a [`Coverage::Complete`] walk.** A listing states numbers -/// and rendered titles, and a title is not a key: it is lossy (Markdown and -/// HTML escaping) and repeats. Matching one is only evidence when the match -/// is unique *and* the walk saw every pull there is to be unique against. +/// A pull the appview has not indexed yet has no number at all. It appears on +/// no listing, so the one row carrying its title belongs to something else — +/// an older namesake, quite possibly a closed one — and a complete walk makes +/// that wrong answer *more* confident rather than less, since completeness is +/// exactly what used to license printing it. Observed on 2026-08-26: +/// `stack view` printed `#316` beside a pull whose record key was minutes +/// old, and #316 was a different pull, closed minutes earlier, that happened +/// to share the title. The earlier fix, for the two pulls that swapped +/// numbers between consecutive `pr list` runs (#194 and #197, 2026-08-14), +/// removed the walk's early stop and this rule survived it. /// -/// Anything else is a hypothesis, and a hypothesis is not printed. It is used -/// to pick which numbers are worth confirming, and if the budget does not -/// reach it the column stays blank — which is what a blank has always meant. +/// # The cost of the fix /// -/// This is the rule the function did not follow. It joined on titles read from -/// a walk that stopped as soon as every row had a guess, so "unique here" was -/// asserted over a subset chosen for ending early, and a twin past that edge -/// was invisible by construction. Worse, [`candidates_to_confirm`]'s ancestor -/// only ever shortlisted rows the join left *blank*, so a wrongly placed row -/// was never revisited. Two pulls sharing a title swapped numbers between -/// consecutive runs of `pr list` — #194 and #197 on this repo, 2026-08-14. +/// Confirming is a page per candidate and the page is up to 2.5 MB, so +/// [`LIST_CONFIRM_BUDGET`] now caps how many numbers a listing can show at +/// all, rather than how many stragglers it can clean up. **More rows read +/// blank than before, and none of them read wrong** — on a thirty-row +/// `pr list` most of the column is now empty, where before it was full and +/// occasionally lying. The budget goes to the rows a reader looks at first +/// (see [`candidates_to_confirm`]), and the single-pull path — `pr view`, +/// `pr create` — still numbers its one pull every time. /// -/// [`number_for_record`], the one-pull path, never had the bug: it confirms -/// every candidate against the at-URI and treats the title as a shortlist, -/// which is exactly the discipline restored here. +/// [`number_for_record`], the one-pull path, never had any of this wrong: it +/// treats the title as a shortlist and confirms every candidate against the +/// at-URI, which is the discipline this now follows too. pub async fn numbers_for_records(web_url: &str, records: &[(&str, &str)]) -> HashMap { let (listed, coverage) = walk_listings(web_url).await; - let joined = join_on_title(&listed, records); + let (mut found, candidates) = plan_from_walk(&listed, records, coverage); - // Under a partial walk the join is a shortlist and nothing more, so it - // starts empty and only confirmations can fill it. - let mut found = match coverage { - Coverage::Complete => joined.clone(), - Coverage::Partial => { - crate::logging::debug::log(format!( - "pull numbers: {} title match(es) held back as unproven; \ - the listings were not read to the end", - joined.len() - )); - HashMap::new() + for (number, uri) in confirm_numbers(web_url, &candidates).await { + // A candidate can confirm as a pull that is not on this page at all — + // the twin of a row, filed under a state this listing is not showing, + // or the older namesake a row would have been given. It answers a + // question nobody asked and is dropped. + if records.iter().any(|(row, _)| *row == uri) { + crate::logging::debug::log(format!("pull number confirm: {uri} is #{number}")); + place_confirmed(&mut found, uri, number); } - }; + } + found +} + +/// The pure half of [`numbers_for_records`]: what one walk of the listings +/// settles about these records, and what it can only nominate. +/// +/// Returns the numbers that may be printed on the strength of the walk alone +/// — none, ever — and the numbers worth opening a pull page for, best first +/// and already cut to [`LIST_CONFIRM_BUDGET`]. +/// +/// The empty map is returned rather than assumed because a non-empty one is +/// the whole defect: every wrong number this column has printed was decided +/// here, from a title match over a walk that could not see the record it was +/// placing. Keeping the decision in a function that takes rows, records and +/// coverage and touches no socket is what lets a test pin it. +fn plan_from_walk( + listed: &[(u32, String)], + records: &[(&str, &str)], + coverage: Coverage, +) -> (HashMap, Vec) { + // A shortlist, and nothing more. Under `Partial` it may not even contain + // the right number, since the walk stopped with listings still unread. + let joined = join_on_title(listed, records); + let found = HashMap::new(); + if !joined.is_empty() { + crate::logging::debug::log(format!( + "pull numbers: {} title match(es) held back as unproven ({coverage:?} walk); \ + a number prints once a pull page names the record", + joined.len() + )); + } - let mut candidates = candidates_to_confirm(&listed, records, &joined, &found); + let mut candidates = candidates_to_confirm(listed, records, &joined, &found); if candidates.len() > LIST_CONFIRM_BUDGET { crate::logging::debug::log(format!( "pull number confirm: {} rows to place, opening the newest {LIST_CONFIRM_BUDGET}; \ @@ -642,47 +695,39 @@ pub async fn numbers_for_records(web_url: &str, records: &[(&str, &str)]) -> Has )); candidates.truncate(LIST_CONFIRM_BUDGET); } - for (number, uri) in confirm_numbers(web_url, &candidates).await { - // A candidate can confirm as a pull that is not on this page at all — - // the twin of a row, filed under a state this listing is not showing. - // It answers a question nobody asked and is dropped. - if records.iter().any(|(row, _)| *row == uri) { - crate::logging::debug::log(format!("pull number confirm: {uri} is #{number}")); - place_confirmed(&mut found, uri, number); - } - } - found + (found, candidates) } -/// Record a confirmed number, evicting whatever the join had said instead. +/// Record a confirmed number, evicting anything else holding it. /// -/// Two evictions, not one. The row itself may have been joined to a different -/// number, and some *other* row may have been joined to this one; a number -/// names exactly one pull, so both of those are now known to be wrong. Leaving -/// the second in place would print the same `#9` against two rows, which reads -/// as a bug in the column rather than as the stale guess it is. +/// A number names exactly one pull, so a row that already has this one is +/// wrong by construction and printing the same `#9` against two rows would +/// read as a bug in the column. Nothing but a confirmation can put a number in +/// the map any more, so this guard should never fire; it is kept because the +/// alternative to it firing is a duplicated number in the output. fn place_confirmed(found: &mut HashMap, uri: String, number: u32) { found.retain(|_, placed| *placed != number); found.insert(uri, number); } -/// Which listed numbers are worth a page, to place the rows the join could not. +/// Which listed numbers are worth a page, to place the rows of this listing. /// -/// The same title hint the join refused to act on, used here as what it is: a -/// shortlist. A number earns a page when some still-blank row on this listing -/// wears its title and no row has already been given it. Newest first, because -/// a page of pull requests is read from the top and the budget may not reach -/// the bottom. +/// A number earns a page when some still-blank row on this listing wears its +/// title and no row has already been given it. Every row is still blank when +/// this runs — nothing prints unconfirmed — so this is now the whole column's +/// shortlist rather than a cleanup pass over the leftovers. Newest first, +/// because a page of pull requests is read from the top and the budget may +/// not reach the bottom. fn candidates_to_confirm( listed: &[(u32, String)], records: &[(&str, &str)], joined: &HashMap, found: &HashMap, ) -> Vec { - // Every record without a number that may be printed — which under a - // partial walk is all of them, joined or not. The old version asked only - // for rows the join left blank, so the rows it had placed *wrongly* were - // the exact ones it never offered up for checking. + // Every record without a number that may be printed — which, now that a + // join is never printed, is every record that has not already been + // confirmed. An older version asked only for rows the join left blank, so + // the rows it had placed *wrongly* were the exact ones it never checked. let wanted: Vec<&str> = records .iter() .filter(|(uri, _)| !found.contains_key(*uri)) @@ -731,7 +776,13 @@ async fn confirm_numbers(web_url: &str, numbers: &[u32]) -> Vec<(u32, String)> { confirmed } -/// The pure half of [`numbers_for_records`]: which record is which number. +/// Which listed number each record's title points at, where a title points at +/// one thing on both sides. +/// +/// A shortlist, not an answer: see [`numbers_for_records`] for why a unique +/// title match is not evidence about a record even when the walk was complete. +/// It is kept because it ranks the candidates — the number a row's title picks +/// out is the number most worth spending a page on. fn join_on_title(listed: &[(u32, String)], records: &[(&str, &str)]) -> HashMap { let mut found = HashMap::new(); for (uri, title) in records { @@ -1288,10 +1339,60 @@ mod tests { ); } - /// Under a complete walk the join is evidence, so its rows are settled and - /// no page is opened for them. That is what keeps the common case free. + /// The defect this file was rewritten around, and the one a complete walk + /// used to make *worse*. + /// + /// The record here is a pull the appview has not indexed yet: it is on no + /// listing, because it has no number to be listed under. The only listed + /// pull wearing its title is #316 — a different pull, closed minutes + /// earlier — and a unique title match over a walk that read every listing + /// to the end is exactly the condition that used to license printing it. + /// `stack view` printed `#316` beside a record whose key was minutes old + /// on 2026-08-26. So: nothing may be printed, and #316 is a candidate to + /// be checked rather than an answer. + #[test] + fn a_complete_walk_cannot_place_a_record_it_has_not_seen() { + let listed = vec![ + ( + 316, + "fix(stack): order a chain past its closed members".to_string(), + ), + ( + 315, + "docs(plan): record what the stack tests cover".to_string(), + ), + ]; + let rows = [( + // Written moments ago, so no listing mentions it. + "at://did:plc:a/sh.tangled.repo.pull/3lqbrandnew", + "fix(stack): order a chain past its closed members", + )]; + + // The title join is as confident as it ever gets: one record, one + // listed pull, and a walk with nothing left to read. + assert_eq!( + join_on_title(&listed, &rows).get(rows[0].0), + Some(&316), + "the join still points at #316; the point is that it is not printed" + ); + + let (printable, shortlist) = plan_from_walk(&listed, &rows, Coverage::Complete); + assert!( + printable.is_empty(), + "a walk alone places nothing, however complete: {printable:?}" + ); + assert_eq!( + shortlist, + vec![316], + "the title is a shortlist, and #316 is the page worth opening" + ); + } + /// The stated cost of that rule, pinned so it is not mistaken for a + /// regression: rows a complete walk joins cleanly, which used to print + /// free, now each cost a pull page. Two clean rows, two candidates, and + /// the column is blank until those pages come back. #[test] - fn a_complete_walk_settles_unique_titles_without_opening_a_page() { + fn a_complete_walk_settles_nothing_without_opening_a_page() { let listed = vec![ (9, "Flesh out .gitignore".to_string()), (8, "Add repo list".to_string()), @@ -1303,14 +1404,58 @@ mod tests { ), ("at://did:plc:a/sh.tangled.repo.pull/z", "Add repo list"), ]; - let joined = join_on_title(&listed, &rows); - assert_eq!(joined.len(), 2); - // `found` starts as the join under Coverage::Complete, so nothing is - // left wanting and the confirm pass has nothing to do. - assert!( - candidates_to_confirm(&listed, &rows, &joined, &joined).is_empty(), - "a settled listing should cost no pull pages" - ); + assert_eq!(join_on_title(&listed, &rows).len(), 2); + + let (printable, shortlist) = plan_from_walk(&listed, &rows, Coverage::Complete); + assert!(printable.is_empty(), "{printable:?}"); + assert_eq!(shortlist, vec![9, 8], "newest first"); + } + /// A listing wider than the confirm budget keeps the newest rows' + /// numbers and leaves the rest blank, rather than filling the tail in + /// with unconfirmed joins. The budget is now the ceiling on how many + /// numbers a column can carry, so it is where the loss lands. + #[test] + fn the_shortlist_stops_at_the_confirm_budget() { + let listed: Vec<(u32, String)> = (1..=10).rev().map(|n| (n, format!("pull {n}"))).collect(); + let owned: Vec<(String, String)> = (1..=10) + .rev() + .map(|n| { + ( + format!("at://did:plc:a/sh.tangled.repo.pull/p{n}"), + format!("pull {n}"), + ) + }) + .collect(); + let rows: Vec<(&str, &str)> = owned + .iter() + .map(|(uri, title)| (uri.as_str(), title.as_str())) + .collect(); + + let (printable, shortlist) = plan_from_walk(&listed, &rows, Coverage::Complete); + assert!(printable.is_empty(), "{printable:?}"); + assert_eq!(shortlist.len(), LIST_CONFIRM_BUDGET); + assert_eq!(shortlist, vec![10, 9, 8, 7, 6, 5], "the newest six"); + } + /// A walk that stopped early cannot even shortlist what it did not read, + /// so a row whose pull is past the edge stays blank and costs no page. + /// The rows it did see are candidates like any other — a partial walk is + /// not a worse kind of evidence here, because none of it was evidence. + #[test] + fn a_partial_walk_shortlists_only_what_it_read() { + let listed = vec![(9, "Flesh out .gitignore".to_string())]; + let rows = [ + ( + "at://did:plc:a/sh.tangled.repo.pull/x", + "Flesh out .gitignore", + ), + ( + "at://did:plc:a/sh.tangled.repo.pull/older", + "A pull past the edge of the walk", + ), + ]; + let (printable, shortlist) = plan_from_walk(&listed, &rows, Coverage::Partial); + assert!(printable.is_empty(), "{printable:?}"); + assert_eq!(shortlist, vec![9]); } /// A confirmation is ground truth and the join is a guess, so landing one /// takes the number off whichever row had been guessing at it. Without the