From 3c6dc2a979351c6c4ccae46a78aa19a5adf404c6 Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Wed, 22 Jul 2026 12:05:30 +0530 Subject: [PATCH 01/14] feat(adbc): scaffold ampup adbc subcommand Add an `adbc` subcommand (install/list/uninstall) mirroring the existing `self` subcommand structure. Runners are stubs that fail loudly until the download/placement path lands. Part of edgeandnode/amp#2600 (optional ADBC driver support in ampup). --- ampup/src/commands.rs | 1 + ampup/src/commands/adbc.rs | 21 +++++++++++++++++++++ ampup/src/main.rs | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+) create mode 100644 ampup/src/commands/adbc.rs diff --git a/ampup/src/commands.rs b/ampup/src/commands.rs index a361ccf..b64901c 100644 --- a/ampup/src/commands.rs +++ b/ampup/src/commands.rs @@ -1,3 +1,4 @@ +pub mod adbc; pub mod build; pub mod init; pub mod install; diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs new file mode 100644 index 0000000..73ce420 --- /dev/null +++ b/ampup/src/commands/adbc.rs @@ -0,0 +1,21 @@ +//! Optional ADBC driver components (#2600). +//! +//! Installs destination-specific ADBC driver libraries from the pinned set +//! shipped with each Amp release, so `ampd` can load them at runtime. + +use anyhow::{Result, bail}; + +/// Install an ADBC driver for the active amp version. +pub async fn install(driver: &str) -> Result<()> { + bail!("`ampup adbc install {driver}` is not yet implemented"); +} + +/// List installed ADBC drivers for the active version. +pub fn list() -> Result<()> { + bail!("`ampup adbc list` is not yet implemented"); +} + +/// Uninstall an ADBC driver. +pub fn uninstall(driver: &str) -> Result<()> { + bail!("`ampup adbc uninstall {driver}` is not yet implemented"); +} diff --git a/ampup/src/main.rs b/ampup/src/main.rs index 449cc2a..5d7a1e1 100644 --- a/ampup/src/main.rs +++ b/ampup/src/main.rs @@ -158,6 +158,12 @@ enum Commands { #[command(subcommand)] command: SelfCommands, }, + + /// Manage optional ADBC driver components + Adbc { + #[command(subcommand)] + command: AdbcCommands, + }, } #[derive(Debug, clap::Subcommand)] @@ -177,6 +183,24 @@ enum SelfCommands { Version, } +#[derive(Debug, clap::Subcommand)] +enum AdbcCommands { + /// Install an ADBC driver for the active amp version + Install { + /// Driver to install (e.g. postgresql) + driver: String, + }, + + /// List installed ADBC drivers for the active version + List, + + /// Uninstall an ADBC driver + Uninstall { + /// Driver to uninstall (e.g. postgresql) + driver: String, + }, +} + #[tokio::main] async fn main() { if let Err(e) = run().await { @@ -266,6 +290,17 @@ async fn run() -> anyhow::Result<()> { println!("ampup {}", env!("VERGEN_GIT_DESCRIBE")); } }, + Some(Commands::Adbc { command }) => match command { + AdbcCommands::Install { driver } => { + commands::adbc::install(&driver).await?; + } + AdbcCommands::List => { + commands::adbc::list()?; + } + AdbcCommands::Uninstall { driver } => { + commands::adbc::uninstall(&driver)?; + } + }, None => { // Default: install latest version (same as 'ampup update') commands::install::run( From 0d8168ce9640cd26c18dab79e2a7aca80f71a1cc Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Wed, 22 Jul 2026 12:26:43 +0530 Subject: [PATCH 02/14] feat(adbc): add driver catalog and release-asset naming Add an `adbc` module with a `Driver` catalog (postgresql) and `asset_name`, mapping a driver + platform/arch to the release asset `adbc-driver---.tar.gz`. Wire the command runners to validate the driver name against the catalog. Part of edgeandnode/amp#2600. --- ampup/src/adbc.rs | 73 ++++++++++++++++++++++++++++++++++++++ ampup/src/commands/adbc.rs | 19 +++++++++- ampup/src/lib.rs | 1 + 3 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 ampup/src/adbc.rs diff --git a/ampup/src/adbc.rs b/ampup/src/adbc.rs new file mode 100644 index 0000000..cf1344a --- /dev/null +++ b/ampup/src/adbc.rs @@ -0,0 +1,73 @@ +//! ADBC driver catalog and release-asset naming. +//! +//! Amp releases ship a pinned set of ADBC driver libraries as archive assets. +//! This module maps a supported driver plus a target platform/arch to the +//! release asset that carries it. + +use crate::platform::{Architecture, Platform}; + +/// A supported ADBC driver, as shipped in Amp releases. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Driver { + Postgresql, +} + +impl Driver { + /// Every supported driver. + pub const ALL: &'static [Driver] = &[Driver::Postgresql]; + + /// The driver's canonical name (the `` segment in asset names). + pub fn as_str(&self) -> &'static str { + match self { + Self::Postgresql => "postgresql", + } + } + + /// Parse a driver from its canonical name, or `None` if unsupported. + pub fn from_name(name: &str) -> Option { + Self::ALL.iter().copied().find(|d| d.as_str() == name) + } +} + +impl std::fmt::Display for Driver { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.as_str()) + } +} + +/// The GitHub release asset name for a driver on a given target. +/// +/// Must match the naming the release pipeline produces (edgeandnode/amp #2599): +/// `adbc-driver---.tar.gz`. +pub fn asset_name(driver: Driver, platform: Platform, arch: Architecture) -> String { + format!( + "adbc-driver-{}-{}-{}.tar.gz", + driver.as_str(), + platform.as_str(), + arch.as_str(), + ) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn asset_name_matches_release_contract() { + assert_eq!( + asset_name(Driver::Postgresql, Platform::Linux, Architecture::X86_64), + "adbc-driver-postgresql-linux-x86_64.tar.gz", + ); + assert_eq!( + asset_name(Driver::Postgresql, Platform::Darwin, Architecture::Aarch64), + "adbc-driver-postgresql-darwin-aarch64.tar.gz", + ); + } + + #[test] + fn from_name_accepts_supported_and_rejects_unknown() { + assert_eq!(Driver::from_name("postgresql"), Some(Driver::Postgresql)); + assert_eq!(Driver::from_name("mysql"), None); + assert_eq!(Driver::from_name(""), None); + } +} diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index 73ce420..858b9ba 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -3,10 +3,13 @@ //! Installs destination-specific ADBC driver libraries from the pinned set //! shipped with each Amp release, so `ampd` can load them at runtime. -use anyhow::{Result, bail}; +use anyhow::{Result, anyhow, bail}; + +use crate::adbc::Driver; /// Install an ADBC driver for the active amp version. pub async fn install(driver: &str) -> Result<()> { + let driver = parse_driver(driver)?; bail!("`ampup adbc install {driver}` is not yet implemented"); } @@ -17,5 +20,19 @@ pub fn list() -> Result<()> { /// Uninstall an ADBC driver. pub fn uninstall(driver: &str) -> Result<()> { + let driver = parse_driver(driver)?; bail!("`ampup adbc uninstall {driver}` is not yet implemented"); } + +/// Resolve a driver name against the catalog, with a helpful error listing the +/// supported drivers. +fn parse_driver(name: &str) -> Result { + Driver::from_name(name).ok_or_else(|| { + let supported = Driver::ALL + .iter() + .map(Driver::as_str) + .collect::>() + .join(", "); + anyhow!("unknown ADBC driver `{name}` (supported: {supported})") + }) +} diff --git a/ampup/src/lib.rs b/ampup/src/lib.rs index fa2c81f..262806f 100644 --- a/ampup/src/lib.rs +++ b/ampup/src/lib.rs @@ -1,3 +1,4 @@ +pub mod adbc; pub mod builder; pub mod commands; pub mod config; From b44c5e3fd8d45807a275298cdd949b21d57b6903 Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Wed, 22 Jul 2026 13:13:15 +0530 Subject: [PATCH 03/14] feat(download): verify artifact SHA-256 against release digest Read the digest GitHub advertises for each release asset and verify the downloaded bytes against it, failing with ChecksumMismatch on a mismatch. Applies to every download (binaries and, later, drivers). Previously only a non-empty check ran. - github: Asset/ResolvedAsset carry an optional digest. - download_manager: verify_artifact checks SHA-256 when a digest is present. - tests: verify_artifact match/mismatch/prefix/no-digest, plus end-to-end matching and mismatched-digest cases through download_all. Part of edgeandnode/amp#2600. --- Cargo.lock | 72 +++++++++++ ampup/Cargo.toml | 1 + ampup/src/download_manager.rs | 224 +++++++++++++++++++++++++++++++--- ampup/src/github.rs | 6 + 4 files changed, 288 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2db55cb..542b263 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -15,6 +15,7 @@ dependencies = [ "reqwest", "semver", "serde", + "sha2", "tempfile", "tokio", "vergen-gitcl", @@ -122,6 +123,15 @@ version = "2.10.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "812e12b5285cc515a9c72a5c1d3b6d46a19dac5acfef5265968c166106e31dd3" +[[package]] +name = "block-buffer" +version = "0.10.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3078c7629b62d3f0439517fa394996acacc5cbc91c5a20d8c658e77abd503a71" +dependencies = [ + "generic-array", +] + [[package]] name = "bumpalo" version = "3.19.1" @@ -258,6 +268,25 @@ version = "0.8.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "773648b94d0e5d620f64f280777445740e61fe701025087ec8b57f45c791888b" +[[package]] +name = "cpufeatures" +version = "0.2.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "59ed5838eebb26a2bb2e58f6d5b5316989ae9d08bab10e0e6d103e656d1b0280" +dependencies = [ + "libc", +] + +[[package]] +name = "crypto-common" +version = "0.1.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "78c8292055d1c1df0cce5d180393dc8cce0abec0a7102adb6c7b1eef6016d60a" +dependencies = [ + "generic-array", + "typenum", +] + [[package]] name = "darling" version = "0.20.11" @@ -345,6 +374,16 @@ dependencies = [ "zeroize", ] +[[package]] +name = "digest" +version = "0.10.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9ed9a281f7bc9b7576e61468ba615a66a5c8cfdff42420a70aa82701a3b1e292" +dependencies = [ + "block-buffer", + "crypto-common", +] + [[package]] name = "displaydoc" version = "0.2.5" @@ -521,6 +560,16 @@ dependencies = [ "slab", ] +[[package]] +name = "generic-array" +version = "0.14.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "85649ca51fd72272d7821adaf274ad91c288277713d9c18820d8499a7ff69e9a" +dependencies = [ + "typenum", + "version_check", +] + [[package]] name = "getrandom" version = "0.2.17" @@ -1438,6 +1487,17 @@ dependencies = [ "serde", ] +[[package]] +name = "sha2" +version = "0.10.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a7507d819769d01a365ab707794a4084392c824f54a7a6a7862f8c3d0892b283" +dependencies = [ + "cfg-if", + "cpufeatures", + "digest", +] + [[package]] name = "shell-words" version = "1.1.1" @@ -1752,6 +1812,12 @@ version = "0.2.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e421abadd41a4225275504ea4d6566923418b7f05506fbc9c0fe86ba7396114b" +[[package]] +name = "typenum" +version = "1.20.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b6f5e870be6c3b371b77fe0ee0bafb859fa4964b4404c27de1d380043c4dda20" + [[package]] name = "unicode-ident" version = "1.0.23" @@ -1838,6 +1904,12 @@ dependencies = [ "rustversion", ] +[[package]] +name = "version_check" +version = "0.9.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0b928f33d975fc6ad9f86c8f283853ad26bdd5b10b7f1542aa2fa15e2289105a" + [[package]] name = "walkdir" version = "2.5.0" diff --git a/ampup/Cargo.toml b/ampup/Cargo.toml index 5d3bf8c..a8d34da 100644 --- a/ampup/Cargo.toml +++ b/ampup/Cargo.toml @@ -25,6 +25,7 @@ reqwest = { version = "0.13", default-features = false, features = [ ] } semver = { version = "1.0.18", features = ["serde"] } serde = { version = "1.0", features = ["derive"] } +sha2 = "0.10" tempfile = "3.13.0" tokio = { version = "1.36.0", features = [ "macros", diff --git a/ampup/src/download_manager.rs b/ampup/src/download_manager.rs index 0a4c02b..e8e32eb 100644 --- a/ampup/src/download_manager.rs +++ b/ampup/src/download_manager.rs @@ -74,6 +74,17 @@ pub enum DownloadError { /// where the semaphore was dropped or closed while tasks were still waiting /// to acquire permits. SemaphoreClosed { artifact_name: String }, + + /// Downloaded artifact's SHA-256 did not match the digest advertised by + /// the release metadata. + /// + /// The bytes were corrupted or truncated in transit, or the release + /// metadata and asset are inconsistent. + ChecksumMismatch { + artifact_name: String, + expected: String, + actual: String, + }, } impl std::fmt::Display for DownloadError { @@ -117,6 +128,21 @@ impl std::fmt::Display for DownloadError { writeln!(f, "Internal error: concurrency semaphore closed")?; write!(f, " Artifact: {}", artifact_name)?; } + Self::ChecksumMismatch { + artifact_name, + expected, + actual, + } => { + writeln!(f, "Downloaded artifact failed integrity check")?; + writeln!(f, " Artifact: {}", artifact_name)?; + writeln!(f, " Expected SHA-256: {}", expected)?; + writeln!(f, " Actual SHA-256: {}", actual)?; + writeln!(f)?; + write!( + f, + " The download was corrupted or truncated. Please try again." + )?; + } } Ok(()) } @@ -127,7 +153,9 @@ impl std::error::Error for DownloadError { match self { Self::TaskFailed { source, .. } => Some(source.as_ref()), Self::StagingWrite { source, .. } => Some(source), - Self::EmptyArtifact { .. } | Self::SemaphoreClosed { .. } => None, + Self::EmptyArtifact { .. } + | Self::SemaphoreClosed { .. } + | Self::ChecksumMismatch { .. } => None, } } } @@ -139,8 +167,9 @@ impl std::error::Error for DownloadError { /// Manages bounded-concurrent downloads of release artifacts. /// /// Downloads proceed in parallel up to `max_concurrent` tasks. Each task -/// downloads an artifact, verifies it (currently: non-empty check), and -/// writes it to a staging directory. Only after all tasks succeed does +/// downloads an artifact, verifies it (non-empty, plus a SHA-256 digest check +/// when the release advertises one), and writes it to a staging directory. +/// Only after all tasks succeed does /// the staging directory get atomically renamed to the final version /// directory. /// @@ -230,7 +259,7 @@ impl DownloadManager { reporter.component_started(&task.artifact_name); let data = download_with_retry(&github, &asset).await?; - verify_artifact(&task.artifact_name, &data)?; + verify_artifact(&task.artifact_name, &data, asset.digest.as_deref())?; write_to_staging(&staging_path, &task.dest_filename, &data)?; Ok(task.artifact_name) @@ -366,17 +395,48 @@ async fn download_with_retry( } } -/// Verify a downloaded artifact. Currently checks non-empty. -/// Per-artifact checksum/attestation verification will be added in a follow-up PR. -fn verify_artifact(artifact_name: &str, data: &[u8]) -> std::result::Result<(), DownloadError> { +/// Verify a downloaded artifact: it must be non-empty, and — when the release +/// advertises a digest — its SHA-256 must match. +/// +/// `expected_digest` is the release-metadata digest (e.g. `"sha256:"`), or +/// `None` when the release does not provide one, in which case only the +/// non-empty check applies. +fn verify_artifact( + artifact_name: &str, + data: &[u8], + expected_digest: Option<&str>, +) -> std::result::Result<(), DownloadError> { if data.is_empty() { return Err(DownloadError::EmptyArtifact { artifact_name: artifact_name.to_string(), }); } + + if let Some(expected) = expected_digest { + // GitHub advertises digests as "sha256:"; tolerate a bare hex too. + let expected_hex = expected.strip_prefix("sha256:").unwrap_or(expected); + let actual_hex = sha256_hex(data); + if !actual_hex.eq_ignore_ascii_case(expected_hex) { + return Err(DownloadError::ChecksumMismatch { + artifact_name: artifact_name.to_string(), + expected: expected_hex.to_ascii_lowercase(), + actual: actual_hex, + }); + } + } + Ok(()) } +/// Lowercase hex SHA-256 of `data`. +fn sha256_hex(data: &[u8]) -> String { + use sha2::{Digest, Sha256}; + Sha256::digest(data) + .iter() + .map(|byte| format!("{byte:02x}")) + .collect() +} + /// Write artifact data to the staging directory. fn write_to_staging( staging_path: &Path, @@ -397,7 +457,8 @@ fn download_error_artifact_name(err: &DownloadError) -> &str { DownloadError::TaskFailed { artifact_name, .. } | DownloadError::EmptyArtifact { artifact_name } | DownloadError::StagingWrite { artifact_name, .. } - | DownloadError::SemaphoreClosed { artifact_name } => artifact_name, + | DownloadError::SemaphoreClosed { artifact_name } + | DownloadError::ChecksumMismatch { artifact_name, .. } => artifact_name, } } @@ -437,7 +498,7 @@ mod tests { let data: Vec = vec![]; //* When - let result = verify_artifact("ampd-linux-x86_64", &data); + let result = verify_artifact("ampd-linux-x86_64", &data, None); //* Then let err = result.expect_err("should return DownloadError for empty data"); @@ -447,6 +508,49 @@ mod tests { err ); } + + /// SHA-256 of "abc": a known vector used to exercise digest matching. + const ABC_SHA256: &str = "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad"; + + /// A matching digest passes, and the `sha256:` prefix is accepted. + #[test] + fn verify_artifact_accepts_matching_digest() { + let data = b"abc"; + verify_artifact("driver", data, Some(&format!("sha256:{ABC_SHA256}"))) + .expect("matching digest should verify"); + // Bare hex (no prefix) is tolerated too. + verify_artifact("driver", data, Some(ABC_SHA256)) + .expect("bare-hex matching digest should verify"); + // Digest comparison is case-insensitive. + verify_artifact("driver", data, Some(&ABC_SHA256.to_uppercase())) + .expect("uppercase digest should verify"); + } + + /// A wrong digest is rejected with `ChecksumMismatch`. + #[test] + fn verify_artifact_rejects_mismatched_digest() { + let data = b"abc"; + let wrong = "sha256:0000000000000000000000000000000000000000000000000000000000000000"; + + let err = verify_artifact("driver", data, Some(wrong)) + .expect_err("mismatched digest should fail"); + + assert!( + matches!(err, DownloadError::ChecksumMismatch { .. }), + "expected ChecksumMismatch, got: {:?}", + err + ); + } + + /// With no advertised digest, only the non-empty check applies. + #[test] + fn verify_artifact_without_digest_only_checks_non_empty() { + verify_artifact("driver", b"abc", None).expect("non-empty data should verify"); + + let err = verify_artifact("driver", b"", None) + .expect_err("empty data should still fail without a digest"); + assert!(matches!(err, DownloadError::EmptyArtifact { .. })); + } } #[cfg(unix)] @@ -720,18 +824,26 @@ mod tests { }) } - /// Build release JSON for the mock server with the given assets. - fn release_json(addr: std::net::SocketAddr, asset_names: &[&str]) -> Vec { - let assets: Vec = asset_names + /// Build release JSON where each asset may advertise a `digest`. + fn release_json_with_digests( + addr: std::net::SocketAddr, + assets: &[(&str, Option<&str>)], + ) -> Vec { + let assets: Vec = assets .iter() .enumerate() - .map(|(i, name)| { + .map(|(i, (name, digest))| { + let digest_field = match digest { + Some(d) => format!(r#","digest":"{d}""#), + None => String::new(), + }; format!( - r#"{{"id":{},"name":"{}","browser_download_url":"http://{}/download/{}"}}"#, + r#"{{"id":{},"name":"{}","browser_download_url":"http://{}/download/{}"{}}}"#, i + 1, name, addr, name, + digest_field, ) }) .collect(); @@ -755,13 +867,25 @@ mod tests { release_assets: &[&str], download_routes: Vec, max_concurrent: usize, + ) -> Self { + let assets: Vec<(&str, Option<&str>)> = + release_assets.iter().map(|name| (*name, None)).collect(); + Self::new_with_digests(&assets, download_routes, max_concurrent).await + } + + /// Like [`new`](Self::new), but each release asset may advertise a + /// `digest` in its metadata. + async fn new_with_digests( + release_assets: &[(&str, Option<&str>)], + download_routes: Vec, + max_concurrent: usize, ) -> Self { let listener = tokio::net::TcpListener::bind("127.0.0.1:0") .await .expect("should bind to a random port"); let addr = listener.local_addr().expect("should have a local address"); - let release_body = release_json(addr, release_assets); + let release_body = release_json_with_digests(addr, release_assets); let mut routes = vec![Route::ok("tags/v1.0.0", release_body)]; routes.extend(download_routes); @@ -852,6 +976,76 @@ mod tests { ); } + /// A correct release digest verifies through the full download pipeline. + #[tokio::test] + async fn download_all_verifies_matching_digest() { + //* Given + let ampd_data = b"fake-ampd-binary".to_vec(); + let digest = format!("sha256:{}", super::super::sha256_hex(&d_data)); + + let fixture = TestFixture::new_with_digests( + &[("ampd-linux-x86_64", Some(digest.as_str()))], + vec![Route::ok("download/ampd-linux-x86_64", ampd_data.clone())], + 4, + ) + .await; + + //* When + let result = fixture + .download(vec![DownloadTask { + artifact_name: "ampd-linux-x86_64".to_string(), + dest_filename: "ampd".to_string(), + optional: false, + }]) + .await; + + //* Then + assert!( + result.is_ok(), + "a valid digest should verify: {:?}", + result.err() + ); + assert_eq!( + fs::read(fixture.version_dir.join("ampd")).expect("should read ampd"), + ampd_data, + ); + } + + /// A wrong release digest fails the batch and leaves no partial install. + #[tokio::test] + async fn download_all_rejects_mismatched_digest() { + //* Given + let ampd_data = b"fake-ampd-binary".to_vec(); + let wrong = "sha256:0000000000000000000000000000000000000000000000000000000000000000"; + + let fixture = TestFixture::new_with_digests( + &[("ampd-linux-x86_64", Some(wrong))], + vec![Route::ok("download/ampd-linux-x86_64", ampd_data)], + 4, + ) + .await; + + //* When + let result = fixture + .download(vec![DownloadTask { + artifact_name: "ampd-linux-x86_64".to_string(), + dest_filename: "ampd".to_string(), + optional: false, + }]) + .await; + + //* Then + let err = result.expect_err("a mismatched digest should fail the download"); + assert!( + err.to_string().contains("integrity check"), + "expected an integrity-check error, got: {err}" + ); + assert!( + !fixture.version_dir.exists(), + "no version directory should be created on integrity failure" + ); + } + /// A missing asset fails the whole batch and leaves no partial install. #[tokio::test] async fn download_all_with_missing_asset_fails_without_partial_install() { diff --git a/ampup/src/github.rs b/ampup/src/github.rs index b31859c..5516f46 100644 --- a/ampup/src/github.rs +++ b/ampup/src/github.rs @@ -184,6 +184,9 @@ pub struct ResolvedAsset { pub name: String, /// Direct browser download URL (used for public repos). pub url: String, + /// Expected content digest from release metadata (e.g. "sha256:"), + /// or `None` when the release does not advertise one. + pub digest: Option, } /// The assets of a single fetched release. @@ -211,6 +214,7 @@ impl ReleaseAssets { id: asset.id, name: asset.name.clone(), url: asset.url.clone(), + digest: asset.digest.clone(), })), // Optional artifacts may be missing from a release; skip them. None if optional => Ok(None), @@ -238,6 +242,8 @@ struct Asset { name: String, #[serde(rename = "browser_download_url")] url: String, + #[serde(default)] + digest: Option, } /// Cloneable so `DownloadManager` can move a handle into each spawned task. From cd7de499c54eeab15443a8ffee94ce6be71fb4fa Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Wed, 22 Jul 2026 13:37:39 +0530 Subject: [PATCH 04/14] feat(adbc): fetch, verify, and extract driver archive Wire `ampup adbc install ` to resolve the active amp version, download the pinned driver archive, verify its SHA-256 digest, and extract the validated members to a staging dir. Adds a flat-tar.gz extractor (traversal-guarded) and the driver runtime-lib naming. Placement into the version dir is not yet wired. Part of edgeandnode/amp#2600. --- Cargo.lock | 74 +++++++++++++++++ ampup/Cargo.toml | 2 + ampup/src/adbc.rs | 22 ++++++ ampup/src/archive.rs | 144 ++++++++++++++++++++++++++++++++++ ampup/src/commands/adbc.rs | 96 ++++++++++++++++++++++- ampup/src/download_manager.rs | 2 +- ampup/src/lib.rs | 1 + ampup/src/main.rs | 45 ++++++++++- 8 files changed, 379 insertions(+), 7 deletions(-) create mode 100644 ampup/src/archive.rs diff --git a/Cargo.lock b/Cargo.lock index 542b263..5760403 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2,6 +2,12 @@ # It is not intended for manual editing. version = 4 +[[package]] +name = "adler2" +version = "2.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "320119579fcad9c21884f5c4861d16174d0e06250625266f50fe6898340abefa" + [[package]] name = "ampup" version = "0.1.0" @@ -10,12 +16,14 @@ dependencies = [ "clap", "console", "dialoguer", + "flate2", "fs-err", "futures", "reqwest", "semver", "serde", "sha2", + "tar", "tempfile", "tokio", "vergen-gitcl", @@ -277,6 +285,15 @@ dependencies = [ "libc", ] +[[package]] +name = "crc32fast" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9481c1c90cbf2ac953f07c8d4a58aa3945c425b7185c9154d67a65e4230da511" +dependencies = [ + "cfg-if", +] + [[package]] name = "crypto-common" version = "0.1.7" @@ -429,12 +446,32 @@ version = "2.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "37909eebbb50d72f9059c3b6d82c0463f2ff062c9e95845c43a6c9c0355411be" +[[package]] +name = "filetime" +version = "0.2.29" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5c287a33c7f0a620c38e641e7f60827713987b3c0f26e8ddc9462cc69cf75759" +dependencies = [ + "cfg-if", + "libc", +] + [[package]] name = "find-msvc-tools" version = "0.1.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5baebc0774151f905a1a2cc41989300b1e6fbb29aff0ceffa1064fdd3088d582" +[[package]] +name = "flate2" +version = "1.1.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "843fba2746e448b37e26a819579957415c8cef339bf08564fe8b7ddbd959573c" +dependencies = [ + "crc32fast", + "miniz_oxide", +] + [[package]] name = "fnv" version = "1.0.7" @@ -977,6 +1014,16 @@ version = "2.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f8ca58f447f06ed17d5fc4043ce1b10dd205e060fb3ce5b979b8ed8e59ff3f79" +[[package]] +name = "miniz_oxide" +version = "0.8.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1fa76a2c86f704bdb222d66965fb3d63269ce38518b83cb0575fca855ebb6316" +dependencies = [ + "adler2", + "simd-adler32", +] + [[package]] name = "mio" version = "1.1.1" @@ -1510,6 +1557,12 @@ version = "1.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0fda2ff0d084019ba4d7c6f371c95d8fd75ce3524c3cb8fb653a3023f6323e64" +[[package]] +name = "simd-adler32" +version = "0.3.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3a219298ac11a56ea9a6d2120044824d6f01aeb034955e7af7bc16858527deea" + [[package]] name = "slab" version = "0.4.12" @@ -1581,6 +1634,17 @@ dependencies = [ "syn", ] +[[package]] +name = "tar" +version = "0.4.46" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3f6221d9a6003c78398e3b239969f352578258df48c8eb051caadae0015bc840" +dependencies = [ + "filetime", + "libc", + "xattr", +] + [[package]] name = "tempfile" version = "3.25.0" @@ -2419,6 +2483,16 @@ version = "0.6.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9edde0db4769d2dc68579893f2306b26c6ecfbe0ef499b013d731b7b9247e0b9" +[[package]] +name = "xattr" +version = "1.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32e45ad4206f6d2479085147f02bc2ef834ac85886624a23575ae137c8aa8156" +dependencies = [ + "libc", + "rustix", +] + [[package]] name = "yoke" version = "0.8.1" diff --git a/ampup/Cargo.toml b/ampup/Cargo.toml index a8d34da..cbef488 100644 --- a/ampup/Cargo.toml +++ b/ampup/Cargo.toml @@ -15,6 +15,7 @@ anyhow = "1.0.80" clap = { version = "4.5.2", features = ["derive", "env"] } console = "0.16" dialoguer = "0.12" +flate2 = "1.0" fs-err = "3.0.0" futures = "0.3" reqwest = { version = "0.13", default-features = false, features = [ @@ -26,6 +27,7 @@ reqwest = { version = "0.13", default-features = false, features = [ semver = { version = "1.0.18", features = ["serde"] } serde = { version = "1.0", features = ["derive"] } sha2 = "0.10" +tar = "0.4" tempfile = "3.13.0" tokio = { version = "1.36.0", features = [ "macros", diff --git a/ampup/src/adbc.rs b/ampup/src/adbc.rs index cf1344a..ee98b91 100644 --- a/ampup/src/adbc.rs +++ b/ampup/src/adbc.rs @@ -27,6 +27,16 @@ impl Driver { pub fn from_name(name: &str) -> Option { Self::ALL.iter().copied().find(|d| d.as_str() == name) } + + /// The runtime library filename this driver's archive carries on + /// `platform` (e.g. `libadbc_driver_postgresql.so` on Linux). + pub fn runtime_lib_filename(&self, platform: Platform) -> String { + let ext = match platform { + Platform::Linux => "so", + Platform::Darwin => "dylib", + }; + format!("libadbc_driver_{}.{ext}", self.as_str()) + } } impl std::fmt::Display for Driver { @@ -70,4 +80,16 @@ mod tests { assert_eq!(Driver::from_name("mysql"), None); assert_eq!(Driver::from_name(""), None); } + + #[test] + fn runtime_lib_filename_is_platform_specific() { + assert_eq!( + Driver::Postgresql.runtime_lib_filename(Platform::Linux), + "libadbc_driver_postgresql.so", + ); + assert_eq!( + Driver::Postgresql.runtime_lib_filename(Platform::Darwin), + "libadbc_driver_postgresql.dylib", + ); + } } diff --git a/ampup/src/archive.rs b/ampup/src/archive.rs new file mode 100644 index 0000000..97afe3b --- /dev/null +++ b/ampup/src/archive.rs @@ -0,0 +1,144 @@ +//! Extraction of ADBC driver release archives (flat gzip-compressed tar). +//! +//! Driver archives contain exactly a driver library plus `LICENSE.txt` and +//! `NOTICE.txt` at the root. Extraction enforces that flat shape and an exact +//! member set, refusing anything that could escape the destination directory. + +use std::{ + collections::BTreeSet, + path::{Component, Path}, +}; + +use anyhow::{Context, Result, bail}; +use flate2::read::GzDecoder; +use tar::Archive; + +/// Extract a gzip-compressed tar archive into `dest`, requiring a flat layout +/// whose entries exactly equal `expected_members`. +/// +/// Every entry must be a single normal path component (no directories, no +/// `..`), so an entry can never be written outside `dest`. Any unexpected or +/// missing member is an error. +pub fn extract_and_validate(data: &[u8], dest: &Path, expected_members: &[&str]) -> Result<()> { + let mut archive = Archive::new(GzDecoder::new(data)); + let mut seen = BTreeSet::new(); + + for entry in archive + .entries() + .context("failed to read archive entries")? + { + let mut entry = entry.context("failed to read an archive entry")?; + let path = entry.path().context("archive entry has an invalid path")?; + + // A flat file name is exactly one normal component. This rejects + // nested paths, absolute paths, and `..`, so `dest.join(name)` stays + // inside `dest`. + let mut components = path.components(); + let name = match (components.next(), components.next()) { + (Some(Component::Normal(name)), None) => name + .to_str() + .context("archive entry name is not valid UTF-8")? + .to_owned(), + _ => bail!("archive entry {:?} is not a flat file name", path), + }; + + if !expected_members.contains(&name.as_str()) { + bail!("unexpected member in driver archive: {name}"); + } + + entry + .unpack(dest.join(&name)) + .with_context(|| format!("failed to extract {name}"))?; + seen.insert(name); + } + + let expected: BTreeSet = expected_members.iter().map(|m| m.to_string()).collect(); + if seen != expected { + let missing: Vec = expected.difference(&seen).cloned().collect(); + bail!( + "driver archive is missing expected members: {}", + missing.join(", ") + ); + } + + Ok(()) +} + +#[cfg(test)] +mod tests { + use flate2::{Compression, write::GzEncoder}; + use tar::{Builder, Header}; + + use super::*; + + /// Build a gzip-compressed tar archive from `(name, contents)` pairs. + fn make_targz(files: &[(&str, &[u8])]) -> Vec { + let mut builder = Builder::new(GzEncoder::new(Vec::new(), Compression::default())); + for (name, data) in files { + let mut header = Header::new_gnu(); + header.set_size(data.len() as u64); + header.set_mode(0o644); + header.set_cksum(); + builder + .append_data(&mut header, name, *data) + .expect("should append tar entry"); + } + builder + .into_inner() + .expect("should finish tar") + .finish() + .expect("should finish gzip") + } + + const MEMBERS: &[&str] = &["libadbc_driver_postgresql.so", "LICENSE.txt", "NOTICE.txt"]; + + #[test] + fn extracts_exact_members() { + let archive = make_targz(&[ + ("libadbc_driver_postgresql.so", b"ELF"), + ("LICENSE.txt", b"license"), + ("NOTICE.txt", b"notice"), + ]); + let dir = tempfile::tempdir().expect("tempdir"); + + extract_and_validate(&archive, dir.path(), MEMBERS).expect("should extract"); + + assert_eq!( + std::fs::read(dir.path().join("libadbc_driver_postgresql.so")).expect("lib"), + b"ELF", + ); + } + + #[test] + fn rejects_unexpected_member() { + let archive = make_targz(&[("evil.sh", b"rm -rf /")]); + let dir = tempfile::tempdir().expect("tempdir"); + + let err = extract_and_validate(&archive, dir.path(), MEMBERS) + .expect_err("unexpected member should fail"); + assert!(err.to_string().contains("unexpected member"), "got: {err}"); + } + + #[test] + fn rejects_missing_member() { + let archive = make_targz(&[("LICENSE.txt", b"license")]); + let dir = tempfile::tempdir().expect("tempdir"); + + let err = extract_and_validate(&archive, dir.path(), &["LICENSE.txt", "NOTICE.txt"]) + .expect_err("missing member should fail"); + assert!( + err.to_string().contains("missing expected members"), + "got: {err}" + ); + } + + #[test] + fn rejects_non_flat_entry() { + let archive = make_targz(&[("nested/lib.so", b"x")]); + let dir = tempfile::tempdir().expect("tempdir"); + + let err = extract_and_validate(&archive, dir.path(), &["lib.so"]) + .expect_err("nested entry should fail"); + assert!(err.to_string().contains("flat file name"), "got: {err}"); + } +} diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index 858b9ba..0201f03 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -3,14 +3,78 @@ //! Installs destination-specific ADBC driver libraries from the pinned set //! shipped with each Amp release, so `ampd` can load them at runtime. -use anyhow::{Result, anyhow, bail}; +use std::path::PathBuf; -use crate::adbc::Driver; +use anyhow::{Context, Result, anyhow, bail}; + +use crate::{ + adbc::Driver, + archive, + config::Config, + download_manager::verify_artifact, + github::GitHubClient, + platform::{Architecture, Platform, PlatformError}, + token, ui, + version_manager::VersionManager, +}; /// Install an ADBC driver for the active amp version. -pub async fn install(driver: &str) -> Result<()> { +pub async fn install( + driver: &str, + install_dir: Option, + repo: String, + github_token: Option, + arch: Option, + platform: Option, + _jobs: usize, +) -> Result<()> { let driver = parse_driver(driver)?; - bail!("`ampup adbc install {driver}` is not yet implemented"); + + let config = Config::new(install_dir)?; + let token = token::resolve_github_token(github_token); + let github = GitHubClient::new(repo, token)?; + let version_manager = VersionManager::new(config); + + // Drivers are pinned to an installed amp version, so one must be active. + let version = version_manager + .get_current()? + .ok_or_else(|| anyhow!("no active amp version; run `ampup install` first"))?; + + let platform = resolve_platform(platform)?; + let arch = resolve_arch(arch)?; + + let asset_name = crate::adbc::asset_name(driver, platform, arch); + ui::info!( + "Installing {} driver for amp {}", + driver, + ui::version(&version) + ); + + let assets = github.fetch_release_assets(&version).await?; + let asset = assets + .resolve(&asset_name, false)? + .expect("a required asset resolves to Some or errors"); + + let data = github.download_resolved_asset(&asset).await?; + verify_artifact(&asset.name, &data, asset.digest.as_deref()) + .context("driver archive failed verification")?; + + // Extract into a staging dir; placement into the version's drivers + // directory is handled separately. + let staging = tempfile::tempdir().context("failed to create staging directory")?; + let lib = driver.runtime_lib_filename(platform); + archive::extract_and_validate( + &data, + staging.path(), + &[lib.as_str(), "LICENSE.txt", "NOTICE.txt"], + )?; + + ui::detail!( + "Fetched, verified, and extracted the {} driver ({} bytes)", + driver, + data.len() + ); + bail!("installing the driver into the amp version is not yet implemented"); } /// List installed ADBC drivers for the active version. @@ -36,3 +100,27 @@ fn parse_driver(name: &str) -> Result { anyhow!("unknown ADBC driver `{name}` (supported: {supported})") }) } + +/// Resolve the target platform from an optional `--platform` override. +fn resolve_platform(over: Option) -> Result { + match over { + Some(p) => match p.as_str() { + "linux" => Ok(Platform::Linux), + "darwin" => Ok(Platform::Darwin), + _ => Err(PlatformError::UnsupportedPlatform { detected: p }.into()), + }, + None => Ok(Platform::detect()?), + } +} + +/// Resolve the target architecture from an optional `--arch` override. +fn resolve_arch(over: Option) -> Result { + match over { + Some(a) => match a.as_str() { + "x86_64" | "amd64" => Ok(Architecture::X86_64), + "aarch64" | "arm64" => Ok(Architecture::Aarch64), + _ => Err(PlatformError::UnsupportedArchitecture { detected: a }.into()), + }, + None => Ok(Architecture::detect()?), + } +} diff --git a/ampup/src/download_manager.rs b/ampup/src/download_manager.rs index e8e32eb..9940f06 100644 --- a/ampup/src/download_manager.rs +++ b/ampup/src/download_manager.rs @@ -401,7 +401,7 @@ async fn download_with_retry( /// `expected_digest` is the release-metadata digest (e.g. `"sha256:"`), or /// `None` when the release does not provide one, in which case only the /// non-empty check applies. -fn verify_artifact( +pub(crate) fn verify_artifact( artifact_name: &str, data: &[u8], expected_digest: Option<&str>, diff --git a/ampup/src/lib.rs b/ampup/src/lib.rs index 262806f..6610a61 100644 --- a/ampup/src/lib.rs +++ b/ampup/src/lib.rs @@ -1,4 +1,5 @@ pub mod adbc; +pub mod archive; pub mod builder; pub mod commands; pub mod config; diff --git a/ampup/src/main.rs b/ampup/src/main.rs index 5d7a1e1..8fc6a1c 100644 --- a/ampup/src/main.rs +++ b/ampup/src/main.rs @@ -187,8 +187,32 @@ enum SelfCommands { enum AdbcCommands { /// Install an ADBC driver for the active amp version Install { + /// Installation directory (defaults to $AMP_DIR or $XDG_CONFIG_HOME/.amp or $HOME/.amp) + #[arg(long, env = "AMP_DIR")] + install_dir: Option, + /// Driver to install (e.g. postgresql) driver: String, + + /// GitHub repository in format "owner/repo" + #[arg(long, default_value_t = DEFAULT_REPO.to_string())] + repo: String, + + /// GitHub token for private repository access (defaults to $GITHUB_TOKEN) + #[arg(long, env = "GITHUB_TOKEN", hide_env = true)] + github_token: Option, + + /// Override architecture detection (x86_64, aarch64) + #[arg(long)] + arch: Option, + + /// Override platform detection (linux, darwin) + #[arg(long)] + platform: Option, + + /// Number of concurrent downloads + #[arg(short = 'j', long = "jobs", default_value_t = DEFAULT_DOWNLOAD_JOBS)] + jobs: usize, }, /// List installed ADBC drivers for the active version @@ -291,8 +315,25 @@ async fn run() -> anyhow::Result<()> { } }, Some(Commands::Adbc { command }) => match command { - AdbcCommands::Install { driver } => { - commands::adbc::install(&driver).await?; + AdbcCommands::Install { + install_dir, + driver, + repo, + github_token, + arch, + platform, + jobs, + } => { + commands::adbc::install( + &driver, + install_dir, + repo, + github_token, + arch, + platform, + jobs, + ) + .await?; } AdbcCommands::List => { commands::adbc::list()?; From 6ef6e8d7abd8885bbd79f1a5a45ba94b58467d58 Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Wed, 22 Jul 2026 10:43:24 +0000 Subject: [PATCH 05/14] feat(adbc): place driver and write manifest into version dir Install now moves the extracted driver into a self-contained ~/.amp/versions//drivers// and writes an ADBC manifest.toml (typed via the toml crate) whose Driver.shared is the absolute library path, so ampd can load the driver by manifest. Staging happens inside the drivers dir so the final placement is a same-filesystem atomic rename; reinstalls replace the existing dir. Part of edgeandnode/amp#2600. --- Cargo.lock | 55 +++++++++++++++++++ ampup/Cargo.toml | 1 + ampup/src/adbc.rs | 49 +++++++++++++++++ ampup/src/commands/adbc.rs | 107 ++++++++++++++++++++++++++++++++++--- ampup/src/config.rs | 12 +++++ 5 files changed, 216 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5760403..5c45e74 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -26,6 +26,7 @@ dependencies = [ "tar", "tempfile", "tokio", + "toml", "vergen-gitcl", ] @@ -1522,6 +1523,15 @@ dependencies = [ "zmij", ] +[[package]] +name = "serde_spanned" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6662b5879511e06e8999a8a235d848113e942c9124f211511b16466ee2995f26" +dependencies = [ + "serde_core", +] + [[package]] name = "serde_urlencoded" version = "0.7.1" @@ -1806,6 +1816,45 @@ dependencies = [ "tokio", ] +[[package]] +name = "toml" +version = "1.1.3+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "53c96ecdfa941c8fc4fcaed14f99ada8ebed502eef533015095a07e3301d4c3c" +dependencies = [ + "indexmap", + "serde_core", + "serde_spanned", + "toml_datetime", + "toml_parser", + "toml_writer", + "winnow", +] + +[[package]] +name = "toml_datetime" +version = "1.1.1+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3165f65f62e28e0115a00b2ebdd37eb6f3b641855f9d636d3cd4103767159ad7" +dependencies = [ + "serde_core", +] + +[[package]] +name = "toml_parser" +version = "1.1.2+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a2abe9b86193656635d2411dc43050282ca48aa31c2451210f4202550afb7526" +dependencies = [ + "winnow", +] + +[[package]] +name = "toml_writer" +version = "1.1.2+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7d56353a2a665ad0f41a421187180aab746c8c325620617ad883a99a1cbe66d2" + [[package]] name = "tower" version = "0.5.3" @@ -2389,6 +2438,12 @@ version = "0.53.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d6bbff5f0aada427a1e5a6da5f1f98158182f26556f345ac9e04d36d0ebed650" +[[package]] +name = "winnow" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "23b97319f7b8343df12cc98938e5c3eb436064524c8d2b4e30a1d3a36eecdf81" + [[package]] name = "wit-bindgen" version = "0.51.0" diff --git a/ampup/Cargo.toml b/ampup/Cargo.toml index cbef488..7300fd4 100644 --- a/ampup/Cargo.toml +++ b/ampup/Cargo.toml @@ -36,6 +36,7 @@ tokio = { version = "1.36.0", features = [ "sync", "test-util", ] } +toml = "1.1.3" [build-dependencies] vergen-gitcl = { version = "9.0.0", features = ["build"] } diff --git a/ampup/src/adbc.rs b/ampup/src/adbc.rs index ee98b91..b93ed2f 100644 --- a/ampup/src/adbc.rs +++ b/ampup/src/adbc.rs @@ -4,6 +4,10 @@ //! This module maps a supported driver plus a target platform/arch to the //! release asset that carries it. +use std::path::Path; + +use serde::Serialize; + use crate::platform::{Architecture, Platform}; /// A supported ADBC driver, as shipped in Amp releases. @@ -58,6 +62,36 @@ pub fn asset_name(driver: Driver, platform: Platform, arch: Architecture) -> Str ) } +/// An ADBC driver manifest (`manifest.toml`), the on-disk contract `ampd` +/// reads to load a driver by absolute path. +/// +/// Only `Driver.shared` is required; the postgres driver's entrypoint is +/// derived from its name, so no `entrypoint` key is emitted. +#[derive(Serialize)] +struct Manifest { + manifest_version: u32, + #[serde(rename = "Driver")] + driver: DriverSection, +} + +#[derive(Serialize)] +struct DriverSection { + shared: String, +} + +/// Render the `manifest.toml` contents pointing `ampd` at the driver library +/// at `lib_path` (an absolute path). +pub fn driver_manifest(lib_path: &Path) -> String { + let manifest = Manifest { + manifest_version: 1, + driver: DriverSection { + shared: lib_path.to_string_lossy().into_owned(), + }, + }; + // Serializing a fixed two-field struct cannot fail. + toml::to_string(&manifest).expect("driver manifest serializes") +} + #[cfg(test)] mod tests { use super::*; @@ -92,4 +126,19 @@ mod tests { "libadbc_driver_postgresql.dylib", ); } + + #[test] + fn driver_manifest_round_trips_the_library_path() { + // A path with a quote and a backslash must survive escaping so the + // manifest stays valid TOML that parses back to the same path. + let path = Path::new("/home/a\"b\\c/drivers/postgresql/libadbc_driver_postgresql.so"); + let rendered = driver_manifest(path); + + let parsed: toml::Value = toml::from_str(&rendered).expect("manifest is valid TOML"); + assert_eq!(parsed["manifest_version"].as_integer(), Some(1)); + assert_eq!( + parsed["Driver"]["shared"].as_str(), + Some(path.to_string_lossy().as_ref()), + ); + } } diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index 0201f03..ec089f1 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -3,9 +3,10 @@ //! Installs destination-specific ADBC driver libraries from the pinned set //! shipped with each Amp release, so `ampd` can load them at runtime. -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use anyhow::{Context, Result, anyhow, bail}; +use fs_err as fs; use crate::{ adbc::Driver, @@ -59,9 +60,13 @@ pub async fn install( verify_artifact(&asset.name, &data, asset.digest.as_deref()) .context("driver archive failed verification")?; - // Extract into a staging dir; placement into the version's drivers - // directory is handled separately. - let staging = tempfile::tempdir().context("failed to create staging directory")?; + // Stage inside the drivers directory so the final rename stays on one + // filesystem (atomic, no cross-device copy). + let drivers_dir = version_manager.config().drivers_dir(&version); + fs::create_dir_all(&drivers_dir).context("failed to create drivers directory")?; + let staging = + tempfile::tempdir_in(&drivers_dir).context("failed to create staging directory")?; + let lib = driver.runtime_lib_filename(platform); archive::extract_and_validate( &data, @@ -69,12 +74,41 @@ pub async fn install( &[lib.as_str(), "LICENSE.txt", "NOTICE.txt"], )?; - ui::detail!( - "Fetched, verified, and extracted the {} driver ({} bytes)", + // Absolute so the manifest's `Driver.shared` resolves regardless of + // ampd's working directory. + let driver_dir = std::path::absolute( + version_manager + .config() + .driver_dir(&version, driver.as_str()), + ) + .context("failed to resolve driver directory")?; + place_driver(staging.path(), &driver_dir, &lib)?; + // The staged files now live at `driver_dir`; disarm TempDir cleanup. + let _ = staging.keep(); + + ui::info!( + "Installed {} driver for amp {} at {}", driver, - data.len() + ui::version(&version), + driver_dir.display(), ); - bail!("installing the driver into the amp version is not yet implemented"); + Ok(()) +} + +/// Assemble the self-contained driver directory: write the manifest into the +/// staged files (pointing `Driver.shared` at the final library path), then +/// atomically move the staged directory into `driver_dir`, replacing any +/// existing install. +fn place_driver(staging: &Path, driver_dir: &Path, lib_name: &str) -> Result<()> { + let manifest = crate::adbc::driver_manifest(&driver_dir.join(lib_name)); + fs::write(staging.join("manifest.toml"), manifest) + .context("failed to write ADBC driver manifest")?; + + if driver_dir.exists() { + fs::remove_dir_all(driver_dir).context("failed to remove the existing driver directory")?; + } + fs::rename(staging, driver_dir).context("failed to move the driver into place")?; + Ok(()) } /// List installed ADBC drivers for the active version. @@ -124,3 +158,60 @@ fn resolve_arch(over: Option) -> Result { None => Ok(Architecture::detect()?), } } + +#[cfg(test)] +mod tests { + use super::*; + + const LIB: &str = "libadbc_driver_postgresql.so"; + + /// Build a staging directory holding the three extracted driver files, + /// with `lib_contents` as the library body. + fn staged_driver(parent: &Path, lib_contents: &[u8]) -> PathBuf { + let staging = tempfile::tempdir_in(parent).expect("staging dir"); + fs::write(staging.path().join(LIB), lib_contents).expect("write lib"); + fs::write(staging.path().join("LICENSE.txt"), b"license").expect("write license"); + fs::write(staging.path().join("NOTICE.txt"), b"notice").expect("write notice"); + staging.keep() + } + + #[test] + fn place_driver_writes_files_and_manifest() { + let root = tempfile::tempdir().expect("root"); + let drivers_dir = root.path().join("drivers"); + fs::create_dir_all(&drivers_dir).expect("drivers dir"); + let staging = staged_driver(&drivers_dir, b"ELF"); + let driver_dir = drivers_dir.join("postgresql"); + + place_driver(&staging, &driver_dir, LIB).expect("place"); + + assert_eq!(fs::read(driver_dir.join(LIB)).expect("lib"), b"ELF"); + assert!(driver_dir.join("LICENSE.txt").exists()); + assert!(driver_dir.join("NOTICE.txt").exists()); + + let manifest = + fs::read_to_string(driver_dir.join("manifest.toml")).expect("manifest exists"); + let parsed: toml::Value = toml::from_str(&manifest).expect("valid TOML"); + assert_eq!( + parsed["Driver"]["shared"].as_str(), + Some(driver_dir.join(LIB).to_string_lossy().as_ref()), + ); + } + + #[test] + fn place_driver_replaces_an_existing_install() { + let root = tempfile::tempdir().expect("root"); + let drivers_dir = root.path().join("drivers"); + fs::create_dir_all(&drivers_dir).expect("drivers dir"); + let driver_dir = drivers_dir.join("postgresql"); + + place_driver(&staged_driver(&drivers_dir, b"old"), &driver_dir, LIB).expect("first"); + // A stray file from the old install must not survive the reinstall. + fs::write(driver_dir.join("stale.txt"), b"x").expect("stray file"); + + place_driver(&staged_driver(&drivers_dir, b"new"), &driver_dir, LIB).expect("second"); + + assert_eq!(fs::read(driver_dir.join(LIB)).expect("lib"), b"new"); + assert!(!driver_dir.join("stale.txt").exists()); + } +} diff --git a/ampup/src/config.rs b/ampup/src/config.rs index 3b0be7d..8421f29 100644 --- a/ampup/src/config.rs +++ b/ampup/src/config.rs @@ -98,6 +98,18 @@ impl Config { self.versions_dir.join(version).join("ampsql") } + /// Get the ADBC drivers directory for a specific version + /// (~/.amp/versions//drivers) + pub fn drivers_dir(&self, version: &str) -> PathBuf { + self.versions_dir.join(version).join("drivers") + } + + /// Get the self-contained directory for a single ADBC driver under a + /// version (~/.amp/versions//drivers/) + pub fn driver_dir(&self, version: &str, driver: &str) -> PathBuf { + self.drivers_dir(version).join(driver) + } + /// Get the active ampsql binary symlink path pub fn active_ampsql_path(&self) -> PathBuf { self.bin_dir.join("ampsql") From 1f3fd430c1a4a4436df6a7820b7508d2cf3efa5c Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Wed, 22 Jul 2026 10:58:07 +0000 Subject: [PATCH 06/14] fix(adbc): reject non-regular archive entries extract_and_validate checked only the entry name, so a symlink or hardlink named as an allowed member would be materialized by unpack as a link instead of the archived bytes. Reject any entry that is not a regular file before unpacking. Part of edgeandnode/amp#2600. --- ampup/src/archive.rs | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/ampup/src/archive.rs b/ampup/src/archive.rs index 97afe3b..f098108 100644 --- a/ampup/src/archive.rs +++ b/ampup/src/archive.rs @@ -46,6 +46,13 @@ pub fn extract_and_validate(data: &[u8], dest: &Path, expected_members: &[&str]) bail!("unexpected member in driver archive: {name}"); } + // Only regular files are expected. A symlink/hardlink entry (even under + // an allowed name) would be materialized by `unpack` as a link rather + // than the archived bytes, so reject anything that isn't a plain file. + if !entry.header().entry_type().is_file() { + bail!("archive member {name} is not a regular file"); + } + entry .unpack(dest.join(&name)) .with_context(|| format!("failed to extract {name}"))?; @@ -141,4 +148,27 @@ mod tests { .expect_err("nested entry should fail"); assert!(err.to_string().contains("flat file name"), "got: {err}"); } + + #[test] + fn rejects_symlink_entry() { + // A symlink named as an allowed member, pointing outside the archive. + let mut builder = Builder::new(GzEncoder::new(Vec::new(), Compression::default())); + let mut header = Header::new_gnu(); + header.set_entry_type(tar::EntryType::Symlink); + header.set_size(0); + header.set_mode(0o644); + builder + .append_link(&mut header, "libadbc_driver_postgresql.so", "/etc/passwd") + .expect("should append symlink entry"); + let archive = builder + .into_inner() + .expect("should finish tar") + .finish() + .expect("should finish gzip"); + let dir = tempfile::tempdir().expect("tempdir"); + + let err = extract_and_validate(&archive, dir.path(), MEMBERS) + .expect_err("symlink member should fail"); + assert!(err.to_string().contains("not a regular file"), "got: {err}"); + } } From e9948adc78bc1d990351ef866dedc181edb9c06e Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Wed, 22 Jul 2026 10:58:07 +0000 Subject: [PATCH 07/14] refactor(adbc): drop unused --jobs flag from adbc install adbc install downloads a single asset directly, so the -j/--jobs flag was accepted and silently ignored. Remove it from the subcommand and the install signature. Part of edgeandnode/amp#2600. --- ampup/src/commands/adbc.rs | 1 - ampup/src/main.rs | 17 ++--------------- 2 files changed, 2 insertions(+), 16 deletions(-) diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index ec089f1..bfca157 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -27,7 +27,6 @@ pub async fn install( github_token: Option, arch: Option, platform: Option, - _jobs: usize, ) -> Result<()> { let driver = parse_driver(driver)?; diff --git a/ampup/src/main.rs b/ampup/src/main.rs index 8fc6a1c..5baad91 100644 --- a/ampup/src/main.rs +++ b/ampup/src/main.rs @@ -209,10 +209,6 @@ enum AdbcCommands { /// Override platform detection (linux, darwin) #[arg(long)] platform: Option, - - /// Number of concurrent downloads - #[arg(short = 'j', long = "jobs", default_value_t = DEFAULT_DOWNLOAD_JOBS)] - jobs: usize, }, /// List installed ADBC drivers for the active version @@ -322,18 +318,9 @@ async fn run() -> anyhow::Result<()> { github_token, arch, platform, - jobs, } => { - commands::adbc::install( - &driver, - install_dir, - repo, - github_token, - arch, - platform, - jobs, - ) - .await?; + commands::adbc::install(&driver, install_dir, repo, github_token, arch, platform) + .await?; } AdbcCommands::List => { commands::adbc::list()?; From c7a6da050f7e3bccd50a1c1c0f179f9204964bdc Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Wed, 22 Jul 2026 11:11:02 +0000 Subject: [PATCH 08/14] feat(adbc): add adbc list and uninstall commands list shows the complete drivers installed for the active amp version (directories that match the catalog and hold a manifest.toml, so leftover staging dirs and partial installs are skipped). uninstall removes a driver's directory and prunes an emptied drivers dir. Part of edgeandnode/amp#2600. --- ampup/src/commands/adbc.rs | 134 +++++++++++++++++++++++++++++++++++-- ampup/src/main.rs | 21 ++++-- 2 files changed, 144 insertions(+), 11 deletions(-) diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index bfca157..389b879 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -110,15 +110,96 @@ fn place_driver(staging: &Path, driver_dir: &Path, lib_name: &str) -> Result<()> Ok(()) } -/// List installed ADBC drivers for the active version. -pub fn list() -> Result<()> { - bail!("`ampup adbc list` is not yet implemented"); +/// List installed ADBC drivers for the active amp version. +pub fn list(install_dir: Option) -> Result<()> { + let config = Config::new(install_dir)?; + let version_manager = VersionManager::new(config); + + let Some(version) = version_manager.get_current()? else { + ui::info!("No active amp version"); + return Ok(()); + }; + + let drivers = installed_drivers(&version_manager.config().drivers_dir(&version))?; + if drivers.is_empty() { + ui::info!( + "No ADBC drivers installed for amp {}", + ui::version(&version) + ); + return Ok(()); + } + + ui::info!("Installed ADBC drivers for amp {}:", ui::version(&version)); + for driver in drivers { + println!(" {driver}"); + } + Ok(()) } -/// Uninstall an ADBC driver. -pub fn uninstall(driver: &str) -> Result<()> { +/// Uninstall an ADBC driver from the active amp version. +pub fn uninstall(install_dir: Option, driver: &str) -> Result<()> { let driver = parse_driver(driver)?; - bail!("`ampup adbc uninstall {driver}` is not yet implemented"); + + let config = Config::new(install_dir)?; + let version_manager = VersionManager::new(config); + + let version = version_manager + .get_current()? + .ok_or_else(|| anyhow!("no active amp version; run `ampup install` first"))?; + + let driver_dir = version_manager + .config() + .driver_dir(&version, driver.as_str()); + if !driver_dir.exists() { + bail!( + "the {driver} driver is not installed for amp {}", + ui::version(&version) + ); + } + + fs::remove_dir_all(&driver_dir).context("failed to remove the driver directory")?; + // Tidy up an otherwise-empty drivers directory; ignore the error when it + // still holds other drivers (or a leftover staging dir). + let _ = fs::remove_dir(version_manager.config().drivers_dir(&version)); + + ui::info!( + "Uninstalled {} driver from amp {}", + driver, + ui::version(&version) + ); + Ok(()) +} + +/// The catalog drivers currently installed under `drivers_dir`. +/// +/// Only complete installs count: an entry must be a directory named after a +/// known driver and hold a `manifest.toml`, which skips leftover staging +/// directories and partial installs. Returns empty when `drivers_dir` is +/// absent (a version with no drivers installed). +fn installed_drivers(drivers_dir: &Path) -> Result> { + if !drivers_dir.exists() { + return Ok(Vec::new()); + } + + let mut drivers = Vec::new(); + for entry in fs::read_dir(drivers_dir).context("failed to read the drivers directory")? { + let entry = entry.context("failed to read a drivers directory entry")?; + if !entry + .file_type() + .context("failed to determine a drivers entry type")? + .is_dir() + { + continue; + } + let Some(driver) = entry.file_name().to_str().and_then(Driver::from_name) else { + continue; + }; + if entry.path().join("manifest.toml").is_file() { + drivers.push(driver); + } + } + drivers.sort_by_key(|d| d.as_str()); + Ok(drivers) } /// Resolve a driver name against the catalog, with a helpful error listing the @@ -213,4 +294,45 @@ mod tests { assert_eq!(fs::read(driver_dir.join(LIB)).expect("lib"), b"new"); assert!(!driver_dir.join("stale.txt").exists()); } + + /// Create `drivers_dir//`, optionally with a `manifest.toml`. + fn make_driver_dir(drivers_dir: &Path, name: &str, with_manifest: bool) { + let dir = drivers_dir.join(name); + fs::create_dir_all(&dir).expect("driver dir"); + if with_manifest { + fs::write(dir.join("manifest.toml"), b"manifest_version = 1").expect("manifest"); + } + } + + #[test] + fn installed_drivers_lists_only_complete_catalog_dirs() { + let root = tempfile::tempdir().expect("root"); + let drivers_dir = root.path().join("drivers"); + make_driver_dir(&drivers_dir, "postgresql", true); // complete -> included + make_driver_dir(&drivers_dir, "mysql", true); // not in catalog -> excluded + make_driver_dir(&drivers_dir, ".tmpABC123", true); // leftover staging -> excluded + fs::write(drivers_dir.join("stray.txt"), b"x").expect("stray file"); // non-dir -> excluded + + assert_eq!( + installed_drivers(&drivers_dir).expect("list"), + vec![Driver::Postgresql], + ); + } + + #[test] + fn installed_drivers_skips_partial_installs() { + let root = tempfile::tempdir().expect("root"); + let drivers_dir = root.path().join("drivers"); + make_driver_dir(&drivers_dir, "postgresql", false); // no manifest -> excluded + + assert!(installed_drivers(&drivers_dir).expect("list").is_empty()); + } + + #[test] + fn installed_drivers_on_missing_dir_is_empty() { + let root = tempfile::tempdir().expect("root"); + let drivers_dir = root.path().join("drivers"); // never created + + assert!(installed_drivers(&drivers_dir).expect("list").is_empty()); + } } diff --git a/ampup/src/main.rs b/ampup/src/main.rs index 5baad91..6fbd8c5 100644 --- a/ampup/src/main.rs +++ b/ampup/src/main.rs @@ -212,10 +212,18 @@ enum AdbcCommands { }, /// List installed ADBC drivers for the active version - List, + List { + /// Installation directory (defaults to $AMP_DIR or $XDG_CONFIG_HOME/.amp or $HOME/.amp) + #[arg(long, env = "AMP_DIR")] + install_dir: Option, + }, /// Uninstall an ADBC driver Uninstall { + /// Installation directory (defaults to $AMP_DIR or $XDG_CONFIG_HOME/.amp or $HOME/.amp) + #[arg(long, env = "AMP_DIR")] + install_dir: Option, + /// Driver to uninstall (e.g. postgresql) driver: String, }, @@ -322,11 +330,14 @@ async fn run() -> anyhow::Result<()> { commands::adbc::install(&driver, install_dir, repo, github_token, arch, platform) .await?; } - AdbcCommands::List => { - commands::adbc::list()?; + AdbcCommands::List { install_dir } => { + commands::adbc::list(install_dir)?; } - AdbcCommands::Uninstall { driver } => { - commands::adbc::uninstall(&driver)?; + AdbcCommands::Uninstall { + install_dir, + driver, + } => { + commands::adbc::uninstall(install_dir, &driver)?; } }, None => { From 8fea67a399c1b7291cdf2914d68611790274bbb6 Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Wed, 22 Jul 2026 11:30:50 +0000 Subject: [PATCH 09/14] fix(adbc): retry driver download on transient failure The driver download called download_resolved_asset directly, so it lacked the single application-level retry that binary downloads get via download_with_retry. Reuse that wrapper (now pub(crate)) so a driver fetch survives a transient failure the same way binaries do. Part of edgeandnode/amp#2600. --- ampup/src/commands/adbc.rs | 4 ++-- ampup/src/download_manager.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index 389b879..c70c047 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -12,7 +12,7 @@ use crate::{ adbc::Driver, archive, config::Config, - download_manager::verify_artifact, + download_manager::{download_with_retry, verify_artifact}, github::GitHubClient, platform::{Architecture, Platform, PlatformError}, token, ui, @@ -55,7 +55,7 @@ pub async fn install( .resolve(&asset_name, false)? .expect("a required asset resolves to Some or errors"); - let data = github.download_resolved_asset(&asset).await?; + let data = download_with_retry(&github, &asset).await?; verify_artifact(&asset.name, &data, asset.digest.as_deref()) .context("driver archive failed verification")?; diff --git a/ampup/src/download_manager.rs b/ampup/src/download_manager.rs index 9940f06..2382d44 100644 --- a/ampup/src/download_manager.rs +++ b/ampup/src/download_manager.rs @@ -374,7 +374,7 @@ fn append_extension(path: &Path, ext: &str) -> PathBuf { /// already handled at the HTTP layer by `GitHubClient::send_with_rate_limit`, /// so a rate-limited request will have been retried there before surfacing /// as an error here. -async fn download_with_retry( +pub(crate) async fn download_with_retry( github: &GitHubClient, asset: &ResolvedAsset, ) -> std::result::Result, DownloadError> { From 318300d58c143885ed4652853d5c78fcc2b85509 Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Wed, 22 Jul 2026 14:51:59 +0000 Subject: [PATCH 10/14] test(adbc): cover install and uninstall end-to-end Split install into a thin wrapper plus install_driver so a test can inject a GitHubClient pointed at a mock server. Add an in-process mock GitHub server and integration tests: install drives the full fetch -> verify -> extract -> place -> manifest path against the mock, and uninstall removes a placed driver and prunes the emptied drivers dir. Part of edgeandnode/amp#2600. --- ampup/src/commands/adbc.rs | 19 ++++-- ampup/src/tests/it_adbc.rs | 120 +++++++++++++++++++++++++++++++++ ampup/src/tests/mock_github.rs | 107 +++++++++++++++++++++++++++++ ampup/src/tests/mod.rs | 2 + 4 files changed, 244 insertions(+), 4 deletions(-) create mode 100644 ampup/src/tests/it_adbc.rs create mode 100644 ampup/src/tests/mock_github.rs diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index c70c047..93f57e6 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -29,10 +29,21 @@ pub async fn install( platform: Option, ) -> Result<()> { let driver = parse_driver(driver)?; - let config = Config::new(install_dir)?; - let token = token::resolve_github_token(github_token); - let github = GitHubClient::new(repo, token)?; + let github = GitHubClient::new(repo, token::resolve_github_token(github_token))?; + install_driver(&github, config, driver, arch, platform).await +} + +/// Fetch, verify, extract, and place `driver` for the active amp version using +/// an already-constructed GitHub client. Split from [`install`] so tests can +/// inject a client pointed at a mock server. +pub(crate) async fn install_driver( + github: &GitHubClient, + config: Config, + driver: Driver, + arch: Option, + platform: Option, +) -> Result<()> { let version_manager = VersionManager::new(config); // Drivers are pinned to an installed amp version, so one must be active. @@ -55,7 +66,7 @@ pub async fn install( .resolve(&asset_name, false)? .expect("a required asset resolves to Some or errors"); - let data = download_with_retry(&github, &asset).await?; + let data = download_with_retry(github, &asset).await?; verify_artifact(&asset.name, &data, asset.digest.as_deref()) .context("driver archive failed verification")?; diff --git a/ampup/src/tests/it_adbc.rs b/ampup/src/tests/it_adbc.rs new file mode 100644 index 0000000..378f9a3 --- /dev/null +++ b/ampup/src/tests/it_adbc.rs @@ -0,0 +1,120 @@ +//! Integration tests for the `ampup adbc` commands (#2600). +//! +//! `adbc_install_places_driver_and_manifest` drives the whole install path +//! (fetch release metadata -> download -> verify digest -> extract -> place + +//! manifest) against an in-process mock GitHub server, so no network is +//! required. The uninstall test exercises the command against a placed driver. + +use flate2::{Compression, write::GzEncoder}; +use fs_err as fs; +use sha2::{Digest, Sha256}; +use tar::{Builder, Header}; + +use crate::{ + adbc::Driver, + commands::adbc, + config::Config, + github::GitHubClient, + tests::{fixtures::TempInstallDir, mock_github}, +}; + +const LIB: &str = "libadbc_driver_postgresql.so"; +const ASSET: &str = "adbc-driver-postgresql-linux-x86_64.tar.gz"; + +/// Build a gzip-compressed tar of `(name, contents)` entries. +fn make_targz(files: &[(&str, &[u8])]) -> Vec { + let mut builder = Builder::new(GzEncoder::new(Vec::new(), Compression::default())); + for (name, data) in files { + let mut header = Header::new_gnu(); + header.set_size(data.len() as u64); + header.set_mode(0o644); + header.set_cksum(); + builder + .append_data(&mut header, name, *data) + .expect("should append tar entry"); + } + builder + .into_inner() + .expect("should finish tar") + .finish() + .expect("should finish gzip") +} + +#[tokio::test] +async fn adbc_install_places_driver_and_manifest() { + let temp = TempInstallDir::new().expect("temp install dir"); + let version = "v1.0.0"; + fs::write(temp.current_version_file(), version).expect("write active version"); + + // The release asset the installer will fetch, plus its advertised digest. + let tarball = make_targz(&[ + (LIB, b"ELF"), + ("LICENSE.txt", b"license"), + ("NOTICE.txt", b"notice"), + ]); + let digest = format!("sha256:{:x}", Sha256::digest(&tarball)); + + // Mock GitHub: release metadata for the tag, plus the asset download. + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind mock server"); + let addr = listener.local_addr().expect("mock server address"); + let release = mock_github::release_json(addr, version, &[(ASSET, Some(&digest))]); + let routes = vec![ + mock_github::Route::ok(format!("tags/{version}"), release), + mock_github::Route::ok("download/", tarball), + ]; + let _server = mock_github::start(listener, routes); + + let github = GitHubClient::with_api_base(format!("http://{addr}")).expect("mock client"); + let config = Config::new(Some(temp.path().to_path_buf())).expect("config"); + + adbc::install_driver( + &github, + config, + Driver::Postgresql, + Some("x86_64".to_string()), + Some("linux".to_string()), + ) + .await + .expect("install should succeed"); + + let driver_dir = temp + .versions_dir() + .join(version) + .join("drivers") + .join("postgresql"); + assert_eq!(fs::read(driver_dir.join(LIB)).expect("lib"), b"ELF"); + assert!(driver_dir.join("LICENSE.txt").exists(), "LICENSE placed"); + assert!(driver_dir.join("NOTICE.txt").exists(), "NOTICE placed"); + + let manifest = fs::read_to_string(driver_dir.join("manifest.toml")).expect("manifest exists"); + let parsed: toml::Value = toml::from_str(&manifest).expect("manifest is valid TOML"); + assert_eq!( + parsed["Driver"]["shared"].as_str(), + Some(driver_dir.join(LIB).to_string_lossy().as_ref()), + "Driver.shared points at the placed library", + ); +} + +#[test] +fn adbc_uninstall_removes_installed_driver() { + let temp = TempInstallDir::new().expect("temp install dir"); + let version = "v1.0.0"; + fs::write(temp.current_version_file(), version).expect("write active version"); + + let drivers_dir = temp.versions_dir().join(version).join("drivers"); + let driver_dir = drivers_dir.join("postgresql"); + fs::create_dir_all(&driver_dir).expect("driver dir"); + fs::write(driver_dir.join(LIB), b"ELF").expect("lib"); + fs::write(driver_dir.join("manifest.toml"), b"manifest_version = 1").expect("manifest"); + + adbc::uninstall(Some(temp.path().to_path_buf()), "postgresql") + .expect("uninstall should succeed"); + + assert!(!driver_dir.exists(), "driver directory removed"); + assert!( + !drivers_dir.exists(), + "emptied drivers directory pruned after removing the last driver", + ); +} diff --git a/ampup/src/tests/mock_github.rs b/ampup/src/tests/mock_github.rs new file mode 100644 index 0000000..1bd8993 --- /dev/null +++ b/ampup/src/tests/mock_github.rs @@ -0,0 +1,107 @@ +//! A minimal in-process HTTP mock for the GitHub release API, used by +//! integration tests that drive the download path without a network. +//! +//! Bind a listener, describe the routes, and point a +//! [`GitHubClient`](crate::github::GitHubClient) at the returned base URL via +//! `with_api_base`. Requests are matched by a path substring. + +use tokio::io::{AsyncReadExt, AsyncWriteExt}; + +/// A single mock route: any request whose path contains `prefix` receives +/// `body` with a 200 response. +#[derive(Clone)] +pub(crate) struct Route { + pub prefix: String, + pub body: Vec, +} + +impl Route { + pub fn ok(prefix: impl Into, body: Vec) -> Self { + Self { + prefix: prefix.into(), + body, + } + } +} + +/// Build the release-metadata JSON GitHub returns for a tag, advertising each +/// asset's download URL (served by the same mock) and optional digest. +pub(crate) fn release_json( + addr: std::net::SocketAddr, + tag: &str, + assets: &[(&str, Option<&str>)], +) -> Vec { + let assets: Vec = assets + .iter() + .enumerate() + .map(|(i, (name, digest))| { + let digest_field = match digest { + Some(d) => format!(r#","digest":"{d}""#), + None => String::new(), + }; + format!( + r#"{{"id":{},"name":"{}","browser_download_url":"http://{}/download/{}"{}}}"#, + i + 1, + name, + addr, + name, + digest_field, + ) + }) + .collect(); + format!( + r#"{{"tag_name":"{}","assets":[{}]}}"#, + tag, + assets.join(",") + ) + .into_bytes() +} + +/// Spawn the mock server on a pre-bound listener. It reads each request's path, +/// returns the first matching route's body with 200, or 404 if none match. +pub(crate) fn start( + listener: tokio::net::TcpListener, + routes: Vec, +) -> tokio::task::JoinHandle<()> { + tokio::spawn(async move { + loop { + let Ok((mut stream, _)) = listener.accept().await else { + break; + }; + let routes = routes.clone(); + + tokio::spawn(async move { + let mut buf = [0u8; 4096]; + let n = stream.read(&mut buf).await.expect("should read request"); + let request = String::from_utf8_lossy(&buf[..n]); + let path = request + .lines() + .next() + .and_then(|line| line.split_whitespace().nth(1)) + .unwrap_or("/"); + + let response = routes + .iter() + .find(|r| path.contains(r.prefix.as_str())) + .map(|route| { + format!( + "HTTP/1.1 200 OK\r\nContent-Length: {}\r\n\r\n", + route.body.len() + ) + .into_bytes() + .into_iter() + .chain(route.body.iter().copied()) + .collect::>() + }) + .unwrap_or_else(|| { + b"HTTP/1.1 404 Not Found\r\nContent-Length: 0\r\n\r\n".to_vec() + }); + + stream + .write_all(&response) + .await + .expect("should write response"); + }); + } + }) +} diff --git a/ampup/src/tests/mod.rs b/ampup/src/tests/mod.rs index f43f76c..81cbaa5 100644 --- a/ampup/src/tests/mod.rs +++ b/ampup/src/tests/mod.rs @@ -1,2 +1,4 @@ mod fixtures; +mod it_adbc; mod it_ampup; +mod mock_github; From 656928256cb9c055f6896fd0ce6793c00ee6fbd2 Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Thu, 23 Jul 2026 05:30:59 +0000 Subject: [PATCH 11/14] fix(adbc): require a digest for driver assets verify_artifact only checks that the download is non-empty when no digest is advertised. Driver assets always carry one, so treat a missing digest as a malformed release and refuse rather than install a library that ampd would load unverified. Part of edgeandnode/amp#2600. --- ampup/src/commands/adbc.rs | 11 +++++++++- ampup/src/tests/it_adbc.rs | 44 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index 93f57e6..4d5e713 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -67,7 +67,16 @@ pub(crate) async fn install_driver( .expect("a required asset resolves to Some or errors"); let data = download_with_retry(github, &asset).await?; - verify_artifact(&asset.name, &data, asset.digest.as_deref()) + // Driver assets always advertise a digest, so a missing one means the + // release is malformed. Refuse rather than install a library that would be + // loaded into ampd without its integrity checked. + let digest = asset.digest.as_deref().ok_or_else(|| { + anyhow!( + "release asset {} has no digest; refusing to install an unverified driver", + asset.name + ) + })?; + verify_artifact(&asset.name, &data, Some(digest)) .context("driver archive failed verification")?; // Stage inside the drivers directory so the final rename stays on one diff --git a/ampup/src/tests/it_adbc.rs b/ampup/src/tests/it_adbc.rs index 378f9a3..ee48f00 100644 --- a/ampup/src/tests/it_adbc.rs +++ b/ampup/src/tests/it_adbc.rs @@ -97,6 +97,50 @@ async fn adbc_install_places_driver_and_manifest() { ); } +#[tokio::test] +async fn adbc_install_rejects_asset_without_digest() { + let temp = TempInstallDir::new().expect("temp install dir"); + let version = "v1.0.0"; + fs::write(temp.current_version_file(), version).expect("write active version"); + + let tarball = make_targz(&[ + (LIB, b"ELF"), + ("LICENSE.txt", b"license"), + ("NOTICE.txt", b"notice"), + ]); + + // Same release, except the asset advertises no digest. + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind mock server"); + let addr = listener.local_addr().expect("mock server address"); + let release = mock_github::release_json(addr, version, &[(ASSET, None)]); + let routes = vec![ + mock_github::Route::ok(format!("tags/{version}"), release), + mock_github::Route::ok("download/", tarball), + ]; + let _server = mock_github::start(listener, routes); + + let github = GitHubClient::with_api_base(format!("http://{addr}")).expect("mock client"); + let config = Config::new(Some(temp.path().to_path_buf())).expect("config"); + + let err = adbc::install_driver( + &github, + config, + Driver::Postgresql, + Some("x86_64".to_string()), + Some("linux".to_string()), + ) + .await + .expect_err("install should refuse an asset without a digest"); + assert!(err.to_string().contains("no digest"), "got: {err}"); + + assert!( + !temp.versions_dir().join(version).join("drivers").exists(), + "nothing should be installed when the digest is missing", + ); +} + #[test] fn adbc_uninstall_removes_installed_driver() { let temp = TempInstallDir::new().expect("temp install dir"); From eb9911c2bf275e374a83b08e28dc50ff82d2954b Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Thu, 23 Jul 2026 06:16:30 +0000 Subject: [PATCH 12/14] feat(adbc): add --version and require the version to be installed The adbc commands were pinned to the active version, unlike `ampup install` which takes one. Add --version to install, list, and uninstall. Resolving the version now also requires it to be installed. Installing amp replaces the whole version directory, so drivers placed under a version whose binaries are not there yet would be destroyed by the next `ampup install`. Part of edgeandnode/amp#2600. --- ampup/src/commands/adbc.rs | 62 ++++++++++++++++++++++++++------------ ampup/src/main.rs | 41 ++++++++++++++++++++----- ampup/src/tests/it_adbc.rs | 43 ++++++++++++++++++++++++-- 3 files changed, 117 insertions(+), 29 deletions(-) diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index 4d5e713..5ecc266 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -19,7 +19,7 @@ use crate::{ version_manager::VersionManager, }; -/// Install an ADBC driver for the active amp version. +/// Install an ADBC driver for an amp version (the active one by default). pub async fn install( driver: &str, install_dir: Option, @@ -27,15 +27,16 @@ pub async fn install( github_token: Option, arch: Option, platform: Option, + version: Option, ) -> Result<()> { let driver = parse_driver(driver)?; let config = Config::new(install_dir)?; let github = GitHubClient::new(repo, token::resolve_github_token(github_token))?; - install_driver(&github, config, driver, arch, platform).await + install_driver(&github, config, driver, arch, platform, version).await } -/// Fetch, verify, extract, and place `driver` for the active amp version using -/// an already-constructed GitHub client. Split from [`install`] so tests can +/// Fetch, verify, extract, and place `driver` for an amp version using an +/// already-constructed GitHub client. Split from [`install`] so tests can /// inject a client pointed at a mock server. pub(crate) async fn install_driver( github: &GitHubClient, @@ -43,13 +44,10 @@ pub(crate) async fn install_driver( driver: Driver, arch: Option, platform: Option, + version: Option, ) -> Result<()> { let version_manager = VersionManager::new(config); - - // Drivers are pinned to an installed amp version, so one must be active. - let version = version_manager - .get_current()? - .ok_or_else(|| anyhow!("no active amp version; run `ampup install` first"))?; + let version = resolve_version(&version_manager, version)?; let platform = resolve_platform(platform)?; let arch = resolve_arch(arch)?; @@ -130,15 +128,18 @@ fn place_driver(staging: &Path, driver_dir: &Path, lib_name: &str) -> Result<()> Ok(()) } -/// List installed ADBC drivers for the active amp version. -pub fn list(install_dir: Option) -> Result<()> { +/// List installed ADBC drivers for an amp version (the active one by default). +pub fn list(install_dir: Option, version: Option) -> Result<()> { let config = Config::new(install_dir)?; let version_manager = VersionManager::new(config); - let Some(version) = version_manager.get_current()? else { + // Without an explicit version, having none active is an empty state rather + // than an error. + if version.is_none() && version_manager.get_current()?.is_none() { ui::info!("No active amp version"); return Ok(()); - }; + } + let version = resolve_version(&version_manager, version)?; let drivers = installed_drivers(&version_manager.config().drivers_dir(&version))?; if drivers.is_empty() { @@ -156,16 +157,17 @@ pub fn list(install_dir: Option) -> Result<()> { Ok(()) } -/// Uninstall an ADBC driver from the active amp version. -pub fn uninstall(install_dir: Option, driver: &str) -> Result<()> { +/// Uninstall an ADBC driver from an amp version (the active one by default). +pub fn uninstall( + install_dir: Option, + driver: &str, + version: Option, +) -> Result<()> { let driver = parse_driver(driver)?; let config = Config::new(install_dir)?; let version_manager = VersionManager::new(config); - - let version = version_manager - .get_current()? - .ok_or_else(|| anyhow!("no active amp version; run `ampup install` first"))?; + let version = resolve_version(&version_manager, version)?; let driver_dir = version_manager .config() @@ -190,6 +192,28 @@ pub fn uninstall(install_dir: Option, driver: &str) -> Result<()> { Ok(()) } +/// Resolve the amp version to operate on: an explicit one, or the active one. +/// +/// The version must already be installed. Installing amp replaces the whole +/// version directory, so drivers placed under a version whose binaries are not +/// there yet would be destroyed by the next `ampup install`. +fn resolve_version(version_manager: &VersionManager, version: Option) -> Result { + let version = match version { + Some(version) => version, + None => version_manager + .get_current()? + .ok_or_else(|| anyhow!("no active amp version; run `ampup install` first"))?, + }; + + if !version_manager.is_installed(&version) { + bail!( + "amp {} is not installed; run `ampup install {version}` first", + ui::version(&version), + ); + } + Ok(version) +} + /// The catalog drivers currently installed under `drivers_dir`. /// /// Only complete installs count: an entry must be a directory named after a diff --git a/ampup/src/main.rs b/ampup/src/main.rs index 6fbd8c5..15cd10e 100644 --- a/ampup/src/main.rs +++ b/ampup/src/main.rs @@ -185,7 +185,7 @@ enum SelfCommands { #[derive(Debug, clap::Subcommand)] enum AdbcCommands { - /// Install an ADBC driver for the active amp version + /// Install an ADBC driver for an amp version Install { /// Installation directory (defaults to $AMP_DIR or $XDG_CONFIG_HOME/.amp or $HOME/.amp) #[arg(long, env = "AMP_DIR")] @@ -209,16 +209,24 @@ enum AdbcCommands { /// Override platform detection (linux, darwin) #[arg(long)] platform: Option, + + /// Amp version to install the driver for (defaults to the active one) + #[arg(long = "version")] + amp_version: Option, }, - /// List installed ADBC drivers for the active version + /// List installed ADBC drivers for an amp version List { /// Installation directory (defaults to $AMP_DIR or $XDG_CONFIG_HOME/.amp or $HOME/.amp) #[arg(long, env = "AMP_DIR")] install_dir: Option, + + /// Amp version to list drivers for (defaults to the active one) + #[arg(long = "version")] + amp_version: Option, }, - /// Uninstall an ADBC driver + /// Uninstall an ADBC driver from an amp version Uninstall { /// Installation directory (defaults to $AMP_DIR or $XDG_CONFIG_HOME/.amp or $HOME/.amp) #[arg(long, env = "AMP_DIR")] @@ -226,6 +234,10 @@ enum AdbcCommands { /// Driver to uninstall (e.g. postgresql) driver: String, + + /// Amp version to uninstall the driver from (defaults to the active one) + #[arg(long = "version")] + amp_version: Option, }, } @@ -326,18 +338,31 @@ async fn run() -> anyhow::Result<()> { github_token, arch, platform, + amp_version, } => { - commands::adbc::install(&driver, install_dir, repo, github_token, arch, platform) - .await?; + commands::adbc::install( + &driver, + install_dir, + repo, + github_token, + arch, + platform, + amp_version, + ) + .await?; } - AdbcCommands::List { install_dir } => { - commands::adbc::list(install_dir)?; + AdbcCommands::List { + install_dir, + amp_version, + } => { + commands::adbc::list(install_dir, amp_version)?; } AdbcCommands::Uninstall { install_dir, driver, + amp_version, } => { - commands::adbc::uninstall(install_dir, &driver)?; + commands::adbc::uninstall(install_dir, &driver, amp_version)?; } }, None => { diff --git a/ampup/src/tests/it_adbc.rs b/ampup/src/tests/it_adbc.rs index ee48f00..b581281 100644 --- a/ampup/src/tests/it_adbc.rs +++ b/ampup/src/tests/it_adbc.rs @@ -15,7 +15,10 @@ use crate::{ commands::adbc, config::Config, github::GitHubClient, - tests::{fixtures::TempInstallDir, mock_github}, + tests::{ + fixtures::{MockBinary, TempInstallDir}, + mock_github, + }, }; const LIB: &str = "libadbc_driver_postgresql.so"; @@ -45,6 +48,7 @@ async fn adbc_install_places_driver_and_manifest() { let temp = TempInstallDir::new().expect("temp install dir"); let version = "v1.0.0"; fs::write(temp.current_version_file(), version).expect("write active version"); + MockBinary::create(&temp, version).expect("install version binaries"); // The release asset the installer will fetch, plus its advertised digest. let tarball = make_targz(&[ @@ -75,6 +79,7 @@ async fn adbc_install_places_driver_and_manifest() { Driver::Postgresql, Some("x86_64".to_string()), Some("linux".to_string()), + None, ) .await .expect("install should succeed"); @@ -102,6 +107,7 @@ async fn adbc_install_rejects_asset_without_digest() { let temp = TempInstallDir::new().expect("temp install dir"); let version = "v1.0.0"; fs::write(temp.current_version_file(), version).expect("write active version"); + MockBinary::create(&temp, version).expect("install version binaries"); let tarball = make_targz(&[ (LIB, b"ELF"), @@ -130,6 +136,7 @@ async fn adbc_install_rejects_asset_without_digest() { Driver::Postgresql, Some("x86_64".to_string()), Some("linux".to_string()), + None, ) .await .expect_err("install should refuse an asset without a digest"); @@ -141,11 +148,43 @@ async fn adbc_install_rejects_asset_without_digest() { ); } +#[tokio::test] +async fn adbc_install_refuses_a_version_that_is_not_installed() { + let temp = TempInstallDir::new().expect("temp install dir"); + // v1.0.0 is active and installed; v2.0.0 has no binaries. + fs::write(temp.current_version_file(), "v1.0.0").expect("write active version"); + MockBinary::create(&temp, "v1.0.0").expect("install version binaries"); + + // No mock server: resolving the version fails before anything is fetched. + let github = GitHubClient::with_api_base("http://127.0.0.1:1".to_string()).expect("client"); + let config = Config::new(Some(temp.path().to_path_buf())).expect("config"); + + let err = adbc::install_driver( + &github, + config, + Driver::Postgresql, + Some("x86_64".to_string()), + Some("linux".to_string()), + Some("v2.0.0".to_string()), + ) + .await + .expect_err("install should refuse a version that is not installed"); + assert!(err.to_string().contains("is not installed"), "got: {err}"); + + // Installing amp replaces the whole version directory, so drivers must not + // be placed under a version whose binaries are not there yet. + assert!( + !temp.versions_dir().join("v2.0.0").exists(), + "no version directory should be created for an uninstalled version", + ); +} + #[test] fn adbc_uninstall_removes_installed_driver() { let temp = TempInstallDir::new().expect("temp install dir"); let version = "v1.0.0"; fs::write(temp.current_version_file(), version).expect("write active version"); + MockBinary::create(&temp, version).expect("install version binaries"); let drivers_dir = temp.versions_dir().join(version).join("drivers"); let driver_dir = drivers_dir.join("postgresql"); @@ -153,7 +192,7 @@ fn adbc_uninstall_removes_installed_driver() { fs::write(driver_dir.join(LIB), b"ELF").expect("lib"); fs::write(driver_dir.join("manifest.toml"), b"manifest_version = 1").expect("manifest"); - adbc::uninstall(Some(temp.path().to_path_buf()), "postgresql") + adbc::uninstall(Some(temp.path().to_path_buf()), "postgresql", None) .expect("uninstall should succeed"); assert!(!driver_dir.exists(), "driver directory removed"); From 2c237fc59df11961f697e97068dcf0acae6870ec Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Thu, 23 Jul 2026 07:20:05 +0000 Subject: [PATCH 13/14] fix(adbc): only gate installation on the version being installed Requiring installed binaries for list and uninstall left an orphaned driver directory impossible to inspect or remove, and made a read-only query fail on a stale .version. Split the check out of version resolution so it guards installation only, and reuse VersionError so the message matches the rest of the CLI. Also name the flag's value VERSION in help, and cover installing for an explicit non-active version plus uninstalling from a version whose binaries are gone. Part of edgeandnode/amp#2600. --- ampup/src/commands/adbc.rs | 54 +++++++++++++++---------- ampup/src/main.rs | 6 +-- ampup/src/tests/it_adbc.rs | 81 +++++++++++++++++++++++++++++++++++++- 3 files changed, 117 insertions(+), 24 deletions(-) diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index 5ecc266..db794ba 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -16,7 +16,7 @@ use crate::{ github::GitHubClient, platform::{Architecture, Platform, PlatformError}, token, ui, - version_manager::VersionManager, + version_manager::{VersionError, VersionManager}, }; /// Install an ADBC driver for an amp version (the active one by default). @@ -48,6 +48,7 @@ pub(crate) async fn install_driver( ) -> Result<()> { let version_manager = VersionManager::new(config); let version = resolve_version(&version_manager, version)?; + ensure_installed(&version_manager, &version)?; let platform = resolve_platform(platform)?; let arch = resolve_arch(arch)?; @@ -135,11 +136,16 @@ pub fn list(install_dir: Option, version: Option) -> Result<()> // Without an explicit version, having none active is an empty state rather // than an error. - if version.is_none() && version_manager.get_current()?.is_none() { - ui::info!("No active amp version"); - return Ok(()); - } - let version = resolve_version(&version_manager, version)?; + let version = match version { + Some(version) => version, + None => match version_manager.get_current()? { + Some(version) => version, + None => { + ui::info!("No active amp version"); + return Ok(()); + } + }, + }; let drivers = installed_drivers(&version_manager.config().drivers_dir(&version))?; if drivers.is_empty() { @@ -193,25 +199,33 @@ pub fn uninstall( } /// Resolve the amp version to operate on: an explicit one, or the active one. -/// -/// The version must already be installed. Installing amp replaces the whole -/// version directory, so drivers placed under a version whose binaries are not -/// there yet would be destroyed by the next `ampup install`. fn resolve_version(version_manager: &VersionManager, version: Option) -> Result { - let version = match version { - Some(version) => version, + match version { + Some(version) => Ok(version), None => version_manager .get_current()? - .ok_or_else(|| anyhow!("no active amp version; run `ampup install` first"))?, - }; + .ok_or_else(|| anyhow!("no active amp version; run `ampup install` first")), + } +} - if !version_manager.is_installed(&version) { - bail!( - "amp {} is not installed; run `ampup install {version}` first", - ui::version(&version), - ); +/// Require `version`'s binaries before placing drivers under it. +/// +/// `ampup install ` skips all work when ``'s binaries are already there; +/// otherwise it replaces the whole version directory, taking any `drivers/` +/// subdirectory with it. Installing only into versions that are past that +/// short-circuit keeps drivers from being destroyed by a later install. +/// +/// Only installation is gated: listing and uninstalling must still work on a +/// version whose binaries have gone missing, or an orphaned driver directory +/// could never be inspected or removed. +fn ensure_installed(version_manager: &VersionManager, version: &str) -> Result<()> { + if !version_manager.is_installed(version) { + return Err(VersionError::NotInstalled { + version: version.to_string(), + } + .into()); } - Ok(version) + Ok(()) } /// The catalog drivers currently installed under `drivers_dir`. diff --git a/ampup/src/main.rs b/ampup/src/main.rs index 15cd10e..bf6de88 100644 --- a/ampup/src/main.rs +++ b/ampup/src/main.rs @@ -211,7 +211,7 @@ enum AdbcCommands { platform: Option, /// Amp version to install the driver for (defaults to the active one) - #[arg(long = "version")] + #[arg(long = "version", value_name = "VERSION")] amp_version: Option, }, @@ -222,7 +222,7 @@ enum AdbcCommands { install_dir: Option, /// Amp version to list drivers for (defaults to the active one) - #[arg(long = "version")] + #[arg(long = "version", value_name = "VERSION")] amp_version: Option, }, @@ -236,7 +236,7 @@ enum AdbcCommands { driver: String, /// Amp version to uninstall the driver from (defaults to the active one) - #[arg(long = "version")] + #[arg(long = "version", value_name = "VERSION")] amp_version: Option, }, } diff --git a/ampup/src/tests/it_adbc.rs b/ampup/src/tests/it_adbc.rs index b581281..f19230f 100644 --- a/ampup/src/tests/it_adbc.rs +++ b/ampup/src/tests/it_adbc.rs @@ -169,7 +169,7 @@ async fn adbc_install_refuses_a_version_that_is_not_installed() { ) .await .expect_err("install should refuse a version that is not installed"); - assert!(err.to_string().contains("is not installed"), "got: {err}"); + assert!(err.to_string().contains("not installed"), "got: {err}"); // Installing amp replaces the whole version directory, so drivers must not // be placed under a version whose binaries are not there yet. @@ -179,6 +179,85 @@ async fn adbc_install_refuses_a_version_that_is_not_installed() { ); } +#[tokio::test] +async fn adbc_install_targets_an_explicit_version() { + let temp = TempInstallDir::new().expect("temp install dir"); + // Active version differs from the one being targeted. + fs::write(temp.current_version_file(), "v1.0.0").expect("write active version"); + MockBinary::create(&temp, "v1.0.0").expect("install active version binaries"); + let target = "v2.0.0"; + MockBinary::create(&temp, target).expect("install target version binaries"); + + let tarball = make_targz(&[ + (LIB, b"ELF"), + ("LICENSE.txt", b"license"), + ("NOTICE.txt", b"notice"), + ]); + let digest = format!("sha256:{:x}", Sha256::digest(&tarball)); + + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind mock server"); + let addr = listener.local_addr().expect("mock server address"); + let release = mock_github::release_json(addr, target, &[(ASSET, Some(&digest))]); + let routes = vec![ + mock_github::Route::ok(format!("tags/{target}"), release), + mock_github::Route::ok("download/", tarball), + ]; + let _server = mock_github::start(listener, routes); + + let github = GitHubClient::with_api_base(format!("http://{addr}")).expect("mock client"); + let config = Config::new(Some(temp.path().to_path_buf())).expect("config"); + + adbc::install_driver( + &github, + config, + Driver::Postgresql, + Some("x86_64".to_string()), + Some("linux".to_string()), + Some(target.to_string()), + ) + .await + .expect("install should succeed for an explicit version"); + + // The driver lands under the requested version, not the active one. + assert!( + temp.versions_dir() + .join(target) + .join("drivers") + .join("postgresql") + .join("manifest.toml") + .exists(), + "driver installed under the requested version", + ); + assert!( + !temp.versions_dir().join("v1.0.0").join("drivers").exists(), + "the active version should be untouched", + ); +} + +#[test] +fn adbc_uninstall_works_for_a_version_without_binaries() { + let temp = TempInstallDir::new().expect("temp install dir"); + let version = "v1.0.0"; + fs::write(temp.current_version_file(), version).expect("write active version"); + + // An orphaned driver directory: binaries are gone, drivers remain. Cleanup + // must still work, otherwise it could never be removed with ampup. + let driver_dir = temp + .versions_dir() + .join(version) + .join("drivers") + .join("postgresql"); + fs::create_dir_all(&driver_dir).expect("driver dir"); + fs::write(driver_dir.join("manifest.toml"), b"manifest_version = 1").expect("manifest"); + + adbc::uninstall(Some(temp.path().to_path_buf()), "postgresql", None) + .expect("uninstall should work without the version's binaries"); + + assert!(!driver_dir.exists(), "orphaned driver directory removed"); +} + #[test] fn adbc_uninstall_removes_installed_driver() { let temp = TempInstallDir::new().expect("temp install dir"); From 5403be6c719f296f772bd659f64747bb6197891a Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Thu, 23 Jul 2026 07:20:05 +0000 Subject: [PATCH 14/14] test: serialize the tests that swap PATH build_from_local_path_with_custom_name and rebuild_removes_stale_optional_binaries both replace the process-global PATH to install a mock cargo. Run concurrently, one restores the original value while the other is mid-build, so the mock disappears and the real cargo fails against the fake repo. Share a mutex so they cannot overlap. --- ampup/src/tests/it_ampup.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ampup/src/tests/it_ampup.rs b/ampup/src/tests/it_ampup.rs index 304e519..fa13b29 100644 --- a/ampup/src/tests/it_ampup.rs +++ b/ampup/src/tests/it_ampup.rs @@ -7,6 +7,11 @@ use tempfile::TempDir; use super::fixtures::{MockBinary, TempInstallDir}; use crate::{DEFAULT_DOWNLOAD_JOBS, DEFAULT_REPO}; +/// Serializes the tests that swap `PATH` to install a mock `cargo`. `PATH` is +/// process-global, so two of them running at once restore each other's value +/// mid-build and the mock binary disappears. +static PATH_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); + #[tokio::test] async fn init_creates_directory_structure() -> Result<()> { let temp = TempInstallDir::new()?; @@ -317,6 +322,7 @@ async fn build_from_local_path_with_custom_name() -> Result<()> { } // Temporarily modify PATH to use mock cargo + let _path_guard = PATH_LOCK.lock().await; let original_path = env::var("PATH").unwrap_or_default(); let new_path = format!("{}:{}", mock_cargo_dir.path().display(), original_path); unsafe { @@ -393,6 +399,7 @@ async fn rebuild_removes_stale_optional_binaries() -> Result<()> { write_executable(&mock_cargo, "#!/bin/sh\nexit 0")?; // Temporarily modify PATH to use mock cargo. + let _path_guard = PATH_LOCK.lock().await; let original_path = env::var("PATH").unwrap_or_default(); let new_path = format!("{}:{}", mock_cargo_dir.path().display(), original_path); unsafe {