From 63a81a3e7fa82d08996bb4e09da831d21836fbca Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Sat, 15 Aug 2026 17:22:51 -0400 Subject: [PATCH] fix(issue): settle state on a parsed instant, not on the raw string MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `newest_state` compared `createdAt` as text, and the records it orders come from two accounts: `state_of` merges the issue author's with the acting account's, which is the whole point of it and is what makes a repo owner's close visible beside the author's own. Two accounts is two clients and two stamp formats — a reopen at `12:00:00-03:00` is an hour after a close at `14:00:00Z` and sorts before it as text, and a fractional second sorts before a bare `Z`. The appview orders by `created_micros desc, at_uri desc`, so a string compare here reports a state tangled.org does not show. TODO.md's note on this hazard exempts the pull half because it reads one PDS; this inherited the exemption by being written beside it, and the note now says so. Co-Authored-By: Claude Opus 5 (1M context) Change-Id: I5ee0abb967c8b5f0c699c8c87bf733e0cac158ca --- TODO.md | 9 +++-- src/cmd/issue/read.rs | 83 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 87 insertions(+), 5 deletions(-) diff --git a/TODO.md b/TODO.md index c26d53f..1747152 100644 --- a/TODO.md +++ b/TODO.md @@ -838,9 +838,12 @@ `Listed::instant` two hundred lines above exists precisely because that does not work: PDS records stamp UTC and an index's come back in whatever offset the writer used, and `…Z` and `…+03:00` do not sort - lexically against each other. The sibling comparisons in `state_of` and - `latest_states` are fine and should stay — they read one PDS, so every - stamp shares a writer and a format + lexically against each other. The sibling comparisons in `pr`'s + `state_of` and `latest_states` are fine and should stay — they read one + PDS, so every stamp shares a writer and a format. `cmd/issue/read.rs`'s + `newest_state` is *not* covered by that exemption and inherited it by + being written beside them: it merges the author's records with the + acting account's, which is two writers, so it parses ## stack (stacked pull requests) A Tangled stack is a chain of ordinary pull records, one per *commit*, each diff --git a/src/cmd/issue/read.rs b/src/cmd/issue/read.rs index 56ef529..eb25e6b 100644 --- a/src/cmd/issue/read.rs +++ b/src/cmd/issue/read.rs @@ -284,6 +284,32 @@ pub(super) struct StateEvent { state: String, } +impl StateEvent { + /// When this record says it was written, as an instant rather than as the + /// string it was written in. + /// + /// The same reason [`Listed::instant`] exists, arriving from a direction + /// TODO.md's note on this hazard explicitly carved out: it says the + /// comparisons in `state_of` are safe *because they read one PDS, so + /// every stamp shares a writer and a format*. That is true of the pull + /// half and false here — [`state_of`] merges the author's records with + /// the acting account's, which is the whole point of it, and those are + /// two accounts running two clients. `…Z`, `…+03:00` and `…000Z` do not + /// sort lexically against each other, so a repo owner's reopen written + /// at `12:00:00-03:00` loses to the author's earlier close at + /// `13:00:00Z` on a string compare and wins on an instant. + /// + /// The appview settles it on an instant too — `stateWinner` orders by + /// `created_micros desc, at_uri desc` — and the two have to agree or + /// `issue view` reports a state tangled.org does not show. + /// + /// `None` for a stamp that will not parse, which sorts below everything + /// dated rather than winning. + fn instant(&self) -> Option { + Datetime::from_str(&self.created_at).ok() + } +} + /// An issue's state, and whether anything available could settle it. /// /// `Unknown` is deliberately not `open`. Open is the lexicon's default for an @@ -402,6 +428,9 @@ async fn state_events(did: &str, issue_uri: &str, issue_rkey: &str) -> Result Option { .iter() .filter(|e| IssueState::from_token(&e.state).is_some()) .max_by(|a, b| { - a.created_at - .cmp(&b.created_at) + a.instant() + .cmp(&b.instant()) .then_with(|| a.uri.cmp(&b.uri)) }) .and_then(|e| IssueState::from_token(&e.state)) @@ -1123,6 +1152,56 @@ mod tests { assert_eq!(newest_state(&[higher, lower]), Some(IssueState::Open)); } + /// **The records being ordered come from two accounts, so the raw + /// strings will not do.** + /// + /// [`state_of`] merges the issue author's state records with the acting + /// account's — that is the whole point of it, and it is what makes a repo + /// owner's close visible next to the author's own. Two accounts means two + /// clients and two stamp formats, and `…Z`, `…±hh:mm` and a fractional + /// second do not sort lexically against each other: a reopen at + /// `12:00:00-03:00` is fifteen hundred seconds *after* a close at + /// `13:00:00Z` and sorts before it as text. The appview settles this on + /// `created_micros`, so a string compare here reports a state tangled.org + /// does not show. + /// + /// TODO.md's note on this hazard exempts the pull half's comparisons + /// because they read one PDS. This one does not, and that exemption was + /// carried over with the code. + #[test] + fn a_later_record_wins_however_its_writer_spelled_the_offset() { + // 12:00-03:00 is 15:00Z, an hour after the close. + let closed = event("2026-08-14T14:00:00Z", "aaa", IssueState::Closed.token()); + let reopened = event("2026-08-14T12:00:00-03:00", "bbb", IssueState::Open.token()); + assert_eq!( + newest_state(&[closed.clone(), reopened.clone()]), + Some(IssueState::Open), + "a string compare puts 12: before 14: and loses the reopen" + ); + assert_eq!(newest_state(&[reopened, closed]), Some(IssueState::Open)); + + // The other direction, so the test cannot pass by preferring `open`: + // 18:00+02:00 is 16:00Z, an hour after the reopen. + let reopened = event("2026-08-14T15:00:00Z", "aaa", IssueState::Open.token()); + let closed = event( + "2026-08-14T18:00:00+02:00", + "bbb", + IssueState::Closed.token(), + ); + assert_eq!(newest_state(&[reopened, closed]), Some(IssueState::Closed)); + + // A fractional second sorts before `Z` as text and after it in time. + let closed = event("2026-08-14T20:00:00Z", "aaa", IssueState::Closed.token()); + let reopened = event("2026-08-14T20:00:00.500Z", "bbb", IssueState::Open.token()); + assert_eq!(newest_state(&[closed, reopened]), Some(IssueState::Open)); + + // A stamp that will not parse loses to one that does, rather than + // winning on whatever it sorts as. + let closed = event("2026-08-14T10:00:00Z", "aaa", IssueState::Closed.token()); + let junk = event("yesterday", "zzz", IssueState::Open.token()); + assert_eq!(newest_state(&[closed, junk]), Some(IssueState::Closed)); + } + /// A state variant this build has never heard of is skipped rather than /// allowed to win, and a set containing only unknown variants settles /// nothing — which is a different answer from "open", and the difference -- 2.51.2