From 067a086aa0820b3d06e501e012cd413e881cd4fc Mon Sep 17 00:00:00 2001 From: GhostFrame Date: Sun, 12 Jul 2026 13:29:09 -0400 Subject: [PATCH 1/4] fix(catalog): make tombstones real on the read path tombstone_pack now recomputes the pack head's latest_version to the newest remaining Active version (same semver_gt comparator register_pack_version uses, now pub so postgres and the test mock share it), clearing it when none remain. Search excludes packs with no resolvable latest in every query branch, including trending, which previously had no tombstone exclusion at all. Trait docs rewritten to match reality: search documents the latest_version mechanism, the versions routes document that they deliberately return tombstoned records with status visible. Covered by three router-level integration tests plus mirrored docker-gated postgres tests. --- .../src/backend.rs | 195 ++++++++++++--- .../tests/postgres_integration.rs | 191 +++++++++++++++ crates/frameshift-catalog/src/backend.rs | 59 ++++- crates/frameshift-catalog/src/records.rs | 15 +- .../tests/admin_tombstone.rs | 223 +++++++++++++++++- .../frameshift-server/tests/mocks/catalog.rs | 50 +++- 6 files changed, 680 insertions(+), 53 deletions(-) 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-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. From 2bdb8addcabc8195d56950ad3eccc60a5a56d7b3 Mon Sep 17 00:00:00 2001 From: GhostFrame Date: Sun, 12 Jul 2026 13:29:09 -0400 Subject: [PATCH 2/4] chore(conformance): remove dead evaluate_upgrade path evaluate_upgrade and GateDecision had no callers outside their own unit tests; evaluate_cross_version is the live gate. Module docs and re-exports updated. --- crates/frameshift-conformance/src/gate.rs | 114 ++-------------------- crates/frameshift-conformance/src/lib.rs | 7 +- 2 files changed, 11 insertions(+), 110 deletions(-) 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}; From ea78331ebc0e5cc20db18d0f671c07e14b3eb972 Mon Sep 17 00:00:00 2001 From: GhostFrame Date: Sun, 12 Jul 2026 13:29:23 -0400 Subject: [PATCH 3/4] feat(cli,mcp): config command, global growth writes, search tag parity frameshift config get/set exposes telemetry_opt_in (allowlisted keys, strict booleans, creates config.toml on first set) so opting in no longer means hand-editing TOML. grow append --global writes a structured Global-scope entry so grow summary --scope global stops summarizing an empty set. The MCP frameshift_search tool gains the CLI's tag filter. --- crates/frameshift-cli/src/cmd/config.rs | 262 ++++++++++++++++++++++++ crates/frameshift-cli/src/cmd/grow.rs | 56 ++++- crates/frameshift-growth/src/lib.rs | 67 ++++++ crates/frameshift-mcp/src/tools.rs | 53 ++++- 4 files changed, 425 insertions(+), 13 deletions(-) create mode 100644 crates/frameshift-cli/src/cmd/config.rs 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/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-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/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()) + ); + } } From 3e8b07221ca1df88bf6da1c9ba6b0b6b7783e4e0 Mon Sep 17 00:00:00 2001 From: GhostFrame Date: Sun, 12 Jul 2026 13:29:23 -0400 Subject: [PATCH 4/4] feat(vault): wire the age-encrypted vault into the CLI and render path The vault subsystem (trait, schema, age/scrypt local backend, template tokens) was fully built with zero callers. frameshift vault init/set/get/rm/list now manages a per-project vault.age in the central store; the passphrase comes from FRAMESHIFT_VAULT_PASSPHRASE or a hidden prompt (CLI only). The client gains an opt-in VaultProvider -- the library never prompts; daemon and MCP wire env-only providers. Packs that ship pack.template.toml get their declared {{tokens}} substituted at render time from the vault, with every missing required token named in one typed error before any output is written. Packs without the manifest render byte-identically to before -- covered by a regression test -- so nothing changes for any existing pack. Vault and template errors are boxed to keep ClientError small per the existing Compose precedent. --- Cargo.lock | 29 ++ Cargo.toml | 2 + crates/frameshift-cli/Cargo.toml | 9 + crates/frameshift-cli/src/cmd/diff.rs | 1 + crates/frameshift-cli/src/cmd/migrate.rs | 2 + crates/frameshift-cli/src/cmd/mod.rs | 2 + crates/frameshift-cli/src/cmd/render.rs | 1 + crates/frameshift-cli/src/cmd/rule.rs | 3 + crates/frameshift-cli/src/cmd/skill.rs | 3 + crates/frameshift-cli/src/cmd/use_persona.rs | 1 + crates/frameshift-cli/src/cmd/vault.rs | 383 ++++++++++++++++++ crates/frameshift-cli/src/main.rs | 51 ++- crates/frameshift-cli/src/util.rs | 27 ++ crates/frameshift-client/Cargo.toml | 14 + crates/frameshift-client/src/error.rs | 58 +++ crates/frameshift-client/src/lib.rs | 333 ++++++++++++++- crates/frameshift-client/src/model.rs | 59 ++- .../frameshift-client/tests/compose_render.rs | 3 + .../frameshift-client/tests/install_flow.rs | 3 + .../tests/memory_requirement.rs | 4 + .../frameshift-client/tests/project_id_env.rs | 1 + .../tests/registry_install.rs | 1 + .../tests/template_render.rs | 312 ++++++++++++++ crates/frameshift-daemon/src/handler.rs | 1 + crates/frameshift-daemon/src/main.rs | 12 +- crates/frameshift-daemon/src/orchestrator.rs | 1 + crates/frameshift-daemon/src/socket.rs | 1 + crates/frameshift-mcp/src/main.rs | 10 +- crates/frameshift-mcp/src/prompts.rs | 1 + 29 files changed, 1318 insertions(+), 10 deletions(-) create mode 100644 crates/frameshift-cli/src/cmd/vault.rs create mode 100644 crates/frameshift-client/tests/template_render.rs 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-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/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/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-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-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, }) }