fix: Potential fixes for 4 code quality findings#12
Merged
Conversation
…ofix Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
…ofix Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
scottschreckengaust
changed the title
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
scottschreckengaust
changed the title
…s.cjs Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
krokoko
approved these changes
Feb 9, 2026
amaksimo
added a commit
that referenced
this pull request
May 8, 2026
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:structure:dump with schema_format = :sql - #8: Prisma: add required --from-empty --to-schema-datamodel flags Error handling: - #16: Add Error Handling section for dsql_lint failures (MCP unavailable, parse error, timeout) Evals: - #5: Add evals 102/103 to results summary table - #9: Fix model metadata (remove specific version) - #11: Add missing category_id column to eval 103 prompt Already fixed in previous commit: - #17: dsql-lint removed from description (93e9c7b) PR body items (1, 2, 4) will be updated separately.
amaksimo
added a commit
that referenced
this pull request
May 8, 2026
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.
amaksimo
added a commit
that referenced
this pull request
May 8, 2026
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.
amaksimo
added a commit
that referenced
this pull request
May 8, 2026
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.
Morlej
pushed a commit
to Morlej/agent-plugins
that referenced
this pull request
May 8, 2026
…dation (awslabs#157) * feat(dsql): add dsql_lint tool integration for SQL compatibility validation Add dsql-lint as a deterministic validation tool the agent invokes before executing externally-sourced SQL. Enables migration support for customers coming from PostgreSQL, MySQL, or ORMs (Django, Rails, Prisma, TypeORM). Changes: - Add references/dsql-lint.md: tool API, fix statuses, usage patterns, ORM integration, unfixable error resolution - Update SKILL.md: add dsql_lint to MCP Tools section, update Workflows 2/6/7 with lint validation steps, add Workflow 9 (Validate & Migrate SQL to DSQL) - Update frontmatter: add lint/ORM trigger phrases and tags The dsql_lint MCP tool (shipping separately in awslabs/mcp) validates SQL and optionally auto-fixes issues, returning structured diagnostics the agent acts on. The skill teaches the agent when and how to use it. * fix: resolve markdownlint errors (MD029, MD032) - MD029: Restart ordered list numbering after section breaks - MD032: Add blank lines before lists after bold headings * refactor: trim SKILL.md to 276 lines (under 300 limit) Move AWS Knowledge limits table reference to development-guide.md (already documented there). Condense Quick Start to 3 lines. Trim workflow descriptions to routing-only — detail lives in reference files. validate-size.py: 276 lines, status 'good' validate-references.py: 0 broken links, 0 new orphans * fix: address review feedback - Restore destructive workflow warning in Workflow 6 - Re-introduce RFC language (MUST, MAY) in Quick Start - Use active voice: 'Use get_schema', 'Use transact', 'Use readonly_query' - Restore 'one DDL per transaction, multiple DML may share' framing - Remove 'lint' from tags (not sufficient alone to trigger skill) - Remove TOC from dsql-lint.md (file is short, TOC adds no value) * style: apply dprint table formatting to dsql-lint.md Run dprint fmt to align table columns per repo formatting rules. * feat: add dsql_lint eval harness and tool availability fallback - Add tools/evals/databases-on-aws/dsql/dsql_lint_evals.json with 4 functional evals covering: pg_dump migration, Django ORM migration, clean SQL validation, and MySQL with unfixable issues - Add availability note to dsql-lint.md: fall back to manual validation using existing DDL rules when the MCP tool is not yet available The evals test that the agent calls dsql_lint before executing SQL, presents warnings to the user, and handles unfixable errors correctly. * fix: remove incorrect availability fallback note The dsql_lint tool and the skill that references it will ship together in the same MCP repo PR. There is no availability gap — the fallback note was based on a wrong assumption about PR splitting. * feat: add eval harness results from local dsql_lint testing Run dsql_lint_evals.json against local MCP server with dsql_lint tool. All 4 evals pass — tool correctly identifies compatibility issues, produces fixed SQL, and reports unfixable errors for manual resolution. Key findings: - Eval 103 (MySQL syntax): dsql-lint uses a PostgreSQL parser, so MySQL-specific syntax (SET, ENGINE, PARTITION BY) triggers a parse error rather than individual rules. Agent falls back to mysql-migrations reference for these cases. * feat: replace tool-only eval results with behavioral with-skill vs baseline comparison Run evals as subagent behavioral tests: one agent with the skill loaded (uses dsql_lint), one baseline without (relies on model knowledge). Key findings: - Baseline hallucinates JSON→JSONB (DSQL rejects JSONB as column type) - Baseline misses CREATE INDEX ASYNC requirement - Baseline doesn't split multi-DDL transactions - Skill-guided agent uses dsql_lint for deterministic validation, produces correct output on all three failure points The iron law holds: the agent fails without this skill change. * fix: address review feedback round 2 - Restore hardcoded limits table (critical for performance, not all are in dev guide, link to DSQL docs prevents stale numbers) - Merge Workflow 9 into Workflow 7 as 'Validate and Migrate to DSQL' (reduces line count, single entry point for all migration sources) - Trim redundant triggers from description (lint SQL covers dsql-lint, migrate to DSQL covers ORM migration DSQL) 290 lines, mise run build passes. * fix: remove dsql-lint from description, trim trigger Per review: 'SQL compatibility validation' is sufficient without naming the tool. Remove 'via dsql-lint' and 'lint SQL for DSQL' trigger — 'migrate to DSQL' already covers the use case. * fix: address code review findings (18-item tracker) Correctness: - awslabs#3: Restore 3k-row cap in Quick Start step 3 - awslabs#6: Add PostgreSQL parser caveat to Workflow 7 (MySQL syntax → parse error → fallback) - awslabs#10: Fix ORM pattern to present fixed_with_warning to user (not auto-accept) - awslabs#12: Unfixable rewrites MUST present to user before substituting Docs accuracy: - awslabs#7: Rails: use db:schema:dump with schema_format = :sql (6.1+) - awslabs#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: - awslabs#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: - awslabs#5: Add evals 102/103 to results MD with detail sections - awslabs#9: Fix model metadata (remove specific version; clarify manual grading) - awslabs#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: - awslabs#17: dsql-lint removed from description (93e9c7b) PR body items (1, 2, 4) will be updated separately.
Morlej
added a commit
to Morlej/agent-plugins
that referenced
this pull request
May 15, 2026
…vals Correctness fixes (review items 1-5): - awslabs#1: push-computation-to-constant — use NUMERIC column 'amount' to avoid integer division non-equivalence - awslabs#2: not-in-to-not-exists — add NULL semantics warning (NOT EXISTS does not preserve NOT IN's NULL-propagation; MUST confirm with user) - awslabs#3/awslabs#4: subquery-unnesting — prefer EXISTS form (true semi-join); document uniqueness precondition for JOIN+DISTINCT alternative - awslabs#5: subquery-unnesting-scalar — add COALESCE(s_count, 0) for COUNT/SUM (LEFT JOIN returns NULL, scalar returns 0) Dangling reference fixes (review items 6-8): - awslabs#6: workflow.md trigger table — "Phase 5" → reassessment re-entry - awslabs#7: Replace all "implicit cast compatibility matrix" references with "pg_amop query in catalog-queries.md" - awslabs#8: plan-interpretation.md L202 — fix cast-vs-operator contradiction Structural fixes (review items 9-14, 24): - awslabs#9: Hedge "integer family" claim with "at time of writing" + verify - awslabs#10: amopmethod=10003 — add provenance comment and verification SQL - awslabs#11: catalog-queries.md TOC — add 3 missing sections - awslabs#12: plan-interpretation.md TOC — add Type Coercion section - awslabs#13: SKILL.md — explicitly delegate routing to workflow.md - awslabs#24: workflow.md — remove em dashes from headings for clean anchors Other fixes (review items 21-23): - awslabs#21: reltuples-estimate — add staleness warning (MUST warn user) - awslabs#22: catalog-queries — add safe_query.build() note for placeholders - awslabs#23: "Skip when" → "SHOULD skip when" in all rewrite files Eval improvements (review items 14, 16): - awslabs#14: README — add query_plan_rewrite_evals to directory tree and eval section - awslabs#16: Add evals 206-210 covering LEFT JOIN, computation push, NOT IN with NULL warning, nested UNION ALL, and negative case (OR across different columns) - awslabs#7 (eval): Update eval 201 expectation — pg_amop instead of matrix Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
anwesham-lab
pushed a commit
to Morlej/agent-plugins
that referenced
this pull request
May 26, 2026
…vals Correctness fixes (review items 1-5): - awslabs#1: push-computation-to-constant — use NUMERIC column 'amount' to avoid integer division non-equivalence - awslabs#2: not-in-to-not-exists — add NULL semantics warning (NOT EXISTS does not preserve NOT IN's NULL-propagation; MUST confirm with user) - awslabs#3/awslabs#4: subquery-unnesting — prefer EXISTS form (true semi-join); document uniqueness precondition for JOIN+DISTINCT alternative - awslabs#5: subquery-unnesting-scalar — add COALESCE(s_count, 0) for COUNT/SUM (LEFT JOIN returns NULL, scalar returns 0) Dangling reference fixes (review items 6-8): - awslabs#6: workflow.md trigger table — "Phase 5" → reassessment re-entry - awslabs#7: Replace all "implicit cast compatibility matrix" references with "pg_amop query in catalog-queries.md" - awslabs#8: plan-interpretation.md L202 — fix cast-vs-operator contradiction Structural fixes (review items 9-14, 24): - awslabs#9: Hedge "integer family" claim with "at time of writing" + verify - awslabs#10: amopmethod=10003 — add provenance comment and verification SQL - awslabs#11: catalog-queries.md TOC — add 3 missing sections - awslabs#12: plan-interpretation.md TOC — add Type Coercion section - awslabs#13: SKILL.md — explicitly delegate routing to workflow.md - awslabs#24: workflow.md — remove em dashes from headings for clean anchors Other fixes (review items 21-23): - awslabs#21: reltuples-estimate — add staleness warning (MUST warn user) - awslabs#22: catalog-queries — add safe_query.build() note for placeholders - awslabs#23: "Skip when" → "SHOULD skip when" in all rewrite files Eval improvements (review items 14, 16): - awslabs#14: README — add query_plan_rewrite_evals to directory tree and eval section - awslabs#16: Add evals 206-210 covering LEFT JOIN, computation push, NOT IN with NULL warning, nested UNION ALL, and negative case (OR across different columns) - awslabs#7 (eval): Update eval 201 expectation — pg_amop instead of matrix Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
2 participants
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.
This PR applies 4/4 suggestions from code quality AI findings. 2 suggestions were skipped to avoid creating conflicts.
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.