diff --git a/AGENTS.md b/AGENTS.md index cbe7ddb..9fb346a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -181,7 +181,7 @@ logseq-cli get-todos --page-type journal # Filter by what the task says (regex, case-insensitive) logseq-cli get-todos --match "review|contract" --json -# --json: {"todos": [{marker, content, page, uuid, journal_day?, ...}], "count": N} +# --json: {"todos": [{marker, content, page, uuid, hash, journal_day?, ...}], "count": N} # Tasks standing in a date range, including ones carried forward by ((block-ref)) logseq-cli get-todos --from 2026-09-14 --to 2026-09-16 --json @@ -470,10 +470,12 @@ 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`, `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 | `block` | | `write_not_verified` | The write does not show in Logseq: not written, or not all of it | `method`, `target`, `expected`, `got` | | `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 | +| `precondition_failed` | `--expect-hash` / `--expect-marker` (`set-todo-status`, `update-block`, `set-block-property`, `remove-property --id`, `move-block`, `copy-block --remove`, `add-journal-block --upsert-heading` when it matches a block: the hash is of that match, not of the `--under-heading` host; with no match the block is created and the precondition is moot), `remove-block --expect-tree-hash`: the block (for `remove-block`: the block or anything under it) is no longer the one the hash was read from, or its marker is not the expected one. Nothing was written. The current hash is not given, on purpose | `expected_hash`, `expected_tree_hash` and/or `expected_marker` (as given), `actual_marker` (`null` for a block without one), `first_line` | | `block_not_found` | The block the command names does not exist; the placeholder Logseq keeps for a missing ref target counts as none | `id`; for `set-todo-status --content` the `content` and `page` searched instead, no `id` | | `dead_ref` | `set-todo-status --follow-refs`: the chain of refs leads to a block that does not exist | `id` (the missing block), `followed` | | `ref_cycle` | `set-todo-status --follow-refs`: the chain of refs comes back to a block it passed | `id` (the block met again), `followed` | @@ -511,6 +513,22 @@ without moving the cursor. An insert whose text holds `((X))` is refused while X is open in the editor and has no `id::` yet (storing the id would write into X). A `--keep-ids` write is refused while any block is open. +On `precondition_required`: read the block with `get-block --json` and pass +its `hash` as `--expect-hash` (or the marker you decided on as +`--expect-marker`). Do not switch the requirement off to get past it; the user +set it. `read_only` is checked first: if both apply, you get `read_only`. + +On `precondition_failed`: the block changed after you read it. Read it again +with `get-block --json` and decide again on what it says now. Do not repeat +the same change with the fresh hash: that writes your old decision over the +change that was made. It differs from `block_changed`, which the command finds +itself between its own read and its write. + +What a precondition does not catch: a change between the check and the write +(reading and writing are two requests), and a move (the hash sees content, not +position). A write's output carries no new hash yet, so read the block again +before changing it a second time. + `write_not_verified` can be a false alarm when someone else wrote the block between the write and the read-back; read the block before retrying. @@ -522,6 +540,28 @@ request; it does not close it. `replace-text` writes the other blocks when one is refused, and names each refused block's reason in `failed_reasons` (`{id: reason}`) beside `failed`. +### 9. Changing a Block You Read Earlier + +Under `--json`, the blocks of `get-block`, `get-page`, `get-journal-range`, +`find-block` and `get-todos` carry a `hash`; `get-block` also carries a +`tree_hash` (not with `--no-children`). The text output shows neither. + +When you read a block, decide, and change it later, pass what you read. The +user may have edited the block in Logseq in between, and without a +precondition the write goes through over that edit, even if the user has not +switched `require_preconditions` on. Read with `get-block --json`, then give +its `hash` as `--expect-hash` (for `set-todo-status` the marker you decided +on can be `--expect-marker`; `remove-block` takes `--expect-tree-hash`): + +```bash +HASH=$(logseq-cli get-block --id 6650a1b2-0000-4000-8000-000000000001 --json | jq -r .hash) +logseq-cli update-block --id 6650a1b2-0000-4000-8000-000000000001 \ + --content "TODO ship the parser, fixed" --expect-hash "$HASH" +``` + +On `precondition_failed` read the block again and decide again; see the +section above. + ## Environment Variables Two matter in agent work: `LOGSEQ_TOKEN`, the documented way to pass the @@ -530,7 +570,9 @@ heading for journal writes. A config setting works like them: `[graph] keep_empty_blocks_last` (or `LOGSEQ_CLI_KEEP_EMPTY_BLOCKS_LAST`) makes a write that ends a section go before the empty blocks that end it, instead of leaving one in front of the entry; it is -off by default, and nothing is overwritten. Every variable, with its default, is in +off by default, and nothing is overwritten. `LOGSEQ_CLI_READ_ONLY` and `LOGSEQ_CLI_REQUIRE_PRECONDITIONS` switch the two +`[safety]` guards on; they only tighten, and are not yours to unset. Every +variable, with its default, is in [docs/configuration.md](docs/configuration.md#environment-variables-and-flags). ## Command Summary diff --git a/CHANGELOG.md b/CHANGELOG.md index c7a7428..cefae29 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,46 @@ file carries most of the reasoning behind the tool. ### Added +- 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`, + `get-journal-range`, `find-block` and `get-todos` give each block a `hash` + under `--json`. The write commands take it back: `--expect-hash` on + `set-todo-status`, `update-block`, `set-block-property`, `remove-property + --id`, `move-block`, `copy-block --remove` and `add-journal-block + --upsert-heading`, `--expect-tree-hash` on `remove-block` (and its alias + `delete-block`), and `--expect-marker` on `set-todo-status`. A block that is + not what was read is refused with `precondition_failed`, before `--dry-run` + and before anything is written; the refusal never carries the current hash, + since the next step is to read the block and decide again, not to repeat the + change with a hash that now matches (`examples/safe-update-block.sh`). It is + not the check `block_changed` makes: that one compares a command with its own + read, this one the caller with its read, minutes earlier. + - The hash is 12 hex characters over the block's text without collapsing, + `id::` and the clock lines of the logbook, so folding a block or a running + clock leaves it alone while an edit, a marker change or a completion moves + it. It sees content, not position: `move-block` does not notice that the + block was moved since. `remove-block` takes the hash of the whole subtree, + because the hash of the block alone would not see a child added after the + read. + - `[safety] require_preconditions = true` (also `LOGSEQ_CLI_REQUIRE_PRECONDITIONS` + and `--require-preconditions`; all three only tighten, like `read_only`) + makes the option mandatory: a call that changes a block it read and passes + none is refused with `precondition_required` before its first request. + `add-journal-block --upsert-heading` owes it only when a block matches; + creating one stays free. It is off by default: calls without a hash would + start to refuse, and since a write's output carries no new hash yet, a + second write to the same block needs a fresh read in between, which protects + no more than `block_changed`. `doctor` shows it. + - Cost: one `hash` line per block in the output, about 30 bytes. Measured on + the HTTP test double with invented blocks: 100 short blocks add 3,000 + characters to indented `get-page --json`; the relative growth depends on + how large the rest of each block is (7% for the double's block shape, with + no children and no properties). + - Not covered, and said so in [docs/safety.md](docs/safety.md): there is no + compare-and-swap (a change between the check and the write is not caught), + the hash does not see position, and write outputs carry no new hash. + - `get-todos --page-type journal|page` keeps the tasks whose block stands on a journal page, or on any other page (#118). Before, the only way to split the two was `jq` on `journal_day`, which leaves `count` at the unfiltered value and diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 874c131..cc8a16a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -38,8 +38,9 @@ logseq-cli/ │ ├── outlinetext.py # Indented outline text to a block tree, and back │ ├── output.py # Results on stdout, failures on stderr, --json │ ├── pagenames.py # Which page a name means (an alias as Logseq resolves it), and the name Logseq creates a page under +│ ├── preconditions.py # Does the block still match what the caller read: --expect-hash, --expect-tree-hash, --expect-marker │ ├── render.py # Blocks to text; finding and resolving references -│ ├── safety.py # What keeps a command from writing: [safety] read_only, the gate every writing command carries +│ ├── safety.py # What keeps a command from writing: [safety] read_only and require_preconditions, the gates every writing command carries │ ├── strictinsert.py # Strict Insert: trees and --keep-ids writes land where sent; whether a move is possible; which empty block a write at the end goes before │ ├── tasks.py # What a task is: the eleven markers, their states, reading and swapping a marker │ ├── writerefused.py # The ways a write ends without being done or proven, each with its reason @@ -76,6 +77,10 @@ logseq-cli/ if it writes: that mark is what makes the command refuse under `[safety] read_only`. `tests/test_read_only.py` fails by comparing the commands that carry it with the commands that write + - A command that changes a block it read also declares `owes=` on that + mark, naming the `--expect-*` options it takes. + `tests/test_precondition_coverage.py` finds such commands by those + options and fails when one of them skips the check A Logseq method the client has not sent before goes into `_METHODS` in `api.py` first, a write with its editor rule and its proof: `call()` diff --git a/GLOSSARY.md b/GLOSSARY.md index 58d0349..d6b6ecb 100644 --- a/GLOSSARY.md +++ b/GLOSSARY.md @@ -106,6 +106,29 @@ refused. It exists because Logseq answers some failed writes with HTTP 200 and a `null` body, so a write is verified after the fact rather than trusted. _Avoid_: safe write, verified write +**Precondition**: +What a caller says it read before it changes a Block: the Hash (or +Tree Hash) of the block, or for `set-todo-status` its marker. The write is +refused, with `precondition_failed`, unless the block is still what the caller +read. It guards the caller's decision across calls, which Strict Insert (was +my write stored) and the `block_changed` re-read (did the block change between +this command's own read and its write) do not: only the caller knows what it +read. `[safety] require_preconditions` makes a Precondition mandatory. +_Avoid_: lock, guard, version check + +**Hash**: +A short digest of one Block's content, shown as `hash` by the read commands +with `--json`. It sees text, properties and the DONE lines of the logbook; it +ignores collapsing, `id::` lines, clock lines and where the block stands, so +moving a block does not change it. Compared by value, never ordered. +_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. +_Avoid_: subtree hash, deep hash + **Dry Run**: A preview of a write that performs no write. Available on every Command that writes. diff --git a/README.md b/README.md index 35bdeec..2c7b8e3 100644 --- a/README.md +++ b/README.md @@ -97,6 +97,7 @@ files would not. | report a write that did not land as done | never; every write is proven | | show a change before making it | does it: `--dry-run` on every command that writes (not under `read_only`, below) | | write when it was told to only read | refuses: `[safety] read_only = true` switches every write off, before the first request | +| change a block you edited after your agent read it | refuses when the agent passes what it read (`--expect-hash`); `[safety] require_preconditions` makes that mandatory, see [docs/safety.md](docs/safety.md#requiring-a-precondition) | | undo a change | cannot; a deletion is final, so preview first | | write while Logseq is closed | cannot; your agent is told to stop rather than edit the files | | read more than fits in its context | cuts page and journal reads and searches to a size it asks for, and says what it left out | @@ -127,6 +128,12 @@ itself, and one that can write files can edit the config or the Markdown files. (config file, environment variable, `--read-only`), how to keep the config file out of an agent's reach, and what to check when it seems not to apply. +To have the agent say what it read before it changes a block, add +`require_preconditions = true` to the same section. A command that changes a +block it read then refuses without `--expect-hash` (`--expect-tree-hash` for +`remove-block`), and `doctor` shows the switch like `read_only`; see +[Requiring a precondition](docs/safety.md#requiring-a-precondition). + ### Bounded output Reads grow with the graph. On a real one (nearly four years of daily entries) @@ -241,7 +248,7 @@ non-zero rather than returning an empty result. See [docs/configuration.md](docs/configuration.md) for every option, what happens without it, and how to read the right values out of your own graph. -The same file can switch writes off for an agent (`[safety] read_only`, see +The same file can switch writes off for an agent (`[safety] read_only` and `require_preconditions`, see [Limiting what an agent can do](#limiting-what-an-agent-can-do)), and keep the empty blocks you leave at the end of a section last, so entries land before them and no empty line is left (`[graph] keep_empty_blocks_last`, off by default, see diff --git a/config.example.de.toml b/config.example.de.toml index 3007a71..1a67664 100644 --- a/config.example.de.toml +++ b/config.example.de.toml @@ -93,3 +93,11 @@ project_tag_prefix = "#vorhaben/" # Keep this file out of the agent's reach, or it is only a request: # docs/safety.md. Check that it applies with `logseq-cli doctor`. # read_only = true + +# Makes a command that changes a block it read refuse unless the call says what +# it read: --expect-hash (the hash of `get-block --json`) or --expect-marker. +# Refused with reason precondition_required, before the first request. The +# environment variable LOGSEQ_CLI_REQUIRE_PRECONDITIONS and the option +# --require-preconditions can switch it on too, but nothing switches a `true` +# here off. Turn it on when an agent writes while you edit in Logseq yourself. +# require_preconditions = true diff --git a/config.example.toml b/config.example.toml index 5b35d55..e193365 100644 --- a/config.example.toml +++ b/config.example.toml @@ -115,3 +115,11 @@ project_tag_prefix = "#projects/" # Keep this file out of the agent's reach, or it is only a request: # docs/safety.md. Check that it applies with `logseq-cli doctor`. # read_only = true + +# Makes a command that changes a block it read refuse unless the call says what +# it read: --expect-hash (the hash of `get-block --json`) or --expect-marker. +# Refused with reason precondition_required, before the first request. The +# environment variable LOGSEQ_CLI_REQUIRE_PRECONDITIONS and the option +# --require-preconditions can switch it on too, but nothing switches a `true` +# here off. Turn it on when an agent writes while you edit in Logseq yourself. +# require_preconditions = true diff --git a/docs/commands.md b/docs/commands.md index f187e15..fe1b313 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -54,6 +54,7 @@ Given before the command name (`logseq-cli --read-only add-journal-block …`): | `--host`, `--port`, `--token` | Where Logseq's HTTP API is and how to authenticate; each has an environment variable ([configuration.md](configuration.md#environment-variables-and-flags)) | | `--no-cache` | Bypass the in-memory read cache for this call | | `--read-only` | Refuse every command that writes, `--dry-run` included, for this call. Can only tighten: nothing loosens a `[safety] read_only = true` in the config. See [safety.md](safety.md) | +| `--require-preconditions` | Refuse a command that changes a block it read unless the call says what it read (`--expect-hash` or `--expect-marker`), for this call. Can only tighten: nothing loosens a `[safety] require_preconditions = true` in the config. `set-todo-status`, `update-block`, `set-block-property`, `remove-property --id`, `move-block` and `copy-block --remove` (and `remove-block`, with `--expect-tree-hash`) ask for it so far. See [safety.md](safety.md) | | `--version` | Print the version | ### Parameter aliases @@ -87,10 +88,10 @@ graph. | Command | Description | |---------|-------------| | `get-all-pages` | List all pages | -| `get-page --page NAME [--no-backlinks] [--resolve-refs] [--with-ids] [--heading "## X"] [--outline] [--max-chars N] [--from-block UUID] [--format markdown]` | Page content with backlinks; optionally inline `((uuid))` refs or prefix UUIDs per line. `--no-backlinks` skips the backlink lookup, `--heading` returns only that section (searched recursively) and fails when the page has no such heading. `--outline` lists the headings with their uuids; `--max-chars` cuts the output to size and `--from-block` continues it — see [Bounded output](../README.md#bounded-output). With `--resolve-refs`, a ref whose target was deleted is named on stderr — on stdout it renders exactly like an unresolved one | -| `get-block --id UUID [--no-children]` | Block by UUID; `--no-children` returns the block alone. Text output starts with `Page: ` and `Parent: `, or `Parent: (page)` directly under the page; the uuid works as the next `--id`. `--json` is Logseq's answer as it comes, with page and parent as database ids | -| `find-block --content TEXT [--page NAME] [--regex] [--first \| --limit N \| --exactly-one] [--with-children \| --uuid-only]` | Find blocks by content. `--uuid-only` prints bare uuids, one per line, and fails on no match. `--exactly-one` fails unless exactly one block matches and lists the matches otherwise; use it for a block to write to, `U=$(... --exactly-one --uuid-only)`, where `--first` would pick one of several. A common word matches thousands of blocks, so `--limit N` caps the output and the number withheld goes to stderr; `--first` is the same with N=1. `--with-children` prints each match with its sub-blocks indented, instead of guessing a line count with `get-page \| grep -A`; costs one extra read per match, capped at 25 with the remainder reported | -| `get-journal-range --from DATE --to DATE [--resolve-refs] [--tail N] [--limit N] [--heading "## Log"] [--max-chars N] [--from-block UUID]` | Batch journal read; parallel (5 workers default). `--tail/--limit/--heading/--max-chars` bound the output, `--from-block` continues a cut one — see [Bounded output](../README.md#bounded-output) | +| `get-page --page NAME [--no-backlinks] [--resolve-refs] [--with-ids] [--heading "## X"] [--outline] [--max-chars N] [--from-block UUID] [--format markdown]` | Page content with backlinks; optionally inline `((uuid))` refs or prefix UUIDs per line. `--no-backlinks` skips the backlink lookup, `--heading` returns only that section (searched recursively) and fails when the page has no such heading. `--outline` lists the headings with their uuids; `--max-chars` cuts the output to size and `--from-block` continues it — see [Bounded output](../README.md#bounded-output). With `--resolve-refs`, a ref whose target was deleted is named on stderr — on stdout it renders exactly like an unresolved one. `--json` blocks carry `hash` (as in `get-block`), children included, computed before `--resolve-refs`; the backlinks carry none, and the text output shows none | +| `get-block --id UUID [--no-children]` | Block by UUID; `--no-children` returns the block alone. Text output starts with `Page: ` and `Parent: `, or `Parent: (page)` directly under the page; the uuid works as the next `--id`. `--json` is Logseq's answer with page and parent as database ids, plus `hash` on the block and on each child block, and `tree_hash` (block and all descendants, content and order) on the block itself unless `--no-children`. The hash ignores collapsing, `id::` and `CLOCK` lines; it changes with text, marker, properties and `State "DONE"` lines. Text output shows neither | +| `find-block --content TEXT [--page NAME] [--regex] [--first \| --limit N \| --exactly-one] [--with-children \| --uuid-only]` | Find blocks by content. `--uuid-only` prints bare uuids, one per line, and fails on no match. `--exactly-one` fails unless exactly one block matches and lists the matches otherwise; use it for a block to write to, `U=$(... --exactly-one --uuid-only)`, where `--first` would pick one of several. A common word matches thousands of blocks, so `--limit N` caps the output and the number withheld goes to stderr; `--first` is the same with N=1. `--with-children` prints each match with its sub-blocks indented, instead of guessing a line count with `get-page \| grep -A`; costs one extra read per match, capped at 25 with the remainder reported. `--json` matches carry `hash` (as in `get-block`), and their children with `--with-children`; the text output shows none | +| `get-journal-range --from DATE --to DATE [--resolve-refs] [--tail N] [--limit N] [--heading "## Log"] [--max-chars N] [--from-block UUID]` | Batch journal read; parallel (5 workers default). `--tail/--limit/--heading/--max-chars` bound the output, `--from-block` continues a cut one — see [Bounded output](../README.md#bounded-output). `--json` blocks carry `hash` (as in `get-block`), children included, computed before `--resolve-refs`; the text output shows none | | `search-pages --query TEXT` | Case-insensitive name search | | `get-backlinks --page NAME [--with-context] [--limit N]` | Pages linking to NAME. `--with-context` also shows the blocks that do the linking — they arrive with the same API call, so it costs no extra read; `--limit` (default 3) caps the blocks per page and reports the remainder; `0` keeps all | | `get-journal-summary --range RANGE [--no-content]` | Journal summary (today, this week, last 30 days). `--no-content` drops the per-day bodies | @@ -111,7 +112,7 @@ graph. |---------|-------------| | `create-page --name NAME [--content TEXT] [--dry-run]` | Create a new page. Fails if it already exists, rather than appending `--content` to what is there; `--dry-run` reports which of the two a run would be. An `id::` line in `--content` is dropped with a note | | `add-journal-entry --content TEXT [--date DATE] [--multi-block] [--dry-run]` | Add journal entry (deprecated, use add-journal-block). `--date` defaults to today; `--multi-block` splits multi-line content into one block per line. An `id::` line in `--content` is dropped with a note | -| `add-journal-block --content TEXT [--date DATE] [--upsert-heading "### X"] [--no-preserve] [--keep-ids]` | Add block to journal — auto-detects hierarchical content (`--under-heading`, `--top-level`, `--dry-run`). `--date` defaults to today. `--upsert-heading` updates a matching child block under `--under-heading` instead of adding a second one; the block keeps its properties, as with `update-block`. `--content-file FILE` reads the whole file as one tree: no shell quoting, flush `- ` lines become sibling roots; `--content-file -` reads stdin. On `update-block`, `insert-block`, `add-note-content` and `add-journal-content`, `--content-file` is `--content` read from a file, with the same rules | +| `add-journal-block --content TEXT [--date DATE] [--upsert-heading "### X" [--expect-hash H]] [--no-preserve] [--keep-ids]` | Add block to journal — auto-detects hierarchical content (`--under-heading`, `--top-level`, `--dry-run`). `--date` defaults to today. `--upsert-heading` updates a matching child block under `--under-heading` instead of adding a second one; the block keeps its properties, as with `update-block`. `--expect-hash H` (the `hash` of `get-block --json`, with `--upsert-heading` only) refuses with `precondition_failed`, before `--dry-run` and before the journal page is created, unless the block it would replace is still the one that was read; with no matching block there is nothing to check, the block is created and a note says the hash had no object. `--content-file FILE` reads the whole file as one tree: no shell quoting, flush `- ` lines become sibling roots; `--content-file -` reads stdin. On `update-block`, `insert-block`, `add-note-content` and `add-journal-content`, `--content-file` is `--content` read from a file, with the same rules | | `add-journal-content (--content TEXT \| --content-file FILE) [--date DATE] [--keep-ids]` | Add hierarchical content to journal (`--under-heading`, `--top-level`, `--dry-run`). `--date` defaults to today | | `add-note-content --page NAME (--content TEXT \| --content-file FILE) [--under-heading "## X"] [--no-create] [--property K=V] [--keep-ids] [--dry-run]` | Add content to any page; optionally under a heading (created if missing). The page is created when missing unless `--no-create` is given. `--property` sets `key:: value` on the root block, repeatable. `--dry-run` reports the target, the block count and whether page or heading would be created | @@ -124,26 +125,26 @@ A write that ends a section (`add-journal-block`, `add-journal-content`, | Command | Description | |---------|-------------| -| `update-block (--id UUID \| --where-content TEXT [--page NAME] [--regex]) (--content TEXT \| --content-file FILE) [--dry-run]` | Update block content. `--content` is ONE block and has no tree path: a `- ` or `# ` line after the first (indented too) or a code fence nothing closes is refused, since Logseq would read it as a block of its own: use `insert-block --child-of` for children. `--where-content` selects by text instead of UUID and aborts unless exactly one block matches. The block's properties are kept as written, values as their original text, unless `--content` sets the key itself; the one change is Logseq's own spelling of a key (`created_at` is stored and written back as `created-at`). The block's own `id::` line passes; one naming another uuid is dropped with a note, since it would become this block's uuid | -| `remove-block --id UUID [--ignore-refs] [--dry-run]` | Delete a block and its children (alias: `delete-block`). `--dry-run` reports the descendant count. Refuses while `((block-refs))` from elsewhere point into the block or its children, and lists them; `--ignore-refs` removes anyway | +| `update-block (--id UUID \| --where-content TEXT [--page NAME] [--regex]) (--content TEXT \| --content-file FILE) [--expect-hash H] [--dry-run]` | Update block content. `--expect-hash H` (the `hash` of `get-block --json`) refuses with `precondition_failed` unless the block still has that hash; it is checked on a read past the cache, before `--dry-run` and before anything is written, and the write then refuses with `block_changed` if the block changed after the check. `--content` is ONE block and has no tree path: a `- ` or `# ` line after the first (indented too) or a code fence nothing closes is refused, since Logseq would read it as a block of its own: use `insert-block --child-of` for children. `--where-content` selects by text instead of UUID and aborts unless exactly one block matches. The block's properties are kept as written, values as their original text, unless `--content` sets the key itself; the one change is Logseq's own spelling of a key (`created_at` is stored and written back as `created-at`). The block's own `id::` line passes; one naming another uuid is dropped with a note, since it would become this block's uuid | +| `remove-block --id UUID [--ignore-refs] [--expect-tree-hash H] [--dry-run]` | Delete a block and its children (alias: `delete-block`). `--dry-run` reports the descendant count. `--expect-tree-hash` (the `tree_hash` of `get-block --json`) refuses, writing nothing, unless the block and everything under it is still what was read: a child added since changes it. There is no `--expect-hash` here, the hash of the block alone would not see that child. Refuses while `((block-refs))` from elsewhere point into the block or its children, and lists them; `--ignore-refs` removes anyway | | `add-block-ref --source-id UUID (--journal-date DATE \| --page NAME) [--under-heading "## X"] [--dry-run]` | Write a `((block-ref))` pointing at an existing block. Journal defaults to today, heading to `LOGSEQ_JOURNAL_HEADING`. A source uuid no block has is refused before anything is written, `--dry-run` included — a ref to it would render as nothing. A source without an `id::` line gets one, as Logseq's editor gives it; every command that writes a `((uuid))` does this | -| `set-todo-status (--id UUID \| --content TEXT --page NAME) --status DONE [--follow-refs] [--dry-run]` | Swap a task's marker without retyping the line. `--follow-refs` updates the original when the block is just a `((ref))` or `{{embed ((ref))}}`, through a chain of them; a chain that loops or reaches a missing block is refused. Ambiguous `--content` aborts and lists candidates. A block Logseq reads no marker in (`todo x`, `TODO:`, `TODO` followed directly by a line break, a block that only holds a ref) is refused, nothing written; `--content` matching blocks without a task is refused too, unless `--follow-refs` is given. Each refusal carries a `reason` under `--json`. `--dry-run` shows the old and new marker | +| `set-todo-status (--id UUID \| --content TEXT --page NAME) --status DONE [--follow-refs] [--expect-hash H] [--expect-marker M] [--dry-run]` | Swap a task's marker without retyping the line. `--follow-refs` updates the original when the block is just a `((ref))` or `{{embed ((ref))}}`, through a chain of them; a chain that loops or reaches a missing block is refused. Ambiguous `--content` aborts and lists candidates. A block Logseq reads no marker in (`todo x`, `TODO:`, `TODO` followed directly by a line break, a block that only holds a ref) is refused, nothing written; `--content` matching blocks without a task is refused too, unless `--follow-refs` is given. Each refusal carries a `reason` under `--json`. `--expect-hash H` (the `hash` of `get-block --json`) refuses with `precondition_failed` unless the block still has that hash; it is checked on a read past the cache, before `--dry-run` and before the no-change answer, and with `--follow-refs` it is the block at the end of the refs that counts. The refusal names the block's marker and first line, never its current hash: read the block again and decide again. `--expect-marker M` does the same for the block's marker, as Logseq stores it: any marker of the task model (`TODO`, `DOING`, `WAITING`, `CANCELED`, ... in any case) or `none` for a block without one; an unknown marker is a usage error that lists the known ones. With both options, both must hold. The refusal carries `expected_hash` and `expected_marker` for the options given, and `actual_marker` (`null` for a block without a marker). A block gone by then is `block_not_found`. Nothing is written either way. `--dry-run` shows the old and new marker | | `replace-text --page NAME --find TEXT --replace TEXT` | Search & replace with regex and dry-run support. `--replace` is literal; with `--regex` it takes `\1` group references. A replacement that turns a line into an `id::` line is refused before any block is written | | `insert-block (--content TEXT \| --content-file FILE) (--page NAME \| --after UUID \| --before UUID \| --child-of UUID)` | Insert one block at a position: appended to a page, as a sibling after or before a block, or as a child. `--property K=V` sets properties on it, repeatable. Without indentation `--content` is one block, and a line Logseq would read as a block of its own is refused before anything is written: a `- ` or `# ` line after the first, or a code fence nothing closes (fine inside a closed code block). The same holds for every write of one block's text: a `--tree` node, `add-journal-block`, `update-block`, `create-page --content`. A quote followed by a blank line and a paragraph is written, with a `Note:` on stderr: Logseq ends the quote at the blank line | | `insert-block --tree "" [--quiet]` | `--quiet` prints only the confirmation line, not one uuid line per block | | `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] [--dry-run]` | Copy/move block with children to another page. 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) [--dry-run]` | Structural move: 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 | +| `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 | +| `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) | Command | Description | |---------|-------------| -| `get-todos [--page NAME] [--state open\|done\|cancelled] [--status MARKER] [--page-type journal\|page] [--tag TAG] [--match REGEX] [--from DATE] [--to DATE] [--due-from DATE] [--due-to DATE] [--refs-limit N] [--no-follow-refs]` | List tasks (page name shown inline in plain-text output). Without `--state` or `--status` it lists every open task, `WAIT`, `WAITING`, `IN-PROGRESS` and `STARTED` included. `--state` (repeatable: `open`, `done`, `cancelled`, default `open`) selects by state; `--status` (repeatable, case does not matter, any marker Logseq reads) selects single markers instead and wins when both are given, with a note on stderr. Tasks come ordered by marker, open work first and finished tasks last, then by page. `--from/--to` date a task by every journal it stands in, the page its block lives on and the ones it was carried into by `((block-ref))` alike; `references` names the latter, `--refs-limit` caps that list (0 lifts the cap) and `references_withheld` counts what was left out — with a range that includes occurrences outside it, so lifting the cap does not make the count zero. `--no-follow-refs` reports only where blocks live, and with `--from/--to` dates a task by the page its block lives on alone. `--page-type journal|page` (case does not matter) keeps the tasks whose block stands on a journal page, or on any other page; a task carried into a journal by a block ref is a `page` task, and a range still counts it through that journal. Journal tasks older than a date, by their own day: `get-todos --page-type journal --no-follow-refs --to DATE`. `--due-from/--due-to` filter by `SCHEDULED`/`DEADLINE` instead. For a repeating task the next occurrence is derived (the date in its text moves on only when the task is ticked off by its checkbox in Logseq) and the range is applied to it; it is reported as `next_due`, and a repeater whose interval cannot be read is left out, counted in `repeating_excluded` and named on stderr. `--match` filters by what the task says (regex, case-insensitive, properties excluded). `--match` runs over the text as stored: `((refs))` are not resolved, and `^`/`$` anchor the whole text unless the pattern starts with `(?m)`. `--json` gives `{"todos": [...], "count": N}`, plus `repeating_excluded` when a due range left out repeaters it could not place; each task has `marker`, `content` (the text without marker, properties, `SCHEDULED`/`DEADLINE` and `LOGBOOK`), `page`, `uuid`, and `journal_day` when the page its block lives on is a journal. `scheduled`, `deadline`, `next_due` (all dates YYYY-MM-DD), `repeating`, `references` and `references_withheld` appear where they apply; a key that does not apply is absent, not null | +| `get-todos [--page NAME] [--state open\|done\|cancelled] [--status MARKER] [--page-type journal\|page] [--tag TAG] [--match REGEX] [--from DATE] [--to DATE] [--due-from DATE] [--due-to DATE] [--refs-limit N] [--no-follow-refs]` | List tasks (page name shown inline in plain-text output). Without `--state` or `--status` it lists every open task, `WAIT`, `WAITING`, `IN-PROGRESS` and `STARTED` included. `--state` (repeatable: `open`, `done`, `cancelled`, default `open`) selects by state; `--status` (repeatable, case does not matter, any marker Logseq reads) selects single markers instead and wins when both are given, with a note on stderr. Tasks come ordered by marker, open work first and finished tasks last, then by page. `--from/--to` date a task by every journal it stands in, the page its block lives on and the ones it was carried into by `((block-ref))` alike; `references` names the latter, `--refs-limit` caps that list (0 lifts the cap) and `references_withheld` counts what was left out — with a range that includes occurrences outside it, so lifting the cap does not make the count zero. `--no-follow-refs` reports only where blocks live, and with `--from/--to` dates a task by the page its block lives on alone. `--page-type journal|page` (case does not matter) keeps the tasks whose block stands on a journal page, or on any other page; a task carried into a journal by a block ref is a `page` task, and a range still counts it through that journal. Journal tasks older than a date, by their own day: `get-todos --page-type journal --no-follow-refs --to DATE`. `--due-from/--due-to` filter by `SCHEDULED`/`DEADLINE` instead. For a repeating task the next occurrence is derived (the date in its text moves on only when the task is ticked off by its checkbox in Logseq) and the range is applied to it; it is reported as `next_due`, and a repeater whose interval cannot be read is left out, counted in `repeating_excluded` and named on stderr. `--match` filters by what the task says (regex, case-insensitive, properties excluded). `--match` runs over the text as stored: `((refs))` are not resolved, and `^`/`$` anchor the whole text unless the pattern starts with `(?m)`. `--json` gives `{"todos": [...], "count": N}`, plus `repeating_excluded` when a due range left out repeaters it could not place; each task has `marker`, `content` (the text without marker, properties, `SCHEDULED`/`DEADLINE` and `LOGBOOK`), `page`, `uuid`, `hash` (as in `get-block`, computed from the stored text, not from `content`), and `journal_day` when the page its block lives on is a journal. `scheduled`, `deadline`, `next_due` (all dates YYYY-MM-DD), `repeating`, `references` and `references_withheld` appear where they apply; a key that does not apply is absent, not null | | `get-properties --page NAME [--property KEY]` | Get page properties, keyed as Logseq stores them (`due-date`, not the API's `dueDate`), with the original text alongside the parsed value | -| `doctor` | Health-check: Python, packages, connectivity, token, API, graph kind, graph, config, and whether writes are off (`read_only`). Exit 0 = ready to read | +| `doctor` | Health-check: Python, packages, connectivity, token, API, graph kind, graph, config, whether writes are off (`read_only`) and whether a precondition is required (`require_preconditions`). Exit 0 = ready to read | | `init [--dry-run] [--force] [--output PATH]` | Write a config file suggested from your graph, with the counts each suggestion rests on. Writes to the config file in use (else the first search path), so a config in `~/.logseq-cli.toml` gets no second file; that file exists, so `--force` is needed, and it keeps `[safety]`. Allowed under `read_only` | ### Properties (3) @@ -151,8 +152,8 @@ A write that ends a section (`add-journal-block`, `add-journal-content`, | Command | Description | |---------|-------------| | `set-property --page NAME --key KEY --value VAL [--dry-run]` | Set/update a page property, in the page's property block (made before the first block when there is none), so Logseq and `query-pages-by-property` see it at once. `title` is refused: it would rename the page; use `rename-page`. So is `collapsed`, which Logseq reads as the block's folded state. `--dry-run` shows the value being overwritten, or that the key is new. The key is stored as Logseq reads it back (lower-case, `_` as `-`, noted on stderr); a key Logseq would drop — whitespace, `/`, `:` and similar — is refused before anything is read, and so are `id` and `custom-id`, which Logseq reads as the block's uuid | -| `remove-property (--page NAME \| --id UUID) --key KEY [--dry-run]` | Remove a page property; the property block goes with its last one, unless it is the page's only block. `--id` targets a single block instead of the page. `--dry-run` names the value that would go, or reports that the key is not set. The key is addressed as `set-property` stores it | -| `set-block-property --id UUID --key KEY --value VAL [--dry-run]` | Set/update a block property. Fails on an unknown UUID, with or without `--dry-run`; `--dry-run` also shows the old value. Keys follow the `set-property` rule | +| `remove-property (--page NAME \| --id UUID) --key KEY [--expect-hash H] [--dry-run]` | `--expect-hash H` goes with `--id` (usage error with `--page`) and refuses with `precondition_failed` unless the block is still the one that was read. Remove a page property; the property block goes with its last one, unless it is the page's only block. `--id` targets a single block instead of the page. `--dry-run` names the value that would go, or reports that the key is not set. The key is addressed as `set-property` stores it | +| `set-block-property --id UUID --key KEY --value VAL [--expect-hash H] [--dry-run]` | Set/update a block property. `--expect-hash H` refuses with `precondition_failed` unless the block is still the one that was read, before `--dry-run` and the write. Fails on an unknown UUID, with or without `--dry-run`; `--dry-run` also shows the old value. Keys follow the `set-property` rule | ### Page Management (2) @@ -167,6 +168,25 @@ A write that ends a section (`add-journal-block`, `add-journal-content`, |---------|-------------| | `query-pages-by-property --key KEY [--value VAL]` | Find pages by property value | +## Changing a block you read + +Read the block with `--json`, decide, and pass its `hash` back. The write is +refused, with nothing written, if the block is another by then: + +```bash +HASH=$(logseq-cli get-block --id 6650a1b2-0000-4000-8000-000000000001 --no-children --json | jq -r .hash) +logseq-cli update-block --id 6650a1b2-0000-4000-8000-000000000001 --content "TODO ship the parser, fixed" --expect-hash "$HASH" +``` + +On `precondition_failed`, read the block again and decide again; sending the same +change with the new hash would write the old decision over the change that was +made. `remove-block` takes `--expect-tree-hash` (the `tree_hash`, which also sees +the children), `set-todo-status` also `--expect-marker`. The hash sees the +block's content, not where it stands. A complete script is +[`examples/safe-update-block.sh`](../examples/safe-update-block.sh); why the +requirement is optional and what it does not catch is in +[safety.md](safety.md#requiring-a-precondition). + ## Previews and refusals Every command that writes takes `--dry-run`. What the previews show, and the @@ -343,5 +363,6 @@ See `examples/` directory: - `export-all-pages.sh` - Export all pages as individual JSON files - `export-page.sh` - Export a page as Logseq-compatible markdown - `morning-log.sh` - Add a timestamped log entry to today's journal +- `safe-update-block.sh` - Replace a block's text only if it is still the one you read (`--expect-hash`); stops, without repeating, when it changed - `top-pages-pipeline.sh` - Pipeline with jq for graph statistics - `weekly-todos.sh` - List all open TODOs, grouped by page diff --git a/docs/configuration.md b/docs/configuration.md index 5415651..0a20060 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -46,8 +46,9 @@ Copy one, delete what you do not need, keep what you do. | `LOGSEQ_CLI_CONFIG` | (none) | Path to a config file, overriding the default locations | | `LOGSEQ_CLI_KEEP_EMPTY_BLOCKS_LAST` | (none) | `1`, `true`, `yes` or `on` turns on, `0`, `false`, `no` or `off` turns off, over `[graph] keep_empty_blocks_last` (see [`[graph] keep_empty_blocks_last`](#graph-keep_empty_blocks_last)); empty is not set, any other value counts as off with a warning | | `LOGSEQ_CLI_READ_ONLY` | (none) | `1`, `true`, `yes` or `on` switches every command that writes off; `0`, `false`, `no`, `off` and empty do nothing; any other value switches it on with a warning. Only tightens (see [`[safety] read_only`](#safety-read_only)) | +| `LOGSEQ_CLI_REQUIRE_PRECONDITIONS` | (none) | Same values and same rule as `LOGSEQ_CLI_READ_ONLY`, for [`[safety] require_preconditions`](#safety-require_preconditions) | -All connection settings can also be passed as CLI flags: `--host`, `--port`, `--token`. `--read-only` is a flag too, not a connection setting: see [`[safety] read_only`](#safety-read_only). +All connection settings can also be passed as CLI flags: `--host`, `--port`, `--token`. `--read-only` and `--require-preconditions` are flags too, not connection settings: see [`[safety] read_only`](#safety-read_only) and [`[safety] require_preconditions`](#safety-require_preconditions). ### Journal heading @@ -117,10 +118,11 @@ none of them set, entries go to the top level of the page. `LOGSEQ_JOURNAL_HEADING` predates the config file and stays authoritative, so existing shell profiles keep working unchanged after you add a config file. -One rule breaks the order: [`[safety] read_only`](#safety-read_only) only -tightens. A config that says `true` is not switched off by an environment -value of `false` or by leaving out `--read-only`; the flag or the environment -can switch it on when the config says nothing. +One rule breaks the order: the `[safety]` switches ([`read_only`](#safety-read_only), +[`require_preconditions`](#safety-require_preconditions)) only tighten. A config +that says `true` is not switched off by an environment value of `false` or by +leaving out the flag; the flag or the environment can switch it on when the +config says nothing. ## The options @@ -339,6 +341,25 @@ keeps the `[safety]` section; it refuses a file it cannot parse. How binding each way of setting it is, and what it does not stop, is in [safety.md](safety.md). +### `[safety] require_preconditions` + +```toml +[safety] +require_preconditions = true +``` + +| | | +|---|---| +| Values | `true` or `false` (a TOML boolean); default `false` | +| Effect | a command that changes a block it read refuses, before its first request, unless the call says what it read: `--expect-hash` (also `--expect-marker` for `set-todo-status`), `--expect-tree-hash` for `remove-block`. Refused with `reason: precondition_required`, and the message names where the switch was set. The rule is the call's, not the command's: a command that changes nothing in a given form stays free in it. Only the commands that take such an option ask for it. `add-journal-block --upsert-heading` asks only when a block matches; creating one stays free | +| Also set by | `LOGSEQ_CLI_REQUIRE_PRECONDITIONS` and `--require-preconditions`; all three only tighten, like `read_only` | +| Order | `read_only` comes first: with both on, a call without a precondition is refused as `read_only` | +| Shown by | `logseq-cli doctor`, every run, in the same forms as `read_only` | + +A config that cannot be read, a `LOGSEQ_CLI_CONFIG` that names a file that is +not there and a value that is not a boolean refuse the commands that ask for a +precondition, as they do for `read_only`: the switch cannot be told to be off. + ## When a section does not exist in your graph Graphs differ. The answer is almost always "leave the setting out", and the @@ -519,7 +540,7 @@ logseq-cli --token "TOKEN" doctor listening on the port, is a token supplied, does the API answer, is a graph loaded. Each step is reported separately, so a failure names which one broke. Exit 0 means ready to read. It also says whether writes are switched off -(`read_only`) and by what; that does not fail it, and the closing line says +(`read_only`) and whether a precondition is required (`require_preconditions`), and by what; that does not fail it, and the closing line says "Ready to read; writes are off." when they are. A config file that does not parse fails it, and the advice is to fix the file, never to remove it: removing a file that holds a limit lifts the limit. diff --git a/docs/design.md b/docs/design.md index e30400f..460493e 100644 --- a/docs/design.md +++ b/docs/design.md @@ -138,6 +138,38 @@ they cannot use. What it does not do is stated in [safety.md](safety.md): an agent with the token or write access to the files is not stopped by a refusal in this CLI. +## A caller can say what it read, and the check is a gate of its own + +`block_changed` already catches a block that changed between a command's read +and its write. It cannot catch a block that changed between the caller's read +and the call: the command reads the block fresh and finds it as it is now, so +the write of a decision made on older text goes through. Only the caller knows +what it read, so it says so: `--expect-hash` (`--expect-tree-hash` for +`remove-block`, `--expect-marker` for a task's marker), and the command refuses +with `precondition_failed` unless the block is still that. The two are not one +check: one compares the command with itself, the other compares it with the +caller. + +The hash is of the block's content, with what Logseq rewrites on its own +taken out (collapsing, `id::` lines, the clock lines of the logbook), because +a hash that moved when a block was folded would refuse writes nobody changed. +`DONE` lines of the logbook stay in, so a completion does move it. It sees no +position: a moved block has the hash it had. A parent's hash does not see a new +child either, so deleting a block, which deletes what is under it, takes a +separate hash over the whole subtree. + +The check sits in one module (`preconditions.py`) and runs on a read past the +cache, before `--dry-run` and before anything is written. Whether a call owes a +precondition is a mark on the command like the one for `read_only`, asked once +in the same place before the first request, so a command that gains the option +cannot forget to be asked, and a form of a command that changes no block it +read (`copy-block` without `--remove`) owes none. `require_preconditions` +turns the obligation on; it is off by default because the write outputs carry +no new hash yet, see [safety.md](safety.md#why-it-is-off-by-default-and-when-to-turn-it-on). +The refusal never prints the block's current hash: the next step is to read the +block and decide again, not to send the same change with a hash that now +matches. + ## Query values are escaped in one place Values entering datalog queries were interpolated with f-strings: one call site diff --git a/docs/safety.md b/docs/safety.md index 3c726a8..4568e9f 100644 --- a/docs/safety.md +++ b/docs/safety.md @@ -80,8 +80,98 @@ What the switch covers: method leaves the process, so a write reaching the network by another path is stopped too. +## Requiring a precondition + +`read_only` is a lock: nothing writes. `require_preconditions` is an +obligation: a command that changes a block it read must say what it read, and +refuses when it does not. + +```toml +[safety] +require_preconditions = true +``` + +```text +$ logseq-cli set-todo-status --id 6650a1b2-0000-4000-8000-000000000001 --status DONE +Error: A precondition is required: require_preconditions = true in [safety] of /home/you/.config/logseq-cli/config.toml. Pass --expect-hash or --expect-marker. Nothing was written. +``` + +Under `--json` the object has `reason: precondition_required` and the same +`source` and `config_path` as `read_only`, plus `options`, the ones that would +do. The refusal comes before the first request, `--dry-run` included, and +`read_only` is checked before it: with both on, a call without a precondition +is refused as `read_only`. Set it the same three ways as `read_only` (config, +`LOGSEQ_CLI_REQUIRE_PRECONDITIONS`, `--require-preconditions`); all only +tighten, and a config that cannot be read refuses the commands that ask for a +precondition. `doctor` shows it on every run. + +The obligation belongs to the call, not to the command: a form of a command +that changes no block that was read is not asked for one. Today +`set-todo-status` takes `--expect-hash` and `--expect-marker`, `update-block` `--expect-hash`, `remove-block` (and its alias `delete-block`) `--expect-tree-hash`; +`set-block-property`, `remove-property --id`, `move-block` and +`copy-block --remove` take `--expect-hash`. `add-journal-block --upsert-heading` owes `--expect-hash` once it has picked the block it replaces, and only when there is one: with no match it creates the block and owes nothing. `remove-property --page` and +`copy-block` without `--remove` change no block that was read: they owe +nothing, and the option is a usage error there. + +### Why it is off by default, and when to turn it on + +A default of "on" would change what every script and person calling +`update-block`, `remove-block` and the other six commands does today: calls that +pass no hash would start to refuse with `precondition_required`. That is reason +enough, but not the whole one. The stronger reason is friction. The output of a +write does not carry the block's new hash yet, so a second write to the same +block with the hash from the first read is refused (the block did change, by +your own first write). With the switch on, the only way through is to read the +block again before every write. That ritual protects no more than the re-read +each command already does between its own read and its write (`block_changed`), +so a mandatory switch costs more than it gives until the write outputs return a +hash. It is off for that reason, and it stays a choice. + +Turn it on when an agent writes and you also edit the graph yourself while it +works: then every write has to show the state it was decided on. Without an +agent, or without edits in parallel, it adds nothing. Without the switch the +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? +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 +the fresh hash writes the old decision over the change that was made. +[`examples/safe-update-block.sh`](../examples/safe-update-block.sh) does it that +way. + +### What it does not do + +- **It is no compare-and-swap.** Reading and writing stay two requests to + Logseq, and a change in the moment between the check and the write is not + 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. +- **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 + everything under it (`--expect-tree-hash`), because the hash of the block + alone would not see a child added since. +- **Write outputs carry no new hash.** Whoever changes the same block twice + reads it again in between. +- **It guards against a mistake, not against an agent that sets out to avoid + it.** An agent can read the block just before writing and pass what it finds, + which proves nothing about what it decided on. The switch makes the honest + path the required one; it does not make the other impossible. + ## Three ways to set it, and how binding each is +The forms below are written for `read_only`. `require_preconditions` has the +same three, with `LOGSEQ_CLI_REQUIRE_PRECONDITIONS` and `--require-preconditions`, +and it only tightens in the same way. + | Way | Set by | Holds as long as | |-----|--------|------------------| | `[safety] read_only = true` in the config file | you, in a file | the agent cannot change the file | @@ -151,6 +241,10 @@ file is not writable by the process the agent runs in. ## When it seems not to apply +The rows are written for `read_only`. `require_preconditions` follows the same +rules: `doctor` shows it on a line of its own, and a wrong value, a misplaced +key or an unreadable file gives the same errors. + | You see | It means | |---------|----------| | `doctor`: `read_only: off (no config file found)` | No config file at any of the places it is looked for, for this user and environment | diff --git a/examples/safe-update-block.sh b/examples/safe-update-block.sh new file mode 100755 index 0000000..e51a8b3 --- /dev/null +++ b/examples/safe-update-block.sh @@ -0,0 +1,45 @@ +#!/usr/bin/env bash +# Replace one block's text only if the block is still the one you read +# +# Reads the block with --json, takes its `hash`, and passes it back as +# --expect-hash. If the block changed in between (you typed in it, another +# tool wrote it), the write is refused with `precondition_failed` and nothing +# is written. The hash sees the block's content, not where it stands: a block +# moved since the read still passes. +# +# On a refusal this script stops. It never reads the block again and repeats +# the same text: the new text was decided on the old content, and a fresh hash +# would write that decision over the change that was made. Read the block +# again (get-block --json), decide again, then run the script again. +# +# Usage: ./safe-update-block.sh BLOCK_UUID "new text" +# ./safe-update-block.sh 6650a1b2-0000-4000-8000-000000000001 "TODO ship the parser, fixed" +# Requires: LOGSEQ_TOKEN or --token, and jq +# Exit: 0 written; 3 the block changed since it was read (nothing written); +# anything else: the call failed for another reason (see stderr) + +set -euo pipefail + +ID="${1:?Usage: $0 BLOCK_UUID \"new text\"}" +NEW="${2:?Usage: $0 BLOCK_UUID \"new text\"}" + +ERR=$(mktemp) +trap 'rm -f "${ERR}"' EXIT + +# --no-children: the hash of the block alone is what update-block checks. +HASH=$(logseq-cli get-block --id "${ID}" --no-children --json | jq -er '.hash') + +if logseq-cli update-block --id "${ID}" --content "${NEW}" \ + --expect-hash "${HASH}" --json 2>"${ERR}"; then + exit 0 +fi + +if [[ "$(jq -r '.reason // empty' "${ERR}" 2>/dev/null)" == "precondition_failed" ]]; then + echo "The block changed after it was read; nothing was written." >&2 + echo "Read it again with: logseq-cli get-block --id ${ID} --json" >&2 + echo "and decide again on what it says now. Do not repeat this change with a fresh hash." >&2 + exit 3 +fi + +cat "${ERR}" >&2 +exit 1 diff --git a/logseq_cli/api.py b/logseq_cli/api.py index 9091031..2ce5d09 100644 --- a/logseq_cli/api.py +++ b/logseq_cli/api.py @@ -1237,7 +1237,7 @@ def move_block(self, src_uuid: str, target_uuid: str, options: dict = None): ) def update_block(self, block_uuid: str, content: str, properties: dict = None, *, - replacing: str = None): + replacing: str = None, expect: str = None): """Replace a block's content, optionally carrying its properties along. Block properties live *inside* the content (``prio:: 1`` as a line of @@ -1267,17 +1267,24 @@ def update_block(self, block_uuid: str, content: str, properties: dict = None, * window to the requests between this read and the write; it does not close it, as the editor gate's does not (see ``_write``). + ``expect`` is the same comparison without the allowance: the text the + block must still hold, for a caller that writes text of its own (a line + in it is wanted, whether or not the block had it) and has checked a + precondition against that read. ``replacing``, when given, is the text + compared and ``expect`` is ignored. + Raises BlockChanged as above, and WriteNotVerified unless the block then reads back with ``content`` and ``properties`` (``_prove_text``). """ refuse_split_block(content, command="logseq-cli", where="The text", replacing=replacing) refuse_id_lines(content, own=block_uuid, replacing=replacing) - if replacing is not None: + held = replacing if replacing is not None else expect + if held is not None: current = self.get_block(block_uuid, include_children=False, cached=False) # An Id Line is Logseq's, not text anyone wrote: a ref written # earlier in the same call stores the id of a block this call # then replaces (#95), and that is not a change made elsewhere. - if without_block_ids((current or {}).get("content", "")) != without_block_ids(replacing): + if without_block_ids((current or {}).get("content", "")) != without_block_ids(held): raise BlockChanged( f"Block {block_uuid[:8]}.. changed since it was read; nothing was " "written over it. Read it again and repeat the command.", diff --git a/logseq_cli/blocktext.py b/logseq_cli/blocktext.py index c0cec1b..161b65d 100644 --- a/logseq_cli/blocktext.py +++ b/logseq_cli/blocktext.py @@ -3,6 +3,7 @@ Kept in a module of its own so that LogseqAPI, which checks every write, depends on this rule and nothing else. Measured against Logseq 0.10.15 throughout. """ +import hashlib import re from collections import Counter from typing import NamedTuple @@ -190,6 +191,13 @@ def drawer_lines(lines: list) -> set: return drawer +# The line Logseq writes into a drawer when a task is completed: +# ``* State "DONE" from "" [yyyy-MM-dd E HH:mm]`` (star, not a dash). +# Recognised by its start, after the indent. One pattern for everything that +# reads those lines; the task rules import it from here, never the reverse. +DONE_LINE_RE = re.compile(r'^[ \t]*\* State "DONE"') + + def attached_line_mask(lines: list) -> list: """For each line of a block's content, whether it belongs to the block above it rather than to its text: a SCHEDULED/DEADLINE line, or a line of @@ -206,7 +214,7 @@ def attached_line_mask(lines: list) -> list: for i, line in enumerate(lines)] -def normalize_block_text(text: str) -> str: +def normalize_block_text(text: str, *, keep_logbook: bool = False) -> str: """``text`` as a write's proof compares it: what Logseq rewrites on its own is taken out, on the sent and the read side alike. @@ -228,15 +236,90 @@ def normalize_block_text(text: str) -> str: fail a write that landed. Whether a drawer arrived at all is block_text_matches' check. + ``keep_logbook=True`` leaves the drawer lines in place, for a caller that + picks among them itself; every other step runs as before. The proof never + sets it. + Nothing else: a rule as loose as "whitespace does not matter" would let a write through that did not land. """ lines = without_block_ids(text).split("\n") - drawer = drawer_lines(lines) + drawer = set() if keep_logbook else drawer_lines(lines) kept = [line.rstrip() for i, line in enumerate(lines) if i not in drawer] return "\n".join(kept).strip() +_COLLAPSED_RE = re.compile(r'^[ \t]*collapsed:: ', re.IGNORECASE) + + +def strip_volatile(text: str) -> str: + """``text`` without the lines that change on their own, for a hash. + + - ``collapsed::``: folding a block in the UI leaves ``content`` alone, but + a block created with the line carries it and one rewritten by + update-block loses it, so whether it is there depends on how the block + came about, not on what it says. In a code block it is code and stays. + - Every line of a closed ``:LOGBOOK:`` drawer except the ``State "DONE"`` + lines: ``CLOCK`` lines only change together with a marker change, which + the hash sees; a completion is a decision, and a replayed one has to + show. A drawer of ``CLOCK`` lines alone hashes like no drawer. + + An unclosed drawer is text (drawer_lines) and stays. + """ + lines = text.split("\n") + in_code, _ = code_block_lines(lines) + drawer = drawer_lines(lines) + mask = property_line_mask(lines) + kept = [line for i, line in enumerate(lines) + if not (i in drawer and not DONE_LINE_RE.match(line)) + and not (mask[i] and not in_code[i] and _COLLAPSED_RE.match(line))] + return "\n".join(kept).strip() + + +def block_hash(content: str) -> str: + """The hash a reader gets for one block, from its raw ``content``. + + 12 hex characters of SHA-256 over the normalised text (normalize_block_text + with the drawer kept, then strip_volatile): change detection, not a + security property. What the write proof ignores, the hash ignores too; + what it also ignores is above. + """ + text = strip_volatile(normalize_block_text(content or "", keep_logbook=True)) + return hashlib.sha256(text.encode("utf-8")).hexdigest()[:12] + + +def tree_hash(block: dict) -> str: + """The hash of a block and all its descendants: content and order. + + Each block contributes its block_hash and, in brackets, its children's + tree hashes in order, so a new child, a moved child and a child one level + deeper all change it. ``block`` is a block as Logseq answers it with its + children as blocks; a child that is only a ``["uuid", ...]`` pair has no + content to hash and is not counted. + """ + kids = ",".join(tree_hash(c) for c in block.get("children") or [] if isinstance(c, dict)) + return hashlib.sha256(f"{block_hash(block.get('content'))}[{kids}]".encode("utf-8")).hexdigest()[:12] + + +def with_hashes(block: dict) -> dict: + """A copy of ``block`` with ``hash`` on it and on each descendant block. + + A copy, because the API client caches what it returns. Computed from the + raw ``content``, so call it before anything rewrites the content. + """ + out = dict(block, hash=block_hash(block.get("content"))) + if "children" in block: + out["children"] = [with_hashes(c) if isinstance(c, dict) else c for c in block["children"]] + return out + + +def with_hashes_in(blocks): + """``with_hashes`` over a list of blocks; anything else comes back as it is.""" + if not isinstance(blocks, list): + return blocks + return [with_hashes(b) if isinstance(b, dict) else b for b in blocks] + + def block_text_matches(sent: str, read: str) -> bool: """Whether ``read`` shows the block text that ``sent`` wrote. diff --git a/logseq_cli/commands/blocks.py b/logseq_cli/commands/blocks.py index e91046a..2a489e3 100644 --- a/logseq_cli/commands/blocks.py +++ b/logseq_cli/commands/blocks.py @@ -2,6 +2,7 @@ import click +from logseq_cli.blocktext import tree_hash, with_hashes, with_hashes_in from logseq_cli.group import cli from logseq_cli.lookup import find_blocks_by_content from logseq_cli.notes import print_note @@ -39,6 +40,12 @@ def get_block(ctx, block_id, no_children, as_json): id=block_id, exists=False) if as_json: + # Hashes come from the raw content. Without children Logseq answers + # uuid pairs, from which no tree can be hashed: the block's own hash + # stays, tree_hash is left out. + block = with_hashes(block) + if include_children: + block["tree_hash"] = tree_hash(block) output(block, True) else: # Logseq answers page and parent as database ids no command takes: @@ -184,7 +191,7 @@ def find_block(ctx, content, page, use_regex, first_only, exactly_one, limit, wi return if as_json: - output(matches, True) + output(with_hashes_in(matches), True) else: if not matches: click.echo("No blocks found.") diff --git a/logseq_cli/commands/edit.py b/logseq_cli/commands/edit.py index d2a9427..12f9cd0 100644 --- a/logseq_cli/commands/edit.py +++ b/logseq_cli/commands/edit.py @@ -50,7 +50,9 @@ uuid_fields, would_go_before_fields, ) -from logseq_cli.safety import WriteCommand +from logseq_cli.preconditions import ( + EXPECT_HASH_HELP, Expect, check_precondition, read_checked, read_fresh) +from logseq_cli.safety import Owed, WriteCommand from logseq_cli.pagenames import PageToWrite, journal_page_name from logseq_cli.strictinsert import ( BEFORE_EMPTY_SUFFIX, @@ -71,7 +73,7 @@ from logseq_cli.writerefused import WriteRefused, partial_state -@cli.command("update-block", cls=WriteCommand, epilog="""\b +@cli.command("update-block", cls=WriteCommand, owes=lambda params: Owed(("--expect-hash",)), epilog="""\b Example: logseq-cli --token TOKEN update-block --id 12345678-... --content "New text" logseq-cli --token TOKEN update-block --where-content "**14:22**" --page "2026-08-21, friday" --content "New text" @@ -96,6 +98,8 @@ The block's own id:: line (as get-block shows it) may stay in --content; one naming another uuid is dropped with a Note, since Logseq would make it this block's uuid and every ((ref)) to the block would dangle. + --expect-hash H (the hash field of get-block --json) refuses, without + writing, unless the block is still the one H was read from. """) @click.option("--id", "block_id", default=None, help="UUID of the block to update") @click.option("--where-content", "where_content", default=None, help="Select the block by content instead of --id; must match exactly one") @@ -103,11 +107,16 @@ @click.option("--regex", "use_regex", is_flag=True, help="With --where-content: interpret it as a regex") @click.option("--content", default=None, help="New content for the block; this or --content-file is required") @click.option("--content-file", "content_file", default=None, help="Read --content from this file instead ('-' reads stdin), so apostrophes, quotes and umlauts need no shell quoting. Mutually exclusive with --content") +@click.option("--expect-hash", "expect_hash", default=None, + help="Refuse, without writing, unless the block is still the one this hash was read " + "from (the hash field of get-block --json). Checked on a read past the cache, " + "before --dry-run and before the write") @click.option("--dry-run", is_flag=True, help="Show the block that would be overwritten, without writing") @click.option("--json", "as_json", is_flag=True, help="JSON output") @click.pass_context @handle_connection_error -def update_block(ctx, block_id, where_content, page, use_regex, content, content_file, dry_run, as_json): +def update_block(ctx, block_id, where_content, page, use_regex, content, content_file, expect_hash, + dry_run, as_json): """Update the content of an existing block.""" content = content_or_file(content, content_file) # This command has no tree path: it replaces ONE block's content, so a @@ -134,6 +143,14 @@ def update_block(ctx, block_id, where_content, page, use_regex, content, content if not block: fail(f"Block not found: {clean_id}", as_json=as_json, id=clean_id) + # The precondition is checked on a read past the cache, ahead of anything + # that writes, so a refusal also keeps a ref in the new text from giving + # its target an id. The write then holds the block to that same text. + expect = Expect(hash=expect_hash) + if expect: + block = read_fresh(api, clean_id) + check_precondition(block, expect) + old_content = block.get("content", "") if isinstance(block, dict) else "" # The block's own id:: line, as getBlock hands it out, goes back as it # came; one naming another uuid is dropped, as every writer drops it (#56). @@ -168,7 +185,9 @@ def update_block(ctx, block_id, where_content, page, use_regex, content, content click.echo(f" keeps: {', '.join(f'{k}::' for k in kept_texts)}") return - api.update_block(clean_id, content, properties=kept_texts or None) + # Held to the text the precondition was checked on; without one, as before. + held = {"expect": old_content} if expect else {} + api.update_block(clean_id, content, properties=kept_texts or None, **held) if as_json: output({"id": clean_id, "old_content": old_content, "new_content": content, @@ -181,7 +200,8 @@ def update_block(ctx, block_id, where_content, page, use_regex, content, content preview = content[:60] + ("..." if len(content) > 60 else "") click.echo(f" now: {preview}") -@cli.command("remove-block", cls=WriteCommand, epilog="""\b +@cli.command("remove-block", cls=WriteCommand, + owes=lambda params: Owed(("--expect-tree-hash",)), epilog="""\b Examples: logseq-cli --token TOKEN remove-block --id 12345678-... --dry-run logseq-cli --token TOKEN remove-block --id 12345678-... @@ -192,18 +212,25 @@ def update_block(ctx, block_id, where_content, page, use_regex, content, content """) @click.option("--id", "block_id", required=True, help="UUID of the block to remove") @click.option("--ignore-refs", is_flag=True, help="Remove even though ((block-refs)) from elsewhere point into it") +@click.option("--expect-tree-hash", "expect_tree_hash", default=None, + help="Refuse, without removing, unless the block and everything under it is still what " + "this hash was read from (the tree_hash field of get-block --json). A child added " + "since changes it. Checked before --dry-run.") @click.option("--dry-run", is_flag=True, help="Show the block and its descendant count, without deleting") @click.option("--json", "as_json", is_flag=True, help="JSON output") @click.pass_context @handle_connection_error -def remove_block_cmd(ctx, block_id, ignore_refs, dry_run, as_json): +def remove_block_cmd(ctx, block_id, ignore_refs, expect_tree_hash, dry_run, as_json): """Remove a block by UUID.""" api = ctx.obj["api"] clean_id = unwrap_block_id(block_id) # Fetch WITH children: removal cascades, so the descendant count is the # decisive fact for --dry-run (and for the confirmation the caller may want). - block = api.get_block(clean_id, include_children=True) + # With a precondition the read is past the cache, children included: the + # hash covers them, and what is counted and removed below is this read. + block = read_checked(api, clean_id, Expect(tree_hash=expect_tree_hash), + include_children=True) if not block: fail(f"Block not found: {clean_id}", as_json=as_json, id=clean_id) @@ -886,7 +913,9 @@ def add_block_ref(ctx, source_id, journal_date, page, under_heading, dry_run, as click.echo(f" {ref_content}") click.echo(f" uuid: {new_uuid}") -@cli.command("copy-block", cls=WriteCommand, epilog="""\b +@cli.command("copy-block", cls=WriteCommand, + owes=lambda params: Owed(("--expect-hash",)) if params.get("remove") else None, + epilog="""\b Examples: logseq-cli --token TOKEN copy-block --id UUID --to-page "Target Page" logseq-cli --token TOKEN copy-block --id UUID --to-page "Target Page" --remove @@ -901,18 +930,22 @@ def add_block_ref(ctx, source_id, journal_date, page, under_heading, dry_run, as [graph] keep_empty_blocks_last (or LOGSEQ_CLI_KEEP_EMPTY_BLOCKS_LAST): a write that ends 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. """) @click.option("--id", "block_id", required=True, help="Source block UUID") @click.option("--to-page", required=True, help="Target page name") @click.option("--remove", is_flag=True, help="Remove source block after copying (move)") @click.option("--ignore-refs", is_flag=True, help="With --remove: remove even though ((block-refs)) point into the source") +@click.option("--expect-hash", "expect_hash", default=None, help="With --remove: " + EXPECT_HASH_HELP) @click.option("--dry-run", is_flag=True, help="Show what would be copied/moved, without writing") @click.option("--json", "as_json", is_flag=True, help="Output as JSON") @click.pass_context @handle_connection_error -def copy_block(ctx, block_id, to_page, remove, ignore_refs, dry_run, as_json): +def copy_block(ctx, block_id, to_page, remove, ignore_refs, expect_hash, dry_run, as_json): """Copy a block (with children) to another page.""" api = ctx.obj["api"] + if expect_hash is not None and not remove: + raise click.UsageError("--expect-hash goes with --remove: a plain copy changes no block.") keep_on = keep_empty_blocks_last(load_config()) # A missing page under the name Logseq creates it with, as for # insert-block --page. @@ -920,7 +953,9 @@ def copy_block(ctx, block_id, to_page, remove, ignore_refs, dry_run, as_json): names = ref.fields("to_page") to_page = target.name block_id = block_id.strip("()") - source = api.get_block(block_id, include_children=True) + # With --expect-hash the copy and the removal both come from a read past + # the cache, so what is checked is what is copied. + source = read_checked(api, block_id, Expect(hash=expect_hash), include_children=True) if not source: fail("Block not found.", as_json=as_json, id=block_id) @@ -1009,7 +1044,8 @@ def _copy_tree(block, parent_uuid=None): if refs: click.echo(f" {len(refs)} block ref(s) into the source now point at nothing") -@cli.command("move-block", cls=WriteCommand, epilog="""\b +@cli.command("move-block", cls=WriteCommand, owes=lambda params: Owed(("--expect-hash",)), + epilog="""\b Examples: logseq-cli --token TOKEN move-block --id UUID --under UUID logseq-cli --token TOKEN move-block --id UUID --before UUID @@ -1027,11 +1063,12 @@ def _copy_tree(block, parent_uuid=None): @click.option("--id", "block_id", required=True, help="UUID of the block to move") @click.option("--under", default=None, help="UUID of the new parent (block becomes its first child)") @click.option("--before", default=None, help="UUID of the block to move in front of (as sibling)") +@click.option("--expect-hash", "expect_hash", default=None, help=EXPECT_HASH_HELP) @click.option("--dry-run", is_flag=True, help="Show what would be moved, without writing") @click.option("--json", "as_json", is_flag=True, help="JSON output") @click.pass_context @handle_connection_error -def move_block_cmd(ctx, block_id, under, before, dry_run, as_json): +def move_block_cmd(ctx, block_id, under, before, expect_hash, dry_run, as_json): """Move a block (with children) under or before another block.""" api = ctx.obj["api"] if bool(under) == bool(before): @@ -1039,7 +1076,7 @@ def move_block_cmd(ctx, block_id, under, before, dry_run, as_json): target = under or before block_id = block_id.strip("()") - source = api.get_block(block_id, include_children=True) + source = read_checked(api, block_id, Expect(hash=expect_hash), include_children=True) if not source: fail("Block not found.", as_json=as_json, id=block_id) diff --git a/logseq_cli/commands/journal.py b/logseq_cli/commands/journal.py index 3b61a64..c4225d1 100644 --- a/logseq_cli/commands/journal.py +++ b/logseq_cli/commands/journal.py @@ -7,7 +7,7 @@ import click from logseq_cli.blockprops import kept_properties -from logseq_cli.blocktext import refuse_split_heading, refuse_split_tree, without_block_ids +from logseq_cli.blocktext import refuse_split_heading, refuse_split_tree, with_hashes_in, without_block_ids from logseq_cli.cliinput import content_or_file, read_content_file, require_content from logseq_cli.config import keep_empty_blocks_last, load_config, resolve_heading from logseq_cli.dates import ( @@ -19,9 +19,8 @@ from logseq_cli.group import cli from logseq_cli.headings import ( first_empty_under, - find_heading, + find_upsert_target, find_or_create_heading, - normalize_heading, strip_title_heading, ) from logseq_cli.ids import ( @@ -53,7 +52,8 @@ uuid_fields, would_go_before_fields, ) -from logseq_cli.safety import WriteCommand +from logseq_cli.preconditions import Expect, check_precondition, read_fresh +from logseq_cli.safety import Owed, WriteCommand, require_precondition from logseq_cli.pagenames import journal_page_name from logseq_cli.render import ( blocks_to_markdown, @@ -263,6 +263,11 @@ def _fetch_one(target): blocks = api.get_page_blocks_tree(page_name) if heading and blocks: blocks = extract_section(blocks, heading) + if as_json: + # From the content as stored: before --resolve-refs rewrites + # the text. The cut hands back the stored blocks, so only the + # section is hashed. + blocks = with_hashes_in(blocks) if resolve_refs and blocks: resolve_refs_in_blocks(api, blocks) return { @@ -446,7 +451,10 @@ def add_journal_entry(ctx, content, date, as_block, as_json, dry_run): else: click.echo(f"Added {blocks_added} block(s) to journal: {page_name}") -@cli.command("add-journal-block", cls=WriteCommand, epilog="""\b +# The precondition belongs to the block --upsert-heading replaces, which is +# unknown until the command has chosen it: so nothing is owed up front, and the +# body asks for it after the choice (see find_upsert_target). +@cli.command("add-journal-block", cls=WriteCommand, owes=lambda params: None, epilog="""\b Examples: logseq-cli --token TOKEN add-journal-block --content "**$(date +%H:%M)** Meeting with [[Bob]]" logseq-cli --token TOKEN add-journal-block --date 2026-05-07 --content "**14:30** Nachtrag" @@ -461,6 +469,9 @@ def add_journal_entry(ctx, content, date, as_block, as_json, dry_run): The replaced block keeps its properties, as with update-block; a property line in the new text is the new value of its key. A first block that is nothing but id:: lines is refused: it would empty the replaced block. + --expect-hash H (with --upsert-heading) refuses, without writing, unless the + block it would replace is still the one H was read from. With no match there + is no block: the hash has no object and the block is created. Auto-detects tab-indented hierarchy in --content; no need to switch to add-journal-content. --content-file reads the whole file as ONE tree: flush "- " lines become sibling roots, tab-indented lines their children. No shell quoting, so @@ -488,6 +499,8 @@ def add_journal_entry(ctx, content, date, as_block, as_json, dry_run): @click.option("--date", default=None, help="Date (YYYY-MM-DD), defaults to today") @click.option("--under-heading", default=None, help="Insert as child of this heading (e.g. '## Log'). Creates heading if missing. Default from LOGSEQ_JOURNAL_HEADING env var, or top-level if unset.") @click.option("--upsert-heading", default=None, help="Find child block matching this heading under --under-heading and update it; insert as new block if not found.") +@click.option("--expect-hash", "expect_hash", default=None, + help="With --upsert-heading: refuse, without writing, unless the block it replaces is still the one this hash was read from (the hash field of get-block --json). Checked on a read past the cache, before --dry-run and before the journal page is created. With no matching block there is nothing to check and the block is created") @click.option("--top-level", is_flag=True, help="Add as top-level block (ignore --under-heading and env var)") @click.option("--preserve-formatting/--no-preserve", default=True, help="Preserve content formatting") @click.option("--dry-run", is_flag=True, help="Show what would be written without making changes") @@ -495,7 +508,7 @@ def add_journal_entry(ctx, content, date, as_block, as_json, dry_run): @click.option("--json", "as_json", is_flag=True, help="Output as JSON") @click.pass_context @handle_connection_error -def add_journal_block(ctx, contents, content_file, date, under_heading, upsert_heading, top_level, preserve_formatting, dry_run, keep_ids, as_json): +def add_journal_block(ctx, contents, content_file, date, under_heading, upsert_heading, expect_hash, top_level, preserve_formatting, dry_run, keep_ids, as_json): """Add one or more blocks to a journal page. Pass --content multiple times for batch inserts under the same heading. @@ -539,6 +552,12 @@ def add_journal_block(ctx, contents, content_file, date, under_heading, upsert_h # --upsert-heading rewrites a block that already exists, and an existing # block's uuid cannot change. The flag would half work there, so the # combination is refused rather than honoured for some blocks only. + if expect_hash is not None and not upsert_heading: + raise click.UsageError("--expect-hash goes with --upsert-heading: it is the hash of " + "the block that option replaces.") + if expect_hash is not None and len(contents) > 1: + raise click.UsageError("--expect-hash takes one --content: --upsert-heading replaces " + "one block.") if keep_ids and upsert_heading: raise click.UsageError("--keep-ids cannot be combined with --upsert-heading: " "the block it replaces keeps its own uuid.") @@ -729,6 +748,28 @@ def add_journal_block(ctx, contents, content_file, date, under_heading, upsert_h tree = trees[0] + # Which block --upsert-heading replaces is chosen first: before the journal + # page and the heading are created, from reads that skip the cache, and the + # same way for the preview as for the run. + heading_uuid, found_uuid = (find_upsert_target(api, page_name, under_heading, upsert_heading) + if upsert_heading else (None, None)) + # The precondition is for that block, so it is checked now: on a read past + # the cache, before --dry-run and before the page or heading are created. + # With no match there is no block and no hash, and creating one is free. + expect = Expect(hash=expect_hash) + held = {} + if upsert_heading and found_uuid: + if expect: + matched = read_fresh(api, found_uuid) + check_precondition(matched, expect) + # Held to the text the hash was checked on, as update-block does. + held = {"expect": matched.get("content") or ""} + else: + require_precondition(Owed(("--expect-hash",)), False) + elif expect: + print_note("Note: --expect-hash has no object: no block matches " + f"'{upsert_heading}' under '{under_heading}', so one is created.") + # Ensure the journal page exists. Deferred when only # previewing: a dry run must not bring the page into existence. existing = api.get_page(page_name) @@ -743,18 +784,10 @@ def add_journal_block(ctx, contents, content_file, date, under_heading, upsert_h # A block to update is written in place; only one that is not there # yet goes at the end of the heading, before the empty blocks there. would_anchor = None - heading_uuid = (find_heading(api, page_name, under_heading) - if keep_on and not would_create_page else None) - if heading_uuid: - kids = (api.get_block(heading_uuid, include_children=True, cached=False) - or {}).get("children") or [] - target_norm = normalize_heading(upsert_heading) - if not any(isinstance(k, dict) - and normalize_heading(k.get("content", "")) == target_norm - for k in kids): - would_anchor = first_empty_at_end( - api, tree if tree is not None else [{"content": content, "children": []}], - parent_uuid=heading_uuid) + if keep_on and heading_uuid and not found_uuid: + would_anchor = first_empty_at_end( + api, tree if tree is not None else [{"content": content, "children": []}], + parent_uuid=heading_uuid) if as_json: output({"page": page_name, "date": str(d), "position": position_desc, "content": content, "dry_run": True, **would_go_before_fields(would_anchor)}, True) @@ -765,15 +798,8 @@ def add_journal_block(ctx, contents, content_file, date, under_heading, upsert_h click.echo(f" would go before the empty block {would_anchor[:8]}...") return - heading_uuid = find_or_create_heading(api, page_name, under_heading, keep_last=keep_on) - found_uuid = None - heading_block = api.get_block(heading_uuid, include_children=True) - children = heading_block.get("children", []) if heading_block else [] - target_norm = normalize_heading(upsert_heading) - for child in children: - if normalize_heading(child.get("content", "")) == target_norm: - found_uuid = child.get("uuid") - break + if not heading_uuid: + heading_uuid = find_or_create_heading(api, page_name, under_heading, keep_last=keep_on) if found_uuid: root_uuid = found_uuid @@ -784,7 +810,7 @@ def add_journal_block(ctx, contents, content_file, date, under_heading, upsert_h text = tree[0]["content"] if tree else content # The matched block keeps its properties, as with update-block (#67). _, kept = kept_properties(api, found_uuid, text) - api.update_block(found_uuid, text, properties=kept or None) + api.update_block(found_uuid, text, properties=kept or None, **held) # n counts the UUIDs the inserts returned, plus 1 for the # update. The API proves each insert and raises on one that did # not land, so this is what the graph took, not the size asked diff --git a/logseq_cli/commands/meta.py b/logseq_cli/commands/meta.py index 2e2ad38..b7286cb 100644 --- a/logseq_cli/commands/meta.py +++ b/logseq_cli/commands/meta.py @@ -13,7 +13,8 @@ get, read_config, ) from logseq_cli.safety import ( - decide, describe, refuse_missing_named_config, render_safety, safety_to_keep, + decide, describe, refuse_missing_named_config, render_safety, safety_keys, + safety_to_keep, ) from logseq_cli.group import cli, resolve_version from logseq_cli.headings import normalize_heading @@ -313,7 +314,8 @@ def _is_plain_top_level(h: str) -> bool: logseq-cli --token TOKEN doctor --json Note: Read-only. Exits 0 when ready to read, non-zero otherwise. Says whether - writes are switched off (read_only) and by what; that does not fail it. + writes are switched off (read_only), whether a precondition is required + (require_preconditions) and by what; neither fails it. Distinguishes "Logseq not running" from "running but HTTP API off" and from "API up but token rejected" - each needs a different fix. """) @@ -503,12 +505,13 @@ def add(name, ok, detail): decision = decide(cfg) except ConfigError as e: problem = e - if decision is not None: - add("read_only", None, describe(decision)) - else: - add("read_only", None, - f"unknown, commands that write refuse until this is fixed: " - f"{str(problem).splitlines()[0]}") + for key in safety_keys(): + if decision is not None: + add(key, None, describe(decision, key)) + else: + add(key, None, + f"unknown, commands that write refuse until this is fixed: " + f"{str(problem).splitlines()[0]}") healthy = all(c["ok"] for c in checks if c["ok"] is not None) @@ -535,7 +538,7 @@ def add(name, ok, detail): click.echo() if healthy and decision is None: click.echo("Ready to read; commands that write refuse until the config is fixed.") - elif healthy and decision.read_only: + elif healthy and decision.is_on("read_only"): click.echo("Ready to read; writes are off.") elif healthy: click.echo("Ready: reads and writes should work.") diff --git a/logseq_cli/commands/pages.py b/logseq_cli/commands/pages.py index d90bb72..7a7f0a2 100644 --- a/logseq_cli/commands/pages.py +++ b/logseq_cli/commands/pages.py @@ -10,7 +10,7 @@ check_property_pairs, parse_property_pairs, ) -from logseq_cli.blocktext import refuse_split_block, refuse_split_heading, refuse_split_tree +from logseq_cli.blocktext import refuse_split_block, refuse_split_heading, refuse_split_tree, with_hashes_in from logseq_cli.cliinput import content_or_file from logseq_cli.config import keep_empty_blocks_last, load_config from logseq_cli.group import cli @@ -228,6 +228,10 @@ def _fetch_one(name, ref): if api.get_page(page_name) is None: missing.append(name) blocks = api.get_page_blocks_tree(page_name) + if as_json: + # From the content as stored: before a cut keeps part of the tree + # and before --resolve-refs rewrites the text. + blocks = with_hashes_in(blocks) if no_backlinks or heading or outline: backlinks = [] else: diff --git a/logseq_cli/commands/properties.py b/logseq_cli/commands/properties.py index 42abab0..42b28b8 100644 --- a/logseq_cli/commands/properties.py +++ b/logseq_cli/commands/properties.py @@ -12,7 +12,8 @@ stored_properties, ) from logseq_cli.output import fail, follow_page, handle_connection_error, output -from logseq_cli.safety import WriteCommand +from logseq_cli.preconditions import EXPECT_HASH_HELP, Expect, check_precondition, read_fresh +from logseq_cli.safety import Owed, WriteCommand from logseq_cli.render import is_properties_block @@ -343,7 +344,9 @@ def set_property(ctx, page, key, value, dry_run, as_json): else: click.echo(f"Set '{key}:: {value}' on page '{page}'") -@cli.command("remove-property", cls=WriteCommand, epilog="""\b +@cli.command("remove-property", cls=WriteCommand, + owes=lambda params: Owed(("--expect-hash",)) if params.get("block_id") else None, + epilog="""\b Examples: logseq-cli --token TOKEN remove-property --name "X" --key "deprecated_key" logseq-cli --token TOKEN remove-property --id UUID --key "prio" @@ -353,19 +356,23 @@ def set_property(ctx, page, key, value, dry_run, as_json): --id removes the property from that one block, wherever it sits. The key is addressed as set-property stores it ("Status" as "status"); a key set-property would refuse is passed through as given. + --expect-hash goes with --id only: a page property has no block that was read. """) @click.option("--page", "--name", default=None, help="Page name (removes a page property)") @click.option("--id", "block_id", default=None, help="Block UUID (removes the property from that block)") @click.option("--key", required=True, help="Property key to remove") +@click.option("--expect-hash", "expect_hash", default=None, help="With --id: " + EXPECT_HASH_HELP) @click.option("--dry-run", "dry_run", is_flag=True, help="Show which property would be removed, without writing") @click.option("--json", "as_json", is_flag=True, help="Output as JSON") @click.pass_context @handle_connection_error -def remove_property(ctx, page, block_id, key, dry_run, as_json): +def remove_property(ctx, page, block_id, key, expect_hash, dry_run, as_json): """Remove a property from a page or from a single block.""" api = ctx.obj["api"] if bool(page) == bool(block_id): fail("Specify exactly one of: --name, --id.", as_json=as_json) + if page and expect_hash is not None: + raise click.UsageError("--expect-hash goes with --id: a page property has no block that was read.") # Address the key under the name set-property stores it as, or "set # --key Status" followed by "remove --key Status" removes nothing and still @@ -384,6 +391,8 @@ def remove_property(ctx, page, block_id, key, dry_run, as_json): block = api.get_block(block_uuid, include_children=False) if not block: fail(f"Block not found: {block_uuid}", as_json=as_json, id=block_uuid) + if expect_hash is not None: + check_precondition(read_fresh(api, block_uuid), Expect(hash=expect_hash)) target = f"block '{block_uuid}'" result = {"id": block_uuid, "property": key, "status": "removed"} else: @@ -463,7 +472,8 @@ def _remove_page_property(api, ref, blocks, key, dry_run, as_json): click.echo(f"'{key}' is not set on page '{page}'; nothing was removed") -@cli.command("set-block-property", cls=WriteCommand, epilog="""\b +@cli.command("set-block-property", cls=WriteCommand, owes=lambda params: Owed(("--expect-hash",)), + epilog="""\b Example: logseq-cli --token TOKEN set-block-property --id UUID --key "status" --value "done" Note: @@ -475,11 +485,12 @@ def _remove_page_property(api, ref, blocks, key, dry_run, as_json): @click.option("--id", "block_id", required=True, help="Block UUID") @click.option("--key", required=True, help="Property key") @click.option("--value", required=True, help="Property value") +@click.option("--expect-hash", "expect_hash", default=None, help=EXPECT_HASH_HELP) @click.option("--dry-run", "dry_run", is_flag=True, help="Show the property change, without writing") @click.option("--json", "as_json", is_flag=True, help="Output as JSON") @click.pass_context @handle_connection_error -def set_block_property(ctx, block_id, key, value, dry_run, as_json): +def set_block_property(ctx, block_id, key, value, expect_hash, dry_run, as_json): """Set or update a property on a specific block.""" api = ctx.obj["api"] @@ -502,6 +513,8 @@ def set_block_property(ctx, block_id, key, value, dry_run, as_json): if not block: fail(f"Block not found: {block_id}", as_json=as_json, reason="block_not_found", id=block_id) + if expect_hash is not None: + check_precondition(read_fresh(api, block_id), Expect(hash=expect_hash)) if dry_run: # The preview also shows the old value: one more read, for the diff --git a/logseq_cli/commands/todos.py b/logseq_cli/commands/todos.py index 6518995..c171724 100644 --- a/logseq_cli/commands/todos.py +++ b/logseq_cli/commands/todos.py @@ -3,7 +3,7 @@ import click -from logseq_cli.blocktext import pointer_target +from logseq_cli.blocktext import block_hash, pointer_target from logseq_cli.dates import ( journal_day_to_date, next_occurrence, @@ -14,7 +14,8 @@ from logseq_cli.lookup import find_blocks_by_content from logseq_cli.notes import print_note from logseq_cli.output import fail, follow_page, handle_connection_error, output -from logseq_cli.safety import WriteCommand +from logseq_cli.preconditions import Expect, check_precondition, parse_marker, read_fresh +from logseq_cli.safety import Owed, WriteCommand from logseq_cli.tasks import (FRONTEND_MARKERS, ORDER, STATES, marker_before_newline, marker_of, markers_in, marker_clause, task_text, with_marker) @@ -253,6 +254,9 @@ def get_todos(ctx, state, status, page, page_type, tag, match, from_date, to_dat "content": clean_content, "page": page_name, "uuid": uuid, + # From the stored content, not from "content" above: that field + # has lost the marker, properties and timestamps. + "hash": block_hash(content), "_journal_day": journal_day, } # The day the task was noted on, for its age. Already read for @@ -487,7 +491,18 @@ def _refuse_no_task(block: dict, content: str, as_json: bool): 'marker, e.g. "TODO …".', as_json, reason="not_a_task", id=uuid) -@cli.command("set-todo-status", cls=WriteCommand, epilog="""\b +def _marker_option(ctx, param, value): + """--expect-marker as a usage error for an unknown marker, before any request.""" + if value is None: + return None + try: + return parse_marker(value) + except ValueError as error: + raise click.BadParameter(str(error)) + + +@cli.command("set-todo-status", cls=WriteCommand, + owes=lambda params: Owed(("--expect-hash", "--expect-marker")), epilog="""\b Examples: logseq-cli --token TOKEN set-todo-status --id UUID --status DONE logseq-cli --token TOKEN set-todo-status --content "ship the parser" \\ @@ -515,11 +530,21 @@ def _refuse_no_task(block: dict, content: str, as_json: bool): @click.option("--follow-refs", is_flag=True, help="If the block holds only a ((uuid)) ref or an embed of one, follow it, " "through every such block, and update the block at the end instead.") +@click.option("--expect-hash", "expect_hash", default=None, + help="Refuse, without writing, unless the block is still the one this hash was read " + "from (the hash field of get-block --json). With --follow-refs the block at " + "the end of the refs counts. Checked before --dry-run and before a no-change answer.") +@click.option("--expect-marker", "expect_marker", default=None, callback=_marker_option, + metavar="MARKER", + help="Refuse, without writing, unless the block's marker is this one: a task " + "marker, or none for a block without one (case does not matter; an unknown " + "marker is refused with the list of known ones). Combined with --expect-hash, both must hold. Checked at the same point.") @click.option("--dry-run", "dry_run", is_flag=True, help="Show the marker change, without writing") @click.option("--json", "as_json", is_flag=True, help="Output as JSON") @click.pass_context @handle_connection_error -def set_todo_status(ctx, block_id, content, page, status, follow_refs, dry_run, as_json): +def set_todo_status(ctx, block_id, content, page, status, follow_refs, expect_hash, expect_marker, + dry_run, as_json): """Update the status of a TODO block (e.g. TODO → DONE). Identify the block either by UUID (--id) or by content substring + page (--content + --page). @@ -579,6 +604,14 @@ def set_todo_status(ctx, block_id, content, page, status, follow_refs, dry_run, if follow_refs: block, chain["followed"] = follow_ref_chain(api, block, as_json) block_id = block.get("uuid") + + # The caller's own read of the target, past the cache: the new content is + # worked out from it, and the check runs on it before anything else can end + # the command (the no-change answer, the preview, the write). + expect = Expect(hash=expect_hash, marker=expect_marker) + if expect: + block = read_fresh(api, block_id) + check_precondition(block, expect) old_content = block.get("content", "") # A task is a block Logseq stored a marker for. The text must show it where diff --git a/logseq_cli/config.py b/logseq_cli/config.py index c4a3ce4..99b77e6 100644 --- a/logseq_cli/config.py +++ b/logseq_cli/config.py @@ -42,7 +42,7 @@ KNOWN_SECTIONS = ("journal", "graph", "analysis", "safety") # The keys `[safety]` knows. Grows with every limit that gets built. -SAFETY_KEYS = ("read_only",) +SAFETY_KEYS = ("read_only", "require_preconditions") class ConfigError(Exception): @@ -125,10 +125,11 @@ def check_safety(config: dict) -> None: close = difflib.get_close_matches(_norm(name), [_norm(k) for k in SAFETY_KEYS], n=1) hint = f", did you mean `{_safety_key(close[0])}`?" if close else "" raise ConfigError(f"{where}: unknown key `{name}` in [safety]{hint}") - value = section.get("read_only", False) - if not isinstance(value, bool): - raise ConfigError( - f"{where}: [safety] read_only must be true or false, got {value!r}.") + for key in SAFETY_KEYS: + value = section.get(key, False) + if not isinstance(value, bool): + raise ConfigError( + f"{where}: [safety] {key} must be true or false, got {value!r}.") def named_config_missing() -> Path | None: diff --git a/logseq_cli/group.py b/logseq_cli/group.py index 6d148f1..574e157 100644 --- a/logseq_cli/group.py +++ b/logseq_cli/group.py @@ -20,7 +20,7 @@ import click from logseq_cli.api import LogseqAPI, InvalidPortError -from logseq_cli.safety import FLAG_KEY +from logseq_cli.safety import flag_key def resolve_version() -> str: @@ -52,8 +52,9 @@ def resolve_version() -> str: @click.option("--token", default=None, help="Logseq API Bearer token") @click.option("--no-cache", "no_cache", is_flag=True, help="Bypass the in-memory read cache for this invocation") @click.option("--read-only", "read_only", is_flag=True, help="Refuse every command that writes, for this invocation. Can only tighten: nothing loosens a [safety] read_only = true") +@click.option("--require-preconditions", "require_preconditions", is_flag=True, help="Refuse a command that changes a block it read unless it names what it read (--expect-hash, --expect-marker), for this invocation. Can only tighten: nothing loosens a [safety] require_preconditions = true") @click.pass_context -def cli(ctx, host, port, token, no_cache, read_only): +def cli(ctx, host, port, token, no_cache, read_only, require_preconditions): """CLI for Logseq knowledge graph - pages, journals, blocks, search, and graph analysis.""" ctx.ensure_object(dict) try: @@ -66,4 +67,5 @@ def cli(ctx, host, port, token, no_cache, read_only): if no_cache: api.cache_enabled = False ctx.obj["api"] = api - ctx.obj[FLAG_KEY] = read_only + ctx.obj[flag_key("read_only")] = read_only + ctx.obj[flag_key("require_preconditions")] = require_preconditions diff --git a/logseq_cli/headings.py b/logseq_cli/headings.py index c910d3b..ae16f60 100644 --- a/logseq_cli/headings.py +++ b/logseq_cli/headings.py @@ -78,7 +78,7 @@ def normalize_heading(text: str) -> str: return ' '.join(stripped.split()) -def find_heading(api, page_name: str, heading: str) -> str | None: +def find_heading(api, page_name: str, heading: str, *, cached: bool = True) -> str | None: """Find an existing heading block's UUID on a page; never create one. Split out of :func:`find_or_create_heading` for the --dry-run paths: a @@ -88,12 +88,37 @@ def find_heading(api, page_name: str, heading: str) -> str | None: Returns the UUID, or None if no block on the page matches the heading. """ target = normalize_heading(heading) - for block in api.get_page_blocks_tree(page_name) or []: + # The flag goes on only when it is off: stand-ins for the client that + # predate it keep working for every caller that reads the cache. + fresh = {} if cached else {"cached": False} + for block in api.get_page_blocks_tree(page_name, **fresh) or []: if normalize_heading(block.get("content", "")) == target: return block.get("uuid") return None +def find_upsert_target(api, page_name: str, under_heading: str, upsert_heading: str): + """Which block ``--upsert-heading`` replaces: ``(heading_uuid, child_uuid)``. + + The one choice the live run and the preview share, so a preview cannot + name another block than the run writes. It reads without the cache, since + the answer decides which block is overwritten, and it never creates + anything: it runs before the journal page and the heading are brought into + existence. ``heading_uuid`` is None when the page has no such heading, + ``child_uuid`` None when no child of the heading matches (the first match + wins). + """ + heading_uuid = find_heading(api, page_name, under_heading, cached=False) + if not heading_uuid: + return None, None + heading_block = api.get_block(heading_uuid, include_children=True, cached=False) + target = normalize_heading(upsert_heading) + for child in (heading_block or {}).get("children") or []: + if isinstance(child, dict) and normalize_heading(child.get("content", "")) == target: + return heading_uuid, child.get("uuid") + return heading_uuid, None + + def find_or_create_heading(api, page_name: str, heading: str, *, keep_last: bool = False) -> str: """Find heading block UUID on page, create if missing. diff --git a/logseq_cli/output.py b/logseq_cli/output.py index ee00bd3..7d02ec5 100644 --- a/logseq_cli/output.py +++ b/logseq_cli/output.py @@ -16,7 +16,7 @@ from logseq_cli.headings import TitleHeadingOnly from logseq_cli.blocktext import IdLineError, SplitBlockError from logseq_cli.notes import hold_notes, release_notes, take_notes -from logseq_cli.safety import guard_write +from logseq_cli.safety import guard_preconditions, guard_write from logseq_cli.pagenames import AliasError, AmbiguousAliasError, resolve_page, resolve_page_to_write from logseq_cli.writerefused import WriteRefused, partial_state @@ -76,10 +76,11 @@ def wrapper(*args, **kwargs): try: # After click has parsed the options, before the command's first # request: a command that writes (WriteCommand) refuses here when - # writes are off. + # writes are off, and then when it owes a precondition it was not given. ctx = click.get_current_context(silent=True) if ctx is not None and getattr(ctx.command, "writes", False): guard_write() + guard_preconditions(ctx.command, ctx.params) return func(*args, **kwargs) # The four failures of the connection can come after writes of the # call landed, or with one sent and not proven: _after_writes adds diff --git a/logseq_cli/preconditions.py b/logseq_cli/preconditions.py new file mode 100644 index 0000000..28b10e0 --- /dev/null +++ b/logseq_cli/preconditions.py @@ -0,0 +1,108 @@ +"""Does the block still match what the caller read? One question, one module. + +A caller that reads a block, decides and writes can be overtaken: the block +changed in between. ``--expect-hash`` lets it say which state it decided on; +:func:`check_precondition` refuses the write when the block is another. The +refusal never carries the current hash (see ``PreconditionFailed``). + +Takes the API, because a check is only worth anything on a read that went +past the cache: :func:`read_fresh` is that read. +""" +from dataclasses import dataclass + +from logseq_cli.blocktext import block_hash, tree_hash +from logseq_cli.tasks import ORDER +from logseq_cli.writerefused import BlockNotFound, PreconditionFailed + +# The --expect-hash help the commands that change a read block share, so a +# change to what the check promises is made once. +EXPECT_HASH_HELP = ( + "Refuse, without writing, unless the block is still the one this hash was read " + "from (the hash field of get-block --json). Checked on a read past the cache, " + "before --dry-run and before the first write.") + + +@dataclass(frozen=True) +class Expect: + """What the caller says the block is. An empty one checks nothing.""" + hash: str = None # --expect-hash + tree_hash: str = None # --expect-tree-hash (remove-block: the block and everything under it) + marker: str = None # --expect-marker, as parse_marker made it ("NONE" = no marker) + + def __bool__(self) -> bool: + return self.hash is not None or self.tree_hash is not None or self.marker is not None + + +NO_MARKER = "NONE" + + +def parse_marker(value: str) -> str: + """The marker an ``--expect-marker`` value names, upper case; ``none`` is "NONE". + + Raises ValueError, listing the accepted values, for anything outside + ``tasks.ORDER``: a typo must not read as a marker no block has. + """ + wanted = value.strip().upper() + if wanted != NO_MARKER and wanted not in ORDER: + raise ValueError(f"unknown marker '{value}'. Use one of {', '.join(ORDER)}, " + "or none for a block without a marker.") + return wanted + + +def read_fresh(api, block_id: str, *, include_children: bool = False) -> dict: + """The block as Logseq holds it now, past the cache. + + Raises BlockNotFound when there is none (or only the placeholder Logseq + keeps for a ref whose block does not exist, see ``api.block_or_none``). + """ + block = api.get_block(block_id, include_children=include_children, cached=False) + if not block: + raise BlockNotFound(f"Block {block_id} not found.", id=block_id) + return block + + +def check_precondition(block: dict, expect: Expect) -> None: + """Raise PreconditionFailed unless ``block`` still matches all of ``expect``. + + ``block`` is the caller's own read of the target, taken past the cache + (:func:`read_fresh`), the one it builds the new content from. Does nothing + for an empty ``expect``. + """ + if not expect: + return + marker = block.get("marker") or None + hash_differs = (expect.hash is not None + and block_hash(block.get("content")) != expect.hash.strip().lower()) + # Needs the children as blocks: the caller read them (read_fresh(include_children=True)). + tree_differs = (expect.tree_hash is not None + and tree_hash(block) != expect.tree_hash.strip().lower()) + # Logseq's own field, as get-todos reads it, not a pattern over the text. + marker_differs = expect.marker is not None and (marker or NO_MARKER) != expect.marker + if hash_differs or tree_differs or marker_differs: + first_line = (block.get("content") or "").split("\n", 1)[0] + expected = {} + if expect.hash is not None: + expected["expected_hash"] = expect.hash + if expect.tree_hash is not None: + expected["expected_tree_hash"] = expect.tree_hash + if expect.marker is not None: + expected["expected_marker"] = expect.marker + raise PreconditionFailed( + f"Block {str(block.get('uuid'))[:8]}.. changed since it was read " + f'(marker is {marker or "none"}, first line: "{first_line}"). ' + "Read it again and decide again.", + **expected, actual_marker=marker, first_line=first_line) + + +def read_checked(api, block_id: str, expect: Expect, *, include_children: bool = False): + """The block a command works from: checked against ``expect`` when it has one. + + With a precondition the read goes past the cache and is checked at once, + so a refusal comes before anything is previewed or written. Without one + it is the plain cached read, which may be falsy for a missing block. + """ + if not expect: + return api.get_block(block_id, include_children=include_children) + block = read_fresh(api, block_id, include_children=include_children) + check_precondition(block, expect) + return block diff --git a/logseq_cli/safety.py b/logseq_cli/safety.py index 21aa359..9a0081b 100644 --- a/logseq_cli/safety.py +++ b/logseq_cli/safety.py @@ -1,6 +1,13 @@ """What keeps a command from writing: ``[safety] read_only`` and its friends. -One function decides whether writes are off and where the switch came from +Each key of ``[safety]`` is a bool switch that only tightens, and every one is +worked out the same way from config, environment and flag. The environment +variable and the flag key are derived from the key (``read_only`` has +``LOGSEQ_CLI_READ_ONLY`` and ``read_only_flag``), so a key added to +``config.SAFETY_KEYS`` is decided, described and kept by ``init`` without a +line of its own here. + +One function decides which switches are on and where each came from (:func:`decide`); everything else asks it, the checks through :func:`write_decision`. The commands that write ask through :class:`WriteCommand`, before their first request; ``LogseqAPI._post`` asks @@ -21,35 +28,54 @@ """ import os from dataclasses import dataclass -from typing import NamedTuple import click +from logseq_cli import config as _config from logseq_cli.config import ( CONFIG_ENV_VAR, ConfigError, check_safety, config_search_paths, named_config_missing, read_config, ) from logseq_cli.notes import print_note -from logseq_cli.writerefused import ReadOnly - -ENV_VAR = "LOGSEQ_CLI_READ_ONLY" +from logseq_cli.writerefused import PreconditionRequired, ReadOnly _ON = ("1", "true", "yes", "on") _OFF = ("0", "false", "no", "off", "") _CACHE = "logseq_cli.safety.decision" -FLAG_KEY = "read_only_flag" + + +def env_var(key: str) -> str: + """The environment variable that switches ``key`` on: ``LOGSEQ_CLI_READ_ONLY``.""" + return f"LOGSEQ_CLI_{key.upper()}" + + +def flag_key(key: str) -> str: + """Where the group's flag for ``key`` leaves its value in the click context.""" + return f"{key}_flag" + + +def flag_name(key: str) -> str: + """The group option that switches ``key`` on: ``--read-only``.""" + return "--" + key.replace("_", "-") + + +def safety_keys() -> tuple: + """The keys ``[safety]`` knows, in the order they are shown.""" + return _config.SAFETY_KEYS @dataclass(frozen=True) class WriteDecision: - """Whether writes are off, and which of config, env and flag switched them.""" - sources: tuple = () + """Which ``[safety]`` switches are on, and which of config, env and flag set each.""" + by_key: tuple = () # ((key, (source, ...)), ...): every key, () when off config_path: str | None = None has_safety: bool = False # the config file has a [safety] section - @property - def read_only(self) -> bool: - return bool(self.sources) + def sources_of(self, key: str) -> tuple: + return dict(self.by_key).get(key, ()) + + def is_on(self, key: str) -> bool: + return bool(self.sources_of(key)) def _obj() -> dict | None: @@ -58,8 +84,9 @@ def _obj() -> dict | None: return ctx.obj if ctx is not None and isinstance(ctx.obj, dict) else None -def _env_on() -> bool: - raw = os.environ.get(ENV_VAR) +def _env_on(key: str) -> bool: + name = env_var(key) + raw = os.environ.get(name) if raw is None: return False value = raw.strip().lower() @@ -67,7 +94,7 @@ def _env_on() -> bool: return True if value in _OFF: return False - print_note(f"warning: {ENV_VAR}={raw!r} is not a value it knows; " + print_note(f"warning: {name}={raw!r} is not a value it knows; " "treating it as on (unknown means off-limits).") return True @@ -87,27 +114,31 @@ def refuse_missing_named_config() -> None: def decide(config: dict) -> WriteDecision: - """Are writes off, from a loaded config, the environment and the flag. + """Which switches are on, from a loaded config, the environment and the flags. - The one place the three are put together; ``write_decision`` (the gate) + The one place the three are put together, per key; ``write_decision`` (the gate) and ``doctor`` both come here. Raises ConfigError for a ``[safety]`` that cannot be trusted. """ check_safety(config) - sources = [] - if (config.get("safety") or {}).get("read_only") is True: - sources.append("config") - if _env_on(): - sources.append("env") - if (_obj() or {}).get(FLAG_KEY): - sources.append("flag") - return WriteDecision(tuple(sources), config.get("_path"), "safety" in config) + section = config.get("safety") or {} + by_key = [] + for key in safety_keys(): + sources = [] + if section.get(key) is True: + sources.append("config") + if _env_on(key): + sources.append("env") + if (_obj() or {}).get(flag_key(key)): + sources.append("flag") + by_key.append((key, tuple(sources))) + return WriteDecision(tuple(by_key), config.get("_path"), "safety" in config) def write_decision() -> WriteDecision: - """Are writes off, and from where. Worked out once per call. + """Which switches are on, and from where. Worked out once per call. - The flag is the group's ``--read-only``, read from the click context. The + The flags are the group's, read from the click context. The answer is kept in that context, so the command gate and the network check see the same one and the config is read once. Without a context (the client used from a script) nothing is kept and the flag is off. @@ -122,28 +153,89 @@ def write_decision() -> WriteDecision: return decision -class _Words(NamedTuple): - refusal: str # in the message of the refusal - state: str # in the line doctor shows +def refusal_reason(key: str, source: str, path) -> str: + """One source of a switch, put in words for a refusal.""" + return { + "config": f"{key} = true in [safety] of {path}", + "env": f"{env_var(key)} is set", + "flag": f"{flag_name(key)} was passed", + }[source] -# How each source is put in words. -_WORDING = { - "config": _Words("read_only = true in [safety] of {path}", "config {path}"), - "env": _Words(f"{ENV_VAR} is set", "env"), - "flag": _Words("--read-only was passed", "flag"), -} +def _state_reason(source: str, path) -> str: + """One source of a switch, put in words for the line doctor shows.""" + return f"config {path}" if source == "config" else source def guard_write() -> None: """Raise :class:`ReadOnly` when writes are off.""" decision = write_decision() - if not decision.read_only: + sources = decision.sources_of("read_only") + if not sources: return - reasons = [_WORDING[name].refusal.format(path=decision.config_path) - for name in decision.sources] + reasons = [refusal_reason("read_only", name, decision.config_path) for name in sources] raise ReadOnly(f"Writes are off: {'; '.join(reasons)}.", - source=list(decision.sources), config_path=decision.config_path) + source=list(sources), config_path=decision.config_path) + + +@dataclass(frozen=True) +class Owed: + """What a call owes when it changes a block it read. + + ``options`` are the ones that would do, any one of them. ``always`` names + why the call owes it whatever ``require_preconditions`` says (``"--next"``), + or is ``None`` when only the switch makes it owe. + """ + options: tuple + always: str | None = None + + +def _param_name(option: str) -> str: + return option.lstrip("-").replace("-", "_") + + +def guard_preconditions(command, params: dict) -> None: + """Raise :class:`PreconditionRequired` when the call owes a precondition it was not given. + + The one place the obligation is checked, right behind :func:`guard_write` + (a call that cannot write is refused as that, whatever it left out) and + before the command's first request. ``command.owes`` is asked with the + parsed parameters, so the answer is the call's, not the command's: it may + owe nothing for one set of options and something for another. A command + without ``owes`` owes nothing. + """ + owes = getattr(command, "owes", None) + owed = owes(params) if owes is not None else None + if owed is None: + return + require_precondition(owed, any(params.get(_param_name(option)) is not None + for option in owed.options)) + + +def require_precondition(owed: Owed, given: bool) -> None: + """Raise :class:`PreconditionRequired` when ``owed`` is not met by ``given``. + + Behind :func:`guard_preconditions`, and called by a command whose obligation + is only known once it has read something (``add-journal-block + --upsert-heading`` owes after it has picked the block it replaces), so the + refusal and its wording stay in one place. + """ + if given: + return + decision = write_decision() + sources = decision.sources_of("require_preconditions") + if not (sources or owed.always): + return + options = " or ".join(owed.options) + if owed.always: + why = f"{owed.always} needs one" + else: + why = "; ".join(refusal_reason("require_preconditions", name, decision.config_path) + for name in sources) + raise PreconditionRequired( + f"A precondition is required: {why}. Pass {options}.", + source=list(sources), config_path=decision.config_path if sources else None, + options=list(owed.options)) class WriteCommand(click.Command): @@ -156,9 +248,16 @@ class WriteCommand(click.Command): JSON whatever order the decorators are in (there is only the one), and ``--dry-run`` is refused too: a preview that says "would write" when the real run cannot would lie about it. + + ``owes`` is the precondition the call owes (:func:`guard_preconditions`): + a function from the parsed parameters to an :class:`Owed`, or ``None``. """ writes = True + def __init__(self, *args, owes=None, **kwargs): + super().__init__(*args, **kwargs) + self.owes = owes + def render_safety(section: dict) -> str: """The ``[safety]`` table as TOML text, for a file that is written anew. @@ -170,16 +269,16 @@ def render_safety(section: dict) -> str: return "\n".join(lines) + "\n" -def describe(decision: WriteDecision) -> str: - """One line for doctor: the state of ``read_only`` and where it comes from.""" - if decision.read_only: - return "on (" + ", ".join(_WORDING[name].state.format(path=decision.config_path) - for name in decision.sources) + ")" +def describe(decision: WriteDecision, key: str) -> str: + """One line for doctor: the state of ``key`` and where it comes from.""" + if decision.is_on(key): + return "on (" + ", ".join(_state_reason(name, decision.config_path) + for name in decision.sources_of(key)) + ")" if decision.config_path is None: return "off (no config file found)" if not decision.has_safety: return f"off (no [safety] in {decision.config_path})" - return f"off ([safety] read_only = false in {decision.config_path})" + return f"off ([safety] {key} = false in {decision.config_path})" def safety_to_keep(target, active): diff --git a/logseq_cli/writerefused.py b/logseq_cli/writerefused.py index f0e8cf5..a62b2fd 100644 --- a/logseq_cli/writerefused.py +++ b/logseq_cli/writerefused.py @@ -44,6 +44,12 @@ class BlockChanged(WriteRefused): 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).""" + reason = "block_not_found" + + class EditorStateUnknown(WriteRefused): """checkEditing answered something not understood. Field: answer.""" reason = "editor_state_unknown" @@ -59,6 +65,22 @@ class PageExists(WriteRefused): reason = "page_exists" +class PreconditionFailed(WriteRefused): + """The block no longer is what the caller said it read. Fields: the + ``expected_*`` the call gave, ``actual_marker`` (None for a block without + one) and ``first_line``. Never the current hash: with it, a caller could + repeat its old text in one step and write over what the check protects.""" + reason = "precondition_failed" + + +class PreconditionRequired(WriteRefused): + """The call changes a block it read and gave no precondition, which it owes. + Fields: source (a list of "config", "env", "flag"; empty when the command + asks for it whatever the switch says), config_path (None when no config + file was found), options (the ones, any of which would do).""" + reason = "precondition_required" + + class RenameRefused(WriteRefused): """renamePage onto an empty or taken name. Fields: old, new, why ("empty" | "exists").""" reason = "rename_refused" diff --git a/skills/logseq-cli/SKILL.md b/skills/logseq-cli/SKILL.md index bdf27fa..ec78425 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`: the block changed after the command read it; read it again. + - `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. - `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/conftest.py b/tests/conftest.py index b83d7c2..4947877 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -6,6 +6,8 @@ from click.testing import CliRunner from logseq_cli.api import _not_verified, block_or_none +from logseq_cli.config import SAFETY_KEYS +from logseq_cli.safety import env_var @pytest.fixture(autouse=True) @@ -18,7 +20,8 @@ def isolate_environment(monkeypatch, tmp_path_factory): the search paths start there. Both point at an empty directory. Tests that want a setting set it themselves. """ - for var in ("LOGSEQ_CLI_CONFIG", "LOGSEQ_JOURNAL_HEADING", "LOGSEQ_CLI_READ_ONLY"): + safety_vars = [env_var(key) for key in SAFETY_KEYS] + for var in ("LOGSEQ_CLI_CONFIG", "LOGSEQ_JOURNAL_HEADING", *safety_vars): monkeypatch.delenv(var, raising=False) empty_home = tmp_path_factory.mktemp("home") monkeypatch.setenv("HOME", str(empty_home)) @@ -260,7 +263,7 @@ def insert_block(self, parent, content, options=None): bucket.insert(0, block) if opts.get("before") else bucket.append(block) return {"uuid": uuid} - def get_block(self, uuid, include_children=True): + def get_block(self, uuid, include_children=True, cached=True): def find(node_uuid): if node_uuid in self.children: return {"uuid": node_uuid, "children": self.children[node_uuid]} @@ -552,13 +555,13 @@ def get_page(self, name): found["file"] = {"id": page["id"] + 5000} return found - def get_page_blocks_tree(self, name): + def get_page_blocks_tree(self, name, cached=True): page = self.page_named(name) if page is None: return None return [self._out(b, page, None) for b in page["blocks"]] - def get_block(self, uuid, include_children=True): + def get_block(self, uuid, include_children=True, cached=True): if isinstance(uuid, int): # getBlock takes a database id too uuid = next((u for u, i in self._block_ids.items() if i == uuid), None) elif isinstance(uuid, str): # and one in capitals (measured, 0.10.15) diff --git a/tests/test_block_hash.py b/tests/test_block_hash.py new file mode 100644 index 0000000..b1d9db8 --- /dev/null +++ b/tests/test_block_hash.py @@ -0,0 +1,216 @@ +"""The hash a reader gets for a block (``get-block --json``). + +It names what a caller decides on: the text, the marker, the properties that +are the block's own, and the completions logged in its LOGBOOK. What Logseq +or the UI changes on its own does not move it: collapsing, the Id Line and +``CLOCK`` lines (a clock line only ever appears together with a marker change, +which the hash sees anyway). +""" +import json + +import pytest + +from logseq_cli.cli import cli +from tests.conftest import split_runner +from tests.logseq_http_double import CLOCK_IN, CLOCK_OUT, LogseqHttpDouble + +DONE_LINE = '* State "DONE" from "TODO" [2026-03-02 Mon 09:15]' +DONE_LINE_NEXT_DAY = '* State "DONE" from "TODO" [2026-03-03 Tue 09:15]' +DRAWER_CLOCK_OPEN = f":LOGBOOK:\n{CLOCK_IN}\n:END:" +DRAWER_CLOCK_CLOSED = f":LOGBOOK:\n{CLOCK_OUT}\n:END:" +DRAWER_DONE = f":LOGBOOK:\n{DONE_LINE}\n:END:" +DRAWER_DONE_AND_CLOCK = f":LOGBOOK:\n{CLOCK_OUT}\n{DONE_LINE}\n:END:" +DRAWER_TWO_DONE = f":LOGBOOK:\n{DONE_LINE}\n{DONE_LINE_NEXT_DAY}\n:END:" + + +def _hash(content): + from logseq_cli.blocktext import block_hash + return block_hash(content) + + +def _read(double, content, *options): + return split_runner().invoke( + cli, ["--token", "t", "get-block", "--id", double.uuid_of(content), *options]) + + +def _json(double, content, *options): + result = _read(double, content, "--json", *options) + assert result.exit_code == 0, result.output + return json.loads(result.stdout) + + +def _hash_via_cli(monkeypatch, content): + double = LogseqHttpDouble().install(monkeypatch) + double.add_page("Probe Page", [{"content": content}]) + return _json(double, content)["hash"] + + +# --- the function ------------------------------------------------------------ + +def test_hash_is_twelve_hex_characters(): + h = _hash("TODO write the report") + assert len(h) == 12 and int(h, 16) >= 0 + + +def test_hash_is_the_same_for_the_same_text(): + assert _hash("TODO a") == _hash("TODO a") + + +@pytest.mark.parametrize("a, b", [ + ("TODO a", "TODO b"), + ("TODO a", "DOING a"), + ("a\nprio:: 1", "a\nprio:: 2"), +], ids=["text", "marker", "property"]) +def test_hash_changes_with_what_the_caller_decides_on(a, b): + assert _hash(a) != _hash(b) + + +@pytest.mark.parametrize("a, b", [ + ("TODO a\ncollapsed:: true", "TODO a"), + ("TODO a\nid:: 6a1d2e3f-4b5c-4d6e-8f70-8192a3b4c5d6", "TODO a"), + ("TODO a \n", "TODO a"), + (f"TODO a\n{DRAWER_CLOCK_OPEN}", "TODO a"), + (f"TODO a\n{DRAWER_CLOCK_CLOSED}", f"TODO a\n{DRAWER_CLOCK_OPEN}"), + (f"TODO a\n\n{DRAWER_CLOCK_OPEN}", "TODO a"), + (f"TODO a\n{DRAWER_DONE_AND_CLOCK}", f"TODO a\n{DRAWER_DONE}"), +], ids=["collapsed", "id", "trailing-space", "clock-drawer-appears", + "clock-line-changes", "blank-line-before-drawer", "clock-beside-done"]) +def test_hash_ignores_what_changes_on_its_own(a, b): + assert _hash(a) == _hash(b) + + +def test_hash_sees_a_new_done_line(): + assert _hash(f"TODO a\n{DRAWER_DONE}") != _hash("TODO a") + assert _hash(f"TODO a\n{DRAWER_DONE}") != _hash(f"TODO a\n{DRAWER_CLOCK_CLOSED}") + + +def test_hash_sees_a_second_done_line(): + assert _hash(f"TODO a\n{DRAWER_TWO_DONE}") != _hash(f"TODO a\n{DRAWER_DONE}") + + +def test_an_unclosed_drawer_is_text(): + assert _hash(f"TODO a\n:LOGBOOK:\n{CLOCK_IN}") != _hash("TODO a") + + +def test_collapsed_in_a_code_block_is_code(): + fenced = "a\n```\ncollapsed:: true\n```" + assert _hash(fenced) != _hash("a\n```\n```") + + +# --- the reading ------------------------------------------------------------- + +@pytest.fixture +def double(monkeypatch): + double = LogseqHttpDouble().install(monkeypatch) + double.add_page("Probe Page", [ + {"content": "TODO parent", "children": [ + {"content": "first", "children": ["grandchild"]}, + {"content": "second"}]}, + {"content": "TODO other"}, + ]) + return double + + +def test_get_block_json_carries_hash_and_tree_hash(double): + block = _json(double, "TODO parent") + assert block["hash"] == _hash("TODO parent") + assert len(block["tree_hash"]) == 12 and block["tree_hash"] != block["hash"] + + +def test_children_carry_a_hash_and_no_tree_hash(double): + block = _json(double, "TODO parent") + first, second = block["children"] + assert first["hash"] == _hash("first") and second["hash"] == _hash("second") + assert first["children"][0]["hash"] == _hash("grandchild") + assert all("tree_hash" not in c for c in block["children"]) + assert "tree_hash" not in first["children"][0] + + +def test_no_children_keeps_hash_and_drops_tree_hash(double): + block = _json(double, "TODO parent", "--no-children") + assert block["hash"] == _hash("TODO parent") + assert "tree_hash" not in block + + +def test_text_output_shows_neither(double): + r = _read(double, "TODO parent") + assert r.exit_code == 0, r.output + assert "hash" not in r.stdout.lower() + + +def test_reading_twice_gives_the_same_answer(double): + """The fields are added to a copy: a block cached by the first read does + not carry them into another command's output.""" + assert _json(double, "TODO parent") == _json(double, "TODO parent") + + +@pytest.mark.parametrize("a, b, same", [ + ("TODO a", "TODO a\ncollapsed:: true", True), + ("TODO a", f"TODO a\n{DRAWER_CLOCK_OPEN}", True), + ("TODO a", f"TODO a\n{DRAWER_CLOCK_CLOSED}", True), + (f"TODO a\n{DRAWER_CLOCK_OPEN}", f"TODO a\n{DRAWER_CLOCK_CLOSED}", True), + ("TODO a", f"TODO a\n{DRAWER_DONE}", False), + ("TODO a", "DOING a", False), +], ids=["collapsed", "clock-drawer-appears", "closed-clock-drawer-appears", + "clock-line-changes", "done-line", "marker"]) +def test_get_block_hash_over_the_api(monkeypatch, a, b, same): + assert (_hash_via_cli(monkeypatch, a) == _hash_via_cli(monkeypatch, b)) is same + + +# --- the tree hash ----------------------------------------------------------- + +def _tree_hash(monkeypatch, blocks): + double = LogseqHttpDouble().install(monkeypatch) + double.add_page("Probe Page", blocks) + return _json(double, "root")["tree_hash"] + + +def test_tree_hash_changes_with_a_new_child(monkeypatch): + one = _tree_hash(monkeypatch, [{"content": "root", "children": ["a"]}]) + two = _tree_hash(monkeypatch, [{"content": "root", "children": ["a", "b"]}]) + assert one != two + + +def test_tree_hash_changes_with_the_order_of_the_children(monkeypatch): + ab = _tree_hash(monkeypatch, [{"content": "root", "children": ["a", "b"]}]) + ba = _tree_hash(monkeypatch, [{"content": "root", "children": ["b", "a"]}]) + assert ab != ba + + +def test_tree_hash_changes_with_the_depth_of_a_descendant(monkeypatch): + flat = _tree_hash(monkeypatch, [{"content": "root", "children": ["a", "b"]}]) + deep = _tree_hash(monkeypatch, [{"content": "root", "children": [ + {"content": "a", "children": ["b"]}]}]) + assert flat != deep + + +def test_tree_hash_changes_with_a_descendants_text(monkeypatch): + a = _tree_hash(monkeypatch, [{"content": "root", "children": [ + {"content": "a", "children": ["deep"]}]}]) + b = _tree_hash(monkeypatch, [{"content": "root", "children": [ + {"content": "a", "children": ["deeper"]}]}]) + assert a != b + + +def test_tree_hash_of_a_leaf_differs_from_its_hash(monkeypatch): + double = LogseqHttpDouble().install(monkeypatch) + double.add_page("Probe Page", [{"content": "root"}]) + block = _json(double, "root") + assert block["tree_hash"] != block["hash"] + + +def test_tree_hash_ignores_what_the_hash_ignores(monkeypatch): + plain = _tree_hash(monkeypatch, [{"content": "root", "children": ["a"]}]) + noisy = _tree_hash(monkeypatch, [{"content": "root", "children": [ + f"a\ncollapsed:: true\n{DRAWER_CLOCK_OPEN}"]}]) + assert plain == noisy + + +def test_tree_hash_is_in_no_other_read(monkeypatch): + double = LogseqHttpDouble().install(monkeypatch) + double.add_page("Probe Page", [{"content": "root", "children": ["a"]}]) + for argv in (["get-page", "--name", "Probe Page", "--json"], + ["find-block", "--content", "root", "--json"]): + r = split_runner().invoke(cli, ["--token", "t", *argv]) + assert r.exit_code == 0, r.output + assert "tree_hash" not in r.stdout diff --git a/tests/test_examples_do_not_hide_failures.py b/tests/test_examples_do_not_hide_failures.py index a7edb8b..d053076 100644 --- a/tests/test_examples_do_not_hide_failures.py +++ b/tests/test_examples_do_not_hide_failures.py @@ -111,3 +111,52 @@ def test_backup_does_not_let_two_pages_share_a_file(tmp_path): assert "could not export: a_b (its file name is taken by another page)" in result.stderr assert (out / "a_b.json").read_text().strip() == '{"page": "a/b"}' assert list(out.glob("*.part")) == [] + + +SAFE_UPDATE = EXAMPLES / "safe-update-block.sh" +BLOCK = "6650a1b2-0000-4000-8000-000000000001" +SAFE_UPDATE_STUB = """case "$1" in + get-block) echo '{"uuid": "%s", "content": "TODO ship the parser", "hash": "0123456789ab"}';; + update-block) echo "$@" >> "$CALLS" + %s;; +esac +""" + + +def _run_safe_update(tmp_path, update_body): + calls = tmp_path / "calls" + env = _stub(tmp_path, SAFE_UPDATE_STUB % (BLOCK, update_body)) + env["CALLS"] = str(calls) + result = subprocess.run(["bash", str(SAFE_UPDATE), BLOCK, "TODO ship the parser, fixed"], + capture_output=True, text=True, env=env, timeout=30) + return result, calls.read_text().splitlines() if calls.exists() else [] + + +@pytest.mark.skipif(not shutil.which("bash") or not shutil.which("jq"), + reason="needs bash and jq") +def test_safe_update_passes_the_hash_it_read(tmp_path): + result, calls = _run_safe_update(tmp_path, "exit 0") + assert result.returncode == 0, result.stderr + assert len(calls) == 1 and "--expect-hash 0123456789ab" in calls[0] + + +@pytest.mark.skipif(not shutil.which("bash") or not shutil.which("jq"), + reason="needs bash and jq") +def test_safe_update_does_not_repeat_a_refused_change_with_a_fresh_hash(tmp_path): + """After `precondition_failed` the text was decided on content that is gone: + the script stops, says so, and sends no second update.""" + refusal = """echo '{"error": "refused", "reason": "precondition_failed"}' >&2; exit 1""" + result, calls = _run_safe_update(tmp_path, refusal) + assert result.returncode == 3 + assert len(calls) == 1 + assert "decide again" in result.stderr + + +@pytest.mark.skipif(not shutil.which("bash") or not shutil.which("jq"), + reason="needs bash and jq") +def test_safe_update_shows_any_other_failure(tmp_path): + result, calls = _run_safe_update( + tmp_path, """echo '{"error": "boom", "reason": "read_only"}' >&2; exit 1""") + assert result.returncode == 1 + assert "boom" in result.stderr + assert len(calls) == 1 diff --git a/tests/test_get_todos_page_type.py b/tests/test_get_todos_page_type.py index 975585b..2967ac3 100644 --- a/tests/test_get_todos_page_type.py +++ b/tests/test_get_todos_page_type.py @@ -10,6 +10,7 @@ import pytest from click.testing import CliRunner +from logseq_cli.blocktext import block_hash from logseq_cli.cli import cli from tests.test_get_todos import _mock_api_for_todos @@ -23,15 +24,16 @@ {"original-name": "Mar 12th, 2026", "name": "mar 12th, 2026", "journal-day": 20260312}) -# The output before the option existed, taken from the unchanged command. +# The output before the option existed, taken from the unchanged command (plus +# the hash every block read carries). BEFORE_JSON = { "todos": [ {"marker": "TODO", "content": "journal task", "page": "Mar 10th, 2026", - "uuid": "u-j1", "journal_day": "2026-03-10"}, + "uuid": "u-j1", "hash": block_hash("TODO journal task"), "journal_day": "2026-03-10"}, {"marker": "TODO", "content": "plain page task", "page": "Project Alpha", - "uuid": "u-p1"}, + "uuid": "u-p1", "hash": block_hash("TODO plain page task")}, {"marker": "TODO", "content": "carried task", "page": "Project Beta", - "uuid": "u-p2", "references": ["Mar 12th, 2026"]}, + "uuid": "u-p2", "hash": block_hash("TODO carried task"), "references": ["Mar 12th, 2026"]}, ], "count": 3, } diff --git a/tests/test_hash_in_read_paths.py b/tests/test_hash_in_read_paths.py new file mode 100644 index 0000000..fe2d88b --- /dev/null +++ b/tests/test_hash_in_read_paths.py @@ -0,0 +1,226 @@ +"""The hash in every read path that returns blocks. + +A caller that reads a block with get-page, get-journal-range, find-block or +get-todos must be able to write to it afterwards without a second read, so each +of them carries the same ``hash`` that ``get-block --json`` does. One function +(``block_hash``) computes it from the raw ``content`` Logseq stored; these tests +pin that every path agrees with it and that none computes it from text another +step has already rewritten. +""" +import json +from unittest.mock import MagicMock, patch + +import pytest + +from logseq_cli.blocktext import block_hash +from logseq_cli.cli import cli +from logseq_cli.preconditions import Expect, check_precondition +from tests.conftest import split_runner +from tests.logseq_http_double import LogseqHttpDouble + +REF = "11111111-2222-3333-4444-555555555555" +DONE_LINE = '* State "DONE" from "TODO" [2026-03-02 Mon 09:15]' +RAW = f"TODO write the report\nSCHEDULED: <2026-03-02 Mon>\n:LOGBOOK:\n{DONE_LINE}\n:END:\ncollapsed:: true" + + +def _run(api, args): + with patch("logseq_cli.group.LogseqAPI", return_value=api): + return split_runner().invoke(cli, ["--token", "t", *args]) + + +def _page_api(tree): + api = MagicMock() + api.get_page.return_value = {"name": "p"} + api.get_page_blocks_tree.return_value = tree + api.get_page_linked_references.return_value = [] + return api + + +def _tree(): + return [{"uuid": "a", "content": RAW, + "children": [{"uuid": "b", "content": "child text", + "children": [{"uuid": "c", "content": "grandchild"}]}]}] + + +# --- get-page ---------------------------------------------------------------- + +def test_get_page_json_has_a_hash_on_every_block_and_child(): + result = _run(_page_api(_tree()), ["get-page", "--name", "p", "--json"]) + assert result.exit_code == 0, result.output + top = json.loads(result.stdout)["blocks"][0] + assert top["hash"] == block_hash(RAW) + assert top["children"][0]["hash"] == block_hash("child text") + assert top["children"][0]["children"][0]["hash"] == block_hash("grandchild") + + +def test_get_page_text_shows_no_hash(): + result = _run(_page_api(_tree()), ["get-page", "--name", "p"]) + assert block_hash("child text") not in result.stdout + + +def test_get_page_hash_is_taken_before_resolve_refs_rewrites_content(): + raw = f"see (({REF}))" + api = _page_api([{"uuid": "a", "content": raw}]) + api.get_block.return_value = {"uuid": REF, "content": "target text", + "page": {"originalName": "Elsewhere"}} + result = _run(api, ["get-page", "--name", "p", "--resolve-refs", "--json"]) + assert result.exit_code == 0, result.output + block = json.loads(result.stdout)["blocks"][0] + assert block["content"] != raw + assert block["hash"] == block_hash(raw) + + +def test_get_page_hash_survives_heading_and_outline_cuts(): + tree = [{"uuid": "h", "content": "## Plan", "properties": {"heading": 2}, + "children": [{"uuid": "x", "content": "step"}]}] + for extra in (["--heading", "## Plan"], ["--outline"]): + result = _run(_page_api(tree), ["get-page", "--name", "p", "--json", *extra]) + assert result.exit_code == 0, result.output + assert json.loads(result.stdout)["blocks"][0]["hash"] == block_hash("## Plan") + + +def test_get_page_backlinks_carry_no_hash(): + api = _page_api(_tree()) + api.get_page_linked_references.return_value = [ + [{"originalName": "Other"}, [{"uuid": "z", "content": "points at p"}]]] + result = _run(api, ["get-page", "--name", "p", "--json"]) + backlinks = json.loads(result.stdout)["backlinks"] + assert backlinks and "hash" not in json.dumps(backlinks) + + +def test_get_page_does_not_change_what_the_client_caches(): + """The client returns the same tree on a second read; the hash is on a + copy, so a caller of the API sees no new key.""" + tree = _tree() + _run(_page_api(tree), ["get-page", "--name", "p", "--json"]) + assert "hash" not in tree[0] + + +# --- get-journal-range ------------------------------------------------------- + +def _journal_api(tree): + api = MagicMock() + api.get_all_pages.return_value = [ + {"originalName": "Aug 1st, 2026", "name": "aug 1st, 2026", "journalDay": 20260801}] + api.get_page_blocks_tree.return_value = tree + return api + + +def test_get_journal_range_json_has_a_hash_on_blocks_and_children(): + result = _run(_journal_api(_tree()), + ["get-journal-range", "--from", "2026-08-01", "--to", "2026-08-01", "--json"]) + assert result.exit_code == 0, result.output + top = json.loads(result.stdout)[0]["blocks"][0] + assert top["hash"] == block_hash(RAW) + assert top["children"][0]["children"][0]["hash"] == block_hash("grandchild") + + +def test_get_journal_range_hash_is_taken_before_resolve_refs(): + raw = f"see (({REF}))" + api = _journal_api([{"uuid": "a", "content": raw}]) + api.get_block.return_value = {"uuid": REF, "content": "target text", + "page": {"originalName": "Elsewhere"}} + result = _run(api, ["get-journal-range", "--from", "2026-08-01", "--to", "2026-08-01", + "--resolve-refs", "--json"]) + assert result.exit_code == 0, result.output + block = json.loads(result.stdout)[0]["blocks"][0] + assert block["content"] != raw + assert block["hash"] == block_hash(raw) + + +# --- find-block -------------------------------------------------------------- + +@pytest.fixture +def double(monkeypatch): + d = LogseqHttpDouble().install(monkeypatch) + d.add_page("Probe Page", [{"content": "TODO parent", "children": ["child note"]}]) + return d + + +def test_find_block_json_has_a_hash(double): + result = split_runner().invoke( + cli, ["--token", "t", "find-block", "--content", "TODO parent", "--json"]) + assert result.exit_code == 0, result.output + (found,) = json.loads(result.stdout) + assert found["hash"] == block_hash("TODO parent") + + +def test_find_block_with_children_hashes_the_children_too(double): + result = split_runner().invoke( + cli, ["--token", "t", "find-block", "--content", "TODO parent", + "--with-children", "--json"]) + assert result.exit_code == 0, result.output + (found,) = json.loads(result.stdout) + assert found["children"][0]["hash"] == block_hash("child note") + + +def test_find_block_text_shows_no_hash(double): + result = split_runner().invoke( + cli, ["--token", "t", "find-block", "--content", "TODO parent"]) + assert block_hash("TODO parent") not in result.stdout + + +# --- get-todos --------------------------------------------------------------- + +def _todos_api(content): + api = MagicMock() + row = ({"content": content, "marker": "TODO", "uuid": "u1"}, + {"original-name": "Project Alpha", "name": "project alpha"}) + api.datascript_query.side_effect = lambda q: [] if ":block/refs" in q else [row] + return api + + +def test_get_todos_hash_comes_from_the_stored_content_not_the_cleaned_field(): + result = _run(_todos_api(RAW), ["get-todos", "--json", "--no-follow-refs"]) + assert result.exit_code == 0, result.output + (todo,) = json.loads(result.stdout)["todos"] + assert todo["content"] != RAW # the output field is cleaned of marker and timestamps + assert todo["hash"] == block_hash(RAW) + + +def test_get_todos_hash_is_accepted_by_the_write_check(): + result = _run(_todos_api(RAW), ["get-todos", "--json", "--no-follow-refs"]) + (todo,) = json.loads(result.stdout)["todos"] + check_precondition({"uuid": "u1", "content": RAW}, Expect(hash=todo["hash"])) + + +def test_get_todos_text_shows_no_hash(): + result = _run(_todos_api(RAW), ["get-todos", "--no-follow-refs"]) + assert block_hash(RAW) not in result.stdout + + +# --- one function, every source --------------------------------------------- + +def test_every_read_path_gives_the_same_hash_for_the_same_block(monkeypatch): + d = LogseqHttpDouble().install(monkeypatch) + d.add_page("Probe Page", [{"content": RAW, "marker": "TODO"}]) + runner = split_runner() + + def invoke(*args): + result = runner.invoke(cli, ["--token", "t", *args, "--json"]) + assert result.exit_code == 0, result.output + return json.loads(result.stdout) + + uuid = d.uuid_of(RAW) + via_block = invoke("get-block", "--id", uuid)["hash"] + via_find = invoke("find-block", "--content", "write the report")[0]["hash"] + via_page = invoke("get-page", "--name", "Probe Page", "--no-backlinks")["blocks"][0]["hash"] + assert via_block == via_find == via_page == block_hash(RAW) + # ... and it is the hash the write check accepts for that block. + raw_block = d.pages[0]["blocks"][0] + for read_hash in (via_block, via_find, via_page): + check_precondition(raw_block, Expect(hash=read_hash)) + + +# --- --max-chars counts the hash --------------------------------------------- + +def test_max_chars_measures_the_json_with_the_hash_in_it(): + """The cap is on the printed JSON, so the hash costs room: a cap that + fitted the page without hashes withholds a block once they are on.""" + tree = [{"uuid": f"u{i}", "content": f"note {i}"} for i in range(3)] + args = ["get-page", "--name", "p", "--json", "--no-backlinks"] + whole = _run(_page_api(tree), args).stdout + fits = json.loads(_run(_page_api(tree), [*args, "--max-chars", str(len(whole))]).stdout) + assert "withheld" not in fits + short = json.loads(_run(_page_api(tree), [*args, "--max-chars", str(len(whole) - 1)]).stdout) + assert short.get("withheld") diff --git a/tests/test_normalize_block_text.py b/tests/test_normalize_block_text.py index 00f7d4f..282d89a 100644 --- a/tests/test_normalize_block_text.py +++ b/tests/test_normalize_block_text.py @@ -133,3 +133,45 @@ def test_update_block_with_a_ref_to_itself_is_proven(monkeypatch): "--content", f"see (({UUID})) here", "--json"]) assert r.exit_code == 0, r.stderr assert double.tree("Probe Page") == [("see here", [])] + + +KEPT_DRAWER = ( + "TODO task\n:LOGBOOK:\n" + '* State "DONE" from "TODO" [2026-09-29 Tue 10:00]\n' + f"{CLOCK_IN}\n:END:" +) + + +def test_keep_logbook_leaves_the_drawer_lines(): + from logseq_cli.blocktext import normalize_block_text + kept = normalize_block_text(KEPT_DRAWER, keep_logbook=True) + assert kept == KEPT_DRAWER + + +def test_keep_logbook_runs_the_other_steps_alike(): + from logseq_cli.blocktext import normalize_block_text + text = f" TODO task \nid:: {UUID}\n:LOGBOOK: \n{CLOCK_IN} \n:END:\n\n" + assert normalize_block_text(text, keep_logbook=True) == ( + f"TODO task\n:LOGBOOK:\n{CLOCK_IN}\n:END:") + + +def test_keep_logbook_leaves_an_unclosed_opener_as_it_was(): + from logseq_cli.blocktext import normalize_block_text + text = "TODO task\n:LOGBOOK:\nno end" + assert normalize_block_text(text, keep_logbook=True) == normalize_block_text(text) + + +def test_the_drawer_goes_by_default(): + # The proof calls without keep_logbook; its text must stay byte for byte. + assert _normalize(KEPT_DRAWER) == "TODO task" + + +def test_done_line_pattern_is_one_public_name_next_to_the_drawer_rule(): + from logseq_cli import blocktext + from logseq_cli.blocktext import DONE_LINE_RE + assert blocktext.drawer_lines # the rule it sits beside + assert DONE_LINE_RE.match('* State "DONE" from "TODO" [2026-09-29 Tue 10:00]') + assert DONE_LINE_RE.match(' * State "DONE" from "NOW" [2026-09-29 Tue 10:00]') + assert not DONE_LINE_RE.match('- State "DONE" from "TODO" [2026-09-29 Tue 10:00]') + assert not DONE_LINE_RE.match('* State "TODO" from "DONE" [2026-09-29 Tue 10:00]') + assert not DONE_LINE_RE.match(CLOCK_IN) diff --git a/tests/test_package_layering.py b/tests/test_package_layering.py index 31cfeab..e993dfd 100644 --- a/tests/test_package_layering.py +++ b/tests/test_package_layering.py @@ -197,3 +197,10 @@ def test_the_pure_modules_need_no_logseq_api(): f"{offenders} — a module in PURE reaches LogseqAPI. Move what needs " f"the API into a module that already takes it, or take the module out " f"of PURE and say why in ADR 0003") + + +def test_tasks_may_import_blocktext_but_blocktext_never_tasks(): + """The ``State "DONE"`` pattern lives in blocktext beside the drawer rule; + the task rules read it from there. The other way round would be a cycle.""" + assert "logseq_cli.tasks" not in _package_imports(PACKAGE / "blocktext.py") + assert "logseq_cli.blocktext" in _package_imports(PACKAGE / "tasks.py") diff --git a/tests/test_precondition_coverage.py b/tests/test_precondition_coverage.py new file mode 100644 index 0000000..86550a9 --- /dev/null +++ b/tests/test_precondition_coverage.py @@ -0,0 +1,134 @@ +"""Every command that takes a precondition checks it before anything else. + +A command that gains an ``--expect-*`` option and forgets to call the check +would accept the option and ignore it: the caller believes it is protected. +The commands are found by their options, so a new one cannot stay out. Each +needs a recipe below (a graph, and a call that would change it); the checks +are the same for all: a wrong precondition refuses without a write, also under +``--dry-run`` and where the call would change nothing. +""" +import json + +import pytest + +from logseq_cli.cli import cli +from tests.conftest import split_runner +from tests.logseq_http_double import LogseqHttpDouble + +TASK = "00000000-0000-4000-8000-0000000000a1" +OTHER = "00000000-0000-4000-8000-0000000000a2" +# Per option, a value the recipe's block does not match (it is a TODO). +WRONG = {"--expect-hash": "0123456789ab", "--expect-marker": "DONE", + "--expect-tree-hash": "0123456789ab"} + +# command -> (pages, options naming a change, options naming no change or None, has --dry-run) +RECIPES = { + "set-todo-status": ( + {"Probe Page": [{"content": "TODO ship the parser", "marker": "TODO", "uuid": TASK}]}, + ["--id", TASK, "--status", "DONE"], + ["--id", TASK, "--status", "TODO"], + True), + "update-block": ( + {"Probe Page": [{"content": "TODO ship the parser", "marker": "TODO", "uuid": TASK}]}, + ["--id", TASK, "--content", "TODO ship the parser, fixed"], + None, + True), + "set-block-property": ( + {"Probe Page": [{"content": "TODO ship the parser", "marker": "TODO", "uuid": TASK}]}, + ["--id", TASK, "--key", "prio", "--value", "high"], + None, + True), + "remove-property": ( + {"Probe Page": [{"content": "TODO ship the parser\nprio:: high", "marker": "TODO", + "uuid": TASK}]}, + ["--id", TASK, "--key", "prio"], + None, + True), + "move-block": ( + {"Probe Page": [{"content": "TODO ship the parser", "marker": "TODO", "uuid": TASK, + "children": [{"content": "a child"}]}, + {"content": "the new parent", "uuid": OTHER}]}, + ["--id", TASK, "--under", OTHER], + None, + True), + "copy-block": ( + {"Probe Page": [{"content": "TODO ship the parser", "marker": "TODO", "uuid": TASK, + "children": [{"content": "a child"}]}], + "Other Page": [{"content": "already here"}]}, + ["--id", TASK, "--to-page", "Other Page", "--remove"], + None, + True), + # The precondition is for the block the upsert replaces, found by its heading. + "add-journal-block": ( + {"2099-01-05, Monday": [{"content": "## Log", "children": [ + {"content": "### Carol\nTODO ship the parser", "marker": "TODO", "uuid": TASK}]}]}, + ["--date", "2099-01-05", "--under-heading", "## Log", "--upsert-heading", "### Carol", + "--content", "### Carol\nTODO ship the parser, fixed"], + None, + True), + # The alias is a second name for the same command and must carry the same gate. + "remove-block": ( + {"Probe Page": [{"content": "TODO ship the parser", "marker": "TODO", "uuid": TASK}]}, + ["--id", TASK], + None, + True), + "delete-block": ( + {"Probe Page": [{"content": "TODO ship the parser", "marker": "TODO", "uuid": TASK}]}, + ["--id", TASK], + None, + True), +} + + +def _expecting_commands(): + """Command name -> its ``--expect-*`` options, from the registry.""" + found = {} + for name, command in cli.commands.items(): + options = [opt for p in command.params for opt in p.opts if opt.startswith("--expect-")] + if options: + found[name] = options + return found + + +# The commands that change a block they read, and so take a precondition. +# `delete-block` is a second name for `remove-block`, not a ninth command. +PRECONDITION_COMMANDS = { + "set-todo-status", "update-block", "set-block-property", "remove-property", + "move-block", "copy-block", "add-journal-block", "remove-block", +} + + +def test_exactly_these_commands_take_a_precondition(): + """A command that loses its option, or gains one unannounced, shows here.""" + names = set(_expecting_commands()) + assert names - {"delete-block"} == PRECONDITION_COMMANDS + assert cli.commands["delete-block"] is cli.commands["remove-block"] + + +def test_every_command_with_a_precondition_has_a_recipe(): + assert set(_expecting_commands()) <= set(RECIPES), ( + "a command takes an --expect-* option and this test does not try it: add a recipe") + + +CASES = [(name, option, variant) + for name, options in sorted(_expecting_commands().items()) if name in RECIPES + for option in options + for variant in ("write", "dry-run", "no-change")] + + +@pytest.mark.parametrize("name,option,variant", CASES, + ids=[f"{n}{o}-{v}" for n, o, v in CASES]) +def test_a_wrong_precondition_refuses_without_a_write(monkeypatch, name, option, variant): + pages, change, no_change, has_dry_run = RECIPES[name] + if variant == "dry-run" and not has_dry_run: + pytest.skip("no --dry-run") + if variant == "no-change" and no_change is None: + pytest.skip("no call that changes nothing") + double = LogseqHttpDouble.installed(monkeypatch, pages) + before = double.snapshot() + args = {"write": change, "dry-run": [*change, "--dry-run"], "no-change": no_change}[variant] + result = split_runner().invoke(cli, ["--token", "t", name, *args, option, WRONG[option], "--json"]) + assert result.exit_code != 0 and result.stdout == "", result.stdout + assert json.loads(result.stderr)["reason"] == "precondition_failed" + assert double.writes() == [] + assert double.snapshot() == before diff --git a/tests/test_require_preconditions.py b/tests/test_require_preconditions.py new file mode 100644 index 0000000..01ecd62 --- /dev/null +++ b/tests/test_require_preconditions.py @@ -0,0 +1,333 @@ +"""``[safety] require_preconditions``: a command that changes a block it read +refuses without a precondition, before its first request. + +The obligation hangs on the call, not on the command (``WriteCommand(owes=...)``), +and is checked in one place, right behind the write gate. Driven through the real +client with ``requests.post`` replaced by a recorder that refuses everything: a +command that gets past the gate fails for another reason, and ``sent`` says so. +""" +import json + +import click +import pytest + +from logseq_cli.cli import cli +from logseq_cli.output import handle_connection_error +from logseq_cli.safety import Owed, WriteCommand +from tests.conftest import split_runner +from tests.logseq_http_double import LogseqHttpDouble + +TASK = "00000000-0000-4000-8000-0000000000a1" +ON = "[safety]\nrequire_preconditions = true\n" + + +@pytest.fixture +def sent(monkeypatch): + """Every request the client tries to send; none gets an answer.""" + import logseq_cli.api + + seen = [] + + def post(url, json=None, **kwargs): + seen.append((json or {}).get("method")) + raise RuntimeError("a request was sent") + + monkeypatch.setattr(logseq_cli.api.requests, "post", post) + return seen + + +@pytest.fixture +def config(tmp_path, monkeypatch): + def write(text): + path = tmp_path / "config.toml" + path.write_text(text, encoding="utf-8") + monkeypatch.setenv("LOGSEQ_CLI_CONFIG", str(path)) + return path + return write + + +def invoke(*args): + return split_runner().invoke(cli, ["--token", "X", *args]) + + +def error_of(result): + return json.loads(result.stderr) + + +def set_status(*extra, flags=()): + return invoke(*flags, "set-todo-status", "--id", TASK, "--status", "DONE", *extra, "--json") + + +class TestTheSwitch: + def test_without_a_precondition_the_command_refuses_before_a_request(self, config, sent): + path = config(ON) + result = set_status() + assert result.exit_code != 0 + error = error_of(result) + assert error["reason"] == "precondition_required" + assert error["source"] == ["config"] + assert error["config_path"] == str(path) + assert error["options"] == ["--expect-hash", "--expect-marker"] + assert error["writes_landed"] == 0 + assert "require_preconditions = true" in error["error"] + assert "--expect-hash" in error["error"] and "--expect-marker" in error["error"] + assert error["error"].count("Nothing was written.") == 1 + assert sent == [] + + def test_the_refusal_reads_as_a_sentence_in_plain_text(self, config, sent): + path = config(ON) + result = invoke("set-todo-status", "--id", TASK, "--status", "DONE") + assert result.exit_code != 0 + assert result.stderr == ( + f"Error: A precondition is required: require_preconditions = true in [safety] of " + f"{path}. Pass --expect-hash or --expect-marker. Nothing was written.\n") + + def test_a_matching_marker_gets_the_call_past_the_gate(self, config, monkeypatch): + config(ON) + LogseqHttpDouble.installed(monkeypatch, { + "Probe Page": [{"content": "TODO ship the parser", "marker": "TODO", "uuid": TASK}]}) + result = set_status("--expect-marker", "todo") + assert result.exit_code == 0, result.stderr + + def test_a_hash_is_a_precondition_too(self, config, monkeypatch): + # The gate lets it through; the check then finds the hash wrong. + config(ON) + LogseqHttpDouble.installed(monkeypatch, { + "Probe Page": [{"content": "TODO ship the parser", "marker": "TODO", "uuid": TASK}]}) + result = set_status("--expect-hash", "0123456789ab") + assert error_of(result)["reason"] == "precondition_failed" + + def test_off_by_default(self, monkeypatch): + LogseqHttpDouble.installed(monkeypatch, { + "Probe Page": [{"content": "TODO ship the parser", "marker": "TODO", "uuid": TASK}]}) + result = set_status() + assert result.exit_code == 0, result.stderr + + def test_a_false_in_the_config_is_off(self, config, monkeypatch): + config("[safety]\nrequire_preconditions = false\n") + LogseqHttpDouble.installed(monkeypatch, { + "Probe Page": [{"content": "TODO ship the parser", "marker": "TODO", "uuid": TASK}]}) + assert set_status().exit_code == 0 + + def test_help_and_usage_errors_come_before_the_refusal(self, config, sent): + config(ON) + assert invoke("set-todo-status", "--help").exit_code == 0 + result = invoke("set-todo-status", "--id", TASK) + assert result.exit_code == 2 and "Missing option" in result.stderr + + +class TestItOnlyTightens: + def test_the_flag_alone_switches_it_on(self, sent): + result = set_status(flags=("--require-preconditions",)) + error = error_of(result) + assert error["reason"] == "precondition_required" + assert error["source"] == ["flag"] + assert error["config_path"] is None + assert sent == [] + + def test_env_alone_switches_it_on(self, sent, monkeypatch): + monkeypatch.setenv("LOGSEQ_CLI_REQUIRE_PRECONDITIONS", "1") + assert error_of(set_status())["source"] == ["env"] + + def test_env_false_does_not_loosen_the_config(self, config, sent, monkeypatch): + config(ON) + monkeypatch.setenv("LOGSEQ_CLI_REQUIRE_PRECONDITIONS", "false") + error = error_of(set_status()) + assert error["reason"] == "precondition_required" + assert error["source"] == ["config"] + + def test_a_config_that_says_false_is_tightened_by_the_flag(self, config, sent): + config("[safety]\nrequire_preconditions = false\n") + assert error_of(set_status(flags=("--require-preconditions",)))["source"] == ["flag"] + + def test_every_source_is_named(self, config, sent, monkeypatch): + config(ON) + monkeypatch.setenv("LOGSEQ_CLI_REQUIRE_PRECONDITIONS", "yes") + error = error_of(set_status(flags=("--require-preconditions",))) + assert error["source"] == ["config", "env", "flag"] + assert "LOGSEQ_CLI_REQUIRE_PRECONDITIONS is set" in error["error"] + assert "--require-preconditions was passed" in error["error"] + + def test_there_is_no_flag_that_loosens_it(self): + options = [opt for p in cli.params for opt in p.opts] + assert "--require-preconditions" in options + assert "--no-require-preconditions" not in options + + def test_an_unknown_env_value_counts_as_on_and_says_so(self, sent, monkeypatch): + monkeypatch.setenv("LOGSEQ_CLI_REQUIRE_PRECONDITIONS", "flase") + result = invoke("set-todo-status", "--id", TASK, "--status", "DONE") + assert result.exit_code != 0 + assert "A precondition is required" in result.stderr + assert "LOGSEQ_CLI_REQUIRE_PRECONDITIONS='flase'" in result.stderr + assert sent == [] + + +class TestFailClosed: + @pytest.mark.parametrize("value", ["'yes'", "1"]) + def test_a_value_that_is_no_bool_is_a_config_error(self, config, sent, value): + config(f"[safety]\nrequire_preconditions = {value}\n") + error = error_of(set_status()) + assert error["reason"] == "config_error" + assert "require_preconditions must be true or false" in error["error"] + assert sent == [] + + def test_a_missing_named_config_refuses_the_command(self, tmp_path, monkeypatch, sent): + monkeypatch.setenv("LOGSEQ_CLI_CONFIG", str(tmp_path / "gone.toml")) + error = error_of(set_status()) + assert error["reason"] == "config_error" + assert sent == [] + + def test_a_misspelt_key_is_refused_with_a_hint(self, config, sent): + config("[safety]\nrequire_precondition = true\n") + error = error_of(set_status()) + assert error["reason"] == "config_error" + assert "did you mean `require_preconditions`" in error["error"] + + def test_the_key_outside_safety_is_refused(self, config, sent): + config("require-preconditions = true\n") + error = error_of(set_status()) + assert error["reason"] == "config_error" + assert "belongs under [safety]" in error["error"] + + +class TestRankOverTheWriteGate: + def test_read_only_wins_over_a_missing_precondition(self, config, sent): + config("[safety]\nread_only = true\nrequire_preconditions = true\n") + error = error_of(set_status()) + assert error["reason"] == "read_only" + assert sent == [] + + def test_read_only_wins_when_the_flags_ask_for_both(self, sent): + error = error_of(set_status(flags=("--read-only", "--require-preconditions"))) + assert error["reason"] == "read_only" + + +class TestOwesIsDecidedPerCall: + """A synthetic command, so the rules are tested without a real command that has them.""" + + @pytest.fixture + def synthetic(self, monkeypatch): + def make(owes): + @click.command("zz-synthetic", cls=WriteCommand, owes=owes) + @click.option("--expect-hash", "expect_hash", default=None) + @click.option("--mode", default="free") + @click.option("--json", "as_json", is_flag=True) + @handle_connection_error + def zz(expect_hash, mode, as_json): + click.echo("body ran") + + monkeypatch.setitem(cli.commands, "zz-synthetic", zz) + return make + + def test_a_command_without_owes_never_owes_anything(self, synthetic, config): + synthetic(None) + config(ON) + result = invoke("zz-synthetic") + assert result.exit_code == 0 and "body ran" in result.stdout + + def test_owes_is_asked_with_the_parsed_parameters(self, synthetic, config): + seen = [] + + def owes(params): + seen.append(dict(params)) + return Owed(("--expect-hash",)) if params["mode"] == "change" else None + + synthetic(owes) + config(ON) + free = invoke("zz-synthetic", "--mode", "free") + assert free.exit_code == 0 and "body ran" in free.stdout + refused = invoke("zz-synthetic", "--mode", "change", "--json") + assert error_of(refused)["reason"] == "precondition_required" + assert "body ran" not in refused.stdout + assert seen[-1]["mode"] == "change" and seen[-1]["expect_hash"] is None + met = invoke("zz-synthetic", "--mode", "change", "--expect-hash", "abc") + assert met.exit_code == 0 and "body ran" in met.stdout + + def test_what_is_owed_is_not_asked_for_when_the_switch_is_off(self, synthetic): + synthetic(lambda params: Owed(("--expect-hash",))) + result = invoke("zz-synthetic") + assert result.exit_code == 0 and "body ran" in result.stdout + + def test_always_owes_without_the_switch(self, synthetic): + synthetic(lambda params: Owed(("--expect-hash",), always="--next")) + result = invoke("zz-synthetic", "--json") + error = error_of(result) + assert result.exit_code != 0 + assert error["reason"] == "precondition_required" + assert error["source"] == [] + assert "--next" in error["error"] + assert "require_preconditions" not in error["error"] + assert "body ran" not in result.stdout + + def test_always_is_met_by_the_option(self, synthetic): + synthetic(lambda params: Owed(("--expect-hash",), always="--next")) + result = invoke("zz-synthetic", "--expect-hash", "abc") + assert result.exit_code == 0 and "body ran" in result.stdout + + def test_always_still_gives_way_to_read_only(self, synthetic): + synthetic(lambda params: Owed(("--expect-hash",), always="--next")) + assert error_of(invoke("--read-only", "zz-synthetic", "--json"))["reason"] == "read_only" + + +def test_every_command_with_a_hash_option_carries_owes(): + """A command that takes ``--expect-hash`` or ``--expect-tree-hash`` and forgets + ``owes`` would stay free under the switch. ``add-journal-block`` is the one + exception: its upsert-heading mode owes after it has picked its block, in its body.""" + missing = [] + for name, command in sorted(cli.commands.items()): + options = {opt for p in command.params for opt in p.opts} + if options & {"--expect-hash", "--expect-tree-hash"}: + if getattr(command, "owes", None) is None: + missing.append(name) + assert missing == [] + + +class TestDoctor: + def _doctor(self, monkeypatch, *args): + from unittest.mock import MagicMock + api = MagicMock() + api.host, api.port, api.token = "127.0.0.1", "12315", "tok" + api.base_url = "http://127.0.0.1:12315/api" + api.call.return_value = {"currentGraph": "logseq_local_/graph"} + api.get_all_pages.return_value = [{"name": "a"}] + monkeypatch.setattr("logseq_cli.group.LogseqAPI", lambda **kwargs: api) + monkeypatch.setattr("logseq_cli.commands.meta._port_has_listener", lambda *a, **k: True) + result = invoke(*args, "doctor", "--json") + return {c["check"]: c["detail"] for c in json.loads(result.stdout)["checks"]}, result + + def test_it_shows_the_switch_and_where_it_comes_from(self, config, monkeypatch): + path = config(ON) + checks, _ = self._doctor(monkeypatch) + assert checks["require_preconditions"] == f"on (config {path})" + + def test_it_shows_the_flag_as_the_source(self, config, monkeypatch): + config("[safety]\nread_only = false\n") + checks, _ = self._doctor(monkeypatch, "--require-preconditions") + assert checks["require_preconditions"] == "on (flag)" + + def test_it_shows_off(self, config, monkeypatch): + path = config("[safety]\nread_only = false\n") + checks, _ = self._doctor(monkeypatch) + assert checks["require_preconditions"] == ( + f"off ([safety] require_preconditions = false in {path})") + + def test_a_broken_config_makes_it_unknown(self, config, monkeypatch): + config("[safety]\nrequire_preconditions = 'yes'\n") + checks, _ = self._doctor(monkeypatch) + assert checks["require_preconditions"].startswith( + "unknown, commands that write refuse until this is fixed:") + + +class TestUpdateBlockOwesAHash: + def test_it_refuses_without_one_before_a_request(self, config, sent): + config(ON) + result = invoke("update-block", "--id", TASK, "--content", "new", "--json") + error = error_of(result) + assert error["reason"] == "precondition_required" + assert error["options"] == ["--expect-hash"] + assert sent == [] + + def test_off_by_default(self, monkeypatch): + LogseqHttpDouble.installed(monkeypatch, {"Probe Page": [{"content": "note", "uuid": TASK}]}) + result = invoke("update-block", "--id", TASK, "--content", "new", "--json") + assert result.exit_code == 0, result.stderr diff --git a/tests/test_safety_keys.py b/tests/test_safety_keys.py new file mode 100644 index 0000000..8374edc --- /dev/null +++ b/tests/test_safety_keys.py @@ -0,0 +1,106 @@ +"""``[safety]`` is worked out per key: a second limit rides on the first one's machinery. + +The key list is widened here with an invented ``extra_limit``. Value check, +config/env/flag decision, the doctor line and ``init``'s copying of the table +must all pick it up without a line of their own; if one of them still names +``read_only``, the invented key is missed there. +""" +import json + +import click +import pytest + +try: # tomllib is stdlib from 3.11; 3.10 uses the tomli backport + import tomllib +except ModuleNotFoundError: # pragma: no cover - exercised on 3.10 only + import tomli as tomllib + +import logseq_cli.config +from logseq_cli.cli import cli +from logseq_cli.config import ConfigError, check_safety +from logseq_cli.safety import decide, describe +from tests.conftest import split_runner +from tests.logseq_http_double import LogseqHttpDouble + + +@pytest.fixture(autouse=True) +def two_keys(monkeypatch): + monkeypatch.setattr(logseq_cli.config, "SAFETY_KEYS", ("read_only", "extra_limit")) + monkeypatch.delenv("LOGSEQ_CLI_EXTRA_LIMIT", raising=False) + + +@pytest.fixture +def config(tmp_path, monkeypatch): + def write(text): + path = tmp_path / "config.toml" + path.write_text(text, encoding="utf-8") + monkeypatch.setenv("LOGSEQ_CLI_CONFIG", str(path)) + return path + return write + + +def invoke(*args): + return split_runner().invoke(cli, ["--token", "X", *args]) + + +class TestValueCheck: + def test_a_second_bool_key_is_accepted(self): + check_safety({"safety": {"extra_limit": True, "read_only": False}}) + + @pytest.mark.parametrize("value", ["yes", 1, "true"]) + def test_a_wrongly_typed_second_key_is_refused(self, value): + with pytest.raises(ConfigError, match=r"extra_limit must be true or false"): + check_safety({"safety": {"extra_limit": value}}) + + def test_the_first_key_is_still_checked(self): + with pytest.raises(ConfigError, match=r"read_only must be true or false"): + check_safety({"safety": {"read_only": "yes"}}) + + +class TestDecision: + def test_each_key_has_its_own_sources(self, monkeypatch): + monkeypatch.setenv("LOGSEQ_CLI_EXTRA_LIMIT", "1") + decision = decide({"safety": {"read_only": True}}) + assert decision.sources_of("read_only") == ("config",) + assert decision.sources_of("extra_limit") == ("env",) + + def test_config_and_flag_reach_the_second_key(self): + ctx = click.Context(cli, obj={"extra_limit_flag": True}) + with ctx: + decision = decide({"safety": {"extra_limit": True}}) + assert decision.sources_of("extra_limit") == ("config", "flag") + assert not decision.is_on("read_only") + + def test_an_unknown_env_value_counts_as_on_for_the_second_key(self, monkeypatch): + monkeypatch.setenv("LOGSEQ_CLI_EXTRA_LIMIT", "flase") + assert decide({}).is_on("extra_limit") + + def test_describe_names_the_key_and_the_source(self): + decision = decide({"safety": {"extra_limit": True}, "_path": "/c.toml"}) + assert describe(decision, "extra_limit") == "on (config /c.toml)" + assert describe(decision, "read_only") == "off ([safety] read_only = false in /c.toml)" + + +class TestDoctorAndInit: + def test_doctor_has_a_line_per_key(self, config, monkeypatch): + from unittest.mock import MagicMock + api = MagicMock() + api.host, api.port, api.token = "127.0.0.1", "12315", "tok" + api.base_url = "http://127.0.0.1:12315/api" + api.call.return_value = {"currentGraph": "logseq_local_/graph"} + api.get_all_pages.return_value = [{"name": "a"}] + monkeypatch.setattr("logseq_cli.group.LogseqAPI", lambda **kwargs: api) + monkeypatch.setattr("logseq_cli.commands.meta._port_has_listener", + lambda *a, **k: True) + config("[safety]\nextra_limit = true\n") + checks = {c["check"]: c["detail"] for c in + json.loads(invoke("doctor", "--json").stdout)["checks"]} + assert checks["extra_limit"].startswith("on (config ") + assert checks["read_only"].startswith("off (") + + def test_init_carries_the_second_key_into_a_new_file(self, config, monkeypatch): + LogseqHttpDouble().install(monkeypatch) + path = config("[safety]\nextra_limit = true\n") + result = invoke("init", "--force") + assert result.exit_code == 0, result.stderr + assert tomllib.loads(path.read_text(encoding="utf-8"))["safety"] == {"extra_limit": True} diff --git a/tests/test_update_block_precondition.py b/tests/test_update_block_precondition.py new file mode 100644 index 0000000..053bda9 --- /dev/null +++ b/tests/test_update_block_precondition.py @@ -0,0 +1,107 @@ +"""update-block --expect-hash refuses a block that is not what the caller read. + +Same double and real ``LogseqAPI`` as the other write tests. +""" +import json + +import pytest + +from logseq_cli.blocktext import block_hash +from logseq_cli.cli import cli +from tests.conftest import split_runner +from tests.logseq_http_double import LogseqHttpDouble + +URL = "http://127.0.0.1:12315/api" +TARGET = "00000000-0000-4000-8000-0000000000a1" +OTHER = "00000000-0000-4000-8000-0000000000b2" +READ = "plain note" +RIGHT = block_hash(READ) +WRONG = "0123456789ab" + + +@pytest.fixture +def double(monkeypatch): + return LogseqHttpDouble.installed(monkeypatch, { + "Probe Page": [{"content": READ, "uuid": TARGET}, + {"content": "referenced elsewhere", "uuid": OTHER}]}) + + +def run(*args): + return split_runner().invoke(cli, ["--token", "t", "update-block", *args, "--json"]) + + +def content(double, uuid=TARGET): + return double.post(URL, json={"method": "logseq.Editor.getBlock", "args": [uuid, {}]}, + headers={"Authorization": "Bearer t"}, timeout=30).json()["content"] + + +@pytest.mark.parametrize("extra", [[], ["--dry-run"]], ids=["write", "dry-run"]) +def test_a_wrong_hash_writes_nothing(double, extra): + before = double.snapshot() + result = run("--id", TARGET, "--content", "new", "--expect-hash", WRONG, *extra) + assert result.exit_code != 0 and result.stdout == "" + assert json.loads(result.stderr)["reason"] == "precondition_failed" + assert double.writes() == [] + assert double.snapshot() == before + + +def test_a_wrong_hash_with_a_ref_gives_the_target_no_id(double): + """The refusal comes before the write that would store the ref target's id.""" + before = double.snapshot() + result = run("--id", TARGET, "--content", f"see (({OTHER}))", "--expect-hash", WRONG) + assert json.loads(result.stderr)["reason"] == "precondition_failed" + assert double.sent("setBlocksId") == [] + assert double.snapshot() == before + + +def test_a_wrong_hash_is_refused_with_where_content(double): + result = run("--where-content", READ, "--content", "new", "--expect-hash", WRONG) + assert json.loads(result.stderr)["reason"] == "precondition_failed" + assert double.writes() == [] + + +def test_the_right_hash_writes(double): + result = run("--id", TARGET, "--content", "new", "--expect-hash", RIGHT) + assert result.exit_code == 0, result.stderr + assert content(double) == "new" + + +def test_the_right_hash_with_where_content_writes(double): + result = run("--where-content", READ, "--content", "new", "--expect-hash", RIGHT) + assert result.exit_code == 0, result.stderr + assert content(double) == "new" + + +def test_a_change_after_the_check_is_block_changed(double, monkeypatch): + original = double.post + reads = [] + + def post(url, json=None, **kwargs): + answer = original(url, json=json, **kwargs) + if json["method"] == "logseq.Editor.getBlock": + reads.append(True) + if len(reads) == 2: # the check's read past the cache + original(url, json={"method": "logseq.Editor.updateBlock", + "args": [TARGET, "changed elsewhere"]}, + headers={"Authorization": "Bearer t"}, timeout=30) + return answer + + monkeypatch.setattr("logseq_cli.api.requests.post", post) + result = run("--id", TARGET, "--content", "new", "--expect-hash", RIGHT) + assert json.loads(result.stderr)["reason"] == "block_changed" + assert content(double) == "changed elsewhere" + + +def test_without_a_precondition_the_write_is_not_held_to_a_text(double, monkeypatch): + """Behaviour unchanged: update_block gets no text to compare.""" + import logseq_cli.api as api_module + seen = [] + original = api_module.LogseqAPI.update_block + + def spy(self, *args, **kwargs): + seen.append(kwargs) + return original(self, *args, **kwargs) + + monkeypatch.setattr(api_module.LogseqAPI, "update_block", spy) + assert run("--id", TARGET, "--content", "new").exit_code == 0 + assert seen and seen[0].get("expect") is None and seen[0].get("replacing") is None diff --git a/tests/test_upsert_heading_choice.py b/tests/test_upsert_heading_choice.py new file mode 100644 index 0000000..b39a7bb --- /dev/null +++ b/tests/test_upsert_heading_choice.py @@ -0,0 +1,95 @@ +"""Which block ``--upsert-heading`` replaces is chosen once, first, and fresh. + +The choice used to follow the creation of the journal page and the heading, +came from a cached read, and in a preview happened only with +``[graph] keep_empty_blocks_last`` on. A choice made later or from a stale +read can overwrite another block than the one the user saw; a preview that +chooses differently from the run shows something the run does not do. + +Run through the real ``LogseqAPI`` against the HTTP double. +""" +import json + +import pytest + +import logseq_cli.commands.journal as journal +from logseq_cli.api import LogseqAPI +from logseq_cli.cli import cli +from tests.conftest import split_runner +from tests.logseq_http_double import LogseqHttpDouble + +JOURNAL = "2099-01-05, Monday" +ENV = "LOGSEQ_CLI_KEEP_EMPTY_BLOCKS_LAST" +READS = ("logseq.Editor.getPageBlocksTree", "logseq.Editor.getBlock") + +UPSERT = ["add-journal-block", "--date", "2099-01-05", "--under-heading", "## Log", + "--upsert-heading", "### Carol", "--content", "### Carol\nnew"] + + +def run(*args): + return split_runner().invoke(cli, ["--token", "t", *args, "--json"]) + + +def log_with(*children): + return [{"content": "## Log", "children": list(children)}] + + +@pytest.fixture(autouse=True) +def _keep_off(monkeypatch): + monkeypatch.delenv(ENV, raising=False) + monkeypatch.setenv("LOGSEQ_CLI_CACHE_TTL", "60") + + +def test_the_choice_does_not_come_from_the_cache(monkeypatch): + double = LogseqHttpDouble.installed( + monkeypatch, {JOURNAL: log_with("### Carol\nold")}) + real_call = LogseqAPI.call + + def stale(self, method, args=None, *, cached=True): + answer = real_call(self, method, args, cached=cached) + if cached and method in READS and not double.writes(): + # What the cache would still hold before the write: the block + # under another name. + return json.loads(json.dumps(answer).replace("Carol", "Dave")) + return answer + + monkeypatch.setattr(LogseqAPI, "call", stale) + r = run(*UPSERT) + assert r.exit_code == 0, r.stderr + assert "(updated)" in json.loads(r.stdout)["position"] + assert double.uuid_of("### Carol\nnew") + + +def test_the_preview_chooses_the_way_the_run_does_with_the_setting_off(monkeypatch): + LogseqHttpDouble.installed(monkeypatch, {JOURNAL: log_with("### Carol\nold")}) + chosen = [] + real_choice = journal.find_upsert_target + + def record(*args): + chosen.append((args[1:], real_choice(*args))) + return chosen[-1][1] + + monkeypatch.setattr(journal, "find_upsert_target", record) + assert run(*UPSERT, "--dry-run").exit_code == 0 + assert run(*UPSERT).exit_code == 0 + preview, live = chosen + assert preview == live + assert preview[1][1] is not None + + +@pytest.mark.parametrize("graph", [{}, {JOURNAL: ["top"]}], + ids=["no journal page", "no heading"]) +def test_nothing_is_written_when_the_choice_is_made(monkeypatch, graph): + double = LogseqHttpDouble.installed(monkeypatch, graph) + written_before = [] + real_choice = journal.find_upsert_target + + def record(*args): + written_before.append(list(double.writes())) + return real_choice(*args) + + monkeypatch.setattr(journal, "find_upsert_target", record) + r = run(*UPSERT) + assert r.exit_code == 0, r.stderr + assert written_before == [[]] + assert double.uuid_of("### Carol\nnew") diff --git a/tests/test_upsert_heading_expect_hash.py b/tests/test_upsert_heading_expect_hash.py new file mode 100644 index 0000000..3846a8d --- /dev/null +++ b/tests/test_upsert_heading_expect_hash.py @@ -0,0 +1,160 @@ +"""add-journal-block --upsert-heading --expect-hash: the precondition is for the +block the upsert replaces. With no match there is no block and no hash: the +precondition has no object, and the block is created. Under +``require_preconditions`` it is owed only when a block matches. + +Run through the real ``LogseqAPI`` against the HTTP double. +""" +import json + +import pytest + +import logseq_cli.api as api_module +from logseq_cli.blocktext import block_hash +from logseq_cli.cli import cli +from tests.conftest import split_runner +from tests.logseq_http_double import LogseqHttpDouble + +JOURNAL = "2099-01-05, Monday" +OLD = "### Carol\nold" +RIGHT = block_hash(OLD) +WRONG = "0123456789ab" +ON = "[safety]\nrequire_preconditions = true\n" + +UPSERT = ["add-journal-block", "--date", "2099-01-05", "--under-heading", "## Log", + "--upsert-heading", "### Carol", "--content", "### Carol\nnew"] + + +def run(*args): + return split_runner().invoke(cli, ["--token", "t", *args, "--json"]) + + +def log_with(*children): + return [{"content": "## Log", "children": list(children)}] + + +@pytest.fixture(autouse=True) +def _env(monkeypatch, tmp_path): + monkeypatch.delenv("LOGSEQ_CLI_KEEP_EMPTY_BLOCKS_LAST", raising=False) + path = tmp_path / "off.toml" + path.write_text("[safety]\nread_only = false\n", encoding="utf-8") + monkeypatch.setenv("LOGSEQ_CLI_CONFIG", str(path)) + + +@pytest.fixture +def switch_on(tmp_path, monkeypatch): + path = tmp_path / "config.toml" + path.write_text(ON, encoding="utf-8") + monkeypatch.setenv("LOGSEQ_CLI_CONFIG", str(path)) + + +@pytest.fixture +def double(monkeypatch): + return LogseqHttpDouble.installed(monkeypatch, {JOURNAL: log_with(OLD)}) + + +@pytest.mark.parametrize("extra", [[], ["--dry-run"]], ids=["write", "dry-run"]) +def test_a_wrong_hash_on_a_match_writes_nothing(double, extra): + before = double.snapshot() + result = run(*UPSERT, "--expect-hash", WRONG, *extra) + assert result.exit_code != 0 and result.stdout == "" + assert json.loads(result.stderr)["reason"] == "precondition_failed" + assert double.writes() == [] + assert double.snapshot() == before + + +def test_a_hash_with_no_journal_page_has_no_object(monkeypatch): + double = LogseqHttpDouble.installed(monkeypatch, {"Other": ["x"]}) + # No page, so no match: nothing to refuse, the page is made. + result = run(*UPSERT, "--expect-hash", WRONG) + assert result.exit_code == 0, result.stderr + assert double.uuid_of("### Carol\nnew") + + +def test_the_right_hash_replaces_the_block(double): + result = run(*UPSERT, "--expect-hash", RIGHT) + assert result.exit_code == 0, result.stderr + assert "(updated)" in json.loads(result.stdout)["position"] + assert double.uuid_of("### Carol\nnew") + + +def test_no_match_creates_the_block_and_says_the_hash_has_no_object(monkeypatch): + double = LogseqHttpDouble.installed(monkeypatch, {JOURNAL: log_with("### Dave\nx")}) + result = run(*UPSERT, "--expect-hash", WRONG) + assert result.exit_code == 0, result.stderr + assert "(created)" in json.loads(result.stdout)["position"] + assert "no object" in result.stderr + assert double.uuid_of("### Carol\nnew") + + +def test_no_match_creates_under_the_switch_without_an_option(monkeypatch, switch_on): + double = LogseqHttpDouble.installed(monkeypatch, {JOURNAL: log_with("### Dave\nx")}) + result = run(*UPSERT) + assert result.exit_code == 0, result.stderr + assert "(created)" in json.loads(result.stdout)["position"] + assert double.uuid_of("### Carol\nnew") + + +def test_no_page_creates_under_the_switch_without_an_option(monkeypatch, switch_on): + double = LogseqHttpDouble.installed(monkeypatch, {}) + result = run(*UPSERT) + assert result.exit_code == 0, result.stderr + assert double.uuid_of("### Carol\nnew") + + +@pytest.mark.parametrize("extra", [[], ["--dry-run"]], ids=["write", "dry-run"]) +def test_a_match_under_the_switch_without_an_option_is_required(double, switch_on, extra): + before = double.snapshot() + result = run(*UPSERT, *extra) + assert result.exit_code != 0 and result.stdout == "" + error = json.loads(result.stderr) + assert error["reason"] == "precondition_required" + assert error["options"] == ["--expect-hash"] + assert "require_preconditions" in error["error"] + assert double.writes() == [] + assert double.snapshot() == before + + +def test_a_match_under_the_switch_with_the_right_hash_writes(double, switch_on): + result = run(*UPSERT, "--expect-hash", RIGHT) + assert result.exit_code == 0, result.stderr + assert double.uuid_of("### Carol\nnew") + + +def test_read_only_wins_over_the_missing_option(double, switch_on): + result = run("--read-only", *UPSERT) + assert json.loads(result.stderr)["reason"] == "read_only" + + +def test_a_hash_without_upsert_heading_is_a_usage_error(double): + result = run("add-journal-block", "--date", "2099-01-05", "--content", "x", + "--expect-hash", RIGHT) + assert result.exit_code == 2 + assert "--upsert-heading" in result.stderr + assert double.writes() == [] + + +def test_the_write_is_held_to_the_text_the_hash_was_checked_on(double, monkeypatch): + seen = [] + original = api_module.LogseqAPI.update_block + + def spy(self, *args, **kwargs): + seen.append(kwargs) + return original(self, *args, **kwargs) + + monkeypatch.setattr(api_module.LogseqAPI, "update_block", spy) + assert run(*UPSERT, "--expect-hash", RIGHT).exit_code == 0 + assert seen[0].get("expect") == OLD + + +def test_without_a_hash_the_write_is_not_held_to_a_text(double, monkeypatch): + seen = [] + original = api_module.LogseqAPI.update_block + + def spy(self, *args, **kwargs): + seen.append(kwargs) + return original(self, *args, **kwargs) + + monkeypatch.setattr(api_module.LogseqAPI, "update_block", spy) + assert run(*UPSERT).exit_code == 0 + assert seen and seen[0].get("expect") is None diff --git a/tests/test_write_against_changed_block.py b/tests/test_write_against_changed_block.py index ae1617f..0183896 100644 --- a/tests/test_write_against_changed_block.py +++ b/tests/test_write_against_changed_block.py @@ -15,6 +15,7 @@ import pytest from logseq_cli.api import BlockChanged, LogseqAPI +from logseq_cli.blocktext import SplitBlockError, block_hash from logseq_cli.cli import cli from tests.conftest import split_runner from tests.logseq_http_double import LogseqHttpDouble @@ -99,3 +100,55 @@ def post(url, json=None, **kwargs): assert json.loads(result.stderr)["reason"] == "block_changed" assert _content(double) == FOREIGN assert len(double.sent("updateBlock")) == 1 + + +SPLIT = "TODO x\n- looks like a block of its own" + + +def test_expect_refuses_a_block_changed_since_the_check(double, api): + _update(double, FOREIGN) + with pytest.raises(BlockChanged) as refused: + api.update_block(UUID, "DONE x", expect="TODO x") + assert refused.value.reason == "block_changed" + assert _content(double) == FOREIGN + assert len(double.sent("updateBlock")) == 1, "only the change made elsewhere was written" + + +def test_expect_writes_a_block_as_it_was_checked(double, api): + api.update_block(UUID, "DONE x", expect="TODO x") + assert _content(double) == "DONE x" + + +def test_expect_gives_a_split_line_no_pass_even_when_the_block_holds_it(double, api): + """Unlike ``replacing``, ``expect`` loosens nothing in the check on the text.""" + _update(double, SPLIT) + changed = SPLIT.replace("TODO", "DONE") + with pytest.raises(SplitBlockError): + api.update_block(UUID, changed, expect=SPLIT) + assert _content(double) == SPLIT + api.update_block(UUID, changed, replacing=SPLIT) + assert _content(double) == changed + + +def test_set_todo_status_with_a_precondition_still_holds_the_block_to_what_it_read( + double, monkeypatch): + """A change between the precondition check and the write is block_changed.""" + original = double.post + reads = [] + + def post(url, json=None, **kwargs): + answer = original(url, json=json, **kwargs) + if json["method"] == "logseq.Editor.getBlock": + reads.append(True) + # 1: the command's lookup, 2: the check's read past the cache. + if len(reads) == 2: + _update(double, FOREIGN) + return answer + + monkeypatch.setattr("logseq_cli.api.requests.post", post) + result = split_runner().invoke(cli, [ + "--token", "t", "set-todo-status", "--id", UUID, "--status", "DONE", + "--expect-hash", block_hash("TODO x"), "--json"]) + assert result.exit_code != 0 + assert json.loads(result.stderr)["reason"] == "block_changed" + assert _content(double) == FOREIGN diff --git a/tests/test_write_expect_hash_decisions.py b/tests/test_write_expect_hash_decisions.py new file mode 100644 index 0000000..3a2eecc --- /dev/null +++ b/tests/test_write_expect_hash_decisions.py @@ -0,0 +1,113 @@ +"""--expect-hash on the four writes that act on a block the caller read: +set-block-property, remove-property --id, move-block, copy-block --remove. + +A right hash lets the call through, a wrong one refuses before the first write +(copy-block --remove: before the first copy). A call with no block to read +(remove-property --page, copy-block without --remove) takes no precondition +and refuses the option as a usage error instead of ignoring it. Under +``require_preconditions`` the four owe one; the free calls do not. +""" +import json + +import pytest + +from logseq_cli.blocktext import block_hash +from logseq_cli.cli import cli +from tests.conftest import split_runner +from tests.logseq_http_double import LogseqHttpDouble + +TASK = "00000000-0000-4000-8000-0000000000a1" +OTHER = "00000000-0000-4000-8000-0000000000a2" +READ = "TODO ship the parser\nprio:: high" +RIGHT = block_hash(READ) +WRONG = "0123456789ab" + +CALLS = { + "set-block-property": ["set-block-property", "--id", TASK, "--key", "prio", "--value", "low"], + "remove-property": ["remove-property", "--id", TASK, "--key", "prio"], + "move-block": ["move-block", "--id", TASK, "--under", OTHER], + "copy-block": ["copy-block", "--id", TASK, "--to-page", "Other Page", "--remove"], +} + + +@pytest.fixture +def double(monkeypatch): + return LogseqHttpDouble.installed(monkeypatch, { + "Probe Page": [{"content": READ, "marker": "TODO", "uuid": TASK, + "children": [{"content": "a child"}]}, + {"content": "the new parent", "uuid": OTHER}], + "Other Page": [{"content": "already here"}]}) + + +def run(*args): + return split_runner().invoke(cli, ["--token", "t", *args, "--json"]) + + +@pytest.mark.parametrize("name", CALLS) +def test_the_hash_that_was_read_lets_the_call_through(double, name): + result = run(*CALLS[name], "--expect-hash", RIGHT) + assert result.exit_code == 0, result.stderr + assert double.writes() != [] + + +@pytest.mark.parametrize("extra", [[], ["--dry-run"]], ids=["write", "dry-run"]) +@pytest.mark.parametrize("name", CALLS) +def test_another_hash_refuses_without_a_write(double, name, extra): + before = double.snapshot() + result = run(*CALLS[name], "--expect-hash", WRONG, *extra) + assert result.exit_code != 0 and result.stdout == "" + error = json.loads(result.stderr) + assert error["reason"] == "precondition_failed" + assert error["expected_hash"] == WRONG + assert RIGHT not in result.stderr + assert double.writes() == [] + assert double.snapshot() == before + + +def test_copy_block_with_a_wrong_hash_makes_no_copy(double): + run(*CALLS["copy-block"], "--expect-hash", WRONG) + assert double.sent("logseq.Editor.insertBlock") == [] + assert double.sent("logseq.Editor.appendBlockInPage") == [] + assert double.tree("Other Page") == [("already here", [])] + + +class TestOwedUnderTheSwitch: + @pytest.fixture(autouse=True) + def on(self, monkeypatch): + monkeypatch.setenv("LOGSEQ_CLI_REQUIRE_PRECONDITIONS", "true") + + @pytest.mark.parametrize("name", CALLS) + def test_the_call_refuses_without_one(self, double, name): + result = run(*CALLS[name]) + error = json.loads(result.stderr) + assert result.exit_code != 0 + assert error["reason"] == "precondition_required" + assert error["options"] == ["--expect-hash"] + assert double.writes() == [] + + @pytest.mark.parametrize("name", CALLS) + def test_the_hash_satisfies_it(self, double, name): + assert run(*CALLS[name], "--expect-hash", RIGHT).exit_code == 0 + + def test_remove_property_on_a_page_is_free(self, double): + double.add_page("Props Page", [{"content": "prio:: high"}]) + result = run("remove-property", "--page", "Props Page", "--key", "prio") + assert result.exit_code == 0, result.stderr + + def test_copy_block_without_remove_is_free(self, double): + result = run("copy-block", "--id", TASK, "--to-page", "Other Page") + assert result.exit_code == 0, result.stderr + + +class TestWithoutABlockToRead: + @pytest.mark.parametrize("args", [ + ["remove-property", "--page", "Probe Page", "--key", "prio", "--expect-hash", RIGHT], + ["copy-block", "--id", TASK, "--to-page", "Other Page", "--expect-hash", RIGHT], + ], ids=["remove-property-page", "copy-block-without-remove"]) + def test_the_option_is_a_usage_error(self, double, args): + before = double.snapshot() + result = split_runner().invoke(cli, ["--token", "t", *args]) + assert result.exit_code == 2 + assert "--expect-hash" in result.stderr + assert double.writes() == [] + assert double.snapshot() == before diff --git a/tests/test_write_expect_marker.py b/tests/test_write_expect_marker.py new file mode 100644 index 0000000..d5c11e7 --- /dev/null +++ b/tests/test_write_expect_marker.py @@ -0,0 +1,138 @@ +"""set-todo-status --expect-marker refuses a block whose marker is not the one the caller read. + +Same double and real ``LogseqAPI`` as the other write tests. +""" +import json + +import pytest + +from logseq_cli.blocktext import block_hash +from logseq_cli.cli import cli +from logseq_cli.tasks import ORDER +from tests.conftest import split_runner +from tests.logseq_http_double import LogseqHttpDouble + +URL = "http://127.0.0.1:12315/api" +TASK = "00000000-0000-4000-8000-0000000000a1" +PLAIN = "00000000-0000-4000-8000-0000000000a3" +READ = "TODO ship the parser" +RIGHT_HASH = block_hash(READ) +WRONG_HASH = "0123456789ab" + + +@pytest.fixture +def double(monkeypatch): + return LogseqHttpDouble.installed(monkeypatch, { + "Probe Page": [{"content": READ, "marker": "TODO", "uuid": TASK}, + {"content": "just a note", "uuid": PLAIN}]}) + + +def run(*args): + return split_runner().invoke(cli, ["--token", "t", "set-todo-status", *args, "--json"]) + + +def content(double, uuid=TASK): + return double.post(URL, json={"method": "logseq.Editor.getBlock", "args": [uuid, {}]}, + headers={"Authorization": "Bearer t"}, timeout=30).json()["content"] + + +def refusal(result): + assert result.exit_code != 0 + assert result.stdout == "" + return json.loads(result.stderr) + + +@pytest.mark.parametrize("given", ["TODO", "todo", "Todo"]) +def test_the_marker_it_has_writes_whatever_the_case(double, given): + result = run("--id", TASK, "--status", "DONE", "--expect-marker", given) + assert result.exit_code == 0, result.stderr + assert content(double) == "DONE ship the parser" + + +@pytest.mark.parametrize("extra", [[], ["--dry-run"]], ids=["write", "dry-run"]) +def test_another_marker_writes_nothing(double, extra): + before = double.snapshot() + error = refusal(run("--id", TASK, "--status", "DONE", "--expect-marker", "DOING", *extra)) + assert error["reason"] == "precondition_failed" + assert error["expected_marker"] == "DOING" + assert error["actual_marker"] == "TODO" + assert "expected_hash" not in error + assert double.writes() == [] + assert double.snapshot() == before + + +def test_another_marker_is_refused_where_nothing_would_change(double): + error = refusal(run("--id", TASK, "--status", "TODO", "--expect-marker", "DONE")) + assert error["reason"] == "precondition_failed" + + +def test_none_on_a_block_with_a_marker_is_refused(double): + error = refusal(run("--id", TASK, "--status", "DONE", "--expect-marker", "none")) + assert error["reason"] == "precondition_failed" + assert error["expected_marker"] == "NONE" + assert error["actual_marker"] == "TODO" + assert double.writes() == [] + + +def test_a_marker_on_a_block_without_one_reports_null(double): + error = refusal(run("--id", PLAIN, "--status", "DONE", "--expect-marker", "TODO")) + assert error["reason"] == "precondition_failed" + assert error["actual_marker"] is None + assert "marker is none" in error["error"] + + +def test_none_on_a_block_without_a_marker_passes_the_check(double): + """The check passes; the command then refuses the block as no task, as it always does.""" + error = refusal(run("--id", PLAIN, "--status", "DONE", "--expect-marker", "none")) + assert error["reason"] != "precondition_failed" + + +def test_the_marker_and_the_hash_must_both_match(double): + ok = run("--id", TASK, "--status", "DONE", "--expect-marker", "TODO", + "--expect-hash", RIGHT_HASH, "--dry-run") + assert ok.exit_code == 0, ok.stderr + + bad_hash = refusal(run("--id", TASK, "--status", "DONE", "--expect-marker", "TODO", + "--expect-hash", WRONG_HASH)) + assert bad_hash["expected_hash"] == WRONG_HASH + assert bad_hash["expected_marker"] == "TODO" + + bad_marker = refusal(run("--id", TASK, "--status", "DONE", "--expect-marker", "DOING", + "--expect-hash", RIGHT_HASH)) + assert bad_marker["expected_marker"] == "DOING" + assert double.writes() == [] + + +def test_the_marker_is_read_past_the_cache(double, monkeypatch): + original = double.post + seen = [] + + def post(url, json=None, **kwargs): + answer = original(url, json=json, **kwargs) + if json["method"] == "logseq.Editor.getBlock" and not seen: + seen.append(True) + original(url, json={"method": "logseq.Editor.updateBlock", + "args": [TASK, "CANCELED ship the parser"]}, **kwargs) + return answer + + monkeypatch.setattr("logseq_cli.api.requests.post", post) + error = refusal(run("--id", TASK, "--status", "DONE", "--expect-marker", "TODO")) + assert error["reason"] == "precondition_failed" + assert error["actual_marker"] == "CANCELED" + assert content(double) == "CANCELED ship the parser" + + +@pytest.mark.parametrize("marker", ORDER) +def test_every_marker_of_the_task_model_is_accepted(double, marker): + result = run("--id", TASK, "--status", "DONE", "--expect-marker", marker, "--dry-run") + if result.exit_code: + assert json.loads(result.stderr)["reason"] == "precondition_failed" + + +def test_an_unknown_marker_is_a_usage_error_naming_the_known_ones(double): + result = run("--id", TASK, "--status", "DONE", "--expect-marker", "MAYBE") + assert result.exit_code == 2 + assert double.writes() == [] + for marker in ORDER: + assert marker in result.stderr + assert "none" in result.stderr diff --git a/tests/test_write_expect_tree_hash.py b/tests/test_write_expect_tree_hash.py new file mode 100644 index 0000000..5bbfaca --- /dev/null +++ b/tests/test_write_expect_tree_hash.py @@ -0,0 +1,102 @@ +"""remove-block --expect-tree-hash refuses a block whose subtree is not the one the caller read. + +The hash of the block alone would still match after a child was added, and the +removal would take the new child along. Same double and real ``LogseqAPI`` as +the other write tests. +""" +import json + +import pytest + +from logseq_cli.cli import cli +from tests.conftest import split_runner +from tests.logseq_http_double import LogseqHttpDouble + + +@pytest.fixture +def double(monkeypatch): + return LogseqHttpDouble.installed(monkeypatch, { + "Probe Page": [{"content": "parent", "children": [{"content": "child"}]}, + {"content": "other"}]}) + + +def run(*args): + return split_runner().invoke(cli, ["--token", "t", *args]) + + +def read(double, content="parent"): + result = run("get-block", "--id", double.uuid_of(content), "--json") + assert result.exit_code == 0, result.stderr + return json.loads(result.stdout) + + +def test_an_unchanged_subtree_is_removed(double): + block = read(double) + result = run("remove-block", "--id", block["uuid"], "--expect-tree-hash", block["tree_hash"]) + assert result.exit_code == 0, result.stderr + assert double.tree("Probe Page") == [("other", [])] + + +def test_a_child_added_since_the_read_refuses_without_a_write(double): + block = read(double) + added = run("insert-block", "--child-of", block["uuid"], "--content", "new child") + assert added.exit_code == 0, added.stderr + before = double.snapshot() + writes = len(double.writes()) + result = run("remove-block", "--id", block["uuid"], "--expect-tree-hash", block["tree_hash"], "--json") + assert result.exit_code != 0 and result.stdout == "" + error = json.loads(result.stderr) + assert error["reason"] == "precondition_failed" + assert error["expected_tree_hash"] == block["tree_hash"] + assert "expected_hash" not in error + assert "actual_tree_hash" not in error and block["tree_hash"] not in error["error"] + assert len(double.writes()) == writes + assert double.snapshot() == before + + +def test_the_hash_of_the_block_alone_does_not_stand_in_for_the_tree_hash(double): + block = read(double) + result = run("remove-block", "--id", block["uuid"], "--expect-tree-hash", block["hash"], "--json") + assert result.exit_code != 0 + assert json.loads(result.stderr)["reason"] == "precondition_failed" + + +def test_dry_run_checks_too(double): + block = read(double) + result = run("remove-block", "--id", block["uuid"], "--expect-tree-hash", "0123456789ab", + "--dry-run", "--json") + assert result.exit_code != 0 + assert json.loads(result.stderr)["reason"] == "precondition_failed" + + +def test_remove_block_has_no_expect_hash(double): + block = read(double) + result = run("remove-block", "--id", block["uuid"], "--expect-hash", block["hash"]) + assert result.exit_code == 2 + assert double.writes() == [] + + +def test_delete_block_takes_the_same_option(double): + block = read(double) + run("insert-block", "--child-of", block["uuid"], "--content", "new child") + result = run("delete-block", "--id", block["uuid"], "--expect-tree-hash", block["tree_hash"], "--json") + assert result.exit_code != 0 + assert json.loads(result.stderr)["reason"] == "precondition_failed" + + +def test_a_missing_block_is_block_not_found(double): + result = run("remove-block", "--id", "00000000-0000-4000-8000-0000000000ff", + "--expect-tree-hash", "0123456789ab", "--json") + assert result.exit_code != 0 + assert json.loads(result.stderr)["reason"] == "block_not_found" + + +@pytest.mark.parametrize("name", ["remove-block", "delete-block"]) +def test_the_switch_owes_the_tree_hash(double, name): + block = read(double) + result = run("--require-preconditions", name, "--id", block["uuid"], "--json") + assert result.exit_code != 0 + error = json.loads(result.stderr) + assert error["reason"] == "precondition_required" + assert error["options"] == ["--expect-tree-hash"] + assert double.writes() == [] diff --git a/tests/test_write_precondition.py b/tests/test_write_precondition.py new file mode 100644 index 0000000..6e60145 --- /dev/null +++ b/tests/test_write_precondition.py @@ -0,0 +1,160 @@ +"""set-todo-status --expect-hash refuses a block that is not what the caller read. + +Same double and real ``LogseqAPI`` as the other write tests, so the cache, the +re-read and the write proof run as they do for a user. +""" +import json + +import pytest + +from logseq_cli.blocktext import block_hash +from logseq_cli.cli import cli +from tests.conftest import split_runner +from tests.logseq_http_double import LogseqHttpDouble + +URL = "http://127.0.0.1:12315/api" +TASK = "00000000-0000-4000-8000-0000000000a1" +HOST = "11111111-0000-4000-8000-0000000000a2" +READ = "TODO ship the parser" +RIGHT = block_hash(READ) +WRONG = "0123456789ab" +NOTHING = "Nothing was written." + + +@pytest.fixture +def double(monkeypatch): + return LogseqHttpDouble.installed(monkeypatch, { + "Probe Page": [{"content": READ, "marker": "TODO", "uuid": TASK}, + {"content": f"(({TASK}))", "uuid": HOST}]}) + + +def _request(double, method, *args): + return double.post(URL, json={"method": method, "args": list(args)}, + headers={"Authorization": "Bearer t"}, timeout=30) + + +def run(*args): + return split_runner().invoke(cli, ["--token", "t", "set-todo-status", *args, "--json"]) + + +def content(double, uuid=TASK): + return _request(double, "logseq.Editor.getBlock", uuid, {}).json()["content"] + + +def refusal(result): + assert result.exit_code != 0 + assert result.stdout == "" + return json.loads(result.stderr) + + +@pytest.mark.parametrize("extra", [[], ["--dry-run"]], ids=["write", "dry-run"]) +def test_a_wrong_hash_writes_nothing(double, extra): + before = double.snapshot() + error = refusal(run("--id", TASK, "--status", "DONE", "--expect-hash", WRONG, *extra)) + assert error["reason"] == "precondition_failed" + assert double.writes() == [] + assert double.snapshot() == before + + +def test_a_wrong_hash_is_refused_where_nothing_would_change(double): + """The status the block already has is a no-op; the check comes first.""" + error = refusal(run("--id", TASK, "--status", "TODO", "--expect-hash", WRONG)) + assert error["reason"] == "precondition_failed" + + +def test_the_right_hash_writes(double): + result = run("--id", TASK, "--status", "DONE", "--expect-hash", RIGHT) + assert result.exit_code == 0, result.stderr + assert content(double) == "DONE ship the parser" + + +def test_the_right_hash_keeps_dry_run_and_no_op(double): + dry = run("--id", TASK, "--status", "DONE", "--expect-hash", RIGHT, "--dry-run") + assert dry.exit_code == 0 and json.loads(dry.stdout)["dry_run"] is True + same = run("--id", TASK, "--status", "TODO", "--expect-hash", RIGHT) + assert same.exit_code == 0 and json.loads(same.stdout)["status"] == "unchanged" + assert double.writes() == [] + + +def test_the_hash_may_be_typed_in_capitals(double): + assert run("--id", TASK, "--status", "DONE", "--expect-hash", RIGHT.upper()).exit_code == 0 + + +def test_a_selection_by_text_with_a_wrong_hash_is_refused_though_the_text_fits(double): + error = refusal(run("--content", "ship the parser", "--page", "Probe Page", + "--status", "DONE", "--expect-hash", WRONG)) + assert error["reason"] == "precondition_failed" + assert content(double) == READ + + +def test_a_selection_by_text_with_the_right_hash_writes(double): + result = run("--content", "ship the parser", "--page", "Probe Page", + "--status", "DONE", "--expect-hash", RIGHT) + assert result.exit_code == 0, result.stderr + assert content(double) == "DONE ship the parser" + + +def test_follow_refs_checks_the_target_not_the_host(double): + error = refusal(run("--id", HOST, "--status", "DONE", "--follow-refs", + "--expect-hash", block_hash(f"(({TASK}))"))) + assert error["reason"] == "precondition_failed" + assert TASK[:8] in error["error"], "the message names the target" + assert HOST[:8] not in error["error"] + assert double.writes() == [] + + +def test_follow_refs_with_the_hash_of_the_target_writes(double): + result = run("--id", HOST, "--status", "DONE", "--follow-refs", "--expect-hash", RIGHT) + assert result.exit_code == 0, result.stderr + assert content(double) == "DONE ship the parser" + assert content(double, HOST) == f"(({TASK}))" + + +def test_the_refusal_names_marker_and_first_line_and_never_the_current_hash(double): + _request(double, "logseq.Editor.updateBlock", TASK, "DOING ship it\nnote") + result = run("--id", TASK, "--status", "DONE", "--expect-hash", RIGHT) + error = refusal(result) + assert error["expected_hash"] == RIGHT + assert error["actual_marker"] == "DOING" + assert error["first_line"] == "DOING ship it" + assert "DOING" in error["error"] and "ship it" in error["error"] + assert "actual_hash" not in error + assert block_hash(content(double)) not in result.stderr + assert error["error"].count(NOTHING) == 1 + assert error["writes_landed"] == 0 + + +def _after_first_block_read(double, monkeypatch, change): + """Run ``change`` right after the command's first getBlock answered.""" + original = double.post + seen = [] + + def post(url, json=None, **kwargs): + answer = original(url, json=json, **kwargs) + if json["method"] == "logseq.Editor.getBlock" and not seen: + seen.append(True) + change() + return answer + + monkeypatch.setattr("logseq_cli.api.requests.post", post) + + +def test_the_check_reads_past_the_cache(double, monkeypatch): + """The first read fills the cache; a change lands; the check has to see it. + Read from the cache it would pass, and only the re-read right before the + write (block_changed) would stop the command.""" + _after_first_block_read(double, monkeypatch, lambda: _request( + double, "logseq.Editor.updateBlock", TASK, "TODO ship the parser, changed")) + error = refusal(run("--id", TASK, "--status", "DONE", "--expect-hash", RIGHT)) + assert error["reason"] == "precondition_failed" + assert content(double) == "TODO ship the parser, changed" + + +def test_a_block_gone_since_it_was_found_is_block_not_found(double, monkeypatch): + _after_first_block_read(double, monkeypatch, lambda: _request( + double, "logseq.Editor.removeBlock", TASK)) + error = refusal(run("--id", TASK, "--status", "DONE", "--expect-hash", RIGHT)) + assert error["reason"] == "block_not_found" + assert error["id"] == TASK + assert error["error"].count(NOTHING) == 1 + assert double.sent("updateBlock") == [] diff --git a/tests/test_write_proof_every_command.py b/tests/test_write_proof_every_command.py index 87d9be2..6c2243f 100644 --- a/tests/test_write_proof_every_command.py +++ b/tests/test_write_proof_every_command.py @@ -157,7 +157,10 @@ "add-journal-block-heading": 2, "add-journal-block-tree": 2, "add-journal-block-keep-ids": 2, - "add-journal-block-upsert": 2, + # Four: the upsert's preview chooses the block it would replace the way + # the run does (the page's blocks, then the heading's children), whether + # or not [graph] keep_empty_blocks_last is on. + "add-journal-block-upsert": 4, "add-journal-block-new-journal": 2, "add-journal-content": 2, "add-journal-content-multiline": 2, diff --git a/tests/test_write_refused_output.py b/tests/test_write_refused_output.py index 625a8e9..3f0b841 100644 --- a/tests/test_write_refused_output.py +++ b/tests/test_write_refused_output.py @@ -23,11 +23,17 @@ # (type, reason, fields): the contract --json callers rely on. TYPES = [ ("BlockChanged", "block_changed", {"block": BLOCK}), + ("BlockNotFound", "block_not_found", {"id": BLOCK}), ("EditorOpen", "open_in_editor", {"block": BLOCK, "page": "Probe Page"}), ("EditorStateUnknown", "editor_state_unknown", {"answer": '{"ok": 1}'}), ("LogseqWriteError", "logseq_error", {"method": "upsertBlockProperty", "logseq_message": "foo is not a valid UUID string."}), ("PageExists", "page_exists", {"page": "Probe Page"}), + ("PreconditionFailed", "precondition_failed", + {"expected_hash": "0123456789ab", "actual_marker": "DONE", "first_line": "DONE x"}), + ("PreconditionRequired", "precondition_required", + {"source": ["config"], "config_path": "/x/config.toml", + "options": ["--expect-hash", "--expect-marker"]}), ("RenameRefused", "rename_refused", {"old": "Old Page", "new": "Taken Page", "why": "exists"}), ("ReadOnly", "read_only", {"source": ["config"], "config_path": "/x/config.toml"}), ("WriteNotVerified", "write_not_verified",