Merge pull request #10532 from Extra-Chill/fix/10408-reviewer-url-v2

fix(artifacts): return exact controller reviewer references

Author: chubes4

crates/homeboy-core/src/artifact_links.rs

@@ -2,6 +2,7 @@ use serde_json::Value;
 use std::path::Path;
 use std::time::Duration;
 
+use crate::error::{Error, Result};
 use crate::execution_contract::encode_uri_component;
 use crate::observation::{ArtifactRecord, ArtifactViewerLink};
 
@@ -81,6 +82,83 @@ pub fn public_artifact_url(artifact: &ArtifactRecord) -> Option<String> {
     ))
 }
 
+/// Return the controller's stable artifact route rather than a filesystem
+/// layout URL. Terminal handoffs use this route because it remains valid when
+/// the runner's artifact layout is unavailable after completion.
+pub fn controller_artifact_url(artifact: &ArtifactRecord) -> Result<Option<String>> {
+    if artifact.artifact_type != "file" {
+        return Err(Error::validation_invalid_argument(
+            "artifact.type",
+            "reviewer artifact URLs require a controller-owned file",
+            Some(artifact.id.clone()),
+            None,
+        ));
+    }
+    let Some(base) = reviewer_public_artifact_base_url()? else {
+        return Ok(None);
+    };
+    Ok(Some(format!(
+        "{}/runs/{}/artifacts/{}",
+        base,
+        encode_uri_component(&artifact.run_id),
+        encode_uri_component(&artifact.id)
+    )))
+}
+
+/// Resolve the public artifact origin once at the configuration boundary.
+/// Terminal handoffs only advertise HTTPS URLs on reviewer-reachable hosts.
+pub fn reviewer_public_artifact_base_url() -> Result<Option<String>> {
+    let configured = crate::defaults::load_config()
+        .artifact_origin
+        .public_base_url;
+    let value = configured.or_else(|| std::env::var(PUBLIC_ARTIFACT_BASE_URL_ENV).ok());
+    let Some(value) = value else {
+        return Ok(None);
+    };
+    let value = value.trim().trim_end_matches('/');
+    if value.is_empty() {
+        return Ok(None);
+    }
+    let url = reqwest::Url::parse(value).map_err(|error| {
+        Error::validation_invalid_argument(
+            PUBLIC_ARTIFACT_BASE_URL_ENV,
+            "public artifact origin must be a valid HTTPS URL",
+            Some(error.to_string()),
+            None,
+        )
+    })?;
+    let host = url.host_str().unwrap_or_default();
+    if url.scheme() != "https" || host.is_empty() || non_public_host(host) {
+        return Err(Error::validation_invalid_argument(
+            PUBLIC_ARTIFACT_BASE_URL_ENV,
+            "public artifact origin must use HTTPS with a reviewer-reachable host",
+            Some(value.to_string()),
+            None,
+        ));
+    }
+    Ok(Some(value.to_string()))
+}
+
+fn non_public_host(host: &str) -> bool {
+    let host = host.trim_matches(['[', ']']);
+    let lower = host.to_ascii_lowercase();
+    if lower == "localhost" || lower.ends_with(".localhost") || lower.ends_with(".local") {
+        return true;
+    }
+    match host.parse::<std::net::IpAddr>() {
+        Ok(std::net::IpAddr::V4(ip)) => {
+            ip.is_loopback() || ip.is_private() || ip.is_link_local() || ip.is_unspecified()
+        }
+        Ok(std::net::IpAddr::V6(ip)) => {
+            ip.is_loopback()
+                || ip.is_unspecified()
+                || (ip.segments()[0] & 0xfe00) == 0xfc00
+                || (ip.segments()[0] & 0xffc0) == 0xfe80
+        }
+        Err(_) => false,
+    }
+}
+
 pub fn public_artifact_path_url(root: &Path, base: &str, path: &Path) -> Option<String> {
     let base = base.trim().trim_end_matches('/');
     if base.is_empty() {
@@ -241,7 +319,9 @@ fn artifact_is_fetchable(artifact: &ArtifactRecord) -> bool {
         || artifact.artifact_type == "remote_file"
 }
 
-fn probe_public_artifact_url(public_url: &str) -> Result<reqwest::StatusCode, reqwest::Error> {
+fn probe_public_artifact_url(
+    public_url: &str,
+) -> std::result::Result<reqwest::StatusCode, reqwest::Error> {
     crate::http_probe::blocking_client(PUBLIC_ARTIFACT_URL_PROBE_TIMEOUT)?
         .get(public_url)
         .header(reqwest::header::RANGE, "bytes=0-0")
@@ -253,6 +333,9 @@ fn probe_public_artifact_url(public_url: &str) -> Result<reqwest::StatusCode, re
 mod tests {
     use super::*;
     use crate::observation::ArtifactRecord;
+    use std::sync::{Mutex, MutexGuard};
+
+    static ENV_LOCK: Mutex<()> = Mutex::new(());
 
     #[test]
     fn derives_viewer_link_from_public_artifact_url_metadata() {
@@ -346,6 +429,71 @@ mod tests {
         );
     }
 
+    #[test]
+    fn controller_url_requires_a_public_https_origin_and_encodes_ids() {
+        let _env = EnvGuard::set(
+            PUBLIC_ARTIFACT_BASE_URL_ENV,
+            "https://artifacts.example.test/reviewer/",
+        );
+        let artifact = ArtifactRecord {
+            id: "source /?%".to_string(),
+            run_id: "run /?%".to_string(),
+            artifact_type: "file".to_string(),
+            ..Default::default()
+        };
+
+        assert_eq!(
+            controller_artifact_url(&artifact)
+                .expect("valid reviewer origin")
+                .as_deref(),
+            Some("https://artifacts.example.test/reviewer/runs/run%20%2F%3F%25/artifacts/source%20%2F%3F%25")
+        );
+    }
+
+    #[test]
+    fn reviewer_public_artifact_base_rejects_local_or_non_https_origins() {
+        for value in ["http://artifacts.example.test", "https://127.0.0.1:7351"] {
+            let _env = EnvGuard::set(PUBLIC_ARTIFACT_BASE_URL_ENV, value);
+            assert!(reviewer_public_artifact_base_url().is_err(), "{value}");
+        }
+    }
+
+    #[test]
+    fn controller_origin_uses_typed_config_before_legacy_environment() {
+        crate::test_support::with_isolated_home(|_| {
+            let _env = EnvGuard::set(PUBLIC_ARTIFACT_BASE_URL_ENV, "https://runner.example.test");
+            let mut config = crate::defaults::HomeboyConfig::default();
+            config.artifact_origin.public_base_url =
+                Some("https://controller.example.test/".to_string());
+            crate::defaults::save_config(&config).expect("save controller config");
+
+            assert_eq!(
+                reviewer_public_artifact_base_url()
+                    .expect("configured origin")
+                    .as_deref(),
+                Some("https://controller.example.test")
+            );
+        });
+    }
+
+    #[test]
+    fn controller_url_is_absent_without_controller_origin() {
+        crate::test_support::with_isolated_home(|_| {
+            let _env = EnvGuard::unset(PUBLIC_ARTIFACT_BASE_URL_ENV);
+            let artifact = ArtifactRecord {
+                id: "artifact-1".to_string(),
+                run_id: "run-1".to_string(),
+                artifact_type: "file".to_string(),
+                ..Default::default()
+            };
+
+            assert_eq!(
+                controller_artifact_url(&artifact).expect("optional URL"),
+                None
+            );
+        });
+    }
+
     #[test]
     fn directory_artifact_public_url_requires_artifact_root_path() {
         let _env = EnvGuard::set(
@@ -399,13 +547,30 @@ mod tests {
     struct EnvGuard {
         key: &'static str,
         prior: Option<String>,
+        _lock: MutexGuard<'static, ()>,
     }
 
     impl EnvGuard {
         fn set(key: &'static str, value: &str) -> Self {
+            let lock = ENV_LOCK.lock().expect("environment lock");
             let prior = std::env::var(key).ok();
             std::env::set_var(key, value);
-            Self { key, prior }
+            Self {
+                key,
+                prior,
+                _lock: lock,
+            }
+        }
+
+        fn unset(key: &'static str) -> Self {
+            let lock = ENV_LOCK.lock().expect("environment lock");
+            let prior = std::env::var(key).ok();
+            std::env::remove_var(key);
+            Self {
+                key,
+                prior,
+                _lock: lock,
+            }
         }
     }
 

crates/homeboy-core/src/artifact_origin.rs

@@ -677,6 +677,120 @@ mod tests {
         assert_eq!(response.body, b"<html>report</html>");
     }
 
+    #[test]
+    fn serves_nested_visual_compare_reviewer_paths_over_http() {
+        let root = tempfile::tempdir().expect("artifact root");
+        let fixture = [
+            ("source.png", b"source bytes".as_slice()),
+            ("candidate.png", b"candidate bytes".as_slice()),
+            ("diff.png", b"diff bytes".as_slice()),
+        ];
+        let path = root.path().join("visual-compare/37-art-gallery-exhibition");
+        std::fs::create_dir_all(&path).expect("artifact directory");
+        for (name, bytes) in fixture {
+            std::fs::write(path.join(name), bytes).expect("visual artifact");
+        }
+
+        let listener = TcpListener::bind("127.0.0.1:0").expect("listener");
+        let address = listener.local_addr().expect("listener address");
+        let root_path = root.path().to_path_buf();
+        let server = std::thread::spawn(move || {
+            for stream in listener.incoming().take(3) {
+                handle_stream(stream.expect("request stream"), &root_path).expect("serve request");
+            }
+        });
+        let client = reqwest::blocking::Client::new();
+        for (name, bytes) in fixture {
+            let response = client
+                .get(format!(
+                    "http://{address}/visual-compare/37-art-gallery-exhibition/{name}"
+                ))
+                .send()
+                .expect("reviewer request");
+            assert_eq!(response.status(), reqwest::StatusCode::OK);
+            assert_eq!(response.bytes().expect("response bytes").as_ref(), bytes);
+        }
+        server.join().expect("origin server");
+    }
+
+    #[test]
+    fn serves_generated_canonical_reviewer_urls_over_http() {
+        crate::test_support::with_isolated_home(|home| {
+            let _env = EnvGuard::set(
+                crate::artifacts::PUBLIC_ARTIFACT_BASE_URL_ENV,
+                "https://artifacts.example.test",
+            );
+            let store = ObservationStore::open_initialized().expect("store");
+            let run = store
+                .start_run(NewRunRecord::builder("runner-exec").build())
+                .expect("run");
+            let fixture = [
+                ("source", "source.png", b"source bytes".as_slice()),
+                ("candidate", "candidate.png", b"candidate bytes".as_slice()),
+                ("diff", "diff.png", b"diff bytes".as_slice()),
+            ];
+            let source_dir = home.path().join("visual-compare/37-art-gallery-exhibition");
+            std::fs::create_dir_all(&source_dir).expect("artifact directory");
+            let artifacts = fixture
+                .iter()
+                .map(|(id, name, bytes)| {
+                    let source = source_dir.join(name);
+                    std::fs::write(&source, bytes).expect("visual artifact");
+                    store
+                        .record_artifact_with_id(
+                            &run.id,
+                            "visual_compare",
+                            &source,
+                            id,
+                            serde_json::json!({}),
+                        )
+                        .expect("controller artifact")
+                })
+                .collect::<Vec<_>>();
+
+            let listener = TcpListener::bind("127.0.0.1:0").expect("listener");
+            let address = listener.local_addr().expect("listener address");
+            let root = home.path().to_path_buf();
+            let server = std::thread::spawn(move || {
+                for stream in listener.incoming().take(3) {
+                    handle_stream(stream.expect("request stream"), &root).expect("serve request");
+                }
+            });
+            let client = reqwest::blocking::Client::new();
+            for ((id, _, bytes), artifact) in fixture.iter().zip(artifacts) {
+                let public_url = crate::artifact_links::controller_artifact_url(&artifact)
+                    .expect("valid reviewer origin")
+                    .expect("configured public reviewer URL");
+                assert_eq!(
+                    public_url,
+                    format!(
+                        "https://artifacts.example.test/runs/{}/artifacts/{id}",
+                        run.id
+                    )
+                );
+                let path = reqwest::Url::parse(&public_url)
+                    .expect("public URL")
+                    .path()
+                    .to_string();
+                let response = client
+                    .get(format!("http://{address}{path}"))
+                    .send()
+                    .expect("unauthenticated reviewer request");
+                assert_eq!(response.status(), reqwest::StatusCode::OK);
+                assert_eq!(
+                    response.headers()[reqwest::header::CONTENT_TYPE],
+                    "image/png"
+                );
+                assert_eq!(
+                    response.headers()["x-homeboy-artifact-sha256"],
+                    artifact.sha256.as_deref().expect("checksum")
+                );
+                assert_eq!(response.bytes().expect("response bytes").as_ref(), *bytes);
+            }
+            server.join().expect("origin server");
+        });
+    }
+
     #[test]
     fn inspect_reports_404_for_missing_workflow_bench_bundle_path() {
         let temp = tempfile::tempdir().expect("tempdir");