[SG-4885] feat(platform): tirith platform check — a pre-plan policy step, no platform changes - #272
[SG-4885] feat(platform): tirith platform check — a pre-plan policy step, no platform changes#272refeed wants to merge 31 commits into
tirith platform check — a pre-plan policy step, no platform changes#272Conversation
Fixed:
- Variable substitution mutated the caller's policy dict. Evaluating the same
parsed policy twice (a policy set, or a retry) leaked substituted values from
one evaluation into the next.
- An unsupported condition.type returned without setting result["result"],
raising KeyError in the pretty printer far from the real cause. The consumer
is hardened with .get("result", []) as well.
- Provider errors reported without a ProviderError severity were discarded and
None was evaluated against the condition, so a typo'd operation_type read as
a genuine policy violation. Five sites across four providers were affected.
These are malformed provider calls, so they deliberately bypass
error_tolerance -- that setting exists to tolerate missing data, not to mask
a broken policy.
Added:
- meta.id/name/description/severity/enforcement/tags/remediation now reach the
result document when declared. Absent keys are omitted, so output for a
policy declaring none of them is unchanged.
Backward compatibility is pinned by tests/golden/json_policy_output.json,
captured before these changes and asserted byte-identical after them.
Runs an organization's policies against a plan, state or arbitrary JSON
document from CI or a laptop: masks the document locally, packs it with the
terraform source into an archive, uploads it, creates a StackGuardian run, polls
it and reports the verdict as JSON and/or markdown.
This moves the StackGuardian protocol out of the GitHub Action, where it was
GitHub-only, untestable off a runner, and unavailable to anyone driving the
platform from GitLab or a Makefile. No new runtime dependencies -- the whole
thing is stdlib urllib, so a runner needs nothing beyond tirith itself.
Subcommands are dispatched before the flat parser sees anything. argparse cannot
express an optional subcommand alongside options like `-policy-path`, and the
local-evaluation surface is a contract that test_output_compatibility.py asserts
byte-for-byte. Also fixes cli.main(args=...), which was ignored because
parse_args() was called with no argument.
Two bugs found while writing this:
* APPROVAL_REQUIRED was missing from the poller's terminal statuses. It is a
resting state, so a run that reached it spun until the timeout and was then
reported as a tool failure -- an outage, rather than a finished evaluation
waiting on a human. It now yields an `approval-required` verdict.
* A file named state.json in the working directory was packed raw.
`terraform state pull > state.json` is the documented way to produce one, so
it routinely sits there unmasked, and it shipped in full beside the masked
copy. plan.json / state.json / infracost.json are now always written by
pack() from an already-masked object and never copied from the source tree.
Exit codes: 0 clean, 3 for a policy failure under --fail-on-error, 1 for an
unreachable platform or a run that produced no verdict -- the last regardless of
the flag, because a run with no verdict must never look like a pass.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new tirith platform check subcommand that runs StackGuardian policy evaluations against a plan/state/JSON document by packaging masked inputs + Terraform source into an archive, creating/polling a StackGuardian run, and emitting JSON/markdown verdict output. It also tightens several core/CLI behaviors to preserve existing output contracts and avoid previously observed failure/leak modes.
Changes:
- Add a stdlib-only StackGuardian “platform” integration (
client,check,archive,redact,report) plus extensive tests for polling, masking, archiving, and rendering. - Add CLI subcommand pre-dispatch (
tirith platform ...) while preserving the legacy flat CLI surface and byte-identical--jsonoutput compatibility. - Fix core behaviors (policy var substitution mutability, unsupported evaluator result shape, provider bare error surfacing) and bump version/changelog.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/platform/test_report.py | Tests for verdict computation and markdown rendering/truncation behavior. |
| tests/platform/test_redact.py | Security-focused tests asserting redaction on serialized bytes for plan/state. |
| tests/platform/test_client.py | Tests for StackGuardian client polling/terminal states and upload behavior. |
| tests/platform/test_archive.py | Tests archive contents/exclusions and ensures masked docs win over disk files. |
| tests/golden/json_policy_output.json | Golden output fixture used to pin legacy JSON byte compatibility. |
| tests/core/test_policy_parameterization.py | Adds regression tests ensuring var substitution doesn’t mutate caller policy dict. |
| tests/core/test_output_compatibility.py | New contract tests ensuring stable output shape/bytes for consumers. |
| tests/core/test_core.py | Adds tests for unsupported evaluator result shape and provider bare error surfacing. |
| tests/cli/test_dispatch.py | Tests for subcommand dispatch without breaking legacy flat CLI contract. |
| src/tirith/status.py | Adds distinct exit code for policy-failed outcomes under --fail-on-error. |
| src/tirith/prettyprinter.py | Avoids KeyError by tolerating missing result key in evaluator output. |
| src/tirith/platform/report.py | Implements result summarization, verdict mapping, and markdown rendering. |
| src/tirith/platform/redact.py | Implements plan slimming + marker-driven redaction and state masking. |
| src/tirith/platform/client.py | Implements stdlib-only StackGuardian API client including polling and artifact fetch. |
| src/tirith/platform/cli.py | Implements tirith platform argparse surface and exit-code semantics. |
| src/tirith/platform/check.py | Orchestrates read→mask→pack→upload→run→poll→fetch→report flow. |
| src/tirith/platform/archive.py | Builds tar.gz archive with exclusions and reserved-name handling. |
| src/tirith/platform/init.py | Introduces platform package with stdlib-only intent documented. |
| src/tirith/core/policy_parameterization.py | Switches var substitution to operate on a deep copy to avoid mutation leaks. |
| src/tirith/core/core.py | Ensures unsupported evaluator still populates result; passes through policy meta keys. |
| src/tirith/cli.py | Adds pre-dispatch for subcommands and fixes main(args=...) honoring provided argv. |
| src/tirith/init.py | Version bump to 1.2.0. |
| setup.py | Updates package version to 1.2.0. |
| CHANGELOG.md | Documents 1.2.0 release changes and notes/contracts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if counts.get(WARN) or counts.get(APPROVAL_REQUIRED): | ||
| return "warned" | ||
| if counts.get(PASS) or counts.get("SKIPPED"): | ||
| return "passed" |
| # The masked documents are written separately and must win. | ||
| if relative in reserved_names: | ||
| skipped += 1 | ||
| continue |
| f"The upload response for {filename} carried no storage key. The platform may " | ||
| f"predate the configuration_upload_url endpoint. Response: {payload}" | ||
| ) | ||
| signed_url = _extract_signed_url({"msg": msg.get("signedUrl")}) |
A third instance of the `planned_values` pattern, caught by a live GitHub Action run: a hardcoded value is masked in `resource_changes` and sits in plaintext in the same document under `configuration.root_module.resources[].expressions[].constant_value`, which carries no sensitivity markers at all. `configuration` cannot be dropped -- three operations read it -- so the literals are scrubbed while the reference graph is kept. Lossless: direct_references_operator reads only `references` and direct_dependencies_operator only `depends_on` (providers/terraform_plan/handler.py:329, :385-388). Covers nested block arguments, repeated blocks (a list of expressions), child modules via module_calls[].module, and variable `default` / output `expression` literals. Note this does not make a plan safe to hand out: the project archive carries the terraform source as written, so a secret hardcoded in HCL still reaches the platform in main.tf. Documented in the action's README rather than papered over.
…c tfstate.json
sensitive_attributes is a list of PATHS -- each entry is itself a list of steps:
[[{"type": "get_attr", "value": "content_base64"}],
[{"type": "get_attr", "value": "content"}]]
The code read only the flat forms, so on real state every entry was skipped: a
list is neither a dict nor a string. Nothing in a resource's attributes was
masked at all. The unit test passed because its fixture invented the flat shape;
verified now against `terraform state pull` output for a local_sensitive_file,
which is where the real shape came from.
Paths can also descend through nested objects and list indices, so the masker
walks them rather than assuming a single key, and deep-copies so the caller's
document is not mutated underneath it.
Renames the archive's state document from state.json to tfstate.json, matching
the TfStateCleaned fact it feeds and the name the terraform step already uses
for state. No collision: the archive unpacks into the user directory, while
managed state lives at the artifacts root, and policy-only forces
managedTerraformState off.
A rule result of APPROVAL_REQUIRED means its author wrote `onFail: APPROVAL_REQUIRED`. The policy-only step records that without pausing the run -- deliberately, since exit 11 would leave the poller spinning -- so the run comes back COMPLETED and only the counts carry the intent. Folding it into `warned` was wrong. `warned` maps to a `neutral` check, which SATISFIES a required status check, so a policy demanding human sign-off silently did not block. Ranked above `warned` it produces the `approval-required` verdict, which the action maps to `action_required` -- honouring the author's intent without implementing the approval workflow, which is out of scope here. Caught by a live run against a real APPROVAL_REQUIRED policy: the rule reported correctly and the verdict said `warned`, so the code handling `approval-required` was unreachable from this path.
refeed
changed the title
tirith platform checktirith platform check
… endpoint
Four changes to make `tirith platform check` runnable with no configuration, and to stop the
CLI depending on an endpoint that is being withdrawn.
regions.py replaces four hardcoded host literals with one table. --region names both URLs at
once, because setting only --api-url was leaving every run link in every PR comment pointing
at the wrong environment -- which reads as a broken integration rather than a
misconfiguration. Explicit URLs still win, permanently, since they are the only way to reach
a self-hosted or dedicated host. Combining --region with an explicit URL is an error rather
than a silent precedence rule. by_id raises on an unknown id instead of falling back to the
first region the way the Raycast extension does: a typo would otherwise point a US org at
production EU and surface only as an unexplainable auth error.
normalize_api_url accepts a base with or without /api/v1. tirith's flag has always included
it while sg-cli, Raycast and the terraform provider all omit it, so a SG_BASE_URL exported
for sg-cli produced 404s here.
discover.py finds plan.json or tfplan.json in the source directory when nothing is named, so
a caller in the conventional layout needs no flags at all. Two matches is an error rather
than "first one wins" -- silently evaluating the wrong document reports a verdict about
infrastructure nobody asked about, and it looks like a pass. --plan-file renders a binary
plan through `terraform show -json` straight into the masker, so no unmasked plan JSON is
written to disk. Binary resolution tries terraform-bin and tofu-bin BEFORE terraform and
tofu: setup-terraform installs a JS wrapper under the plain name whose setOutput('stdout')
would copy the entire plan into $GITHUB_OUTPUT, readable by every later step in the job.
test_the_plan_never_reaches_github_output pins that.
--workflow-id is now validated against the platform's own slug rule before any HTTP call.
It is interpolated unquoted into every API path, so a value like `live/prod/vpc` produced a
malformed URL rather than a usable error; the message suggests a slug that would work.
upload_archive moves from configuration_upload_url to file_upload_url, which is the same view
and the same core call and already produces a byte-identical key -- confirmed against QA. The
key now comes from `data.key` rather than a bespoke `msg` object, so `msg` stays the bare URL
string every other consumer reads. contentType is requested explicitly so the signature
matches the PUT header.
The archive uploads as `__sg.<tag>.tar.gz`. The prefix is load-bearing: the artifact prefix is
synced into every subsequent run of the workflow and re-uploaded with no --delete, so an
unexcluded name accumulates forever. `sg.` is not enough -- the awscli patterns match the key
relative to the sync source and the archive sits under a per-commit folder, so only the
`*__sg.*` / `*/__sg.*` patterns catch it at that depth.
refeed
changed the title
tirith platform checktirith platform check — region key, document discovery, shared upload endpoint
…r the run
Three changes, all about what is left behind.
The run facts become the primary source of policy results, and the results artifact is only
consulted when the facts come back empty -- i.e. an older step image that still writes it.
That reverses the previous order, which existed only because the facts endpoint answered
"does not exist" for every run. It turned out to be a key mismatch in the run controller
rather than a missing record.
Fixing that exposed a second bug: get_policy_results read `body.get("signedUrl")` while the
endpoint returns `signed_url`, so the facts path always fell through to {}. It went unnoticed
for exactly as long as the results artifact was covering for it. Now goes through
_extract_signed_url, which already handles both spellings.
The project archive is deleted once the run reaches a terminal state. Nothing prunes the
artifact prefix -- there is no lifecycle rule and neither sync passes --delete -- so an
archive left behind is one permanent object per commit, per workflow, forever. Measured on the
QA e2e workflow: 27 permanent directories, 10 of them archives, all pulled into every later
run's working directory.
That required flattening the archive name from `<sha7>/__sg.<tag>.tar.gz` to
`__sg.<sha7>-<tag>.tar.gz`. Not cosmetic: a nested name is swallowed by the authorizer's
greedy <path:wfGrp> converter, so `DELETE .../artifacts/<sha7>/<name>/` matches
`DELETE .../wfgrps/<wfGrp>/` -- the workflow-group delete -- and is checked against entirely
the wrong permission. Verified against auth's own matcher. Keeping the sha and tag in the
filename preserves uniqueness, so two pull requests uploading concurrently still cannot
overwrite each other's archive before their runs start. Deletion is best-effort: it happens
after the verdict is known, so a failure warns and changes nothing.
--repo-url and --repo-ref record the source repository on the workflow via GIT_OTHER -- the
connector-less provider, which with isPrivate false needs no auth and skips the GitHub repo-id
extraction that rejects anything it cannot parse. It is metadata only: core pops iacVCSConfig
from the run's RuntimeParameters whenever terraformProjectZip is set, and the runner takes the
archive branch of its if/elif regardless. Set on creation only, so a workflow that already
exists keeps its blank repo field.
urlencode stringifies None to the literal "None", and the endpoint treats any non-empty folder as a subfolder -- so the archive landed at .../artifacts/None/__sg.<sha>-<tag>.tar.gz. Two consequences, both silent: a bogus None/ directory in the workflow's artifact prefix, and a nested key that the post-run delete could not address, so cleanup no-opped on a 404 and the archive persisted anyway. Caught on a live QA run. The folder is now sent only when set; the archive passes none, which is what puts it at the artifacts root where it can be deleted.
Update: artifact cleanup, source repo, and a live E2EThree additions since the last review, plus a full end-to-end run on QA against a freshly created private repo using the zero-config invocation. Run facts are now the primary source of results
With that fixed, the step stops writing The project archive is deleted after the runNothing prunes the artifact prefix: no lifecycle rule, and neither sync passes The archive name flattens from
Uniqueness moves from the folder into the filename, so two PRs uploading concurrently still cannot collide.
|
| policy | rule | result |
|---|---|---|
DO_NOT_TOUCH |
cost-control | PASS |
best-practices |
Policy-Rule-1 | WARN |
tirith-e2e-must-fail |
no-null-resources | FAIL |
Check run Tirith Policy: failure — 1 failed, 1 warned, 1 passed; sticky comment rendered with per-rule detail and the failing resource address (null_resource.untagged). Job stayed green because fail-on-error defaults false.
The artifact prefix after the run:
sub-prefixes: (none)
objects: (none)
Empty. Workflow record confirms GIT_OTHER | https://github.com/refeed/tirith-e2e-08050726 | ref = add-storage, WfType: TERRAFORM, WfStepsConfig: [], action policy-only.
One caveat, measured rather than predicted
On a private repo the async repo-insights/security-scan lambda that fires on workflow creation settles at scan_status: "error" (not in_progress as I guessed), with "Something went wrong while scanning your repository". It cannot fail the create — separate thread, broad except — but it is user-visible on the workflow. Worth deciding whether to suppress it for archive-based workflows.
Deployed to QA and verified live: file_upload_url returns data.key, configuration_upload_url is 404, an unsupported contentType is rejected.
422 passed in tirith, 26 in the action, 95 in the step.
"policy-only" described what the action does not do. "tirith-check" names the thing it runs, matches the CLI subcommand (tirith platform check) and the action users add to their workflow, so the same word appears at every layer. Nothing has shipped under the old name -- it exists only on these branches and in QA test runs -- so there is no alias and no migration. The action is a per-run RuntimeParameter, not stored on the workflow, so existing workflows simply get the new value on their next run.
…d show cost in the comment
Infracost and Checkov read `planned_values` and nothing else. The masker drops terraform's
copy -- correctly, because it mirrors every value with NO sensitivity markers, so masking
`resource_changes` leaves the same secret in plaintext there, and a real plan leaked a
`local_sensitive_file` body through exactly that path.
The consequence was that both tools returned a clean, empty and entirely wrong answer.
Measured against infracost 0.10.27 with a real API key, same binary, same plan, differing only
by this section:
with planned_values totalMonthlyCost 39.8 1 priced resource
without (what we ship) totalMonthlyCost 0 0 priced resources
So the estimate was never a key problem. QA's image key works -- the last run returned
well-formed infracost JSON with no error, just nothing in it.
redact_plan now rebuilds `planned_values` from the *masked* `resource_changes`, after
_mask_by_marker has run. Same data, same shape, no unmarked copy. Only `after`, and only for
resources that will exist: a destroy has no planned value. Module resources are grouped under
`child_modules`; verified that flat and nested forms price identically, and both tools address
resources by the full `address`, which already encodes the module path.
The pull-request comment now carries a cost line, with the delta from the change when infracost
supplies one. Rendered even at zero or on failure, because silence is indistinguishable from
"this change costs nothing" -- very different things to tell a reviewer. It sits outside the
truncation path, so a wall of findings cannot push it out of the comment. Also surfaced as
`monthly_cost` in --output-json for a caller aggregating several units.
client.get_run_facts replaces the narrower get_policy_results as the fetch: the document
carries the verdict and the cost, and embeds the whole plan, so fetching it twice is worth
avoiding. get_policy_results stays as a thin accessor.
196 tests pass, 17 new -- including that the rebuilt section carries __SG_REDACTED__ rather
than the secret, and that terraform's original copy is replaced rather than merged.
A Checkov policy rendered as `❌ best-practices › Policy-Rule-1` with an entirely blank
<details> body -- twelve real findings (EC2 detailed monitoring, EBS encryption, IMDSv1, S3
KMS encryption) reduced to nothing, in the one place a reviewer looks. The verdict was right;
the reasons were invisible.
_extract_detail only understood tirith's shape: a list under `result`, each carrying `message`
and `meta.address`. Checkov entries are `{"description", "keys"}`, so every loop found nothing
and appended nothing. Both shapes now render.
`keys` are reduced to the resource address: Checkov reports `<type>.<name>.<attribute path>`
and the path can be arbitrarily deep, so
`aws_s3_bucket.data.rule.apply_server_side_encryption_by_default.sse_algorithm` becomes
`aws_s3_bucket.data`. The suffix is what the check inspected; the address is what a reviewer
navigates by, and reducing it also collapses several keys on one resource to a single entry.
Tests use the exact payload from QA run iqkxb26uzi1n rather than an invented fixture -- a
fixture is what let this through, since the renderer was only ever exercised against the shape
it already understood. Malformed keys are parametrized, and two tests pin that the tirith
shape and the engine-error path still work.
Also adds CHANGELOG_2026-08-05.md and updates the roadmap: the facts table now reflects that
PolicyEvalResults comes from the run facts rather than a per-run artifact, that Infracost is
written on every run, and that TfStateCleaned is deliberately not written by tirith-check.
…e no longer true Updated against what is now verified on QA rather than what was true when it was written: - TfStateCleaned moves from⚠️ "deliberately not written" to ✅. A post-apply check now updates the workflow's Resources view. The reasoning that kept it out was half right: the shape mismatch was real and is what the conversion fixes; the workflow-scoped pointer is the *intent* for a post-apply check, not a hazard. - Infracost moves from ⚪ "not exercised" to ✅ generated on every run. - The archive is now flat and deleted after the run, so the "where it lands" row said something that stopped being true. - A new section records the two-phase pipeline with the facts each phase writes, and why a policy with no document on one pass reports WARN. Three corrections rather than additions: - "TfStateCleaned and TfPlan are unreachable" was the old symptom of the wfrunfacts bug. Both are reachable; the bug is that wfrunfacts 404s on shared-ec2, and its scope is narrower than first described -- external.py was never affected, which is why the E2E kept working after the fixes were reverted out of this batch. - The Infracost `$0` finding is added to the ship-blocking table with the evidence that isolates it to the image's key: the same plan prices at $35.99 locally, and an invalid key reproduces QA's output exactly while a missing key errors loudly. - Residual `policy-only` references renamed. Also adds CHANGELOG_2026-08-05.md: everything that changed today, each item linked to the run that proves it.
The archive is the source that produced the findings, and another system reads it to generate autofixes. Deleting it after the run removed the only copy of what was actually evaluated. Retaining it is safe for the runs themselves: the `__sg.` prefix keeps it out of the per-run artifact sync, so it never lands in a later run's working directory -- which was the problem worth solving. It is not free, and the code says so: nothing prunes this prefix, so it is one object per commit and tag, kept indefinitely, and it wants an S3 lifecycle rule. No fact is written to point at it, because the pointer already exists. The key is on the run record as RuntimeParameters.terraformProjectZip, verified on a live QA run, so a consumer holding only a run id can reach the bundle with no platform change and nothing duplicated: GET .../wfruns/<id>/ -> RuntimeParameters.terraformProjectZip GET .../wfs/<wf>/get_artifact/?artifactPath=<basename> -> the bytes GET .../wfruns/<id>/wfrunfacts/default/ -> PolicyEvalResults The plan called for recording the key in SGCustomWorkflowRunFacts. That is dropped: it would copy data already on the record into a second place that can disagree with it, and the step cannot see terraformProjectZip anyway -- only wfStepInputData reaches the container, so it would have needed a core change to carry a value the consumer can already read. `archive_key` is added to --output-json for a caller that has the result document in hand. `client.delete_artifact` stays: it is tested, and a retention sweep will want it. Note for consumers: the archive holds the masked plan and, only when `source-dir` is set, the terraform source. The default ships no source, so autofix callers must set it or they will get a bundle with nothing to fix.
A state document uploaded with --state-path was only reachable by unpacking the
run's archive, so it appeared in neither the State view nor the artifacts list.
It is now also written to `artifacts/tfstate.json`.
That name is canonical rather than chosen: the managed-state backend writes it,
state locking keys on the literal basename, and the state-backends listing
special-cases it. So no new API endpoint is needed either -- `tfstate_upload_url`
and `file_upload_url` are the same view, and its default filename is already
`tfstate.json`.
Unlike the archive this object is deliberately NOT `__sg.`-prefixed: it is meant
to be seen.
Two guards, because the same property that makes the name useful makes it
dangerous:
* If the workflow manages its own terraform state, the upload is skipped. For
such a workflow that object IS the live state, and writing a masked document
over it is data loss. An unreadable answer counts as managed -- absent is not
the same as false, and not being able to tell is not a reason to overwrite.
* The log says the published copy is masked and cannot be used to run
terraform. A file at the canonical state key full of __SG_REDACTED__ is a
footgun for whoever downloads it next.
The upload is best-effort: a run whose policies evaluated correctly must not go
red because a convenience copy could not be written.
`upload_archive` becomes `upload_file` with a content type, since a JSON state
document cannot be sent with the archive's `application/gzip` -- S3 signs the
content type into the URL. Its body parameter is named `content`: calling it
`payload` shadowed the response variable and sent the JSON response to S3 in
place of the file, which an existing test caught.
376 tests pass, 10 new.
The terraform source is packed by default, so an exclusion that does not fire --
a committed vendor directory, a build output tree -- turned a working policy
check into a failed run. `archive.pack` raises above 100 MB gzipped and nothing
caught it: the pack call sat outside run_check's try block.
That trade is the wrong way round. The verdict gates the merge; the source is a
convenience for whatever reads the bundle afterwards. So an oversized archive now
degrades to documents-only and says so, loudly, instead of taking the check down
with it.
Only when a source tree was actually requested. Already documents-only and still
over the limit means the *documents* are too big and there is nothing left to
drop, so that stays fatal -- uploading an archive with no documents is not a
check at all.
The result document records `source_packed` and `source_skipped_reason`, because
"the bundle has no code" and "no code was wanted" have to be distinguishable by
a consumer that only has the document. The GitHub annotation is raised by the
action, not here: this module stays VCS-agnostic so a GitLab or Jenkins caller
reuses it unchanged.
Two things fixed while in here:
* The size message reported anything under a megabyte as "0 MB, over the 0 MB
limit" from integer division. It is now human-readable, which matters because
the message is surfaced on a pull request.
* MAX_ARCHIVE_BYTES is overridable via TIRITH_MAX_ARCHIVE_BYTES. With the
source packed by default, the only other lever was dropping it entirely, so a
large monorepo that genuinely needs to ship its code had nowhere to go. A
non-numeric value is ignored rather than failing a run.
214 platform tests pass, 4 new.
The pull-request comment is edited in place across runs, so it shows the latest verdict and nothing else. Without naming the revision, a reader has no way to tell whether what they are looking at is about the head of the branch or about a push from an hour ago -- and the more confident the verdict reads, the worse that ambiguity is. `render_markdown` takes an optional `commit`, rendered as a subline under the headline. Doing it here rather than letting the caller append means the check-run summary and the job summary get it too, from one place. Abbreviated to seven characters, as git does -- but only when it actually looks like a hex sha. A tag or branch name is passed through whole: truncating one would produce something that looks like a sha and is not. `check.py` threads the existing `opts.sha`, which already feeds the archive name, so nothing new has to be plumbed in. 219 platform tests pass, 5 new.
Two clean-ups, both of my own making. `git add -A` in cbc397c swept in eighteen untracked files from the working tree -- an unrelated ansible/jq/jmespath exploration under tests/providers/json/ -- and 9d0cc81 did the same with two of my session notes. None of it belongs to SG-4885, and test_ansible_best_practices_jq.py fails ("operation_type: jq_query is not supported"), which is what turned this PR's unittest and coverage jobs red. Removed with `git rm --cached`: every file stays on disk exactly as it was, untracked and unchanged. Then black over the files this branch actually owns. Measured on clean checkouts rather than the working tree, because the working tree is full of untracked files that skew it: main already fails black on 14 files, so the lint job was red before this branch existed. This branch was adding nine more; those are fixed. The pre-existing fourteen are deliberately left alone -- reformatting them is a repo-wide decision, not this PR's, and it would bury the diff. 385 tests pass.
f32ff37 to
b758b52
Compare
refeed
left a comment
refeed
left a comment
There was a problem hiding this comment.
Review focused on the masking path, since a leak there is the worst outcome in this feature. Five blocking findings, four of them leaks.
Blocking
1. resource_drift is never masked. src/tirith/platform/redact.py:198
redact_plan walks resource_changes and output_changes only. resource_drift is a top-level list of the same object shape (change.before/after, before_sensitive/after_sensitive) and is neither dropped nor masked. Verified: a plan with resource_drift[0].change.before = {"password":"hunter2"} and before_sensitive={"password":true} ships hunter2 in cleartext into the archive. Any terraform plan -refresh=true against a resource whose password drifted leaks it. No test mentions resource_drift.
2. Nested output sensitivity is ignored. redact.py:319
_redact_output_change masks a whole side only when sensitive/<side>_sensitive is True. Terraform emits structured markers for structured outputs. Verified: output_changes.conn = {"after":{"url":"x","password":"s3cret"},"after_sensitive":{"password":true}} → s3cret survives. _mask_by_marker already handles this correctly; the output path just doesn't use it.
3. Raw state and plan files in the source tree are packed. archive.py:46,204
DEFAULT_EXCLUDES covers *.tfstate* only, and RESERVED_DOCUMENTS is matched against the root-relative path. Verified: state.json, tfplan-out.json and envs/plan.json all land in the tarball unmasked. So --state-path state.json --source-dir . — the exact flow the module docstring describes — uploads the masked copy as tfstate.json and the plaintext original as state.json. pack() is never told which paths were just masked.
This one got worse with the change making source-dir default to ..
4. redact_state silently no-ops on terraform show -json output. redact.py:341
That shape nests under values.root_module.resources, so neither branch fires: redaction count is 0, nothing is logged, and full plaintext state is packed and published as artifacts/tfstate.json. prepare_documents:100 warns for the inverse mistake but not this one.
5. An unreadable facts document renders as green. client.py:395 → check.py:299
get_run_facts returns {} on any non-200 and on any exception fetching the signed URL; get_results_artifact returns None on non-200. A COMPLETED run whose results cannot be fetched therefore produces empty policy_results → verdict() = no-policies → exit 0, and the comment reads "no policies in scope". That is exactly the "green when the verdict is unknown" case the design exists to prevent. {} from a transport failure must be distinguishable from {} from an empty result.
Non-blocking
- The "best-effort" state publish is not best-effort.
check.py:196—manages_terraform_stateis an HTTP call sitting outside thetry/except SGError, so a 401 or network failure there raisesCheckErrorand kills the whole check beforecreate_run. The test only fakesupload_fileraising. report.py:47—rule.get("result", PASS)defaults a rule with noresultkey to a pass.client.py:105— non-idempotent POSTs are retried;create_runon a 504 after the run was created makes a second run, and the client polls only the second.
Tests
test_client.py:19/29/34 only re-assert membership in the constant under test — they pass even if wait_for_run ignored TERMINAL_STATUSES. test_wait_for_run_timeout_is_an_error_never_a_pass passes timeout=-1, so the loop never executes. test_archive.py:72 names state.json in its docstring as the motivating leak but parametrizes only the three reserved names — the named case is finding 3. run_check, which maps status → verdict → exit code, has no test at all.
Clean: regions.py, discover.py including the $GITHUB_OUTPUT wrapper guard, _mask_by_marker's positional list walk, and rebuild_planned_values — deletes excluded, replaces retained, modules grouped, values genuinely taken post-masking.
Reviewed by Claude Opus 5
…xt tag The action becomes `tirith-iac-governance`, after the GitHub Action that submits these runs. The archive key travels as a `codeZipWfArtifactPath` context tag instead of a `terraformProjectZip` run field. That field is the CLI-driven workflow's contract and stays exactly as it is; reusing the generic tag mechanism keeps the run schema from carrying two first-class keys for one idea. Worth knowing rather than discovering: the tag is rendered in the dashboard's run list and run detail, and is searchable org-wide. That is accepted, and documented alongside the consumer notes -- an internal mechanism that happens to be visible is better than an invisible one people guess at. 385 tests pass.
…not gate Two changes. The code bundle travels as CodeZipWfArtifactPath rather than a context tag. Run context tags are indexed into ClickHouse for global search, with an org-wide aggregation returning the distinct keys for a typeahead, so an internal storage key would have surfaced in customers' tag pickers and every bundle path in the org would have been enumerable. create_run now reads the key back off the created run: an api that predates the field drops it during validation and the run then evaluates a VCS checkout instead of the uploaded code -- the wrong answer, delivered without complaint. A policy carrying `onFail: APPROVAL_REQUIRED` now warns instead of blocking. There is nothing to approve on these runs: the step exits 0 (it never uses exit 11), so the run reaches COMPLETED, and the run controller engages an approval only on exit 11 and skips it on the last step anyway -- of which a policy-only run has exactly one. The intent arrived as a count on an already-finished run, and gating on it produced a red check with nothing to click. The count, the icon and the "N need approval" headline all stay, so the author's intent is still visible. A run the platform genuinely paused still errors when it produced no results: never green for a run that evaluated nothing.
D301 (a docstring containing backslashes needs an r-prefix) and D403 (first word capitalisation). Both were introduced by this branch, so the lint job goes green on what this branch added rather than staying red on it.
refeed
left a comment
refeed
left a comment
There was a problem hiding this comment.
Reviewed the platform client end-to-end (src/tirith/platform/*.py) against the load-bearing behaviours in the design. Seven findings: two are masking leaks that put plaintext secrets on the platform, three can produce a green check for a run whose policies were never read or that actually failed. Everything below was reproduced against the code on this branch.
Ordered most severe first; details inline.
- The unmasked input document ships inside the archive (
archive.py) — only the three fixed output names are skipped from the packed tree, not the file the caller pointed at. resource_driftis never masked (redact.py) — same shape and same markers asresource_changes, emitted whenever refresh finds drift.- A FAIL is downgraded to
warnedwhen the run rests atAPPROVAL_REQUIRED(report.py) — exit 0 even under--fail-on-error. - A failed facts fetch is indistinguishable from "no policies" (
client.py) —no-policies, "no policies in scope", exit 0. - A missing or unrecognised
resultcounts as a pass (report.py). - Provisioner literals survive
_scrub_configuration(redact.py). - The setup-terraform wrapper guard has no opentofu counterpart (
discover.py).
Two minor ones, not worth an inline thread:
_requestretries POST (client.py:112), so a 502/504 onPOST .../wfruns/returned after the platform already created the run produces a duplicate run, and the client then polls only the last one.prepare_documents(check.py:110) addscount_redactions(masked_state)to the total on the branch where that masked state is immediately discarded, over-reporting the "Masked N sensitive value(s)" line when both--input-path(state) and--state-pathare given.
The rest held up under reading: the planned_values rebuild really does read the already-masked resource_changes (asserted on the rebuilt bytes, not the input); the __sg. prefix and the flat archive name are consistent with the delete/sync reasoning given; manages_terraform_state fails safe on an unreadable answer; create_run reads the stored key back and refuses a run without it; errored fails closed regardless of --fail-on-error; and pack_documents degrades to documents-only rather than taking the gate down.
Reviewed by Claude Opus 5
| skipped += 1 | ||
| continue | ||
| # The masked documents are written separately and must win. | ||
| if relative in reserved_names: |
There was a problem hiding this comment.
The unmasked input document is packed into the archive.
reserved_names is RESERVED_DOCUMENTS — the three names pack() writes. It is not the name of the file the caller read from, and --source-dir defaults to ., so the original travels next to the masked copy. Four paths, all of them ordinary usage:
--plan-file tfplan— the whole point of that flag (and ofterraform_show_jsonparsing straight off the pipe) is that no unmasked plan touches the disk. Buttfplanitself is a zip carrying the plan protobuf and the prior state, it sits in the source dir, and nothing excludes it.--state-path state.json— the exact filename the comment above cites (terraform state pull > state.json).state.jsonmatches neither*.tfstatenor*.tfstate.*.tfplan.json— the second namediscover.PLAN_FILENAMESaccepts, so discovery can pick it, mask it intoplan.json, and then pack the raw one alongside.- any
--input-paththat happens to live under the source tree.
Reproduced on this branch:
open(f"{d}/state.json","w").write(json.dumps({"version":4,"outputs":{"db":{"value":SECRET,"sensitive":True}}}))
open(f"{d}/tfplan.json","w").write(json.dumps({"resource_changes":[{"change":{"after":{"pw":SECRET},"after_sensitive":{"pw":True}}}]}))
open(f"{d}/tfplan","wb").write(b"PK\x03\x04" + SECRET.encode())
body, _ = archive.pack(source_dir=d, plan={"masked": SENTINEL}, state={"masked": SENTINEL})
# members: ['main.tf', 'plan.json', 'state.json', 'tfplan', 'tfplan.json', 'tfstate.json']
# SECRET present in archive: Truetests/platform/test_archive.py:72 parametrizes over exactly the three reserved names, so the scenario its own docstring describes (state.json) is the one case it does not cover.
Suggested fix: have check.pack_documents pass the resolved --input-path / --state-path / --plan-file, relativised to source_dir, through extra_excludes; and add tfplan, tfplan.json and state.json to DEFAULT_EXCLUDES so the conventional names are covered even when the caller named nothing.
| redacted = dict(plan) | ||
| redacted.pop("variables", None) | ||
|
|
||
| resource_changes = redacted.get("resource_changes") |
There was a problem hiding this comment.
resource_drift is never masked.
slim_plan drops prior_state and planned_values, and this function masks resource_changes and output_changes. But terraform show -json also emits a top-level resource_drift with exactly the same shape — change.before / change.after plus before_sensitive / after_sensitive — and it is populated on any plan where refresh detected an out-of-band change, which for a state-backed workflow is routine rather than exotic.
plan = {
"resource_drift": [{"address": "aws_secretsmanager_secret_version.v",
"change": {"actions": ["update"],
"before": {"secret_string": SECRET}, "after": {"secret_string": SECRET},
"before_sensitive": {"secret_string": True}, "after_sensitive": {"secret_string": True}}}],
"resource_changes": [ ...the same resource... ],
}
out = redact.redact_plan(plan)
# resource_changes masked: True
# SECRET leaks via resource_drift: TrueThe identical value, masked in one section and plaintext in the other, in the same document — which is the planned_values failure this module's docstring describes, in a third location.
Either add "resource_drift" to SLIM_DROP_KEYS (nothing in providers/terraform_plan/handler.py reads it, so dropping is lossless for evaluation), or run the same _mask_by_marker loop over it. There is no test for it in tests/platform/test_redact.py either way.
| error when there are none, and never report a pass for a run that evaluated nothing. | ||
| """ | ||
| if run_status == "APPROVAL_REQUIRED": | ||
| return "warned" if any(counts.get(k) for k in (FAIL, WARN, APPROVAL_REQUIRED, PASS, "SKIPPED")) else "errored" |
There was a problem hiding this comment.
A hard FAIL is reported as warned here.
This branch returns before the counts.get(FAIL) check below, so run status wins over rule result:
counts, _ = report.summarize({"p": [{"rule_name": "no-public-s3", "result": "FAIL",
"evaluations": {"fails": [{"result": [{"message": "bucket is public"}]}]}}]})
report.verdict(counts, "APPROVAL_REQUIRED") # -> 'warned'
report.headline(counts, "warned") # -> 'Tirith — 1 failed'So the PR comment renders a ❌ row and the headline literally says "1 failed", while cli.py falls through to ExitStatus.SUCCESS — exit 0 even with --fail-on-error. A failing policy stops gating because of where the run happened to rest.
test_verdict_failed_outranks_approval_required asserts exactly this precedence for the rule result on a COMPLETED run; the same precedence does not hold once the run status is APPROVAL_REQUIRED. Check counts.get(FAIL) first regardless of status, and keep the "results may be partial" reasoning for the WARN / PASS / no-results cases only.
| f"/wfgrps/{urllib.parse.quote(wfgrp)}/wfs/{workflow_id}/wfruns/{run_id}/wfrunfacts/default/", | ||
| ) | ||
| if status != 200: | ||
| return {} |
There was a problem hiding this comment.
"could not read the facts" and "the facts contained no failures" both come back as {}.
This returns {} for any non-200, and the presigned-GET path below is wrapped in a bare except Exception: return {}. get_results_artifact likewise returns None for a non-200, so the fallback at check.py:307 cannot tell the two apart either.
On a COMPLETED run whose GET .../wfrunfacts/default/ answers 403 (a token without the facts permission), 500, or whose presigned S3 GET times out or comes back malformed:
policy_results = {} → every count zero → verdict(counts, "COMPLETED") → "no-policies" → headline "Tirith — no policies in scope for this workflow" → cli.main reaches return ExitStatus.SUCCESS.
A green PR check, plus a comment positively asserting there were no policies in scope, for a run whose FAIL results were sitting in a document we failed to fetch. no-policies is only a safe answer when an empty result set was actually read; here it is being used for "we do not know", which is what errored exists for.
Suggest raising SGError (or returning a distinct sentinel) when the facts document could not be read at all, and letting run_check map that to errored — that path already fails closed regardless of --fail-on-error.
| ) | ||
| continue | ||
|
|
||
| result = rule.get("result", PASS) |
There was a problem hiding this comment.
Two ways a non-pass becomes a pass on this line and the next.
Absent result defaults to PASS. rule.get("result", PASS) counts a rule entry with no result key as a pass. skip is already handled above, so anything reaching here without a result is a shape the client did not expect — and "unexpected" should not resolve to the most favourable answer.
An unrecognised value is invisible to verdict. counts.get(result, 0) + 1 creates a bucket verdict never reads:
counts, _ = report.summarize({"p": [{"rule_name": "r", "result": "ERROR"}]})
# {'FAIL': 0, 'WARN': 0, 'APPROVAL_REQUIRED': 0, 'PASS': 0, 'SKIPPED': 0, 'ERROR': 1}
report.verdict(counts, "COMPLETED") # -> 'no-policies' -> exit 0so a result vocabulary this client does not know about — a new platform state, a lowercase spelling — renders as "no policies in scope for this workflow" and exits 0, having evaluated one rule that said something else. Folding anything outside {FAIL, WARN, APPROVAL_REQUIRED, PASS} into a counted UNKNOWN that verdict maps to errored would close both.
| if not isinstance(resource, dict): | ||
| return resource | ||
|
|
||
| expressions = resource.get("expressions") |
There was a problem hiding this comment.
Provisioner literals are not scrubbed.
This walks only resource["expressions"]. configuration.root_module.resources[].provisioners[].expressions is a second literal-bearing map on the same object, and it is the one most likely to hold an actual credential:
{"provisioners": [{"type": "remote-exec", "expressions": {
"connection": {"password": {"constant_value": "ssh-password-hardcoded"}},
"inline": {"constant_value": ["echo ssh-password-hardcoded"]}}}]}survives redact_plan verbatim — verified against this branch. Same class as the constant_value leak d24ac62 closed, one level further down the tree. It matters most under the documented source-dir: "" opt-out, which is otherwise the answer for "do not ship my HCL to the platform": the plan's configuration section still carries the provisioner's password.
Two smaller ones in the same area, both verified:
_scrub_config_modulepopsmodule_calls[name]["expressions"]only inside theisinstance(call.get("module"), dict)branch, so a call whosemodulekey is absent keeps every argument literal.count_expression/for_each_expressionare never reduced on either resources or module calls.
| and handed to the masker. stdout is never logged, for the same reason. | ||
| """ | ||
| executable = _resolve_binary(binary) | ||
| if not binary and os.environ.get("TERRAFORM_CLI_PATH") and os.path.basename(executable) == "terraform": |
There was a problem hiding this comment.
The guard is terraform-only, but _resolve_binary's docstring says opentofu/setup-opentofu does the same thing with tofu-bin, and the candidate list treats the two symmetrically. With TOFU_CLI_PATH set and no tofu-bin beside it, resolution falls through to the tofu wrapper and this check does not fire:
os.environ["TOFU_CLI_PATH"] = d # d contains only the `tofu` wrapper
discover._resolve_binary() # -> '<d>/tofu' — no DiscoveryErrorwhich is exactly the case that copies the whole plan into $GITHUB_OUTPUT for every later step in the job to read. Making it a pair — (("TERRAFORM_CLI_PATH", "terraform"), ("TOFU_CLI_PATH", "tofu")) — would cover both.
…t green
From review. Each was reachable on an ordinary run.
Masking:
* the file a document was READ FROM was packed beside the masked copy. Reserving
only the three names pack() writes missed the common cases -- the input is
routinely `tfplan.json` or `state.json` -- so the plaintext original shipped one
filename away from the redacted one. The source paths are now excluded.
* the BINARY plan (`tfplan`, `*.tfplan`) is excluded. It embeds the prior state, so
it carries every attribute of every existing resource, and it matched none of the
*.tfstate patterns -- `--plan-file`'s whole in-memory design was undone by the
source walk.
* `resource_drift` was never masked, though it has the identical shape and markers
as `resource_changes` and terraform emits it whenever a refresh finds drift.
* provisioner expressions (`connection.password`, `inline`) and module-call
arguments survived `_scrub_configuration`, which ships even under `source-dir: ""`.
Verdicts:
* a paused run returned before the FAIL check, so a run carrying a failing policy
reported `warned` -- a neutral check, which satisfies a required check -- while
the headline said "1 failed". It now ranks by the same ladder and is floored: a
run that did not finish can never report a clean pass either.
* a rule with no `result`, or one this module does not recognise, counted as PASS or
vanished into a key `verdict` never reads. Both are UNKNOWN now, and rank as
`errored`. "We cannot tell" is not a pass.
* `get_run_facts` returned {} for a 403 or a failed presigned GET, which is
indistinguishable from "no policies in scope" -- so an unreadable run whose
policies had failed reported "no policies in scope" and exited 0. It raises now,
and the caller only tolerates it if the legacy artifact answers.
Also: the setup-opentofu wrapper had no counterpart to the setup-terraform guard, so
`tofu show -json` could copy an unmasked plan into $GITHUB_OUTPUT.
A legacy results artifact that came back legitimately empty -- an older step image with no policies in scope -- is a real no-policies result, not a failed read. The guard now fires only when neither the facts nor the artifact answered.
refeed
left a comment
refeed
left a comment
There was a problem hiding this comment.
Re-review of the two fix commits (a47f63a, df57513): all six fixes land, one is incomplete, and two of them change behaviour in ways the new tests don't cover.
Verified by reading the code and running things, not by reasoning about the diff. tests/platform/ — 235 passed.
What holds.
redact.redact_plannow masksresource_driftby the same markers; the new test fails against the old code._scrub_config_resourcecoveringprovisioners,count_expression,for_each_expression, andmodule_calls[].expressionsunconditionally is lossless for evaluation — I checkedproviders/terraform_plan/handler.py: the only things it reads out ofconfigurationaremodule_calls[].module(recursion, handler.py:416) andresources[].expressions[].references(handler.py:385-388). Nothing reads any of the four keys now dropped.verdict()'s ladder is right in every direction: paused+FAIL →failed, paused+PASS →warned, paused+nothing →errored, and UNKNOWN outranks WARN. The floor is real._relative_sourcescorrectly refuses to reduce an out-of-tree path to a basename, andrealpathon both sides keeps it aligned with how_add_treenames members.- The
document_sourceswiring works end to end — I ran the action's own harness against this branch and the uploaded archive is['outputs.txt', 'event.json', 'src/main.tf', 'plan.json'].src/plan.json, the unmasked original, is gone.
One caveat on the archive that isn't a bug but is worth knowing. For --input-kind json / kubernetes the document is shipped unmasked (plan = document in prepare_documents), so excluding its source path from the tree buys nothing and costs the file at its original path — an autofix consumer that wants to patch k8s/deploy.json now finds it only as plan.json. Narrow, but the exclusion is unconditional on kind.
Findings inline. The --plan-file one is the one I'd fix before merge.
Reviewed by Claude Opus 5
| plan, | ||
| state, | ||
| infracost, | ||
| document_sources=(opts.input_path, opts.state_path, opts.infracost_path), |
There was a problem hiding this comment.
opts.plan_file is missing from this tuple, so the leak the commit message describes is only half closed. CONFIRMED, demonstrated.
The comment two lines up says "--plan-file supplies the document in memory and no path, which is exactly the case that needs no exclusion." That is the wrong way round: the binary plan is very much on disk at opts.plan_file, and it is the most dangerous file in the tree — it embeds the prior state, as archive.py:45-49 now says itself. The three new filename patterns (tfplan, *.tfplan, *.tfplan.*) are a proxy for the path, and they only cover the names the README happens to use.
I packed a tree the way run_check would on the --plan-file path:
members: ['main.tf', 'plan.json', 'plan.out', 'tfplan.bin']
secret present in archive: True
terraform plan -out=plan.out is at least as common as -out=tfplan, and --plan-file takes an arbitrary path. Add getattr(opts, "plan_file", None) to the tuple and the pattern list becomes belt-and-braces rather than the actual defence.
| f"/wfgrps/{urllib.parse.quote(wfgrp)}/wfs/{workflow_id}/wfruns/{run_id}/wfrunfacts/default/", | ||
| ) | ||
| if status != 200: | ||
| raise SGError(f"Could not read the run facts for {run_id} (HTTP {status}): {payload.get('msg')}") |
There was a problem hiding this comment.
This raise has no test in either repository, and the one fixture that models this endpoint says it returns 404.
The action's stub (tests/test_action_integration.py, do_GET) answers /wfrunfacts/ with 404 not found and serves the verdict from /artifacts/ instead. So every platform-mode test in that suite reaches its verdict through the legacy artifact — the same artifact get_results_artifact's own docstring says current step images no longer write. Which means this branch is exercised by nothing.
I checked what happens if the fallback isn't there: with /artifacts/ also returning 404, 10 of 54 action tests fail, every one of them with
ERROR: The run completed but its results could not be read: Could not read the run facts for wfrun-1 (HTTP 404): not found
including runs that previously reported no-policies and exited 0.
So one of two things is true and I can't tell which from here: either the stub is modelling a shape the platform no longer returns (in which case it should be fixed, because it is hiding the entire new code path), or a non-200 from wfrunfacts/default/ is still a live shape for a run with no facts document — and then this turns every such run red. get_results_artifact's docstring ("it existed only because the facts endpoint used to answer 'does not exist' for every run") suggests the 404 was real at least once.
Worth pinning both directions with unit tests on get_run_facts (non-200 raises; failed presigned GET raises; 200 with no signed_url returns {}) and on run_check's facts_error guard (raises when legacy is None, does not when the artifact answered {}). df57513 in particular is a subtle piece of logic with zero coverage.
|
|
||
| if verdict_value == "errored": | ||
| header += [ | ||
| f"The workflow run finished as `{run_status}` without producing policy results.", |
There was a problem hiding this comment.
With the new UNKNOWN bucket, this narrative can be flatly false. CONFIRMED.
verdict() now returns errored for a COMPLETED run that produced plenty of results, as long as one of them is unrecognised. This paragraph is written for the other case. Rendering {'p1': [{'rule_name':'r1','result':'ERROR'}, {'rule_name':'r2','result':'PASS'}]} against COMPLETED:
## 🛡️ Tirith could not evaluate policies
The workflow run finished as `COMPLETED` without producing policy results.
This is reported as a failure rather than a pass: no verdict is not the same as a clean one.
| | Policy | Rule | Resource |
|---|---|---|---|
| ⚪ | `p1` | r1 | — |
| ✅ | `p1` | r2 | — |
<sub>✅ 1 passed · View run in StackGuardian</sub>
"without producing policy results" sits directly above two policy results, and the footer says one passed.
Two smaller things fall out of the same change: an UNKNOWN row draws ⚪, the same icon as SKIPPED (_ICONS.get(..., "⚪")), so the reader cannot tell which rule was the unresolved one; and _render_detail is only built for FAIL/APPROVAL_REQUIRED/WARN, so an UNKNOWN finding gets no detail block explaining itself. Gating on UNKNOWN is right — but the person reading the comment needs to be told which rule the tool could not read, and this currently tells them the opposite.
| "failed": counts.get(report.FAIL, 0), | ||
| "warned": counts.get(report.WARN, 0), | ||
| "approval_required": counts.get(report.APPROVAL_REQUIRED, 0), | ||
| "skipped": counts.get("SKIPPED", 0), |
There was a problem hiding this comment.
counts in the result document has no unknown key, so the new bucket is invisible to every consumer of --output-json.
A run that errors because of an unrecognised result publishes {"passed": 1, "failed": 0, "warned": 0, "approval_required": 0, "skipped": 0} alongside "verdict": "errored". The action copies these straight into outputs.passed / outputs.failed, so a workflow gating on outputs.failed == '0' sees a clean count for a run the tool refused to vouch for. The exit code and the verdict are both correct, so nothing is unsafe — but the counts now disagree with the verdict, and adding "unknown": counts.get(report.UNKNOWN, 0) costs one line.
| # setup-terraform and setup-opentofu both install a wrapper that echoes stdout into | ||
| # $GITHUB_OUTPUT, and both advertise it the same way. Guarding only the terraform spelling | ||
| # left the opentofu one to copy the whole unmasked plan into the step output. | ||
| for env_var, wrapper in (("TERRAFORM_CLI_PATH", "terraform"), ("TOFU_CLI_PATH", "tofu")): |
There was a problem hiding this comment.
The opentofu half of this guard is correct but untested — test_discover.py only ever delenvs TOFU_CLI_PATH, and test_a_wrapper_without_its_real_binary_is_refused covers the terraform spelling alone.
The pairing is right (a mismatched env/basename combination can't fire), and _resolve_binary already prefers tofu-bin, so this is only reachable when the wrapper was installed without its usual layout — which is exactly the case worth a test, since it is the one where the plan would otherwise land in $GITHUB_OUTPUT. Parametrising the existing test over (TERRAFORM_CLI_PATH, terraform) and (TOFU_CLI_PATH, tofu) would cover it.
…rrow the facts raise From re-review. document_sources omitted opts.plan_file -- the one path most worth excluding. --plan-file converts the BINARY plan in memory precisely so nothing unmasked touches the disk, but the binary plan is already on disk and embeds the prior state: every attribute of every existing resource. The tfplan name patterns only cover the spellings the README uses, and `terraform plan -out=plan.out` is at least as common. get_run_facts now treats 404 as absent rather than unreadable. A run that produced no facts document answers that way, and that is a legitimate empty result -- raising on it would have turned healthy runs red, the opposite of the mistake being fixed. 403/500 and a failed presigned GET still raise. Also: an errored verdict caused by UNKNOWN results rendered "finished without producing policy results" directly above a populated table; UNKNOWN now has its own icon and a detail block so the reader can see which rule was unresolved; and `unknown` is published in counts, so a consumer can tell "nothing failed" from "we could not read part of it".
The step routes on which document is present in the archive, so a stored kind added nothing it could not work out -- and could be wrong. A two-phase pipeline gates the plan and then checks the state against the same workflow, whose identity derives from the repository and workflow name; the workflow is created once, by whichever phase ran first, so the stored kind was that phase's and the other phase fed its document to the wrong provider. Every policy came back unevaluated and the phase looked like it had passed. --input-kind stays: it drives client-side masking (redact_plan vs redact_state) and which document slot the archive gets, which is a different question entirely.
refeed
changed the title
tirith platform check — region key, document discovery, shared upload endpointtirith platform check — the client behind the IaC Governance action
Found by an end-to-end run, not a unit test -- every unit test used the shape the code already understood. redact_state was written for the raw state (`terraform state pull`): top-level `resources`, each instance naming its own `sensitive_attributes`. Handed `terraform show -json <state>` output instead -- resources under `values.root_module.resources`, sensitivity in a parallel `sensitive_values` tree -- it matched nothing and returned the document unchanged. No error, no warning: every attribute of every resource shipped in plaintext, which for state is every attribute there is. Both shapes are handled now, including child_modules, whose resources are nested rather than flattened, and sensitive outputs in either.
…raform action
The workflow now carries the policy step in `TerraformConfig.prePlanWfStepsConfig`, and
the run is created with `TerraformAction: {"action": "plan"}` -- a dummy. core splices
pre-plan steps ahead of `generate-terraform-plan`, and the step exits 12, which tells the
run controller to complete the run successfully and skip everything after it. So the plan
never runs, and core needs to know nothing about this feature.
That is the point: expressing "run one step, then stop" with primitives the platform
already had removes the core and sg-run-controller changes entirely and reduces api to a
single field. `plan` is chosen only because it is the action whose synthesis splices
pre-plan steps in.
The archive travels in `terraformProjectZip`, the CLI-driven workflow's field (SG-3809),
which core and both runners have read since December. One cost, recorded in the comment:
sharing it means a policy-check archive can no longer be distinguished from that feature's,
so a future rule cannot reject the field for the wrong action.
Everything else -- masking, packing, polling, rendering -- is untouched.
…plate-id The step template is not a caller's choice. The archive layout, the exit-12 contract and the shape of the facts document are one agreement between this client and that image; pointing the workflow at anything else produces a run that looks like a policy check without being one. Asserted at both ends: the config always names the constant, and the parser offers no way to ask for something else.
…raform The flag predates the pivot to a pre-plan policy step, so its help text still described overriding the terraform step template. It has only ever been passed to terraform_config as the wfStepTemplateId of the spliced policy step.
Three docstrings still described a dedicated CodeZipWfArtifactPath key. Reusing terraformProjectZip is what lets core and the run controller stay untouched, and the cost -- a policy archive being indistinguishable from the CLI-driven workflow's -- is now stated where the reuse happens rather than only on the PR.
refeed
changed the title
tirith platform check — the client behind the IaC Governance actiontirith platform check — a pre-plan policy step that exits 12
…a run field
Removes the last api dependency. The bundle is PUT into the workflow's own
artifact prefix, which the run controller already syncs down into
$LOCAL_ARTIFACTS_DIR before any step executes, and the step is told its name in
wfStepInputData. So the run body names no archive, api needs no serializer field
and no new response key, and api#1708 closes outright.
Three things this had to get right:
The name. It was __sg.{sha}-{tag}.tar.gz, deliberately prefixed to stay OUT of
that same sync. Now the sync is the delivery mechanism, so the prefix would make
the bundle invisible to the step -- it must match none of the sync's excludes
(sg.*, *__sg.*, *pci_*, the compliance globs) and must not be tfstate.json.
Growth. Losing the sha loses uniqueness, so the name is fixed and overwritten in
place: one object per workflow however many runs happen. A per-commit name could
not be cleaned up -- the artifact prefix has no lifecycle rule, neither sync
passes --delete, and api serves only GET and POST on artifacts. The step also
deletes the bundle from the volume after unpacking, so it is not carried forward
into later runs by the sync.
Races. A fixed name means a concurrent run of the same workflow can replace the
bundle between our upload and our step's read, which would report a verdict on
the wrong commit -- silently. It cannot be prevented here: the bundle is
uploaded before the run exists, and wfStepInputData is frozen at workflow
creation, so no per-run expectation can be passed in. Instead the client writes a
nonce into the bundle, the step echoes it into the facts, and the client fails
closed on a mismatch with the fix named. A step reporting no nonce is treated as
'cannot tell', not as a mismatch, so older images do not break.
file_upload_url no longer needs data.key, and no longer asks for a signed
contentType: it signs application/json regardless, and S3 checks the signature
against the header sent, so the PUT sends application/json and the stored object
is merely labelled wrongly.
|
❌ The last analysis has failed. |
refeed
changed the title
tirith platform check — a pre-plan policy step that exits 12tirith platform check — a pre-plan policy step, no platform changes
What
tirith platform check— the client behind the IaC Governance GitHub Action. It packs thedocuments a policy needs, masks them, uploads them, runs them through a StackGuardian workflow, and
renders the verdict as a PR comment and a check run.
How the run is shaped
A check is an ordinary
planrun whose workflow carries oneprePlanWfStepsConfigentry pointingat the
tirith-iac-governancestep template. That step exits 12, which tells the run controller tocomplete the run and skip everything after it — so
generate-terraform-plannever executes and theplanaction is never acted on. It is a dummy.No platform repo changes at all. core, sg-run-controller and api are untouched — core#1235,
sg-run-controller#298 and api#1708 are all closed.
How the bundle reaches the step
Not through a run field. The bundle is PUT into the workflow's own artifact prefix, which the run
controller already syncs down into
$LOCAL_ARTIFACTS_DIRbefore any step executes(
external.py:2524). The step finds it by name; the run body names nothing. That is what removed thelast api dependency — no serializer field, no
data.key, no?contentType=.Three things this had to get right:
The name. It was
__sg.{sha}-{tag}.tar.gz, prefixed deliberately to stay out of that same sync.Now the sync is the delivery mechanism, so the prefix would make the bundle invisible to the step. The
name must match none of the sync's excludes (
sg.*,*__sg.*,*pci_*, the compliance globs) and mustnot be
tfstate.json.Growth. Losing the sha loses uniqueness, so the name is fixed and overwritten in place: one object
per workflow however many runs happen. A per-commit name could not be cleaned up — the artifact prefix
has no lifecycle rule, neither sync passes
--delete, and api serves only GET and POST on artifacts.The step also deletes the bundle from the volume after unpacking so the sync does not carry it forward.
Races. A fixed name means a concurrent run of the same workflow can replace the bundle between our
upload and our step's read, which would report a verdict on the wrong commit — silently. It cannot be
prevented here: the bundle is uploaded before the run exists, so there is no run identity to name it
after, and
wfStepInputDatais frozen at workflow creation (ensure_workflow409s and updatesnothing), so no per-run expectation can be passed in. So the client writes a nonce into the bundle, the
step echoes it into the facts, and the client fails closed on a mismatch, naming the fix. A step
reporting no nonce is "cannot tell", not a mismatch, so older images keep working.
On the content type:
file_upload_urlsignsapplication/jsonwhatever the filename, and S3 validatesthe signature against the header the client sends, not the body. So the PUT sends
application/jsonwith a gzip body and is accepted; the stored object is merely labelled wrongly, which nothing reads.
Consequences worth stating plainly:
planrun, scheduled drift proceeds off it. The dedicated actionused to switch drift off. Neither behaviour is obviously right; this one is at least not silent.
Masking
Everything is masked client-side, before anything leaves the runner.
redact_statehandles bothstate shapes — raw
terraform state pull(top-levelresources, per-instancesensitive_attributes) andterraform show -json <state>(values.root_module.resources[].valueswith parallel
sensitive_values, plus nestedchild_modules). Only the first was handledinitially; the second shipped plaintext, found by E2E rather than by the unit suite, because every
masking test used the shape the code already understood.
Committed source ships as written, so a secret hardcoded in HCL still reaches the platform.
Documented;
--source-dir ""opts out.Verdicts
FAIL → failed,UNKNOWN → errored,APPROVAL_REQUIRED/WARN → warned,PASS/SKIPPED → passed(or
warnedif the run paused), empty →no-policies(orerroredif paused). A rule with noresultisUNKNOWN, never an implied pass — the rule this codebase holds to is never green whennothing was evaluated.
Exit codes:
0ok,1tool failure (ignores--fail-on-error),3policy failed under it.Verified on QA
The exit-12 mechanism had never run in production. Both directions were checked with real runs on
the shared-external runner:
wfStepsConfig=[evaluate-policies, generate-terraform-plan]COMPLETEDaton_0_evaluate-policies;generate-terraform-planhas no status entryon_0_generate-terraform-planran — terraform unaffectedprePlanWfStepsConfiground-trips through apiTests
242 in
tests/platform, 494 across the suite.Known limitation
ensure_workflowreturns 409 for an existing workflow and updates nothing, so a workflow createdbefore this feature keeps its old
TerraformConfigand gains no policy step. Fresh workflow ids arerequired; this is why the E2E uses new ones.