docs(adapters): Windows containment stopped being unimplemented - #344
Merged
Conversation
Three comments still described the code as it was before #331 and #342. This repository treats prose describing code that no longer exists as a defect, and one of the three was user-visible: `command_flow()` told a Windows adopter "egress containment is not implemented on Windows yet", which is now simply false. The wording had to change for a second reason, and it is the more interesting one. That reason was written when the answer was permanent — nothing on Windows could be contained, so the tier said so. Now the tier is decided by the RUN, and `command_flow()` is a PRE-RUN probe. Its job changed from "reporting a limitation" to "not predicting a result". So it says what is true of a run that has not started: not contained, and here is whether this host can support it. The `Enforced` arm stays absent for a reason that outlived the filters landing — seven steps can fail after the probe passes, and a prediction reported as a result is the false green of #300 and #301 by optimism rather than by silence. The test that pinned the old wording asserted on the roadmap phrase, so it would have gone green forever while the message rotted. It now asserts the negative: the reason must NOT claim the feature is unimplemented, and the tier must not be optimistic. Same claim it always made, minus the hostage to a sentence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XdXrbksFKirm7yW6EDunur
There was a problem hiding this comment.
Pull request overview
Updates user-facing and internal documentation around Windows egress containment to reflect the post-#331/#342 reality: Windows containment is implemented, but Containment::command_flow() remains a pre-run probe that must not predict enforcement. Also hardens the Windows-specific test to assert the invariant (never optimistic) without pinning to a single roadmap sentence.
Changes:
- Updates the Windows
Containment::command_flow()“not contained” reasons to describe the pre-run/probe nature rather than claiming “not implemented”. - Renames and refocuses the Windows test to assert non-optimism and avoid depending on a specific phrasing.
- Refreshes module-level docs in
lib.rsandegress_windows.rsto describe the now-existing end-to-end Windows path and the feature-gating rationale.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/flowproof-adapters/src/lib.rs | Updates crate-level comment to reflect Windows containment implementation and why achieved tier must come from the run. |
| crates/flowproof-adapters/src/egress.rs | Updates Windows probe docs, changes user-visible reason strings, and renames/updates the Windows test invariant. |
| crates/flowproof-adapters/src/egress_windows.rs | Updates module docs to describe the end-to-end Windows containment pipeline and gating constraints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //! asserts the tier. Claiming enforcement before a filter exists would let | ||
| //! `assert_no_egress` certify a run nothing was containing - the same false | ||
| //! green as #300 and #301 by a third route. | ||
| //! **Nothing here names `Containment`.** That is structural rather than a |
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.
Three comments still described the code as it was before #331 and #342. This repository treats prose describing code that no longer exists as a defect — and one of the three was user-visible.
The one that reached an adopter
Containment::command_flow()on Windows returned:It is implemented. That string prints on stdout and lands in
--jsonon every Windows agent run.Why the wording had to change anyway
The more interesting reason. That sentence was written when the answer was permanent: nothing on Windows could be contained, so the tier said so and there was nothing more to say.
Now the tier is decided by the run (#341), and
command_flow()is a pre-run probe. Its job changed from reporting a limitation to not predicting a result. So it now says what is true of a run that has not started:and, when the host can't:
The
Enforcedarm stays absent for a reason that outlived the filters landing: seven steps can fail after the probe passes — account, logon, privileges, desktop grant, engine, sublayer, filters, plus collection. A prediction reported as a result is the false green of #300 and #301 arriving by optimism rather than by silence.The test was a hostage to the sentence
enforced_is_unreachable_on_windowsassertedreason.contains("not implemented on Windows yet"). Its own doc comment said "the step that installs filters is the step that deletes this test" — but that step came and went, and the test would have stayed green forever while the message rotted.It is now
the_pre_run_prediction_is_never_optimistic_on_windowsand asserts the negative: the reason must NOT claim the feature is unimplemented, and the tier must not be optimistic. Same claim it always made, minus the dependency on one sentence surviving. Renamed, not deleted — ratchets confirm771 -> 771.The other two:
egress_windows's module doc opened "Still installs no WFP filter and launches nothing", andlib.rs's gate comment said "Today this module is the capability PROBE only." Both now describe the module as it is, and both keep the load-bearing part — that this module never namesContainment, because it is gated on thewindowsdependency whileContainmentis gated on theagentfeature, and collapsing that would cost the Win32 code its only typecheck without a Windows runner.Verification
cargo test --workspace --all-features— greencargo check -p flowproof-adapters --features sap-com --all-targets --target x86_64-pc-windows-msvc— cleancargo fmt --check— cleandiff size 93 <= 400, tests771 -> 771, nothing silencedStill open, deliberately
The behaviour half of the same defect. The CLI computes
containment(spec)and prints it before the run, then reuses that predicted tier for the--jsonpayload and the run record — so a Windows run that WAS enforced still reports as uncontained in those two places. The trace lane is already correct (check_egressresolves the run's tier beforecontainment_tag).Fixing it means threading the achieved tier out of
agent_flow::record/replayto four call sites including the run-record field, which is a behaviour change wanting its own tests. Not smuggled in here.Generated by Claude Code