fix(core): a tolerated skip no longer erases an earlier verdict (#293) - #365
Merged
Merged
Conversation
Closes #293. generate_evaluator_result rolled resources up with one variable that every branch wrote to: a failing resource set it False, a tolerated skip set it None, unconditionally. So a violating resource followed by a skipped one reported the evaluator as skipped, the None id was removed from eval_expression, and the violation vanished. A passing resource followed by a skipped one reported skipped too, so a plan with one compliant and one destroyed resource exited 1. Both depended on the order of resource_changes, and a destroyed resource is severity 0, which is tolerated at every error_tolerance, so every terraform_plan attribute policy was exposed on every plan that destroys a resource of the type it checks. The roll-up is now two flags decided once at the end: any failure fails the evaluator; otherwise it passes if at least one resource was actually evaluated; only when every resource was tolerated away is it skipped. The all-skipped case, the hard-failure branches for bare provider errors and above-tolerance severities, and the empty-input case are unchanged. [PASS, skip, PASS] is now True. It used to print "Passed: 0 Failed: 0 Skipped: 1" and exit 1, which read as a tool failure for a plan that was compliant. Regression tests cover both orderings of fail+skip, three shapes of pass+skip, all-skipped, and a bare provider error followed by a skip. Verified against real plans: a compliant bucket next to a destroyed one now exits 0 (was 1); an open security group next to one with no ingress blocks now exits 3 (was 1); an RDS instance with retention 3 next to one with retention unset now exits 3 (was 0).
AkashS0510
requested review from
Akshat0694,
arunim2405 and
refeed
as code owners
September 4, 2026 11:28
|
❌ The last analysis has failed. |
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
refeed
approved these changes
Sep 4, 2026
arunim2405
approved these changes
Sep 4, 2026
AkashS0510
added a commit
that referenced
this pull request
Sep 4, 2026
PR #365 changed the roll-up so a tolerated skip no longer erases a sibling's verdict. Both skills now state the fixed behaviour where they used to warn about the bug, and the SSH example's notes say a skipped group leaves the bastion's failure standing. Re-verified: the probe plan that masked the bastion before now exits 3, and a compliant bucket next to a destroyed one exits 0.
refeed
added a commit
that referenced
this pull request
Sep 27, 2026
* feat: restore and expose the Skills page in the documentation * Refactor documentation styles and enhance agent skills installation - Updated CSS for verification lines to improve layout and readability. - Introduced a new DocBreadcrumbsWrapper component to integrate CopyPageMenu with breadcrumbs. - Added styles for the new DocBreadcrumbsWrapper component. - Refactored PaginatorNavLink to use a reusable Arrow component for navigation arrows. - Updated installation instructions in multiple documentation files to specify versioning for Tirith. - Added a new agent skills documentation file detailing the installation and usage of the Tirith skill pack. - Enhanced CI integration documentation to clarify usage and improve instructions. - Created a skill.sh script for installing the Tirith policy skill, ensuring a clean and safe installation process. * Ship a worked example with the skill pack, and fix stale exit-code text The skill's core instruction is "run the policy against a plan that should fail it", but the pack pointed at src/tirith/tui/examples/, which does not exist in a project that installed via skill.sh. examples/required-tags/ now ships with the pack: the SKILL.md policy, a plan that fails it (exit 3) and one that passes (exit 0). skill.sh downloads it alongside the markdown; all four exits in its README were run. Exit 2 was described three ways, all stale: "declared in status.py but never returned" (SKILL.md, Cursor rule), "platform check timeout" (verdicts.md, platform.md). status.py no longer declares it, and a platform timeout raises CheckError, which exits 1. The pack now says 2 is argparse's usage error and Tirith has no timeout code. The `tirith lint` warning was explained in full in SKILL.md, schema.md and validate.md, each with its own roadmap speculation. validate.md is now the one place; the others are a sentence and a pointer. The "When lint ships" section is gone: a skill should describe the present, and three copies of a forecast go wrong together. SKILL.md is restructured around lists rather than paragraphs, with the same facts. * Add tirith-migrate: a skill for translating Sentinel policies into Tirith A separate skill rather than a reference inside tirith-policies, because the shape is the same for every source language (inventory, classify, translate, verify, report) and only the mapping tables differ. Sentinel is the first; Checkov and Rego slot in as further reference files. The mapping is measured, not guessed. All 110 policies in hashicorp/terraform-sentinel-policies and the CIS AWS policy set were classified: 41 translate exactly, 40 approximately, 29 not at all. The full table ships as reference/sentinel-corpus.md so an agent can look a public policy up instead of re-deriving it. The two dominant losses are per-block conjunction (issue #316) and instance-level pairing across resources; tfconfig-only policies account for most of the cloud-agnostic set. Five worked examples, one per fidelity story, each verified against the engine. Approximate translations ship diverges.json, a plan where Sentinel and Tirith disagree, because a fidelity note nobody can run is a fidelity note nobody reads. Three engine behaviours the translation tables depend on were verified rather than assumed: Contains on a map tests keys; a present-but-null attribute is evaluated, not skipped; and the action operation emits one result per action, so a destroy guard does not fire on a replacement. skill.sh installs both skills unconditionally. All downloads still complete before anything is copied into place, so a failed fetch of either pack installs neither. * Fix the destroy-guard guidance and four other defects found by a live skill test A second Claude session installed both skills over the network, wrote three policies from plain-English requirements and reported what it found. Each finding was reproduced against the engine before being fixed. - terraform-plan.md's action example used "destroy"; plan JSON says "delete". Copied literally, the guard exits 0 on a real delete. The section is rewritten around the fact that `action` emits one result per action element: `NotEquals "delete"` blocks deletes and replacements, `ContainedIn ["delete"]` with `!` blocks only a pure delete, and the order of a replacement's two actions cannot be tested. - The migrate example prevent-database-destroy moves from approximate to exact using the universal form; its divergence plan becomes a second failing plan. sentinel.md and two corpus rows are corrected the same way; one row had put a list into NotEquals, which never equals an action string and passes everything. - A type-scoped policy on a plan with none of that type exits 3 by default and 1 with error_tolerance 1; there is no "nothing in scope, pass". Documented in verdicts.md and as a trap in SKILL.md. verdicts.md's exit table no longer claims 0 for that case, and debug-ci.md no longer sends the reader there. - The docs page said "no restart"; a running session did not see the newly installed skills until restarted. Qualified. * Warn about skip-masking (#293) and null list attributes in both skills The live test's third phase migrated a four-policy Sentinel set and found that the skill's own restrict-ssh-ingress example returns no verdict when the plan also holds a security group with no ingress blocks: the tolerated skip erases the bastion's failure (issue #293). Both skills now say that the error_tolerance-2 "where the attribute exists" idiom is unsafe on mixed plans until #293 is fixed, and the plan reference recommends one evaluator per type over a wildcard with tolerance. Also from that phase: an unset optional list renders as null in plan JSON and Contains or NotContains on null is a hard unsupported-type failure that no tolerance forgives, so the `x else default` idiom row is split; scope differs even for exact rows because Sentinel's find_resources excludes no-op and deleted resources; an approximation that drops the test a param fed must name the orphaned param rather than ship an unread variables.json; and the mock-to-fixture recipe now says to read the test .hcl for the expected verdict first. * Correct three claims found by a second smoke test, and footnote the helper table A fresh session auto-discovered both skills, ran fourteen fixtures and probes with every exit code as predicted, and checked the text against the engine. Three claims were wrong or half right: - An unknown condition.type does exit 3 with `errors` empty, but its result message names the evaluator. The skill said it was indistinguishable from a real violation, which sent the reader to the plan instead of to the message. Six passages corrected. - Exit 2 is never returned. The CLI catches argparse's SystemExit and exits 1, so a bad argument or `tirith lint` is 1, not 2. The morning's correction had replaced one wrong story about exit 2 with another. - `tirith --version` is mentioned in install.md but not where the install command is given. The Sentinel helper table now says its fidelity column rates the test only: scope and a computed attribute can still make a translation approximate. The smoke test's migration of an allowed-regions policy on aws_s3_bucket was exactly that case, since `region` is usually inherited from the provider and absent from change.after. * feat: Add Terraform and Kubernetes providers with lessons and documentation - Introduced Terraform plan provider with lessons covering resource attributes, actions, and counts. - Added Kubernetes provider with lessons on manifest handling and attribute paths. - Updated the lessons structure to support multiple providers and their respective lessons. - Enhanced the learn page to display lessons grouped by provider, improving navigation. - Updated styles for track headers to maintain consistency across the documentation. - Expanded provider overview documentation to include guidance on creating new providers and examples of potential use cases. * Drop the #293 warnings now that the engine fix has merged PR #365 changed the roll-up so a tolerated skip no longer erases a sibling's verdict. Both skills now state the fixed behaviour where they used to warn about the bug, and the SSH example's notes say a skipped group leaves the bastion's failure standing. Re-verified: the probe plan that masked the bastion before now exits 3, and a compliant bucket next to a destroyed one exits 0. * feat(learn): update lesson numbering and enhance track chooser functionality * Add lint and format documentation, update exit codes, and enhance skills - Introduced new documentation for `tirith lint` and `tirith fmt` commands, detailing their usage, flags, and exit codes. - Updated `llms-full.txt` and `llms.txt` to reflect the addition of linting and formatting capabilities, including their integration in CI and pre-commit hooks. - Enhanced the `skill.sh` script to download three skills: `tirith-policies`, `tirith-standards`, and `tirith-migrate`, with appropriate file structures. - Clarified exit codes for `tirith lint` and `tirith fmt`, ensuring consistency across commands. - Added examples and notes regarding the usage of the new commands and their integration into development workflows. * tirith-migrate: Azure Policy as a second source Akshat needs Azure Policy built-ins translated into Tirith for a demo in the Azure test subscription. The migrate skill was built so a new source is a reference file plus verified examples; this adds both. reference/azure-policy.md leads with the two decisions that make the translation more than a rename. Polarity: an Azure `if` describes the violation, so every leaf becomes its opposite and `anyOf` in the violation is `&&` in the compliance test, while `allOf` over two attributes of one resource is approximate (stricter) because Tirith cannot bind both tests to one resource. Target: Terraform plan via the alias-to-azurerm crosswalk, or ARM template via the json provider and the type-guard idiom. Then the operator table, effects (deny, audit, the remediation effects that are not evaluation), modes (Indexed is error_tolerance 2 on location/tags), parameters (-var, and why a type list cannot be one), and what to expect. The crosswalk rows are attributes the tirith-policy-corpus evaluates end to end in its Powerpipe terraform-azure translations, plus the azurerm renames (https_traffic_only_enabled, allow_nested_items_to_be_public). The corpus also classified 713 Azure built-ins for the ARM target: 376 translated, all approximate because the json provider scopes the document, and 110 blocked with taxonomy codes. reference/azure-policy-corpus.md lists all 486 by GUID and display name so an agent looks a built-in up before translating it. The corpus skipped exactly the built-ins a subscription is assigned first, because they are parameter-driven and untyped: Allowed locations, Require a tag, Allowed and Not allowed resource types, Allowed VM SKUs. examples/azure-policy/ translates each of those to the Terraform target, plus Secure transfer (Terraform and ARM), an NSG rule as the approximate case, and Inherit a tag (modify) refused in words. Every fixture was run: 19 plans, each exiting as its notes say. Two engine facts were verified for them: `count` honours exclude_resource_types over `*`, which makes an allow-list of resource types expressible, and `count` of an absent type is 0. * skill.sh: one archive download instead of a hundred raw fetches Installing over a slow link took more than ten minutes and looked hung: the script fetched each of ~100 files from raw.githubusercontent.com in sequence, printing nothing until all had arrived, and each fetch was taking four to five seconds. A single codeload tarball of the branch (4.9 MB) took 1.4 seconds on the same link. The installer now downloads that one archive, extracts it to a temporary directory, and copies every directory under .claude/skills/ that has a SKILL.md into place. Consequences: the three hand-maintained file lists are gone, so a new example no longer has to be registered anywhere; a new skill directory installs without touching the script; each pack is replaced wholesale on re-run, so a fixture renamed upstream no longer lingers; and a tag works as a ref as well as a branch, since refs/tags is tried when refs/heads is not found. Same flags, same output shape, same all-or-nothing behaviour. TIRITH_SKILL_ARCHIVE overrides the URL so the script can be tested against a local `git archive` tarball, which has no top-level directory; both layouts are handled. Verified: local tarball with --cursor, identical to the checkout; a stale file in an installed pack removed on re-run; unknown flag exits 2; the real network install of this branch in 2.2 s, identical to the checkout. * fix(docs): correct stale 1.2.0 claims and skill installer issues - pin installs and the pre-commit rev at 1.2.1, which ships lint, fmt and the hooks - exit 2 is argparse rejecting a flag, not "never returned" - skill.sh: one bare-ref archive URL so --ref takes a branch, tag or commit; --help works when piped from curl; say a skill directory is replaced wholesale - CopyPageMenu: hand the pending text to the clipboard so Safari keeps the click's activation - verdicts.md: point type-scoped policies at the count guard, not an advisory exit 1 - playground: treat an empty `after` as no changes, as the engine does - skills page: fifteen references, not fourteen --------- Co-authored-by: Akash S <96624761+AkashS0510@users.noreply.github.com> Co-authored-by: Akash S <akashsuresh0510@gmail.com> Co-authored-by: Rafid Aslam <rafid.aslam@stackguardian.io>
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
3 participants
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.
Closes #293.
The bug
generate_evaluator_resultrolled resources up with one variable that every branch wrote to. A failing resource set itFalse; a tolerated skip set itNone, unconditionally. Two consequences, both order-dependent:Noneid was removed fromeval_expression, so the violation vanished. Under!idthe policy passed.A destroyed resource is severity 0, tolerated at every
error_tolerance, so everyterraform_planattribute policy was exposed on every plan that destroys a resource of the type it checks. Theerror_tolerance: 2idiom ("where the attribute exists, it must be X") was exposed on any plan mixing resources with and without the attribute.The fix
Two flags, decided once at the end:
FalseTrueNoneThe all-skipped case, the hard-failure branches (bare provider error, severity above tolerance) and the empty-input case are unchanged.
[PASS, skip, PASS]is nowTrue; the issue asked for this to be decided here.Verdict change
This changes exit codes on real plans, so CHANGELOG labels it a verdict change under Fixed:
error_tolerance: 2GreaterThanEqualTo 7with tolerance 2Tests
Regression tests for both orderings of fail+skip, three shapes of pass+skip, all-skipped, and a bare provider error followed by a skip. core, cli, providers, platform and the readme tests pass.
tests/tui/test_app.pyhangs onmainas well in this environment and was excluded from the local run.Found while a second agent session exercised the new agent skills (#361) against the engine; those skills carry a warning about this bug that can be removed once this merges.