feat: GitHub setup incremental improvements#11
Merged
scottschreckengaust merged 61 commits intoFeb 8, 2026
Conversation
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
Signed-off-by: Scott Schreckengaust <scottschreckengaust@users.noreply.github.com>
krokoko
approved these changes
Feb 8, 2026
scottschreckengaust
deleted the
feature/github-setup-incremental-improvements
branch
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>
Morlej
added a commit
to Morlej/agent-plugins
that referenced
this pull request
Jun 17, 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>
Morlej
added a commit
to Morlej/agent-plugins
that referenced
this pull request
Jun 19, 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
Jun 22, 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>
Morlej
added a commit
to Morlej/agent-plugins
that referenced
this pull request
Jun 24, 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>
sbbhimji
pushed a commit
to sbbhimji/agent-plugins
that referenced
this pull request
Jun 30, 2026
…ction, rewrites, and workflow extraction (awslabs#162) * feat(dsql): enhance query plan explainability with type coercion detection and rewrite references - Add structured trigger phrases and routing criteria for query plan diagnosis - Add type coercion index bypass detection (implicit cast compatibility matrix) - Extend catalog queries with indexed column type retrieval - Add generic SQL rewrite reference (11 patterns: OR-to-IN, subquery unnesting, etc.) - Add DSQL-specific rewrite reference (reltuples estimate, split large joins for DP threshold) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(dsql): extract query plan workflow, add rewrite evals - Extract Workflow 8 (query plan explainability) from SKILL.md into references/query-plan/workflow.md to stay under the 300 LOC limit - Wire query-rewrites-generic.md and query-rewrites-dsql-specific.md into the workflow (Phase 0 load list + Phase 2 evidence gathering) - Add behavioral evals (query_plan_rewrite_evals.json) covering type coercion detection, subquery unnesting, OR-to-IN, GROUP BY pushdown, large join splitting, and reltuples estimation - Add eval results (query_plan_rewrite_eval_results.md) with with-skill vs baseline comparison Validation: - validate-size.py: 275 lines (good) - validate-references.py: 0 broken links, 0 new orphans Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: correct markdown link fragments in workflow.md TOC Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * style: apply dprint table formatting Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor(dsql): address PR review — split rewrites, positive language, RFC keywords Review feedback from amaksimo: - Split query-rewrites-generic.md into 11 individual files under query-rewrites/ subdirectory to reduce context consumption - Split query-rewrites-dsql-specific.md into individual files - Convert monolithic files to index tables pointing to sub-files - Fix DATEADD() SQL Server syntax → PostgreSQL NOW() - INTERVAL - Flip negative language ("Do not apply") to positive ("Skip when") - Add RFC keywords (MUST, SHOULD, MAY) throughout - Remove psql fallback from workflow.md (enforce MCP usage) - Update plan-interpretation.md recommendation template with RFC language - Make Phase 0 loading explicit: MUST for core refs, SHOULD for rewrites Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(dsql): address PR awslabs#162 review — correctness, references, evals Correctness fixes (review items 1-5): - #1: push-computation-to-constant — use NUMERIC column 'amount' to avoid integer division non-equivalence - #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> * fix(dsql): address remaining PR awslabs#162 review items - awslabs#17: Downgrade eval results to qualitative comparison, record model and version, note n=1 and recommend n>=3 for production confidence - awslabs#18: SKILL.md is 281 lines (will update PR body) - awslabs#20: Strengthen awsknowledge fallback to MUST — refuse fallback when recommendation depends on exact limit value - awslabs#21: Already addressed in prior commit (reltuples staleness) - awslabs#15: Document manual-only status and future Python converter direction (per anwesham-lab's suggestion for deterministic rewrites) - awslabs#19: MCP mirror PR noted as follow-up in PR body Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(dsql): address self-review findings — links, inclusive language, evals - Revert broken relative links in SKILL.md (mcp/.mcp.json, scripts/) back to correct ../../ paths - Rename blacklisted_customers → excluded_customers and blacklist → exclusion_list for inclusive language compliance - Fix stale 'implicit cast compatibility matrix' → pg_amop in eval results - Add eval results for 206–210 (LEFT JOIN, computation push, NOT IN NULL warning, nested UNION ALL, negative OR case) - Include hooks.json update * fix: remove stray conflict marker in evals README * style: apply dprint table formatting to eval results * fix(dsql): wire workflow.md and rewrite indexes into SKILL.md Addresses amaksimo's review comment: after rebase onto main, the Workflow 9 section lost its link to workflow.md and the rewrite index files, making all 16 new query-plan files unreachable orphans. - Add workflow.md as the entry gate in the reference table - Add query-rewrites-generic.md and query-rewrites-dsql-specific.md - Update Workflow 9 section to load workflow.md instead of listing the 4 Phase-0 files directly (workflow.md handles that routing) * fix(dsql): address review findings awslabs#4, awslabs#5, awslabs#7, awslabs#8, awslabs#9 - awslabs#4: COALESCE rule in subquery-unnesting-scalar.md restricted to COUNT only; SUM/MAX/MIN return NULL on empty sets in both forms - awslabs#5: verify-comment in catalog-queries.md changed from amname='btree' to amname='btree_index' (DSQL uses btree_index AM) - awslabs#7: workflow.md context disambiguation removes psql fallback offer, now says no MCP means no plan capture (consistent with Safety) - awslabs#8: split-large-joins.md example expanded from 7 to 11 tables, exceeding the stated DP threshold of 10; CTEs project explicit cols - awslabs#9: plan-interpretation.md recommendation template changed ::float to ::integer (only integer-family cross-type operators registered) * fix(dsql): correct safe_query substitution guidance in catalog-queries Lift the substitution rule to the file preamble with per-position guidance: identifier positions (FROM, GROUP BY) use ident(); string- literal positions (WHERE = '{schema}', IN ('{table}')) use allow() or regex(). The prior note incorrectly prescribed ident() for all positions, which would produce invalid SQL in WHERE clauses. * refactor(dsql): merge in-subquery-to-exists into subquery-unnesting-uncorrelated Delete redundant in-subquery-to-exists.md — same input shape and same rewrite as subquery-unnesting-uncorrelated.md. Merge the 'large result set' trigger and 'small static set' skip condition into the canonical file. Update index and eval 200 to route exclusively there. * style: apply dprint table formatting to workflow.md * fix(dsql): address merge-blocking review findings #1-awslabs#4 - #1: Strip surrounding quotes from all placeholders in catalog-queries so safe_query helpers (which emit their own quotes) don't double-quote. Add worked safe_query.build() example to preamble. - #2: DP threshold changed from 10 to 8 (validated: SHOW join_collapse_limit = 8 on live DSQL). Agent now instructed to SHOW the value rather than hardcoding. - awslabs#3: Remove pg_stat_user_tables.last_analyze cross-check (DSQL never populates it). Guard reltuples with GREATEST(..., 0) for the -1 sentinel on never-analyzed tables. - awslabs#4: Fix '11 generic' to '10 generic' in SKILL.md reference table. * feat(dsql): add blog learnings — filter model, DPU, CTE rewrite - Add Three-Layer Filter Model (Index Cond / Storage Filter / Query Processor Filter) with optimization table to plan-interpretation.md - Add Fixing Storage Lookups guidance (INCLUDE columns) with example - Add Cost Number Interpretation (startup ~100 is normal in DSQL) - Add DPU Interpretation (Read DPU as primary signal, optimization loop) - Add CTE late materialization as DSQL-specific rewrite pattern (defer Storage Lookups past LIMIT) - Update workflow.md Phase 1: recommend plain EXPLAIN first for expensive queries before EXPLAIN ANALYZE VERBOSE * chore: bump databases-on-aws plugin version to 1.4.0 --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
scottschreckengaust
added a commit
that referenced
this pull request
Jul 7, 2026
Replaces the [auto_exclude] source policy with explicit per-rule entries, per review feedback. Three concerns addressed: 1. No silent exclusion. source=/r/None is a reliable junk signal *today* (all 104 probes lack the `license:` field every real rule carries), but it is not guaranteed — a future legitimate rule could carry /r/None and would have been auto-dropped. Now every exclusion is listed explicitly in exclusions.toml; nothing is dropped implicitly, so a new upstream rule can never silently disappear — it surfaces as `new` for triage. The 79 probe rules (78 + bbp-pattern-inject) are now explicit entries. 2. Correct PR provenance (from git pickaxe, replacing the earlier PR#TBD->220 guess): the 21 original exclusions were PR #11 (2026-02-07), OpenAI was PR #89, bbp-pattern-inject was PR #200. Only the 78 new probe exclusions are PR #220. 3. Duplicate rows fixed. r/all reuses some ids across blocks (bbp-x x11, bbp-ssrf x8, ...); EXCLUSIONS.md now collapses rows by id. Status legend documents excluded/active/new (auto-excluded removed). Also: when an excluded rule is later removed upstream, update.py now surfaces the orphaned entry in a "Removed upstream (safe to prune)" doc section (and console), rather than only a console warning — a human prunes it, nothing auto-deletes. Result: 101 explicit entries (21 PR#11, 1 PR#89, 1 PR#200, 78 PR#220), 3005 active rules, semgrep exit 0 with no parse errors, build green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 7, 2026
Morlej
pushed a commit
to Morlej/agent-plugins
that referenced
this pull request
Jul 8, 2026
…wslabs#220) * feat(security): vendor Semgrep r/all ruleset with tracked exclusions Implements RFC awslabs#211. All Semgrep invocations (pre-commit, CI, mise) now run against a single pinned, locally-vendored ruleset instead of fetching r/all live from the registry on every scan. Exclusions are baked into the vendored file, and a generated doc records what was decided and why. Adds tools/semgrep/: - update.py stdlib-only update mechanism (goal C). Downloads r/all, diffs each rule's version_id against the prior snapshot to derive a "last changed" date, drops excluded rules, and regenerates the artifacts. Byte-preserving: it removes whole rule blocks and never re-serializes YAML (re-serializing was verified to silently break the ruleset). - exclusions.toml human-owned source of truth (id -> status/pr/reason), seeded from the 22 pre-existing --exclude-rule flags. - r-all.active.yaml pre-filtered rules; the only file scans load (goal A). - rule-state.json version_id/date baseline for diffing. - EXCLUSIONS.md generated tracking table (goal B). Integration: - mise.toml: SEMGREP_RULES env (bare `semgrep` uses vendored rules with zero flags) + `semgrep:update` task; drop 22 --exclude-rule flags. - CI + pre-commit: --config=tools/semgrep/r-all.active.yaml; drop exclude flags. Baseline/SARIF logic unchanged. - .semgrepignore: skip tools/semgrep/*.yaml (rules self-match). - .gitleaks.toml: allowlist the active file (contains secret regexes). - dprint.json: exclude generated EXCLUSIONS.md / rule-state.json. - .pre-commit-config.yaml: top-level `exclude` for the generated files (r-all.active.yaml, rule-state.json, EXCLUSIONS.md) so content hooks (detect-private-key, check-vcs-permalinks, check-added-large-files) skip them and autofix hooks never corrupt the byte-for-byte ruleset. The hand-authored update.py/exclusions.toml stay in scope. - Remove stub .semgrep.yaml (never loaded by Semgrep; misleading). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(security): record PR#220 in Semgrep exclusions tracking Replace the PR#TBD placeholders in exclusions.toml and the generated EXCLUSIONS.md with this PR's number (awslabs#220). PR#89 (the OpenAI rule) is left as-is. Same-length swap; no table realignment or rule content change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): exclude broken r/all rule bbp-pattern-inject Rebased onto main, which added an exclusion for `bbp-pattern-inject` (PR awslabs#200) — a malformed community rule that fails to parse ("Invalid pattern for Python: Stdlib.Parsing.Parse_error") and makes semgrep exit 2 on every run. main handled it with a --exclude-rule flag; this expresses the same decision through the vendoring mechanism by adding it to exclusions.toml, so update.py physically drops it from r-all.active.yaml. Regenerated the active file, rule-state.json, and EXCLUSIONS.md (23 excluded / 3108 active). Verified: semgrep now exits 0 with no parse errors against the vendored ruleset. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(security): auto-exclude bug-bounty probe rules from r/all by source The r/all feed carries 104 rules with source=https://semgrep.dev/r/None: a single-burst set of offensive-security probes (SSRF via rule metadata targeting 169.254.169.254, stored-XSS payloads in message/fix fields, command-injection and secret-exfil tests, plus PWNED-PRODUCTION-RULE and a rule impersonating a real id). None have a canonical registry source; none appear in semgrep/semgrep-rules. See PR awslabs#220 provenance analysis. Rather than list 104 ids by hand, add a source-based auto-exclusion policy so this generalizes to FUTURE probes: - exclusions.toml gains [auto_exclude]: any rule whose `source` matches a listed value is dropped automatically. A rule with no canonical source (/r/None) is not a curated community rule — this is how new r/all rules are judged (no positive "valid" signal exists; absence of a real source is a reliable invalidity signal). An explicit [rules."id"] status=active force-keeps a rule the policy would drop. - update.py parses each rule's `source`, applies the policy, reports "N auto-excluded", and lists them in EXCLUSIONS.md as `auto-excluded`. - Harden the doc generator: sanitize untrusted rule messages (escape <>`| ) so XSS/markup payloads in rule text render as inert text instead of injecting markup or breaking the table (this is what test-bbp-xss-msg's "<img src=x onerror=...>" message was probing). Result: 3005 active / 126 excluded. The real subprocess-shell-true rule survives (the /None impersonator is dropped), semgrep exits 0 with no parse errors, and the Semgrep OSS code-scanning check clears (bbp-rate-check no longer flags print() calls). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(security): drop redundant dprint exclude for rule-state.json dprint already leaves rule-state.json untouched — update.py emits json.dumps(indent=2, sort_keys=True), which matches dprint's JSON style (verified: `dprint check` exits 0 with no diff). The exclude was belt-and-suspenders and, since dprint.json is strict JSON with no comment syntax, it read as accidental. Remove it and document the generated-file formatting policy in update.py's docstring instead, where comments are allowed. EXCLUSIONS.md stays dprint-excluded (dprint would reflow its tables and churn on every regeneration); rule-state.json stays in the pre-commit exclude (large-file/private-key hooks would still act on it). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(security): emit dprint-aligned tables, drop last dprint exclude update.py's render_table now column-pads the EXCLUSIONS.md markdown table exactly as the dprint markdown plugin does (each cell ljust to its column's max code-point width, delimiter dashes to match, min 3). Verified: with the exclude removed, `dprint check tools/semgrep/EXCLUSIONS.md` exits 0 with no diff, so the generated file already matches what dprint would produce. Removes the last tools/semgrep entry from dprint.json's excludes — both generated files (EXCLUSIONS.md, rule-state.json) are now emitted in dprint's own format, so neither needs a formatter exclude. They remain in the pre-commit exclude only for content hooks (large-file/private-key/whitespace). Trade-off: render_table mirrors dprint's table algorithm, so a future markdown-plugin bump or config change could require a matching update.py tweak; the build's fmt:check catches any such drift immediately. Cells are already sanitized ASCII, so len()-based widths match dprint's. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): sort vendored rules by id for deterministic output The Semgrep registry serves r/all in a nondeterministic block order, so re-running update.py reshuffled r-all.active.yaml and produced a spurious ~46k-line diff even when no rule actually changed (verified: same ids, same version_ids, 0 added/removed/changed — order only). That would make every future `mise run semgrep:update` unreviewable. Sort active rule blocks by rule id before writing. Now output is a pure function of content: two consecutive runs are byte-identical, and a real upstream change is the only thing that shows up in git — matching the RFC's "reviewable drift" goal (rule-state.json is already sort_keys=True). Verified: 3005 rules intact, semgrep scans the sorted file with exit 0 and no parse errors, build green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): remediate GHAS findings in update.py (no suppressions) Address four Semgrep code-scanning findings with real code changes, not # nosec/# noqa suppressions (all suppression comments removed). CWE-939 / dynamic-urllib-use-detected (urlopen with non-literal URL): the default urllib opener installs file:// and ftp:// handlers, so a URL resolving to another scheme could read local files. Replace urllib.request.urlopen with an OpenerDirector that installs ONLY the HTTPS/redirect/error handlers — no FileHandler/FTPHandler — so non-HTTPS schemes have no handler and are unreachable by construction, plus an explicit https scheme guard. Verified: file:// and ftp:// return no content; https download of r/all still succeeds byte-for-byte. string-concat-in-list (implicitly concatenated string literals in the render_doc lines list): make the concatenation explicit with `+` so each list element is unambiguously one string (guards against a missing-comma bug the rule warns about). Verified with the exact flagged rules: dynamic-urllib 1->0 findings, string-concat-in-list 0 findings, bandit "No issues" with 0 nosec skips, update.py still produces byte-identical output (3005 active rules). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(security): list all exclusions explicitly; fix PRs; dedup doc Replaces the [auto_exclude] source policy with explicit per-rule entries, per review feedback. Three concerns addressed: 1. No silent exclusion. source=/r/None is a reliable junk signal *today* (all 104 probes lack the `license:` field every real rule carries), but it is not guaranteed — a future legitimate rule could carry /r/None and would have been auto-dropped. Now every exclusion is listed explicitly in exclusions.toml; nothing is dropped implicitly, so a new upstream rule can never silently disappear — it surfaces as `new` for triage. The 79 probe rules (78 + bbp-pattern-inject) are now explicit entries. 2. Correct PR provenance (from git pickaxe, replacing the earlier PR#TBD->220 guess): the 21 original exclusions were PR awslabs#11 (2026-02-07), OpenAI was PR awslabs#89, bbp-pattern-inject was PR awslabs#200. Only the 78 new probe exclusions are PR awslabs#220. 3. Duplicate rows fixed. r/all reuses some ids across blocks (bbp-x x11, bbp-ssrf x8, ...); EXCLUSIONS.md now collapses rows by id. Status legend documents excluded/active/new (auto-excluded removed). Also: when an excluded rule is later removed upstream, update.py now surfaces the orphaned entry in a "Removed upstream (safe to prune)" doc section (and console), rather than only a console warning — a human prunes it, nothing auto-deletes. Result: 101 explicit entries (21 PR#11, 1 PR#89, 1 PR#200, 78 PR#220), 3005 active rules, semgrep exit 0 with no parse errors, build green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(security): align semgrep verbosity/log flags across all invocations Make the three semgrep entry points consistent on the cosmetic log flags (these affect only stderr verbosity — never findings or exit code): - mise `security:semgrep`: swap `--quiet` for `--verbose` (they are mutually exclusive in semgrep, so --quiet had to go). Keeps --max-log-list-entries=0, which was previously a no-op under --quiet. - pre-commit hook: add `--max-log-list-entries=0` (it was --verbose without the flag, so it printed "<SKIPPED DATA>" instead of the full rule list). - CI: unchanged (already --verbose --max-log-list-entries=0). `--max-log-list-entries=0` disables truncation → shows ALL rule-ids/skipped files (verified empirically: =2 prints "<SKIPPED DATA>", =0 prints the full list). Intentional per-invocation differences remain: --baseline-commit (local mise scans everything; CI/pre-commit diff a baseline) and --sarif-output/--error (CI/pre-commit only). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(security): remove dead source parsing from update.py The [auto_exclude] source policy was removed earlier, leaving parse_block extracting a `source` value that main() unpacked and discarded (_source), plus a now-orphaned SOURCE_RE whose comment referenced the deleted policy. Drop SOURCE_RE and return a 3-tuple from parse_block — pure behavior- preserving refactor; generated files are unaffected (only update.py changes). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(security): rename to rules-* prefix; drop derived files; add orphaned state Coordinate the tools/semgrep/ filenames under a rules-* prefix and simplify to the files that actually carry state: - r-all.active.yaml -> rules-vendored.yaml (what semgrep runs) - exclusions.toml -> rules-status.toml (human decisions: excluded/active) - update.py -> rules-update.py (the updater) - rule-state.json -> DELETED (see below) - EXCLUSIONS.md -> DELETED (see below) Why the two deletions: - EXCLUSIONS.md was a rendered join of the toml (decisions) + rule-state.json (dates) + snapshot (descriptions). Its only non-derivable column was the description gloss; the rest duplicated rules-status.toml. It's redundant with the source files, so it's dropped. - rule-state.json stored {version_id, updated} per rule. `updated` existed only to fill EXCLUSIONS.md's rule-updated column; `version_id` existed only to compute `updated`. With the doc gone, nothing consumes either field — so the file has no reason to exist ("no version or updated" left). Deleted. Decision model is now two stored statuses (excluded/active) in rules-status.toml plus two DERIVED states reported each run for human action: - new — a feed rule with no decision, not seen last run (triage it). - orphaned — a rules-status.toml entry whose rule left the feed (prune or keep). Both are derived statelessly from artifacts already in git (the prior rules-vendored.yaml + rules-status.toml reconstruct "ids seen last run"), so no state file is needed. Nothing is auto-deleted; the human decides. rules-update.py loses the json/date/render_doc/render_table/sanitize machinery (~130 lines) and only reads rules-status.toml, so hand-written TOML comments are preserved. Verified: rules-vendored.yaml rule bodies are byte-identical to the old r-all.active.yaml (3005 active / 126 excluded); regeneration is deterministic; new/orphaned detection works; build green. Updated all references: mise.toml (SEMGREP_RULES, security:semgrep, semgrep:update), CI security-scanners.yml, .pre-commit-config.yaml (config path + exclude regex), .gitleaks.toml. .semgrepignore's tools/semgrep/*.yaml glob still covers the renamed vendored file. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): allowlist former vendored-file name for gitleaks history scan gitleaks scans full git history. Renaming r-all.active.yaml -> rules-vendored.yaml left the old-named blobs in earlier branch commits (c8f5403, b189d87) no longer matched by the allowlist, so gitleaks flagged their embedded secret-detection regexes (slack-webhook-url, private-key) — failing both the standalone `gitleaks` check and the `security:gitleaks` step of `mise run build`. Allowlist BOTH the current and former filename so the branch history scans clean. (Post-squash-merge, main only ever has the new name, but CI scans the branch history, which still contains the old name.) Verified: gitleaks reports "no leaks found" over full history. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(security): add tools/semgrep/README.md Add a short directory README so the vendored-ruleset setup is legible in the GitHub folder view without opening the files: what each file is (and which is human-edited vs generated), how to refresh it (`mise run semgrep:update`), and a glossary of the excluded/active/new/orphaned states. Deliberately carries no volatile figures (rule counts, ids) so it can't go stale; the rules-update.py docstring remains the authoritative design reference. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(security): address PR review nits on vendored-semgrep tooling Three minor, non-blocking review items: - rules-status.toml header named the old files (exclusions.toml, r-all.active.yaml, EXCLUSIONS.md, update.py). It's the file humans edit, so fix the comments to name the files that actually exist (rules-status.toml, rules-vendored.yaml, rules-update.py) and drop the removed EXCLUSIONS.md references — new/orphaned are reported on the console by rules-update.py. - Make the deterministic sort total: tie-break active blocks on their text (rb[0], rb[1]) so duplicate ids (r/all reuses some) also order stably instead of falling back to the download's nondeterministic order. Latent today (0 active dups — the dups are all excluded); byte-identical output verified, so no change to the vendored file now. - Document split_rules' assumption that rule content never begins a column-0 "- " line; if it ever did, the mis-split block fails rule_id_of and aborts loudly rather than silently corrupting the ruleset. (The reviewer's third note — security:semgrep --quiet -> --verbose — was the intentional flag alignment; no change needed.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Sphia Sadek <isadeks@gmail.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
3 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.
More GitHub Improvements
Related
Changes
dprintand added security scanners intomise.tomlNote
clamavandsonarqubearen't available within miseImportant
A follow up PR should keep the security runs in one place
Option A: Shared shell scripts with mode flags
Extract each scanner's core arguments that accepts a switch:
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.