Skip to content

Commit 6fed1df

Browse files
committed
fix: address code review findings (18-item tracker)
Correctness: - #3: Restore 3k-row cap in Quick Start step 3 - #6: Add PostgreSQL parser caveat to Workflow 7 (MySQL syntax → parse error → fallback) - #10: Fix ORM pattern to present fixed_with_warning to user (not auto-accept) - #12: Unfixable rewrites MUST present to user before substituting Docs accuracy: - #7: Rails: use db:schema:dump with schema_format = :sql (6.1+) - #8: Prisma: add required --from-empty --to-schema-datamodel flags - SQLAlchemy: use CreateTable(table).compile(engine) instead of metadata.create_all(echo=True) which executes DDL Error handling: - #16: Add Error Handling section for dsql_lint failures (MCP unavailable, parse error, timeout) with user-confirmation gates - Add dsql_lint-unavailable entry to SKILL.md Error Scenarios Evals: - #5: Add evals 102/103 to results MD with detail sections - #9: Fix model metadata (remove specific version; clarify manual grading) - #11: Add missing category_id column to eval 103 prompt - Tighten eval expectations to reference concrete tool outputs (rule names, summary fields) - Replace emoji markers with PASS/FAIL/PARTIAL to fix dprint table alignment - Bump recorded dsql-lint version to 0.1.4 Self-review fixes (17 sub-agent review rounds): - Document accurate fix_result.status enum: fixed | fixed_with_warning | unfixable (tool emits status='unfixable' explicitly; earlier doc incorrectly implied absence) - Scope Unfixable Errors table to truly-unfixable rules only (set_transaction, truncate, create_table_as, add_column_constraint, index_expression, index_partial, unsupported_alter_table_op); note that temp_table, inherits, index_using, transaction_isolation are fixed/fixed_with_warning - Fix transaction_isolation vs set_transaction rule-id confusion - Promote reference-load gate from SHOULD to MUST with tightened trigger - Workflow 2: explicit lint gate for async index DDL (step 5) - Workflow 6: lint every generated DDL in Table Recreation Pattern - Workflow 7: cross-check MySQL source against type-mapping.md even on clean lint (ENGINE=, SET() pass silently through PostgreSQL parser) - Document 1M-char SQL limit and 30s server timeout - Require user confirmation before destructive DDL (DROP/RENAME/TRUNCATE), MCP-unavailable fallback, parse_error manual rewrite, and timeout split-retry paths - Forbid executing fixed_sql while any unfixable diagnostic remains (re-lint until clean) - Add user override semantics for "just run it" requests - Remove redundant Usage Patterns, Exit Codes, and Additional Resources sections Already fixed in previous commit: - #17: dsql-lint removed from description (93e9c7b) PR body items (1, 2, 4) will be updated separately.
1 parent 93e9c7b commit 6fed1df

4 files changed

Lines changed: 148 additions & 127 deletions

File tree

plugins/databases-on-aws/skills/dsql/SKILL.md

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ sampled in [.mcp.json](../../.mcp.json)
118118

119119
#### [dsql-lint.md](references/dsql-lint.md)
120120

121-
**When:** SHOULD load when validating SQL for DSQL compatibility or migrating schemas
121+
**When:** MUST load before running `dsql_lint`, processing externally-sourced SQL (pg_dump, ORM migrations, user-pasted DDL), or resolving `fixed_with_warning` / unfixable diagnostics
122122
**Contains:** `dsql_lint` MCP tool reference, fix statuses, ORM integration, unfixable error resolution
123123

124124
---
@@ -181,7 +181,7 @@ See [scripts/README.md](../../scripts/README.md) for usage and hook configuratio
181181

182182
1. **Explore:** Use `readonly_query` with `information_schema` to list tables. Use `get_schema` for table structure.
183183
2. **Query:** Use `readonly_query` for SELECT queries. **MUST** include `tenant_id` in WHERE for multi-tenant apps. **MUST** build SQL with `safe_query.build()`.
184-
3. **Schema changes:** Use `transact` with one DDL per transaction; multiple DML statements **MAY** share a transaction. **MUST** use `CREATE INDEX ASYNC` in a separate call. Use `dsql_lint` to validate first.
184+
3. **Schema changes:** Use `transact` with one DDL per transaction. **MUST** batch DML under 3,000 rows. **MUST** use `CREATE INDEX ASYNC` in a separate call. Use `dsql_lint` to validate first.
185185

186186
---
187187

@@ -201,13 +201,15 @@ See [scripts/README.md](../../scripts/README.md) for usage and hook configuratio
201201

202202
### Workflow 2: Safe Data Migration
203203

204-
1. Validate DDL with `dsql_lint(sql=..., fix=true)` — apply fixes if needed
204+
Every DDL statement generated in this workflow MUST be validated with `dsql_lint(fix=true)` before its `transact` call — applies to step 2 (ADD COLUMN) and step 5 (async index). DML (`UPDATE` in step 3) does not require linting.
205+
206+
1. Validate ALTER TABLE DDL with `dsql_lint(sql=..., fix=true)` — handle diagnostics per [dsql-lint.md](references/dsql-lint.md)
205207
2. Add column using transact: `transact(["ALTER TABLE ... ADD COLUMN ..."])`
206208
3. Populate existing rows with UPDATE in separate transact calls (batched under 3,000 rows)
207209
4. Verify migration with readonly_query using COUNT
208-
5. Create async index for new column using transact if needed
210+
5. If an index is needed: validate CREATE INDEX ASYNC DDL with `dsql_lint(sql=..., fix=true)`, then create via transact
209211

210-
- MUST validate DDL with `dsql_lint` before executing
212+
- MUST validate every externally-sourced or generated DDL statement with `dsql_lint` before executing
211213
- MUST add column first, populate later
212214
- MUST issue ADD COLUMN with only name and type; apply DEFAULT via separate UPDATE
213215
- MUST batch updates under 3,000 rows in separate transact calls
@@ -235,13 +237,18 @@ MUST load [access-control.md](references/access-control.md) for role setup, IAM
235237

236238
### Workflow 6: Table Recreation DDL Migration
237239

238-
DSQL does NOT support direct `ALTER COLUMN TYPE`, `DROP COLUMN`, `DROP CONSTRAINT`, or `MODIFY PRIMARY KEY`. These require the **Table Recreation Pattern**. This is a destructive workflow that requires user confirmation at each step. Validate the new CREATE TABLE with `dsql_lint(sql=..., fix=true)` before execution.
240+
DSQL does NOT support direct `ALTER COLUMN TYPE`, `DROP COLUMN`, `DROP CONSTRAINT`, or `MODIFY PRIMARY KEY`. These require the **Table Recreation Pattern**. This is a destructive workflow that requires user confirmation at each step. Every generated DDL in the pattern (CREATE new, INSERT ... SELECT, DROP old, RENAME) MUST be validated with `dsql_lint(sql=..., fix=true)` before execution.
239241

240242
MUST load [ddl-migrations/overview.md](references/ddl-migrations/overview.md) before attempting any of these operations.
241243

242244
### Workflow 7: Validate and Migrate to DSQL
243245

244-
Run `dsql_lint(sql=source_sql, fix=true)` to validate and auto-convert SQL from any source (PostgreSQL, MySQL, ORM-generated). MUST load [dsql-lint.md](references/dsql-lint.md) for the full workflow, ORM-specific guidance, and unfixable error resolution. MUST load [mysql-migrations/type-mapping.md](references/mysql-migrations/type-mapping.md) for MySQL-specific types not covered by auto-fix.
246+
MUST load [dsql-lint.md](references/dsql-lint.md) before running `dsql_lint` — it defines diagnostic handling, the three `fix_result.status` values (`fixed`, `fixed_with_warning`, `unfixable`), and user-confirmation gates.
247+
248+
Run `dsql_lint(sql=source_sql, fix=true)` to validate and auto-convert PostgreSQL-compatible SQL. `dsql_lint` uses a PostgreSQL parser, so MySQL dialect syntax that PostgreSQL cannot parse (e.g., `PARTITION BY HASH`, `AUTO_INCREMENT` in some positions) surfaces as a `parse_error` rule rather than individual diagnostics.
249+
250+
- For MySQL-origin SQL, MUST cross-check the source against [mysql-migrations/type-mapping.md](references/mysql-migrations/type-mapping.md) even when lint returns clean — `ENGINE=` clauses and `SET(...)` column types can pass silently through the PostgreSQL parser.
251+
- On `parse_error`, fall back to [mysql-migrations/type-mapping.md](references/mysql-migrations/type-mapping.md) for manual conversion, then re-run `dsql_lint` on the converted output before executing.
245252

246253
### Workflow 8: Query Plan Explainability
247254

@@ -276,6 +283,7 @@ PGPASSWORD="$TOKEN" psql "host=$HOST port=5432 user=admin dbname=postgres sslmod
276283
## Error Scenarios
277284

278285
- **`awsknowledge` returns no results:** Use the default limits in the table above and note that limits should be verified against [DSQL documentation](https://docs.aws.amazon.com/aurora-dsql/latest/userguide/).
286+
- **`dsql_lint` unavailable or timing out:** See the Error Handling section of [dsql-lint.md](references/dsql-lint.md). Do not silently skip validation — inform the user and require explicit confirmation before proceeding with manual rules from [development-guide.md](references/development-guide.md).
279287
- **OCC serialization error:** Retry the transaction. If persistent, check for hot-key contention — see [troubleshooting.md](references/troubleshooting.md).
280288
- **Transaction exceeds limits:** Split into batches under 3,000 rows — see [batched-migration.md](references/ddl-migrations/batched-migration.md).
281289
- **Token expiration mid-operation:** Generate a fresh IAM token — see [authentication-guide.md](references/auth/authentication-guide.md). See [troubleshooting.md](references/troubleshooting.md) for other issues.

0 commit comments

Comments
 (0)