refactor: init ocaml-version logseq-cli#12768
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47f85e4108
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let bool_option_value key options = | ||
| match option_value key options with | ||
| | Some value -> Some (String.lowercase_ascii (String.trim value) = "true") | ||
| | None -> if option_present key options then Some true else None |
There was a problem hiding this comment.
For options parsed through bool_option_value (for example upsert property --hide, --public, list page --include-built-in, or show --linked-references), any value other than a case-insensitive true is silently treated as false. This means inputs like --hide tru or --linked-references maybe are accepted and change command behavior instead of producing an actionable invalid-options error, contrary to the CLI boundary behavior expected for user-supplied options. Please validate these values as only true/false when a value is provided.
Useful? React with 👍 / 👎.
| let prefix_len = String.length prefix in | ||
| String.length value >= prefix_len && String.sub value 0 prefix_len = prefix | ||
|
|
||
| let create_graph value = String.trim value |
There was a problem hiding this comment.
--graph values are only trimmed here, so empty strings and path-segment names like . or .. are accepted as graph names. Since filesystem commands later build paths with Filename.concat <root>/graphs (Graph_dir.encode_graph_dir_name graph), those inputs resolve to <root>/graphs, <root>/graphs/., or <root>/graphs/..; commands such as graph remove and backup operations can then operate on the graph root or CLI root instead of a graph directory. Reject empty, . and .. graph names at the boundary before constructing actions.
Useful? React with 👍 / 👎.
RCmerci
force-pushed
the
refactor/ocaml-logseq-cli
branch
2 times, most recently
from
5546a6f to
ee878ee
Compare
|
@codex, review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee878eebb3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match rest with | ||
| | value :: tail when not (is_option value) -> | ||
| loop ((key, Some value) :: options) positional tail | ||
| | _ -> loop ((key, None) :: options) positional rest) |
There was a problem hiding this comment.
When a value-taking option is followed by another option or by the end of argv, this records (key, None) and continues; later validation and config resolution treat that the same as the option being absent. For example, logseq --root-dir --graph work remove page --page Home silently falls back to the default root instead of failing that --root-dir has no value, which can make write commands target the wrong graph root after a CLI typo.
Useful? React with 👍 / 👎.
| match ensure_root_config config with | ||
| | Error err -> |
There was a problem hiding this comment.
Every parsed command reaches ensure_root_config here, so logseq --version resolves and creates/checks the default root (~/logseq) before returning build metadata. The docs explicitly call out that --version should exit without requiring repo/root-dir, so in a locked-down HOME or read-only root this read-only metadata command fails or mutates the filesystem even though the version action itself does not use graph storage.
Useful? React with 👍 / 👎.
| | Some _ -> failwith "tag not found" | ||
| | None -> failwith "tag not found") |
There was a problem hiding this comment.
When upsert block --update-tags ... creates blocks and a supplied tag name/id resolves to no tag, this failwith rejects the effect instead of returning a Cli_result error; main.ml then prints a raw OCaml exception and bypasses the requested JSON/EDN error formatting. This is a normal user-input failure (missing/private tag), so create mode should report it through the same structured option-resolution path used by update mode.
Useful? React with 👍 / 👎.
| bind (find_repo_server config repo) (function | ||
| | Some server -> pure (Ok (invoke_config_of_server config server)) |
There was a problem hiding this comment.
When a graph command finds an existing db-worker-node for the repo, this returns that endpoint without comparing server.revision with the local CLI revision. The documented desktop/CLI contract says startup must restart a daemon with a different revision before returning a usable endpoint, so after an upgrade the new CLI can keep talking to a stale worker with an incompatible API or schema.
Useful? React with 👍 / 👎.
| (Server_runtime.cleanup_revision_mismatched_servers config | ||
| ~cli_revision:"unknown") (function |
There was a problem hiding this comment.
server cleanup is documented to treat revision != local CLI revision as a mismatch, but it always passes the literal "unknown". In normal builds db-worker health reports the actual revision, so running cleanup will classify every CLI-owned server with a real revision as mismatched and stop it even when it already matches the current CLI.
Useful? React with 👍 / 👎.
| progress = | ||
| (if option_present "progress" options then Some true | ||
| else None); |
There was a problem hiding this comment.
The tokenizer accepts boolean literals, and the docs say --progress false suppresses sync download progress, but this parse step discards the parsed value and records Some true whenever the option is present. As a result both --progress false and --progress=false force progress streaming instead of disabling it, which breaks scripted JSON/EDN downloads that explicitly turn progress off.
Useful? React with 👍 / 👎.
| | None, Some repo -> Server_runtime.ensure_server config repo ~create_empty_db | ||
| | None, None -> Cli_effect.pure (Error (Error.missing_graph ())) |
There was a problem hiding this comment.
For graphless sync commands such as sync remote-graphs, config.repo is None and config.base_url is only set by LOGSEQ_CLI_BASE_URL, so normal user invocations fall through to Error.missing_graph before any remote graph request is made. These commands are documented as not requiring --graph; they need a real global/default endpoint path instead of requiring an unrelated graph worker.
Useful? React with 👍 / 👎.
| | _, Some b, _, _, _, _, _, _ -> Js.Json.boolean b | ||
| | _, _, Some i, _, _, _, _, _ -> Js.Json.number (float_of_int i) | ||
| | _, _, _, Some f, _, _, _, _ -> Js.Json.number f | ||
| | _, _, _, _, Some s, _, _, _ -> Js.Json.string (strip_leading_colon s) |
There was a problem hiding this comment.
This branch handles both EDN strings and keywords via as_string_like, then strips a leading colon from all of them. In JSON output, user data such as a block title or property value ":meeting" is serialized as "meeting", corrupting machine-readable output; only keyword values or map keys should have their EDN colon removed.
Useful? React with 👍 / 👎.
tiensonqin
force-pushed
the
refactor/ocaml-logseq-cli
branch
from
4a22405 to
61b1350
Compare
2 participants
opam install dune)cd cli && dune build @allcd cli && node _build/default/dist/logseq-cli.js