[SG-4885] feat(platform): tirith platform check — a pre-plan policy step, no platform changes - #272
Open
refeed wants to merge 59 commits into
Open
[SG-4885] feat(platform): tirith platform check — a pre-plan policy step, no platform changes#272refeed wants to merge 59 commits into
tirith platform check — a pre-plan policy step, no platform changes#272refeed wants to merge 59 commits into
Conversation
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.
Comment on lines
+107
to
+110
| if counts.get(WARN) or counts.get(APPROVAL_REQUIRED): | ||
| return "warned" | ||
| if counts.get(PASS) or counts.get("SKIPPED"): | ||
| return "passed" |
Comment on lines
+176
to
+179
| # 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.
Member
Author
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.
refeed
force-pushed
the
feat/gate-capable-engine
branch
from
August 6, 2026 08:38
f32ff37 to
b758b52
Compare
refeed
commented
Aug 6, 2026
refeed
left a comment
refeed
left a comment
Member
Author
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
arunim2405
previously approved these changes
Aug 11, 2026
…s it from
The `--no-source` clarification was applied to the *embedded* `--help` block in
docs/platform-check.md, so the page and the program disagreed: `cli.py` still said
"Send only the documents, not the source tree." That block is a verbatim copy of
`--help`, so it can only be edited by changing the source of the copy. Moved there,
regenerated, and the stray double period is gone with it.
Three flags reworded, all from questions asked on review -- which is the useful signal
that the help text was not carrying its weight:
--plan-file said "Binary terraform plan", and a reader still had to ask whether it
wanted the binary or the JSON. Now names what produces it
(`terraform plan -out=`) and points at --input-path for the JSON.
--no-source said only what it does not send. It also decides where the plan is
looked for, since it clears --source-dir and discovery falls back to
the current directory -- so "documents only" was half the behaviour.
--artifact-tag said "Namespaces the archive within a commit", which does not tell
anyone when to set it. Now names the two cases that need it: a
plan phase and a state phase, or matrix legs sharing one workflow.
README's flag table follows, and its `--source-dir ""` row is now `--no-source` -- the
empty string is the Action's spelling of it, not the CLI's.
…a metadata.json The bundle was flat: masked documents and every walked source file shared the archive root. Two consequences. Source and documents occupied one namespace, which is the only reason RESERVED_DOCUMENTS existed -- a `tfstate.json` in the working directory could displace the masked one. The source now sits under `code/`, so the root belongs to us alone. And a consumer holding only the bundle could not tell what it had: not which repository or commit produced it, not which subdirectory of that repository the code came from, not whether the documents were masked, not whether absent code meant "none wanted" or "dropped for size". The bundle is meant to be read by other systems; it described itself not at all. `metadata.json` answers those. `code.repo_path` is the field that could not be recovered any other way. Members are named relative to --source-dir, so the path *within* the repository is destroyed at pack time: `--source-dir infra/prod` means `code/main.tf` belongs at `infra/prod/main.tf`. Declared with the new --repo-path, or inferred by walking up for a `.git` entry (a `.git` *file* counts -- worktrees and submodules). `repo_path_from` records which, because for anything about to write into a repository, declared and inferred are different confidences. `""` is the repository root, deliberately not `"."` and not `null`: joining still works and it stays distinguishable from "could not tell". Assembled in two halves. check.py owns intent, archive.pack owns observation -- it fills in whether a tree was really walked, under what prefix, and the counts, then writes the member last. So `code.present` means "there are members under the prefix", not "a source directory was requested": a tree whose every file was excluded yields present=false with files=0, and the tar and the metadata cannot disagree. The alternative was a second pack to learn the counts, which would double peak memory on a 100 MB cap and run the size check twice with different answers. Built for a local run first. From a laptop there is no trigger payload, usually no --sha and no --repo-url; those fields are null rather than omitted or invented, and `origin.kind` says `local` as a positive statement instead of leaving CI to be inferred from an absence. `repository` is sniffed from the host and kept independent of `origin`, because a GitHub Actions job can check out a GitLab repository -- and an unrecognised host is `unknown` with the host still recorded, not guessed. snake_case throughout: it matches every JSON this tool authors -- the result document, the manifest, the plan.json sitting beside it -- and camelCase in this package appears only where it mirrors the platform's wire API, which this file never crosses. Two things deliberately absent. No size field: it cannot describe the archive containing it, and Content-Length already answers it. No actor, PR title or commit message -- PII and free-form human text, the most common accidental secret channel, with no value to a consumer. And any credential in --repo-url is stripped before it is written: `https://x-access-token:ghs_...@github.com/...` is an ordinary CI value, and this file outlives the run. The documents stay at the root, which is load-bearing rather than incidental: the step joins those three names onto the extraction directory and treats absence as normal, so moving one under a prefix would not raise -- every policy would report unevaluated and the run would look like it passed with warnings. Now asserted, since nothing else would catch it.
`tirith -policy-path ... -input-path ...` returned 0 whether the policy passed or failed,
so on its own it could not gate anything. The README said so and then pointed at the
hosted path as the remedy -- which made the open-source surface second class in the one
way that matters: you could only block a merge by talking to StackGuardian.
`--fail-on-error` fixes it, opt-in. The default stays 0 because anyone already running
this in CI depends on that, knowingly or not, and a silent change would turn their
pipeline red on an upgrade they did not ask for. Same flag name and same exit code as
`platform check`, so a caller scripting both does not learn two vocabularies.
The care is in what 3 does NOT cover. `final_result` is False both for a policy that
failed and for one that could not be evaluated -- an operator the evaluator does not
implement, an unresolved variable -- and those are different answers: 3 says the
infrastructure violates a policy, 1 says tirith could not tell you. `errors` separates
them, and the missing-variables path returns errors with no `final_result` key at all, so
absence is handled the same way. A job that conflates them reports an outage as a
violation. Verified end to end across all four paths, not just unit tested.
README, from the review:
* `error_tolerance` now has a section. It appeared in five examples explained in none,
and it is what produces the `"passed": null` outputs further down -- a third outcome
that is neither pass nor fail. Includes the two consequences worth knowing: a policy
whose every check is skipped evaluates to a pass, and --fail-on-error exits 0 for
that, because nothing failed.
* The Kubernetes "Example 1" and "Example 2" had byte-identical policies -- the second
only added expected output. Merged into one rather than deleting either, since the
output was the part worth keeping.
* An output sample was missing its closing brace. Checked every JSON block in the file
while there; the other seven that do not parse are deliberate `...` elisions.
* Removed three commented-out sections and their commented TOC entries, and merged the
two near-identical contributor invitations that sat 1250 lines apart.
* Usage block and exit-code table regenerated -- the currency test caught both, which
is what it is for.
`platform` was internal vocabulary escaping into a user-facing verb. In English
"platform check" parses as *a check of the platform* -- which is what `--platform` means
in most tools a reader has already used, and the phrase turns up in our own codebase
meaning exactly that ("the AZURE-platform check"). `remote` names the distinction that
actually exists: the policies and the evaluation live somewhere else.
The concern that prompted this was open-source usage, and the important half of that was
fixed separately -- the local surface could not gate at all until --fail-on-error, and no
rename would have papered over it. This is the smaller half: a subcommand an OSS user
never needs should not sound like the tool's centre of gravity.
`platform` still dispatches, undocumented, and prints a deprecation line to **stderr** --
not stdout, where it would corrupt `--json` output being piped into something. Keeping it
is cheap and means no snippet anyone has copied off this branch breaks. It is deliberately
absent from the help and the README: two documented names for one command is how the
vagueness complaint arrives a second time. Drop it around 1.5.0.
Renamed `docs/platform-check.md` to `docs/remote-check.md` with git mv, so the history
follows, and regenerated its embedded --help under the new name.
The directory `src/tirith/platform/` keeps its name. Only the word the user types changed;
renaming the package would put churn in every import for no reader's benefit.
Not done here, and worth stating: `check` as a top-level verb was considered and rejected.
It would make the policy *source* depend on whether SG_API_TOKEN happened to be exported,
so an ambient environment variable could silently swap your repository's policy files for
your organization's enforced set. The dispatch test now says so, since "why isn't it just
`tirith check`" is the obvious next question.
There is nothing to be backwards compatible with. py-tirith is not on PyPI, the action pins a branch, and the subcommand only ever existed on this unmerged branch -- so the alias was keeping faith with callers that do not exist, at the cost of a second name to explain in the help, the README and the tests forever. `platform` now falls through to the flat parser, where it is an unrecognised positional and fails like any other typo. That is better than accepting it silently: a name that works but is undocumented is how you end up supporting it anyway. The action already invokes `remote check`, so the two repositories are consistent. Note this does remove the property that made their order not matter -- an old action checkout would now break against this CLI. Irrelevant in practice, since neither is released, but it is no longer true and the previous commit message said it was.
refeed
commented
Aug 12, 2026
refeed
left a comment
refeed
left a comment
Member
Author
There was a problem hiding this comment.
Review — bundle layout, --fail-on-error, and the platform → remote rename
Focused on 326a387, 04fb161, 29193a4 and 1860c6b. Every finding below has a reproduced failure case; nothing is speculative. Baseline confirmed first: 18 tests fail on this machine before the diff is considered, and 7 of them come from a file this PR adds (see the scope comment).
The two I would not merge without:
_split_repo_urlemits the credential verbatim when it cannot recover a host — including for a well-formedhttps://URL with an empty authority, the shape a CI template produces when its host variable is unset. It lands inmetadata.json, inside a bundle the code deliberately retains.--fail-on-erroron the flat CLI returns 3 for two of the three "could not be evaluated" shapes and 1 for a genuine violation, which is both halves of its stated contract inverted. The README section added in the same commit documents the opposite behaviour again.
Also found: ArchiveError conflating a missing --source-dir with an oversized one (and writing absent_reason: "too_large" about it); two places where code.present / source_packed can disagree with what is actually in the tar, which is the invariant _observed_metadata exists to hold; symlinked directories dropped from the bundle uncounted; --repo-path accepting ../..; parts.port raising an uncaught ValueError; a dead exclude pattern; and stale platform check wording in the changelog and status.py.
Checks that came back clean, since they were asked for explicitly:
_add_tree's member naming is sound.relativederives fromos.path.relpathunderos.walk(source_dir), so it can never be absolute or contain.., and a path separator cannot appear inside a filename on either platform — no traversal, no absolute member, no extraction outside the target. Theposixpathreasoning in the docstring is right._observed_metadatadoes not mutate the caller's dict, and neither does the oversize retry inpack_documents— the caller'smetadataand its nestedcodeboth survive a pack byte-identical._repo_path's upward walk terminates correctly on both root shapes (os.path.dirnamefixpoint afterrealpath), handles a.gitfile, returns""and not"."for the repo root, and returns(None, None)for a broken symlink, an unreadable parent, and asource_diroutside any checkout. The inferred value cannot be absolute; only the declared one is a problem.- The rename is complete in the code — no
platformsubcommand survives, andtests/cli/test_dispatch.pycovers the fall-through.
Scope, beyond the ansible files: the changes to src/tirith/core/core.py, core/policy_parameterization.py, prettyprinter.py and their tests are the four fixes the changelog files under a separate release (1.1.0: metadata passthrough, non-mutating substitution, unsupported condition.type, unattributed provider errors). They are good changes and unrelated to a remote engine; landing them separately would let this PR be reviewed as one idea and would unblock them from it.
Reviewed by Claude Opus 4.6
…ode contract
Two real bugs from review, both in code I added today, both verified before and after.
**The credential sanitizer returned its input verbatim when it could not parse a host.**
`_split_repo_url` fell back to `return text, None`, so anything urlsplit found no hostname
in went into metadata.json unchanged -- inside a bundle that is retained indefinitely.
Both shapes that land there carry secrets:
https://oauth2:glpat-SECRET@/acme/infra.git empty authority: what
`https://oauth2:$TOKEN@$HOST/x`
renders to with HOST unset
gitlab-ci-token:glcbt-SECRET@gitlab.com/x.git scheme-less; urlsplit reads the
username as a scheme
The second is the exact GitLab CI_REPOSITORY_URL shape the docstring cites as its reason
for existing, which is a fair measure of how much a hand-checked sanitizer is worth. It
now fails closed: a URL we cannot parse is a URL we cannot sanitise, and losing it from
the metadata is far cheaper than leaking the token in it. Also fixed while there: an
invalid port raised out of `parts.port`, and IPv6 hosts lost their brackets. Eleven
cases now covered.
**--fail-on-error had both halves of its contract inverted.** It gated on `errors`, which
looks like a tool-failure signal and is not -- it is populated only by the eval-expression
pass, and the one thing that puts a message there beside a real verdict is the
informational "these ids are not defined and have been removed" note. Measured:
genuine violation, expression names a typo'd id was 1, should be 3
policy naming an unknown provider was 3, and stays 3 (see below)
every check skipped via error_tolerance was 3, should be 1
`final_result` is tri-state and that is the whole answer: True -> 0, False -> 3, None
-> 1. None means nothing ran, which is neither a pass nor a violation -- and reporting it
green is the failure the flag exists to prevent. The README claimed the opposite ("evaluates
to a pass ... exits 0"), and a docs review arrived at the same fix independently.
The limit is now stated instead of implied: a *misconfigured* policy -- unsupported
condition type, unknown provider -- comes back from the engine as an ordinary failed check
with no error attached, so it is indistinguishable from a violation and exits 3. It fails
closed, but it points at your infrastructure when the fault is in the policy. Fixing that
needs the engine to report it distinctly, not this branch guessing from free text.
Also drops 17 files from the PR that belong to unrelated ansible/jq/jmespath work, swept in
twice now. They are 7 of the 18 failures I had been calling a pre-existing baseline --
`test_ansible_best_practices_jq.py` asserts a `jq_query` operation that exists nowhere in
src/, so it fails for everyone. Untracked, not deleted.
17 files under tests/providers/json/ belonging to separate work -- ansible-lint and ansible-best-practices fixtures, jq and JMESPath READMEs. Swept into this branch twice, and cleaned out once before. Not inert. test_ansible_best_practices_jq.py asserts an `operation_type: "jq_query"` that exists nowhere in src/, so it fails 7 tests for anyone who checks the branch out -- and those are 7 of the 18 failures I had been attributing to a pre-existing baseline. They would go red on main. Untracked rather than deleted: the files stay on disk for whoever is working on them. Doing it as its own commit because the previous one tried to include it and `git add -A tests` silently re-added everything `git rm --cached` had just staged -- the same way a stray infracost fixture came back earlier in this branch's history.
**`--repo-path` could escape the repository.** `strip("/")` left `../..` intact, and the one
use of that field is a consumer joining it to write files back into the repository it
thinks it is patching. Now normalised, and a value that climbs out or is absolute is
refused with a warning and left absent -- absent is a state consumers already handle,
wrong is not.
**`added` counted members `tar.add` never wrote.** It does not raise for a type
`gettarinfo` cannot classify -- a unix socket, a fifo -- it debug-logs and returns. So
`files` could exceed what the tar held and `code.present` could be true over an empty
prefix, which is exactly the disagreement `_observed_metadata` exists to prevent. Guarded
on `os.path.isfile`.
**`source_packed` in the result document contradicted `code.present` in the metadata.** One
was derived from what was requested, the other from what was packed; a tree that emptied
into the exclude list made them disagree. Both now come from the count.
**Symlinked directories vanished without trace.** `os.walk` does not follow them and the
islink guard only covered files, so a symlinked `modules/` was neither packed nor counted
-- the manifest was the only place a caller could have noticed, and it said nothing. Now
counted as skipped.
**`absent_reason` could relabel a deliberate documents-only run as an oversize failure.**
The retry stamped `too_large` unconditionally; it now only fills a reason the caller did
not give.
Smaller, all from the same review: dropped `*.tfstate.backup`, which `*.tfstate.*` on the
line above already matches; fixed the one-column continuation in `--help` left by the
rename; and `status.py` no longer names `platform check`.
CHANGELOG: corrected to `remote check`, added the local `--fail-on-error` and the bundle
layout, recorded the rename, and fixed a claim that exit 1 "applies even without
--fail-on-error" -- it does not on the local surface, where without the flag everything
still exits 0.
Third attempt. The previous two were undone by a later `git add -A tests` in the same session, which re-staged every file `git rm --cached` had just removed -- the files are still on disk, so -A sees them as new. Added to .git/info/exclude locally so it cannot happen again; not .gitignore, because they belong to someone else's in-flight work and should stay visible on their branch. 17 files, unrelated to a gate-capable remote engine: ansible-lint and ansible-best-practices fixtures, jq and JMESPath policy examples and READMEs. test_ansible_best_practices_jq.py asserts an `operation_type: "jq_query"` that exists nowhere in src/, so it fails 7 tests for anyone who checks this branch out.
refeed
changed the title
tirith platform check — a pre-plan policy step, no platform changestirith remote check — a pre-plan policy step, no platform changes
The exit-codes section opened by explaining that the local form exits 0 either way and only then mentioned --fail-on-error. That framing is for a reader protecting an existing pipeline; a newcomer has none, and it reads as an apology for something that now works. Gating comes first, with the command. The default is a one-line note after it, which is where a compatibility caveat belongs. Also merged the two paragraphs that were both explaining 3-vs-1 in slightly different words.
…e files I touched **The 3.8 and 3.9 unittest jobs were failing on a test I wrote**, and for a reason the test itself created. `test_the_usage_block_is_the_real_help_output` compared the README's Usage block byte for byte against `tirith --help` -- but argparse renamed its section header from "optional arguments:" to "options:" in 3.10, so a block generated on any one interpreter cannot match on the other half of the matrix. It passed on 3.10-3.12 and failed on 3.8-3.9, which is the worst shape for a guard test: it looks like it works. Now compares the *set of option strings* in both directions -- accepted but undocumented, documented but not accepted. That is the thing the test existed to catch (a `-var-path` that shipped undocumented), and it does not depend on how argparse decides to lay out a heading. Verified against a simulated 3.8 header. **Black.** Four files, all touched by this PR: core.py, platform/check.py, tests/platform/test_client.py, tests/platform/test_report.py. Formatted only those -- `origin/main` itself fails Black on 14 files because CI pins `psf/black@stable` (unpinned, so whatever is newest) against a tree formatted by an older release, and reformatting the other ten here would bury this PR in an unrelated diff. That drift is worth fixing on main by pinning the version, separately.
The Black job was red on this PR and on `main` alike, on 14 files nobody had touched. Cause: `psf/black@stable` resolves to whatever Black is newest when the job runs, so a Black release reformats the world and every open branch goes red with nothing in the repository having changed. main last passed this job in November 2025 and fails it today. Pinned to 25.1.0, which is the release the tree is actually formatted for -- verified by running it against `origin/main`, which comes back clean, and against 24.10.0, which also does. Bumping it should be a deliberate commit that reformats, not a surprise from upstream. Reverted my earlier attempt to fix this by formatting four files with 26.5.1: that was chasing the newest release rather than the pinned one, and would have left the tree inconsistent with the other 85 files. The tree is now clean under 25.1.0 end to end.
archive.pack raises ArchiveError both for an oversized archive and for a source directory that does not exist, and pack_documents' degrade path only knew about the first. So a typo'd --source-dir was reported as "the tree was too large", the code was dropped, and the run completed -- a check that passed having evaluated no source at all, with the bundle's own metadata.json stating the wrong reason for its absence. Checked before packing, where the two are still distinguishable. A missing directory is a user error and should stop the run; only a genuinely oversized tree degrades.
…st F2)
A secret used in a resource tag was masked at `tags.Password` and uploaded in **cleartext**
at `tags_all.Password`. Both state shapes leaked, and the bundle is retained indefinitely,
so the plaintext outlived the run.
`redact_plan` has swept for this since the equivalent plan leak was found: it collects the
plaintext of every marked-sensitive value and replaces that value everywhere in the
document, precisely because a provider writes computed *mirrors* of an attribute carrying
the same secret with no sensitivity marker of their own. `tags_all` is the confirmed case
-- terraform does not propagate sensitivity into values it computes for you.
`redact_state` masked by marker and returned. Same hole, worse place: state carries every
attribute of every resource, not just what a plan surfaces. Found by a penetration test
(F2, Medium), reproduced code-level, and reproduced again here against both shapes before
and after.
It now collects at all four masking sites and sweeps the whole document on the way out:
show -json resources _collect_sensitive_values against `sensitive_values` -- the same
marker convention as a plan, so this is a direct reuse
raw instances read the plaintext at each resolved `sensitive_attributes` path
outputs, both shapes collect `output.value` before it is replaced
The raw shape needed a path *reader*: `_mask_attribute_path` walks to a leaf and assigns,
with nothing to read the same path back. `_read_attribute_path` mirrors that walk exactly,
because the two have to agree on what a path means or the sweep collects a different value
from the one that was masked. It reads from the untouched original rather than the copy
being masked -- after the first path is masked the copy holds the sentinel there, and
sweeping for that would do nothing.
Outputs are worth noting separately: an output's plaintext was discarded when it was
masked, so nothing else knew it was a secret, and the same value in an ordinary attribute
stayed in cleartext.
Six tests, in both shapes, all failing before this change. Including the two bounds that
keep a value-based sweep honest: `region` survives, and a value under
MIN_SWEPT_SECRET_LENGTH is not swept, so masking `Env: dev` does not redact every `dev` in
the document.
…pentest F1) Every attacker-influenced string in the report was interpolated raw or wrapped in a single backtick, and a backtick *in the value* closes that span so the remainder renders as markdown and HTML. A pull-request author controls the terraform a plan is built from, so they controlled the report a reviewer reads: the pen test produced a fake "all policies passed" banner and a link whose text said app.stackguardian.io and whose href pointed elsewhere. Reviewers get that by email too. The verdict and the exit code were never affected -- this is report corruption, not a gate bypass. The report named three sinks. There are eight, and two it did not mention: the infracost `currency` and the `str(monthly)` fallback go inside `<sub>`, and the run URL goes inside an `href`. Also `rule_name` was the only field with no wrapping *at all*, in a table cell and inside `<summary>` -- the strongest sink in the file. Two primitives, because the sinks are in two different languages: `_code()` for markdown contexts. A code span whose fence is one backtick longer than the longest run inside the value, per CommonMark, so it cannot be closed from within. Inert by construction rather than by enumerating dangerous characters. Escaping was the alternative and is worse here: the engine deliberately puts backticks in its own messages (`json_format_value` wraps every compared value), so escaping them puts visible backslashes through every finding, and it holds only while the character list stays complete. Newlines collapse to a space, and pipes become `\|` in table cells -- GFM's documented escape and the one that works inside a span. `_html()` for values going into `<summary>`, `<code>`, `<sub>` and the `href`. A code span is wrong there: GFM does not reliably render markdown inside inline HTML. It also escapes backticks, which `html.escape` does not -- they cannot close a span in the summary because there is none, but an odd one OPENS one that swallows the markdown after it, so ``cost-control` `` still distorted the report with the tags already neutralised. Found while verifying, not while designing. `check.py` also now quotes org, workflow group and workflow id into the run URL, the way client.py already quotes the same three on every API path. Nine tests, all failing before this change, asserting against markdown **rendered by a CommonMark parser** rather than against the source -- the payload is still in the source by design, inside a span where it is inert, so a substring check on the source proves nothing. That was the mistake I made first while verifying this. One visible change beyond the fix: `rule_name` now renders in a code span like `policy_id` already did, so ordinary rows gain backticks around the rule. Consistent, but not byte-identical to before -- I had assumed benign output would be unchanged and a test proved otherwise.
The rename to `remote` was made on the argument that "platform check" can read as *a check of the platform*, which is what `--platform` means in most tools. Reverted: the vagueness is minor next to the cost of having two names in circulation, and the concern that prompted it was really that the open-source surface could not gate at all -- which `--fail-on-error` fixed, and no rename would have. No alias in either direction. Nothing is released -- py-tirith is not on PyPI and the action pins a branch -- so there was never a caller to keep working, which is what made both this and the original rename cheap. `docs/remote-check.md` moves back with git mv so the history follows, and both embedded `--help` blocks are regenerated under the restored name. The dispatch test now pins the outcome rather than the direction: exactly one subcommand name, and `remote` is not quietly still accepted. Kept from the rename work, because neither depended on the name: the one-column continuation fix in `--help`, and `status.py` no longer naming a subcommand at all in its exit-code comment.
refeed
changed the title
tirith remote check — a pre-plan policy step, no platform changestirith platform check — a pre-plan policy step, no platform changes
AkashS0510
previously approved these changes
Aug 13, 2026
arunim2405
approved these changes
Aug 13, 2026
arunim2405
previously approved these changes
Aug 13, 2026
The opening still described a StackGuardian-coupled policy framework, which is no longer what this is. Lead with what it does to a pipeline and with the reason it is a plugin -- one policy set covering every CI system you run it from -- and move StackGuardian to one late mention as the optional platform mode. Also: a pinned-version install example, a note that PyPI's `tirith` is an unrelated project so nobody installs the wrong package, and a CI section with the two-line GitHub Actions form and a GitLab job, since the CLI is the only route on non-GitHub runners.
|
❌ The last analysis has failed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 is told which bundle to read; the run body names no archive. That iswhat removed the last api dependency — no serializer field, no
data.key, no?contentType=.The name is per commit, and travels per run.
tirith-bundle-<sha7>-<tag>.tar.gz.A single shared name would be one that two concurrent runs can overwrite — and the action derives one
workflow id per repository, so two open pull requests is the ordinary case, not a corner. One run
would then evaluate the other's code and report the verdict as its own, silently, on a merge gate.
Per-run naming is possible because core merges the run's
TerraformConfigover the workflow's(
workflowruns/__init__.py:1646), so each run sends its ownprePlanWfStepsConfig. The copy storedon the workflow is only a fallback —
ensure_workflow409s for an existing workflow and updatesnothing. Verified on QA: the same entry sent as a top-level
WfStepsConfigis silently discardedfor TERRAFORM workflows (core synthesises the steps), which is why it must travel inside
TerraformConfig.TerraformConfigis already declared onWorkflowRunSerializer, so still no apichange. The merge is shallow, so the entry is sent complete and nothing else goes in it —
terraformVersionandmanagedTerraformStatekeep coming from the workflow.Two further constraints on the name: it must match none of the sync's exclude patterns (
sg.*,*__sg.*,*pci_*, the compliance globs) or it never reaches the container — the old__sg.prefixexisted precisely to keep it out of that sync — and it must not be
tfstate.json, which at theartifact root is a managed-state workflow's live state.
Accepted cost: growth. The artifact prefix has no lifecycle rule, neither sync passes
--delete,and api serves only GET and POST on artifacts, so bundles accumulate and every later run downloads
all of them. Correctness over transfer cost;
delete_artifactis kept for a retention sweep.On the content type:
file_upload_urlsignsapplication/jsonwhatever the filename, and S3 checksthe signature against the header the client sends, not the body. So the PUT sends
application/jsonwith a gzip body; the stored object is merely labelled wrongly, which nothing reads.
Consequences worth stating plainly:
planrun, scheduled drift proceeds off it. Neither behaviour isobviously right; this one is at least not silent.
The bundle's shape
The archive is a contract — the step reads its inputs from it, and other systems read it to see the
code a verdict came from:
Documents stay at the root and that is load-bearing: the step joins those names onto the extraction
directory and treats absence as normal, so moving one under a prefix would not raise — every policy
would report unevaluated and the run would look like it passed with warnings.
metadata.jsonexists because a consumer holding only the bundle could not tell which repository orcommit produced it, which subdirectory
code/came from, or whether absent code meant "none wanted" or"dropped for size".
code.repo_pathis the field that cannot be recovered any other way — members arenamed relative to
--source-dir, so--source-dir infra/prodmeanscode/main.tfbelongs atinfra/prod/main.tf. snake_case, VCS-neutral (providersniffed from the host,unknownrather thanguessed), and every repository field nullable so a local run degrades honestly instead of inventing a
repo. Any credential in the URL is stripped before it is written — this file outlives the run.
Private repositories
A run sends
VCSConfig: {}to suppress the checkout. The workflow keeps its own config so thedashboard still shows the repository, but core resolves the run's copy as
data.get("VCSConfig", wfDetails.get("VCSConfig", {}))— a present empty value beats the workflow's.Without it every platform-mode job on a private repository ERRORED in
pre_0_step, before the stepran:
fatal: could not read Password for 'https://None@github.com'. Public repositories hid itentirely, because an anonymous clone succeeds. It also means the clone stopped putting unmasked
source in the run workspace, which was quietly undercutting the point of masking client-side.
Fixed from a penetration test
Two findings, both reproduced before the fix and re-verified after.
Report spoofing (F1). Every plan- and policy-derived string was interpolated into the comment raw or
wrapped in a single backtick, and a backtick in the value closes that span so the rest renders as
markdown and HTML. A pull-request author controls the terraform a plan is built from, so they controlled
the report a reviewer reads — the tester produced a fake "all policies passed" banner and a link whose
text said
app.stackguardian.ioand whose href pointed elsewhere. The gate itself was never affected.Now escaped once in the renderer, so the comment, the check summary and the job summary are all covered.
Two primitives, because the sinks are in two languages: a code span with a dynamic-length fence for
markdown contexts (inert by construction, rather than by enumerating dangerous characters), and
html.escapeplus backtick-to-entity for values going inside<summary>,<code>,<sub>and thehref. The report named three sinks; there were eight. Verified against markdown rendered by aCommonMark parser and an HTML parse of the anchor — asserting on the markdown source proves nothing,
since the payload is still there by design, inside a span where it is inert.
State documents leaked computed mirrors (F2).
redact_planalready swept the plaintext of everymarked value across the whole document, precisely because a provider writes computed mirrors carrying
the same secret with no sensitivity marker —
tags_allis the confirmed case.redact_statedid not,so a secret in a tag was masked at
tags.Passwordand uploaded in cleartext attags_all.Password, then retained in the bundle. Same hole, worse place: state carries every attributeof every resource. It now collects at all four masking sites and sweeps on the way out. Outputs leaked
the same way and the report did not mention it — a masked output's plaintext was discarded, so nothing
knew it was a secret, and the same value in an ordinary attribute stayed in cleartext.
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, on both surfaces:
0passed,3a policy failed,1no verdict could be reached.3only with--fail-on-error.--fail-on-errornow works on the local form too (tirith -policy-path … -input-path …), whichpreviously exited
0pass or fail and so could not gate anything — the open-source path could onlyblock a merge by talking to StackGuardian. Default is still
0for compatibility.The discriminator is
final_result, which is tri-state:True→ 0,False→ 3,None→ 1.Nonemeans every check was skipped, so nothing ran — not a pass. An earlier attempt gated on
errorsandinverted both halves: that field carries the informational "these ids are not defined and have been
removed" note, so a genuine violation whose expression had a typo exited 1 while a policy naming an
unknown provider exited 3.
Verified on QA
Latest full E2E, on a private repository —
run 31605798822,
all seven cases green: plan, plan-file, state, two-phase, infracost, defaults, local.
The assertions run through the documented consumer path (run →
bundlePath→get_artifact→ signedURL → untar) and check the bytes that actually left the runner: masked documents at the archive root,
source under
code/, the unmasked committedplan.jsonabsent, andmetadata.jsoncarrying nocredential. Infracost priced for real (
$23.832), so the cost policy compared a real number ratherthan passing against a
$0that meant "could not price this".Also confirmed: runs reach
COMPLETEDaton_0_tirith-iac-governancewithgenerate-terraform-plancarrying no status entry at all, and a control workflow with no pre-plan step still runs terraform.
Depends on StackGuardian/sg-run-controller#301, which fixes exit 12 being honoured only when the
step container was still running when polled. Without it the fast cases (
source-dir: "") error.Tests
535 across the suite. The 11 that fail need a
terraformbinary and an editable install — prerequisites CONTRIBUTING does not currently mention.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.