Skip to content

feat(databases-on-aws): add native DSQL foreign key support - #261

Merged
anwesham-lab merged 3 commits into
awslabs:mainfrom
davidrz15:enable-foreign-key-support
Sep 1, 2026
Merged

feat(databases-on-aws): add native DSQL foreign key support#261
anwesham-lab merged 3 commits into
awslabs:mainfrom
davidrz15:enable-foreign-key-support

Conversation

@davidrz15

@davidrz15davidrz15 commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add native Aurora DSQL foreign-key guidance using referenced and referencing terminology.
  • Update DDL, migration, ORM, and troubleshooting guidance to preserve supported foreign keys and remove the application-layer referential-integrity workflow.
  • Add focused foreign-key eval coverage and update plugin metadata.
  • Keep the functional-eval harness unchanged.

Part of the foreign-key rollout with:

Validation

  • mise run build
  • Python tests: 65 passed
  • JSON, Markdown, formatting, manifest, cross-reference, reference, and size validation
  • Bandit, Gitleaks, Checkov, and Grype

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.

@davidrz15
davidrz15 requested review from a team as code ownersAugust 27, 2026 16:48
@davidrz15davidrz15 changed the title Foreign key feature releasefeat(databases-on-aws): add native DSQL foreign key supportAug 27, 2026
@davidrz15
davidrz15force-pushed the enable-foreign-key-support branch from c8df943 to d32d3bdCompareAugust 27, 2026 17:23
@anwesham-lab

anwesham-lab commented Aug 27, 2026

Copy link
Copy Markdown
Member

Note: Automated review pass using the dsql-skill-author review instructions spawning a full fleet using pr-toolkit-review:review-pr and code-review:code-review. All findings are graded for confidence.

Reviewed the FK rollout across the skill content and the eval harness. The DSQL-side premise checks out: native FK support is confirmed in the Aurora DSQL release notes (2026-08-26), every row of the new Supported Options table matches the CREATE TABLE / ALTER TABLE / SET CONSTRAINTS syntax pages, and every DDL example added here passes dsql_lint with 0 diagnostics. The fk-replacement.md deletion is clean (0 dangling refs, 0 broken links, 0 bad anchors across 372 links in the skill tree), all three manifests bump to 1.8.0 in lockstep, and all 22 CI checks are green.

The blocking problems are in the eval harness, and they are not theoretical — they were reproduced by running it.--bare removes the Skill tool from the subject run, so the suite grades the bare model rather than the skill (#1). Two of the four README commands this PR adds exit 1 immediately because .claude/.mcp.json isn't in the repo (#2). A third ran for 114s, spent $0.47, made 17 tool calls against a hardcoded --max-turns 10, and was then discarded as an infrastructure error with empty stderr — reported as 0/0 (0%) rather than as a failure (#3). Three further silent-green paths mean an ungraded or mis-graded suite still exits 0 (#4, #5, #7). Net effect: no eval currently exercises this PR's content, and the harness cannot presently tell you that.

On the content side, the dsql_linttransact snippets execute the unlinted original at 9 new call sites (#6), the destructive-operation gate in constraint-operations.md became bypassable (#8), and dsql_lint_eval_results.md still publishes PASS verdicts whose recorded evidence is now the behaviour this PR defines as wrong (#9). #19 onward are quality and consistency items.

This PR is also two changes in one: FK skill content, and ~890 lines of unrelated eval-harness hardening (judge sandboxing, redaction, infra-error taxonomy, exit codes). Landing the harness first and re-baselining would make the FK content's eval deltas mean something.

Scope note: findings about the ORM adapters not yet emitting FK DDL, and about companion-PR merge ordering, are excluded per the author's confirmation that those PRs are in flight.

#ConfidenceAreaFindingSuggestionReviewed SHA
195run_functional_evals.py#L31-L33correctness--bare on the subject run strips the Skill tool. Verified on this branch: with --bare --plugin-dir plugins/databases-on-aws the init event reports tools: ['Bash','Edit','Read', ...] with no Skill entry, and the model answers "NONE" when asked which skills it can see; without --bare the same prompt emits TOOL: Skill {'skill': 'databases-on-aws:dsql'} then reads references and calls dsql_lint. Every functional / pg-migration / lint / safe_query eval therefore grades the bare model rather than this PR's guidance. --bare additionally skips the plugin's PostToolUse hook in hooks/hooks.json, so the suite no longer exercises the plugin as users install it.Drop --bare. For hermeticity use --strict-mcp-config --setting-sources "" and keep Skill available; if --bare must stay, prefix each prompt with /databases-on-aws:dsql (the only way skills resolve under --bare) and assert in run_prompt that the subject actually loaded the skill.d32d3bd
292README.md#L92-L120docsTwo of the four new copy-paste commands pass --mcp-config .claude/.mcp.json, which is not in the repo and not gitignored (ls .claude/ → only settings.json). Both were run verbatim and both die before executing any eval: ERROR: MCP config does not exist: .claude/.mcp.json / EXIT=1. The accompanying caveat at L123-124 is also wrong: running with --mcp-config plugins/databases-on-aws/.mcp.json --strict-mcp-config produced an init event listing mcp__aurora-dsql__dsql_lint, get_schema, readonly_query, transact"disabled": true is not honoured on that path.Either document .claude/.mcp.json as operator-supplied and gitignored (as the Workflow-9 section already does at L242-244), or commit an eval-only config and point the commands at it. Drop --mcp-config from the lint command entirely — dsql_lint needs no cluster and is reachable via the shipped config. Replace the caveat with the real constraint (transact/readonly_query need CLUSTER_ENDPOINT; dsql_lint does not).d32d3bd
390run_functional_evals.py#L34correctness--max-turns 10 is hardcoded, and turn exhaustion is misclassified as infrastructure. Observed on the README's own safe_query FK command: eval 5 made 17 tool calls, claude exited 1, and the eval was discarded as "infrastructure_error": "Subject run exited 1: " with empty stderr — printed as Result: 0/0 passed (0%) after 114s and $0.47, with nothing to diagnose from. subtype == "error_max_turns" is bucketed the same way (L142-161), and a single stray non-JSON stdout line (L107, L119-122) voids a whole eval identically. The evals this PR adds need strictly more turns (212 polls sys.jobs; 214 is a 9-step flow with lint plus pre- and post-verification), so they are the cases most likely to vanish rather than be graded.Make --max-turns a CLI flag with a higher default (~25). Restrict infrastructure_error to returncode != 0 with a real stderr, OSError, and a missing final result event; grade error_max_turns normally or bucket it as truncated and count it failed. When returncode != 0 with empty stderr, surface the last result event and turn count. Downgrade lone parse errors to a warning when a result event was captured.d32d3bd
492run_functional_evals.py#L472-L474correctnessThe PR deletes the negation-aware "not use foreign key" regex branch and moves FK assertions onto llm_judge, but the mode stays an optional key read as bool(eval_item.get("llm_judge", False)), so a missing or misspelled key silently falls back to keyword-ratio grading. Reproduced by executing grade_eval at this SHA: expectation "Preserves the native foreign key and does not report it as an unsupported DSQL feature" graded against text saying "DSQL does not support the FOREIGN KEY feature; it is an unsupported DSQL feature" returned passed: true, "Matched 6/8 keywords" — the keyword grader passes the exact inverse of the assertion. "llm_judge": "false" is also truthy.Make the grader explicit and validated at load: a required "grader": "regex" | "llm_judge" parsed into an Enum, raising on unknown or non-bool values, so a missing or stringified flag is a load-time error rather than a silent downgrade to the weakest grader.d32d3bd
592run_functional_evals.py#L474-L476correctnesseval_item.get("expectations", []) turns a missing, misspelled or empty expectations key into zero graded assertions, and the new exit gate (return 1 if total_failed or infrastructure_errors else 0) then returns 0. Verified by execution: an item with "expectation" instead of "expectations" grades nothing; a two-eval file with one missing key and one [] exits 0 reporting total_expectations: 0. A fully ungraded suite reports green. Relatedly, pointing --evals at trigger_evals.json (a top-level list) raises a bare TypeError at L775.Validate the loaded file up front: require isinstance(evals_data, dict), a skill_name, and a non-empty expectations on every case; reject unknown keys; exit 1 with a message naming the expected schema. Add if total_expectations == 0: return 1.d32d3bd
688foreign-keys.md#L20-L36correctnessAll 9 new lint snippets call dsql_lint(sql=X, fix=True) on a bare line, discard the return value, then transact([X]) — executing the unlinted original and throwing away fixed_sql. The prose one line above says "Proceed only when the lint result has no unfixable diagnostics", so the example contradicts its own rule, and agents follow the example. This bites hardest on the case the PR is about: the linter's job on a post-creation FK is to inject the missing NOT VALID (rule foreign_key_not_valid, fixed_with_warning), so copying this pattern executes DDL that DSQL rejects. Same at constraint-operations.md L88-89 / L99-100 / L136-137 and ddl-type-alternatives.md L149-155.lint = dsql_lint(sql=ddl, fix=true) → stop and report if any diagnostic is unfixabletransact([lint["fixed_sql"]]). Apply to all 9 sites. Also add the lint step to examples/patterns.md L53-73, which omits it entirely while SKILL.md:255 makes it a MUST.d32d3bd
785run_functional_evals.py#L709-L730correctnessJudge and infra failures are dropped from both the per-eval total and the aggregate denominator, so overall_pass_rate covers only expectations that were actually graded. A run reported overall_pass_rate: 1.0 / OVERALL: 3/3 (100%) with 3 of 6 expectations never graded. The verbose printer compounds it: it prints those same expectations as [FAIL] directly beneath Result: 3/3 passed (100%), and when the subject run dies it prints only 0/0 passed (0%) while the reason lands solely in grading.json. Anyone pasting that rate into a results doc publishes an invented 100%.Emit graded_total alongside requested_total; return null for overall_pass_rate when infrastructure_errors > 0; tag ungraded expectations [ERROR] not [FAIL]; print grading["infrastructure_error"] to stderr.d32d3bd
886constraint-operations.md#L143-L146correctnessThe file-level **MUST read [overview.md](overview.md) first** for destructive operation warnings moved off line 5 to an unheaded paragraph at L145, while the new TOC at L5-13 links straight to #drop-constraint-migration and #modify-primary-key-migration — both below L145. An agent routing via the TOC now reaches destructive table-recreation steps without ever passing the confirmation gate. This is the same class of deletion held as non-negotiable in #157 ("This statement definitely cannot be deleted regardless of content condensing").Restore the MUST read overview.md first line in the file header ahead of the TOC, scoped to "before any table-recreation section", and/or repeat it as the first line under each of the three table-recreation headings.d32d3bd
993dsql_lint_eval_results.md#L46-L57docsThe +4/-4 caveat is not enough — the file still publishes PASS verdicts whose recorded evidence is the behaviour this PR defines as wrong. L49 Foreign key guidance | PASS App-layer enforcement | PASS App-layer enforcement | Both correct; L55 ...removed FK...; L109 Warned about foreign key removal requiring app-layer enforcement — which would now fail eval 101's new expectation #4. Re-running both prompts through live dsql_lint gives eval 100 → 2 diagnostics (serial_type, index_async), not the recorded 4, and eval 101 → 3, not the recorded 5; foreign_key is gone from both and the FK survives verbatim in fixed_sql. L144 also claims all four expectations in ... eval 102 when eval 102 has three (true on main too).Re-run and regenerate the snapshot, or mark the 100/101 verdicts STALE (pre-native-FK) inline in the summary table and strike the per-row "Foreign key guidance / Both correct" claims — a blanket note at L8 cannot flip a recorded PASS. Fix "four" → "three" at L144.d32d3bd
1090dsql_lint_evals.json#L4-L16testsEval 100's prompt contains team_id INT REFERENCES teams(id), yet it is the one FK-bearing lint case left completely untouched: no llm_judge flip and, unlike 101/103, no preserve-FK expectation — so all 5 of its expectations run through the weak keyword-overlap fallback. Its expectation 4 (For fixed_with_warning diagnostics, explains application-layer implications before proceeding) was written against the foreign_key app-layer warning that live dsql_lint no longer emits; the two surviving fixed_with_warning details are identity-type widening and async-index readiness, nothing to do with referential integrity.Add "Preserves the native foreign key to teams and does not report it as an unsupported DSQL feature" to eval 100, set "llm_judge": true to match 101/103, rewrite expectation 4 against the diagnostics the linter actually returns, and add 100 to the README's lint --eval-ids list (currently 101,103).d32d3bd
1190safe_query_evals.json#L80-L88testsEval 5 grew 5→7 expectations and was narrowed in ways the skill contradicts. It now requires tenant_id validated "with the UUID regex", but Pattern 5 in mcp/tools/workflow-patterns.md:101 — rewritten by this same PR — still emits regex(tenant_id, TENANT_SLUG), as does every other tenant example in mcp/tools/*.md; the removed UUID or TENANT_SLUG disjunction was the tolerance that made the assertion valid. It also newly requires the active-parent SELECT and INSERT "in the same transaction so OCC detects a concurrent parent status change", but the PR deleted the SELECT entirely from Pattern 5, leaving one build() and a bare transact([insert]), with authorized_tenant_ids referenced but never defined. Expectations 1, 5 and 6 are now untaught by the reference the eval exercises, and expectation 1 still says "existence check" after the prompt moved to an active check.Restore a build()-ed active-parent SELECT inside the same transact([...]) as the INSERT, noting that the SELECT is what places the parent row in the OCC read set; align the validator (UUID in Pattern 5, or restore the disjunction in the eval); resolve authorized_tenant_ids from an explicit argument; reword expectation 1 to "active-parent check".d32d3bd
1290run_functional_evals.py#L597-L620correctnessFlipping evals.json eval 2 to llm_judge: true orphaned two dedicated regex branches into dead code: create index async (L597-606) and separate transaction (L608-620) each had exactly one consumer on main (eval 2) and now have zero across all four corpora; the tenant_id substring branch dropped from 4 consumers to 2. Eval 2 traded three cheap deterministic signals for judge verdicts. The parallel inline comment was also left stale: L687 still says evals 6-9 and L689 Regex branches below cover only evals 1-5, both false now, and contradicting the docstring the PR did update at L440-443.Keep eval 2's CREATE INDEX ASYNC and separate-DDL-transaction assertions in a regex-graded companion eval, or delete the now-unreachable branches. Update L687-690 to match the docstring (and note the branches sit above, not below).d32d3bd
1384full-example.md#L40-L56correctnessThe migrated DDL adds CONSTRAINT products_tenant_fkey FOREIGN KEY (tenant_id) REFERENCES tenants(id), but the example never creates tenants — so it fails with relation "tenants" does not exist, violating this PR's own rule at foreign-keys.md:7-9 ("Create the referenced table first"). The new mapping row also pins tenant_id to INTEGER while the same file maps INT AUTO_INCREMENT → UUID, so a tenants.id migrated by this guide would not be INTEGER.Add the tenants CREATE TABLE as step 0 in its own transact, with an explicit id type, and reword the mapping row to say the child column MUST match whatever the parent key actually became.d32d3bd
1482ddl-type-alternatives.md#L140-L156multi-tenant-leakThe new canonical FK example emits FOREIGN KEY (customer_id) REFERENCES customers(id) with no tenant_id on either side — contradicting L163-164 three lines below and foreign-keys.md:38-39, which state that referencing the child key alone "can permit a child row to reference another tenant's parent". Agents imitate the code block, not the caveat under it. It also converts the child's own PK to UUID while leaving customer_id INTEGER REFERENCES customers(id), so if customers.id is converted by the same rule the DDL fails on type mismatch, and it silently promotes a nullable MySQL column to NOT NULL.Make the copy-paste block the tenant-scoped composite form (tenant_id UUID NOT NULL, FOREIGN KEY (tenant_id, customer_id) REFERENCES customers (tenant_id, customer_id)); keep the single-column form only as an explicitly labelled single-tenant variant; match the FK column type to the converted parent key; call out the nullability change.d32d3bd
1580ddl-structural.md#L19-L27correctnessThe new line "Add a CHECK constraint directly with NOT VALID, then validate it asynchronously" is rejected by the shipped linter: dsql_lint on ALTER TABLE ... ADD CONSTRAINT ... CHECK (...) NOT VALID returns at_unsupported_add_check with fix_result.status = unfixable, and the skill forbids executing while any diagnostic is unfixable — so the agent deadlocks. (The FK path is fine: foreign_key_not_valid / fixed_with_warning.) L29-47 also still demand a full pre-scan with MUST ABORT if invalid_count > 0, exactly what the NOT VALID path exists to avoid.Keep this PR scoped to FKs: drop the ADD CHECK sentence here and the "CHECK or foreign key" wording at ddl-migrations/overview.md:66-67, and scope the Pre-Migration Validation block to the UNIQUE / table-recreation path.d32d3bd
1678type-mapping.md#L124correctnessThis line still reads -- After (DSQL) — run dsql_lint first for SERIAL/FK/index fixes, implying the linter rewrites FKs — the exact premise of the deleted fk-replacement.md. Verified: dsql_lint(fix=true) on a composite-FK CREATE TABLE returns 0 diagnostics and byte-identical fixed_sql. The file is a MUST-load for Workflow 10, so a migrating agent is told the opposite of the new policy.Change to run dsql_lint first for SERIAL/index fixes; supported foreign keys are preserved and link pg-migrations/foreign-keys.md.d32d3bd
1778run_functional_evals.py#L406-L419correctness_redact_judge_value(message.get("content", ""))[:1000] slices whatever the recursive redactor returns. Real stream-json tool_result blocks carry content as a list, so the guard becomes a 1000-element slice, not a 1000-char cap — one list-shaped error with 5000 chars expanded to 4879 chars and consumed the whole TOOL ERRORS section; a dict-shaped content raises TypeError: unhashable type: 'slice'. Both tests pass content as a str, so the realistic shape is never exercised. Separately, only is_error results reach the judge, so assertions about successful tool output (dsql_lint_evals.json:52, pg_migration_evals.json:170) are graded on the agent's word alone.json.dumps(_redact_judge_value(...), default=str)[:1000]; add a list-shaped test case; include redacted successful tool results in a TOOL RESULTS: section so output-dependent assertions are gradeable.d32d3bd
1875run_functional_evals.py#L776-L784correctnessAn unresolved --eval-ids entry only prints WARNING: eval IDs not found and continues, so a run that graded none of the requested evals still exits 0 if whatever it found passed. All four new README commands hard-code explicit ID lists (202,206,207,212,213,214, 2,13, 5, 101,103), so one renumbered ID silently drops the FK eval and still reports green. No test covers --eval-ids at all.Make a partial miss fatal (return 1), or record skipped_eval_ids in the summary and include it in the exit condition. Add a test asserting non-zero for an absent ID.d32d3bd
1975pg_migration_evals.json#L29testsEvals 202 and 207 require "Preserves ON DELETE CASCADE", but the guidance they grade says the opposite: foreign-keys.md:80-81 "SHOULD default to NO ACTION. Use CASCADE ... only when the user explicitly intends the dependent-row changes and confirms their impact", echoed at constraint-operations.md:108. A skill-compliant agent that surfaces CASCADE and asks first is graded as failing.Reword both to "Preserves ON DELETE CASCADE from the source schema after surfacing its dependent-row and transaction-limit impact for confirmation."d32d3bd
2074test_run_functional_evals.py#L333-L347tests19 tests pass, but the ones guarding this PR's behaviour don't. The two ..._uses_llm_judge tests assert only llm_judge is True on JSON this PR edited — tautological, and passing with grade_eval / _llm_judge / _build_judge_evidence fully broken; they also cover safe_query 5 and dsql_lint 101/103 but not evals.json 2 or the new pg_migration 212/213/214, so the guard is asymmetric with the change. The two main() tests (L407-530) monkeypatch bothrun_prompt and grade_eval, verifying exit arithmetic over a hand-written summary and nothing else — test_main_aggregates_... returns a run_result lacking result_text/tool_calls/messages, which the real grade_eval would KeyError on.Replace the flag tests with one that monkeypatches _llm_judge and asserts it is invoked per expectation with no regex branch taken, extended to eval 2 and 212-214. For main(), mock only subprocess.run and let run_promptgrade_evalmain run for real, asserting grading.json contents and summary.json totals.d32d3bd
2172foreign-keys.md#L39-L57multi-tenant-leakThe file mandates composite (tenant_id, customer_id) FKs as the cross-tenant guarantee and correctly documents MATCH SIMPLE as the default — but never connects the two: under MATCH SIMPLE a NULL in either child column skips the check entirely, so the tenant-scoping guarantee silently does not hold unless the child key columns are NOT NULL or MATCH FULL is used. The read-set / snapshot-isolation explanation that eval 5 depends on was also lost with fk-replacement.md (grep -ri "read set|snapshot isolation" over the skill now returns nothing).Add "MUST declare child FK columns NOT NULL (or use MATCH FULL) for tenant-scoped foreign keys", restore the read-set paragraph to Transactions and Concurrency, and link working-with-foreign-key-constraints.html, whose KEY SHARE model the concurrency bullets paraphrase.d32d3bd
2272foreign-keys.md#L32styleThe canonical new-table example appends bare DEFERRABLE, departing from DSQL's documented NOT DEFERRABLE default with no rationale (semantics only appear 16 lines later at L48; L80 names only NO ACTION as a default). Bare DEFERRABLE also changes nothing without SET CONSTRAINTS, so it gets cargo-culted into every FK an agent generates — and deferring the check to COMMIT means intra-transaction reads can observe a row whose cross-tenant parent reference is not yet validated. No other FK example added in this PR includes it.Drop DEFERRABLE from L32 and add "SHOULD default to NOT DEFERRABLE" beside the NO ACTION default at L80, keeping the deferred form only in the SET CONSTRAINTS example at L62-78.d32d3bd
2372constraint-operations.md#L56-L61correctnessOption B changed from readonly_query("SELECT sys.wait_for_job(...)") to a bare CALL sys.wait_for_job('<job_id>'); plus "run through a database client with autocommit enabled". CALL is probably right (dsql_lint's own suggestion text uses it, and the async-index page calls it a procedure), but no shipped tool can execute it — readonly_query won't accept CALL and transact always opens an explicit transaction — so #238 deliberately wrote both options as MCP calls and Option B is now unreachable. The autocommit requirement appears in no AWS doc. The fence is also tagged ```sql while containing Python.Point Option B at plugins/databases-on-aws/scripts/psql-connect.sh as the concrete autocommit client (explicitly "not transact"), or drop Option B and keep only the sys.jobs polling loop the MCP tools can actually run. Split the mixed fence.d32d3bd
2470foreign-keys.md#L11-L12testsSeveral new hard rules have no eval, so the behaviour claims are unverified and can regress silently. Greps across all four corpora return 0 hits for ALTER CONSTRAINT (the only FK lifecycle op with no net), NO ACTION (the rule that stops an agent volunteering CASCADE), and non-deferrable (the referenced-key PK/UNIQUE requirement); the ON UPDATE column-subset prohibition and the child-side CREATE INDEX ASYNC for eval 212's cascading-delete scenario are likewise unasserted. Conversely no eval asserts that dropping a CHECK/UNIQUE constraint still needs table recreation — the over-generalization this PR creates by narrowing the rule to "non-FK DROP CONSTRAINT".Add the five cases above plus the CHECK/UNIQUE inverse, and extend eval 212 with "Recommends CREATE INDEX ASYNC on the child FK columns (tenant_id, customer_id)".d32d3bd
2570foreign-keys.md#L3styleAuthoring-style issues concentrated in the new content: "Preserve supported foreign keys" appears 7 times but no file says which FKs are unsupported, making the qualifier unactionable; the composite-tenant-key rule is restated 6 times carrying MUST only once (L38), and the 40001-retry and cascade/transaction-limit rules 4 times each with no keyword; FK/FKs/foreign key/FOREIGN KEY are interchanged on ~10 changed lines (SKILL.md:61 uses two in one row); referenced/referencing competes with parent/child inside single paragraphs; and the 9 new call sites write fix=True where all 16 pre-existing ones write fix=true.Add an explicit "Unsupported" subsection (MATCH PARTIAL, ON UPDATE column subsets, referenced key without non-deferrable PK/UNIQUE) and drop the "supported" qualifier; keep each rule's MUST statement in foreign-keys.md only, replacing the copies with links; standardize on foreign key, referenced/referencing, and fix=true.d32d3bd
2670foreign-keys.md#L1-L119routingThe canonical native-FK reference lives under pg-migrations/ and is listed in SKILL.md under ### PostgreSQL Migrations:, yet it is the general FK reference — inbound from SKILL.md:238 (Workflow 4), SKILL.md:273, development-guide.md:110, mysql-migrations/ddl-structural.md:27, and ddl-type-alternatives.md:164. An agent doing MySQL-migration or greenfield DDL work reads a PG-scoped group and can miss the MUST-load. Worse, SKILL.md:45 still routes unqualified "ADD/DROP CONSTRAINT" to constraint-operations.md, which carries a complete standalone ADD/DROP FOREIGN KEY procedure and zero links back to foreign-keys.md — so that path executes FK DDL without the tenant-composite MUST, the NO ACTION default, or the SET CONSTRAINTS rule. SKILL.md:61 also advertises "validation", which actually lives in constraint-operations.md.Move the file to references/foreign-keys.md, list it under ### Core:, qualify SKILL.md:45 ("ADD/DROP CHECK or UNIQUE CONSTRAINT ... for foreign keys load pg-migrations/foreign-keys.md first"), and add a MUST read back-pointer at constraint-operations.md:71 and :114.d32d3bd
2768troubleshooting.md#L82-L93styleThe new FK entry sits as an ### nested under ## Incompatibility → "remember DSQL doesn't support:", so an agent scanning that section reads FK as an incompatibility. It also abandons the file's convention — all seven siblings are ### Error: "<literal text>" with **Cause:**/**Solution:** — and the PR deleted the greppable ### Error: "Foreign key constraint not supported" anchor, so an agent matching a runtime error string no longer lands here.Promote it to its own ## outside ## Incompatibility, split into ### Error: "cannot add foreign key constraint without NOT VALID" and ### Error: foreign key VALIDATE CONSTRAINT job failed, each with **Cause:**/**Solution:** quoting the real server messages.d32d3bd
2868README.md#L17-L19docsThe two lines this PR adds to the directory tree carry no prompt/assertion counts while every sibling does (evals.json 16/60, safe_query_evals.json 6/~30, query_explainability_evals.json 9/70). Verified actual counts: dsql_lint_evals.json 4/19, pg_migration_evals.json 18/97. Three new evals were added to pg_migration_evals.json with no "What it checks" row anywhere. The tree also still omits four files that exist in that directory (data_loading_eval_results.md, dsql_lint_eval_results.md, pg_migration_hallucination_evals.json, pg_migration_hallucination_results.md) while reading as exhaustive. This was a Fix-severity item on #207.Annotate both lines with counts, add a "What it checks" table for pg_migration_evals.json covering 212/213/214, and either list the four missing files or mark the tree partial.d32d3bd
2965pg_migration_evals.json#L22testsDead schema, hand-maintained by this PR. expected_output and files are present on all 44 cases across the four corpora, name on pg_migration/safe_query cases, and a top-level focus on two files — and no .py consumer reads any of them (the runner reads only id, prompt, expectations, llm_judge, and top-level skill_name; eval_name is hardcoded to f"eval-{eval_id}"). This PR carefully rewrote expected_output on 6 cases and renamed name on 202 and safe_query 5, none of which affects grading — prose that can silently drift from the assertions that do.Either feed expected_output into the judge context / eval_metadata.json so it becomes load-bearing, or drop the field from the schema. Add a test asserting expected_output shares key terms with the expectations so the two cannot diverge.d32d3bd
3088pr-body / commit-msgprocessNo eval results are posted, despite this PR flipping 8 existing expectations, converting 5 evals to LLM-judge, adding evals 212-214, and adding four README commands to run exactly those evals. Maintainer precedent is explicit: "we should re-run evals after this, I would expect different results" (#162), "You can also submit the eval results here or via comment" (#168), "we should add some eval tests to the agent to confirm that the agents behave in the expected manner" (#238); #168/#176/#207 all posted tables. The body and commit message are byte-identical and mention neither the 1.7.1 → 1.8.0 release, the ~890-line harness rewrite, nor the new non-zero exit contract; .github/pull_request_template.md requires #### Related and #### Changes, both absent. Findings #1-#5 must be fixed first — any numbers produced by the harness as it stands would be meaningless.Fix #1-#5, re-run, and post a per-suite pass/fail table (subject model, judge model, date) plus a committed fk_eval_results.md. Fill in the template sections and mirror the breakdown into the commit body, including "run_functional_evals.py now exits 1 on failed assertions".d32d3bd

Reviewed at head SHA d32d3bd08e3a4098a7a38eed2fdc90ef6799d9a0. Compare against current HEAD to spot stale rows.

@davidrz15

Copy link
Copy Markdown
ContributorAuthor

Moved harness changes into #262

@davidrz15
davidrz15force-pushed the enable-foreign-key-support branch 2 times, most recently from e409b27 to ce36b84CompareAugust 31, 2026 15:53
@anwesham-lab
anwesham-labforce-pushed the enable-foreign-key-support branch from ce36b84 to 24739c9CompareAugust 31, 2026 19:08
Comment threadplugins/databases-on-aws/skills/dsql/mcp/tools/workflow-patterns.md Outdated
@anwesham-lab

Copy link
Copy Markdown
Member

Re-reviewed ce36b84696247df755c21d3a8dc187977e3f84d6 (the earlier 30-finding comment was against d32d3bd0; harness changes moved to #262). Method: 36-agent fleet, the DSQL skill-author review procedure, /code-review and /pr-review-toolkit, two security passes, official Aurora DSQL and Rails documentation, live dsql_lint calls, and ~70 isolated statements against the supplied DSQL cluster in a disposable scratch schema.

The paradigm change is correct and complete. Six independent sweeps found no residual "FKs are unsupported / enforce in the application layer" guidance anywhere in plugins/databases-on-aws/, no dangling links to the deleted fk-replacement.md, and no novelty framing. The findings below are execution defects, not a disagreement with the direction.

Confidence = confidence the finding is real and will be hit in practice. 100 = reproduced directly against the cluster or the live tool; 85-98 = verified from code/docs with a judgment component; 75-84 = rests on repo convention plus reviewer precedent rather than a hard failure. Anything under 75 is parked below the table.

Validated findings

#ConfidenceAreaFindingSuggestionReviewed SHA
1100dsql_lint_evals.json#L23-L55testsEnabling llm_judge on evals 101/103 moves operational assertions (tool invoked, fix=true passed, execution order, reference loaded, non-execution) to a judge that receives only the final answer. An agent can claim it called dsql_lint without doing so; a correct terse answer can fail. Worst on 103, an anti-hallucination eval. Validated: Traced grade_eval() at this SHA — the regex path builds full_text from tool calls + messages, the llm_judge path passes only result_text. In-memory probes reproduced both a false pass and a false fail.Split per-expectation graders: inspect captured tool calls deterministically for operational assertions, reserve the judge for semantic ones.ce36b84
2100workflow-patterns.md#L88-L96correctnessPattern 5 is a bare table-constraint fragment, not a workflow pattern. Patterns 1-4 are all runnable transact([...]) calls; this one cannot execute. It also drops the file's only safe_query.build() + literal() demonstration. Validated:dsql_lint(fix=true) returned an unfixable parse_error: "Expected an SQL statement, found FOREIGN."Embed in a complete CREATE TABLE inside transact([...]), or show the ALTER TABLE ... ADD CONSTRAINT ... NOT VALID workflow. Restore the build() usage.ce36b84
3100development-guide.md#L79-L86, constraint-operations.md#L44-L50correctnessSELECT sys.wait_for_job(...) is invalid. This commit added the correctCALL form at development-guide.md:85, four lines below the incorrect SELECT at :81, and left constraint-operations.md:48 wrapping SELECT in readonly_query. One file now teaches both forms. Validated: Cluster: pg_proc.prokind = 'p'. SELECT sys.wait_for_job(...)ERROR: sys.wait_for_job(unknown) is a procedure. HINT: To call a procedure, use CALL.CALL succeeded.Change both SELECT sites to CALL. Note that transact/readonly_query open a transaction, so CALL must come from an autocommit session — poll sys.jobs through the MCP tools instead.ce36b84
4100foreign-keys.md#L104-L117correctnessNOT VALID is presented as a performance choice — "so it applies to new writes without scanning existing referencing rows". It is mandatory. troubleshooting.md:69 states it correctly, so the MUST-load reference is the file that understates it. Validated: Cluster: bare ADD CONSTRAINT ... FOREIGN KEYERROR: unsupported ALTER TABLE ADD CONSTRAINT statement. With NOT VALID → succeeds."Post-creation foreign keys MUST be added NOT VALID; Aurora DSQL rejects ADD CONSTRAINT ... FOREIGN KEY without it."ce36b84
5100foreign-keys.md#L119-L128correctnessASYNC is required for VALIDATE CONSTRAINT and the skill never says so, while development-guide.md:86 ("DDL ALWAYS runs asynchronously") primes agents to attach ASYNC to ADD CONSTRAINT too, where it is ungrammatical. Validated: Cluster: ALTER TABLE ... VALIDATE CONSTRAINTERROR: unsupported ALTER TABLE VALIDATE CONSTRAINT statement. ALTER TABLE ASYNC ... VALIDATE CONSTRAINT → returned a job_id.State that ASYNC applies only to VALIDATE CONSTRAINT and is required there; ADD CONSTRAINT ... NOT VALID is synchronous and returns no job_id.ce36b84
6100foreign-keys.md#L48-L56correctness"RESTRICT actions always remain immediate" is true but incomplete as an implied rule. Singling out RESTRICT invites the inference that DEFERRABLE INITIALLY DEFERRED + ON DELETE CASCADE defers the cascade. It does not. Validated: Cluster: FK with ON DELETE CASCADE DEFERRABLE INITIALLY DEFERRED; inside a transaction, child row count was already 0 immediately after the parent DELETE."Only the NO ACTION check can be deferred. RESTRICT, CASCADE, SET NULL, and SET DEFAULT are always evaluated immediately, even on a DEFERRABLE INITIALLY DEFERRED constraint."ce36b84
7100foreign-keys.md#L48-L56correctness"It changes only deferrable foreign keys" reads as a silent skip. The named and ALL forms behave differently, and the difference is a transaction abort. Validated: Cluster: named → ERROR: constraint "dfr1" is not deferrable. SET CONSTRAINTS ALL DEFERRED with a non-deferrable constraint present → succeeded silently."Naming a NOT DEFERRABLE constraint raises an error and rolls back the transaction; SET CONSTRAINTS ALL silently affects only deferrable constraints."ce36b84
8100foreign-keys.md#L79-L97correctnessThe example uses a bare SET CONSTRAINTS orders_customer_fkey DEFERRED. Constraint names are unique per table, not per schema, so a bare name matches every constraint of that name in the search path — defeating the file's own "make only the required constraint deferrable" goal at :66-67. Validated: Cluster: same constraint name created on two tables (one DEFERRABLE, one not). Bare SET CONSTRAINTS dfr1 DEFERREDERROR: constraint "dfr1" is not deferrable — it matched both.Schema-qualify the name, and require globally-unique constraint names (<referencing>_<referenced>_fkey) so collisions are structurally impossible.ce36b84
9100foreign-keys.md#L58-L62correctness"For SET NULL, each referencing column selected by the action MUST be nullable" reads as a DDL-time prerequisite. It is a runtime failure, so a reader will not add a delete-path test. Validated: Cluster: CREATE TABLE with ON DELETE SET NULL over NOT NULL columns succeeded; the parent DELETE then failed with ERROR: null value in column "t" ... violates not-null constraint."…MUST be nullable. A non-nullable column fails at delete time with a not-null violation, not when the constraint is created."ce36b84
10100foreign-keys.md#L6-L10correctnessTwo gaps in the referenced-key precondition: a unique index does not satisfy it (unlike stock PostgreSQL, which accepts a non-partial unique index), and type compatibility is never stated at all. Also "non-deferrable" is unreachable wording — DSQL rejects DEFERRABLE PK/UNIQUE outright, so the qualifier describes an impossible state. Validated: Cluster: CREATE UNIQUE INDEX ASYNC then FK → ERROR: there is no unique constraint matching given keys. PRIMARY KEY (...) DEFERRABLE and UNIQUE (...) DEFERRABLEERROR: DEFERRABLE constraint not supported.Require a PRIMARY KEY/UNIQUEconstraint, state that each referencing column must be type-compatible with its referenced column, and drop "non-deferrable" (or note that DSQL applies DEFERRABLE to foreign keys only).ce36b84
1199foreign-keys.md#L15-L41safetyAll 7 new code blocks gate on summary.errors only, then execute fixed_sql. Per this repo's own dsql-lint.md:47 that counts unfixable only, so a fixed_with_warning rewrite ships unreviewed — violating dsql-lint.md:57 (obtain acknowledgement) and :79 (surface fixed_sql; prompt-injection defense). Not a linter-version concern. Validated: Live tool: foreign_key_not_valid and index_using both return {errors:0, warnings:1}. Response shape verified — summary.errors/warnings are ints, so the key paths are valid; the field chosen is wrong. Scope checked:git grep finds 0 instances in parent 8b13a50 and 7 at ce36b84 — introduced here, not pre-existing.Replace the 7 copies with one helper that surfaces every diagnostic, stops on unfixable, and requires acknowledgement for fixed_with_warning. Saves ~30 lines.ce36b84
1299type-mapping.md#L105-L112product framingNative foreign keys remain under "MySQL Features Requiring DSQL Alternatives", whose lead-in is "MUST use the following DSQL alternatives", listing FK beside genuinely unsupported features (FULLTEXT, TRIGGERS, PARTITION BY). A FK is not an alternative to a FK. This is the clearest surviving remnant of the old paradigm. Validated: Official DSQL docs show ordinary FOREIGN KEY ... REFERENCES syntax as supported; the heading directly contradicts its own row and the commit's intent.Delete the row and add FK to Directly Supported Operations: preserve the constraint, translate only source-specific DDL syntax, link ../foreign-keys.md.ce36b84
1399pg_migration_evals.json#L160-L190, evals README#L13-L27test coverageThe new FK evals (212, 213) and the changed lint evals sit outside any documented or automated regression command, and the README index omits both corpora entirely. This commit's FK coverage can regress while the documented suite stays green. Validated: Searched mise.toml, .github/**, and tools/** at this SHA: no task or command runs either file. grep for either filename in the README returns nothing. Confirmed by the repo itself — dsql_lint_eval_results.md#L6: "Automated grading for these evals is not yet wired into run_functional_evals.py; PASS/FAIL is a human assessment."Add a documented mise aggregate task covering evals.json, pg_migration_evals.json, and dsql_lint_evals.json; wire it into CI. Add both files plus rows for 212/213 to the README index.ce36b84
1498overview.md#L58-L114, ddl-operations.md#L39-L56data integrityThe Table Recreation Pattern now acknowledges FKs in prose but never inventories or restores them. It does DROP TABLERENAME → recreate indexes only. Inbound FKs block the swap; CASCADE "unblocks" it by silently deleting the other table's constraint, which step 7 never restores. Outbound FKs are dropped with the old table. Validated: Cluster: DROP TABLE on a referenced table → ERROR: cannot drop table ... because other objects depend on it ... constraint f on table tc2. With CASCADENOTICE: drop cascades to constraint f on table tc2, and the constraint was gone.Add a step-0 inbound/outbound pg_constraint inventory (contype='f'), re-declare outbound FKs on the new table, plan inbound removal/recreation with NOT VALID + async validate, and add MUST NOT use DROP TABLE ... CASCADE to clear inbound FKs.ce36b84
1598foreign-keys.md#L43-L44security/correctnessThe tenant rule is wrong in both directions. Too broad: requiring tenant_id in every multi-tenant FK is not a DSQL rule — shared/global reference tables and globally-unique IDs legitimately use ordinary FKs, and an FK enforces integrity, not caller authorization. Too narrow: where it does apply, it never requires the referencing key columns to be NOT NULL, and under the default MATCH SIMPLE a NULL in any key column skips the check entirely — so the stated guarantee is defeatable. Validated: Official DSQL FK docs impose no tenant-key requirement; shared lookup tables are a concrete counterexample. Cluster: with t UUID NOT NULL, c UUID and a composite FK, INSERT (t='<nonexistent>', c=NULL)succeededtenant_id was never validated despite being NOT NULL.Make it conditional: include the tenant key when the relationship must enforce tenant equality or identifiers overlap across tenants; keep ordinary FKs for global/shared rows. Where it applies, add: every column of the key MUST be NOT NULL, or the constraint MUST be MATCH FULL. Qualify the unconditional restatements at workflow-patterns.md:95 and examples/patterns.md:41.ce36b84
1698foreign-keys.md#L58-L62securityON DELETE SET DEFAULT is permitted with only the constraint that the default "match a referenced row". If it matches another tenant's row the FK is satisfied, so nothing errors — and a routine parent delete silently reassigns the child's tenant. SET DEFAULT is also described column-wise rather than as one resulting composite tuple. Validated: Cluster, reproduced end to end: parent rows for tenant A and tenant B, child in tenant A with DEFAULT pointing at tenant B. DELETE tenant A's parent → the child's tenant_id became tenant B's, no error.MUST NOT include the tenant key in the column list of ON DELETE SET NULL/SET DEFAULT; MUST NOT declare a DEFAULT on a referencing tenant key column. Validate SET DEFAULT against the complete resulting tuple, which MUST resolve within the same tenant.ce36b84
1797full-example.md#L40-L54correctnessThe flagship end-to-end example emits an FK that cannot be created, for two reasons. tenants is never defined anywhere in the skill — only two FK references to it exist — so the DDL fails on a missing relation. And the child tenant_id is converted to VARCHAR(255) (per this file's own row at :70) while tenants(id) is an unconverted MySQL INT. Validated: Cluster: VARCHAR(255)UUID FK → ERROR: foreign key constraint cannot be implemented. DETAIL: Key columns are of incompatible types: character varying and uuid.git grep 'tenants' across the skill returns only full-example.md:26 and :53.Add a Step 0 creating tenants in its own transact call, ordered before products, and make both FK sides the same type. Because :69/:100 mandate AUTO_INCREMENT → UUID/GENERATED AS IDENTITY, "just keep INTEGER" only works if tenants.id is a plain INT PK — which the example never establishes. Name the constraint.ce36b84
1897ddl-type-alternatives.md#L133-L150correctnessThree defects in the canonical copy-paste FK block. It converts orders.id to UUID but leaves customer_id INTEGER referencing customers(id). It has no tenant_id on either side, contradicting the MUST restated three lines below at :148-150. And this commit deleted the CREATE INDEX ASYNC idx_orders_customer line — which matters because InnoDB auto-creates that index for every FK and DSQL does not, so a MySQL schema migrated by this guide silently loses it. Validated: Type mechanism cluster-verified as in #17. Referencing-side index absence confirmed on cluster: after creating a composite FK, only orders_pkey existed. Index deletion confirmed in the diff.Make both sides UUID, add tenant_id to both sides (or move the block out of a multi-tenant framing), keep customer_id nullable or state the tightening, and restore the index with a note that InnoDB created it implicitly. Re-wrap in transact([...]) to match every other example in the file.ce36b84
1997dsql_lint_evals.json#L5-L15test coverageEval 100's prompt contains team_id INT REFERENCES teams(id), but it gained no FK-preservation assertion (101 and 103 did), and it still expects "explains application-layer implications" — the phrasing of the retired paradigm. Validated: Compared the input against every expectation. A linter that deletes the FK, reports the change, and explains "application-layer implications" satisfies all current assertions.Add deterministic assertions that fixed_sql retains REFERENCES teams(id) and that no FK-removal/unsupported-FK diagnostic appears. Reword the fixed_with_warning expectation to drop "application-layer".ce36b84
2096orm-guides/overview.md#L25-L69ORM"Keep add_foreign_key" / "Keep ForeignKey" covers only the CREATE TABLE path. Post-creation migrations in Rails, Django, and Hibernate emit a bare ALTER TABLE ... ADD CONSTRAINT ... FOREIGN KEY, which DSQL rejects. Only the new SQLAlchemy row at :69 mentions NOT VALID, which makes the omission an internal inconsistency. No row states a minimum adapter version, though aurora-dsql-orms#598 is merged but unreleased. Validated: Rails 8.1 docs/source checked via Context7. dsql_lint on standard Rails-generated ALTER DDL returned foreign_key_not_valid / fixed_with_warning and injected NOT VALID. No release in aurora-dsql-orms since 2026-08-25.Rails: add_foreign_key ..., validate: false then ALTER TABLE ASYNC ... VALIDATE CONSTRAINT, capture the job, verify. Add the equivalent to Django and Hibernate, and a minimum adapter version per row (the EF Core row at :31 already does this).ce36b84
2195troubleshooting.md#L67-L78operationsTwo problems. Every failed validation job is diagnosed as unmatched referencing rows, which can prompt unnecessary data modification when the cause is operational. And there are no 23503 diagnostics anywhere in the skill (grep → 0 hits) — the commit deleted the old FK error section and added only DDL-time failures, while occ-retry-patterns.md:25 says "Retryable: 40001 only" without naming 23503, so an agent may route a violation through the retry loop. Validated: Docs define failed as a general terminal state and direct users to details; cluster confirmed the details column and the four statuses. Cluster captured both real messages: insert or update on table ... violates foreign key constraint and update or delete on table ... violates ... on table ....Inspect sys.jobs.status and details first; repair rows only when details identifies a constraint violation. Add both 23503 message shapes with an orphan-locating query, marked not retryable. Restore the file's ### Error: "<literal>" convention so an agent holding an error string can match it.ce36b84
2295foreign-keys.md#L167-L176operations"Batch referenced-row changes" is offered as the remedy for cascade limit failures, but a single parent with large fan-out still exceeds the transaction row limit no matter how small the batch. Validated: Official DSQL docs warn that cascading actions count toward transaction limits and recommend exactly NO ACTION/RESTRICT for unbounded child cardinality — the doc's own prescription, omitted here.Assess per-parent fan-out. Prefer NO ACTION/RESTRICT where child cardinality is unbounded or unpredictable; delete children in bounded transactions before touching the parent.ce36b84
2395foreign-keys.md#L119-L135operationsThe async-validate example discards the transact result, so the job_id the prose tells you to poll is never captured, and no terminal states, failure branch, or timeout are given. Separately, ALTER CONSTRAINT ... INITIALLY IMMEDIATE 15 lines later performs no retroactive check, while :55 says changing to IMMEDIATE "checks outstanding changes immediately" without naming SET CONSTRAINTS — an agent will conflate the two and skip validation. Validated: Cluster: sys.jobs columns are job_id, status, details, job_type, class_id, object_id, object_name, start_time, update_time; statuses submitted/processing/completed/failed; job_type = 'VALIDATE_CONSTRAINT'. convalidated flips to t only after completion. ALTER CONSTRAINT on a NOT VALID FK left convalidated = f.Capture job_id, ship the poll as code with the four statuses, and verify pg_constraint.convalidated as the durable success signal. At :55, name SET CONSTRAINTS and state that ALTER CONSTRAINT validates nothing.ce36b84
2490foreign-keys.md#L48-L62correctnessThe subset SET NULL guidance never states its interaction with MATCH FULL. The two compose at DDL time, so the constraint is created successfully and then fails at delete time — the same deferred-failure trap as #9. Validated: Cluster: MATCH FULL ... ON DELETE SET NULL (c) was accepted at CREATE TABLE; the parent DELETE then failed with MATCH FULL does not allow mixing of null and nonnull key values.State that a column subset leaving other columns non-null is coherent only under MATCH SIMPLE; under MATCH FULL the resulting tuple must be all-null or all-non-null, so the delete will fail.ce36b84
2582foreign-keys.md#L43authoringRFC 2119 lapse on the security rule. The tenant requirement carries **MUST**only here; it is restated as a bare imperative at onboarding.md:254, full-example.md:79, ddl-type-alternatives.md:149, and workflow-patterns.md:95. The cascade and 40001 rules at :171-176 are plain bullets. Validated: Repo convention evidenced on five prior PRs (#157, #162, #168, #176, #207) — e.g. #168r3260956933: "some places we use MUST load, but here just load? Generally we try to follow RFC language."Bold **MUST** at all five restatement sites and add the keyword to the two operational rules.ce36b84
2678foreign-keys.md#L1-L4authoring180 lines of new canonical reference with zerodocs.aws.amazon.com links, and it never routes through awsknowledge/dsql_read_documentation, while asserting as fact the referenced-key requirement, RESTRICT immediacy, MATCH semantics, and the transaction-limit interaction. working-with-foreign-key-constraints.html is not linked anywhere in the skill. Validated: Sibling troubleshooting.md:92 cites, so the convention is live in-repo. Reviewer precedent: #168 finding #10, #176 finding #5, #162 finding #9 all flagged unsourced pinned facts as rot risk.Link working-with-foreign-key-constraints.html, create-table-syntax-support.html#create-table-foreign-keys, and set-constraints-syntax-support.html from the intro and the options table. Replace the undefined qualifier "supported foreign keys" (7 sites) with the actual boundary.ce36b84

Parked below the confidence threshold (60):foreign-keys.md is 180 lines with no table of contents, against the reference-file convention for files over 150 lines. Scored low because the convention is unenforced repo-wide — 14 other >150-line reference files also lack one, including constraint-operations.md at 258 lines. Worth a repo-wide pass, not a gate on this PR.

Candidates dropped after validation

CandidateDispositionValidation / reason dropped
dsql-lint < 0.2.17 strips FKs, so the skill needs a foreign_key-rule guard and a version floorDropped — out of scopeA clean resolve of awslabs.aurora-dsql-mcp-server 1.1.0 installs 0.2.17 (verified in a fresh venv), so an earlier "16 of 17 versions" framing was wrong. The residual exposure lives in server.py:617 (shutil.which('dsql-lint') PATH lookup) and :622 (pip install dsql-lint, no floor) — both in awslabs/mcp, neither touched here. Belongs upstream.
Rewrite dsql_lint_eval_results.md because it records app-layer FK enforcement as PASSDroppedThe file is an explicitly dated snapshot that names the old linter version and states historical judgments are intentionally preserved. Rewriting the results would falsify the snapshot. An added historical note is optional, not required.
The eval runner's "not use foreign key" regex branch rewards FK avoidanceDropped from this commitThe branch predates this SHA and its only matching expectation was removed here, so it is currently unreachable. Remove as separate cleanup, not as a regression introduced by this change.
"Foreign key exception" wording in the DDL-migration files implies FKs are exceptionalDroppedIn the cited locations "exception" is narrowly scoped to the table-recreation mapping, not to FK support. A rephrase may read better but did not clear the correctness gate.
The commit message's "foreign key rollout" phrasing is novelty framingDroppedNot loaded into agent context and does not change runtime guidance. The checked-in wording, evals, and coverage carry the customer impact.
CALL sys.wait_for_job(...) is invalidDroppedCluster catalog inspection shows sys.wait_for_job is a procedure (prokind='p') and CALL succeeded. CALL is correct — the defect is the surviving SELECT form, reported as #3.
Post-creation ADD FOREIGN KEY ... NOT VALID is unsupportedDroppedDocs, dsql_lint, and a live validation job confirm it is supported. The defect is that NOT VALID is framed as optional when it is mandatory, reported as #4.
Re-validating an already-valid constraint errors, so a convalidated pre-check is neededDroppedCluster: ALTER TABLE ASYNC ... VALIDATE CONSTRAINT on an already-valid constraint returned 0 rows, no error. Silent no-op; no pre-check required.
sys.jobs.job_type has no constraint-validation value, so never filter on itDroppedCluster shows job_type = 'VALIDATE_CONSTRAINT'. The documented enum is simply incomplete.
The column-granular OCC claim at :177-178 is an unsafe import from PostgreSQL's row-lock modelDroppedDSQL documents it explicitly — implicit KEY SHARE on referenced rows, a worked non-key-update no-conflict example, and a formal key/non-key definition in working-with-concurrency-control.html. The skill's claim is correct and doc-grounded.
The AWS unsupported-features page still lists FOREIGN KEY, so skill and docs disagreeDroppedThat URL 301s to the migration guide, which states "Aurora DSQL supports foreign keys, table relationships, and JOIN operations." Release notes dated 2026-08-26 confirm the launch.
MATCH FULL and the column-subset SET NULL form are mutually exclusiveDroppedCluster accepted MATCH FULL ... ON DELETE SET NULL (c) at DDL time. They compose; the failure is deferred to delete time — reported as #24.
A deferrable PK is reachable, so "non-deferrable" at :8-9 is load-bearingInvertedPRIMARY KEY (...) DEFERRABLE and UNIQUE (...) DEFERRABLE are both rejected, so the qualifier is vacuous — folded into #10 as the opposite of the original claim.
NOT DEFERRABLE INITIALLY DEFERRED shorthand is a proven defectDroppedThe shorthand is ambiguous but dsql_lint accepted it and a cluster outage prevented decisive isolated execution. Listing the three valid states explicitly remains a low-risk clarity improvement.
fix=True vs fix=true is inconsistent term usageWithdrawnAll 8 fix=True are inside Python fences; all 16 fix=true are inline prose. Python-idiomatic, not drift.
transact result shape / summary.errors truthiness is wrongNot a bugsummary.errors is an int count of unfixable diagnostics; the key paths and truthiness test are correct. The defect is the field chosen, reported as #11.
Prompt injection, credential exfiltration, endpoint or account leakage, unsafe auth examplesNo findingTwo independent security passes over all 23 changed files found none. All example UUIDs are all-zero placeholders; outbound domains are AWS docs, GitHub, and postgresql.org only.

Correction to my earlier review at d32d3bd0: it cited an eval 214. No such eval exists at ce36b84 — only 212 and 213. Disregard that item. Of the rest, #6, #8, #15, #22, #26 and part of #27 are fixed here; #9, #13, #14, #16, #25, #28, #30 and the remainder of #27 are carried forward above.

Reviewed at head SHA ce36b84696247df755c21d3a8dc187977e3f84d6. Compare against current HEAD to spot stale rows.

@anwesham-lab

Copy link
Copy Markdown
Member

PR #261 takeover update

Implemented the validated foreign-key review findings on top of PR #261 while keeping the
functional-eval runner changes in PR #262.

What changed

  • Simplified the canonical foreign-key reference from 180 to 136 lines.
  • Established the natural default: use native Aurora DSQL foreign keys for referential integrity.
  • Kept only the DSQL-specific functional differences:
    • post-creation FKs require NOT VALID;
    • validation requires ALTER TABLE ASYNC ... VALIDATE CONSTRAINT and terminal job verification;
    • FK checks perform reads and can surface retryable 40001 under DSQL OCC;
    • cascading actions count toward DSQL transaction limits.
  • Corrected tenant-scoped FK guidance while preserving ordinary FKs for shared/global parents.
  • Added a write-fenced, resumable table-recreation workflow that inventories and restores exact
    inbound/outbound FK definitions and recorded validation state.
  • Corrected MySQL migration examples, including compatible key types, explicit referencing-side
    indexes, and direct DROP CONSTRAINT mapping.
  • Corrected ORM guidance for inline versus post-creation foreign keys.
  • Added focused native-FK, shared-parent, table-swap, async-validation, 23503, and timeless
    framing eval coverage.
  • Consolidated duplicated migration guidance and restored compatibility anchors for existing
    procedure links.

Empirical corrections

  • Retested standalone unique indexes after waiting for CREATE UNIQUE INDEX ASYNC to complete.
    A valid completed unique index can be referenced by a foreign key. The earlier contrary result
    was a timing artifact and that claim was removed.
  • Confirmed the running MCP environment is:
    • awslabs.aurora-dsql-mcp-server 1.1.0
    • dsql-lint 0.2.17
  • Representative inline FK, post-creation NOT VALID, async validation, direct drop, composite
    FK, identity, JSON/JSONB, and async-index DDL passed dsql-lint 0.2.17.
  • All temporary live-cluster validation tables were removed; the final cleanup query returned 0.

Validation

  • mise run build — passed
  • Python tests — 65 passed
  • Markdown lint — 0 errors
  • dprint formatting check — passed
  • manifest, cross-reference, reference-integrity, and size checks — passed
  • Bandit, Semgrep, Gitleaks, Checkov, Grype, and advisory zizmor tasks completed
  • DSQL skill quick validator — Skill is valid!
  • git diff --check — passed
  • 20-lane DSQL authoring/security self-review completed in six-agent waves
  • Isolated four-scenario baseline-versus-plugin efficacy eval:
    • baseline — 6/20 assertions (30%)
    • plugin — 20/20 assertions (100%)
    • improvement — 14 assertions / 70 percentage points

The efficacy run used the same prompts with no database, MCP, shell, write, or web tools. The
baseline repeatedly claimed that DSQL lacked foreign-key support; the plugin consistently used
native FKs and supplied the DSQL-specific NOT VALID, async validation, table-recreation, and
tenant/shared-parent guidance. A complete multi-model corpus run remains part of the post-#262
rebase validation.

Merge order

PR #261 and PR #262 intentionally overlap in five eval files. The FK eval semantics belong in
#261; the versioned corpus schema and hardened runner belong in #262.

  1. Merge feat(databases-on-aws): add native DSQL foreign key support #261 first.
  2. Rebase fix(databases-on-aws): harden DSQL functional eval harness #262 onto updated main.
  3. Preserve feat(databases-on-aws): add native DSQL foreign key support #261's FK prompts, assertions, and removal of the obsolete application-layer FK eval.
  4. Apply fix(databases-on-aws): harden DSQL functional eval harness #262's schema_version: 2, grader, required_mcp_servers, and deterministic grader
    rules to the resulting complete corpus.
  5. Recalculate corpus counts and re-run the full build and eval suites after the rebase.

The standalone MCP skill and Kiro Power still require a separately tracked synchronization
change in awslabs/mcp.

@anwesham-lab

anwesham-lab commented Aug 31, 2026

Copy link
Copy Markdown
Member

Consolidated review matrix

This table records the areas reviewed for PR #261, the resulting finding or disposition,
confidence, how the follow-up commit addressed it, and the validation performed.

Validated findings and changes

Area reviewedFinding / dispositionConfidenceResolution in e980366 / 075f735Validation
Overall product framingForeign keys should be presented as normal native Aurora DSQL functionality, without rollout/history framing.HighReframed the skill to recommend ordinary foreign keys naturally and removed legacy replacement framing.Repository-wide phrase search found no remaining claims that DSQL does not support FKs or routes normal integrity to the application layer. Baseline/plugin eval improved from 30% to 100%.
Canonical FK referenceThe PR repeated standard behavior and made FKs appear exceptional.HighReduced foreign-keys.md from 180 to 136 lines and retained only DSQL-specific operational guidance.DSQL authoring review, Markdown lint, size validation, anchor validation, and full build passed.
Adding an FK to an existing tablePost-creation FKs require NOT VALID.HighAdded the exact ADD CONSTRAINT ... FOREIGN KEY ... NOT VALID workflow consistently.Checked against DSQL documentation, dsql-lint 0.2.17, live-cluster operations, and efficacy prompts.
Validating an existing-table FKExisting rows must be validated with ALTER TABLE ASYNC ... VALIDATE CONSTRAINT.HighCorrected synchronous/PostgreSQL-style validation examples and separated add from async validation.Retained DDL linted cleanly; generated plugin answers used the async sequence.
Async completionA returned job must be tracked, and catalog state must confirm validation.HighAdded job capture/polling, failure-detail inspection, and pg_constraint.convalidated verification.Workflow review, eval assertions, and live DSQL validation.
FK checks and OCCFK enforcement performs reads; concurrent referenced-key changes can produce retryable 40001, including when the visible operation is a write whose reference check is a read.HighCorrected the OCC explanation and retry guidance.Confirmed against DSQL guidance and David’s service-team clarification.
SQLSTATE 23503FK violations are not retryable OCC failures.HighKept explicit RFC language: MUST NOT route 23503 through the 40001 retry loop; correct the relationship or apply the intended referential action.Cross-file search and focused troubleshooting/retry review.
Referential actions and limitsCascades and other referential actions count toward DSQL transaction row/data limits.HighAdded concise operational guidance and recommended bounded/default actions unless destructive behavior is intended.DSQL documentation review and eval coverage.
Default FK behaviorSafe defaults had been removed during iteration.HighRestored the recommendation to default to NO ACTION and NOT DEFERRABLE; require explicit intent for destructive actions or deferral.Canonical-reference review and regression audit.
Composite tenant FKsTenant identity must be represented on both sides of tenant-scoped relationships.HighRequired tenant columns in referenced and referencing keys and explained nullability safely.Multi-tenant eval and DDL review.
Composite-key null semanticsThe tenant key—not every optional relationship column—must be non-null to prevent MATCH SIMPLE from bypassing tenant equality.HighRequires the tenant key to be NOT NULL on both sides; optional relationship columns may remain nullable under MATCH SIMPLE. Uses MATCH FULL only when partially populated composite keys must be rejected.Four live-cluster scenarios plus a targeted plugin efficacy prompt.
MATCH behaviorMATCH SIMPLE, MATCH FULL, and unsupported MATCH PARTIAL needed concise, DSQL-grounded treatment.HighAdded minimal guidance for the supported modes and the partial-null implications.DSQL documentation comparison and eval 213 review.
Shared/global parentsComposite tenant guidance must not be generalized to shared lookup tables.HighAdded ordinary single-column FK guidance for global parents such as countries, with authorization kept separate.Dedicated shared-parent eval passed in the plugin efficacy comparison.
Destructive action confirmationDestructive FK actions and table replacement lacked an adequate confirmation gate.HighAdded explicit confirmation, including an irreversible yes checkpoint before the swap.Workflow review and eval assertions.
Direct FK removalDropping only an FK should not trigger table recreation or MySQL DROP FOREIGN KEY syntax.HighUses ALTER TABLE ... DROP CONSTRAINT for direct removal.DDL review and migration eval 212.
Table-recreation inventoryRelationship inventory ran too late, assumed public, omitted dependent views and supporting standalone unique indexes, and could double-handle self-references.HighAdds a schema-aware pre-create gate that inventories exact inbound/outbound/self FKs, classifies self-FKs once, records standalone unique indexes that support inbound FKs, detects dependent views before the write fence, and stops for a separately approved view plan.Exact catalog queries verified against live self-FK, inbound-FK, dependent-view, and standalone-index objects; cleanup count zero.
Table-recreation write safetyConcurrent writes could enter an FK-free interval during cutover.HighHolds a write fence from final synchronization through swap and relationship restoration.Adversarial workflow review and eval 5.
Exact relationship restorationHard-coded restoration could change actions, match mode, deferrability, composite columns, schemas, original validation state, or referenced-key availability.HighRebuilds standalone unique indexes and waits for indisvalid before restoring dependent FKs; replays recorded definitions, normalizes NOT VALID once, preserves original validation state, and handles self-FKs only on the replacement side.Full live cutover showed pre-index FK restoration fail and index-first restoration plus async validation succeed.
Existing-table UNIQUEThe skill used destructive table recreation even though DSQL supports promoting a completed unique index.HighUses CREATE UNIQUE INDEX ASYNC, waits for job completion and indisvalid, then runs ADD CONSTRAINT ... UNIQUE USING INDEX.Current DSQL documentation, live cluster promotion, and targeted plugin efficacy prompt.
Direct constraint alterationsThe skill incorrectly required table recreation for dropping non-FK constraints and omitted documented post-creation CHECK support.HighUses direct DROP CONSTRAINT for CHECK, UNIQUE, and FK constraints; adds CHECK constraints with NOT VALID and validates asynchronously.Current DSQL documentation, live cluster DDL, and targeted plugin efficacy prompt.
Referenced primary-key changesDemoting a referenced PK column could make inbound FKs impossible to restore.HighRequires preflight of every inbound FK, retained PRIMARY KEY/UNIQUE coverage for preserved relationships, explicit approval for relationship removal, and abort otherwise.Focused eval and targeted plugin efficacy prompt.
Original validation statePreviously unvalidated constraints could be incorrectly forced to validated.HighRecords and restores each constraint to its original validation state.Workflow branch review and eval assertion.
DROP TABLE ... CASCADECASCADE could silently destroy relationships outside the intended operation.HighExcluded it from the relationship-preserving workflow.Negative eval assertion and cross-file review.
Table-recreation recoveryFailure handling did not distinguish pre-swap and post-swap recovery.HighAdded resumable recovery guidance for both phases and delayed write resumption until restoration completes.Failure-path review.
Preflight behaviorA proposed preflight could constrain or interfere with live writes.HighUses catalog-only compatibility checks and reserves enforcement for the controlled cutover.Operational safety review.
MySQL identifiersAn example changed related integer IDs to UUIDs without a complete remapping strategy.HighUses compatible BIGINT GENERATED BY DEFAULT AS IDENTITY CACHE 65536, preserving explicit imported IDs.Python-fence compilation, DDL lint, and migration review.
MySQL FK migrationSupported FKs were being described as replacements/removals.HighPreserves native FKs and documents only MySQL-to-DSQL syntax or operational deltas.Migration evals 202, 207, 212–214 and efficacy comparison.
MySQL indexes and table optionsReferencing-side indexes and MySQL-only syntax needed explicit conversion.HighUses separate CREATE INDEX ASYNC and omits ENGINE, partition, and other MySQL-only clauses.dsql-lint 0.2.17 and MySQL migration eval review.
ORM guidanceDjango, EF Core, Rails, and general ORM guidance still taught FK replacement or time-bound behavior.HighPreserves normal FK modeling, distinguishes inline creation from post-creation validation, and keeps guidance timeless.ORM-focused review and updated eval assertions.
Secondary examplesOnboarding, patterns, troubleshooting, and workflow references duplicated or contradicted the canonical file.HighConsolidated rules into the canonical reference and routed secondary files to it.Cross-reference and duplicate-guidance review.
Broken anchorsFK and MySQL workflow links were broken during iteration.HighRestored compatibility anchors and canonical routes.Custom anchor validator found zero changed-file failures.
Python code fencesSome unrelated snippets became invalid when fence labels changed.HighReverted incorrect relabeling and retained only valid Python fences.Every changed Python fence compiled after Markdown indentation removal.
validation_result initializationA workflow example used validation_result before assignment.HighBound the value before use.Static review and Python-fence compilation.
Constraint namingA schema-qualified constraint-name rule was incorrectly introduced.HighRemoved the rule; constraint names remain unqualified and relationship-specific.SQL review and repository search.
FK lint behaviorEval expectations still rewarded removal of supported FKs.HighUpdated lint evals to preserve REFERENCES and avoid unsupported/app-layer messaging.dsql-lint 0.2.17, JSON validation, and focused review.
Functional eval coverageThe corpus lacked key native-FK and relationship-safe recreation regressions.HighExpanded to 20 prompts / 86 assertions, including tenant optionality, dependency preflight, self-FKs, standalone-index restoration order, direct UNIQUE promotion, direct CHECK/drop-constraint changes, referenced-PK preservation, exact restoration, 23503, and timeless framing.JSON validation, semantic grading for lint eval 100, baseline/plugin comparison, and targeted plugin efficacy prompts.
EfficacyThe baseline repeatedly claimed DSQL lacked FK support.High for direction; moderate for exact scoreThe plugin consistently selected native FKs and supplied only DSQL-specific operational details.Four-scenario isolated run: baseline 6/20 (30%), plugin 20/20 (100%), +70 percentage points.
DSQL lint versionValidation needed the latest published lint behavior, including known disagreement with documented service syntax.HighTested against dsql-lint 0.2.17. The separate DSQL-LINT-FOLLOWUPS.md handoff records the required support for UNIQUE USING INDEX and CHECK NOT VALID; the skill does not teach a lint-bypass path.Version inspection, current DSQL docs, live cluster DDL, and targeted efficacy prompts.
Live-cluster cleanupValidation must not leave test objects behind.HighUsed uniquely named temporary objects and removed them after testing.Final query against the supplied DSQL cluster found zero remaining test objects.
PR #261 / #262 orderingThe PRs overlap in five eval files but do not have a semantic dependency that forces #262 first.HighDocumented #261-first merge order; #262 will rebase and migrate the complete FK corpus to v2.Branch/diff analysis identified the exact five overlapping files and required conflict resolution.
Scope controlUnrelated content must remain untouched.HighCommit changes are limited to 19 DSQL FK guidance/eval files.Commit stat and diff review: 19 files, 448 insertions, 353 deletions.
Repository verificationThe final tree needed a complete project-level check.HighNo additional code changes were needed after verification.mise run build passed; 65 Python tests passed; Markdown, dprint, manifests, cross-references, references, size checks, Bandit, Semgrep, Gitleaks, Checkov, Grype, and advisory zizmor tasks completed.

Findings dropped or narrowed after validation

Candidate findingFinal dispositionConfidenceWhy it was dropped or narrowed
A standalone unique index cannot be an FK targetDropped as incorrectHighLive DSQL testing showed that a completed standalone unique index can be referenced. The earlier failure occurred because the FK was attached before CREATE UNIQUE INDEX ASYNC reached pg_index.indisvalid = true.
Referenced columns must belong specifically to a PRIMARY KEY or UNIQUE constraintNarrowedHighThe important requirement is a completed qualifying unique target; a valid standalone unique index works.
Deferrable PK/UNIQUE limitations are a key FK differenceRemoved from the FK-differences framingHighThis is an adjacent constraint-system limitation, not one of the key functional differences in DSQL FK behavior.
Reads cannot participate in FK-related OCC conflictsDropped as incorrectHighFK enforcement performs reference reads; a referenced-row change committed after the transaction snapshot can cause 40001.
Every FK to a shared table should include tenant_idDropped as overgeneralizedHighShared/global identities use ordinary FKs. Tenant-composite keys apply when the referenced identity is tenant-scoped.
Existing-table FKs can be added and fully validated synchronouslyDropped as incorrectHighDSQL requires NOT VALID for the add and asynchronous validation with a job ID.
Application-layer FK checking remains the recommended migration patternRemoved as obsoleteHighNative FKs are supported and should be used naturally. Application authorization remains separate from referential integrity.
PR #262 must merge before PR #261Dropped as unnecessaryHigh#261 can merge first. Rebasing #262 afterward requires deliberate resolution of five corpus files but preserves a clean semantic-then-harness sequence.

@anwesham-lab
anwesham-labforce-pushed the enable-foreign-key-support branch 2 times, most recently from f36cd3d to e980366CompareAugust 31, 2026 21:53
anwesham-lab
anwesham-lab previously approved these changes Aug 31, 2026
amaksimo
amaksimo previously requested changes Aug 31, 2026

@amaksimoamaksimo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough follow-up. I found one migration-ordering issue that can break the cutover, plus two eval consistency issues. The inline comments cover the first two. One more: dsql_lint_eval_results.md still marks app-layer FK enforcement and removing the FK as PASS (lines 49, 55, and 109), although these evals now require preserving native FKs and current dsql_lint does so. Please regenerate the snapshot or label those specific results as pre-native-FK/stale, so readers are not told the old behavior is still correct.

Comment threadplugins/databases-on-aws/skills/dsql/references/ddl-migrations/overview.md Outdated
Comment threadtools/evals/databases-on-aws/dsql/dsql_lint_evals.json
@anwesham-lab
anwesham-labforce-pushed the enable-foreign-key-support branch from a6a0b30 to 075f735CompareAugust 31, 2026 22:39
@anwesham-lab

Copy link
Copy Markdown
Member

Addressed the remaining review-summary item in 075f735: dsql_lint_eval_results.md now labels the app-layer FK-removal observations in evals 100/101 as historical pre-native-FK behavior, and corrects eval 102 from four expectations to three. The current eval corpus requires native FK preservation; eval 100 now uses semantic grading.

@anwesham-lab
anwesham-lab dismissed amaksimo’s stale reviewAugust 31, 2026 22:41

Folded into 2nd commit for direct constraint

@anwesham-lab
anwesham-lab self-requested a review August 31, 2026 22:41
Comment threadtools/evals/databases-on-aws/dsql/pg_migration_evals.json Outdated
@anwesham-lab
anwesham-labforce-pushed the enable-foreign-key-support branch from 075f735 to 8922ad6CompareAugust 31, 2026 23:18
davidrz15and others added 3 commits August 31, 2026 16:21
Present foreign keys as normal native DSQL functionality and remove retired application-layer replacement framing.
Correct tenant-scoped optional relationship semantics, DSQL post-creation validation, OCC and SQLSTATE boundaries, deferral transaction scope, referential-action limits, and shared-parent modeling.
Make table recreation relationship-safe with a schema-aware pre-create FK and dependent-view gate, single handling for self-references, exact restoration, a write fence, explicit destructive confirmation, and phase-specific recovery.
Replace obsolete UNIQUE table recreation with documented async-index promotion, protect referenced keys during primary-key and AUTO_INCREMENT migrations, and correct focused MySQL, ORM, lint, and routing guidance.
Expand the functional corpus to 19 prompts and 80 assertions covering dependency preflight, self-FKs, direct UNIQUE promotion, referenced-primary-key preservation, tenant nullability, and recovery.
Document direct CHECK, UNIQUE, constraint, default, and DROP NOT NULL operations while keeping table recreation only for true structural changes.\n\nSimplify generic table recreation to a dependency guard and user-approved bespoke plan, retain SELECT FOR UPDATE for write-skew decisions with OCC retry, and use concise foreign-key-constraint terminology.\n\nRebase FK eval semantics onto the schema-v2 harness from #262, remove the obsolete application-layer FK eval, use semantic grading for lint preservation assertions, and retain separate DSQL Lint follow-up work.
@anwesham-lab
anwesham-labforce-pushed the enable-foreign-key-support branch from 8922ad6 to 97057bdCompareAugust 31, 2026 23:26
@anwesham-lab

Copy link
Copy Markdown
Member

David feedback tracker — PR #261

Reviewed SHA: 97057bd7c712d0203f3e6b0853ef00852d1a498b (rebased onto #262 / upstream/main).

FeedbackDispositionChange madeValidation
The inbound-FK swap procedure is a weird generic flowAppliedReduced table recreation to a last-resort pattern. Tables with FK or view dependencies now require a dedicated, approved migration plan; the generic guide no longer prescribes the cutover choreography.Dependency/view and standalone-index behavior tested on the live cluster.
SELECT FOR UPDATE can manage write skewAppliedORM guidance now recommends SELECT FOR UPDATE / framework locking when a write depends on rows read, while retaining whole-transaction OCC retry.Two live transactions: Tx1 selected FOR UPDATE, Tx2 updated the row, and Tx1 failed at commit with an OCC conflict.
“Native foreign keys” is unnecessary languageAppliedReplaced user-facing “native foreign keys” wording with “foreign key constraints” or “database constraints.”Cross-file terminology scan.
Do not make MATCH PARTIAL a DSQL-specific differenceAppliedKept concise MATCH SIMPLE / MATCH FULL guidance and removed the detailed MATCH PARTIAL migration eval.Current DSQL and PostgreSQL behavior treated as standard semantics.
Do not discourage deferrability like CASCADE/SET actionsAppliedDefault now recommends NO ACTION; destructive referential actions require explicit intent. Deferrability is chosen by transaction validation timing.Live SELECT FOR UPDATE / OCC validation and current DSQL documentation review.
Non-FK constraints are removable directlyAppliedDirect DROP CONSTRAINT now covers CHECK, UNIQUE, and FK constraints; table recreation is reserved for type, SET NOT NULL, and primary-key changes.Live DDL dropped a UNIQUE constraint directly.
Direct ALTER coverage is broader than old migration routesApplied where ownedDirect CHECK NOT VALID, UNIQUE promotion, default changes, and DROP NOT NULL are documented. DROP COLUMN remains owned by PR #264 to avoid overlap.Live DDL tested CHECK, UNIQUE promotion, defaults, DROP NOT NULL, SET NOT NULL rejection, and index drops.
Legacy application-layer FK eval remains from earlier guidanceRemovedRemoved the safe_query application-layer referential-integrity prompt and its README entry; FK evals now exercise database constraints.JSON schema/count validation.
DSQL Lint disagrees with service supportSeparate follow-upRecorded ADD CHECK ... NOT VALID and UNIQUE USING INDEX false positives in DSQL-LINT-FOLLOWUPS.md; the skill does not teach bypassing lint.Current dsql-lint 0.2.17 compared with live DDL and DSQL docs.

@anwesham-lab
anwesham-lab added this pull request to the merge queueSep 1, 2026
Merged via the queue into awslabs:main with commit 8d867f0Sep 1, 2026
24 checks passed
@davidrz15
davidrz15 deleted the enable-foreign-key-support branch September 1, 2026 20:45
pullBot pushed a commit to bhardwajRahul/Awslap-mcp that referenced this pull request Sep 1, 2026
…s#4565)
Part of the foreign key release.
Bring the Aurora DSQL MCP server's agent steering in line with native DSQL
foreign key support, mirroring the canonical guidance from the
databases-on-aws plugin:
awslabs/agent-plugins#261
- Kiro Power tree (kiro_power/): POWER.md updated for the FK reframe
(Workflows 1/3/6, steering bullet, error scenarios); 21 steering/*.md
files brought byte-identical to the agent-plugins tip references —
native FK usage (NOT VALID + ALTER TABLE ASYNC ... VALIDATE CONSTRAINT),
direct constraint operations, table-recreation narrowing, and the
associated MySQL/PostgreSQL/ORM/troubleshooting guidance.
- Packaging: CHANGELOG.md, pyproject.toml, uv.lock.
Note: the standalone dsql-skill was deprecated upstream (awslabs#4562); its
deprecation stubs are retained and the FK content lives canonically in
agent-plugins and the Kiro Power tree.
praba2210 pushed a commit that referenced this pull request Sep 2, 2026
Aurora DSQL shipped @aws/aurora-dsql-drizzle, which the skill had no
coverage of. Adds a Drizzle gotchas section to the ORM guide at EF Core
depth, registers the adapter in language.md, connectivity-tools.md,
dsql-lint.md, and the SKILL.md triggers and workflow.
Adds two functional evals and three trigger cases. Version goes to
1.9.0 since #261 already claims 1.8.0.
praba2210 pushed a commit that referenced this pull request Sep 2, 2026
Aurora DSQL shipped @aws/aurora-dsql-drizzle, which the skill had no
coverage of. Adds a Drizzle gotchas section to the ORM guide at EF Core
depth, registers the adapter in language.md, connectivity-tools.md,
dsql-lint.md, and the SKILL.md triggers and workflow.
Adds two functional evals and three trigger cases. Version goes to
1.9.0 since #261 already claims 1.8.0.
praba2210 pushed a commit that referenced this pull request Sep 2, 2026
Aurora DSQL shipped @aws/aurora-dsql-drizzle, which the skill had no
coverage of. Adds a Drizzle gotchas section to the ORM guide at EF Core
depth, registers the adapter in language.md, connectivity-tools.md,
dsql-lint.md, and the SKILL.md triggers and workflow.
Adds two functional evals and three trigger cases. Version goes to
1.9.0 since #261 already claims 1.8.0.
praba2210 pushed a commit that referenced this pull request Sep 3, 2026
Aurora DSQL shipped @aws/aurora-dsql-drizzle, which the skill had no
coverage of. Adds a Drizzle gotchas section to the ORM guide at EF Core
depth, registers the adapter in language.md, connectivity-tools.md,
dsql-lint.md, and the SKILL.md triggers and workflow.
Adds two functional evals and three trigger cases. Version goes to
1.9.0 since #261 already claims 1.8.0.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@davidrz15@anwesham-lab@krokoko@amaksimo