fix(triune): fail closed in the two renderers (Copilot findings from #113) - #115
Merged
Conversation
…113) 1. render-cluster-member.py: load_inventory() returned yaml.safe_load(...) directly, so an empty file (None) or a non-mapping root would crash resolve_member() on .get(). Now refuses with a clear error when the root is not a mapping; main() wraps the load in the fail-closed handler so the CLI exits 1 without a traceback. 2. render-admission-pack.py: build_pack() trusted --dry-run-result even when the --dry-run-output artifact recorded its own , allowing a hash-bound pack that claims one verdict while its evidence says another. Now reconciles the two and refuses on mismatch. Adds a refusal-path test for each. Full suite 372 passed. procyber/semantic/ untouched.
There was a problem hiding this comment.
Pull request overview
This PR tightens Triune renderer behavior to fail closed on malformed inputs and to prevent inconsistencies between CLI flags and hash-bound evidence artifacts, adding targeted regression tests to ensure these refusal paths are enforced.
Changes:
render-cluster-member.py: refuse inventories whose YAML root is not a mapping and surface load failures without emitting a record.render-admission-pack.py: refuse packs where--dry-run-resultcontradicts the dry-run artifact’s own recordedresult.- Add tests covering non-mapping inventories and contradictory dry-run results.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/triune/render-cluster-member.py | Adds root-type validation for inventory YAML and wraps load/resolve in a fail-closed error handler. |
| tools/triune/render-admission-pack.py | Adds reconciliation check to prevent CLI-reported dry-run results from contradicting the hash-bound dry-run output. |
| tests/test_triune_cluster_member_render.py | Adds tests ensuring empty/non-mapping inventory YAML is refused. |
| tests/test_triune_admission_pack_render.py | Adds a test ensuring contradictory --dry-run-result vs artifact result is refused. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
155
to
+165
| dr = load_dry_run(dry_run_path) | ||
| # Fail closed: the pack is hash-bound to this artifact, so the CLI's | ||
| # --dry-run-result must not contradict the artifact's own recorded result. | ||
| # Otherwise a pack could claim `pass` while the hash-bound evidence says `fail`. | ||
| output_result = dr.get("result") | ||
| if output_result is not None and output_result != dry_run_result: | ||
| raise ValueError( | ||
| f"--dry-run-result={dry_run_result!r} contradicts the dry-run output's own " | ||
| f"result={output_result!r} in {dry_run_path}; refusing to emit an " | ||
| "inconsistent, hash-bound pack" | ||
| ) |
Comment on lines
+57
to
+63
| data = yaml.safe_load(path.read_text(encoding="utf-8")) | ||
| if not isinstance(data, dict): | ||
| raise ValueError( | ||
| f"inventory {str(path)!r} must be a YAML mapping at its root; got " | ||
| f"{type(data).__name__} (an empty file parses as null). Refusing to render." | ||
| ) | ||
| return data |
- render-cluster-member.py: convert yaml.YAMLError (malformed YAML syntax) into a clear ValueError instead of leaking a traceback. - render-admission-pack.py: refuse when the dry-run output JSON root is not an object (list/scalar would crash dr.get(...)). Adds a refusal test for each. Full suite green.
Member
Author
|
Addressed both Copilot comments in the latest commit: malformed YAML now converts |
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
Fixes the two real fail-open bugs Copilot flagged during review of #113 — they live in the Triune renderers (merged via #112), not the semantic kernel.
1.
render-cluster-member.py— non-mapping inventory crashes.load_inventory()returnedyaml.safe_load(...)directly. An empty file parses asNoneand a list/scalar root as a non-mapping, soresolve_member()then crashed oninventory.get(...). Now it refuses with a clear error when the root is not a mapping, andmain()wraps the load in the fail-closed handler (exit 1, no traceback).2.
render-admission-pack.py— CLI result can contradict the hash-bound artifact.build_pack()trusted--dry-run-resulteven when--dry-run-outputrecorded its ownresult. That allowed a pack to claimpasswhile the hash-bound evidence saidfail. Now the CLI flag is reconciled against the artifact's ownresultand refused on mismatch.Tests
test_non_mapping_inventory_is_refused(empty file + list root).test_flag_result_contradicting_output_result_is_refused.exit 1, clear error, no traceback.procyber/semantic/untouched.🤖 Generated with Claude Code