fix(gcp-to-aws): read compliance from design_constraints, not a top-level key

[herosjourney]

Problem

When a founder migrating from GCP says they have a compliance requirement (SOC 2, PCI, HIPAA, or FedRAMP), the tool is supposed to add the matching security controls — AWS Config and Security Hub — to their generated infrastructure, and include their cost in the estimate. Today it silently doesn't. The founder answers the compliance question, but the generated baseline.tf comes out without those controls and the estimate omits their cost, with no warning. So someone who asked for a compliance-ready setup quietly gets one that isn't.

The cause is a plumbing mismatch: the step that records the founder's answer saves it in one place, and the steps that generate the Terraform and the estimate look for it somewhere else — so they always see "no compliance requirement" and skip the controls. This is the underlying bug behind the README claim that PR #311 had to walk back (the README promised GCP compliance controls that never actually shipped).

In code, for reviewers

Clarify writes the declared frameworks to preferences.json → design_constraints.compliance.value. But generate-artifacts-infra.md (Step 1.5 retention + the compliance-conditional baseline.tf section) and estimate-infra.md (the security_baseline_compliance cost line) read a top-level preferences.json.compliance key that Clarify never writes. A normal declared-framework answer therefore lands in the empty-compliance branch: the Config/Security Hub section is not emitted and the compliance cost line is dropped.

Solution

Align the two readers to design_constraints.compliance.value — the canonical shape the other five GCP compliance readers already use (design-ai.md, design-billing.md, migration-complexity.md, generate-artifacts-ai.md, networking.md):

• generate-artifacts-infra.md: a new Step 0 normalizes design_constraints.compliance.value (absent / none / unknown → []); the baseline.tf descriptions reference the same shape; every downstream "compliance contains …" resolves against the normalized array.
• estimate-infra.md: the security_baseline_compliance gate reads the same location.

Blast radius verified: I traced every GCP compliance reader — only these two used the top-level shape; all others already read design_constraints.compliance. So this is a consistency fix bringing two outliers in line with the established pattern, not a new mechanism. heroku-to-aws is unaffected (it reads preferences.json.global.compliance, which its Clarify writes).

Validation

• Verified against real data shape: no fixture's preferences.json carries a top-level compliance key; compliance lives under design_constraints — which is why the bug never tripped a golden (no GCP fixture declares a framework).
• This is prose-interpreted generator/estimator logic; there is no executable asserter over the compliance→controls emission path. A GCP compliance golden fixture would be a separate, larger addition — noted as a coverage gap, not built here to avoid scope creep.
• Byte-identical across migrate and advisor trees. Green locally: drift:check, fmt:check, lint:md, lint:frontmatter, lint:types, fixtures:assert (22 ok), security:bandit.

Follow-up: with GCP controls now firing, migrate/README.md's GCP Config/Security Hub assurance — qualified out in #311 — can be restored.

Type of Change

• New plugin/power/tool
• Bug fix
• Enhancement to existing content
• Documentation update
• Guardrail/CI update

Team Folder

• advisor/
• migrate/
• solution-architecture/
• Other: ___

Checklist

• I have read the CONTRIBUTING.md guidelines
• My changes do not include hardcoded secrets, credentials, or internal-only content
• I have run mise run build locally and it passes
• I have updated documentation if needed
• My changes are scoped to my team's folder only

Both plugin folders are required to keep the distributed skill copies aligned.

[ayn-builds]

Review: align GCP compliance reads to design_constraints

Diagnosis is right and the fix is correct. Verified independently:

• schema-preferences.md mandates the {value, chosen_by, prompt, design_consequence} wrapper for every design_constraints key, and Clarify writes design_constraints.compliance (full-flow Q2 in clarify-global.md:73-84, AI-only Q1.5 in clarify-ai-only.md:141/:359). Nothing writes a top-level compliance.
• Post-fix grep over the repo: zero remaining top-level reads. The only hits are the new "do NOT read" warnings.
• .value is the correct leaf, and generate-artifacts-report.md:283 already uses compliance.value, so the new text is stricter than the sibling readers cited in the description (those use the looser design_constraints.compliance shorthand), not divergent from them.
• The new Step 0 tracks heroku-to-aws/.../generate-terraform.md:145 almost word for word, and its none/unknown/absent semantics match design-ai.md:81 and Clarify's ["unknown"] contract.
• Both trees byte-identical for both files. Heroku unaffected (preferences.json.global.compliance, scalar-or-array). Fixture claim holds: only heroku fixtures carry compliance (global.compliance: "none"), no GCP fixture declares a framework, so no golden churn.

Worth strengthening in the Problem section

The miss was visible in the output, not only silent. generate-artifacts-report.md:283 fires the Section 4 compliance-controls note, and Appendix G renders "Compliance-conditional (only when SOC 2/PCI/HIPAA/FedRAMP declared in preferences)" listing AWS Config and Security Hub + FSBP with monthly costs (:312-316). Both are gated on the declared preference, which reads the correct shape. So pre-fix a SOC 2 run shipped a report documenting Config and Security Hub as part of the baseline while baseline.tf contained neither and the estimate omitted the cost line. That is a documented-vs-generated contradiction, a stronger statement of impact than "silently does not."

On the coverage gap

Agreed a GCP compliance golden is out of scope here. The cheap durable guard is a grep invariant in mise.toml next to drift:check that fails on any top-level preferences.json.compliance read (excluding the "do NOT read" warning line), in the same spirit as the reviewer-workflow-invariants check. Follow-up alongside the migrate/README.md restore.

Three inline notes below, all nits - nothing blocking.

[ayn-builds]

Step 3.0's caller context at line 297 is the one read in this file still pointing at the raw location:

• compliance — the value of preferences.json → design_constraints.compliance (array;
  may be empty/absent).

Two things: per schema-preferences.md that path resolves to the {value, chosen_by, prompt, design_consequence} wrapper, not an array, and it bypasses this Step 0, so ["unknown"]/["none"] enter the set the skill is handed.

Not behavioral - tf-best-practices/references/security-posture-rules.md:280-292 gates emissions on membership of soc2/pci/hipaa/fedramp and treats a non-gating set like empty, so nothing wrong gets emitted. But heroku's counterpart is already precise (heroku-to-aws/.../generate-terraform.md:82: "the normalized compliance array (see Step 1.5 item 0 ...)"), and Step 3.0 runs after Step 1.5, so this can just say the same thing:

- **compliance** — the normalized compliance array from Step 1.5 item 0 (empty ⇒ the skill emits no compliance-conditional hardening ...)

Same line in the migrate/ copy.

[herosjourney]

Done in cee210a4 — Step 3.0's caller-context compliance bullet now reads "the normalized compliance array from Step 1.5 item 0 (read from design_constraints.compliance.value, with none/unknown/absent normalized to [])", matching heroku's precise wording so ["unknown"]/["none"] no longer enter the set. Both copies.

[ayn-builds]

(Anchored here; the line this is about is 418, outside the diff.)

Self-check at line 418, now that this gate actually fires:

• If compliance is empty, absent, or contains only gdpr, baseline.tf does NOT contain any aws_config_* or aws_securityhub_* resources.

Two small drifts against the new Step 0. "absent" is no longer reachable (Step 0 normalizes absent to []), and ccpa is missing: Q2 option 7 writes ["ccpa"] (clarify-global.md:78), which is non-gating, so a ccpa-only run correctly emits no controls but is not covered by this assertion.

Suggest: "If compliance is empty or contains only non-gating frameworks (gdpr, ccpa), ...". Same line in the migrate/ copy.

[leon1418]

[🤖 AI review 🤖] [P2] Define retention for the CCPA value now reaching the lookup

The CCPA case also exposes a gap before the self-check: Q2 option 7 writes design_constraints.compliance.value = ["ccpa"], which the new normalization preserves. Step 1.5 item 1 then requires max() across the declared frameworks, but its table has no ccpa entry; the 90-day fallback applies only to an empty array. A valid CCPA-only selection therefore has no defined cloudtrail_retention_days, and mixed selections containing CCPA need unspecified handling too. Before this patch, the absent top-level key took the 90-day fallback.

Please define the CCPA mapping or an explicit non-gating fallback in both generator copies. Verify CCPA-only resolves to a positive integer without Config/Security Hub, and CCPA + HIPAA preserves the HIPAA maximum. This concerns the generator's lookup, not a statutory CCPA retention requirement. Verified from the exact producer/table and independent source-derived probes; no deployment was run.

[herosjourney]

Fixed in cee210a4. Added a ccpa → 365 row to the Step 1.5 item 1 retention table and generalized the 90-day fallback so it applies to any array that yields no mapping, not only []. So:

• CCPA-only (["ccpa"]) → cloudtrail_retention_days = 365, and — since ccpa is non-gating — no Config/Security Hub controls, matching the self-check.
• CCPA + HIPAA → max(365, 2190) = 2190, HIPAA maximum preserved.
• Any lone non-gating framework not in the table still floors to 90 rather than being undefined.

Applied in both the advisor/ and migrate/ copies (byte-identical).

[herosjourney]

Done in cee210a4. Self-check now reads "If compliance is empty or contains only non-gating frameworks (gdpr, ccpa), ..." — dropped the now-unreachable "absent" (Step 0 normalizes it to []) and added ccpa so the ccpa-only case (which correctly emits no controls) is covered by the assertion. Both copies.

[leon1418]

[🤖 AI review 🤖] Verified fixed at 2cd2fdc2efeb5a5d2394f76c420788cae50b1ee9 in both plugin copies. CCPA-only resolves to 365 days without Config/Security Hub; CCPA + HIPAA retains 2190 days; empty or unmapped sets fall back to 90. The normalized authoring handoff and non-gating self-check are also explicit now. Source-derived checks cover all 64 combinations of the six supported frameworks plus default/boundary cases. This addresses the retention finding; no cloud deployment was performed.

[ayn-builds]

Worth noting this now states the none/unknown/absent rule inline, which makes three copies of it (here, generate-artifacts-infra.md Step 1.5 item 0, and heroku's generate-terraform.md:145).

Estimate runs before Generate so it cannot cite Generate's Step 0, and I would not restructure for it in this PR. But if the semantics ever change, this copy is the easiest to miss - one option is to state the normalization once in schema-preferences.md under the compliance row and have all three cite it.

[leon1418]

[🤖 AI review 🤖]

Reviewed 64046abbd9fe8eac4dc9121108b1ce6ec52c36ce with OCR delegation, an independent general reviewer, repository contract tracing, and a completed evidence gate. The canonical .value read is correct. Two P2 corrections remain in generate-artifacts-infra.md, in both plugin copies:

1. Define retention for CCPA-only and mixed declarations (Step 1.5 item 1). Details are in the existing CCPA thread.
2. Derive the newly enabled Config managed-policy ARN from the target partition so FedRAMP/GovCloud output uses aws-us-gov (Step 1.5 item 6).

Validation: source-derived framework/default/combination probes, exact mirror comparison, drift check, formatting, Markdown lint, git diff --check, and both fixture-asserter runners passed. The probes expose the two contract gaps; existing fixtures do not exercise compliance generation. No model-driven Terraform generation or AWS deployment was performed. CI is green at this head; GitHub still reports the branch behind, with approval and conversation requirements outstanding.

[leon1418]

[🤖 AI review 🤖] [P2] Use the target partition for the newly enabled Config policy

Recognizing fedramp here now enables Step 1.5 item 6, which specifies arn:aws:iam::aws:policy/service-role/AWS_ConfigRole. Q2 requires GovCloud for FedRAMP, whose ARNs use aws-us-gov (AWS ARN contract). The generated role attachment therefore targets a commercial-partition policy instead of the GovCloud policy and cannot apply as specified.

Derive this policy ARN from the provider partition, including the referenced partition data source, in both advisor/ and migrate/ copies. Check that commercial regions retain arn:aws:... and both GovCloud regions use arn:aws-us-gov:.... The literal predates this PR, but this corrected input read newly activates it for normal FedRAMP preferences.

[herosjourney]

Fixed in cee210a4. Item 6 no longer hard-codes the aws partition — it now instructs adding a data "aws_partition" "current" {} source and setting policy_arn = "arn:${data.aws_partition.current.partition}:iam::aws:policy/service-role/AWS_ConfigRole". Commercial regions resolve to arn:aws:... and GovCloud (which Q2 pins FedRAMP to) resolves to arn:aws-us-gov:..., so the role attachment applies as specified. Applied in both advisor/ and migrate/ copies.

[leon1418]

[🤖 AI review 🤖] Verified fixed at 2cd2fdc2efeb5a5d2394f76c420788cae50b1ee9 in both plugin copies. Item 6 declares data "aws_partition" "current" {} when needed and uses its partition attribute in the Config role policy ARN. Source-derived rendering for commercial regions and both GovCloud regions matches the frozen HashiCorp/AWS contracts and preserves AWS_ConfigRole. This addresses the partition finding; validation did not include Terraform provider execution or an AWS deployment.

[leon1418]

[🤖 AI review 🤖]

Verified 2cd2fdc2efeb5a5d2394f76c420788cae50b1ee9 with OCR delegation, one fresh independent general reviewer, parent contract review, and the completed convergence-v1 evidence gate.

Both earlier findings are fixed: CCPA retention is defined, and the Config policy ARN uses the target partition. I replied in their original threads. The normalized Terraform authoring handoff and non-gating self-check are also corrected.

One P2 remains: a saved decision estimated before the canonical compliance read can resume into the new generator without refreshing its estimate, so generated controls and the reported costs/budget disagree. I missed this existing resume path in the previous review. The consolidated correction is a retained-estimate compatibility guard in both generate-artifacts-infra.md copies, with no unnecessary repricing of compatible runs; details are inline.

Validation: source-derived framework/partition probes; a separate four-revision resume trace; actual report-validator positive/negative controls; drift, shared-file, formatting, Markdown, GCP frontmatter, both fixture suites, and whitespace checks. No model-driven generation/resume or AWS deployment was run. Approval and any change to the blocking review state remain for the user.

[leon1418]

[🤖 AI review 🤖] [P2] Check retained estimate coverage before enabling controls on resume

A run can finish Estimate under the old top-level reader with design_constraints.compliance.value = ["soc2"], then stop at the supported decide-complete state. After updating the skill, accepting Generate loads this corrected reader while SKILL.md explicitly preserves the old estimate. Config/Security Hub are now emitted, but estimation-infra.json still lacks security_baseline_compliance; the budget and final report reuse those understated totals. Neither Generate's entry checks nor the pre-report gate catches that mismatch.

Add a compatibility check before conditional emission in both generator copies. If the saved estimate lacks the required compliance entry, stop with a targeted request to refresh Estimate through the authorized flow. Compatible estimates should resume without recomputation or double-counting, and non-gating inputs should remain unaffected.

This path already existed at 64046ab; I missed it in the previous review. The latest CCPA and partition corrections are verified fixed. This finding is supported by the explicit resume/source contracts and labeled simulations, not a live generator run.

[jkzietz]

Heads up: the aws-startup-advisor plugin has a new home

The plugin now lives in the Agent Toolkit for AWS, merged to main earlier today:
https://github.com/aws/agent-toolkit-for-aws/tree/main/plugins/aws-startup-advisor

We are beginning plans to deprecate this repository, so that's the copy to build on going forward — changes landed here won't reach customers once distribution repoints. Please re-open this PR against aws/agent-toolkit-for-aws.

Porting your diff: paths move from advisor/plugins/aws-startup-advisor/… to plugins/aws-startup-advisor/…. Two differences to expect:

• The plugin declares a single MCP server there — aws-mcp, the unified AWS MCP Server — and pricing is cache-only, with no live lookup.
• The destination enforces markdownlint's default rule set, which this repo does not. MD036 (emphasis used as a heading) and MD059 (descriptive link text) are the two that usually need fixing.

Happy to help with the move if anything doesn't map cleanly.

[herosjourney]

Ported to aws/agent-toolkit-for-aws#341 per the move to the new plugin home. Paths remapped from advisor/plugins/aws-startup-advisor/… to plugins/aws-startup-advisor/…; the port passes the destination's markdownlint default ruleset and tools/validate.py. Closing this one in favor of the ported PR, since this repo is being deprecated.