feat: rename iii-database to database#169
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR comprehensively renames the ChangesWorker Renaming and API Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
skill-check — worker1 verified, 10 skipped (no docs/). 1 error across the verified workers.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
database/src/handlers/transaction_query.rs (1)
36-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
InvalidParamfor empty SQL instead ofDriverError.This is handler-level input validation, so returning
DriverErrormisclassifies client input as backend failure. Please returnDbError::InvalidParam(index/field forsql) to keep error semantics consistent with the transaction-control guard path.Suggested diff
- if req.sql.trim().is_empty() { - return Err(err_to_str(DbError::DriverError { - driver: "(tx)".into(), - code: None, - message: "empty SQL".into(), - failed_index: None, - })); - } + if req.sql.trim().is_empty() { + return Err(err_to_str(DbError::InvalidParam { + index: 0, + reason: "empty SQL".into(), + })); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/src/handlers/transaction_query.rs` around lines 36 - 43, Replace the current DbError::DriverError return for empty SQL in transaction_query.rs with DbError::InvalidParam to reflect client input validation: when req.sql.trim().is_empty() return Err(err_to_str(DbError::InvalidParam { index: Some("sql".into()), field: Some("sql".into()), message: "empty SQL".into() })); locate the empty-SQL guard in the transaction SQL handler (the block that checks req.sql.trim().is_empty()) and update the error variant and payload accordingly so the handler-level validation matches the transaction-control guard semantics.
🧹 Nitpick comments (1)
database/tests/e2e/workers/harness/src/test.ts (1)
3-17: ⚡ Quick wintest.ts is not currently wired into the harness execution; consider removing or integrating it as a proper test case.
test.ts exists as an orphaned file and is not imported by any case file, npm script, or executed as part of the harness. If this is intended as a manual example or future test case, the SQL portability concerns are valid:
INTEGER PRIMARY KEYis SQLite-specific (ROWID aliasing), and bare?placeholders without explicit column mapping could have portability issues when integrated. If the file is to remain dormant, no action is needed. If it is to become an active test, apply the proposed fixes for multi-driver compatibility (useINT PRIMARY KEY, explicitly list columns in INSERT, and add result assertions).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/tests/e2e/workers/harness/src/test.ts` around lines 3 - 17, test.ts is an orphaned harness file; either delete it or wire it into the test harness (import it from a case file or add an npm script) and make its SQL portable and assertive: replace SQLite-specific "CREATE TABLE IF NOT EXISTS users (id INTEGER PRIMARY KEY, ...)" with a portable "INT PRIMARY KEY" form, update the INSERT to explicitly list columns and use explicit placeholders per value (e.g., INSERT INTO users (email) VALUES (?), (?) → use one placeholder per value and matching params or convert to two separate rows with matching params), ensure call('database::execute') and call('database::query') are used with matching params counts, and add assertions on the returned rows from call('database::query') to validate inserted data before enabling this file in the harness.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/database-e2e.yml:
- Around line 36-41: Replace the mutable action ref "uses:
actions/setup-node@v4" with a pinned commit SHA ref (e.g., "uses:
actions/setup-node@<commit-SHA>") to satisfy the unpinned-uses policy; update
the line that contains actions/setup-node@v4 in the workflow to the chosen
commit SHA and repeat the same pinning for other workflow steps that use mutable
tags to ensure all GitHub Action refs are pinned.
In `@database/Cargo.toml`:
- Line 4: The package rename removed the explicit library crate name so imports
like iii_database:: now fail; add an explicit [lib] section in Cargo.toml
setting name = "iii_database" (so the library crate keeps the old symbol) to
restore those iii_database:: imports, and ensure src/lib.rs exists as the
library entrypoint referenced by that lib definition.
In `@database/skills/iii-database/transaction.md`:
- Line 164: The sentence about `database::query` is misleading; update the docs
to state that `database::query` is a separate read-only API and should not be
treated as part of the `database::transaction` surface; inside
`database::transaction` callers must include raw SELECT statements in the
`statements` array and expect result rows to follow the transaction response
shape (not the `database::query` envelope), and if you only need row counts use
the transaction's `affected_rows`-equivalent handling rather than referring to
`database::query`.
In `@database/tests/e2e/workers/harness/src/runner.ts`:
- Line 105: The test harness currently calls waitForDatabaseWorker only for
drivers[0], so later drivers may not be ready; update runner.ts to wait for
every driver in the drivers array by iterating over drivers and awaiting
waitForDatabaseWorker(driver) for each (either sequentially with for...of await
or concurrently with Promise.all), referencing the existing
waitForDatabaseWorker function and the drivers variable so every driver is
readiness-checked before tests run.
---
Outside diff comments:
In `@database/src/handlers/transaction_query.rs`:
- Around line 36-43: Replace the current DbError::DriverError return for empty
SQL in transaction_query.rs with DbError::InvalidParam to reflect client input
validation: when req.sql.trim().is_empty() return
Err(err_to_str(DbError::InvalidParam { index: Some("sql".into()), field:
Some("sql".into()), message: "empty SQL".into() })); locate the empty-SQL guard
in the transaction SQL handler (the block that checks req.sql.trim().is_empty())
and update the error variant and payload accordingly so the handler-level
validation matches the transaction-control guard semantics.
---
Nitpick comments:
In `@database/tests/e2e/workers/harness/src/test.ts`:
- Around line 3-17: test.ts is an orphaned harness file; either delete it or
wire it into the test harness (import it from a case file or add an npm script)
and make its SQL portable and assertive: replace SQLite-specific "CREATE TABLE
IF NOT EXISTS users (id INTEGER PRIMARY KEY, ...)" with a portable "INT PRIMARY
KEY" form, update the INSERT to explicitly list columns and use explicit
placeholders per value (e.g., INSERT INTO users (email) VALUES (?), (?) → use
one placeholder per value and matching params or convert to two separate rows
with matching params), ensure call('database::execute') and
call('database::query') are used with matching params counts, and add assertions
on the returned rows from call('database::query') to validate inserted data
before enabling this file in the harness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6d49e96-d9c8-42d3-8227-5edb1a608a59
⛔ Files ignored due to path filters (2)
database/Cargo.lockis excluded by!**/*.lockdatabase/tests/e2e/workers/harness/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (81)
.github/workflows/create-tag.yml.github/workflows/database-e2e.yml.github/workflows/release.yml.gitignoreREADME.mddatabase/.gitignoredatabase/Cargo.tomldatabase/README.mddatabase/config.yamldatabase/config.yaml.exampledatabase/iii.worker.yamldatabase/skills/iii-database/execute.mddatabase/skills/iii-database/interactive-transaction.mddatabase/skills/iii-database/prepared-statements.mddatabase/skills/iii-database/query.mddatabase/skills/iii-database/transaction.mddatabase/skills/index.mddatabase/src/config.rsdatabase/src/driver/mod.rsdatabase/src/driver/mysql.rsdatabase/src/driver/postgres.rsdatabase/src/driver/sqlite.rsdatabase/src/error.rsdatabase/src/handle.rsdatabase/src/handlers/begin_transaction.rsdatabase/src/handlers/commit_transaction.rsdatabase/src/handlers/execute.rsdatabase/src/handlers/mod.rsdatabase/src/handlers/prepare.rsdatabase/src/handlers/query.rsdatabase/src/handlers/rollback_transaction.rsdatabase/src/handlers/run_statement.rsdatabase/src/handlers/transaction.rsdatabase/src/handlers/transaction_execute.rsdatabase/src/handlers/transaction_query.rsdatabase/src/handlers/tx_sql_guard.rsdatabase/src/lib.rsdatabase/src/main.rsdatabase/src/pool/mod.rsdatabase/src/pool/mysql.rsdatabase/src/pool/postgres.rsdatabase/src/pool/sqlite.rsdatabase/src/pool/tls.rsdatabase/src/transaction.rsdatabase/src/triggers/handler.rsdatabase/src/triggers/mod.rsdatabase/src/triggers/row_change.rsdatabase/src/value.rsdatabase/tests/e2e/.gitignoredatabase/tests/e2e/README.mddatabase/tests/e2e/config.yamldatabase/tests/e2e/docker-compose.ymldatabase/tests/e2e/reports/.gitkeepdatabase/tests/e2e/run-tests.shdatabase/tests/e2e/workers/harness/iii.worker.yamldatabase/tests/e2e/workers/harness/package.jsondatabase/tests/e2e/workers/harness/src/cases-boundary.tsdatabase/tests/e2e/workers/harness/src/cases-concurrency.tsdatabase/tests/e2e/workers/harness/src/cases-interactive-tx.tsdatabase/tests/e2e/workers/harness/src/cases-protocol.tsdatabase/tests/e2e/workers/harness/src/cases-row-change.tsdatabase/tests/e2e/workers/harness/src/cases-transaction.tsdatabase/tests/e2e/workers/harness/src/cases-tx-control-bypass.tsdatabase/tests/e2e/workers/harness/src/cases.tsdatabase/tests/e2e/workers/harness/src/dialect.test.tsdatabase/tests/e2e/workers/harness/src/dialect.tsdatabase/tests/e2e/workers/harness/src/runner.tsdatabase/tests/e2e/workers/harness/src/test.tsdatabase/tests/e2e/workers/harness/src/worker.tsdatabase/tests/e2e/workers/harness/tsconfig.jsondatabase/tests/integration.rsdatabase/tests/value_coercion.rsiii-database/skills/index.mdiii-database/tests/e2e/reports/.gitkeepiii-database/tests/e2e/workers/harness/src/test.tsshell/tests/e2e/README.mdstorage/src/error.rsstorage/src/handlers/mod.rsstorage/src/triggers/dispatcher.rsstorage/tests/e2e/config.yamlstorage/tests/e2e/run-tests.sh
💤 Files with no reviewable changes (2)
- iii-database/skills/index.md
- iii-database/tests/e2e/workers/harness/src/test.ts
| - name: Install Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '20' | ||
| cache: 'npm' | ||
| cache-dependency-path: iii-database/tests/e2e/workers/harness/package-lock.json | ||
| cache-dependency-path: database/tests/e2e/workers/harness/package-lock.json |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
fd -e yml -e yaml .github/workflows | while read -r wf; do
echo "== $wf =="
rg -n '^\s*uses:\s*[^@\s]+@[^\s#]+' "$wf" \
| rg -n '@(v[0-9]+(\.[0-9]+)?|main|master)$' || true
doneRepository: iii-hq/workers
Length of output: 432
🏁 Script executed:
# First, let's verify the specific file and line numbers
cat -n .github/workflows/database-e2e.yml | sed -n '30,45p'Repository: iii-hq/workers
Length of output: 726
🏁 Script executed:
# Run the corrected command to find all tag/branch-pinned actions in workflows
fd . '.github/workflows' -e yml -e yaml | while read -r wf; do
echo "== $wf =="
rg -n '^\s*uses:\s*[^@\s]+@[^\s#]+' "$wf" \
| rg '@(v[0-9]+(\.[0-9]+)?|main|master)$' || true
doneRepository: iii-hq/workers
Length of output: 1409
Pin GitHub Action refs to commit SHAs.
Line 37 uses actions/setup-node@v4 (mutable version tag), which violates strict workflow hardening practices and can fail unpinned-uses policy checks. Convert to a pinned commit SHA (e.g., actions/setup-node@1e21ff14b671a668ad81a1eac58d86bc5915f5a6).
Note: This pattern appears across multiple workflows—consider auditing all workflow files to pin GitHub Actions consistently.
🧰 Tools
🪛 zizmor (1.25.2)
[error] 37-37: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/database-e2e.yml around lines 36 - 41, Replace the mutable
action ref "uses: actions/setup-node@v4" with a pinned commit SHA ref (e.g.,
"uses: actions/setup-node@<commit-SHA>") to satisfy the unpinned-uses policy;
update the line that contains actions/setup-node@v4 in the workflow to the
chosen commit SHA and repeat the same pinning for other workflow steps that use
mutable tags to ensure all GitHub Action refs are pinned.
| - `iii-database::prepareStatement` + `iii-database::runStatement` — for repeating one parameterized statement many times; not a substitute for atomic multi-statement commit. | ||
| - [`database::beginTransaction` + `transactionQuery` / `transactionExecute` + `commitTransaction` / `rollbackTransaction`](iii://database/interactive-transaction) — **stateful interactive** transaction with a configurable timeout-driven auto-rollback. Use this surface when you need to take a decision in application code *between* statements (read-your-writes across round-trips). The batch handler on this page requires every statement up-front, so it can't carry that inter-statement logic. | ||
| - `database::execute` — single-statement variant; skips the BEGIN/COMMIT framing for one-shot writes. | ||
| - `database::query` — read-only; cannot be combined with writes inside this surface but is fine to mix into the `statements` array if you only need its rows for `affected_rows`-equivalent counts. |
There was a problem hiding this comment.
Clarify database::query guidance to avoid contradictory transaction semantics.
This line currently says database::query is outside this surface, but then suggests mixing it into statements with affected_rows-equivalent behavior. Inside database::transaction, callers should add SELECT SQL directly to statements, and result rows follow the transaction response shape (not database::query’s envelope).
✏️ Suggested doc fix
-- `database::query` — read-only; cannot be combined with writes inside this surface but is fine to mix into the `statements` array if you only need its rows for `affected_rows`-equivalent counts.
+- `database::query` — read-only standalone surface. Within `database::transaction`, include read SQL directly in `statements` when needed; results come back in the transaction `results` shape.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `database::query` — read-only; cannot be combined with writes inside this surface but is fine to mix into the `statements` array if you only need its rows for `affected_rows`-equivalent counts. | |
| - `database::query` — read-only standalone surface. Within `database::transaction`, include read SQL directly in `statements` when needed; results come back in the transaction `results` shape. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@database/skills/iii-database/transaction.md` at line 164, The sentence about
`database::query` is misleading; update the docs to state that `database::query`
is a separate read-only API and should not be treated as part of the
`database::transaction` surface; inside `database::transaction` callers must
include raw SELECT statements in the `statements` array and expect result rows
to follow the transaction response shape (not the `database::query` envelope),
and if you only need row counts use the transaction's `affected_rows`-equivalent
handling rather than referring to `database::query`.
| const drivers: DriverKey[] = this.opts.filterDriver ? [this.opts.filterDriver] : [...DRIVER_KEYS] | ||
| // Wait for the database worker to be reachable on the first driver before kicking off. | ||
| await this.waitForDatabaseWorker(drivers[0]); | ||
| await this.waitForDatabaseWorker(drivers[0]) |
There was a problem hiding this comment.
Wait for each driver, not only the first one.
Readiness is checked only for drivers[0], but tests execute against every driver. Later drivers can still race startup and fail/skip their suite spuriously.
Suggested fix
- // Wait for the database worker to be reachable on the first driver before kicking off.
- await this.waitForDatabaseWorker(drivers[0])
+ // Wait for each driver right before running its cases.
const results: CaseResult[] = []
@@
for (const driver of drivers) {
+ await this.waitForDatabaseWorker(driver)
if (includeFullSuite) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await this.waitForDatabaseWorker(drivers[0]) | |
| // Wait for each driver right before running its cases. | |
| const results: CaseResult[] = [] | |
| for (const driver of drivers) { | |
| await this.waitForDatabaseWorker(driver) | |
| if (includeFullSuite) { | |
| // ... rest of the code | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@database/tests/e2e/workers/harness/src/runner.ts` at line 105, The test
harness currently calls waitForDatabaseWorker only for drivers[0], so later
drivers may not be ready; update runner.ts to wait for every driver in the
drivers array by iterating over drivers and awaiting
waitForDatabaseWorker(driver) for each (either sequentially with for...of await
or concurrently with Promise.all), referencing the existing
waitForDatabaseWorker function and the drivers variable so every driver is
readiness-checked before tests run.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
iii-databasetodatabase; all API functions and triggers now use thedatabase::namespace.