Repository navigation
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
cephalization
added
bug
|
Hi @BlueX888, sorry this hasn't had a review yet. Are you still interested in getting it merged? On main, Generated by Claude Code |
|
Hi @mikeldking, yes — still very much interested, thanks for taking a look! I'll rebase onto the latest main (picking up #16879's changes to |
67d13ab to
6d01064
Compare
|
Rebased onto latest main (picking up #16879) — the guard re-applied cleanly on top of the refactor. Re-ran the new cases: |
… upload/cancel, docs content built fine)
|
Hi @mikeldking — yes, still interested in getting this merged. Rebase is done: the guard re-applied cleanly on top of #16879's The change wraps the Is anything else needed from us before a maintainer review? |
|
The factory started a review of aedc775 (session |
![arize-phoenix-factory[bot]](https://avatars.githubusercontent.com/in/5483146?s=60&v=4){kind=small}
There was a problem hiding this comment.
Factory review of aedc775: Approve
Every criterion passes: the fix does what it claims, there are no findings, and the new regression tests fail without the fix. Review depth is raised only because the PostgreSQL test run couldn't happen in the sandbox.
| Criterion | Result | Notes |
|---|---|---|
| Fit | Pass | does what it claims |
| Checks | Not run | --db postgresql tests couldn't run; ruff, format, mypy and SQLite tests pass |
| Tests | Pass | both overflow tests fail with OverflowError without the fix |
| Blocking findings | 0 | none |
| Non-blocking findings | 0 | none |
| Risk | Low | backwards compatibility: SQLite statements that crashed before are now refused with a message |
| Review depth | Deep | Checks Not run (PostgreSQL unavailable in sandbox) |
Details
Intent: On SQLite, executeSql should refuse an admitted timestamp literal whose UTC conversion falls outside years 1-9999 (for example '0001-01-01T00:00:00+00:30') with an AnalyticsSqlError(UNSUPPORTED_SYNTAX) envelope, not crash with an uncaught OverflowError.
Fit: The PR guards the only unguarded format_timestamp_for_sqlite call that a caller literal can reach (rewrite.py:384).
Fit: The other call, the epoch path at rewrite.py:422, gets an instant that is already UTC and checked at admission, so it can't overflow.
Fit: Admission doesn't call astimezone, and PostgreSQL never re-renders the literal.
Fit: The area reviewer traced comparison, reversed, BETWEEN, IN, CAST, join ON, compact and bare-offset spellings, and each one now returns the envelope.
Fit: The diff makes no changes beyond what the PR describes.
Verified: ruff check and ruff format --check pass on the changed files and repo-wide. mypy reports no issues on the changed files.
Verified: pytest tests/unit/server/mcp/sql/ tests/unit/server/test_mcp_sql_tools.py on SQLite: 2036 passed, 464 skipped.
Verified: I reverted rewrite.py to the merge base and ran the new tests. Both overflow cases failed with OverflowError: date value out of range. The two boundary cases passed both ways, as expected, since they guard against refusing too much.
Not verified: The --db postgresql runs, because PostgreSQL won't start as root in the sandbox; CI covers them. The diff changes no code on the PostgreSQL path.
Not verified: The full repo-wide make typecheck-python. Frontend checks weren't relevant, since no js/ files changed.
Findings without a diff line: none
Risk: side effects none, performance none, backwards compatibility low, from the area reviewers
Graded against the review criteria.
|
The factory recommends Approve for aedc775. Read the review. |
arize-phoenix-factory
Bot
removed
the
factory-review
Summary
executeSqlon the SQLite backend raised an unhandledOverflowErrorfor an admitted timestamp literal whose conversion to UTC falls outside years 1-9999, instead of returning the documented error envelope.Why
Admission accepts any date-shaped literal that resolves to an instant (
parse_timestamp_literal,src/phoenix/server/mcp/sql/normalize.py:183), so0001-01-01T00:00:00+00:30is admitted. Re-emitting the literal for SQLite then converts the instant to UTC insideformat_timestamp_for_sqlite(src/phoenix/server/mcp/sql/normalize.py:253):astimezonecomputes 0000-12-31 23:30, which adatetimecannot hold, and raisesOverflowError: date value out of rangeat the call site (src/phoenix/server/mcp/sql/rewrite.py:397).That exception is not an
AnalyticsSqlError, so it bypassed the error envelopeexecuteSqldocuments — "Returns either the columns, rows, and applied limits, or an error envelope when the SQL cannot be accepted" (src/phoenix/server/mcp/sql/tools.py:270) — and reached the caller as an internal failure. The same statement on PostgreSQL, which does not rewrite string literals, executes and returns rows. Every sibling path already refuses through the envelope: unreadable or naive timestamps at admission (src/phoenix/server/mcp/sql/parse.py:2079-2101), and Unix-epoch integers that cannot be read (unix_epoch_to_utccatchesOSError, OverflowError, ValueErroratsrc/phoenix/server/mcp/sql/normalize.py:56, and its caller refuses with a clean message).Changes
src/phoenix/server/mcp/sql/rewrite.py: wrap theformat_timestamp_for_sqlitecall for string literals in_normalize_timestamp_literalsintry/except OverflowErrorand raiseAnalyticsSqlError(code=UNSUPPORTED_SYNTAX)naming the UTC boundary spellings to write instead (0001-01-01T00:00:00+00:00,9999-12-31T23:59:59.999999+00:00).tests/unit/server/mcp/sql/test_execute.py: regression tests — a literal at either end whose UTC instant falls outside years 1-9999 is refused withUNSUPPORTED_SYNTAXthrough the envelope rather than crashing, and the first/last UTC instants themselves are still comparable (no over-refusal).Testing
4 passed— both overflow literals refused via the error envelope on SQLite; both UTC boundary literals still compare and return[[3]].pytest tests/unit/server/mcp/sql/— sqlite:2015 passed, 452 skipped;--db postgresql:2219 passed, 248 skipped.pytest tests/unit/A/B against unpatchedmainat the same commit: failure lists identical (20 pre-existing, unrelated modules),+4 passedfrom the new tests.ruff check/ruff format --checkon the changed files — pass;mypy(strict, repo config) — no errors in the changed files.UNSUPPORTED_SYNTAXerror envelope; PostgreSQL returns rows for the same statement.