From 261f02439baf1f14cf02ac2d4747ec5772ad1a0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?m=C3=BCde?= Date: Tue, 7 Jul 2026 14:42:43 +0200 Subject: [PATCH] fix(forge): review fixes for the forgejo-api port --- Cargo.lock | 1 - hive-ag3nt/src/forge_notify.rs | 4 +++ hive-c0re/Cargo.toml | 1 - hive-c0re/src/forge/repos.rs | 52 +++++++++++++++++++++--------- hive-c0re/src/workers/knowledge.rs | 17 ++++++++-- hive-sh4re/src/assets.rs | 4 +-- 6 files changed, 57 insertions(+), 22 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8b59dfef..ebf70b9c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1439,7 +1439,6 @@ dependencies = [ "serde", "serde_json", "tempfile", - "time", "tokio", "tokio-stream", "tracing", diff --git a/hive-ag3nt/src/forge_notify.rs b/hive-ag3nt/src/forge_notify.rs index da6f6d89..8d5c8e9c 100644 --- a/hive-ag3nt/src/forge_notify.rs +++ b/hive-ag3nt/src/forge_notify.rs @@ -381,6 +381,10 @@ async fn format_notification( let subj = notif.thread.subject.as_ref(); let title = subj.and_then(|s| s.title.as_deref()).unwrap_or("?"); let subject_type = subj.and_then(|s| s.r#type); + // forgejo-api maps a blank `html_url` (Go marshals empty strings) to + // `None`, so the API-url fallback also covers present-but-empty — + // deliberate: a fetchable API link beats the raw-HTTP predecessor's + // empty `url:` line. let html_url = subj .and_then(|s| s.html_url.as_ref().or(s.url.as_ref())) .map_or("", url::Url::as_str); diff --git a/hive-c0re/Cargo.toml b/hive-c0re/Cargo.toml index b6dbd84c..de3e6b77 100644 --- a/hive-c0re/Cargo.toml +++ b/hive-c0re/Cargo.toml @@ -14,7 +14,6 @@ bcrypt.workspace = true reqwest.workspace = true forgejo-api.workspace = true url.workspace = true -time.workspace = true clap.workspace = true clap_complete.workspace = true clap-markdown = "0.1" diff --git a/hive-c0re/src/forge/repos.rs b/hive-c0re/src/forge/repos.rs index 215e4e25..9eacedeb 100644 --- a/hive-c0re/src/forge/repos.rs +++ b/hive-c0re/src/forge/repos.rs @@ -117,6 +117,18 @@ fn is_conflict(e: &ForgejoError) -> bool { } } +/// Whether an error is Forgejo saying 404 — the resource is absent, +/// as opposed to a transport / auth / server failure. +fn is_not_found(e: &ForgejoError) -> bool { + match e { + ForgejoError::ApiError(api) => { + matches!(api.error_kind(), ApiErrorKind::NotFound { .. }) + } + ForgejoError::UnexpectedStatusCode(s) => *s == StatusCode::NOT_FOUND, + _ => false, + } +} + /// Fold a repo-creation result's "already exists" (409 / 422) into /// success. `label` is `/` — purely for log + error /// context. @@ -474,23 +486,33 @@ async fn ensure_mirror_repo( admin_token: &str, ) -> Result<()> { let client = api(admin_token)?; - if client.repo_get(owner, repo).await.is_ok() { - // Mirror already present. Patch interval so mirrors seeded before - // this field was introduced (or with a different value) converge. - let mut edit = sparse_edit_repo_option(); - edit.mirror_interval = Some(MIRROR_INTERVAL.to_owned()); - match client.repo_edit(owner, repo, edit).await { - Ok(_) => { - tracing::debug!(%owner, %repo, interval = MIRROR_INTERVAL, "forge: pull-mirror interval updated"); - } - Err(e) => { - tracing::warn!( - %owner, %repo, error = %e, - "forge: failed to set mirror_interval on existing pull-mirror" - ); + match client.repo_get(owner, repo).await { + Ok(_) => { + // Mirror already present. Patch interval so mirrors seeded before + // this field was introduced (or with a different value) converge. + let mut edit = sparse_edit_repo_option(); + edit.mirror_interval = Some(MIRROR_INTERVAL.to_owned()); + match client.repo_edit(owner, repo, edit).await { + Ok(_) => { + tracing::debug!(%owner, %repo, interval = MIRROR_INTERVAL, "forge: pull-mirror interval updated"); + } + Err(e) => { + tracing::warn!( + %owner, %repo, error = %e, + "forge: failed to set mirror_interval on existing pull-mirror" + ); + } } + return Ok(()); + } + // Absent — fall through to migrate. + Err(e) if is_not_found(&e) => {} + // Anything else (transport, auth, 5xx) leaves the repo's existence + // unknown: migrating anyway would fold a 409 into success and skip + // the interval patch this pass. Surface it instead. + Err(e) => { + return Err(e).with_context(|| format!("get pull-mirror {owner}/{repo}")); } - return Ok(()); } let opts = MigrateRepoOptions { auth_password: None, diff --git a/hive-c0re/src/workers/knowledge.rs b/hive-c0re/src/workers/knowledge.rs index 7eef8455..8eaba76e 100644 --- a/hive-c0re/src/workers/knowledge.rs +++ b/hive-c0re/src/workers/knowledge.rs @@ -148,13 +148,23 @@ async fn seed_readme(core_token: &str) -> Result<()> { /// Called at startup alongside [`ensure_local_clone`]. No-op when the /// core token is absent (forge not yet provisioned). pub async fn ensure_webhook(core_token: &str, dashboard_port: u16) -> Result<()> { + // The typed client carries no per-request timeout, so each call is + // wrapped in one: this runs as a detached startup task, and a forge + // that accepts connections but never answers would otherwise hang + // it forever (and the hourly pull fallback masks the missing hook). + const HTTP_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); + let target_url = format!("http://127.0.0.1:{dashboard_port}/webhook/knowledge"); let client = crate::forge::api(core_token)?; // List existing hooks — skip creation if ours is already there. // Best-effort like the raw-HTTP predecessor: a listing failure // falls through to the create attempt. - match client.repo_list_hooks(ORG, REPO).all().await { + let listed = tokio::time::timeout(HTTP_TIMEOUT, client.repo_list_hooks(ORG, REPO).all()) + .await + .map_err(anyhow::Error::from) + .and_then(|r| r.map_err(anyhow::Error::from)); + match listed { Ok(hooks) => { let already_exists = hooks.iter().any(|h| { h.config @@ -186,9 +196,10 @@ pub async fn ensure_webhook(core_token: &str, dashboard_port: u16) -> Result<()> events: Some(vec!["push".to_owned()]), r#type: CreateHookOptionType::Forgejo, }; - client - .repo_create_hook(ORG, REPO, hook) + tokio::time::timeout(HTTP_TIMEOUT, client.repo_create_hook(ORG, REPO, hook)) .await + .map_err(anyhow::Error::from) + .and_then(|r| r.map_err(anyhow::Error::from)) .with_context(|| format!("create webhook for {ORG}/{REPO}"))?; tracing::info!(%target_url, "knowledge: push webhook created"); Ok(()) diff --git a/hive-sh4re/src/assets.rs b/hive-sh4re/src/assets.rs index 7f6d04bf..d0abf43d 100644 --- a/hive-sh4re/src/assets.rs +++ b/hive-sh4re/src/assets.rs @@ -1,7 +1,7 @@ //! Resolve the on-disk path to hyperhive's static assets (branding + //! claude prompts). Single source of truth for both the host daemon -//! (`hive-c0re`) and the in-container harness (the `hive` binary) so -//! they agree on the lookup contract. +//! (`hive-c0re`) and the in-container harness binaries so they agree +//! on the lookup contract. //! //! At runtime, the path is read from `$HIVE_ASSETS_DIR`. In nix //! builds that env var is set by the `hive-c0re` / `harness-base` modules