Show attribute-level changes in the plan diff - #292
Merged
Merged
Conversation
The resource list said what was changing. This says what about it.
Rendered from the masked plan document, as before -- `before`, `after`,
`after_unknown` and the `*_sensitive` trees carry everything needed, including
`replace_paths`, which names the one attribute whose change is costing a destroy
and recreate. That line is the most consequential thing in a plan review and no
amount of resource-level summary conveys it.
What each action shows differs on purpose. A create names the values the author
chose and counts the computed ones, because a wall of "(known after apply)" is
not information. An update and a replace show only what changed. A destroy shows
nothing: the resource is going away, its former values do not inform the
decision, and they do not belong in a public comment.
Two things this had to get right, both found by writing it wrong first.
Attribute values are as author-controlled as addresses -- more so, being the
literal text of their terraform. A first draft interpolated them raw and a value
of three backticks closed the block: five fence terminators where there should be
two, with a forged verdict heading rendering as markdown after it. Keys and
values now both go through _fence_safe. The guard belongs on both or it protects
neither.
Sensitivity is reported as a tree mirroring the value, not a boolean:
{"triggers_replace": [false]} means the element is NOT sensitive. A truthy test
on that list says the opposite, and duly printed "(sensitive value)" over a value
that was never secret. _contains_true walks the tree and treats any true anywhere
as marked -- conservative on purpose, since for sensitivity it over-hides rather
than leaks.
The cap is now counted in lines rather than resources, because a resource brings
its attributes with it and it is the line count that decides readability.
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
Modules need no code change -- terraform flattens them into the same resource_changes list, so nesting shows up only as a longer address. These tests pin that down so it stays true, and add the one genuinely new attack surface: a module for_each key lands before the resource part of the address, so a guard written around resource names rather than whole addresses would miss it.
Two changes to how an oversized plan is handled. The block is no longer collapsed behind a <details> above twelve resources. The plan is the thing this block was added to show, and putting it behind a click makes the common case a reviewer who never sees it. And when the plan does not fit, detail gives way before resources: every attribute row is dropped so that every resource stays named, and only a bare list that is still too long is cut. A reviewer can act on "this is being destroyed" without knowing which field changed, but not the reverse. Detail is dropped wholesale rather than at the cut-off point -- trimming there would annotate the first few resources and leave the rest bare, which reads as though the later ones had nothing to say.
|
❌ The last analysis has failed. |
refeed
added a commit
that referenced
this pull request
Sep 4, 2026
* feat: add plan diff information in the comment The comment said whether policies passed and never what was changing. To learn that a pull request creates one bucket, a reviewer had to leave the review and open the job log or the run. It now carries the planned changes and terraform's summary line, rendered from the *masked* plan document rather than from `terraform show` output. That is a security choice, not a convenience one: masking is the only thing keeping a sensitive value out of a public comment, and captured text would not carry it. no-op resources are counted, not listed. A plan against applied infrastructure carries one for every resource in state -- on the demo repository five of six rows -- and listing them buries the one that changed. Replacements are counted separately rather than folded into add and destroy, which is where terraform puts them. "1 to replace" is the number that should make a reviewer look twice, and it disappears when spread across two columns. Markers are +, - and ! only. terraform's ~ means nothing to the diff highlighting, so an update row would render plain and the fence would buy nothing. The block is dropped first when the comment is too long, ahead of any finding. A comment that keeps the diff and loses the violation has failed at its job. `action_summary` moves out of the TUI into a shared module so both surfaces answer the replacement question identically. * fix: the unchanged count is plain text, not <sub> Every other <sub> in the reporter wraps a whole line -- the cost line, the context line, the footer. This one wrapped a fragment mid-line, gluing an HTML tag onto a line that otherwise reads as terraform output. The count itself stays: it is what explains why the list is shorter than the plan, so a reviewer who knows there are seven resources does not wonder where six went. It just does not need to shout. Also strengthens the newline guard test to use a real newline rather than an escaped one. Terraform escapes newlines in a for_each key into an inert literal, so the original test was checking the harmless case; a hand-written state or a different tool can still carry a real one, and one real newline in an address is one forged row in the diff. * Show attribute-level changes in the plan diff (#292) * feat: show attribute-level changes in the plan block The resource list said what was changing. This says what about it. Rendered from the masked plan document, as before -- `before`, `after`, `after_unknown` and the `*_sensitive` trees carry everything needed, including `replace_paths`, which names the one attribute whose change is costing a destroy and recreate. That line is the most consequential thing in a plan review and no amount of resource-level summary conveys it. What each action shows differs on purpose. A create names the values the author chose and counts the computed ones, because a wall of "(known after apply)" is not information. An update and a replace show only what changed. A destroy shows nothing: the resource is going away, its former values do not inform the decision, and they do not belong in a public comment. Two things this had to get right, both found by writing it wrong first. Attribute values are as author-controlled as addresses -- more so, being the literal text of their terraform. A first draft interpolated them raw and a value of three backticks closed the block: five fence terminators where there should be two, with a forged verdict heading rendering as markdown after it. Keys and values now both go through _fence_safe. The guard belongs on both or it protects neither. Sensitivity is reported as a tree mirroring the value, not a boolean: {"triggers_replace": [false]} means the element is NOT sensitive. A truthy test on that list says the opposite, and duly printed "(sensitive value)" over a value that was never secret. _contains_true walks the tree and treats any true anywhere as marked -- conservative on purpose, since for sensitivity it over-hides rather than leaks. The cap is now counted in lines rather than resources, because a resource brings its attributes with it and it is the line count that decides readability. * test: cover module addresses and the module for_each injection surface Modules need no code change -- terraform flattens them into the same resource_changes list, so nesting shows up only as a longer address. These tests pin that down so it stays true, and add the one genuinely new attack surface: a module for_each key lands before the resource part of the address, so a guard written around resource names rather than whole addresses would miss it. * feat: always expand the plan block, and drop detail before resources Two changes to how an oversized plan is handled. The block is no longer collapsed behind a <details> above twelve resources. The plan is the thing this block was added to show, and putting it behind a click makes the common case a reviewer who never sees it. And when the plan does not fit, detail gives way before resources: every attribute row is dropped so that every resource stays named, and only a bare list that is still too long is cut. A reviewer can act on "this is being destroyed" without knowing which field changed, but not the reverse. Detail is dropped wholesale rather than at the cut-off point -- trimming there would annotate the first few resources and leave the rest bare, which reads as though the later ones had nothing to say.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #291, stacked on it so this diff is only the attribute work. #291 gives one line per
resource — address and action. This says what about it.
Still rendered from the masked document.
before,after,after_unknownand the*_sensitivetrees carry everything needed — includingreplace_paths, which names the oneattribute whose change is costing a destroy and recreate. That line is the most consequential thing
in a plan review, and no amount of resource-level summary conveys it.
What each action shows, and why they differ
(known after apply)is not information# forces replacementTwo things this had to get right, both found by writing it wrong first
Attribute values are a new attack surface, and my first draft was vulnerable. Values are as
author-controlled as addresses — more so, being the literal text of the author's terraform. The
first version interpolated them raw, and a value of three backticks closed the block: five fence
terminators where there should be two, with a forged
## Tirith — all policies passedheadingrendering as markdown after it. Exactly the hole #291's address guard was written to close, reopened
one layer in. Keys and values now both go through
_fence_safe; the guard belongs on both or itprotects neither.
Sensitivity is a tree, not a boolean. terraform reports
{"triggers_replace": [false]}— thelist is structure, the
falseis the answer. A truthy test on that node says the opposite, and dulyprinted
(sensitive value)over a value that was never secret, hiding information for no reason._contains_truewalks the tree and treats anytrueanywhere as marked. Deliberatelyconservative: for sensitivity it over-hides rather than leaks, and for unknown-ness it prefers "we
cannot show this" to printing half a value as if it were whole.
Both are pinned by tests named for the failure rather than the feature:
test_an_attribute_value_cannot_escape_the_fenceandtest_a_shape_shaped_sensitivity_tree_is_not_read_as_true.Bounds
Attributes are capped per resource, and the block cap is now counted in lines rather than
resources — a resource brings its attributes with it, and it is the line count that decides
whether the comment is readable.
PLAN_ROW_LIMITbecomesPLAN_LINE_LIMITfor that reason, whichis why one test in #291's file is renamed here.
Checks
tests/platform/test_report_plan_attributes.py.baseline: zero new failures. The 12 are the pre-existing ones on
main— 11 fixtureFileNotFoundErrors and the README-drift test.tofu, not just fixtures: afor_eachkey and anattribute value both carrying fence terminators render inert, with 2 fence terminators and no
backticks inside the block.
Not in scope
Nested attribute trees. A map or list renders compacted on one line rather than as a nested diff —
enough to answer "did this change and roughly to what". A real nested diff is a larger feature and
would need its own thinking about depth limits.
Follow-up in this branch: always expanded, and detail gives way first
Two changes to how the block behaves once it gets large.
It is never collapsed. The block used to hide behind
<details><summary>Show plan</summary>above twelve resources. The plan is the thing this block exists to show, and putting it behind a
click makes the common case a reviewer who never sees it.
Detail is given up before resources. When the plan exceeds the line cap, every attribute row
is dropped so that every resource stays named; only a bare list that is still too long is cut off
the end. A reviewer can act on "this is being destroyed" without knowing which field changed, but
not the reverse.
Detail is dropped wholesale rather than from the cut-off point onward. Trimming at the boundary
would annotate the first handful of resources and leave the rest bare — which reads as though the
later ones had nothing to say, and the bare-looking ones are exactly where a reviewer stops looking.
Also adds module coverage. Modules needed no code change: terraform flattens module resources
into the same flat
resource_changeslist, so nesting shows up only as a longer address. The testspin that down, and cover the one genuinely new attack surface — a module
for_eachkey landsbefore the resource part of the address, so a guard written around resource names rather than
whole addresses would miss it.
Verified end to end
Three real plans in tirith-plan-diff-test#1,
all runs green:
tag=default(platform)tag=local(no credentials)tag=oversizedThe oversized comment degrades in the designed order:
Module addresses on a real plan, with state seeded so they plan updates and a replace rather than
only creates:
Fence integrity holds across all three: exactly two line-initial fences and zero backticks
inside the fence body, including the run carrying a hostile module
for_eachkey.Full suite: 12 failed, 781 passed. Diffed against
main's baseline — the failure sets areidentical, so zero new failures.