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.

Testing

• Mutate each half separately. Neuter only the regex and confirm a .. fixture fails; neuter only the containment check and confirm a symlink fixture fails. One combined test passes with either half deleted and proves neither.
• Write the sibling-name fixture explicitly. A test whose repository tracks .state/data.json passes against HasPrefix(path, ".state") too. The test that separates the implementations tracks .state-backup/old.json and asserts the operation is refused.
• Assert nothing was mutated on refusal, not just that an error came back: the branch is unchanged, the tracked file list is unchanged, the in-memory progress counters are unchanged. That is the actual claim.
• Existing fixtures become the rejected shape. Older tests for the destructive path were often built loosely — a repository tracking app.txt alongside the state directory, because nothing cared. After the guard, such a test no longer reaches the code it was written to cover; it now exercises the refusal. Tighten those fixtures to the allowed shape and add the refusal case as a new test, or you silently lose coverage of the destructive path.
• Reproduce the failure with the narrowest hook. When you need one specific operation to fail in order to test the recovery path, target it precisely — a blanket hook that rejects every operation of that kind aborts the run earlier than the path you were trying to reach, and the test proves something else.

Checklist

• Every irreversible operation states its precondition in code, not in a doc
• Violation returns an error; no --force, no warn-and-continue
• The guard runs before the first statement with an effect
• Ambiguous state (detached HEAD, missing target, multiple candidates) rejected up front
• Path/ownership classification is structural, never a bare string prefix
• Caller-supplied names are validated and resolved-path contained
• Every entry point routes through both checks, including internal helpers
• The dry-run/preview gap is intentional and stated
• Tests mutate each half of the guard independently
• A sibling-name (x-backup/) and a symlink fixture both exist
• Refusal tests assert the target is unchanged, not just that an error was raised

Note for this repository (ffmpeg-skill)

This repo's actual exposure is much narrower than the examples above — there is no --rebuild/--purge-shaped command, no history rewriting, and "Keep originals" is already a stated design principle (README.md design principle #9: no tool overwrites its input; a test hashes every input after every run, see tests/test_all.py/tests/test_contract.py's input-hash checks). The closest analogue is -o/output-path handling: every tool takes a caller-supplied output path and ffmpeg_base() always passes -y (overwrite without prompting) — there is no confirmation step before an existing file at that path is silently replaced. Since this is a local CLI a human or agent invokes directly (not a server resolving untrusted network input against a base directory), the "boundary escape" half of this skill is low-stakes here; the "refuse before the first mutation" and "existing input-hash tests still prove nothing was touched" halves are the parts that actually apply.

Source: wdm0006/python-skills (MIT).