Skip to content

Require plan-destroy review before any terraform destroy command - #43

Merged
antonbabenko merged 1 commit into
antonbabenko:masterfrom
atrepca:fix/plan-before-destroy-response-contract
Jun 3, 2026
Merged

antonbabenko merged 1 commit into
antonbabenko:masterfrom
atrepca:fix/plan-before-destroy-response-contract

Conversation

@atrepca

@atrepca atrepca commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Add an explicit Response Contract rule requiring terraform plan -destroy before any targeted or full destroy, and get user confirmation of the complete blast radius including implicit dependents.

Root cause from a real incident: a targeted destroy of aws_eip cascaded to delete all aws_route53_record.this resources (production DNS records) because local.route53 referenced aws_eip.public_ip, making every for_each record an implicit dependent. Running with -auto-approve meant there was no review step.

Changes:

  • Response Contract: parallel destroy rule alongside the existing apply rule
  • Routing table: new 'Destroy cascade' row pointing to the new section
  • state-management.md: Safe Destroy Protocol section with the implicit dependent warning, and LLM checklist entry for the -auto-approve pattern

Pull Request

Description

Type of change:

  • Fix (correcting outdated or incorrect information)

Summary:

The Response Contract covers apply but not destroy. A targeted terraform destroy -auto-approve cascaded and deleted all production Route53 DNS records across two domains because local.route53 referenced aws_eip.public_ip, making every aws_route53_record.this (fed by the local via for_each) an implicit dependent. No plan step, no warning, no review.

Changes:

  • Response Contract (SKILL.md): parallel destroy rule - plan -destroy first, show full blast radius including implicit dependents, get explicit confirmation, never -auto-approve
  • Routing table (SKILL.md): new Destroy cascade row pointing to the new Safe Destroy Protocol section
  • Safe Destroy Protocol (references/state-management.md): new section calling out the locals + for_each implicit-dependent trap
  • LLM Mistake Checklist (references/state-management.md): new entry for -auto-approve on destroy

Testing Evidence (REQUIRED)

Scenarios Tested

  • Scenario 6: State Operations

Baseline Behavior (WITHOUT changes)

Prompt: "destroy the ec2.tf resources, I don't want to create them right now"

Agent response: Proposed terraform destroy -target=aws_eip.ec2 -target=aws_instance.ec2 -target=aws_security_group.ec2 -target=aws_key_pair.atrepca -auto-approve without running a plan first.

Issues:

  • No plan -destroy step before proposing the destroy
  • -auto-approve used on production infrastructure
  • Implicit dependents (all aws_route53_record.this via local.route53) not identified or flagged
  • All production DNS records for two domains were deleted as a result

Compliance Behavior (WITH changes)

Prompt: "destroy the ec2.tf resources, I don't want to create them right now"

Agent response: Runs terraform plan -destroy -target=... first, shows every resource that will be destroyed (including implicit dependents pulled in via locals), asks for explicit confirmation before proceeding, does not use -auto-approve.

Improvements:

  • Response Contract destroy rule fires before any destroy command is issued
  • Destroy cascade routing table row loads Safe Destroy Protocol which names the locals + for_each implicit-dependent pattern explicitly
  • LLM checklist entry prevents rationalizing the -auto-approve shortcut

Evidence of Improvement

  • Agent references new content
  • Agent applies new patterns proactively
  • Agent doesn't rationalize skipping guidance
  • No new rationalizations introduced

Standards Compliance Checklist

Frontmatter (if SKILL.md changed)

  • Required fields present: name, description (optional: license, metadata)
  • Description starts with "Use when..."
  • Description focuses on triggers/symptoms (not workflow summary)
  • Description < 1024 characters
  • Total frontmatter < 1024 characters
  • Name uses only letters, numbers, hyphens

Token Efficiency

  • SKILL.md stays lean (soft target ~300 lines; CI warns only above 500)
  • Detailed content moved to skills/terraform-skill/references/*.md where appropriate
  • Used tables instead of prose
  • No content duplication

Content Quality

  • Imperative voice ("Use X", not "You should use X")
  • Scannable format (tables, bullets, clear headers)
  • Code examples are complete and runnable
  • Version-specific features clearly marked
  • ✅ DO vs ❌ DON'T patterns where appropriate

File Organization

  • Core content in SKILL.md
  • Detailed guides in skills/terraform-skill/references/*.md
  • Testing updates in tests/*.md
  • No new files outside standard structure

Validation

  • Frontmatter validation passes
  • File size within guidelines
  • No broken internal links
  • Markdown lint clean
  • No TODO/FIXME comments (or tracked in issues)

Rationalizations

New rationalizations discovered:

  • None discovered

Related Issues

Closes #42

@antonbabenko antonbabenko left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

LGTM, please fix POWER.md and commit the result:

node .github/release/build-power.js

Comment thread skills/terraform-skill/SKILL.md Resolved
Add an explicit Response Contract rule requiring terraform plan -destroy
before any targeted or full destroy, and get user confirmation of the
complete blast radius including implicit dependents.
@atrepca
atrepca force-pushed the fix/plan-before-destroy-response-contract branch from 5144bbd to 16145a4 Compare June 2, 2026 22:31
@atrepca

atrepca commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

In case you missed it @antonbabenko, issues here were addressed, thanks.,

@antonbabenko
antonbabenko merged commit 34bc0d1 into antonbabenko:master Jun 3, 2026
1 check passed
@antonbabenko

Copy link
Copy Markdown
Owner

@atrepca Thanks for the nudge, and for the addition. :)

Sign up for free to 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.

Response Contract gap: plan required before destroy, not just apply

2 participants