From 267d1de7b8a8e7fd22b156433567c88216ec3d0f Mon Sep 17 00:00:00 2001 From: Craig Jennings Date: Fri, 24 Jul 2026 10:56:50 -0500 Subject: fix: harden the org-file mutators and close four verified bugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An overnight hygiene run over this repo, reviewed adversarially afterward. Every problem below was reproduced before it was fixed, and every fix was re-checked by a skeptic who was told to refute it. cj-remove-block could silently destroy content. Its range check validated only the first and last lines, so a span from one cj block's opener to a later block's closer passed and the removal deleted everything between — prose, headings, whole tasks, with a zero exit. That is the failure the check exists to prevent, and drift is its normal case, since the calling skill edits the file while processing. It now refuses a range holding more than one block, backs up first, and writes atomically while preserving the file's mode and following symlinks to the real target. todo-cleanup rewrote todo.org with no backup, alone among the three tools that mutate these files. It now copies to the temp dir like lint-org does. Both backup helpers suffix rather than overwrite, because a second-resolution stamp collapsed two back-to-back invocations into one backup holding already-mutated content — and running todo-cleanup twice in a row is the shipped path. audit.bats failed about one run in eight, in teardown rather than in its assertions, so a passing test reported as a false red. git commit spawns a detached maintenance process that is still writing packs when the commit returns, racing the cleanup. The fixture disables it. route_recommend downgraded a correct strong match to weak when two projects shared a basename. lint.sh now sweeps the scripts that get symlinked onto PATH, which were the only shell in the repo with no gate over them. Two test defects found in review and fixed here: a suite that deleted real backups from the shared temp dir, and one that depended on that directory's contents. Isolation now lives in an autouse fixture rather than in each test's memory. Known and filed, not closed: the range check still cannot prove the range is the block that was scanned, so drift by exactly one whole block deletes the wrong annotation. That needs a content assertion and a CLI contract change. --- .ai/scripts/cj-remove-block.py | 81 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 79 insertions(+), 2 deletions(-) (limited to '.ai/scripts/cj-remove-block.py') diff --git a/.ai/scripts/cj-remove-block.py b/.ai/scripts/cj-remove-block.py index 71c7b3d..d5137a3 100755 --- a/.ai/scripts/cj-remove-block.py +++ b/.ai/scripts/cj-remove-block.py @@ -16,8 +16,12 @@ Companion to the /respond-to-cj-comments skill and to cj-scan.py. from __future__ import annotations import argparse +import os import re +import shutil import sys +import tempfile +from datetime import datetime from pathlib import Path SRC_OPEN_RE = re.compile(r"^\s*#\+begin_src\s+cj:", re.IGNORECASE) @@ -57,12 +61,83 @@ def looks_like_cj_range(lines: list[str], start: int, end: int) -> tuple[bool, s f"Line {end} does not look like a #+end_src closing fence " f"(got: {last[:60]!r})" ) + + # The range must hold exactly ONE block. Checking only the first and last + # lines let a drifted range run from one block's opener to a *later* block's + # closer: validation passed and the removal silently deleted everything + # between, prose and headings included. Drift is the case this check exists + # for, so it has to look inside the range, not just at its ends. + for offset, line in enumerate(lines[start:end - 1], start=start + 1): + if SRC_CLOSE_RE.match(line): + return False, ( + f"Range {start}..{end} covers more than one cj block — " + f"a #+end_src appears at line {offset}, before the range ends. " + f"Re-scan for current line numbers; removing this range would " + f"delete everything between the two blocks." + ) + if SRC_OPEN_RE.match(line): + return False, ( + f"Range {start}..{end} covers more than one cj block — " + f"a second #+begin_src cj: appears at line {offset}. " + f"Re-scan for current line numbers." + ) return True, "" +def _backup(path: Path) -> Path: + """Copy path to /tmp before mutating it, mirroring lint-org.el's convention. + + These are Craig's org files. lint-org.el, the other tool that rewrites them, + leaves a /tmp copy before touching anything; this matches it so a bad edit is + always recoverable without reaching for git (which only reaches the last + commit, losing intra-session work). + """ + stamp = datetime.now().strftime("%Y%m%d-%H%M%S") + base = Path(tempfile.gettempdir()) / f"{path.name}.before-cj-remove.{stamp}" + # Never overwrite an earlier backup. The skill removes several annotations + # in quick succession, so a second-resolution stamp collides and the later + # copy would replace the earlier one with already-mutated content — losing + # the pre-session original the backup exists to preserve. + dest = base + n = 2 + while dest.exists(): + dest = base.with_name(f"{base.name}-{n}") + n += 1 + shutil.copy2(path, dest) + return dest + + +def _atomic_write(path: Path, text: str) -> None: + """Write text to path via a temp sibling and os.replace. + + A bare write_text truncates the target on open, so a mid-write failure left + the org file truncated with no complete copy on disk. Writing a temp sibling + and renaming means the file is either its old content or its new content, + never a partial. + """ + # Follow a symlink to the file it names. os.replace would otherwise swap the + # symlink itself for a regular file, leaving the real target holding the old + # content — the edit silently goes nowhere. Resolving also puts the temp + # sibling on the same filesystem as the real file, which os.replace needs. + path = path.resolve() + fd, tmp = tempfile.mkstemp(dir=path.parent, prefix=f".{path.name}.", suffix=".tmp") + os.close(fd) + tmp_path = Path(tmp) + # Carry the original's permissions across. mkstemp creates 0600, and + # defaulting to the umask instead widened a deliberately-restricted file + # (a 0600 org file came back 0644). + shutil.copymode(path, tmp_path) + try: + tmp_path.write_text(text, encoding="utf-8") + os.replace(tmp_path, path) + except BaseException: + tmp_path.unlink(missing_ok=True) + raise + + def remove_range(path: Path, start: int, end: int) -> None: """Read path, validate range looks like cj content, remove the range, write back.""" - text = path.read_text() + text = path.read_text(encoding="utf-8") had_trailing_newline = text.endswith("\n") lines = text.splitlines(keepends=False) @@ -77,7 +152,9 @@ def remove_range(path: Path, start: int, end: int) -> None: new_text += "\n" elif not new_lines and had_trailing_newline: new_text = "" - path.write_text(new_text) + + _backup(path) + _atomic_write(path, new_text) def main() -> int: -- cgit v1.2.3