Add plan diff information in the comment - #291
Merged
Merged
Conversation
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.
Codecov Report❌ Patch coverage is
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
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.
* 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.
|
❌ The last analysis has failed. |
arunim2405
approved these changes
Sep 4, 2026
refeed
added a commit
that referenced
this pull request
Sep 7, 2026
`Black linting` has been red on `main` since #291 landed: one stray blank line in `platform/report.py` and three call sites Black joins back onto a single line, all of which fit inside the configured 120 columns. Formatting only, no behaviour change; `tests/platform` passes unchanged. The job pins Black 25.1.0 deliberately, so this is what the tree is formatted for rather than whatever is newest today.
refeed
added a commit
that referenced
this pull request
Sep 8, 2026
* chore(release): 1.2.1 Promote the `Unreleased` changelog section, bump `setup.py` and `src/tirith/__init__.py`, and move the documentation's install pins onto the new tag. `tirith lint`, `tirith fmt`, the pre-commit hooks and `tirith ui` have been on `main` since 1.2.0 and were unreachable from a pinned install, so the editor page carried a note telling readers to install from `main` instead. The release contains them; the note goes, and the pre-commit `rev:` moves from `main` to the tag that publishes the hooks. The README's install-verification output is pinned to `__version__` by `tests/test_readme_is_current.py`, so it moves with the bump. Closes #371 * style: apply black 25.1.0 to the plan-diff report `Black linting` has been red on `main` since #291 landed: one stray blank line in `platform/report.py` and three call sites Black joins back onto a single line, all of which fit inside the configured 120 columns. Formatting only, no behaviour change; `tests/platform` passes unchanged. The job pins Black 25.1.0 deliberately, so this is what the tree is formatted for rather than whatever is newest today.
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
2 participants
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.
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 StackGuardian run.
The verdict was there; the subject of the verdict was not.
What it looks like:
🛡️ Tirith — 1 failed, 2 passed
Scanned commit
2c44536· dir.· workflowplan-diff-render-testPlan: 6 to add, 4 to change, 2 to destroy, 2 to replace. 1 unchanged.
tirith-demo-cost-ceilingmonthly-cost-ceilingtirith-demo-opa-checks3-naming-and-regiontirith-demo-s3-owner-tags3-buckets-must-declare-an-owner❌ tirith-demo-s3-owner-tag › s3-buckets-must-declare-an-owner
Owner tag on every planned S3 bucketresource_type: 'aws_s3_bucket' is not found💵 Estimated monthly cost: 0.00 USD
✅ 2 passed · View run in StackGuardian