Guarding Destructive Operations

Guarding Destructive Operations

Some operations have no undo: an orphan-branch rewrite, a recursive delete of every tracked file, an overwrite of a stored artifact, a DROP/purge path. The implementation is usually short and usually correct for the setup its author had in mind. The bug is never the deletion itself — it is that nothing checked whether the target was the thing the author imagined.

Two shapes recur, and they take the same fix:

• Blast radius. The operation is correct only in a dedicated, single-purpose target and is catastrophic in a mixed one.
• Boundary escape. A caller-supplied name is joined onto a base directory and lands outside it.

Refuse; don't warn, and don't add an override

A destructive operation that discovers its precondition is violated should return an error and change nothing. A warning is read by nobody, and a --force escape hatch turns the guard into documentation.

// A rebuild that recreates history does `checkout --orphan` then
// `rm -rf --ignore-unmatch .`, keeping only the state files it rewrites.
// That is correct in a dedicated mirror repository whose ONLY tracked content
// is `.state/`. Run it where source code also lives and the new branch loses
// the source code.
func (e *Engine) rebuild() error {
    if err := e.ensureDedicatedRepo(); err != nil {
        return err   // nothing mutated yet
    }
    ...
}

The honest justification for having no override flag: there is no situation where the operator wants this command to delete unrelated tracked files. If a real one appears later, that is a separate, differently-named command — not a boolean on this one.

Put the guard ahead of the first mutation

"Refuses" is only true if the refusal happens before anything has changed. Walk the function and find the first statement with an effect — it is often not the obvious deletion. In-memory state clearing, a branch checkout, and a read-then-rewrite of the very files you are preserving all count.

// Order that makes the refusal safe:
//   1. reject detached HEAD / ambiguous state
//   2. ensureDedicatedRepo()          <-- the new guard, nothing mutated yet
//   3. read the state files to preserve
//   4. CheckoutOrphan()
//   5. RemoveAllTrackedFiles()
//   6. ClearAllProgressCounts()

Steps 3-6 are all mutations of something (3 is not, but it is where the "what to preserve" snapshot is taken, and a guard after it has already committed you to a shape). Land the guard at 2 and a rejected run leaves the working tree byte-identical.

Also refuse ambiguous state before rewriting it. A history rewrite that assumes a named branch should reject detached HEAD up front rather than silently recreating the wrong ref.

Classify structurally, not by string prefix

The guard needs to answer "is every tracked path inside the directory this command owns?". A HasPrefix/startswith on the bare directory name is the wrong answer and looks right in every test you would think to write.

// Bad — admits `.state-backup/notes.md` and `.stateful.txt`, so a repository
// full of unrelated content passes the guard and gets deleted.
func owned(path string) bool {
    return strings.HasPrefix(path, ".state")
}

// Good — the directory itself, or something genuinely under it.
func owned(path string) bool {
    return path == ".state" || strings.HasPrefix(path, ".state/")
}

The same distinction in Python is Path(p) == base or base in Path(p).parents, not p.startswith(str(base)). Any time a check compares paths as strings, ask what a sibling with a longer name does to it.

Name validation and resolved containment are two checks, not one

When a public argument selects a file — a baseline name, a template id, a report slug — do both, in this order:

NAME_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]*$")

def _validate_name(name: str) -> str:
    if ".." in name or not NAME_RE.match(name):
        raise ValueError(f"invalid name: {name!r}")
    return name

def _resolve(name: str, base: Path) -> Path:
    candidate = (base / f"{_validate_name(name)}.json").resolve()
    if not candidate.is_relative_to(base.resolve()):
        raise ValueError(f"name escapes {base}: {name!r}")
    return candidate

The containment half is not redundant with the regex. The regex stops .. segments and absolute paths; only the resolve() + containment check stops a symlink placed inside the directory that points out of it — a name matching ^[A-Za-z0-9] all the way through can still resolve anywhere. Route every entry point through both: the loader, the saver, and any path-building helper a public method still calls directly. A single unrouted helper reinstates the whole hole.

Without this, an implicit suffix (name + ".json") plus an absolute-path argument reads any file on disk whose name happens to end in that suffix.

A guard on the destructive path is invisible to the preview

If the real run reaches the guard but --dry-run short-circuits earlier, the preview cheerfully describes an operation the real run will refuse. That is a defensible design — the destructive path is the one that must be safe — but be explicit about it:

• The preview will show a full rebuild in a repository where the rebuild is forbidden. Only the real invocation errors.
• Anything meant to warn the operator earlier has to live in the preview stage or the CLI argument-validation layer, duplicated deliberately, not moved.

Say which one you chose in the PR description; a reviewer cannot tell from the diff whether the dry-run gap is intentional.

Know what the caller does with your refusal

Tightening validation only helps if the new error reaches somebody. A broad except Exception two frames up will turn a hard refusal into an ordinary result shape:

try:
    features = self._load(baseline)
except Exception as e:                       # already there, catches your ValueError
    return {"error": f"Analysis failed: {e}", "features": {}, "flags": {...}}

This is often fine — an API boundary that never raises is a real contract, and the refusal surfaces as a normal error-shaped response rather than a protocol error. But check it deliberately: if that shape is what your caller sees, the integration test asserts on result["error"], not on pytest.raises.