Skip to content

Allow workspace: to target a checked-out repository alias#332

Merged
jamesadevine merged 3 commits intomainfrom
copilot/augment-workspace-repo-name
Apr 28, 2026
Merged

Allow workspace: to target a checked-out repository alias#332
jamesadevine merged 3 commits intomainfrom
copilot/augment-workspace-repo-name

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 28, 2026

The workspace: front matter field only accepted root or repo, leaving no way to point the agent's working directory at a specific additional checkout when multiple repositories are pulled in.

Changes

  • New accepted values for workspace: — in addition to root and repo, the field now accepts self (synonym for repo) and any alias from the checkout: list, which resolves to $(Build.SourcesDirectory)/<alias>.
  • Validation — a non-reserved value must appear in checkout: and there must be at least one additional checkout; otherwise compilation fails with a message naming the bad value and listing valid choices. compute_effective_workspace now returns Result<String>.
  • generate_working_directory — explicit arms for root / repo plus an alias:<name> sentinel; unexpected values trip a debug_assert!.
  • DocsAGENTS.md updated for the workspace: field and the {{ working_directory }} template marker, including the example below.

Example

repositories:
  - repository: exp23-a7-nw
    type: git
    name: msazuresphere/exp23-a7-nw
checkout:
  - exp23-a7-nw
workspace: exp23-a7-nw    # → $(Build.SourcesDirectory)/exp23-a7-nw

Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/ca667e13-6614-485c-a083-01e258ba39aa

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

/rust-review

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 28, 2026

Rust PR Reviewer was cancelled. Please review the logs for details.

@jamesadevine
Copy link
Copy Markdown
Collaborator

/rust-review

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 28, 2026

Rust PR Reviewer completed successfully!

@github-actions
Copy link
Copy Markdown
Contributor

🔍 Rust PR Review

Summary: Mostly looks good — clean implementation with solid tests. Two issues worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

  • common.rs:355-367 — Reserved names shadow same-named repo aliases silently
    The match ws arms for "root", "repo", and "self" are checked before the alias fallthrough arm. If a user has a repository actually named "repo" (or "root") in their checkout: list and writes workspace: repo, they get the special-cased behavior (a warning + $(Build.Repository.Name) path) rather than the alias resolution they intended. It fails silently with the wrong working directory.

    The error message in the alias arm compounds this: "Valid values: 'root', 'repo' (or 'self'), or one of {:?}" where {:?} expands to the checkout list. If checkout contains ["root"], the message implies workspace: root will resolve to the repo subfolder, but it won't.

    Suggested fix: either add a compile-time error/warning when a reserved name is also present as a checkout: entry, or document explicitly in the validation error that root/repo/self are always reserved regardless of checkout list contents.

  • common.rs:369-376!has_additional_checkouts check is redundant and misleading
    The first guard (if !has_additional_checkouts { bail! }) fires when checkout: is empty. The second guard (if !checkout.iter().any(|c| c == alias) { bail! }) would also catch this case (an empty slice never contains the alias). The first check produces a slightly better error message, but consider collapsing or documenting why both are needed to avoid confusion.

⚠️ Suggestions

  • common.rs:368-387 — No path-safety validation on alias before embedding
    generate_working_directory embeds the alias directly: format!("$(Build.SourcesDirectory)/{}", alias). An alias containing .. (e.g. a repo named ../sibling) would produce $(Build.SourcesDirectory)/../sibling. This is constrained by validate_checkout_list requiring the alias to match a repository: name, and the author controls their own YAML, so severity is low — but a contains("..") or starts_with('/') guard here would be cheap insurance and is consistent with path-safety checks elsewhere in the codebase.

  • Tests — missing coverage for workspace: self with no checkouts
    test_workspace_explicit_repo_no_checkouts_still_returns_repo exists for the repo variant, but there's no corresponding test that workspace: self with an empty checkout: list emits the warning and still returns "repo". The code path is the same (both hit the "repo" | "self" arm) but the symmetry gap is easy to miss.

✅ What Looks Good

  • The WORKSPACE_ALIAS_PREFIX sentinel pattern is a clean way to shuttle the validated alias through the String return type without changing the function signature to an enum — and it's well-documented.
  • Error messages include the offending alias value and the valid choices, which is exactly right for a CLI compiler.
  • debug_assert! in generate_working_directory for unreachable states is the right call; not a panic in release builds.
  • The \{\{ workspace }} deferred-substitution chain (source_path / pipeline_path use \{\{ workspace }} which is later replaced by working_directory) works correctly because replacements fold sequentially — the alias path propagates to both paths without any extra wiring.
  • Tests cover both compute-only and compute+generate round-trips. Good separation.

Generated by Rust PR Reviewer for issue #332 · ● 834K ·

@jamesadevine
Copy link
Copy Markdown
Collaborator

@copilot Address feedback and suggestions

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 28, 2026

@copilot Address feedback and suggestions

Addressed all four findings in a49bc9e:

  • Reserved-name shadowing: validate_checkout_list now rejects root/repo/self as checkout aliases, so they can never silently shadow the reserved workspace: arms.
  • Redundant guard collapsed: the alias arm now does a single checkout.iter().any(...) check and tailors the error message based on whether the list is empty.
  • Path-safety: aliases containing .., /, \, or starting with . are rejected before being embedded into the working-directory path (defense in depth on top of validate_checkout_list).
  • Test gap closed: added test_workspace_explicit_self_no_checkouts_still_returns_repo, traversal/slash safety tests, and test_validate_checkout_list_reserved_name_fails.

cargo test 901 passing, clippy clean.

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:

  • dev.azure.com
    • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-9a1c412f8eace73c /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-9a1c412f8eace73c --quiet /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw_derive-200f38d8081bcd5a/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-3b13783209cfb098.1kaqz472u2x7u4wrzxydofou9.1d7ihde.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw_derive-200f38d8081bcd5a/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-3b13783209cfb098.1l6p24bqam7vjd2lhd8qwsx9u.1d7ihde.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw_derive-200f38d8081bcd5a/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-3b13783209cfb098.1mc3s5m7i3go2c4ozf1hqb4q6.1d7ihde.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw_derive-200f38d8081bcd5a/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-3b13783209cfb098.1ofye6g3w5oqac01kzpct9a0k.1d7ihde.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw_derive-200f38d8081bcd5a/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-3b13783209cfb098.1rc9shviizw544vyorj2heggq.1d7ihde.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw_derive-200f38d8081bcd5a/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-3b13783209cfb098.1vxb5rw11m0thmbn6dz5erupb.1d7ihde.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw_derive-200f38d8081bcd5a/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-3b13783209cfb098.1zf9lqe2tmao0mjy55q3i2db7.1d7ihde.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw_derive-200f38d8081bcd5a.24x�� /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw_derive-200f38d8081bcd5a.2eic40pkfx5wikhd73nzwyiy4.1ol190f.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw_derive-200f38d8081bcd5a.2s1x3nf0f4deblv1kjt23ls7h.1ol190f.rcgu.o lap_�� lap_derive.40302fb925fcb60e-cgu.13.rcgu.o lap_derive.40302fb925fcb60e-cgu.14.rcgu.o 7db13e5c0/build_-Wl,-Bstatic lib/rustlib/x86_cc lib/rustlib/x86_-Wl,--version-script=/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/rustcrnj3/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-3b13783209cfb098.0ellgqglwg582barz68fbig19.1d7ihde.rcgu.o lib/rustlib/x86_-Wl,--no-undefined-version lib/rustlib/x86_-m64 (dns block)
    • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-9a1c412f8eace73c /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-9a1c412f8eace73c --quiet s/clap_derive-28get-url known-linux-gnu/origin s/cl�� in.so /lto-wrapper s/clap_derive-28fe0dd3cb6f860c.c/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw_derive-get-url s/clap_derive-28git s/clap_derive-28remote s/clap_derive-28get-url s/clap_derive-28origin s/cl�� s/clap_derive-28fe0dd3cb6f860c.clap_derive.40302fb925fcb60e-cgu.13.rcgu.o s/clap_derive-28fe0dd3cb6f860c.clap_derive.40302fb925fcb60e-cgu.14.rcgu.o target/debug/build/paste-2d34beb7db13e5c0/build_-Wl,-Bstatic stable-x86_64-ungit stable-x86_64-unremote stable-x86_64-unget-url stable-x86_64-unorigin (dns block)

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

Comment thread src/compile/common.rs
}
Ok("repo".to_string())
}
alias => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot do we not have a validators library that will detect path escaping?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — src/validate.rs is the shared home for structural validators but it didn't have a path-segment helper (only is_valid_command_path for full paths, plus an inline .. check buried in cache_memory/execute.rs). Added validate::is_safe_path_segment in 6e9979c with its own unit tests, and switched the workspace alias check to use it.

Copilot AI requested a review from jamesadevine April 28, 2026 12:33
@jamesadevine jamesadevine merged commit 6877def into main Apr 28, 2026
@jamesadevine jamesadevine deleted the copilot/augment-workspace-repo-name branch April 28, 2026 13:07
jamesadevine added a commit that referenced this pull request Apr 28, 2026
This is an empty commit to trigger release-please to open a release PR
so that the changelog can be manually updated to include changes from
commits #330, #331, and #332 which were merged without Conventional
Commit prefixes and were silently dropped from the changelog.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.

2 participants