From 1373b96873cfc4e626c3b2de9e7ab002365afca9 Mon Sep 17 00:00:00 2001 From: Mayfield Date: Fri, 28 Aug 2026 16:48:45 -0400 Subject: [PATCH 1/2] =?UTF-8?q?feat(github):=20wayfinder=20map=20transport?= =?UTF-8?q?=20=E2=80=94=20GraphQL=20whole-map=20read,=20REST=20issue=20ref?= =?UTF-8?q?resh?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements #117 (spec §4). fetch_wayfinder_map runs the spec's verbatim one-query map read (cost 10, flat in map size) and normalizes straight into ticket::Effort: claims from assignees, Type from the wayfinder: label, dependencies from the blockedBy list (same-effort / external / unknown), never the eventually-consistent summary counters. get_issue / list_issues_with_label cover the single-ticket refresh and label listing; the §4.4 fallback dialect is detected (task-list map bodies, leading Part of / Blocked by lines) but never parsed into the model. --- src/github/graphql.rs | 452 +++++++++++++++++++++++++++++++++++- src/github/graphql/tests.rs | 315 ++++++++++++++++++++++++- src/github/rest.rs | 130 ++++++++++- src/github/rest/tests.rs | 108 ++++++++- src/github/types.rs | 93 ++++++++ src/ticket.rs | 3 +- 6 files changed, 1089 insertions(+), 12 deletions(-) diff --git a/src/github/graphql.rs b/src/github/graphql.rs index 5bdd515..a34b3ea 100644 --- a/src/github/graphql.rs +++ b/src/github/graphql.rs @@ -8,7 +8,7 @@ //! posture as `github::rest::parse_pr`. The `GraphQlClient` owns only the HTTP; //! concurrency limiting happens at the command layer. -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use anyhow::{Context, Result}; use chrono::{DateTime, Utc}; @@ -16,9 +16,12 @@ use serde::Deserialize; use serde_json::json; use crate::{ - github::types::{FullReviewThread, PRState, ReviewComment, ReviewStatus, is_bot}, + github::types::{FullReviewThread, IssueState, PRState, ReviewComment, ReviewStatus, is_bot}, repo_slug::RepoSlug, secret::Secret, + ticket::{ + Claim, Dependency, Effort, EffortKey, ExternalDependency, Ticket, TicketKey, TicketType, + }, }; const GITHUB_GRAPHQL_URL: &str = "https://api.github.com/graphql"; @@ -328,6 +331,390 @@ fn parse_thread_comment(comment: RawThreadComment, bot_logins: &[String]) -> Rev } } +// ── wayfinder map read ─────────────────────────────────────────────────────── + +#[derive(Debug, Deserialize)] +pub struct WayfinderMapResponse { + #[serde(default)] + data: Option, + #[serde(default)] + errors: Vec, +} + +impl GraphQlErrors for WayfinderMapResponse { + fn errors(&self) -> &[GraphQlError] { + &self.errors + } +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct WayfinderMapData { + #[serde(default)] + rate_limit: Option, + #[serde(default)] + repository: Option, +} + +#[derive(Debug, Deserialize)] +struct RawRateLimit { + #[serde(default)] + cost: u64, + #[serde(default)] + remaining: u64, +} + +#[derive(Debug, Deserialize)] +struct WayfinderMapRepo { + #[serde(default)] + issues: Option, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct RawMapConnection { + page_info: RawPageInfo, + #[serde(default)] + nodes: Vec, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct RawMapNode { + number: u64, + #[serde(default)] + title: String, + #[serde(default)] + body: String, + #[serde(default)] + sub_issues: Option, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct RawTicketConnection { + page_info: RawPageInfo, + #[serde(default)] + nodes: Vec, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct RawTicketNode { + number: u64, + #[serde(default)] + title: String, + #[serde(default)] + state: Option, + #[serde(default)] + assignees: Option, + #[serde(default)] + labels: Option, + #[serde(default)] + blocked_by: Option, +} + +#[derive(Debug, Deserialize)] +struct RawLoginConnection { + #[serde(default)] + nodes: Vec, +} + +#[derive(Debug, Deserialize)] +struct RawLogin { + login: String, +} + +#[derive(Debug, Deserialize)] +struct RawLabelConnection { + #[serde(default)] + nodes: Vec, +} + +#[derive(Debug, Deserialize)] +struct RawLabelName { + name: String, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct RawBlockerConnection { + page_info: RawPageInfo, + #[serde(default)] + nodes: Vec, +} + +#[derive(Debug, Deserialize)] +struct RawBlockerNode { + number: u64, + #[serde(default)] + state: Option, + #[serde(default)] + title: Option, + /// The blocker's home repo. Read from the payload, never assumed: a + /// blocker in another repo is an External Dependency. + #[serde(default)] + repository: Option, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct RawRepoName { + name_with_owner: String, +} + +/// One Effort normalized out of the whole-map read, plus its §4.4 degradation +/// signal. +// TODO(#120): remove once the fetch layer consumes map reads. +#[allow(dead_code)] +#[derive(Debug)] +pub struct WayfinderMapRead { + pub effort: Effort, + /// The map has zero native sub-issues but its body carries a task list — + /// the body-line fallback dialect, which v1 detects but never parses. The + /// effort card renders a degradation notice instead of silently showing + /// an empty Effort. + pub fallback_dialect: bool, +} + +/// Every open wayfinder map in one repo — the result of one map read. +// TODO(#120): remove once the fetch layer consumes map reads. +#[allow(dead_code)] +#[derive(Debug)] +pub struct WayfinderMapPage { + pub maps: Vec, + /// The repo has more open maps than the query's `first:10` window. The + /// query is fixed (spec §4.1), so the surplus is reported, not fetched. + pub has_more_maps: bool, +} + +/// Parse a whole-map response into normalized Efforts. Blocked-ness inputs +/// come from each ticket's `blockedBy` list (filtered on state at derivation +/// time) — never from the eventually-consistent `issueDependenciesSummary` +/// counters, which this parse doesn't even read. +fn parse_wayfinder_maps( + response: WayfinderMapResponse, + slug: &RepoSlug, +) -> Result { + let connection = response + .data + .and_then(|d| d.repository) + .and_then(|r| r.issues); + let Some(connection) = connection else { + return Ok(WayfinderMapPage { + maps: Vec::new(), + has_more_maps: false, + }); + }; + let maps = connection + .nodes + .into_iter() + .map(|node| parse_map_node(node, slug)) + .collect::>>()?; + Ok(WayfinderMapPage { + maps, + has_more_maps: connection.page_info.has_next_page, + }) +} + +fn parse_map_node(node: RawMapNode, slug: &RepoSlug) -> Result { + // `first:100` is the hard sub-issue cap per parent, so a next page + // "can't" exist; if it ever does, say so rather than silently showing a + // partial Effort. + if node + .sub_issues + .as_ref() + .is_some_and(|c| c.page_info.has_next_page) + { + tracing::warn!(map = node.number, "sub-issue list truncated at 100"); + } + let ticket_nodes = node.sub_issues.map(|c| c.nodes).unwrap_or_default(); + // Same-effort membership is "is a sub-issue of this map", not "lives in + // this repo": a same-repo blocker outside the map stays External. + let members: HashSet = ticket_nodes.iter().map(|t| t.number).collect(); + let tickets: Vec = ticket_nodes + .into_iter() + .map(|t| parse_ticket_node(t, slug, &members)) + .collect(); + let fallback_dialect = tickets.is_empty() && body_has_task_list(&node.body); + let effort = Effort::new( + EffortKey::GitHub { + repo_slug: slug.clone(), + map_number: node.number, + }, + node.title, + destination_from_map_body(&node.body), + tickets, + ) + .with_context(|| format!("normalizing map {slug}#{}", node.number))?; + Ok(WayfinderMapRead { + effort, + fallback_dialect, + }) +} + +fn parse_ticket_node(node: RawTicketNode, slug: &RepoSlug, members: &HashSet) -> Ticket { + let claim = node + .assignees + .into_iter() + .flat_map(|c| c.nodes) + .next() + .map(|assignee| Claim::By(assignee.login)); + // The Type is the `wayfinder:` label, prefix stripped; a ticket + // without one gets the empty Type (shown verbatim, Mode Either). Other + // labels (triage vocabulary) never masquerade as a Type. + let ty = node + .labels + .into_iter() + .flat_map(|c| c.nodes) + .find_map(|label| label.name.strip_prefix("wayfinder:").map(str::to_owned)) + .map(TicketType) + .unwrap_or_else(|| TicketType(String::new())); + let mut dependencies = Vec::new(); + if let Some(connection) = node.blocked_by { + dependencies.extend( + connection + .nodes + .into_iter() + .map(|blocker| parse_blocker(blocker, slug, members)), + ); + // `first:50` is GitHub's hard relation cap, so this "can't" be true — + // but unseen blockers must never put a ticket on the Frontier, so a + // truncated list degrades to an Unknown Dependency. + if connection.page_info.has_next_page { + dependencies.push(Dependency::Unknown { + raw: "blockers beyond the first 50".to_owned(), + }); + } + } + Ticket { + key: TicketKey::GitHub { + repo_slug: slug.clone(), + number: node.number, + }, + title: node.title, + state: IssueState::parse(node.state.as_deref()).into(), + claim, + ty, + dependencies, + } +} + +fn parse_blocker(node: RawBlockerNode, slug: &RepoSlug, members: &HashSet) -> Dependency { + let repo = match node.repository { + Some(repo) => match RepoSlug::parse(&repo.name_with_owner) { + Ok(parsed) => parsed, + Err(_) => { + return Dependency::Unknown { + raw: format!("{}#{}", repo.name_with_owner, node.number), + }; + } + }, + None => { + return Dependency::Unknown { + raw: format!("#{}", node.number), + }; + } + }; + if repo == *slug && members.contains(&node.number) { + // Key on the map's slug so the edge is byte-identical to its target + // ticket's key (RepoSlug equality is case-insensitive anyway). + Dependency::SameEffort(TicketKey::GitHub { + repo_slug: slug.clone(), + number: node.number, + }) + } else { + // Outside this Effort — another repo, or a same-repo issue that isn't + // one of this map's sub-issues. The payload's state and title are + // captured so the closed/open signal survives without another fetch. + Dependency::External(ExternalDependency { + key: TicketKey::GitHub { + repo_slug: repo, + number: node.number, + }, + state: IssueState::parse(node.state.as_deref()).into(), + title: node.title, + }) + } +} + +/// The first paragraph under the map body's `## Destination` heading — the +/// one-liner the effort card and ticket detail header show. Wrapped lines are +/// joined; `None` when the body has no such heading or the section is empty. +fn destination_from_map_body(body: &str) -> Option { + let mut lines = body.lines(); + lines.by_ref().find(|line| { + line.trim() + .strip_prefix("##") + .is_some_and(|rest| rest.trim().eq_ignore_ascii_case("destination")) + })?; + let mut paragraph: Vec<&str> = Vec::new(); + for line in lines { + let trimmed = line.trim(); + if trimmed.starts_with('#') { + break; + } + if trimmed.is_empty() { + if paragraph.is_empty() { + continue; + } + break; + } + paragraph.push(trimmed); + } + (!paragraph.is_empty()).then(|| paragraph.join(" ")) +} + +/// Whether a map body carries a GitHub task list — half of the §4.4 fallback +/// signal (with zero native sub-issues). +fn body_has_task_list(body: &str) -> bool { + body.lines().any(|line| { + let Some(rest) = line.trim_start().strip_prefix(['-', '*', '+']) else { + return false; + }; + let rest = rest.trim_start(); + rest.starts_with("[ ]") || rest.starts_with("[x]") || rest.starts_with("[X]") + }) +} + +/// Whether a ticket body opens with the fallback dialect's dependency lines +/// (`Part of #n` / `Blocked by: #n`) — the other half of the §4.4 signal, +/// checked when a ticket body arrives (drill-in / single-ticket refresh). +/// Only leading lines count, stopping at the first `##` heading, so refs in +/// prose or code fences don't match; a bare "blocked by" with no issue ref +/// doesn't either. Detection only — the lines are never parsed into the +/// model, and where native data exists a stale `Part of` line is advisory. +// TODO(#120): remove once the per-ticket fetch checks arriving bodies. +#[allow(dead_code)] +pub fn has_fallback_dependency_lines(body: &str) -> bool { + for line in body.lines() { + let trimmed = line.trim(); + if trimmed.starts_with("##") { + break; + } + let lower = trimmed.to_ascii_lowercase(); + let is_dependency_line = lower.starts_with("part of ") || lower.starts_with("blocked by:"); + if is_dependency_line && line_has_issue_ref(trimmed) { + return true; + } + } + false +} + +/// An issue ref in any of the fallback dialect's accepted forms: `#123`, +/// `owner/repo#123`, or a github.com URL. +fn line_has_issue_ref(line: &str) -> bool { + if line.contains("github.com/") { + return true; + } + line.match_indices('#').any(|(i, _)| { + line[i + 1..] + .chars() + .next() + .is_some_and(|c| c.is_ascii_digit()) + }) +} + // ── transport ──────────────────────────────────────────────────────────────── #[derive(serde::Serialize)] @@ -414,6 +801,67 @@ impl GraphQlClient { Ok(out) } + /// One whole-map read: every open issue labelled `label` (the wayfinder + /// maps) in `slug`, with sub-issues and their blockers, normalized into + /// Efforts. The N+1 collapse that puts this in GraphQL: one query, + /// measured cost 10 of 5,000 points/hr, flat in map size (spec §4.1). + // TODO(#120): remove once the fetch layer dispatches map reads. + #[allow(dead_code)] + #[tracing::instrument(name = "fetch_wayfinder_map", skip(self))] + pub async fn fetch_wayfinder_map( + &self, + slug: &RepoSlug, + label: &str, + ) -> Result { + // Verbatim from spec §4.1. `first:100`/`first:50` are GitHub's hard + // relation caps, so `hasNextPage` can't be true — `pageInfo` is + // selected anyway, and the parse degrades if it ever lies. + const QUERY: &str = "query($owner:String!, $repo:String!, $label:String!) { + rateLimit { cost remaining resetAt } + repository(owner:$owner, name:$repo) { + issues(first:10, labels:[$label], states:[OPEN]) { + pageInfo { hasNextPage endCursor } + nodes { + number title state url body + subIssuesSummary { total completed percentCompleted } + subIssues(first:100) { + pageInfo { hasNextPage endCursor } + nodes { + number title state stateReason url + assignees(first:5) { nodes { login } } + labels(first:10) { nodes { name } } + issueDependenciesSummary { blockedBy blocking totalBlockedBy totalBlocking } + blockedBy(first:50) { + pageInfo { hasNextPage endCursor } + nodes { number state title repository { nameWithOwner } } + } + } + } + } + } + } + }"; + let body = GraphQlRequest { + query: QUERY.to_owned(), + variables: json!({ "owner": slug.owner(), "repo": slug.name(), "label": label }), + }; + let response: WayfinderMapResponse = self.post(&body).await?; + if let Some(rate) = response.data.as_ref().and_then(|d| d.rate_limit.as_ref()) { + tracing::debug!( + cost = rate.cost, + remaining = rate.remaining, + "map read cost" + ); + } + let page = parse_wayfinder_maps(response, slug)?; + if page.has_more_maps { + // The fixed query reads one `first:10` window; a repo with more + // open maps gets the surplus reported, not silently dropped. + tracing::warn!(%slug, "more than 10 open wayfinder maps; reading the first 10"); + } + Ok(page) + } + /// Fetch every review thread for a PR, following pagination. #[tracing::instrument(name = "fetch_review_threads", skip(self, bot_logins))] pub async fn fetch_review_threads( diff --git a/src/github/graphql/tests.rs b/src/github/graphql/tests.rs index 70e6dc2..c665bb5 100644 --- a/src/github/graphql/tests.rs +++ b/src/github/graphql/tests.rs @@ -1,8 +1,13 @@ use super::{ - ReviewStatusResponse, ThreadsResponse, ensure_no_errors, parse_review_status, - parse_review_threads, + ReviewStatusResponse, ThreadsResponse, WayfinderMapResponse, destination_from_map_body, + ensure_no_errors, has_fallback_dependency_lines, parse_review_status, parse_review_threads, + parse_wayfinder_maps, }; use crate::github::types::PRState; +use crate::repo_slug::RepoSlug; +use crate::ticket::{ + Claim, Dependency, EffortKey, ExternalDependency, TicketKey, TicketState, TicketType, +}; #[test] fn parses_review_status_batch_with_latest_commit() { @@ -204,6 +209,312 @@ fn graphql_errors_with_http_200_surface_as_err() { ); } +// ── wayfinder map parsing ──────────────────────────────────────────────────── + +fn map_slug() -> RepoSlug { + RepoSlug::new("mayfieldiv/legit") +} + +fn same_effort_key(number: u64) -> TicketKey { + TicketKey::GitHub { + repo_slug: map_slug(), + number, + } +} + +#[test] +fn parses_wayfinder_map_into_effort() { + let raw = r###"{ "data": { + "rateLimit": { "cost": 10, "remaining": 4990, "resetAt": "2026-08-28T21:00:00Z" }, + "repository": { "issues": { + "pageInfo": { "hasNextPage": false, "endCursor": null }, + "nodes": [ { + "number": 123, + "title": "Map: ticket surface", + "state": "OPEN", + "url": "https://github.com/mayfieldiv/legit/issues/123", + "body": "## Destination\n\nAll eight issues merged to main.\n\n## Notes\n\n- execution map\n", + "subIssuesSummary": { "total": 3, "completed": 1, "percentCompleted": 33 }, + "subIssues": { + "pageInfo": { "hasNextPage": false, "endCursor": null }, + "nodes": [ + { + "number": 116, "title": "Domain types", "state": "CLOSED", + "stateReason": "COMPLETED", + "url": "u116", + "assignees": { "nodes": [ { "login": "mayfieldiv" } ] }, + "labels": { "nodes": [ + { "name": "ready-for-agent" }, { "name": "wayfinder:task" } + ] }, + "issueDependenciesSummary": { "blockedBy": 0, "blocking": 2, "totalBlockedBy": 0, "totalBlocking": 2 }, + "blockedBy": { "pageInfo": { "hasNextPage": false, "endCursor": null }, "nodes": [] } + }, + { + "number": 117, "title": "GitHub transport", "state": "OPEN", + "stateReason": null, + "url": "u117", + "assignees": { "nodes": [] }, + "labels": { "nodes": [ { "name": "wayfinder:task" } ] }, + "issueDependenciesSummary": { "blockedBy": 0, "blocking": 1, "totalBlockedBy": 1, "totalBlocking": 1 }, + "blockedBy": { "pageInfo": { "hasNextPage": false, "endCursor": null }, "nodes": [ + { "number": 116, "state": "CLOSED", "title": "Domain types", + "repository": { "nameWithOwner": "mayfieldiv/legit" } } + ] } + }, + { + "number": 120, "title": "Fetch integration", "state": "OPEN", + "stateReason": null, + "url": "u120", + "assignees": { "nodes": [] }, + "labels": { "nodes": [ { "name": "question" } ] }, + "issueDependenciesSummary": { "blockedBy": 3, "blocking": 0, "totalBlockedBy": 3, "totalBlocking": 0 }, + "blockedBy": { "pageInfo": { "hasNextPage": false, "endCursor": null }, "nodes": [ + { "number": 117, "state": "OPEN", "title": "GitHub transport", + "repository": { "nameWithOwner": "MayfieldIV/Legit" } }, + { "number": 999, "state": "OPEN", "title": "External thing", + "repository": { "nameWithOwner": "other/repo" } }, + { "number": 55, "state": "CLOSED", "title": "Same repo, not in map", + "repository": { "nameWithOwner": "mayfieldiv/legit" } } + ] } + } + ] + } + } ] + } } + } }"###; + let response: WayfinderMapResponse = serde_json::from_str(raw).expect("deserialize"); + + let page = parse_wayfinder_maps(response, &map_slug()).expect("parse"); + + assert!(!page.has_more_maps); + assert_eq!(page.maps.len(), 1); + let read = &page.maps[0]; + assert!(!read.fallback_dialect); + + let effort = &read.effort; + assert_eq!( + effort.key, + EffortKey::GitHub { + repo_slug: map_slug(), + map_number: 123, + } + ); + assert_eq!(effort.title, "Map: ticket surface"); + assert_eq!( + effort.destination.as_deref(), + Some("All eight issues merged to main.") + ); + + let tickets: Vec<_> = effort.tickets().collect(); + assert_eq!(tickets.len(), 3); + + assert_eq!(tickets[0].key, same_effort_key(116)); + assert_eq!(tickets[0].title, "Domain types"); + assert_eq!(tickets[0].state, TicketState::Closed); + assert_eq!(tickets[0].claim, Some(Claim::By("mayfieldiv".to_owned()))); + // The Type is the `wayfinder:` label, prefix stripped; other labels + // (triage vocabulary) never masquerade as a Type. + assert_eq!(tickets[0].ty, TicketType("task".to_owned())); + assert!(tickets[0].dependencies.is_empty()); + + assert_eq!(tickets[1].claim, None); + assert_eq!( + tickets[1].dependencies, + vec![Dependency::SameEffort(same_effort_key(116))], + "a closed same-effort blocker is kept (the detail page shows it ✓)" + ); + + // No `wayfinder:` label at all → an empty Type, shown verbatim, Mode Either. + assert_eq!(tickets[2].ty, TicketType(String::new())); + assert_eq!( + tickets[2].dependencies, + vec![ + // Same repo (case-insensitively) + a sub-issue of this map. + Dependency::SameEffort(same_effort_key(117)), + // Another repo: External, with the captured state and title. + Dependency::External(ExternalDependency { + key: TicketKey::GitHub { + repo_slug: RepoSlug::new("other/repo"), + number: 999, + }, + state: TicketState::Open, + title: Some("External thing".to_owned()), + }), + // Same repo but not one of this map's sub-issues: also External — + // the payload's state/title are captured, so lookup must not + // degrade it to Unknown. + Dependency::External(ExternalDependency { + key: same_effort_key(55), + state: TicketState::Closed, + title: Some("Same repo, not in map".to_owned()), + }), + ] + ); + + // Blocked-ness comes from the dependency list filtered on state — #117's + // only blocker is closed, so it is on the Frontier; #120 waits on #117. + assert!( + effort + .ticket(&same_effort_key(117)) + .unwrap() + .is_on_frontier(), + "closed blockers must not block" + ); + assert!(effort.ticket(&same_effort_key(120)).unwrap().is_blocked()); +} + +#[test] +fn map_parse_flags_more_maps_beyond_first_page() { + let raw = r#"{ "data": { "repository": { "issues": { + "pageInfo": { "hasNextPage": true, "endCursor": "c1" }, + "nodes": [] + } } } }"#; + let response: WayfinderMapResponse = serde_json::from_str(raw).expect("deserialize"); + + let page = parse_wayfinder_maps(response, &map_slug()).expect("parse"); + + assert!(page.has_more_maps); +} + +#[test] +fn unreadable_blocker_repo_degrades_to_unknown_dependency() { + let raw = r#"{ "data": { "repository": { "issues": { + "pageInfo": { "hasNextPage": false, "endCursor": null }, + "nodes": [ { + "number": 1, "title": "m", "state": "OPEN", "url": "u", "body": "", + "subIssuesSummary": { "total": 1, "completed": 0, "percentCompleted": 0 }, + "subIssues": { "pageInfo": { "hasNextPage": false, "endCursor": null }, "nodes": [ { + "number": 2, "title": "t", "state": "OPEN", + "assignees": { "nodes": [] }, "labels": { "nodes": [] }, + "blockedBy": { "pageInfo": { "hasNextPage": false, "endCursor": null }, "nodes": [ + { "number": 7, "state": "OPEN", "title": "x", "repository": null } + ] } + } ] } + } ] + } } } }"#; + let response: WayfinderMapResponse = serde_json::from_str(raw).expect("deserialize"); + + let page = parse_wayfinder_maps(response, &map_slug()).expect("parse"); + + let effort = &page.maps[0].effort; + let ticket = effort.tickets().next().unwrap(); + assert_eq!( + ticket.dependencies, + vec![Dependency::Unknown { + raw: "#7".to_owned() + }] + ); + assert!(ticket.is_blocked(), "an Unknown Dependency always blocks"); +} + +#[test] +fn truncated_blocker_list_keeps_ticket_off_the_frontier() { + // `first:50` is GitHub's hard cap, so this "can't happen" — but if it + // ever does, the unseen blockers must not put the ticket on the Frontier. + let raw = r#"{ "data": { "repository": { "issues": { + "pageInfo": { "hasNextPage": false, "endCursor": null }, + "nodes": [ { + "number": 1, "title": "m", "state": "OPEN", "url": "u", "body": "", + "subIssuesSummary": { "total": 1, "completed": 0, "percentCompleted": 0 }, + "subIssues": { "pageInfo": { "hasNextPage": false, "endCursor": null }, "nodes": [ { + "number": 2, "title": "t", "state": "OPEN", + "assignees": { "nodes": [] }, "labels": { "nodes": [] }, + "blockedBy": { "pageInfo": { "hasNextPage": true, "endCursor": "c" }, "nodes": [] } + } ] } + } ] + } } } }"#; + let response: WayfinderMapResponse = serde_json::from_str(raw).expect("deserialize"); + + let page = parse_wayfinder_maps(response, &map_slug()).expect("parse"); + + let effort = &page.maps[0].effort; + assert!(effort.tickets().next().unwrap().is_blocked()); +} + +#[test] +fn map_with_task_list_body_and_no_sub_issues_is_fallback_dialect() { + let raw = r###"{ "data": { "repository": { "issues": { + "pageInfo": { "hasNextPage": false, "endCursor": null }, + "nodes": [ + { + "number": 1, "title": "fallback map", "state": "OPEN", "url": "u", + "body": "Tickets:\n\n- [x] #2 done thing\n- [ ] #3 open thing\n", + "subIssuesSummary": { "total": 0, "completed": 0, "percentCompleted": 0 }, + "subIssues": { "pageInfo": { "hasNextPage": false, "endCursor": null }, "nodes": [] } + }, + { + "number": 4, "title": "genuinely empty map", "state": "OPEN", "url": "u", + "body": "## Destination\n\nNothing charted yet.\n", + "subIssuesSummary": { "total": 0, "completed": 0, "percentCompleted": 0 }, + "subIssues": { "pageInfo": { "hasNextPage": false, "endCursor": null }, "nodes": [] } + } + ] + } } } }"###; + let response: WayfinderMapResponse = serde_json::from_str(raw).expect("deserialize"); + + let page = parse_wayfinder_maps(response, &map_slug()).expect("parse"); + + assert!( + page.maps[0].fallback_dialect, + "task-list body with zero sub-issues" + ); + assert!( + !page.maps[1].fallback_dialect, + "an empty map without a task list is just empty" + ); +} + +#[test] +fn wayfinder_map_errors_surface_as_err() { + // The envelope must `impl GraphQlErrors`: GitHub reports query failures + // as HTTP 200 + `errors`, which must not parse as an empty success. + let raw = r#"{ "data": null, "errors": [ { "message": "Bad credentials" } ] }"#; + let response: WayfinderMapResponse = serde_json::from_str(raw).expect("deserialize"); + + let err = ensure_no_errors(response).expect_err("errors must surface as Err"); + assert!(err.to_string().contains("Bad credentials")); +} + +// ── map-body helpers ───────────────────────────────────────────────────────── + +#[test] +fn destination_extracts_first_paragraph_under_the_heading() { + let body = "## Destination\n\nAll eight issues merged\nand usable in the TUI.\n\nSecond paragraph.\n\n## Notes\n"; + assert_eq!( + destination_from_map_body(body).as_deref(), + Some("All eight issues merged and usable in the TUI.") + ); + assert_eq!(destination_from_map_body("no heading here"), None); + assert_eq!( + destination_from_map_body("## Destination\n\n## Notes\n"), + None, + "an empty section yields None, not an empty string" + ); +} + +#[test] +fn detects_fallback_dependency_lines_in_leading_body_lines() { + assert!(has_fallback_dependency_lines("Part of #123\n\nSome body.")); + assert!(has_fallback_dependency_lines( + "Blocked by: #4, #5\n\nSome body." + )); + assert!( + has_fallback_dependency_lines("part of mayfieldiv/legit#123"), + "case-insensitive, slug-qualified refs count" + ); + assert!( + !has_fallback_dependency_lines("## Question\n\nPart of #123 appears after a heading"), + "scanning stops at the first heading" + ); + assert!( + !has_fallback_dependency_lines("Blocked by: the weather"), + "a dependency line needs an issue ref" + ); + assert!(!has_fallback_dependency_lines( + "This ticket is part of the plan." + )); +} + #[test] fn no_errors_passes_response_through() { let raw = r#"{ "data": { "repository": {} } }"#; diff --git a/src/github/rest.rs b/src/github/rest.rs index 60b23a7..7100884 100644 --- a/src/github/rest.rs +++ b/src/github/rest.rs @@ -8,7 +8,10 @@ use tokio::sync::{OnceCell, mpsc}; use crate::{ file_category::FileChange, - github::types::{CheckRun, IssueComment, PRState, Review, ReviewStatus, is_bot}, + github::types::{ + CheckRun, DependenciesSummary, Issue, IssueComment, IssueState, PRState, Review, + ReviewStatus, SubIssuesSummary, is_bot, + }, repo_slug::RepoSlug, secret::Secret, }; @@ -466,6 +469,50 @@ impl OctocrabRest { Ok(parse_issue_comments(raw, bot_logins)) } + /// Fetch one issue — the single-ticket refresh (spec §4.1): state, + /// labels, assignees, both summaries, `parent_issue_url`, and the body + /// all ride on this one payload. Never call `GET …/parent`: it 404s on a + /// parentless issue, while `parent_issue_url` already answers parentage — + /// so every 404 here stays a genuine error (including a token without + /// Issues read scope, which surfaces as 404, not 403). + // TODO(#120): remove once the fetch layer dispatches ticket refreshes. + #[allow(dead_code)] + #[tracing::instrument(name = "get_issue", skip(self))] + pub async fn get_issue(&self, slug: &RepoSlug, number: u64) -> Result { + let route = format!("/repos/{slug}/issues/{number}"); + let raw: RawRestIssue = self + .client + .get(&route, None::<&()>) + .await + .with_context(|| format!("fetching issue {slug}#{number}"))?; + Ok(parse_issue(raw)) + } + + /// List a repo's issues carrying `label`, dropping the pull requests the + /// endpoint mixes in. `state` is GitHub's filter vocabulary: `open`, + /// `closed`, or `all`. + // TODO(#120): remove once the fetch layer dispatches map discovery. + #[allow(dead_code)] + #[tracing::instrument(name = "list_issues_with_label", skip(self))] + pub async fn list_issues_with_label( + &self, + slug: &RepoSlug, + label: &str, + state: &str, + ) -> Result> { + let route = format!("/repos/{slug}/issues"); + let params = IssueListParams { + labels: label, + state, + per_page: 100, + }; + let raw = self + .get_all_with::(&route, ¶ms) + .await + .with_context(|| format!("listing {label:?} issues for {slug}"))?; + Ok(parse_issues(raw)) + } + /// Fetch all CI check runs for a commit, each tagged with its `workflow / job` /// name. The companion Actions workflow-name lookup is independent of the /// check-runs fetch, so the two run concurrently and the labelling latency @@ -669,11 +716,20 @@ impl OctocrabRest { /// Follow Link-header pagination for an array endpoint, collecting every /// page into one `Vec`. Mirrors the pagination in `list_open_prs`. async fn get_all(&self, route: &str) -> octocrab::Result> { + self.get_all_with(route, &PerPageParams { per_page: 100 }) + .await + } + + /// [`Self::get_all`] with caller-supplied query params, for endpoints + /// that filter (the label-filtered issues listing). Params must carry + /// their own `per_page`. + async fn get_all_with( + &self, + route: &str, + params: &P, + ) -> octocrab::Result> { let mut items = Vec::new(); - let mut page: Page = self - .client - .get(route, Some(&PerPageParams { per_page: 100 })) - .await?; + let mut page: Page = self.client.get(route, Some(params)).await?; loop { items.extend(page.take_items()); match self.client.get_page::(&page.next).await? { @@ -695,6 +751,14 @@ struct PerPageParams { per_page: u8, } +/// Query params for the label-filtered issues listing. +#[derive(serde::Serialize)] +struct IssueListParams<'a> { + labels: &'a str, + state: &'a str, + per_page: u8, +} + /// Generic page cursor for endpoints that paginate by `?per_page&page` (the /// check-runs and workflows lists). #[derive(serde::Serialize)] @@ -804,6 +868,62 @@ struct RawCommentUser { user_type: Option, } +/// Permissive wire shape for a GitHub issue, from the single-issue endpoint +/// (`GET /repos/:owner/:repo/issues/:number`) and the label-filtered listing. +/// Same posture as `RawRestPR`: everything GitHub may omit is optional or +/// defaulted. Private — the module's contract is `Issue`. +#[derive(Debug, Clone, Deserialize)] +struct RawRestIssue { + number: u64, + #[serde(default)] + title: String, + #[serde(default)] + state: Option, + #[serde(default)] + html_url: String, + #[serde(default)] + body: Option, + #[serde(default)] + labels: Vec, + #[serde(default)] + assignees: Vec, + #[serde(default)] + sub_issues_summary: SubIssuesSummary, + #[serde(default)] + issue_dependencies_summary: DependenciesSummary, + #[serde(default)] + parent_issue_url: Option, + /// Present exactly when this "issue" is really a pull request — the + /// issues listing returns both, and the ticket surface must drop PRs. + #[serde(default)] + pull_request: Option, +} + +/// Parse a raw REST issue into the domain `Issue`. Pure; tested directly. +fn parse_issue(raw: RawRestIssue) -> Issue { + Issue { + number: raw.number, + title: raw.title, + state: IssueState::parse(raw.state.as_deref()), + url: raw.html_url, + body: raw.body.unwrap_or_default(), + labels: raw.labels.into_iter().map(|l| l.name).collect(), + assignees: raw.assignees.into_iter().map(|u| u.login).collect(), + sub_issues_summary: raw.sub_issues_summary, + dependencies_summary: raw.issue_dependencies_summary, + parent_issue_url: raw.parent_issue_url, + } +} + +/// Parse an issues listing, dropping the pull requests the endpoint mixes in +/// (marked by their `pull_request` key). Pure; tested directly. +fn parse_issues(raw: Vec) -> Vec { + raw.into_iter() + .filter(|issue| issue.pull_request.is_none()) + .map(parse_issue) + .collect() +} + /// Wire shape for the single-PR detail endpoint (`GET /repos/:owner/:repo/pulls/:number`). /// We only need the `body` field here; all other PR fields are sourced from /// the enriched list PR rather than re-parsed from this response. diff --git a/src/github/rest/tests.rs b/src/github/rest/tests.rs index 9a3a814..3a48724 100644 --- a/src/github/rest/tests.rs +++ b/src/github/rest/tests.rs @@ -4,9 +4,10 @@ use std::collections::HashMap; use chrono::TimeZone; use super::{ - Label, PR, PRState, RawCheckRunsResponse, RawIssueComment, RawRestPR, RawReview, - parse_check_runs, parse_issue_comments, parse_pr, parse_reviews, + Label, PR, PRState, RawCheckRunsResponse, RawIssueComment, RawRestIssue, RawRestPR, RawReview, + parse_check_runs, parse_issue, parse_issue_comments, parse_issues, parse_pr, parse_reviews, }; +use crate::github::types::{DependenciesSummary, Issue, IssueState, SubIssuesSummary}; fn deserialize(raw: &str) -> RawRestPR { serde_json::from_str(raw).expect("fixture should deserialize") @@ -294,3 +295,106 @@ fn issue_comments_detect_bots_and_default_ghost() { assert_eq!(comments[3].author, "ghost"); assert!(!comments[3].is_bot); } + +// ── issue parsing (wayfinder ticket refresh) ───────────────────────────────── + +#[test] +fn parses_issue_from_single_issue_endpoint() { + let raw: RawRestIssue = serde_json::from_str( + r#"{ + "number": 117, + "title": "GitHub transport", + "state": "open", + "state_reason": null, + "html_url": "https://github.com/mayfieldiv/legit/issues/117", + "body": "Implements part of the wayfinder ticket surface.", + "labels": [ + { "name": "wayfinder:task", "color": "5319E7" }, + { "name": "ready-for-agent", "color": "" } + ], + "assignees": [{ "login": "mayfieldiv" }, { "login": "alice" }], + "sub_issues_summary": { "total": 0, "completed": 0, "percent_completed": 0 }, + "issue_dependencies_summary": { + "blocked_by": 1, "total_blocked_by": 2, + "blocking": 3, "total_blocking": 3 + }, + "parent_issue_url": "https://api.github.com/repos/mayfieldiv/legit/issues/123" + }"#, + ) + .expect("deserialize"); + + let issue = parse_issue(raw); + + assert_eq!( + issue, + Issue { + number: 117, + title: "GitHub transport".to_owned(), + state: IssueState::Open, + url: "https://github.com/mayfieldiv/legit/issues/117".to_owned(), + body: "Implements part of the wayfinder ticket surface.".to_owned(), + labels: vec!["wayfinder:task".to_owned(), "ready-for-agent".to_owned()], + assignees: vec!["mayfieldiv".to_owned(), "alice".to_owned()], + sub_issues_summary: SubIssuesSummary { + total: 0, + completed: 0, + percent_completed: 0, + }, + dependencies_summary: DependenciesSummary { + blocked_by: 1, + blocking: 3, + total_blocked_by: 2, + total_blocking: 3, + }, + parent_issue_url: Some( + "https://api.github.com/repos/mayfieldiv/legit/issues/123".to_owned() + ), + } + ); +} + +#[test] +fn issue_parse_defaults_everything_but_number_and_title() { + // Permissive posture: a stripped payload (or one from a GHES-ish proxy + // that omits the newer summary objects) still parses. + let raw: RawRestIssue = + serde_json::from_str(r#"{ "number": 5, "title": "bare" }"#).expect("deserialize"); + + let issue = parse_issue(raw); + + assert_eq!(issue.number, 5); + assert_eq!(issue.state, IssueState::Open, "absent state defaults Open"); + assert_eq!(issue.body, ""); + assert!(issue.labels.is_empty()); + assert!(issue.assignees.is_empty()); + assert_eq!(issue.sub_issues_summary, SubIssuesSummary::default()); + assert_eq!(issue.dependencies_summary, DependenciesSummary::default()); + assert_eq!(issue.parent_issue_url, None); +} + +#[test] +fn issue_parse_reads_closed_state() { + let raw: RawRestIssue = + serde_json::from_str(r#"{ "number": 6, "title": "done", "state": "closed" }"#) + .expect("deserialize"); + + assert_eq!(parse_issue(raw).state, IssueState::Closed); +} + +#[test] +fn issue_listing_drops_pull_requests() { + // `GET /issues?labels=…` returns PRs too, marked by the `pull_request` + // key; the listing parse must filter them out. + let raw: Vec = serde_json::from_str( + r#"[ + { "number": 1, "title": "a real issue" }, + { "number": 2, "title": "a PR", "pull_request": { "url": "u" } } + ]"#, + ) + .expect("deserialize"); + + let issues = parse_issues(raw); + + assert_eq!(issues.len(), 1); + assert_eq!(issues[0].number, 1); +} diff --git a/src/github/types.rs b/src/github/types.rs index 895a33c..d70f531 100644 --- a/src/github/types.rs +++ b/src/github/types.rs @@ -111,6 +111,99 @@ pub struct IssueComment { pub is_bot: bool, } +// ── wayfinder ticket transport types ───────────────────────────────────────── + +/// Lifecycle state for a GitHub issue. REST reports `open`/`closed`, GraphQL +/// `OPEN`/`CLOSED`; [`IssueState::parse`] accepts both. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum IssueState { + Open, + Closed, +} + +impl IssueState { + /// Parse either transport's casing. An absent or unrecognised value + /// defaults to `Open` — the safe direction both ways it's used: an open + /// Ticket stays visible, and an open blocker keeps its dependent off the + /// Frontier. Mirrors `parse_pr_state`'s `_ => Open` fallback. + pub fn parse(state: Option<&str>) -> Self { + match state { + Some(s) if s.eq_ignore_ascii_case("closed") => IssueState::Closed, + _ => IssueState::Open, + } + } +} + +/// The transport-to-domain state mapping both the GraphQL map read and the +/// REST single-ticket refresh normalize through. +impl From for crate::ticket::TicketState { + fn from(state: IssueState) -> Self { + match state { + IssueState::Open => Self::Open, + IssueState::Closed => Self::Closed, + } + } +} + +/// A Map issue's sub-issue progress counters, as GitHub reports them on every +/// issue payload. Drives the effort card's `N/M decided` counts. Deserializes +/// from both transports' casings (REST snake_case, GraphQL camelCase). +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, serde::Deserialize)] +pub struct SubIssuesSummary { + #[serde(default)] + pub total: u64, + #[serde(default)] + pub completed: u64, + #[serde(default, alias = "percentCompleted")] + pub percent_completed: u64, +} + +/// An issue's dependency counters (`issue_dependencies_summary` / +/// `issueDependenciesSummary`). `blocked_by`/`blocking` count OPEN +/// counterparts only; the `total_*` pair counts open and closed. +/// +/// Never derive blocked-ness from these: the summary is eventually consistent +/// (~10s after a close), so a read right after a blocker closes still counts +/// it. Blocked-ness comes from the `blockedBy` *list* filtered on each node's +/// state (spec §4.2); these ride along as display-only data. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, serde::Deserialize)] +pub struct DependenciesSummary { + #[serde(default, alias = "blockedBy")] + pub blocked_by: u64, + #[serde(default, alias = "blocking")] + pub blocking: u64, + #[serde(default, alias = "totalBlockedBy")] + pub total_blocked_by: u64, + #[serde(default, alias = "totalBlocking")] + pub total_blocking: u64, +} + +/// A GitHub issue as the ticket surface consumes it — the REST single-issue +/// refresh's parsed output (the `PR` analog for issues). The whole-map GraphQL +/// read normalizes straight into `ticket::Effort` instead; this type serves +/// the per-ticket paths (drill-in body, single-ticket refresh, label listing). +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Issue { + pub number: u64, + pub title: String, + pub state: IssueState, + pub url: String, + /// The issue's markdown body; empty when the author left it blank. + pub body: String, + /// Label names only — the ticket surface derives Type from them and + /// renders no label chips. + pub labels: Vec, + /// Assignee logins; the first one is the Claim. + pub assignees: Vec, + pub sub_issues_summary: SubIssuesSummary, + pub dependencies_summary: DependenciesSummary, + /// API URL of the parent issue, when this issue is a sub-issue. On every + /// payload, which is why `GET …/parent` (404 when parentless) is never + /// called — every 404 stays a genuine error, including the + /// missing-Issues-scope case, which surfaces as 404. + pub parent_issue_url: Option, +} + /// Whether a commenter is a bot. Mirrors the TS rule: a GraphQL `Bot` typename /// (or REST `user.type == "Bot"`), a `[bot]` login suffix, or a configured /// `botLogins` entry. `type_name` carries whichever the source provides. diff --git a/src/ticket.rs b/src/ticket.rs index b36bfbb..c75a789 100644 --- a/src/ticket.rs +++ b/src/ticket.rs @@ -7,7 +7,8 @@ //! and local dialect parser (#118) normalize their wire/file shapes into //! these types; the fetch and view layers consume them. -// TODO(#117): remove once the transport/fetch layers consume this module. +// TODO(#118/#120): remove once the local parser constructs the Local variants +// and the fetch/view layers consume the derivations. #![allow(dead_code)] use crate::canonical_path::CanonicalPathBuf; From ee756e8baee9b37b112fae9e5f469666d8cdf917 Mon Sep 17 00:00:00 2001 From: Mayfield Date: Fri, 28 Aug 2026 16:56:09 -0400 Subject: [PATCH 2/2] fix(github): degrade map normalization per-Effort, share Claim/Type rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fixes on #117: a malformed map now lands in failed_maps instead of discarding the whole read (spec §5.5); the Claim and wayfinder-label Type rules move to shared helpers so the single-ticket refresh (#120) can't drift from the map read; Destination extraction accepts any heading level; the summary types drop their unused GraphQL aliases and their docs now tell the truth about who deserializes them. --- src/github/graphql.rs | 95 +++++++++++++++++++------------------ src/github/graphql/tests.rs | 55 +++++++++++++++++++-- src/github/types.rs | 59 ++++++++++++++++------- 3 files changed, 142 insertions(+), 67 deletions(-) diff --git a/src/github/graphql.rs b/src/github/graphql.rs index a34b3ea..9bbe291 100644 --- a/src/github/graphql.rs +++ b/src/github/graphql.rs @@ -1,12 +1,14 @@ -//! Hand-written GitHub GraphQL transport (reqwest + serde). Covers the two -//! queries REST can't serve well: the batched per-repo review-status query and -//! the full review-thread query (with `isResolved` + bot detection). Mirrors -//! the GraphQL half of the TS `src/lib/github-transport.ts`. +//! Hand-written GitHub GraphQL transport (reqwest + serde). Covers the +//! queries REST can't serve well: the batched per-repo review-status query, +//! the full review-thread query (with `isResolved` + bot detection) — both +//! mirroring the GraphQL half of the TS `src/lib/github-transport.ts` — and +//! the whole-map wayfinder read (the same N+1 collapse, spec §4.1). //! //! Parsing is split into pure functions (`parse_review_status`, -//! `parse_review_threads`) tested directly against fixture JSON — the same -//! posture as `github::rest::parse_pr`. The `GraphQlClient` owns only the HTTP; -//! concurrency limiting happens at the command layer. +//! `parse_review_threads`, `parse_wayfinder_maps`) tested directly against +//! fixture JSON — the same posture as `github::rest::parse_pr`. The +//! `GraphQlClient` owns only the HTTP; concurrency limiting happens at the +//! command layer. use std::collections::{HashMap, HashSet}; @@ -16,12 +18,13 @@ use serde::Deserialize; use serde_json::json; use crate::{ - github::types::{FullReviewThread, IssueState, PRState, ReviewComment, ReviewStatus, is_bot}, + github::types::{ + FullReviewThread, IssueState, PRState, ReviewComment, ReviewStatus, claim_from_assignees, + is_bot, ticket_type_from_labels, + }, repo_slug::RepoSlug, secret::Secret, - ticket::{ - Claim, Dependency, Effort, EffortKey, ExternalDependency, Ticket, TicketKey, TicketType, - }, + ticket::{Dependency, Effort, EffortKey, ExternalDependency, Ticket, TicketKey}, }; const GITHUB_GRAPHQL_URL: &str = "https://api.github.com/graphql"; @@ -483,6 +486,10 @@ pub struct WayfinderMapRead { #[derive(Debug)] pub struct WayfinderMapPage { pub maps: Vec, + /// Maps that couldn't be normalized, as (map number, error). Parse + /// failures degrade per-Effort (spec §5.5): one malformed map must not + /// discard the repo's other Efforts, and never disappears silently. + pub failed_maps: Vec<(u64, String)>, /// The repo has more open maps than the query's `first:10` window. The /// query is fixed (spec §4.1), so the surplus is reported, not fetched. pub has_more_maps: bool, @@ -492,29 +499,32 @@ pub struct WayfinderMapPage { /// come from each ticket's `blockedBy` list (filtered on state at derivation /// time) — never from the eventually-consistent `issueDependenciesSummary` /// counters, which this parse doesn't even read. -fn parse_wayfinder_maps( - response: WayfinderMapResponse, - slug: &RepoSlug, -) -> Result { +fn parse_wayfinder_maps(response: WayfinderMapResponse, slug: &RepoSlug) -> WayfinderMapPage { let connection = response .data .and_then(|d| d.repository) .and_then(|r| r.issues); let Some(connection) = connection else { - return Ok(WayfinderMapPage { + return WayfinderMapPage { maps: Vec::new(), + failed_maps: Vec::new(), has_more_maps: false, - }); + }; }; - let maps = connection - .nodes - .into_iter() - .map(|node| parse_map_node(node, slug)) - .collect::>>()?; - Ok(WayfinderMapPage { + let mut maps = Vec::new(); + let mut failed_maps = Vec::new(); + for node in connection.nodes { + let number = node.number; + match parse_map_node(node, slug) { + Ok(read) => maps.push(read), + Err(error) => failed_maps.push((number, format!("{error:#}"))), + } + } + WayfinderMapPage { maps, + failed_maps, has_more_maps: connection.page_info.has_next_page, - }) + } } fn parse_map_node(node: RawMapNode, slug: &RepoSlug) -> Result { @@ -554,22 +564,14 @@ fn parse_map_node(node: RawMapNode, slug: &RepoSlug) -> Result } fn parse_ticket_node(node: RawTicketNode, slug: &RepoSlug, members: &HashSet) -> Ticket { - let claim = node - .assignees - .into_iter() - .flat_map(|c| c.nodes) - .next() - .map(|assignee| Claim::By(assignee.login)); - // The Type is the `wayfinder:` label, prefix stripped; a ticket - // without one gets the empty Type (shown verbatim, Mode Either). Other - // labels (triage vocabulary) never masquerade as a Type. - let ty = node - .labels - .into_iter() - .flat_map(|c| c.nodes) - .find_map(|label| label.name.strip_prefix("wayfinder:").map(str::to_owned)) - .map(TicketType) - .unwrap_or_else(|| TicketType(String::new())); + let claim = claim_from_assignees( + node.assignees + .into_iter() + .flat_map(|c| c.nodes) + .map(|assignee| assignee.login), + ); + let labels = node.labels.map(|c| c.nodes).unwrap_or_default(); + let ty = ticket_type_from_labels(labels.iter().map(|label| label.name.as_str())); let mut dependencies = Vec::new(); if let Some(connection) = node.blocked_by { dependencies.extend( @@ -583,7 +585,7 @@ fn parse_ticket_node(node: RawTicketNode, slug: &RepoSlug, members: &HashSet) } } -/// The first paragraph under the map body's `## Destination` heading — the +/// The first paragraph under the map body's `Destination` heading (any +/// heading level — the wayfinder template says `##`, real maps drift) — the /// one-liner the effort card and ticket detail header show. Wrapped lines are /// joined; `None` when the body has no such heading or the section is empty. fn destination_from_map_body(body: &str) -> Option { let mut lines = body.lines(); lines.by_ref().find(|line| { - line.trim() - .strip_prefix("##") - .is_some_and(|rest| rest.trim().eq_ignore_ascii_case("destination")) + let trimmed = line.trim(); + let level = trimmed.bytes().take_while(|&b| b == b'#').count(); + (1..=6).contains(&level) && trimmed[level..].trim().eq_ignore_ascii_case("destination") })?; let mut paragraph: Vec<&str> = Vec::new(); for line in lines { @@ -853,7 +856,7 @@ impl GraphQlClient { "map read cost" ); } - let page = parse_wayfinder_maps(response, slug)?; + let page = parse_wayfinder_maps(response, slug); if page.has_more_maps { // The fixed query reads one `first:10` window; a repo with more // open maps gets the surplus reported, not silently dropped. diff --git a/src/github/graphql/tests.rs b/src/github/graphql/tests.rs index c665bb5..ed85c8a 100644 --- a/src/github/graphql/tests.rs +++ b/src/github/graphql/tests.rs @@ -284,7 +284,7 @@ fn parses_wayfinder_map_into_effort() { } }"###; let response: WayfinderMapResponse = serde_json::from_str(raw).expect("deserialize"); - let page = parse_wayfinder_maps(response, &map_slug()).expect("parse"); + let page = parse_wayfinder_maps(response, &map_slug()); assert!(!page.has_more_maps); assert_eq!(page.maps.len(), 1); @@ -371,7 +371,7 @@ fn map_parse_flags_more_maps_beyond_first_page() { } } } }"#; let response: WayfinderMapResponse = serde_json::from_str(raw).expect("deserialize"); - let page = parse_wayfinder_maps(response, &map_slug()).expect("parse"); + let page = parse_wayfinder_maps(response, &map_slug()); assert!(page.has_more_maps); } @@ -394,7 +394,7 @@ fn unreadable_blocker_repo_degrades_to_unknown_dependency() { } } } }"#; let response: WayfinderMapResponse = serde_json::from_str(raw).expect("deserialize"); - let page = parse_wayfinder_maps(response, &map_slug()).expect("parse"); + let page = parse_wayfinder_maps(response, &map_slug()); let effort = &page.maps[0].effort; let ticket = effort.tickets().next().unwrap(); @@ -425,7 +425,7 @@ fn truncated_blocker_list_keeps_ticket_off_the_frontier() { } } } }"#; let response: WayfinderMapResponse = serde_json::from_str(raw).expect("deserialize"); - let page = parse_wayfinder_maps(response, &map_slug()).expect("parse"); + let page = parse_wayfinder_maps(response, &map_slug()); let effort = &page.maps[0].effort; assert!(effort.tickets().next().unwrap().is_blocked()); @@ -452,7 +452,7 @@ fn map_with_task_list_body_and_no_sub_issues_is_fallback_dialect() { } } } }"###; let response: WayfinderMapResponse = serde_json::from_str(raw).expect("deserialize"); - let page = parse_wayfinder_maps(response, &map_slug()).expect("parse"); + let page = parse_wayfinder_maps(response, &map_slug()); assert!( page.maps[0].fallback_dialect, @@ -464,6 +464,46 @@ fn map_with_task_list_body_and_no_sub_issues_is_fallback_dialect() { ); } +#[test] +fn a_malformed_map_degrades_without_discarding_the_others() { + // Duplicate ticket keys (Effort::new's guard) are the one way + // normalization can fail; the read's other maps must survive it — parse + // failures degrade per-Effort, never silently drop (spec §5.5). + let raw = r#"{ "data": { "repository": { "issues": { + "pageInfo": { "hasNextPage": false, "endCursor": null }, + "nodes": [ + { + "number": 1, "title": "broken", "state": "OPEN", "url": "u", "body": "", + "subIssues": { "pageInfo": { "hasNextPage": false, "endCursor": null }, "nodes": [ + { "number": 2, "title": "a", "state": "OPEN", + "assignees": { "nodes": [] }, "labels": { "nodes": [] }, + "blockedBy": { "pageInfo": { "hasNextPage": false, "endCursor": null }, "nodes": [] } }, + { "number": 2, "title": "a again", "state": "OPEN", + "assignees": { "nodes": [] }, "labels": { "nodes": [] }, + "blockedBy": { "pageInfo": { "hasNextPage": false, "endCursor": null }, "nodes": [] } } + ] } + }, + { + "number": 9, "title": "healthy", "state": "OPEN", "url": "u", "body": "", + "subIssues": { "pageInfo": { "hasNextPage": false, "endCursor": null }, "nodes": [] } + } + ] + } } } }"#; + let response: WayfinderMapResponse = serde_json::from_str(raw).expect("deserialize"); + + let page = parse_wayfinder_maps(response, &map_slug()); + + assert_eq!(page.maps.len(), 1); + assert_eq!(page.maps[0].effort.title, "healthy"); + assert_eq!(page.failed_maps.len(), 1); + assert_eq!(page.failed_maps[0].0, 1); + assert!( + page.failed_maps[0].1.contains("duplicate"), + "error names the cause: {}", + page.failed_maps[0].1 + ); +} + #[test] fn wayfinder_map_errors_surface_as_err() { // The envelope must `impl GraphQlErrors`: GitHub reports query failures @@ -484,6 +524,11 @@ fn destination_extracts_first_paragraph_under_the_heading() { destination_from_map_body(body).as_deref(), Some("All eight issues merged and usable in the TUI.") ); + assert_eq!( + destination_from_map_body("# Destination\n\nAny heading level works.\n").as_deref(), + Some("Any heading level works."), + "real maps drift from the template's ## level" + ); assert_eq!(destination_from_map_body("no heading here"), None); assert_eq!( destination_from_map_body("## Destination\n\n## Notes\n"), diff --git a/src/github/types.rs b/src/github/types.rs index d70f531..9eaf236 100644 --- a/src/github/types.rs +++ b/src/github/types.rs @@ -1,12 +1,16 @@ //! Domain types for the per-PR enrichment layer (review status, checks, -//! reviews, review threads, issue comments). Field sets mirror the TS -//! `src/lib/types.ts` so downstream consumers (blocker engine, summary panel, -//! detail view) stay in lockstep with the reference implementation. Strings are -//! kept permissive (e.g. `mergeable`, `state`, `conclusion`) rather than enums -//! so a value GitHub adds later doesn't fail parsing — same posture as `PR`. +//! reviews, review threads, issue comments) and the wayfinder ticket +//! transport (issues, sub-issue and dependency summaries). The PR-side field +//! sets mirror the TS `src/lib/types.ts` so downstream consumers (blocker +//! engine, summary panel, detail view) stay in lockstep with the reference +//! implementation. Strings are kept permissive (e.g. `mergeable`, `state`, +//! `conclusion`) rather than enums so a value GitHub adds later doesn't fail +//! parsing — same posture as `PR`. use chrono::{DateTime, Utc}; +use crate::ticket::{Claim, TicketType}; + /// Lifecycle state for a pull request. Mirrors the TS `PRState` discriminated /// type so the rest of the app can compare against the same values. #[derive(Debug, Clone, PartialEq, Eq)] @@ -146,21 +150,23 @@ impl From for crate::ticket::TicketState { } /// A Map issue's sub-issue progress counters, as GitHub reports them on every -/// issue payload. Drives the effort card's `N/M decided` counts. Deserializes -/// from both transports' casings (REST snake_case, GraphQL camelCase). +/// REST issue payload (`sub_issues_summary`). The GraphQL map read doesn't +/// deserialize its equivalent — an Effort's counts derive from the tickets +/// themselves, which the summary can only lag. #[derive(Debug, Clone, Copy, Default, PartialEq, Eq, serde::Deserialize)] pub struct SubIssuesSummary { #[serde(default)] pub total: u64, #[serde(default)] pub completed: u64, - #[serde(default, alias = "percentCompleted")] + #[serde(default)] pub percent_completed: u64, } -/// An issue's dependency counters (`issue_dependencies_summary` / -/// `issueDependenciesSummary`). `blocked_by`/`blocking` count OPEN -/// counterparts only; the `total_*` pair counts open and closed. +/// An issue's dependency counters, from the REST payload's +/// `issue_dependencies_summary` (the GraphQL map read doesn't deserialize its +/// equivalent). `blocked_by`/`blocking` count OPEN counterparts only; the +/// `total_*` pair counts open and closed. /// /// Never derive blocked-ness from these: the summary is eventually consistent /// (~10s after a close), so a read right after a blocker closes still counts @@ -168,13 +174,13 @@ pub struct SubIssuesSummary { /// state (spec §4.2); these ride along as display-only data. #[derive(Debug, Clone, Copy, Default, PartialEq, Eq, serde::Deserialize)] pub struct DependenciesSummary { - #[serde(default, alias = "blockedBy")] + #[serde(default)] pub blocked_by: u64, - #[serde(default, alias = "blocking")] + #[serde(default)] pub blocking: u64, - #[serde(default, alias = "totalBlockedBy")] + #[serde(default)] pub total_blocked_by: u64, - #[serde(default, alias = "totalBlocking")] + #[serde(default)] pub total_blocking: u64, } @@ -200,10 +206,31 @@ pub struct Issue { /// API URL of the parent issue, when this issue is a sub-issue. On every /// payload, which is why `GET …/parent` (404 when parentless) is never /// called — every 404 stays a genuine error, including the - /// missing-Issues-scope case, which surfaces as 404. + /// missing-Issues-scope case, which surfaces as 404. Kept as the raw wire + /// string: no consumer navigates by it yet; one that does should parse it + /// into an `EffortKey` rather than compare URLs. pub parent_issue_url: Option, } +/// The Claim a GitHub assignee list carries: the first assignee (the model +/// holds one claimant). Shared by the whole-map read and the single-ticket +/// refresh so the rule can't drift between them. +pub(crate) fn claim_from_assignees(assignees: impl IntoIterator) -> Option { + assignees.into_iter().next().map(Claim::By) +} + +/// The Type a GitHub label list carries: the first `wayfinder:` label, +/// prefix stripped. A ticket without one gets the empty Type (shown verbatim, +/// Mode Either); other labels (triage vocabulary) never masquerade as a Type. +/// Shared by the whole-map read and the single-ticket refresh. +pub(crate) fn ticket_type_from_labels<'a>(labels: impl IntoIterator) -> TicketType { + labels + .into_iter() + .find_map(|name| name.strip_prefix("wayfinder:").map(str::to_owned)) + .map(TicketType) + .unwrap_or_else(|| TicketType(String::new())) +} + /// Whether a commenter is a bot. Mirrors the TS rule: a GraphQL `Bot` typename /// (or REST `user.type == "Bot"`), a `[bot]` login suffix, or a configured /// `botLogins` entry. `type_name` carries whichever the source provides.