tix/

hierarchy.rs

//! The single owner of hierarchy behaviour (mission
//! `parent-issues-subissues-01M3BXX3`, WP01).
//!
//! Everything about an issue's parent lives here: child derivation, parent
//! set/change/clear, validation, the rollup figures, and unresolved-parent
//! detection. Surfaces call this rather than re-deriving, because a second
//! implementation is exactly the divergence the mission's intake escalates.
//!
//! ## What this module deliberately does not do
//!
//! - **It touches no files.** It takes an in-memory issue set and a
//!   `done_column` the caller has already resolved, so the whole surface is
//!   testable with no `Store` and no `tempfile` (T005 step 4). Storage owns the
//!   write; this module owns the rule.
//! - **It does not know about the issue model.** [`HierarchyNode`] is the
//!   minimum an issue must expose, and WP02 implements it for `Issue`. WP01 is
//!   forbidden from touching `src/model.rs`, so the service is defined against
//!   the shape WP02 will provide rather than against `Issue` directly.
//! - **It stores no child list.** Child membership is derived by inverting the
//!   child's parent pointer, never persisted (C-003).
//!
//! ## Decisions taken here, and why
//!
//! **The frontmatter key is `parent`.** It has to be unambiguous against the
//! existing Spec Kitty pointer fields `mission` and `task`, which are
//! governance metadata and not a hierarchy (research R2, C-007). `parent` is
//! distinct from all three, and it is reused verbatim by `FilterField::key()` in
//! WP06 (research R4).
//!
//! **The stored reference is an issue number, not a selector.** A selector can be
//! a title fragment, and a title is not stable: retitling a parent would silently
//! break every child pointing at it. Selectors are the *input* to
//! [`set_parent`] and friends; the number is what lands in the file.
//!
//! **The completion basis is direct children only.** Both figure fields in
//! `data-model.md` §3 — `direct_children` and `completed_children` — are
//! direct-child counts, and a subtree rollup is `O(V)` per parent, which turns a
//! board render into `O(V²)`. The basis is carried on the value anyway
//! ([`CompletionBasis`]) so a consumer can never guess which figure it is looking
//! at, and so changing the basis later is a visible change rather than a silent
//! one.
//!
//! **History events are declared here, not in `model.rs`.** The mission requires
//! the `## History` section to record parent set/changed/cleared, but
//! `HistoryEventKind` is `model.rs`'s and WP01 may not edit it. The three kinds
//! are therefore declared as [`ParentEventKind`] and WP02 maps them onto
//! `HistoryEventKind` when it adds the field. Declaring them here keeps the
//! service complete and the boundary honest.

use std::collections::{BTreeMap, BTreeSet};

use crate::error::{TixError, render_diagnostics};
use crate::planning::Progress;
use crate::relationships::RelationshipGraph;

/// The frontmatter key carrying an issue's parent reference.
///
/// Chosen once here and reused verbatim by the search layer in WP06.
pub const PARENT_KEY: &str = "parent";

/// The minimum an issue must expose for hierarchy derivation.
///
/// WP02 implements this for [`crate::model::Issue`]. Keeping it a trait is what
/// lets this module be written and tested without `src/model.rs`, which WP02
/// owns.
pub trait HierarchyNode {
    /// The issue's number, which is its stable identity.
    fn number(&self) -> u32;
    /// The issue's title, used only for the derived display label.
    fn title(&self) -> &str;
    /// The issue's raw status, compared against the caller's completion column.
    fn status(&self) -> &str;
    /// The stored parent reference, as an issue number. `None` means no parent.
    fn parent_number(&self) -> Option<u32>;
}

// ---------------------------------------------------------------------------
// T001 — ParentReference and HierarchyIndex
// ---------------------------------------------------------------------------

/// A resolved reference to another issue, with its derived display label.
///
/// `data-model.md` §1. The label is computed on read and **never stored**: a
/// stored title goes stale the moment the referenced issue is retitled.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct ParentReference {
    /// The referenced issue's number.
    pub number: u32,
    /// `#<number> <title>`, or just `#<number>` when the title is blank.
    pub display: String,
}

impl ParentReference {
    /// Build a reference, deriving the label from the title.
    pub fn new<N: HierarchyNode>(issue: &N) -> Self {
        Self {
            number: issue.number(),
            display: display_label(issue.number(), issue.title()),
        }
    }
}

/// The shared display form for a referenced issue: `#12 Fix the thing`.
///
/// One function, because a second formatting site is how two surfaces end up
/// disagreeing about what an issue is called.
pub fn display_label(number: u32, title: &str) -> String {
    let title = title.trim();
    if title.is_empty() {
        format!("#{number}")
    } else {
        format!("#{number} {title}")
    }
}

/// A child, as `data-model.md` §2's ordered list element.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct ChildRef {
    /// The child's number.
    pub number: u32,
    /// The child's derived display label.
    pub display: String,
}

/// The derived set of an issue's direct children (`data-model.md` §2).
///
/// Derived, never stored: this is the mechanical guard for C-003, and
/// `no_child_list_is_stored_in_the_index` pins it.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct ChildMembership {
    /// The issue the children belong to.
    pub parent: u32,
    /// Direct children, ordered by issue number.
    pub children: Vec<ChildRef>,
    /// `children.len()`, carried because callers ask for it constantly.
    pub child_count: usize,
}

impl ChildMembership {
    fn empty(parent: u32) -> Self {
        Self {
            parent,
            children: Vec::new(),
            child_count: 0,
        }
    }
}

/// The derived parent/child view over one snapshot of issues.
///
/// Built once and read many times. Both maps are `BTreeMap`, so every iteration
/// is in issue-number order and repeated reads of the same snapshot are
/// byte-identical (NFR-003).
#[derive(Debug, Clone, Default, PartialEq, Eq)]
pub struct HierarchyIndex {
    /// child number -> parent number. Only issues that *have* a parent appear.
    parent_by_child: BTreeMap<u32, u32>,
    /// parent number -> child numbers, ascending.
    children_by_parent: BTreeMap<u32, Vec<u32>>,
    /// number -> display label, for the issues this index knows about.
    labels: BTreeMap<u32, String>,
    /// number -> status, for completion resolution.
    statuses: BTreeMap<u32, String>,
}

impl HierarchyIndex {
    /// Build the index from an in-memory issue set.
    ///
    /// A child whose recorded parent is not in the set is **kept** in
    /// `parent_by_child`: dropping it would turn a diagnosable dangling reference
    /// into a silently absent one, which is the failure `UnresolvedParentState`
    /// exists to prevent. [`Self::unresolved`] reports those.
    pub fn build<N: HierarchyNode>(issues: &[N]) -> Self {
        let mut index = Self::default();
        for issue in issues {
            let number = issue.number();
            index
                .labels
                .insert(number, display_label(number, issue.title()));
            index.statuses.insert(number, issue.status().to_string());
        }
        for issue in issues {
            if let Some(parent) = issue.parent_number() {
                index.parent_by_child.insert(issue.number(), parent);
                index
                    .children_by_parent
                    .entry(parent)
                    .or_default()
                    .push(issue.number());
            }
        }
        // `children_by_parent` is filled in issue-set order, which is not
        // guaranteed sorted. Sorting here rather than assuming is what makes
        // NFR-003 hold for an arbitrary input order.
        for children in index.children_by_parent.values_mut() {
            children.sort_unstable();
        }
        index
    }

    /// The ordered direct children of `issue`.
    ///
    /// An issue with no children returns an empty membership, **not** an error:
    /// "has no children" is the ordinary state of most issues.
    pub fn children_of(&self, issue: u32) -> ChildMembership {
        let Some(numbers) = self.children_by_parent.get(&issue) else {
            return ChildMembership::empty(issue);
        };
        let children: Vec<ChildRef> = numbers
            .iter()
            .map(|n| ChildRef {
                number: *n,
                display: self.label(*n),
            })
            .collect();
        ChildMembership {
            parent: issue,
            child_count: children.len(),
            children,
        }
    }

    /// The stored parent number of `issue`, if it has one.
    ///
    /// Returns the number without resolving it, so a dangling reference is still
    /// visible here. Use [`Self::unresolved`] to separate the two cases.
    pub fn parent_of(&self, issue: u32) -> Option<u32> {
        self.parent_by_child.get(&issue).copied()
    }

    /// The derived display label for an issue this index knows.
    pub fn label(&self, issue: u32) -> String {
        self.labels
            .get(&issue)
            .cloned()
            .unwrap_or_else(|| format!("#{issue}"))
    }

    /// An issue's raw status, as the index recorded it.
    pub fn status_of(&self, issue: u32) -> Option<&str> {
        self.statuses.get(&issue).map(String::as_str)
    }

    /// Whether `issue` exists in the indexed set.
    pub fn contains(&self, issue: u32) -> bool {
        self.labels.contains_key(&issue)
    }

    /// Every child whose recorded parent no longer exists.
    ///
    /// Each retains the **recorded** number so the reference can be repaired
    /// rather than merely reported (`data-model.md` §5).
    pub fn unresolved(&self) -> Vec<UnresolvedParentState> {
        let mut out: Vec<UnresolvedParentState> = self
            .parent_by_child
            .iter()
            .filter(|(_, parent)| !self.labels.contains_key(parent))
            .map(|(child, parent)| UnresolvedParentState {
                child: *child,
                recorded_parent: *parent,
                reason: unresolved_reason(*child, *parent),
            })
            .collect();
        out.sort_by_key(|s| s.child);
        out
    }
}

// ---------------------------------------------------------------------------
// T005 — UnresolvedParentState and deletion impact
// ---------------------------------------------------------------------------

/// The diagnosed state of a child whose parent no longer exists
/// (`data-model.md` §5).
///
/// The child's file is **not** modified when its parent is deleted: the
/// reference survives so it can be repaired. Any cascade would be an escalation
/// trigger.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct UnresolvedParentState {
    /// The child holding the reference.
    pub child: u32,
    /// The reference exactly as stored, retained for repair.
    pub recorded_parent: u32,
    /// Human-readable text, reused verbatim by lint, the CLI, the TUI and MCP.
    ///
    /// One string, produced here, because four surfaces re-deriving it is four
    /// chances to word it differently.
    pub reason: String,
}

/// The single wording for an unresolved parent reference.
pub fn unresolved_reason(child: u32, parent: u32) -> String {
    format!(
        "{child}'s parent #{parent} no longer exists (the reference is kept so it can be repaired)"
    )
}

/// The issues a delete of `parent` would orphan, in issue-number order.
///
/// It **reports**; it does not delete, and the module has no mutating sibling
/// (T005 step 2). WP05 consumes this for the delete-time impact notice.
pub fn affected_by_deletion<N: HierarchyNode>(issues: &[N], parent: u32) -> Vec<ChildRef> {
    let index = HierarchyIndex::build(issues);
    index
        .children_by_parent
        .get(&parent)
        .map(|children| {
            children
                .iter()
                .map(|n| ChildRef {
                    number: *n,
                    display: index.label(*n),
                })
                .collect()
        })
        .unwrap_or_default()
}

// ---------------------------------------------------------------------------
// T002 — set / change / clear, with single-parent enforced by the API
// ---------------------------------------------------------------------------

/// The three hierarchy mutations, as the `## History` section records them.
///
/// Declared here rather than added to `model::HistoryEventKind` because WP01 may
/// not edit `src/model.rs`; WP02 maps these onto that enum.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum ParentEventKind {
    /// An issue gained its first parent.
    ParentSet,
    /// An existing parent reference was repointed.
    ParentChanged,
    /// The reference was removed.
    ParentCleared,
}

impl ParentEventKind {
    /// The canonical snake_case spelling the History section will carry.
    pub fn as_str(self) -> &'static str {
        match self {
            ParentEventKind::ParentSet => "parent_set",
            ParentEventKind::ParentChanged => "parent_changed",
            ParentEventKind::ParentCleared => "parent_cleared",
        }
    }
}

/// One hierarchy mutation, ready for storage to append to `## History`.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct ParentEvent {
    /// Which of the three mutations this is.
    pub kind: ParentEventKind,
    /// The child that changed.
    pub child: u32,
    /// The parent after the change; `None` for a clear.
    pub parent: Option<u32>,
}

/// The outcome of a hierarchy mutation: the new state plus the event to record.
///
/// The service produces the event and **does not write**; storage performs the
/// write using it, so the atomicity rule lives in exactly one place.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct ParentChange {
    /// The child whose reference changed.
    pub child: u32,
    /// The child's parent after the change.
    pub parent: Option<u32>,
    /// The event to append, or `None` when the mutation was a no-op.
    ///
    /// `None` for clearing a child that had no parent: that is not a change, and
    /// recording it would put an entry in the audit trail for nothing.
    pub event: Option<ParentEvent>,
}

/// Why a hierarchy mutation was refused.
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum HierarchyError {
    /// A validation rule rejected the proposal. Blocking: no write, no event.
    ///
    /// This is the **only** refusal the service produces. It used to carry a
    /// second variant, `UnknownIssue { selector }`, for "the named issue is not in
    /// the snapshot" — but the service takes issue **numbers**, not selectors, so a
    /// selector-shaped variant was the wrong shape and nothing ever constructed it.
    /// An unknown child is now a `Rejected` result carrying
    /// [`HierarchyDiagnosticKind::UnknownChild`], which flows through the same
    /// renderer as every other refusal (decision on tix #1373, option 1).
    Rejected(HierarchyValidationResult),
}

impl std::fmt::Display for HierarchyError {
    /// The user-facing message, **without** an `invalid input:` prefix.
    ///
    /// The prefix belongs to [`crate::error::TixError::InvalidInput`], which
    /// [`From`] supplies. Emitting it here too produced the doubled
    /// `invalid input: invalid input: ...` on the one path that reached this impl.
    ///
    /// A rejected proposal renders **every** diagnostic through the shared
    /// [`render_diagnostics`], the same renderer the relationship service uses, so
    /// a user who learned `tix link`'s refusal format does not have to learn a
    /// second one. It deliberately does not report only the first: a rejection can
    /// carry several findings, and showing one at a time makes a user fix and retry
    /// repeatedly to discover the rest.
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        match self {
            HierarchyError::Rejected(result) if result.errors.is_empty() => {
                f.write_str("hierarchy validation failed")
            }
            HierarchyError::Rejected(result) => f.write_str(&render_diagnostics(
                "E-tix-hierarchy",
                result
                    .errors
                    .iter()
                    .map(|e| (e.kind.as_str(), e.message.as_str())),
            )),
        }
    }
}

impl std::error::Error for HierarchyError {}

/// A hierarchy refusal becomes user input error, so `?` works at the call site.
///
/// The conversion is total and lossless: the type's own `Display` already renders
/// the full diagnostic set, and `InvalidInput` supplies the `invalid input:` prefix
/// every other validation failure in the crate carries.
impl From<HierarchyError> for TixError {
    fn from(err: HierarchyError) -> TixError {
        TixError::Refused(err.to_string())
    }
}

/// Point `child` at `parent`, whatever it pointed at before.
///
/// Single-parent is enforced by the **shape**: `parent` is one `Option<u32>`, and
/// no function here accepts a collection. A request for two parents is not
/// expressible, not merely rejected (C-001).
///
/// Validates before producing anything, so a refused call yields no change and
/// no event.
///
/// Setting the parent the child already has is a **no-op**, and reports one:
/// `ParentChange::event` documents `None` "when the mutation was a no-op: that is
/// not a change, and recording it would put an entry in the audit trail for
/// nothing". This function was the one place that did not honour its own return
/// type's contract — it emitted a `parent_changed` whose detail read
/// `parent #1 -> #1` — while `change_parent` did.
///
/// **Validation runs first, and the no-op check second.** The order is the whole
/// point. An early return for the no-op case was tried and rejected: it skipped
/// `validate_proposed`, so a child that *already recorded* itself as its own
/// parent — or a parent that no longer exists — was accepted as a no-op, while
/// the identical request against a child that did not already record it was
/// correctly refused. Contract §3 says a self-parent is "**always** rejected" and
/// an unknown parent is "rejected at write time"; no prior state may buy an
/// exception. A no-op is a statement about a *valid* request that happens to be
/// redundant, so it can only be recognised after the request is known to be valid.
pub fn set_parent(
    index: &HierarchyIndex,
    child: u32,
    parent: u32,
) -> Result<ParentChange, HierarchyError> {
    let result = validate_proposed(index, child, Some(parent));
    if result.blocking {
        return Err(HierarchyError::Rejected(result));
    }
    if index.parent_of(child) == Some(parent) {
        return Ok(ParentChange {
            child,
            parent: Some(parent),
            event: None,
        });
    }
    let previous = index.parent_of(child);
    let kind = if previous.is_some() {
        ParentEventKind::ParentChanged
    } else {
        ParentEventKind::ParentSet
    };
    Ok(ParentChange {
        child,
        parent: Some(parent),
        event: Some(ParentEvent {
            kind,
            child,
            parent: Some(parent),
        }),
    })
}

/// Repoint `child` at a new parent.
///
/// The same rule as [`set_parent`]; the distinct name is what callers read at the
/// call site, and the History event is the same `parent_changed` kind. Re-applying
/// the same parent is a no-op and yields **no** event, so a retried command does
/// not pad the audit trail.
///
/// Delegates with no early return of its own. An early return here looked like it
/// documented "repoint if different", but it also **skipped validation**: a child
/// that already recorded itself as its own parent, or already recorded a parent
/// that no longer exists, was accepted as a no-op — the one state where the
/// contract's "always rejected" rules must not bend. [`set_parent`] validates
/// first and no-ops second, and the two operations then cannot disagree.
pub fn change_parent(
    index: &HierarchyIndex,
    child: u32,
    parent: u32,
) -> Result<ParentChange, HierarchyError> {
    set_parent(index, child, parent)
}

/// Remove `child`'s parent, yielding the absent state rather than a dangling
/// value (`data-model.md` §1).
///
/// Clearing a child that has no parent is a no-op that produces no event.
///
/// An unknown **child** is refused, not treated as the no-op: validation runs
/// first (contract §3.5), so "valid and unchanged" stays distinct from "invalid"
/// (decision on tix #1373, option 1).
pub fn clear_parent(index: &HierarchyIndex, child: u32) -> Result<ParentChange, HierarchyError> {
    let result = validate_proposed(index, child, None);
    if result.blocking {
        return Err(HierarchyError::Rejected(result));
    }
    if index.parent_of(child).is_none() {
        return Ok(ParentChange {
            child,
            parent: None,
            event: None,
        });
    }
    Ok(ParentChange {
        child,
        parent: None,
        event: Some(ParentEvent {
            kind: ParentEventKind::ParentCleared,
            child,
            parent: None,
        }),
    })
}

// ---------------------------------------------------------------------------
// T003 — validation
// ---------------------------------------------------------------------------

/// The diagnostic kinds `contracts/hierarchy-service.md` §3 names, spelled exactly.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum HierarchyDiagnosticKind {
    /// `child == parent`.
    SelfParent,
    /// The proposed chain would close a cycle.
    Cycle,
    /// The child is not in the snapshot at all, so the write was refused.
    ///
    /// **Sibling of [`Self::UnknownParent`]**: the service validates both ends of
    /// the proposed edge. `Store` resolves selectors before calling in, so the CLI
    /// cannot reach this today, but two direct consumers can — the parity harness
    /// and any future caller that hands the service a snapshot and a number
    /// (decision on tix #1373, option 1).
    UnknownChild,
    /// The parent selector resolved to no issue, so the write was refused.
    UnknownParent,
    /// An issue records a parent that is not in the snapshot (WP05 T022).
    ///
    /// **Distinct from [`Self::UnknownParent`]**, and the distinction is the
    /// difference between a rejected write and retained data. `unknown_parent` means
    /// the service refused to write anything, so there is nothing to repair;
    /// `unresolved_parent` means a reference *was* written and its target has since
    /// gone, so a user must re-point it. Contract §3 rule 4 draws the line ("a
    /// reference that *later* becomes unresolvable is not rejected — it becomes the
    /// `UnresolvedParentState` described in §5") and T022 step 1 fixes the name.
    UnresolvedParent,
}

impl HierarchyDiagnosticKind {
    /// The contract's spelling, used verbatim in lint and in MCP payloads.
    pub fn as_str(self) -> &'static str {
        match self {
            HierarchyDiagnosticKind::SelfParent => "self_parent",
            HierarchyDiagnosticKind::UnresolvedParent => "unresolved_parent",
            HierarchyDiagnosticKind::Cycle => "cycle",
            HierarchyDiagnosticKind::UnknownChild => "unknown_child",
            HierarchyDiagnosticKind::UnknownParent => "unknown_parent",
        }
    }
}

/// One field-aware hierarchy diagnostic.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct HierarchyDiagnostic {
    /// Which rule was broken.
    pub kind: HierarchyDiagnosticKind,
    /// The issue the diagnostic is about.
    pub issue: u32,
    /// The frontmatter field at fault — always [`PARENT_KEY`] today, carried
    /// explicitly so a future field does not need a new diagnostic type.
    pub field: String,
    /// Human-readable message.
    pub message: String,
    /// The offending chain, for a cycle: the walk from the proposed parent up to
    /// the issue that closes the loop, inclusive.
    pub path: Option<Vec<u32>>,
}

impl HierarchyDiagnostic {
    fn new(kind: HierarchyDiagnosticKind, issue: u32, message: String) -> Self {
        Self {
            kind,
            issue,
            field: PARENT_KEY.to_string(),
            message,
            path: None,
        }
    }
}

/// The result of hierarchy validation (`data-model.md` §4).
///
/// Mirrors the relationship service's shape, so lint consumes both the same way.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct HierarchyValidationResult {
    /// Every diagnostic found.
    pub errors: Vec<HierarchyDiagnostic>,
    /// True when the write must be rejected.
    pub blocking: bool,
}

impl HierarchyValidationResult {
    /// A clean result: no findings, not blocking.
    pub fn clean() -> Self {
        Self {
            errors: Vec::new(),
            blocking: false,
        }
    }

    /// A result carrying one blocking diagnostic.
    pub fn blocking(diagnostic: HierarchyDiagnostic) -> Self {
        Self {
            blocking: true,
            errors: vec![diagnostic],
        }
    }

    /// Whether anything at all was found, blocking or not.
    pub fn is_empty(&self) -> bool {
        self.errors.is_empty()
    }

    /// The diagnostic kinds present, in order.
    pub fn kinds(&self) -> Vec<HierarchyDiagnosticKind> {
        self.errors.iter().map(|e| e.kind).collect()
    }
}

/// Validate a proposed parent change, before any write.
///
/// Checks, in order: the child exists, the parent exists, `child != parent`, and
/// the proposed chain does not close a cycle. Returns a result rather than
/// `Err` for rule violations so `tix lint` can call the same code.
///
/// **Both ends of the edge are validated.** The child check is the symmetric half
/// of the `unknown_parent` check: a service that emits an event for a child that
/// does not exist is the kind of thing a later direct caller relies on by
/// accident. It also runs for a `clear` (`parent: None`), so an unknown child is
/// refused rather than returning the "already absent" no-op (decision on tix
/// #1373, option 1).
pub fn validate_proposed(
    index: &HierarchyIndex,
    child: u32,
    parent: Option<u32>,
) -> HierarchyValidationResult {
    if !index.contains(child) {
        return HierarchyValidationResult::blocking(HierarchyDiagnostic::new(
            HierarchyDiagnosticKind::UnknownChild,
            child,
            format!("{PARENT_KEY}: child #{child} does not exist"),
        ));
    }
    let Some(parent) = parent else {
        // A clear cannot violate a hierarchy rule: it removes an edge.
        return HierarchyValidationResult::clean();
    };
    if !index.contains(parent) {
        return HierarchyValidationResult::blocking(HierarchyDiagnostic::new(
            HierarchyDiagnosticKind::UnknownParent,
            child,
            format!("{PARENT_KEY}: #{parent} does not exist"),
        ));
    }
    if child == parent {
        return HierarchyValidationResult::blocking(HierarchyDiagnostic::new(
            HierarchyDiagnosticKind::SelfParent,
            child,
            format!("{PARENT_KEY}: an issue cannot be its own parent"),
        ));
    }
    match cycle_path(index, child, parent) {
        Some(path) => {
            let mut diagnostic = HierarchyDiagnostic::new(
                HierarchyDiagnosticKind::Cycle,
                child,
                format!(
                    "{PARENT_KEY}: setting parent #{parent} closes a cycle ({})",
                    render_path(&path)
                ),
            );
            diagnostic.path = Some(path);
            HierarchyValidationResult::blocking(diagnostic)
        }
        None => HierarchyValidationResult::clean(),
    }
}

/// The `## History` detail line for a parent change: `parent #1 -> #2`.
///
/// **One renderer, in the service, for every caller.** Three call sites need this
/// string — the storage write path, the `--parent` edit flag, and MCP's `update_issue`
/// when WP07 lands — and a spelling rule that lives at the call site is a spelling
/// rule that will drift. It lives here, beside the event kinds it describes, and
/// takes plain numbers so it depends on no model type.
pub fn change_detail(before: Option<u32>, after: Option<u32>) -> String {
    let render = |n: Option<u32>| match n {
        Some(n) => format!("#{n}"),
        None => "none".to_string(),
    };
    format!("parent {} -> {}", render(before), render(after))
}

/// Validate a proposed parent for an issue that **does not exist yet**.
///
/// The create path's entry point, and the one place the two obvious rules cannot
/// apply: a not-yet-created child has no number, so it cannot be its own parent and
/// cannot close a cycle. `unknown_parent` is therefore the only rule that can bite,
/// and asking `validate_proposed` with a placeholder child number would encode that
/// reasoning as a lie in a diagnostic.
///
/// The diagnostic's `issue` is [`NO_ISSUE_YET`] for this path, because there is no
/// issue to name. A caller with a real child must use [`set_parent`] or
/// [`change_parent`], which report the real number.
pub fn validate_new_parent(index: &HierarchyIndex, parent: u32) -> Result<(), HierarchyError> {
    if index.contains(parent) {
        return Ok(());
    }
    Err(HierarchyError::Rejected(
        HierarchyValidationResult::blocking(HierarchyDiagnostic::new(
            HierarchyDiagnosticKind::UnknownParent,
            NO_ISSUE_YET,
            format!("{PARENT_KEY}: #{parent} does not exist"),
        )),
    ))
}

/// The issue number a diagnostic carries when it is about an issue that does not
/// exist yet — the create path's "no child" marker.
///
/// `u32::MAX`, and **provably** not a real issue number: `Store::resolve_next_number`
/// allocates with `max.checked_add(1)` and returns `Err("issue-number overflow")`
/// when that would wrap, so `u32::MAX` is never handed out.
///
/// `0` was the first choice and it is **wrong** — `resolve_next_number` returns
/// `Ok(0)` for an empty directory, so the first issue in every fresh repository is
/// `#0` (found by running the binary, not by reading the code). A `0` sentinel would
/// have named a real issue in exactly the repositories most likely to use the create
/// path. Pre-existing number allocation, filed separately as tix #1376.
pub const NO_ISSUE_YET: u32 = u32::MAX;

/// The chain that closes a cycle if `child` were pointed at `parent`, or `None`.
///
/// **Iterative**, from `parent` upward through the existing parents. Because each
/// issue has at most one parent the hierarchy is a *functional* graph, so this is
/// the only walk needed — there is no branching to explore. Recursion would
/// overflow the stack on a 10,000-deep chain (NFR-001), so the loop carries its
/// own `visited` set and terminates on the first repeat or the root.
fn cycle_path(index: &HierarchyIndex, child: u32, parent: u32) -> Option<Vec<u32>> {
    let mut path = vec![child];
    let mut visited: BTreeSet<u32> = BTreeSet::new();
    visited.insert(child);
    let mut current = parent;
    loop {
        if !visited.insert(current) {
            // `current` already appears in the walk, so the proposed edge closes
            // the loop. The path is reported from the first occurrence so the
            // message shows the cycle itself, not the tail that led to it.
            let start = path.iter().position(|n| *n == current).unwrap_or(0);
            return Some(path[start..].to_vec());
        }
        path.push(current);
        match index.parent_of(current) {
            // A dangling reference ends the walk. It cannot close a cycle, and
            // it is reported by `unresolved` instead.
            Some(next) if index.contains(next) => current = next,
            _ => return None,
        }
    }
}

/// `1 -> 2 -> 3`, for a cycle message.
fn render_path(path: &[u32]) -> String {
    path.iter()
        .map(|n| n.to_string())
        .collect::<Vec<_>>()
        .join(" -> ")
}

/// Validate a whole in-memory snapshot.
///
/// **Never** returns `Err` — lint calls it directly and must keep scanning, so a
/// single bad issue cannot abort the sweep (the relationship service's FR-018
/// rule, reused per C-002).
///
/// Reports every self-parent, every dangling reference, and every cycle.
pub fn validate_hierarchy<N: HierarchyNode>(issues: &[N]) -> HierarchyValidationResult {
    let index = HierarchyIndex::build(issues);
    let mut errors: Vec<HierarchyDiagnostic> = Vec::new();

    let mut dangling: Vec<HierarchyDiagnostic> = index
        .unresolved()
        .into_iter()
        .map(|state| {
            // `UnresolvedParent`, not `UnknownParent`: the reference exists and is
            // retained, so there is something for a user to repair. The two are one
            // situation only if you forget that one of them already wrote a file.
            HierarchyDiagnostic::new(
                HierarchyDiagnosticKind::UnresolvedParent,
                state.child,
                state.reason,
            )
        })
        .collect();
    errors.append(&mut dangling);

    let mut self_parents: Vec<HierarchyDiagnostic> = issues
        .iter()
        .filter(|issue| issue.parent_number() == Some(issue.number()))
        .map(|issue| {
            HierarchyDiagnostic::new(
                HierarchyDiagnosticKind::SelfParent,
                issue.number(),
                format!("{PARENT_KEY}: an issue cannot be its own parent"),
            )
        })
        .collect();
    errors.append(&mut self_parents);

    // One cycle per connected component, reported on the issue whose parent edge
    // closes it. Deterministic because the walk order is issue-number order.
    let mut seen: BTreeSet<u32> = BTreeSet::new();
    let mut sorted: Vec<u32> = issues.iter().map(|i| i.number()).collect();
    sorted.sort_unstable();
    for child in sorted {
        let Some(parent) = index.parent_of(child) else {
            continue;
        };
        if !index.contains(parent) || seen.contains(&child) {
            continue;
        }
        if let Some(path) = cycle_path(&index, child, parent) {
            for node in &path {
                seen.insert(*node);
            }
            let mut diagnostic = HierarchyDiagnostic::new(
                HierarchyDiagnosticKind::Cycle,
                child,
                format!("{PARENT_KEY}: cycle ({})", render_path(&path)),
            );
            diagnostic.path = Some(path);
            errors.push(diagnostic);
        }
    }

    HierarchyValidationResult {
        blocking: !errors.is_empty(),
        errors,
    }
}

// ---------------------------------------------------------------------------
// T004 — the typed rollup
// ---------------------------------------------------------------------------

/// Which population the rollup's figures describe (`data-model.md` §3).
///
/// Carried on the value so a consumer can never mistake one figure for the other,
/// and so widening the basis later is a visible change.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum CompletionBasis {
    /// Only the issue's direct children.
    DirectChildren,
}

impl CompletionBasis {
    /// The wording a surface should print next to the figure.
    pub fn as_str(self) -> &'static str {
        match self {
            CompletionBasis::DirectChildren => "direct children",
        }
    }
}

/// The derived progress figures for a parent (`data-model.md` §3, FR-003).
///
/// A typed value, never a rendered string, so CLI, TUI and MCP can be compared
/// for equality (NFR-002).
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct ParentRollup {
    /// The issue the rollup is about.
    pub parent: u32,
    /// Direct children.
    pub direct_children: usize,
    /// Direct children in the caller's completion column.
    pub completed_children: usize,
    /// The typed progress value — [`planning::Progress`], reused rather than
    /// reinvented (research R3).
    pub progress: Progress,
    /// Which population the figures describe.
    pub completion_basis: CompletionBasis,
}

/// Compute the rollup for `issue`.
///
/// `done_column` is the board's **configured** completion column, resolved by the
/// caller from config. It is a parameter and never a literal `"done"` here, so a
/// board that calls its completion column something else still reports correctly.
///
/// Reuses the index's child list rather than re-scanning the issue set, so a
/// board render stays linear in the number of children.
pub fn rollup(index: &HierarchyIndex, issue: u32, done_column: &str) -> ParentRollup {
    let membership = index.children_of(issue);
    let completed_children = membership
        .children
        .iter()
        .filter(|child| index.status_of(child.number) == Some(done_column))
        .count();
    ParentRollup {
        parent: issue,
        direct_children: membership.child_count,
        completed_children,
        progress: Progress {
            done: completed_children,
            total: membership.child_count,
        },
        completion_basis: CompletionBasis::DirectChildren,
    }
}

/// Reuse the relationship graph's validation idiom rather than a second one.
///
/// Not used by this module's own validation — the hierarchy's functional-graph
/// walk is genuinely different from a relationship cycle check, and reusing a
/// general graph walk here would mean exploring branches that cannot exist. This
/// exists so the *naming* has one home and a future reader can see that the
/// similarity was considered and rejected for a reason.
#[allow(dead_code)]
fn relationship_graph_of<'a, N: HierarchyNode + 'a>(issues: &'a [N]) -> RelationshipGraph<'a> {
    // A hierarchy-only snapshot has no edges, so the graph is built empty. The
    // call exists to document the decision, not to validate anything.
    let _ = issues;
    RelationshipGraph::new(&[], "done")
}

#[cfg(test)]
mod tests {
    use super::*;

    /// An in-memory issue. Nothing here touches the filesystem, which is the point.
    #[derive(Debug, Clone, PartialEq, Eq)]
    struct Node {
        number: u32,
        title: String,
        status: String,
        parent: Option<u32>,
    }

    impl Node {
        fn new(number: u32, title: &str) -> Self {
            Self {
                number,
                title: title.to_string(),
                status: "todo".to_string(),
                parent: None,
            }
        }
        fn with_status(mut self, status: &str) -> Self {
            self.status = status.to_string();
            self
        }
    }

    impl HierarchyNode for Node {
        fn number(&self) -> u32 {
            self.number
        }
        fn title(&self) -> &str {
            &self.title
        }
        fn status(&self) -> &str {
            &self.status
        }
        fn parent_number(&self) -> Option<u32> {
            self.parent
        }
    }

    /// A node with an explicit status and parent, for the rollup fixtures.
    fn node(number: u32, title: &str, status: &str, parent: Option<u32>) -> Node {
        let mut n = Node::new(number, title).with_status(status);
        n.parent = parent;
        n
    }

    fn nodes(spec: &[(u32, &str, Option<u32>)]) -> Vec<Node> {
        spec.iter()
            .map(|(n, t, p)| {
                let mut node = Node::new(*n, t);
                node.parent = *p;
                node
            })
            .collect()
    }

    // -- T001 ------------------------------------------------------------

    #[test]
    fn children_are_ordered_by_issue_number_regardless_of_input_order() {
        let shuffled = nodes(&[
            (9, "I", Some(1)),
            (3, "C", Some(1)),
            (7, "G", Some(1)),
            (5, "E", Some(1)),
        ]);
        let index = HierarchyIndex::build(&shuffled);
        let membership = index.children_of(1);
        assert_eq!(
            membership
                .children
                .iter()
                .map(|c| c.number)
                .collect::<Vec<_>>(),
            vec![3, 5, 7, 9],
            "children must be ascending by number even when the input is not"
        );
        assert_eq!(membership.child_count, 4);
        assert_eq!(membership.children[0].display, "#3 C");
    }

    #[test]
    fn the_index_is_stable_across_repeated_builds_from_the_same_input() {
        let issues = nodes(&[(1, "A", None), (2, "B", Some(1)), (3, "C", Some(1))]);
        let first = HierarchyIndex::build(&issues);
        let second = HierarchyIndex::build(&issues);
        assert_eq!(first, second, "two builds of one snapshot must be equal");
        // And the derived reads, not just the struct, are byte-identical.
        assert_eq!(first.children_of(1), second.children_of(1));
        assert_eq!(rollup(&first, 1, "done"), rollup(&second, 1, "done"));
    }

    #[test]
    fn a_child_appears_under_exactly_one_parent() {
        let issues = nodes(&[(1, "A", None), (2, "B", Some(1)), (3, "C", Some(2))]);
        let index = HierarchyIndex::build(&issues);
        let mut appearances = 0;
        for parent in 1..=3 {
            appearances += index.children_of(parent).child_count;
        }
        assert_eq!(
            appearances, 2,
            "each of the two parented issues must be counted exactly once"
        );
        assert_eq!(index.parent_of(2), Some(1));
        assert_eq!(index.parent_of(1), None);
    }

    #[test]
    fn no_child_list_is_stored_in_the_index() {
        // C-003's mechanical guard: the index holds two maps of *numbers*. There is
        // nowhere for a child list to live, so the "derived, never stored"
        // invariant cannot be violated by a later change that adds one.
        let issues = nodes(&[(1, "A", None), (2, "B", Some(1))]);
        let index = HierarchyIndex::build(&issues);
        // `HierarchyIndex` has no field that is a `Vec<ChildRef>`; if one were
        // added, this file would fail to compile against the struct literal below.
        let rebuilt = HierarchyIndex {
            parent_by_child: index.parent_by_child.clone(),
            children_by_parent: index.children_by_parent.clone(),
            labels: index.labels.clone(),
            statuses: index.statuses.clone(),
        };
        assert_eq!(rebuilt, index, "the index is exactly these four maps");
        // And `children_of` is the only way to get children — the private map is
        // number-only, so a display label can only come from derivation.
        assert!(
            index
                .children_by_parent
                .values()
                .all(|v| v.iter().all(|n| index.labels.contains_key(n)))
        );
    }

    #[test]
    fn an_issue_with_no_children_returns_an_empty_membership_not_an_error() {
        let index = HierarchyIndex::build(&nodes(&[(1, "A", None)]));
        let membership = index.children_of(1);
        assert_eq!(membership.child_count, 0);
        assert_eq!(membership.children, [] as [crate::hierarchy::ChildRef; 0]);
        assert_eq!(membership.parent, 1);
    }

    // -- T002 ------------------------------------------------------------

    #[test]
    fn set_then_change_then_clear_produces_three_distinct_states() {
        let issues = nodes(&[
            (1, "P", None),
            (2, "A", None),
            (3, "B", None),
            (4, "C", None),
        ]);
        let index = HierarchyIndex::build(&issues);

        let set = set_parent(&index, 2, 1).unwrap();
        assert_eq!(set.parent, Some(1));
        assert_eq!(
            set.event.as_ref().map(|e| e.kind),
            Some(ParentEventKind::ParentSet)
        );

        // Change needs an index that already reflects the set.
        let mut after_set = issues.clone();
        after_set[1].parent = Some(1);
        let index2 = HierarchyIndex::build(&after_set);
        let changed = change_parent(&index2, 2, 3).unwrap();
        assert_eq!(changed.parent, Some(3));
        assert_eq!(
            changed.event.as_ref().map(|e| e.kind),
            Some(ParentEventKind::ParentChanged)
        );

        let mut after_change = after_set.clone();
        after_change[1].parent = Some(3);
        let index3 = HierarchyIndex::build(&after_change);
        let cleared = clear_parent(&index3, 2).unwrap();
        assert_eq!(cleared.parent, None);
        assert_eq!(
            cleared.event.as_ref().map(|e| e.kind),
            Some(ParentEventKind::ParentCleared)
        );

        // Three distinct resulting states, so the sequence is observable.
        let states = [set.parent, changed.parent, cleared.parent];
        assert_eq!(states, [Some(1), Some(3), None]);
    }

    #[test]
    fn clearing_a_child_with_no_parent_is_a_no_op_with_no_event() {
        let index = HierarchyIndex::build(&nodes(&[(1, "A", None)]));
        let cleared = clear_parent(&index, 1).unwrap();
        assert_eq!(cleared.parent, None);
        assert!(
            cleared.event.is_none(),
            "a no-op must not pad the audit trail"
        );
    }

    #[test]
    fn setting_the_parent_the_child_already_has_is_a_no_op_with_no_event() {
        // The `set_parent` half of the pair. `change_parent` already had this
        // behaviour; `set_parent` emitted a `parent_changed` naming nothing,
        // contradicting `ParentChange::event`'s own documentation.
        let issues = nodes(&[(1, "P", None), (2, "C", Some(1))]);
        let index = HierarchyIndex::build(&issues);
        let again = set_parent(&index, 2, 1).unwrap();
        assert_eq!(
            again.parent,
            Some(1),
            "the resulting state is still reported"
        );
        assert!(
            again.event.is_none(),
            "an accepted mutation that changes nothing must not pad the audit trail"
        );
    }

    #[test]
    fn a_no_op_check_must_not_launder_a_self_parent() {
        // Regression. The no-op guard used to run *before* validation, so a child
        // that already recorded itself as its own parent was accepted as a no-op
        // while the same request against any other child was correctly refused.
        // Contract §3: a self-parent is rejected *always*.
        let index = HierarchyIndex::build(&nodes(&[(1, "P", None), (2, "C", Some(2))]));
        for (label, outcome) in [
            ("set_parent", set_parent(&index, 2, 2)),
            ("change_parent", change_parent(&index, 2, 2)),
        ] {
            let err = outcome.expect_err(&format!(
                "{label}(child=2, parent=2) must be refused even when 2 already records it"
            ));
            assert!(
                matches!(&err, HierarchyError::Rejected(r) if r.kinds() == [HierarchyDiagnosticKind::SelfParent]),
                "{label} refused for the wrong reason: {err:?}"
            );
        }
        // The control, so the test cannot pass by the rule being absent entirely.
        let clean = HierarchyIndex::build(&nodes(&[(1, "P", None), (2, "C", None)]));
        assert!(
            set_parent(&clean, 2, 2).is_err(),
            "the rule must still fire"
        );
    }

    #[test]
    fn a_no_op_check_must_not_launder_an_unknown_parent() {
        // Same regression, second rule. A child already pointing at a parent that
        // no longer exists must still be *refused* when asked to re-point there,
        // not accepted as a redundant request. The stored reference is kept for
        // repair (UnresolvedParentState); keeping it is not the same as accepting
        // a new write of the same dangling value.
        let index = HierarchyIndex::build(&nodes(&[(1, "P", None), (2, "C", Some(999))]));
        for (label, outcome) in [
            ("set_parent", set_parent(&index, 2, 999)),
            ("change_parent", change_parent(&index, 2, 999)),
        ] {
            let err = outcome.expect_err(&format!(
                "{label}(child=2, parent=999) must be refused: #999 is not in the snapshot"
            ));
            assert!(
                matches!(&err, HierarchyError::Rejected(r) if r.kinds() == [HierarchyDiagnosticKind::UnknownParent]),
                "{label} refused for the wrong reason: {err:?}"
            );
        }
        // A self-parent against a *clean* child is still refused, and a
        // legitimately redundant request is still a no-op.
        let clean = HierarchyIndex::build(&nodes(&[(1, "P", None), (2, "C", None)]));
        assert!(set_parent(&clean, 2, 2).is_err());
        let ok = HierarchyIndex::build(&nodes(&[(1, "P", None), (2, "C", Some(1))]));
        assert!(
            set_parent(&ok, 2, 1).unwrap().event.is_none(),
            "no-op still a no-op"
        );
    }

    #[test]
    fn re_applying_the_same_parent_is_a_no_op_with_no_event() {
        let issues = nodes(&[(1, "P", None), (2, "C", Some(1))]);
        let index = HierarchyIndex::build(&issues);
        let again = change_parent(&index, 2, 1).unwrap();
        assert_eq!(again.parent, Some(1));
        assert!(
            again.event.is_none(),
            "a retried command must not append a second event"
        );
    }

    #[test]
    fn a_second_parent_cannot_be_expressed() {
        // C-001 is enforced by the *shape*: `parent` is one `Option<u32>`, so a
        // request for two parents does not compile. The runtime half of the rule
        // is that a change replaces rather than adds.
        let issues = nodes(&[(1, "A", None), (2, "B", Some(1)), (3, "C", Some(1))]);
        assert_eq!(
            HierarchyIndex::build(&issues).children_of(1).child_count,
            2,
            "precondition: two children start under #1"
        );
        let mut after = issues.clone();
        after[1].parent = Some(3);
        let index2 = HierarchyIndex::build(&after);
        assert_eq!(
            index2.children_of(1).child_count,
            1,
            "repointing must move the child, never duplicate it"
        );
        assert_eq!(index2.parent_of(2), Some(3));
        assert_eq!(index2.children_of(3).child_count, 1);
    }

    // -- T003 ------------------------------------------------------------

    #[test]
    fn a_self_parent_is_rejected_with_its_own_diagnostic_kind() {
        let index = HierarchyIndex::build(&nodes(&[(1, "A", None)]));
        let result = validate_proposed(&index, 1, Some(1));
        assert!(result.blocking);
        assert_eq!(result.kinds(), vec![HierarchyDiagnosticKind::SelfParent]);
        assert_eq!(result.errors[0].field, PARENT_KEY);
    }

    #[test]
    fn an_unknown_parent_is_rejected_with_its_own_diagnostic_kind() {
        let index = HierarchyIndex::build(&nodes(&[(1, "A", None)]));
        let result = validate_proposed(&index, 1, Some(99));
        assert!(result.blocking);
        assert_eq!(result.kinds(), vec![HierarchyDiagnosticKind::UnknownParent]);
        assert!(
            result.errors[0].message.contains("#99"),
            "the message must name the missing parent: {}",
            result.errors[0].message
        );
    }

    #[test]
    fn a_three_node_cycle_is_rejected_and_the_path_is_reported() {
        let index = HierarchyIndex::build(&nodes(&[
            (1, "A", Some(2)),
            (2, "B", Some(3)),
            (3, "C", None),
        ]));
        let result = validate_proposed(&index, 3, Some(1));
        assert!(result.blocking);
        assert_eq!(result.kinds(), vec![HierarchyDiagnosticKind::Cycle]);
        let path = result.errors[0]
            .path
            .as_ref()
            .expect("a cycle must report its path");
        // The path is the loop in walk order: 3 -> 1 -> 2, and the *proposed* edge
        // 3->1 is what closes it, because 2's stored parent is 3. So the invariant
        // is that the last element's stored parent is the first — not that the
        // first and last are equal, which is what the first version asserted and
        // which was simply wrong about what a cycle path is.
        assert_eq!(
            index.parent_of(*path.last().expect("a cycle path is never empty")),
            path.first().copied(),
            "the reported path must be a closed loop"
        );
        assert_eq!(path.len(), 3, "the whole 3-node cycle must be shown");
        assert!(
            result.errors[0].message.contains("->"),
            "the message must render the chain: {}",
            result.errors[0].message
        );
    }

    #[test]
    fn a_deep_chain_is_walked_without_stack_overflow() {
        // NFR-001's guard. A recursive DFS passes every small case and dies here.
        const DEPTH: u32 = 10_000;
        let mut issues: Vec<Node> = (1..=DEPTH + 1)
            .map(|n| {
                let mut node = Node::new(n, "n");
                // Node `DEPTH + 1` is a second root, so the child lives in the
                // snapshot but outside the chain. It has to be *in* the index at
                // all: the service refuses a child it cannot see (#1373), so the
                // walk is what is under test here, not the membership check.
                node.parent = if n == 1 || n == DEPTH + 1 {
                    None
                } else {
                    Some(n - 1)
                };
                node
            })
            .collect();
        let index = HierarchyIndex::build(&issues);

        // A walk that runs all the way to the root and finds no cycle. The child
        // sits outside the chain, so this is the long *non-cyclic* path — the case
        // a recursive DFS blows the stack on.
        //
        // (The first version proposed `1 -> 2` and asserted it was *not* a cycle.
        // It is one: 2's stored parent is already 1. The assertion was wrong, not
        // the walk.)
        let outside_child = DEPTH + 1;
        let result = validate_proposed(&index, outside_child, Some(DEPTH / 2));
        assert!(
            !result.blocking,
            "pointing a new child into the middle of a 10,000-deep chain is not a \
             cycle: {:?}",
            result.errors
        );

        // And closing the loop at the far end *is* detected, iteratively.
        let root = DEPTH;
        issues[root as usize - 1].parent = Some(root);
        let index2 = HierarchyIndex::build(&issues);
        let result = validate_proposed(&index2, 1, Some(root));
        assert!(
            result.blocking,
            "a 10,000-deep chain closed into a cycle must be caught"
        );
        assert_eq!(result.kinds(), vec![HierarchyDiagnosticKind::Cycle]);
    }

    #[test]
    fn a_clean_hierarchy_validates_with_no_findings() {
        let issues = nodes(&[
            (1, "Root", None),
            (2, "A", Some(1)),
            (3, "B", Some(1)),
            (4, "C", Some(2)),
        ]);
        let result = validate_hierarchy(&issues);
        assert!(result.is_empty(), "unexpected: {:?}", result.errors);
        assert!(!result.blocking);
    }

    #[test]
    fn validate_hierarchy_reports_each_kind_over_a_snapshot() {
        let issues = nodes(&[
            (1, "Self", Some(1)),
            (2, "Dangling", Some(99)),
            (3, "A", Some(4)),
            (4, "B", Some(3)),
        ]);
        let result = validate_hierarchy(&issues);
        assert!(result.blocking);
        let kinds = result.kinds();
        for expected in [
            HierarchyDiagnosticKind::SelfParent,
            // `UnresolvedParent`, not `UnknownParent`. WP01 folded the two together
            // and this test asserted the folded spelling; WP05 T022 step 1 fixes the
            // name, and the distinction is the difference between a write the
            // service **refused** and a reference that was written and whose target
            // has since gone. The diagnostic's `issue` is the **child**, which is
            // what lint attributes to a file — so the contract's "names the child and
            // the recorded parent" is satisfied by the pair `(issue, message)`.
            HierarchyDiagnosticKind::UnresolvedParent,
            HierarchyDiagnosticKind::Cycle,
        ] {
            assert!(
                kinds.contains(&expected),
                "expected {expected:?} in {kinds:?}"
            );
        }
        // The unknown-parent kind still exists and still means a refused write.
        assert_eq!(
            HierarchyDiagnosticKind::UnknownParent.as_str(),
            "unknown_parent"
        );
        assert_eq!(
            HierarchyDiagnosticKind::UnresolvedParent.as_str(),
            "unresolved_parent",
            "T022 step 1 fixes this spelling; a different one is a contract violation"
        );
    }

    #[test]
    fn a_rejected_proposal_yields_no_change_and_no_event() {
        let index = HierarchyIndex::build(&nodes(&[(1, "A", None), (2, "B", Some(1))]));
        let err = set_parent(&index, 1, 1).unwrap_err();
        let HierarchyError::Rejected(result) = err;
        assert!(result.blocking);
    }

    #[test]
    fn an_unknown_child_is_refused_by_set_change_and_clear() {
        // Decision on tix #1373, option 1. The child half of the edge is validated
        // symmetrically with the parent half: a service that emits an event for a
        // child it cannot see is the kind of thing a later direct caller relies on
        // by accident.
        let index = HierarchyIndex::build(&nodes(&[(1, "P", None), (2, "C", None)]));
        let missing = 99;

        // `validate_proposed` carries the finding, so a lint or direct caller sees
        // it too, not only the three mutation entry points.
        let result = validate_proposed(&index, missing, Some(1));
        assert!(result.blocking);
        assert_eq!(result.kinds(), vec![HierarchyDiagnosticKind::UnknownChild]);
        assert_eq!(result.errors[0].issue, missing);
        assert!(
            result.errors[0].message.contains("#99"),
            "the message must name the missing child: {}",
            result.errors[0].message
        );

        for (label, outcome) in [
            ("set_parent", set_parent(&index, missing, 1)),
            ("change_parent", change_parent(&index, missing, 1)),
            ("clear_parent", clear_parent(&index, missing)),
        ] {
            let err = outcome.expect_err(&format!("{label}(child={missing}) must be refused"));
            assert!(
                matches!(&err, HierarchyError::Rejected(r) if r.kinds() == [HierarchyDiagnosticKind::UnknownChild]),
                "{label} refused for the wrong reason: {err:?}"
            );
        }

        // The control, so the test cannot pass by the rule firing everywhere: a
        // known child is still accepted (set) and still a no-op (clear).
        assert!(set_parent(&index, 2, 1).is_ok());
        assert!(clear_parent(&index, 2).unwrap().event.is_none());
    }

    #[test]
    fn the_child_check_runs_before_the_parent_check() {
        // Both ends are missing. The child is named, because it is the end the
        // caller passed first and the one it can act on by re-resolving the child.
        let index = HierarchyIndex::build(&nodes(&[(1, "P", None)]));
        let result = validate_proposed(&index, 77, Some(88));
        assert_eq!(result.kinds(), vec![HierarchyDiagnosticKind::UnknownChild]);
    }

    #[test]
    fn an_unknown_child_is_not_laundered_into_a_clear_no_op() {
        // Contract §3.5: the no-op path is reachable only after validation. An
        // unknown child that trivially "has no parent" must not return the absent
        // state as if the request had been accepted.
        let index = HierarchyIndex::build(&nodes(&[(1, "A", None)]));
        assert!(clear_parent(&index, 42).is_err());
        assert!(
            clear_parent(&index, 1).is_ok(),
            "a known child with no parent is still a no-op"
        );
    }

    // -- T004 ------------------------------------------------------------

    #[test]
    fn a_parent_with_three_children_reports_counts_and_typed_progress() {
        let issues = nodes(&[
            (1, "Parent", None),
            (2, "A", Some(1)),
            (3, "B", Some(1)),
            (4, "C", Some(1)),
        ]);
        let mut issues = issues;
        issues[1].status = "done".to_string();
        let index = HierarchyIndex::build(&issues);
        let rollup = rollup(&index, 1, "done");
        assert_eq!(rollup.direct_children, 3);
        assert_eq!(rollup.completed_children, 1);
        assert_eq!(
            rollup.progress,
            Progress { done: 1, total: 3 },
            "progress must be the typed value, not a formatted string"
        );
        assert_eq!(rollup.completion_basis, CompletionBasis::DirectChildren);
    }

    #[test]
    fn an_issue_with_no_children_rolls_up_to_zero() {
        let index = HierarchyIndex::build(&nodes(&[(1, "Lonely", None)]));
        let rollup = rollup(&index, 1, "done");
        assert_eq!(rollup.direct_children, 0);
        assert_eq!(rollup.completed_children, 0);
        assert_eq!(rollup.progress, Progress { done: 0, total: 0 });
    }

    #[test]
    fn completion_is_resolved_through_the_supplied_column_not_a_literal() {
        // A board that calls its completion column "shipped".
        let issues = vec![
            Node::new(1, "Parent"),
            Node {
                parent: Some(1),
                ..Node::new(2, "A").with_status("shipped")
            },
            Node {
                parent: Some(1),
                ..Node::new(3, "B").with_status("done")
            },
        ];
        let index = HierarchyIndex::build(&issues);
        assert_eq!(
            index.children_of(1).child_count,
            2,
            "precondition: the parent must actually have two children — the first \
             version of this test never set a parent, so it was measuring zero \
             children and asserting a count of one"
        );
        assert_eq!(
            rollup(&index, 1, "shipped").completed_children,
            1,
            "with 'shipped' as the completion column, A counts and B does not"
        );
        assert_eq!(
            rollup(&index, 1, "done").completed_children,
            1,
            "and with 'done', B counts instead"
        );
        // Both columns find exactly one child, and it is a *different* one: the
        // figure is produced by the supplied column, not by a hardcoded "done".
        // `children_of` exposes the labels, so the difference is checkable.
        let membership = index.children_of(1);
        let complete_under_shipped = membership
            .children
            .iter()
            .find(|c| index.status_of(c.number) == Some("shipped"))
            .map(|c| c.number);
        let complete_under_done = membership
            .children
            .iter()
            .find(|c| index.status_of(c.number) == Some("done"))
            .map(|c| c.number);
        assert_eq!(complete_under_shipped, Some(2));
        assert_eq!(complete_under_done, Some(3));
        assert_ne!(complete_under_shipped, complete_under_done);
    }

    #[test]
    fn repeated_rollup_over_the_same_snapshot_returns_equal_values() {
        let issues = nodes(&[(1, "P", None), (2, "A", Some(1)), (3, "B", Some(1))]);
        let index = HierarchyIndex::build(&issues);
        assert_eq!(rollup(&index, 1, "done"), rollup(&index, 1, "done"));
    }

    // -- T005 ------------------------------------------------------------

    #[test]
    fn an_unresolved_parent_retains_the_recorded_reference_for_repair() {
        let issues = nodes(&[(1, "Child", Some(42))]);
        let index = HierarchyIndex::build(&issues);
        let unresolved = index.unresolved();
        assert_eq!(unresolved.len(), 1);
        assert_eq!(unresolved[0].child, 1);
        assert_eq!(
            unresolved[0].recorded_parent, 42,
            "the reference must survive so it can be repaired, not be forgotten"
        );
        assert!(
            unresolved[0].reason.contains("#42"),
            "the reason must name the missing parent: {}",
            unresolved[0].reason
        );
    }

    #[test]
    fn deletion_reports_affected_children_without_deleting_them() {
        let issues = nodes(&[
            (1, "Parent", None),
            (2, "A", Some(1)),
            (3, "B", Some(1)),
            (4, "Elsewhere", None),
        ]);
        let affected = affected_by_deletion(&issues, 1);
        assert_eq!(
            affected.iter().map(|c| c.number).collect::<Vec<_>>(),
            vec![2, 3]
        );
        // The set is untouched: this reports, it does not mutate.
        assert_eq!(issues.len(), 4);
        assert_eq!(issues[1].parent, Some(1));
        assert!(
            affected_by_deletion(&issues, 4).is_empty(),
            "an issue with no children affects nobody"
        );
    }

    #[test]
    fn the_whole_surface_runs_without_a_store_or_a_tempfile() {
        // T005 step 4 as an executable property: everything above ran on
        // in-memory `Node`s. This asserts the module exposes no filesystem entry
        // point by naming the only constructor and calling the full path.
        let issues = nodes(&[
            (1, "Root", None),
            (2, "A", Some(1)),
            (3, "B", Some(1)),
            (4, "Orphan", Some(77)),
        ]);
        let index = HierarchyIndex::build(&issues);
        assert_eq!(index.children_of(1).child_count, 2);
        assert_eq!(rollup(&index, 1, "done").direct_children, 2);
        assert_eq!(index.unresolved().len(), 1);
        assert!(!validate_hierarchy(&issues).is_empty());
        assert!(set_parent(&index, 3, 2).is_ok());
    }

    // -----------------------------------------------------------------------
    // WP03 — the create path's own entry point
    // -----------------------------------------------------------------------

    #[test]
    fn a_new_issues_parent_must_exist() {
        // `validate_new_parent` is the only rule a not-yet-created child can break,
        // and it is tested at its own level because the CLI cannot reach it: a
        // selector that does not resolve is refused by `Store::resolve_issue` first,
        // and a selector that does resolve names an issue that is necessarily in the
        // index. Asserted here so the function is not untested code waiting for a
        // caller that never arrives.
        let index = HierarchyIndex::build(&nodes(&[(1, "P", None)]));
        assert!(
            validate_new_parent(&index, 1).is_ok(),
            "an existing issue is a valid parent"
        );

        let err = validate_new_parent(&index, 404).expect_err("a missing issue is not");
        assert!(
            matches!(&err, HierarchyError::Rejected(r) if r.kinds() == [HierarchyDiagnosticKind::UnknownParent]),
            "refused for the wrong reason: {err:?}"
        );
        // The diagnostic has no child to name, and says so with the documented
        // marker rather than a number that could be a real issue.
        let HierarchyError::Rejected(r) = err;
        assert_eq!(r.errors[0].issue, NO_ISSUE_YET);
        assert!(
            r.errors[0].message.contains("404"),
            "the message must name the missing parent: {}",
            r.errors[0].message
        );
    }

    #[test]
    fn no_issue_yet_is_not_a_number_the_store_can_hand_out() {
        // The sentinel is only sound if it can never be a real issue number.
        // `Store::resolve_next_number` allocates with `checked_add(1)` and errors on
        // overflow, so `u32::MAX` is unreachable.
        //
        // `0` was the first choice and is **not** sound. It was unsound for a
        // different reason than "it looks like a default": the allocator *did*
        // hand it out, because `max()` over an empty issue set has no answer and
        // `0` was the fallback — so every fresh repository's first issue was `#0`,
        // making the sentinel a real number. `resolve_next_number` now starts at
        // `1` (tix #1376), which is what closes the hole; this constant is the
        // other half of the same fix.
        assert_eq!(NO_ISSUE_YET, u32::MAX);
        assert!(
            NO_ISSUE_YET.checked_add(1).is_none(),
            "the sentinel must be the one value the allocator cannot produce"
        );
    }

    // -----------------------------------------------------------------------
    // WP04 / T015 — the completion basis, pinned
    // -----------------------------------------------------------------------

    #[test]
    fn t015_the_completion_basis_is_direct_children_and_the_rollup_says_so() {
        // data-model.md §3.1 records the decision; this makes the artifact and the
        // code agree, and stops a later reader reinterpreting the figure.
        //
        // The rollup is 1/2 for a parent whose two direct children are `done` and
        // `todo` — **even though the `done` child has a `done` child of its own**.
        // A subtree-recursive basis would report 2/2 here, so the fixture is built
        // to distinguish the two: the expected value only holds for a
        // direct-children basis.
        let issues = vec![
            node(1, "Parent", "todo", None),
            node(2, "Done child", "done", Some(1)),
            node(3, "Todo child", "todo", Some(1)),
            node(4, "Grandchild of #2", "done", Some(2)),
        ];
        let index = HierarchyIndex::build(&issues);
        let r = rollup(&index, 1, "done");
        assert_eq!(r.completion_basis, CompletionBasis::DirectChildren);
        assert_eq!(r.direct_children, 2, "the direct children are counted");
        assert_eq!(
            r.completed_children, 1,
            "only #2 is in the completion column"
        );
        assert_eq!(r.progress.done, 1);
        assert_eq!(r.progress.total, 2);
        // The grandchild must not have leaked into either figure.
        assert_eq!(r.direct_children + r.progress.total, 4, "2 + 2, not 3");
    }

    #[test]
    fn t015_a_grandchild_completed_does_not_complete_its_grandparent() {
        // The other half of the same distinction, stated as its own case: a
        // grandparent whose only child is done, but whose grandchild is not.
        let issues = vec![
            node(1, "Grandparent", "todo", None),
            node(2, "Child", "done", Some(1)),
            node(3, "Grandchild", "todo", Some(2)),
        ];
        let index = HierarchyIndex::build(&issues);
        // Bound as `r`, not `rollup`: the binding shadows the function for the rest
        // of the body, and the first version of this test hit exactly that.
        let r = rollup(&index, 1, "done");
        assert_eq!(
            r.completed_children, 1,
            "the child counts, not the grandchild"
        );
        assert_eq!(r.progress.total, 1);
        // And the child, whose own grandchild is not done, is itself 0/1.
        let child_rollup = rollup(&index, 2, "done");
        assert_eq!(child_rollup.completed_children, 0);
        assert_eq!(child_rollup.progress.total, 1);
    }

    #[test]
    fn t015_direct_children_are_reported_separately_from_the_rollup() {
        // T015's note: "direct children must be reported separately from the
        // rollup figure". Two fields, so `children: 40  completed: 12/40` cannot be
        // misread as a single figure.
        let issues: Vec<Node> = (1..=6)
            .map(|n| {
                node(
                    n,
                    &format!("Child {n}"),
                    if n % 2 == 0 { "done" } else { "todo" },
                    Some(1),
                )
            })
            .collect();
        let index = HierarchyIndex::build(&issues);
        let r = rollup(&index, 1, "done");
        assert_eq!(r.direct_children, 6);
        assert_eq!(r.completed_children, 3);
        assert_eq!(r.progress.done, 3);
        assert_eq!(r.progress.total, 6);
    }

    // -----------------------------------------------------------------------
    // WP05 / T020 — the unresolved-parent contract, confirmed
    // -----------------------------------------------------------------------
    //
    // T020 is a *confirm* subtask: WP01 already provides both behaviours. These
    // tests exist so the confirmation is evidence rather than a reading of the
    // source, and so the two properties the subtask names that were **not**
    // previously covered are covered now.

    #[test]
    fn t020_dropping_a_parent_from_the_set_does_not_clear_the_childs_reference() {
        // The mechanical guard against a cascade. A child whose parent leaves the
        // snapshot keeps its recorded number — `unresolved()` exists to report it,
        // and a reference that vanished with its target could not be repaired.
        let before = vec![
            node(1, "Parent", "todo", None),
            node(2, "Child", "todo", Some(1)),
        ];
        let mut after = before.clone();
        after.retain(|n| n.number != 1);

        let index = HierarchyIndex::build(&after);
        assert_eq!(
            index.parent_of(2),
            Some(1),
            "the recorded reference must survive its target"
        );
        let unresolved = index.unresolved();
        assert_eq!(unresolved.len(), 1, "and be diagnosed");
        assert_eq!(unresolved[0].child, 2);
        assert_eq!(unresolved[0].recorded_parent, 1);
        // The child's own `parent` field is untouched too — nothing in this module
        // can reach the issue set to rewrite it, and the type carries no mutator.
        assert_eq!(after[0].parent, Some(1));
    }

    #[test]
    fn t020_repairing_a_child_resolves_it_with_no_cleanup_step() {
        // T020 step 4. Re-pointing the child is the *only* repair: there is no
        // separate "clear the unresolved state" call, because there is no stored
        // unresolved state to clear — it is derived on every read.
        let dangling = vec![node(2, "Child", "todo", Some(1))];
        let index = HierarchyIndex::build(&dangling);
        assert_eq!(index.unresolved().len(), 1, "starts unresolved");

        // The repair: a parent exists and the child points at it.
        let repaired = vec![
            node(1, "Parent", "todo", None),
            node(2, "Child", "todo", Some(1)),
        ];
        let index = HierarchyIndex::build(&repaired);
        assert!(
            index.unresolved().is_empty(),
            "the unresolved state disappears because it is derived, not stored"
        );
        assert_eq!(index.parent_of(2), Some(1), "and the parent now resolves");
        assert!(index.contains(1), "and the target is in the index");

        // Re-pointing at a *different* live parent also resolves it, with no
        // intermediate step.
        let repointed = vec![
            node(1, "Parent", "todo", None),
            node(2, "Child", "todo", Some(9)),
            node(9, "Other", "todo", None),
        ];
        let index = HierarchyIndex::build(&repointed);
        assert_eq!(
            index.unresolved(),
            [] as [crate::hierarchy::UnresolvedParentState; 0]
        );
        assert_eq!(index.parent_of(2), Some(9));
    }

    #[test]
    fn t020_unresolved_is_ordered_by_child_so_a_lint_run_is_deterministic() {
        // T022's determinism acceptance criterion rests on this: `unresolved()`
        // iterates a `BTreeMap`, so the order is by child number regardless of the
        // order issues were read in. Two different input orders must give the same
        // output.
        let forward = vec![
            node(1, "A", "todo", Some(99)),
            node(2, "B", "todo", Some(98)),
            node(3, "C", "todo", Some(97)),
        ];
        let mut reversed = forward.clone();
        reversed.reverse();
        let a = HierarchyIndex::build(&forward).unresolved();
        let b = HierarchyIndex::build(&reversed).unresolved();
        assert_eq!(a, b, "the report must not depend on the read order");
        assert_eq!(
            a.iter().map(|s| s.child).collect::<Vec<_>>(),
            vec![1, 2, 3],
            "and it is ordered by child number"
        );
    }

    #[test]
    fn t020_a_resolved_parent_is_never_reported_unresolved() {
        // The control for the three above. Without it, a `unresolved()` that
        // reported everything would pass `repaired`-style assertions written as
        // "is_empty" only by accident, and a `retain` that dropped nothing would
        // pass the first.
        let clean = vec![
            node(1, "Parent", "todo", None),
            node(2, "Child", "todo", Some(1)),
            node(3, "Grandchild", "todo", Some(2)),
        ];
        let index = HierarchyIndex::build(&clean);
        assert!(
            index.unresolved().is_empty(),
            "a clean hierarchy reports nothing: {:?}",
            index.unresolved()
        );
    }
}