[peco-engineer-bot]

Summary

Exposes the server SQLSTATE as a top-level sqlState on Thrift OperationStateError, matching StatusError and the kernel backend. Partially addresses #33.

sqlState is read only from response.sqlState. response.status is the GetOperationStatus RPC status, and by the time this error is thrown it has passed Status.assert, so it is always a success status and its sqlState says nothing about the failed query. The property is non-enumerable, with the same descriptor the kernel backend's defineErrorMetadata uses. The errorCode enum is unchanged.

This doesn't fully resolve #33: for the payload in that issue (sqlState: null at both levels) nothing changes. The status.statusCode and errorCode of 0 that the reporter saw are set by the server and only mean the status RPC succeeded.

Test plan

• Unit (tests/unit/errors/OperationStateError.test.ts): top-level value is exposed, status.sqlState is ignored, the property descriptor matches the kernel backend, and no property is defined when the server sends no SQLSTATE.
• e2e (tests/e2e/operation_error_codes.test.ts): SELECT raise_error('boom') on a live warehouse yields an OperationStateError whose sqlState is the 5-character wire response.sqlState. Last run against a live warehouse before 49c9058, which only narrowed the expected value to response.sqlState.

🤖 Generated by engineer-bot (bug-fix flow) — review before merge.

This pull request and its description were written by Isaac.

[peco-review-bot]

Verdict: 1 Low · 2 Nit

The change looks correct and small: it exposes the server SQLSTATE at the top level of Thrift OperationStateError. I have three low-severity notes: the new property is enumerable on Thrift but non-enumerable on the kernel path, an empty-string sqlState doesn't fall back to status.sqlState, and the tests use casts that are no longer needed and don't cover the no-SQLSTATE case.

Other findings

• ⚪ Nit — response?.sqlState ?? response?.status?.sqlState only falls back when the top-level value is null or undefined. If the server sends sqlState: '' (an empty string, which the Thrift optional string allows), there's no fallback to status.sqlState, and the if (sqlState) check then leaves the field unset. If you meant to skip empty values (which the truthiness check suggests), || matches that intent better than ??. A unit test for the empty-string case would pin down the behaviour.
• ⚪ Nit — sqlState is now a declared public field on OperationStateError, so the as OperationStateError & { sqlState?: string } casts here (and the matching one at tests/e2e/operation_error_codes.test.ts:43) do nothing. They also hide a regression: if the field declaration were removed, these tests would still type-check. Drop the casts so the compiler checks the public contract. There's also no test for a response with no SQLSTATE at all (and for new OperationStateError(code) with no response) that asserts sqlState is undefined.

[peco-review-bot]

Verdict: 2 Nit

Looks good. sqlState is now exposed on Thrift OperationStateError using the same non-enumerable descriptor as the kernel path, and the unit tests cover the primary value, the fallback, descriptor parity, and the case where the server sends no SQLSTATE. The tsconfig target is ES2018 and useDefineForClassFields isn't set, so the sqlState field declaration doesn't create an own enumerable property that would conflict with the defineProperty call. I found only two nits.

Other findings

• ⚪ Nit — The as OperationStateError & { sqlState?: string } casts in both tests are no longer needed now that sqlState is declared on the class (lib/errors/OperationStateError.ts:26).
• ⚪ Nit — The Object.defineProperty descriptor copies defineErrorMetadata in lib/kernel/KernelErrorMapping.ts:105-112. The guard also differs: here it is if (sqlState), while the kernel uses !== undefined, so an empty-string SQLSTATE is handled differently on each backend. You could move the helper into a shared module so both backends stay identical.

[peco-review-bot]

Verdict: 1 Low

Looks good overall, with one low-severity inconsistency. The change is small, backward-compatible (it only adds an optional, non-enumerable field) and covered by both unit and e2e tests. Nit: public sqlState?: string is now declared on the class, so the as OperationStateError & { sqlState?: string } casts in the tests are redundant.