From 464472510daa759f7a44c2736e6971b565f4e8d7 Mon Sep 17 00:00:00 2001 From: Patrick <320190286+muellerei@users.noreply.github.com> Date: Tue, 13 Oct 2026 15:03:34 +0200 Subject: [PATCH 1/3] Refuse to remove a block whose subtree changed since it was read removeBlock cascades, so a child added between a command's read and its delete went with the block. LogseqAPI.remove_block now takes a required expect_tree: the tree hash of the block as the caller read it, or None for a block nobody read a subtree of (strictinsert's stand-in). With a hash it reads the block with its children again past the cache, right before the delete, and raises BlockChanged when the tree is another and BlockNotFound when the block is gone. No default, so a new caller cannot skip the check by forgetting it. remove-block and the page's property block in remove-property pass the hash of their read; copy-block passes None until it gets its own check. --- CHANGELOG.md | 9 ++ GLOSSARY.md | 6 +- docs/design.md | 10 ++ logseq_cli/api.py | 24 +++- logseq_cli/commands/edit.py | 5 +- logseq_cli/commands/properties.py | 9 +- logseq_cli/strictinsert.py | 4 +- logseq_cli/writerefused.py | 8 +- tests/conftest.py | 2 +- tests/logseq_http_double.py | 20 ++++ tests/test_destructive_dry_run.py | 4 +- tests/test_editor_gate.py | 36 ++++-- tests/test_incoming_refs.py | 12 +- tests/test_move_block.py | 2 +- tests/test_remove_block_expect_tree.py | 149 ++++++++++++++++++++++++ tests/test_write_proof_every_command.py | 2 +- 16 files changed, 273 insertions(+), 29 deletions(-) create mode 100644 tests/test_remove_block_expect_tree.py diff --git a/CHANGELOG.md b/CHANGELOG.md index cefae29..161a646 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -146,6 +146,15 @@ file carries most of the reasoning behind the tool. ### Changed +- `remove-block` (and its alias `delete-block`) reads the block with its + children again just before the delete and refuses with `block_changed`, + removing nothing, when anything under it changed since the command read it. + It used to delete whatever stood there by then, a child added in between + included, while the count it reported was the one it had read. A block gone + by then is reported as `block_not_found`, exit 1, where the command used to + report it removed. The same check guards the page's property block that + `remove-property` removes with its last key. One read more per removal. + - `analyze-journal-patterns` counts tasks from the database, one count per journal day. `tasks` holds `open`, `done`, `cancelled` and `done_rate`, and the `tasks` of each entry holds `open`, `done` and `cancelled`. A task is a diff --git a/GLOSSARY.md b/GLOSSARY.md index d6b6ecb..3676360 100644 --- a/GLOSSARY.md +++ b/GLOSSARY.md @@ -126,7 +126,11 @@ _Avoid_: version, etag, checksum **Tree Hash**: The Hash of a Block together with everything under it, content and order (`tree_hash` of `get-block --json`). The Precondition of `remove-block`, since -the hash of the block alone would not see a child added after the read. +the hash of the block alone would not see a child added after the read. Also +the check before every removal of a block a command read: a removal cascades, +so the block is read again just before the delete and refused with +`block_changed` when its Tree Hash is another (`remove-block`, and the page's +property block that `remove-property` removes with its last key). _Avoid_: subtree hash, deep hash **Dry Run**: diff --git a/docs/design.md b/docs/design.md index 460493e..ac78a65 100644 --- a/docs/design.md +++ b/docs/design.md @@ -51,6 +51,16 @@ adds it when a ref to the block is written earlier in the same call. This shrinks the window to the requests between that read and the write; it does not close it, as no read can. +A removal has the same gap, and a wider loss: `removeBlock` takes everything +under the block with it, so a child added after the command read the block +would be deleted unseen. Every caller of the API's removal says which state it +read, as a tree hash, or says explicitly that it read none (a stand-in the +same call wrote a moment before). The API reads the block with its children +again, past the cache, just before the delete and refuses with `block_changed` +when the tree is another, or `block_not_found` when the block is gone. The +parameter has no default, so a new caller cannot leave the check out by +forgetting it. + The second half of the sentence answers a loss no read-back can catch. Measured on Logseq 0.10.15: a write to the block being edited replaced the editor content at once and dropped what had not been saved; Logseq answered diff --git a/logseq_cli/api.py b/logseq_cli/api.py index 2ce5d09..afe7227 100644 --- a/logseq_cli/api.py +++ b/logseq_cli/api.py @@ -23,6 +23,7 @@ refuse_split_property, refuse_split_tree, stored_property_key, + tree_hash, tree_texts, without_block_ids, ) @@ -34,6 +35,7 @@ # strictinsert and output need not import the HTTP client for them. from logseq_cli.writerefused import ( # noqa: F401 re-exported BlockChanged, + BlockNotFound, EditorOpen, EditorStateUnknown, LogseqWriteError, @@ -1342,16 +1344,36 @@ def set_blocks_id(self, block_uuids: list, *, editing=_UNSET): return self._write("logseq.Editor.setBlocksId", [block_uuids], target=block_uuids, editing=editing, count=len(block_uuids)) - def remove_block(self, block_uuid: str, *, written_here: bool = False): + def remove_block(self, block_uuid: str, *, expect_tree: str | None, + written_here: bool = False): """Remove a block with its children; raises WriteNotVerified unless getBlock then finds none (``_prove_no_block``). + ``expect_tree``: the ``tree_hash`` of the block as the caller read it, + or ``None`` for a block nobody read a subtree of (strictinsert's + stand-in). The removal cascades, so a child made since the read would + go too: the block is read again, past the cache, just before the + delete, and BlockChanged is raised when its subtree is another one, + BlockNotFound when it is gone. Required, with no default, so that no + caller leaves the check out by forgetting it. What stays open is the + requests between that read and the delete; the API has no + compare-and-swap. + ``written_here``: the block is one this call wrote a moment before and counted (strictinsert's stand-in). Once it is gone, neither write remains, and a refusal that counted them would tell a retry of writes that are gone: the removal takes the insert's count back instead of adding its own. A removal that fails raises before, and the block, which stays, counts.""" + if expect_tree is not None: + current = self.get_block(block_uuid, include_children=True, cached=False) + if not current: + raise BlockNotFound(f"Block {block_uuid[:8]}.. is gone; nothing was removed.", + id=block_uuid) + if tree_hash(current) != expect_tree: + raise BlockChanged( + f"Block {block_uuid[:8]}.. or a block under it changed since it was " + "read; nothing was removed.", block=block_uuid) return self._write("logseq.Editor.removeBlock", [block_uuid], target=block_uuid, count=-1 if written_here else 1) diff --git a/logseq_cli/commands/edit.py b/logseq_cli/commands/edit.py index 12f9cd0..f90b913 100644 --- a/logseq_cli/commands/edit.py +++ b/logseq_cli/commands/edit.py @@ -10,6 +10,7 @@ refuse_split_block, refuse_split_heading, refuse_split_tree, + tree_hash, unwrap_block_id, without_block_ids, ) @@ -259,7 +260,7 @@ def remove_block_cmd(ctx, block_id, ignore_refs, expect_tree_hash, dry_run, as_j click.echo(f" incoming block refs that would dangle: {len(refs)}") return - api.remove_block(clean_id) + api.remove_block(clean_id, expect_tree=tree_hash(block)) if as_json: output({"id": clean_id, "removed": True, "content": content, @@ -1028,7 +1029,7 @@ def _copy_tree(block, parent_uuid=None): if remove: # Only reached when every insert above was proven, so the source is # removed against a copy that is known to exist, never a claimed one. - api.remove_block(block_id) + api.remove_block(block_id, expect_tree=None) action = "Moved" if remove else "Copied" result_data = {"action": action.lower(), "blocks": count, **names, "source_id": block_id} diff --git a/logseq_cli/commands/properties.py b/logseq_cli/commands/properties.py index 42b28b8..6ff1f0a 100644 --- a/logseq_cli/commands/properties.py +++ b/logseq_cli/commands/properties.py @@ -2,7 +2,12 @@ import click -from logseq_cli.blocktext import refuse_split_property, unwrap_block_id, with_property_line +from logseq_cli.blocktext import ( + refuse_split_property, + tree_hash, + unwrap_block_id, + with_property_line, +) from logseq_cli.group import cli from logseq_cli.datalog import edn_keyword, edn_string from logseq_cli.blockprops import ( @@ -215,7 +220,7 @@ def _write_property_block(api, blocks, block, content): else: uuid, old = block["uuid"], block.get("content") or "" if not content and block is not None and not block.get("children") and len(blocks) > 1: - api.remove_block(uuid) + api.remove_block(uuid, expect_tree=tree_hash(block)) return if content == old and old: api.update_block(uuid, "", replacing=old) diff --git a/logseq_cli/strictinsert.py b/logseq_cli/strictinsert.py index 5ee95f8..21a7c52 100644 --- a/logseq_cli/strictinsert.py +++ b/logseq_cli/strictinsert.py @@ -417,7 +417,7 @@ def insert_tree_keeping_ids(api, tree: list, where: str, target: str) -> list: raise if stand_in: # The batch landed: a stand-in left behind is an error of its own. - api.remove_block(stand_in, written_here=True) + api.remove_block(stand_in, expect_tree=None, written_here=True) expected = count_blocks(tree) after_tree = api.get_page_blocks_tree(page_name) or [] @@ -467,7 +467,7 @@ def _remove_stand_in(api, stand_in: str, failed: Exception) -> None: which would hide why the batch failed. Any WriteRefused: the removal can meet an error object, an open editor, or a removal that did not show.""" try: - api.remove_block(stand_in, written_here=True) + api.remove_block(stand_in, expect_tree=None, written_here=True) except WriteRefused as refused: note = (f"The empty stand-in block {stand_in[:8]}... written for the batch " f"stayed too: {refused}") diff --git a/logseq_cli/writerefused.py b/logseq_cli/writerefused.py index a62b2fd..35a0b81 100644 --- a/logseq_cli/writerefused.py +++ b/logseq_cli/writerefused.py @@ -39,14 +39,16 @@ class EditorOpen(WriteRefused): class BlockChanged(WriteRefused): - """The block no longer holds the text the caller read and meant to replace. + """The block no longer holds the text the caller read and meant to replace, + or, before a removal, it or a block under it changed since it was read. Field: block.""" reason = "block_changed" class BlockNotFound(WriteRefused): - """The block the caller meant is gone, found out before anything was - written. Field: id (or content and page, for a block picked by its text).""" + """The block the caller meant is gone, found out before the write it + refuses (a call may have written other blocks before, as copy-block does). + Field: id (or content and page, for a block picked by its text).""" reason = "block_not_found" diff --git a/tests/conftest.py b/tests/conftest.py index 4947877..25ff877 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -700,7 +700,7 @@ def append_block_in_page(self, name, content, options=None): page["blocks"].append(block) return self._out(block, page, None) - def remove_block(self, uuid, *, written_here=False): + def remove_block(self, uuid, *, expect_tree, written_here=False): # Single writes are not counted here (only batches are), so # ``written_here`` has nothing to take back. found = self.locate(uuid) diff --git a/tests/logseq_http_double.py b/tests/logseq_http_double.py index ec77d8b..d784236 100644 --- a/tests/logseq_http_double.py +++ b/tests/logseq_http_double.py @@ -393,6 +393,26 @@ def uuid_of(self, content: str) -> str: raise LookupError(f"{len(found)} blocks hold {content!r}") return found[0] + # --- changes made in Logseq meanwhile ------------------------------------- + def add_child(self, parent, content="added later"): + """Someone adds a child under ``parent`` in Logseq, last among its + children; returns its uuid.""" + _, siblings, i, _ = self._locate(parent) + node = self._build(content) + siblings[i]["children"].append(node) + self._index(node) + return node["uuid"] + + def set_content(self, uuid, content): + """Someone edits the block's text in Logseq.""" + _, siblings, i, _ = self._locate(uuid) + siblings[i]["content"] = content + + def delete_block(self, uuid): + """Someone deletes the block, with its children, in Logseq.""" + _, siblings, i, _ = self._locate(uuid) + del siblings[i] + def tree(self, name): """The page as first lines, nested: for assertions.""" def walk(nodes): diff --git a/tests/test_destructive_dry_run.py b/tests/test_destructive_dry_run.py index f0a66cc..c7a0af7 100644 --- a/tests/test_destructive_dry_run.py +++ b/tests/test_destructive_dry_run.py @@ -10,6 +10,7 @@ import pytest from click.testing import CliRunner +from logseq_cli.blocktext import tree_hash from logseq_cli.cli import cli from tests.conftest import answer_property_pulls, split_runner @@ -97,7 +98,8 @@ def test_without_dry_run_removes(self, api): api.get_block.return_value = {"uuid": "root", "content": "Parent"} result = CliRunner().invoke(cli, ["remove-block", "--id", "root"]) assert result.exit_code == 0 - api.remove_block.assert_called_once_with("root") + api.remove_block.assert_called_once_with( + "root", expect_tree=tree_hash({"uuid": "root", "content": "Parent"})) class TestCopyBlockDryRun: diff --git a/tests/test_editor_gate.py b/tests/test_editor_gate.py index ae5ec61..e61f4f8 100644 --- a/tests/test_editor_gate.py +++ b/tests/test_editor_gate.py @@ -57,12 +57,8 @@ def cached_api(double): def _open_new_child(double, parent, content="typed here"): """Someone presses Enter under ``parent`` and types in the new child; returns its uuid.""" - _, siblings, i, _ = double._locate(parent) - node = double._build(content) - siblings[i]["children"].append(node) - double._index(node) - double.editing = node["uuid"] - return node["uuid"] + double.editing = double.add_child(parent, content) + return double.editing def _uuid(double, content): @@ -177,12 +173,12 @@ def test_gate_refuses_set_blocks_id(double, api): def test_gate_refuses_remove_block_target(double, api): x = _uuid(double, "target block") - _assert_refused(double, lambda: api.remove_block(x), x) + _assert_refused(double, lambda: api.remove_block(x, expect_tree=None), x) def test_gate_refuses_remove_block_subtree(double, api): parent, kid = _uuid(double, "parent block"), _uuid(double, "kid block") - _assert_refused(double, lambda: api.remove_block(parent), kid) + _assert_refused(double, lambda: api.remove_block(parent, expect_tree=None), kid) # The subtree and the open block are read when the gate asks, not taken from @@ -195,7 +191,7 @@ def test_gate_remove_block_sees_a_child_made_after_the_first_read(double, cached kid = _open_new_child(double, parent) before = double.snapshot() with pytest.raises(EditorOpen) as refused: - cached_api.remove_block(parent) + cached_api.remove_block(parent, expect_tree=None) assert refused.value.fields["block"] == kid assert double.writes() == [] and double.snapshot() == before @@ -225,7 +221,8 @@ def test_gate_rename_page_sees_a_link_saved_after_the_first_read(double, cached_ def test_remove_block_command_sees_a_child_made_during_the_call(double): # Through the command, with its cache: the child appears while the call - # looks for refs into the block, after it read the block's subtree. + # looks for refs into the block, after it read the block's subtree. The + # read just before the delete sees it first, before the editor gate. parent = _uuid(double, "parent block") query = double._handlers["logseq.DB.datascriptQuery"] @@ -235,6 +232,23 @@ def child_appears(args): double._handlers["logseq.DB.datascriptQuery"] = child_appears r = split_runner().invoke(cli, ["--token", "t", "remove-block", "--id", parent, "--json"]) assert r.exit_code == 1 + assert _error_object(r)["reason"] == "block_changed" + assert double.sent("removeBlock") == [] + + +def test_remove_block_command_sees_a_child_typed_after_the_last_read(double): + # The child appears only when the gate asks, after the read before the + # delete: the editor gate is what refuses it. + parent = _uuid(double, "parent block") + check = double._check_editing + + def child_typed(): + if double.editing is None: + _open_new_child(double, parent) + return check() + double._check_editing = child_typed + r = split_runner().invoke(cli, ["--token", "t", "remove-block", "--id", parent, "--json"]) + assert r.exit_code == 1 assert _error_object(r)["reason"] == "open_in_editor" assert double.sent("removeBlock") == [] @@ -285,7 +299,7 @@ def test_gate_lets_unrelated_writes_through(double, api, write): _uuid(double, "body\nprio:: 1"), "prio"), "move_block_next_to_open": lambda: api.move_block(x, anchor, {"children": True}), "set_blocks_id": lambda: api.set_blocks_id([y]), - "remove_block": lambda: api.remove_block(x), + "remove_block": lambda: api.remove_block(x, expect_tree=None), "delete_page": lambda: api.delete_page("Props Page"), "rename_page": lambda: api.rename_page("Props Page", "Renamed Props"), } diff --git a/tests/test_incoming_refs.py b/tests/test_incoming_refs.py index 9c191c6..9f632c8 100644 --- a/tests/test_incoming_refs.py +++ b/tests/test_incoming_refs.py @@ -16,6 +16,7 @@ from click.testing import CliRunner +from logseq_cli.blocktext import tree_hash from logseq_cli.cli import cli from logseq_cli.lookup import incoming_block_refs @@ -131,6 +132,11 @@ def test_a_page_entity_as_source_is_its_own_page(self): {"target": CHILD, "block": REF_OUT, "page": "Notes"}] +def _tree_as_read(api): + """The tree hash of the root as the command read it.""" + return tree_hash(api.get_block(ROOT, include_children=True)) + + class TestRemoveBlock: def test_refuses_with_incoming_refs(self): api = _api(_graph()) @@ -144,14 +150,14 @@ def test_ignore_refs_removes_and_says_what_broke(self): api = _api(_graph()) r = _run(api, "remove-block", "--id", ROOT, "--ignore-refs") assert r.exit_code == 0, r.output - api.remove_block.assert_called_once_with(ROOT) + api.remove_block.assert_called_once_with(ROOT, expect_tree=_tree_as_read(api)) assert "2 incoming block ref(s)" in r.output def test_no_refs_removes_as_before(self): api = _api(_graph(refs_from_outside=False, refs_from_inside=False)) r = _run(api, "remove-block", "--id", ROOT) assert r.exit_code == 0, r.output - api.remove_block.assert_called_once_with(ROOT) + api.remove_block.assert_called_once_with(ROOT, expect_tree=_tree_as_read(api)) def test_dry_run_predicts_the_refusal(self): api = _api(_graph()) @@ -233,5 +239,5 @@ def test_ignore_refs_moves(self): r = _run(api, "copy-block", "--id", ROOT, "--to-page", "Elsewhere", "--remove", "--ignore-refs", "--json") assert r.exit_code == 0, r.output - api.remove_block.assert_called_once_with(ROOT) + api.remove_block.assert_called_once_with(ROOT, expect_tree=None) assert json.loads(r.output)["refs_broken"] == 3 diff --git a/tests/test_move_block.py b/tests/test_move_block.py index 6f1823b..4611c1d 100644 --- a/tests/test_move_block.py +++ b/tests/test_move_block.py @@ -154,7 +154,7 @@ def test_successful_copy_still_removes(self): r = CliRunner().invoke(cli, [ "copy-block", "--id", SRC, "--to-page", "Target", "--remove"]) assert r.exit_code == 0, r.output - api.remove_block.assert_called_once_with(SRC) + api.remove_block.assert_called_once_with(SRC, expect_tree=None) class _Graph: diff --git a/tests/test_remove_block_expect_tree.py b/tests/test_remove_block_expect_tree.py new file mode 100644 index 0000000..ccd1b3c --- /dev/null +++ b/tests/test_remove_block_expect_tree.py @@ -0,0 +1,149 @@ +"""A removal deletes only the subtree the caller read. + +``removeBlock`` cascades: whatever stands under the block when it runs goes +with it. A caller that read the block, counted and checked it, and then +removes it, passes the tree hash of that read; ``LogseqAPI.remove_block`` +reads the block again just before the delete and refuses when the subtree is +another one. The window that stays is the requests between that read and the +delete: the API has no compare-and-swap. +""" +import json + +import pytest + +from logseq_cli.api import LogseqAPI +from logseq_cli.blocktext import tree_hash +from logseq_cli.cli import cli +from logseq_cli.writerefused import BlockChanged, BlockNotFound +from tests.conftest import split_runner +from tests.logseq_http_double import LogseqHttpDouble + + +@pytest.fixture +def double(monkeypatch): + double = LogseqHttpDouble().install(monkeypatch) + double.add_page("Probe Page", [ + {"content": "parent block", "children": ["kid block"]}, + "other block", + ]) + return double + + +@pytest.fixture +def api(double): + return LogseqAPI(token="t") + + +def _read_tree(api, uuid): + return tree_hash(api.get_block(uuid, include_children=True, cached=False)) + + +# --- The API ---------------------------------------------------------------- + +def test_a_changed_subtree_is_not_removed(double, api): + parent = double.uuid_of("parent block") + read = _read_tree(api, parent) + double.add_child(parent) + with pytest.raises(BlockChanged) as refused: + api.remove_block(parent, expect_tree=read) + assert refused.value.fields["block"] == parent + assert "nothing was removed" in str(refused.value) + assert "repeat" not in str(refused.value).lower() + assert double.sent("removeBlock") == [] + + +def test_a_changed_root_text_is_not_removed(double, api): + parent = double.uuid_of("parent block") + read = _read_tree(api, parent) + double.set_content(parent, "parent edited") + with pytest.raises(BlockChanged): + api.remove_block(parent, expect_tree=read) + assert double.sent("removeBlock") == [] + + +def test_a_block_gone_before_the_delete_is_reported(double, api): + parent = double.uuid_of("parent block") + read = _read_tree(api, parent) + double.delete_block(parent) + with pytest.raises(BlockNotFound) as refused: + api.remove_block(parent, expect_tree=read) + assert refused.value.fields["id"] == parent + assert double.sent("removeBlock") == [] + + +def test_the_subtree_as_read_is_removed(double, api): + parent = double.uuid_of("parent block") + api.remove_block(parent, expect_tree=_read_tree(api, parent)) + assert len(double.sent("removeBlock")) == 1 + assert double.tree("Probe Page") == [("other block", [])] + + +def test_without_a_state_the_removal_is_as_before(double, api): + parent = double.uuid_of("parent block") + double.add_child(parent) + api.remove_block(parent, expect_tree=None) + assert double.tree("Probe Page") == [("other block", [])] + + +def test_every_caller_says_which_state_it_removes(double, api): + with pytest.raises(TypeError): + api.remove_block(double.uuid_of("parent block")) + + +# --- remove-block ----------------------------------------------------------- + +def test_remove_block_keeps_a_child_made_between_read_and_delete(double): + parent = double.uuid_of("parent block") + query = double._handlers["logseq.DB.datascriptQuery"] + added = [] + + def child_appears(args): + # The command looks for refs into the block after it read the subtree. + if not added: + added.append(double.add_child(parent)) + return query(args) + double._handlers["logseq.DB.datascriptQuery"] = child_appears + r = split_runner().invoke(cli, ["--token", "t", "remove-block", "--id", parent, "--json"]) + assert r.exit_code == 1, r.stderr + error = json.loads(r.stderr) + assert error["reason"] == "block_changed" + assert double.sent("removeBlock") == [] + assert double._locate(added[0]) is not None + + +def test_remove_block_reports_a_block_gone_before_the_delete(double): + parent = double.uuid_of("parent block") + query = double._handlers["logseq.DB.datascriptQuery"] + + def block_goes(args): + if double._locate(parent): + double.delete_block(parent) + return query(args) + double._handlers["logseq.DB.datascriptQuery"] = block_goes + r = split_runner().invoke(cli, ["--token", "t", "remove-block", "--id", parent, "--json"]) + assert r.exit_code == 1, r.stderr + assert json.loads(r.stderr)["reason"] == "block_not_found" + assert double.sent("removeBlock") == [] + + +# --- The page's property block ---------------------------------------------- + +def test_remove_property_keeps_a_property_block_that_got_a_child(monkeypatch): + double = LogseqHttpDouble().install(monkeypatch) + double.add_page("Props Page", ["kind:: a", "text"]) + block = double.uuid_of("kind:: a") + tree = double._handlers["logseq.Editor.getPageBlocksTree"] + added = [] + + def child_appears(args): + answer = tree(args) + if not added: + added.append(double.add_child(block)) + return answer + double._handlers["logseq.Editor.getPageBlocksTree"] = child_appears + r = split_runner().invoke(cli, ["--token", "t", "remove-property", "--name", "Props Page", + "--key", "kind", "--json"]) + assert r.exit_code == 1, r.stderr + assert json.loads(r.stderr)["reason"] == "block_changed" + assert double.sent("removeBlock") == [] + assert double._locate(block) is not None and double._locate(added[0]) is not None diff --git a/tests/test_write_proof_every_command.py b/tests/test_write_proof_every_command.py index 6c2243f..45ef5e8 100644 --- a/tests/test_write_proof_every_command.py +++ b/tests/test_write_proof_every_command.py @@ -476,7 +476,7 @@ def test_set_property_the_page_does_not_show_is_not_verified(monkeypatch): # --- removeBlock, deletePage, renamePage, createPage, setBlocksId ----------- @pytest.mark.parametrize("write,landed", [ - (lambda api, d: api.remove_block(d.uuid_of("alpha block")), 1), + (lambda api, d: api.remove_block(d.uuid_of("alpha block"), expect_tree=None), 1), (lambda api, d: api.delete_page("Other Page"), 1), (lambda api, d: api.rename_page("Other Page", "Renamed Page"), 1), (lambda api, d: api.create_page("Fresh Page"), 1), From dd8ca9237ae76dbdaf2f468207ac4db5716fd6df Mon Sep 17 00:00:00 2001 From: Patrick <320190286+muellerei@users.noreply.github.com> Date: Tue, 13 Oct 2026 15:06:07 +0200 Subject: [PATCH 2/3] Name the copy when copy-block fails after the copy landed copy-block writes the copy block by block and removes the source last, with no rollback. A failure after the copy's root landed left a copy in the target that a caller could not find (the error object had only writes_landed), and a retry made a second one. The command now registers copied_id and "The copy is ." with output.on_failure as soon as the root is proven; the two ways a failure is reported (a refused or unproven write, and a connection failure) add them to the error object and the message. A root that did not land names nothing. --- AGENTS.md | 17 ++++- CHANGELOG.md | 8 +++ docs/commands.md | 2 +- logseq_cli/commands/edit.py | 6 ++ logseq_cli/output.py | 32 ++++++--- .../test_copy_block_remove_changed_source.py | 69 +++++++++++++++++++ tests/test_editor_gate.py | 3 + 7 files changed, 125 insertions(+), 12 deletions(-) create mode 100644 tests/test_copy_block_remove_changed_source.py diff --git a/AGENTS.md b/AGENTS.md index 9fb346a..c0a71a6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -465,13 +465,13 @@ rollback), and the message ends with how many, or "Nothing was written.". | `reason` | Meaning | Fields | |----------|---------|--------| -| `open_in_editor` | The write would change the block open in Logseq's editor, where someone is typing | `block`, `page` | +| `open_in_editor` | The write would change the block open in Logseq's editor, where someone is typing | `block`, `page`; `copied_id` from `copy-block` | | `editor_state_unknown` | Logseq's answer to which block is open was not understood. The write was not sent; asked after a multi-block write, its blocks landed and count in `writes_landed` | `answer` | | `logseq_error` | Logseq threw on the write and answered with an error object | `method`, `logseq_message` | | `page_exists` | `create-page` on a page that exists, whose properties Logseq would drop | `page` | | `rename_refused` | `rename-page` onto a name another page has (Logseq would merge the two) or an empty one | `old`, `new`, `why` (`exists` or `empty`) | | `block_changed` | `set-todo-status`, `update-block --expect-hash`, `add-journal-block --upsert-heading --expect-hash`, `replace-text`, `set-property` and `remove-property` (every write that replaces a block's text) read the block again right before writing, and it no longer holds the text they read: someone changed it in between, and the write would have replaced that change. Nothing was written | `block` | -| `write_not_verified` | The write does not show in Logseq: not written, or not all of it | `method`, `target`, `expected`, `got` | +| `write_not_verified` | The write does not show in Logseq: not written, or not all of it | `method`, `target`, `expected`, `got`; `copied_id` from `copy-block` | | `read_only` | Writes are switched off by `[safety] read_only`, `LOGSEQ_CLI_READ_ONLY` or `--read-only`; every command that writes refuses, `--dry-run` included, before its first request | `source` (list of `config`, `env`, `flag`), `config_path` (`null` when no config file was found) | | `precondition_required` | A precondition is required and the call gave none: `[safety] require_preconditions`, `LOGSEQ_CLI_REQUIRE_PRECONDITIONS` or `--require-preconditions` is on, and the command got none of the options it takes: `--expect-hash` (`update-block` takes it too; `set-todo-status` also takes `--expect-marker`; `add-journal-block --upsert-heading` owes it only when it matches a block, otherwise it creates one and owes nothing), or `--expect-tree-hash` for `remove-block` / `delete-block`. Refused before the first request, nothing was written | `source` (list of `config`, `env`, `flag`), `config_path` (`null` when no config file was found), `options` | | `config_error` | The config file cannot be used: it does not parse, `LOGSEQ_CLI_CONFIG` names a file that is not there, or `[safety]` does not check out: a key it does not know, a `[safety]` key outside `[safety]`, a value that is not a boolean, or a `[graph] keep_empty_blocks_last` that is not. A command that writes refuses rather than run without the limits the file may hold. Nothing was sent | none | @@ -493,6 +493,15 @@ failed between a write and its proof, `unproven_write` names that write: it may have landed or not. Read the target before retrying; a retry of the whole command writes again what landed. +`copy-block` adds `copied_id` to every failure after the root of its copy +landed, whatever its `reason`: a refusal, a write not verified, an error +Logseq threw, a connection failure, at the removal too. The message says "The copy is .". The copy stays (no rollback), so +repeating `copy-block` makes a second one: read the copy first. When the root +itself failed, there is no `copied_id`, and neither when the root went before +the empty blocks at the end of the page (`keep_empty_blocks_last`) and the +step that proves its place failed: then `writes_landed` says that it landed, +so read the target page before retrying. + On `read_only`: the user switched writes off. Do not retry, do not edit the config file, unset `LOGSEQ_CLI_READ_ONLY`, change `HOME` or `LOGSEQ_CLI_CONFIG`, write the Markdown files, or call the HTTP API yourself; each of those goes @@ -504,7 +513,9 @@ call. [docs/safety.md](docs/safety.md) describes the switch. On `open_in_editor`: the refused write did not happen; earlier writes of the same call may have (the message says how many). Ask the user to leave the -block, check, then retry. Do not retry in a loop. +block, check, then retry. Do not retry in a loop. After `copy-block --remove` +the copy stands (`copied_id`): once the source is free, remove it with +`remove-block` instead of repeating the move. What counts as changing the open block: a write to it, a removal or a move of it or of a block above it, a page deletion or a rename of its page, and a diff --git a/CHANGELOG.md b/CHANGELOG.md index 161a646..df0bdae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,14 @@ file carries most of the reasoning behind the tool. ### Added +- `copy-block` names its copy when it fails after the copy's root landed: + `copied_id` in the error object and "The copy is ." in the message, + for a refused removal (`open_in_editor`), a block of the copy not verified + (`write_not_verified`) and a connection failure, at the removal too. The copy + stays, as before (no rollback), but a caller had only `writes_landed` and no + way to find it, and a retry made a second copy. A root that did not land + names nothing. + - A write can say which state of a block it was decided on, and is refused when the block is another by then. `get-block --json` gives a block a `hash` (and `tree_hash` for the block with everything under it); `get-page`, diff --git a/docs/commands.md b/docs/commands.md index fe1b313..e7e6ea4 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -135,7 +135,7 @@ A write that ends a section (`add-journal-block`, `add-journal-content`, | `insert-block --child-of UUID --first` | Insert as FIRST child instead of appending last (works with `--content` and `--tree`; order preserved). Only valid with `--child-of` | | `insert-block --tree "" [--child-of UUID \| --page NAME --top-level]` | Batch-insert a hierarchy in one call (DFS pre-order UUIDs returned). `--tree-file FILE` reads the same tab-indented text or JSON from a file | | `insert-block --tree ... --keep-ids` | Keep the `id::` values in the content instead of letting Logseq mint new ones, for moving or restoring an outline — top-level blocks included. Without it they are dropped and the count is reported on stderr; content that is nothing but `id::` lines is refused, since nothing would be left to write. With it, an id a block or page still has is refused before anything is written (a copy would put one uuid on two blocks), and so is a second `id::` line in one block. One that survives only as a `((ref))` target is restored: the block takes over the placeholder Logseq keeps under that uuid, and the ref resolves again. An `id::` line inside a code block (```` ``` ```` or `~~~`) is code and is written as is. The same flag and rule apply to `--content` and to `add-note-content`, `add-journal-block` and `add-journal-content` | -| `copy-block --id UUID --to-page NAME [--remove] [--ignore-refs] [--expect-hash H] [--dry-run]` | Copy/move block with children to another page. `--expect-hash H` (the `hash` of `get-block --json`, with `--remove` only; without it a usage error) refuses with `precondition_failed` unless the block is still the one that was read, before `--dry-run` and before the first copy; the copy and the removal come from that read. The copy gets new UUIDs and leaves the source's `id::` lines out, so `--remove` refuses while `((block-refs))` point into the source (`--ignore-refs` overrides); `move-block` keeps them | +| `copy-block --id UUID --to-page NAME [--remove] [--ignore-refs] [--expect-hash H] [--dry-run]` | Copy/move block with children to another page. `--expect-hash H` (the `hash` of `get-block --json`, with `--remove` only; without it a usage error) refuses with `precondition_failed` unless the block is still the one that was read, before `--dry-run` and before the first copy; the copy and the removal come from that read. Every failure after the copy's root landed and was proven names the copy in `copied_id` ("The copy is ."): it stays, and a retry makes a second one. A root placed before the empty blocks at the end of the page whose place could not be proven is not named; `writes_landed` says it landed. The copy gets new UUIDs and leaves the source's `id::` lines out, so `--remove` refuses while `((block-refs))` point into the source (`--ignore-refs` overrides); `move-block` keeps them | | `move-block --id UUID (--under UUID \| --before UUID) [--expect-hash H] [--dry-run]` | Structural move (`--expect-hash H`: refuses with `precondition_failed`, before `--dry-run` and the move, unless the block is still the one that was read; it sees the content, not the position): the block keeps its UUID, so `((block-refs))` to it survive. Prefer over `copy-block --remove`, which writes a new block and deletes the original. `--under` nests as first child, `--before` places it in front as a sibling | ### Meta (4) diff --git a/logseq_cli/commands/edit.py b/logseq_cli/commands/edit.py index f90b913..ccfd730 100644 --- a/logseq_cli/commands/edit.py +++ b/logseq_cli/commands/edit.py @@ -47,6 +47,7 @@ follow_page, follow_page_to_write, handle_connection_error, + on_failure, output, uuid_fields, would_go_before_fields, @@ -932,6 +933,8 @@ def add_block_ref(ctx, source_id, journal_date, page, under_heading, dry_run, as a section goes before the empty blocks that end it, so they stay last, and nothing is overwritten. --expect-hash goes with --remove only: a plain copy changes no block. + A failure after the copy's root landed names it (copied_id, "The copy + is ."): the copy stays, and running the command again copies again. """) @click.option("--id", "block_id", required=True, help="Source block UUID") @click.option("--to-page", required=True, help="Target page name") @@ -1017,6 +1020,9 @@ def _copy_tree(block, parent_uuid=None): lambda: [api.append_block_in_page(to_page, content)["uuid"]], keep_last=keep_on, page_name=to_page) new_uuid = root[0] + # Proven from here on: whatever fails next leaves this copy in + # place, and a retry would make another. + on_failure(f"The copy is {new_uuid}.", copied_id=new_uuid) copied = 1 for child in block.get("children", []): copied += _copy_tree(child, new_uuid) diff --git a/logseq_cli/output.py b/logseq_cli/output.py index 7d02ec5..6513248 100644 --- a/logseq_cli/output.py +++ b/logseq_cli/output.py @@ -21,6 +21,26 @@ from logseq_cli.writerefused import WriteRefused, partial_state +def on_failure(sentence: str, **fields) -> None: + """Say ``sentence`` and add ``fields`` to every failure of the rest of + the call, refused or cut off: what only the command knows about what its + landed writes left behind (copy-block: where the copy is). One call; a + later one replaces it. Both ways a failure is reported read it, so the + command needs no ``try`` of its own per kind of failure.""" + ctx = click.get_current_context(silent=True) + if ctx is not None and isinstance(ctx.obj, dict): + ctx.obj["on_failure"] = (sentence, fields) + + +def _with_on_failure(message: str, fields: dict) -> tuple: + """``message`` and ``fields`` with what the command left with + :func:`on_failure` added, the sentence last.""" + ctx = click.get_current_context(silent=True) + obj = ctx.obj if ctx is not None and isinstance(ctx.obj, dict) else {} + sentence, own = obj.get("on_failure") or ("", {}) + return (f"{message} {sentence}" if sentence else message), {**fields, **own} + + def _after_writes(message: str) -> tuple: """``message`` and the fields for a failure of the connection, which can come after writes of the same call landed, and between a write and its @@ -43,7 +63,7 @@ def _after_writes(message: str) -> tuple: message = f"{message} No earlier write of this call landed." if landed or unproven: fields["writes_landed"] = landed - return message, fields + return _with_on_failure(message, fields) def handle_connection_error(func): @@ -151,13 +171,9 @@ def wrapper(*args, **kwargs): # with its source open) must say so, and one raised outside the # central write says it too. Exit 1 for all of them. landed = click.get_current_context().obj["api"].writes_landed - fail( - f"{e} {partial_state(landed)}", - as_json=as_json, - reason=e.reason, - writes_landed=landed, - **e.fields, - ) + message, fields = _with_on_failure(f"{e} {partial_state(landed)}", + {**e.fields, "writes_landed": landed}) + fail(message, as_json=as_json, reason=e.reason, **fields) except IdLineError as e: # Refused before the write, like a line that splits the block: the # text would give the block another uuid (#56). diff --git a/tests/test_copy_block_remove_changed_source.py b/tests/test_copy_block_remove_changed_source.py new file mode 100644 index 0000000..7fbfce7 --- /dev/null +++ b/tests/test_copy_block_remove_changed_source.py @@ -0,0 +1,69 @@ +"""copy-block names its copy whenever it fails after the copy's root landed. + +The copy is written block by block and the source removed last, with no +rollback: a failure after the root landed leaves a copy in the target that a +retry would copy again. The error object names it (``copied_id``) and the +message says "The copy is .", so a caller checks it instead of +repeating the command. +""" +import json + +import pytest +import requests + +from logseq_cli.cli import cli +from tests.conftest import split_runner +from tests.logseq_http_double import LogseqHttpDouble + +SOURCE = {"content": "source block", "children": ["first kid", "second kid", "third kid"]} + + +@pytest.fixture +def double(monkeypatch): + return LogseqHttpDouble.installed( + monkeypatch, {"Source Page": [SOURCE], "Target Page": ["target block"]}) + + +def _move(double): + uuid = double.uuid_of("source block") + return split_runner().invoke(cli, ["--token", "t", "copy-block", "--id", uuid, + "--to-page", "Target Page", "--remove", "--json"]) + + +def _copy_root(double): + """The uuid of the copy's root: the last top-level block of the target.""" + return double._require_page("Target Page")["blocks"][-1]["uuid"] + + +def test_a_child_copy_not_proven_names_the_copy(double): + double.set_mode("insertBlock", "noop", from_call=2) + r = _move(double) + assert r.exit_code == 1 + error = json.loads(r.stderr) + assert error["reason"] == "write_not_verified" + assert error["copied_id"] == _copy_root(double) + assert f"The copy is {_copy_root(double)}." in error["error"] + assert double.tree("Target Page")[-1] == ("source block", [("first kid", [])]) + assert double.sent("removeBlock") == [] + + +def test_a_timeout_at_the_removal_names_the_copy(double): + def timeout(args): + raise requests.Timeout("removeBlock timed out (test)") + double._handlers["logseq.Editor.removeBlock"] = timeout + r = _move(double) + assert r.exit_code == 1 + error = json.loads(r.stderr) + assert error["reason"] == "timeout" + assert error["copied_id"] == _copy_root(double) + assert f"The copy is {_copy_root(double)}." in error["error"] + + +def test_a_root_that_did_not_land_names_no_copy(double): + double.set_mode("appendBlockInPage", "noop") + r = _move(double) + assert r.exit_code == 1 + error = json.loads(r.stderr) + assert error["reason"] == "write_not_verified" + assert "copied_id" not in error + assert "The copy is" not in error["error"] diff --git a/tests/test_editor_gate.py b/tests/test_editor_gate.py index e61f4f8..6649be1 100644 --- a/tests/test_editor_gate.py +++ b/tests/test_editor_gate.py @@ -452,6 +452,9 @@ def test_copy_block_remove_open_source_reports_landed_copies(double): assert error["block"] == parent assert error["writes_landed"] == 2 assert "2 earlier write(s) in this call landed and remain" in error["error"] + copy = next(p for p in double.pages if p["name"] == "Other Page")["blocks"][-1]["uuid"] + assert error["copied_id"] == copy + assert f"The copy is {copy}." in error["error"] assert double.sent("removeBlock") == [] assert double.tree("Other Page")[-1] == ("parent block", [("kid block", [])]) assert ("parent block", [("kid block", [])]) in double.tree("Probe Page") From ce29d8904f9609a1f7194141173e96d1df083107 Mon Sep 17 00:00:00 2001 From: Patrick <320190286+muellerei@users.noreply.github.com> Date: Tue, 13 Oct 2026 15:08:44 +0200 Subject: [PATCH 3/3] Remove the source of copy-block --remove only as it was copied copy-block --remove reads the source once, copies it block by block and then removes it, and the removal cascades: a child added under the source while the copy ran was deleted, missing from the copy, and the command reported success. Reproduced against the test double. The removal now passes the tree hash of the state that was copied, so the API refuses with block_changed when anything under the source changed, removing nothing; the copy is named in copied_id. A source deleted elsewhere during the copy is a move done: exit 0 with source_gone, since a failure would invite cleaning up the only version left. docs/safety.md replaces the known gap with what stays open: the requests between the last read and the delete, a copy that breaks off, and a ref set during the copy. --- AGENTS.md | 16 ++++- CHANGELOG.md | 20 ++++++ GLOSSARY.md | 5 +- docs/commands.md | 2 +- docs/safety.md | 26 ++++++-- logseq_cli/commands/edit.py | 19 +++++- skills/logseq-cli/SKILL.md | 2 +- .../test_copy_block_remove_changed_source.py | 65 +++++++++++++++++++ tests/test_incoming_refs.py | 2 +- tests/test_move_block.py | 4 +- 10 files changed, 146 insertions(+), 15 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c0a71a6..bc6e8f8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -470,7 +470,7 @@ rollback), and the message ends with how many, or "Nothing was written.". | `logseq_error` | Logseq threw on the write and answered with an error object | `method`, `logseq_message` | | `page_exists` | `create-page` on a page that exists, whose properties Logseq would drop | `page` | | `rename_refused` | `rename-page` onto a name another page has (Logseq would merge the two) or an empty one | `old`, `new`, `why` (`exists` or `empty`) | -| `block_changed` | `set-todo-status`, `update-block --expect-hash`, `add-journal-block --upsert-heading --expect-hash`, `replace-text`, `set-property` and `remove-property` (every write that replaces a block's text) read the block again right before writing, and it no longer holds the text they read: someone changed it in between, and the write would have replaced that change. Nothing was written | `block` | +| `block_changed` | `set-todo-status`, `update-block --expect-hash`, `add-journal-block --upsert-heading --expect-hash`, `replace-text`, `set-property` and `remove-property` (every write that replaces a block's text) read the block again right before writing, and it no longer holds the text they read: someone changed it in between, and the write would have replaced that change. Nothing was written to the block. `copy-block --remove`, `remove-block` and `remove-property` (removing the page's property block) read the block with everything under it again right before the delete, and something under it changed: nothing was removed. For `copy-block` the copy is already written and stays | `block`; `copied_id` from `copy-block` | | `write_not_verified` | The write does not show in Logseq: not written, or not all of it | `method`, `target`, `expected`, `got`; `copied_id` from `copy-block` | | `read_only` | Writes are switched off by `[safety] read_only`, `LOGSEQ_CLI_READ_ONLY` or `--read-only`; every command that writes refuses, `--dry-run` included, before its first request | `source` (list of `config`, `env`, `flag`), `config_path` (`null` when no config file was found) | | `precondition_required` | A precondition is required and the call gave none: `[safety] require_preconditions`, `LOGSEQ_CLI_REQUIRE_PRECONDITIONS` or `--require-preconditions` is on, and the command got none of the options it takes: `--expect-hash` (`update-block` takes it too; `set-todo-status` also takes `--expect-marker`; `add-journal-block --upsert-heading` owes it only when it matches a block, otherwise it creates one and owes nothing), or `--expect-tree-hash` for `remove-block` / `delete-block`. Refused before the first request, nothing was written | `source` (list of `config`, `env`, `flag`), `config_path` (`null` when no config file was found), `options` | @@ -493,6 +493,13 @@ failed between a write and its proof, `unproven_write` names that write: it may have landed or not. Read the target before retrying; a retry of the whole command writes again what landed. +`copy-block --remove` whose source was deleted elsewhere during the copy +succeeds (exit 0, `"action": "moved"`) with `"source_gone": true` (`false` +otherwise): the copy is there and the source is gone, which is the move. Do +not remove the copy to clean up: it may be the only version left. If another +call moved the same block at the same time, its copy is there too; check the +target page before removing anything. + `copy-block` adds `copied_id` to every failure after the root of its copy landed, whatever its `reason`: a refusal, a write not verified, an error Logseq threw, a connection failure, at the removal too. The message says "The copy is .". The copy stays (no rollback), so @@ -548,6 +555,13 @@ elsewhere stands. Read the block again and repeat the command if it still makes sense. The check narrows the time between reading and writing to one request; it does not close it. +Not after `copy-block --remove`: there the copy is already written and +stays (`copied_id`), only the source was not removed, because something under +it changed during the copy. Repeating the command makes a second copy. Read +the copy and the source, then either keep the copy and remove the source with +`remove-block`, or remove the copy and move again. After `remove-block` the +block was not removed; read it again and decide. + `replace-text` writes the other blocks when one is refused, and names each refused block's reason in `failed_reasons` (`{id: reason}`) beside `failed`. diff --git a/CHANGELOG.md b/CHANGELOG.md index df0bdae..ed2ef04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -292,6 +292,26 @@ file carries most of the reasoning behind the tool. ### Fixed +- `copy-block --remove` deleted a block it had not copied. It reads the source + once, copies it block by block and then removes it, and the removal takes + everything under the source with it: a child added in Logseq while the copy + ran was deleted, missing from the copy, and the command reported success. + The window grows with the tree. Found by an adversarial pass over the + precondition work and reproduced against the test double (a fourth child + added after the second copied block: exit 0, `"blocks": 4`, the fourth child + gone); the gap was already on `main`. The removal now goes through the check + every removal has (see Changed): the source is read again with everything + under it right before the delete and compared with the state that was + copied, and a difference refuses with `block_changed`, nothing removed and + the copy named in `copied_id`. The message no longer asks to repeat the + command, which would make a second copy. A source deleted elsewhere during + the copy used to be reported as moved without a word; it is still a success, + the move is done, and the result says `"source_gone": true`. Moving the + source with `moveBlock` instead would close the window, but it places a block + only before another or as a first child, so the end of a page would need a + stand-in with its own removal, and `copy-block --remove` would become + `move-block`, which exists. + - A change made to a block while a command was changing it was written over without a word. `set-todo-status`, `replace-text`, `set-property` and `remove-property` read the block, build the new text from it and send the diff --git a/GLOSSARY.md b/GLOSSARY.md index 3676360..12133d3 100644 --- a/GLOSSARY.md +++ b/GLOSSARY.md @@ -129,8 +129,9 @@ The Hash of a Block together with everything under it, content and order the hash of the block alone would not see a child added after the read. Also the check before every removal of a block a command read: a removal cascades, so the block is read again just before the delete and refused with -`block_changed` when its Tree Hash is another (`remove-block`, and the page's -property block that `remove-property` removes with its last key). +`block_changed` when its Tree Hash is another (`copy-block --remove`, which +compares with the state it copied, `remove-block`, and the page's property +block that `remove-property` removes with its last key). _Avoid_: subtree hash, deep hash **Dry Run**: diff --git a/docs/commands.md b/docs/commands.md index e7e6ea4..b5c01da 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -135,7 +135,7 @@ A write that ends a section (`add-journal-block`, `add-journal-content`, | `insert-block --child-of UUID --first` | Insert as FIRST child instead of appending last (works with `--content` and `--tree`; order preserved). Only valid with `--child-of` | | `insert-block --tree "" [--child-of UUID \| --page NAME --top-level]` | Batch-insert a hierarchy in one call (DFS pre-order UUIDs returned). `--tree-file FILE` reads the same tab-indented text or JSON from a file | | `insert-block --tree ... --keep-ids` | Keep the `id::` values in the content instead of letting Logseq mint new ones, for moving or restoring an outline — top-level blocks included. Without it they are dropped and the count is reported on stderr; content that is nothing but `id::` lines is refused, since nothing would be left to write. With it, an id a block or page still has is refused before anything is written (a copy would put one uuid on two blocks), and so is a second `id::` line in one block. One that survives only as a `((ref))` target is restored: the block takes over the placeholder Logseq keeps under that uuid, and the ref resolves again. An `id::` line inside a code block (```` ``` ```` or `~~~`) is code and is written as is. The same flag and rule apply to `--content` and to `add-note-content`, `add-journal-block` and `add-journal-content` | -| `copy-block --id UUID --to-page NAME [--remove] [--ignore-refs] [--expect-hash H] [--dry-run]` | Copy/move block with children to another page. `--expect-hash H` (the `hash` of `get-block --json`, with `--remove` only; without it a usage error) refuses with `precondition_failed` unless the block is still the one that was read, before `--dry-run` and before the first copy; the copy and the removal come from that read. Every failure after the copy's root landed and was proven names the copy in `copied_id` ("The copy is ."): it stays, and a retry makes a second one. A root placed before the empty blocks at the end of the page whose place could not be proven is not named; `writes_landed` says it landed. The copy gets new UUIDs and leaves the source's `id::` lines out, so `--remove` refuses while `((block-refs))` point into the source (`--ignore-refs` overrides); `move-block` keeps them | +| `copy-block --id UUID --to-page NAME [--remove] [--ignore-refs] [--expect-hash H] [--dry-run]` | Copy/move block with children to another page. `--expect-hash H` (the `hash` of `get-block --json`, with `--remove` only; without it a usage error) refuses with `precondition_failed` unless the block is still the one that was read, before `--dry-run` and before the first copy; the copy and the removal come from that read. With `--remove` the source is removed only as it was copied: a block added or changed under it during the copy refuses the removal with `block_changed`, and a source deleted elsewhere in the meantime is a move done (`"source_gone": true`, exit 0). Every failure after the copy's root landed and was proven names the copy in `copied_id` ("The copy is ."): it stays, and a retry makes a second one. A root placed before the empty blocks at the end of the page whose place could not be proven is not named; `writes_landed` says it landed. The copy gets new UUIDs and leaves the source's `id::` lines out, so `--remove` refuses while `((block-refs))` point into the source (`--ignore-refs` overrides); `move-block` keeps them | | `move-block --id UUID (--under UUID \| --before UUID) [--expect-hash H] [--dry-run]` | Structural move (`--expect-hash H`: refuses with `precondition_failed`, before `--dry-run` and the move, unless the block is still the one that was read; it sees the content, not the position): the block keeps its UUID, so `((block-refs))` to it survive. Prefer over `copy-block --remove`, which writes a new block and deletes the original. `--under` nests as first child, `--before` places it in front as a sibling | ### Meta (4) diff --git a/docs/safety.md b/docs/safety.md index 4568e9f..6a1a41c 100644 --- a/docs/safety.md +++ b/docs/safety.md @@ -134,7 +134,8 @@ protection holds only for calls that pass a hash; a call that passes none is written as before. A precondition and `block_changed` guard different things. `block_changed` is -the command's own re-read: between its read and its write, did the block change? +the command's own re-read: between its read and its write (or its removal), +did the block change? A precondition is the caller's: is the block still what I read, minutes ago, when I decided what to write? A precondition that fails is `precondition_failed`; after it, read the block again and decide again. Repeating the same change with @@ -149,11 +150,24 @@ way. caught by the precondition. `update-block` narrows that gap with its own re-read (`block_changed`); nothing closes it, because the API has no conditional write. -- **It does not cover the copy in `copy-block --remove`.** The command copies - the subtree block by block and deletes the source at the end, so a child - added under the source while the copy runs is deleted with it. The window - grows with the size of the tree, and `--expect-hash` checks the state before - the copy starts, not during it. +- **A removal is checked, not made atomic.** `copy-block --remove` copies the + subtree block by block and deletes the source at the end; `--expect-hash` + checks the state before the copy starts, not during it. Before the delete + the command reads the source with everything under it again and refuses + with `block_changed` if that is no longer what it copied, so a child added + while the copy runs stays. `remove-block` and the page's property block that + `remove-property` removes do the same. What stays open is the few requests + between that read and the delete: the editor check, with a block open also + its read of the subtree, which takes longer the larger the tree is. +- **A copy that breaks off stays.** There is no rollback: when the removal is + refused, or a block of the copy or the connection fails after the copy's + root landed, the copy (or the part of it that landed) remains, and the + error names it in `copied_id`. A root placed before the empty blocks at the + end of a page (`keep_empty_blocks_last`) that lands while the step after it + fails is not named; `writes_landed` says that something did. +- **A ref set during a copy is not seen.** `copy-block --remove` looks for + `((block-refs))` into the source before it copies; one added while the copy + runs points at nothing after the move. Nothing is lost, but the ref dangles. - **The hash sees content, not position.** `move-block --expect-hash` does not notice that the block was moved since it was read; it notices that its text or properties changed. `remove-block` takes the hash of the block and diff --git a/logseq_cli/commands/edit.py b/logseq_cli/commands/edit.py index ccfd730..f42dda8 100644 --- a/logseq_cli/commands/edit.py +++ b/logseq_cli/commands/edit.py @@ -72,7 +72,7 @@ insert_tree_at_page_end, move_block_verified, ) -from logseq_cli.writerefused import WriteRefused, partial_state +from logseq_cli.writerefused import BlockNotFound, WriteRefused, partial_state @cli.command("update-block", cls=WriteCommand, owes=lambda params: Owed(("--expect-hash",)), epilog="""\b @@ -933,6 +933,9 @@ def add_block_ref(ctx, source_id, journal_date, page, under_heading, dry_run, as a section goes before the empty blocks that end it, so they stay last, and nothing is overwritten. --expect-hash goes with --remove only: a plain copy changes no block. + --remove removes the source only as it was copied: something added or + changed under it during the copy refuses with block_changed and removes + nothing. A source deleted elsewhere meanwhile is a move done (source_gone). A failure after the copy's root landed names it (copied_id, "The copy is ."): the copy stays, and running the command again copies again. """) @@ -1032,22 +1035,34 @@ def _copy_tree(block, parent_uuid=None): create_missing_page(api, target) count = _copy_tree(source) + source_gone = False if remove: # Only reached when every insert above was proven, so the source is # removed against a copy that is known to exist, never a claimed one. - api.remove_block(block_id, expect_tree=None) + # And only as it was copied: a child added under it since the read + # would go with it, and the copy does not have it (block_changed). + try: + api.remove_block(block_id, expect_tree=tree_hash(source)) + except BlockNotFound: + # Deleted elsewhere during the copy: copy there, source gone, which + # is the move asked for. A failure would invite cleaning up, and + # that would hit the only version left. + source_gone = True action = "Moved" if remove else "Copied" result_data = {"action": action.lower(), "blocks": count, **names, "source_id": block_id} result_data.update(before_empty_fields(ahead_of.get("uuid"))) if remove: result_data["refs_broken"] = len(refs) + result_data["source_gone"] = source_gone if as_json: output(result_data, True) else: click.echo(f"{action} {count} block(s) to '{to_page}'" f"{BEFORE_EMPTY_SUFFIX if ahead_of.get('uuid') else ''}.") + if source_gone: + click.echo(" The source was already gone.") if refs: click.echo(f" {len(refs)} block ref(s) into the source now point at nothing") diff --git a/skills/logseq-cli/SKILL.md b/skills/logseq-cli/SKILL.md index ec78425..e8dffa3 100644 --- a/skills/logseq-cli/SKILL.md +++ b/skills/logseq-cli/SKILL.md @@ -65,7 +65,7 @@ for example the token from Logseq's API settings, passed as `LOGSEQ_TOKEN`. how many. Ask the user to leave the block, then retry once (not in a loop). - `write_not_verified`: read the block before retrying. Someone writing there at the same time can make a write that landed look missing. - - `block_changed`, `precondition_failed`: the block changed after it was read; read it again and decide again. `precondition_required`: pass `--expect-hash` (from `get-block --json`) or `--expect-marker`; `remove-block` wants `--expect-tree-hash` (its `tree_hash`). `precondition_failed` is the caller's check (`block_changed` is the command's own re-read): never repeat the same change with a fresh hash. The hash sees content, not position; a write's output has no new hash, so read again before a second write to the block. Pass `--expect-hash` whenever you change a block you read earlier, switch or not. + - `block_changed`, `precondition_failed`: the block changed after it was read; read it again and decide again. `precondition_required`: pass `--expect-hash` (from `get-block --json`) or `--expect-marker`; `remove-block` wants `--expect-tree-hash` (its `tree_hash`). `precondition_failed` is the caller's check (`block_changed` is the command's own re-read): never repeat the same change with a fresh hash. The hash sees content, not position; a write's output has no new hash, so read again before a second write to the block. Pass `--expect-hash` whenever you change a block you read earlier, switch or not. After `copy-block` a failure may name `copied_id`: that copy stays, and repeating the command makes a second one. - `rename_refused`, `page_exists`: nothing was written. - `read_only`: writes are off on purpose. Do not retry or go around it; tell the user. diff --git a/tests/test_copy_block_remove_changed_source.py b/tests/test_copy_block_remove_changed_source.py index 7fbfce7..c724694 100644 --- a/tests/test_copy_block_remove_changed_source.py +++ b/tests/test_copy_block_remove_changed_source.py @@ -67,3 +67,68 @@ def test_a_root_that_did_not_land_names_no_copy(double): assert error["reason"] == "write_not_verified" assert "copied_id" not in error assert "The copy is" not in error["error"] + + +# --- The source is removed only as it was copied ---------------------------- + +def _after_child_insert(double, n, then): + """Run ``then`` once the copy's ``n``-th child insert has landed. The + root goes through appendBlockInPage, the children through insertBlock.""" + insert = double._handlers["logseq.Editor.insertBlock"] + calls = [] + + def hooked(args): + answer = insert(args) + calls.append(1) + if len(calls) == n: + then() + return answer + double._handlers["logseq.Editor.insertBlock"] = hooked + + +def test_a_child_added_during_the_copy_is_not_removed(double): + source = double.uuid_of("source block") + _after_child_insert(double, 2, lambda: double.add_child(source, "added during the copy")) + r = _move(double) + assert r.exit_code == 1 + error = json.loads(r.stderr) + assert error["reason"] == "block_changed" + assert error["copied_id"] == _copy_root(double) + assert "repeat" not in error["error"].lower() + assert f"The copy is {_copy_root(double)}." in error["error"] + assert double.sent("removeBlock") == [] + assert double.tree("Source Page") == [("source block", [ + ("first kid", []), ("second kid", []), ("third kid", []), + ("added during the copy", [])])] + assert double.tree("Target Page")[-1] == ("source block", [ + ("first kid", []), ("second kid", []), ("third kid", [])]) + + +def test_a_source_deleted_during_the_copy_is_a_move_done(double): + source = double.uuid_of("source block") + _after_child_insert(double, 1, lambda: double.delete_block(source)) + r = _move(double) + assert r.exit_code == 0, r.stderr + result = json.loads(r.stdout) + assert result["action"] == "moved" + assert result["source_gone"] is True + assert double.sent("removeBlock") == [] + assert double.tree("Target Page")[-1] == ("source block", [ + ("first kid", []), ("second kid", []), ("third kid", [])]) + + +def test_a_source_deleted_during_the_copy_says_so_in_text(double): + source = double.uuid_of("source block") + _after_child_insert(double, 1, lambda: double.delete_block(source)) + r = split_runner().invoke(cli, ["--token", "t", "copy-block", "--id", source, + "--to-page", "Target Page", "--remove"]) + assert r.exit_code == 0, r.stderr + assert "The source was already gone." in r.stdout + + +def test_an_unchanged_source_is_moved_as_before(double): + r = _move(double) + assert r.exit_code == 0, r.stderr + result = json.loads(r.stdout) + assert (result["action"], result["blocks"], result["source_gone"]) == ("moved", 4, False) + assert double.tree("Source Page") == [] diff --git a/tests/test_incoming_refs.py b/tests/test_incoming_refs.py index 9f632c8..6b8fb0a 100644 --- a/tests/test_incoming_refs.py +++ b/tests/test_incoming_refs.py @@ -239,5 +239,5 @@ def test_ignore_refs_moves(self): r = _run(api, "copy-block", "--id", ROOT, "--to-page", "Elsewhere", "--remove", "--ignore-refs", "--json") assert r.exit_code == 0, r.output - api.remove_block.assert_called_once_with(ROOT, expect_tree=None) + api.remove_block.assert_called_once_with(ROOT, expect_tree=_tree_as_read(api)) assert json.loads(r.output)["refs_broken"] == 3 diff --git a/tests/test_move_block.py b/tests/test_move_block.py index 4611c1d..bc0a8e4 100644 --- a/tests/test_move_block.py +++ b/tests/test_move_block.py @@ -20,6 +20,7 @@ import pytest from click.testing import CliRunner +from logseq_cli.blocktext import tree_hash from logseq_cli.cli import cli from tests.conftest import split_runner from tests.logseq_http_double import LogseqHttpDouble @@ -154,7 +155,8 @@ def test_successful_copy_still_removes(self): r = CliRunner().invoke(cli, [ "copy-block", "--id", SRC, "--to-page", "Target", "--remove"]) assert r.exit_code == 0, r.output - api.remove_block.assert_called_once_with(SRC, expect_tree=None) + api.remove_block.assert_called_once_with( + SRC, expect_tree=tree_hash({"uuid": SRC, "content": "Head", "children": []})) class _Graph: