LYT-515 | Lex unquoted negative numeric literals as signed literals in FilterQL

[onkarj-47]

🤖 Opened by an AI agent — not a person. This PR was created by the lytics-developer-agent skill (Claude, Anthropic) running unattended. It shows under the assignee's GitHub account because it uses their token, but a human did not hand-write it — and comment replies on this PR from this account are also posted by the agent, not typed by a person. Review, approval, and merge stay human decisions; the agent never marks the PR ready-for-review and never merges.

Merge order

This is the producer in a multi-PR change for LYT-515:

1. this PR (qlbridge) — merge first, then cut/point a release the consumer can pin.
2. lytics/lio#39150 — the segment-compiler half. It no longer bumps qlbridge and can merge independently of this PR.
3. a follow-up in lio — pins this PR's squash-merge commit on master, which is what unblocks IN (-1) end to end.

What was broken

FilterQL / SegmentQL rejected unquoted negative numeric literals in value positions, while the quoted form was accepted. This blocked the customer-facing remainder of LYT-486 (P&G escalation). Two distinct failure modes, both rooted here in the lexer:

Input	Before	Why
FILTER visitct IN (-1) FROM user	hard parse error (Unrecognized input)	LexListOfArgs backs up on - → LexExpression emits a standalone TokenMinus, desyncing the array token stream
FILTER visitct IN (-1, 3) FROM user	hard parse error	same, plus the list continuation was never re-pushed
FILTER visitct = -1 FROM user	parsed as UnaryNode{Minus, Number} → printed as - (1)	the - is torn off as TokenMinus before number-lookahead

The number scanner already accepted a leading sign (scanNumericOrDuration), but that path was never reached in comparison-RHS / IN-list positions.

The fix

The lexer is a state machine with an explicit stack of continuations, where a StateFn returning nil means pop a frame and run it. LexNumber ends that way, so the invariant is: push your continuation before delegating to it.

When a - appears in a value position — the previous emitted token is a comparator, arithmetic operator, (, ,, logic op, IN/BETWEEN, or start-of-input (TokenNil) — and the next rune begins a literal the scanner will accept, lex it as a single signed numeric token instead of TokenMinus:

• LexExpression pushes l.clauseState() and returns LexNumber. Without the push, LexNumber's nil consumed the enclosing clause's frame, so anything after the literal (an infix AND/OR) lost the state needed to lex it, and a trailing operator (= -1-) live-locked the state machine in a push/pop cycle that consumed no input.
• LexListOfArgs pushes itself and goes straight to LexNumber, matching what it already does for a positive literal. Routing through LexExpression made the two changes mutually dependent — each only worked while the other was wrong.
• numericAfterSign requires a digit immediately after the sign and rejects 0x, so the gate agrees with what scanNumericOrDuration will actually accept. A committed gate has no fallback, so a disagreement turned -.5 and -0x1A into terminal bad number syntax errors instead of leaving them lexable.

Gating is strictly on l.lastToken.T, so binary subtraction (a - b, where the previous token is an identity/number/value/)) is unchanged — important because LexExpression is shared with the SQL dialect.

Result

= -1, city = -1, = -1.5, IN (-1), IN (-1, 3), IN (1, -3), IN ("a", -1), NOT IN (-1), eq(x, -1) and BETWEEN -5 AND -1 all parse; each negative is a single *expr.NumberNode whose .Text carries the sign; .String() canonicalizes to the bare signed form (never - (1)) and re-parses idempotently.

Negatives followed by an infix AND/OR keep parsing, -.5 / -0x1A keep their previous TokenMinus handling, and = -1- / = -1/ terminate. Positive, quoted, and binary-minus behavior is unchanged.

The SQL dialect changes shape for the same inputs — WHERE age > -1 now yields a NumberNode where it previously yielded UnaryNode{Minus, Number}. It parses either way; flagging it because consumers that type-switch on the RHS see a different tree.

Testing

• lex/dialect_filterql_test.go — token streams for = -1, = -1.5, IN single/multi, string-named fields, negatives before an infix AND/OR, negatives not first in a list, sign directly after BETWEEN, the -.5 / -0x1A fallbacks, and a termination test for = -1- / = -1/ that runs on a goroutine so a live-lock fails the test rather than hanging the suite.
• lex/lexer_test.go — binary-minus guards now include WHERE (x - 1) > 5 and WHERE x > 5 - 3, which traverse the changed branch (the original SELECT column-list cases did not), plus signed-literal coverage in a SQL WHERE.
• rel/parse_filterql_test.go — parse-level equivalents, with the infix rows added to the shared FilterTests round-trip corpus.
• go test -race ./... → 19 packages, 0 failures.

Known observations (not changed here)

• go vet ./... reports a pre-existing unreachable code at testutil/testsuite.go:215 — present on the base commit, untouched by this PR, and not part of this repo's CI (.github/workflows/test.yml runs go test -race ./...).
• expr.ParseExpression("-1") hangs on master as well as on this branch — a pre-existing bug in the expression dialect, unrelated to this change. It is why the TokenNil entry in valueExpectedTokens has no test: the only route to it fails on both commits.
• IN (…) AND … fails on master with all-positive literals (IN (1, 3) AND city = "sf" → Unrecognized input). A pre-existing FilterQL limitation, not sign-related; the prefix form AND ( x IN (…), … ) works and is what the tests use.

Files

• lex/lexer.go — value-position signed-literal lexing (valueExpectedTokens, numericAfterSign(), the LexExpression / LexListOfArgs - handling).
• lex/lexer_test.go, lex/dialect_filterql_test.go, rel/parse_filterql_test.go — tests.

[ryan-cstk]

Review — signed negative numeric literals in the FilterQL lexer

Reviewed at e136cba across three lenses (security / correctness / quality). The diagnosis in this PR is right and the IN (-1) breakage is real — but the current implementation introduces a CPU-exhaustion hang and a parse regression on FilterQL that works today, both from a single missing state-stack push. Everything below was measured by running both commits side by side; no worktree was modified.

---

CRITICAL 1 — new infinite loop, reachable from the segment API

FILTER x = -1- and FILTER x = -1/ spin forever inside one NextToken() call. No further tokens, no EOF, no panic.

34c67a3 (master):  TERMINATES | FILTER x = -1-
e136cba (this PR): *** HANGS *** | FILTER x = -1-
34c67a3 (master):  TERMINATES | FILTER x = -1/
e136cba (this PR): *** HANGS *** | FILTER x = -1/

A differential fuzz (8000 inputs x 2 dialects) found 12 hanging inputs on this branch and 0 of them on master; master's own pre-existing hangs reproduce on both.

This is reachable from untrusted input in lio: src/api/rw/segment.go:164 passes the raw request body into models.NewSegmentFromQL -> rel.ParseFilterQL. The lexer takes no context.Context and has no iteration bound, so a request deadline does not stop the goroutine, and lio's recover() does not catch it — a live-lock is not a panic. Each malicious POST permanently pins one core.

CRITICAL 2 — FilterQL that parses today stops parsing

Any negative literal followed by an infix AND/OR now fails. Same probe, both commits:

FilterQL	34c67a3 (master)	e136cba (this PR)
visitct = -1	OK -> - (1)	OK -> -1 ✅ fix
visitct IN (-1)	ERR	OK ✅ fix
visitct IN (-1, 3)	ERR	OK ✅ fix
visitct = -1 AND city = "x"	OK	ERR ❌
visitct = -1 OR city = "x"	OK	ERR ❌
visitct > -1 AND visitct < 5	OK	ERR ❌
visitct != -1 AND city = "x"	OK	ERR ❌
visitct = -1.5 AND city = "x"	OK	ERR ❌
visitct = -.5	OK	ERR ❌
visitct = 1 AND city = "x"	OK	OK (unchanged)
a - b > 1	OK	OK (unchanged)
visitct BETWEEN -5 AND -1	OK	OK (unchanged)

Three fixes, six regressions — and the regressing shape (= -1 AND ...) is considerably more common in real segments than the IN (-1) shape being fixed.

This breaks stored data, not just new input: rel.ParseFilterQL sets Raw = m.l.RawInput(), and lio persists that raw text verbatim. Once lio bumps past its current pin, already-saved segments containing = -1 AND ... fail to load; segmentlist.go marks them Invalid, the enrichment pipe skips them, and invalidity cascades to every parent segment that includes them. Affected audiences silently empty out.

See the inline comments for the root cause and a verified fix.

---

Verified clean

Security

• Binary subtraction is genuinely untouched — 91-query differential corpus (SELECT a-1, SELECT a - b, WHERE x > y-1, WHERE (x - 1) > 0, now() -1h, SELECT count(*) - 1, SELECT a--1 inline comment) produces byte-identical token streams and ASTs on both commits. The gate set cannot be reached with an identity/number/) as the previous token, so no binary->unary flip is possible.
• The SQL dialect is unaffected — SELECT * FROM users WHERE age > -1 AND name = "bob" parses identically. The clause-continuation bug is FilterQL-specific.
• lastToken hygiene is sound — written only inside Emit (both branches, before the channel send). errorf bypasses it but terminates the scan, so no stale read follows. Fresh lexer per statement, so nothing leaks across statement boundaries.
• PeekX clamps at len(l.input), so numericAfterSign cannot read past end of input. No panic originated in the new code across 16,000 fuzz cases.
• No logging or credential path touched.

Correctness

• .String() round-trip is idempotent for every input that still parses, including BETWEEN -1 AND -2, IN (-1, -2) and nested AND ( ..., OR ( ... ) ).
• Pre-existing and not caused by this PR (confirmed identical on master): FILTER x = - nil-deref; SELECT (2)-1 hang; UPDATE ... SET String() defect; FILTER x IN (-1) AND y = 2 failing.
• The PR does fix real breakage: IN (-1), IN (-1, 3), IN (1, -3), IN ("a", -1), NOT IN (-1), eq(x, -1), INSERT ... VALUES (-1, 2) all go from parse-error to correct.

Quality

• valueExpectedTokens is a package-level map built once at init — right shape for a lexer hot path.
• Test layering is good: token-level in dialect_filterql_test.go, AST-level in parse_filterql_test.go, plus a direct-AST-construction case that correctly isolates the stored-AST scenario from the lexer fix. t.Parallel() usage matches each file's existing convention.
• go vet ./... reports only the pre-existing testutil/testsuite.go:215 unreachable-code warning — confirmed pre-existing (unchanged since 2021, not in this diff), exactly as the PR body claims. gofmt -l flags four files, none of them in this diff.

---

Recommendation

Request changes. The approach is sound and the fix is small — the two lexer changes need to be reworked together (see inline), plus the numericAfterSign/scanNumericOrDuration disagreement and regression tests for the shapes that currently slip through. Two reviewers independently applied the combined fix and measured zero regressions against master across 56 FilterQL + 63 SQL + 30 malformed inputs, with ./lex ./rel ./expr ./vm green.

Reviewed with /lio-review.

[ryan-cstk]

Approving — both criticals verified fixed

Co-reviewed: the original round was mine; this verification pass was run with Claude Code and I've reviewed the results. Every measurement below is reproducible — commands and corpora are described inline.

Nice work on this. You fixed the root cause rather than the symptom, the two coupled changes landed together in d24ab0a as they had to, and the lextrace=1 stack comparison in your reply is exactly the right way to have confirmed the diagnosis before touching anything.

I re-ran the differential rather than reading the diff, across three commits: 34c67a3 (lio's current pin), e136cba (what I reviewed), and 7b72862 (now).

Both criticals: resolved

CRITICAL 1 — the hang is gone. Across a 480-input FilterQL corpus: 0 hangs on 7b72862, versus the 12 hanging inputs the previous head produced. FILTER visitct = -1- and = -1/ now emit output byte-identical to master:

34c67a3 : PANIC(runtime error: invalid memory address or nil pointer dereference)
e136cba : *** HANG ***
7b72862 : PANIC(runtime error: invalid memory address or nil pointer dereference)

Worth stating why that's a genuine fix and not a lateral move: lio's recover() catches a panic, so the request fails cleanly. It cannot catch a live-lock — that pinned a core until the pod died. Returning to master's pre-existing nil-deref is the correct outcome for this PR. (That nil-deref on trailing-operator input is a real master bug, but it predates you and deserves its own issue.)

CRITICAL 2 — no regressions. Same 480-input corpus, 34c67a3 vs 7b72862:

ERR -> OK  (fixes)                 : 25
OK  -> ERR (REGRESSIONS)           : 0
OK  -> OK  (representation change) : 205
panic/hang transitions             : 0

All seven shapes I flagged are back, including the one your newest commit targets:

FilterQL	34c67a3	e136cba	7b72862
visitct = -1 AND city = "x"	OK	ERR	OK
visitct = -1 OR city = "x"	OK	ERR	OK
visitct > -1 AND visitct < 5	OK	ERR	OK
visitct != -1 AND city = "x"	OK	ERR	OK
visitct = -1.5 AND city = "x"	OK	ERR	OK
visitct = -.5	OK	ERR	OK
visitct = -0x1A	OK	ERR	OK
visitct BETWEEN -5 AND -1 AND city = "x"	OK	ERR	OK
visitct IN (-1, 3)	ERR	OK	OK ✅
eq(visitct, -1)	ERR	OK	OK ✅

The 205 representation changes are semantically identical. - (1) → -1 is a different AST, so I didn't assume equivalence — I evaluated every query that parses on both commits through vm.Matches against rows spanning negative, zero and positive (visitct ∈ {-1, 0, 1, -5, -1.5, -0.5}). 424 of 424 identical verdicts. Cosmetic in the AST, no behavioural change.

Tests. ./lex ./rel ./expr ./vm green on both commits under TZ=UTC. The expr/builtins extract() failure some of us will see locally is pre-existing and timezone-dependent — it fails identically on 34c67a3, and passes on both under UTC. Not yours.

One suggestion, non-blocking

My earlier warning about test coverage is mostly addressed — the new tests genuinely pin both criticals. Mutation-verified, harness validated with a deliberate syntax error first:

Mutation	Result
revert the LexExpression push (CRITICAL 1 root cause)	KILLED
revert the LexListOfArgs push (CRITICAL 2 coupling)	KILLED
disable the numericAfterSign gate	KILLED — TestFilterQLNegativeLiteral, TestFilterQLNegativeLiteralInfix, TestLexSignedLiteralInWhere
drop valueExpectedTokens[l.lastToken.T] from the gate	SURVIVED

That last one is the gap. The lastToken guard is what prevents a binary minus being lexed as a signed literal, and removing it passes the entire suite while breaking ten real inputs:

FILTER a -1 > 0            guard: a - 1 > 0          no guard: ERR
FILTER visitct -1.5 > 0    guard: visitct - 1.5 > 0  no guard: ERR
FILTER (a) -1 > 0          guard: a - 1 > 0          no guard: ERR
FILTER a.b -1 > 0          guard: a.b - 1 > 0        no guard: ERR

I checked it isn't an equivalent mutant before raising it — 582 inputs, and those ten distinguish the two versions.

a -1 > 0 (space before the minus, none after) is ordinary arithmetic someone will write, and preventing exactly this binary→unary flip is the property the whole gate exists for. One fixture would pin it:

{`FILTER a -1 > 0`, `a - 1 > 0`},   // lastToken guard: binary minus must not become a signed literal

Happy for that to land here or as a follow-up — it doesn't block.

Two notes for whoever bumps lio's pin

Neither is yours to action, but they belong on the record:

1. The range carries a second PR. 34c67a3..7b72862 includes 82ddc14 — qlbridge #56, feat(esgen) Painless queries, ~380 lines under generators/esgen/. That's the qlbridge half of lio #38998 so it's expected, but the bump review needs to cover both changes, not just this one.
2. The signed-literal change reaches the SQL dialect too, not only FilterQL. 8 of 28 SQL probes changed representation (WHERE age > - (1) → WHERE age > -1), with no regressions and SELECT a - 1 preserved. Benign, but worth knowing it isn't FilterQL-scoped.

Dismissing my changes-requested. 👍