From 0aa9a854bcc9129f1ed71b80e3625179377f3995 Mon Sep 17 00:00:00 2001 From: atlas Date: Wed, 5 Aug 2026 22:06:12 +0200 Subject: [PATCH] fix(forge): validate list's label + milestone filters, and paginate both A filter value the forge cannot resolve is DISCARDED, not rejected, so a typo does not narrow the result set -- it returns the unfiltered one. That does not waste a query, it inverts the answer: "is anything open in this milestone" comes back as every open issue and reads as yes, and a duplicate check gets a list that never narrowed. `list` now resolves both before querying. Labels reuse the write side's resolver; the ids are discarded because this endpoint filters by name, so resolution here is a spell-check rather than a lookup -- reusing it keeps the message identical to the one the write side has always produced. Milestones accept a title or an id and are checked against the ALL-state set: filtering on a closed milestone is a normal query, and validating against open-only would reject exactly the retrospective ones. Both fetchers paginate. `repo_labels` asked for one page of 100 and treated it as the population -- the inverse of the trailer bug, same root: a valid label past the cut fails to resolve, and the error then prints an "available labels" list that is itself truncated, so the message argues for the typo. `--assignee` / `--author` stay unvalidated on purpose: someone who has left still legitimately appears on old issues, so a login that is not a current member is not necessarily a typo. Also drops the docs paragraph claiming unknown labels are silently dropped on the write side; that has not been true since the resolver landed. --- docs/tools/forge.md | 25 +++--- hive-forge/src/verbs/labels.rs | 40 ++++++--- hive-forge/src/verbs/list.rs | 36 ++++++-- hive-forge/src/verbs/milestone.rs | 141 +++++++++++++++++++++++++++--- 4 files changed, 206 insertions(+), 36 deletions(-) diff --git a/docs/tools/forge.md b/docs/tools/forge.md index 14d8ca28..bd515d95 100644 --- a/docs/tools/forge.md +++ b/docs/tools/forge.md @@ -276,20 +276,25 @@ to discover valid label names before triaging or to audit the label set. - Do NOT use raw `curl` for forge access -- the CLI handles auth, error checking, and output formatting. - `issue-create --label ` / `pr-create --label ` are - repeatable and take the same spelling `labels add` does. Unknown - names are silently dropped (matching `labels add`'s existing - behavior) rather than erroring, so a typo just means the label - doesn't land — check `hive-forge repo-labels` if one's missing. On + repeatable and take the same spelling `labels add` does. **An + unknown name is an error, not a silent drop** — the command fails + listing the names that didn't resolve plus every label the repo has, + so it's fixable from the message without a second call. On `pr-create --agit`, labels are applied as a follow-up call once the PR number is parsed back out of the push output (the AGit push itself has no label field), so they're silently skipped if that parse fails — same fallback as the deferred multi-line body. -- `list --milestone ` takes a milestone **name or id**, is - repeatable, and the forge *discards* one it doesn't recognise. So a - typo returns the **unfiltered** list rather than an empty one — the - failure looks like "this milestone contains everything", not like an - error. Confirm the spelling with `hive-forge milestone`. (Same - silent-discard shape as unknown labels above.) +- `list --label ` / `list --milestone ` are repeatable and + validated the same way, for a sharper reason: the forge **discards** a + filter value it doesn't recognise, so a typo returns the + **unfiltered** list rather than an empty one. That doesn't waste a + query, it inverts the answer — "is anything open in this milestone" + comes back as every open issue. `--milestone` takes a title or an id, + and closed milestones count as valid (filtering on a shipped one is a + normal query). +- ⚠️ `--assignee` / `--author` are deliberately **not** validated: someone + who has left still legitimately appears on old issues, so a login that + isn't a current member is not necessarily a typo. - `list --limit N` is a *request*: the forge clamps page size to its own `api.MAX_RESPONSE_ITEMS` (50 by default), so `--limit 400` returns at most 50 rows. The stderr trailer reports the real total from the diff --git a/hive-forge/src/verbs/labels.rs b/hive-forge/src/verbs/labels.rs index 9a55ca7a..e3566d19 100644 --- a/hive-forge/src/verbs/labels.rs +++ b/hive-forge/src/verbs/labels.rs @@ -90,18 +90,38 @@ pub fn run(client: &Client, args: Args) -> Result<()> { Ok(()) } -/// First page (100) of the repo's label set, for name → id resolution. -/// `pub(crate)` so other creation verbs (`issue-create`, `pr-create`) can -/// resolve a `--label` name to the id the create-payload structs need -/// without duplicating the lookup. +/// The repo's whole label set, for name → id resolution. `pub(crate)` so +/// other verbs (`issue-create`, `pr-create`, `list`) can resolve a label +/// name without duplicating the lookup. +/// +/// Paginated, where this used to ask for one page of 100 — **a page size +/// with no page loop is a lie the size of the page.** Truncation here is +/// worse than it looks: it doesn't drop a result, it makes a *valid* +/// label past the cut fail to resolve, and then prints an "available +/// labels" list that is itself incomplete, so the error argues for the +/// typo. Same root as the `list` trailer bug (assuming one request +/// returns the whole population), opposite direction — that one was a +/// false positive, this is a false negative. pub(crate) fn repo_labels(client: &Client) -> Result> { + /// Generous cap so a misbehaving server can't spin us forever. + const MAX_PAGES: u32 = 50; + const PAGE: u32 = 100; let (owner, name) = client.owner_repo()?; - let (_, labels) = client - .api() - .issue_list_labels(owner, name, IssueListLabelsQuery::default()) - .page_size(100) - .send()?; - Ok(labels) + let mut all = Vec::new(); + for page in 1..=MAX_PAGES { + let (_, batch) = client + .api() + .issue_list_labels(owner, name, IssueListLabelsQuery::default()) + .page(page) + .page_size(PAGE) + .send()?; + let short = batch.len() < PAGE as usize; + all.extend(batch); + if short { + break; + } + } + Ok(all) } /// Resolve label names to ids, hard-erroring if any name doesn't match an diff --git a/hive-forge/src/verbs/list.rs b/hive-forge/src/verbs/list.rs index 91cb386f..01cd0439 100644 --- a/hive-forge/src/verbs/list.rs +++ b/hive-forge/src/verbs/list.rs @@ -24,7 +24,7 @@ use forgejo_api::structs::{ use serde_json::Value; use crate::client::Client; -use crate::verbs::print_json; +use crate::verbs::{labels, milestone, print_json}; /// What kind of items to return. Maps onto Forgejo's `type` query /// parameter: `issues` / `pulls`, or no filter at all for `both` @@ -89,12 +89,15 @@ pub struct Args { #[arg(long)] mention: Option, /// Filter to items carrying any of these label names. Repeatable. + /// Validated client-side: a name the forge can't resolve is dropped + /// from the filter rather than rejected, which returns MORE results + /// than asked for, not fewer. #[arg(long = "label")] labels: Vec, - /// Filter to items in any of these milestones, by name or id. - /// Repeatable. A name that doesn't exist is discarded by the forge - /// rather than rejected — so a typo returns the UNFILTERED list, not - /// an empty one. Check the spelling against `milestone list`. + /// Filter to items in any of these milestones, by title or id. + /// Repeatable. Validated client-side against the repo's milestones + /// (closed ones included), since the forge would silently discard a + /// name it can't resolve and return the UNFILTERED list. #[arg(long = "milestone")] milestones: Vec, /// Full-text search over title AND body, server-side. Composes with @@ -117,6 +120,29 @@ pub struct Args { pub fn run(client: &Client, args: Args) -> Result<()> { let (owner, name) = client.owner_repo()?; + // Validate the name-based filters BEFORE querying. The forge + // *discards* a label or milestone it can't resolve instead of + // rejecting it, so a typo doesn't narrow the result set — it returns + // the UNFILTERED one. That doesn't waste a query, it inverts the + // answer: "is anything open in this milestone" comes back as every + // open issue and reads as yes, and a duplicate check gets a list that + // never narrowed and concludes there isn't one. + // + // The ids are discarded on purpose — this endpoint filters by name, + // so resolution here is a spell-check, not a lookup. `resolve_ids` is + // reused rather than reimplemented so the message stays identical to + // the one the write side has always produced. + // + // Costs one extra round-trip per filtered invocation, and only when + // the filter is actually used. + if !args.labels.is_empty() { + let all = labels::repo_labels(client)?; + labels::resolve_ids(&all, &args.labels)?; + } + if !args.milestones.is_empty() { + let all = milestone::repo_milestones(client, "all")?; + milestone::ensure_filters_resolve(&all, &args.milestones)?; + } let query = IssueListIssuesQuery { state: Some(args.state.query_state()), // The forge parses `labels` as a comma-separated list of names. diff --git a/hive-forge/src/verbs/milestone.rs b/hive-forge/src/verbs/milestone.rs index 268dc76c..ed1966c2 100644 --- a/hive-forge/src/verbs/milestone.rs +++ b/hive-forge/src/verbs/milestone.rs @@ -1,10 +1,10 @@ //! `milestone list|create|close` — manage milestones. Default action: //! list. -use anyhow::{Context, Result}; +use anyhow::{Context, Result, bail}; use clap::{Args as ClapArgs, Subcommand}; use forgejo_api::structs::{ - CreateMilestoneOption, EditMilestoneOption, IssueGetMilestonesListQuery, + CreateMilestoneOption, EditMilestoneOption, IssueGetMilestonesListQuery, Milestone, }; use serde_json::{Value, json}; use time::OffsetDateTime; @@ -42,19 +42,80 @@ enum Action { }, } +/// The repo's milestones in `state` (`open` / `closed` / `all`), all +/// pages. `pub(crate)` so `list` can validate a `--milestone` filter +/// against them. +/// +/// Paginated for the reason `labels::repo_labels` spells out: a page size +/// with no page loop silently truncates the population, and a validator +/// reading a truncated set rejects legitimate input. +pub(crate) fn repo_milestones(client: &Client, state: &str) -> Result> { + /// Generous cap so a misbehaving server can't spin us forever. + const MAX_PAGES: u32 = 50; + const PAGE: u32 = 50; + let (owner, name) = client.owner_repo()?; + let mut all = Vec::new(); + for page in 1..=MAX_PAGES { + let query = IssueGetMilestonesListQuery { + state: Some(state.to_owned()), + name: None, + }; + let (_, batch) = client + .api() + .issue_get_milestones_list(owner, name, query) + .page(page) + .page_size(PAGE) + .send()?; + let short = batch.len() < PAGE as usize; + all.extend(batch); + if short { + break; + } + } + Ok(all) +} + +/// Check every `--milestone` filter value names a real milestone, +/// erroring with the ones that don't plus what is available. +/// +/// Accepts a title **or** a numeric id, because Forgejo's filter does. +/// +/// ⚠️ Callers must pass the `all`-state set, not the open one: filtering +/// on a *closed* milestone is a normal query ("what shipped in v1"), and +/// validating against open-only would reject exactly the retrospective +/// queries that need it most. Same reasoning that keeps `--assignee` and +/// `--author` unvalidated — a name being no longer current is not a typo. +pub(crate) fn ensure_filters_resolve(all: &[Milestone], values: &[String]) -> Result<()> { + let unresolved: Vec<&str> = values + .iter() + .filter(|v| { + !all.iter().any(|m| { + m.title.as_deref() == Some(v.as_str()) + || v.parse::().is_ok_and(|id| m.id == Some(id)) + }) + }) + .map(String::as_str) + .collect(); + if !unresolved.is_empty() { + let available: Vec<&str> = all.iter().filter_map(|m| m.title.as_deref()).collect(); + bail!( + "unresolved milestone(s): {} — available milestones: {}", + unresolved.join(", "), + if available.is_empty() { + "(none)".to_owned() + } else { + available.join(", ") + } + ); + } + Ok(()) +} + pub fn run(client: &Client, args: Args) -> Result<()> { let (owner, name) = client.owner_repo()?; match args.action.unwrap_or(Action::List) { Action::List => { - let query = IssueGetMilestonesListQuery { - state: Some("open".to_owned()), - name: None, - }; - let (_, milestones) = client - .api() - .issue_get_milestones_list(owner, name, query) - .page_size(50) - .send()?; + let milestones = repo_milestones(client, "open")?; let trimmed: Vec = milestones .iter() .map(|m| { @@ -112,3 +173,61 @@ pub fn run(client: &Client, args: Args) -> Result<()> { } } } + +#[cfg(test)] +mod tests { + use super::{Milestone, ensure_filters_resolve}; + + /// Struct literal rather than `serde_json::from_value`: the JSON + /// fixture compiled and then failed at *runtime* on a required field + /// (`closed_at`), which is a worse trade than listing the fields — + /// this way a field the upstream crate adds is a compile error, and + /// the neighbouring `labels.rs` fixture already does it this way. + fn milestone(id: i64, title: &str) -> Milestone { + Milestone { + closed_at: None, + closed_issues: None, + created_at: None, + description: None, + due_on: None, + id: Some(id), + open_issues: None, + state: None, + title: Some(title.to_owned()), + updated_at: None, + } + } + + #[test] + fn titles_and_ids_both_resolve() { + let all = vec![milestone(1, "v1"), milestone(2, "v2")]; + ensure_filters_resolve(&all, &["v2".to_owned(), "1".to_owned()]).unwrap(); + } + + #[test] + fn unresolved_value_errors_listing_the_typo_and_available_titles() { + let all = vec![milestone(1, "v1"), milestone(2, "v2")]; + let err = ensure_filters_resolve(&all, &["v3".to_owned()]).unwrap_err(); + let msg = err.to_string(); + assert!(msg.contains("v3"), "missing the typo: {msg}"); + assert!( + msg.contains("v1") && msg.contains("v2"), + "missing what is available: {msg}" + ); + } + + #[test] + fn a_numeric_value_is_an_id_not_a_title() { + // "2" must not match the milestone *titled* "2" by accident of + // parsing, nor resolve because some milestone exists — an id + // filter that silently matches nothing is the whole bug. + let all = vec![milestone(1, "v1")]; + assert!(ensure_filters_resolve(&all, &["2".to_owned()]).is_err()); + } + + #[test] + fn unresolved_on_a_repo_with_no_milestones_says_none() { + let err = ensure_filters_resolve(&[], &["v1".to_owned()]).unwrap_err(); + assert!(err.to_string().contains("(none)")); + } +}