diff --git a/Cargo.lock b/Cargo.lock index 5b13cd6..27d9a6e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1745,6 +1745,10 @@ dependencies = [ "frameshift-growth", "frameshift-orchestrator", "frameshift-source", + "frameshift-vault", + "frameshift-vault-local", + "rpassword", + "secrecy", "serde_json", "tempfile", "thiserror 2.0.18", @@ -1766,8 +1770,12 @@ dependencies = [ "frameshift-conformance", "frameshift-pack", "frameshift-source", + "frameshift-template", + "frameshift-vault", + "frameshift-vault-local", "hex", "rand_core 0.6.4", + "secrecy", "serde", "serde_json", "sha2 0.10.9", @@ -4388,6 +4396,27 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "rpassword" +version = "7.5.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2da316a15f47e3d053de9cb2c439650bd8fa4aaeb9365f2e5f27f492ff73c196" +dependencies = [ + "libc", + "rtoolbox", + "windows-sys 0.61.2", +] + +[[package]] +name = "rtoolbox" +version = "0.0.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "50a0e551c1e27e1731aba276dbeaeac73f53c7cd34d1bda485d02bd1e0f36844" +dependencies = [ + "libc", + "windows-sys 0.59.0", +] + [[package]] name = "rusqlite" version = "0.31.0" diff --git a/Cargo.toml b/Cargo.toml index 9225b27..678971f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -74,3 +74,5 @@ mimalloc = "0.1" clap = { version = "4", features = ["derive"] } regex = "1" notify = { version = "6", default-features = false, features = ["macos_kqueue"] } +# Hidden-input passphrase/value prompts for the `frameshift vault` CLI subcommand. +rpassword = "7" diff --git a/crates/frameshift-catalog-postgres/src/backend.rs b/crates/frameshift-catalog-postgres/src/backend.rs index 88f30bf..c49860b 100644 --- a/crates/frameshift-catalog-postgres/src/backend.rs +++ b/crates/frameshift-catalog-postgres/src/backend.rs @@ -581,6 +581,18 @@ impl CatalogBackend for PostgresCatalog { /// are set, both `@>` clauses are ANDed (intersection of intersections), /// which Postgres resolves via the GIN index efficiently. /// + /// # Tombstone exclusion mechanism + /// + /// Every query issued by this method (the plain DSL branches below and + /// the raw-SQL helpers `search_raw` / `search_trending_raw`) + /// unconditionally adds `latest_version IS NOT NULL` to its `WHERE` + /// clause. `latest_version` is recomputed by `tombstone_pack` on every + /// call to be the newest remaining `Active` version, or `NULL` when the + /// pack has zero `Active` versions left. So a pack "falls out" of search + /// exactly when its last `Active` version is tombstoned -- there is no + /// separate per-version status check here, because `search_packs` + /// operates on the `packs` head table, not `pack_versions`. + /// /// NOTE: Large offsets degrade because Postgres must scan and skip rows. /// Keyset pagination is a tracked future improvement. #[instrument(skip(self, filters))] @@ -619,6 +631,9 @@ impl CatalogBackend for PostgresCatalog { .await? } SortMode::TopRated => packs::table + // Dead packs (zero Active versions) have latest_version + // cleared by tombstone_pack's head recompute; exclude them. + .filter(packs::latest_version.is_not_null()) .select(PackRow::as_select()) .order((packs::total_downloads.desc(), packs::name.asc())) .limit(limit_i) @@ -627,6 +642,8 @@ impl CatalogBackend for PostgresCatalog { .await .map_err(|e| map_diesel_error(e, "pack", String::new()))?, SortMode::Recent => packs::table + // See the TopRated arm above for why this filter exists. + .filter(packs::latest_version.is_not_null()) .select(PackRow::as_select()) .order((packs::created_at.desc(), packs::name.asc())) .limit(limit_i) @@ -736,19 +753,29 @@ impl CatalogBackend for PostgresCatalog { Ok(new_count.max(0) as u64) } - /// Mark a specific pack version as tombstoned. + /// Mark a specific pack version as tombstoned and recompute the pack head. /// - /// SQL shape: - /// ```sql - /// UPDATE pack_versions SET status = $1 - /// WHERE pack_name = $2 AND version = $3 - /// ``` - /// The `status` column is set to the JSON serialisation of - /// `PackStatus::Tombstone { reason, recorded_at }`. No rows are deleted. + /// Executed inside a single transaction: + /// 1. `UPDATE pack_versions SET status = $1 WHERE pack_name = $2 AND + /// version = $3`. The `status` column is set to the JSON serialisation + /// of `PackStatus::Tombstone { reason, recorded_at }`. No rows are + /// deleted; content-addressed retrieval by hash still works afterwards. + /// 2. Every remaining version row for the pack is read back and its + /// `status` is deserialised. The `Active` versions are compared with + /// [`semver_gt`] (the SAME true-semver-precedence comparator + /// `register_pack_version` uses for D8) to find the newest one. + /// 3. `packs.latest_version` is set to that newest `Active` version, or to + /// `NULL` when no `Active` version remains -- this is what makes the + /// pack "disappear" from `search_packs` (see its doc for the + /// mechanism) while the version rows themselves stay queryable via + /// `get_pack_version` / `list_pack_versions` with their tombstoned + /// status visible. /// /// Re-tombstoning an already-tombstoned version is idempotent (last-writer /// wins). This differs from some adapters that return `Conflict` on - /// re-tombstone; the choice here favors operational simplicity. + /// re-tombstone; the choice here favors operational simplicity. The head + /// recompute step still runs on every call, which is a harmless no-op when + /// the newest `Active` version has not changed. #[instrument(skip(self, name, version, record), fields(pack = %name, version = %version))] async fn tombstone_pack( &self, @@ -765,26 +792,106 @@ impl CatalogBackend for PostgresCatalog { let status_json = serde_json::to_value(&status).map_err(|e| CatalogError::BackendError(Box::new(e)))?; - let rows_affected = diesel::update( - pack_versions::table.filter( - pack_versions::pack_name - .eq(name) - .and(pack_versions::version.eq(version)), - ), - ) - .set(pack_versions::status.eq(status_json)) - .execute(&mut *conn) - .await - .map_err(|e| map_diesel_error(e, "pack_version", format!("{name}@{version}")))?; + let name_owned = name.to_string(); + let version_owned = version.to_string(); - if rows_affected == 0 { - return Err(CatalogError::NotFound { - kind: "pack_version", - key: format!("{name}@{version}"), - }); + // Same `TxError` wrapper pattern as `register_pack_version`: diesel-async's + // `transaction` requires `E: From`, and `CatalogError` + // is a cross-crate type we cannot implement that for directly. + enum TxError { + Catalog(CatalogError), + Diesel(diesel::result::Error), + } + /// Required by `diesel_async::AsyncConnection::transaction`. + impl From for TxError { + /// Wrap a raw Diesel error in `TxError::Diesel` for transport + /// through the transaction boundary. + fn from(e: diesel::result::Error) -> Self { + TxError::Diesel(e) + } } - Ok(()) + use diesel_async::AsyncConnection as _; + let tx_result = conn + .transaction::<(), TxError, _>(async move |conn| { + let pack_name = name_owned; + let version = version_owned; + + let rows_affected = diesel::update( + pack_versions::table.filter( + pack_versions::pack_name + .eq(&pack_name) + .and(pack_versions::version.eq(&version)), + ), + ) + .set(pack_versions::status.eq(status_json)) + .execute(conn) + .await + .map_err(|e| { + TxError::Catalog(map_diesel_error( + e, + "pack_version", + format!("{pack_name}@{version}"), + )) + })?; + + if rows_affected == 0 { + return Err(TxError::Catalog(CatalogError::NotFound { + kind: "pack_version", + key: format!("{pack_name}@{version}"), + })); + } + + // Recompute the head: read back every version's (version, status) + // pair, keep the Active ones, and fold to find the newest by + // true semver precedence (mirrors D8's `semver_gt` in + // `register_pack_version`, not a new comparator). + let rows: Vec<(String, serde_json::Value)> = pack_versions::table + .filter(pack_versions::pack_name.eq(&pack_name)) + .select((pack_versions::version, pack_versions::status)) + .load(conn) + .await + .map_err(|e| { + TxError::Catalog(map_diesel_error(e, "pack_version", pack_name.clone())) + })?; + + let newest_active = rows + .into_iter() + .filter_map(|(v, status_json)| { + let status: frameshift_catalog::PackStatus = + serde_json::from_value(status_json).ok()?; + matches!(status, frameshift_catalog::PackStatus::Active).then_some(v) + }) + .fold(None::, |best, candidate| match best { + None => Some(candidate), + Some(cur) if semver_gt(&candidate, &cur) => Some(candidate), + Some(cur) => Some(cur), + }); + + // A no-op when the pack head row does not exist (cannot happen + // via the public API, since `register_pack_version` always + // creates the head before a version can be tombstoned). + diesel::update(packs::table.filter(packs::name.eq(&pack_name))) + .set(packs::latest_version.eq(newest_active)) + .execute(conn) + .await + .map_err(|e| { + TxError::Catalog(map_diesel_error(e, "pack", pack_name.clone())) + })?; + + Ok(()) + }) + .await; + + match tx_result { + Ok(()) => Ok(()), + Err(TxError::Catalog(e)) => Err(e), + Err(TxError::Diesel(e)) => Err(map_diesel_error( + e, + "pack_version", + format!("{name}@{version}"), + )), + } } /// Retrieve the Ed25519 public key currently mapped to a handle. @@ -1093,7 +1200,12 @@ impl PostgresCatalog { offset, } = params; let mut bind_idx: usize = 1; - let mut where_parts: Vec = Vec::new(); + // `latest_version IS NOT NULL` is unconditional (a literal, not a bind + // parameter) so it does not shift `bind_idx`. It excludes dead packs + // (zero Active versions -- see search_packs's doc for the mechanism) + // from every filtered search, matching the plain-DSL branches in + // `search_packs` for the unfiltered case. + let mut where_parts: Vec = vec!["latest_version IS NOT NULL".to_string()]; if tag.is_some() { where_parts.push(format!("tags @> ARRAY[${bind_idx}]::TEXT[]")); @@ -1123,11 +1235,8 @@ impl PostgresCatalog { bind_idx += 1; } - let where_sql = if where_parts.is_empty() { - String::new() - } else { - format!("WHERE {}", where_parts.join(" AND ")) - }; + // `where_parts` always has at least the latest_version clause above. + let where_sql = format!("WHERE {}", where_parts.join(" AND ")); let order_sql = match sort { SortMode::TopRated | SortMode::Trending => "ORDER BY total_downloads DESC, name ASC", @@ -1468,7 +1577,11 @@ impl PostgresCatalog { // Bind order matches the branch arms below: tag, target_context, author, // query_text, extends. The window interval is bound last before limit/offset. let mut bind_idx: usize = 1; - let mut where_parts: Vec = Vec::new(); + // `p.latest_version IS NOT NULL` is unconditional (a literal, not a bind + // parameter) so it does not shift `bind_idx`. It excludes dead packs + // (zero Active versions -- see search_packs's doc for the mechanism) + // from every trending search, filtered or not. + let mut where_parts: Vec = vec!["p.latest_version IS NOT NULL".to_string()]; if tag.is_some() { where_parts.push(format!("p.tags @> ARRAY[${bind_idx}]::TEXT[]")); @@ -1494,11 +1607,8 @@ impl PostgresCatalog { bind_idx += 1; } - let where_sql = if where_parts.is_empty() { - String::new() - } else { - format!("WHERE {}", where_parts.join(" AND ")) - }; + // `where_parts` always has at least the latest_version clause above. + let where_sql = format!("WHERE {}", where_parts.join(" AND ")); // The subquery interval bound index comes after all filter params. let interval_idx = bind_idx; @@ -1884,7 +1994,14 @@ fn parse_semver(s: &str) -> Option<(u64, u64, u64, Option)> { /// /// Unparseable versions are treated as lower than any parseable version. /// If both sides are unparseable, returns `false` (not strictly greater). -fn semver_gt(a: &str, b: &str) -> bool { +/// +/// `pub` (not `pub(crate)`) so that other in-workspace `CatalogBackend` +/// implementations -- notably the in-memory mock used by +/// `frameshift-server`'s integration tests -- can recompute a pack head's +/// `latest_version` using the exact same ordering `register_pack_version` +/// and `tombstone_pack` use here, instead of reimplementing (and risking +/// drift from) the comparator. +pub fn semver_gt(a: &str, b: &str) -> bool { match (parse_semver(a), parse_semver(b)) { // `a` is unparseable -- can never be greater. (None, _) => false, diff --git a/crates/frameshift-catalog-postgres/tests/postgres_integration.rs b/crates/frameshift-catalog-postgres/tests/postgres_integration.rs index b3fae48..5729bc9 100644 --- a/crates/frameshift-catalog-postgres/tests/postgres_integration.rs +++ b/crates/frameshift-catalog-postgres/tests/postgres_integration.rs @@ -432,6 +432,197 @@ async fn test_tombstone_not_found() { ); } +/// Tombstoning the current latest of two `Active` versions recomputes the +/// pack head's `latest_version` to the older remaining `Active` version +/// (spec_42eb1942 item 1: the head, not just the version row, must reflect +/// the tombstone). The pack must remain visible in `search_packs` because it +/// still has one `Active` version left. +#[tokio::test] +#[ignore = "requires Docker"] +async fn test_tombstone_latest_recomputes_head_to_older_active_version() { + let (catalog, _container) = setup_catalog().await; + + catalog + .register_author(make_author(50, "morgan")) + .await + .expect("register author failed"); + + catalog + .register_pack_version(make_version("head-recompute-pack", "1.0.0", 50, 100)) + .await + .expect("register 1.0.0 failed"); + catalog + .register_pack_version(make_version("head-recompute-pack", "2.0.0", 50, 101)) + .await + .expect("register 2.0.0 failed"); + + // Sanity: latest_version is "2.0.0" before the tombstone. + let before = catalog + .get_pack("head-recompute-pack") + .await + .expect("get_pack before tombstone failed"); + assert_eq!(before.latest_version, Some("2.0.0".to_string())); + + catalog + .tombstone_pack( + "head-recompute-pack", + "2.0.0", + TombstoneRecord { + reason: TombstoneReason::AuthorRequest, + recorded_at: chrono::Utc::now(), + }, + ) + .await + .expect("tombstone_pack failed"); + + let after = catalog + .get_pack("head-recompute-pack") + .await + .expect("get_pack after tombstone failed"); + assert_eq!( + after.latest_version, + Some("1.0.0".to_string()), + "latest_version must fall back to the newest remaining Active version" + ); + + let results = catalog + .search_packs(&PackSearchFilters { + sort: SortMode::Recent, + limit: 50, + offset: 0, + ..Default::default() + }) + .await + .expect("search_packs failed"); + assert!( + results.iter().any(|r| r.pack.name == "head-recompute-pack"), + "pack must still appear in search after tombstoning its (non-only) latest version" + ); +} + +/// Tombstoning the ONLY version of a pack clears the head's `latest_version` +/// to `NULL`, which removes the pack from `search_packs` entirely. The +/// version record itself remains readable via `get_pack_version` with +/// `Tombstone` status. +#[tokio::test] +#[ignore = "requires Docker"] +async fn test_tombstone_only_version_clears_head_and_hides_from_search() { + let (catalog, _container) = setup_catalog().await; + + catalog + .register_author(make_author(51, "nadia")) + .await + .expect("register author failed"); + + catalog + .register_pack_version(make_version("solo-pack", "1.0.0", 51, 102)) + .await + .expect("register 1.0.0 failed"); + + catalog + .tombstone_pack( + "solo-pack", + "1.0.0", + TombstoneRecord { + reason: TombstoneReason::TosViolation, + recorded_at: chrono::Utc::now(), + }, + ) + .await + .expect("tombstone_pack failed"); + + let after = catalog + .get_pack("solo-pack") + .await + .expect("get_pack after tombstone failed"); + assert_eq!( + after.latest_version, None, + "latest_version must clear to NULL when no Active version remains" + ); + + let results = catalog + .search_packs(&PackSearchFilters { + sort: SortMode::Recent, + limit: 50, + offset: 0, + ..Default::default() + }) + .await + .expect("search_packs failed"); + assert!( + !results.iter().any(|r| r.pack.name == "solo-pack"), + "pack must disappear from search once its only version is tombstoned" + ); + + let version = catalog + .get_pack_version("solo-pack", "1.0.0") + .await + .expect("get_pack_version must still return the tombstoned record"); + assert!( + matches!(version.status, PackStatus::Tombstone { .. }), + "tombstoned version record must remain directly readable with its status intact" + ); +} + +/// Tombstoning a non-latest version leaves the head's `latest_version` +/// untouched and does not affect search visibility. +#[tokio::test] +#[ignore = "requires Docker"] +async fn test_tombstone_non_latest_version_leaves_head_unchanged() { + let (catalog, _container) = setup_catalog().await; + + catalog + .register_author(make_author(52, "oscar")) + .await + .expect("register author failed"); + + catalog + .register_pack_version(make_version("stable-pack", "1.0.0", 52, 103)) + .await + .expect("register 1.0.0 failed"); + catalog + .register_pack_version(make_version("stable-pack", "2.0.0", 52, 104)) + .await + .expect("register 2.0.0 failed"); + + // Tombstone the OLDER, non-latest version. + catalog + .tombstone_pack( + "stable-pack", + "1.0.0", + TombstoneRecord { + reason: TombstoneReason::Dmca, + recorded_at: chrono::Utc::now(), + }, + ) + .await + .expect("tombstone_pack failed"); + + let after = catalog + .get_pack("stable-pack") + .await + .expect("get_pack after tombstone failed"); + assert_eq!( + after.latest_version, + Some("2.0.0".to_string()), + "latest_version must be unchanged when a non-latest version is tombstoned" + ); + + let results = catalog + .search_packs(&PackSearchFilters { + sort: SortMode::Recent, + limit: 50, + offset: 0, + ..Default::default() + }) + .await + .expect("search_packs failed"); + assert!( + results.iter().any(|r| r.pack.name == "stable-pack"), + "pack must remain in search after tombstoning a non-latest version" + ); +} + /// set_handle_pubkey transfers handle ownership; get_handle_pubkey reflects it. #[tokio::test] #[ignore = "requires Docker"] diff --git a/crates/frameshift-catalog/src/backend.rs b/crates/frameshift-catalog/src/backend.rs index f29b375..b331c42 100644 --- a/crates/frameshift-catalog/src/backend.rs +++ b/crates/frameshift-catalog/src/backend.rs @@ -151,6 +151,16 @@ pub trait CatalogBackend: Send + Sync { /// Retrieve a specific version record for the given pack and version string. /// + /// This method DOES return tombstoned records, with `status` set to + /// `PackStatus::Tombstone { .. }` so the caller can see exactly what + /// happened and when. It never hides a version just because it was taken + /// down -- direct, targeted lookup by `(name, version)` is precisely the + /// operation an auditor or a consumer chasing a broken dependency needs, + /// and answering it honestly (rather than returning `NotFound` for a + /// version that in fact exists) is the deliberate design choice here. + /// Callers that want to reject serving tombstoned content (e.g. the pack + /// download route) MUST check `status` themselves after the call. + /// /// # Errors /// /// - `CatalogError::NotFound` (kind `"pack_version"`) -- no such version. @@ -170,6 +180,15 @@ pub trait CatalogBackend: Send + Sync { /// Returns an empty `Vec` if the pack has no published versions. Returns /// `CatalogError::NotFound` if the pack does not exist at all. /// + /// This method returns EVERY version, including tombstoned ones, with + /// `status` set to `PackStatus::Tombstone { .. }` on those records. This is + /// deliberate transparency (the same norm most package registries follow: + /// a takedown removes a version from discovery/installation, but the + /// version history itself stays visible so consumers can see what + /// happened to a version they may already depend on). Callers that want to + /// hide tombstoned versions from an end-user-facing list must filter on + /// `status` themselves; this method does not do it for them. + /// /// # Errors /// /// - `CatalogError::NotFound` (kind `"pack"`) -- pack does not exist. @@ -183,9 +202,19 @@ pub trait CatalogBackend: Send + Sync { /// Search for packs matching the given filters. /// /// Returns results ordered by the sort mode specified in `filters`, with a - /// deterministic `name ASC` tiebreaker for equal scores. Tombstoned versions - /// are excluded from results unless the adapter explicitly supports - /// `include_tombstoned` (which this filter set does not -- future extension). + /// deterministic `name ASC` tiebreaker for equal scores. + /// + /// Tombstoned content is excluded from results via the pack head's + /// `latest_version` field: `tombstone_pack` recomputes `latest_version` to + /// the newest remaining `Active` version every time it tombstones a + /// version, clearing it to `None` when no `Active` version remains. A pack + /// with `latest_version == None` (zero `Active` versions) MUST NOT appear + /// in `search_packs` results. There is no per-version status check inside + /// this method -- it operates entirely on the pack head, and the head's + /// `latest_version` is the single source of truth for "is this pack + /// currently installable." Adapters MUST implement the `latest_version IS + /// NOT NULL` exclusion (or equivalent) in every code path this method can + /// take, not just the default/no-filter path. /// /// Returns an empty `Vec` (not an error) when no packs match. /// @@ -227,6 +256,30 @@ pub trait CatalogBackend: Send + Sync { /// `PackStatus::Active` to `PackStatus::Tombstone`. Content-addressed /// retrieval by hash still works after tombstoning. /// + /// # Head recompute contract + /// + /// After flipping the version's status, the implementation MUST recompute + /// the parent pack's `latest_version` field so that it never points at a + /// tombstoned version: + /// + /// - Find the newest remaining `PackStatus::Active` version for the pack, + /// using the SAME version-precedence ordering `register_pack_version` + /// uses to decide `latest_version` (true semver precedence, not + /// lexicographic or insertion order -- see the adapter's + /// `register_pack_version` doc for specifics). + /// - Set `latest_version` to that version. + /// - If no `Active` version remains for the pack (this was the last one), + /// clear `latest_version` to `None` rather than leaving it pointing at + /// the version that was just tombstoned. This is what makes the pack + /// disappear from `search_packs` (which excludes packs with + /// `latest_version == None`) while still allowing direct lookups + /// (`get_pack_version`, `list_pack_versions`) to see it. + /// + /// This recompute MUST happen atomically with the status update (same + /// transaction where the adapter supports transactions), so a reader can + /// never observe a pack head whose `latest_version` points at a + /// `Tombstone` version. + /// /// Adapter MUST document whether re-tombstoning an already-tombstoned version /// is idempotent or returns `CatalogError::Conflict`. /// diff --git a/crates/frameshift-catalog/src/records.rs b/crates/frameshift-catalog/src/records.rs index 26b8af5..2c3c160 100644 --- a/crates/frameshift-catalog/src/records.rs +++ b/crates/frameshift-catalog/src/records.rs @@ -85,7 +85,10 @@ pub struct OauthLink { /// /// - `name` is unique within the catalog. /// - `latest_version` is `None` until at least one version has been published, -/// and is updated atomically when a new version is registered. +/// is updated atomically when a new version is registered, and is +/// recomputed (possibly back to `None`) whenever a version is tombstoned -- +/// see [`crate::backend::CatalogBackend::tombstone_pack`]'s recompute +/// contract. /// - `total_downloads` is a monotonically increasing counter; it is never /// decremented even if a version is tombstoned. /// - `tags` may be empty; duplicates within the vec are discouraged but not @@ -113,10 +116,14 @@ pub struct PackRecord { /// UTC timestamp when this pack was first created in the catalog. pub created_at: DateTime, - /// The semver string of the most-recently published version. + /// The semver string of the newest `PackStatus::Active` version. /// - /// `None` until the first version is registered. Updated atomically by - /// `register_pack_version`. + /// `None` until the first version is registered, and also `None` again if + /// every version is later tombstoned. Updated atomically by + /// `register_pack_version` on publish and recomputed by `tombstone_pack` + /// on takedown (see that method's doc for the recompute contract). This + /// field, not any per-version status, is what `search_packs` uses to + /// decide whether a pack is currently installable. pub latest_version: Option, /// Cumulative download count across all versions of this pack. diff --git a/crates/frameshift-cli/Cargo.toml b/crates/frameshift-cli/Cargo.toml index b90d2a6..0bf4704 100644 --- a/crates/frameshift-cli/Cargo.toml +++ b/crates/frameshift-cli/Cargo.toml @@ -20,7 +20,13 @@ frameshift-growth = { path = "../frameshift-growth" } frameshift-conformance = { path = "../frameshift-conformance", features = ["cli-runner"] } frameshift-orchestrator = { path = "../frameshift-orchestrator" } frameshift-embed-candle = { path = "../frameshift-embed-candle", optional = true } +# `frameshift vault` subcommand: vault schema/error/backend-trait types and the +# age-encrypted local backend (init/set/get/rm/list operate on these directly). +frameshift-vault = { path = "../frameshift-vault" } +frameshift-vault-local = { path = "../frameshift-vault-local" } clap = { workspace = true } +# Wraps the vault passphrase/value input so it is never accidentally logged. +secrecy = { workspace = true } serde_json = { workspace = true } thiserror = { workspace = true } tokio = { workspace = true } @@ -28,6 +34,9 @@ tokio = { workspace = true } # recording) that must warn on failure without failing the command. tracing = { workspace = true } tracing-subscriber = { workspace = true } +# Hidden-input prompts for `frameshift vault` passphrases/values and the +# render-time interactive vault provider (only used when stdin is a TTY). +rpassword = { workspace = true } [features] # Semantic selection via a local candle sentence-embedding model. Off by diff --git a/crates/frameshift-cli/src/cmd/config.rs b/crates/frameshift-cli/src/cmd/config.rs new file mode 100644 index 0000000..a54f254 --- /dev/null +++ b/crates/frameshift-cli/src/cmd/config.rs @@ -0,0 +1,262 @@ +//! CLI handler for the `frameshift config ` subcommand. +//! +//! Reads and writes the current project's central config +//! (`projects//config.toml`) via `frameshift_client::Client::project_config` +//! and `Client::save_project_config`. Keys are scoped to an explicit allowlist +//! so unknown keys fail clearly instead of silently no-op-ing; the allowlist is +//! expected to grow as more `ProjectConfig` fields become CLI-settable. + +use clap::{Args, Subcommand}; +use frameshift_client::Client; + +use crate::util::CliError; + +/// Arguments for the `config` subcommand. +#[derive(Debug, Args)] +pub struct ConfigArgs { + /// Action to perform on the project config. + #[command(subcommand)] + pub action: ConfigAction, +} + +/// Available config actions. +#[derive(Debug, Subcommand)] +pub enum ConfigAction { + /// Print the current value of a config key. + Get { + /// Config key to read (see the allowlist in `cmd::config`). + key: String, + }, + /// Set a config key to a new value, persisting it to config.toml. + Set { + /// Config key to write (see the allowlist in `cmd::config`). + key: String, + /// New value for the key. + value: String, + }, +} + +/// The single config key currently exposed through this subcommand. +/// +/// Kept as a named constant (rather than inlined) so the allowlist has one +/// obvious place to grow when more `ProjectConfig` fields become settable. +const KEY_TELEMETRY_OPT_IN: &str = "telemetry_opt_in"; + +/// Execute the `config` subcommand. The project is the current working +/// directory, matching every sibling subcommand's resolution. +pub fn run_config(client: &Client, args: ConfigArgs) -> Result<(), CliError> { + let project_root = std::env::current_dir()?; + match args.action { + ConfigAction::Get { key } => run_get(client, &project_root, &key), + ConfigAction::Set { key, value } => run_set(client, &project_root, &key, &value), + } +} + +/// Execute `config get ` -- print the key's current value to stdout. +/// Takes the project root explicitly so tests can target a temp project. +fn run_get(client: &Client, project_root: &std::path::Path, key: &str) -> Result<(), CliError> { + let config = client.project_config(project_root)?; + + match key { + KEY_TELEMETRY_OPT_IN => { + println!("{}", config.telemetry_opt_in); + Ok(()) + } + other => Err(unknown_key_error(other)), + } +} + +/// Execute `config set ` -- parse `value` for `key`'s type, +/// persist the updated config, and confirm on stdout. Creates config.toml +/// with defaults for any fields not being set, if the file did not already +/// exist. +/// Takes the project root explicitly so tests can target a temp project. +fn run_set( + client: &Client, + project_root: &std::path::Path, + key: &str, + value: &str, +) -> Result<(), CliError> { + let mut config = client.project_config(project_root)?; + + match key { + KEY_TELEMETRY_OPT_IN => { + config.telemetry_opt_in = parse_bool(key, value)?; + } + other => return Err(unknown_key_error(other)), + } + + client.save_project_config(project_root, &config)?; + println!("{key} = {value}"); + Ok(()) +} + +/// Build the "unknown key" error for a key that is not in the allowlist. +fn unknown_key_error(key: &str) -> CliError { + CliError::Config(format!( + "unknown config key '{key}'; known keys: {KEY_TELEMETRY_OPT_IN}" + )) +} + +/// Parse `value` as a strict `true`/`false` boolean for `key`, rejecting any +/// other spelling (e.g. `1`, `yes`) with a clear error naming the key. +fn parse_bool(key: &str, value: &str) -> Result { + match value { + "true" => Ok(true), + "false" => Ok(false), + other => Err(CliError::Config(format!( + "invalid value '{other}' for key '{key}'; expected 'true' or 'false'" + ))), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use frameshift_client::ClientOptions; + + /// Build a `Client` rooted at a fresh temp directory so tests never touch + /// the developer's real `$XDG_DATA_HOME/frameshift` store. + fn test_client(tmp: &std::path::Path) -> Client { + Client::new(ClientOptions { + data_root: tmp.to_path_buf(), + config_root: None, + vault: None, + }) + } + + /// `config get telemetry_opt_in` on a project with no config.toml yet + /// reads the default (`false`) without erroring. + #[test] + fn get_defaults_to_false_when_config_missing() { + let tmp = tempfile::tempdir().expect("tempdir"); + let client = test_client(tmp.path()); + let project_root = tmp.path().join("project"); + std::fs::create_dir_all(&project_root).expect("create project dir"); + + let config = client.project_config(&project_root).expect("read config"); + assert!(!config.telemetry_opt_in); + } + + /// `config set telemetry_opt_in true` followed by a read round-trips the + /// new value, creating config.toml along the way. + #[test] + fn set_then_get_round_trips_true() { + let tmp = tempfile::tempdir().expect("tempdir"); + let client = test_client(tmp.path()); + let project_root = tmp.path().join("project"); + std::fs::create_dir_all(&project_root).expect("create project dir"); + + run_set(&client, &project_root, KEY_TELEMETRY_OPT_IN, "true").expect("set should succeed"); + + let config = client.project_config(&project_root).expect("read config"); + assert!(config.telemetry_opt_in); + } + + /// Setting `telemetry_opt_in` back to `false` after it was `true` + /// persists the flip rather than leaving the stale value in place. + #[test] + fn set_then_get_round_trips_false_after_true() { + let tmp = tempfile::tempdir().expect("tempdir"); + let client = test_client(tmp.path()); + let project_root = tmp.path().join("project"); + std::fs::create_dir_all(&project_root).expect("create project dir"); + + run_set(&client, &project_root, KEY_TELEMETRY_OPT_IN, "true") + .expect("first set should succeed"); + run_set(&client, &project_root, KEY_TELEMETRY_OPT_IN, "false") + .expect("second set should succeed"); + + let config = client.project_config(&project_root).expect("read config"); + assert!(!config.telemetry_opt_in); + } + + /// `config get` on an unknown key returns `CliError::Config` naming the + /// rejected key. + #[test] + fn get_unknown_key_errors() { + let tmp = tempfile::tempdir().expect("tempdir"); + let client = test_client(tmp.path()); + let project_root = tmp.path().join("project"); + std::fs::create_dir_all(&project_root).expect("create project dir"); + + let err = run_get(&client, &project_root, "not_a_real_key").unwrap_err(); + match err { + CliError::Config(msg) => assert!(msg.contains("not_a_real_key")), + other => panic!("expected CliError::Config, got {other:?}"), + } + } + + /// `config set` on an unknown key returns `CliError::Config` and does not + /// write anything. + #[test] + fn set_unknown_key_errors() { + let tmp = tempfile::tempdir().expect("tempdir"); + let client = test_client(tmp.path()); + let project_root = tmp.path().join("project"); + std::fs::create_dir_all(&project_root).expect("create project dir"); + + let err = run_set(&client, &project_root, "not_a_real_key", "true").unwrap_err(); + assert!(matches!(err, CliError::Config(_))); + } + + /// `config set telemetry_opt_in ` rejects any spelling other + /// than the literal strings `true`/`false`. + #[test] + fn set_rejects_non_boolean_value() { + let tmp = tempfile::tempdir().expect("tempdir"); + let client = test_client(tmp.path()); + let project_root = tmp.path().join("project"); + std::fs::create_dir_all(&project_root).expect("create project dir"); + + let err = run_set(&client, &project_root, KEY_TELEMETRY_OPT_IN, "yes").unwrap_err(); + assert!(matches!(err, CliError::Config(_))); + } + + /// Clap parsing: `config get telemetry_opt_in` parses into `Get`. + #[test] + fn parse_get_with_key() { + use clap::Parser; + + /// Helper command hosting `ConfigAction` so clap can parse a full + /// `frameshift config ...` invocation in isolation. + #[derive(Debug, Parser)] + #[command(name = "frameshift", no_binary_name = true)] + struct TestCli { + #[command(subcommand)] + action: ConfigAction, + } + + let parsed = + TestCli::try_parse_from(["get", "telemetry_opt_in"]).expect("get should parse"); + match parsed.action { + ConfigAction::Get { key } => assert_eq!(key, "telemetry_opt_in"), + other => panic!("expected Get, got {other:?}"), + } + } + + /// Clap parsing: `config set telemetry_opt_in true` parses into `Set`. + #[test] + fn parse_set_with_key_and_value() { + use clap::Parser; + + /// Helper command hosting `ConfigAction` so clap can parse a full + /// `frameshift config ...` invocation in isolation. + #[derive(Debug, Parser)] + #[command(name = "frameshift", no_binary_name = true)] + struct TestCli { + #[command(subcommand)] + action: ConfigAction, + } + + let parsed = + TestCli::try_parse_from(["set", "telemetry_opt_in", "true"]).expect("set should parse"); + match parsed.action { + ConfigAction::Set { key, value } => { + assert_eq!(key, "telemetry_opt_in"); + assert_eq!(value, "true"); + } + other => panic!("expected Set, got {other:?}"), + } + } +} diff --git a/crates/frameshift-cli/src/cmd/diff.rs b/crates/frameshift-cli/src/cmd/diff.rs index 1f195cb..209009a 100644 --- a/crates/frameshift-cli/src/cmd/diff.rs +++ b/crates/frameshift-cli/src/cmd/diff.rs @@ -174,6 +174,7 @@ mod tests { let client = Client::new(ClientOptions { data_root, config_root: None, + vault: None, }); let args = DiffArgs { persona_a: "pa".to_string(), diff --git a/crates/frameshift-cli/src/cmd/grow.rs b/crates/frameshift-cli/src/cmd/grow.rs index e6db50d..c0de733 100644 --- a/crates/frameshift-cli/src/cmd/grow.rs +++ b/crates/frameshift-cli/src/cmd/grow.rs @@ -41,6 +41,16 @@ pub struct AppendArgs { /// Text content to append. #[arg(long)] pub text: String, + + /// Write a structured global-scope entry instead of a project-scope one. + /// + /// When set, this writes ONLY a structured `Scope::Global` entry via + /// `frameshift_growth::append_global` to the persona's global + /// growth.jsonl (`/personas//growth.jsonl`). The legacy + /// markdown growth.md append stays project-scoped and is skipped + /// entirely in this mode -- there is no global markdown growth file. + #[arg(long)] + pub global: bool, } /// Arguments for grow log. @@ -96,18 +106,38 @@ pub fn run(args: GrowArgs) -> Result<(), CliError> { } } -/// Execute grow append -- write a timestamped entry to the persona's growth.md -/// and structured growth.jsonl (via `frameshift_growth::append`'s dual-write). +/// Execute grow append. +/// +/// Default (no `--global`): writes a timestamped entry to the persona's +/// growth.md and structured growth.jsonl (via `frameshift_growth::append`'s +/// dual-write), both project-scoped. +/// +/// With `--global`: writes ONLY a structured `Scope::Global` entry via +/// `frameshift_growth::append_global`. The legacy markdown append is skipped +/// entirely -- there is no global markdown growth file. fn run_append(args: AppendArgs) -> Result<(), CliError> { let client = frameshift_client::Client::with_default_data_root()?; let project_root = std::env::current_dir() .map_err(|e| CliError::Growth(format!("cannot determine current directory: {}", e)))?; let project_id = client.project_id(&project_root)?; - frameshift_growth::append(client.data_root(), &project_id, &args.persona, &args.text) + if args.global { + frameshift_growth::append_global( + client.data_root(), + &project_id, + &args.persona, + &args.text, + ) .map_err(|e| CliError::Growth(e.to_string()))?; - - println!("Growth entry appended for persona '{}'.", args.persona); + println!( + "Global growth entry appended for persona '{}'.", + args.persona + ); + } else { + frameshift_growth::append(client.data_root(), &project_id, &args.persona, &args.text) + .map_err(|e| CliError::Growth(e.to_string()))?; + println!("Growth entry appended for persona '{}'.", args.persona); + } Ok(()) } @@ -199,7 +229,8 @@ mod tests { action: GrowAction, } - /// `grow append --persona

--text ` parses both required flags. + /// `grow append --persona

--text ` parses both required flags, + /// leaving `--global` at its default of `false`. #[test] fn parse_append_with_persona_and_text() { let parsed = TestCli::try_parse_from(["append", "--persona", "rust", "--text", "hi"]) @@ -208,11 +239,24 @@ mod tests { GrowAction::Append(args) => { assert_eq!(args.persona, "rust"); assert_eq!(args.text, "hi"); + assert!(!args.global); } other => panic!("expected Append, got {other:?}"), } } + /// `grow append --persona

--text --global` sets the `global` flag. + #[test] + fn parse_append_with_global_flag() { + let parsed = + TestCli::try_parse_from(["append", "--persona", "rust", "--text", "hi", "--global"]) + .expect("append --global should parse"); + match parsed.action { + GrowAction::Append(args) => assert!(args.global), + other => panic!("expected Append, got {other:?}"), + } + } + /// `grow log --persona

` parses with the default limit of 10. #[test] fn parse_log_defaults_limit_to_ten() { diff --git a/crates/frameshift-cli/src/cmd/migrate.rs b/crates/frameshift-cli/src/cmd/migrate.rs index e4864ec..e5b118b 100644 --- a/crates/frameshift-cli/src/cmd/migrate.rs +++ b/crates/frameshift-cli/src/cmd/migrate.rs @@ -123,6 +123,7 @@ mod tests { let client = Client::new(ClientOptions { data_root: tmp.path().join("data"), config_root: None, + vault: None, }); let persona_dir = client .data_root() @@ -185,6 +186,7 @@ mod tests { let client = Client::new(ClientOptions { data_root: tmp.path().join("data"), config_root: None, + vault: None, }); let migrated = diff --git a/crates/frameshift-cli/src/cmd/mod.rs b/crates/frameshift-cli/src/cmd/mod.rs index be9f533..756ca0b 100644 --- a/crates/frameshift-cli/src/cmd/mod.rs +++ b/crates/frameshift-cli/src/cmd/mod.rs @@ -4,6 +4,7 @@ //! `Subcommand` enum in `main.rs` dispatches to the appropriate module. pub mod automate; +pub mod config; pub mod diff; pub mod feedback; pub mod grow; @@ -17,4 +18,5 @@ pub mod search; pub mod select; pub mod skill; pub mod use_persona; +pub mod vault; pub mod verify; diff --git a/crates/frameshift-cli/src/cmd/render.rs b/crates/frameshift-cli/src/cmd/render.rs index 4af005d..97247f6 100644 --- a/crates/frameshift-cli/src/cmd/render.rs +++ b/crates/frameshift-cli/src/cmd/render.rs @@ -112,6 +112,7 @@ mod tests { let client = Client::new(ClientOptions { data_root, config_root: None, + vault: None, }); let args = RenderArgs { persona: "render-test".to_string(), diff --git a/crates/frameshift-cli/src/cmd/rule.rs b/crates/frameshift-cli/src/cmd/rule.rs index 45ab9d2..cb42ba8 100644 --- a/crates/frameshift-cli/src/cmd/rule.rs +++ b/crates/frameshift-cli/src/cmd/rule.rs @@ -177,6 +177,7 @@ mod tests { let client = Client::new(ClientOptions { data_root, config_root: None, + vault: None, }); let args = RuleAddArgs { persona: "test-persona".to_string(), @@ -226,6 +227,7 @@ mod tests { let client = Client::new(ClientOptions { data_root, config_root: None, + vault: None, }); let args = RuleRemoveArgs { persona: "p2".to_string(), @@ -269,6 +271,7 @@ mod tests { let client = Client::new(ClientOptions { data_root, config_root: None, + vault: None, }); let args = RuleAddArgs { persona: "dup".to_string(), diff --git a/crates/frameshift-cli/src/cmd/skill.rs b/crates/frameshift-cli/src/cmd/skill.rs index 2bc87f3..95a5f3b 100644 --- a/crates/frameshift-cli/src/cmd/skill.rs +++ b/crates/frameshift-cli/src/cmd/skill.rs @@ -119,6 +119,7 @@ mod tests { let client = Client::new(ClientOptions { data_root, config_root: None, + vault: None, }); let args = SkillAddArgs { persona: "sp1".to_string(), @@ -166,6 +167,7 @@ mod tests { let client = Client::new(ClientOptions { data_root, config_root: None, + vault: None, }); let args = SkillRemoveArgs { persona: "sp2".to_string(), @@ -207,6 +209,7 @@ mod tests { let client = Client::new(ClientOptions { data_root, config_root: None, + vault: None, }); let args = SkillAddArgs { persona: "dup-skill".to_string(), diff --git a/crates/frameshift-cli/src/cmd/use_persona.rs b/crates/frameshift-cli/src/cmd/use_persona.rs index 95aad0c..14c2a38 100644 --- a/crates/frameshift-cli/src/cmd/use_persona.rs +++ b/crates/frameshift-cli/src/cmd/use_persona.rs @@ -220,6 +220,7 @@ mod tests { let client = Client::new(ClientOptions { data_root: tmp.path().join("data"), config_root: None, + vault: None, }); let project_root = tmp.path().join("project"); std::fs::create_dir_all(&project_root).unwrap(); diff --git a/crates/frameshift-cli/src/cmd/vault.rs b/crates/frameshift-cli/src/cmd/vault.rs new file mode 100644 index 0000000..7e3483d --- /dev/null +++ b/crates/frameshift-cli/src/cmd/vault.rs @@ -0,0 +1,383 @@ +//! CLI handler for the `frameshift vault ` subcommand. +//! +//! Operates on the current project's vault (`ProjectPaths::vault_path`, a +//! sibling of `config.toml` in the central store), which holds the +//! `{{token}}` values a templated pack (one shipping `pack.template.toml`) +//! substitutes at render time. All operations go through +//! `frameshift-vault-local::LocalAgeBackend` and, before any write, +//! `frameshift_vault::validate`. +//! +//! # Passphrase resolution +//! +//! Every subcommand resolves the vault passphrase the same way (see +//! [`resolve_passphrase`]): `FRAMESHIFT_VAULT_PASSPHRASE` first, then -- +//! only when stdin is an interactive terminal -- a hidden `rpassword` +//! prompt. A non-interactive invocation with no env var set fails with a +//! typed error rather than hanging on a prompt nobody can answer. + +use std::collections::BTreeMap; +use std::io::IsTerminal; + +use clap::{Args, Subcommand}; +use frameshift_client::{Client, ProjectPaths, VAULT_PASSPHRASE_ENV}; +use frameshift_vault::{ + validate, Auth, Identity, Preferences, RuntimeMode, VaultBackend, VaultData, VaultError, +}; +use frameshift_vault_local::{LocalAgeBackend, Recipients}; +use secrecy::SecretString; + +use crate::util::CliError; + +/// Arguments for the `vault` subcommand. +#[derive(Debug, Args)] +pub struct VaultArgs { + /// Action to perform on the project vault. + #[command(subcommand)] + pub action: VaultAction, +} + +/// Available vault actions. +#[derive(Debug, Subcommand)] +pub enum VaultAction { + /// Create an empty vault for the current project. Refuses if one + /// already exists at `ProjectPaths::vault_path`. + Init, + /// Set a token value in the vault. Prompts for the value (hidden, never + /// echoed) when `--value` is omitted. + Set { + /// Token key to set. + key: String, + /// Value to store. If omitted, prompted for interactively (hidden). + #[arg(long)] + value: Option, + }, + /// Print a token's raw value to stdout. + Get { + /// Token key to read. + key: String, + }, + /// Remove a token from the vault. + Rm { + /// Token key to remove. + key: String, + }, + /// List every token key currently set in the vault. Never prints values. + List, +} + +/// Execute the `vault` subcommand for the project rooted at the current +/// working directory. +pub fn run_vault(client: &Client, args: VaultArgs) -> Result<(), CliError> { + let project_root = std::env::current_dir()?; + let paths = client.project_paths(&project_root)?; + + match args.action { + VaultAction::Init => run_init(&paths), + VaultAction::Set { key, value } => run_set(&paths, &key, value), + VaultAction::Get { key } => run_get(&paths, &key), + VaultAction::Rm { key } => run_rm(&paths, &key), + VaultAction::List => run_list(&paths), + } +} + +/// Resolve the vault passphrase: [`VAULT_PASSPHRASE_ENV`] first, then -- +/// only when stdin is an interactive terminal -- a hidden `rpassword` +/// prompt. +/// +/// Shared by every `frameshift vault` subcommand in this module and by the +/// CLI's render-time vault provider (`main::cli_open_vault`), so both call +/// sites resolve the passphrase identically. +/// +/// # Errors +/// +/// Returns [`VaultError::BackendUnavailable`] when the env var is unset (or +/// empty) and stdin is not a terminal -- this call never blocks waiting on a +/// prompt that cannot be answered -- or when the prompt itself fails to read. +pub(crate) fn resolve_passphrase() -> Result { + if let Ok(value) = std::env::var(VAULT_PASSPHRASE_ENV) { + if !value.is_empty() { + return Ok(SecretString::new(value)); + } + } + + if std::io::stdin().is_terminal() { + let entered = rpassword::prompt_password("vault passphrase: ").map_err(|e| { + VaultError::BackendUnavailable(format!("failed to read passphrase: {e}")) + })?; + return Ok(SecretString::new(entered)); + } + + Err(VaultError::BackendUnavailable(format!( + "vault passphrase not available: set {VAULT_PASSPHRASE_ENV} or run interactively" + ))) +} + +/// Build the [`LocalAgeBackend`] for `paths.vault_path`, resolving the +/// passphrase via [`resolve_passphrase`]. +fn open_backend(paths: &ProjectPaths) -> Result { + let passphrase = resolve_passphrase()?; + Ok(LocalAgeBackend::new( + paths.vault_path.clone(), + Recipients::Passphrase(passphrase), + )) +} + +/// Build an empty vault for a fresh project. +/// +/// The `identity` section is a placeholder, not a real key: this vault +/// stores template token values for `{{token}}` substitution, not a +/// personal identity, but `frameshift_vault::validate` requires non-empty +/// identity fields regardless of how the vault is used. `handle` is set to +/// the project id so the vault file is self-describing; `keypair_pub` is +/// explicitly `"unset"` rather than a fabricated key, since this backend +/// uses passphrase (not recipient-key) encryption and no age keypair is +/// ever generated or consulted here. +fn empty_vault_data(project_id: &str) -> VaultData { + VaultData { + schema_version: frameshift_vault::MAX_SUPPORTED_SCHEMA_VERSION, + identity: Identity { + keypair_pub: "unset".to_owned(), + handle: project_id.to_owned(), + }, + auth: Auth { + methods: vec!["passphrase".to_owned()], + unlock: "passphrase".to_owned(), + }, + preferences: Preferences { + runtime_mode: RuntimeMode::Rendered, + publish_intent: "no".to_owned(), + recovery: "own-backup".to_owned(), + }, + memory: None, + variables: BTreeMap::new(), + overlays: BTreeMap::new(), + } +} + +/// Execute `vault init` -- create an empty vault, refusing if one exists. +fn run_init(paths: &ProjectPaths) -> Result<(), CliError> { + let backend = open_backend(paths)?; + if backend.exists()? { + return Err(CliError::Vault(format!( + "vault already exists at {}; refusing to overwrite", + paths.vault_path.display() + ))); + } + + let data = empty_vault_data(&paths.project_id); + validate(&data)?; + backend.save(&data)?; + println!("vault initialised at {}", paths.vault_path.display()); + Ok(()) +} + +/// Execute `vault set [--value ]` -- store `value` (prompted, hidden, +/// if not given on the command line) under `key`. +fn run_set(paths: &ProjectPaths, key: &str, value: Option) -> Result<(), CliError> { + let backend = open_backend(paths)?; + let mut data = backend.open()?; + + let value = match value { + Some(value) => value, + None => rpassword::prompt_password(format!("value for '{key}': ")) + .map_err(|e| CliError::Vault(format!("failed to read value: {e}")))?, + }; + + data.set_variable(key.to_owned(), value); + validate(&data)?; + backend.save(&data)?; + println!("set {key}"); + Ok(()) +} + +/// Read a token's raw value from the vault. Separated from [`run_get`] so +/// the value is directly assertable in tests without capturing stdout. +fn get_value(paths: &ProjectPaths, key: &str) -> Result { + let backend = open_backend(paths)?; + let data = backend.open()?; + data.get_variable(key) + .map(str::to_owned) + .ok_or_else(|| CliError::Vault(format!("key '{key}' is not set in the vault"))) +} + +/// Execute `vault get ` -- print the token's raw value to stdout. This +/// is the subcommand's entire purpose: the value goes to stdout unredacted +/// so callers can pipe it (e.g. `frameshift vault get api_key | some-tool`). +fn run_get(paths: &ProjectPaths, key: &str) -> Result<(), CliError> { + println!("{}", get_value(paths, key)?); + Ok(()) +} + +/// Execute `vault rm ` -- remove the token, erroring if it was unset. +fn run_rm(paths: &ProjectPaths, key: &str) -> Result<(), CliError> { + let backend = open_backend(paths)?; + let mut data = backend.open()?; + if data.remove_variable(key).is_none() { + return Err(CliError::Vault(format!( + "key '{key}' is not set in the vault" + ))); + } + backend.save(&data)?; + println!("removed {key}"); + Ok(()) +} + +/// List every token key currently set in the vault, in sorted order. Never +/// returns values. Separated from [`run_list`] so the key set is directly +/// assertable in tests without capturing stdout. +fn list_keys(paths: &ProjectPaths) -> Result, CliError> { + let backend = open_backend(paths)?; + let data = backend.open()?; + Ok(data.variables().keys().cloned().collect()) +} + +/// Execute `vault list` -- print every token key, one per line, never a value. +fn run_list(paths: &ProjectPaths) -> Result<(), CliError> { + for key in list_keys(paths)? { + println!("{key}"); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::path::Path; + use std::sync::Mutex; + + /// Serializes tests in this module that mutate the process-global + /// `FRAMESHIFT_VAULT_PASSPHRASE` env var. Rust's default test harness + /// runs every `#[test]` in this binary on parallel threads, and env vars + /// are process-global; no other test file in this crate reads or writes + /// this specific env var, so serializing within this module is + /// sufficient (mirrors the reasoning documented on + /// `frameshift-client/tests/project_id_env.rs`, which isolates its own + /// env-mutating test into a separate binary instead, since it shares a + /// crate with tests that are not under this module's lock). + static ENV_LOCK: Mutex<()> = Mutex::new(()); + + /// Build a [`ProjectPaths`] rooted at `tmp` without touching a real + /// `Client`/data root -- every field is `pub`, so a fresh literal is the + /// simplest fixture for vault-only tests. + fn test_paths(tmp: &Path) -> ProjectPaths { + ProjectPaths { + project_root: tmp.to_path_buf(), + project_id: "test-project".to_owned(), + config_path: tmp.join("config.toml"), + lock_path: tmp.join("lock.toml"), + vault_path: tmp.join("vault.age"), + cache_dir: tmp.join("cache"), + project_state_dir: tmp.to_path_buf(), + active_path: tmp.join("active"), + personas_dir: tmp.join("personas"), + } + } + + /// Set [`VAULT_PASSPHRASE_ENV`] for the duration of the caller's guard. + fn set_env_passphrase(value: &str) { + std::env::set_var(VAULT_PASSPHRASE_ENV, value); + } + + /// Clear [`VAULT_PASSPHRASE_ENV`]. + fn clear_env_passphrase() { + std::env::remove_var(VAULT_PASSPHRASE_ENV); + } + + /// `init` -> `set` -> `get`/`list` -> `rm` round-trips correctly through + /// the age-encrypted backend when the passphrase comes from the env var. + #[test] + fn round_trip_init_set_get_list_rm_via_env_passphrase() { + let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + set_env_passphrase("hunter2"); + + let tmp = tempfile::tempdir().expect("tempdir"); + let paths = test_paths(tmp.path()); + + run_init(&paths).expect("init"); + run_set(&paths, "greeting", Some("hello".to_owned())).expect("set greeting"); + run_set(&paths, "api_key", Some("secret-value".to_owned())).expect("set api_key"); + + assert_eq!( + get_value(&paths, "greeting").expect("get greeting"), + "hello" + ); + assert_eq!( + get_value(&paths, "api_key").expect("get api_key"), + "secret-value" + ); + + let keys = list_keys(&paths).expect("list"); + assert_eq!(keys, vec!["api_key".to_string(), "greeting".to_string()]); + + run_rm(&paths, "api_key").expect("rm api_key"); + let keys_after_rm = list_keys(&paths).expect("list after rm"); + assert_eq!(keys_after_rm, vec!["greeting".to_string()]); + assert!( + get_value(&paths, "api_key").is_err(), + "api_key must be gone after rm" + ); + + clear_env_passphrase(); + } + + /// `init` on a project that already has a vault refuses to overwrite it. + #[test] + fn init_refuses_when_vault_already_exists() { + let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + set_env_passphrase("hunter2"); + + let tmp = tempfile::tempdir().expect("tempdir"); + let paths = test_paths(tmp.path()); + + run_init(&paths).expect("first init succeeds"); + let err = run_init(&paths).expect_err("second init must refuse"); + assert!( + matches!(err, CliError::Vault(_)), + "expected CliError::Vault, got {err:?}" + ); + + clear_env_passphrase(); + } + + /// `get`/`rm` on a key that was never set name the key in the error. + #[test] + fn get_and_rm_unknown_key_error() { + let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + set_env_passphrase("hunter2"); + + let tmp = tempfile::tempdir().expect("tempdir"); + let paths = test_paths(tmp.path()); + run_init(&paths).expect("init"); + + let get_err = get_value(&paths, "missing").expect_err("get must fail"); + assert!(matches!(get_err, CliError::Vault(msg) if msg.contains("missing"))); + + let rm_err = run_rm(&paths, "missing").expect_err("rm must fail"); + assert!(matches!(rm_err, CliError::Vault(msg) if msg.contains("missing"))); + + clear_env_passphrase(); + } + + /// Opening the vault with the wrong passphrase propagates as + /// `CliError::Vault` (wrapping the underlying `VaultError::Crypto`), not + /// a panic. + #[test] + fn wrong_passphrase_propagates_as_cli_error_not_panic() { + let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + + let tmp = tempfile::tempdir().expect("tempdir"); + let paths = test_paths(tmp.path()); + + set_env_passphrase("correct-horse"); + run_init(&paths).expect("init with correct passphrase"); + + set_env_passphrase("battery-staple"); + let err = get_value(&paths, "anything").expect_err("wrong passphrase must fail, not panic"); + assert!( + matches!(err, CliError::Vault(_)), + "expected CliError::Vault, got {err:?}" + ); + + clear_env_passphrase(); + } +} diff --git a/crates/frameshift-cli/src/main.rs b/crates/frameshift-cli/src/main.rs index 88ca5f9..79f7873 100644 --- a/crates/frameshift-cli/src/main.rs +++ b/crates/frameshift-cli/src/main.rs @@ -16,6 +16,7 @@ use tracing_subscriber::EnvFilter; use frameshift_client::{Client, InstallRequest, InstallSource, PersonaSpec}; use cmd::automate::AutomateArgs; +use cmd::config::ConfigArgs; use cmd::diff::DiffArgs; use cmd::feedback::FeedbackArgs; use cmd::grow::GrowArgs; @@ -29,6 +30,7 @@ use cmd::search::SearchArgs; use cmd::select::SelectArgs; use cmd::skill::{SkillArgs, SkillCommand}; use cmd::use_persona::UseArgs; +use cmd::vault::VaultArgs; use cmd::verify::VerifyArgs; use util::CliError; @@ -134,6 +136,15 @@ enum Command { /// Record a persona selection override for preference learning. Feedback(FeedbackArgs), + + /// Get or set a key in the current project's central config.toml. + Config(ConfigArgs), + + // ------------------------------------------------------------------ + // Vault: {{token}} values for templated packs + // ------------------------------------------------------------------ + /// Manage this project's vault of `{{token}}` values for templated packs. + Vault(VaultArgs), } /// Typed run-level error that carries an exit code alongside a message. @@ -201,12 +212,38 @@ fn main() -> ExitCode { } } -/// Build a `Client` using the default central data root. +/// Build a `Client` using the default central data root, with the CLI's +/// vault provider attached (see [`cli_open_vault`]). +/// +/// The provider is attached unconditionally: it costs nothing for the many +/// subcommands that never render a templated pack, since +/// `frameshift_client::VaultProvider::open_vault` is only invoked when a +/// pack actually ships `pack.template.toml`. /// /// Fails with a `RunError::General` if the data root cannot be determined /// (e.g., `$HOME` is not set). fn make_client() -> Result { - Client::with_default_data_root().map_err(|e| RunError::General(e.to_string())) + Client::with_default_data_root_and_vault(Some(cli_vault_provider())) + .map_err(|e| RunError::General(e.to_string())) +} + +/// Build the CLI's [`frameshift_client::VaultProvider`]: passphrase from +/// `FRAMESHIFT_VAULT_PASSPHRASE`, or (only when stdin is an interactive +/// terminal) a hidden `rpassword` prompt. See [`cmd::vault::resolve_passphrase`], +/// which this delegates to so the `frameshift vault` subcommands and this +/// render-time provider resolve the passphrase identically. +fn cli_vault_provider() -> std::sync::Arc { + std::sync::Arc::new(cli_open_vault) +} + +/// Open the vault at `vault_path` using the CLI's passphrase-resolution +/// policy. Matches the `frameshift_client::VaultProvider` signature so it +/// can be used directly via the blanket `Fn` impl. +fn cli_open_vault( + vault_path: &std::path::Path, +) -> Result { + let passphrase = cmd::vault::resolve_passphrase()?; + frameshift_client::open_vault_with_passphrase(vault_path, passphrase) } /// Execute the parsed subcommand. @@ -457,6 +494,16 @@ fn run() -> Result<(), RunError> { let client = make_client()?; cmd::feedback::run_feedback(&client, args).map_err(RunError::from) } + + Command::Config(args) => { + let client = make_client()?; + cmd::config::run_config(&client, args).map_err(RunError::from) + } + + Command::Vault(args) => { + let client = make_client()?; + cmd::vault::run_vault(&client, args).map_err(RunError::from) + } } } diff --git a/crates/frameshift-cli/src/util.rs b/crates/frameshift-cli/src/util.rs index cb8ff5e..54a68de 100644 --- a/crates/frameshift-cli/src/util.rs +++ b/crates/frameshift-cli/src/util.rs @@ -95,6 +95,32 @@ pub enum CliError { /// Publish/register flow error (missing argument, registry rejection hint). #[error("{0}")] Publish(String), + + /// `frameshift config` error: unknown key or a value that fails to parse + /// for the requested key's type. + #[error("{0}")] + Config(String), + + /// `frameshift vault` error: a `frameshift-vault`/`frameshift-vault-local` + /// failure (open/save/validate), or a CLI-level vault condition (e.g. + /// `init` refusing to overwrite an existing vault) that has no direct + /// `VaultError` variant of its own. + #[error("vault error: {0}")] + Vault(String), +} + +/// Stringifies a [`frameshift_vault::VaultError`] into [`CliError::Vault`] +/// rather than deriving `#[from]` directly on the variant, so `CliError` +/// does not need to name `frameshift_vault::VaultError` in its own type +/// (mirrors `impl From for ClientError` in +/// `frameshift-client/src/error.rs`, which boxes for a different reason but +/// follows the same "manual `From`, not `#[from]`" shape). +impl From for CliError { + /// Convert a `VaultError` into `CliError::Vault` by rendering its + /// `Display` message. + fn from(e: frameshift_vault::VaultError) -> Self { + CliError::Vault(e.to_string()) + } } /// Validate that a registry server URL uses an `http`/`https` scheme. @@ -378,6 +404,7 @@ mod tests { let client = Client::new(ClientOptions { data_root: data_root.clone(), config_root: None, + vault: None, }); // Load path: persona_source_dir must reject the symlink. diff --git a/crates/frameshift-client/Cargo.toml b/crates/frameshift-client/Cargo.toml index bdd7e9f..c3b8126 100644 --- a/crates/frameshift-client/Cargo.toml +++ b/crates/frameshift-client/Cargo.toml @@ -16,9 +16,23 @@ frameshift-source = { path = "../frameshift-source" } # path (RegressionGate::evaluate_cross_version); default features only, no # cli-runner (no `agy`/tokio-process dependency needed here). frameshift-conformance = { path = "../frameshift-conformance" } +# Template manifest/placeholder parsing for `{{token}}` substitution in +# templated packs (those shipping `pack.template.toml`). +frameshift-template = { path = "../frameshift-template" } +# Vault schema/error/backend-trait types (`VaultData`, `VaultError`, +# `VaultBackend`, `validate`) consumed by the `VaultProvider` contract. +frameshift-vault = { path = "../frameshift-vault" } +# Ready-made `VaultProvider` constructors (`open_vault_with_passphrase`, +# `env_only_vault_provider`) open the project vault through the age-encrypted +# local backend; kept here (rather than pushed out to every caller) so the +# CLI, daemon, and MCP server share one passphrase-to-`VaultData` path. +frameshift-vault-local = { path = "../frameshift-vault-local" } base64.workspace = true ed25519-dalek.workspace = true hex.workspace = true +# Wraps the vault passphrase so it is never accidentally `Debug`-printed or +# logged; matches how frameshift-vault-local's own recipients are typed. +secrecy.workspace = true serde.workspace = true # JSON request bodies (author registration) and publish response parsing. serde_json.workspace = true diff --git a/crates/frameshift-client/src/error.rs b/crates/frameshift-client/src/error.rs index c3d34f3..2bf1f41 100644 --- a/crates/frameshift-client/src/error.rs +++ b/crates/frameshift-client/src/error.rs @@ -190,6 +190,64 @@ pub enum ClientError { /// The central project config.toml that would declare the adapter. config_path: PathBuf, }, + + /// A templated pack (one shipping a `pack.template.toml` manifest, see + /// `frameshift_template::TemplateManifest`) declares one or more + /// `required = true` tokens that have no value available. This fires in + /// three situations that all boil down to "no usable value exists": + /// no [`crate::VaultProvider`] was configured on the `Client`, the + /// project's vault file does not exist yet, or the vault exists but is + /// missing one or more of the required values. Every missing token is + /// named, not just the first, so a single failure gives the complete + /// remediation list. This check runs before any render output is + /// written, so a failure here never leaves a persona's `rendered/` + /// directory partially updated. + #[error( + "persona {persona:?} requires vault token(s) {tokens:?} but they have no value in the \ + vault at {vault_path}; run `frameshift vault init` (if the vault does not exist yet) \ + then `frameshift vault set ` for each listed token" + )] + MissingRequiredTokens { + /// The persona whose template render was blocked. + persona: String, + /// The project's vault file path (see `ProjectPaths::vault_path`). + vault_path: PathBuf, + /// Every required token name with no value, in sorted (`BTreeMap`) order. + tokens: Vec, + }, + + /// A configured [`crate::VaultProvider`] failed to open the project + /// vault for a reason other than "the file does not exist" -- a wrong + /// passphrase, corrupt ciphertext, or an unsupported schema version, for + /// example. Wraps the underlying `VaultError` unchanged so the real + /// cause reaches the caller instead of a generic message. + /// The error is boxed to keep `ClientError` small; an inline + /// `VaultError` trips `clippy::result_large_err` on every function that + /// returns `Result<_, ClientError>` (same treatment as `Compose`). + #[error("failed to open vault at {vault_path}: {source}")] + VaultOpen { + /// The vault file path that failed to open. + vault_path: PathBuf, + /// The underlying vault-backend error. + #[source] + source: Box, + }, + + /// A templated pack's `pack.template.toml` manifest, or its rendered + /// markdown once vault values are substituted in, failed to parse as a + /// `frameshift_template` document. `context` names what was being + /// parsed (the manifest path, or the persona/target being rendered) so + /// the error is actionable without a second lookup. + /// The error is boxed to keep `ClientError` small, mirroring `Compose` + /// and `VaultOpen`. + #[error("template parse error ({context}): {source}")] + Template { + /// Human-readable description of what was being parsed. + context: String, + /// The underlying template parse error. + #[source] + source: Box, + }, } /// Box the composition error so `ClientError` stays small while `?` on a bare diff --git a/crates/frameshift-client/src/lib.rs b/crates/frameshift-client/src/lib.rs index e5b28fd..461d7ef 100644 --- a/crates/frameshift-client/src/lib.rs +++ b/crates/frameshift-client/src/lib.rs @@ -14,6 +14,11 @@ pub use error::ClientError; /// on the decision variants without adding their own `frameshift-conformance` /// dependency just to name the type. pub use frameshift_conformance::CrossVersionDecision; +/// Re-exported vault schema/error types so callers implementing +/// [`VaultProvider`] (the CLI, daemon, MCP server) do not need their own +/// direct `frameshift-vault` dependency just to name `VaultData`/`VaultError` +/// in a provider function's signature. +pub use frameshift_vault::{VaultData, VaultError}; pub use model::{ ClientOptions, GcReport, InstallReport, InstallRequest, InstallSource, LockedPersona, Lockfile, MemoryConfig, MemoryRequirementStatus, PersonaSpec, ProjectConfig, ProjectPaths, SyncReport, @@ -28,10 +33,14 @@ pub use selection::{ use base64::{engine::general_purpose, Engine as _}; use ed25519_dalek::VerifyingKey; use frameshift_pack::Pack; +use frameshift_vault::VaultBackend as _; +use frameshift_vault_local::{LocalAgeBackend, Recipients}; +use secrecy::SecretString; use sha2::{Digest, Sha256}; use std::collections::BTreeSet; use std::fs; use std::path::{Path, PathBuf}; +use std::sync::Arc; use tracing::{debug, info, warn}; /// Legacy filename written to the project root by pre-WS-1 versions. @@ -43,9 +52,19 @@ const LEGACY_LOCK_FILENAME: &str = "frameshift.lock"; const CENTRAL_CONFIG_FILENAME: &str = "config.toml"; /// Canonical lock filename inside the central store: `projects//lock.toml`. const CENTRAL_LOCK_FILENAME: &str = "lock.toml"; +/// Canonical vault filename inside the central store: `projects//vault.age`. +const CENTRAL_VAULT_FILENAME: &str = "vault.age"; const ACTIVE_FILENAME: &str = "active"; /// Env var to override the auto-derived path-hash project_id. const PROJECT_ID_ENV: &str = "FRAMESHIFT_PROJECT_ID"; +/// Env var consulted by [`env_only_vault_provider`] and (as a first choice, +/// before any interactive fallback) by the `frameshift` CLI's own provider. +pub const VAULT_PASSPHRASE_ENV: &str = "FRAMESHIFT_VAULT_PASSPHRASE"; +/// Filename of a templated pack's companion token/section manifest. Its +/// presence in a pack's cache directory is the sole gate for token +/// substitution: packs that do not ship this file render exactly as they +/// did before this feature existed, with no vault lookup performed at all. +const PACK_TEMPLATE_MANIFEST_FILENAME: &str = "pack.template.toml"; const RENDER_TARGETS: [(&str, &str); 4] = [ ("claude", "CLAUDE.md"), @@ -62,6 +81,107 @@ pub struct Client { data_root: PathBuf, /// Root of the XDG config directory (for infrastructure overlay). config_root: Option, + /// Optional vault-data provider used to substitute `{{token}}` + /// placeholders when materializing a templated pack's render output. + /// See [`VaultProvider`]'s never-prompts contract. + vault: Option>, +} + +// ── Vault provider ────────────────────────────────────────────────────────── + +/// Supplies decrypted vault data for `{{token}}` substitution during +/// template rendering. +/// +/// # Never-prompts contract +/// +/// Implementations of this trait MUST NOT read from stdin or otherwise block +/// on interactive input. The client library itself never prompts for a +/// vault passphrase; any interactive prompting must happen in the caller +/// (e.g. the `frameshift` CLI's own provider) *before* the passphrase is +/// captured into the closure/impl passed here, so that `open_vault` itself +/// is always a non-interactive operation from this crate's point of view. +pub trait VaultProvider: Send + Sync { + /// Open and decrypt the vault at `vault_path`, returning its contents. + /// + /// # Errors + /// + /// Returns whatever [`VaultError`] the underlying backend produces -- + /// typically [`VaultError::Io`] for a missing file, [`VaultError::Crypto`] + /// for a wrong passphrase, or [`VaultError::BackendUnavailable`] when no + /// passphrase could be resolved (e.g. non-interactive with no env var set). + fn open_vault(&self, vault_path: &Path) -> Result; +} + +/// Blanket impl so any `Fn(&Path) -> Result + Send + +/// Sync` (a plain function item or a non-capturing closure) can be used +/// directly as a [`VaultProvider`] without a dedicated wrapper type. +impl VaultProvider for F +where + F: Fn(&Path) -> Result + Send + Sync, +{ + /// Delegates to the wrapped callable. + fn open_vault(&self, vault_path: &Path) -> Result { + self(vault_path) + } +} + +/// Open the age-encrypted vault at `vault_path` using a passphrase recipient, +/// then run [`frameshift_vault::validate`] on the decrypted contents. +/// +/// This is the one place `Client` (and its callers) talk to +/// `frameshift-vault-local::LocalAgeBackend`; every ready-made provider below +/// and the `frameshift` CLI's own interactive provider funnel through here so +/// the backend choice and the post-open validation pass stay in one spot. +/// +/// # Errors +/// +/// Returns whatever [`VaultError`] `LocalAgeBackend::open` or +/// [`frameshift_vault::validate`] produces. +pub fn open_vault_with_passphrase( + vault_path: &Path, + passphrase: SecretString, +) -> Result { + let backend = + LocalAgeBackend::new(vault_path.to_path_buf(), Recipients::Passphrase(passphrase)); + let data = backend.open()?; + frameshift_vault::validate(&data)?; + Ok(data) +} + +/// Resolve the vault passphrase strictly from [`VAULT_PASSPHRASE_ENV`], +/// never touching stdin. +/// +/// # Errors +/// +/// Returns [`VaultError::BackendUnavailable`] when the env var is unset or +/// empty. +fn env_vault_passphrase() -> Result { + match std::env::var(VAULT_PASSPHRASE_ENV) { + Ok(value) if !value.is_empty() => Ok(SecretString::new(value)), + _ => Err(VaultError::BackendUnavailable(format!( + "vault passphrase not available: set {VAULT_PASSPHRASE_ENV}" + ))), + } +} + +/// [`VaultProvider`] implementation used by [`env_only_vault_provider`]. +/// A plain function item (captures nothing), so it satisfies the blanket +/// `VaultProvider` impl directly. +fn env_only_open_vault(vault_path: &Path) -> Result { + open_vault_with_passphrase(vault_path, env_vault_passphrase()?) +} + +/// Build a [`VaultProvider`] that resolves the passphrase strictly from +/// [`VAULT_PASSPHRASE_ENV`] and never touches stdin. +/// +/// Intended for daemon/background-service callers (`frameshift-daemon`, +/// `frameshift-mcp`) where there is no interactive terminal attached and a +/// blocking stdin read would hang the service (or, for the MCP server, +/// corrupt the stdin/stdout JSON-RPC protocol stream). When the env var is +/// unset, the returned provider fails with [`VaultError::BackendUnavailable`] +/// rather than blocking or falling back to an unauthenticated vault open. +pub fn env_only_vault_provider() -> Arc { + Arc::new(env_only_open_vault) } /// Inherent methods implementing the Frameshift client's public API surface. @@ -71,14 +191,31 @@ impl Client { Self { data_root: options.data_root, config_root: options.config_root, + vault: options.vault, } } - /// Construct a `Client` using the XDG data and config roots resolved from environment variables. + /// Construct a `Client` using the XDG data and config roots resolved from + /// environment variables, with no vault provider configured. Templated + /// packs (those shipping `pack.template.toml`) will fail to render with + /// [`ClientError::MissingRequiredTokens`] under a `Client` built this + /// way; callers that need vault-backed token substitution should use + /// [`Client::with_default_data_root_and_vault`] instead. pub fn with_default_data_root() -> Result { + Self::with_default_data_root_and_vault(None) + } + + /// Construct a `Client` using the XDG data and config roots resolved from + /// environment variables, attaching `vault` as the render-time + /// [`VaultProvider`]. Pass `None` for callers that never render templated + /// packs (equivalent to [`Client::with_default_data_root`]). + pub fn with_default_data_root_and_vault( + vault: Option>, + ) -> Result { Ok(Self::new(ClientOptions { data_root: default_data_root()?, config_root: Some(default_config_root()?), + vault, })) } @@ -184,6 +321,7 @@ impl Client { project_id, config_path: project_state_dir.join(CENTRAL_CONFIG_FILENAME), lock_path: project_state_dir.join(CENTRAL_LOCK_FILENAME), + vault_path: project_state_dir.join(CENTRAL_VAULT_FILENAME), cache_dir, active_path: project_state_dir.join(ACTIVE_FILENAME), personas_dir, @@ -370,6 +508,24 @@ impl Client { } } + /// Write `config` to the central project config (`projects//config.toml`), + /// creating the containing directory if it does not exist yet. + /// + /// This is the write-side counterpart to [`Client::project_config`]. Used by + /// the `frameshift config set` CLI subcommand and any other caller that + /// needs to persist a `ProjectConfig` change (e.g. toggling + /// `telemetry_opt_in`) rather than only reading it. + pub fn save_project_config( + &self, + project_root: &Path, + config: &ProjectConfig, + ) -> Result<(), ClientError> { + let paths = self.project_paths(project_root)?; + ensure_dir(&paths.project_state_dir)?; + let raw = toml::to_string_pretty(config)?; + write_file(&paths.config_path, raw.as_bytes()) + } + /// Append a persona-selection event to the project's local selection history /// (`projects//selection-history.jsonl`). Local-only, write-only audit /// log: it is never sent anywhere and nothing in this codebase reads it back. @@ -718,6 +874,7 @@ impl Client { &paths.cache_dir, &cache_path, &rendered_root, + &paths.vault_path, &persona.name, lockfile, )?; @@ -756,11 +913,20 @@ impl Client { /// - `extends`/`mixin` declared but no `persona.toml`: warns and falls /// back to the markdown-only render path, since there is no typed /// source for the composer to operate on. + /// + /// Independently of which of the three paths above is taken: if the pack + /// at `cache_path` ships a `pack.template.toml` manifest, every render + /// target's markdown is additionally passed through `{{token}}` + /// substitution (see [`load_template_context`] / [`substitute_tokens`]) + /// before being written. The vault is opened at most once per call + /// (not once per render target). Packs that ship no such manifest render + /// byte-identically to how they did before this feature existed. fn materialize_persona_rendered_outputs( &self, cache_dir: &Path, cache_path: &Path, rendered_root: &Path, + vault_path: &Path, persona_name: &str, lockfile: &Lockfile, ) -> Result<(), ClientError> { @@ -779,6 +945,12 @@ impl Client { let has_composition = manifest.extends.is_some() || !manifest.mixin.is_empty(); let has_typed_source = cache_path.join("persona.toml").is_file(); + // Loaded once regardless of which render branch runs below, so a + // templated pack opens its vault a single time per materialize call + // rather than once per render target. + let template_ctx = + load_template_context(cache_path, vault_path, self.vault.as_ref(), persona_name)?; + if has_composition && has_typed_source { let root = frameshift_source::PersonaSource::load_from_dir(cache_path) .map_err(frameshift_compose::ComposeError::from)?; @@ -818,9 +990,13 @@ impl Client { let markdown = frameshift_source::render_to_markdown(&src, target); let composed_content = compose_rendered_content(persona_name, &markdown, self.config_root.as_deref()); + let context = + format!("rendered markdown for persona {persona_name:?} (target {target_dir})"); + let final_content = + substitute_tokens(&composed_content, &context, template_ctx.as_ref())?; let dir = rendered_root.join(target_dir); ensure_dir(&dir)?; - write_file(&dir.join(filename), composed_content.as_bytes())?; + write_file(&dir.join(filename), final_content.as_bytes())?; } return Ok(()); @@ -838,6 +1014,7 @@ impl Client { rendered_root, persona_name, self.config_root.as_deref(), + template_ctx.as_ref(), ) } } @@ -1191,6 +1368,7 @@ fn materialize_rendered_outputs( rendered_root: &Path, persona_name: &str, config_root: Option<&Path>, + template_ctx: Option<&(frameshift_template::TemplateManifest, VaultData)>, ) -> Result<(), ClientError> { let render_source = find_render_source(cache_path)?; let persona_content = fs::read_to_string(&render_source).map_err(|source| ClientError::Io { @@ -1199,16 +1377,155 @@ fn materialize_rendered_outputs( })?; let composed = compose_rendered_content(persona_name, &persona_content, config_root); + let final_content = substitute_tokens( + &composed, + &format!("rendered markdown for persona {persona_name:?}"), + template_ctx, + )?; for (target_dir, filename) in RENDER_TARGETS { let dir = rendered_root.join(target_dir); ensure_dir(&dir)?; - write_file(&dir.join(filename), composed.as_bytes())?; + write_file(&dir.join(filename), final_content.as_bytes())?; } Ok(()) } +/// Load the token-substitution context for a templated pack, or `None` when +/// the pack at `cache_path` does not ship +/// [`PACK_TEMPLATE_MANIFEST_FILENAME`] -- the byte-identical-rendering fast +/// path used by every pack today, and asserted by +/// `pack_without_manifest_renders_byte_identically_regardless_of_vault` in +/// this module's tests. +/// +/// When the manifest *is* present, this opens the project vault via +/// `vault_provider` and checks that every `required = true` token declared +/// in the manifest has a value, failing before any render output is written +/// with [`ClientError::MissingRequiredTokens`] naming every missing token, +/// not just the first. +/// +/// # Errors +/// +/// - [`ClientError::Io`]: the manifest file exists but could not be read. +/// - [`ClientError::Template`]: the manifest could not be parsed as TOML. +/// - [`ClientError::MissingRequiredTokens`]: no vault provider was +/// configured, the vault file does not exist yet, or the vault exists but +/// is missing one or more required token values. +/// - [`ClientError::VaultOpen`]: the vault provider returned some other +/// failure while opening the vault (e.g. a wrong passphrase). +fn load_template_context( + cache_path: &Path, + vault_path: &Path, + vault_provider: Option<&Arc>, + persona_name: &str, +) -> Result, ClientError> { + let manifest_path = cache_path.join(PACK_TEMPLATE_MANIFEST_FILENAME); + if !manifest_path.is_file() { + return Ok(None); + } + + let manifest_raw = fs::read_to_string(&manifest_path).map_err(|source| ClientError::Io { + path: manifest_path.clone(), + source, + })?; + let manifest = + frameshift_template::TemplateManifest::from_toml(&manifest_raw).map_err(|source| { + ClientError::Template { + context: format!("manifest {}", manifest_path.display()), + source: Box::new(source), + } + })?; + + // Local helper: every `required = true` token name, in sorted order. + let required_tokens = |manifest: &frameshift_template::TemplateManifest| -> Vec { + manifest + .tokens + .iter() + .filter(|(_, decl)| decl.required) + .map(|(name, _)| name.clone()) + .collect() + }; + + let Some(provider) = vault_provider else { + return Err(ClientError::MissingRequiredTokens { + persona: persona_name.to_owned(), + vault_path: vault_path.to_path_buf(), + tokens: required_tokens(&manifest), + }); + }; + + let vault = match provider.open_vault(vault_path) { + Ok(vault) => vault, + Err(VaultError::Io { source, .. }) if source.kind() == std::io::ErrorKind::NotFound => { + return Err(ClientError::MissingRequiredTokens { + persona: persona_name.to_owned(), + vault_path: vault_path.to_path_buf(), + tokens: required_tokens(&manifest), + }); + } + Err(source) => { + return Err(ClientError::VaultOpen { + vault_path: vault_path.to_path_buf(), + source: Box::new(source), + }); + } + }; + + let missing: Vec = manifest + .tokens + .iter() + .filter(|(name, decl)| decl.required && vault.get_variable(name).is_none()) + .map(|(name, _)| name.clone()) + .collect(); + if !missing.is_empty() { + return Err(ClientError::MissingRequiredTokens { + persona: persona_name.to_owned(), + vault_path: vault_path.to_path_buf(), + tokens: missing, + }); + } + + Ok(Some((manifest, vault))) +} + +/// Apply `{{token}}` substitution to `content` using an already-loaded +/// template context, or return `content` unchanged (byte-identical, no +/// alloc-and-compare needed since this is a plain clone) when `template_ctx` +/// is `None`. +/// +/// Optional (`required = false`) tokens absent from the vault are left as +/// `{{name}}` in the output: `frameshift_template::TokenDecl` has no +/// default-value field, so there is nothing to substitute, and this matches +/// `frameshift_runtime::Runtime::render`'s existing behavior (missing tokens +/// stay visible rather than becoming empty strings). +/// +/// `context` is a human-readable description of what `content` is (used +/// only in the [`ClientError::Template`] error message on a parse failure). +/// +/// # Errors +/// +/// Returns [`ClientError::Template`] if `content` cannot be parsed as a +/// [`frameshift_template::Template`] (unclosed section, invalid token name, +/// etc.). +fn substitute_tokens( + content: &str, + context: &str, + template_ctx: Option<&(frameshift_template::TemplateManifest, VaultData)>, +) -> Result { + let Some((_, vault)) = template_ctx else { + return Ok(content.to_owned()); + }; + + let template = + frameshift_template::Template::parse(content).map_err(|source| ClientError::Template { + context: context.to_owned(), + source: Box::new(source), + })?; + + Ok(template.render(vault.variables(), vault.overlays())) +} + /// Compose the final rendered content from infrastructure overlay + persona context header + persona content. /// If no infrastructure overlay exists, returns persona content unchanged. fn compose_rendered_content( @@ -1651,6 +1968,7 @@ mod tests { let client = Client::new(ClientOptions { data_root: tmp.path().join("data"), config_root: None, + vault: None, }); client @@ -1749,6 +2067,7 @@ mod tests { let client = Client::new(ClientOptions { data_root: tmp.path().join("data"), config_root: None, + vault: None, }); assert!(client.list_personas(&project_root).unwrap().is_empty()); } @@ -1788,6 +2107,7 @@ mod tests { let client = Client::new(ClientOptions { data_root: tmp.path().join("data"), config_root: None, + vault: None, }); let dirs = client.installed_persona_source_dirs(&project_root).unwrap(); assert!(dirs.is_empty()); @@ -1829,6 +2149,7 @@ mod tests { let client = Client::new(ClientOptions { data_root: tmp.path().join("data"), config_root: None, + vault: None, }); let err = client .rendered_persona(&project_root, "ghost", "claude") @@ -1909,6 +2230,7 @@ mod tests { let client = Client::new(ClientOptions { data_root: data_root.clone(), config_root: Some(config_root), + vault: None, }); client .install(InstallRequest { @@ -1973,6 +2295,7 @@ mod tests { let client = Client::new(ClientOptions { data_root: data_root.clone(), config_root: None, + vault: None, }); client .install(InstallRequest { @@ -2070,6 +2393,7 @@ mod tests { let client = Client::new(ClientOptions { data_root: tmp.path().join("data"), config_root: None, + vault: None, }); let report = client @@ -2096,6 +2420,7 @@ mod tests { let client = Client::new(ClientOptions { data_root: tmp.path().join("data"), config_root: None, + vault: None, }); let old_pack_dir = tmp.path().join("old-pack"); @@ -2143,6 +2468,7 @@ mod tests { let client = Client::new(ClientOptions { data_root: tmp.path().join("data"), config_root: None, + vault: None, }); let old_pack_dir = tmp.path().join("old-pack"); @@ -2191,6 +2517,7 @@ mod tests { let client = Client::new(ClientOptions { data_root: tmp.path().join("data"), config_root: None, + vault: None, }); let old_pack_dir = tmp.path().join("old-pack"); diff --git a/crates/frameshift-client/src/model.rs b/crates/frameshift-client/src/model.rs index b4900d2..704c508 100644 --- a/crates/frameshift-client/src/model.rs +++ b/crates/frameshift-client/src/model.rs @@ -1,5 +1,6 @@ use serde::{Deserialize, Serialize}; use std::path::PathBuf; +use std::sync::Arc; pub const SCHEMA_VERSION: u32 = 1; @@ -169,7 +170,12 @@ pub struct InstallRequest { } /// Options for constructing a Frameshift `Client`. -#[derive(Debug, Clone, PartialEq, Eq)] +/// +/// `Debug`/`PartialEq`/`Eq` are implemented manually (rather than derived) +/// because `vault` holds a trait object: [`crate::VaultProvider`] +/// implementations are not generally `Debug` or comparable by value. See +/// the manual impls below for exactly what each derives to. +#[derive(Clone)] pub struct ClientOptions { /// Root of the Frameshift data directory (e.g. ~/.local/share/frameshift). pub data_root: PathBuf, @@ -177,8 +183,54 @@ pub struct ClientOptions { /// When set, the engine looks for `frameshift/infrastructure.md` /// under this path and composes it into rendered output. pub config_root: Option, + /// Optional supplier of decrypted vault data, used to substitute + /// `{{token}}` placeholders when materializing a templated pack (one + /// that ships `pack.template.toml`). `None` means templated packs fail + /// render with [`crate::ClientError::MissingRequiredTokens`] rather than + /// silently leaving `{{token}}` placeholders unsubstituted. + /// + /// # Never-prompts contract + /// + /// The client library itself never prompts for a vault passphrase. + /// Implementations of [`crate::VaultProvider`] passed here MUST NOT + /// block on interactive input (stdin reads, TTY prompts, etc.); any + /// interactive passphrase prompting belongs in the caller (e.g. the + /// `frameshift` CLI's `make_client`), performed before the passphrase is + /// captured into the provider closure/impl. + pub vault: Option>, +} + +impl std::fmt::Debug for ClientOptions { + /// Prints whether a vault provider is configured, without attempting to + /// format the trait object itself (arbitrary `VaultProvider` impls are + /// not required to be `Debug`). + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("ClientOptions") + .field("data_root", &self.data_root) + .field("config_root", &self.config_root) + .field("vault_configured", &self.vault.is_some()) + .finish() + } +} + +impl PartialEq for ClientOptions { + /// Compares `data_root`/`config_root` structurally and `vault` by + /// `Arc` pointer identity (`Arc::ptr_eq`) -- trait objects have no + /// general notion of value equality, but pointer identity is still a + /// well-defined equivalence relation, so this remains a lawful `Eq`. + fn eq(&self, other: &Self) -> bool { + self.data_root == other.data_root + && self.config_root == other.config_root + && match (&self.vault, &other.vault) { + (None, None) => true, + (Some(a), Some(b)) => Arc::ptr_eq(a, b), + _ => false, + } + } } +impl Eq for ClientOptions {} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct ProjectPaths { pub project_root: PathBuf, @@ -188,6 +240,11 @@ pub struct ProjectPaths { /// Central-store lock path: `$XDG_DATA_HOME/frameshift/projects//lock.toml`. /// This is the canonical lock location -- nothing is written to the project root. pub lock_path: PathBuf, + /// Central-store vault path: `$XDG_DATA_HOME/frameshift/projects//vault.age`. + /// Sibling of `config_path`. Holds this project's `{{token}}` values for + /// templated packs, age-encrypted via `frameshift-vault-local`. Nothing + /// is written to the project root. + pub vault_path: PathBuf, pub cache_dir: PathBuf, pub project_state_dir: PathBuf, pub active_path: PathBuf, diff --git a/crates/frameshift-client/tests/compose_render.rs b/crates/frameshift-client/tests/compose_render.rs index 17afd42..87f4eb7 100644 --- a/crates/frameshift-client/tests/compose_render.rs +++ b/crates/frameshift-client/tests/compose_render.rs @@ -53,6 +53,7 @@ fn install_composes_extends_base() { let client = Client::new(ClientOptions { data_root: data_root.clone(), config_root: None, + vault: None, }); // Base pack: typed source with one L1 rule, no composition of its own. @@ -143,6 +144,7 @@ fn install_fails_when_extends_missing() { let client = Client::new(ClientOptions { data_root: data_root.clone(), config_root: None, + vault: None, }); let child_dir = temp.path().join("child-pack"); @@ -194,6 +196,7 @@ fn mixin_l1_override_fails_install() { let client = Client::new(ClientOptions { data_root: data_root.clone(), config_root: None, + vault: None, }); // Base pack owns L1 rule "no-panic". diff --git a/crates/frameshift-client/tests/install_flow.rs b/crates/frameshift-client/tests/install_flow.rs index d8bf8a3..aebba87 100644 --- a/crates/frameshift-client/tests/install_flow.rs +++ b/crates/frameshift-client/tests/install_flow.rs @@ -33,6 +33,7 @@ version = "0.3.1" let client = Client::new(ClientOptions { data_root: data_root.clone(), config_root: None, + vault: None, }); let report = client @@ -121,6 +122,7 @@ fn migrates_legacy_project_files() { let client = Client::new(ClientOptions { data_root: data_root.clone(), config_root: None, + vault: None, }); // project_paths() triggers the migration shim. @@ -169,6 +171,7 @@ version = "0.3.1" let client = Client::new(ClientOptions { data_root: data_root.clone(), config_root: None, + vault: None, }); let report = client .install(InstallRequest { diff --git a/crates/frameshift-client/tests/memory_requirement.rs b/crates/frameshift-client/tests/memory_requirement.rs index dc14d50..e7240e2 100644 --- a/crates/frameshift-client/tests/memory_requirement.rs +++ b/crates/frameshift-client/tests/memory_requirement.rs @@ -68,6 +68,7 @@ fn hard_requirement_without_adapter_blocks_activation() { let client = Client::new(ClientOptions { data_root: data_root.clone(), config_root: None, + vault: None, }); install(&client, &project_root, &pack_root, "archivist"); @@ -101,6 +102,7 @@ fn hard_requirement_with_adapter_activates() { let client = Client::new(ClientOptions { data_root: data_root.clone(), config_root: None, + vault: None, }); install(&client, &project_root, &pack_root, "archivist"); declare_memory(&client, &data_root, &project_root); @@ -123,6 +125,7 @@ fn soft_requirement_activates_and_reports_unmet() { let client = Client::new(ClientOptions { data_root, config_root: None, + vault: None, }); install(&client, &project_root, &pack_root, "coach"); @@ -161,6 +164,7 @@ version = "0.1.0" let client = Client::new(ClientOptions { data_root, config_root: None, + vault: None, }); install(&client, &project_root, &pack_root, "plain"); diff --git a/crates/frameshift-client/tests/project_id_env.rs b/crates/frameshift-client/tests/project_id_env.rs index 9231837..83ee21e 100644 --- a/crates/frameshift-client/tests/project_id_env.rs +++ b/crates/frameshift-client/tests/project_id_env.rs @@ -17,6 +17,7 @@ fn project_id_env_override_is_used_verbatim() { let client = Client::new(ClientOptions { data_root, config_root: None, + vault: None, }); let result = client.project_id(&project_root); std::env::remove_var("FRAMESHIFT_PROJECT_ID"); diff --git a/crates/frameshift-client/tests/registry_install.rs b/crates/frameshift-client/tests/registry_install.rs index fc247cf..e198ce2 100644 --- a/crates/frameshift-client/tests/registry_install.rs +++ b/crates/frameshift-client/tests/registry_install.rs @@ -250,6 +250,7 @@ fn test_client_and_project(temp: &tempfile::TempDir) -> (Client, std::path::Path let client = Client::new(ClientOptions { data_root, config_root: None, + vault: None, }); (client, project_root) } diff --git a/crates/frameshift-client/tests/template_render.rs b/crates/frameshift-client/tests/template_render.rs new file mode 100644 index 0000000..7e92a25 --- /dev/null +++ b/crates/frameshift-client/tests/template_render.rs @@ -0,0 +1,312 @@ +//! Integration tests for `{{token}}` substitution at render time -- the +//! `pack.template.toml` + [`frameshift_client::VaultProvider`] pipeline +//! wired into `Client::materialize_persona_rendered_outputs` / +//! `materialize_rendered_outputs`. +//! +//! The load-bearing invariant under test: a pack that ships no +//! `pack.template.toml` renders byte-identically to how it did before this +//! feature existed, regardless of whether a vault provider is configured. + +use frameshift_client::{ + Client, ClientError, ClientOptions, InstallRequest, InstallSource, PersonaSpec, VaultData, + VaultProvider, +}; +use frameshift_vault::{Auth, Identity, Preferences, RuntimeMode}; +use std::collections::BTreeMap; +use std::fs; +use std::path::Path; +use std::sync::Arc; +use tempfile::TempDir; + +/// Writes `pack.toml` (and any extra plain files) into `dir`. +fn write_pack_manifest(dir: &Path, manifest_toml: &str, extra_files: &[(&str, &str)]) { + fs::create_dir_all(dir).expect("create pack dir"); + fs::write(dir.join("pack.toml"), manifest_toml).expect("write pack.toml"); + for (relative, content) in extra_files { + let path = dir.join(relative); + if let Some(parent) = path.parent() { + fs::create_dir_all(parent).expect("create parent"); + } + fs::write(path, content).expect("write extra file"); + } +} + +/// Builds a minimal, schema-valid [`VaultData`] whose `variables` map is +/// `vars`. The identity/auth/preferences sections are placeholders -- these +/// tests only exercise the `variables` map that `{{token}}` substitution +/// reads from. +fn vault_with_variables(vars: &[(&str, &str)]) -> VaultData { + VaultData { + schema_version: 1, + identity: Identity { + keypair_pub: "age1test".to_owned(), + handle: "tester".to_owned(), + }, + auth: Auth { + methods: vec!["passphrase".to_owned()], + unlock: "passphrase".to_owned(), + }, + preferences: Preferences { + runtime_mode: RuntimeMode::Rendered, + publish_intent: "no".to_owned(), + recovery: "own-backup".to_owned(), + }, + memory: None, + variables: vars + .iter() + .map(|(k, v)| ((*k).to_owned(), (*v).to_owned())) + .collect(), + overlays: BTreeMap::new(), + } +} + +/// A [`VaultProvider`] that always returns a clone of `data`, ignoring the +/// requested path -- sufficient for tests with a single vault. +fn fixed_vault_provider(data: VaultData) -> Arc { + Arc::new( + move |_path: &Path| -> Result { + Ok(data.clone()) + }, + ) +} + +/// A minimal, schema-valid `pack.toml` shared by every test in this file +/// (name/version are overwritten per test via string formatting where needed). +const PACK_TOML: &str = r#" +schema_version = 1 +name = "templated" +author_handle = "alice" +author_pubkey = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef" +version = "0.1.0" +"#; + +/// A pack that ships `pack.template.toml` declaring a required token, paired +/// with a vault that supplies it, substitutes the token into every render +/// target's output file. +#[test] +fn templated_pack_substitutes_required_token_in_every_target() { + let temp = TempDir::new().expect("tempdir"); + let data_root = temp.path().join("data-root"); + let project_root = temp.path().join("project"); + fs::create_dir_all(&project_root).expect("create project"); + + let vault = vault_with_variables(&[("greeting_name", "Ada")]); + let client = Client::new(ClientOptions { + data_root: data_root.clone(), + config_root: None, + vault: Some(fixed_vault_provider(vault)), + }); + + let pack_dir = temp.path().join("templated-pack"); + write_pack_manifest( + &pack_dir, + PACK_TOML, + &[ + ("AGENTS.md", "Hello {{greeting_name}}!\n"), + ( + "pack.template.toml", + r#" +[tokens] +greeting_name = { type = "string", required = true, description = "Who to greet" } +"#, + ), + ], + ); + + client + .install(InstallRequest { + project_root: project_root.clone(), + spec: PersonaSpec { + name: "templated".to_string(), + version: "0.1.0".to_string(), + }, + source: InstallSource::LocalPath(pack_dir), + }) + .expect("install"); + + let project_id = client.project_id(&project_root).expect("project id"); + let rendered_root = data_root + .join("projects") + .join(&project_id) + .join("personas/templated/rendered"); + + for (target_dir, filename) in [ + ("claude", "CLAUDE.md"), + ("codex", "AGENTS.md"), + ("gemini", "GEMINI.md"), + ("generic", "AGENTS.md"), + ] { + let content = fs::read_to_string(rendered_root.join(target_dir).join(filename)) + .unwrap_or_else(|e| panic!("read {target_dir}/{filename}: {e}")); + assert!( + content.contains("Hello Ada!"), + "{target_dir}/{filename} must have the token substituted; got:\n{content}" + ); + assert!( + !content.contains("{{greeting_name}}"), + "{target_dir}/{filename} must not leave the placeholder unsubstituted" + ); + } +} + +/// Required tokens with no vault value fail install with +/// `ClientError::MissingRequiredTokens` naming every missing token (not just +/// the first), and never produce a `rendered/` directory for the persona. +#[test] +fn missing_required_tokens_fail_install_cleanly_and_name_every_token() { + let temp = TempDir::new().expect("tempdir"); + let data_root = temp.path().join("data-root"); + let project_root = temp.path().join("project"); + fs::create_dir_all(&project_root).expect("create project"); + + // Vault exists but supplies neither required token. + let vault = vault_with_variables(&[]); + let client = Client::new(ClientOptions { + data_root: data_root.clone(), + config_root: None, + vault: Some(fixed_vault_provider(vault)), + }); + + let pack_dir = temp.path().join("templated-pack"); + write_pack_manifest( + &pack_dir, + PACK_TOML, + &[ + ( + "AGENTS.md", + "Hello {{greeting_name}}, key is {{api_key}}!\n", + ), + ( + "pack.template.toml", + r#" +[tokens] +greeting_name = { type = "string", required = true, description = "Who to greet" } +api_key = { type = "string", required = true, description = "API key" } +"#, + ), + ], + ); + + let err = client + .install(InstallRequest { + project_root: project_root.clone(), + spec: PersonaSpec { + name: "templated".to_string(), + version: "0.1.0".to_string(), + }, + source: InstallSource::LocalPath(pack_dir), + }) + .expect_err("install must fail when required tokens are missing"); + + let ClientError::MissingRequiredTokens { tokens, .. } = &err else { + panic!("expected ClientError::MissingRequiredTokens, got {err}"); + }; + assert_eq!( + tokens, + &vec!["api_key".to_string(), "greeting_name".to_string()], + "every missing required token must be named, not just the first" + ); + + let project_id = client.project_id(&project_root).expect("project id"); + let rendered_dir = data_root + .join("projects") + .join(&project_id) + .join("personas/templated/rendered"); + assert!( + !rendered_dir.exists(), + "a failed install must not leave a partial rendered/ directory" + ); +} + +/// A pack that ships no `pack.template.toml` renders byte-identically +/// whether or not a vault provider is configured on the `Client` -- the +/// load-bearing regression invariant: every pack that predates this feature +/// must behave exactly as it did before. +#[test] +fn pack_without_manifest_renders_byte_identically_regardless_of_vault() { + let temp = TempDir::new().expect("tempdir"); + let project_root = temp.path().join("project"); + fs::create_dir_all(&project_root).expect("create project"); + + /// Same shape as `PACK_TOML` but named to match this test's install spec. + const PLAIN_PACK_TOML: &str = r#" +schema_version = 1 +name = "plain" +author_handle = "alice" +author_pubkey = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef" +version = "0.1.0" +"#; + + let pack_dir = temp.path().join("plain-pack"); + write_pack_manifest( + &pack_dir, + PLAIN_PACK_TOML, + &[( + "AGENTS.md", + "Hello {{not_a_declared_token}}, this pack has no manifest.\n", + )], + ); + + // Render with no vault provider at all. + let data_root_no_vault = temp.path().join("data-root-no-vault"); + let client_no_vault = Client::new(ClientOptions { + data_root: data_root_no_vault.clone(), + config_root: None, + vault: None, + }); + client_no_vault + .install(InstallRequest { + project_root: project_root.clone(), + spec: PersonaSpec { + name: "plain".to_string(), + version: "0.1.0".to_string(), + }, + source: InstallSource::LocalPath(pack_dir.clone()), + }) + .expect("install without a vault provider configured"); + + // Render again with a vault provider present and populated -- but the + // pack still ships no pack.template.toml, so the provider must never + // even be consulted, and the token marker must survive untouched. + let data_root_with_vault = temp.path().join("data-root-with-vault"); + let vault = vault_with_variables(&[("not_a_declared_token", "should never be used")]); + let client_with_vault = Client::new(ClientOptions { + data_root: data_root_with_vault.clone(), + config_root: None, + vault: Some(fixed_vault_provider(vault)), + }); + client_with_vault + .install(InstallRequest { + project_root: project_root.clone(), + spec: PersonaSpec { + name: "plain".to_string(), + version: "0.1.0".to_string(), + }, + source: InstallSource::LocalPath(pack_dir), + }) + .expect("install with a vault provider configured"); + + let project_id = client_no_vault + .project_id(&project_root) + .expect("project id"); + let rendered_path = |root: &Path| { + root.join("projects") + .join(&project_id) + .join("personas/plain/rendered/claude/CLAUDE.md") + }; + + let no_vault_content = + fs::read_to_string(rendered_path(&data_root_no_vault)).expect("read no-vault render"); + let with_vault_content = + fs::read_to_string(rendered_path(&data_root_with_vault)).expect("read with-vault render"); + + assert_eq!( + no_vault_content, with_vault_content, + "rendering must be byte-identical regardless of vault provider presence" + ); + assert!( + no_vault_content.contains("{{not_a_declared_token}}"), + "an unmanifested pack's {{{{token}}}} markers are left untouched, exactly as before \ + this feature existed" + ); +} diff --git a/crates/frameshift-conformance/src/gate.rs b/crates/frameshift-conformance/src/gate.rs index 8b991af..3322e86 100644 --- a/crates/frameshift-conformance/src/gate.rs +++ b/crates/frameshift-conformance/src/gate.rs @@ -1,29 +1,12 @@ -use crate::score::Score; use frameshift_pack::ConformanceBaseline; -/// Decision returned by [`RegressionGate::evaluate_upgrade`]. -#[derive(Debug, Clone, PartialEq)] -pub enum GateDecision { - /// Upgrade clears the baseline. - Pass, - /// New score is below the old baseline by `delta` (positive value). - FailRegression { delta: f32 }, - /// Bundle hash changed, so the baseline cannot be compared directly. - FailBundleChanged, - /// A score was non-finite or outside `0.0..=1.0`; the gate fails closed - /// rather than letting a malformed baseline or buggy scorer slip through. - FailInvalidScore, -} - /// Decision returned by [`RegressionGate::evaluate_cross_version`]. /// -/// Unlike [`GateDecision`] (which compares a freshly-run [`Score`] against the -/// baseline shipped with the *previous* pack version), this compares two -/// already-*shipped* baselines directly -- the conformance score each pack -/// version asserts about itself at publish time -- without re-running any -/// conformance tests. It answers "does the pack version we are about to -/// install over an existing install claim to be at least as good as the one -/// it replaces, and is that claim trustworthy?" +/// Compares two already-*shipped* baselines directly -- the conformance +/// score each pack version asserts about itself at publish time -- without +/// re-running any conformance tests. It answers "does the pack version we +/// are about to install over an existing install claim to be at least as +/// good as the one it replaces, and is that claim trustworthy?" #[derive(Debug, Clone, PartialEq)] pub enum CrossVersionDecision { /// The incoming version's shipped baseline meets or exceeds the @@ -73,37 +56,6 @@ pub enum CrossVersionDecision { pub struct RegressionGate; impl RegressionGate { - /// Compare a new run's score against the baseline shipped with the - /// previous pack version. - /// - /// Rules: - /// 1. If the bundle hash changed, fail with [`GateDecision::FailBundleChanged`]. - /// Comparing scores across different bundles is meaningless. - /// 2. Otherwise if `new_score < old_baseline.score`, fail regression. - /// 3. Otherwise pass. - pub fn evaluate_upgrade( - old_baseline: &ConformanceBaseline, - new_score: Score, - new_bundle_hash: &str, - ) -> GateDecision { - // Fail closed on non-finite or out-of-range scores: a NaN comparison - // returns false and would otherwise fall through to Pass, letting a - // malformed baseline or buggy custom scorer bypass regression blocking. - let valid = |s: f32| s.is_finite() && (0.0..=1.0).contains(&s); - if !valid(old_baseline.score) || !valid(new_score.0) { - return GateDecision::FailInvalidScore; - } - if old_baseline.bundle_hash != new_bundle_hash { - return GateDecision::FailBundleChanged; - } - if new_score.0 < old_baseline.score { - return GateDecision::FailRegression { - delta: old_baseline.score - new_score.0, - }; - } - GateDecision::Pass - } - /// Compare the *shipped* conformance baselines of an already-installed /// pack version and the incoming version about to replace it, without /// re-running any conformance tests. @@ -160,9 +112,9 @@ impl RegressionGate { }; }; - // Fail closed on non-finite or out-of-range scores, mirroring - // evaluate_upgrade's guard against a malformed baseline slipping - // through a NaN comparison. + // Fail closed on non-finite or out-of-range scores: a NaN comparison + // returns false and would otherwise fall through to Pass, letting a + // malformed baseline slip through. let valid = |s: f32| s.is_finite() && (0.0..=1.0).contains(&s); if !valid(installed.score) || !valid(incoming.score) { return CrossVersionDecision::InvalidScore; @@ -189,56 +141,6 @@ mod tests { } } - /// Non-finite or out-of-range scores fail closed. - #[test] - fn gate_fails_closed_on_invalid_score() { - let nan_baseline = baseline(f32::NAN, "abc"); - assert_eq!( - RegressionGate::evaluate_upgrade(&nan_baseline, Score(0.9), "abc"), - GateDecision::FailInvalidScore - ); - let ok_baseline = baseline(0.8, "abc"); - assert_eq!( - RegressionGate::evaluate_upgrade(&ok_baseline, Score(f32::INFINITY), "abc"), - GateDecision::FailInvalidScore - ); - assert_eq!( - RegressionGate::evaluate_upgrade(&ok_baseline, Score(1.5), "abc"), - GateDecision::FailInvalidScore - ); - } - - #[test] - fn gate_passes_when_score_meets_baseline() { - let b = baseline(0.8, "abc"); - let decision = RegressionGate::evaluate_upgrade(&b, Score(0.85), "abc"); - assert_eq!(decision, GateDecision::Pass); - - let decision_eq = RegressionGate::evaluate_upgrade(&b, Score(0.8), "abc"); - assert_eq!(decision_eq, GateDecision::Pass); - } - - #[test] - fn gate_fails_on_regression() { - let b = baseline(0.9, "abc"); - let decision = RegressionGate::evaluate_upgrade(&b, Score(0.7), "abc"); - match decision { - GateDecision::FailRegression { delta } => { - assert!((delta - 0.2).abs() < 1e-6, "delta was {delta}"); - } - other => panic!("expected FailRegression, got {other:?}"), - } - } - - #[test] - fn gate_fails_on_bundle_change() { - let b = baseline(0.5, "abc"); - let decision = RegressionGate::evaluate_upgrade(&b, Score(1.0), "xyz"); - assert_eq!(decision, GateDecision::FailBundleChanged); - } - - // ---- evaluate_cross_version ---- - /// Incoming score above the installed baseline, with a verified bundle, /// passes. #[test] diff --git a/crates/frameshift-conformance/src/lib.rs b/crates/frameshift-conformance/src/lib.rs index d8419bf..90a2e37 100644 --- a/crates/frameshift-conformance/src/lib.rs +++ b/crates/frameshift-conformance/src/lib.rs @@ -6,9 +6,8 @@ //! - Scoring ([`score`]) //! - Upgrade-regression gate ([`gate`]) //! -//! The runtime invokes a [`Runner`] for each [`TestCase`] in a [`TestBundle`], -//! produces a [`Score`], and can feed it to [`RegressionGate::evaluate_upgrade`] -//! during upgrades. Separately, [`RegressionGate::evaluate_cross_version`] +//! The runtime invokes a [`Runner`] for each [`TestCase`] in a [`TestBundle`] +//! and produces a [`Score`]. Separately, [`RegressionGate::evaluate_cross_version`] //! compares two packs' already-*shipped* baselines directly (no test run //! required) and is wired into `frameshift_client::Client::install`'s //! install-over-existing-version path as a warn-only, non-blocking check. @@ -29,6 +28,6 @@ pub use bundle::{bundle_hash, load_from_dir, TestBundle}; pub use caller::{score_bundle_with_caller, CallerScorer}; pub use case::{ExpectedBehavior, ScorerKind, TestCase}; pub use error::ConformanceError; -pub use gate::{CrossVersionDecision, GateDecision, RegressionGate}; +pub use gate::{CrossVersionDecision, RegressionGate}; pub use runner::{MockRunner, Runner}; pub use score::{bundle_score, score_test, Score}; diff --git a/crates/frameshift-daemon/src/handler.rs b/crates/frameshift-daemon/src/handler.rs index 947e109..9c0f38f 100644 --- a/crates/frameshift-daemon/src/handler.rs +++ b/crates/frameshift-daemon/src/handler.rs @@ -172,6 +172,7 @@ mod tests { Client::new(ClientOptions { data_root: tmp.path().to_path_buf(), config_root: None, + vault: None, }) } diff --git a/crates/frameshift-daemon/src/main.rs b/crates/frameshift-daemon/src/main.rs index 6d2872b..5b4d408 100644 --- a/crates/frameshift-daemon/src/main.rs +++ b/crates/frameshift-daemon/src/main.rs @@ -39,10 +39,16 @@ async fn main() -> Result<(), Box> { .with_env_filter(EnvFilter::from_default_env()) .init(); - // Build the shared client using XDG-derived paths. + // Build the shared client using XDG-derived paths. The vault provider is + // env-only: the daemon runs unattended with no interactive terminal, so + // a passphrase prompt would simply hang the process. Operators who need + // vault-backed template tokens available to the daemon must export + // FRAMESHIFT_VAULT_PASSPHRASE in its environment. let client = Arc::new( - frameshift_client::Client::with_default_data_root() - .expect("failed to initialize frameshift client"), + frameshift_client::Client::with_default_data_root_and_vault(Some( + frameshift_client::env_only_vault_provider(), + )) + .expect("failed to initialize frameshift client"), ); // Determine the socket directory from XDG_RUNTIME_DIR (fallback: /tmp). diff --git a/crates/frameshift-daemon/src/orchestrator.rs b/crates/frameshift-daemon/src/orchestrator.rs index ae2919f..2eca390 100644 --- a/crates/frameshift-daemon/src/orchestrator.rs +++ b/crates/frameshift-daemon/src/orchestrator.rs @@ -211,6 +211,7 @@ mod tests { Client::new(ClientOptions { data_root: data_root.to_path_buf(), config_root: None, + vault: None, }) } diff --git a/crates/frameshift-daemon/src/socket.rs b/crates/frameshift-daemon/src/socket.rs index bbcbfda..a264a00 100644 --- a/crates/frameshift-daemon/src/socket.rs +++ b/crates/frameshift-daemon/src/socket.rs @@ -154,6 +154,7 @@ mod tests { Client::new(ClientOptions { data_root: tmp.path().to_path_buf(), config_root: None, + vault: None, }) } diff --git a/crates/frameshift-growth/src/lib.rs b/crates/frameshift-growth/src/lib.rs index f4fa4b4..603587d 100644 --- a/crates/frameshift-growth/src/lib.rs +++ b/crates/frameshift-growth/src/lib.rs @@ -88,6 +88,35 @@ pub fn append( append_jsonl(data_root, project_id, persona_name, &entry) } +/// Append a structured `Scope::Global` growth entry with the current UTC +/// timestamp, writing ONLY to the persona's global growth.jsonl +/// (`/personas//growth.jsonl`). +/// +/// Unlike [`append`], this never touches the legacy markdown growth.md -- +/// there is no global markdown growth file, so global entries are +/// structured-only from the start. Structured fields not derivable from +/// these parameters (`auto_selected`, `task`, `intent`) are populated with +/// their default/`None` forms, matching `append`'s best-effort projection. +pub fn append_global( + data_root: &Path, + project_id: &str, + persona_name: &str, + entry_text: &str, +) -> Result<(), GrowthError> { + let entry = GrowthEntry { + ts: format_utc_now(), + session: current_session_id(), + project_id: project_id.to_string(), + persona: persona_name.to_string(), + auto_selected: false, + task: None, + intent: None, + text: entry_text.to_string(), + scope: Scope::Global, + }; + append_jsonl(data_root, project_id, persona_name, &entry) +} + /// Return a best-effort session identifier for structured growth entries. /// /// Uses the current process ID, which is stable for the lifetime of the @@ -758,6 +787,44 @@ mod tests { assert!(path.exists()); } + /// `append_global` writes a `Scope::Global` entry that `summarize` finds + /// under `Scope::Global` and that is entirely absent from `Scope::Project` + /// -- the CLI's `grow append --global` path relies on this separation to + /// avoid leaking a global entry into a project-scoped read. + #[test] + fn append_global_is_visible_in_global_scope_only() { + let tmp = tempfile::tempdir().unwrap(); + append_global(tmp.path(), "proj1", "rust", "prefer thiserror in libraries").unwrap(); + + let global_entries = read_entries(tmp.path(), "proj1", "rust", Scope::Global).unwrap(); + assert_eq!(global_entries.len(), 1); + assert_eq!(global_entries[0].text, "prefer thiserror in libraries"); + assert_eq!(global_entries[0].scope, Scope::Global); + + let project_entries = read_entries(tmp.path(), "proj1", "rust", Scope::Project).unwrap(); + assert!( + project_entries.is_empty(), + "a global append must not appear in project scope" + ); + + let summary = summarize(tmp.path(), "proj1", "rust", Scope::Global).unwrap(); + assert!(summary.contains("prefer thiserror in libraries")); + } + + /// `append_global` never touches growth.md -- there is no global markdown + /// growth file, so the legacy markdown path must not be created. + #[test] + fn append_global_does_not_write_markdown() { + let tmp = tempfile::tempdir().unwrap(); + append_global(tmp.path(), "proj1", "rust", "global-only note").unwrap(); + + let md_path = tmp.path().join("projects/proj1/personas/rust/growth.md"); + assert!( + !md_path.exists(), + "append_global must not create a project-scoped growth.md" + ); + } + #[test] fn read_entries_returns_all_entries() { let tmp = tempfile::tempdir().unwrap(); diff --git a/crates/frameshift-mcp/src/main.rs b/crates/frameshift-mcp/src/main.rs index 4bb79c5..ea49815 100644 --- a/crates/frameshift-mcp/src/main.rs +++ b/crates/frameshift-mcp/src/main.rs @@ -16,7 +16,14 @@ async fn main() { .with_env_filter(tracing_subscriber::EnvFilter::from_default_env()) .init(); - let client = match Client::with_default_data_root() { + // Env-only vault provider: stdin/stdout here *are* the JSON-RPC protocol + // channel, so there is never an interactive terminal to prompt on, and a + // blocking stdin read would corrupt the protocol stream. Callers that + // need vault-backed template tokens available to this server must export + // FRAMESHIFT_VAULT_PASSPHRASE in its environment. + let client = match Client::with_default_data_root_and_vault(Some( + frameshift_client::env_only_vault_provider(), + )) { Ok(c) => c, Err(e) => { // stdout is the protocol channel and no request id exists yet, so the @@ -237,6 +244,7 @@ mod tests { Client::new(ClientOptions { data_root: data_root.to_path_buf(), config_root: None, + vault: None, }) } diff --git a/crates/frameshift-mcp/src/prompts.rs b/crates/frameshift-mcp/src/prompts.rs index ae9fd24..be4bccd 100644 --- a/crates/frameshift-mcp/src/prompts.rs +++ b/crates/frameshift-mcp/src/prompts.rs @@ -356,6 +356,7 @@ mod tests { Client::new(ClientOptions { data_root: data_root.to_path_buf(), config_root: None, + vault: None, }) } diff --git a/crates/frameshift-mcp/src/tools.rs b/crates/frameshift-mcp/src/tools.rs index 132d4d8..1441a29 100644 --- a/crates/frameshift-mcp/src/tools.rs +++ b/crates/frameshift-mcp/src/tools.rs @@ -181,11 +181,15 @@ pub fn tool_definitions() -> Vec { }, ToolDef { name: "frameshift_search".to_string(), - description: "Search the registry's pack catalog by free-text query and return matching packs with name, latest version, download count, tags, and description. Read-only; does not install anything. Use this to discover packs before calling frameshift_install.".to_string(), + description: "Search the registry's pack catalog by free-text query, optionally restricted to a single tag, and return matching packs with name, latest version, download count, tags, and description. Read-only; does not install anything. Use this to discover packs before calling frameshift_install.".to_string(), input_schema: serde_json::json!({ "type": "object", "properties": { "query": {"type": "string"}, + "tag": { + "type": "string", + "description": "Restrict results to packs carrying this tag." + }, "limit": { "type": "integer", "description": "Maximum number of results to return (default 20, capped at 100)." @@ -953,14 +957,26 @@ fn parse_search_limit(arguments: &serde_json::Value) -> u32 { .unwrap_or(DEFAULT_SEARCH_LIMIT) } +/// Resolve the optional `tag` argument for `frameshift_search`. +/// +/// Returns `None` when `tag` is absent or not a JSON string, matching how +/// `RegistrySearchQuery::tag` treats "no filter" -- mirrors the CLI's +/// `--tag` flag, which is likewise optional. +fn parse_search_tag(arguments: &serde_json::Value) -> Option { + arguments + .get("tag") + .and_then(|v| v.as_str()) + .map(str::to_string) +} + /// Handle the frameshift_search tool call. /// /// Searches the registry's pack catalog (`GET /v1/packs`) via /// `client.search_registry`, mirroring the CLI's `frameshift search` -/// subcommand (`frameshift_cli::cmd::search::run_search`). Returns -/// `{ "results": [{name, latest_version, description, tags, total_downloads, -/// score}, ...] }` so MCP-only agents can discover packs before calling -/// `frameshift_install`. +/// subcommand (`frameshift_cli::cmd::search::run_search`), including its +/// optional `--tag` filter. Returns `{ "results": [{name, latest_version, +/// description, tags, total_downloads, score}, ...] }` so MCP-only agents can +/// discover packs before calling `frameshift_install`. fn call_search(arguments: &serde_json::Value, client: &Client) -> ToolResult { let query = match arguments.get("query").and_then(|v| v.as_str()) { Some(s) => s, @@ -968,10 +984,11 @@ fn call_search(arguments: &serde_json::Value, client: &Client) -> ToolResult { }; let limit = parse_search_limit(arguments); + let tag = parse_search_tag(arguments); let search_query = RegistrySearchQuery { query: Some(query.to_string()), - tag: None, + tag, limit: Some(limit), }; @@ -1048,6 +1065,7 @@ mod tests { Client::new(ClientOptions { data_root: data_root.to_path_buf(), config_root: None, + vault: None, }) } @@ -1060,7 +1078,8 @@ mod tests { } /// frameshift_search is present in tool_definitions with `query` required - /// and `limit` present but optional in its input schema. + /// and `limit`/`tag` present but optional in its input schema, matching + /// the CLI's `frameshift search --tag` surface. #[test] fn tool_definitions_includes_search() { let defs = tool_definitions(); @@ -1080,10 +1099,18 @@ mod tests { !required.iter().any(|v| v == "limit"), "limit must not be required" ); + assert!( + !required.iter().any(|v| v == "tag"), + "tag must not be required" + ); assert!( search.input_schema["properties"]["limit"].is_object(), "limit must be a declared property" ); + assert!( + search.input_schema["properties"]["tag"].is_object(), + "tag must be a declared property" + ); } /// Verify that calling an unknown tool name returns an is_error result. @@ -1645,4 +1672,16 @@ mod tests { MAX_SEARCH_LIMIT ); } + + /// parse_search_tag returns `None` when `tag` is absent or not a string, + /// and `Some(_)` with the string value when present. + #[test] + fn parse_search_tag_extracts_optional_string() { + assert_eq!(parse_search_tag(&serde_json::json!({})), None); + assert_eq!(parse_search_tag(&serde_json::json!({"tag": 5})), None); + assert_eq!( + parse_search_tag(&serde_json::json!({"tag": "rust"})), + Some("rust".to_string()) + ); + } } diff --git a/crates/frameshift-server/tests/admin_tombstone.rs b/crates/frameshift-server/tests/admin_tombstone.rs index c1c28b0..354d49d 100644 --- a/crates/frameshift-server/tests/admin_tombstone.rs +++ b/crates/frameshift-server/tests/admin_tombstone.rs @@ -33,7 +33,7 @@ use secrecy::SecretString; use tower::ServiceExt as _; use frameshift_catalog::identity::Ed25519PublicKey; -use frameshift_catalog::records::PackVersionRecord; +use frameshift_catalog::records::{PackRecord, PackVersionRecord}; use frameshift_catalog::status::PackStatus; use frameshift_objects::ObjectHash; @@ -124,6 +124,29 @@ fn seed_active_version(catalog: &MockCatalog, name: &str, version: &str) { .insert((name.to_string(), version.to_string()), record); } +/// Insert a pack head record for `name` directly into `catalog`'s in-memory +/// state, bypassing the publish flow, with `latest_version` set to +/// `latest_version`. +/// +/// Paired with [`seed_active_version`] to build a pack whose head has a +/// working `latest_version` -- required for the head-recompute tests below, +/// since [`seed_active_version`] alone only writes a `pack_versions` row and +/// (matching the real publish path being bypassed) never touches the head. +fn seed_pack_head(catalog: &MockCatalog, name: &str, latest_version: &str) { + let mut state = catalog.state.write().unwrap(); + let record = PackRecord { + name: name.to_string(), + current_author: Ed25519PublicKey([7u8; 32]), + tags: vec![], + description: String::new(), + created_at: Utc::now(), + latest_version: Some(latest_version.to_string()), + total_downloads: 0, + extends: None, + }; + state.packs.insert(name.to_string(), record); +} + /// Issue a signed (or unsigned, when `key` is `None`) JSON POST against the /// real router and return the response. async fn post_signed_json( @@ -295,3 +318,201 @@ async fn repeat_tombstone_is_idempotent_200() { "re-tombstoning must be idempotent, not a conflict" ); } + +// --------------------------------------------------------------------------- +// Tombstone read-path: head recompute + search/download visibility +// (spec_42eb1942 item 1). MockCatalog's `tombstone_pack` mirrors the +// Postgres adapter's `latest_version` recompute exactly (see +// `crates/frameshift-server/tests/mocks/catalog.rs`), so these assertions +// hold for both backends. +// --------------------------------------------------------------------------- + +/// Issue a plain unsigned GET against the real router and return the response. +async fn get_unsigned(state: AppState, path: &str) -> axum::http::Response { + let req = Request::builder() + .method("GET") + .uri(path) + .body(Body::empty()) + .unwrap(); + app(state).oneshot(req).await.unwrap() +} + +/// Parse a response body as JSON. +async fn json_body(resp: axum::http::Response) -> serde_json::Value { + let bytes = resp.into_body().collect().await.unwrap().to_bytes(); + serde_json::from_slice(&bytes).unwrap() +} + +/// Tombstoning the current latest of two `Active` versions recomputes the +/// pack head's `latest_version` to the older remaining `Active` version, and +/// the pack stays visible in search because it still has one `Active` +/// version left. +#[tokio::test] +async fn tombstone_latest_of_two_recomputes_head_to_older_version() { + let admin = SigningKey::from_bytes(&[60u8; 32]); + let catalog = MockCatalog::new(); + seed_active_version(&catalog, "multi-pack", "1.0.0"); + seed_active_version(&catalog, "multi-pack", "2.0.0"); + seed_pack_head(&catalog, "multi-pack", "2.0.0"); + + let resp = post_signed_json( + mk_state(catalog.clone(), vec![pubkey_b64(&admin)]), + "/v1/admin/packs/multi-pack/2.0.0/tombstone", + tombstone_body(), + Some(&admin), + ) + .await; + assert_eq!(resp.status(), StatusCode::OK, "tombstone should 200"); + + // Head recompute: latest_version falls back to the older Active version. + { + let s = catalog.state.read().unwrap(); + let head = s + .packs + .get("multi-pack") + .expect("pack head must still exist"); + assert_eq!( + head.latest_version, + Some("1.0.0".to_string()), + "latest_version must fall back to the newest remaining Active version" + ); + } + + // Search still returns the pack -- it has one Active version left. + let search_resp = get_unsigned( + mk_state(catalog.clone(), vec![pubkey_b64(&admin)]), + "/v1/packs", + ) + .await; + assert_eq!(search_resp.status(), StatusCode::OK); + let body = json_body(search_resp).await; + let results = body["results"].as_array().expect("results is an array"); + assert!( + results.iter().any(|r| r["pack"]["name"] == "multi-pack"), + "multi-pack must still appear in search after tombstoning its \ + (non-only) latest version, got: {results:?}" + ); + + // GET /v1/packs/multi-pack reflects the recomputed latest_version too. + let head_resp = get_unsigned( + mk_state(catalog, vec![pubkey_b64(&admin)]), + "/v1/packs/multi-pack", + ) + .await; + let head_body = json_body(head_resp).await; + assert_eq!(head_body["latest_version"], "1.0.0"); +} + +/// Tombstoning the ONLY version of a pack clears the head's `latest_version` +/// to `None`. The pack then disappears from search and its (only) version can +/// no longer be downloaded, but a direct `GET` of the version record still +/// shows it with `Tombstone` status (deliberate transparency). +#[tokio::test] +async fn tombstone_only_version_clears_head_hides_search_and_download_404s() { + let admin = SigningKey::from_bytes(&[61u8; 32]); + let catalog = MockCatalog::new(); + seed_active_version(&catalog, "solo-pack", "1.0.0"); + seed_pack_head(&catalog, "solo-pack", "1.0.0"); + + let resp = post_signed_json( + mk_state(catalog.clone(), vec![pubkey_b64(&admin)]), + "/v1/admin/packs/solo-pack/1.0.0/tombstone", + tombstone_body(), + Some(&admin), + ) + .await; + assert_eq!(resp.status(), StatusCode::OK, "tombstone should 200"); + + // Head recompute: latest_version clears -- zero Active versions remain. + { + let s = catalog.state.read().unwrap(); + let head = s + .packs + .get("solo-pack") + .expect("pack head record is retained, not deleted"); + assert_eq!( + head.latest_version, None, + "latest_version must clear when no Active version remains" + ); + } + + // The pack disappears from search entirely. + let search_resp = get_unsigned( + mk_state(catalog.clone(), vec![pubkey_b64(&admin)]), + "/v1/packs", + ) + .await; + let body = json_body(search_resp).await; + let results = body["results"].as_array().expect("results is an array"); + assert!( + !results.iter().any(|r| r["pack"]["name"] == "solo-pack"), + "solo-pack must disappear from search once its only version is \ + tombstoned, got: {results:?}" + ); + + // A direct GET of the version record still shows it, with Tombstone + // status visible -- get_pack_version does not hide tombstoned records. + let version_resp = get_unsigned( + mk_state(catalog.clone(), vec![pubkey_b64(&admin)]), + "/v1/packs/solo-pack/versions/1.0.0", + ) + .await; + assert_eq!(version_resp.status(), StatusCode::OK); + let version_body = json_body(version_resp).await; + assert_eq!(version_body["status"]["kind"], "tombstone"); + + // Install-by-name/latest resolution 404s: a real client resolves latest + // via GET /v1/packs/{name} (latest_version is now None here, so a client + // has no version to request) and, were it to still request the formerly + // latest version directly, download_pack_bytes refuses to serve a + // Tombstone-status version even via the direct URL -- this closes the + // takedown bypass regardless of how the caller arrived at the version. + let download_resp = get_unsigned( + mk_state(catalog, vec![pubkey_b64(&admin)]), + "/v1/packs/solo-pack/versions/1.0.0/pack", + ) + .await; + assert_eq!(download_resp.status(), StatusCode::NOT_FOUND); +} + +/// Tombstoning a non-latest version leaves the head's `latest_version` +/// unchanged and does not affect search visibility. +#[tokio::test] +async fn tombstone_non_latest_version_leaves_head_and_search_unchanged() { + let admin = SigningKey::from_bytes(&[62u8; 32]); + let catalog = MockCatalog::new(); + seed_active_version(&catalog, "stable-pack", "1.0.0"); + seed_active_version(&catalog, "stable-pack", "2.0.0"); + seed_pack_head(&catalog, "stable-pack", "2.0.0"); + + // Tombstone the OLDER, non-latest version. + let resp = post_signed_json( + mk_state(catalog.clone(), vec![pubkey_b64(&admin)]), + "/v1/admin/packs/stable-pack/1.0.0/tombstone", + tombstone_body(), + Some(&admin), + ) + .await; + assert_eq!(resp.status(), StatusCode::OK, "tombstone should 200"); + + { + let s = catalog.state.read().unwrap(); + let head = s + .packs + .get("stable-pack") + .expect("pack head must still exist"); + assert_eq!( + head.latest_version, + Some("2.0.0".to_string()), + "latest_version must be unchanged when a non-latest version is tombstoned" + ); + } + + let search_resp = get_unsigned(mk_state(catalog, vec![pubkey_b64(&admin)]), "/v1/packs").await; + let body = json_body(search_resp).await; + let results = body["results"].as_array().expect("results is an array"); + assert!( + results.iter().any(|r| r["pack"]["name"] == "stable-pack"), + "stable-pack must remain in search after tombstoning a non-latest version" + ); +} diff --git a/crates/frameshift-server/tests/mocks/catalog.rs b/crates/frameshift-server/tests/mocks/catalog.rs index b71f013..fc62abc 100644 --- a/crates/frameshift-server/tests/mocks/catalog.rs +++ b/crates/frameshift-server/tests/mocks/catalog.rs @@ -22,6 +22,10 @@ use frameshift_catalog::filters::{PackSearchFilters, PackSearchResult}; use frameshift_catalog::identity::Ed25519PublicKey; use frameshift_catalog::records::{AuthorRecord, PackRecord, PackVersionRecord}; use frameshift_catalog::status::{PackStatus, TombstoneRecord}; +// Reuse the exact same version-precedence comparator the Postgres adapter +// uses for `register_pack_version`'s D8 `latest_version` selection, so the +// mock's tombstone head-recompute can never drift from the real ordering. +use frameshift_catalog_postgres::backend::semver_gt; /// Shared mutable state for [`MockCatalog`]. /// @@ -256,7 +260,12 @@ impl CatalogBackend for MockCatalog { Ok(versions) } - /// Search packs (returns all stored packs with score 1.0, ignoring filters). + /// Search packs (returns stored packs with score 1.0, ignoring filters + /// other than the tombstone-driven `latest_version` exclusion). + /// + /// Mirrors the Postgres adapter's `latest_version IS NOT NULL` predicate: + /// a pack whose head has zero remaining `Active` versions (recomputed by + /// `tombstone_pack` to `None`) is excluded from every search result set. async fn search_packs( &self, _filters: &PackSearchFilters, @@ -268,6 +277,7 @@ impl CatalogBackend for MockCatalog { let results = state .packs .values() + .filter(|pack| pack.latest_version.is_some()) .cloned() .map(|pack| PackSearchResult { pack, score: 1.0 }) .collect(); @@ -299,6 +309,14 @@ impl CatalogBackend for MockCatalog { /// (last-writer-wins on `reason`/`recorded_at`), never `Conflict`. /// Returns `NotFound` when the `(name, version)` pair has no version /// record, matching the trait's documented contract. + /// + /// After flipping the status, recomputes the pack head's `latest_version` + /// (when a head row exists) to the newest remaining `Active` version using + /// [`semver_gt`] -- the exact same comparator the Postgres adapter uses + /// for `register_pack_version`'s D8 ordering -- or clears it to `None` + /// when no `Active` version remains. A head that was never seeded (tests + /// that only call `seed_active_version`-style helpers without inserting a + /// `PackRecord`) is left absent; there is nothing to recompute. async fn tombstone_pack( &self, name: &str, @@ -316,13 +334,33 @@ impl CatalogBackend for MockCatalog { reason: record.reason, recorded_at: record.recorded_at, }; - Ok(()) } - None => Err(CatalogError::NotFound { - kind: "pack_version", - key: format!("{name}@{version}"), - }), + None => { + return Err(CatalogError::NotFound { + kind: "pack_version", + key: format!("{name}@{version}"), + }); + } } + + // Recompute the newest remaining Active version for this pack, the + // same way the Postgres adapter does inside its transaction. + let newest_active = state + .versions + .values() + .filter(|v| v.pack_name == name && matches!(v.status, PackStatus::Active)) + .map(|v| v.version.clone()) + .fold(None::, |best, candidate| match best { + None => Some(candidate), + Some(cur) if semver_gt(&candidate, &cur) => Some(candidate), + Some(cur) => Some(cur), + }); + + if let Some(pack) = state.packs.get_mut(name) { + pack.latest_version = newest_active; + } + + Ok(()) } /// Get the public key for a handle.