Skip to content

fix(execute): match safe-output repository by name or alias#469

Merged
jamesadevine merged 3 commits intomainfrom
copilot/fix-repository-name-matching
May 8, 2026
Merged

fix(execute): match safe-output repository by name or alias#469
jamesadevine merged 3 commits intomainfrom
copilot/fix-repository-name-matching

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 8, 2026

Summary

Safe-output executors looked up the agent-supplied repository field only by pipeline-resource alias key, so an otherwise valid ADO repository name (e.g. sdk-FtdiDeviceControl for the resource repo-sdk-ftdidevicecontrol → 4x4/sdk-FtdiDeviceControl) was rejected with Repository '…' not in allowed list.

Changes:

  • New helper lookup_allowed_repository in src/safeoutputs/mod.rs that resolves an input against ctx.allowed_repositories by (1) exact alias key, (2) case-insensitive value match (full project/repo), or (3) case-insensitive trailing repo-name match (ADO repo names are case-insensitive).
  • New helper input_refers_to_self (pub(crate)) in src/safeoutputs/mod.rs that returns true for "self", the empty string (with a debug log), or a case-insensitive match against ctx.repository_name (full value or trailing part).
  • Wired through resolve_repo_name and the inline lookups in create_pr, add_pr_comment, reply_to_pr_comment, create_branch, and create_git_tag. create_pr calls the shared input_refers_to_self helper rather than inlining equivalent logic.
  • docs/safe-outputs.md updated to document the accepted forms for the repository field.

Per-tool operator allowlists (config.allowed_repositories: Vec<String> in front matter) are unchanged — those continue to be matched against the agent-supplied value as configured.

Example — all four of these now resolve to the same repository:

{"repository": "self"}                          // pipeline's own repo
{"repository": "repo-sdk-ftdidevicecontrol"}    // alias key
{"repository": "4x4/sdk-FtdiDeviceControl"}     // full ADO name
{"repository": "sdk-FtdiDeviceControl"}         // bare repo name (case-insensitive)

Test plan

  • Added unit tests for lookup_allowed_repository covering alias / full-value (exact and case-insensitive) / trailing-name (exact and case-insensitive) / no-match / no-slash-value cases.
  • Added unit tests for resolve_repo_name covering the self literal, None, trailing-name and full-value matches against ctx.repository_name, alias resolution, and the unknown-repository failure path.
  • cargo build clean; cargo test --bin ado-aw safeoutputs:: → 447 passed.

Copilot AI changed the title [WIP] Fix safe output executor to match repository by name fix(execute): match safe-output repository by name or alias May 8, 2026
Copilot AI requested a review from jamesadevine May 8, 2026 15:18
@jamesadevine
Copy link
Copy Markdown
Collaborator

/rust-review

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 8, 2026

Rust PR Reviewer completed successfully!

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 8, 2026

🔍 Rust PR Review

Summary: Good fix with solid test coverage — one meaningful logic inconsistency and a minor robustness gap to address.


Findings

🐛 Bugs / Logic Issues

  • src/safeoutputs/create_pr.rs:593–603 — Duplicated self-detection logic, not using input_refers_to_self

    The PR introduces input_refers_to_self(input, ctx) in mod.rs and uses it in resolve_repo_name. But create_pr.rs was updated to inline an equivalent check directly:

    let repo_id = if self.repository == "self"
        || ctx.repository_name.as_deref().is_some_and(|n| {
            n.eq_ignore_ascii_case(&self.repository)
                || n.rsplit('/').next().unwrap_or(n).eq_ignore_ascii_case(&self.repository)
        })
    {

    This is semantically equivalent to input_refers_to_self(&self.repository, ctx) but bypasses the shared helper. If the logic changes in input_refers_to_self (e.g., empty-string handling), create_pr.rs will silently diverge. The fix is straightforward:

    let repo_id = if crate::safeoutputs::input_refers_to_self(&self.repository, ctx) {

    This requires making input_refers_to_self pub(crate) instead of fn.

⚠️ Suggestions

  • src/safeoutputs/mod.rs:204–209 — Step 2 is case-sensitive on full paths; Step 3 is case-insensitive on trailing names — inconsistent

    If a caller passes the full configured value ("4x4/sdk-FtdiDeviceControl") it matches Step 2 exactly. If they pass the same path in different case ("4x4/sdk-ftdidevicecontrol"), Step 2 fails (case-sensitive) and Step 3 only checks the trailing component after /, so it won't match either. The doc comment says "exact value match" for Step 2 which is clear, but a caller who knows the full path but has the wrong case gets a confusing failure where the trailing-name form would have succeeded. Given ADO repo names are case-insensitive, making Step 2 case-insensitive as well would be consistent:

    // 2. Case-insensitive full value match
    if let Some((_, name)) = allowed_repositories.iter().find(|(_, v)| v.eq_ignore_ascii_case(input)) {
        return Some(name);
    }
  • src/safeoutputs/mod.rs:226 — Empty string silently maps to "self"

    input_refers_to_self returns true for "". This is reachable via resolve_repo_name(Some(""), ctx). An empty-string alias passed by an agent would silently target the pipeline's own repository with no error. If an empty string is never a valid user-supplied value this is fine, but it's worth a debug-level log or an early check upstream. It's not a security issue given the controlled tooling environment, but it can make debugging agent misbehavior harder.

  • Missing resolve_repo_name tests — The new tests only cover lookup_allowed_repository directly. The resolve_repo_name function now delegates to input_refers_to_self with the new broader matching logic, but has no new test cases verifying the combined behavior (e.g., trailing-name match on ctx.repository_name falling through to the correct repository_name value).

✅ What Looks Good

  • Centralizing the lookup in lookup_allowed_repository is the right call — the old scattered HashMap::get() calls were brittle.
  • The three-tier lookup order (alias → full value → trailing name) is clearly documented and follows a sensible precedence.
  • Test coverage for lookup_allowed_repository is thorough: alias, full value, case-sensitive/insensitive trailing name, no-slash values, and the no-match case.
  • All existing tests still pass (1342/1342).

Generated by Rust PR Reviewer for issue #469 · ● 437.1K ·

@jamesadevine
Copy link
Copy Markdown
Collaborator

@copilot implement bug fixes and suggestions

…full-value match; tests for resolve_repo_name

Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/8f2f7644-0e82-411f-873e-6dccd8ce881f

Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 8, 2026

@copilot implement bug fixes and suggestions

Addressed rust-review feedback in f0d9cf0:

  • create_pr.rs now calls the shared input_refers_to_self helper (made pub(crate)) instead of inlining the equivalent check.
  • lookup_allowed_repository step 2 (full value) is now case-insensitive, consistent with step 3.
  • input_refers_to_self logs a debug message when an empty alias is treated as self.
  • Added resolve_repo_name tests (self literal, trailing/full-value match against ctx.repository_name, alias, unknown) plus a case-insensitive full-value lookup_allowed_repository test. All 447 safeoutputs:: tests pass.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • spsprodweu4.vssps.visualstudio.com
    • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-632c4d56de6fcebc /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-632c4d56de6fcebc safeoutputs:: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/clap_derive-28fe0dd3cb6f860c.cdebuginfo=2 /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/clap_derive-28fe0dd3cb6f860c.c--cfg dz/list /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/clap_derive-28fe0dd3cb6f860c.c--cfg /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/clap_derive-28fe0dd3cb6f860c.cfeature=&#34;std&#34; /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/clap_derive-28fe0dd3cb6f860c.c--check-cfg sync_trait.a0ce32b56b003c25-cgu.00.rcgu.o sync_trait.a0ce32b56b003c25-cgu.01.rcgu.o sync_trait.a0ce32b56b003c25-cgu.02.rcgu.o sync_trait.a0ce32b56b003c25-cgu.03.rcgu.o sync_trait.a0ce32b56b003c25-cgu.04.rcgu.o sync_trait.a0ce32b56b003c25-cgu.05.rcgu.o sync_trait.a0ce32b56b003c25-cgu.06.rcgu.o sync_trait.a0ce32b56b003c25-cgu.07.rcgu.o sync_trait.a0ce32b56b003c25-cgu.08.rcgu.o sync_trait.a0ce32b56b003c25-cgu.09.rcgu.o pl-d776f39dc20c6-Wl,--no-undefined-version f4irwy95cb0znc27wmjjyqz3.rcgu.o (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@jamesadevine jamesadevine marked this pull request as ready for review May 8, 2026 15:46
@jamesadevine jamesadevine merged commit 76cd618 into main May 8, 2026
20 checks passed
@jamesadevine jamesadevine deleted the copilot/fix-repository-name-matching branch May 8, 2026 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Safe output executor should match repository by name, not just resource alias

2 participants