Open
Conversation
Implements Task 1 of the Phase 1 query plan: - query/result.go: DoctorResult and AdvancedResult types - query/backend/httpapi/client.go: HTTP client with DoRaw, ProbeDBQ, ProbeDatascriptQuery methods, AuthError type - query/backend/httpapi/execute.go: RunDoctor and RunAdvancedQuery with DB.q-first/datascriptQuery-fallback strategy - query/backend/httpapi/client_test.go: 18 httptest-based tests covering success, auth, timeout, malformed JSON, unreachable, probe fallback, and advanced query execution
…lidation
Fix three Task 1 review findings:
1. Reachability classification:
- Added TransportError type for connectivity failures (connection
refused, DNS, timeout).
- Added MethodError type for method-layer failures (non-200 status,
malformed JSON, error envelopes).
- RunDoctor now sets reachable=false only for TransportError.
- API reachable but both methods failing → reachable=true, error set.
2. BaseURL / APIURL contract:
- BaseURL is now the full API endpoint (e.g. http://127.0.0.1:12315/api).
- DoRaw posts directly to BaseURL without appending /api.
- DoctorResult.APIURL reports the same value.
- Matches spec illustrative output and --api-url flag semantic.
3. Probe validation:
- ProbeDBQ/ProbeDatascriptQuery now check for JSON error envelopes.
- isProbeSuccess rejects 200 responses where the body is a JSON object
with a non-null, non-empty "error" field.
- Arrays, scalars, and objects without error fields pass.
- Documented the heuristic in code comments.
Tests: 27 tests (was 18), all passing.
…ages
Reduce Task 1 residual risks before Task 2:
1. Probe validation tightened:
- isProbeSuccess now uses generic map parsing to handle error values
of any JSON type (string, object, number), not just strings.
- Added boolFlagFalse helper to catch {"ok":false} and
{"success":false} envelopes.
- Full decision table documented in code comments.
- 7 new probe tests covering: error objects, numeric errors,
ok:false, success:false, ok:true, non-boolean ok.
2. Real API validation seam:
- Added ValidateAgainstReal() no-op function with 6 targeted TODOs
documenting exactly what must be verified against a real Logseq
instance before the probe heuristic can be considered final.
3. Error observability:
- bothFailedMsg helper produces actionable messages that include
the underlying error text from both logseq.DB.q and
logseq.DB.datascriptQuery methods.
- Doctor tests now verify error messages contain both method names.
Tests: 34 (was 27), all passing.
Implements Task 2 of the Phase 1 query plan: - query/result.go: refined DoctorResult and AdvancedResult types with spec-matching doc comments; added RenderResult(format, result) public API supporting json, ndjson, and text output formats; format constants FormatJSON, FormatNDJSON, FormatText. JSON: compact single-line with trailing newline, struct-order keys. NDJSON: for AdvancedResult, expands results array into one JSON line per element; falls back to full envelope for errors, null, empty, or non-array results. Doctor and other types emit one line. Text: human-readable summary for doctor (field-per-line); pretty- printed JSON for advanced query results. - query/result_test.go: 21 tests covering all three formats for both result types, including null/empty handling, pointer receivers, deterministic key order, and unsupported format rejection.
Fix two Task 2 review findings:
1. warnings normalization in JSON/NDJSON output:
- renderJSON now type-switches on DoctorResult/AdvancedResult
and calls normalizeWarnings() before json.Marshal, converting
nil []string to []string{} so "warnings":null never appears.
- Callers are not required to pre-initialize the slice.
- marshalLine() helper extracted to avoid repetition.
- Same normalization applies to NDJSON envelope fallbacks
because advancedNDJSON delegates to renderJSON.
2. NDJSON warning context lost on successful fallback results:
- advancedNDJSON now emits the full envelope as one JSON line
whenever len(r.Warnings) > 0, even when results is a non-empty
array that could otherwise be expanded.
- This ensures e.g. the datascriptQuery fallback warning is always
visible in the output stream.
- Expanded-lines mode (one item per line) is now only used when
results is a non-empty array AND warnings is empty AND no error.
- Comments updated to reflect the complete decision table.
Tests: 24 in query package (was 21):
- Fixed: TestRenderResult_DoctorJSON_NilWarningsBecomesEmptyArray
now uses a genuinely nil slice (not []string{})
- Added: TestRenderResult_AdvancedJSON_NilWarningsBecomesEmptyArray
- Added: TestRenderResult_DoctorNDJSON_NilWarnings
- Added: TestRenderResult_AdvancedNDJSON_WarningsPreventsExpansion
Implements Task 3 of the Phase 1 query plan:
- main.go: intercept os.Args[1]=="query" before legacy flag parsing,
delegate to cmd.RunQuery, and exit with the returned code. All legacy
CLI behavior is completely untouched.
- cmd/query.go: RunQuery parses the query subcommand and flags
(--backend, --format, --api-url, --api-token-env, --explain), validates
inputs, then dispatches to the doctor handler. runDoctor creates an
httpapi.Client, runs RunDoctor, renders output via query.RenderResult,
and returns exit 0 (healthy) or 1 (error found).
- cmd/query_test.go: 9 tests covering all three formats, default format
(text), unsupported format/backend, no/unknown subcommand, and backend
auto→http resolution. All use httptest fixtures.
Supported CLI surface:
lsq query doctor [--format text|json|ndjson] [--backend auto|http]
[--api-url URL] [--api-token-env ENV] [--explain]
Design note: the plan called for query/router.go but that creates an
import cycle (query → httpapi → query). Routing logic lives in cmd/
instead, which imports both query (for rendering/types) and httpapi
(for execution) without cycles.
The no-subcommand and unknown-subcommand paths were printing: usage: lsq query <doctor|advanced> [flags] This implied 'advanced' is implemented, which it is not in Task 3. Updated both to: usage: lsq query doctor [flags] Updated TestRunQuery_NoSubcommand and TestRunQuery_UnknownSubcommand to pin the exact usage string and explicitly assert that 'advanced' does not appear in the output.
Implements Task 4 of the Phase 1 query plan: - cmd/query.go: added --query and --file flags, 'advanced' subcommand dispatch, and runAdvanced handler. Validates exactly one of --query or --file is provided. --file reads from disk, trims whitespace, rejects empty files. Query text is passed through to httpapi.RunAdvancedQuery. Output rendered via query.RenderResult. Exit 0 on success, 1 on error. Usage strings updated to reflect the full <doctor|advanced> surface. - cmd/query_test.go: 10 new tests (19 total in cmd): - AdvancedJSON: JSON output with correct fields/results - AdvancedNDJSON: results expanded to 2 lines - AdvancedText: text output contains results - AdvancedFileInput: reads query from .edn file - AdvancedMissingQuery: rejects no --query/--file - AdvancedBothQueryAndFile: rejects conflicting inputs - AdvancedFileNotFound: rejects nonexistent file - AdvancedEmptyFile: rejects whitespace-only file - AdvancedBackendValidation: --backend file rejected - DoctorStillWorksAfterAdvanced: doctor regression guard CLI surface after this task: lsq query doctor [--format text|json|ndjson] [flags] lsq query advanced --query '...' [--format text|json|ndjson] [flags] lsq query advanced --file ./q.edn [--format text|json|ndjson] [flags]
A whitespace-only --query value was incorrectly treated as present because the presence check ran before trimming. This created inconsistent behavior: --file content was already trimmed and rejected if empty, but --query was not. Fix: trim both queryStr and queryFile at the start of runAdvanced, before the hasQuery/hasFile checks. A blank-only --query now triggers: error: one of --query or --file is required Added TestRunQuery_AdvancedWhitespaceOnlyQuery to pin this behavior.
Implements Task 5 of the Phase 1 query plan.
tests/integration/integration.go:
Added two focused helpers (plan: Step 3, prefer one binary-build
helper over package-wide refactors):
- BuildBinary(moduleRoot): compiles lsq to os.TempDir once
- RunCLI(binaryPath, args, env...): exec-runs the binary, captures
stdout/stderr, and returns exit code as CLIResult
tests/integration/query_cli_test.go:
TestMain builds the binary once for the package (not per-test).
12 tests using os/exec against the real compiled binary:
doctor:
- TestQueryCLI_DoctorJSON_Healthy: full JSON shape (all spec fields,
reachable, warnings:[], null error, nested auth/capabilities)
- TestQueryCLI_DoctorJSON_Unreachable: exit 1, reachable=false,
error set
- TestQueryCLI_DoctorNDJSON_Healthy: single valid JSON line
- TestQueryCLI_DoctorText_Healthy: Reachable/Backend in output
advanced:
- TestQueryCLI_AdvancedJSON_Success: full JSON shape (backend,
input_kind, query_method, results, warnings:[], null error)
- TestQueryCLI_AdvancedJSON_BothMethodsFail: exit 1, error set,
null results
- TestQueryCLI_AdvancedNDJSON_ExpandsResults: 3 results → 3 lines
- TestQueryCLI_AdvancedText_Success: result content in text
- TestQueryCLI_AdvancedFileInput: reads from .edn file
validation:
- TestQueryCLI_AdvancedMissingQuery: exit 1, validation error
- TestQueryCLI_UnsupportedFormat: exit 1, format error in stderr
- TestQueryCLI_UnknownSubcommand: exit 1, subcommand error in stderr
Fixes a Task 6 review finding indicating that the alias search behavior of '-f' was not guarded against regressions. - Added TestLegacyCLI_FindAlias: creates a mock page with an 'alias::' property and runs 'lsq -f <alias>' to verify that the query short-circuit entrypoint does not break trie alias parsing.
Real Logseq instances return HTTP 200 with body 'null' rather than an HTTP error when DB.q encounters an unsupported query or fails. This caused the advanced query executor to incorrectly report success with a 'null' result array instead of falling back to datascriptQuery. - Added an isJSONNull helper to detect 'null' in raw json bytes. - RunAdvancedQuery now treats a 'null' response from DB.q as an execution failure and properly proceeds to the fallback path. - Added tests to cover fallback on 'null' returning DB.q calls.
Implements Task 8. Removes the transitional DB.q fallback heuristic from RunAdvancedQuery. Advanced queries now map strictly and universally to logseq.DB.datascriptQuery, avoiding redundant HTTP errors and honoring the API boundary verified. - Cleaned up execute.go, eliminating the isJSONNull check cleanly. - Overhauled httpapi backend testing validating direct datascriptQuery assertions. - Adjusted CLI integration tests confirming pure downstream routing.
Phase 2 Task 1: Add remote RunSimpleQuery execution. - RunSimpleQuery routes raw simple DSL expressions directly to logseq.DB.q without fallback (no datascriptQuery attempt) - Reuses AdvancedResult with InputKind="simple" and QueryMethod="logseq.DB.q" so existing rendering works unchanged - Tests cover: success, method dispatch verification, failure, auth failure, and 4 DSL expression shapes (page-ref, task, and-combinator, page-property)
Phase 2 Task 2: Route simple query through CLI dispatch. - Add --expr flag for simple DSL expression input - Add 'simple' subcommand dispatch to runSimple handler - Validate --expr is required and non-whitespace - Reuse existing HTTP backend (RunSimpleQuery) and result rendering - --backend file rejected (Phase 2 is HTTP-only) - Update usage strings to include 'simple' - Tests: success (JSON/text), (task now), missing --expr, whitespace-only --expr, --backend file rejection, regression coverage for doctor and advanced paths
Phase 2 Task 2 completion: The router extraction.
- Creates `query/router.go` implementing `query.RunSimple`
- Offloads backend resolution ('auto'/'http' evaluation)
into the router layer, replacing direct execution in
`cmd/query.go`
- Introduces `HTTPSimpleExecutor` to decouple execution, avoiding
an import cycle between the `query` boundary models and `httpapi`
- Adds `query/router_test.go` to guarantee backends like
'file' and 'local' predictably yield unsupported errors natively
- Fully preserves all existing semantic behaviors and validations
Phase 2 Task 3: Build Smoke Integrations. - Added `tests/integration/query_simple_cli_test.go` - Covered `lsq query simple --expr '[[logseq]]' --format json` - Covered `lsq query simple --expr '(task now)' --format json` - Verified output envelope, `backend=http`, `input_kind=simple`, and `query_method=logseq.DB.q` routing - Added smoke test for `200 + null` response correctly triggering a CLI error - Updated legacy `TestQueryCLI_UnknownSubcommand` to use 'magic' instead of 'simple' to prevent collisions
Phase 2 Task 4: User-Facing Documentation. - Add `lsq query simple` capabilities to README showcasing representative HTTP evaluation examples - Note explicit macro / advanced datalog constraints for boundary context - Remove prior claim that simple queries are unsupported - Update `CHANGELOG.md` noting remote capability addition against `logseq.DB.q`
During final Phase 2 verification, real-world evaluations revealed Logseq natively returns HTTP 200 responses containing generic JSON generic error objects if simple queries arbitrarily fail execution (e.g. `{"error": ...}`).
Previously, `RunSimpleQuery` correctly rejected explicit literal `null` responses natively. This commit expands execution coverage resolving the bug by strictly reusing the battle-tested `isProbeSuccess` error-envelope classification heuristic natively resolving execution correctly, cascading an explicit CLI exit 1 without broadening valid evaluation boundaries arbitrarily.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/query.go`:
- Around line 334-349: validateExcludedSimpleInput currently misses top-level
EDN vectors; add a guard that rejects inputs whose first non-space character is
'[' (after strings.TrimSpace(expr)) unless the entire trimmed expression exactly
matches the allowed page-ref pattern (e.g. "[[page-ref]]" allowing optional
inner spacing). In the function validateExcludedSimpleInput (use wrapped,
prefix, errorPrefix for context), check trimmed := strings.TrimSpace(expr); if
strings.HasPrefix(trimmed, "[") and it does NOT match the allowed page-ref form,
return a descriptive error (reuse errorPrefix and prefix) indicating raw vectors
are not allowed.
In `@query/backend/httpapi/execute.go`:
- Around line 115-132: The response struct leaves QueryMethod empty on errors
because QueryMethod is only set after c.DoRaw succeeds; set QueryMethod when
constructing the AdvancedResult (e.g., initialize QueryMethod:
"logseq.DB.datascriptQuery") so failed responses keep the method, and make the
identical change in the other branch referenced (the block around lines 147-176)
that sets QueryMethod after success; update the AdvancedResult literal(s) rather
than assigning after c.DoRaw to ensure stable output shape on failure.
- Around line 35-64: When DB.q fails with a transport error
(isTransport(dbqErr)) but ProbeDatascriptQuery(ctx) succeeds, don't treat this
as a confident "DB.q missing but datascript available" success; instead surface
an inconsistent-probe error. Modify the branch that currently sets res.Reachable
= true, setAuthSucceeded(&res), res.Capabilities.DatascriptQuery = true and
appends the fallback warning: instead set res.Reachable = true, call
setAuthSucceeded(&res) if appropriate, do NOT set
res.Capabilities.DatascriptQuery, and set res.Error to a clear
inconsistent-probe message (use bothFailedMsg-like wording referencing dbqErr
and a successful datascript) and append a warning about transport vs
method-layer mismatch so the caller sees an inconsistent diagnosis; reference
isTransport, c.ProbeDatascriptQuery, setAuthSucceeded,
res.Capabilities.DatascriptQuery, res.Warnings and bothFailedMsg to locate the
code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 553dbd93-68a6-474d-8293-53c6037902ba
📒 Files selected for processing (14)
cmd/query.gocmd/query_test.goquery/backend/httpapi/client.goquery/backend/httpapi/client_test.goquery/backend/httpapi/execute.goquery/result.goquery/result_test.goquery/router.goquery/router_test.gotests/integration/integration.gotests/integration/legacy_cli_smoke_test.gotests/integration/query_cli_test.gotests/integration/query_simple_cli_test.gotests/integration/query_simple_macro_test.go
✅ Files skipped from review due to trivial changes (3)
- query/router_test.go
- query/result_test.go
- query/backend/httpapi/client_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/integration/query_simple_macro_test.go
- query/router.go
- tests/integration/legacy_cli_smoke_test.go
- tests/integration/query_simple_cli_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds HTTP-backed query support to
lsqand incorporates the follow-up fixes requested during review of #71.Scope:
lsq query doctorprobes Logseq HTTP API reachability, auth, and method supportlsq query advancedexecutes advanced Datalog queries vialogseq.DB.datascriptQuerylsq query simpleexecutes raw simple DSL vialogseq.DB.q{{query ...}}wrappers around simple DSLtext,json,ndjsonDeliberate exclusions:
query simpleFollow-up fixes included here:
This PR supersedes the review-fix follow-up requested in #71 while carrying the full feature branch diff against
master.Summary
Adds HTTP-backed Query support to lsq: three new CLI subcommands (doctor, advanced, simple) that call a running Logseq HTTP server, an HTTP client and execution layer with robust error classification, result rendering in text/json/ndjson, simple-macro-aware input normalization, tightened CLI validation, and a comprehensive suite of unit and integration tests. Documentation and changelog updates accompany the feature. Several follow-up fixes harden NDJSON output, null detection, probe validation, and test stability.
Changelog
Added
lsq query doctor— probe Logseq HTTP API reachability, authentication, and method supportlsq query advanced— run advanced Datalog queries vialogseq.DB.datascriptQuery(supports --query / --file)lsq query simple— run simple DSL vialogseq.DB.q(supports --expr with limited{{query ...}}macro stripping)RunDoctor,RunAdvancedQuery,RunSimpleQuery(query/backend/httpapi/execute.go)DoctorResult,AdvancedResult, andRenderResultsupportingtext,json, andndjsonnormalizeSimpleExpr) with explicit rejections for unsupported macro forms/EDN shapesquery.RunSimpleandHTTPSimpleExecutor) to dispatch HTTP-backed simple queriescmd.RunQuery(...)and newcmd/query.goimplementing subcommand parsing, flag validation, and dispatchBuildBinary,RunCLI) and a large suite of unit and integration tests covering client, router, renderer, CLI parsing, macro stripping, and end-to-end CLI behavior--backend,--format,--api-url,--api-token-env,--query,--file,--expr), output formats, and limitationsChanged
main.gointercepts the top-levelquerysubcommand to callcmd.RunQuery(...)before legacy flag parsing, isolating the new query flow--query/--file, and improved diagnosticsFixed