fix: harden Windows window target discovery - #2246
Merged
Merged
Conversation
Member
Author
|
hey @greptileai, please re-review the PR |
Member
Author
|
hey @greptileai, please re-review the PR |
Member
Author
|
hey @greptileai, please re-review the PR |
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.
Harden Windows window discovery around the empty CLI target-list report in #2164. Use the documented
EnumWindowstop-level API, log enumeration errors, and fix ignored-executable matching: the previous code compared names containing.exeagainst extensionless entries.Adds native Windows coverage for visible foreign-process top-level and owned-popup windows, hidden/minimized/child/tool/current-process exclusions, metadata required by recording targets, and HWND/WindowId round-tripping. The test initializes per-monitor DPI awareness like the CLI. The CI workflow also runs the same fixture against the PR base implementation and uses pinned action revisions.
Validation on head
626d994969f516153ccf058662ff11c397e81398: the native Windows Server 2022 job passed all 3 unit tests, 2 integration test targets, and crate Clippy with-D warnings. The base implementation also passed the integration fixture. This does not reproduce #2164, and the enumeration call alone is not a proven root cause of the report. Native test and baseline logs.Local checks also passed:
cargo fmt --all --check, macOScargo check --locked -p scap-targets, Windows-target checking/Clippy for the exact source and test targets with repository lints,actionlint, and isolated diff checks. Existing uncommitted v0.6 work is excluded from this PR.Release gate for #2164: verify the exact Windows 11 24H2/Camoufox CLI discovery and recording workflow. Window-content isolation is separate: Windows recording currently captures a display region, so moved or occluded windows require separate validation. This PR does not automatically close the issue.
Greptile Summary
This PR restores Windows target discovery by enumerating top-level windows, fixes ignored-executable matching by comparing executable stems, and adds native Windows regression coverage and CI validation.
EnumWindowswhile retaining visibility, minimized, child, tool-window, and current-process filtering.Confidence Score: 5/5
The PR appears safe to merge with no outstanding correctness or repository-rule issues.
The DPI-awareness and immutable-action findings were manually resolved, and the remaining base-commit finding is now fixed because the pull-request merge checkout retains two commits, including the exact base parent used by
git show.Important Files Changed
Reviews (3): Last reviewed commit: "ci: fetch the base revision for Windows ..." | Re-trigger Greptile
Context used: