Skip to content

[SG-4917] feat(terraform_plan): name the resource and attribute in result messages (on main) - #287

Open
refeed wants to merge 2 commits into
mainfrom
feat/result-message-context-on-main
Open

[SG-4917] feat(terraform_plan): name the resource and attribute in result messages (on main)#287
refeed wants to merge 2 commits into
mainfrom
feat/result-message-context-on-main

Conversation

@refeed

@refeed refeed commented Aug 28, 2026

Copy link
Copy Markdown
Member

Pull Request Template

The same change also exists on the releasebranch/1.0.6 branch, which is based on
1.0.5 (before tirith platform check) and is being released directly from there rather than
merged into main — see the closed #286. This is the PR for main.

Description

  • What changes are being made?

    • A result message used to state only the comparison it made. NotEquals printed `"public-read"` is not equal to `"private"` and nothing else — no resource, no attribute — so a check run across a plan with 30 buckets printed 30 identical lines. For the terraform_plan provider, every message now names the resource address, its planned action and the attribute being evaluated:

      Check: no_public_buckets
        FAILED
          1. FAILED: [aws_s3_bucket.example (create)] acl: `"public-read"` is not equal to `"private"`
          2. PASSED: [aws_s3_bucket.logs (update)] acl: `"private"` is equal to `"private"`
          3. FAILED: [aws_vpc.main (create)] attribute: 'flow_log_id' is not found
      
    • Providers may now return an optional context key alongside value / meta / err. core.generate_evaluator_result renders it into the front of the message via a new format_context_prefix() in providers/common.py, and keeps it as structured fields on the result:

      {
        "passed": false,
        "message": "[aws_s3_bucket.example (create)] acl: `\"public-read\"` is not equal to `\"private\"`",
        "context": {
          "operation_type": "attribute",
          "resource_type": "aws_s3_bucket",
          "resource_address": "aws_s3_bucket.example",
          "action": "create",
          "attribute": "acl"
        },
        "meta": { "…": "full resource_change" }
      }
    • A wildcard attribute now reports the index it resolved to. ebs_block_device.*.tags.application_acronym used to emit N results per resource that read identically; there was no way to tell which block device was the untagged one. _get_exp_attribute now tracks the traversed path:

      1. FAILED: [aws_instance.example (create)] ebs_block_device.0.tags.application_acronym: `null` is empty
      2. PASSED: [aws_instance.example (create)] ebs_block_device.1.tags.application_acronym: `"TTO"` is not empty
      3. PASSED: [aws_instance.example (create)] ebs_block_device.2.tags.application_acronym: `"TTO"` is not empty
      
    • attribute: '…' is not found now names its resource. With terraform_resource_type: "*" this error was emitted once per resource with identical text and no meta at all.

    • Could not find input value — the one message with no context whatsoever — now names the provider arguments that produced no value.

  • Why are these changes necessary?

    • The detail was already in the result document: the provider attaches the whole resource_change as meta on every output. But nothing between the provider and the pretty printer surfaced it.
    • This is the same gap the TUI's Explorer was built to close, from the other side. The Explorer's changelog entry says it plainly: "the pretty printer prints only the message, so this detail was reachable only by piping --json into another tool." The Explorer makes the detail browsable; this puts it in the message, so a CI log is legible without opening anything. They compose rather than overlap — the context field also gives the Explorer and platform/report.py a ready-made label instead of re-deriving one from meta.
  • Which issues or tickets does this PR close or relate to?

Type of Change

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Other (please specify):

Checklist

  • Code has been linted.
  • Documentation has been updated (if needed).
  • Tests have been added or updated (if needed).
  • Any breaking changes have been communicated to the team.

Additional Information

Design — why the context is composed in the core, not in the evaluators. BaseEvaluator.evaluate(evaluator_input, evaluator_data) receives two bare values and nothing else, and all 13 evaluators have exact-dict unit tests. Threading context into each of them would mean rewriting every one of those tests for no gain, so the prefix is composed once in generate_evaluator_result from whatever context the provider chose to attach.

Only terraform_plan populates context here. json, kubernetes, infracost and sg_workflow return outputs without the key, so their messages are unchanged and the context field is omitted from their results entirely. Verified by diffing --json output for all 7 of their fixtures against 1.0.5: byte-for-byte identical. tests/core/test_output_compatibility.py and its golden file pass untouched.

Provider-argument errors stay uncontextualised (severity_value: 99, resource_type: '…' is not found) — they are about the policy, not about a resource, so there is nothing to name.

Verification

  • pytest: 774 passed. The 11 failures are pre-existing on main and all need a terraform binary on PATH (test_direct_references.py, test_e2e_inline_vars); CI has one, so those paths are covered there — on [SG-4917] feat(terraform_plan): name the resource and attribute in result messages #286 they passed on all five interpreters.
  • tests/core/test_output_compatibility.py (golden --json) and tests/test_readme_is_current.py both pass unmodified.
  • black --check src tests clean under black 25.9.0, which main is also clean under. Under the newest black (26.5.1) this branch adds zero complaints beyond the 13 files already flagged on main — that job fails on main today for the same reason, since psf/black@stable moved forward.
  • pydocstyle clean.
  • tests/providers/terraform_plan/test_dot_star_attr.py is untouched and still green — it calls _wrapper_get_exp_attribute directly with 6 parametrized flat-list assertions, which is the regression guard for the traversal refactor. That wrapper keeps its old signature and return value; the path-tracking variant is a sibling.
  • New: tests/providers/terraform_plan/test_message_context.py (10 tests, policies built inline so they don't depend on a fixture) and format_context_prefix cases in tests/providers/test_common.py. tests/providers/kubernetes/test_attribute.py and tests/providers/json/test_get_value.py gained an assertion each pinning the "other providers unchanged" contract.

A result message only stated the comparison it made, so a check run across a
plan with 30 buckets printed 30 identical lines with no way to tell which
resource had failed. The detail was already in the result document - the
provider attaches the whole resource_change as `meta` on every output - but
nothing between the provider and the pretty printer surfaced it, so reaching it
meant piping --json into another tool.

Providers may now return an optional `context` alongside value/meta/err. The
core renders it into the front of the message and keeps it as structured fields
in the result document, so a failure reads:

    [aws_s3_bucket.example (create)] acl: `"public-read"` is not equal to `"private"`

Only terraform_plan populates it, so every other provider's messages are
unchanged. Two related improvements come with it: a wildcard attribute reports
the index it resolved to (ebs_block_device.0.tags.application_acronym), which
tells apart results that used to read identically, and an "attribute is not
found" error names the resource it is about instead of repeating the same text
once per resource.
@notion-workspace

Copy link
Copy Markdown

@codecov

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.76923% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/tirith/core/core.py 78.94% 2 Missing and 2 partials ⚠️
src/tirith/providers/terraform_plan/handler.py 93.93% 2 Missing ⚠️
Files with missing lines Coverage Δ
src/tirith/providers/common.py 98.48% <100.00%> (+0.33%) ⬆️
src/tirith/providers/terraform_plan/handler.py 68.88% <93.93%> (+2.48%) ⬆️
src/tirith/core/core.py 85.37% <78.94%> (-0.85%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…urce type

A real destroy-approval gate is one `action` evaluator with
`terraform_resource_type: "*"`, so it emits a result per resource and eleven of
twelve lines read `"no-op"` is not equal to `"delete"` byte for byte. The one
line that matters is buried in them and names nothing, so finding the resource
that is about to be destroyed meant counting to the same position in
`resource_changes`.

Fixtures for that shape - a plan of mostly untouched infrastructure, a few
additions and one resource on its way out - plus the passing variant where
nothing is being destroyed.
@sonarqubecloud

Copy link
Copy Markdown

❌ The last analysis has failed.

See analysis details on SonarQube Cloud

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.

Tirith evaluator message should be more descriptive

1 participant