feat(gcp-to-aws): extraction-consistency gates + canonical disk_size_gb schema - #181
feat(gcp-to-aws): extraction-consistency gates + canonical disk_size_gb schema#181herosjourney wants to merge 5 commits into
Conversation
…b, generate cross-check - Clarify handoff gate: fail closed when Q13b/Q19 were asked despite an unambiguous inventory answer (exempt full flow and user-converted rows) - Discover: normalize Cloud SQL config to canonical disk_size_gb / availability_type / tier / database_version; document in schema-discover-iac.md; fix gcp_config vs config naming in redaction rule - Clarify/clarify-database: reference the canonical path, keep legacy fallbacks for older inventories - Generate: cross-check db_size against inventory disk size before pg_dump/pgcopydb/DMS selection; warn and prefer Terraform on mismatch - SKILL.md: add 'Extract before ask' to Philosophy
|
Context: how this relates to the merged extraction lineage (#15 → #57 → #30 → #127 → #174) A reasonable question is what this adds given that auto-extraction already exists. Short answer: those PRs tell the agent what to do; this one makes Clarify fail closed when the agent didn't do it. Evidence this is needed: a real migration run on 2026-07-23 — with #57, #30, #127, and #174 all merged — still ran ask-everything mode (20 questions in What each merged PR does vs. what this PR adds:
Same fail-closed pattern as existing gate checks 1–6; purely additive. |
…actual data size - Q13b auto-detect (clarify.md, clarify-database.md): label extracted disk size as allocated capacity / upper bound; sheet row shows 'up to N GB allocated'; accept downward user corrections without pushback - Generate cross-check: asymmetric — allocated band only overrides a LARGER user answer (data cannot exceed allocation); smaller user answers stand - 02-migrate-data.sh: always measure actual data size via pg_database_size before migrating; pg_dump script hard-stops when actual >= 10 GB (undersized tooling is the dangerous direction), pgcopydb script advises regeneration when actual < 10 GB
set -euo pipefail would kill the script at the psql call on a fresh dry run where SOURCE_HOST/SOURCE_DB_PASSWORD are still TODO — skip the measurement with a notice instead, and null-guard the tool advisory branches.
leon1418
left a comment
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖]
Scope reviewed: All 6 changed files in full, PR description, author context comment, mainline versions of all modified files, sibling PRs #180/#182–#186 for merge-order interactions, and schema/script placement correctness.
Summary: This PR closes the gap between existing extraction-first instructions and a fail-closed gate that enforces them. The design is sound: gate checks 7–8 follow the same additive pattern as 1–6, the allocated-vs-actual semantic correction is well-reasoned with safe asymmetric override logic, and the runtime measurement block in generate-artifacts-scripts.md correctly degrades when connection vars are unset. The canonical config.disk_size_gb schema pin (with legacy fallback) eliminates an observed triple-path defensive-read pattern.
No blocking findings. Two observations below (Nit, FYI).
What's good:
- The fail-closed →
GATE_FAILmodel (same as 1–6) avoids the common antipattern of advisory-only checks that degrade silently. - Asymmetric cross-check at Generate (allocated only overrides larger user answer, never smaller) respects the semantics correctly.
- The
pg_dumphard-stop at ≥10 GB (dangerous direction) vspgcopydbsoft-advisory at <10 GB (merely inconvenient) is the right safety asymmetry. - Legacy-path fallback acceptance in both Clarify and the schema prevents older inventories from breaking.
Interactions with open sibling PRs:
- #182 (resume/recovery): touches
clarify.mdStep 0 (resume logic) andclarify-global.md— no overlap with #181's gate-check or Step 2 changes. Clean parallel merge. - #183 (journey orientation): touches
clarify.mdbut in the Step 0/intro area, not the gate section. No conflict. - #184 (last-mile generate): touches
generate-artifacts-scripts.mdbut only adds the fail-fast fill-in pattern in the shared conventions section (line ~62). #181's additions are at line ~83 (the tool-selection section). Adjacent but non-overlapping — should merge cleanly in either order. - #185 (decision gate): touches
SKILL.mdbut in the state-machine table (line ~81+), not the Philosophy bullet (line ~10). No conflict with #181's one-liner. - #180 (heroku parity): entirely in
heroku-to-aws/skill — zero file overlap.
Recommended merge order: #181 is independent and can land in any position relative to siblings. No dependency in either direction.
|
|
||
| - **`db_size.value` band is LARGER than the allocated band** — data cannot exceed its allocation (barring `disk_autoresize` growth since the Terraform was written). Warn and use the allocated band: | ||
|
|
||
| > **Database size inconsistency:** Clarify recorded `db_size: [value]` but Terraform only allocates `disk_size_gb: [N]` ([band]). Using the allocated band for tool selection — say "use my answer instead" if the disk has grown since this Terraform was written. |
There was a problem hiding this comment.
[🤖 AI review 🤖]
Nit: The parenthetical "(barring disk_autoresize growth since the Terraform was written)" is correct but introduces a concept the reader has no tooling to resolve — Discover doesn't currently extract disk_autoresize into the inventory, so there's no way for the agent to know whether autoresize is enabled. The caveat is fine as documentation (explains why the user might legitimately push back), but if disk_autoresize is commonly set, a future follow-up could extract it and suppress the warning when autoresize=true AND no user band has been recorded.
No action needed — just flagging the implied extension point.
There was a problem hiding this comment.
Took the extension point now rather than leaving the caveat tooling-less — it was small since this PR already pins the canonical config keys (a36e6c6):
- Discover extracts
config.disk_autoresizeforgoogle_sql_database_instance, but ONLY when the attribute is explicitly present in HCL. The provider default istrue, but an omitted attribute isn't an authored choice, so absence stays neutral rather than synthesizing the default. - This cross-check now branches on it: explicit
true→ allocation is a floor, not a cap; keep the user's band, no warning, note it in the script header. Explicitfalseor absent → existing warn-and-use-allocated behavior, with the warning text now mentioning autoresize-outside-Terraform as the push-back reason.
That keeps the fail-closed default (absent = warn) while suppressing the false-positive exactly where the Terraform proves growth is possible.
| 7. **Extraction consistency — Q13b** _(skip this check when `metadata.clarify_mode` is `"full"` — the user explicitly opted into the full question flow)_: If every `google_sql_database_instance` in `gcp-resource-inventory.json` carries an unambiguous disk size (`config.disk_size_gb` or legacy variants, all mapping to the same Q13b band) → `Q13b` must NOT appear in `metadata.questions_asked` unless `design_constraints.db_size.chosen_by` is `"user"` (the user converted the sheet row via "ask me about X" or corrected it). Otherwise: `GATE_FAIL | phase=clarify | field=metadata.questions_asked | reason=extractable_question_asked_Q13b`. | ||
| 8. **Extraction consistency — Q19** _(skip when `metadata.clarify_mode` is `"full"`)_: If `ai-workload-profile.json` exists with `models[0].model_id` set at confidence ≥ 0.8 → `Q19` must NOT appear in `metadata.questions_asked` unless `ai_constraints.ai_model_baseline.chosen_by` is `"user"`. Otherwise: `GATE_FAIL | phase=clarify | field=metadata.questions_asked | reason=extractable_question_asked_Q19`. (Detect-confirm cards for sub-threshold or tied detection per `clarify-ai.md` are not affected — they fire only when confidence < 0.8.) | ||
|
|
||
| **On any FAIL:** Emit `GATE_FAIL | phase=clarify | field=<path> | reason=missing`. **Do NOT modify artifacts to pass the gate.** **Do NOT update `.phase-status.json`.** Tell the user to answer the missing question or re-run Clarify. |
There was a problem hiding this comment.
[🤖 AI review 🤖]
FYI: Gate check 7 inspects config.disk_size_gb "or legacy variants, all mapping to the same Q13b band" — but doesn't specify what happens when multiple google_sql_database_instance resources exist with different bands (e.g., one 8 GB dev instance + one 200 GB prod instance). The clarify-database.md auto-detect table handles this ("If multiple instances disagree, ask Q13b"), but this gate check's wording — "every … carries an unambiguous disk size … all mapping to the same Q13b band" — correctly implies the check fires only when they agree. Just noting this is implicit rather than explicit; the existing wording is technically sufficient.
There was a problem hiding this comment.
Made it explicit in a36e6c6 — gate check 7 now states outright that when instances map to different bands the check does not fire and asking Q13b is the expected behavior, quoting the clarify-database.md auto-detect rule so the two stay visibly in sync.
… explicit Addresses both review observations on PR awslabs#181: - Nit (disk_autoresize had no tooling): Discover now extracts config.disk_autoresize for google_sql_database_instance, ONLY when explicitly set in HCL (provider default is true, but an omitted attribute is not an authored choice). Generate's allocated-vs-actual cross-check branches on it: explicit true keeps the user's answer with no warning (allocation is a floor, not a cap); explicit false or absent keeps the existing warn-and-use-allocated behavior. - FYI (gate check 7 multi-instance wording): the disagreeing-bands case is now explicit — check does not fire, asking Q13b is expected, matching the clarify-database.md auto-detect rule.
2 participants
Summary
The problem, in plain English: A real run of Clarify asked all 20 questions — including database size (Q13b) and current AI model (Q19) — even though the user's Terraform said
disk_size = 10and app-code discovery had already identified the model. The wrong Q13b answer then propagated into the data-migration script (pg_dump vs pgcopydb selection). The wizard spec already says extract-first, but nothing fails when an agent asks a question the inventory had already answered, and nothing downstream cross-checks the answer against the Terraform ground truth. On top of that, the ground truth itself was being misread: Terraformdisk_sizeis allocated capacity (Cloud SQL minimum: 10 GB), not actual data — so even perfect extraction routes a nearly-empty dev database to pgcopydb when pg_dump would finish in seconds.Changes:
clarify.md, checks 7–8): the fail-closed Clarify handoff gate now emitsGATE_FAIL | reason=extractable_question_asked_Q13bwhen Q13b appears inmetadata.questions_askeddespite an unambiguous inventory disk size, and the same for Q19 whenmodels[0].model_idwas detected at confidence ≥ 0.8. Both checks exemptclarify_mode: "full"(user explicitly opted into the long path) andchosen_by: "user"(sheet-row conversion via "ask me about X" or a correction). The feat(gcp-to-aws): AI clarify detect-confirm cards + AI-only mini assumption sheet #174 detect-confirm cards are unaffected — they only fire below the confidence bar.discover-iac.md,schema-discover-iac.md): Discover now normalizesgoogle_sql_database_instancetoconfig.disk_size_gb(from Terraformsettings.disk_size),config.availability_type,config.tier, andconfig.database_version, and the schema documents them with an example — previously the schema never mentioned these fields and three path variants circulated (config.disk_size,config.disk_size_gb,gcp_config.disk_size_gb). Clarify andclarify-database.mdnow name the canonical path and keep the legacy variants as accepted fallbacks for older inventories. Also fixes thegcp_configvsconfignaming inconsistency in the Step 0 redaction rule.clarify.md,clarify-database.md,generate-artifacts-scripts.md): extracted disk size is now labeled as an upper bound everywhere — the Assumption Sheet row reads "up to N GB allocated (actual data may be less)", and downward user corrections are accepted without pushback (data below allocation is the normal case). The extracted band remains safe for tool selection because it never underestimates.generate-artifacts-scripts.md): before selecting pg_dump / pgcopydb / DMS for02-migrate-data.sh, comparedesign_constraints.db_size.valueagainst the inventory band — asymmetrically: the allocated band only overrides a larger user answer (data can't exceed its allocation); a smaller user answer stands. And because neither value measures actual data, the generated script now always measures it (pg_database_size) before migrating: the pg_dump variant hard-stops when actual ≥ 10 GB (undersized tooling is the dangerous direction), the pgcopydb variant advises regenerating when actual < 10 GB (oversized is merely inconvenient).SKILL.mdPhilosophy: one-line "Extract before ask" principle at the top, so the posture isn't only discoverable inside clarify.md Step 2.Why
Post-mortem of a real migration run (SF beach stack: Cloud Run + Cloud SQL + Gemini). The agent fell into ask-everything mode; the spec's extract-first instructions existed but were advisory at the boundary that matters. This PR makes the two highest-signal extractable questions machine-checkable at the handoff gate (same fail-closed pattern as checks 1–6), pins down the schema ambiguity that made defensive triple-path reads necessary, corrects the allocated-vs-actual semantics that would produce wrong tooling even under perfect extraction, and adds one defense-in-depth check where the bad answer did concrete damage (migration tooling selection).
Test plan
mise run build— green (lint, fmt:check, gitleaks, checkov, grype)mise run lint:md— 0 errors across 804 filesmise run lint:frontmatter— OKconfig.disk_sizeandgcp_config.disk_size_gbas fallbacksdisk_size = 10and confirm Q13b lands on the Assumption Sheet as "up to 10 GB allocated", and that a preferences.json with Q13b inquestions_askedfails the gateOut of scope
clarify-extractions.jsonmanifest artifact — the Assumption Sheet + per-constraintsourceprovenance +preferences-draft.jsonalready cover "show your work"; a parallel artifact would add drift surfacepg_database_sizeis PostgreSQL; the MySQL path keeps band-based selection)Type of Change
Team Folder
migrate/