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/tests/test_cj_remove_block.py | 167 ++++++++++++++++++++++++++++++ 1 file changed, 167 insertions(+) (limited to '.ai/scripts/tests/test_cj_remove_block.py') diff --git a/.ai/scripts/tests/test_cj_remove_block.py b/.ai/scripts/tests/test_cj_remove_block.py index 2c8dade..3cdee46 100644 --- a/.ai/scripts/tests/test_cj_remove_block.py +++ b/.ai/scripts/tests/test_cj_remove_block.py @@ -14,6 +14,34 @@ import pytest SCRIPT = Path(__file__).parent.parent / "cj-remove-block.py" +@pytest.fixture(autouse=True) +def isolated_tmpdir(tmp_path, monkeypatch): + """Give every test in this module a private TMPDIR. + + The script backs up to the system temp dir under a name derived from the + edited file's BASENAME. The real todo.org shares that basename, so any test + operating on a fixture named todo.org writes something indistinguishable + from a production backup — and an earlier version of this file globbed the + shared /tmp and unlinked every match, so a routine `make test` destroyed + Craig's real backups (found in review, 2026-07-24). + + Isolating at module scope rather than per-test is deliberate: the same bug + was fixed once in the elisp sibling and left here, so relying on each new + test to remember is exactly how it recurred. Autouse makes it structural. + """ + d = tmp_path / "_tmpdir" + d.mkdir() + # TMPDIR covers subprocess invocations of the script. + monkeypatch.setenv("TMPDIR", str(d)) + # tempfile.gettempdir() caches its answer on first call, so a test that + # loads the module in-process would keep writing to the real /tmp no matter + # what TMPDIR says. Override the cache too — this is the gap that made the + # env-var-only version still leak one backup per suite run. + import tempfile as _tempfile + monkeypatch.setattr(_tempfile, "tempdir", str(d)) + return d + + @pytest.fixture def run_remove(tmp_path): """Write content to a temp org file, run cj-remove-block, return new contents.""" @@ -155,3 +183,142 @@ class TestCjRemoveBlockSafety: err, post_content = run_remove_expecting_failure(original, start=4, end=2) assert err.returncode != 0 assert post_content == original + + +class TestMultiBlockRangeRefused: + """The validation exists to catch a drifted range, but it only checked the + first and last lines of that range. A span from one block's opening fence to + a LATER block's closing fence passed, and the removal silently deleted every + line between — real prose, headings, whole tasks — with a zero exit. Drift is + the skill's normal operating mode (respond-to-cj-comments edits the file as it + processes, and a file under cj review usually holds several blocks), so this + is the exact scenario the check was written for. Reproduced 2026-07-24.""" + + TWO_BLOCKS = ( + "* Alpha\n" + "#+begin_src cj:\n" + "note A\n" + "#+end_src\n" + "KEEP THIS LINE\n" + "* Beta\n" + "#+begin_src cj:\n" + "note B\n" + "#+end_src\n" + ) + + def test_range_spanning_two_blocks_is_refused(self, run_remove_expecting_failure): + # Lines 2..9: block one's opener through block two's closer. + err, content = run_remove_expecting_failure(self.TWO_BLOCKS, 2, 9) + assert err.returncode == 1 + assert "KEEP THIS LINE" in content, "content between the blocks was destroyed" + assert "* Beta" in content, "a heading between the blocks was destroyed" + + def test_refusal_names_the_reason(self, run_remove_expecting_failure): + err, _ = run_remove_expecting_failure(self.TWO_BLOCKS, 2, 9) + assert "more than one" in err.stderr.decode().lower() + + def test_a_correct_single_block_range_still_removes(self, run_remove): + # The fix must not over-tighten: the legitimate range still works. + out = run_remove(self.TWO_BLOCKS, 2, 4) + assert "note A" not in out + assert "KEEP THIS LINE" in out + assert "note B" in out, "the second block must be untouched" + + def test_a_nested_end_src_inside_the_range_is_refused(self, run_remove_expecting_failure): + # Any #+end_src before the final line means the range covers >1 block. + content = ( + "#+begin_src cj:\n" + "a\n" + "#+end_src\n" + "middle\n" + "#+begin_src cj:\n" + "b\n" + "#+end_src\n" + ) + err, after = run_remove_expecting_failure(content, 1, 7) + assert err.returncode == 1 + assert "middle" in after + + +class TestSafeMutation: + """The script rewrites Craig's org files (todo.org, notes.org). It wrote with + a bare write_text, which truncates the target on open, and took no backup — + so a mid-write failure left the file truncated with no copy to recover from. + lint-org.el, the other tool that mutates these files, backs up to a temp dir + first. Match that, and make the write atomic. + + Every test here redirects TMPDIR to a private directory. The backup name + derives from the file's basename, and the real todo.org shares it, so a test + globbing the shared temp dir cannot tell its own artifact from a genuine + backup — and an earlier version of this class globbed /tmp and unlinked every + match, so a routine `make test` destroyed real backups (found in review, + 2026-07-24). Never glob or delete across the shared temp dir.""" + + ONE_BLOCK = "* T\n#+begin_src cj:\nnote\n#+end_src\nkeep\n" + + def test_a_backup_is_written_before_mutating(self, tmp_path): + import subprocess, glob, os + bdir = tmp_path / "bk" + bdir.mkdir() + f = tmp_path / "todo.org" + f.write_text(self.ONE_BLOCK) + subprocess.run( + ["python3", str(SCRIPT), "--file", str(f), "--start", "2", "--end", "4"], + check=True, capture_output=True, + env={**os.environ, "TMPDIR": str(bdir)}, + ) + backups = glob.glob(str(bdir / "todo.org.before-cj-remove.*")) + assert backups, "no backup was written before mutating the org file" + assert "note" in Path(max(backups)).read_text() + + def test_no_partial_file_when_the_write_fails(self, tmp_path, monkeypatch): + import importlib.util + spec = importlib.util.spec_from_file_location("crb", SCRIPT) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + bdir = tmp_path / "bk" + bdir.mkdir() + monkeypatch.setenv("TMPDIR", str(bdir)) + f = tmp_path / "todo.org" + f.write_text(self.ONE_BLOCK) + def boom(*a, **k): + raise OSError("disk full") + monkeypatch.setattr(mod.os, "replace", boom) + with pytest.raises(OSError): + mod.remove_range(f, 2, 4) + # The original survives intact — no truncation, no partial. + assert f.read_text() == self.ONE_BLOCK + + +class TestBackupNeverOverwrites: + """Same defect class as todo-cleanup's, and more reachable here: the + respond-to-cj-comments skill removes several annotations in quick + succession, so a second-resolution stamp collides and the later backup + overwrote the earlier one with already-mutated content.""" + + TWO_BLOCKS = ( + "* A\n#+begin_src cj:\nfirst\n#+end_src\n" + "* B\n#+begin_src cj:\nsecond\n#+end_src\n" + ) + + def test_consecutive_removals_each_keep_a_backup(self, tmp_path, monkeypatch): + import subprocess, glob + bdir = tmp_path / "bk" + bdir.mkdir() + monkeypatch.setenv("TMPDIR", str(bdir)) + f = tmp_path / "todo.org" + f.write_text(self.TWO_BLOCKS) + original = f.read_text() + # Remove the second block, then the first — back to back, same second. + subprocess.run(["python3", str(SCRIPT), "--file", str(f), + "--start", "6", "--end", "8"], + check=True, capture_output=True, + env={**__import__("os").environ, "TMPDIR": str(bdir)}) + subprocess.run(["python3", str(SCRIPT), "--file", str(f), + "--start", "2", "--end", "4"], + check=True, capture_output=True, + env={**__import__("os").environ, "TMPDIR": str(bdir)}) + backups = glob.glob(str(bdir / "todo.org.before-cj-remove.*")) + assert len(backups) == 2, f"expected 2 backups, got {len(backups)}" + contents = [Path(b).read_text() for b in backups] + assert original in contents, "no backup holds the true original" -- cgit v1.2.3