Skip to content

m2 package source selection and filters

Kazushi Kamegawa edited this page Jul 25, 2026 · 1 revision

M2: Package source selection, filters, and outstanding PR review fixes

Status: Completed. Tracked by issue #4 (M2 milestone), sub-issues #34, #35, #36. Delivered as PR #87, stacked on PR #86 (stacked on PR #85).

Context

Issue #4 (M2: package enumeration) has 10 sub-issues. #27-#31 were covered by PR #86, #32/#33 by PR #85 (both open, not yet merged at the time this plan was written). The remaining three map directly to the unchecked items in docs/TODO.md M2:

  • #34 --source com|fs|auto switch (auto: degrade from COM to the filesystem scan on failure)
  • #35 --include / --exclude glob filters
  • #36 Unit tests for filesystem enumeration and COM/FS switching logic

Issue #4's acceptance criterion "every sub-issue is closed" also requires PR #85/#86 to merge, and both PRs had five outstanding Copilot review comments. Those were folded into this same body of work rather than left for a separate pass.

Branch stack: main <- feature/32-fs-package-scan (PR #85) <- feature/27-winget-com-source (PR #86) <- feature/34-source-selection (PR #87, closes #34/#35/#36).

Part A - PR #85 review fixes (on feature/32-fs-package-scan)

  1. FsScanSource.cpp skips a directory whose name derives an empty package id (a name beginning with _), instead of emitting an InstalledPackage with an empty id/name that downstream logic is not prepared to handle.
  2. TempDirectory.h (test helper) includes the process id in its generated directory name, not just an in-process counter, so two test-host processes running the same binary in parallel cannot collide.
  3. New tests cover both changes.

The third review comment on this PR - that FsScanSource treats every filesystem error as "no packages", contradicting IPackageSource's "throw on unrecoverable failure" contract - could not be fixed on this branch: the PackageSourceError type it needs to throw does not exist here yet. That fix moved to Part B.

Part B - PR #86 review fixes (on feature/27-winget-com-source, after merging Part A)

  1. WingetComSource.cpp: FindPackagesOptions / PackageMatchFilter activation moved from a per-call helper into the Impl constructor, wrapped in a try/catch translating any winrt::hresult_error into PackageSourceError via mapHresultToKind(). This resolved two review comments at once: an untranslated exception escaping the documented IPackageSource contract, and a header comment claiming construction alone activates everything, which was not true until this change. The comment was also tightened to state that enumeratePackages() can still throw at query time, and that a --source auto implementation must cover both windows.
  2. FsScanSource.cpp (deferred from Part A): now distinguishes "the Packages directory is absent" (the normal state of a machine with no portable packages - returns empty) from every other filesystem error (denied access, an I/O error), which now raises PackageSourceError(ScanFailed) per the interface contract.

Part C - --include / --exclude (#35)

New core/PackageFilter. This resolves the glob semantics that an earlier ADR had explicitly deferred to "the PR that implements PackageFilter":

  • Matching is per executable, tested against either the owning package's identifier or the executable's bare file name.
  • Comparison is ordinal and case-insensitive (CompareStringOrdinal), matching the existing installer-type check elsewhere in core/ - a locale-dependent case fold must not change whether a pattern matches.
  • Supported syntax is * and ? only - no character classes, no path semantics, because the matched values are an identifier or bare file name, never a path.
  • Exclude always beats include. A package left with no executables after filtering is dropped entirely.
  • The glob matcher backtracks iteratively, not recursively, so a pathological pattern cannot overflow the stack.
  • PackageFilter is pure logic applied to the result of package enumeration, not inside a source, so both sources filter identically. It is deliberately not wired to the parsed CLI options yet - that lands with the M6 CLI milestone, the first code with parsed options to hand.

Part D - --source com|fs|auto (#34)

New core/PackageSourceFactory, centered on AutoPackageSource:

  • The COM attempt happens inside enumeratePackages(), not in a constructor, so a single catch covers both the construction-time failure window (COM activation, catalog connect) and the query-time failure window (the enumeration call itself failing after successful construction).
  • Only PackageSourceError triggers degradation to the filesystem source. Any other exception propagates - it is not evidence that COM is unavailable.
  • A COM source that succeeds with zero packages does not degrade. A machine with no portable packages installed legitimately enumerates none; falling back to the filesystem there would be second-guessing a source that worked correctly.
  • Explicit --source com never degrades (its failure is the user's to see; the M6 CLI maps the failure kind to an exit code). Explicit --source fs never constructs the COM source at all.
  • The public header exposes no COM/WinRT type, matching the existing pattern for the COM source's own header, so it stays usable from the test binary, which has no include path to the generated COM projection.

Part E - tests (#36)

  • Glob matcher and filter-semantics tests, including a multi-* backtracking case that a naive greedy matcher gets wrong.
  • Source-selection tests using fake IPackageSource implementations (constructing a real COM source is not exercised by automated tests at all in this project - out-of-process COM activation for the relevant class has been observed to terminate the whole test process in the sandboxed development environment used, so the switching logic is verified against injectable fakes instead). Coverage: COM success, degradation from a construction failure, degradation from an enumeration failure, the degrade callback firing with the right error, degradation not firing on an empty-but-successful result, a non-source-error propagating instead of degrading, and both explicit-source paths.

Part F - documentation

  • docs/TODO.md M2 checked off.
  • A new ADR entry records: the glob syntax decision, the auto-degrade policy, and the FsScanSource behavior change (returning empty vs. throwing).
  • Architecture tree listings in the design doc and the agent instructions file updated with the two new modules.
  • Work log entry added with issue/PR references.

Outcome

  • PR #87 delivered Parts C-F; Parts A/B were pushed directly onto PR #85 / PR #86.
  • All five outstanding review comments were replied to individually and the review threads resolved.
  • Verified in all four build configurations (Debug/Release times x64/ARM64) at the project's warnings-as-errors setting, with zero new warnings.
  • Full test suite: 73 of 73 tests passed, up from 39 before this work, across all four configurations. The ARM64 runs were genuine native execution (the development machine used is Windows on ARM), not a cross-build-only claim.
  • The executable project still fails to link with an unresolved entry point, which is expected until the M6 CLI milestone adds main.cpp.
  • No new third-party dependency was introduced.

Clone this wiki locally