feat(cli): reject --reset --schema awa and document substrate ownership#313
Conversation
…ship The default `awa` schema holds both the queue-storage substrate and the canonical migration tables (schema_version, runtime_instances, storage_transition_state, runtime_storage_backends, ...). A `DROP SCHEMA awa CASCADE` would take the migration tables with it and leave the database unrecoverable, so `awa storage prepare-queue-storage-schema --schema awa --reset` is now hard-rejected with an error that points operators at the custom-schema path or `awa storage abort`. Adds docs/queue-storage-substrate.md as the canonical ownership contract: who installs what (`awa migrate` for the default schema plus all canonical tables; `prepare_schema()` / the CLI for custom schemas), the default-shape requirements the helper enforces for `p_schema = 'awa'`, and the reset-guard semantics. Cross-linked from migrations.md, architecture.md, and security.md. Also clarifies the `to_regclass` guard in v022 — the counter table is materialised by the v023 migration, so the guard is a no-op on the default install; it still matters for custom schemas, partially-prepared substrates, and external migration tools that apply DDL out of order.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe PR adds a CLI guard refusing ChangesQueue-storage substrate reset guard and ownership documentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1c89abd86
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .connect(&database_url()) | ||
| .await | ||
| .expect("connect"); | ||
| let version_before: i32 = sqlx::query_scalar("SELECT MAX(version) FROM awa.schema_version") |
There was a problem hiding this comment.
In the CI test job I checked (.github/workflows/ci.yml runs cargo test --workspace against a fresh Postgres service without a prior migrate step), this new test connects directly and immediately queries awa.schema_version. If this test binary runs before any other test happens to call the shared setup helper, or if it is run by itself, the awa schema/table does not exist and the test panics before exercising the reset guard. Please run the normal migration/setup path in this test before asserting on schema_version.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
awa-cli/src/main.rs (1)
888-888:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUnsafe identifier interpolation in
DROP SCHEMAquery.
schemais user-controlled and interpolated directly into SQL. This allows SQL injection via crafted schema names.Suggested fix
- if reset { - sqlx::query(&format!("DROP SCHEMA IF EXISTS {schema} CASCADE")) - .execute(&pool) - .await?; - } + if reset { + // Quote as SQL identifier safely. + let quoted_schema: String = + sqlx::query_scalar("SELECT pg_catalog.quote_ident($1)") + .bind(&schema) + .fetch_one(&pool) + .await?; + let drop_sql = format!("DROP SCHEMA IF EXISTS {quoted_schema} CASCADE"); + sqlx::query(&drop_sql).execute(&pool).await?; + }🤖 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 `@awa-cli/src/main.rs` at line 888, The interpolated DROP SCHEMA string sqlx::query(&format!("DROP SCHEMA IF EXISTS {schema} CASCADE")) is vulnerable to SQL injection; fix it by either validating/sanitizing the user-controlled schema identifier (e.g., enforce a strict regex like ^[A-Za-z0-9_]+$ and return an error for invalid names) or safely quote/escape the identifier before interpolation (replace any " with "" and wrap the name in double quotes) and then use that safe identifier in the format call; update the code where sqlx::query(... "DROP SCHEMA IF EXISTS {schema} CASCADE" ...) is constructed to perform one of these protections before executing the query.
🤖 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 `@awa-cli/src/main.rs`:
- Around line 870-880: The current guard checks schema == "awa" which only
blocks that exact casing; update the if condition that references reset and
schema so it compares case-insensitively (e.g., use
schema.eq_ignore_ascii_case("awa") or compare schema.to_ascii_lowercase() ==
"awa") and also consider trimming whitespace if inputs may include it, ensuring
the destructive DROP path is blocked for any case variant of "awa" in the same
if block where reset and schema are evaluated.
In `@docs/queue-storage-substrate.md`:
- Around line 69-75: The fenced error-output block containing the multi-line
Error: "Refusing to DROP SCHEMA awa CASCADE …" needs a markdown fence language
tag (markdownlint MD040); update that fenced code block to start with ```text
(or ```console) so the block is explicitly marked as plain text and the docs
lint will pass.
---
Outside diff comments:
In `@awa-cli/src/main.rs`:
- Line 888: The interpolated DROP SCHEMA string sqlx::query(&format!("DROP
SCHEMA IF EXISTS {schema} CASCADE")) is vulnerable to SQL injection; fix it by
either validating/sanitizing the user-controlled schema identifier (e.g.,
enforce a strict regex like ^[A-Za-z0-9_]+$ and return an error for invalid
names) or safely quote/escape the identifier before interpolation (replace any "
with "" and wrap the name in double quotes) and then use that safe identifier in
the format call; update the code where sqlx::query(... "DROP SCHEMA IF EXISTS
{schema} CASCADE" ...) is constructed to perform one of these protections before
executing the query.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57fe9f32-17e5-435d-b078-efa5852b805b
📒 Files selected for processing (7)
awa-cli/src/main.rsawa-cli/tests/prepare_schema_reset_guard_test.rsawa-model/migrations/v022_delete_compat_terminal_counter.sqldocs/architecture.mddocs/migrations.mddocs/queue-storage-substrate.mddocs/security.md
| if reset && schema == "awa" { | ||
| return Err( | ||
| "Refusing to DROP SCHEMA awa CASCADE — schema 'awa' is the \ | ||
| default migration-owned queue-storage substrate and also \ | ||
| contains the canonical migration tables (schema_version, \ | ||
| runtime_instances, storage_transition_state, etc.). \ | ||
| Use --schema <other> for a throwaway substrate, or \ | ||
| 'awa storage abort' to rewind an in-flight transition." | ||
| .into(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Guard can be bypassed with case variants of awa.
schema == "awa" only blocks one exact casing. --schema AWA --reset still maps to awa in PostgreSQL (unquoted identifier folding), so the destructive path is still reachable.
Suggested fix
- if reset && schema == "awa" {
+ let is_default_awa = schema.eq_ignore_ascii_case("awa");
+ if reset && is_default_awa {📝 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.
| if reset && schema == "awa" { | |
| return Err( | |
| "Refusing to DROP SCHEMA awa CASCADE — schema 'awa' is the \ | |
| default migration-owned queue-storage substrate and also \ | |
| contains the canonical migration tables (schema_version, \ | |
| runtime_instances, storage_transition_state, etc.). \ | |
| Use --schema <other> for a throwaway substrate, or \ | |
| 'awa storage abort' to rewind an in-flight transition." | |
| .into(), | |
| ); | |
| } | |
| let is_default_awa = schema.eq_ignore_ascii_case("awa"); | |
| if reset && is_default_awa { | |
| return Err( | |
| "Refusing to DROP SCHEMA awa CASCADE — schema 'awa' is the \ | |
| default migration-owned queue-storage substrate and also \ | |
| contains the canonical migration tables (schema_version, \ | |
| runtime_instances, storage_transition_state, etc.). \ | |
| Use --schema <other> for a throwaway substrate, or \ | |
| 'awa storage abort' to rewind an in-flight transition." | |
| .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 `@awa-cli/src/main.rs` around lines 870 - 880, The current guard checks schema
== "awa" which only blocks that exact casing; update the if condition that
references reset and schema so it compares case-insensitively (e.g., use
schema.eq_ignore_ascii_case("awa") or compare schema.to_ascii_lowercase() ==
"awa") and also consider trimming whitespace if inputs may include it, ensuring
the destructive DROP path is blocked for any case variant of "awa" in the same
if block where reset and schema are evaluated.
1 participant
Summary
The default
awaschema holds both the queue-storage substrate and the canonical migration tables (schema_version,runtime_instances,storage_transition_state,runtime_storage_backends, ...). ADROP SCHEMA awa CASCADEwould take the migration tables with it and leave the database unrecoverable.This PR is the third of three landing the v0.6 substrate-ownership work:
awa storage prepare-queue-storage-schema --schema awa --resetis now hard-rejected with an error pointing operators at the custom-schema escape (--schema <other>) or the in-flight transition rewind (awa storage abort).docs/queue-storage-substrate.md— who installs what (awa migratefor the default schema plus all canonical tables;prepare_schema()/ the CLI for custom schemas), the default-shape requirements the helper enforces forp_schema = 'awa', and the reset-guard semantics. Cross-linked frommigrations.md,architecture.md, andsecurity.md.to_regclassguard indelete_job_compatis a no-op on the default install (v023 materialises the counter table via migrate); it still matters for custom schemas, partially-prepared substrates, and external migration tooling that applies DDL out of order.Test plan
cargo fmt --allcargo clippy --workspace --all-targets --all-features -- -D warningscargo build --workspacecargo test -p awa-cliagainst local Postgres — newprepare_schema_reset_guard_testpasses both cases (rejection error message contains all three required hints; rejected reset does not mutateawa.schema_version); existingstorage_finalize_cli_testand other CLI tests still greenawa storage prepare-queue-storage-schema --schema awa --resetrejected with the canonical-schema error;--schema <other> --resetproceeds normallySummary by CodeRabbit
Release Notes
Bug Fixes
Documentation