apexe 0.8.0

use std::path::Path;

use apcore::{match_pattern, ACLRule, ErrorCode, ModuleError, ACL};
use apcore_toolkit::ScannedModule;
use serde::{Deserialize, Serialize};

/// Serializable representation of an ACL config file (rules + default_effect).
#[derive(Debug, Serialize, Deserialize)]
struct AclConfig {
    rules: Vec<ACLRule>,
    default_effect: String,
}

/// Marks the rule `generate_default`/`merge_default` builds from readonly
/// annotations, so a merge can find and update it without touching a
/// hand-authored rule that happens to have the same effect.
const READONLY_RULE_DESCRIPTION: &str = "Auto-allow readonly CLI commands";
/// Marks the rule built from destructive annotations; see
/// [`READONLY_RULE_DESCRIPTION`].
const DESTRUCTIVE_RULE_DESCRIPTION: &str = "Block destructive CLI commands by default";
/// Marks the rule built from `open_world` annotations; see
/// [`READONLY_RULE_DESCRIPTION`].
const OPEN_WORLD_RULE_DESCRIPTION: &str = "Block open-world CLI commands by default";

/// Manages access control for CLI modules using apcore's ACL system.
///
/// # Rule ordering is first-match-wins, not most-specific-wins
///
/// apcore's `ACL::check` walks the rule list in order and returns the effect of
/// the *first* rule whose `callers` and `targets` both match; it never scores
/// rules by specificity. So
///
/// ```yaml
/// rules:
///   - { callers: ["*"], targets: ["cli.*"], effect: allow }
///   - { callers: ["*"], targets: ["cli.cp"], effect: deny }   # never reached
/// ```
///
/// lets `cli.cp` through — the broad `allow` wins because it comes first. This
/// is the opposite of the "most specific rule wins" convention that firewall-
/// and IAM-style rule lists usually follow, and it is deliberate on apcore's
/// side (documented as "first-match-wins evaluation"), so apexe does not
/// reorder or re-rank the rules it loads. Write exceptions *above* the broad
/// rule they carve out of.
pub struct AclManager {
    acl: ACL,
    /// Cached default_effect so we can serialize without needing an accessor on ACL.
    default_effect: String,
}

impl AclManager {
    /// Load ACL from a YAML config file.
    #[allow(clippy::result_large_err)] // ModuleError is 184 bytes; acceptable at crate boundary
    pub fn from_config(config_path: &Path) -> Result<Self, ModuleError> {
        let acl = ACL::load(&config_path.to_string_lossy()).map_err(|e| {
            // `GeneralInvalidInput`, not `GeneralInternalError`: every way
            // `ACL::load` fails is a problem with the file the operator pointed
            // `--acl` at, not a fault inside apexe. Since apcore 0.29 that
            // includes the pattern-arity refusals (apcore#112) apexe used to
            // catch itself in `validate_acl_rules`, which reported them as
            // invalid input -- so mapping apcore's own refusal to an internal
            // error would have *downgraded* the diagnosis for exactly the
            // policy defects upstream just started catching.
            ModuleError::new(
                ErrorCode::GeneralInvalidInput,
                format!("Failed to load ACL '{}': {e}", config_path.display()),
            )
            .with_retryable(false)
            .with_ai_guidance(
                "The ACL file is malformed. Check the rule that the message names, then \
                 re-run. A rule's `callers`/`targets` must each hold at least one non-empty \
                 pattern -- `[]` matches nothing and protects nothing.",
            )
        })?;
        // Re-read the file to extract default_effect since ACL has no public accessor.
        let default_effect = Self::read_default_effect(config_path);
        Ok(Self {
            acl,
            default_effect,
        })
    }

    /// Generate default ACL from scanned modules based on annotations.
    pub fn generate_default(modules: &[ScannedModule]) -> Self {
        let readonly_ids = Self::readonly_ids(modules);
        let destructive_ids = Self::destructive_ids(modules);
        let open_world_ids = Self::open_world_ids(modules);

        let mut rules = Vec::new();
        if !readonly_ids.is_empty() {
            rules.push(Self::readonly_rule(readonly_ids));
        }
        if !destructive_ids.is_empty() {
            rules.push(Self::destructive_rule(destructive_ids));
        }
        if !open_world_ids.is_empty() {
            rules.push(Self::open_world_rule(open_world_ids));
        }

        let acl = ACL::new(rules, "deny", None);
        Self {
            acl,
            default_effect: "deny".to_string(),
        }
    }

    /// Merge freshly-generated default rules into an existing ACL file.
    ///
    /// `apexe scan` runs incrementally — one invocation covers only the
    /// tools named on that command line, while `bindings_dir` accumulates
    /// bindings across every scan ever run. Regenerating from `modules`
    /// alone and overwriting the file (the previous behaviour) discarded
    /// every earlier scan's readonly/destructive membership, and any rule
    /// the operator hand-authored, the moment a second `apexe scan` ran.
    ///
    /// This merges instead: within the two auto-generated rules (matched by
    /// their fixed `description`), the module ids belonging to THIS batch
    /// are replaced with wherever this batch's fresh scan says they now
    /// belong — so a module whose annotations changed moves into its new
    /// rule and out of its old one — while a module from an earlier scan
    /// that this batch never touched keeps whatever membership it already
    /// had. Every other rule (hand-authored, or generated by a scan of a
    /// different tool) is carried over unchanged, in its original position.
    /// `default_effect` is likewise taken from the existing file, not reset.
    #[allow(clippy::result_large_err)] // ModuleError is 184 bytes; acceptable at crate boundary
    pub fn merge_default(
        existing_path: &Path,
        modules: &[ScannedModule],
    ) -> Result<Self, ModuleError> {
        let existing = Self::from_config(existing_path)?;
        let batch_ids: std::collections::HashSet<&str> =
            modules.iter().map(|m| m.module_id.as_str()).collect();
        let mut fresh_readonly = Self::readonly_ids(modules);
        let mut fresh_destructive = Self::destructive_ids(modules);
        let mut fresh_open_world = Self::open_world_ids(modules);

        let mut rules = Vec::new();
        for mut rule in existing.acl.rules().to_vec() {
            match rule.description.as_deref() {
                Some(READONLY_RULE_DESCRIPTION) => {
                    rule.targets.retain(|id| !batch_ids.contains(id.as_str()));
                    rule.targets.append(&mut fresh_readonly);
                    if rule.targets.is_empty() {
                        continue;
                    }
                }
                Some(DESTRUCTIVE_RULE_DESCRIPTION) => {
                    rule.targets.retain(|id| !batch_ids.contains(id.as_str()));
                    rule.targets.append(&mut fresh_destructive);
                    if rule.targets.is_empty() {
                        continue;
                    }
                }
                Some(OPEN_WORLD_RULE_DESCRIPTION) => {
                    rule.targets.retain(|id| !batch_ids.contains(id.as_str()));
                    rule.targets.append(&mut fresh_open_world);
                    if rule.targets.is_empty() {
                        continue;
                    }
                }
                _ => {}
            }
            rules.push(rule);
        }
        // Only reached when the existing file had no rule of that kind yet
        // (fresh_readonly / fresh_destructive is drained by the loop above
        // once a matching rule consumes it).
        if !fresh_readonly.is_empty() {
            rules.push(Self::readonly_rule(fresh_readonly));
        }
        if !fresh_destructive.is_empty() {
            rules.push(Self::destructive_rule(fresh_destructive));
        }
        if !fresh_open_world.is_empty() {
            rules.push(Self::open_world_rule(fresh_open_world));
        }

        let default_effect = existing.default_effect.clone();
        let acl = ACL::new(rules, &default_effect, None);
        Ok(Self {
            acl,
            default_effect,
        })
    }

    /// Modules the generated policy auto-allows.
    ///
    /// **`open_world` disqualifies a module here even when it is `readonly`.**
    /// `readonly` is a claim about local state — that the command does not
    /// modify the machine it runs on — and it says nothing about what the
    /// command *sends*. A read that reaches the network is the exfiltration
    /// shape, not the safe shape, so auto-allowing it on the strength of
    /// `readonly` alone would hand out the one permission that cannot be taken
    /// back once used.
    ///
    /// Excluding rather than reordering is deliberate. Rule evaluation is
    /// first-match-wins (see the type docs) and the readonly `allow` is written
    /// before the open-world `deny`, so a module in both lists would be allowed
    /// by whichever came first — a correctness question settled by list order,
    /// which is exactly the kind of thing that breaks silently when a rule
    /// moves. Keeping the three lists disjoint means no ordering makes this
    /// policy wrong, including in a file whose rules an operator has since
    /// rearranged.
    ///
    /// No module produced by inference alone is currently both: the name lists
    /// behind `readonly` and `open_world` do not intersect. An overlay can
    /// assert both, and that became reachable when `open_world` was added to
    /// the overlay schema, so this is a latent case closed on purpose rather
    /// than a live one being fixed.
    fn readonly_ids(modules: &[ScannedModule]) -> Vec<String> {
        modules
            .iter()
            .filter(|m| {
                m.annotations
                    .as_ref()
                    .is_some_and(|a| a.readonly && !a.open_world)
            })
            .map(|m| m.module_id.clone())
            .collect()
    }

    /// Modules denied for reaching outside the machine.
    ///
    /// `destructive` is excluded because it already has its own `deny` rule and
    /// an id in two rules is noise in a file an operator has to read: the
    /// second listing changes no outcome and invites the reader to wonder which
    /// one applies.
    ///
    /// The effect is `deny`, which is what these modules already got from
    /// `default_effect: deny` by falling through every rule. Stating it changes
    /// no decision today and buys two things. It survives an operator setting
    /// `default_effect: allow`, where silence would otherwise turn into blanket
    /// network access. And it gives network reach one place to be granted: an
    /// operator who wants `git fetch` back edits this rule — or moves those ids
    /// out of it — instead of discovering by trial which of a hundred module
    /// ids were denied by nothing more than the absence of a rule.
    fn open_world_ids(modules: &[ScannedModule]) -> Vec<String> {
        modules
            .iter()
            .filter(|m| {
                m.annotations
                    .as_ref()
                    .is_some_and(|a| a.open_world && !a.destructive)
            })
            .map(|m| m.module_id.clone())
            .collect()
    }

    fn open_world_rule(targets: Vec<String>) -> ACLRule {
        // `approval` is deliberately left unset rather than paired with
        // `allow`, which is the other defensible reading of "the network needs
        // a human". That would *loosen* the shipped default from deny to
        // allow-with-a-prompt, and choosing to be reachable is an operator's
        // decision to make explicitly, not one to inherit from a generated
        // file. `ACLRule::new` sets neither `approval` nor `conditions`, which
        // is exactly what this rule wants.
        let mut rule = ACLRule::new(vec!["*".to_string()], targets, "deny");
        rule.description = Some(OPEN_WORLD_RULE_DESCRIPTION.to_string());
        rule
    }

    fn readonly_rule(targets: Vec<String>) -> ACLRule {
        // apcore 0.28 (PROTOCOL_SPEC §6.1.6) splits a rule's answer into
        // authorization and approval requirement. Read-only commands need
        // neither, so the second axis stays unset -- which is what
        // `ACLRule::new` leaves it as.
        let mut rule = ACLRule::new(vec!["*".to_string()], targets, "allow");
        rule.description = Some(READONLY_RULE_DESCRIPTION.to_string());
        rule
    }

    /// Destructive commands are denied outright here rather than gated on a
    /// human, which is deliberate for a *generated* default: apexe cannot know
    /// at scan time which caller should be trusted to confirm.
    ///
    /// Since apcore 0.28 the ACL layer *can* ask rather than only refuse — a
    /// rule may carry `approval: required` alongside `effect: allow`, and the
    /// built-in `arguments` condition (PROTOCOL_SPEC §6.1.7) scopes it to the
    /// calls that actually carry an escalating flag. That is the mechanism a
    /// hand-authored ACL should use; the generated default stays a refusal so
    /// an operator who never opens the file is not silently prompted instead
    /// of blocked. Approval for annotation-gated modules still runs through
    /// the Executor's ApprovalHandler (see `crate::module::build_executor`).
    fn destructive_ids(modules: &[ScannedModule]) -> Vec<String> {
        modules
            .iter()
            .filter(|m| m.annotations.as_ref().is_some_and(|a| a.destructive))
            .map(|m| m.module_id.clone())
            .collect()
    }

    fn destructive_rule(targets: Vec<String>) -> ACLRule {
        // `approval` MUST stay unset on a `deny` rule: §6.1.6 rule 2 rejects
        // `approval: required` paired with `deny` at every entry point,
        // because "refused AND put it to a human" means nothing.
        // `ACLRule::new` leaves it unset.
        let mut rule = ACLRule::new(vec!["*".to_string()], targets, "deny");
        rule.description = Some(DESTRUCTIVE_RULE_DESCRIPTION.to_string());
        rule
    }

    /// Write ACL to a YAML file.
    #[allow(clippy::result_large_err)] // ModuleError is 184 bytes; acceptable at crate boundary
    pub fn write_config(&self, path: &Path) -> Result<(), ModuleError> {
        let config = AclConfig {
            rules: self.acl.rules().to_vec(),
            default_effect: self.default_effect.clone(),
        };
        let yaml = serde_yaml::to_string(&config).map_err(|e| {
            ModuleError::new(
                ErrorCode::GeneralInternalError,
                format!("Failed to serialize ACL: {e}"),
            )
        })?;
        std::fs::write(path, yaml).map_err(|e| {
            ModuleError::new(
                ErrorCode::GeneralInternalError,
                format!("Failed to write ACL file: {e}"),
            )
        })?;
        Ok(())
    }

    /// Consume the manager and return the inner ACL.
    pub fn into_inner(self) -> ACL {
        self.acl
    }

    /// The `default_effect` this policy falls back to when no rule matches.
    pub fn default_effect(&self) -> &str {
        &self.default_effect
    }

    /// Explain how this policy would decide a call to `target_id`, for
    /// display (`apexe list --verbose`) rather than enforcement.
    ///
    /// Walks the rules in the same first-match-wins order [`ACL::check`]
    /// does, reusing [`target_patterns`] (which already resolves `$or`/`$not`
    /// against a pattern list) for both `callers` and `targets`. That reuse
    /// is safe rather than coincidental: apcore's own `match_acl_pattern` is
    /// a glob match identical to [`match_pattern`] for every caller spelling
    /// this method can be asked about — the only two literals it treats
    /// specially, `@external` and `@system`, agree with a plain glob match
    /// once there is no real caller (`@external` has no wildcard, so glob
    /// equality already means the same thing; `@system` needs a `Context`
    /// identity this method never has, and neither does a glob match against
    /// the literal string `"@external"`). So evaluating every rule against
    /// the fixed caller `"@external"` — [`ACL::check`]'s own fallback for a
    /// `None` caller — reproduces what an unauthenticated call would get.
    ///
    /// What it cannot reproduce is a rule's `conditions` block: those run
    /// through apcore's registered condition handlers against a real
    /// caller's identity and role, which this method has neither of.
    /// [`AclDecision::matched_rule_has_conditions`] flags that gap rather
    /// than silently guessing.
    pub fn explain(&self, target_id: &str) -> AclDecision {
        // apcore's own sentinel rather than a second copy of the literal: the
        // value has to agree with what `ACL::check_inner` resolves a `None`
        // caller to, and a private constant here could drift from it silently.
        const GENERIC_CALLER: &str = apcore::EXTERNAL_CALLER;
        let rule_matches = |patterns: &[String], value: &str| {
            target_patterns(patterns)
                .iter()
                .any(|p| p.negated != match_pattern(p.pattern, value))
        };
        for (idx, rule) in self.acl.rules().iter().enumerate() {
            if rule_matches(&rule.callers, GENERIC_CALLER) && rule_matches(&rule.targets, target_id)
            {
                return AclDecision {
                    effect: rule.effect.clone(),
                    default_effect: self.default_effect.clone(),
                    matched_rule_index: Some(idx),
                    matched_rule_description: rule.description.clone(),
                    matched_rule_has_conditions: rule.conditions.is_some(),
                };
            }
        }
        AclDecision {
            effect: self.default_effect.clone(),
            default_effect: self.default_effect.clone(),
            matched_rule_index: None,
            matched_rule_description: None,
            matched_rule_has_conditions: false,
        }
    }

    /// Read `default_effect` from a YAML file (best-effort, falls back to "deny").
    fn read_default_effect(path: &Path) -> String {
        std::fs::read_to_string(path)
            .ok()
            .and_then(|s| serde_yaml::from_str::<serde_json::Value>(&s).ok())
            .and_then(|v| v.get("default_effect")?.as_str().map(String::from))
            .unwrap_or_else(|| "deny".to_string())
    }
}

/// The outcome of [`AclManager::explain`] for one module id.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct AclDecision {
    /// What this policy would decide: `"allow"` or `"deny"`.
    pub effect: String,
    /// The policy's `default_effect`, for context when no rule matched.
    pub default_effect: String,
    /// Index into the ACL file's `rules` list, when a rule matched rather
    /// than the call falling through to `default_effect`.
    pub matched_rule_index: Option<usize>,
    /// The matched rule's `description`, if it has one.
    pub matched_rule_description: Option<String>,
    /// Whether the matched rule also carries a `conditions` block. Those
    /// evaluate against a real caller's identity/role at call time, which
    /// `explain` has no access to, so `effect` may not hold for every caller
    /// when this is `true`.
    pub matched_rule_has_conditions: bool,
}

/// Compound-operator sentinels apcore recognises as the FIRST element of a
/// `callers`/`targets` list. `$or` consumes every following pattern; `$not`
/// consumes exactly one.
const OR_SENTINEL: &str = "$or";
const NOT_SENTINEL: &str = "$not";

/// Maximum number of near-miss module ids named in one diagnostic.
const MAX_SUGGESTIONS: usize = 3;

/// A target pattern that matches none of the module ids actually registered.
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub struct UnmatchedTarget {
    /// Index of the rule in the ACL file's `rules` list.
    pub rule_index: usize,
    /// The rule's declared effect.
    pub effect: String,
    /// The literal pattern as written in the ACL file.
    pub pattern: String,
    /// `true` when `pattern` is the operand of a `$not`, which inverts what
    /// "matches nothing" means for the rule — see [`TargetPattern`].
    pub negated: bool,
    /// Registered module ids that differ from `pattern` only in separator,
    /// case, or leading segments — i.e. the spelling the operator probably
    /// meant. Empty when nothing close is registered.
    pub suggestions: Vec<String>,
}

impl UnmatchedTarget {
    /// Whether a registered module is a near-identical spelling of this
    /// pattern. A near miss proves the module *is* present under a different
    /// spelling, which makes the pattern a typo rather than a forward-looking
    /// rule for a module that does not exist here yet.
    pub fn is_near_miss(&self) -> bool {
        !self.suggestions.is_empty()
    }
}

/// Outcome of checking a loaded ACL against the registry it will guard.
#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)]
pub struct AclValidationReport {
    /// Target patterns that match no registered module id.
    ///
    /// The report used to carry a second kind, `inert_rules`, for a `callers` or
    /// `targets` list apcore's matcher could never satisfy (`[]`, a bare `$or`,
    /// a bare `$not`). apcore 0.29 closed that shape at every door it exposes —
    /// `ACL::load` and `try_new` refuse it, the infallible `ACL::new` and
    /// `add_rule` panic on it (apcore#112) — so no such rule can reach this
    /// function, from a file or from a library consumer. Detecting it here was
    /// then unreachable code asserting a guarantee upstream now makes, which is
    /// the duplication worth removing rather than keeping as a belt.
    ///
    /// Near-miss detection stays, because apcore does not do it: a rule naming
    /// `cli.git.cat-file` when the registry holds `cli.git.cat_file` is
    /// well-formed by every upstream check and still protects nothing.
    pub unmatched_targets: Vec<UnmatchedTarget>,
}

impl AclValidationReport {
    /// Whether the findings warrant refusing to start. See
    /// [`validate_acl_rules`] for the refuse-vs-warn split.
    pub fn is_fatal(&self) -> bool {
        self.unmatched_targets
            .iter()
            .any(UnmatchedTarget::is_near_miss)
    }

    /// Log the non-fatal findings — target patterns that match nothing today
    /// but are not typos of anything registered.
    pub fn emit_warnings(&self) {
        for target in self.unmatched_targets.iter().filter(|t| !t.is_near_miss()) {
            if target.negated {
                tracing::warn!(
                    rule_index = target.rule_index,
                    effect = %target.effect,
                    pattern = %target.pattern,
                    "ACL `$not` operand matches no registered module, so apcore matches \
                     this rule against EVERY module rather than none. Under \
                     first-match-wins an `allow` here nullifies every rule below it. \
                     Harmless if the operand anticipates a module this server does not \
                     serve (a glob, or an ACL shared with a differently filtered \
                     server); a typo otherwise."
                );
            } else {
                tracing::warn!(
                    rule_index = target.rule_index,
                    effect = %target.effect,
                    pattern = %target.pattern,
                    "ACL target matches no registered module — this rule currently \
                     protects nothing. Harmless if the pattern anticipates a module \
                     this server does not serve (a glob, or an ACL shared with a \
                     differently filtered server); a typo otherwise."
                );
            }
        }
    }

    /// Build the refusal error for the fatal findings, or `None` when there
    /// are none.
    pub fn fatal_error(&self, acl_path: &Path) -> Option<ModuleError> {
        if !self.is_fatal() {
            return None;
        }
        let lines: Vec<String> = self
            .unmatched_targets
            .iter()
            .filter(|t| t.is_near_miss())
            .map(describe_near_miss)
            .collect();
        Some(
            ModuleError::new(
                ErrorCode::GeneralInvalidInput,
                format!(
                    "ACL '{}' contains {} rule(s) that protect nothing:\n{}\n\
                     Refusing to start: an operator who asked for access control must not \
                     silently get less of it than they wrote.",
                    acl_path.display(),
                    lines.len(),
                    lines.join("\n")
                ),
            )
            .with_retryable(false),
        )
    }
}

/// One human-readable line describing a target that is a typo of a registered
/// module id.
fn describe_near_miss(target: &UnmatchedTarget) -> String {
    if target.negated {
        return format!(
            "  - rule {} ({}): `$not` operand '{}' matches no registered module, so this rule \
             applies to every module instead of excluding one; did you mean {}?",
            target.rule_index,
            target.effect,
            target.pattern,
            target.suggestions.join(" or ")
        );
    }
    format!(
        "  - rule {} ({}): target '{}' matches no registered module; did you mean {}?",
        target.rule_index,
        target.effect,
        target.pattern,
        target.suggestions.join(" or ")
    )
}

/// Check a loaded ACL's rules against the module ids that are actually
/// registered on this server.
///
/// # Why this cannot live in [`AclManager::from_config`]
///
/// The ACL is loaded before anything knows which modules exist.
/// `crate::module::build_executor` populates the `Registry` first and
/// constructs the ACL second, so it is the only place where both sets are in
/// hand — and the ids must come from the registry rather than from the
/// bindings directory, because `ModuleFilter` deliberately drops modules at
/// registration time.
///
/// # Refuse vs. warn
///
/// **Refuse** on a structurally inert rule (`targets: []`, `callers: []`, a
/// bare `$not`/`$or`). apcore's `match_patterns` returns `false` for an empty
/// pattern list, so such a rule can never fire against *any* registry, present
/// or future. apcore already rejects an *omitted* `callers`/`targets` key for
/// exactly this reason; accepting the empty list leaves the same hole one
/// character away. This is the posture `--acl` already takes on a missing or
/// malformed file.
///
/// **Refuse** on a target that matches nothing while a registered module id
/// differs from it only in separator, case, or leading segments
/// (`cli.git.cat-file` vs. the registered `cli.git.cat_file`; `cp` vs.
/// `cli.cp`). The near miss is proof the module is present under another
/// spelling, so the rule is a typo whose subject is live and unguarded.
///
/// **Warn** on any other target that matches nothing. A glob is forward-
/// looking by design (`cli.*.status` may match a module tomorrow's scan adds),
/// and one ACL file is meant to be shared across servers whose registries
/// differ (`--prefix`, `--tags`, different scans). A target naming a module
/// that is not registered also cannot itself let anything through: the
/// registry has nothing to serve under that name. Refusing here would turn a
/// portable policy file into a per-host one, which is a worse failure than the
/// warning.
///
/// The target check is skipped entirely when the registry is empty — there is
/// nothing to validate against, and every pattern would be reported.
pub fn validate_acl_rules(rules: &[ACLRule], registered_ids: &[String]) -> AclValidationReport {
    let mut report = AclValidationReport::default();
    for (rule_index, rule) in rules.iter().enumerate() {
        if registered_ids.is_empty() {
            continue;
        }
        collect_unmatched_targets(rule_index, rule, registered_ids, &mut report);
    }
    report
}

/// Record the rule's target patterns that match no registered module id.
fn collect_unmatched_targets(
    rule_index: usize,
    rule: &ACLRule,
    registered_ids: &[String],
    report: &mut AclValidationReport,
) {
    for target in target_patterns(&rule.targets) {
        if registered_ids
            .iter()
            .any(|id| match_pattern(target.pattern, id))
        {
            continue;
        }
        report.unmatched_targets.push(UnmatchedTarget {
            rule_index,
            effect: rule.effect.clone(),
            pattern: target.pattern.to_string(),
            negated: target.negated,
            suggestions: suggest_similar(target.pattern, registered_ids),
        });
    }
}

/// One entry of a `targets` list, with apcore's `$not` sentinel resolved into
/// the polarity it gives the pattern.
///
/// The polarity is not cosmetic: it inverts what "this pattern matches nothing
/// registered" *means*. Positively, the rule fires for no module and protects
/// nothing. Under `$not`, apcore evaluates `!match(operand, module_id)`, so an
/// operand matching nothing makes the rule fire for **every** module — and
/// under first-match-wins a blanket `allow` there nullifies every deny below
/// it. Reporting the two the same way told the operator the opposite of what
/// was happening.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
struct TargetPattern<'a> {
    /// The module-id pattern, sentinel stripped.
    pattern: &'a str,
    /// `true` when `pattern` is the operand of a leading `$not`.
    negated: bool,
}

/// The concrete module-id patterns in a `targets` list, with apcore's compound
/// sentinels stripped.
///
/// Mirrors `ACL::match_patterns`: a leading `$or` makes every following entry a
/// pattern, and a leading `$not` consumes exactly *one*. Anything after a
/// `$not` operand is never consulted by the matcher, so it is not reported —
/// flagging it would be a false alarm about a pattern that has no effect either
/// way.
fn target_patterns(targets: &[String]) -> Vec<TargetPattern<'_>> {
    fn positive(pattern: &str) -> TargetPattern<'_> {
        TargetPattern {
            pattern,
            negated: false,
        }
    }
    match targets.first().map(String::as_str) {
        Some(OR_SENTINEL) => targets[1..]
            .iter()
            .map(String::as_str)
            .map(positive)
            .collect(),
        Some(NOT_SENTINEL) => targets
            .get(1)
            .map(|pattern| TargetPattern {
                pattern: pattern.as_str(),
                negated: true,
            })
            .into_iter()
            .collect(),
        _ => targets.iter().map(String::as_str).map(positive).collect(),
    }
}

/// Registered ids that differ from `pattern` only in separator, case, or
/// leading segments.
///
/// Deliberately not a general edit distance: the spellings operators actually
/// reach for are the pre-#19 hyphenated id (`cli.git.cat-file`), the bare
/// command (`cp`), and the shell invocation (`git log`). Normalising every
/// separator to `.` and testing for equality or a trailing-segment match names
/// exactly those without inventing matches between unrelated modules.
fn suggest_similar(pattern: &str, registered_ids: &[String]) -> Vec<String> {
    let needle = normalize_id(pattern);
    if needle.is_empty() || needle.contains('*') {
        return vec![];
    }
    let suffix = format!(".{needle}");
    let mut hits: Vec<String> = registered_ids
        .iter()
        .filter(|id| {
            let normalized = normalize_id(id);
            normalized == needle || normalized.ends_with(&suffix)
        })
        .cloned()
        .collect();
    hits.truncate(MAX_SUGGESTIONS);
    hits
}

/// Fold the spellings that differ only in separator or case into one form.
fn normalize_id(id: &str) -> String {
    id.trim()
        .to_ascii_lowercase()
        .chars()
        .map(|c| {
            if c == '-' || c == '_' || c == '/' || c.is_whitespace() {
                '.'
            } else {
                c
            }
        })
        .collect()
}

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

    fn make_module_with_annotations(id: &str, readonly: bool, destructive: bool) -> ScannedModule {
        let mut module = ScannedModule::new(
            id.to_string(),
            format!("Test {id}"),
            json!({"type": "object"}),
            json!({"type": "object"}),
            vec!["cli".to_string()],
            format!("exec:///usr/bin/test {id}"),
        );
        module.annotations = Some(apcore::module::ModuleAnnotations {
            readonly,
            destructive,
            requires_approval: destructive,
            // Set explicitly, never defaulted: apcore's default is `true`
            // (MCP's conservative `openWorldHint`), which would make every
            // fixture here an open-world command and quietly empty the
            // auto-allow rule these cases are about. `annotations::infer`
            // always sets the field, so an explicit value is also the
            // realistic one -- see the same trap and the same fix in
            // `adapter::contract`'s test helper.
            open_world: false,
            ..Default::default()
        });
        module
    }

    #[test]
    fn test_acl_generate_default_readonly() {
        let modules = vec![
            make_module_with_annotations("cli.git.status", true, false),
            make_module_with_annotations("cli.git.log", true, false),
        ];
        let mgr = AclManager::generate_default(&modules);
        let rules = mgr.acl.rules();
        assert_eq!(rules.len(), 1);
        assert_eq!(rules[0].effect, "allow");
        assert_eq!(rules[0].targets.len(), 2);
    }

    #[test]
    fn test_acl_generate_default_destructive() {
        let modules = vec![make_module_with_annotations("cli.git.clean", false, true)];
        let mgr = AclManager::generate_default(&modules);
        let rules = mgr.acl.rules();
        assert_eq!(rules.len(), 1);
        assert_eq!(rules[0].effect, "deny");
        // Must be an unconditional deny: apcore only registers `identity_types`,
        // `roles`, `max_call_depth`, `$or`, `$not` as condition keys, so any
        // other key (e.g. `require_approval`) can never match and would let
        // the rule silently fall through instead of denying.
        assert!(rules[0].conditions.is_none());
    }

    #[test]
    fn test_acl_generate_default_destructive_rule_denies_on_its_own() {
        // Regression for the CRITICAL finding: the destructive-deny rule
        // previously carried `conditions: Some({"require_approval": true})`,
        // a condition key apcore never registers. An unregistered condition
        // key can never be satisfied, so the rule could never match — it
        // relied entirely on `default_effect: "deny"` for protection. Prove
        // the rule denies on its own merits by appending a more permissive
        // rule after it (lower priority) and confirming the deny still wins,
        // which only happens if the deny rule actually matches.
        let modules = vec![make_module_with_annotations("cli.git.clean", false, true)];
        let mgr = AclManager::generate_default(&modules);
        let mut rules = mgr.acl.rules().to_vec();
        rules.push(ACLRule::new(
            vec!["*".to_string()],
            vec!["cli.git.clean".to_string()],
            "allow",
        ));
        let acl = ACL::new(rules, "allow", None);
        assert!(
            !acl.check(None, "cli.git.clean", None),
            "destructive command must be denied by its own ACL rule, not merely \
             by a coincidental default_effect"
        );
    }

    fn make_module_with_open_world(
        id: &str,
        readonly: bool,
        destructive: bool,
        open_world: bool,
    ) -> ScannedModule {
        let mut module = make_module_with_annotations(id, readonly, destructive);
        if let Some(a) = module.annotations.as_mut() {
            a.open_world = open_world;
        }
        module
    }

    #[test]
    fn test_acl_generate_default_open_world_rule_denies_on_its_own() {
        // Same shape as the destructive regression above, and for the same
        // reason: these modules were already denied by `default_effect: deny`
        // while matching no rule at all, so the only way to show the new rule
        // is load-bearing is to flip the default to `allow` and confirm the
        // deny still wins. That flip is not hypothetical — it is precisely the
        // configuration in which silence would have become blanket network
        // access.
        let modules = vec![make_module_with_open_world("cli.curl", false, false, true)];
        let mgr = AclManager::generate_default(&modules);
        let acl = ACL::new(mgr.acl.rules().to_vec(), "allow", None);
        assert!(
            !acl.check(None, "cli.curl", None),
            "an open-world command must be denied by its own rule, not merely by \
             a coincidental default_effect"
        );
    }

    #[test]
    fn test_acl_open_world_is_not_auto_allowed_even_when_readonly() {
        // `readonly` means the command does not modify local state. It says
        // nothing about what the command sends, and a read that reaches the
        // network is the exfiltration shape. Inference alone never produces
        // this combination -- the two name lists do not intersect -- but an
        // overlay can assert both, which became reachable when `open_world`
        // joined the overlay schema.
        let modules = vec![make_module_with_open_world("cli.curl", true, false, true)];
        let mgr = AclManager::generate_default(&modules);

        let allow_rule = mgr
            .acl
            .rules()
            .iter()
            .find(|r| r.description.as_deref() == Some(READONLY_RULE_DESCRIPTION));
        assert!(
            allow_rule.is_none(),
            "a readonly command that reaches the network must not land in the \
             auto-allow rule"
        );
        assert!(
            !mgr.acl.check(None, "cli.curl", None),
            "and it must actually be denied"
        );
    }

    #[test]
    fn test_acl_destructive_open_world_is_listed_in_one_rule_only() {
        // Both rules deny, so a double listing changes no decision -- it just
        // makes an operator read the same id twice and wonder which applies.
        let modules = vec![make_module_with_open_world(
            "cli.git.push",
            false,
            true,
            true,
        )];
        let mgr = AclManager::generate_default(&modules);
        let listings: Vec<_> = mgr
            .acl
            .rules()
            .iter()
            .filter(|r| r.targets.iter().any(|t| t == "cli.git.push"))
            .map(|r| r.description.clone().unwrap_or_default())
            .collect();
        assert_eq!(
            listings,
            vec![DESTRUCTIVE_RULE_DESCRIPTION.to_string()],
            "destructive already denies it; the open-world rule must not repeat it"
        );
    }

    #[test]
    fn test_acl_merge_default_updates_the_open_world_rule() {
        let dir = tempfile::tempdir().unwrap();
        let path = dir.path().join("acl.yaml");

        let first = vec![make_module_with_open_world("cli.curl", false, false, true)];
        AclManager::generate_default(&first)
            .write_config(&path)
            .unwrap();

        // A second scan of a different tool must extend the rule, not replace it.
        let second = vec![make_module_with_open_world("cli.wget", false, false, true)];
        let merged = AclManager::merge_default(&path, &second).unwrap();
        let rule = merged
            .acl
            .rules()
            .iter()
            .find(|r| r.description.as_deref() == Some(OPEN_WORLD_RULE_DESCRIPTION))
            .expect("the open-world rule must survive a merge");
        assert!(rule.targets.contains(&"cli.curl".to_string()));
        assert!(rule.targets.contains(&"cli.wget".to_string()));
    }

    #[test]
    fn test_acl_generate_default_mixed() {
        let modules = vec![
            make_module_with_annotations("cli.git.status", true, false),
            make_module_with_annotations("cli.git.clean", false, true),
        ];
        let mgr = AclManager::generate_default(&modules);
        let rules = mgr.acl.rules();
        assert_eq!(rules.len(), 2);
    }

    #[test]
    fn test_acl_generate_default_empty() {
        let mgr = AclManager::generate_default(&[]);
        let rules = mgr.acl.rules();
        assert!(rules.is_empty());
        assert_eq!(mgr.default_effect, "deny");
    }

    #[test]
    fn test_acl_merge_default_preserves_targets_from_an_earlier_scan() {
        // Regression: `apexe scan ls` then `apexe scan echo` must not lose
        // the ls allow rule just because echo's batch doesn't happen to
        // contain any readonly module. The bug: write_acl regenerated the
        // whole file from the CURRENT BATCH alone and truncate-wrote it, so
        // cli.ls's allow rule vanished the moment a second scan ran.
        let tmp = tempfile::TempDir::new().unwrap();
        let path = tmp.path().join("acl.yaml");

        let first_batch = vec![make_module_with_annotations("cli.ls", true, false)];
        AclManager::generate_default(&first_batch)
            .write_config(&path)
            .unwrap();

        let second_batch = vec![make_module_with_annotations("cli.echo", false, false)];
        let merged = AclManager::merge_default(&path, &second_batch).unwrap();

        let readonly_rule = merged
            .acl
            .rules()
            .iter()
            .find(|r| r.description.as_deref() == Some(READONLY_RULE_DESCRIPTION))
            .expect("the readonly-allow rule from the first scan must survive");
        assert!(
            readonly_rule.targets.contains(&"cli.ls".to_string()),
            "{:?}",
            readonly_rule.targets
        );
    }

    #[test]
    fn test_acl_merge_default_moves_a_module_between_rules_when_its_annotations_change() {
        // A module rescanned with different annotations must move to its
        // new rule and out of its old one, not linger in both.
        let tmp = tempfile::TempDir::new().unwrap();
        let path = tmp.path().join("acl.yaml");

        let first_batch = vec![make_module_with_annotations("cli.tool", true, false)];
        AclManager::generate_default(&first_batch)
            .write_config(&path)
            .unwrap();

        // Same module, now destructive instead of readonly.
        let second_batch = vec![make_module_with_annotations("cli.tool", false, true)];
        let merged = AclManager::merge_default(&path, &second_batch).unwrap();

        let readonly_rule = merged
            .acl
            .rules()
            .iter()
            .find(|r| r.description.as_deref() == Some(READONLY_RULE_DESCRIPTION));
        if let Some(rule) = readonly_rule {
            assert!(
                !rule.targets.contains(&"cli.tool".to_string()),
                "{:?}",
                rule.targets
            );
        }
        let destructive_rule = merged
            .acl
            .rules()
            .iter()
            .find(|r| r.description.as_deref() == Some(DESTRUCTIVE_RULE_DESCRIPTION))
            .expect("cli.tool must now be in the destructive-deny rule");
        assert!(destructive_rule.targets.contains(&"cli.tool".to_string()));
    }

    #[test]
    fn test_acl_merge_default_keeps_hand_authored_rules_untouched() {
        let tmp = tempfile::TempDir::new().unwrap();
        let path = tmp.path().join("acl.yaml");
        std::fs::write(
            &path,
            "rules:\n  - callers: [\"*\"]\n    targets: [\"cli.special\"]\n    effect: deny\n    description: Hand-authored exception\ndefault_effect: allow\n",
        )
        .unwrap();

        let batch = vec![make_module_with_annotations("cli.ls", true, false)];
        let merged = AclManager::merge_default(&path, &batch).unwrap();

        assert_eq!(
            merged.default_effect, "allow",
            "existing default_effect must be preserved"
        );
        let hand_rule = merged
            .acl
            .rules()
            .iter()
            .find(|r| r.description.as_deref() == Some("Hand-authored exception"))
            .expect("hand-authored rule must survive a merge");
        assert_eq!(hand_rule.targets, vec!["cli.special".to_string()]);
    }

    #[test]
    fn test_acl_merge_default_falls_back_to_generate_default_when_no_file_exists() {
        let tmp = tempfile::TempDir::new().unwrap();
        let path = tmp.path().join("does-not-exist.yaml");
        let batch = vec![make_module_with_annotations("cli.ls", true, false)];
        let result = AclManager::merge_default(&path, &batch);
        assert!(
            result.is_err(),
            "merging against a missing file should error, not silently start from empty"
        );
    }

    #[test]
    fn test_acl_write_and_load() {
        let tmp = tempfile::TempDir::new().unwrap();
        let path = tmp.path().join("acl_manager.yaml");

        let modules = vec![
            make_module_with_annotations("cli.git.status", true, false),
            make_module_with_annotations("cli.git.clean", false, true),
        ];
        let mgr = AclManager::generate_default(&modules);
        mgr.write_config(&path).unwrap();

        let loaded = AclManager::from_config(&path).unwrap();
        assert_eq!(loaded.acl.rules().len(), 2);
        assert_eq!(loaded.default_effect, "deny");
    }

    fn rule(targets: &[&str], effect: &str) -> ACLRule {
        ACLRule::new(
            vec!["*".to_string()],
            targets.iter().map(|t| (*t).to_string()).collect(),
            effect,
        )
    }

    fn registered(ids: &[&str]) -> Vec<String> {
        ids.iter().map(|id| (*id).to_string()).collect()
    }

    #[test]
    fn test_validate_acl_rules_accepts_exact_registered_target() {
        let report = validate_acl_rules(
            &[rule(&["cli.cp"], "deny")],
            &registered(&["cli.cp", "cli.ls"]),
        );
        assert_eq!(report, AclValidationReport::default());
        assert!(!report.is_fatal());
    }

    /// The inert shapes apexe used to detect are refused by apcore itself now.
    ///
    /// apexe carried a `never_matches` detector for a `callers`/`targets` list
    /// apcore's matcher could never satisfy — `[]`, a bare `$or`, a bare `$not`.
    /// apcore 0.29 closed that shape at every door (apcore#112), so the detector
    /// became unreachable and was removed rather than kept as a belt over a
    /// guarantee upstream now makes.
    ///
    /// This test replaces it by pinning the guarantee where it now lives: both
    /// doors, both directions. Without it, an upstream regression would silently
    /// reopen the hole apexe used to cover, and nothing here would notice.
    #[test]
    fn test_apcore_refuses_an_inert_pattern_list_at_every_door() {
        for (label, callers, targets) in [
            ("empty targets", vec!["*".to_string()], vec![]),
            ("empty callers", vec![], vec!["cli.cp".to_string()]),
            (
                "bare $or",
                vec!["*".to_string()],
                vec![OR_SENTINEL.to_string()],
            ),
            (
                "bare $not",
                vec!["*".to_string()],
                vec![NOT_SENTINEL.to_string()],
            ),
        ] {
            let mut r = ACLRule::new(vec!["*".to_string()], vec!["cli.cp".to_string()], "deny");
            r.callers = callers;
            r.targets = targets;

            // The fallible door reports it...
            assert!(
                ACL::try_new(vec![r.clone()], "deny", None).is_err(),
                "{label}: ACL::try_new must refuse an inert pattern list"
            );
            // ...and the infallible one panics rather than admitting it.
            assert!(
                std::panic::catch_unwind(move || ACL::new(vec![r], "deny", None)).is_err(),
                "{label}: ACL::new must panic rather than build an inert rule"
            );
        }
    }

    #[test]
    fn test_validate_acl_rules_flags_hyphenated_id_as_near_miss() {
        // #39 item 5(a): the pre-#19 hyphenated id. The module is registered
        // under `cli.git.cat_file`, so this deny protects nothing.
        let report = validate_acl_rules(
            &[rule(&["cli.git.cat-file"], "deny")],
            &registered(&["cli.git.cat_file"]),
        );
        assert_eq!(report.unmatched_targets.len(), 1);
        assert_eq!(
            report.unmatched_targets[0].suggestions,
            vec!["cli.git.cat_file".to_string()]
        );
        assert!(report.unmatched_targets[0].is_near_miss());
        assert!(report.is_fatal());
    }

    #[test]
    fn test_validate_acl_rules_flags_bare_command_names_as_near_miss() {
        // The other three spellings from #39 item 5(a): `cp`, `ls`, `git log`.
        for (pattern, expected) in [
            ("cp", "cli.cp"),
            ("ls", "cli.ls"),
            ("git log", "cli.git.log"),
        ] {
            let report = validate_acl_rules(
                &[rule(&[pattern], "deny")],
                &registered(&["cli.cp", "cli.ls", "cli.git.log"]),
            );
            assert_eq!(
                report.unmatched_targets[0].suggestions,
                vec![expected.to_string()],
                "'{pattern}' should point at '{expected}'"
            );
            assert!(report.is_fatal(), "'{pattern}' should refuse to start");
        }
    }

    #[test]
    fn test_validate_acl_rules_warns_but_does_not_refuse_on_unknown_target() {
        // A target naming nothing close to a registered module is the shared-
        // ACL / not-yet-scanned case: report it, but do not refuse — the
        // registry has nothing to serve under that name anyway.
        let report = validate_acl_rules(
            &[rule(&["cli.kubectl.apply"], "deny")],
            &registered(&["cli.cp", "cli.ls"]),
        );
        assert_eq!(report.unmatched_targets.len(), 1);
        assert!(report.unmatched_targets[0].suggestions.is_empty());
        assert!(!report.is_fatal());
    }

    #[test]
    fn test_validate_acl_rules_accepts_working_globs() {
        // `cli.c*`, `*.log` and `cli.*.status` are verified working in #39 and
        // must not produce a finding — a validator that cries wolf on a
        // functioning rule is worse than no validator.
        let report = validate_acl_rules(
            &[
                rule(&["cli.c*"], "deny"),
                rule(&["*.log"], "allow"),
                rule(&["cli.*.status"], "allow"),
                rule(&["*"], "deny"),
            ],
            &registered(&["cli.cp", "cli.git.log", "cli.git.status"]),
        );
        assert_eq!(report, AclValidationReport::default());
    }

    #[test]
    fn test_validate_acl_rules_warns_on_glob_matching_nothing() {
        // A glob that matches nothing today may match a module added tomorrow,
        // so it is reported without refusing to start.
        let report = validate_acl_rules(&[rule(&["cli.z*"], "deny")], &registered(&["cli.cp"]));
        assert_eq!(report.unmatched_targets.len(), 1);
        assert!(!report.is_fatal());
    }

    #[test]
    fn test_validate_acl_rules_understands_compound_targets() {
        // `$or` makes every following entry a target; `$not` consumes exactly
        // one, so entries past the first are never consulted by apcore's
        // matcher and must not be reported.
        let report = validate_acl_rules(
            &[
                rule(&["$or", "cli.cp", "cli.ls"], "deny"),
                rule(&["$not", "cli.cp", "never.consulted"], "allow"),
            ],
            &registered(&["cli.cp", "cli.ls"]),
        );
        assert_eq!(report, AclValidationReport::default());
    }

    #[test]
    fn test_validate_acl_rules_flags_unmatched_operand_inside_compound() {
        let report = validate_acl_rules(
            &[rule(&["$or", "cli.cp", "cli.git.cat-file"], "deny")],
            &registered(&["cli.cp", "cli.git.cat_file"]),
        );
        assert_eq!(report.unmatched_targets.len(), 1);
        assert_eq!(report.unmatched_targets[0].pattern, "cli.git.cat-file");
        assert!(report.is_fatal());
    }

    #[test]
    fn test_validate_acl_rules_reports_a_not_operand_as_covering_everything() {
        // apcore evaluates `$not` as `!match(operand, module_id)`, so an
        // operand matching nothing makes the rule match EVERY module. The old
        // diagnostic stripped the sentinel and said "this rule currently
        // protects nothing" — the exact opposite, about a blanket allow that
        // under first-match-wins nullifies every deny below it.
        let report = validate_acl_rules(
            &[rule(&["$not", "cli.kubectl.apply"], "allow")],
            &registered(&["cli.cp", "cli.ls"]),
        );
        assert_eq!(report.unmatched_targets.len(), 1);
        let finding = &report.unmatched_targets[0];
        assert!(finding.negated, "the $not polarity must be carried through");
        assert_eq!(finding.pattern, "cli.kubectl.apply");
        // Not a typo of anything registered, so it warns rather than refuses —
        // the same shared-ACL allowance the positive case gets.
        assert!(!finding.is_near_miss());
        assert!(!report.is_fatal());
    }

    #[test]
    fn test_validate_acl_rules_refuses_a_near_miss_not_operand() {
        // The operator meant "every module except cli.git.cat_file" and wrote
        // the pre-#19 hyphenated spelling, so the one module the rule was
        // written to carve out is the one it now covers.
        let report = validate_acl_rules(
            &[rule(&["$not", "cli.git.cat-file"], "allow")],
            &registered(&["cli.git.cat_file", "cli.cp"]),
        );
        assert_eq!(report.unmatched_targets.len(), 1);
        assert!(report.unmatched_targets[0].negated);
        assert!(report.is_fatal());

        let message = report
            .fatal_error(Path::new("/etc/apexe/acl.yaml"))
            .expect("a near-miss $not operand must refuse to start")
            .message;
        assert!(message.contains("$not"), "{message}");
        assert!(
            message.contains("applies to every module"),
            "the diagnostic must not claim the rule protects nothing: {message}"
        );
        assert!(message.contains("cli.git.cat_file"), "{message}");
    }

    #[test]
    fn test_validate_acl_rules_skips_target_check_on_empty_registry() {
        // Nothing to validate against — reporting every pattern would be noise.
        let report = validate_acl_rules(&[rule(&["cli.cp"], "deny")], &[]);
        assert_eq!(report, AclValidationReport::default());
    }

    #[test]
    fn test_acl_validation_report_fatal_error_names_the_findings() {
        let report = validate_acl_rules(
            &[rule(&["cli.git.cat-file"], "deny")],
            &registered(&["cli.git.cat_file"]),
        );
        let err = report
            .fatal_error(Path::new("/etc/apexe/acl.yaml"))
            .expect("report should be fatal");
        assert_eq!(err.code, ErrorCode::GeneralInvalidInput);
        assert!(err.message.contains("/etc/apexe/acl.yaml"));
        // The near miss names the spelling that IS registered, which is the
        // whole value of the diagnostic: the operator wrote a rule that looks
        // right and guards nothing.
        assert!(err.message.contains("cli.git.cat_file"));
        assert!(err.message.contains("cli.git.cat-file"));
    }

    #[test]
    fn test_acl_validation_report_fatal_error_none_when_clean() {
        let report = validate_acl_rules(&[rule(&["cli.cp"], "deny")], &registered(&["cli.cp"]));
        assert!(report.fatal_error(Path::new("/tmp/acl.yaml")).is_none());
    }

    #[test]
    fn test_normalize_id_folds_separators_and_case() {
        assert_eq!(normalize_id("cli.git.cat-file"), "cli.git.cat.file");
        assert_eq!(normalize_id("CLI_GIT_LOG"), "cli.git.log");
        assert_eq!(normalize_id("  git log  "), "git.log");
    }

    #[test]
    fn test_suggest_similar_does_not_guess_across_unrelated_modules() {
        // A trailing-segment match is the whole heuristic; unrelated ids that
        // merely share letters must not be offered as "did you mean".
        assert!(suggest_similar("clip", &registered(&["cli.cp"])).is_empty());
        assert!(suggest_similar("cli.c*", &registered(&["cli.cp"])).is_empty());
    }

    #[tokio::test]
    async fn test_acl_decision_recorded_via_audit_logger() {
        // F5: with an audit logger attached, ACL allow/deny decisions are
        // persisted (so an operator can answer "who was denied which module").
        use std::sync::Arc;
        let tmp = tempfile::TempDir::new().unwrap();
        let audit_path = tmp.path().join("audit.jsonl");
        let audit = Arc::new(crate::governance::AuditManager::new(&audit_path));

        let modules = vec![make_module_with_annotations("cli.rm", false, true)];
        let mut acl = AclManager::generate_default(&modules).into_inner();
        {
            let audit = audit.clone();
            acl.set_audit_logger(move |entry| audit.log_acl_decision(entry));
        }

        // A destructive module is denied for an external caller; audited.
        let allowed = acl.check(Some(apcore::EXTERNAL_CALLER), "cli.rm", None);
        assert!(!allowed);

        // `log_acl_decision` offloads its write via a fire-and-forget
        // `spawn_blocking` (it is invoked from apcore's synchronous `ACL`
        // callback, which has no `.await` point to offload onto), so it can
        // still be in flight when `check` returns. Poll briefly rather than
        // asserting immediately.
        let mut content = String::new();
        for _ in 0..200 {
            content = std::fs::read_to_string(&audit_path).unwrap_or_default();
            if !content.is_empty() {
                break;
            }
            tokio::time::sleep(std::time::Duration::from_millis(5)).await;
        }
        assert!(
            content.contains("\"decision\""),
            "ACL decision not recorded: {content}"
        );
        assert!(content.contains("cli.rm"));

        // The audit log must be owner-only (0o600) even when an ACL denial is
        // the first write — it carries caller identities and denied targets.
        #[cfg(unix)]
        {
            use std::os::unix::fs::PermissionsExt;
            let mode = std::fs::metadata(&audit_path).unwrap().permissions().mode();
            assert_eq!(mode & 0o777, 0o600, "audit log should be owner-only");
        }
    }

    // ---- AclManager::explain, for `apexe list --verbose` -----------------

    #[test]
    fn test_explain_reports_the_generated_readonly_allow_rule() {
        let modules = vec![make_module_with_annotations("cli.git.status", true, false)];
        let mgr = AclManager::generate_default(&modules);

        let decision = mgr.explain("cli.git.status");

        assert_eq!(decision.effect, "allow");
        assert_eq!(decision.matched_rule_index, Some(0));
        assert_eq!(
            decision.matched_rule_description.as_deref(),
            Some(READONLY_RULE_DESCRIPTION)
        );
        assert!(!decision.matched_rule_has_conditions);
    }

    #[test]
    fn test_explain_reports_the_generated_destructive_deny_rule() {
        let modules = vec![make_module_with_annotations("cli.rm", false, true)];
        let mgr = AclManager::generate_default(&modules);

        let decision = mgr.explain("cli.rm");

        assert_eq!(decision.effect, "deny");
        assert_eq!(
            decision.matched_rule_description.as_deref(),
            Some(DESTRUCTIVE_RULE_DESCRIPTION)
        );
    }

    #[test]
    fn test_explain_falls_back_to_default_effect_when_no_rule_matches() {
        let modules = vec![make_module_with_annotations("cli.echo", false, false)];
        let mgr = AclManager::generate_default(&modules);

        let decision = mgr.explain("cli.echo");

        assert_eq!(decision.effect, "deny");
        assert_eq!(decision.default_effect, "deny");
        assert_eq!(decision.matched_rule_index, None);
        assert_eq!(decision.matched_rule_description, None);
    }

    #[test]
    fn test_explain_agrees_with_acl_check_for_an_external_caller() {
        // `explain` reimplements first-match-wins for display; it must never
        // report an effect `ACL::check` itself would disagree with.
        let modules = vec![
            make_module_with_annotations("cli.git.status", true, false),
            make_module_with_annotations("cli.rm", false, true),
            make_module_with_annotations("cli.echo", false, false),
        ];
        let mgr = AclManager::generate_default(&modules);

        for id in ["cli.git.status", "cli.rm", "cli.echo"] {
            let decision = mgr.explain(id);
            let allowed = mgr.acl.check(Some(apcore::EXTERNAL_CALLER), id, None);
            assert_eq!(
                decision.effect == "allow",
                allowed,
                "explain({id}) disagreed with ACL::check"
            );
        }
    }

    #[test]
    fn test_explain_flags_a_matched_rule_that_carries_conditions() {
        let mut rule = ACLRule::new(
            vec!["*".to_string()],
            vec!["cli.deploy".to_string()],
            "allow",
        );
        rule.description = Some("Ops-only deploy".to_string());
        rule.conditions = Some(json!({"roles": ["ops"]}));
        let acl = ACL::new(vec![rule], "deny", None);
        let mgr = AclManager {
            acl,
            default_effect: "deny".to_string(),
        };

        let decision = mgr.explain("cli.deploy");

        assert!(decision.matched_rule_has_conditions);
    }

    #[test]
    fn test_default_effect_accessor() {
        let mgr = AclManager::generate_default(&[]);
        assert_eq!(mgr.default_effect(), "deny");
    }
}