claude@clouddev1
f8a91f41c9
- F2-broad: replay failures now render with real schema context instead of
a contextless friendly_message(). Extract App::build_translate_context into
the shared App::translate_context_for(command, facts, verbosity); run_replay
enriches via enrich_dsl_failure + that builder. ctx_* fallbacks degrade to
neutral prose so the rare non-replay contextless callsites can't leak raw
{name} either. (SQL INSERT/UPDATE values aren't retained — ADR-0033 verbatim
— so those show real table/column + neutral "that value".)
- Gap C: SQL ALTER … ADD FOREIGN KEY on a missing child column refuses with an
SQL-appropriate "add it first", not the DSL-only --create-fk flag.
- Gap B: dropping a single-column-UNIQUE column refuses with a pointer to
`drop constraint unique from T.col` (was an opaque generic refusal).
- Gap D: 4e drop/rename CHECK-guard + 4f change-type FK-guard refusals reworded
to explain why; static_refusal reasons left as-is.
Tests: +4, 3 strengthened. 1926 pass / 0 fail / 0 skip; clippy clean.
188 lines
10 KiB
Markdown
188 lines
10 KiB
Markdown
# Plan — ADR-0035 Phase-4 `/runda` follow-ups F1 / F2 / F3 (2026-05-26)
|
||
|
||
Bundle of the three error-message / capability follow-ups surfaced by the
|
||
whole-Phase-4 `/runda` (handoff-42 §3). All three live on the **safe**
|
||
composite-UNIQUE edge (dropping a UNIQUE-covered column is correctly
|
||
*refused* today — no corruption); the work improves messaging and adds a
|
||
way to drop the constraint itself.
|
||
|
||
## Phase 1 — requirements
|
||
|
||
- **F1 — friendly refusal for dropping a composite-UNIQUE column.**
|
||
`do_drop_column`'s covering-index guard reads `read_table_indexes`,
|
||
which filters to `origin='c'` (explicit `CREATE INDEX`) and excludes
|
||
the UNIQUE-constraint auto-index (`origin='u'`). So `drop column c`
|
||
when `unique (b, c)` spans `c` skips the guard, reaches the engine, and
|
||
is refused with an unhelpful generic message. Add an up-front guard
|
||
detecting the column in `schema.unique_constraints` (composite only —
|
||
`read_unique_constraints` routes single-column UNIQUEs to the column
|
||
flag, multi-column to `unique_constraints`), refusing with the
|
||
constraint's **derived name** (F3) + the drop command. Behaviour stays
|
||
"refused"; only the message improves. **Message-only — no `--cascade`
|
||
extension** (the SQL drop-column has no `--cascade` spelling; dropping
|
||
a constraint via cascade is a larger semantic change, out of scope
|
||
unless the user asks).
|
||
|
||
- **F2 — literal `{table}` leak in contextless `friendly_message()`.**
|
||
`Verbosity` defaults to `Verbose`, so `friendly_message()` (which uses
|
||
`TranslateContext::default()`, no table) renders the generic hint
|
||
`"…current state of `{table}`."` with the literal placeholder via
|
||
`ctx_table()`'s `"{table}"` fallback. Hits every contextless
|
||
`friendly_message()` callsite whose error lands in the **generic
|
||
bucket**: replay, undo, rebuild-from-text, export. Fix: a tableless
|
||
generic-hint variant selected when `ctx.table` is `None`. **Broader
|
||
finding** (DA): the same `{name}`-marker fallbacks leak in *other*
|
||
templates (e.g. a replayed UNIQUE violation → `error.unique.*`) when
|
||
reached contextless. The documented F2 is the generic case; the broader
|
||
leak is surfaced for the user to scope, not silently expanded/narrowed.
|
||
|
||
- **F3 — a way to drop an anonymous composite UNIQUE (user-raised).**
|
||
By design (§4a.2/§4g) a composite `UNIQUE(a,b)` is anonymous —
|
||
PRAGMA-detected, a bare column-list, no name — so `DROP CONSTRAINT
|
||
<name>` can't target it and recreating the table is the only escape.
|
||
Add a way to drop it. **(Amends ADR-0035 — see Amendment 1.)**
|
||
|
||
Baseline: `cargo test` → 1917 pass / 0 fail / 0 skip / 1 ignored doctest;
|
||
`cargo clippy --all-targets -- -D warnings` clean.
|
||
|
||
## Phase 2/3 — F3 design (the genuine fork; user-decided)
|
||
|
||
Composite UNIQUE has no name. Options considered:
|
||
|
||
- **A — name composite UNIQUEs (user-supplied):** reverse the §4g
|
||
anonymity decision; needs a new `__rdbms_*` table + YAML round-trip +
|
||
rebuild-arrival migration (the cost §4a.3 deliberately avoided). Most
|
||
SQL-standard, largest.
|
||
- **B — positional drop by column-list** (`drop … unique (cols)`):
|
||
preserves anonymity, no metadata, but needs a *new* grammar form.
|
||
- **C — auto-assigned, engine-neutral *derived* name (chosen).** The
|
||
name is a deterministic function of the columns (`unique_<cols>`),
|
||
recomputed live wherever shown or matched. Storage stays a bare
|
||
column-list (anonymity preserved); the name is purely a
|
||
presentation/addressing label. **Reuses the existing `DROP CONSTRAINT
|
||
<name>` grammar — no new syntax at all.** Zero metadata, zero
|
||
migration, round-trips for free. Tracks column renames.
|
||
|
||
**User decisions (2026-05-26):** approach **C / derived (no storage)**;
|
||
name format **`unique_<cols>`**; doc vehicle **amend ADR-0035**; scope
|
||
**advanced-SQL only** (matching the 4g `ADD` form — no simple-mode verb).
|
||
|
||
### DA critique (written down)
|
||
|
||
1. **Ambiguous derived names** (e.g. a column literally named `b_c` vs
|
||
`UNIQUE (b, c)`): drop-by-name must **detect ambiguity and refuse**,
|
||
never guess. *In scope.*
|
||
2. **Collision with a user-named CHECK/FK** of the same string: the
|
||
`do_drop_constraint_by_name` order is CHECK → FK → UNIQUE, so a
|
||
CHECK/FK shadows a derived UNIQUE name. Acceptable given the
|
||
distinctive `unique_` prefix; **document the order**.
|
||
3. **F1 `--cascade`**: not extended to drop a covering UNIQUE
|
||
(constraint, not index). Refuse-only. *Flagged.*
|
||
4. **F2 breadth**: the leak is broader than `error.generic.hint`. Fix the
|
||
documented generic case; **surface** the broader leak. *Flagged.*
|
||
5. **Single-column UNIQUE column drop**: a parallel gap (a single-column
|
||
UNIQUE column drop also reaches the engine with a poor message) exists
|
||
but is **outside the documented F1 scope** (different mechanism —
|
||
ADR-0029 column-level `drop constraint`). Noted, not fixed here.
|
||
|
||
## Phase 4 — execution (order: F3 → F1 → F2)
|
||
|
||
1. **F3.** `unique_constraint_name(cols) -> "unique_<cols>"` helper
|
||
(`db.rs`, `pub(crate)`). Extend `do_drop_constraint_by_name` with a
|
||
third step: match each composite UNIQUE's derived name; >1 match →
|
||
refuse (ambiguous); 1 match → `rebuild_table` with that entry removed
|
||
from `unique_constraints` (mirrors `do_alter_add_unique` in reverse) +
|
||
`do_describe_table`. Annotate the describe "Table constraints:"
|
||
section: `unique_b_c: UNIQUE (b, c)`.
|
||
2. **F1.** Up-front guard in `do_drop_column` after the index-covering
|
||
guard: column in any `schema.unique_constraints` entry → refuse with
|
||
the derived name + `alter table T drop constraint <name>`.
|
||
3. **F2.** `error.generic.hint_no_table` catalog entry; in
|
||
`translate_generic`, pick it when `ctx.table` is `None`.
|
||
|
||
Test-first for each (reproduce → fail → fix → pass), across the worker
|
||
API (Tier-1/3) and the friendly-layer unit tests + insta snapshots.
|
||
|
||
## Phase 5 — verification
|
||
|
||
Full `cargo test` + clippy; compare to baseline; every checklist item
|
||
addressed; engine-neutral vocab held (no SQLite/STRICT/PRAGMA in new
|
||
user-facing strings); ADR + README + this plan lockstep.
|
||
|
||
**Shipped 2026-05-26** as commit `cb8ff8a` — 1922 pass / 0 fail / 0 skip,
|
||
clippy clean.
|
||
|
||
## Follow-up (2026-05-26, user-approved) — broad F2 + message gaps B/C/D
|
||
|
||
After the F1–F3 commit the user asked to take the broader F2 leak plus the
|
||
remaining message gaps. Scope (user-decided):
|
||
|
||
- **F2-broad — enrich replay + neutral-prose safety net.** The constraint
|
||
templates (`error.unique.*`, `error.foreign_key.*`, `error.check.*`)
|
||
carry `{table}`/`{column}`/`{value}` in the **headline**, so they leak
|
||
whenever rendered via contextless `friendly_message()`. The realistic
|
||
surface is **replay of a constraint-violating scripted command**
|
||
(`run_replay`'s failure branch, `runtime.rs`, calls bare
|
||
`e.friendly_message()`). Fix: (a) replay reuses `enrich_dsl_failure` +
|
||
the operation-from-`Command` mapping so a replayed failure shows the
|
||
**real** table/column/value (best UX); (b) the `ctx_*` fallback markers
|
||
become neutral prose (`{table}` → "the table", etc.) so the rare
|
||
non-replay contextless callsites (undo/rebuild/export) can't leak raw
|
||
`{name}` either. Requires extracting `App::build_translate_context` into
|
||
a `pub(crate)` free fn (parameterised by verbosity) so replay and the
|
||
App share one Command→context mapping.
|
||
- **Gap C — `--create-fk` leak.** SQL `ALTER … ADD FOREIGN KEY` on a
|
||
missing child column reuses `do_add_relationship`'s DSL-flavoured error
|
||
suggesting `--create-fk` (a DSL flag, meaningless in SQL). Fix:
|
||
`do_alter_add_foreign_key` pre-validates the child column and emits an
|
||
SQL-appropriate "add it first" refusal with no flag mention.
|
||
- **Gap B — single-column UNIQUE column drop.** Parallel to F1 but a
|
||
different mechanism: a single-column UNIQUE rides on the column `unique`
|
||
flag (ADR-0029), not `unique_constraints`. Characterise current
|
||
behaviour with a test, then add a friendly, actionable refusal pointing
|
||
at the column-level `drop constraint unique from T.col`.
|
||
- **Gap D — terse CHECK-guard / type-conversion wording.** Polish the 4e
|
||
drop/rename-column CHECK-guard refusals and the 4f type-conversion
|
||
diagnostics for clarity, staying engine-neutral. Conservative — wording
|
||
only, no behaviour change.
|
||
|
||
### DA critique (follow-up)
|
||
|
||
1. **Refactor risk.** Extracting `build_translate_context` from `App` is a
|
||
pure move + signature change (add `verbosity`); the App method becomes
|
||
a thin delegator. Covered by the existing app tests + a new replay
|
||
render test.
|
||
2. **`ctx_*` neutral prose looks odd backtick-wrapped** (`` `the table` ``)
|
||
— accepted by the user as a last-resort safety net; it renders only in
|
||
the near-impossible non-replay constraint case (replay is enriched).
|
||
3. **Gap B may be a non-issue** if the engine drops a single-column-UNIQUE
|
||
column cleanly — characterise first, only guard if it refuses.
|
||
4. **No marker pinned anywhere.** No test/snapshot asserts a literal
|
||
`{table}`/`{column}` as expected output, so changing the fallbacks is
|
||
low-risk (verified by grep).
|
||
|
||
### Outcome (implemented 2026-05-26)
|
||
|
||
- **F2-broad** — `App::build_translate_context` extracted to the shared
|
||
`App::translate_context_for(command, facts, verbosity)`; `run_replay`'s
|
||
failure branch now enriches via `enrich_dsl_failure` + that builder, so a
|
||
replayed failure shows the real table/column (and value/parent/rule
|
||
where resolvable). `ctx_*` fallbacks are neutral prose. **Discovered
|
||
limitation:** replay parses in advanced mode → SQL `INSERT`/`UPDATE`,
|
||
whose values are raw SQL text (ADR-0033 verbatim), not retained — so the
|
||
offending *value* degrades to "that value" (no leak), while table/column
|
||
are real. DSL `insert`/`update` still show the value. (Same gap exists
|
||
on the interactive SQL-DML path; the safety net covers it.)
|
||
- **Gap C** — `do_alter_add_foreign_key` pre-validates the child column
|
||
and emits an SQL-appropriate "add it first" refusal (no `--create-fk`).
|
||
- **Gap B** — `do_drop_column` guards a single-column UNIQUE
|
||
(`col_info.unique`) with a refusal pointing at `drop constraint unique
|
||
from T.col`.
|
||
- **Gap D** — polished the 4e drop/rename CHECK-guard refusals and the 4f
|
||
change-type FK guard to explain *why*; left `static_refusal` reasons
|
||
as-is (already clear — avoided gratuitous churn).
|
||
|
||
Tests +4 (replay no-leak, safety-net unit, FK-missing-column, single-col
|
||
UNIQUE drop) + 3 strengthened (2× CHECK-guard wording, 1× change-type FK
|
||
wording). **1926 pass / 0 fail / 0 skip**, clippy clean.
|