Skip to content

Commit 051d45c

Browse files
fix(compile): detect and prevent workspace checkout collision with self repo (#456)
* Initial plan * fix(compile): detect workspace/self checkout collision Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/ca3cf1a1-f294-4f08-9982-5ceaaee7609b Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> * refactor: address review feedback on rsplit fallback Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/ca3cf1a1-f294-4f08-9982-5ceaaee7609b Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
1 parent aac3f8c commit 051d45c

1 file changed

Lines changed: 148 additions & 0 deletions

File tree

src/compile/common.rs

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,67 @@ pub fn generate_checkout_self() -> String {
340340
/// wrong working directory. We reject this at compile time instead.
341341
const RESERVED_WORKSPACE_NAMES: &[&str] = &["root", "repo", "self"];
342342

343+
/// Validate that no entry in `checkout` resolves to the same on-disk
344+
/// directory as the `self` checkout.
345+
///
346+
/// In ADO multi-repo checkout, both `checkout: self` and an additional
347+
/// `checkout: <alias>` land in `s/<RepositoryName>`, where
348+
/// `<RepositoryName>` is `Build.Repository.Name` for `self` and the
349+
/// trailing path segment of the `name:` field for each `repositories:`
350+
/// entry. When these collide, the second checkout runs `git clean -ffdx`
351+
/// and resets to its configured ref, silently wiping files that exist on
352+
/// the trigger branch but not on the workspace ref. Failing fast at
353+
/// compile time is much more discoverable than the resulting runtime
354+
/// "file not found" errors downstream.
355+
///
356+
/// `self_repo_name` is the trigger repo's `Build.Repository.Name` —
357+
/// usually the trailing segment of the trigger repo's full name, inferred
358+
/// from the local git remote. When `None` (e.g. compiling outside an ADO
359+
/// clone, or in unit tests) the check is skipped because we have no
360+
/// reliable identity for `self`.
361+
pub fn validate_checkout_self_collision(
362+
repositories: &[Repository],
363+
checkout: &[String],
364+
self_repo_name: Option<&str>,
365+
) -> Result<()> {
366+
let Some(self_name) = self_repo_name else {
367+
return Ok(());
368+
};
369+
if checkout.is_empty() {
370+
return Ok(());
371+
}
372+
373+
for alias in checkout {
374+
let Some(repo) = repositories.iter().find(|r| r.repository == *alias) else {
375+
// Unknown aliases are reported by `validate_checkout_list`.
376+
continue;
377+
};
378+
// `rsplit('/').next()` on any &str always yields `Some` — even for
379+
// names without a slash the whole string is returned.
380+
let last_segment = repo.name.rsplit('/').next().expect("rsplit always yields one item");
381+
// ADO is case-insensitive on Windows agents and case-sensitive on
382+
// Linux. Use a case-insensitive comparison so the collision is
383+
// caught regardless of agent OS — the resulting pipeline would
384+
// break on at least one platform either way.
385+
if last_segment.eq_ignore_ascii_case(self_name) {
386+
anyhow::bail!(
387+
"Checkout entry '{}' (repository name '{}') resolves to the same \
388+
directory ('s/{}') as the trigger repository checked out as 'self'. \
389+
The second checkout would overwrite the first, replacing files \
390+
from the trigger branch with the workspace ref. Remove '{}' from \
391+
'checkout:' — the 'self' checkout already provides access to this \
392+
repository.",
393+
alias,
394+
repo.name,
395+
self_name,
396+
alias,
397+
);
398+
}
399+
}
400+
401+
Ok(())
402+
}
403+
343404
/// Validate that all entries in checkout list exist in repositories
344405
pub fn validate_checkout_list(repositories: &[Repository], checkout: &[String]) -> Result<()> {
345406
if checkout.is_empty() {
@@ -2029,6 +2090,15 @@ pub async fn compile_shared(
20292090
// 1. Validate
20302091
validate_front_matter_identity(front_matter)?;
20312092

2093+
// Detect workspace/self checkout collisions now that we have ado_context
2094+
// (which provides the trigger repo's name). Skipped when no remote is
2095+
// available — see `validate_checkout_self_collision` for details.
2096+
validate_checkout_self_collision(
2097+
&front_matter.repositories,
2098+
&front_matter.checkout,
2099+
ctx.ado_context.as_ref().map(|c| c.repo_name.as_str()),
2100+
)?;
2101+
20322102
// 2. Generate schedule
20332103
let schedule = match front_matter.schedule() {
20342104
Some(s) => generate_schedule(&front_matter.name, s)
@@ -2487,6 +2557,84 @@ mod tests {
24872557
assert!(msg.contains("'repo'"), "msg: {msg}");
24882558
}
24892559

2560+
// ─── validate_checkout_self_collision ────────────────────────────────────
2561+
2562+
#[test]
2563+
fn test_validate_self_collision_detects_match() {
2564+
// Workspace repo's name last segment matches the self repo's name,
2565+
// so both `checkout: self` and `checkout: my-repo` would land in
2566+
// `s/my-repo`. Must error.
2567+
let repos = vec![Repository {
2568+
repository: "my-repo".to_string(),
2569+
repo_type: "git".to_string(),
2570+
name: "some-org/my-repo".to_string(),
2571+
repo_ref: "refs/heads/main".to_string(),
2572+
}];
2573+
let checkout = vec!["my-repo".to_string()];
2574+
let err =
2575+
validate_checkout_self_collision(&repos, &checkout, Some("my-repo")).unwrap_err();
2576+
let msg = err.to_string();
2577+
assert!(msg.contains("'my-repo'"), "msg: {msg}");
2578+
assert!(msg.contains("same"), "msg: {msg}");
2579+
assert!(msg.contains("'self'"), "msg: {msg}");
2580+
}
2581+
2582+
#[test]
2583+
fn test_validate_self_collision_no_collision_passes() {
2584+
// Different repo name → different `s/<name>` directory, no collision.
2585+
let repos = vec![Repository {
2586+
repository: "other".to_string(),
2587+
repo_type: "git".to_string(),
2588+
name: "some-org/other".to_string(),
2589+
repo_ref: "refs/heads/main".to_string(),
2590+
}];
2591+
let checkout = vec!["other".to_string()];
2592+
let result = validate_checkout_self_collision(&repos, &checkout, Some("my-repo"));
2593+
assert!(result.is_ok());
2594+
}
2595+
2596+
#[test]
2597+
fn test_validate_self_collision_case_insensitive() {
2598+
// ADO is case-insensitive on Windows; treat differing-only-by-case
2599+
// names as a collision so the pipeline doesn't break on one OS.
2600+
let repos = vec![Repository {
2601+
repository: "my-repo".to_string(),
2602+
repo_type: "git".to_string(),
2603+
name: "Some-Org/My-Repo".to_string(),
2604+
repo_ref: "refs/heads/main".to_string(),
2605+
}];
2606+
let checkout = vec!["my-repo".to_string()];
2607+
let err =
2608+
validate_checkout_self_collision(&repos, &checkout, Some("my-repo")).unwrap_err();
2609+
assert!(err.to_string().contains("same"));
2610+
}
2611+
2612+
#[test]
2613+
fn test_validate_self_collision_no_self_name_skipped() {
2614+
// No git remote / no inferred self name → can't detect, skip.
2615+
let repos = vec![Repository {
2616+
repository: "my-repo".to_string(),
2617+
repo_type: "git".to_string(),
2618+
name: "org/my-repo".to_string(),
2619+
repo_ref: "refs/heads/main".to_string(),
2620+
}];
2621+
let checkout = vec!["my-repo".to_string()];
2622+
let result = validate_checkout_self_collision(&repos, &checkout, None);
2623+
assert!(result.is_ok());
2624+
}
2625+
2626+
#[test]
2627+
fn test_validate_self_collision_empty_checkout_passes() {
2628+
let repos = vec![Repository {
2629+
repository: "my-repo".to_string(),
2630+
repo_type: "git".to_string(),
2631+
name: "org/my-repo".to_string(),
2632+
repo_ref: "refs/heads/main".to_string(),
2633+
}];
2634+
let result = validate_checkout_self_collision(&repos, &[], Some("my-repo"));
2635+
assert!(result.is_ok());
2636+
}
2637+
24902638
// ─── Engine::args (copilot params) ──────────────────────────────────────
24912639

24922640
#[test]

0 commit comments

Comments
 (0)