Staging#582
Merged
Merged
Conversation
- Add detailed assertions for query results (customer names, counts, etc.) - Add tests for filter queries, count aggregation, and joins - Validate SQL query structure and result data - Add session-scoped event loop to fix pytest-asyncio issues - Handle async event loop cleanup errors gracefully with skip - Expand model serialization tests
Disable warnings that are intentional architectural choices: - C0415: import-outside-toplevel (lazy imports for SDK) - W0718: broad-exception-caught (error handling) - R0902: too-many-instance-attributes (dataclasses) - R0903: too-few-public-methods - R0911: too-many-return-statements - R0913/R0917: too-many-arguments (SDK API design) - C0302: too-many-lines
- Extract SDK sync functions to new api/core/text2sql_sync.py module - Split QueryResult into composition: QueryResult + QueryMetadata + QueryAnalysis - Reduce local variables in query_database_sync with helper functions - Fix broad exception handling - use specific Redis/Connection/OS errors - Refactor query method to accept Union[str, QueryRequest] - Add compatibility properties to QueryResult for backwards compatibility - Document lazy imports in client.py module docstring Pylint score improved from 9.81/10 to 9.91/10 Remaining E0401 errors are missing dependencies in venv, not code issues
- Add waitForGraphPresent() polling helper to apiCalls.ts to retry getGraphs() until expected graph appears instead of one-shot calls - Add connectDatabaseWithRetry() helper to retry streaming connection on transient errors with diagnostic logging - Enhance parseStreamingResponse() to log error message details - Update all database.spec.ts tests to use scoped test.setTimeout(120000/180000) - Increase waitForDatabaseConnection timeout to 90s in all DB connection tests - Replace bare getGraphs() calls with waitForGraphPresent() polling - Add console.log diagnostics throughout for easier CI debugging Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
Bumps [playwright](https://github.com/microsoft/playwright-python) from 1.57.0 to 1.58.0. - [Release notes](https://github.com/microsoft/playwright-python/releases) - [Commits](microsoft/playwright-python@v1.57.0...v1.58.0) --- updated-dependencies: - dependency-name: playwright dependency-version: 1.58.0 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
…cific DB predicates, polling for deletion - connectDatabaseWithRetry: wrap per-attempt logic in try/catch so network/parse exceptions don't abort retries; log with attempt# via console.error; backoff delay behaviour unchanged - Add expect(messages.length).toBeGreaterThan(0) guard before accessing finalMessage in all 4 caller blocks (PostgreSQL API, MySQL API, PostgreSQL delete, MySQL delete) - Fix UI-to-API test predicates from generic 'graphs.length > 0' to 'testdb'/'_testdb' match, avoiding false positives on pre-existing graphs - Replace wait(1000)+getGraphs() in both delete tests with waitForGraphPresent polling until the deleted graphId is absent Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
- Rename waitForGraphPresent -> waitForGraphs in apiCalls.ts (more neutral name since it's used for both presence and absence checks) - Update all 10 call sites in database.spec.ts accordingly - Change outer test.describe -> test.describe.serial to prevent cross-test interference on local multi-worker runs (CI is already single-worker via workers: CI ? 1 : undefined in playwright.config.ts) Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
Replace id.includes('testdb_delete') with
id === 'testdb_delete' || id.endsWith('_testdb_delete') in both
delete test predicates and find() calls so only the exact graph forms
('testdb_delete' or '{userId}_testdb_delete') match, preventing
accidental matches on unrelated graph names.
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
…wright-logs Fix flaky Playwright e2e tests: DB connection timeouts and streaming response errors
…ht-1.58.0 Bump playwright from 1.57.0 to 1.58.0
Update dependency versions: - fastapi: ~=0.131.0 → ~=0.133.0 - uvicorn: ~=0.40.0 → ~=0.41.0 - litellm: ~=1.80.9 → ~=1.81.15 - playwright: ~=1.57.0 → ~=1.58.0 - globals (npm): ^15.15.0 → ^17.3.0 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Return generic 400 for RequestValidationError instead of Pydantic details
Add a global RequestValidationError exception handler that returns
{"detail": "Bad request"} with status 400, preventing internal
Pydantic validation details from leaking to clients. This primarily
affects the SPA catch-all proxy route when accessed without the
expected path parameter.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Scope validation handler to SPA catch-all, add logging, fix tests
Address PR review feedback:
- Scope the generic 400 handler to only the SPA catch-all route
(query._full_path errors) so API consumers still get useful 422
responses with field-level detail
- Add logging.warning of validation details for server-side debugging
- Make test assertions unconditional instead of guarding behind
status-code checks
- Add test verifying API routes preserve 422 with field-level info
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Fix SPA catch-all route parameter name mismatch
The function parameter `_full_path` didn't match the URL template
`{full_path:path}`, causing FastAPI to treat it as a required query
parameter and return 422 for every non-API route.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Remove validation error handler workaround
The handler was masking a parameter name mismatch in the catch-all
route. Now that the root cause is fixed, the handler, its import,
pylint suppression, and test file are no longer needed.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Suppress pylint unused-argument for catch-all route parameter
The parameter name must match the URL template to avoid validation
errors, but the function body doesn't use it.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
- Rewrite _email_account_exists as a UNION of two label-scoped, index-friendly lookups (LIMIT 1 per side). The previous chained OPTIONAL MATCH could form a Cartesian product (review feedback), and an aggregating WITH variant returned NULL under FalkorDB parameter binding, which would have made the check fail open. Verified the UNION query against FalkorDB for present/absent/identity-only cases. - Add tests pinning the helper's result interpretation (non-empty -> exists, empty -> not exists, query error propagates so the endpoint fails closed). - Regenerate root package-lock.json so `npm ci` is in sync with app/package.json after the @vitejs/plugin-react-swc bump (#554), unblocking the Playwright job. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses CodeRabbit review: per AGENTS.md and the strict-markers config, test functions must carry a registered marker. Apply unit and auth module-wide. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-Authored-By: Copilot <223556219+Copilot@users.noreply.github.com>
…-bypass fix(auth): prevent signup token issuance for existing accounts (CVE-2026-10130)
Completed Working on "Code Review"✅ Review publishing completed successfully. Review submitted: COMMENT. Total comments: 1 across 1 files. ✅ Workflow completed successfully. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![overcut-ai[bot]](https://avatars.githubusercontent.com/in/1367736?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Findings by importance
- 1 MAJOR
- 0 BLOCKER, 0 CRITICAL, 0 MINOR, 0 SUGGESTION, 0 PRAISE
Key theme
- CI supply-chain hardening is incomplete: one workflow still uses floating action versions instead of immutable pins.
Actionable next step
- In
.github/workflows/tests.yml, pinactions/checkoutandactions/setup-pythonin thesdk-testsjob to specific commit SHAs (consistent with the other workflow hardening changes in this PR).
railway-app
Bot
temporarily deployed
to
queryweaver / QueryWeaver-pr-582
|
🚅 Deployed to the QueryWeaver-pr-582 environment in queryweaver
|
- CodeQL py/log-injection (alerts 251/252/253): reorder `_sanitize_for_log`
so the returned value comes from `.replace('\n', '')`. CodeQL's
ReplaceLineBreaksSanitizer only recognises a `.replace` whose first arg is
"\n" or "\r\n"; the previous outermost call used "\r", so the helper's
return value was not treated as sanitised. Behaviour is unchanged (all
CR/LF still stripped).
- CI hardening (overcut-ai): pin `actions/checkout` and `actions/setup-python`
in the tests.yml `sdk-tests` job to the same commit SHAs already used by the
`unit-tests` job, instead of floating @v6 tags.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…583) Guard the non-string `str(value)` conversion with try/except and fall back to '<unprintable>' so log sanitisation can never raise into the auth flow if a custom __str__ throws. Preserves the CodeQL-recognised line-break sanitizer (returned value still comes from `.replace('\n', '')`). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
fix: address AI review findings on staging promotion (#582)
railway-app
Bot
temporarily deployed
to
queryweaver / QueryWeaver-pr-582
railway-app
Bot
temporarily deployed
to
queryweaver / QueryWeaver-pr-582
Dependency ReviewThe following issues were found:
|
…A-r95x-qfjj-fjj2) authlib 1.7.0 is vulnerable to an unauthenticated open redirect in the OpenIDImplicitGrant/OpenIDHybridGrant authorization endpoint (moderate). 1.7.1+ is patched; resolves to 1.7.2 within the existing ~=1.7.0 constraint. Clears the failing Dependency Review check on the staging->main promotion (#582). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
railway-app
Bot
temporarily deployed
to
queryweaver / QueryWeaver-pr-582
galshubeli
approved these changes
May 31, 2026
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.
No description provided.