fix(ngsiem): surface LogScale's real syntax errors in validate-query - #30
Merged
Conversation
validate-query could only ever emit "no detail returned by API", even though
the API returns a full diagnostic with named error codes and caret positions.
Two independent defects stacked.
DEFECT 1 — read a key that is never present.
test_query_syntax read response.get("body"), but NGSIEM.start_search() pops
"body" and re-exposes the payload as "resources":
# falconpy/ngsiem.py
if "body" in returned:
returned["resources"] = returned["body"]
returned.pop("body")
Verified against a raw falconpy.ngsiem.NGSIEM instance on both a 200 and a 400:
keys are ['status_code', 'headers', 'resources']. Hosts.query_devices_by_filter()
returns 'body' as normal, so the rename is specific to this method. The
passthrough the docstring promised was dead code and had never fired.
Now reads "resources" with "body" as fallback — the Uber class
(command(operation="StartSearchV1")) does not rename, so both shapes occur
depending on invocation style.
DEFECT 2 — the useful detail never reaches the SDK caller at all.
LogScale serves syntax errors as text/plain. falconpy only parses
application/json; on JSONDecodeError it assumes an empty body ("No response
content, but a successful request was made") and raises NoContentWarning
without reading response.text. The caller gets a fabricated
{"errors": [{"message": "No content was received for this request."}]} — the
opposite of what happened, pointing authors at connectivity rather than syntax.
Fixing defect 1 alone would therefore only upgrade the message to that
fabrication. _raw_syntax_error() re-issues the rejected query using the token
and base_url falconpy already authenticated, and reads the text body directly.
No second OAuth round trip, no duplicated credential handling. Best-effort:
any failure returns None and we degrade to the previous behaviour.
Before:
INVALID: LogScale rejected query (status=400, no detail returned by API)
After:
INVALID: LogScale rejected query (status=400):
Function calls are not supported in filter expressions.
See https://library.humio.com/... (Error: FunctionCallsNotSupportedInFilterExpressions)
2: | NOT (in(field="ContextBaseFileName", values=["a","b"]) #event_simpleName="…
^^
Output path also fixed: console.print now passes markup=False (CQL is full of
square brackets, which rich parses as style tags) and soft_wrap=True (wrapping
breaks the caret alignment that makes the diagnostic useful).
TESTING NOTE — the existing tests mocked a "body" key, a shape this endpoint
never returns, which is why the dead passthrough went unnoticed. They are kept
(the Uber class does return "body") and paired with explicit "resources"
coverage. 5 new tests: resources-key regression, raw diagnostic preferred over
the SDK fabrication, caret alignment preserved verbatim, no extra request on
the 200 path, and graceful degradation on transport failure.
865 passed, ruff clean. Verified live against US-2 on three real queries.
Upstream falconpy issue to be filed for the discarded text/plain body; this
bypass can be removed once the SDK stops dropping it.
CI lint went red without a source change. `pip install -e .[dev]` resolves `ruff>=0.8.0` to the newest release, which is now 0.16.2, and 0.16 widened the default rule set and altered formatter output. Measured with ruff 0.16.2 against this repo: master (unmodified) 1,378 lint errors this branch 1,379 lint errors ruff format --check 2 files would be reformatted master's own CI is green only because it last ran on 2026-06-29, before 0.16 shipped. Re-run it today and it fails identically. The breakage is not attributable to any commit — it arrived on ruff's release schedule and landed on whichever PR happened to be open. Verified clean under the pinned range (resolves to 0.15.22 today): ruff check src/ tests/ --exclude src/talonctl/_version.py -> All checks passed ruff format --check ... -> 134 files already formatted Note the fix is a version bound rather than an explicit `lint.select`, because selecting rules would not address the formatter change that fails the second CI step. The 0.16 findings are largely legitimate (import ordering, Optional[X] -> X | None) and worth adopting — but that is a ~1,378-finding migration touching most of the tree, and it belongs in a dedicated PR where it can be reviewed as such rather than buried in an unrelated change. Raise the bound there.
_raw_syntax_error() runs only after start_search() returned non-200, so its own request is expected to fail too. When it instead returns 200 — a transient failure on the first call — it has created a real LogScale query job and was returning None without stopping it, orphaning the job. The primary path already calls _cleanup_search() for the job it starts; this path now does the same. Jobs do expire on their own, so the leak was bounded, but the inconsistency was mine and had no reason to exist. Parsing is defensive: a 200 whose body is not JSON, or carries no id, must not raise out of a best-effort diagnostics helper and take the verdict with it. 2 tests added: the job is stopped with the id from the response, and an unparseable 200 body neither raises nor attempts cleanup. 867 passed, ruff clean. Re-verified live against US-2 that both the INVALID and VALID paths are unchanged.
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.
validate-querycould only ever emitno detail returned by API— even though the API returns a complete diagnostic with named error codes and caret positions. Two independent defects stacked; fixing either alone is not enough.Before / after
Defect 1 — we read a key that is never present
test_query_syntaxreadresponse.get("body"). ButNGSIEM.start_search()pops it:Verified against a raw
falconpy.ngsiem.NGSIEMinstance (no wrapper), on both a 200 and a 400 — keys are['status_code', 'headers', 'resources'].Hosts.query_devices_by_filter()returnsbodyas normal, so the rename is specific to this one method.So
response.get("body")was always{},errorswas alwaysNone, and the passthrough the docstring promised was dead code that had never fired.Now reads
resourceswithbodyas fallback — the Uber class (command(operation="StartSearchV1")) does not rename, so both shapes occur depending on invocation style. This is also why it's a fallback rather than a swap.Defect 2 — the useful detail never reaches the SDK caller
LogScale serves syntax errors as
text/plain. falconpy only parsesapplication/json; onJSONDecodeErrorit assumes the body was empty — the comment reads "No response content, but a successful request was made" — and raisesNoContentWarningwithout ever readingresponse.text. The caller receives a fabricated:{"errors": [{"message": "No content was received for this request."}]}That message is the SDK's invention and states the opposite of what happened:
Content-Lengthwas 1,382. It sends authors looking at connectivity when the problem is a specific column.So fixing defect 1 alone would only have upgraded us to surfacing that fabrication.
_raw_syntax_error()re-issues the rejected query using the token andbase_urlfalconpy already authenticated (client.auth_object), and reads the text body directly — no second OAuth round trip, no duplicated credential handling. It is best-effort: any failure returnsNoneand we degrade to the previous status-only message.Output path
console.printnow passesmarkup=Falseandsoft_wrap=True. Both are required: CQL is full of square brackets, which rich parses as style tags (values=["a","b"]would be mangled), and wrapping breaks the caret alignment that makes the diagnostic worth having.Testing
The existing tests mocked a
bodykey — a shape this endpoint never returns. That mismatch is precisely why the dead passthrough went unnoticed. They're kept, since the Uber class does returnbodyand both shapes must work, and paired with explicitresourcescoverage.5 new tests: the
resources-key regression, raw diagnostic preferred over the SDK fabrication, caret alignment preserved verbatim, no extra request on the 200 path, and graceful degradation when the bypass fails.865 passed,ruff checkclean,ruff format --checkclean.Verified live against US-2 on three real queries —
if()with a regex condition,in()insideNOT (...), and a valid control. Note the test suite must be run withPYTHONPATH=./src; the editable install in this environment resolves to a worktree, so a plainpytestrun silently tests different source.Follow-up
The upstream falconpy behaviour is worth reporting separately — it affects every consumer, including CrowdStrike's own
falcon-mcp, whosecommon/errors.pyreads onlybody.errorswith notext/plainpath and works around the missing diagnostics with a static CQL hint blob. Once the SDK stops discarding non-JSON bodies,_raw_syntax_error()can be deleted.Why this matters
This turns
validate-queryfrom a pass/fail gate into a real CQL linter. TheFunctionCallsNotSupportedInFilterExpressionserror above is a live example: diagnosing that one in the detections repo meant guessing at a rewrite, when the API had already named the rule and pointed at the token.Second commit:
build(dev): pin ruff <0.16The lint job failed on the first push. It was not caused by this change — CI resolves
ruff>=0.8.0to the newest release, now 0.16.2, which widened the default rule set and changed formatter output.Measured with ruff 0.16.2:
master, unmodifiedruff format --checkmaster's CI is green only because it last ran 2026-06-29, before 0.16 shipped. Re-run it today and it fails identically. The breakage arrived on ruff's release schedule, not on a commit, and landed on whichever PR happened to be open.My new test file is clean even under 0.16.2, so nothing here is being papered over.
Pinned to
>=0.8.0,<0.16, verified in a clean venv (resolves to 0.15.22 today):A version bound rather than an explicit
lint.select, because rule selection would not address the formatter change that fails the second CI step.The 0.16 findings are largely legitimate — import ordering,
Optional[X]→X | None— and worth adopting. But that is a ~1,378-finding migration across most of the tree and deserves its own PR rather than being buried here. Raise the bound there.