Skip to content

cowork: fix terraform rollback generator correctness and safety - #48

Draft
Coding-Dev-Tools wants to merge 3 commits into
mainfrom
cowork/improve-deploydiff
Draft

cowork: fix terraform rollback generator correctness and safety#48
Coding-Dev-Tools wants to merge 3 commits into
mainfrom
cowork/improve-deploydiff

Conversation

@Coding-Dev-Tools

Copy link
Copy Markdown
Owner

Fixes two correctness/safety issues in rollback.py Terraform rollback generation:

  1. Empty plans: previously produced a header plus a blanket terraform destroy -auto-approve && terraform apply -auto-approve suggestion even when there was nothing to roll back. Now short-circuits to a single # No changes to roll back line.
  2. Contradictory commands for replacements: resources with create_before_delete/delete_before_create/replace actions appeared in both the creates list (destroy) and the destructive list (apply), emitting destroy-then-apply for the same resource. Replacements now revert via a single apply of the previous config; pure creates get destroy, pure deletes get an explicit restore comment.

Adds tests/test_rollback_safety.py regression tests and updates the empty-plan edge-case test. 125/125 tests green, ruff clean.

Coding-Dev-Tools and others added 2 commits August 21, 2026 19:38
- Empty plans now short-circuit to '# No changes to roll back' instead of
  emitting a header plus a dangerous blanket 'terraform destroy -auto-approve
  && terraform apply -auto-approve' suggestion for a plan with no changes.
- Replacements (create-before-delete / delete-before-create / replace) no
  longer produce contradictory destroy + apply commands for the same
  resource; they revert via apply of the previous config only.
- Pure creates get destroy; pure deletes get an explicit restore comment
  before their apply.
- Added regression tests (tests/test_rollback_safety.py); updated the
  empty-plan edge-case test to assert the new safe behavior. 125/125 green,
  ruff clean.
…ut accident; keep rollback safety fixes intact
@github-actions

Copy link
Copy Markdown

🤖 Automated Code Review

✅ Ruff Lint — No issues

⚠️ Ruff Format — Formatting needed

unformatted: File would be reformatted
   --> src/deploydiff/cli.py:76:15
    |
75  | )
76  + @click.option("-v", "--verbose", is_flag=True, help="Show before/after details for each change")
77  | @click.option(
    -     "-v", "--verbose", is_flag=True, help="Show before/after details for each change"
    - )
    - @click.option(
78  |     "--exit-on-destroy",
79  |     is_flag=True,
80  |     help="Exit with code 1 if the plan contains destructive changes (deletes or replaces)",
81  | )
    - def preview(
    -     terraform_file, cloudformation_file, pulumi_file, verbose, exit_on_destroy
    - ) -> None:
82  + def preview(terraform_file, cloudformation_file, pulumi_file, verbose, exit_on_destroy) -> None:
83  |     """Preview infrastructure changes from a plan file."""
--------------------------------------------------------------------------------
129 | )
    - def cost(
    -     terraform_file, cloudformation_file, pulumi_file, pricing_file, threshold
    - ) -> None:
130 + def cost(terraform_file, cloudformation_file, pulumi_file, pricing_file, threshold) -> None:
131 |     """Estimate monthly cost impact of infrastructure changes. (Pro feature)"""
--------------------------------------------------------------------------------
199 |     if len(provided) > 1:
    -         console.print(
    -             "[red]Error: Provide only one source file (--tf, --cfn, or --pulumi)[/red]"
    -         )

✅ Secret Detection — Clean

✅ Large Files — Within limits

📊 Diff Stats — 3 file(s) changed

 src/deploydiff/rollback.py    | 43 ++++++++++++++++++++++++++++++-------
 tests/test_edge_cases.py      |  5 +++--
 tests/test_rollback_safety.py | 50 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+), 10 deletions(-)

Verdict: ⚠️ Warnings — Lint/format issues found. Recommend fixing before merge.

Automated by Coding-Dev-Tools/.github reusable workflow.

All three parsers (terraform/cfn/pulumi) silently returned an empty plan for
any well-formed JSON that was not a plan file, so the CLI reported "no
changes" and exited 0 - a silent-green trap. Add PlanFormatError raised on
structural mismatch, validated immediately after input load, and surface it
in _load_plan as a red CLI error with exit 1. +13 regression tests.
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.

1 participant