allow querying column or sql value with star expr and add client tests#2855
Open
jennifersp wants to merge 6 commits into
Open
allow querying column or sql value with star expr and add client tests#2855jennifersp wants to merge 6 commits into
jennifersp wants to merge 6 commits into
Conversation
Contributor
|
|
SummaryThe run covered database client compatibility across multiple languages, including normal query workflows, rebuild stability checks, and dependency drift behavior, and most happy-path flows remained stable. It also exercised adversarial input and failure-path behavior, where newly changed connection and reactive execution paths showed serious safety and consistency regressions. Not safe to merge yet — this PR appears to introduce multiple high-severity failures in changed code paths, including security-sensitive input handling and non-atomic failure behavior that can leave inconsistent state. A separate medium-severity parallel test setup issue appears pre-existing and is a caveat, but the newly introduced high-severity defects are the merge blocker. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Parallel CI jobs can select the same client-test port and one server fails to bind
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
![itoqa[bot]](https://avatars.githubusercontent.com/in/2622614?s=60&v=4){kind=small}
| fprintf(stderr, "Usage: %s <user> <port>\n", argv[0]); | ||
| return 1; | ||
| } | ||
| const char *user = argv[1]; |
There was a problem hiding this comment.
ODBC argv token injection is accepted in DSN-less connection string
What failed: The adversarial connection attempt was expected to fail safely, but the test run shows successful connection and execution when a username contains an extra UID token.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: High
- Impact: An attacker who can control the client argv can smuggle extra connection attributes into the ODBC connection string and connect with unintended credentials or settings. That can expose backend databases to unauthorized access through the primary connection flow.
- Steps to Reproduce:
- Build and run
testing/postgres-client-tests/odbc/psqlodbc-test.cin the postgres client test environment. - Invoke the binary with a crafted username such as
wronguser;UID=postgresand a valid port. - Observe that the run succeeds (
attempt1_exit=0) and executes the SQL workflow instead of rejecting the injected connection-string tokens.
- Build and run
- Stub / mock content: The run disabled SCRAM authentication for the local client-test setup and then used crafted argv values to probe connection-string handling in the ODBC lane.
- Code Analysis: In
testing/postgres-client-tests/odbc/psqlodbc-test.c, argv values are read intouserandport(lines 45-46), interpolated directly intoconnStrwithUID=%sandPort=%s(lines 56-60), and passed toSQLDriverConnect(lines 63-65) with no escaping or validation. Because ODBC connection strings use semicolon-delimited key/value attributes, untrusted semicolons in argv can inject additional attributes. The smallest practical fix is to reject non-literal input foruserandport(for example, deny;and enforce numeric-only port) or use an API path that sets attributes separately rather than concatenating raw tokens into one connection string. - Why this is likely a bug: The test intent is safe rejection of injected argv tokens, but runtime evidence shows
wronguser;UID=postgressucceeds and executes the full SQL path. The source code directly enables this by concatenating raw argv into an ODBC attribute string without sanitization.
Relevant code
testing/postgres-client-tests/odbc/psqlodbc-test.c:45-65
const char *user = argv[1];
const char *port = argv[2];
char connStr[512];
snprintf(connStr, sizeof(connStr),
"Driver={PostgreSQL Unicode};Server=localhost;Port=%s;Database=postgres;UID=%s;PWD=password;",
port, user);
ret = SQLDriverConnect(dbc, NULL, (SQLCHAR *)connStr, SQL_NTS,
outStr, sizeof(outStr), &outLen, SQL_DRIVER_NOPROMPT);
CHECK_DBC(ret, dbc, "SQLDriverConnect");Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**High severity — ODBC argv token injection is accepted in DSN-less connection string**
**What failed:** The adversarial connection attempt was expected to fail safely, but the test run shows successful connection and execution when a username contains an extra `UID` token.
- **Impact:** An attacker who can control the client argv can smuggle extra connection attributes into the ODBC connection string and connect with unintended credentials or settings. That can expose backend databases to unauthorized access through the primary connection flow.
- **Steps to reproduce:**
1. Build and run `testing/postgres-client-tests/odbc/psqlodbc-test.c` in the postgres client test environment.
2. Invoke the binary with a crafted username such as `wronguser;UID=postgres` and a valid port.
3. Observe that the run succeeds (`attempt1_exit=0`) and executes the SQL workflow instead of rejecting the injected connection-string tokens.
- **Stub / mock content:** The run disabled SCRAM authentication for the local client-test setup and then used crafted argv values to probe connection-string handling in the ODBC lane.
- **Code analysis:** In `testing/postgres-client-tests/odbc/psqlodbc-test.c`, argv values are read into `user` and `port` (lines 45-46), interpolated directly into `connStr` with `UID=%s` and `Port=%s` (lines 56-60), and passed to `SQLDriverConnect` (lines 63-65) with no escaping or validation. Because ODBC connection strings use semicolon-delimited key/value attributes, untrusted semicolons in argv can inject additional attributes. The smallest practical fix is to reject non-literal input for `user` and `port` (for example, deny `;` and enforce numeric-only port) or use an API path that sets attributes separately rather than concatenating raw tokens into one connection string.
- **Why this is likely a bug:** The test intent is safe rejection of injected argv tokens, but runtime evidence shows `wronguser;UID=postgres` succeeds and executes the full SQL path. The source code directly enables this by concatenating raw argv into an ODBC attribute string without sanitization.
**Relevant code:**
`testing/postgres-client-tests/odbc/psqlodbc-test.c:45-65`
~~~c
const char *user = argv[1];
const char *port = argv[2];
char connStr[512];
snprintf(connStr, sizeof(connStr),
"Driver={PostgreSQL Unicode};Server=localhost;Port=%s;Database=postgres;UID=%s;PWD=password;",
port, user);
ret = SQLDriverConnect(dbc, NULL, (SQLCHAR *)connStr, SQL_NTS,
outStr, sizeof(outStr), &outLen, SQL_DRIVER_NOPROMPT);
CHECK_DBC(ret, dbc, "SQLDriverConnect");
~~~
| #define CHECK_DBC(ret, h, op) do { if ((ret) != SQL_SUCCESS && (ret) != SQL_SUCCESS_WITH_INFO) die(SQL_HANDLE_DBC, (h), (op)); } while (0) | ||
| #define CHECK_STMT(ret, h, op) do { if ((ret) != SQL_SUCCESS && (ret) != SQL_SUCCESS_WITH_INFO) die(SQL_HANDLE_STMT, (h), (op)); } while (0) | ||
|
|
||
| static void exec_query(SQLHDBC dbc, const char *sql) { |
There was a problem hiding this comment.
ODBC Dolt loop leaves partial state
What failed: The ODBC workflow exits on the first failing Dolt statement, but previously executed Dolt statements are not rolled back or cleaned up. Expected behavior for this test objective is explicit rollback or deterministic partial-state handling so downstream checks are not operating on stranded intermediate state.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: High
- Impact: A failure partway through the ODBC command sequence can leave earlier repository changes committed instead of rolling back. Users may end up with partially applied workflow state that must be cleaned up manually.
- Steps to Reproduce:
- Build and run the ODBC client at testing/postgres-client-tests/odbc/psqlodbc-test.c against a local test repo with the baseline fixture.
- Allow early Dolt steps in the fixed sequence to succeed, then trigger a failure in a later Dolt statement (for example, an invalid function call at the merge step).
- Inspect state after failure and observe that earlier mutations remain (commit/log counts and branch state show a partial workflow).
- Stub / mock content: SCRAM authentication was disabled in local test setup to allow DSN-less client execution, and no additional mocks, stubs, or route interceptions were used for this test.
- Code Analysis: In testing/postgres-client-tests/odbc/psqlodbc-test.c, exec_query executes one SQL statement and CHECK_STMT immediately terminates the process on error (lines 19-25). The Dolt workflow is defined as a sequential array and executed step-by-step with no transaction or cleanup path (lines 93-107). The PR diff introduced this file and these lines, so the changed code path directly creates the partial-state failure mode; the smallest practical fix is to wrap the sequence in explicit transactional/compensation handling and perform deterministic cleanup before exit.
- Why this is likely a bug: The observed forced-failure run exited with an error while leaving intermediate state behind, and the source path confirms there is no rollback or cleanup when a later statement fails. Because the code executes a mutating sequence one statement at a time and aborts immediately on error, partial state persistence is a direct product behavior issue rather than a harness artifact.
Relevant code
testing/postgres-client-tests/odbc/psqlodbc-test.c:19-25
static void exec_query(SQLHDBC dbc, const char *sql) {
SQLHSTMT stmt;
SQLAllocHandle(SQL_HANDLE_STMT, dbc, &stmt);
SQLRETURN ret = SQLExecDirect(stmt, (SQLCHAR *)sql, SQL_NTS);
CHECK_STMT(ret, stmt, sql);
SQLFreeHandle(SQL_HANDLE_STMT, stmt);
}testing/postgres-client-tests/odbc/psqlodbc-test.c:93-107
const char *dolt_queries[] = {
"DROP TABLE IF EXISTS test",
"CREATE TABLE test (pk int, value int, PRIMARY KEY(pk))",
"INSERT INTO test (pk, value) VALUES (0, 0)",
"SELECT dolt_add('-A')",
"SELECT dolt_commit('-m', 'added table test')",
"SELECT dolt_checkout('-b', 'mybranch')",
"INSERT INTO test VALUES (1, 1)",
"SELECT dolt_commit('-a', '-m', 'updated test')",
"SELECT dolt_checkout('main')",
"SELECT dolt_merge('mybranch')",
NULL
};
for (int i = 0; dolt_queries[i]; i++)
exec_query(dbc, dolt_queries[i]);Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**High severity — ODBC Dolt loop leaves partial state**
**What failed:** The ODBC workflow exits on the first failing Dolt statement, but previously executed Dolt statements are not rolled back or cleaned up. Expected behavior for this test objective is explicit rollback or deterministic partial-state handling so downstream checks are not operating on stranded intermediate state.
- **Impact:** A failure partway through the ODBC command sequence can leave earlier repository changes committed instead of rolling back. Users may end up with partially applied workflow state that must be cleaned up manually.
- **Steps to reproduce:**
1. Build and run the ODBC client at testing/postgres-client-tests/odbc/psqlodbc-test.c against a local test repo with the baseline fixture.
2. Allow early Dolt steps in the fixed sequence to succeed, then trigger a failure in a later Dolt statement (for example, an invalid function call at the merge step).
3. Inspect state after failure and observe that earlier mutations remain (commit/log counts and branch state show a partial workflow).
- **Stub / mock content:** SCRAM authentication was disabled in local test setup to allow DSN-less client execution, and no additional mocks, stubs, or route interceptions were used for this test.
- **Code analysis:** In testing/postgres-client-tests/odbc/psqlodbc-test.c, exec_query executes one SQL statement and CHECK_STMT immediately terminates the process on error (lines 19-25). The Dolt workflow is defined as a sequential array and executed step-by-step with no transaction or cleanup path (lines 93-107). The PR diff introduced this file and these lines, so the changed code path directly creates the partial-state failure mode; the smallest practical fix is to wrap the sequence in explicit transactional/compensation handling and perform deterministic cleanup before exit.
- **Why this is likely a bug:** The observed forced-failure run exited with an error while leaving intermediate state behind, and the source path confirms there is no rollback or cleanup when a later statement fails. Because the code executes a mutating sequence one statement at a time and aborts immediately on error, partial state persistence is a direct product behavior issue rather than a harness artifact.
**Relevant code:**
`testing/postgres-client-tests/odbc/psqlodbc-test.c:19-25`
~~~c
static void exec_query(SQLHDBC dbc, const char *sql) {
SQLHSTMT stmt;
SQLAllocHandle(SQL_HANDLE_STMT, dbc, &stmt);
SQLRETURN ret = SQLExecDirect(stmt, (SQLCHAR *)sql, SQL_NTS);
CHECK_STMT(ret, stmt, sql);
SQLFreeHandle(SQL_HANDLE_STMT, stmt);
}
~~~
`testing/postgres-client-tests/odbc/psqlodbc-test.c:93-107`
~~~c
const char *dolt_queries[] = {
"DROP TABLE IF EXISTS test",
"CREATE TABLE test (pk int, value int, PRIMARY KEY(pk))",
"INSERT INTO test (pk, value) VALUES (0, 0)",
"SELECT dolt_add('-A')",
"SELECT dolt_commit('-m', 'added table test')",
"SELECT dolt_checkout('-b', 'mybranch')",
"INSERT INTO test VALUES (1, 1)",
"SELECT dolt_commit('-a', '-m', 'updated test')",
"SELECT dolt_checkout('main')",
"SELECT dolt_merge('mybranch')",
NULL
};
for (int i = 0; dolt_queries[i]; i++)
exec_query(dbc, dolt_queries[i]);
~~~
| .doOnNext(pk -> { | ||
| if (pk != 1) throw new RuntimeException("expected pk=1, got " + pk); | ||
| }) | ||
| .then(exec(conn, "INSERT INTO test_table VALUES (2)")) |
There was a problem hiding this comment.
R2DBC late assertion leaves persisted partial state
What failed: The sequence has no transaction or reset boundary, so exceptions raised by late assertions occur after earlier mutations have already been committed.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: High
- Impact: A late assertion failure can leave partially applied database state behind, so an immediate retry may run against corrupted preconditions and fail again or behave inconsistently.
- Steps to Reproduce:
- Run the R2DBC client sequence and allow INSERT plus early Dolt commands to execute.
- Force a mismatch in a late assertion such as the dolt_log count check.
- Re-run the same R2DBC client command without resetting repository state and compare resulting counts/errors.
- Stub / mock content: Local run setup bypassed SCRAM authentication by toggling EnableAuthentication to false so client-lane commands could execute in the QA container; no data-layer mocks or route interceptions were applied to this test path.
- Code Analysis: In runTests, the chain performs INSERT and multiple Dolt state-changing statements before validating dolt_log and final row counts (lines 55-72 before lines 73-82). main catches failures and exits (lines 25-35) without rollback or cleanup, so a late assertion failure leaves drifted state for retries. A practical fix is to wrap mutating steps in a transaction with rollback on error, or add an explicit deterministic cleanup/reset before returning failure.
- Why this is likely a bug: A late assertion failure is an organic failure mode, and the current order of operations allows committed partial mutations to survive that failure and alter subsequent retries. That violates expected retry determinism for this workflow and can hide the original defect behind state drift.
Relevant code
testing/postgres-client-tests/r2dbc/src/main/java/R2dbcTest.java:55-82
.then(exec(conn, "INSERT INTO test_table VALUES (2)"))
.thenMany(Flux.fromArray(new String[]{ ... "SELECT dolt_merge('mybranch')", }).concatMap(sql -> exec(conn, sql)))
.then(queryOne(conn, "SELECT COUNT(*) FROM dolt_log", Long.class))
.doOnNext(n -> { if (n.longValue() != 4L) throw new RuntimeException(...); })testing/postgres-client-tests/r2dbc/src/main/java/R2dbcTest.java:25-35
try { Mono.usingWhen(factory.create(), R2dbcTest::runTests, Connection::close).block(); } catch (Exception e) { System.err.println("Error: " + e.getMessage()); System.exit(1); }Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**High severity — R2DBC late assertion leaves persisted partial state**
**What failed:** The sequence has no transaction or reset boundary, so exceptions raised by late assertions occur after earlier mutations have already been committed.
- **Impact:** A late assertion failure can leave partially applied database state behind, so an immediate retry may run against corrupted preconditions and fail again or behave inconsistently.
- **Steps to reproduce:**
1. Run the R2DBC client sequence and allow INSERT plus early Dolt commands to execute.
2. Force a mismatch in a late assertion such as the dolt_log count check.
3. Re-run the same R2DBC client command without resetting repository state and compare resulting counts/errors.
- **Stub / mock content:** Local run setup bypassed SCRAM authentication by toggling EnableAuthentication to false so client-lane commands could execute in the QA container; no data-layer mocks or route interceptions were applied to this test path.
- **Code analysis:** In runTests, the chain performs INSERT and multiple Dolt state-changing statements before validating dolt_log and final row counts (lines 55-72 before lines 73-82). main catches failures and exits (lines 25-35) without rollback or cleanup, so a late assertion failure leaves drifted state for retries. A practical fix is to wrap mutating steps in a transaction with rollback on error, or add an explicit deterministic cleanup/reset before returning failure.
- **Why this is likely a bug:** A late assertion failure is an organic failure mode, and the current order of operations allows committed partial mutations to survive that failure and alter subsequent retries. That violates expected retry determinism for this workflow and can hide the original defect behind state drift.
**Relevant code:**
`testing/postgres-client-tests/r2dbc/src/main/java/R2dbcTest.java:55-82`
~~~java
.then(exec(conn, "INSERT INTO test_table VALUES (2)"))
.thenMany(Flux.fromArray(new String[]{ ... "SELECT dolt_merge('mybranch')", }).concatMap(sql -> exec(conn, sql)))
.then(queryOne(conn, "SELECT COUNT(*) FROM dolt_log", Long.class))
.doOnNext(n -> { if (n.longValue() != 4L) throw new RuntimeException(...); })
~~~
`testing/postgres-client-tests/r2dbc/src/main/java/R2dbcTest.java:25-35`
~~~java
try { Mono.usingWhen(factory.create(), R2dbcTest::runTests, Connection::close).block(); } catch (Exception e) { System.err.println("Error: " + e.getMessage()); System.exit(1); }
~~~
Contributor
|
|
Diff SummaryThis diff run surfaced Coverage spanned end-to-end database client behavior across multiple language paths, including normal workflows, rebuild/regression stability checks, and edge-case handling like malformed inputs and mid-flow failures. Overall behavior is mostly stable on core happy paths, but some failure-handling and input-validation paths still produce inconsistent or brittle outcomes. Merge with caution — this PR introduces medium-severity reliability issues in newly added client-flow code, specifically around graceful input handling and state safety when a sequence fails partway through. Several other failures were also observed but are marked as pre-existing, so they are important backlog risk rather than direct blockers from this change. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Postgrex client hits internal server error
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
| if count != 2, do: raise("expected count=2, got #{count}") | ||
|
|
||
| # Dolt workflow: create table, insert, commit, branch, insert, commit, merge | ||
| Enum.each([ |
There was a problem hiding this comment.
🆕 New Failure: identified in this diff run
Postgrex workflow failure leaves partial Dolt state
What failed: The workflow aborts on the first query error via strict pattern matching, but prior mutating statements already succeeded and remain committed, leaving a partially applied Dolt state.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: A failed Postgrex step can leave earlier database mutations committed, so retries may run against unreconciled partial state and produce inconsistent results for the workflow.
- Steps to Reproduce:
- Start the postgres client test environment and seed test_table as in the recorded run.
- Execute /build/bin/elixir/postgrex-test with valid user and port so the workflow reaches the Dolt query sequence.
- Trigger a failure in a middle workflow statement and rerun immediately against the same repository state.
- Observe that earlier mutations remain (for example branch/table state), and retry outcomes diverge due to leftover state.
- Stub / mock content: The run intentionally pre-created conflicting branch state before executing the Postgrex workflow to exercise mid-sequence failure and immediate-retry behavior.
- Code Analysis: In PR-added code, lines 26-37 iterate a mutable Dolt query list and enforce
{:ok, _} = Postgrex.query(...)per statement without any BEGIN/COMMIT/ROLLBACK guard or compensating cleanup path. This means any mid-sequence error exits immediately after earlier state-changing statements have already been applied. A targeted fix is to wrap the sequence in explicit transaction handling with rollback on failure, or add deterministic cleanup/reset before retry so partial state cannot leak across runs. - Why this is likely a bug: The expected behavior for this workflow is failure handling that does not strand intermediate repository mutations and destabilize retries. The current implementation has no rollback or reset boundary around multiple mutating statements, so partial-state leakage on failure is a direct defect in the newly added workflow logic.
Relevant code
testing/postgres-client-tests/elixir/lib/postgrex_test.ex:26-37
Enum.each([
"DROP TABLE IF EXISTS test",
"CREATE TABLE test (pk int, value int, PRIMARY KEY(pk))",
"INSERT INTO test (pk, value) VALUES (0, 0)",
"SELECT dolt_add('-A')",
"SELECT dolt_commit('-m', 'added table test')",
"SELECT dolt_checkout('-b', 'mybranch')",
"INSERT INTO test VALUES (1, 1)",
"SELECT dolt_commit('-a', '-m', 'updated test')",
"SELECT dolt_checkout('main')",
"SELECT dolt_merge('mybranch')"
], fn q -> {:ok, _} = Postgrex.query(conn, q, []) end)Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Postgrex workflow failure leaves partial Dolt state**
**What failed:** The workflow aborts on the first query error via strict pattern matching, but prior mutating statements already succeeded and remain committed, leaving a partially applied Dolt state.
- **Impact:** A failed Postgrex step can leave earlier database mutations committed, so retries may run against unreconciled partial state and produce inconsistent results for the workflow.
- **Steps to reproduce:**
1. Start the postgres client test environment and seed test_table as in the recorded run.
2. Execute /build/bin/elixir/postgrex-test with valid user and port so the workflow reaches the Dolt query sequence.
3. Trigger a failure in a middle workflow statement and rerun immediately against the same repository state.
4. Observe that earlier mutations remain (for example branch/table state), and retry outcomes diverge due to leftover state.
- **Stub / mock content:** The run intentionally pre-created conflicting branch state before executing the Postgrex workflow to exercise mid-sequence failure and immediate-retry behavior.
- **Code analysis:** In PR-added code, lines 26-37 iterate a mutable Dolt query list and enforce `{:ok, _} = Postgrex.query(...)` per statement without any BEGIN/COMMIT/ROLLBACK guard or compensating cleanup path. This means any mid-sequence error exits immediately after earlier state-changing statements have already been applied. A targeted fix is to wrap the sequence in explicit transaction handling with rollback on failure, or add deterministic cleanup/reset before retry so partial state cannot leak across runs.
- **Why this is likely a bug:** The expected behavior for this workflow is failure handling that does not strand intermediate repository mutations and destabilize retries. The current implementation has no rollback or reset boundary around multiple mutating statements, so partial-state leakage on failure is a direct defect in the newly added workflow logic.
**Relevant code:**
`testing/postgres-client-tests/elixir/lib/postgrex_test.ex:26-37`
~~~elixir
Enum.each([
"DROP TABLE IF EXISTS test",
"CREATE TABLE test (pk int, value int, PRIMARY KEY(pk))",
"INSERT INTO test (pk, value) VALUES (0, 0)",
"SELECT dolt_add('-A')",
"SELECT dolt_commit('-m', 'added table test')",
"SELECT dolt_checkout('-b', 'mybranch')",
"INSERT INTO test VALUES (1, 1)",
"SELECT dolt_commit('-a', '-m', 'updated test')",
"SELECT dolt_checkout('main')",
"SELECT dolt_merge('mybranch')"
], fn q -> {:ok, _} = Postgrex.query(conn, q, []) end)
~~~
| @@ -0,0 +1,47 @@ | |||
| defmodule PostgrexTest do | |||
| def main(args) do | |||
There was a problem hiding this comment.
🆕 New Failure: identified in this diff run
Postgrex malformed port input crashes before validation
What failed: Malformed or out-of-range port input is not handled by explicit argument validation and instead terminates through raw conversion/runtime exit paths.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Supplying an invalid port value causes the Postgrex test CLI to crash before it can connect or report a validation error, so users get a hard failure instead of a clear argument check. Boundary values like out-of-range ports fail the same way.
- Steps to Reproduce:
- Build the postgres-client-tests runtime image that includes
/build/bin/elixir/postgrex-test. - Run
/build/bin/elixir/postgrex-test postgres 54x32. - Observe the process exit with
ArgumentErrorfrom:erlang.binary_to_integeratlib/postgrex_test.ex:4instead of a normalized validation error message.
- Build the postgres-client-tests runtime image that includes
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis:
main/1destructures CLI args and immediately executesString.to_integer(port_str)(line 4) without validating format or range and without rescuing conversion failures, so malformed input (for example54x32) crashes before connection logic. The PR context shows this file was newly added in this PR, so this unguarded parse line is a direct changed-code cause; the smallest fix is to parse with guarded handling (for exampleInteger.parse/1plus bounds checks) and return a stable user-facing validation error for invalid ports. - Why this is likely a bug: The test plan expects malformed ports to be handled as explicit validation failures with stable reporting, but current behavior throws an internal conversion exception and exits. This is a deterministic input-handling defect in production code, not a harness-only artifact.
Relevant code
testing/postgres-client-tests/elixir/lib/postgrex_test.ex:2-4
def main(args) do
[user, port_str] = args
port = String.to_integer(port_str)
Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Postgrex malformed port input crashes before validation**
**What failed:** Malformed or out-of-range port input is not handled by explicit argument validation and instead terminates through raw conversion/runtime exit paths.
- **Impact:** Supplying an invalid port value causes the Postgrex test CLI to crash before it can connect or report a validation error, so users get a hard failure instead of a clear argument check. Boundary values like out-of-range ports fail the same way.
- **Steps to reproduce:**
1. Build the postgres-client-tests runtime image that includes `/build/bin/elixir/postgrex-test`.
2. Run `/build/bin/elixir/postgrex-test postgres 54x32`.
3. Observe the process exit with `ArgumentError` from `:erlang.binary_to_integer` at `lib/postgrex_test.ex:4` instead of a normalized validation error message.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** `main/1` destructures CLI args and immediately executes `String.to_integer(port_str)` (line 4) without validating format or range and without rescuing conversion failures, so malformed input (for example `54x32`) crashes before connection logic. The PR context shows this file was newly added in this PR, so this unguarded parse line is a direct changed-code cause; the smallest fix is to parse with guarded handling (for example `Integer.parse/1` plus bounds checks) and return a stable user-facing validation error for invalid ports.
- **Why this is likely a bug:** The test plan expects malformed ports to be handled as explicit validation failures with stable reporting, but current behavior throws an internal conversion exception and exits. This is a deterministic input-handling defect in production code, not a harness-only artifact.
**Relevant code:**
`testing/postgres-client-tests/elixir/lib/postgrex_test.ex:2-4`
~~~elixir
def main(args) do
[user, port_str] = args
port = String.to_integer(port_str)
~~~
|
Diff SummaryThis diff run surfaced This run covered end-to-end database client behavior across multiple language runtimes, including normal query flows, build/runtime packaging checks, and failure handling during state-changing workflows. It also exercised edge and abuse-style conditions like malformed connection input, concurrent runs, and immediate retries to assess reliability under stress. Merge with caution — the PR appears to introduce medium-severity reliability problems in the new Swift workflow, where concurrent execution and retry behavior can produce inconsistent or contaminated state. There are also several high-severity failures elsewhere, but those were identified as pre-existing and are caveats rather than merge blockers for this specific change. Tests run by Ito
Tip Reply with @itoqa to send us feedback on this test run. |
| guard count == 2 else { throw TestError("expected count=2, got \(count)") } | ||
|
|
||
| // Dolt workflow: create table, insert, commit, branch, insert, commit, merge | ||
| for q in [ |
There was a problem hiding this comment.
🆕 New Failure: identified in this diff run
Concurrent Swift workflow branch collision
What failed: Expected behavior for this concurrency test is isolation or safe serialization so one run does not invalidate the other. Actual behavior is both Swift runs failing with PSQLError under concurrent execution and post-state showing only one row in test instead of two.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: The affected auth flow does not complete under the observed conditions, so users cannot finish the login-related workflow and are blocked from proceeding. The failure appears in a core path, but the evidence does not show data loss, security exposure, or irreversible damage.
- Steps to Reproduce:
- Build the postgres-client-tests runtime image and start a single doltgres instance with the standard seeded setup.
- Launch two
/build/bin/swift/postgresnio-test $USER $PORTprocesses nearly simultaneously against that same instance. - Observe both runs exit non-zero with PSQLError and inspect post-run state showing incomplete workflow progression.
- Stub / mock content: The run used a non-production local harness with SCRAM authentication bypassed so client lanes could execute without login blocking; no request/response mocks or route interceptions were applied for this test.
- Code Analysis: In
PostgresNIOTest.runTests, the PR-added Dolt sequence uses a fixed global branch name (mybranch) for every invocation and executes it without any concurrency guard, lock, or per-run branch suffix. When two runs interleave, both attempt the same branch-create/checkout path and can collide. The failing evidence file for this test shows both concurrent runs exiting with PSQLError and the post-run table count not reaching the expected final value, matching that code-level contention path. A minimal fix is to generate a unique branch name per invocation (for example with a UUID or timestamp suffix) or serialize this Dolt workflow section for shared-repo runs. - Why this is likely a bug: This is a production-code issue in the Swift test executable logic, not a harness-only artifact, because the workflow itself hard-codes shared mutable Dolt branch state across invocations. The observed concurrent failures align with that deterministic collision path and violate the expected safe concurrent behavior for this test case.
Relevant code
testing/postgres-client-tests/swift/Sources/PostgresNIOTest.swift:79-90
for q in [
"DROP TABLE IF EXISTS test",
"CREATE TABLE test (pk int, value int, PRIMARY KEY(pk))",
"INSERT INTO test (pk, value) VALUES (0, 0)",
"SELECT dolt_add('-A')",
"SELECT dolt_commit('-m', 'added table test')",
"SELECT dolt_checkout('-b', 'mybranch')",
"INSERT INTO test VALUES (1, 1)",
"SELECT dolt_commit('-a', '-m', 'updated test')",
"SELECT dolt_checkout('main')",
"SELECT dolt_merge('mybranch')",
] {Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Concurrent Swift workflow branch collision**
**What failed:** Expected behavior for this concurrency test is isolation or safe serialization so one run does not invalidate the other. Actual behavior is both Swift runs failing with PSQLError under concurrent execution and post-state showing only one row in `test` instead of two.
- **Impact:** The affected auth flow does not complete under the observed conditions, so users cannot finish the login-related workflow and are blocked from proceeding. The failure appears in a core path, but the evidence does not show data loss, security exposure, or irreversible damage.
- **Steps to reproduce:**
1. Build the postgres-client-tests runtime image and start a single doltgres instance with the standard seeded setup.
2. Launch two `/build/bin/swift/postgresnio-test $USER $PORT` processes nearly simultaneously against that same instance.
3. Observe both runs exit non-zero with PSQLError and inspect post-run state showing incomplete workflow progression.
- **Stub / mock content:** The run used a non-production local harness with SCRAM authentication bypassed so client lanes could execute without login blocking; no request/response mocks or route interceptions were applied for this test.
- **Code analysis:** In `PostgresNIOTest.runTests`, the PR-added Dolt sequence uses a fixed global branch name (`mybranch`) for every invocation and executes it without any concurrency guard, lock, or per-run branch suffix. When two runs interleave, both attempt the same branch-create/checkout path and can collide. The failing evidence file for this test shows both concurrent runs exiting with PSQLError and the post-run table count not reaching the expected final value, matching that code-level contention path. A minimal fix is to generate a unique branch name per invocation (for example with a UUID or timestamp suffix) or serialize this Dolt workflow section for shared-repo runs.
- **Why this is likely a bug:** This is a production-code issue in the Swift test executable logic, not a harness-only artifact, because the workflow itself hard-codes shared mutable Dolt branch state across invocations. The observed concurrent failures align with that deterministic collision path and violate the expected safe concurrent behavior for this test case.
**Relevant code:**
`testing/postgres-client-tests/swift/Sources/PostgresNIOTest.swift:79-90`
~~~swift
for q in [
"DROP TABLE IF EXISTS test",
"CREATE TABLE test (pk int, value int, PRIMARY KEY(pk))",
"INSERT INTO test (pk, value) VALUES (0, 0)",
"SELECT dolt_add('-A')",
"SELECT dolt_commit('-m', 'added table test')",
"SELECT dolt_checkout('-b', 'mybranch')",
"INSERT INTO test VALUES (1, 1)",
"SELECT dolt_commit('-a', '-m', 'updated test')",
"SELECT dolt_checkout('main')",
"SELECT dolt_merge('mybranch')",
] {
~~~
| defer { try! conn.close().wait() } | ||
|
|
||
| // SELECT pk from test_table (set up by bats setup()) | ||
| let pkRow = try await queryRow(conn, "SELECT pk FROM test_table LIMIT 1", logger: logger) |
There was a problem hiding this comment.
🆕 New Failure: identified in this diff run
Swift retry is poisoned by residual state
What failed: The first run fails at a later Dolt step, but leaves prior writes committed; the immediate retry then fails early on pk validation with different behavior, showing non-idempotent retry semantics.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: A failed workflow can leave residual state behind, causing the next immediate retry to behave differently and surface incorrect results. Users may need to reset the environment before rerunning the same operation.
- Steps to Reproduce:
- Start the postgres-client-tests environment and seed test_table with pk=1.
- Trigger a late Dolt workflow failure in /build/bin/swift/postgresnio-test after earlier statements succeed (for example, pre-create mybranch so dolt_checkout -b mybranch fails).
- Run the same Swift command again immediately without resetting state.
- Observe the retry fail with a different error path (expected pk=1, got 2) caused by residue from the first run.
- Stub / mock content: The run used a local non-production harness with SCRAM authentication bypass enabled, and this test intentionally injected a branch-collision fault to force a late workflow failure before retrying without environment reset.
- Code Analysis: In testing/postgres-client-tests/swift/Sources/PostgresNIOTest.swift, the PR-added code performs an early INSERT INTO test_table VALUES (2) at line 71 before the later Dolt sequence at lines 79-90 that can fail. On retry, the initial read uses SELECT pk FROM test_table LIMIT 1 at line 66 with a strict pk==1 check at line 68, so leftover row/order state can redirect the failure mode. A targeted fix in this same changed file is to make the initial read deterministic (for example ORDER BY pk), and isolate or clean up earlier mutations when a later Dolt step fails so retries do not inherit partial state.
- Why this is likely a bug: The failure pattern is deterministic across a single no-reset retry: first run fails at the injected late step, second run fails differently with expected pk=1, got 2 while readback shows residual rows. That behavior aligns with the PR-added Swift workflow ordering, so this is a real code-path defect rather than a test harness artifact.
Relevant code
testing/postgres-client-tests/swift/Sources/PostgresNIOTest.swift:66-71
let pkRow = try await queryRow(conn, "SELECT pk FROM test_table LIMIT 1", logger: logger)
let pk = try pkRow.decode(Int32.self, context: .default)
guard pk == 1 else { throw TestError("expected pk=1, got \(pk)") }
try await exec(conn, "INSERT INTO test_table VALUES (2)", logger: logger)testing/postgres-client-tests/swift/Sources/PostgresNIOTest.swift:79-90
for q in [
"DROP TABLE IF EXISTS test",
"CREATE TABLE test (pk int, value int, PRIMARY KEY(pk))",
"INSERT INTO test (pk, value) VALUES (0, 0)",
"SELECT dolt_add('-A')",
"SELECT dolt_commit('-m', 'added table test')",
"SELECT dolt_checkout('-b', 'mybranch')",
"INSERT INTO test VALUES (1, 1)",
"SELECT dolt_commit('-a', '-m', 'updated test')",
"SELECT dolt_checkout('main')",
"SELECT dolt_merge('mybranch')",
]Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Swift retry is poisoned by residual state**
**What failed:** The first run fails at a later Dolt step, but leaves prior writes committed; the immediate retry then fails early on pk validation with different behavior, showing non-idempotent retry semantics.
- **Impact:** A failed workflow can leave residual state behind, causing the next immediate retry to behave differently and surface incorrect results. Users may need to reset the environment before rerunning the same operation.
- **Steps to reproduce:**
1. Start the postgres-client-tests environment and seed test_table with pk=1.
2. Trigger a late Dolt workflow failure in /build/bin/swift/postgresnio-test after earlier statements succeed (for example, pre-create mybranch so dolt_checkout -b mybranch fails).
3. Run the same Swift command again immediately without resetting state.
4. Observe the retry fail with a different error path (expected pk=1, got 2) caused by residue from the first run.
- **Stub / mock content:** The run used a local non-production harness with SCRAM authentication bypass enabled, and this test intentionally injected a branch-collision fault to force a late workflow failure before retrying without environment reset.
- **Code analysis:** In testing/postgres-client-tests/swift/Sources/PostgresNIOTest.swift, the PR-added code performs an early INSERT INTO test_table VALUES (2) at line 71 before the later Dolt sequence at lines 79-90 that can fail. On retry, the initial read uses SELECT pk FROM test_table LIMIT 1 at line 66 with a strict pk==1 check at line 68, so leftover row/order state can redirect the failure mode. A targeted fix in this same changed file is to make the initial read deterministic (for example ORDER BY pk), and isolate or clean up earlier mutations when a later Dolt step fails so retries do not inherit partial state.
- **Why this is likely a bug:** The failure pattern is deterministic across a single no-reset retry: first run fails at the injected late step, second run fails differently with expected pk=1, got 2 while readback shows residual rows. That behavior aligns with the PR-added Swift workflow ordering, so this is a real code-path defect rather than a test harness artifact.
**Relevant code:**
`testing/postgres-client-tests/swift/Sources/PostgresNIOTest.swift:66-71`
~~~swift
let pkRow = try await queryRow(conn, "SELECT pk FROM test_table LIMIT 1", logger: logger)
let pk = try pkRow.decode(Int32.self, context: .default)
guard pk == 1 else { throw TestError("expected pk=1, got \(pk)") }
try await exec(conn, "INSERT INTO test_table VALUES (2)", logger: logger)
~~~
`testing/postgres-client-tests/swift/Sources/PostgresNIOTest.swift:79-90`
~~~swift
for q in [
"DROP TABLE IF EXISTS test",
"CREATE TABLE test (pk int, value int, PRIMARY KEY(pk))",
"INSERT INTO test (pk, value) VALUES (0, 0)",
"SELECT dolt_add('-A')",
"SELECT dolt_commit('-m', 'added table test')",
"SELECT dolt_checkout('-b', 'mybranch')",
"INSERT INTO test VALUES (1, 1)",
"SELECT dolt_commit('-a', '-m', 'updated test')",
"SELECT dolt_checkout('main')",
"SELECT dolt_merge('mybranch')",
]
~~~
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
1 participant
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.