fix(safeoutputs): support glob wildcards anywhere in allowed-tags patterns

[jamesadevine]

Summary

tag_matches_pattern only handled a trailing * (prefix match). Patterns with a wildcard in the middle — like copilot:repo=msazuresphere/4x4/*@main — silently fell through to exact-string comparison, rejecting valid tags such as copilot:repo=msazuresphere/4x4/VsCodeExtension@main.

This PR introduces a purpose-built wildcard_match() function that treats * as matching any character including / — tags and artifact names are not file paths, so / should not act as a segment separator. This replaces both the previous glob_match::glob_match call (which treated / as a separator) and the scattered hand-rolled strip_suffix('*') + starts_with patterns across safe-output modules.

Changes

• src/safeoutputs/mod.rs: New wildcard_match() function, plus tag_matches_pattern() (case-insensitive) and name_matches_pattern() (case-sensitive) wrappers
• src/safeoutputs/add_build_tag.rs: Consolidated to use tag_matches_pattern (gains case-insensitive matching it was missing)
• src/safeoutputs/upload_build_attachment.rs: Consolidated to use name_matches_pattern
• src/safeoutputs/upload_pipeline_artifact.rs: Consolidated to use name_matches_pattern
• queue_build.rs: Left as-is — branch matching has intentionally different semantics (case-sensitive, requires / separator)
• docs/safe-outputs.md: Updated to describe * wildcard support instead of "prefix wildcards"

Test plan

• cargo test — all 1186+ tests pass
• cargo clippy --all-targets --all-features — no new warnings
• New tests cover: exact match, trailing *, middle *, * crossing /, multiple *s, case sensitivity, and name_matches_pattern behavior

[github-actions]

🔍 Rust PR Review

Summary: Good fix with correct implementation — but has two documentation gaps and one undocumented behavioral regression worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/safeoutputs/mod.rs:212-220 — Stale docstring describes old behavior
  The doc comment above tag_matches_pattern still says:

  "Patterns ending with * are prefix wildcards: "agent-*" matches any tag whose prefix (before the *) case-insensitively equals the start of tag."
  "All other patterns are compared with case-insensitive exact equality."

  Neither bullet is accurate anymore — the function now performs full glob matching. The doc should be rewritten to describe glob semantics and note that * does not cross /.

• src/safeoutputs/mod.rs:224 — * no longer crosses / (behavioral regression for hierarchical tags)
  The glob_match crate's source explicitly states // ** allows path separators, whereas * does not. This means:

  • Old behavior: pattern = "team/*", tag = "team/subgroup/item" → tag.starts_with("team/") → matches ✓
  • New behavior: glob_match("team/*", "team/subgroup/item") → * stops at / → does not match ✗

  ADO build tags support /-separated hierarchies (for tag grouping). Any operator who relied on "prefix/*" to cover all sub-tags now gets a silent regression. There's no test for this case. If the intent is to keep the old semantics for trailing *, the pattern could be automatically normalized to "prefix/**" when there's a trailing * and no / after it, or the documentation should explicitly warn that * no longer matches across /.

⚠️ Suggestions

• docs/safe-outputs.md:75,381 — User-facing docs still say "prefix wildcards"
  Both occurrences say "Supports prefix wildcards: entries ending with * match by prefix". This should be updated to explain full glob semantics (e.g., * matches within a single path component, ** matches across /, etc.).

• src/safeoutputs/mod.rs — No test for * not crossing /
  Add a test asserting !tag_matches_pattern("team/subgroup/item", "team/*") to make the new semantics explicit and prevent future confusion. This makes the behavioral change visible and intentional in the test suite.

✅ What Looks Good

• Consolidating the duplicated inline matching logic in add_build_tag.rs to use tag_matches_pattern is a clean improvement — the old inline code also missed the case-insensitive comparison.
• Using the already-present glob_match dependency is the right approach; no new dep needed.
• New test cases in test_tag_matches_pattern_middle_wildcard cover the motivating scenario well.
• All 1185+ existing tests pass on the PR branch.

Generated by Rust PR Reviewer for issue #442 · ● 722.7K · ◷

[github-actions]

🔍 Rust PR Review

Summary: The updated implementation is correct and well-tested — the previous review's concerns have been addressed. One minor behavioral change in add_build_tag.rs is worth being explicit about.

Findings

⚠️ Suggestions

• src/safeoutputs/add_build_tag.rs:164 — exact-match patterns are now case-insensitive (was case-sensitive)
  Before this PR, the hand-rolled check in add_build_tag.rs used *pattern == final_tag for non-wildcard entries — a case-sensitive byte comparison. After consolidating to tag_matches_pattern, exact-match patterns like "Bug" now also match "bug" and "BUG". The PR description calls this "gains case-insensitive matching it was missing", but it's a breaking change for any operator relying on case-sensitive exact-match enforcement in add-build-tag's allowed-tags. ADO normalises tag case, so this almost certainly doesn't matter in practice — worth one sentence in the commit message or PR notes to close the loop.

• src/safeoutputs/mod.rs:234 — undocumented invariant: * in values is never a wildcard
  The doc comment correctly notes that * cannot be escaped in patterns (intentional). It also asserts ADO tags/artifact names cannot contain *, but this invariant isn't validated at input time. Since wildcard_match only treats * as special in the pattern (never the value), a rogue * in a tag value is harmless — but documenting "values are compared literally, * in values has no special meaning" would make the security reasoning explicit for future readers.

✅ What Looks Good

• Algorithm correctness: The classic linear backtracking wildcard-match is the right tool here. usize::MAX sentinel is safe since star_p + 1 is gated on star_p != usize::MAX, and star_v is always bounded by the value length.
• /-crossing *: The key fix from the previous review — wildcard_match explicitly documents and tests that * crosses /, matching the motivation (copilot:repo=org/project/*@main``). Previous glob_match::glob_match treated `/` as a separator, silently rejecting the motivating pattern.
• Test coverage: All the important cases are covered — exact, trailing *, middle *, /-crossing, multiple *, empty string, case-sensitivity contrast between tag_matches_pattern and name_matches_pattern.
• Consolidation of duplicate logic: Three call sites had subtly inconsistent hand-rolled implementations; collapsing them to shared helpers is the right move.
• No new dependencies: Dropping glob_match in favour of a purpose-built 40-line function is strictly better here.

Generated by Rust PR Reviewer for issue #442 · ● 228.9K · ◷