[F] Webhook issues

tests/e2e/sequential.rs

@@ -613,11 +613,18 @@ namespace = "{}"
     fn webhook_commands_and_receiver_work(&self) -> Result<()> {
         let repo = self.repo_name("webhook");
         let source = self.primary_provider();
+        let webhook_url = format!("https://example.com/refray-e2e/{}/{repo}", self.run_id);
+        let webhook_url_with_slash = format!("{webhook_url}/");
         self.seed_all_main(&repo, "webhook base", 1_700_001_601)?;
         self.sync_repo(&repo, [])?;
+        self.set_webhook_url(&webhook_url)?;
 
         self.refray(["webhook", "install", "--dry-run"])?;
         self.refray(["webhook", "uninstall", "--dry-run"])?;
+        self.refray(["webhook", "install"])?;
+        self.assert_webhook_exists_all(&repo, &webhook_url)?;
+        self.refray(["webhook", "uninstall", &webhook_url_with_slash])?;
+        self.assert_webhook_absent_all(&repo, &webhook_url)?;
         self.refray([
             "webhook",
             "update",
@@ -783,6 +790,41 @@ namespace = "{}"
             .with_context(|| format!("failed to write {}", self.config_path.display()))
     }
 
+    fn set_webhook_url(&self, url: &str) -> Result<()> {
+        let contents = fs::read_to_string(&self.config_path)
+            .with_context(|| format!("failed to read {}", self.config_path.display()))?;
+        let escaped_url = url.replace('"', "\\\"");
+        let mut in_webhook = false;
+        let mut replaced = false;
+        let mut updated = contents
+            .lines()
+            .map(|line| {
+                if line.trim() == "[webhook]" {
+                    in_webhook = true;
+                    return line.to_string();
+                }
+                if line.starts_with('[') {
+                    in_webhook = false;
+                }
+                if in_webhook && line.starts_with("url = ") {
+                    replaced = true;
+                    format!("url = \"{escaped_url}\"")
+                } else {
+                    line.to_string()
+                }
+            })
+            .collect::<Vec<_>>()
+            .join("\n");
+        if contents.ends_with('\n') {
+            updated.push('\n');
+        }
+        if !replaced {
+            bail!("config is missing [webhook].url");
+        }
+        fs::write(&self.config_path, updated)
+            .with_context(|| format!("failed to write {}", self.config_path.display()))
+    }
+
     fn sync<const N: usize>(&self, args: [&str; N]) -> Result<()> {
         let mut command = vec!["sync"];
         command.extend(args);
@@ -1014,6 +1056,42 @@ namespace = "{}"
         })
     }
 
+    fn assert_webhook_exists_all(&self, repo: &str, url: &str) -> Result<()> {
+        retry("webhook installed", || {
+            for provider in &self.settings.providers {
+                let urls = provider.list_webhook_urls(repo)?;
+                if !urls
+                    .iter()
+                    .any(|candidate| webhook_urls_match(candidate, url))
+                {
+                    bail!(
+                        "webhook {url} missing on {} for {repo}; found {urls:?}",
+                        provider.site_name
+                    );
+                }
+            }
+            Ok(())
+        })
+    }
+
+    fn assert_webhook_absent_all(&self, repo: &str, url: &str) -> Result<()> {
+        retry("webhook uninstalled", || {
+            for provider in &self.settings.providers {
+                let urls = provider.list_webhook_urls(repo)?;
+                if urls
+                    .iter()
+                    .any(|candidate| webhook_urls_match(candidate, url))
+                {
+                    bail!(
+                        "webhook {url} still exists on {} for {repo}",
+                        provider.site_name
+                    );
+                }
+            }
+            Ok(())
+        })
+    }
+
     fn refs_by_provider(&self, repo: &str) -> Result<BTreeMap<String, GitRefs>> {
         let mut output = BTreeMap::new();
         for provider in &self.settings.providers {
@@ -1081,17 +1159,17 @@ impl ProviderAccount {
     ) -> Self {
         let mut base_url = trim_url(&base_url).to_string();
         let mut username = username;
-        if let Ok(url) = Url::parse(&username) {
-            if let Some(host) = url.host_str() {
-                let path = url.path().trim_matches('/');
-                if !path.is_empty() {
-                    let mut profile_base_url = format!("{}://{}", url.scheme(), host);
-                    if let Some(port) = url.port() {
-                        profile_base_url.push_str(&format!(":{port}"));
-                    }
-                    base_url = profile_base_url;
-                    username = path.to_string();
+        if let Ok(url) = Url::parse(&username)
+            && let Some(host) = url.host_str()
+        {
+            let path = url.path().trim_matches('/');
+            if !path.is_empty() {
+                let mut profile_base_url = format!("{}://{}", url.scheme(), host);
+                if let Some(port) = url.port() {
+                    profile_base_url.push_str(&format!(":{port}"));
                 }
+                base_url = profile_base_url;
+                username = path.to_string();
             }
         }
         Self {
@@ -1454,6 +1532,33 @@ impl ProviderAccount {
             .collect())
     }
 
+    fn list_webhook_urls(&self, repo: &str) -> Result<Vec<String>> {
+        let url = match self.kind {
+            ProviderKind::Github => format!(
+                "{}/repos/{}/{}/hooks?per_page=100",
+                self.api_base(),
+                self.username,
+                repo
+            ),
+            ProviderKind::Gitlab => format!(
+                "{}/projects/{}/hooks?per_page=100",
+                self.api_base(),
+                urlencoding(&format!("{}/{}", self.username, repo))
+            ),
+            ProviderKind::Gitea | ProviderKind::Forgejo => format!(
+                "{}/repos/{}/{}/hooks?limit=50",
+                self.api_base(),
+                self.username,
+                repo
+            ),
+        };
+        Ok(self
+            .paged_get_values(&url)?
+            .into_iter()
+            .filter_map(|value| webhook_url_from_json(&value))
+            .collect())
+    }
+
     fn paged_get(&self, first_url: &str) -> Result<Vec<ProviderRepo>> {
         Ok(self
             .paged_get_values(first_url)?
@@ -1808,6 +1913,42 @@ fn trim_url(value: &str) -> &str {
     value.trim_end_matches('/')
 }
 
+fn webhook_url_from_json(value: &Value) -> Option<String> {
+    value
+        .pointer("/config/url")
+        .or_else(|| value.get("url"))
+        .and_then(Value::as_str)
+        .map(ToOwned::to_owned)
+}
+
+fn webhook_urls_match(left: &str, right: &str) -> bool {
+    if left == right {
+        return true;
+    }
+    match (normalize_webhook_url(left), normalize_webhook_url(right)) {
+        (Some(left), Some(right)) => left == right,
+        _ => false,
+    }
+}
+
+fn normalize_webhook_url(value: &str) -> Option<String> {
+    let url = Url::parse(value).ok()?;
+    let scheme = url.scheme().to_ascii_lowercase();
+    if scheme != "http" && scheme != "https" {
+        return None;
+    }
+    let host = url.host_str()?.to_ascii_lowercase();
+    let port = url.port_or_known_default()?;
+    let username = url.username();
+    let password = url.password().unwrap_or_default();
+    let path = url.path().trim_end_matches('/');
+    let path = if path.is_empty() { "/" } else { path };
+    let query = url.query().unwrap_or_default();
+    Some(format!(
+        "{scheme}://{username}:{password}@{host}:{port}{path}?{query}"
+    ))
+}
+
 fn urlencoding(value: &str) -> String {
     url::form_urlencoded::byte_serialize(value.as_bytes()).collect()
 }

tests/unit/config.rs

@@ -1,7 +1,7 @@
 use super::*;
 
 #[test]
-fn parses_token_forms() {
+fn parses_value_tokens() {
     let config: Config = toml::from_str(
         r#"
         jobs = 8
@@ -9,7 +9,7 @@ fn parses_token_forms() {
         [webhook]
         install = true
         url = "https://mirror.example.test/webhook"
-        secret = { env = "WEBHOOK_SECRET" }
+        secret = { value = "webhook-secret" }
         full_sync_interval_minutes = 60
         reachability_check_interval_minutes = 15
 
@@ -17,7 +17,7 @@ fn parses_token_forms() {
         name = "github"
         provider = "github"
         base_url = "https://github.com"
-        token = { env = "GITHUB_TOKEN" }
+        token = { value = "github-token" }
 
         [[mirrors]]
         name = "personal"
@@ -62,11 +62,28 @@ fn parses_token_forms() {
     assert_eq!(webhook.url, "https://mirror.example.test/webhook");
     assert_eq!(
         webhook.secret,
-        TokenConfig::Env("WEBHOOK_SECRET".to_string())
+        TokenConfig::Value("webhook-secret".to_string())
     );
     assert_eq!(webhook.full_sync_interval_minutes, Some(60));
 }
 
+#[test]
+fn env_token_form_is_rejected() {
+    let err = toml::from_str::<Config>(
+        r#"
+        [[sites]]
+        name = "github"
+        provider = "github"
+        base_url = "https://github.com"
+        token = { env = "GITHUB_TOKEN" }
+        "#,
+    )
+    .unwrap_err()
+    .to_string();
+
+    assert!(err.contains("unknown variant") || err.contains("expected"));
+}
+
 #[test]
 fn config_defaults_jobs() {
     let config: Config = toml::from_str("").unwrap();
@@ -206,6 +223,34 @@ fn validation_rejects_invalid_repo_filter_regex() {
     assert!(err.contains("invalid repo_whitelist regex"));
 }
 
+#[test]
+fn validation_rejects_duplicate_mirror_endpoints() {
+    let duplicate = EndpointConfig {
+        site: "github".to_string(),
+        kind: NamespaceKind::User,
+        namespace: "alice".to_string(),
+    };
+    let config = Config {
+        jobs: crate::config::DEFAULT_JOBS,
+        sites: vec![site("github", ProviderKind::Github)],
+        mirrors: vec![MirrorConfig {
+            name: "broken".to_string(),
+            endpoints: vec![duplicate.clone(), duplicate],
+            sync_visibility: SyncVisibility::All,
+            repo_whitelist: Vec::new(),
+            repo_blacklist: Vec::new(),
+            create_missing: true,
+            visibility: Visibility::Private,
+            conflict_resolution: ConflictResolutionStrategy::Fail,
+        }],
+        webhook: None,
+    };
+
+    let err = validate_config(&config).unwrap_err().to_string();
+
+    assert!(err.contains("duplicate endpoint"));
+}
+
 #[test]
 fn validation_rejects_zero_jobs() {
     let mut config = Config {

tests/unit/provider.rs

@@ -62,6 +62,26 @@ fn group_paths_are_url_encoded_for_gitlab() {
     assert_eq!(urlencoding("parent/child group"), "parent%2Fchild+group");
 }
 
+#[test]
+fn webhook_url_matching_tolerates_trailing_slash_and_default_port() {
+    assert!(webhook_urls_match(
+        "https://example.test",
+        "https://example.test/"
+    ));
+    assert!(webhook_urls_match(
+        "https://example.test:443/webhook/",
+        "https://EXAMPLE.test/webhook"
+    ));
+    assert!(!webhook_urls_match(
+        "https://example.test/webhook",
+        "https://example.test/"
+    ));
+    assert!(!webhook_urls_match(
+        "https://example.test/webhook?token=one",
+        "https://example.test/webhook?token=two"
+    ));
+}
+
 #[test]
 fn validate_token_checks_user_endpoint_with_provider_auth_header() {
     let (api_url, handle) = one_request_server("200 OK", "{}", |request| {
@@ -217,7 +237,7 @@ fn uninstall_webhook_deletes_matching_github_hook() {
         vec![
             (
                 "200 OK",
-                r#"[{"id":42,"config":{"url":"https://mirror.example.test/webhook"}}]"#,
+                r#"[{"id":42,"url":"https://api.github.com/repos/alice/repo/hooks/42","config":{"url":"https://mirror.example.test/webhook"}}]"#,
             ),
             ("204 No Content", ""),
         ],
@@ -255,11 +275,55 @@ fn uninstall_webhook_deletes_matching_github_hook() {
     handle.join().unwrap();
 }
 
+#[test]
+fn uninstall_webhook_matches_github_hook_url_without_trailing_slash() {
+    let (api_url, handle) = request_server(
+        vec![
+            (
+                "200 OK",
+                r#"[{"id":42,"url":"https://api.github.com/repos/alice/repo/hooks/42","config":{"url":"https://mirror.example.test"}}]"#,
+            ),
+            ("204 No Content", ""),
+        ],
+        |index, request| match index {
+            0 => assert!(
+                request.starts_with("GET /repos/alice/repo/hooks "),
+                "request was {request}"
+            ),
+            1 => assert!(
+                request.starts_with("DELETE /repos/alice/repo/hooks/42 "),
+                "request was {request}"
+            ),
+            _ => unreachable!(),
+        },
+    );
+    let site = SiteConfig {
+        api_url: Some(api_url),
+        ..site(ProviderKind::Github, None)
+    };
+    let client = ProviderClient::new(&site).unwrap();
+
+    let removed = client
+        .uninstall_webhook(
+            &EndpointConfig {
+                site: "github".to_string(),
+                kind: NamespaceKind::User,
+                namespace: "alice".to_string(),
+            },
+            "repo",
+            "https://mirror.example.test/",
+        )
+        .unwrap();
+
+    assert!(removed);
+    handle.join().unwrap();
+}
+
 #[test]
 fn uninstall_webhook_reports_missing_github_hook() {
     let (api_url, handle) = one_request_server(
         "200 OK",
-        r#"[{"id":42,"config":{"url":"https://old.example.test/webhook"}}]"#,
+        r#"[{"id":42,"url":"https://api.github.com/repos/alice/repo/hooks/42","config":{"url":"https://old.example.test/webhook"}}]"#,
         |request| {
             assert!(
                 request.starts_with("GET /repos/alice/repo/hooks "),

tests/unit/sync.rs

@@ -59,11 +59,11 @@ fn ref_state_persists_and_requires_exact_remote_ref_match() {
     let temp = tempfile::TempDir::new().unwrap();
     let mut refs = BTreeMap::new();
     refs.insert(
-        "github_alice".to_string(),
+        remote_key("github"),
         remote_ref_state("abc", &[("main", "111")]),
     );
     refs.insert(
-        "gitea_alice".to_string(),
+        remote_key("gitea"),
         remote_ref_state("def", &[("main", "111")]),
     );
     let mut state = RefState::default();
@@ -76,13 +76,13 @@ fn ref_state_persists_and_requires_exact_remote_ref_match() {
 
     let mut changed_hash = refs.clone();
     changed_hash.insert(
-        "github_alice".to_string(),
+        remote_key("github"),
         remote_ref_state("changed", &[("main", "111")]),
     );
     assert!(!loaded.repo_matches("sync-1", "repo-a", &changed_hash));
 
     let mut missing_remote = refs;
-    missing_remote.remove("gitea_alice");
+    missing_remote.remove(&remote_key("gitea"));
     assert!(!loaded.repo_matches("sync-1", "repo-a", &missing_remote));
 }
 
@@ -179,16 +179,16 @@ fn repo_deletion_decision_propagates_previous_synced_repo_deletion() {
     let mirror = test_mirror();
     let mut previous = BTreeMap::new();
     previous.insert(
-        "github_alice".to_string(),
+        remote_key("github"),
         remote_ref_state("a", &[("main", "111")]),
     );
     previous.insert(
-        "gitea_alice".to_string(),
+        remote_key("gitea"),
         remote_ref_state("b", &[("main", "111")]),
     );
     let mut current = BTreeMap::new();
     current.insert(
-        "gitea_alice".to_string(),
+        remote_key("gitea"),
         remote_ref_state("b", &[("main", "111")]),
     );
 
@@ -202,8 +202,8 @@ fn repo_deletion_decision_propagates_previous_synced_repo_deletion() {
     assert_eq!(
         decision,
         RepoDeletionDecision::Propagate {
-            deleted_remotes: vec!["github_alice".to_string()],
-            target_remotes: vec!["gitea_alice".to_string()],
+            deleted_remotes: vec![remote_key("github")],
+            target_remotes: vec![remote_key("gitea")],
         }
     );
 }
@@ -213,16 +213,16 @@ fn repo_deletion_decision_conflicts_when_remaining_repo_changed() {
     let mirror = test_mirror();
     let mut previous = BTreeMap::new();
     previous.insert(
-        "github_alice".to_string(),
+        remote_key("github"),
         remote_ref_state("a", &[("main", "111")]),
     );
     previous.insert(
-        "gitea_alice".to_string(),
+        remote_key("gitea"),
         remote_ref_state("b", &[("main", "111")]),
     );
     let mut current = BTreeMap::new();
     current.insert(
-        "gitea_alice".to_string(),
+        remote_key("gitea"),
         remote_ref_state("changed", &[("main", "222")]),
     );
 
@@ -236,8 +236,8 @@ fn repo_deletion_decision_conflicts_when_remaining_repo_changed() {
     assert_eq!(
         decision,
         RepoDeletionDecision::Conflict {
-            deleted_remotes: vec!["github_alice".to_string()],
-            changed_remotes: vec!["gitea_alice".to_string()],
+            deleted_remotes: vec![remote_key("github")],
+            changed_remotes: vec![remote_key("gitea")],
         }
     );
 }
@@ -247,11 +247,11 @@ fn repo_deletion_decision_removes_state_when_deleted_everywhere() {
     let mirror = test_mirror();
     let mut previous = BTreeMap::new();
     previous.insert(
-        "github_alice".to_string(),
+        remote_key("github"),
         remote_ref_state("a", &[("main", "111")]),
     );
     previous.insert(
-        "gitea_alice".to_string(),
+        remote_key("gitea"),
         remote_ref_state("b", &[("main", "111")]),
     );
 
@@ -260,7 +260,7 @@ fn repo_deletion_decision_removes_state_when_deleted_everywhere() {
     assert_eq!(
         decision,
         RepoDeletionDecision::DeletedEverywhere {
-            deleted_remotes: vec!["github_alice".to_string(), "gitea_alice".to_string()],
+            deleted_remotes: vec![remote_key("github"), remote_key("gitea")],
         }
     );
 }
@@ -270,7 +270,7 @@ fn repo_deletion_decision_removes_partial_state_when_deleted_everywhere() {
     let mirror = test_mirror();
     let mut previous = BTreeMap::new();
     previous.insert(
-        "gitea_alice".to_string(),
+        remote_key("gitea"),
         remote_ref_state("b", &[("main", "111")]),
     );
 
@@ -279,7 +279,7 @@ fn repo_deletion_decision_removes_partial_state_when_deleted_everywhere() {
     assert_eq!(
         decision,
         RepoDeletionDecision::DeletedEverywhere {
-            deleted_remotes: vec!["github_alice".to_string(), "gitea_alice".to_string()],
+            deleted_remotes: vec![remote_key("github"), remote_key("gitea")],
         }
     );
 }
@@ -289,12 +289,12 @@ fn repo_deletion_decision_ignores_repos_not_previously_synced_everywhere() {
     let mirror = test_mirror();
     let mut previous = BTreeMap::new();
     previous.insert(
-        "gitea_alice".to_string(),
+        remote_key("gitea"),
         remote_ref_state("b", &[("main", "111")]),
     );
     let mut current = BTreeMap::new();
     current.insert(
-        "gitea_alice".to_string(),
+        remote_key("gitea"),
         remote_ref_state("b", &[("main", "111")]),
     );
 
@@ -316,7 +316,7 @@ fn filtered_sync_visibility_does_not_treat_state_only_repos_as_deleted() {
     ref_state.set_repo(
         &mirror.name,
         "private-repo",
-        BTreeMap::from([("github_alice".to_string(), remote_ref_state("a", &[]))]),
+        BTreeMap::from([(remote_key("github"), remote_ref_state("a", &[]))]),
     );
 
     let repo_filter = mirror.repo_filter().unwrap();
@@ -332,7 +332,7 @@ fn all_visibility_keeps_state_only_repos_for_deletion_detection() {
     ref_state.set_repo(
         &mirror.name,
         "deleted-repo",
-        BTreeMap::from([("github_alice".to_string(), remote_ref_state("a", &[]))]),
+        BTreeMap::from([(remote_key("github"), remote_ref_state("a", &[]))]),
     );
 
     let repo_filter = mirror.repo_filter().unwrap();
@@ -350,7 +350,7 @@ fn repo_name_filters_do_not_treat_state_only_repos_as_deleted() {
     ref_state.set_repo(
         &mirror.name,
         "private-repo",
-        BTreeMap::from([("github_alice".to_string(), remote_ref_state("a", &[]))]),
+        BTreeMap::from([(remote_key("github"), remote_ref_state("a", &[]))]),
     );
 
     let names = sync_candidate_repo_names(&HashMap::new(), &ref_state, &mirror, &repo_filter);
@@ -377,6 +377,25 @@ fn conflict_branch_prefixes_are_reversible_not_slug_collisions() {
     );
 }
 
+#[test]
+fn endpoint_remote_names_do_not_slug_collide() {
+    let slash = EndpointConfig {
+        site: "gitlab".to_string(),
+        kind: crate::config::NamespaceKind::Group,
+        namespace: "parent/child".to_string(),
+    };
+    let underscore = EndpointConfig {
+        site: "gitlab".to_string(),
+        kind: crate::config::NamespaceKind::Group,
+        namespace: "parent_child".to_string(),
+    };
+
+    assert_ne!(
+        remote_name_for_endpoint(&slash),
+        remote_name_for_endpoint(&underscore)
+    );
+}
+
 fn remote_ref_state(hash: &str, branches: &[(&str, &str)]) -> RemoteRefState {
     RemoteRefState {
         hash: hash.to_string(),
@@ -425,6 +444,10 @@ fn endpoint(site: &str) -> EndpointConfig {
     }
 }
 
+fn remote_key(site: &str) -> String {
+    remote_name_for_endpoint(&endpoint(site))
+}
+
 fn endpoint_repo(site: &str) -> EndpointRepo {
     EndpointRepo {
         endpoint: endpoint(site),

tests/unit/webhook.rs

@@ -342,7 +342,6 @@ fn blocked_webhook_install_is_skipped_and_recorded() {
             dry_run: false,
         },
         &state,
-        false,
     )
     .unwrap();
 
@@ -401,7 +400,7 @@ fn duplicate_webhook_error_records_existing_installation() {
 }
 
 #[test]
-fn install_task_state_cache_is_only_used_for_sync() {
+fn install_task_rechecks_cached_installation() {
     let (api_url, handle) = request_server(
         vec![("200 OK", "[]"), ("201 Created", r#"{"id":1}"#)],
         |index, request| match index {
@@ -442,7 +441,6 @@ fn install_task_state_cache_is_only_used_for_sync() {
             dry_run: false,
         },
         &state,
-        false,
     )
     .unwrap();