fix(novelty): skill shorten and analyze refuse an unusable --cache-dir up front (#701)

dokimi/changelog.d/20261114_130000_cache-dir-unusable.md

@@ -0,0 +1,3 @@
+### Fixed
+
+- **`skill shorten` and `skill novelty analyze` refuse a `--cache-dir` they cannot store into, before any paid call** — a `--cache-dir` naming a regular file (or whose parent is one, a dangling symlink, an unwritable directory, or a path below a directory without search permission) ended both commands in a traceback on the first retention adjudication (`FileExistsError` for a regular file, another `OSError` for the rest). They now exit 2 with `--cache-dir <path> is not a directory` (or names the unwritable directory). A directory that exists, and one that does not exist yet, are taken as before.

dokimi/dokimi/commands/novelty.py

@@ -19,7 +19,7 @@
 
 from dokimi.commands.inputs import placebo_text_for_arms_or_exit
 from dokimi.commands.novelty_run import (add_judge2_args, add_spend_cap, judge2_client,
-                                         exit_if_analyze_partial, stamp)
+                                         exit_if_analyze_partial, stamp, usable_cache_dir)
 from dokimi.commands.novelty_routing_inputs import (
     checked_out_path,
     load_all_vs_all_field,
@@ -109,6 +109,7 @@ def cmd_novelty_analyze(args: argparse.Namespace) -> None:
                 file=sys.stderr,
             )
         return
+    cache_dir = usable_cache_dir(args.cache_dir)
     print_run_estimate(args, settings, artifact_bundle.text, cost.estimate_usd)
 
     placebo_text = placebo_text_for_arms_or_exit(args.placebo_text, _arms(args.arms))
@@ -125,7 +126,7 @@ def cmd_novelty_analyze(args: argparse.Namespace) -> None:
         concurrency=args.concurrency,
         allow_self_judge=args.allow_self_judge,
         placebo_text=placebo_text, context_framing=args.context_framing,
-        cache_dir=Path(args.cache_dir) if args.cache_dir else None,
+        cache_dir=cache_dir,
         seed=args.seed,
         include_resources=args.include_resources,
         resource_roles=tuple(args.resource_roles) if args.resource_roles else None,

dokimi/dokimi/commands/novelty_atoms_file.py

@@ -180,16 +180,16 @@ def score_candidates(args: argparse.Namespace, spans: list, atoms: list, scored:
     A stop at ``--max-usd`` exits with a message that says no candidate table and no ``--out``
     file was written and what a rerun can reuse; a table scored on part of the verdicts is not made.
     """
-    from pathlib import Path
-
+    from dokimi.commands.novelty_run import usable_cache_dir
     from dokimi.novelty import cost
     from dokimi.novelty.client import SpendCapReached
     from dokimi.novelty.retention import RetentionOracle
     from dokimi.novelty.shorten import build_candidates
 
+    cache_dir = usable_cache_dir(args.cache_dir)
     client = cost.client(args.extract_model or args.model, args.provider, "oracle", args) \
         if not args.lexical_only else None
-    oracle = RetentionOracle(client=client, cache_dir=Path(args.cache_dir) if args.cache_dir else None)
+    oracle = RetentionOracle(client=client, cache_dir=cache_dir)
     try:
         candidates = build_candidates(spans, atoms, oracle=oracle, budgets=tuple(args.budgets),
                                       seed=args.seed, scored_atoms=scored)

dokimi/dokimi/commands/novelty_run.py

@@ -10,7 +10,9 @@
 import argparse
 import functools
 import math
+import os
 import sys
+from pathlib import Path
 
 
 def stops_at_cap(command):
@@ -28,6 +30,36 @@ def run(args: argparse.Namespace) -> None:
     return run
 
 
+def usable_cache_dir(raw: str | None) -> Path | None:
+    """``--cache-dir`` as a path the retention cache can store into, or ``None`` when unset.
+
+    A directory that does not exist yet is fine (created on the first store); one that exists
+    must be writable, and a missing one needs an existing, writable directory above it. Anything
+    else raises ``InputError`` before a paid call (#701).
+    """
+    from dokimi.utils import InputError
+
+    if not raw:
+        return None
+    path = Path(raw)
+    probe = path
+    try:
+        while not probe.exists() and not probe.is_symlink() and probe != probe.parent:
+            probe = probe.parent
+        is_dir = probe.is_dir()
+    except OSError as exc:
+        # An ancestor without search permission: exists()/is_dir() raise PermissionError (Python < 3.14).
+        raise InputError(f"--cache-dir {path}: cannot inspect {probe} ({exc.strerror or type(exc).__name__}); "
+                         "pass a directory the retention verdicts can be stored in") from exc
+    if not is_dir:
+        where = "is not a directory" if probe == path else f"has {probe} above it, which is not a directory"
+        raise InputError(f"--cache-dir {path} {where}; pass a directory the retention verdicts can be stored in")
+    if not os.access(probe, os.W_OK | os.X_OK):
+        raise InputError(f"--cache-dir {path}: {probe} is not writable; pass a directory the retention "
+                         "verdicts can be stored in")
+    return path
+
+
 def exit_if_analyze_partial(result, out) -> None:
     """Save and exit when the cap stopped ``analyze`` (``result.partial``); a finished run returns."""
     from dokimi.commands.novelty_budget import save_analyze_partial

dokimi/tests/test_novelty_cache_dir_usable.py

@@ -0,0 +1,164 @@
+# Copyright (C) 2026 Intel Corporation
+# SPDX-License-Identifier: Apache-2.0
+
+"""`--cache-dir` of `skill shorten` and `skill novelty analyze` is checked before any paid call (#701)."""
+
+from __future__ import annotations
+
+import os
+
+import pytest
+
+from dokimi.cli import build_parser
+from dokimi.commands.novelty_run import usable_cache_dir
+from dokimi.novelty import cost as novelty_cost
+from dokimi.novelty.atoms import Atom, save_atoms
+from dokimi.novelty.spans import segment
+from dokimi.utils import InputError
+from tests.cli_refusal import refusal
+
+
+def test_usable_cache_dir_accepts_unset_a_directory_and_a_missing_child(tmp_path):
+    assert usable_cache_dir(None) is None
+    assert usable_cache_dir("") is None
+    assert usable_cache_dir(str(tmp_path)) == tmp_path
+    assert usable_cache_dir(str(tmp_path / "new" / "deeper")) == tmp_path / "new" / "deeper"
+
+
+def test_usable_cache_dir_refuses_a_regular_file_naming_the_flag_and_path(tmp_path):
+    target = tmp_path / "cache"
+    target.write_text("x", encoding="utf-8")
+    with pytest.raises(InputError, match="--cache-dir") as err:
+        usable_cache_dir(str(target))
+    assert str(target) in str(err.value) and "is not a directory" in str(err.value)
+
+
+def test_usable_cache_dir_refuses_a_child_of_a_regular_file(tmp_path):
+    parent = tmp_path / "file"
+    parent.write_text("x", encoding="utf-8")
+    with pytest.raises(InputError) as err:
+        usable_cache_dir(str(parent / "cache"))
+    assert str(parent / "cache") in str(err.value) and f"has {parent} above it" in str(err.value)
+
+
+def test_usable_cache_dir_refuses_a_dangling_symlink_and_a_child_of_one(tmp_path):
+    link = tmp_path / "cache"
+    try:
+        link.symlink_to(tmp_path / "missing-target")
+    except OSError:
+        pytest.skip("symlinks need privileges here")
+    with pytest.raises(InputError, match="is not a directory") as err:
+        usable_cache_dir(str(link))
+    assert f"--cache-dir {link} is not a directory" in str(err.value)
+    with pytest.raises(InputError) as err:
+        usable_cache_dir(str(link / "child"))
+    assert f"has {link} above it" in str(err.value)
+
+
+@pytest.mark.skipif(not hasattr(os, "geteuid") or os.geteuid() == 0, reason="root writes anywhere")
+def test_usable_cache_dir_refuses_an_unwritable_directory_and_a_child_of_one(tmp_path):
+    locked = tmp_path / "locked"
+    locked.mkdir()
+    locked.chmod(0o500)
+    try:
+        for raw in (locked, locked / "cache"):
+            with pytest.raises(InputError) as err:
+                usable_cache_dir(str(raw))
+            assert str(raw) in str(err.value) and "is not writable" in str(err.value)
+    finally:
+        locked.chmod(0o700)
+
+
+@pytest.mark.skipif(not hasattr(os, "geteuid") or os.geteuid() == 0, reason="root writes anywhere")
+def test_usable_cache_dir_refuses_a_writable_directory_without_search_permission(tmp_path):
+    closed = tmp_path / "closed"
+    closed.mkdir()
+    closed.chmod(0o600)
+    try:
+        with pytest.raises(InputError) as err:
+            usable_cache_dir(str(closed))
+        assert str(closed) in str(err.value) and "is not writable" in str(err.value)
+    finally:
+        closed.chmod(0o700)
+
+
+@pytest.mark.skipif(not hasattr(os, "geteuid") or os.geteuid() == 0, reason="root searches anywhere")
+def test_usable_cache_dir_refuses_a_path_below_a_directory_it_cannot_search(tmp_path):
+    closed = tmp_path / "closed"
+    (closed / "sub").mkdir(parents=True)
+    closed.chmod(0o600)
+    try:
+        for raw in (closed / "sub" / "cache", closed / "sub"):
+            with pytest.raises(InputError) as err:
+                usable_cache_dir(str(raw))
+            # Python 3.14 reads the unsearchable ancestor as missing and refuses it as unwritable instead.
+            assert f"--cache-dir {raw}: " in str(err.value)
+            assert "cannot inspect" in str(err.value) or "is not writable" in str(err.value)
+    finally:
+        closed.chmod(0o700)
+
+
+def test_usable_cache_dir_names_the_flag_when_inspecting_the_path_raises(tmp_path, monkeypatch):
+    # Python < 3.14 raises PermissionError from is_dir() below a directory without search permission;
+    # simulate it so the refusal is checked on every version and as root.
+    def deny(self):
+        raise PermissionError(13, "Permission denied")
+
+    monkeypatch.setattr(type(tmp_path), "is_dir", deny)
+    raw = tmp_path / "cache"
+    with pytest.raises(InputError) as err:
+        usable_cache_dir(str(raw))
+    message = str(err.value)
+    assert f"--cache-dir {raw}: cannot inspect {tmp_path} (Permission denied)" in message
+    assert isinstance(err.value.__cause__, PermissionError)
+
+
+def _atoms(tmp_path):
+    rows = [f"Configure module{i} with parameter{i} always." for i in range(5)]
+    (tmp_path / "rows.md").write_text("# H\n\n" + "\n\n".join(rows) + "\n", encoding="utf-8")
+    spans = segment((tmp_path / "rows.md").read_text(encoding="utf-8"))
+    atoms = [Atom(statement=f"Configure module{i} with parameter{i} never restarts service{i}",
+                  question="q", answer="a", span_ids=(span.id,))
+             for i, span in enumerate(s for s in spans if s.kind != "heading")]
+    save_atoms(atoms, spans, tmp_path / "atoms.json", extractor="e", cap=None)
+
+
+def _never_called(monkeypatch):
+    def fail(*args, **kwargs):
+        pytest.fail("a model client was built for an unusable --cache-dir")
+
+    monkeypatch.setattr(novelty_cost, "client", fail)
+    monkeypatch.setattr(novelty_cost, "clients", fail)
+
+
+@pytest.mark.parametrize("capped", [True, False], ids=["max-usd", "no-cap"])
+def test_shorten_refuses_a_cache_dir_that_is_a_file_before_any_call(tmp_path, monkeypatch, capped):
+    _atoms(tmp_path)
+    cache = tmp_path / "cache"
+    cache.write_text("x", encoding="utf-8")
+    _never_called(monkeypatch)
+    args = build_parser().parse_args([
+        "skill", "shorten", "--artifact", f"text:{tmp_path / 'rows.md'}",
+        "--atoms", str(tmp_path / "atoms.json"), "--model", "o", "--cache-dir", str(cache),
+        *(["--max-usd", "0.05"] if capped else [])])
+    with refusal() as out:
+        args.func(args)
+    assert out.code == 2
+    assert f"--cache-dir {cache} is not a directory" in out.err
+    assert "Traceback" not in out.err
+
+
+def test_analyze_refuses_a_cache_dir_that_is_a_file_before_any_call(tmp_path, monkeypatch):
+    (tmp_path / "rows.md").write_text("# H\n\nConfigure module with parameter always.\n", encoding="utf-8")
+    cache = tmp_path / "cache"
+    cache.write_text("x", encoding="utf-8")
+    _never_called(monkeypatch)
+    args = build_parser().parse_args([
+        "skill", "novelty", "analyze", "--artifact", f"text:{tmp_path / 'rows.md'}",
+        "--model", "o", "--cache-dir", str(cache), "--max-usd", "0.05",
+        "--out-dir", str(tmp_path / "out")])
+    with refusal() as out:
+        args.func(args)
+    assert out.code == 2
+    assert f"--cache-dir {cache} is not a directory" in out.err
+    assert not (tmp_path / "out").exists()