[SG-320] feat(local): use tirith -policy-path as a CI gate — a directory of policies, masked input, and the verdict written out - #283
Closed
refeed wants to merge 2 commits into
Closed
[SG-320] feat(local): use tirith -policy-path as a CI gate — a directory of policies, masked input, and the verdict written out#283refeed wants to merge 2 commits into
tirith -policy-path as a CI gate — a directory of policies, masked input, and the verdict written out#283refeed wants to merge 2 commits into
Conversation
…cal check`
Local, credential-free policy evaluation has existed for a while, but only inside the
GitHub Action (`scripts/tirith_action/local.py`). A second CI front end — the GitLab
component this unblocks — would have had to fork it, which is exactly what that module's
own docstring says it exists to avoid. So it moves here, and both front ends drive one
implementation.
A sibling subcommand rather than a flag on `platform check`. That command is documented
top to bottom as "masks, packs, uploads, runs on StackGuardian, polls"; a path doing none
of those under the same verb makes its help text wrong for half its readers. Mechanically
it would be worse: `--workflow-id` is `required=True` there, so a flag would have to lift
that requirement out of argparse into a hand-rolled conditional, weakening the platform
path to accommodate a mode with no workflows — and ~20 other flags would become no-ops
needing per-flag conflict handling. `platform check` gains no flag at all, so its help
block and reference page are byte-identical.
Local mode is never entered implicitly. `platform check` with no credentials stays a hard
error rather than falling back, because a fallback evaluates whatever happens to be
committed and reports green when a token was simply misspelled. A front end that wants "no
credentials therefore local" chooses that itself. For the same reason the command rejects
every credential, workflow, archive and run flag instead of accepting and ignoring them.
One result document, two modes
------------------------------
Both modes now build `--output-json` through `report.result_document`, so the shapes cannot
drift by editing one caller. Every key exists in every mode; a key with no meaning on a
path is null rather than omitted or invented — omission forces every consumer to write two
parsers, and a fabricated `wfrun_url` links to a run that does not exist. A `mode`
discriminator says which path ran. Specified in docs/output-contract.md.
Two consequent changes on the platform side, both additive and both failing closed:
* The pre-poll write is now a full document instead of three keys, so a consumer reading
it after a timeout meets no partial shape.
* `CheckError` paths write both output files rather than returning having written
nothing. A caller editing a sticky comment in place needs a marker-first body there or
it orphans the comment it was updating — that has happened. `render_markdown` gained an
optional `notes=` so the report can say why, instead of claiming "the workflow run
finished as ERRORED" in a mode that has no run.
Gating
------
Exit codes mirror `platform check`: 0, 3 with `--fail-on-error`, 1 for anything that
produced no verdict. Two paths fail closed regardless of the flag, because both are tool
health rather than policy decisions: no policy files found at `--policy-path`, and a policy
that could not be evaluated at all. A green result for a change nothing was evaluated
against is the one outcome this mode must never produce.
Recorded rather than fixed: a policy whose every check was skipped reports `passed` here,
matching platform mode, while the flat surface exits 1 for the same policy on the grounds
that "nothing ran" is not a pass. Both readings are defensible, so the divergence is pinned
by a test to make it visible instead of letting either surface drift into the other. It
needs one decision across all three surfaces.
…nstead of a subcommand
Replaces the `tirith local check` subcommand added in the previous commit. Evaluating
policy files committed in a repository is what `tirith -policy-path ... -input-path ...`
has always done, so the capability a CI job needs belongs on that command as flags rather
than in a second command doing the same job slightly differently. Two surfaces for one
task drift, and the one people already know is the one to improve.
Three additions, all optional and all off unless asked for:
* `-policy-path` accepts a directory or a glob and evaluates every policy it finds. This
is new capability rather than changed behaviour: a directory previously reached
`open()` and failed with a bare `ERROR`. `-var-path` and `-var` apply to every policy,
so a parameterised policy behaves the same whether you name the file or its directory.
* `--input-kind` masks the document before evaluation. It matters even with nothing
uploaded, because evaluator messages embed the values they compared and those messages
are copied into whatever comment a CI job posts.
* `--output-json`, `--output-markdown`, `--comment-marker`, `--markdown-limit` and
`--sha` write the verdict out in the shapes `platform check` already writes, so one CI
integration drives either.
Keeping the frozen contract frozen
----------------------------------
This command's `--json` stdout is pinned byte-for-byte by
tests/core/test_output_compatibility.py, and masking changes the evaluator messages that
document contains. So the routing is deliberately narrow: a single policy file with none
of the new flags reaches the original code path untouched, which is why masking is opt-in
rather than automatic. A test pins that routing in both directions.
The all-skipped question, now decided
-------------------------------------
The previous commit left this open, because the subcommand and the flat surface disagreed:
a policy whose every check was skipped reported `passed` in one and exited 1 in the other.
With one surface there is one answer, and it is the released one -- exit 1, "nothing was
examined is not a pass". The report says so in words rather than rendering a green verdict
beside a red exit code.
This remains a deliberate difference from `platform check`, which counts skips separately
and reports them as a pass. Both readings are defensible; a single surface disagreeing
with itself depending on how many policies you pointed it at is not.
Docs
----
docs/local-check.md becomes docs/evaluating-policy-files.md, and the two doc-currency
guards in tests/test_readme_is_current.py follow it -- one of them caught a missing flag
while this was being written. The Usage block in the README is regenerated from the real
--help, and `--input-kind` takes a `KIND` metavar because spelling its four choices out
pushed every option's help text far to the right.
|
❌ The last analysis has failed. |
refeed
changed the title
tirith local checktirith -policy-path as a CI gate — a directory of policies, masked input, and the verdict written out
Member
Author
|
Closing unmerged, in favour of duplicating this logic into the GitLab CI component Recording what was here, in case it is wanted later:
The last three are platform-mode improvements that stand on their own merits and are not GitLab-specific. |
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
1 participant
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.
Why
Local, credential-free policy evaluation has existed for a while, but only inside the GitHub Action
(
scripts/tirith_action/local.py, 360 lines). The GitLab CI component this unblocks would have had tofork it — which is exactly what that module's own docstring says it exists to avoid. So it moves into
the CLI, and both front ends drive one implementation.
It moves onto the command that already does this,
tirith -policy-path … -input-path …, as flags —not into a subcommand of its own. Two surfaces for one task drift, and the one people already know is
the one worth improving.
What's new on the flat surface
-policy-pathaccepts a directory or a glob and evaluates every policy it finds — recursively for*.tirith.json, or failing that any.jsonfile shaped like a policy. This is new capability, notchanged behaviour: a directory previously reached
open()and failed with a bareERROR. The shapetest is load-bearing rather than defensive — a policy directory routinely also holds
plan.json, andwithout it the plan is evaluated as a policy, reporting a spurious failure that buries the real
findings.
-var-pathand-varapply to every policy, so a parameterised policy behaves the samewhether you name the file or the directory holding it.
--input-kindmasks the document before evaluation. This matters even though nothing is uploaded:evaluator messages embed the values they compared, and those messages are copied verbatim into whatever
comment a CI job posts, so an unmasked run publishes plan values to a code host. It also keeps a local
verdict identical to
platform check's for the same plan, because that path evaluates the maskeddocument too.
--output-json/--output-markdown/--comment-marker/--markdown-limit/--shawrite theverdict out in the same shapes
platform checkwrites, so one CI integration drives either.Keeping the frozen contract frozen
This command's
--jsonstdout is pinned byte-for-byte bytests/core/test_output_compatibility.py, andmasking changes the evaluator messages that document contains. So:
--input-kindrather than automatic;original code path untouched. A test pins that in both directions.
One result document, two modes
Both modes now build
--output-jsonthroughreport.result_document, so the shapes cannot drift byediting one caller. Every key exists in every mode; a key with no meaning on a path is
nullratherthan omitted or invented. Omission forces every consumer to write two parsers, and a fabricated
wfrun_urlrenders as a link to a run that does not exist. Specified indocs/output-contract.md.Two consequent changes on the platform side, both additive and both failing closed:
meets no partial shape;
CheckErrorpaths write both output files rather than returning having written nothing. A callerediting a sticky comment in place needs a marker-first body there or it orphans the comment it was
updating — that has happened.
render_markdowngained an optionalnotes=so the report can saywhy, instead of claiming "the workflow run finished as
ERRORED" in a mode with no run.Gating
0,3with--fail-on-error,1for anything that produced no verdict. Three paths fail closedregardless of the flag, because all three are tool health rather than policy decisions:
-policy-path— a green result for a change nothing was evaluated against isthe one outcome this must never produce;
a failure rather than a pass.
That last one is a deliberate difference from
platform check, which counts skips separately and reportsthem as a pass. Both readings are defensible; a single surface disagreeing with itself depending on how
many policies you pointed it at is not. Pinned by a test and documented.
Verification
tests/local/; 729 passing overall. The 11 failures on this branch arepre-existing on
main— they need aterraformbinary.--jsoncontract andtests/cli/pass unchanged.black==25.1.0(the version CI pins) andpydocstyleclean.npm run buildindocumentation/succeeds. The new page's flag block is fenced rather than indentedbecause MDX dropped indented code blocks and
{terraform_plan,…}would otherwise parse as JSX — thatfailed the build once here.
docs/evaluating-policy-files.mdfrom rotting the way the older pagesdid. One caught a missing flag while this was being written.
where it used to error, exit
3under--fail-on-error, exit1with no policies, and amarker-first body on every path.
What this unblocks
Tag
1.3.0, then the GitLab CI/CD componentpins it and the action's
local.pyis deleted in favour of it. The component is already built and itsself-test is green against this branch, pinned by commit sha until the tag exists.
Note SonarCloud fails here, as it does on #272, #279 and #280 — all merged.