Skip to content

fix: reject non-plan input, report unknown costs instead of guessing, expose Action outputs - #1

Merged
moveeeax merged 1 commit into
mainfrom
fix/plan-validation-unknown-pricing-and-action-outputs
Jul 25, 2026
Merged

fix: reject non-plan input, report unknown costs instead of guessing, expose Action outputs#1
moveeeax merged 1 commit into
mainfrom
fix/plan-validation-unknown-pricing-and-action-outputs

Conversation

@moveeeax

Copy link
Copy Markdown
Owner

Six confirmed defects, all reproduced against main before fixing. The theme: the tool could deliver a wrong number confidently, and the Action exposed nothing to the calling workflow.

1. Any valid JSON was accepted as a plan

terraform show -json of state (rather than of a plan file), or a truncated artifact download, parsed cleanly to zero changes. On main:

$ tf-cost-diff --plan state.json --threshold 10
### 💸 Terraform monthly cost estimate: **$0.00/mo**
_No cost-relevant changes detected._
exit=0

A reassuring "nothing changed" comment on a plan that was never read. Plan documents are now validated; bad input exits 2 naming the likely cause.

2. A missing price silently became an invented number

aws_instance fell back to a guessed $0.05/hr for any unrecognised instance_type. An m5.24xlarge really costs ~$3,363/mo:

$ tf-cost-diff --plan big.json --threshold 100     # m5.24xlarge
### 💸 Terraform monthly cost estimate: **+$36.50/mo**
exit=0        # sailed straight through a $100 gate

Worse, an attribute not yet known at plan time (Terraform writes null) hit the same fallback: instance_type: null fabricated $36.50/mo out of nothing, while size: null on an EBS volume silently became $0.00.

Estimators now return None for unknown. Such resources go in a separate Cost unknown table, are excluded from the total, and the headline is marked (partial) with unpriced/complete outputs and a ::warning::.

3. NaN in a price sheet silently disabled the threshold gate

json.load accepts NaN/Infinity, and every comparison against NaN is False:

$ tf-cost-diff --plan p.json --price-sheet nan.json --threshold 1
### 💸 Terraform monthly cost estimate: **$nan/mo**
exit=0        # expected 1

Non-finite JSON constants and a non-finite --threshold are now rejected. Price sheet values must be finite and non-negative.

4. A hostile for_each key broke out of the Markdown table

A backtick in a resource address closed the code span, letting <!-- comment out every row below it; pipes forged extra columns and newlines split the table. Cells are now sanitised (backticks neutralised, pipes escaped, control characters stripped, length capped), verified by asserting nothing attacker-supplied reaches the Markdown parser outside a code span.

5. Oversized comments posted nothing at all

Bodies over GitHub's 65536-char limit were rejected with HTTP 422 and a traceback — no comment on exactly the large plans people most want reviewed. The body is now fitted to the limit, keeping the largest deltas and stating how many rows were dropped; the headline total still counts every resource.

6. action.yml had drifted from the tool

  • No outputs: at all — nothing the tool computed was readable by the calling workflow. Eight outputs are now declared, written before the threshold gate so they survive the run where the gate fails.
  • working-directory: ${{ github.action_path }} made the documented relative plan: plan.json resolve inside the action's own checkout instead of the workspace, so the action could not work as documented. Replaced with PYTHONPATH.
  • Inputs were interpolated straight into the run: script; they now arrive via env:.

Also

  • Paginate the sticky-comment search — with >100 comments on a PR the sticky was never found and a duplicate was posted every run.
  • 30s API timeout (a hung call previously hung the job indefinitely).
  • Comment posting is best effort: a fork PR gets a read-only GITHUB_TOKEN, and failing to comment must not mask the threshold verdict.
  • Money rounds to cents throughout, so table rows add up to the headline total.

How verified

Tests go 6 → 46, including contract tests over action.yml that fail if the declared outputs drift from what the CLI writes, or if working-directory/script interpolation come back.

$ python3 -m venv /tmp/venv2 && /tmp/venv2/bin/pip install -e '.[dev]'
$ /tmp/venv2/bin/python -m pytest
46 passed in 0.05s

Each fix was proven to be load-bearing by reverting it in a scratch copy and confirming the matching tests fail:

Reverted Failing tests
guessed price fallback 4
plan validation 1
cell sanitising + size cap 3
outputs: block 2
working-directory + interpolation 2

The composite step was also run end-to-end against examples/plan.json from a simulated workspace: the relative plan path resolves, $GITHUB_OUTPUT is populated on both the passing and the gate-failing run, and the rendered figures match the README example (+$97.96/mo) exactly.

No public interface was removed: all existing CLI flags, output names and the six built-in priced resource types are unchanged. --github-output and --version are additive. Behaviour changes are the bug fixes above — input that was previously accepted-and-misreported is now rejected loudly.

🤖 Generated with Claude Code

… expose Action outputs

The tool could report a confidently wrong number, and the Action exposed
nothing to the calling workflow. Six confirmed defects:

- Any valid JSON parsed as a plan. Feeding it `terraform show -json` of
  *state*, or a truncated artifact download, rendered a reassuring
  '$0.00/mo - No cost-relevant changes detected' comment and exited 0.
  Plan documents are now validated and bad input exits 2 with a message
  naming the likely cause.

- Unknown prices became invented numbers. An unrecognised instance_type
  fell back to a guessed $0.05/hr, so an m5.24xlarge (~$3,363/mo) was
  reported as $36.50/mo and passed a --threshold 100 gate. An attribute
  not known until apply (Terraform writes null) became $0.00 silently.
  Estimators now return None for unknown; such resources are listed in a
  'Cost unknown' table, excluded from the total, and the headline is
  marked (partial).

- NaN/Infinity in a price sheet disabled the threshold gate. json.load
  accepts them, every comparison against NaN is False, so the run exited
  0 while rendering '$nan'. Non-finite JSON constants and non-finite
  --threshold values are now rejected.

- A hostile for_each key broke out of the Markdown table. A backtick
  closed the code span, letting '<!--' comment out every row below it;
  pipes forged columns and newlines split the table. Cells are now
  sanitised.

- Comments over GitHub's 65536-char limit were rejected with HTTP 422 and
  a traceback, posting nothing on exactly the large plans people most
  want reviewed. The body is now fitted, keeping the biggest deltas.

- action.yml declared no outputs:, so nothing the tool computed was
  readable by the workflow, and working-directory: ${{ github.action_path }}
  made the documented relative 'plan: plan.json' resolve inside the action
  checkout rather than the workspace - the action could not work as
  documented. Eight outputs are now declared and written before the
  threshold gate; inputs reach the script via env: instead of being
  interpolated into it.

Also: paginate the sticky-comment search (a PR with >100 comments got a
duplicate comment every run), add a 30s API timeout, treat comment
posting as best effort so fork PRs with a read-only token still gate, and
round money to cents so table rows add up to the headline total.

Tests go 6 -> 46, including action.yml/CLI contract tests that fail if the
outputs drift apart again. Each fix was verified to fail its test when
reverted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@moveeeax
moveeeax merged commit 5d3c3b8 into main Jul 25, 2026
4 checks passed
@moveeeax
moveeeax deleted the fix/plan-validation-unknown-pricing-and-action-outputs branch July 25, 2026 21:01
moveeeax added a commit that referenced this pull request Jul 26, 2026
… expose Action outputs (#1)

The tool could report a confidently wrong number, and the Action exposed
nothing to the calling workflow. Six confirmed defects:

- Any valid JSON parsed as a plan. Feeding it `terraform show -json` of
  *state*, or a truncated artifact download, rendered a reassuring
  '$0.00/mo - No cost-relevant changes detected' comment and exited 0.
  Plan documents are now validated and bad input exits 2 with a message
  naming the likely cause.

- Unknown prices became invented numbers. An unrecognised instance_type
  fell back to a guessed $0.05/hr, so an m5.24xlarge (~$3,363/mo) was
  reported as $36.50/mo and passed a --threshold 100 gate. An attribute
  not known until apply (Terraform writes null) became $0.00 silently.
  Estimators now return None for unknown; such resources are listed in a
  'Cost unknown' table, excluded from the total, and the headline is
  marked (partial).

- NaN/Infinity in a price sheet disabled the threshold gate. json.load
  accepts them, every comparison against NaN is False, so the run exited
  0 while rendering '$nan'. Non-finite JSON constants and non-finite
  --threshold values are now rejected.

- A hostile for_each key broke out of the Markdown table. A backtick
  closed the code span, letting '<!--' comment out every row below it;
  pipes forged columns and newlines split the table. Cells are now
  sanitised.

- Comments over GitHub's 65536-char limit were rejected with HTTP 422 and
  a traceback, posting nothing on exactly the large plans people most
  want reviewed. The body is now fitted, keeping the biggest deltas.

- action.yml declared no outputs:, so nothing the tool computed was
  readable by the workflow, and working-directory: ${{ github.action_path }}
  made the documented relative 'plan: plan.json' resolve inside the action
  checkout rather than the workspace - the action could not work as
  documented. Eight outputs are now declared and written before the
  threshold gate; inputs reach the script via env: instead of being
  interpolated into it.

Also: paginate the sticky-comment search (a PR with >100 comments got a
duplicate comment every run), add a 30s API timeout, treat comment
posting as best effort so fork PRs with a read-only token still gate, and
round money to cents so table rows add up to the headline total.

Tests go 6 -> 46, including action.yml/CLI contract tests that fail if the
outputs drift apart again. Each fix was verified to fail its test when
reverted.
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