Make plan predict deploy, and warn on silently-dropped source - #14
Merged
Conversation
Fixes #9, fixes #10, fixes #12, fixes #7. Plan/deploy parity (#9, #10). Invoke-Plan.ps1 was never updated when the deploy engine moved to create-or-leave, so it still described the old update-in-place model and disagreed with what a deploy would do. A diff that mispredicts the apply is worse than no diff, since the whole reason to run plan is to see the apply in advance. Plan now: - reports an AdvancedRule difference as RENAME REQUIRED rather than UPDATE, and states that deploy will leave the rule unchanged. The service rejects Set-DlpComplianceRule -AdvancedRule, so an update is not something deploy can perform, and promising one sends the operator to look for a fix that will never arrive. - fetches rules per policy with -Policy instead of individually by -Identity, which is what makes rules absent from the manifest visible at all, and reports them as REMOVE to match deploy's prune. - defers those removals under the same condition deploy does, reporting them as DEFERRED when a declared rule is missing or in PendingDeletion, since deploy suppresses pruning until every declared rule is in place. - reports rules in PendingDeletion, which deploy skips. Compiler warnings (#12, #7). Two cases where source compiled to less than it appeared to say, with nothing to tell the author: - generateAlert / generateIncidentReport given as a bare boolean deploy to nothing. The value is still accepted, because it is meaningful when codifying an existing tenant rule where a bare true records observed behaviour, but the compiler now says it will not be deployed and how to make it deploy. - A policy with no scope.group compiles to org-wide coverage. Scope is deliberately not made a schema requirement: org-wide is a legitimate end state that a tool should be able to express. But it should be a decision rather than the consequence of a missing key, so the compiler names the surfaces affected. Copilot is exempt, since it scopes per user. Warnings go to stderr and never fail the build. unscoped_locations() and boolean_only_actions() are pure and directly tested. The assistant prompt claimed "SCOPE IS REQUIRED", a guarantee the compiler did not provide, which is the inconsistency reported in #7. It now tells the model to always scope its own drafts and to ask rather than assume when a request appears to call for org-wide, while describing the compiler's actual behaviour. Also fixes the examples, which were themselves teaching the #12 defect: three shipped policies, the assistant's golden example, and the README all used generateIncidentReport: true and therefore compiled to nothing. They now use placeholder recipient lists and actually deploy. example-raw-advanced-rule.yaml deliberately keeps its boolean, since actions on a codified rule are observed documentation rather than intent. Verified: 18/18 compiler tests, 12/12 assistant tests, compile clean with no warnings on the shipped policies, powershell/*.ps1 pure ASCII and parsing, and Invoke-Plan.ps1 exercised against mocked cmdlets across in-sync, differing-rule, orphan, orphan-with-missing-rule, and PendingDeletion scenarios.
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.
Closes #9, closes #10, closes #12, closes #7. All reported by @jim-t-trevenx in #6.
Plan now predicts deploy (#9, #10)
Invoke-Plan.ps1was never updated when the deploy engine moved to create-or-leave, so it still described the old update-in-place model. A diff that mispredicts the apply is worse than no diff, since seeing the apply in advance is the entire reason to run plan.AdvancedRuledifference is reported as! RENAME REQUIREDinstead of~ UPDATE, and states that deploy will leave the rule unchanged. The service rejectsSet-DlpComplianceRule -AdvancedRule, so an update is not something deploy can perform; promising one sends the operator looking for a fix that never arrives.-Policyinstead of individually by-Identity. That is what makes rules absent from the manifest visible at all, and they are now reported as- REMOVEto match deploy's prune.DEFERREDunder the same condition deploy applies, when a declared rule is missing or inPendingDeletion, since deploy suppresses pruning until every declared rule is in place.PendingDeletionare reported, since deploy skips them.Compiler warnings (#12, #7)
Two cases where source compiled to less than it appeared to say, with nothing telling the author.
Boolean alert actions.
generateAlert: truevalidates and deploys nothing. The boolean is still accepted, because it is meaningful when codifying an existing tenant rule where a baretruerecords observed behaviour, but the compiler now says it will not be deployed and how to make it deploy.Org-wide policies. A policy with no
scope.groupcompiles to org-wide coverage. Scope is deliberately not made a schema requirement, as discussed in #7: org-wide is a legitimate end state and a tool that cannot express it is broken. But it should be a decision rather than the consequence of a missing key, so the compiler names the affected surfaces. Copilot is exempt since it scopes per user.Warnings go to stderr and never fail the build.
unscoped_locations()andboolean_only_actions()are pure and directly tested.Assistant prompt. It claimed "SCOPE IS REQUIRED", a guarantee the compiler did not provide, which is the inconsistency at the heart of #7. It now tells the model to always scope its own drafts and to ask rather than assume when a request appears to call for org-wide, while describing the compiler's actual behaviour.
The examples were teaching the bug
Turning on the #12 warning immediately flagged the shipped examples. Three policies, the assistant's golden example, and the README quickstart all used
generateIncidentReport: trueand therefore compiled to nothing, teaching the exact pattern the issue reports. They now use placeholder recipient lists and actually deploy.example-raw-advanced-rule.yamldeliberately keeps its boolean: actions on a codified rule are observed documentation rather than intent, and the warning correctly does not fire there.Verification
compile.pyruns clean with no warnings on the shipped policies, and the manifest now carriesGenerateAlert/GenerateIncidentReportwhere it previously dropped them.powershell/*.ps1pure ASCII and parsing.Invoke-Plan.ps1exercised against mocked S&C cmdlets across five scenarios: in-sync, differing rule, orphan, orphan-with-missing-rule, and PendingDeletion. Confirmed it reports RENAME rather than UPDATE, surfaces orphans, and defers pruning exactly where deploy does.#13 is left open as an enhancement.