diff --git a/src/tirith/plan_actions.py b/src/tirith/plan_actions.py
new file mode 100644
index 00000000..69956e4b
--- /dev/null
+++ b/src/tirith/plan_actions.py
@@ -0,0 +1,212 @@
+"""
+Terraform plan actions, in vocabulary a human reads.
+
+Shared because two very different surfaces need the same answer: the TUI's result list and the
+pull-request comment. Keeping one implementation is what stops them disagreeing about whether
+`["delete", "create"]` is a replacement or two separate operations.
+"""
+
+import json
+
+# What terraform calls the operation, keyed by the exact action tuple it reports.
+#
+# The replacement pair is ordered, and which way round it is changes the risk rather than the
+# wording: delete-then-create has downtime, create-then-delete does not. Reporting both as
+# "replace" would hide the difference a reviewer most needs to see.
+_ACTION_NAMES = {
+ ("no-op",): "no change",
+ ("create",): "create",
+ ("delete",): "destroy",
+ ("update",): "update in place",
+ ("read",): "read",
+ ("delete", "create"): "replace (destroy first)",
+ ("create", "delete"): "replace (create first)",
+}
+
+# Which markers a ```diff fence understands. GitHub highlights `+`, `-` and `!` and nothing else --
+# terraform's own `~` for an update renders as plain text, which defeats the point of the fence. So
+# update and replace share `!` and the words carry the precision.
+_CREATE = "+"
+_DESTROY = "-"
+_CHANGE = "!"
+
+_ACTION_MARKERS = {
+ ("create",): _CREATE,
+ ("delete",): _DESTROY,
+ ("update",): _CHANGE,
+ ("delete", "create"): _CHANGE,
+ ("create", "delete"): _CHANGE,
+}
+
+
+def action_summary(actions):
+ """
+ The planned action, in terraform's own vocabulary.
+
+ An unrecognised combination is joined rather than dropped: a tuple this module has not seen is
+ still worth showing verbatim, and inventing a friendly name for it would be a guess.
+ """
+ actions = tuple(a for a in actions or () if a)
+ if not actions:
+ return ""
+ return _ACTION_NAMES.get(actions) or ", ".join(actions)
+
+
+def action_marker(actions):
+ """The diff marker for an action tuple, or "" for no-op and anything unrecognised."""
+ actions = tuple(a for a in actions or () if a)
+ return _ACTION_MARKERS.get(actions, "")
+
+
+def is_no_op(actions):
+ return tuple(a for a in actions or () if a) == ("no-op",)
+
+
+def plan_counts(resource_changes):
+ """
+ Count a plan the way its summary line reports it.
+
+ Replacements are counted on their own rather than folded into add + destroy. Terraform reports
+ them inside those two, so this deliberately differs: "2 to replace" is the number that should
+ make a reviewer look twice, and it disappears when it is spread across the other columns.
+ """
+ counts = {"add": 0, "change": 0, "destroy": 0, "replace": 0, "no_op": 0}
+ for change in resource_changes or []:
+ actions = tuple(a for a in ((change.get("change") or {}).get("actions") or []) if a)
+ if actions in (("delete", "create"), ("create", "delete")):
+ counts["replace"] += 1
+ elif actions == ("create",):
+ counts["add"] += 1
+ elif actions == ("update",):
+ counts["change"] += 1
+ elif actions == ("delete",):
+ counts["destroy"] += 1
+ elif actions == ("no-op",):
+ counts["no_op"] += 1
+ return counts
+
+
+def summary_line(counts):
+ """
+ The one line a reviewer looks for.
+
+ `replace` is only mentioned when there is one, so the common case reads exactly like terraform's
+ own summary and the unusual case is conspicuous.
+ """
+ parts = [
+ f"{counts.get('add', 0)} to add",
+ f"{counts.get('change', 0)} to change",
+ f"{counts.get('destroy', 0)} to destroy",
+ ]
+ if counts.get("replace"):
+ parts.append(f"{counts['replace']} to replace")
+ return "Plan: " + ", ".join(parts) + "."
+
+
+# Rendered in place of a value we must not or cannot print. Both are terraform's own wording, so a
+# reader who knows plan output needs no translation.
+SENSITIVE = "(sensitive value)"
+UNKNOWN = "(known after apply)"
+
+
+def _contains_true(node):
+ """
+ Whether a sensitivity or unknown-ness tree marks anything at all.
+
+ These trees mirror the *shape* of the value rather than being booleans: terraform reports
+ `{"triggers_replace": [false]}`, where the list is structure and the `false` is the answer. A
+ truthy test on the node therefore says "sensitive" for a list that says the opposite -- which is
+ how a first attempt printed "(sensitive value)" over a value that was never secret.
+
+ Any `True` anywhere means the whole value is treated as marked. That is deliberately
+ conservative: 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.
+ """
+ if node is True:
+ return True
+ if isinstance(node, dict):
+ return any(_contains_true(v) for v in node.values())
+ if isinstance(node, list):
+ return any(_contains_true(v) for v in node)
+ return False
+
+
+def attribute_changes(change, limit=8):
+ """
+ Per-attribute changes for one resource, as (marker, key, before, after, forces_replacement).
+
+ `before` and `after` are already rendered to strings here, because whether a value may be shown
+ at all is plan semantics -- sensitivity and unknown-ness live in the document -- while *how* to
+ make a string safe for the surface it lands on belongs to the renderer.
+
+ What each action shows, and why it differs:
+
+ create every attribute with a known value. The unknown ones are what terraform fills in, and
+ a wall of "(known after apply)" says nothing a reader can act on, so they are counted.
+ update only what changed. That is the whole question being asked.
+ replace the same, plus which attribute forced it -- the most useful line in a plan review.
+ delete nothing. The resource is going away; its former values neither inform the decision nor
+ belong in a public comment.
+ """
+ actions = tuple(a for a in ((change or {}).get("actions") or []) if a)
+ if not actions or actions == ("no-op",) or actions == ("delete",):
+ return [], 0, 0
+
+ before = (change.get("before") or {}) if isinstance(change.get("before"), dict) else {}
+ after = (change.get("after") or {}) if isinstance(change.get("after"), dict) else {}
+ unknown = change.get("after_unknown") or {}
+ after_sensitive = change.get("after_sensitive")
+ after_sensitive = after_sensitive if isinstance(after_sensitive, dict) else {}
+ before_sensitive = change.get("before_sensitive")
+ before_sensitive = before_sensitive if isinstance(before_sensitive, dict) else {}
+ forces = {path[0] for path in (change.get("replace_paths") or []) if path}
+
+ creating = actions == ("create",)
+ rows = []
+ hidden_unknown = 0
+
+ for key in sorted(set(before) | set(after) | (set(unknown) if isinstance(unknown, dict) else set())):
+ node = unknown.get(key) if isinstance(unknown, dict) else None
+ is_unknown = _contains_true(node)
+ old, new = before.get(key), after.get(key)
+
+ if not is_unknown and old == new:
+ continue
+
+ if creating:
+ if is_unknown:
+ # Counted rather than printed: on a create these are every computed attribute, and
+ # naming them crowds out the values the author actually chose.
+ hidden_unknown += 1
+ continue
+ rows.append(("+", key, None, _render_value(new, after_sensitive.get(key)), key in forces))
+ continue
+
+ rows.append(
+ (
+ "~",
+ key,
+ _render_value(old, before_sensitive.get(key)),
+ UNKNOWN if is_unknown else _render_value(new, after_sensitive.get(key)),
+ key in forces,
+ )
+ )
+
+ dropped = max(0, len(rows) - limit)
+ return rows[:limit], dropped, hidden_unknown
+
+
+def _render_value(value, sensitivity_node):
+ if _contains_true(sensitivity_node):
+ return SENSITIVE
+ if value is None:
+ return "null"
+ if isinstance(value, str):
+ return f'"{value}"'
+ if isinstance(value, bool):
+ return "true" if value else "false"
+ if isinstance(value, (int, float)):
+ return str(value)
+ # Nested structures are compacted rather than rendered as a tree. A full nested diff is a much
+ # larger feature, and a compact form still answers "did this change and roughly to what".
+ return json.dumps(value, separators=(",", ":"), sort_keys=True)
diff --git a/src/tirith/platform/check.py b/src/tirith/platform/check.py
index 643bf627..bb27c6d4 100644
--- a/src/tirith/platform/check.py
+++ b/src/tirith/platform/check.py
@@ -707,6 +707,10 @@ def run_check(opts):
limit=opts.markdown_limit,
cost_breakdown=cost_breakdown,
commit=opts.sha,
+ # The masked document, never the raw one -- see report.render_plan_block.
+ plan=plan,
+ source_dir=opts.source_dir,
+ workflow_id=opts.workflow_id,
)
try:
with open(opts.output_markdown, "w") as f:
diff --git a/src/tirith/platform/report.py b/src/tirith/platform/report.py
index ffb15cd5..7ccfd8fd 100644
--- a/src/tirith/platform/report.py
+++ b/src/tirith/platform/report.py
@@ -7,6 +7,8 @@
import html
+from .. import plan_actions
+
FAIL = "FAIL"
WARN = "WARN"
PASS = "PASS"
@@ -24,6 +26,160 @@
_ICONS = {FAIL: "❌", WARN: "⚠️", APPROVAL_REQUIRED: "⏳", PASS: "✅", UNKNOWN: "❓"}
+# A hard cap on lines, independent of the comment limit. A thousand-resource plan would otherwise
+# consume the whole budget and take the findings down with it during truncation. Counted in lines
+# rather than resources because each resource brings its changed attributes with it.
+#
+# The block is never collapsed behind a . It was, above twelve resources -- but the plan is
+# the thing the comment was extended to show, and putting it behind a click means the common case is
+# a reviewer who never sees it. What gives way under this cap is detail, not visibility.
+PLAN_LINE_LIMIT = 60
+
+
+def _fence_safe(value):
+ """
+ Make a plan-derived string safe to place inside a ``` fence.
+
+ The same class of bug `_code` exists for, one layer out. A resource address comes from the plan,
+ and the plan comes from terraform a pull-request author controls -- `for_each` keys make
+ `aws_s3_bucket.demo["```"]` a legal address. Inside a fence a triple backtick does not merely
+ close an inline span, it closes the whole block, and everything after it renders as markdown: a
+ forged "all policies passed" banner, a stray hiding the real findings, a link whose
+ text and href disagree.
+
+ Backticks are removed rather than escaped, because there is no escape that works inside a fence.
+ Newlines would fabricate extra rows, so they go too.
+ """
+ text = str(value)
+ text = text.replace("`", "").replace("\r", " ").replace("\n", " ")
+ if len(text) > 200:
+ text = text[:197] + "..."
+ return text
+
+
+def _render_attributes(change):
+ """
+ The per-attribute lines under one resource row.
+
+ Every key and every value goes through `_fence_safe`, and that is the whole reason this is a
+ separate function rather than an f-string at the call site. An attribute *value* is as
+ author-controlled as an address -- more so, since it is the literal text of their terraform -- and
+ a first version of this passed values through raw. A value of "```" closed the block and let the
+ rest of the comment render as markdown, reopening exactly the hole the address guard was written
+ to close. The guard belongs on both or it protects neither.
+ """
+ rows, dropped, hidden_unknown = plan_actions.attribute_changes(change)
+
+ lines = []
+ for marker, key, before, after, forces in rows:
+ rendered_key = _fence_safe(key)
+ if before is None:
+ line = f" {marker} {rendered_key} = {_fence_safe(after)}"
+ else:
+ line = f" {marker} {rendered_key} = {_fence_safe(before)} -> {_fence_safe(after)}"
+ if forces:
+ # Terraform's own wording, and the most consequential thing on the row: it names the one
+ # attribute whose change is costing a destroy and recreate.
+ line += " # forces replacement"
+ lines.append(line)
+
+ trailer = []
+ if dropped:
+ trailer.append(f" … and {dropped} more changed attribute(s)")
+ if hidden_unknown:
+ trailer.append(f" … and {hidden_unknown} computed attribute(s), known after apply")
+ return lines + trailer
+
+
+def render_plan_block(plan):
+ """
+ The planned changes, as a diff-fenced list plus terraform's summary line.
+
+ Rendered from the *masked* plan document, never from `terraform show` output. The masking is the
+ only thing keeping a sensitive value out of a public pull-request comment, and text captured from
+ terraform would not carry it. See redact.redact_plan.
+
+ `no-op` resources are counted, not listed. A plan against applied infrastructure carries one for
+ every resource in state, and listing them buries the handful that changed.
+ """
+ if not isinstance(plan, dict):
+ return []
+
+ changes = plan.get("resource_changes")
+ if not isinstance(changes, list) or not changes:
+ return []
+
+ counts = plan_actions.plan_counts(changes)
+
+ entries = []
+ for change in changes:
+ if not isinstance(change, dict):
+ continue
+ actions = (change.get("change") or {}).get("actions") or []
+ if plan_actions.is_no_op(actions):
+ continue
+ marker = plan_actions.action_marker(actions)
+ if not marker:
+ continue
+ address = _fence_safe(change.get("address") or change.get("type") or "")
+ if not address:
+ continue
+ row = f"{marker} {address:<48} {_fence_safe(plan_actions.action_summary(actions))}".rstrip()
+ entries.append((row, _render_attributes(change.get("change") or {})))
+
+ listed_resources = len(entries)
+ rows, hidden_detail, dropped_resources = _fit_plan_rows(entries)
+
+ summary = plan_actions.summary_line(counts)
+ if counts.get("no_op"):
+ # Plain text, not . Every other in this module wraps a whole line; wrapping a
+ # fragment mid-line glues an HTML tag onto a line that otherwise reads as terraform output.
+ # The count still earns its place -- it is what explains why the list is shorter than the
+ # plan -- but it does not need to shout, and a sentence is quieter than a tag.
+ summary += f" {counts['no_op']} unchanged."
+
+ if not rows:
+ # Nothing is changing, so there is no list to show -- but the line saying so is still worth
+ # having, otherwise the comment looks like it simply forgot to mention the plan.
+ return [summary, ""]
+
+ notes = []
+ if hidden_detail:
+ notes += ["", f"… attribute detail omitted: {hidden_detail} more line(s) than a comment can carry"]
+ if dropped_resources:
+ notes += ["", f"… and {dropped_resources} more resource(s), truncated"]
+
+ return ["```diff"] + rows + notes + ["```", "", summary, ""]
+
+
+def _fit_plan_rows(entries):
+ """
+ Fit the plan into PLAN_LINE_LIMIT lines, giving up detail before resources.
+
+ Returns (rows, hidden_detail_lines, dropped_resources).
+
+ The order of sacrifice is the whole point. A resource row says *what is happening to your
+ infrastructure*; an attribute row elaborates on one. So an oversized plan loses every attribute
+ row before it loses a single resource, and only a resource list that is still too long after that
+ gets cut off the end.
+
+ 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 -- the most misleading shape available, since the untouched-looking
+ ones are exactly where a reviewer stops looking.
+ """
+ full = [line for row, attributes in entries for line in [row] + attributes]
+ if len(full) <= PLAN_LINE_LIMIT:
+ return full, 0, 0
+
+ bare = [row for row, _ in entries]
+ hidden_detail = len(full) - len(bare)
+ if len(bare) <= PLAN_LINE_LIMIT:
+ return bare, hidden_detail, 0
+
+ return bare[:PLAN_LINE_LIMIT], hidden_detail, len(bare) - PLAN_LINE_LIMIT
+
+
def summarize(policy_results):
"""
@@ -256,7 +412,16 @@ def render_cost(breakdown):
def render_markdown(
- policy_results, run_status, run_url, marker=None, limit=COMMENT_LIMIT, cost_breakdown=None, commit=None
+ policy_results,
+ run_status,
+ run_url,
+ marker=None,
+ limit=COMMENT_LIMIT,
+ cost_breakdown=None,
+ commit=None,
+ plan=None,
+ source_dir=None,
+ workflow_id=None,
):
"""
Render the results as markdown, truncating detail before the summary table.
@@ -277,8 +442,17 @@ def render_markdown(
f"## 🛡️ {headline(counts, verdict_value)}",
"",
]
- if commit:
- header += [f"Scanned commit {_html(_short_commit(commit))}", ""]
+ if commit or source_dir or workflow_id:
+ # One line of provenance. `dir` and the workflow matter when a matrix posts several comments
+ # on one pull request: today only comment-tag distinguishes them, and that is invisible.
+ bits = []
+ if commit:
+ bits.append(f"Scanned commit {_html(_short_commit(commit))}")
+ if source_dir:
+ bits.append(f"dir {_html(source_dir)}")
+ if workflow_id:
+ bits.append(f"workflow {_html(workflow_id)}")
+ header += [f"{' · '.join(bits)}", ""]
if verdict_value == "errored":
# Two different reasons land here, and saying the wrong one is worse than saying nothing:
@@ -306,18 +480,28 @@ def render_markdown(
detail_sections = [_render_detail(f) for f in findings if f["result"] in (FAIL, APPROVAL_REQUIRED, WARN, UNKNOWN)]
- body = "\n".join(header + table + detail_sections + footer)
+ plan_block = render_plan_block(plan)
+
+ body = "\n".join(header + plan_block + table + detail_sections + footer)
if len(body) <= limit:
return body
- # Drop detail sections from the end until it fits, keeping the summary table intact -- the
+ # The plan goes first, before any finding is touched. It is context; the findings are the point,
+ # and a comment that keeps the diff while dropping the violation has failed at its job.
+ if plan_block:
+ plan_block = []
+ body = "\n".join(header + plan_block + table + detail_sections + footer)
+ if len(body) <= limit:
+ return body
+
+ # Then drop detail sections from the end until it fits, keeping the summary table intact -- the
# table is the part a reviewer scans first.
kept = list(detail_sections)
while kept and len(body) > limit:
kept.pop()
omitted = len(detail_sections) - len(kept)
note = [f"", f"_… and {omitted} more finding(s). See the full run in StackGuardian._", ""]
- body = "\n".join(header + table + kept + note + footer)
+ body = "\n".join(header + plan_block + table + kept + note + footer)
if len(body) > limit:
# Even the table is too large; truncate hard rather than risk a 422.
diff --git a/src/tirith/tui/results.py b/src/tirith/tui/results.py
index 2b6cc0e0..ee753869 100644
--- a/src/tirith/tui/results.py
+++ b/src/tirith/tui/results.py
@@ -17,6 +17,8 @@
from typing import Any, Dict, Iterator, List, NamedTuple, Optional
+from .. import plan_actions
+
# The tri-state a check reports. `None` means skipped, and skipped is not a pass -- the engine
# is careful about this distinction (see the --fail-on-error commentary in cli.py) and so is
# every count here.
@@ -55,24 +57,7 @@ def is_empty(self) -> bool:
@property
def action_summary(self) -> str:
"""The planned action, in terraform's own vocabulary."""
- actions = [a for a in self.actions if a]
- if not actions:
- return ""
- if actions == ["no-op"]:
- return "no change"
- if actions == ["create"]:
- return "create"
- if actions == ["delete"]:
- return "destroy"
- if actions == ["update"]:
- return "update in place"
- # terraform expresses a replacement as an ordered pair, and which way round it is
- # changes the risk: delete-then-create has downtime, create-then-delete does not.
- if actions == ["delete", "create"]:
- return "replace (destroy first)"
- if actions == ["create", "delete"]:
- return "replace (create first)"
- return ", ".join(actions)
+ return plan_actions.action_summary(self.actions)
@property
def label(self) -> str:
diff --git a/tests/platform/test_report_plan_attributes.py b/tests/platform/test_report_plan_attributes.py
new file mode 100644
index 00000000..bfb6e434
--- /dev/null
+++ b/tests/platform/test_report_plan_attributes.py
@@ -0,0 +1,265 @@
+"""
+Attribute-level detail in the plan block.
+
+Two of these matter more than the rest. `test_an_attribute_value_cannot_escape_the_fence` covers the
+hole this feature opens -- a first draft passed values through raw and a value of "```" closed the
+block. `test_a_sensitive_value_is_never_printed` covers the one where a bug leaks rather than merely
+looks wrong.
+"""
+
+from tirith import plan_actions
+from tirith.platform import report
+
+
+def _render(changes):
+ return report.render_markdown(
+ {}, "COMPLETED", "https://example.invalid/run", plan={"resource_changes": changes}
+ )
+
+
+def _fence(body):
+ return body.split("```diff")[1].split("```")[0]
+
+
+# --- what a reviewer sees ---------------------------------------------------------------------
+
+
+def test_an_update_shows_only_what_changed():
+ body = _render(
+ [
+ {
+ "address": "terraform_data.x",
+ "type": "terraform_data",
+ "change": {
+ "actions": ["update"],
+ "before": {"input": "before", "untouched": "same"},
+ "after": {"input": "after", "untouched": "same"},
+ },
+ }
+ ]
+ )
+ fence = _fence(body)
+ assert '~ input = "before" -> "after"' in fence
+ assert "untouched" not in fence
+
+
+def test_a_create_shows_known_values_and_counts_the_computed_ones():
+ """
+ A wall of "(known after apply)" tells a reader nothing they can act on, so those are counted
+ while the values the author actually chose are named.
+ """
+ body = _render(
+ [
+ {
+ "address": "aws_s3_bucket.b",
+ "type": "aws_s3_bucket",
+ "change": {
+ "actions": ["create"],
+ "before": None,
+ "after": {"bucket": "demo-x"},
+ "after_unknown": {"arn": True, "id": True},
+ },
+ }
+ ]
+ )
+ fence = _fence(body)
+ assert '+ bucket = "demo-x"' in fence
+ assert "arn" not in fence
+ assert "2 computed attribute(s), known after apply" in fence
+
+
+def test_a_destroy_lists_no_attributes():
+ """
+ The resource is going away. Its former values do not inform the decision, and printing them puts
+ the old state of a public comment's worth of infrastructure into the comment.
+ """
+ body = _render(
+ [
+ {
+ "address": "aws_db_instance.old",
+ "type": "aws_db_instance",
+ "change": {"actions": ["delete"], "before": {"password": "hunter2"}, "after": None},
+ }
+ ]
+ )
+ fence = _fence(body)
+ assert "- aws_db_instance.old" in fence
+ assert "hunter2" not in fence
+ assert "password" not in fence
+
+
+def test_the_attribute_that_forces_a_replacement_is_named():
+ """The most consequential line in a plan review: which change is costing a destroy."""
+ body = _render(
+ [
+ {
+ "address": "terraform_data.r",
+ "type": "terraform_data",
+ "change": {
+ "actions": ["delete", "create"],
+ "before": {"triggers_replace": ["v1"], "input": "same"},
+ "after": {"triggers_replace": ["v2"], "input": "same"},
+ "replace_paths": [["triggers_replace"]],
+ },
+ }
+ ]
+ )
+ fence = _fence(body)
+ assert "# forces replacement" in fence
+ assert [line for line in fence.splitlines() if "forces replacement" in line][0].lstrip().startswith(
+ "~ triggers_replace"
+ )
+
+
+# --- sensitivity, where a bug leaks -----------------------------------------------------------
+
+
+def test_a_sensitive_value_is_never_printed():
+ body = _render(
+ [
+ {
+ "address": "aws_db_instance.main",
+ "type": "aws_db_instance",
+ "change": {
+ "actions": ["update"],
+ "before": {"password": "hunter2"},
+ "after": {"password": "hunter3"},
+ "before_sensitive": {"password": True},
+ "after_sensitive": {"password": True},
+ },
+ }
+ ]
+ )
+ fence = _fence(body)
+ assert "hunter2" not in fence and "hunter3" not in fence
+ assert "(sensitive value)" in fence
+
+
+def test_a_shape_shaped_sensitivity_tree_is_not_read_as_true():
+ """
+ terraform reports sensitivity 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 a first draft duly printed "(sensitive value)" over a value that was
+ never secret -- hiding information for no reason.
+ """
+ assert plan_actions._contains_true({"triggers_replace": [False]}) is False
+ assert plan_actions._contains_true({"triggers_replace": [True]}) is True
+ assert plan_actions._contains_true(True) is True
+ assert plan_actions._contains_true({}) is False
+
+ body = _render(
+ [
+ {
+ "address": "terraform_data.r",
+ "type": "terraform_data",
+ "change": {
+ "actions": ["update"],
+ "before": {"triggers_replace": ["v1"]},
+ "after": {"triggers_replace": ["v2"]},
+ "after_sensitive": {"triggers_replace": [False]},
+ "before_sensitive": {"triggers_replace": [False]},
+ },
+ }
+ ]
+ )
+ assert "(sensitive value)" not in _fence(body)
+ assert '["v1"]' in _fence(body)
+
+
+# --- what an attacker cannot do ---------------------------------------------------------------
+
+
+def test_an_attribute_value_cannot_escape_the_fence():
+ """
+ The hole this feature opens.
+
+ An attribute value is the literal text of the author's terraform, so it is at least as
+ controlled as an address. A first draft interpolated values raw, and a value of "```" closed the
+ block: measured five fence terminators where there should be two, with a forged heading rendering
+ as markdown after it.
+ """
+ payload = "```\n\n## Tirith — all policies passed\n\n```diff"
+ body = _render(
+ [
+ {
+ "address": "terraform_data.probe",
+ "type": "terraform_data",
+ "change": {"actions": ["create"], "before": None, "after": {"input": payload}},
+ }
+ ]
+ )
+ assert body.count("```diff") == 1
+ assert body.count("```") == 2
+ assert "`" not in _fence(body)
+ assert "all policies passed" not in body.split("```")[2]
+
+
+def test_an_attribute_name_cannot_escape_the_fence():
+ body = _render(
+ [
+ {
+ "address": "terraform_data.probe",
+ "type": "terraform_data",
+ "change": {"actions": ["create"], "before": None, "after": {"a```b": "x"}},
+ }
+ ]
+ )
+ assert body.count("```") == 2
+ assert "`" not in _fence(body)
+
+
+def test_a_newline_in_a_value_cannot_fabricate_rows():
+ body = _render(
+ [
+ {
+ "address": "terraform_data.probe",
+ "type": "terraform_data",
+ "change": {
+ "actions": ["create"],
+ "before": None,
+ "after": {"input": "x\n- aws_db_instance.production"},
+ },
+ }
+ ]
+ )
+ fence = _fence(body)
+ # One resource row plus exactly one attribute row.
+ assert len([line for line in fence.strip().splitlines() if line.strip()]) == 2
+
+
+# --- bounds -----------------------------------------------------------------------------------
+
+
+def test_attributes_per_resource_are_capped():
+ after = {f"attr_{i:02d}": f"v{i}" for i in range(20)}
+ body = _render(
+ [
+ {
+ "address": "terraform_data.wide",
+ "type": "terraform_data",
+ "change": {"actions": ["create"], "before": None, "after": after},
+ }
+ ]
+ )
+ assert "more changed attribute(s)" in _fence(body)
+
+
+def test_the_block_is_capped_in_lines_not_resources():
+ changes = [
+ {
+ "address": f"terraform_data.r{i}",
+ "type": "terraform_data",
+ "change": {
+ "actions": ["create"],
+ "before": None,
+ "after": {"a": "1", "b": "2", "c": "3"},
+ },
+ }
+ for i in range(40)
+ ]
+ body = _render(changes)
+ fence = _fence(body)
+ assert len(fence.strip().splitlines()) <= report.PLAN_LINE_LIMIT + 3
+ # 40 resources fit; 40 resources plus three attributes each do not. The detail is what gives way.
+ assert "attribute detail omitted" in fence
+ assert "terraform_data.r39" in fence
diff --git a/tests/platform/test_report_plan_block.py b/tests/platform/test_report_plan_block.py
new file mode 100644
index 00000000..2afd4dba
--- /dev/null
+++ b/tests/platform/test_report_plan_block.py
@@ -0,0 +1,288 @@
+"""
+The plan block in the pull-request comment.
+
+The load-bearing test here is the injection one. Everything else describes what a reviewer sees;
+that one describes what an attacker cannot do.
+"""
+
+from tirith.platform import report
+
+
+def _change(address, actions, resource_type="aws_s3_bucket"):
+ return {"address": address, "type": resource_type, "change": {"actions": list(actions)}}
+
+
+def _plan(*changes):
+ return {"resource_changes": list(changes)}
+
+
+def _render(plan, **kwargs):
+ kwargs.setdefault("run_url", "https://example.invalid/run")
+ return report.render_markdown({}, "COMPLETED", kwargs.pop("run_url"), plan=plan, **kwargs)
+
+
+# --- what a reviewer sees ---------------------------------------------------------------------
+
+
+def test_a_create_is_listed_and_counted():
+ body = _render(_plan(_change("aws_s3_bucket.analytics", ["create"])))
+ assert "+ aws_s3_bucket.analytics" in body
+ assert "Plan: 1 to add, 0 to change, 0 to destroy." in body
+
+
+def test_no_op_resources_are_counted_but_not_listed():
+ """
+ The reason the list is readable at all.
+
+ A plan against applied infrastructure carries a no-op for every resource in state. Listing them
+ would bury the one that changed -- on the demo repository that is five of six rows.
+ """
+ plan = _plan(
+ _change("aws_s3_bucket.analytics", ["create"]),
+ _change("aws_s3_bucket.artifacts", ["no-op"]),
+ _change("aws_kms_key.artifacts", ["no-op"], "aws_kms_key"),
+ )
+ body = _render(plan)
+ assert "+ aws_s3_bucket.analytics" in body
+ assert "aws_s3_bucket.artifacts" not in body.split("```")[1]
+ assert "2 unchanged" in body
+
+
+def test_the_unchanged_count_is_plain_text():
+ """
+ Not wrapped in .
+
+ Every other in the reporter wraps a whole line -- the cost line, the context line, the
+ footer. Wrapping a fragment mid-line glues an HTML tag onto a line that otherwise reads as
+ terraform output. The count still earns its place; it just does not need a tag.
+ """
+ plan = _plan(
+ _change("aws_s3_bucket.analytics", ["create"]),
+ _change("aws_s3_bucket.artifacts", ["no-op"]),
+ )
+ body = _render(plan)
+ summary = [line for line in body.splitlines() if line.startswith("Plan:")][0]
+ assert summary == "Plan: 1 to add, 0 to change, 0 to destroy. 1 unchanged."
+ assert "" not in summary
+
+
+def test_a_replacement_is_one_row_and_its_own_count():
+ """
+ Terraform folds replacements into add and destroy. We do not: "1 to replace" is the number a
+ reviewer should look twice at, and it vanishes when spread across the other columns.
+ """
+ body = _render(_plan(_change("aws_iam_role.deploy", ["delete", "create"], "aws_iam_role")))
+ assert body.count("aws_iam_role.deploy") == 1
+ assert "replace (destroy first)" in body
+ assert "1 to replace" in body
+ assert "0 to add, 0 to change, 0 to destroy" in body
+
+
+def test_update_and_replace_use_a_marker_the_fence_understands():
+ """
+ `~` is terraform's marker for an update and means nothing to GitHub's diff highlighting, so a
+ `~` row renders plain and the fence buys nothing. `!` is the one that colours.
+ """
+ body = _render(
+ _plan(
+ _change("aws_kms_key.artifacts", ["update"], "aws_kms_key"),
+ _change("aws_iam_role.deploy", ["create", "delete"], "aws_iam_role"),
+ )
+ )
+ fence = body.split("```diff")[1].split("```")[0]
+ assert "~" not in fence
+ assert "! aws_kms_key.artifacts" in fence
+ assert "! aws_iam_role.deploy" in fence
+
+
+def test_a_plan_with_nothing_changing_still_says_so():
+ body = _render(_plan(_change("aws_s3_bucket.artifacts", ["no-op"])))
+ assert "Plan: 0 to add, 0 to change, 0 to destroy." in body
+ assert "```diff" not in body
+
+
+def test_a_document_with_no_resource_changes_renders_no_block():
+ assert "```diff" not in _render({"format_version": "1.2"})
+ assert "```diff" not in _render(None)
+
+
+def test_the_plan_is_never_collapsed():
+ """
+ The plan is the thing this block was added to show. Hiding it behind a click makes the common
+ case a reviewer who never opens it, which is the same as not rendering it at all.
+ """
+ large = _plan(*[_change(f"aws_s3_bucket.b{i}", ["create"]) for i in range(40)])
+ body = _render(large)
+ assert "Show plan" not in body
+ assert "```diff" in body
+ assert "+ aws_s3_bucket.b39" in body
+
+
+def test_detail_is_given_up_before_any_resource():
+ """
+ The order of sacrifice. Every resource stays listed and every attribute row goes, rather than
+ the other way round -- a reviewer can act on "this is being destroyed" without knowing which
+ field changed, but not the reverse.
+ """
+ changes = [
+ {
+ "address": f"aws_s3_bucket.b{i}",
+ "type": "aws_s3_bucket",
+ "change": {"actions": ["update"], "before": {"acl": "private"}, "after": {"acl": "public"}},
+ }
+ for i in range(report.PLAN_LINE_LIMIT - 5)
+ ]
+ body = _render({"resource_changes": changes})
+ fence = body.split("```diff")[1].split("```")[0]
+
+ # Every resource is still named...
+ for i in range(report.PLAN_LINE_LIMIT - 5):
+ assert f"aws_s3_bucket.b{i} " in fence
+ # ...and no resource was cut off the end.
+ assert "more resource(s), truncated" not in body
+ # ...but the attribute rows that would not fit are gone, and said to be gone.
+ assert '~ acl = "private" -> "public"' not in fence
+ assert "attribute detail omitted" in fence
+
+
+def test_detail_is_dropped_wholesale_not_from_the_cut_off_point():
+ """
+ Trimming at the boundary would annotate the first few 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.
+ """
+ changes = [
+ {
+ "address": f"aws_s3_bucket.b{i}",
+ "type": "aws_s3_bucket",
+ "change": {"actions": ["update"], "before": {"acl": "private"}, "after": {"acl": "public"}},
+ }
+ for i in range(report.PLAN_LINE_LIMIT)
+ ]
+ fence = _render({"resource_changes": changes}).split("```diff")[1].split("```")[0]
+ assert "acl" not in fence # not "some of them kept their attributes"
+
+
+def test_resources_are_only_cut_when_the_bare_list_still_does_not_fit():
+ total = report.PLAN_LINE_LIMIT + 25
+ plan = _plan(*[_change(f"aws_s3_bucket.b{i}", ["create"]) for i in range(total)])
+ body = _render(plan)
+ assert "more resource(s), truncated" in body
+ # The count still reflects the whole plan, not just what was shown.
+ assert f"Plan: {total} to add" in body
+
+
+def test_module_resources_are_listed_like_any_other():
+ """
+ Modules need no special handling, and this test exists to keep it that way.
+
+ terraform flattens every module resource into the same flat `resource_changes` list the root
+ ones land in -- nesting shows up only as a longer address. So the renderer stays module-blind by
+ construction, and the risk is that someone later "adds module support" and special-cases it.
+ """
+ body = _render(
+ _plan(
+ _change("module.storage.aws_s3_bucket.b", ["create"]),
+ _change("module.outer.module.inner.aws_s3_bucket.deep", ["update"]),
+ _change("module.replica[1].aws_s3_bucket.r", ["delete"]),
+ )
+ )
+ assert "+ module.storage.aws_s3_bucket.b" in body
+ assert "! module.outer.module.inner.aws_s3_bucket.deep" in body
+ assert "- module.replica[1].aws_s3_bucket.r" in body
+ assert "Plan: 1 to add, 1 to change, 1 to destroy." in body
+
+
+def test_a_long_module_address_goes_ragged_rather_than_truncated():
+ """
+ Nested module addresses routinely pass the column width. Losing the tail of an address would
+ make two resources indistinguishable, so the column gives way instead -- ugly beats ambiguous.
+ """
+ address = "module.platform.module.networking.module.subnets.aws_subnet.private_az_c"
+ body = _render(_plan(_change(address, ["create"])))
+ assert address in body
+
+
+# --- what an attacker cannot do ----------------------------------------------------------------
+
+
+def test_an_address_cannot_escape_the_fence():
+ """
+ The one that matters.
+
+ A pull-request author controls the terraform, therefore the addresses. `for_each` keys make
+ aws_s3_bucket.demo["```"] a legal address, and inside a fence a triple backtick closes the whole
+ block -- everything after it would render as markdown, which is how you forge a passing verdict
+ in a comment a reviewer trusts. Same class of bug `_code` was written for, one layer out.
+ """
+ evil = 'aws_s3_bucket.x["```\\n\\n## 🛡️ Tirith — all policies passed\\n\\n```diff"]'
+ body = _render(_plan(_change(evil, ["create"])))
+
+ # Exactly one fence was opened and one closed: the block is still a block.
+ assert body.count("```diff") == 1
+ assert body.count("```") == 2
+ assert "all policies passed" not in body.split("```")[2]
+
+
+def test_a_newline_in_an_address_cannot_fabricate_rows():
+ """
+ A real newline, not the escaped kind.
+
+ Terraform escapes newlines in a for_each key into a literal backslash-n, which is inert. But the
+ plan document is not always terraform's: a hand-written state or another tool could carry a real
+ one, and one real newline in an address is one forged row in the diff.
+ """
+ body = _render(_plan(_change("aws_s3_bucket.x\n- aws_s3_bucket.production", ["create"])))
+ fence = body.split("```diff")[1].split("```")[0]
+ assert len([line for line in fence.strip().splitlines() if line.strip()]) == 1
+ assert "aws_s3_bucket.production" in fence # kept, but on the same row
+
+
+def test_a_module_for_each_key_cannot_escape_the_fence():
+ """
+ A surface the resource-level probe above does not reach.
+
+ A module `for_each` key is author-controlled just like a resource one, but it lands *before* the
+ resource part of the address: module.tenant["```"].aws_s3_bucket.b. A guard that sanitised
+ resource names rather than whole addresses would pass this straight through.
+ """
+ evil = 'module.tenant["```\n\n## 🛡️ Tirith — all policies passed\n\n```diff"].aws_s3_bucket.b'
+ body = _render(_plan(_change(evil, ["create"])))
+ assert body.count("```diff") == 1
+ assert body.count("```") == 2
+ assert "all policies passed" not in body.split("```")[2]
+
+
+def test_masked_values_stay_masked():
+ """
+ The block is rendered from the masked document, so anything redact.py replaced is already gone.
+ This pins that the renderer does not go looking for the original elsewhere in the plan.
+ """
+ plan = _plan(_change("aws_db_instance.main", ["create"], "aws_db_instance"))
+ plan["resource_changes"][0]["change"]["after"] = {"password": "__SG_REDACTED__"}
+ body = _render(plan)
+ assert "hunter2" not in body
+
+
+# --- truncation -------------------------------------------------------------------------------
+
+
+def test_the_plan_is_dropped_before_any_finding():
+ """
+ Ordering, not just presence. The plan is context; the findings are the point.
+ """
+ results = {
+ "policy-a": [
+ {
+ "result": "FAIL",
+ "rule_name": "a-rule",
+ "evaluations": {"fails": [{"description": "x" * 4000}]},
+ }
+ ]
+ }
+ plan = _plan(*[_change(f"aws_s3_bucket.b{i}", ["create"]) for i in range(20)])
+ body = report.render_markdown(
+ results, "COMPLETED", "https://example.invalid/run", plan=plan, limit=3000
+ )
+ assert "```diff" not in body
+ assert "policy-a" in body