From 4ce48be72c3154ff851813c28f7a8936af84d870 Mon Sep 17 00:00:00 2001 From: GhostFrame Date: Sat, 11 Jul 2026 16:01:51 -0400 Subject: [PATCH 01/10] feat(client): registry host fix, selection telemetry, cross-version conformance gate Points DEFAULT_REGISTRY_URL at the -api host that actually serves the registry API. Wires the CLI's explicit persona activation into the existing local selection-history log and the opt-in-gated telemetry sender, matching the activation contract shared by every client surface; both calls are best-effort and never fail the command. Adds RegressionGate::evaluate_cross_version with a typed CrossVersionDecision (Pass, Regression, IntegrityFailure, MissingBaseline, InvalidScore), leaving evaluate_upgrade untouched. Client::install evaluates it before overwriting an installed persona and records the outcome on InstallReport as an additive optional field; the CLI prints a warning for non-clean outcomes. All outcomes are warn-only -- installs are never blocked. Also corrects selection.rs and lib.rs docs that claimed the history log feeds intelligent selection; that learning lives in the orchestrator's preferences store. --- Cargo.lock | 4 + crates/frameshift-cli/Cargo.toml | 4 + crates/frameshift-cli/src/cmd/use_persona.rs | 58 +++ crates/frameshift-cli/src/main.rs | 105 +++++ crates/frameshift-client/Cargo.toml | 4 + crates/frameshift-client/src/lib.rs | 399 ++++++++++++++++++- crates/frameshift-client/src/model.rs | 16 +- crates/frameshift-client/src/registry.rs | 6 +- crates/frameshift-client/src/selection.rs | 14 +- crates/frameshift-conformance/src/gate.rs | 288 +++++++++++++ crates/frameshift-conformance/src/lib.rs | 8 +- 11 files changed, 895 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2f7ecde..5b13cd6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1749,6 +1749,8 @@ dependencies = [ "tempfile", "thiserror 2.0.18", "tokio", + "tracing", + "tracing-subscriber", ] [[package]] @@ -1761,6 +1763,7 @@ dependencies = [ "flate2", "frameshift-catalog", "frameshift-compose", + "frameshift-conformance", "frameshift-pack", "frameshift-source", "hex", @@ -2012,6 +2015,7 @@ dependencies = [ "secrecy", "serde", "serde_json", + "tempfile", "thiserror 2.0.18", "tokio", "toml 0.8.23", diff --git a/crates/frameshift-cli/Cargo.toml b/crates/frameshift-cli/Cargo.toml index 45ebc55..b90d2a6 100644 --- a/crates/frameshift-cli/Cargo.toml +++ b/crates/frameshift-cli/Cargo.toml @@ -24,6 +24,10 @@ clap = { workspace = true } serde_json = { workspace = true } thiserror = { workspace = true } tokio = { workspace = true } +# Structured logging for best-effort paths (e.g. telemetry/selection-history +# recording) that must warn on failure without failing the command. +tracing = { workspace = true } +tracing-subscriber = { workspace = true } [features] # Semantic selection via a local candle sentence-embedding model. Off by diff --git a/crates/frameshift-cli/src/cmd/use_persona.rs b/crates/frameshift-cli/src/cmd/use_persona.rs index 7f34f96..95aad0c 100644 --- a/crates/frameshift-cli/src/cmd/use_persona.rs +++ b/crates/frameshift-cli/src/cmd/use_persona.rs @@ -100,6 +100,10 @@ pub fn run_use(client: &Client, args: UseArgs) -> Result<(), CliError> { } } + // Record this activation to the local selection-history audit log and + // (only if the project has opted in) send anonymous selection telemetry. + record_use_selection_and_telemetry(client, &project_root, &args.name); + // Read and print the rendered persona for the claude target. let rendered = client.rendered_persona(&project_root, &args.name, "claude")?; println!("{}", rendered); @@ -141,9 +145,36 @@ fn record_persona_use(prefs_path: &Path, persona: &str) -> Result<(), String> { prefs.save(prefs_path).map_err(|e| e.to_string()) } +/// Record `persona`'s explicit activation to the local selection-history +/// audit log and, only if the project has opted in, send anonymous selection +/// telemetry for it. +/// +/// Mirrors the activation contract shared by every FrameShift client +/// surface: a session id of `":"`, `auto = false` since this +/// is an explicit user choice (not an automatic pick), and no rationale. +/// `Client::send_telemetry_for_persona` +/// no-ops internally unless `ProjectConfig.telemetry_opt_in` is set, so a +/// stock CLI invocation never phones home. +/// +/// Both calls are best-effort: `run_use` has already committed the activation +/// by the time this runs, so a history-log or telemetry failure must never +/// fail the command. Failures are logged via `tracing::warn!` and swallowed. +fn record_use_selection_and_telemetry(client: &Client, project_root: &Path, persona: &str) { + let session = format!("cli:{}", std::process::id()); + let history_result = + client.record_selection_event(project_root, persona, &session, false, None); + if let Err(error) = history_result { + tracing::warn!(persona, %error, "record_selection_event failed"); + } + if let Err(error) = client.send_telemetry_for_persona(project_root, persona, &session) { + tracing::warn!(persona, %error, "send_telemetry_for_persona failed"); + } +} + #[cfg(test)] mod tests { use super::*; + use frameshift_client::ClientOptions; use tempfile::TempDir; /// Recording a use bumps the persona's bias and persists it to the shared @@ -176,4 +207,31 @@ mod tests { assert!(second >= first, "bias should not decrease on repeated use"); } + + /// record_use_selection_and_telemetry appends exactly one local + /// selection-history event for the activated persona (`auto = false`, + /// matching an explicit user choice), and never panics or errors even + /// though telemetry stays disabled -- the default `telemetry_opt_in = + /// false` makes `send_telemetry_for_persona` a silent no-op with no + /// network access attempted. + #[test] + fn record_use_selection_and_telemetry_writes_history_event() { + let tmp = TempDir::new().unwrap(); + let client = Client::new(ClientOptions { + data_root: tmp.path().join("data"), + config_root: None, + }); + let project_root = tmp.path().join("project"); + std::fs::create_dir_all(&project_root).unwrap(); + + record_use_selection_and_telemetry(&client, &project_root, "rust"); + + let state_dir = client.orchestrator_state_dir(&project_root).unwrap(); + let history_path = state_dir.join(frameshift_client::SELECTION_HISTORY_FILENAME); + let raw = std::fs::read_to_string(&history_path).unwrap(); + let lines: Vec<&str> = raw.lines().collect(); + assert_eq!(lines.len(), 1, "expected exactly one recorded event"); + assert!(lines[0].contains("\"persona\":\"rust\"")); + assert!(lines[0].contains("\"auto\":false")); + } } diff --git a/crates/frameshift-cli/src/main.rs b/crates/frameshift-cli/src/main.rs index bae6e36..8ae93c2 100644 --- a/crates/frameshift-cli/src/main.rs +++ b/crates/frameshift-cli/src/main.rs @@ -11,6 +11,7 @@ mod util; use clap::{Parser, Subcommand}; use std::path::PathBuf; use std::process::ExitCode; +use tracing_subscriber::EnvFilter; use frameshift_client::{Client, InstallRequest, InstallSource, PersonaSpec}; @@ -175,6 +176,15 @@ impl From for RunError { /// - 1: general error (I/O, parse, patch conflict, etc.) /// - 2: feature not yet implemented (M2+ stubs) fn main() -> ExitCode { + // Initialize structured tracing output (mirrors frameshift-daemon/mcp/server). + // Silent by default (`RUST_LOG` unset -> only ERROR-level events surface); + // set `RUST_LOG=warn` to see best-effort warnings such as a failed + // telemetry send or selection-history write. + tracing_subscriber::fmt() + .with_env_filter(EnvFilter::from_default_env()) + .with_writer(std::io::stderr) + .init(); + match run() { Ok(()) => ExitCode::SUCCESS, Err(RunError::NotImplemented(msg)) => { @@ -243,6 +253,15 @@ fn run() -> Result<(), RunError> { "installed {}@{} ({})", report.persona.name, report.persona.version, report.persona.hash ); + // Surface the additive, warn-only cross-version conformance + // comparison computed during install. `report.conformance_upgrade` + // is `None` for a fresh install; installation has already + // succeeded by this point regardless of what the decision says. + if let Some(decision) = &report.conformance_upgrade { + if let Some(message) = conformance_upgrade_warning(&report.persona.name, decision) { + eprintln!("warning: {message}"); + } + } Ok(()) } @@ -444,3 +463,89 @@ fn run() -> Result<(), RunError> { fn current_dir() -> Result { std::env::current_dir().map_err(|e| RunError::General(e.to_string())) } + +/// Build the human-readable warning message for a cross-version +/// conformance-baseline comparison that is not clean, per +/// `frameshift_conformance::CrossVersionDecision` (re-exported as +/// `frameshift_client::CrossVersionDecision`). Returns `None` for `Pass` and +/// `MissingBaseline`, which are expected, non-fatal outcomes with nothing to +/// report. +/// +/// Returning a message rather than printing directly keeps this testable +/// without capturing stderr. This is purely informational: `Client::install` +/// has already committed the install by the time the caller prints this, so +/// nothing here can or should block anything -- see +/// `evaluate_conformance_upgrade` in `frameshift-client/src/lib.rs` for the +/// blocking-semantics rationale. +fn conformance_upgrade_warning( + persona: &str, + decision: &frameshift_client::CrossVersionDecision, +) -> Option { + use frameshift_client::CrossVersionDecision; + match decision { + CrossVersionDecision::Pass | CrossVersionDecision::MissingBaseline { .. } => None, + CrossVersionDecision::Regression { delta } => Some(format!( + "{persona}'s conformance baseline dropped by {delta:.3} relative to the \ + version it replaced (install not blocked)" + )), + CrossVersionDecision::IntegrityFailure { + declared_hash, + actual_hash, + } => Some(format!( + "{persona}'s shipped conformance baseline failed integrity verification \ + (declared hash {declared_hash}, actual {actual_hash:?}); install not blocked" + )), + CrossVersionDecision::InvalidScore => Some(format!( + "{persona}'s conformance baseline score is invalid; cannot evaluate this \ + upgrade (install not blocked)" + )), + } +} + +#[cfg(test)] +mod conformance_warning_tests { + use super::*; + use frameshift_client::CrossVersionDecision; + + /// `Pass` and `MissingBaseline` are expected, non-fatal outcomes: no + /// message should be printed for either. + #[test] + fn no_message_for_clean_outcomes() { + assert!(conformance_upgrade_warning("rust", &CrossVersionDecision::Pass).is_none()); + assert!(conformance_upgrade_warning( + "rust", + &CrossVersionDecision::MissingBaseline { + installed_present: true, + incoming_present: false, + } + ) + .is_none()); + } + + /// Every non-clean variant produces a message naming the persona and + /// stating that the install was not blocked. + #[test] + fn message_for_every_non_clean_variant() { + let regression = + conformance_upgrade_warning("rust", &CrossVersionDecision::Regression { delta: 0.2 }) + .unwrap(); + assert!(regression.contains("rust")); + assert!(regression.contains("not blocked")); + + let integrity = conformance_upgrade_warning( + "rust", + &CrossVersionDecision::IntegrityFailure { + declared_hash: "abc".to_string(), + actual_hash: Some("tampered".to_string()), + }, + ) + .unwrap(); + assert!(integrity.contains("rust")); + assert!(integrity.contains("not blocked")); + + let invalid = + conformance_upgrade_warning("rust", &CrossVersionDecision::InvalidScore).unwrap(); + assert!(invalid.contains("rust")); + assert!(invalid.contains("not blocked")); + } +} diff --git a/crates/frameshift-client/Cargo.toml b/crates/frameshift-client/Cargo.toml index dd8fa13..bdd7e9f 100644 --- a/crates/frameshift-client/Cargo.toml +++ b/crates/frameshift-client/Cargo.toml @@ -12,6 +12,10 @@ frameshift-pack = { path = "../frameshift-pack" } frameshift-compose = { path = "../frameshift-compose" } # Typed persona source loading and deterministic markdown rendering. frameshift-source = { path = "../frameshift-source" } +# Cross-version conformance-baseline comparison for the install-over-existing +# path (RegressionGate::evaluate_cross_version); default features only, no +# cli-runner (no `agy`/tokio-process dependency needed here). +frameshift-conformance = { path = "../frameshift-conformance" } base64.workspace = true ed25519-dalek.workspace = true hex.workspace = true diff --git a/crates/frameshift-client/src/lib.rs b/crates/frameshift-client/src/lib.rs index b31f747..229c22d 100644 --- a/crates/frameshift-client/src/lib.rs +++ b/crates/frameshift-client/src/lib.rs @@ -10,6 +10,10 @@ mod registry; mod selection; pub use error::ClientError; +/// Re-exported so callers of [`InstallReport::conformance_upgrade`] can match +/// on the decision variants without adding their own `frameshift-conformance` +/// dependency just to name the type. +pub use frameshift_conformance::CrossVersionDecision; pub use model::{ ClientOptions, GcReport, InstallReport, InstallRequest, InstallSource, LockedPersona, Lockfile, MemoryConfig, MemoryRequirementStatus, PersonaSpec, ProjectConfig, ProjectPaths, SyncReport, @@ -194,8 +198,15 @@ impl Client { } }; + // Best-effort, warn-only comparison of shipped conformance baselines + // against any version of this persona already installed for the + // project. Must be evaluated before `finish_install` overwrites the + // previously-installed persona directory. Never fails the install -- + // see `evaluate_conformance_upgrade`. + let conformance_upgrade = evaluate_conformance_upgrade(&paths, &locked); + // Shared tail: upsert into lockfile and materialize project state. - finish_install(self, &paths, locked) + finish_install(self, &paths, locked, conformance_upgrade) } pub fn activate(&self, project_root: &Path, persona: &str) -> Result<(), ClientError> { @@ -316,8 +327,10 @@ impl Client { } /// Append a persona-selection event to the project's local selection history - /// (`projects//selection-history.jsonl`). Local-only state that the - /// intelligent-selection feature learns from; it is never sent anywhere. + /// (`projects//selection-history.jsonl`). Local-only, write-only audit + /// log: it is never sent anywhere and nothing in this codebase reads it back. + /// The intelligent-selection feature learns from a separate mechanism, the + /// `Preferences` store in `automate-prefs.json` (see `frameshift_orchestrator`). /// `auto` distinguishes automatic selections from explicit user choices, and /// `reason` is an optional rationale. pub fn record_selection_event( @@ -1238,11 +1251,16 @@ fn read_dir_sorted(path: &Path) -> Result, ClientError> { /// the lockfile, and materialize project state. Both the LocalPath and Registry /// arms call this after producing their locked persona. /// +/// `conformance_upgrade` is threaded straight through into the returned +/// [`InstallReport`]; it was already computed by the caller (before this +/// function overwrites any previously-installed persona directory). +/// /// Returns an [`InstallReport`] on success. fn finish_install( client: &Client, paths: &ProjectPaths, locked: LockedPersona, + conformance_upgrade: Option, ) -> Result { let mut lockfile = load_lockfile(&paths.lock_path)?.unwrap_or_default(); upsert_locked_persona(&mut lockfile, locked.clone()); @@ -1253,6 +1271,7 @@ fn finish_install( project_id: paths.project_id.clone(), cache_path: paths.cache_dir.join(&locked.hash), persona: locked, + conformance_upgrade, }) } @@ -1268,6 +1287,113 @@ fn install_from_registry( registry::fetch_and_install(spec, paths) } +/// Best-effort, non-blocking comparison of `locked`'s shipped conformance +/// baseline against the baseline of any version of the same persona already +/// installed for this project. +/// +/// Returns `None` when there is no previously-installed version of +/// `locked.name` (a fresh install, not an upgrade) -- this is the only +/// signal `Client::install` needs to decide whether a cross-version +/// comparison applies at all. When there is a previous version, delegates +/// to `frameshift_conformance::RegressionGate::evaluate_cross_version` and +/// logs the result via `tracing`: `Regression`/`IntegrityFailure`/ +/// `InvalidScore` at `warn!` (all warn-only -- installation is never +/// blocked), `MissingBaseline` at `debug!` (an expected, non-fatal outcome +/// for packs that ship no baseline). +/// +/// Must be called *before* `finish_install`/`materialize_project_state` +/// overwrites the previously-installed persona directory, since this reads +/// the old `pack.toml` from exactly that location. +fn evaluate_conformance_upgrade( + paths: &ProjectPaths, + locked: &LockedPersona, +) -> Option { + let installed_manifest_path = paths + .personas_dir + .join(&locked.name) + .join("source") + .join("pack.toml"); + if !installed_manifest_path.is_file() { + // Fresh install: no previous version of this persona to compare against. + return None; + } + + let installed_baseline = read_pack_conformance_baseline(&installed_manifest_path); + + let new_pack_dir = paths.cache_dir.join(&locked.hash); + let incoming_baseline = read_pack_conformance_baseline(&new_pack_dir.join("pack.toml")); + let incoming_actual_bundle_hash = + frameshift_conformance::load_from_dir(&new_pack_dir.join("conformance")) + .ok() + .and_then(|bundle| frameshift_conformance::bundle_hash(&bundle).ok()); + + let decision = frameshift_conformance::RegressionGate::evaluate_cross_version( + installed_baseline.as_ref(), + incoming_baseline.as_ref(), + incoming_actual_bundle_hash.as_deref(), + ); + + match &decision { + frameshift_conformance::CrossVersionDecision::Pass => {} + frameshift_conformance::CrossVersionDecision::Regression { delta } => { + warn!( + persona = %locked.name, + version = %locked.version, + delta, + "conformance regression detected on upgrade (warn-only, install not blocked)" + ); + } + frameshift_conformance::CrossVersionDecision::IntegrityFailure { + declared_hash, + actual_hash, + } => { + warn!( + persona = %locked.name, + version = %locked.version, + declared_hash, + actual_hash = ?actual_hash, + "incoming pack's conformance baseline failed integrity verification \ + (warn-only, install not blocked)" + ); + } + frameshift_conformance::CrossVersionDecision::MissingBaseline { + installed_present, + incoming_present, + } => { + debug!( + persona = %locked.name, + installed_present, + incoming_present, + "no cross-version conformance-baseline comparison possible for this upgrade" + ); + } + frameshift_conformance::CrossVersionDecision::InvalidScore => { + warn!( + persona = %locked.name, + version = %locked.version, + "conformance baseline score is invalid; cannot evaluate upgrade \ + (warn-only, install not blocked)" + ); + } + } + + Some(decision) +} + +/// Read and parse `manifest_path` as a [`frameshift_pack::PackManifest`], +/// returning its `conformance_baseline` if present. +/// +/// Any I/O or parse failure yields `None` silently: this backs an advisory, +/// best-effort check (`evaluate_conformance_upgrade`) that must never fail +/// `Client::install`. +fn read_pack_conformance_baseline( + manifest_path: &Path, +) -> Option { + let raw = fs::read_to_string(manifest_path).ok()?; + let manifest: frameshift_pack::PackManifest = toml::from_str(&raw).ok()?; + manifest.conformance_baseline +} + #[cfg(test)] mod tests { use super::*; @@ -1652,4 +1778,271 @@ mod tests { "no infra overlay expected" ); } + + // ---- evaluate_conformance_upgrade / InstallReport.conformance_upgrade ---- + + /// Build a pack directory whose `pack.toml` declares `[conformance_baseline]` + /// with the given `score`. When `ship_matching_bundle` is true, also writes a + /// real `conformance/bundle.toml` and sets `bundle_hash` to that bundle's + /// actual computed hash (integrity passes); when false, no bundle is shipped + /// at all and `bundle_hash` is a valid-looking but unverifiable placeholder + /// (integrity fails via the "no bundle shipped" path). + fn write_pack_with_baseline( + pack_dir: &Path, + name: &str, + version: &str, + score: f32, + ship_matching_bundle: bool, + ) { + use frameshift_conformance::{ + bundle_hash, ExpectedBehavior, ScorerKind, TestBundle, TestCase, + }; + + let bundle = TestBundle { + name: name.to_string(), + version: version.to_string(), + tests: vec![TestCase { + id: "case-1".to_string(), + prompt: "hello".to_string(), + expected: ExpectedBehavior::Contains { + value: "hi".to_string(), + }, + scorer: ScorerKind::Substring, + }], + }; + let actual_hash = bundle_hash(&bundle).unwrap(); + + fs::create_dir_all(pack_dir).unwrap(); + fs::write( + pack_dir.join("pack.toml"), + format!( + "schema_version = 1\nname = \"{name}\"\nauthor_handle = \"test\"\nauthor_pubkey = \"deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef\"\nversion = \"{version}\"\n\n[conformance_baseline]\nscore = {score}\nbundle_hash = \"{actual_hash}\"\n" + ), + ) + .unwrap(); + fs::write(pack_dir.join("AGENTS.md"), format!("# {name}\n\nTest.\n")).unwrap(); + + if ship_matching_bundle { + let conformance_dir = pack_dir.join("conformance"); + fs::create_dir_all(&conformance_dir).unwrap(); + fs::write( + conformance_dir.join("bundle.toml"), + toml::to_string(&bundle).unwrap(), + ) + .unwrap(); + } + } + + /// A fresh install (no persona of this name previously installed for the + /// project) reports no conformance-upgrade comparison at all, even though + /// the pack ships a baseline -- there is nothing to compare against yet. + #[test] + fn fresh_install_has_no_conformance_upgrade_report() { + let tmp = tempfile::tempdir().unwrap(); + let pack_dir = tmp.path().join("pack"); + write_pack_with_baseline(&pack_dir, "conform-fresh", "1.0.0", 0.9, true); + + let project_root = tmp.path().join("project"); + fs::create_dir_all(&project_root).unwrap(); + let client = Client::new(ClientOptions { + data_root: tmp.path().join("data"), + config_root: None, + }); + + let report = client + .install(InstallRequest { + project_root, + spec: PersonaSpec { + name: "conform-fresh".to_string(), + version: "1.0.0".to_string(), + }, + source: InstallSource::LocalPath(pack_dir), + }) + .unwrap(); + + assert_eq!(report.conformance_upgrade, None); + } + + /// Installing a lower-scoring version over an already-installed, + /// integrity-verified version reports `Regression` with the correct delta. + #[test] + fn upgrade_with_lower_score_reports_regression() { + let tmp = tempfile::tempdir().unwrap(); + let project_root = tmp.path().join("project"); + fs::create_dir_all(&project_root).unwrap(); + let client = Client::new(ClientOptions { + data_root: tmp.path().join("data"), + config_root: None, + }); + + let old_pack_dir = tmp.path().join("old-pack"); + write_pack_with_baseline(&old_pack_dir, "conform-regress", "1.0.0", 0.9, true); + client + .install(InstallRequest { + project_root: project_root.clone(), + spec: PersonaSpec { + name: "conform-regress".to_string(), + version: "1.0.0".to_string(), + }, + source: InstallSource::LocalPath(old_pack_dir), + }) + .unwrap(); + + let new_pack_dir = tmp.path().join("new-pack"); + write_pack_with_baseline(&new_pack_dir, "conform-regress", "2.0.0", 0.5, true); + let report = client + .install(InstallRequest { + project_root, + spec: PersonaSpec { + name: "conform-regress".to_string(), + version: "2.0.0".to_string(), + }, + source: InstallSource::LocalPath(new_pack_dir), + }) + .unwrap(); + + match report.conformance_upgrade { + Some(frameshift_conformance::CrossVersionDecision::Regression { delta }) => { + assert!((delta - 0.4).abs() < 1e-6, "delta was {delta}"); + } + other => panic!("expected Some(Regression), got {other:?}"), + } + } + + /// Installing a higher-scoring, integrity-verified version over an + /// already-installed version reports `Pass` -- and, crucially, never + /// blocks or fails the install itself. + #[test] + fn upgrade_with_higher_score_reports_pass() { + let tmp = tempfile::tempdir().unwrap(); + let project_root = tmp.path().join("project"); + fs::create_dir_all(&project_root).unwrap(); + let client = Client::new(ClientOptions { + data_root: tmp.path().join("data"), + config_root: None, + }); + + let old_pack_dir = tmp.path().join("old-pack"); + write_pack_with_baseline(&old_pack_dir, "conform-pass", "1.0.0", 0.5, true); + client + .install(InstallRequest { + project_root: project_root.clone(), + spec: PersonaSpec { + name: "conform-pass".to_string(), + version: "1.0.0".to_string(), + }, + source: InstallSource::LocalPath(old_pack_dir), + }) + .unwrap(); + + let new_pack_dir = tmp.path().join("new-pack"); + write_pack_with_baseline(&new_pack_dir, "conform-pass", "2.0.0", 0.9, true); + let report = client + .install(InstallRequest { + project_root, + spec: PersonaSpec { + name: "conform-pass".to_string(), + version: "2.0.0".to_string(), + }, + source: InstallSource::LocalPath(new_pack_dir), + }) + .unwrap(); + + assert_eq!( + report.conformance_upgrade, + Some(frameshift_conformance::CrossVersionDecision::Pass) + ); + } + + /// An incoming pack that declares a baseline but ships no + /// `conformance/bundle.toml` to verify it against reports + /// `IntegrityFailure` with `actual_hash: None`, never `Pass` or + /// `Regression` -- and the install still succeeds (warn-only). + #[test] + fn upgrade_with_unshipped_bundle_reports_integrity_failure() { + let tmp = tempfile::tempdir().unwrap(); + let project_root = tmp.path().join("project"); + fs::create_dir_all(&project_root).unwrap(); + let client = Client::new(ClientOptions { + data_root: tmp.path().join("data"), + config_root: None, + }); + + let old_pack_dir = tmp.path().join("old-pack"); + write_pack_with_baseline(&old_pack_dir, "conform-noverify", "1.0.0", 0.5, true); + client + .install(InstallRequest { + project_root: project_root.clone(), + spec: PersonaSpec { + name: "conform-noverify".to_string(), + version: "1.0.0".to_string(), + }, + source: InstallSource::LocalPath(old_pack_dir), + }) + .unwrap(); + + // ship_matching_bundle = false: pack.toml claims a baseline but no + // conformance/bundle.toml is written to back it up. + let new_pack_dir = tmp.path().join("new-pack"); + write_pack_with_baseline(&new_pack_dir, "conform-noverify", "2.0.0", 0.9, false); + let report = client + .install(InstallRequest { + project_root, + spec: PersonaSpec { + name: "conform-noverify".to_string(), + version: "2.0.0".to_string(), + }, + source: InstallSource::LocalPath(new_pack_dir), + }) + .unwrap(); + + match report.conformance_upgrade { + Some(frameshift_conformance::CrossVersionDecision::IntegrityFailure { + actual_hash, + .. + }) => { + assert_eq!(actual_hash, None); + } + other => panic!("expected Some(IntegrityFailure), got {other:?}"), + } + } + + /// Upgrading a persona that has never shipped a baseline (old or new) + /// reports `MissingBaseline`, and the install still succeeds. + #[test] + fn upgrade_without_any_baseline_reports_missing_baseline() { + let tmp = tempfile::tempdir().unwrap(); + let (client, project_root) = install_test_persona(&tmp, "conform-nobaseline"); + + // install_test_persona's pack has no [conformance_baseline] section. + let new_pack_dir = tmp.path().join("new-pack-nobaseline"); + fs::create_dir_all(&new_pack_dir).unwrap(); + fs::write( + new_pack_dir.join("pack.toml"), + "schema_version = 1\nname = \"conform-nobaseline\"\nauthor_handle = \"test\"\nauthor_pubkey = \"deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef\"\nversion = \"0.2.0\"\n", + ) + .unwrap(); + fs::write(new_pack_dir.join("AGENTS.md"), "# conform-nobaseline v2\n").unwrap(); + + let report = client + .install(InstallRequest { + project_root, + spec: PersonaSpec { + name: "conform-nobaseline".to_string(), + version: "0.2.0".to_string(), + }, + source: InstallSource::LocalPath(new_pack_dir), + }) + .unwrap(); + + assert_eq!( + report.conformance_upgrade, + Some( + frameshift_conformance::CrossVersionDecision::MissingBaseline { + installed_present: false, + incoming_present: false, + } + ) + ); + } } diff --git a/crates/frameshift-client/src/model.rs b/crates/frameshift-client/src/model.rs index 1f0f65a..b4900d2 100644 --- a/crates/frameshift-client/src/model.rs +++ b/crates/frameshift-client/src/model.rs @@ -194,11 +194,25 @@ pub struct ProjectPaths { pub personas_dir: PathBuf, } -#[derive(Debug, Clone, PartialEq, Eq)] +/// Outcome of [`crate::Client::install`]. +/// +/// `conformance_upgrade` is additive: it carries a best-effort, warn-only +/// comparison of the incoming pack's shipped conformance baseline against +/// the previously-installed version's baseline (see +/// `frameshift_conformance::RegressionGate::evaluate_cross_version`). +/// No derive of `Eq` here (unlike the other report types) because +/// `CrossVersionDecision` carries `f32` score deltas, which are not `Eq`. +#[derive(Debug, Clone, PartialEq)] pub struct InstallReport { pub project_id: String, pub persona: LockedPersona, pub cache_path: PathBuf, + /// Cross-version conformance-baseline comparison against the version + /// previously installed for this project, when there was one. `None` + /// for a fresh install (no prior version) or when the comparison could + /// not be attempted for any reason -- this field is advisory only and + /// never blocks or fails the install. See [`crate::Client::install`]. + pub conformance_upgrade: Option, } #[derive(Debug, Clone, PartialEq, Eq)] diff --git a/crates/frameshift-client/src/registry.rs b/crates/frameshift-client/src/registry.rs index 61b4294..83a6552 100644 --- a/crates/frameshift-client/src/registry.rs +++ b/crates/frameshift-client/src/registry.rs @@ -36,7 +36,11 @@ use crate::model::{LockedPersona, PersonaSpec, ProjectPaths}; pub const REGISTRY_URL_ENV: &str = "FRAMESHIFT_REGISTRY_URL"; /// Default registry base URL used when [`REGISTRY_URL_ENV`] is not set. -const DEFAULT_REGISTRY_URL: &str = "https://frameshift.syntheos.dev"; +/// +/// The public registry API lives on the `-api` subdomain; every FrameShift +/// client surface points there. The bare `frameshift.syntheos.dev` host does +/// not serve the API -- do not point this back at it. +const DEFAULT_REGISTRY_URL: &str = "https://frameshift-api.syntheos.dev"; /// Maximum number of decompressed bytes we will accept from a registry pack archive. /// diff --git a/crates/frameshift-client/src/selection.rs b/crates/frameshift-client/src/selection.rs index 27d9b57..683604a 100644 --- a/crates/frameshift-client/src/selection.rs +++ b/crates/frameshift-client/src/selection.rs @@ -2,10 +2,16 @@ //! //! Two distinct concerns live here, deliberately separated by privacy posture: //! -//! - **Selection history** is a per-project, append-only JSONL log of which -//! persona was selected and why. It is written to the central project state -//! directory and never leaves the machine; the intelligent-selection feature -//! reads it back to learn from past choices. +//! - **Selection history** is a per-project, append-only, write-only JSONL +//! audit log of which persona was selected and why. It is written to the +//! central project state directory and never leaves the machine. Nothing in +//! this codebase reads it back today -- it is not the mechanism the +//! intelligent-selection feature learns from. That mechanism is the +//! separate `Preferences` store persisted to `automate-prefs.json` +//! (`frameshift_orchestrator::Preferences`, written by the CLI's `use`, +//! `feedback`, and `prefs` commands and read by `select`/`automate`). +//! `selection-history.jsonl` exists purely as a local record for future +//! analysis or export. //! - **Telemetry** is the optional, off-by-default network side. It is sent //! only when the project has explicitly opted in *and* a telemetry endpoint is //! configured via the environment. There is intentionally no default diff --git a/crates/frameshift-conformance/src/gate.rs b/crates/frameshift-conformance/src/gate.rs index 113b097..8b991af 100644 --- a/crates/frameshift-conformance/src/gate.rs +++ b/crates/frameshift-conformance/src/gate.rs @@ -15,6 +15,60 @@ pub enum GateDecision { 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?" +#[derive(Debug, Clone, PartialEq)] +pub enum CrossVersionDecision { + /// The incoming version's shipped baseline meets or exceeds the + /// installed version's shipped baseline, and the incoming baseline's + /// integrity check passed. + Pass, + /// The incoming version's baseline score is below the installed + /// version's baseline score by `delta` (positive value). + Regression { + /// `installed_baseline.score - incoming_baseline.score`, always positive. + delta: f32, + }, + /// The incoming pack's declared `conformance_baseline.bundle_hash` does not + /// match the hash actually computed from its own shipped + /// `conformance/bundle.toml`, or no such bundle is shipped at all (in + /// which case `actual_hash` is `None`). Either way the incoming pack's + /// claimed score cannot be verified against the bundle it was supposedly + /// measured on, so it must not be trusted to pass an upgrade -- checked + /// and reported before any score comparison, independent of whether the + /// installed version has a baseline to compare against. + IntegrityFailure { + /// The bundle hash declared in the incoming pack's `pack.toml`. + declared_hash: String, + /// The hash actually computed from the incoming pack's shipped + /// bundle, or `None` if the incoming pack ships no + /// `conformance/bundle.toml` at all. + actual_hash: Option, + }, + /// The installed version, the incoming version, or both ship no + /// `[conformance_baseline]`, so there is nothing to compare. + /// + /// This is explicitly non-fatal: shipping a conformance baseline is + /// optional, and an upgrade must never be blocked merely because + /// historical baseline data happens to be absent on one or both sides. + MissingBaseline { + /// Whether the currently-installed version ships a baseline. + installed_present: bool, + /// Whether the incoming version ships a baseline. + incoming_present: bool, + }, + /// A baseline score was non-finite or outside `0.0..=1.0`; the gate fails + /// closed rather than letting a malformed baseline slip through. + InvalidScore, +} + /// Stateless evaluator. The runtime constructs one per upgrade attempt. pub struct RegressionGate; @@ -49,6 +103,79 @@ impl RegressionGate { } 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. + /// + /// Evaluation order (first match wins): + /// + /// 1. **Integrity** -- if `incoming_baseline` is present, its declared + /// `bundle_hash` must equal `incoming_actual_bundle_hash` (the hash + /// actually computed from the incoming pack's own shipped + /// `conformance/bundle.toml`). A mismatch, or no bundle shipped at all + /// (`incoming_actual_bundle_hash` is `None`), yields + /// [`CrossVersionDecision::IntegrityFailure`] immediately -- an + /// unverifiable claim is rejected regardless of whether the installed + /// version has a baseline to compare against. + /// 2. **Missing baseline** -- if either side has no baseline (including + /// the incoming side having passed step 1 vacuously because it has no + /// baseline to check), yields [`CrossVersionDecision::MissingBaseline`]. + /// Not fatal: baselines are optional. + /// 3. **Score validity** -- both scores must be finite and within + /// `0.0..=1.0`, else [`CrossVersionDecision::InvalidScore`] (fail closed). + /// 4. **Regression** -- `incoming_baseline.score < installed_baseline.score` + /// yields [`CrossVersionDecision::Regression`]; otherwise + /// [`CrossVersionDecision::Pass`]. + pub fn evaluate_cross_version( + installed_baseline: Option<&ConformanceBaseline>, + incoming_baseline: Option<&ConformanceBaseline>, + incoming_actual_bundle_hash: Option<&str>, + ) -> CrossVersionDecision { + let Some(incoming) = incoming_baseline else { + return CrossVersionDecision::MissingBaseline { + installed_present: installed_baseline.is_some(), + incoming_present: false, + }; + }; + + // Integrity check takes priority: an incoming baseline that cannot be + // verified against its own shipped bundle is untrustworthy on its own + // terms, independent of whether we even have an installed baseline + // to compare it to. + match incoming_actual_bundle_hash { + Some(actual) if actual == incoming.bundle_hash => {} + other => { + return CrossVersionDecision::IntegrityFailure { + declared_hash: incoming.bundle_hash.clone(), + actual_hash: other.map(str::to_string), + }; + } + } + + let Some(installed) = installed_baseline else { + return CrossVersionDecision::MissingBaseline { + installed_present: false, + incoming_present: true, + }; + }; + + // Fail closed on non-finite or out-of-range scores, mirroring + // evaluate_upgrade's guard against a malformed baseline slipping + // through a NaN comparison. + let valid = |s: f32| s.is_finite() && (0.0..=1.0).contains(&s); + if !valid(installed.score) || !valid(incoming.score) { + return CrossVersionDecision::InvalidScore; + } + + if incoming.score < installed.score { + return CrossVersionDecision::Regression { + delta: installed.score - incoming.score, + }; + } + + CrossVersionDecision::Pass + } } #[cfg(test)] @@ -109,4 +236,165 @@ mod tests { 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] + fn cross_version_passes_when_incoming_exceeds_installed() { + let installed = baseline(0.7, "abc"); + let incoming = baseline(0.9, "abc"); + let decision = + RegressionGate::evaluate_cross_version(Some(&installed), Some(&incoming), Some("abc")); + assert_eq!(decision, CrossVersionDecision::Pass); + } + + /// An equal score also passes (not a regression). + #[test] + fn cross_version_passes_when_incoming_equals_installed() { + let installed = baseline(0.7, "abc"); + let incoming = baseline(0.7, "abc"); + let decision = + RegressionGate::evaluate_cross_version(Some(&installed), Some(&incoming), Some("abc")); + assert_eq!(decision, CrossVersionDecision::Pass); + } + + /// Incoming score below the installed baseline reports a regression with + /// the correct positive delta. + #[test] + fn cross_version_reports_regression() { + let installed = baseline(0.9, "abc"); + let incoming = baseline(0.7, "abc"); + let decision = + RegressionGate::evaluate_cross_version(Some(&installed), Some(&incoming), Some("abc")); + match decision { + CrossVersionDecision::Regression { delta } => { + assert!((delta - 0.2).abs() < 1e-6, "delta was {delta}"); + } + other => panic!("expected Regression, got {other:?}"), + } + } + + /// The incoming pack's declared bundle_hash not matching its own actual + /// shipped bundle hash is an integrity failure, even though the score + /// would otherwise pass. + #[test] + fn cross_version_integrity_failure_on_hash_mismatch() { + let installed = baseline(0.5, "abc"); + let incoming = baseline(0.9, "abc"); + let decision = RegressionGate::evaluate_cross_version( + Some(&installed), + Some(&incoming), + Some("tampered"), + ); + assert_eq!( + decision, + CrossVersionDecision::IntegrityFailure { + declared_hash: "abc".to_string(), + actual_hash: Some("tampered".to_string()), + } + ); + } + + /// An incoming baseline with no shipped bundle at all (no + /// `conformance/bundle.toml`) is also an integrity failure -- a claimed + /// score with nothing to verify it against is untrustworthy. + #[test] + fn cross_version_integrity_failure_on_missing_bundle() { + let installed = baseline(0.5, "abc"); + let incoming = baseline(0.9, "abc"); + let decision = + RegressionGate::evaluate_cross_version(Some(&installed), Some(&incoming), None); + assert_eq!( + decision, + CrossVersionDecision::IntegrityFailure { + declared_hash: "abc".to_string(), + actual_hash: None, + } + ); + } + + /// Integrity failure on the incoming side is reported even when the + /// installed side has no baseline at all -- the check does not require + /// a comparison partner. + #[test] + fn cross_version_integrity_failure_takes_priority_over_missing_installed() { + let incoming = baseline(0.9, "abc"); + let decision = + RegressionGate::evaluate_cross_version(None, Some(&incoming), Some("tampered")); + assert_eq!( + decision, + CrossVersionDecision::IntegrityFailure { + declared_hash: "abc".to_string(), + actual_hash: Some("tampered".to_string()), + } + ); + } + + /// No incoming baseline at all: MissingBaseline, regardless of whether + /// the installed side has one. + #[test] + fn cross_version_missing_baseline_when_incoming_absent() { + let installed = baseline(0.5, "abc"); + let decision = RegressionGate::evaluate_cross_version(Some(&installed), None, None); + assert_eq!( + decision, + CrossVersionDecision::MissingBaseline { + installed_present: true, + incoming_present: false, + } + ); + + let decision_both_absent = RegressionGate::evaluate_cross_version(None, None, None); + assert_eq!( + decision_both_absent, + CrossVersionDecision::MissingBaseline { + installed_present: false, + incoming_present: false, + } + ); + } + + /// Incoming baseline present and verified, but no installed baseline to + /// compare against: MissingBaseline, not Pass or Regression. + #[test] + fn cross_version_missing_baseline_when_installed_absent() { + let incoming = baseline(0.9, "abc"); + let decision = RegressionGate::evaluate_cross_version(None, Some(&incoming), Some("abc")); + assert_eq!( + decision, + CrossVersionDecision::MissingBaseline { + installed_present: false, + incoming_present: true, + } + ); + } + + /// Non-finite or out-of-range scores on either side fail closed, once + /// past the integrity and missing-baseline checks. + #[test] + fn cross_version_fails_closed_on_invalid_score() { + let installed_nan = baseline(f32::NAN, "abc"); + let incoming = baseline(0.9, "abc"); + assert_eq!( + RegressionGate::evaluate_cross_version( + Some(&installed_nan), + Some(&incoming), + Some("abc") + ), + CrossVersionDecision::InvalidScore + ); + + let installed = baseline(0.5, "abc"); + let incoming_out_of_range = baseline(1.5, "abc"); + assert_eq!( + RegressionGate::evaluate_cross_version( + Some(&installed), + Some(&incoming_out_of_range), + Some("abc") + ), + CrossVersionDecision::InvalidScore + ); + } } diff --git a/crates/frameshift-conformance/src/lib.rs b/crates/frameshift-conformance/src/lib.rs index d35001c..d8419bf 100644 --- a/crates/frameshift-conformance/src/lib.rs +++ b/crates/frameshift-conformance/src/lib.rs @@ -7,7 +7,11 @@ //! - Upgrade-regression gate ([`gate`]) //! //! The runtime invokes a [`Runner`] for each [`TestCase`] in a [`TestBundle`], -//! produces a [`Score`], and feeds it to the [`RegressionGate`] during upgrades. +//! produces a [`Score`], and can feed it to [`RegressionGate::evaluate_upgrade`] +//! during upgrades. 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. pub mod bundle; pub mod caller; @@ -25,6 +29,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::{GateDecision, RegressionGate}; +pub use gate::{CrossVersionDecision, GateDecision, RegressionGate}; pub use runner::{MockRunner, Runner}; pub use score::{bundle_score, score_test, Score}; From e8f013a181b3153998ed22d4486c177c3ae2446e Mon Sep 17 00:00:00 2001 From: GhostFrame Date: Sat, 11 Jul 2026 16:02:29 -0400 Subject: [PATCH 02/10] fix(daemon): make the shutdown RPC actually stop the server The watch channel's sender was created and immediately discarded, so the shutdown method returned its canned reply and closed that one connection while the accept loop ran forever. The sender is now threaded into serve() and cloned into each connection; a shutdown RPC flips the flag and the accept loop exits. Covered by an end-to-end socket test. --- crates/frameshift-daemon/src/main.rs | 12 ++-- crates/frameshift-daemon/src/socket.rs | 88 +++++++++++++++++++++++--- 2 files changed, 88 insertions(+), 12 deletions(-) diff --git a/crates/frameshift-daemon/src/main.rs b/crates/frameshift-daemon/src/main.rs index d81d0fe..6d2872b 100644 --- a/crates/frameshift-daemon/src/main.rs +++ b/crates/frameshift-daemon/src/main.rs @@ -69,8 +69,10 @@ async fn main() -> Result<(), Box> { std::fs::set_permissions(&socket_path, std::fs::Permissions::from_mode(0o600))?; tracing::info!(path = %socket_path.display(), "daemon listening"); - // Shutdown signalling channel; `serve` watches this for `true`. - let (_shutdown_tx, shutdown_rx) = tokio::sync::watch::channel(false); + // Shutdown signalling channel. `serve`'s accept loop watches `shutdown_rx` + // for `true`; `shutdown_tx` is cloned into every accepted connection so a + // `shutdown` RPC on any one of them can flip the flag and stop the loop. + let (shutdown_tx, shutdown_rx) = tokio::sync::watch::channel(false); // Start file watcher on the data root so the daemon can react to external changes. // Events are forwarded to the orchestrator evaluation hook; the watcher is kept @@ -113,8 +115,10 @@ async fn main() -> Result<(), Box> { }; // Drive the JSON-RPC server loop. The watcher handle above stays alive for - // the duration of this call. - frameshift_daemon::socket::serve(listener, client, shutdown_rx).await; + // the duration of this call. `shutdown_tx` is threaded through so that a + // `shutdown` RPC received on any connection can signal the accept loop + // to stop. + frameshift_daemon::socket::serve(listener, client, shutdown_rx, shutdown_tx).await; // Best-effort socket cleanup on graceful shutdown. let _ = std::fs::remove_file(&socket_path); diff --git a/crates/frameshift-daemon/src/socket.rs b/crates/frameshift-daemon/src/socket.rs index f6dd91b..bbcbfda 100644 --- a/crates/frameshift-daemon/src/socket.rs +++ b/crates/frameshift-daemon/src/socket.rs @@ -16,11 +16,14 @@ use tokio::sync::watch; /// /// The function returns when either the shutdown watch channel is set to /// `true` or the listener itself errors. Each accepted connection is driven -/// in its own independent tokio task. +/// in its own independent tokio task and is given a clone of `shutdown_tx` so +/// that a `shutdown` RPC received on that connection can flip the watch +/// channel and stop this accept loop. pub async fn serve( listener: UnixListener, client: Arc, - mut shutdown: watch::Receiver, + mut shutdown_rx: watch::Receiver, + shutdown_tx: watch::Sender, ) { loop { tokio::select! { @@ -50,8 +53,8 @@ pub async fn serve( } } let client = Arc::clone(&client); - let shutdown_tx_clone = shutdown.clone(); - tokio::spawn(handle_connection(stream, client, shutdown_tx_clone)); + let conn_shutdown_tx = shutdown_tx.clone(); + tokio::spawn(handle_connection(stream, client, conn_shutdown_tx)); } Err(err) => { tracing::error!(error = %err, "accept error; stopping server loop"); @@ -60,8 +63,8 @@ pub async fn serve( } } // Observe shutdown signal. - _ = shutdown.changed() => { - if *shutdown.borrow() { + _ = shutdown_rx.changed() => { + if *shutdown_rx.borrow() { tracing::info!("shutdown signal received; stopping accept loop"); break; } @@ -74,11 +77,13 @@ pub async fn serve( /// /// Reads newline-delimited JSON lines, dispatches each to `handler::dispatch`, /// writes the response, and stops when the connection closes or the client -/// sends a `shutdown` method call. +/// sends a `shutdown` method call. On a `shutdown` call this also sends +/// `true` on `shutdown_tx`, which signals the `serve` accept loop (running in +/// a different task) to stop accepting new connections. async fn handle_connection( stream: tokio::net::UnixStream, client: Arc, - _shutdown: watch::Receiver, + shutdown_tx: watch::Sender, ) { let (read_half, mut write_half) = stream.into_split(); let reader = BufReader::new(read_half); @@ -127,7 +132,74 @@ async fn handle_connection( } if is_shutdown { + // Signal the accept loop to stop. The send only fails if every + // receiver (the `serve` loop and every other connection's clone) + // has already been dropped, meaning the server is already gone; + // that error carries no actionable information here. + let _ = shutdown_tx.send(true); break; } } } + +#[cfg(test)] +mod tests { + use super::*; + use frameshift_client::ClientOptions; + use tokio::net::UnixStream; + use tokio::time::{timeout, Duration}; + + /// Build a test Client backed by a temporary directory. + fn test_client(tmp: &tempfile::TempDir) -> Client { + Client::new(ClientOptions { + data_root: tmp.path().to_path_buf(), + config_root: None, + }) + } + + /// Verify that sending the `shutdown` RPC on one connection both returns + /// the canned `{"shutting_down": true}` reply and causes `serve`'s accept + /// loop to return. This is the end-to-end proof that the watch channel is + /// actually wired up, rather than the RPC merely closing the calling + /// connection while the server keeps accepting new ones. + #[tokio::test] + async fn shutdown_rpc_terminates_serve() { + let data_dir = tempfile::tempdir().unwrap(); + let client = Arc::new(test_client(&data_dir)); + + let socket_dir = tempfile::tempdir().unwrap(); + let socket_path = socket_dir.path().join("daemon.sock"); + let listener = UnixListener::bind(&socket_path).expect("bind should succeed"); + + let (shutdown_tx, shutdown_rx) = watch::channel(false); + let serve_handle = tokio::spawn(serve(listener, client, shutdown_rx, shutdown_tx)); + + // Connect and send a `shutdown` request. + let mut stream = UnixStream::connect(&socket_path) + .await + .expect("connect should succeed"); + stream + .write_all(b"{\"jsonrpc\":\"2.0\",\"id\":1,\"method\":\"shutdown\"}\n") + .await + .expect("write should succeed"); + + // Read the canned reply back to confirm the RPC was handled normally. + let mut reader = BufReader::new(&mut stream); + let mut response_line = String::new(); + timeout(Duration::from_secs(2), reader.read_line(&mut response_line)) + .await + .expect("response should arrive within 2 seconds") + .expect("read should succeed"); + let response: serde_json::Value = + serde_json::from_str(response_line.trim()).expect("response should be valid JSON"); + assert_eq!(response["result"]["shutting_down"], true); + + // The accept loop in `serve` must observe the shutdown signal and + // return on its own; if the sender were never wired up this join + // would hang and the timeout below would fail the test. + timeout(Duration::from_secs(2), serve_handle) + .await + .expect("serve() should return after a shutdown RPC") + .expect("serve task should not panic"); + } +} From 24b20022b34f5dfb4ea15170a854ad024e5593d9 Mon Sep 17 00:00:00 2001 From: GhostFrame Date: Sat, 11 Jul 2026 16:02:29 -0400 Subject: [PATCH 03/10] fix(orchestrator): index anti-patterns and make section weighting real PersonaProfile::from_source dropped anti-pattern and general-pattern text from the keyword corpus entirely. High-signal section extraction ignored heading levels and its output was deduplicated away against the prepended full body, making the weighting inert; signal sections now lead the corpus so extraction order matters. --- crates/frameshift-orchestrator/src/index.rs | 221 ++++++++++++++++++-- 1 file changed, 201 insertions(+), 20 deletions(-) diff --git a/crates/frameshift-orchestrator/src/index.rs b/crates/frameshift-orchestrator/src/index.rs index 962d6d3..c3ccc42 100644 --- a/crates/frameshift-orchestrator/src/index.rs +++ b/crates/frameshift-orchestrator/src/index.rs @@ -34,7 +34,8 @@ pub struct PersonaProfile { /// Deduplicated lowercase keyword tokens extracted from name, description, /// voice (tone + text), anchor texts, rule texts, skill `invoke_when` fields, - /// and pattern category/items. Stopwords and short tokens removed. + /// pattern category/items, antipattern text/replacement/reasoning, and + /// general-pattern text. Stopwords and short tokens removed. pub keywords: Vec, /// Required tools declared in the capability manifest (empty if none). @@ -151,6 +152,23 @@ impl PersonaProfile { text_parts.push(ex.language.clone()); text_parts.push(ex.context.clone()); } + // Anti-patterns and general patterns are first-class pattern categories + // (schema, validation, merge, conflict, and render all treat them as + // such) so their text must feed the same keyword corpus as stack items + // and examples do, or a persona whose distinguishing content lives here + // is systematically under-ranked during selection. + for antipattern in &src.patterns.antipatterns { + text_parts.push(antipattern.text.clone()); + if let Some(use_instead) = &antipattern.use_instead { + text_parts.push(use_instead.clone()); + } + if let Some(reasoning) = &antipattern.reasoning { + text_parts.push(reasoning.clone()); + } + } + for pattern in &src.patterns.patterns { + text_parts.push(pattern.text.clone()); + } let combined = text_parts.join(" "); let keywords = extract_keywords(&combined); @@ -202,8 +220,15 @@ impl PersonaProfile { /// `dir` must contain `AGENTS.md`. `pack.toml` is optional but used for /// name, description, and capability_manifest when present. The markdown /// body is tokenized for keywords; high-signal sections (L2 anchor, Tech - /// Stack, Concrete Patterns, Operating Frame) are weighted by including - /// their tokens twice. Language detection runs the language lexicon over + /// Stack, Concrete Patterns, Operating Frame) are weighted by being + /// prepended to the corpus ahead of the full body. `extract_keywords` + /// dedups by first-seen order, so a section's tokens must be the first + /// occurrence it sees to matter at all -- appending them after a body that + /// already contains the same text (the sections are extracted from the + /// body, so it always does) would contribute nothing. Prepending instead + /// makes the tokens land first in the resulting keyword vector, which is + /// what `persona_text` (policy.rs) and any future length-limited embedder + /// rely on for priority. Language detection runs the language lexicon over /// the resulting keyword set. pub fn from_agents_md(dir: &Path) -> Result { let agents_md_path = dir.join("AGENTS.md"); @@ -228,16 +253,21 @@ impl PersonaProfile { .unwrap_or_else(|| "unknown".to_string()) }); - // Build keyword corpus: always include the full body. - // High-signal sections are included twice for weighting. - let mut corpus = body.clone(); + // Build keyword corpus. High-signal sections go first so their tokens + // are the first occurrence `extract_keywords` records (see doc comment + // above); the persona name follows for the same reason, then the full + // body supplies everything else. Appending the sections after the body + // instead (the previous behavior) made them inert: every token they + // contain already occurred once when the body was scanned. + let mut corpus = String::new(); for section in extract_high_signal_sections(&body) { - corpus.push(' '); corpus.push_str(§ion); + corpus.push(' '); } // Include the persona name itself for self-match. - corpus.push(' '); corpus.push_str(&name); + corpus.push(' '); + corpus.push_str(&body); // Fold curated pack.toml metadata into the corpus so the persona's // description and topical tags bias keyword-based selection alongside // the AGENTS.md body. Empty values contribute nothing. @@ -434,7 +464,11 @@ const KNOWN_LANGUAGES: &[&str] = &[ /// /// Scans `body` for headings containing any of the high-signal keywords /// (case-insensitive). Returns the text under each matching heading until the -/// next heading of the same or higher level. +/// next heading of the same or higher level -- a deeper subheading (e.g. a +/// `###` nested under a matching `##`) does not end the section; its own +/// heading text is folded into the captured content instead, so subsections +/// like "### Anti-patterns (do NOT use)" nested under "## Concrete Patterns" +/// still contribute their tokens. fn extract_high_signal_sections(body: &str) -> Vec { const HIGH_SIGNAL: &[&str] = &[ "l2 anchor", @@ -454,24 +488,38 @@ fn extract_high_signal_sections(body: &str) -> Vec { for line in body.lines() { if line.starts_with('#') { - // Save the previous section if it was a signal section. - if current_is_signal && !current_section.is_empty() { - sections.push(current_section.clone()); - } - current_section.clear(); - // Compute heading level (number of leading '#' chars). let level = line.chars().take_while(|c| *c == '#').count(); - let heading_text = line.trim_start_matches('#').trim().to_lowercase(); - current_heading_level = level; - current_is_signal = HIGH_SIGNAL.iter().any(|s| heading_text.contains(s)); + // A heading only ends the current signal section when its level is + // the same as or shallower than (numerically <=) the level that + // opened the section. A deeper heading is a subsection of the + // signal content, not a sibling boundary. + if current_is_signal && level <= current_heading_level { + if !current_section.is_empty() { + sections.push(current_section.clone()); + } + current_section.clear(); + current_is_signal = false; + } + + if current_is_signal { + // Nested subheading inside an already-open signal section: + // fold its own heading text into the captured content rather + // than treating it as a new section boundary. + current_section.push_str(line); + current_section.push('\n'); + } else { + // Either no section was open, or the one that was open just + // closed above -- evaluate this heading as a fresh candidate. + let heading_text = line.trim_start_matches('#').trim().to_lowercase(); + current_heading_level = level; + current_is_signal = HIGH_SIGNAL.iter().any(|s| heading_text.contains(s)); + } } else if current_is_signal { current_section.push_str(line); current_section.push('\n'); } - // Suppress unused warning on level variable (used for context only). - let _ = current_heading_level; } // Capture trailing section. @@ -874,4 +922,137 @@ mod tests { assert!(names.contains(&"rust"), "rust persona must be indexed"); assert!(names.contains(&"typed"), "typed persona must be indexed"); } + + /// from_source feeds anti-pattern text (text, use_instead, reasoning) and + /// general-pattern text into the keyword corpus, not just stack/examples. + #[test] + fn profile_includes_antipattern_and_general_pattern_text_in_keywords() { + use frameshift_source::*; + let mut src = minimal_source("crypto-guard", "wary and precise"); + src.patterns.antipatterns = vec![AntiPattern { + id: "no-openssl".to_string(), + text: "roll your own xchachapoly implementation".to_string(), + use_instead: Some("ring or rustcrypto primitives".to_string()), + reasoning: Some("handrolled ciphers leak timing sidechannels".to_string()), + }]; + src.patterns.patterns = vec![GeneralPattern { + id: "config-lookup".to_string(), + text: "envelopeencryption keyrotation schedule".to_string(), + }]; + + let profile = PersonaProfile::from_source(&src); + + assert!( + profile.keywords.iter().any(|k| k == "xchachapoly"), + "antipattern.text should be in keywords; got: {:?}", + profile.keywords + ); + assert!( + profile.keywords.iter().any(|k| k == "rustcrypto"), + "antipattern.use_instead should be in keywords; got: {:?}", + profile.keywords + ); + assert!( + profile.keywords.iter().any(|k| k == "sidechannels"), + "antipattern.reasoning should be in keywords; got: {:?}", + profile.keywords + ); + assert!( + profile.keywords.iter().any(|k| k == "envelopeencryption"), + "general pattern text should be in keywords; got: {:?}", + profile.keywords + ); + } + + /// extract_high_signal_sections must not close a section on a deeper + /// subheading: content nested under "### Signing" and "### Anti-patterns" + /// inside a matching "## Concrete Patterns" section belongs to that one + /// section, mirroring how render.rs actually nests generated documents. + #[test] + fn extract_high_signal_sections_captures_nested_subheadings() { + let body = "## Concrete Patterns\n\n### Signing\n\ned25519dalek\n\n### Anti-patterns (do NOT use)\n\nrot13warning\n\n## Some Other Heading\n\nirrelevant\n"; + + let sections = extract_high_signal_sections(body); + + assert_eq!( + sections.len(), + 1, + "expected exactly one captured section, got: {:?}", + sections + ); + assert!( + sections[0].contains("ed25519dalek"), + "content nested under a ### subheading must survive; got: {:?}", + sections[0] + ); + assert!( + sections[0].contains("rot13warning"), + "content nested under a second ### subheading must survive; got: {:?}", + sections[0] + ); + assert!( + !sections[0].contains("irrelevant"), + "a sibling ## heading (same level as the opener) must still close the section; got: {:?}", + sections[0] + ); + } + + /// A same-or-shallower heading closes the section as documented, so two + /// consecutive top-level signal headings produce two separate sections + /// rather than merging into one. + #[test] + fn extract_high_signal_sections_closes_on_same_level_heading() { + let body = "## Tech Stack\n\nzzalpha\n\n## Concrete Patterns\n\nzzbravo\n"; + + let sections = extract_high_signal_sections(body); + + assert_eq!( + sections.len(), + 2, + "expected two sections, got: {:?}", + sections + ); + assert!(sections[0].contains("zzalpha")); + assert!(sections[1].contains("zzbravo")); + } + + /// from_agents_md previously appended high-signal sections after the full + /// body, so extract_keywords's first-seen dedup meant they contributed + /// nothing (every token already occurred once in the body). Prepending + /// them means signal-section-only tokens now appear earlier in the + /// resulting keyword vector than tokens that occur only outside a signal + /// section -- proof the weighting is no longer inert. + #[test] + fn from_agents_md_high_signal_tokens_precede_body_only_tokens() { + let tmp = TempDir::new().unwrap(); + let dir = tmp.path().join("weighted"); + fs::create_dir_all(&dir).unwrap(); + + fs::write( + dir.join("AGENTS.md"), + "# Weighted Context\n\nzzalpha zzbravo zzcharlie\n\n## Tech Stack\n\nzzdelta zzecho zzfoxtrot\n", + ) + .unwrap(); + + let profile = PersonaProfile::from_agents_md(&dir).unwrap(); + + let idx_signal = profile + .keywords + .iter() + .position(|k| k == "zzdelta") + .expect("zzdelta (from the Tech Stack signal section) must be a keyword"); + let idx_body_only = profile + .keywords + .iter() + .position(|k| k == "zzalpha") + .expect("zzalpha (body-only, non-signal) must be a keyword"); + + assert!( + idx_signal < idx_body_only, + "high-signal token 'zzdelta' (index {}) should precede body-only token 'zzalpha' (index {}); got: {:?}", + idx_signal, + idx_body_only, + profile.keywords + ); + } } From 2a494d82a1c7502187eb6c35674b80ea2add7ec8 Mon Sep 17 00:00:00 2001 From: GhostFrame Date: Sat, 11 Jul 2026 16:02:29 -0400 Subject: [PATCH 04/10] feat(growth): wire the JSONL layer into append, migrate, and the CLI The structured JSONL growth layer existed with zero callers. append() now dual-writes: markdown first (unchanged behavior), then a best-effort structured entry; a JSONL failure after a successful markdown write propagates as the call's error. frameshift migrate gains a growth-log pass (idempotent per persona), and grow log / grow summary expose the structured history. The silent chmod failure on the growth file now logs. --- crates/frameshift-cli/src/cmd/grow.rs | 253 ++++++++++++++++++++++- crates/frameshift-cli/src/cmd/migrate.rs | 147 +++++++++++++ crates/frameshift-growth/src/lib.rs | 112 +++++++++- 3 files changed, 508 insertions(+), 4 deletions(-) diff --git a/crates/frameshift-cli/src/cmd/grow.rs b/crates/frameshift-cli/src/cmd/grow.rs index 6fc533e..e6db50d 100644 --- a/crates/frameshift-cli/src/cmd/grow.rs +++ b/crates/frameshift-cli/src/cmd/grow.rs @@ -1,7 +1,14 @@ -//! CLI handler for the `grow append` subcommand. +//! CLI handler for the `frameshift grow ` subcommand. +//! +//! `append` writes to the persona's growth log (both the legacy markdown file +//! and the structured JSONL file, via `frameshift_growth::append`). `log` and +//! `summary` are the read surfaces over the structured JSONL data, backed by +//! `frameshift_growth::recent_entries` and `frameshift_growth::summarize` +//! respectively. use crate::util::CliError; use clap::Args; +use frameshift_growth::Scope; /// Arguments for the grow subcommand. #[derive(Debug, Args)] @@ -16,6 +23,12 @@ pub struct GrowArgs { pub enum GrowAction { /// Append a growth entry for a persona. Append(AppendArgs), + + /// Show the most recent structured growth entries for a persona. + Log(LogArgs), + + /// Print an algorithmic summary of a persona's growth entries. + Summary(SummaryArgs), } /// Arguments for grow append. @@ -30,14 +43,61 @@ pub struct AppendArgs { pub text: String, } +/// Arguments for grow log. +#[derive(Debug, Args)] +pub struct LogArgs { + /// Name of the persona whose growth log to read. + #[arg(long)] + pub persona: String, + + /// Maximum number of entries to print, most recent first. + #[arg(long, default_value_t = 10)] + pub limit: usize, +} + +/// Arguments for grow summary. +#[derive(Debug, Args)] +pub struct SummaryArgs { + /// Name of the persona whose growth log to summarize. + #[arg(long)] + pub persona: String, + + /// Scope of entries to summarize. + #[arg(long, value_enum, default_value = "project")] + pub scope: ScopeArg, +} + +/// CLI-facing mirror of `frameshift_growth::Scope` so `clap::ValueEnum` can +/// be derived without adding a `clap` dependency to the growth crate. +#[derive(Debug, Clone, Copy, clap::ValueEnum)] +pub enum ScopeArg { + /// Learning specific to the current project. + Project, + /// Universal learning applicable across projects. + Global, +} + +impl From for Scope { + /// Convert the CLI-facing scope argument into the growth crate's `Scope`. + fn from(arg: ScopeArg) -> Self { + match arg { + ScopeArg::Project => Scope::Project, + ScopeArg::Global => Scope::Global, + } + } +} + /// Execute the grow subcommand. pub fn run(args: GrowArgs) -> Result<(), CliError> { match args.action { GrowAction::Append(append_args) => run_append(append_args), + GrowAction::Log(log_args) => run_log(log_args), + GrowAction::Summary(summary_args) => run_summary(summary_args), } } -/// Execute grow append -- write a timestamped entry to the persona's growth.md. +/// Execute grow append -- write a timestamped entry to the persona's growth.md +/// and structured growth.jsonl (via `frameshift_growth::append`'s dual-write). fn run_append(args: AppendArgs) -> Result<(), CliError> { let client = frameshift_client::Client::with_default_data_root()?; let project_root = std::env::current_dir() @@ -50,3 +110,192 @@ fn run_append(args: AppendArgs) -> Result<(), CliError> { println!("Growth entry appended for persona '{}'.", args.persona); Ok(()) } + +/// Execute grow log -- print the persona's most recent structured growth +/// entries (project and global scope combined, newest first). +fn run_log(args: LogArgs) -> 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)?; + + let entries = frameshift_growth::recent_entries( + client.data_root(), + &project_id, + &args.persona, + args.limit, + ) + .map_err(|e| CliError::Growth(e.to_string()))?; + + print_entries(&args.persona, &entries); + Ok(()) +} + +/// Print a human-readable rendering of `entries` for `persona`, or a +/// "no entries" message when the log is empty. +fn print_entries(persona: &str, entries: &[frameshift_growth::GrowthEntry]) { + if entries.is_empty() { + println!("no growth entries recorded for persona '{persona}'."); + return; + } + + for entry in entries { + let scope = match entry.scope { + Scope::Project => "project", + Scope::Global => "global", + }; + let mut tags = vec![scope.to_string()]; + if entry.auto_selected { + tags.push("auto-selected".to_string()); + } + if let Some(intent) = &entry.intent { + tags.push(format!("intent={intent}")); + } + println!("{} [{}]", entry.ts, tags.join(", ")); + println!(" {}", entry.text); + } +} + +/// Execute grow summary -- print the algorithmic summary of a persona's +/// growth entries for the requested scope. +fn run_summary(args: SummaryArgs) -> 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)?; + + let summary = frameshift_growth::summarize( + client.data_root(), + &project_id, + &args.persona, + args.scope.into(), + ) + .map_err(|e| CliError::Growth(e.to_string()))?; + + if summary.is_empty() { + println!( + "no growth entries to summarize for persona '{}'.", + args.persona + ); + } else { + println!("{summary}"); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use clap::Parser; + use frameshift_growth::GrowthEntry; + + /// Helper command that hosts `GrowAction` so clap can parse a full + /// `frameshift grow ...` invocation without depending on the + /// real top-level `Command` enum. + #[derive(Debug, Parser)] + #[command(name = "frameshift", no_binary_name = true)] + struct TestCli { + #[command(subcommand)] + action: GrowAction, + } + + /// `grow append --persona

--text ` parses both required flags. + #[test] + fn parse_append_with_persona_and_text() { + let parsed = TestCli::try_parse_from(["append", "--persona", "rust", "--text", "hi"]) + .expect("append should parse"); + match parsed.action { + GrowAction::Append(args) => { + assert_eq!(args.persona, "rust"); + assert_eq!(args.text, "hi"); + } + other => panic!("expected Append, got {other:?}"), + } + } + + /// `grow log --persona

` parses with the default limit of 10. + #[test] + fn parse_log_defaults_limit_to_ten() { + let parsed = + TestCli::try_parse_from(["log", "--persona", "rust"]).expect("log should parse"); + match parsed.action { + GrowAction::Log(args) => { + assert_eq!(args.persona, "rust"); + assert_eq!(args.limit, 10); + } + other => panic!("expected Log, got {other:?}"), + } + } + + /// `grow log --persona

--limit ` overrides the default limit. + #[test] + fn parse_log_with_explicit_limit() { + let parsed = TestCli::try_parse_from(["log", "--persona", "rust", "--limit", "3"]) + .expect("log should parse"); + match parsed.action { + GrowAction::Log(args) => assert_eq!(args.limit, 3), + other => panic!("expected Log, got {other:?}"), + } + } + + /// `grow summary --persona

` parses with the default scope of project. + #[test] + fn parse_summary_defaults_scope_to_project() { + let parsed = TestCli::try_parse_from(["summary", "--persona", "rust"]) + .expect("summary should parse"); + match parsed.action { + GrowAction::Summary(args) => assert!(matches!(args.scope, ScopeArg::Project)), + other => panic!("expected Summary, got {other:?}"), + } + } + + /// `grow summary --persona

--scope global` selects the global scope. + #[test] + fn parse_summary_with_explicit_global_scope() { + let parsed = TestCli::try_parse_from(["summary", "--persona", "rust", "--scope", "global"]) + .expect("summary should parse"); + match parsed.action { + GrowAction::Summary(args) => assert!(matches!(args.scope, ScopeArg::Global)), + other => panic!("expected Summary, got {other:?}"), + } + } + + /// `print_entries` reports a friendly message when there are no entries. + #[test] + fn print_entries_handles_empty_list() { + // No stdout capture available in this harness; this exercises the + // empty branch without panicking, which is the behavior under test. + print_entries("rust", &[]); + } + + /// `print_entries` does not panic on a populated entry list covering + /// every optional field (auto_selected, intent, both scopes). + #[test] + fn print_entries_handles_populated_list() { + let entries = vec![ + GrowthEntry { + ts: "2026-01-01T00:00:00Z".to_string(), + session: "1".to_string(), + project_id: "p".to_string(), + persona: "rust".to_string(), + auto_selected: true, + task: Some("fix bug".to_string()), + intent: Some("debugging".to_string()), + text: "learned something".to_string(), + scope: Scope::Project, + }, + GrowthEntry { + ts: "2026-01-02T00:00:00Z".to_string(), + session: "2".to_string(), + project_id: "p".to_string(), + persona: "rust".to_string(), + auto_selected: false, + task: None, + intent: None, + text: "a universal lesson".to_string(), + scope: Scope::Global, + }, + ]; + print_entries("rust", &entries); + } +} diff --git a/crates/frameshift-cli/src/cmd/migrate.rs b/crates/frameshift-cli/src/cmd/migrate.rs index a14f61e..e4864ec 100644 --- a/crates/frameshift-cli/src/cmd/migrate.rs +++ b/crates/frameshift-cli/src/cmd/migrate.rs @@ -5,6 +5,9 @@ //! central store. The migration is normally triggered as a side-effect of //! `client.project_paths()`; this command makes it explicit and prints a //! human-readable summary of what was moved. +//! +//! It also migrates each installed persona's legacy `growth.md` to the +//! structured `growth.jsonl` format via `frameshift_growth::migrate_growth_md`. use clap::Args; @@ -30,6 +33,13 @@ pub struct MigrateArgs {} /// Because the migration side-effect is wired inside `project_paths`, /// we do not need direct access to the private `migrate_legacy_project_files` /// function -- calling `project_paths` is sufficient. +/// +/// After the legacy-file migration, this also walks every persona installed +/// for the current project and migrates its legacy `growth.md` to the +/// structured `growth.jsonl` format via `frameshift_growth::migrate_growth_md`. +/// This step is idempotent: a persona is only migrated when its `growth.md` +/// exists and its `growth.jsonl` does not yet exist, so re-running `migrate` +/// never re-appends already-migrated entries. pub fn run_migrate(client: &Client, _args: MigrateArgs) -> Result<(), CliError> { let cwd = std::env::current_dir().map_err(|source| frameshift_client::ClientError::Io { path: std::path::PathBuf::from("."), @@ -43,5 +53,142 @@ pub fn run_migrate(client: &Client, _args: MigrateArgs) -> Result<(), CliError> println!("migrate: project id {}", paths.project_id); println!(" central store: {}", paths.project_state_dir.display()); println!(" legacy files checked and migrated if present"); + + // Migrate each installed persona's growth.md -> growth.jsonl, skipping + // any persona that has already been migrated (growth.jsonl present). + let personas = client.list_personas(&cwd)?; + let persona_names: Vec = personas.into_iter().map(|p| p.name).collect(); + let migrated = migrate_persona_growth_logs(client, &paths.project_id, &persona_names)?; + + if migrated.is_empty() { + println!(" growth: nothing to migrate"); + } else { + for (name, count) in &migrated { + println!(" growth: migrated {count} entries for persona '{name}'"); + } + } + Ok(()) } + +/// Migrate the legacy `growth.md` for each name in `persona_names` to the +/// structured `growth.jsonl` format, skipping any persona whose `growth.md` +/// is absent or whose `growth.jsonl` already exists. +/// +/// Returns the `(persona_name, migrated_entry_count)` pairs for personas that +/// were actually migrated by this call -- an empty vector means nothing was +/// migrated (either nothing to migrate, or everything was already migrated by +/// a previous run). Skipping personas with a pre-existing `growth.jsonl` is +/// what makes repeated `migrate` invocations idempotent: `migrate_growth_md` +/// itself appends every parsed entry unconditionally, so calling it again on +/// an already-migrated persona would duplicate every entry. +fn migrate_persona_growth_logs( + client: &Client, + project_id: &str, + persona_names: &[String], +) -> Result, CliError> { + let mut migrated = Vec::new(); + for name in persona_names { + let persona_dir = client + .data_root() + .join("projects") + .join(project_id) + .join("personas") + .join(name); + let md_path = persona_dir.join("growth.md"); + let jsonl_path = persona_dir.join("growth.jsonl"); + + if !md_path.exists() || jsonl_path.exists() { + continue; + } + + let count = frameshift_growth::migrate_growth_md(client.data_root(), project_id, name) + .map_err(|e| CliError::Growth(e.to_string()))?; + migrated.push((name.clone(), count)); + } + Ok(migrated) +} + +#[cfg(test)] +mod tests { + use super::*; + use frameshift_client::ClientOptions; + use std::fs; + use tempfile::TempDir; + + /// Builds a `Client` backed by a fresh temp data root and writes a legacy + /// `growth.md` for `persona` under `project_id` with two entries. + fn client_with_legacy_growth_md(project_id: &str, persona: &str) -> (TempDir, Client) { + let tmp = TempDir::new().unwrap(); + let client = Client::new(ClientOptions { + data_root: tmp.path().join("data"), + config_root: None, + }); + let persona_dir = client + .data_root() + .join("projects") + .join(project_id) + .join("personas") + .join(persona); + fs::create_dir_all(&persona_dir).unwrap(); + fs::write( + persona_dir.join("growth.md"), + "---\n\n\nfirst\n\n---\n\n\nsecond\n\n", + ) + .unwrap(); + (tmp, client) + } + + /// A persona with a legacy `growth.md` and no `growth.jsonl` yet is + /// migrated, and the returned count matches the number of parsed entries. + #[test] + fn migrates_persona_with_legacy_growth_md() { + let (_tmp, client) = client_with_legacy_growth_md("proj1", "rust"); + + let migrated = + migrate_persona_growth_logs(&client, "proj1", &["rust".to_string()]).unwrap(); + + assert_eq!(migrated, vec![("rust".to_string(), 2)]); + let jsonl_path = client + .data_root() + .join("projects/proj1/personas/rust/growth.jsonl"); + assert!(jsonl_path.exists()); + } + + /// Running the migration twice does not duplicate entries: the second + /// call sees the `growth.jsonl` already present and skips the persona. + #[test] + fn migration_is_idempotent() { + let (_tmp, client) = client_with_legacy_growth_md("proj1", "rust"); + + migrate_persona_growth_logs(&client, "proj1", &["rust".to_string()]).unwrap(); + let second = migrate_persona_growth_logs(&client, "proj1", &["rust".to_string()]).unwrap(); + + assert!( + second.is_empty(), + "second migration run must skip an already-migrated persona" + ); + let entries = frameshift_growth::read_entries( + client.data_root(), + "proj1", + "rust", + frameshift_growth::Scope::Project, + ) + .unwrap(); + assert_eq!(entries.len(), 2, "entries must not be duplicated"); + } + + /// A persona with no `growth.md` at all is skipped without error. + #[test] + fn skips_persona_with_no_legacy_growth_md() { + let tmp = TempDir::new().unwrap(); + let client = Client::new(ClientOptions { + data_root: tmp.path().join("data"), + config_root: None, + }); + + let migrated = + migrate_persona_growth_logs(&client, "proj1", &["rust".to_string()]).unwrap(); + assert!(migrated.is_empty()); + } +} diff --git a/crates/frameshift-growth/src/lib.rs b/crates/frameshift-growth/src/lib.rs index 0b12815..f4fa4b4 100644 --- a/crates/frameshift-growth/src/lib.rs +++ b/crates/frameshift-growth/src/lib.rs @@ -49,6 +49,22 @@ pub enum GrowthError { } /// Append a growth entry with the current UTC timestamp. +/// +/// This dual-writes to both formats: the legacy markdown `growth.md` (for +/// human readability and backward compatibility) and the structured +/// `growth.jsonl` (for the read surfaces in this crate -- `read_entries`, +/// `recent_entries`, `summarize`). The markdown write happens first; if it +/// succeeds but the subsequent JSONL write fails, this function returns the +/// JSONL error even though the markdown entry was already persisted. Callers +/// that need transactional all-or-nothing semantics across both files are not +/// supported by this function -- the markdown file is treated as the +/// source of truth for "did the append happen at all", and the JSONL file may +/// legitimately lag behind it if a JSONL write fails. +/// +/// Structured fields not derivable from these parameters (`auto_selected`, +/// `task`, `intent`) are populated with their default/`None` forms; richer +/// callers should call `append_jsonl` directly with a fully populated +/// `GrowthEntry` instead of relying on this best-effort projection. pub fn append( data_root: &Path, project_id: &str, @@ -56,7 +72,29 @@ pub fn append( entry_text: &str, ) -> Result<(), GrowthError> { let ts = format_utc_now(); - append_with_timestamp(data_root, project_id, persona_name, entry_text, &ts) + append_with_timestamp(data_root, project_id, persona_name, entry_text, &ts)?; + + let entry = GrowthEntry { + ts, + 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::Project, + }; + 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 +/// calling process (CLI invocation, daemon connection handler, etc.) and +/// requires no additional state to thread through `append`'s call sites. +fn current_session_id() -> String { + std::process::id().to_string() } /// Append a growth entry with a caller-supplied timestamp string. @@ -100,7 +138,11 @@ pub fn append_with_timestamp( #[cfg(unix)] { use std::os::unix::fs::PermissionsExt as _; - let _ = file.set_permissions(fs::Permissions::from_mode(0o600)); + file.set_permissions(fs::Permissions::from_mode(0o600)) + .map_err(|source| GrowthError::Io { + path: growth_path.clone(), + source, + })?; } writeln!( @@ -522,6 +564,49 @@ mod tests { assert_eq!(mode, 0o600, "growth file must be owner-only readable"); } + /// Appending to a pre-existing growth.md that was widened out-of-band + /// (e.g. by an umask change or manual chmod) must re-tighten the mode to + /// 0o600 rather than silently leaving it world-readable. + #[cfg(unix)] + #[test] + fn append_to_existing_file_retightens_permissions() { + use std::os::unix::fs::PermissionsExt; + let tmp = tempfile::tempdir().unwrap(); + append_with_timestamp( + tmp.path(), + "proj1", + "cryptographic", + "first entry", + "2026-01-01T00:00:00Z", + ) + .unwrap(); + let path = tmp + .path() + .join("projects/proj1/personas/cryptographic/growth.md"); + + // Simulate a widened pre-existing file (e.g. left over from an old + // umask) before the second append. + fs::set_permissions(&path, fs::Permissions::from_mode(0o644)).unwrap(); + + append_with_timestamp( + tmp.path(), + "proj1", + "cryptographic", + "second entry", + "2026-01-02T00:00:00Z", + ) + .unwrap(); + + let mode = fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode, 0o600, + "append to a pre-existing widened file must re-tighten to owner-only" + ); + let content = fs::read_to_string(&path).unwrap(); + assert!(content.contains("first entry")); + assert!(content.contains("second entry")); + } + #[test] fn append_accumulates_entries() { let tmp = tempfile::tempdir().unwrap(); @@ -548,6 +633,29 @@ mod tests { assert!(content.find("entry one") < content.find("entry two")); } + /// `append` (the legacy entry point) must dual-write: the markdown entry + /// lands in `growth.md` as before, and a best-effort `GrowthEntry` also + /// lands in `growth.jsonl` with unknown fields left as `None`/default. + #[test] + fn append_dual_writes_markdown_and_jsonl() { + let tmp = tempfile::tempdir().unwrap(); + append(tmp.path(), "proj1", "rust", "learned something").unwrap(); + + let md_path = tmp.path().join("projects/proj1/personas/rust/growth.md"); + assert!(md_path.exists()); + let md_content = fs::read_to_string(&md_path).unwrap(); + assert!(md_content.contains("learned something")); + + let entries = read_entries(tmp.path(), "proj1", "rust", Scope::Project).unwrap(); + assert_eq!(entries.len(), 1); + assert_eq!(entries[0].text, "learned something"); + assert_eq!(entries[0].scope, Scope::Project); + assert!(!entries[0].auto_selected); + assert!(entries[0].task.is_none()); + assert!(entries[0].intent.is_none()); + assert!(!entries[0].session.is_empty()); + } + #[test] fn append_rejects_traversal_in_persona_name() { let tmp = tempfile::tempdir().unwrap(); From 1a1e8e9fb84b2600979a2374686910c90c43714a Mon Sep 17 00:00:00 2001 From: GhostFrame Date: Sat, 11 Jul 2026 16:02:46 -0400 Subject: [PATCH 05/10] fix(seed): seed pack.toml-only personas end to end The persona gate required persona.toml or AGENTS.md, skipping every curated pack.toml-only directory; pack.toml is now a sufficient marker. Curated manifests ship a placeholder author_pubkey that the strict parser rejects, so the seeder repairs it in place with the real signing key via a surgical single-line rewrite that preserves comments, key order, and every other byte of the hand-written file. Marketplace metadata now comes from pack.toml description/tags first, with the legacy persona.toml/AGENTS.md derivation as fallback. --- crates/frameshift-seed/Cargo.toml | 3 + crates/frameshift-seed/src/main.rs | 432 ++++++++++++++++++++++++++++- 2 files changed, 420 insertions(+), 15 deletions(-) diff --git a/crates/frameshift-seed/Cargo.toml b/crates/frameshift-seed/Cargo.toml index 45b51fc..bd37c12 100644 --- a/crates/frameshift-seed/Cargo.toml +++ b/crates/frameshift-seed/Cargo.toml @@ -30,3 +30,6 @@ unicode-normalization = { workspace = true } toml = { workspace = true } diesel = { workspace = true } diesel-async = { workspace = true } + +[dev-dependencies] +tempfile = { workspace = true } diff --git a/crates/frameshift-seed/src/main.rs b/crates/frameshift-seed/src/main.rs index 2f27949..d603fe9 100644 --- a/crates/frameshift-seed/src/main.rs +++ b/crates/frameshift-seed/src/main.rs @@ -1,10 +1,13 @@ //! One-shot seeder for the frameshift catalog and object store. //! //! Reads persona directories from a configurable root path, builds a pack for -//! each directory that contains an `AGENTS.md` file plus a `pack.toml` manifest -//! (or synthesizes one), signs it with a generated Ed25519 key, stores the -//! canonical pack bytes in the object store, and registers the pack version and -//! author in the catalog. +//! each directory that carries a `pack.toml` manifest, a legacy `persona.toml`, +//! or an `AGENTS.md` file. A missing `pack.toml` is synthesized; a `pack.toml` +//! that already exists (as every curated `personas/*` directory does) has its +//! placeholder `author_pubkey` repaired in place so the strict manifest parser +//! can load it. The pack is then signed with a generated Ed25519 key, its +//! canonical bytes are stored in the object store, and the pack version and +//! author are registered in the catalog. //! //! # Usage //! @@ -43,7 +46,7 @@ use frameshift_catalog::{ use frameshift_catalog_postgres::{PostgresCatalog, PostgresCatalogConfig}; use frameshift_objects::PackStore; use frameshift_objects_fs::{FsPackStore, FsPackStoreConfig}; -use frameshift_pack::{ObjectHash, Pack}; +use frameshift_pack::{ObjectHash, Pack, PackManifest}; use secrecy::SecretString; use tracing::{error, info, warn}; @@ -148,9 +151,7 @@ async fn run() -> Result<(), SeedError> { continue; } - let persona_toml = path.join("persona.toml"); - let agents_md = path.join("AGENTS.md"); - if !persona_toml.exists() && !agents_md.exists() { + if !is_persona_dir(&path) { continue; } @@ -164,15 +165,21 @@ async fn run() -> Result<(), SeedError> { continue; } - // Synthesize a pack.toml if one does not exist. let pack_toml_path = path.join("pack.toml"); if !pack_toml_path.exists() { + // Synthesize a pack.toml if one does not exist. write_synthetic_pack_toml( &pack_toml_path, dir_name, &config.author_handle, &verifying_key, )?; + } else { + // Curated pack.toml files ship with a placeholder `author_pubkey` + // (e.g. "UNSIGNED") that fails PackManifest's strict 64-hex-char + // validator. Repair it in place with the real key so `Pack::from_dir` + // below can parse the manifest; all other fields are left untouched. + repair_placeholder_author_pubkey(&pack_toml_path, &verifying_key)?; } match seed_persona(&path, &catalog, &objects, &signing_key, author_pubkey).await { @@ -231,6 +238,18 @@ impl SeedConfig { } } +/// Whether a directory looks like a persona pack worth seeding. +/// +/// Any of the three marker files is sufficient: `pack.toml` (curated +/// `personas/*` directories in this repo are pack.toml-only by design), +/// a legacy `persona.toml`, or an `AGENTS.md`. `pack.toml` is synthesized +/// from the legacy files when it is the only one absent. +fn is_persona_dir(path: &Path) -> bool { + path.join("pack.toml").exists() + || path.join("persona.toml").exists() + || path.join("AGENTS.md").exists() +} + /// Derive a stable default key path that is namespaced by author handle. fn default_signing_key_path(object_store_root: &str, author_handle: &str) -> PathBuf { PathBuf::from(object_store_root) @@ -482,6 +501,98 @@ fn load_or_create_signing_key(path: &Path) -> Result { } } +/// Encode an Ed25519 verifying key as the 64-lowercase-hex-character string +/// that `PackManifest::author_pubkey` requires. +fn verifying_key_hex(verifying_key: &VerifyingKey) -> String { + verifying_key + .to_bytes() + .iter() + .map(|b| format!("{b:02x}")) + .collect() +} + +/// Whether `s` matches the strict encoding `PackManifest` requires for +/// `author_pubkey`: exactly 64 lowercase hex characters. Mirrors +/// `frameshift_pack::manifest`'s private validator so the repair check here +/// never disagrees with what the real parser will accept. +fn is_valid_pubkey_hex(s: &str) -> bool { + s.len() == 64 && s.bytes().all(|b| matches!(b, b'0'..=b'9' | b'a'..=b'f')) +} + +/// Repair a placeholder `author_pubkey` in an existing `pack.toml`. +/// +/// Curated repo personas (`personas/*/pack.toml`) ship with a literal +/// `"UNSIGNED"` placeholder for `author_pubkey` -- fine for humans reading the +/// file, but rejected by `PackManifest`'s deserializer, which requires exactly +/// 64 lowercase hex characters. Left unrepaired, `Pack::from_dir` fails to +/// parse every one of them. +/// +/// Curated pack.toml files are hand-written persona content: several carry +/// comments and deliberate key ordering that a parse-and-reserialize round +/// trip through `toml::Value` would destroy (comments dropped, keys +/// reordered). The rewrite is therefore a surgical single-line replacement: +/// only the root-table `author_pubkey = ...` line changes; every other byte +/// of the file is preserved verbatim. A no-op when the existing value already +/// satisfies the strict check; an error when the root table has no +/// `author_pubkey` key at all, since that file needs a human, not a seeder. +fn repair_placeholder_author_pubkey( + path: &Path, + verifying_key: &VerifyingKey, +) -> Result<(), SeedError> { + let content = std::fs::read_to_string(path)?; + let doc: toml::Value = toml::from_str(&content).map_err(|e| { + SeedError::Io(std::io::Error::other(format!( + "parse {} for author_pubkey repair: {e}", + path.display() + ))) + })?; + + let already_valid = doc + .get("author_pubkey") + .and_then(|v| v.as_str()) + .is_some_and(is_valid_pubkey_hex); + if already_valid { + return Ok(()); + } + + let hex = verifying_key_hex(verifying_key); + let mut in_root_table = true; + let mut replaced = false; + let mut rewritten = String::with_capacity(content.len() + 80); + for line in content.lines() { + let trimmed = line.trim_start(); + // `author_pubkey` is a root-table key; a same-named key inside a + // sub-table (however unlikely) must not be touched. + if trimmed.starts_with('[') { + in_root_table = false; + } + let is_pubkey_line = in_root_table + && !replaced + && trimmed + .strip_prefix("author_pubkey") + .is_some_and(|rest| rest.trim_start().starts_with('=')); + if is_pubkey_line { + rewritten.push_str(&format!("author_pubkey = \"{hex}\"")); + replaced = true; + } else { + rewritten.push_str(line); + } + rewritten.push('\n'); + } + if !replaced { + return Err(SeedError::Io(std::io::Error::other(format!( + "{}: no root-level author_pubkey key found to repair", + path.display() + )))); + } + if !content.ends_with('\n') { + rewritten.pop(); + } + std::fs::write(path, rewritten)?; + info!("repaired placeholder author_pubkey in {}", path.display()); + Ok(()) +} + /// Write a synthetic `pack.toml` manifest for a persona directory. /// /// The manifest is minimal but valid. The `author_pubkey` field is encoded as @@ -494,11 +605,7 @@ fn write_synthetic_pack_toml( author_handle: &str, verifying_key: &VerifyingKey, ) -> Result<(), SeedError> { - let pubkey_hex: String = verifying_key - .to_bytes() - .iter() - .map(|b| format!("{b:02x}")) - .collect(); + let pubkey_hex = verifying_key_hex(verifying_key); // Guard against TOML injection: dir_name (filesystem) and author_handle (env) // are interpolated into quoted TOML strings below. A value containing a quote, @@ -610,8 +717,43 @@ async fn update_pack_metadata( Ok(()) } -/// Derive pack metadata from persona.toml when present, otherwise from AGENTS.md. +/// Derive pack metadata for the marketplace listing. +/// +/// Prefers the pack's own `pack.toml` `description`/`tags` fields -- first-class +/// since commit b75344d and present on every curated `personas/*` manifest -- +/// and falls back to the legacy `persona.toml` (description + patterns.toml +/// stack tags) or `AGENTS.md` (first prose line) derivation for packs that +/// predate curated pack.toml metadata. fn derive_pack_metadata(path: &Path, dir_name: &str) -> Result, SeedError> { + let pack_toml_path = path.join("pack.toml"); + if pack_toml_path.exists() { + let pack_content = std::fs::read_to_string(&pack_toml_path)?; + let manifest: PackManifest = toml::from_str(&pack_content).map_err(|e| { + SeedError::Io(std::io::Error::other(format!( + "parse pack.toml for {dir_name}: {e}" + ))) + })?; + + let mut tags = manifest.tags.clone(); + if tags.is_empty() { + tags = derive_pattern_tags(path)?; + } + if tags.is_empty() { + tags = default_tags(dir_name); + } + + let description = match manifest.description.filter(|d| !d.is_empty()) { + Some(d) => d, + None => derive_legacy_description(path, dir_name), + }; + + return Ok(Some(PackMetadata { + name: manifest.name, + description, + tags, + })); + } + let persona_path = path.join("persona.toml"); if persona_path.exists() { let persona_content = std::fs::read_to_string(&persona_path)?; @@ -653,6 +795,27 @@ fn derive_pack_metadata(path: &Path, dir_name: &str) -> Result String { + let persona_path = path.join("persona.toml"); + if let Ok(content) = std::fs::read_to_string(&persona_path) { + if let Ok(persona) = toml::from_str::(&content) { + if !persona.description.is_empty() { + return persona.description; + } + } + } + + let agents_path = path.join("AGENTS.md"); + if let Some(description) = derive_agents_description(&agents_path) { + return description; + } + + fallback_description(dir_name) +} + /// Extract stack-category tags from patterns.toml when that file exists. fn derive_pattern_tags(path: &Path) -> Result, SeedError> { let mut tags = Vec::new(); @@ -700,3 +863,242 @@ fn derive_agents_description(path: &Path) -> Option { } None } + +/// Unit tests for the seeder's persona gate, pack.toml repair, and +/// metadata derivation. +#[cfg(test)] +mod tests { + use super::*; + use ed25519_dalek::SigningKey; + use tempfile::TempDir; + + /// A pack.toml fixture shaped exactly like the curated `personas/*` + /// directories in this repo: pack.toml-only, with the literal + /// `"UNSIGNED"` author_pubkey placeholder and first-class + /// description/tags (commit b75344d). + const CURATED_PACK_TOML: &str = r#"# Curated persona pack manifest. +schema_version = 1 +name = "agents" +author_handle = "ghost-frame" +author_pubkey = "UNSIGNED" +version = "0.1.0" +description = "Multi-agent coordination, delegation, and parallel execution workflows." +tags = ["agents", "coordination", "delegation", "parallel"] +license = "Elastic-2.0" + +# Capability surface this persona expects from the host agent. +[capability_manifest] +required_tools = ["Read", "Edit", "Write", "Bash", "Grep", "Glob"] +network_egress = false +filesystem_scope = "project-only" +memory_required = "none" +memory_required_ops = [] +"#; + + /// Deterministic test keypair (mirrors the pattern used in + /// `frameshift_pack::pack::tests`). + fn test_verifying_key() -> ed25519_dalek::VerifyingKey { + SigningKey::from_bytes(&[7u8; 32]).verifying_key() + } + + #[test] + /// A pack.toml-only directory (no persona.toml, no AGENTS.md) must pass + /// the persona-directory gate -- this is the exact shape of every + /// `personas/*` directory in the repo. + fn is_persona_dir_accepts_pack_toml_only() { + let tmp = TempDir::new().unwrap(); + std::fs::write(tmp.path().join("pack.toml"), CURATED_PACK_TOML).unwrap(); + assert!(is_persona_dir(tmp.path())); + } + + #[test] + /// A directory with none of the three marker files is not a persona dir + /// (this is the shape of `personas/assets/`, which holds only images). + fn is_persona_dir_rejects_directory_with_no_markers() { + let tmp = TempDir::new().unwrap(); + std::fs::write(tmp.path().join("banner.png"), b"not a persona").unwrap(); + assert!(!is_persona_dir(tmp.path())); + } + + #[test] + /// The strict-hex check must accept only exactly 64 lowercase hex chars, + /// matching `frameshift_pack::manifest::deserialize_author_pubkey`. + fn pubkey_hex_validation_matches_manifest_parser() { + assert!(!is_valid_pubkey_hex("UNSIGNED")); + assert!(!is_valid_pubkey_hex("")); + assert!(!is_valid_pubkey_hex(&"a".repeat(63))); + assert!(!is_valid_pubkey_hex(&"A".repeat(64))); // uppercase rejected + assert!(is_valid_pubkey_hex(&"a".repeat(64))); + } + + #[test] + /// Repairing a curated pack.toml's placeholder author_pubkey must leave + /// every other field -- including the first-class description/tags -- + /// untouched, and the result must parse as a valid `PackManifest`. + fn repair_placeholder_author_pubkey_preserves_curated_fields() { + let tmp = TempDir::new().unwrap(); + let pack_toml_path = tmp.path().join("pack.toml"); + std::fs::write(&pack_toml_path, CURATED_PACK_TOML).unwrap(); + + // Before repair: the strict manifest parser must reject "UNSIGNED". + let content = std::fs::read_to_string(&pack_toml_path).unwrap(); + assert!(toml::from_str::(&content).is_err()); + + repair_placeholder_author_pubkey(&pack_toml_path, &test_verifying_key()).unwrap(); + + let repaired = std::fs::read_to_string(&pack_toml_path).unwrap(); + let manifest: PackManifest = toml::from_str(&repaired) + .expect("repaired pack.toml must parse as a valid PackManifest"); + + assert_eq!(manifest.name, "agents"); + assert_eq!(manifest.author_handle, "ghost-frame"); + assert!(is_valid_pubkey_hex(&manifest.author_pubkey)); + assert_eq!( + manifest.description.as_deref(), + Some("Multi-agent coordination, delegation, and parallel execution workflows.") + ); + assert_eq!( + manifest.tags, + vec!["agents", "coordination", "delegation", "parallel"] + ); + assert_eq!(manifest.license.as_deref(), Some("Elastic-2.0")); + assert_eq!( + manifest.capability_manifest.unwrap().required_tools, + vec!["Read", "Edit", "Write", "Bash", "Grep", "Glob"] + ); + } + + #[test] + /// The repair must be a surgical single-line rewrite: comments, blank + /// lines, key order, and every byte outside the `author_pubkey` line are + /// hand-written persona content and must survive verbatim (several real + /// curated `personas/*/pack.toml` files carry comments). + fn repair_placeholder_author_pubkey_preserves_comments_and_layout() { + let tmp = TempDir::new().unwrap(); + let pack_toml_path = tmp.path().join("pack.toml"); + std::fs::write(&pack_toml_path, CURATED_PACK_TOML).unwrap(); + + repair_placeholder_author_pubkey(&pack_toml_path, &test_verifying_key()).unwrap(); + + let repaired = std::fs::read_to_string(&pack_toml_path).unwrap(); + let expected_line = format!( + "author_pubkey = \"{}\"", + verifying_key_hex(&test_verifying_key()) + ); + let expected = CURATED_PACK_TOML.replace("author_pubkey = \"UNSIGNED\"", &expected_line); + assert_eq!(repaired, expected); + } + + #[test] + /// A pack.toml with no root-level author_pubkey key at all must fail the + /// repair loudly rather than have a key silently invented for it. + fn repair_placeholder_author_pubkey_errors_when_key_missing() { + let tmp = TempDir::new().unwrap(); + let pack_toml_path = tmp.path().join("pack.toml"); + std::fs::write( + &pack_toml_path, + "schema_version = 1\nname = \"x\"\nauthor_handle = \"h\"\nversion = \"0.1.0\"\n", + ) + .unwrap(); + + let err = repair_placeholder_author_pubkey(&pack_toml_path, &test_verifying_key()) + .expect_err("missing author_pubkey key must be an error"); + assert!(err.to_string().contains("author_pubkey")); + } + + #[test] + /// A pack.toml whose author_pubkey is already valid hex must be left + /// byte-for-byte unchanged. + fn repair_placeholder_author_pubkey_is_noop_when_already_valid() { + let tmp = TempDir::new().unwrap(); + let pack_toml_path = tmp.path().join("pack.toml"); + let valid_hex = "a".repeat(64); + let content = format!( + "schema_version = 1\nname = \"x\"\nauthor_handle = \"h\"\n\ + author_pubkey = \"{valid_hex}\"\nversion = \"0.1.0\"\n" + ); + std::fs::write(&pack_toml_path, &content).unwrap(); + + repair_placeholder_author_pubkey(&pack_toml_path, &test_verifying_key()).unwrap(); + + let after = std::fs::read_to_string(&pack_toml_path).unwrap(); + assert_eq!(after, content); + } + + #[test] + /// End-to-end: a pack.toml-only persona directory shaped exactly like a + /// curated `personas/*` entry must survive the full pre-seed pipeline -- + /// gate, pubkey repair, and `Pack::from_dir` + sign -- without the + /// missing persona.toml/AGENTS.md ever being required. + fn pack_toml_only_persona_seeds_end_to_end() { + let tmp = TempDir::new().unwrap(); + std::fs::write(tmp.path().join("pack.toml"), CURATED_PACK_TOML).unwrap(); + + // 1. Gate: must be recognized as a persona dir. + assert!(is_persona_dir(tmp.path())); + + // 2. Repair: placeholder author_pubkey must be fixed in place. + let pack_toml_path = tmp.path().join("pack.toml"); + let signing_key = SigningKey::from_bytes(&[9u8; 32]); + let verifying_key = signing_key.verifying_key(); + repair_placeholder_author_pubkey(&pack_toml_path, &verifying_key).unwrap(); + + // 3. Load: Pack::from_dir must now succeed against the repaired manifest. + let mut pack = Pack::from_dir(tmp.path()).expect("pack.toml-only persona must load"); + assert_eq!(pack.manifest().name, "agents"); + + // 4. Sign: the loaded pack must be signable, exactly as seed_persona does. + pack.sign(&signing_key).expect("signing must succeed"); + assert!(pack.verify(&verifying_key).is_ok()); + + // 5. Canonical bytes: the object-store payload must build without error. + let bytes = pack_canonical_bytes(tmp.path()).expect("canonical bytes must build"); + assert!(!bytes.is_empty()); + + // 6. Metadata: the marketplace description/tags must come straight from + // pack.toml, not from a nonexistent persona.toml/AGENTS.md fallback. + let metadata = derive_pack_metadata(tmp.path(), "agents") + .expect("metadata derivation must not error") + .expect("pack.toml-only dir must yield metadata"); + assert_eq!(metadata.name, "agents"); + assert_eq!( + metadata.description, + "Multi-agent coordination, delegation, and parallel execution workflows." + ); + assert_eq!( + metadata.tags, + vec!["agents", "coordination", "delegation", "parallel"] + ); + } + + #[test] + /// When a pack.toml carries no description/tags, `derive_pack_metadata` + /// must fall back to the legacy AGENTS.md derivation rather than + /// returning an empty description. + fn derive_pack_metadata_falls_back_to_agents_md_when_pack_toml_has_no_description() { + let tmp = TempDir::new().unwrap(); + std::fs::write( + tmp.path().join("pack.toml"), + "schema_version = 1\nname = \"bare\"\nauthor_handle = \"h\"\n\ + author_pubkey = \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\"\n\ + version = \"0.1.0\"\n", + ) + .unwrap(); + std::fs::write( + tmp.path().join("AGENTS.md"), + "# Bare Persona\n\nA persona description long enough to pass the filter.\n", + ) + .unwrap(); + + let metadata = derive_pack_metadata(tmp.path(), "bare") + .unwrap() + .expect("must yield metadata"); + assert_eq!(metadata.name, "bare"); + assert_eq!( + metadata.description, + "A persona description long enough to pass the filter." + ); + // No tags anywhere -> falls back to the generic default tags. + assert_eq!(metadata.tags, vec!["bare", "persona"]); + } +} From 7d693f94f0b6549dafc7680ebb9c2fac9bb0a080 Mon Sep 17 00:00:00 2001 From: GhostFrame Date: Sat, 11 Jul 2026 16:02:46 -0400 Subject: [PATCH 06/10] feat(mcp): add frameshift_search registry tool The stdio MCP server had no registry-search parity with the CLI's search command. Adds a frameshift_search tool (query required, limit optional with a default of 20 and a client-side cap of 100) backed by Client::search_registry. --- crates/frameshift-mcp/src/main.rs | 6 +- crates/frameshift-mcp/src/tools.rs | 170 ++++++++++++++++++++++++++++- 2 files changed, 169 insertions(+), 7 deletions(-) diff --git a/crates/frameshift-mcp/src/main.rs b/crates/frameshift-mcp/src/main.rs index c611437..4bb79c5 100644 --- a/crates/frameshift-mcp/src/main.rs +++ b/crates/frameshift-mcp/src/main.rs @@ -276,16 +276,16 @@ mod tests { assert_eq!(serialized["error"]["code"], -32601); } - /// Verify that tools/list returns the expected nine tool names. + /// Verify that tools/list returns the expected ten tool names. #[test] - fn tools_list_returns_nine_tools() { + fn tools_list_returns_ten_tools() { let tmp = tempfile::tempdir().unwrap(); let client = make_client(tmp.path()); let line = r#"{"jsonrpc":"2.0","id":3,"method":"tools/list"}"#; let response = handle_message(line, &client).expect("should produce a response"); let serialized = serde_json::to_value(&response).unwrap(); let tools = serialized["result"]["tools"].as_array().unwrap(); - assert_eq!(tools.len(), 9); + assert_eq!(tools.len(), 10); } /// Verify tools/call with frameshift_install succeeds end-to-end. diff --git a/crates/frameshift-mcp/src/tools.rs b/crates/frameshift-mcp/src/tools.rs index 5c83cbf..132d4d8 100644 --- a/crates/frameshift-mcp/src/tools.rs +++ b/crates/frameshift-mcp/src/tools.rs @@ -2,7 +2,7 @@ /// /// Each tool maps directly to a frameshift-client or frameshift-growth operation. use frameshift_capabilities::{CapabilityFilter, Tool as CapabilityTool}; -use frameshift_client::{Client, InstallRequest, InstallSource, PersonaSpec}; +use frameshift_client::{Client, InstallRequest, InstallSource, PersonaSpec, RegistrySearchQuery}; use frameshift_orchestrator::{ AuditLog, Embedder, Mode, ModeState, PolicyWeights, Preferences, SelectionInputs, }; @@ -179,6 +179,21 @@ pub fn tool_definitions() -> Vec { "required": ["project_root", "action"] }), }, + 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(), + input_schema: serde_json::json!({ + "type": "object", + "properties": { + "query": {"type": "string"}, + "limit": { + "type": "integer", + "description": "Maximum number of results to return (default 20, capped at 100)." + } + }, + "required": ["query"] + }), + }, ] } @@ -389,6 +404,7 @@ pub fn call_tool(name: &str, arguments: &serde_json::Value, client: &Client) -> "frameshift_automate" => call_automate(arguments, client), "frameshift_prefs" => call_prefs(arguments, client), "frameshift_capabilities" => call_capabilities(arguments, client), + "frameshift_search" => call_search(arguments, client), _ => err_result(format!("unknown tool: {}", name)), } } @@ -910,6 +926,76 @@ fn call_prefs(arguments: &serde_json::Value, client: &Client) -> ToolResult { } } +/// Default result-page size for `frameshift_search` when the caller omits +/// `limit`. Matches the registry server's own default (see +/// `frameshift_server::routes::packs`) so behavior is consistent whether the +/// caller specifies a limit or not. +const DEFAULT_SEARCH_LIMIT: u32 = 20; + +/// Upper bound on `frameshift_search`'s `limit` argument, enforced on the MCP +/// side regardless of what the registry server itself would allow. Keeps a +/// single tool call from flooding the calling agent's context with an +/// unbounded result page. +const MAX_SEARCH_LIMIT: u32 = 100; + +/// Resolve the `limit` argument for `frameshift_search` into a validated page +/// size. +/// +/// A missing, non-numeric, zero, or negative value falls back to +/// [`DEFAULT_SEARCH_LIMIT`]; any positive value is clamped to +/// `[1, MAX_SEARCH_LIMIT]`. +fn parse_search_limit(arguments: &serde_json::Value) -> u32 { + arguments + .get("limit") + .and_then(|v| v.as_u64()) + .filter(|&n| n > 0) + .map(|n| n.clamp(1, MAX_SEARCH_LIMIT as u64) as u32) + .unwrap_or(DEFAULT_SEARCH_LIMIT) +} + +/// 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`. +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, + None => return err_result("missing required argument: query".to_string()), + }; + + let limit = parse_search_limit(arguments); + + let search_query = RegistrySearchQuery { + query: Some(query.to_string()), + tag: None, + limit: Some(limit), + }; + + match client.search_registry(&search_query) { + Ok(results) => { + let entries: Vec = results + .iter() + .map(|hit| { + serde_json::json!({ + "name": hit.pack.name, + "latest_version": hit.pack.latest_version, + "description": hit.pack.description, + "tags": hit.pack.tags, + "total_downloads": hit.pack.total_downloads, + "score": hit.score, + }) + }) + .collect(); + ok_result(serde_json::json!({ "results": entries }).to_string()) + } + Err(e) => err_result(format!("search failed: {}", e)), + } +} + #[cfg(test)] mod tests { use super::*; @@ -966,11 +1052,38 @@ mod tests { } /// Verify that tool_definitions returns the expected number of tools - /// (4 original + 4 automate/prefs additions + 1 capabilities). + /// (4 original + 4 automate/prefs additions + 1 capabilities + 1 search). #[test] - fn tool_definitions_returns_nine() { + fn tool_definitions_returns_ten() { let defs = tool_definitions(); - assert_eq!(defs.len(), 9); + assert_eq!(defs.len(), 10); + } + + /// frameshift_search is present in tool_definitions with `query` required + /// and `limit` present but optional in its input schema. + #[test] + fn tool_definitions_includes_search() { + let defs = tool_definitions(); + let search = defs + .iter() + .find(|d| d.name == "frameshift_search") + .expect("tool_definitions must include frameshift_search"); + + let required = search.input_schema["required"] + .as_array() + .expect("input_schema.required must be an array"); + assert!( + required.iter().any(|v| v == "query"), + "query must be a required argument" + ); + assert!( + !required.iter().any(|v| v == "limit"), + "limit must not be required" + ); + assert!( + search.input_schema["properties"]["limit"].is_object(), + "limit must be a declared property" + ); } /// Verify that calling an unknown tool name returns an is_error result. @@ -1483,4 +1596,53 @@ mod tests { serde_json::json!(["Read", "Bash"]) ); } + + /// frameshift_search rejects a call with no `query` argument, without + /// ever reaching the network (call_search must validate before dispatch). + #[test] + fn tool_call_search_requires_query() { + let tmp = tempfile::tempdir().unwrap(); + let client = make_client(tmp.path()); + + let result = call_tool("frameshift_search", &serde_json::json!({}), &client); + assert_eq!(result.is_error, Some(true)); + assert!(result.content[0].text.contains("query")); + } + + /// parse_search_limit falls back to DEFAULT_SEARCH_LIMIT when `limit` is + /// absent, non-numeric, zero, or negative. + #[test] + fn parse_search_limit_defaults_on_missing_or_invalid() { + assert_eq!( + parse_search_limit(&serde_json::json!({})), + DEFAULT_SEARCH_LIMIT + ); + assert_eq!( + parse_search_limit(&serde_json::json!({"limit": "not a number"})), + DEFAULT_SEARCH_LIMIT + ); + assert_eq!( + parse_search_limit(&serde_json::json!({"limit": 0})), + DEFAULT_SEARCH_LIMIT + ); + assert_eq!( + parse_search_limit(&serde_json::json!({"limit": -5})), + DEFAULT_SEARCH_LIMIT + ); + } + + /// parse_search_limit passes through an in-range positive value and + /// clamps one above MAX_SEARCH_LIMIT down to the cap. + #[test] + fn parse_search_limit_clamps_to_range() { + assert_eq!(parse_search_limit(&serde_json::json!({"limit": 5})), 5); + assert_eq!( + parse_search_limit(&serde_json::json!({"limit": MAX_SEARCH_LIMIT})), + MAX_SEARCH_LIMIT + ); + assert_eq!( + parse_search_limit(&serde_json::json!({"limit": MAX_SEARCH_LIMIT as u64 + 1000})), + MAX_SEARCH_LIMIT + ); + } } From 73216ecfe04e6529609daa171c4969cf79f0a986 Mon Sep 17 00:00:00 2001 From: GhostFrame Date: Sat, 11 Jul 2026 16:02:46 -0400 Subject: [PATCH 07/10] fix(server): sanitize public health detail, add authors route, paginate versions /healthz previously passed backend detail strings (filesystem roots, pool counts, raw driver errors) verbatim to an unauthenticated endpoint; detail is now a fixed ok/degraded string and the rich detail goes to server-side tracing only. Publish validation errors no longer leak tempdir paths. GET /v1/authors exposes the already-implemented catalog listing with limit/offset, the versions route gains the same pagination (default 100), and the downloads rustdoc now matches the status codes the handlers actually return. --- crates/frameshift-server/src/router.rs | 2 +- .../frameshift-server/src/routes/authors.rs | 98 ++++++- .../frameshift-server/src/routes/downloads.rs | 6 +- crates/frameshift-server/src/routes/mod.rs | 3 +- crates/frameshift-server/src/routes/ops.rs | 81 +++++- crates/frameshift-server/src/routes/packs.rs | 104 +++++-- crates/frameshift-server/tests/integration.rs | 253 +++++++++++++++++- .../frameshift-server/tests/mocks/catalog.rs | 17 +- crates/frameshift-server/tests/publish.rs | 80 ++++++ 9 files changed, 599 insertions(+), 45 deletions(-) diff --git a/crates/frameshift-server/src/router.rs b/crates/frameshift-server/src/router.rs index 0e9dad1..fd47835 100644 --- a/crates/frameshift-server/src/router.rs +++ b/crates/frameshift-server/src/router.rs @@ -78,7 +78,7 @@ use crate::state::AppState; /// /metrics -- ops /// /v1 /// /packs -- pack read endpoints + POST publish (signed-request) -/// /authors -- author lookup + POST register / POST {handle}/rotate (signed-request) +/// /authors -- GET list (paginated) + lookup; POST register / rotate (signed-request) /// /handles -- handle lookup /// /telemetry -- POST /selection opt-in selection telemetry sink /// /memory -- GET /health read-only memory backend health diff --git a/crates/frameshift-server/src/routes/authors.rs b/crates/frameshift-server/src/routes/authors.rs index b534551..3950feb 100644 --- a/crates/frameshift-server/src/routes/authors.rs +++ b/crates/frameshift-server/src/routes/authors.rs @@ -2,6 +2,8 @@ //! //! # Read (anonymous) //! +//! - `GET /` -> [`list_authors_route`] -- paginated listing of all +//! registered authors. //! - `GET /{pubkey}` -> [`get_author`] -- look up an author by base64url //! Ed25519 public key. //! @@ -17,8 +19,8 @@ //! a new key. The request MUST be signed by the handle's *current* owner key //! (the old key authorizes its own replacement). -use axum::extract::{Path, State}; -use axum::http::StatusCode; +use axum::extract::{Path, Query, State}; +use axum::http::{HeaderValue, StatusCode}; use axum::response::{IntoResponse, Response}; use axum::routing::{get, post}; use axum::{Extension, Json, Router}; @@ -38,12 +40,18 @@ use crate::state::AppState; /// Build the authors **read** sub-router, mounted at `/v1/authors`. /// /// Routes: +/// - `GET /` -> [`list_authors_route`] /// - `GET /{pubkey}` -> [`get_author`] /// /// The mutating routes are built by [`authors_write_router`] and wired with the -/// signed-request layer in [`crate::router::app`]. +/// signed-request layer in [`crate::router::app`]; that router also declares a +/// `POST /` (`register_author_route`), which axum merges with this router's +/// `GET /` onto the same path, mirroring how `packs_router`'s `GET /` and +/// [`crate::routes::packs::publish_pack`]'s `POST /` share `/v1/packs`. pub fn authors_router() -> Router { - Router::new().route("/{pubkey}", get(get_author)) + Router::new() + .route("/", get(list_authors_route)) + .route("/{pubkey}", get(get_author)) } /// Build the authors **write** sub-router (mutating routes only). @@ -92,6 +100,88 @@ pub async fn get_author( Ok(Json(author)) } +/// Query parameters accepted by `GET /v1/authors`. +/// +/// Both fields are optional. `limit` defaults to `100` and is clamped to +/// `config.max_search_limit`, mirroring +/// [`crate::routes::packs::SearchQuery`] and +/// [`crate::routes::packs::VersionsQuery`]; `offset` defaults to `0`. +#[derive(Debug, Default, Deserialize)] +pub struct AuthorsListQuery { + /// Maximum number of author records to return. Clamped to + /// `config.max_search_limit`. + /// + /// A value of `0` is valid and returns an empty array. + pub limit: Option, + + /// Number of author records to skip before returning matches. + pub offset: Option, +} + +/// Response body for `GET /v1/authors`. +#[derive(Debug, Serialize)] +pub struct AuthorsListResponse { + /// The requested page of registered authors, in the stable order + /// documented on [`frameshift_catalog::CatalogBackend::list_authors`]. + pub authors: Vec, +} + +/// `GET /v1/authors?limit=&offset=` +/// +/// List registered authors. Anonymous; no auth required. +/// +/// The `limit` parameter defaults to `100` and is clamped to +/// `config.max_search_limit`, the same convention +/// [`crate::routes::packs::search_packs`] and +/// [`crate::routes::packs::list_pack_versions`] use. When clamped, the +/// response includes a `Warning` header: `299 - "limit clamped to "`. +/// +/// Unlike `GET /v1/packs/{name}/versions`, pagination here is pushed all the +/// way down into `catalog.list_authors(limit, offset)`, which already +/// accepts both parameters at the trait level. +/// +/// # Response +/// +/// `200 OK` with body `{"authors": [AuthorRecord, ...]}`. +/// +/// # Backend calls +/// +/// - `catalog.list_authors(limit, offset)` -- single catalog read. +/// +/// # Errors +/// +/// - `500 Internal Server Error` on backend failure (request-id only; no +/// internal details in body). +pub async fn list_authors_route( + State(state): State, + Query(q): Query, +) -> Result { + let max = state.config.max_search_limit; + let raw_limit = q.limit.unwrap_or(100); + let clamped = raw_limit.min(max); + let was_clamped = clamped < raw_limit; + let offset = q.offset.unwrap_or(0); + + let authors = state + .catalog + .list_authors(clamped, offset) + .await + .map_err(|e| AppError::from_catalog(e, "author"))?; + + let body = Json(AuthorsListResponse { authors }); + + if was_clamped { + let warning_value = format!("299 - \"limit clamped to {max}\""); + let mut resp = (StatusCode::OK, body).into_response(); + if let Ok(hv) = HeaderValue::from_str(&warning_value) { + resp.headers_mut().insert("Warning", hv); + } + Ok(resp) + } else { + Ok((StatusCode::OK, body).into_response()) + } +} + /// Request body for `POST /v1/authors`. #[derive(Debug, Deserialize)] pub struct RegisterAuthorRequest { diff --git a/crates/frameshift-server/src/routes/downloads.rs b/crates/frameshift-server/src/routes/downloads.rs index e6a8d1e..e137cae 100644 --- a/crates/frameshift-server/src/routes/downloads.rs +++ b/crates/frameshift-server/src/routes/downloads.rs @@ -20,7 +20,7 @@ //! # Disabling //! //! When `DOWNLOAD_SECRET` is empty in [`crate::config::ServerConfig`], the signer -//! returns `503 Service Unavailable` and the verifier returns `404 Not Found` +//! returns `400 Bad Request` and the verifier returns `403 Forbidden` //! (no special-case status; the route stays mounted but every request rejects). use axum::body::Body; @@ -95,11 +95,11 @@ pub struct DownloadQuery { /// /// # Errors /// -/// - `400 Bad Request` if `name` or `version` fails validation. +/// - `400 Bad Request` if `name` or `version` fails validation, or if +/// `DOWNLOAD_SECRET` is empty (downloads disabled). /// - `404 Not Found` if the pack version does not exist. /// - `500 Internal Server Error` if `DOWNLOAD_SECRET` is misconfigured (set but not /// valid 32-byte hex). -/// - `503 Service Unavailable` if `DOWNLOAD_SECRET` is empty (downloads disabled). pub async fn mint_download_url( State(state): State, Path((name, version)): Path<(String, String)>, diff --git a/crates/frameshift-server/src/routes/mod.rs b/crates/frameshift-server/src/routes/mod.rs index 02779b4..204599a 100644 --- a/crates/frameshift-server/src/routes/mod.rs +++ b/crates/frameshift-server/src/routes/mod.rs @@ -3,7 +3,8 @@ //! Each sub-module corresponds to a logical grouping of endpoints: //! //! - [`packs`] -- `GET /v1/packs*` read endpoints. -//! - [`authors`] -- `GET /v1/authors/{pubkey}` lookup. +//! - [`authors`] -- `GET /v1/authors` paginated listing and +//! `GET /v1/authors/{pubkey}` lookup. //! - [`handles`] -- `GET /v1/handles/{handle}` lookup. //! - [`ops`] -- `GET /healthz` and `GET /metrics` operational endpoints. //! - [`telemetry`] -- `POST /v1/telemetry/selection` opt-in selection telemetry sink. diff --git a/crates/frameshift-server/src/routes/ops.rs b/crates/frameshift-server/src/routes/ops.rs index 4c58760..64848af 100644 --- a/crates/frameshift-server/src/routes/ops.rs +++ b/crates/frameshift-server/src/routes/ops.rs @@ -99,6 +99,20 @@ pub struct MemoryHealthSummary { pub detail: String, } +/// Reduce a backend's rich health detail to a fixed public-safe string. +/// +/// Adapters return operator-facing detail strings (filesystem paths, pool +/// counts, raw driver errors) that must never cross the unauthenticated +/// `/healthz` boundary. Callers should log the real detail via `tracing` +/// before discarding it in favor of this sanitized value. +fn sanitized_detail(healthy: bool) -> String { + if healthy { + "ok".to_string() + } else { + "degraded".to_string() + } +} + /// `GET /healthz` /// /// Returns the health status of all backends. Always responds with `200 OK` @@ -131,16 +145,37 @@ pub struct MemoryHealthSummary { /// - `memory.health()` (only when configured) -- may return /// `MemoryError` which is mapped to `healthy: false`. /// +/// # Public boundary sanitization +/// +/// This is an unauthenticated, internet-facing endpoint. Adapters +/// (`frameshift-catalog-postgres`, `frameshift-objects-fs`, ...) return rich +/// `detail` strings for operator-facing surfaces (structured logs, internal +/// dashboards) that may embed filesystem paths, pool internals, or raw driver +/// error text. None of that may reach this public response. Every `detail` +/// field in the returned JSON is therefore reduced to the fixed string `"ok"` +/// (healthy) or `"degraded"` (unhealthy); the adapter's real detail is logged +/// server-side via `tracing` (`info` when healthy, `warn` when degraded) and +/// never serialized. +/// /// # Errors /// /// This handler never returns an HTTP error. Backend failures are represented /// as `healthy: false` in the response body. pub async fn healthz(State(state): State) -> impl IntoResponse { let catalog_health = match state.catalog.health().await { - Ok(h) => CatalogHealthSummary { - healthy: h.healthy, - detail: h.detail, - }, + Ok(h) => { + // The adapter's detail may embed internals (e.g. pool counts, raw + // driver errors); log it server-side but never expose it publicly. + if h.healthy { + tracing::info!(detail = %h.detail, "catalog health check ok"); + } else { + tracing::warn!(detail = %h.detail, "catalog health check degraded"); + } + CatalogHealthSummary { + healthy: h.healthy, + detail: sanitized_detail(h.healthy), + } + } Err(e) => { // Log the raw error internally; never expose it in the public response. tracing::warn!(error = %e, "catalog health check failed"); @@ -152,12 +187,21 @@ pub async fn healthz(State(state): State) -> impl IntoResponse { }; let objects_health = match state.objects.health().await { - Ok(h) => ObjectsHealthSummary { - healthy: h.healthy, - total_objects: h.total_objects, - total_bytes: h.total_bytes, - detail: h.detail, - }, + Ok(h) => { + // The adapter's detail may embed the object store's absolute + // filesystem root path; log it server-side, never expose it. + if h.healthy { + tracing::info!(detail = %h.detail, "object store health check ok"); + } else { + tracing::warn!(detail = %h.detail, "object store health check degraded"); + } + ObjectsHealthSummary { + healthy: h.healthy, + total_objects: h.total_objects, + total_bytes: h.total_bytes, + detail: sanitized_detail(h.healthy), + } + } Err(e) => { // Log the raw error internally; never expose it in the public response. tracing::warn!(error = %e, "object store health check failed"); @@ -175,10 +219,19 @@ pub async fn healthz(State(state): State) -> impl IntoResponse { // nothing else in the server consumes memory yet. let memory_health = match &state.memory { Some(adapter) => Some(match adapter.health().await { - Ok(h) => MemoryHealthSummary { - healthy: h.healthy, - detail: h.message, - }, + Ok(h) => { + // The adapter's message may embed backend internals; log it + // server-side, never expose it in the public response. + if h.healthy { + tracing::info!(detail = %h.message, "memory health check ok"); + } else { + tracing::warn!(detail = %h.message, "memory health check degraded"); + } + MemoryHealthSummary { + healthy: h.healthy, + detail: sanitized_detail(h.healthy), + } + } Err(e) => { // Log the raw error internally; never expose it in the public response. tracing::warn!(error = %e, "memory health check failed"); diff --git a/crates/frameshift-server/src/routes/packs.rs b/crates/frameshift-server/src/routes/packs.rs index 0258725..bbe2f4a 100644 --- a/crates/frameshift-server/src/routes/packs.rs +++ b/crates/frameshift-server/src/routes/packs.rs @@ -200,11 +200,18 @@ async fn extract_targz(archive_bytes: Vec, dir: std::path::PathBuf) -> Resul archive.set_preserve_permissions(false); archive.set_overwrite(true); - let entries = archive - .entries() - .map_err(|e| AppError::BadRequest(format!("tar entries: {e}")))?; + let entries = archive.entries().map_err(|e| { + // The underlying io::Error text may embed the server's temp + // directory path; log it internally and return a generic, + // path-free message to the client. + tracing::warn!(error = %e, "failed to read tar entries"); + AppError::BadRequest("invalid archive: unreadable tar entries".to_string()) + })?; for entry in entries { - let mut entry = entry.map_err(|e| AppError::BadRequest(format!("tar entry: {e}")))?; + let mut entry = entry.map_err(|e| { + tracing::warn!(error = %e, "failed to read tar entry"); + AppError::BadRequest("invalid archive: unreadable tar entry".to_string()) + })?; // Reject any entry that is not a regular file or directory. Symlinks, // hardlinks, and device nodes have no legitimate place in a pack and // could be used to plant a link that escapes the extraction dir or @@ -218,7 +225,10 @@ async fn extract_targz(archive_bytes: Vec, dir: std::path::PathBuf) -> Resul // Path-traversal protection: only allow paths relative to dir. let path = entry .path() - .map_err(|e| AppError::BadRequest(format!("tar entry path: {e}")))? + .map_err(|e| { + tracing::warn!(error = %e, "failed to read tar entry path"); + AppError::BadRequest("invalid archive: unreadable entry path".to_string()) + })? .into_owned(); if path.is_absolute() || path @@ -229,9 +239,13 @@ async fn extract_targz(archive_bytes: Vec, dir: std::path::PathBuf) -> Resul "pack archive contains unsafe path".to_string(), )); } - entry - .unpack_in(&dir) - .map_err(|e| AppError::BadRequest(format!("tar unpack: {e}")))?; + entry.unpack_in(&dir).map_err(|e| { + // io::Error from unpack_in may embed the server's absolute + // temp-directory path; log it internally, keep the client + // response generic. + tracing::warn!(error = %e, "failed to unpack tar entry"); + AppError::BadRequest("invalid archive: failed to extract entry".to_string()) + })?; } Ok(()) }) @@ -371,8 +385,13 @@ pub async fn publish_pack( std::fs::write(pack_root.join("signature.sig"), &signature_bytes) .map_err(|e| AppError::Internal(format!("write signature.sig: {e}")))?; - let pack = Pack::from_dir(&pack_root) - .map_err(|e| AppError::BadRequest(format!("invalid pack: {e}")))?; + let pack = Pack::from_dir(&pack_root).map_err(|e| { + // `PackError` variants can embed the server's absolute temp-directory + // path (e.g. `Io`, `NonUtf8Path`); log the detailed error server-side + // and return a generic, path-free message to the client. + tracing::warn!(error = %e, "failed to load pack from extracted archive"); + AppError::BadRequest("invalid pack".to_string()) + })?; // Verify signature against canonical hash using the registered pubkey. // This is the authentication check: a wrong key means 401. @@ -766,13 +785,42 @@ pub async fn get_pack( Ok(Json(pack)) } -/// `GET /v1/packs/{name}/versions` +/// Query parameters accepted by `GET /v1/packs/{name}/versions`. +/// +/// Both fields are optional. `limit` defaults to `100` and is clamped to +/// `config.max_search_limit`, mirroring [`SearchQuery`]; `offset` defaults to +/// `0`. +#[derive(Debug, Default, Deserialize)] +pub struct VersionsQuery { + /// Maximum number of version records to return. Clamped to + /// `config.max_search_limit`. + /// + /// A value of `0` is valid and returns an empty array. + pub limit: Option, + + /// Number of version records to skip before returning matches, applied + /// after ordering by `published_at ASC`. + pub offset: Option, +} + +/// `GET /v1/packs/{name}/versions?limit=&offset=` +/// +/// List published versions of a pack, ordered by `published_at ASC`. +/// +/// The `limit` parameter defaults to `100` and is clamped to +/// `config.max_search_limit`, the same convention [`search_packs`] uses. When +/// clamped, the response includes a `Warning` header: +/// `299 - "limit clamped to "`. /// -/// List all published versions of a pack, ordered by `published_at ASC`. +/// The catalog trait's `list_pack_versions` has no `limit`/`offset` of its +/// own (unlike [`frameshift_catalog::CatalogBackend::list_authors`]), so +/// pagination is applied here, over the full version list returned by the +/// backend, rather than pushed down into the query. /// /// # Response /// -/// `200 OK` with body `[PackVersionRecord, ...]`. +/// `200 OK` with body `[PackVersionRecord, ...]`, containing at most `limit` +/// records starting at `offset`. /// /// # Backend calls /// @@ -786,14 +834,40 @@ pub async fn get_pack( pub async fn list_pack_versions( State(state): State, Path(name): Path, -) -> Result>, AppError> { + Query(q): Query, +) -> Result { validate_pack_name(&name)?; + + let max = state.config.max_search_limit; + let raw_limit = q.limit.unwrap_or(100); + let clamped = raw_limit.min(max); + let was_clamped = clamped < raw_limit; + let offset = q.offset.unwrap_or(0) as usize; + let versions = state .catalog .list_pack_versions(&name) .await .map_err(|e| AppError::from_catalog(e, "pack"))?; - Ok(Json(versions)) + + let page: Vec<_> = versions + .into_iter() + .skip(offset) + .take(clamped as usize) + .collect(); + + let body = Json(page); + + if was_clamped { + let warning_value = format!("299 - \"limit clamped to {max}\""); + let mut resp = (StatusCode::OK, body).into_response(); + if let Ok(hv) = HeaderValue::from_str(&warning_value) { + resp.headers_mut().insert("Warning", hv); + } + Ok(resp) + } else { + Ok((StatusCode::OK, body).into_response()) + } } /// `GET /v1/packs/{name}/versions/{version}` diff --git a/crates/frameshift-server/tests/integration.rs b/crates/frameshift-server/tests/integration.rs index 16b0b8a..501dd18 100644 --- a/crates/frameshift-server/tests/integration.rs +++ b/crates/frameshift-server/tests/integration.rs @@ -14,10 +14,13 @@ //! - `GET /v1/packs/../etc/passwd` -> 400 path validation //! - `GET /v1/packs/{name}/versions/{version}/pack` -> 200 octet-stream //! - `GET /v1/packs/{name}/versions/{version}/pack` -> 502 when blob missing +//! - `GET /v1/packs/{name}/versions` -> 200, default/clamped/offset pagination //! - `GET /v1/authors/{valid_base64url}` -> 200 //! - `GET /v1/authors/not-base64!!!` -> 400 //! - `GET /v1/authors/{valid_but_unknown}` -> 404 -//! - `GET /healthz` -> 200 with both backends healthy +//! - `GET /v1/authors` -> 200, default/clamped/offset pagination +//! - `GET /healthz` -> 200 with both backends healthy, `detail` sanitized to +//! `"ok"`/`"degraded"` (never the adapter's raw internal detail text) //! - `GET /mcp/anything` -> 501 //! - All responses include `x-request-id` header //! - `AppError::Internal` does not leak source details in body @@ -242,6 +245,109 @@ fn make_version( } } +// --------------------------------------------------------------------------- +// /v1/packs/{name}/versions (list, paginated) +// --------------------------------------------------------------------------- + +/// Insert `count` distinct version records for `pack_name` into `catalog`, +/// registering the parent pack record first. Returns nothing; callers only +/// need the side effect. +fn seed_pack_versions( + catalog: &MockCatalog, + pack_name: &str, + count: u32, + author: Ed25519PublicKey, +) { + let mut state = catalog.state.write().unwrap(); + state + .packs + .insert(pack_name.to_string(), make_pack(pack_name, author)); + for i in 0..count { + let version = format!("0.{i}.0"); + let hash = ObjectHash::of(format!("{pack_name}-{version}").as_bytes()); + state.versions.insert( + (pack_name.to_string(), version.clone()), + make_version(pack_name, &version, hash, author), + ); + } +} + +/// `GET /v1/packs/{name}/versions` with no query params returns every +/// version when the count is well under the default `limit=100`. +#[tokio::test] +async fn pack_versions_default_limit_returns_all_when_under_default() { + let author = Ed25519PublicKey([30u8; 32]); + let catalog = MockCatalog::new(); + seed_pack_versions(&catalog, "many-versions", 3, author); + + let state = make_state(catalog, MockPackStore::new()); + let resp = oneshot_get(state, "/v1/packs/many-versions/versions").await; + assert_eq!(resp.status(), StatusCode::OK); + let body = body_json(resp).await; + let arr = body.as_array().expect("body must be a JSON array"); + assert_eq!(arr.len(), 3); +} + +/// `GET /v1/packs/{name}/versions?limit=2` caps the response to 2 records +/// even though more versions exist -- the fix for the unbounded version +/// history response. +#[tokio::test] +async fn pack_versions_limit_caps_response_size() { + let author = Ed25519PublicKey([31u8; 32]); + let catalog = MockCatalog::new(); + seed_pack_versions(&catalog, "many-versions", 5, author); + + let state = make_state(catalog, MockPackStore::new()); + let resp = oneshot_get(state, "/v1/packs/many-versions/versions?limit=2").await; + assert_eq!(resp.status(), StatusCode::OK); + let body = body_json(resp).await; + let arr = body.as_array().expect("body must be a JSON array"); + assert_eq!(arr.len(), 2); +} + +/// `GET /v1/packs/{name}/versions?offset=2` skips the first 2 records (by +/// whatever order the backend returns them in). +#[tokio::test] +async fn pack_versions_offset_skips_records() { + let author = Ed25519PublicKey([32u8; 32]); + let catalog = MockCatalog::new(); + seed_pack_versions(&catalog, "many-versions", 3, author); + + let state = make_state(catalog, MockPackStore::new()); + let resp = oneshot_get(state, "/v1/packs/many-versions/versions?offset=2").await; + assert_eq!(resp.status(), StatusCode::OK); + let body = body_json(resp).await; + let arr = body.as_array().expect("body must be a JSON array"); + assert_eq!(arr.len(), 1); +} + +/// `GET /v1/packs/{name}/versions?limit=999999` is clamped to +/// `max_search_limit` (100 in the test config) and the response includes a +/// `Warning` header, mirroring `GET /v1/packs`'s clamping behavior. +#[tokio::test] +async fn pack_versions_limit_clamped_includes_warning_header() { + let author = Ed25519PublicKey([33u8; 32]); + let catalog = MockCatalog::new(); + seed_pack_versions(&catalog, "many-versions", 3, author); + + let state = make_state(catalog, MockPackStore::new()); + let resp = oneshot_get(state, "/v1/packs/many-versions/versions?limit=999999").await; + assert_eq!(resp.status(), StatusCode::OK); + assert!( + resp.headers().contains_key("warning"), + "response must contain a Warning header when limit is clamped" + ); +} + +/// `GET /v1/packs/{name}/versions` for an unknown pack still returns 404, +/// unaffected by the new query parameters. +#[tokio::test] +async fn pack_versions_unknown_pack_returns_404() { + let state = make_state(MockCatalog::new(), MockPackStore::new()); + let resp = oneshot_get(state, "/v1/packs/unknown/versions").await; + assert_eq!(resp.status(), StatusCode::NOT_FOUND); +} + /// `GET /v1/packs/{name}/versions/{version}/pack` returns 200 with the correct /// bytes and `Content-Type: application/octet-stream` when both catalog and /// object store have the artifact. @@ -406,12 +512,110 @@ async fn get_author_404_when_unknown() { assert_eq!(resp.status(), StatusCode::NOT_FOUND); } +// --------------------------------------------------------------------------- +// /v1/authors (list, paginated) +// --------------------------------------------------------------------------- + +/// Insert `count` distinct registered authors into `catalog`, with handles +/// `"author-0"`, `"author-1"`, ... and pubkeys derived from `seed` so each +/// call site can use a disjoint byte range. +fn seed_authors(catalog: &MockCatalog, count: u8, seed: u8) { + let mut state = catalog.state.write().unwrap(); + for i in 0..count { + let pubkey_bytes = [seed.wrapping_add(i); 32]; + let key = Ed25519PublicKey(pubkey_bytes); + let handle = format!("author-{i}"); + state + .authors + .insert(key.to_string(), make_author(pubkey_bytes, &handle)); + } +} + +/// `GET /v1/authors` with an empty catalog returns 200 with `{"authors":[]}`. +#[tokio::test] +async fn list_authors_empty_catalog_returns_200_empty_array() { + let state = make_state(MockCatalog::new(), MockPackStore::new()); + let resp = oneshot_get(state, "/v1/authors").await; + assert_eq!(resp.status(), StatusCode::OK); + let body = body_json(resp).await; + assert_eq!(body["authors"], serde_json::json!([])); +} + +/// `GET /v1/authors` with no query params returns every registered author +/// when the count is well under the default `limit=100`. +#[tokio::test] +async fn list_authors_default_limit_returns_all_when_under_default() { + let catalog = MockCatalog::new(); + seed_authors(&catalog, 3, 40); + + let state = make_state(catalog, MockPackStore::new()); + let resp = oneshot_get(state, "/v1/authors").await; + assert_eq!(resp.status(), StatusCode::OK); + let body = body_json(resp).await; + let arr = body["authors"] + .as_array() + .expect("authors must be an array"); + assert_eq!(arr.len(), 3); +} + +/// `GET /v1/authors?limit=2` caps the response to 2 records even though more +/// authors are registered. +#[tokio::test] +async fn list_authors_limit_caps_response_size() { + let catalog = MockCatalog::new(); + seed_authors(&catalog, 5, 60); + + let state = make_state(catalog, MockPackStore::new()); + let resp = oneshot_get(state, "/v1/authors?limit=2").await; + assert_eq!(resp.status(), StatusCode::OK); + let body = body_json(resp).await; + let arr = body["authors"] + .as_array() + .expect("authors must be an array"); + assert_eq!(arr.len(), 2); +} + +/// `GET /v1/authors?offset=2` skips the first 2 records in stable +/// `created_at ASC` order. +#[tokio::test] +async fn list_authors_offset_skips_records() { + let catalog = MockCatalog::new(); + seed_authors(&catalog, 3, 80); + + let state = make_state(catalog, MockPackStore::new()); + let resp = oneshot_get(state, "/v1/authors?offset=2").await; + assert_eq!(resp.status(), StatusCode::OK); + let body = body_json(resp).await; + let arr = body["authors"] + .as_array() + .expect("authors must be an array"); + assert_eq!(arr.len(), 1); +} + +/// `GET /v1/authors?limit=999999` is clamped to `max_search_limit` (100 in +/// the test config) and the response includes a `Warning` header, mirroring +/// `GET /v1/packs`'s and `GET /v1/packs/{name}/versions`'s clamping behavior. +#[tokio::test] +async fn list_authors_limit_clamped_includes_warning_header() { + let catalog = MockCatalog::new(); + seed_authors(&catalog, 3, 100); + + let state = make_state(catalog, MockPackStore::new()); + let resp = oneshot_get(state, "/v1/authors?limit=999999").await; + assert_eq!(resp.status(), StatusCode::OK); + assert!( + resp.headers().contains_key("warning"), + "response must contain a Warning header when limit is clamped" + ); +} + // --------------------------------------------------------------------------- // /healthz // --------------------------------------------------------------------------- /// `GET /healthz` returns 200 with `ok: true` when both mock backends report -/// healthy. +/// healthy, and the public `detail` fields are sanitized to the fixed string +/// `"ok"` rather than the adapter's rich internal detail text. #[tokio::test] async fn healthz_returns_200_with_both_backends_healthy() { let state = make_state(MockCatalog::new(), MockPackStore::new()); @@ -421,6 +625,27 @@ async fn healthz_returns_200_with_both_backends_healthy() { assert_eq!(body["ok"], true); assert_eq!(body["catalog"]["healthy"], true); assert_eq!(body["objects"]["healthy"], true); + assert_eq!(body["catalog"]["detail"], "ok"); + assert_eq!(body["objects"]["detail"], "ok"); +} + +/// `GET /healthz` never echoes an adapter's rich health detail (the mocks' +/// own detail strings) into the public response body; only the sanitized +/// `"ok"`/`"degraded"` values may appear. +#[tokio::test] +async fn healthz_does_not_leak_adapter_detail_text() { + let state = make_state(MockCatalog::new(), MockPackStore::new()); + let resp = oneshot_get(state, "/healthz").await; + let body = body_json(resp).await; + let raw = serde_json::to_string(&body).expect("health body must serialize"); + assert!( + !raw.contains("mock catalog is always healthy"), + "public /healthz body must not leak adapter detail text: {raw}" + ); + assert!( + !raw.contains("mock object store is always healthy"), + "public /healthz body must not leak adapter detail text: {raw}" + ); } // --------------------------------------------------------------------------- @@ -453,7 +678,8 @@ async fn memory_health_configured_reports_adapter_status() { } /// `GET /healthz` includes a `memory` summary only when a backend is -/// configured; its `healthy` flag reflects the adapter's reported status. +/// configured; its `healthy` flag reflects the adapter's reported status, and +/// its `detail` is sanitized to `"ok"`, not the adapter's raw message text. /// When no backend is configured, `memory` is absent/null and `ok` is /// unaffected. #[tokio::test] @@ -464,6 +690,7 @@ async fn healthz_includes_memory_when_configured() { assert_eq!(resp.status(), StatusCode::OK); let body = body_json(resp).await; assert_eq!(body["memory"]["healthy"], true); + assert_eq!(body["memory"]["detail"], "ok"); assert_eq!(body["ok"], true); // Default (unconfigured) state: memory is null and does not affect ok. @@ -474,6 +701,26 @@ async fn healthz_includes_memory_when_configured() { assert_eq!(body["ok"], true); } +/// `GET /healthz` reports `detail: "degraded"` for a configured memory +/// backend that is unhealthy, and never leaks the adapter's raw message text +/// into the public response body. +#[tokio::test] +async fn healthz_memory_degraded_sanitizes_detail() { + let mut state = make_state(MockCatalog::new(), MockPackStore::new()); + state.memory = Some(Arc::new(MockMemoryAdapter { healthy: false })); + let resp = oneshot_get(state, "/healthz").await; + assert_eq!(resp.status(), StatusCode::OK); + let body = body_json(resp).await; + assert_eq!(body["memory"]["healthy"], false); + assert_eq!(body["memory"]["detail"], "degraded"); + assert_eq!(body["ok"], false); + let raw = serde_json::to_string(&body).expect("health body must serialize"); + assert!( + !raw.contains("mock memory adapter is unhealthy"), + "public /healthz body must not leak adapter detail text: {raw}" + ); +} + // --------------------------------------------------------------------------- // /mcp // --------------------------------------------------------------------------- diff --git a/crates/frameshift-server/tests/mocks/catalog.rs b/crates/frameshift-server/tests/mocks/catalog.rs index 6cad18e..e64a781 100644 --- a/crates/frameshift-server/tests/mocks/catalog.rs +++ b/crates/frameshift-server/tests/mocks/catalog.rs @@ -165,17 +165,26 @@ impl CatalogBackend for MockCatalog { }) } - /// List authors (returns all stored authors, ignoring pagination). + /// List authors, paginated by `limit`/`offset` and ordered by + /// `created_at ASC` for a stable order matching the trait's documented + /// contract (mirrors the real Postgres backend's `ORDER BY created_at`). async fn list_authors( &self, - _limit: u32, - _offset: u32, + limit: u32, + offset: u32, ) -> Result, CatalogError> { let state = self .state .read() .map_err(|e| CatalogError::BackendError(e.to_string().into()))?; - Ok(state.authors.values().cloned().collect()) + let mut authors: Vec = state.authors.values().cloned().collect(); + authors.sort_by_key(|a| a.created_at); + let page = authors + .into_iter() + .skip(offset as usize) + .take(limit as usize) + .collect(); + Ok(page) } /// Register a pack version. diff --git a/crates/frameshift-server/tests/publish.rs b/crates/frameshift-server/tests/publish.rs index e187b1c..4e0cbea 100644 --- a/crates/frameshift-server/tests/publish.rs +++ b/crates/frameshift-server/tests/publish.rs @@ -13,6 +13,9 @@ //! - **bad signature** -- POST a pack with a tampered signature, assert `401`. //! - **unregistered author** -- POST with an unknown `author_handle`, assert `401`. //! - **duplicate** -- POST the same pack twice, assert second call is `409`. +//! - **malformed archive/manifest** -- POST a corrupt tar.gz or invalid +//! `pack.toml`, assert `400` with a fixed, generic error message that never +//! echoes the server's temp-directory path or raw `io::Error`/tar text. mod mocks; @@ -602,3 +605,80 @@ async fn publish_replayed_request_returns_401() { "replay with the same nonce must be rejected" ); } + +// --------------------------------------------------------------------------- +// malformed archive / manifest (temp-directory path leak regression) +// --------------------------------------------------------------------------- + +/// POST a `pack` field that is not a valid gzip/tar stream at all -> `400` +/// with a fixed, generic `"invalid archive: ..."` message. Regression test +/// for the leak where the raw tar/io error text (which can embed the +/// server's absolute temp-directory path) was echoed straight into the +/// response body. +#[tokio::test] +async fn publish_malformed_tar_returns_400_generic_message_without_path_leak() { + let signing = SigningKey::from_bytes(&[50u8; 32]); + let (catalog, _pubkey) = catalog_with_author(&signing, "mallory"); + // Not a valid gzip stream: decompression fails while the tar reader + // pulls the first header block, well before any pack.toml is touched. + let garbage_archive = b"not a valid gzip stream at all".to_vec(); + // The signature field only needs to be 64 bytes; extraction fails before + // the signature is ever verified. + let fake_signature = vec![0u8; 64]; + + let state = make_state(catalog, MockPackStore::new()); + let resp = post_publish( + state, + &garbage_archive, + &fake_signature, + "mallory", + Some(&signing), + ) + .await; + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + + let body = body_json(resp).await; + let msg = body["error"] + .as_str() + .expect("error message must be a string"); + assert!( + msg.starts_with("invalid archive"), + "expected a generic 'invalid archive' message, got: {msg}" + ); + assert!( + !msg.contains("/tmp") && !msg.to_lowercase().contains("os error"), + "error message must not leak server filesystem details: {msg}" + ); +} + +/// POST a well-formed tar.gz whose `pack.toml` is not valid TOML -> `400` +/// with the fixed message `"invalid pack"`, never the underlying +/// `PackError` text (which can embed the server's absolute +/// temp-directory path via `PackError::Io`/`NonUtf8Path`). +#[tokio::test] +async fn publish_malformed_manifest_returns_400_generic_message_without_path_leak() { + let signing = SigningKey::from_bytes(&[51u8; 32]); + let (catalog, _pubkey) = catalog_with_author(&signing, "trent"); + + let tmp = tempfile::TempDir::new().unwrap(); + std::fs::write(tmp.path().join("pack.toml"), b"this is not valid toml {{{").unwrap(); + let archive_bytes = make_targz(tmp.path()); + let fake_signature = vec![0u8; 64]; + + let state = make_state(catalog, MockPackStore::new()); + let resp = post_publish( + state, + &archive_bytes, + &fake_signature, + "trent", + Some(&signing), + ) + .await; + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + + let body = body_json(resp).await; + assert_eq!( + body["error"], "invalid pack", + "publish must return the fixed generic message, not the underlying PackError text" + ); +} From 6c9705d1e7344421a6fcb7b07f95c387a990ca22 Mon Sep 17 00:00:00 2001 From: GhostFrame Date: Sat, 11 Jul 2026 16:02:46 -0400 Subject: [PATCH 08/10] docs: truth pass on README, CI comment, and preflight Documents all 27 server config vars from config.rs, adds a Downloads subsection explaining direct vs signed delivery, corrects the MSRV noted in the CI workflow comment to the declared rust-version 1.86, and drops preflight's reference to a mirror job that no longer exists. --- .github/workflows/ci.yml | 4 +-- README.md | 66 ++++++++++++++++++++++++++++++++++------ scripts/preflight.sh | 4 +-- 3 files changed, 60 insertions(+), 14 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6a764bd..21100fd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -3,8 +3,8 @@ # Gates every pull request (and pushes to the default branch) on formatting, # lints, the unit/integration test suite, the Docker-gated Postgres tests, and # a supply-chain advisory scan. Jobs run on the stable toolchain to match the -# local development environment; the workspace declares rust-version 1.75 but is -# built on current stable, so a dedicated 1.75 MSRV-verification job is deferred +# local development environment; the workspace declares rust-version 1.86 but is +# built on current stable, so a dedicated 1.86 MSRV-verification job is deferred # to the later tier rather than risking a spurious MSRV failure. name: CI diff --git a/README.md b/README.md index be074e4..7aeea48 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A persona engine for AI coding agents. Install behavioral identities as versioned packs, activate them per-project, and let the engine pick the right one for the task. -**Status:** The CLI, pack system, orchestrator, and watch daemon work. The marketplace server and web frontend do not -- both are under active development. You can clone this repo and use the personas today via the CLI. +**Status:** The CLI, pack system, orchestrator, watch daemon, marketplace server, and web frontend all work. You can clone this repo and use the personas today via the CLI, or publish and browse packs against the live marketplace. Personas are not instruction lists. They are complete behavioral frames -- identity, rules, skills, operating posture -- that survive long sessions, surprising inputs, and the slow drift that turns careful agents into sloppy ones around turn 200. Same model, different frame. @@ -99,6 +99,10 @@ Signing is Ed25519 over that 32-byte hash; the signature travels alongside the p There is no central signing authority. An author **claims a handle** (e.g. `ghost-frame`) with a signed request, and the registry binds that handle to the key that signed it, first-claim-wins. Key rotation must be signed by the current key -- the old key authorizes its own replacement. At publish, the server checks that the live signer owns the handle and that the pack signature verifies against that registered key. On install from the registry, the client verifies the pack signature against the key in the **registry's record** for that version, not the key embedded in the manifest, so a tampered manifest cannot smuggle in a different key. Installing directly from a local path verifies a signature if one is present, and installs unsigned local packs as-is. +### Downloads + +`frameshift install` and `publish --server` fetch pack bytes from the direct, unauthenticated `GET /v1/packs/{name}/versions/{version}/pack` route -- this is the supported download path today. The server also implements a signed-download flow: `POST .../download-url` mints a short-lived, HMAC-signed `/dl/{hash}` URL (gated on the `DOWNLOAD_SECRET` env var, disabled when it is unset). That flow is fully built and tested server-side but has no caller yet in the CLI or client, so treat it as experimental / not-yet-default until something mints and follows those URLs. + ### Install and the central store `frameshift install` resolves a pack (from the registry, or `--from-path`), verifies it, and materializes it into a central store -- your project tree is never written to. The project is keyed by `project-id = sha256(realpath(project_root))`, so the same directory always maps to the same state regardless of how you path to it. @@ -143,10 +147,12 @@ A persona can declare a memory requirement in its manifest, and it is enforced a ### Growth -Each persona keeps an append-only local growth log at `personas//growth.md` -- things learned, mistakes caught, patterns discovered over a working session. +Each persona keeps an append-only local growth log -- things learned, mistakes caught, patterns discovered over a working session. Entries are dual-written to the legacy `personas//growth.md` and a structured `growth.jsonl`; `grow log` and `grow summary` read the structured form back. ```bash -frameshift grow append rust "orphan rules prevent implementing foreign traits on foreign types" +frameshift grow append --persona rust --text "orphan rules prevent implementing foreign traits on foreign types" +frameshift grow log --persona rust --limit 5 +frameshift grow summary --persona rust --scope project ``` ### Interfaces @@ -156,7 +162,7 @@ The same selection engine backs every surface: - **CLI** -- `frameshift ` (see below). - **Stdio MCP server** -- a JSON-RPC server exposing tools (install, activate, list, select, use, automate, prefs, grow, capabilities) and prompts (`active_persona`, `select_persona`, `automate_status`) as slash commands in any MCP-capable agent. - **Watch daemon** -- an optional background service over a peer-authenticated Unix socket, offering install/activate/sync/gc operations to editor integrations. -- **Registry / marketplace HTTP server** -- publish, search, download, and author/handle registration. The server and web frontend are under active development. +- **Registry / marketplace HTTP server** -- publish, search, download, and author/handle registration. Automate mode itself is applied by the host integration: a session hook (or equivalent) reads the per-project automate flag, calls `frameshift select` for the current task, and activates the best-fit persona. @@ -187,7 +193,10 @@ frameshift automate on [--sensitivity 0.0-1.0] Enable a frameshift automate off | status | lock | unlock Disable / inspect / pin / unpin frameshift feedback --chosen [--auto-pick ] Record a selection override [--intent ] [--reason ] -frameshift prefs [show|reset] View or reset preference biases +frameshift prefs show View current per-persona bias values +frameshift prefs bump Increase a persona's bias +frameshift prefs decay Decrease a persona's bias +frameshift prefs reset Clear all recorded preferences ``` Authoring and registry: @@ -197,16 +206,22 @@ frameshift rule add --id --layer --text Add a frameshift rule remove --id Remove a rule frameshift skill add --id --text Add a skill entry to a persona frameshift skill remove --id Remove a skill entry -frameshift grow append Append to a persona's growth log +frameshift grow append --persona --text Append to a persona's growth log +frameshift grow log --persona [--limit ] Show recent structured growth entries (default limit: 10) +frameshift grow summary --persona [--scope project|global] Summarize growth entries (default scope: project) frameshift diff Semantic diff between two personas frameshift render Render persona source to markdown -frameshift verify Run conformance checks +frameshift verify (--persona | --bundle

) Run conformance checks (exactly one of the two) + [--runner mock|cli] [--model ] [--threshold <0.0-1.0>] frameshift register --server --handle [--display-name ] Claim an author handle -frameshift publish Publish a persona pack +frameshift publish --persona [--out ] Build a persona pack (add --server + --handle to sign and upload) + [--server --handle ] frameshift search [QUERY] [--tag ] [--limit ] Search the registry frameshift project-id Print the hashed project ID ``` +`verify` defaults to `--runner mock` (canned, offline responses, used by CI) with `--threshold 0.5`; pass `--runner cli` to drive the subscription-backed `agy` runner against `--model` (default `Gemini 3.1 Pro (High)`), which needs a logged-in `agy`. `publish` writes the pack to `--out` (default `publish-output/`) unconditionally; the upload step only runs when `--server` is set, and `--handle` is then required. + ## What this repo contains - `crates/` -- Rust workspace: CLI, client engine, pack tooling, composition, conformance, catalog, memory, vault, object storage, HTTP server, MCP server, watch daemon, orchestrator, embeddings, growth @@ -214,6 +229,16 @@ frameshift project-id Print ## Building +Requires `libpq` (the PostgreSQL client library) for the diesel/pq-sys-backed catalog crate -- install it before building the workspace: + +```bash +# Debian/Ubuntu +sudo apt-get install libpq-dev + +# macOS +brew install libpq +``` + ```bash cargo build cargo test @@ -236,16 +261,37 @@ cargo run -p frameshift-cli -- select --task "optimize a hot loop" --format json ### Server +All variables are read with no prefix (e.g. `BIND_ADDR`, not `FRAMESHIFT_BIND_ADDR`); see `crates/frameshift-server/src/config.rs` for the authoritative parser. + | Variable | Default | Purpose | |---|---|---| | `BIND_ADDR` | `0.0.0.0:3000` | HTTP bind address | -| `POSTGRES_URL` | `""` | PostgreSQL connection URL | +| `POSTGRES_URL` | `""` | PostgreSQL connection URL (production must override) | | `OBJECT_STORE_ROOT` | `/tmp/frameshift-objects` | Filesystem object store root | -| `LOG_LEVEL` | `info` | Log filter | +| `LOG_LEVEL` | `info` | `tracing` subscriber filter string | | `LOG_FORMAT` | `text` | `text` or `json` | | `MAX_REQUEST_BYTES` | `1048576` | Max request body size | | `MAX_SEARCH_LIMIT` | `200` | Max search `limit` | | `SHUTDOWN_GRACE` | `30` | Grace period in seconds | +| `CORS_ALLOWED_ORIGINS` | `""` | Comma-separated allowed CORS origins; empty disables CORS | +| `DOWNLOAD_SECRET` | `""` | 64-char hex (32 bytes) HMAC key for signed download URLs; empty disables the signed-download endpoints | +| `DOWNLOAD_TOKEN_TTL` | `300` | Default TTL (seconds) for newly minted download tokens | +| `DOWNLOAD_MAX_TOKEN_TTL` | `1800` | Hard cap (seconds) on token TTL accepted by the verifier | +| `DOWNLOAD_RATE_PER_MIN` | `10` | Per-IP rate limit on the mint endpoint (requests/minute); `0` disables | +| `OBJECT_STORE_BACKEND` | `fs` | `fs` (filesystem) or `r2` (S3-compatible / Cloudflare R2) | +| `R2_ENDPOINT` | `""` | S3 endpoint URL for R2 (required when backend is `r2`) | +| `R2_BUCKET` | `""` | Bucket name (required when backend is `r2`) | +| `R2_PREFIX` | `objects` | Key prefix for pack blobs inside the bucket | +| `R2_REGION` | `auto` | S3 region (R2 always uses `auto`) | +| `R2_ACCESS_KEY_ID` | `""` | Access key ID for the bucket | +| `R2_SECRET_ACCESS_KEY` | `""` | Secret access key | +| `TRUST_FORWARDED_FOR` | `false` | Trust `X-Forwarded-For` for rate-limit key extraction; set `true` only behind a trusted proxy | +| `SIGNED_REQUEST_MAX_SKEW_SECS` | `300` | Max clock skew (seconds) allowed between a signed write request's timestamp and server time | +| `MEMORY_BACKEND` | `none` | `none`, `http`, or `sqlite` | +| `MEMORY_HTTP_ENDPOINT` | `""` | Base URL for the HTTP memory endpoint; used when `MEMORY_BACKEND=http` | +| `MEMORY_HTTP_AUTH` | `none` | `none` or `bearer:`; used when `MEMORY_BACKEND=http` | +| `MEMORY_HTTP_TIMEOUT_SECS` | `30` | Per-attempt request timeout for the HTTP memory adapter | +| `MEMORY_SQLITE_PATH` | `""` | Path to the SQLite database file; required when `MEMORY_BACKEND=sqlite` | ## License diff --git a/scripts/preflight.sh b/scripts/preflight.sh index 3e07d36..8cdbf86 100755 --- a/scripts/preflight.sh +++ b/scripts/preflight.sh @@ -3,8 +3,8 @@ # preflight.sh -- run the same gates CI runs, locally, before you push. # # Mirrors .github/workflows/ci.yml so a green preflight means a green CI (minus -# the Postgres-integration and mirror jobs, which need Docker / the remote and -# are not reproduced here). Run this before opening or updating a PR. +# the Postgres-integration job, which needs Docker and is not reproduced here). +# Run this before opening or updating a PR. # # Usage: # scripts/preflight.sh # fmt + clippy + test + audit From c9cd5ba8399090256e69c8dd1cb0e2bc0dc391af Mon Sep 17 00:00:00 2001 From: GhostFrame Date: Sat, 11 Jul 2026 19:06:15 -0400 Subject: [PATCH 09/10] feat(client): hard-block installs on conformance integrity failure A shipped baseline whose declared bundle hash does not match the bundle the pack actually ships means the conformance evidence cannot be trusted -- unlike a score regression, this indicates tampering or a broken publish pipeline, so the install-over-existing now fails with ConformanceIntegrityFailure instead of warning. The previously installed version is left untouched. Operators can consciously proceed with FRAMESHIFT_ALLOW_CONFORMANCE_INTEGRITY_FAILURE=1, in which case the CLI warning names the override. Regression, MissingBaseline, and InvalidScore stay warn-only. Also fills in missing doc comments across the client library surface. --- crates/frameshift-cli/src/main.rs | 25 ++- crates/frameshift-client/src/error.rs | 23 +++ crates/frameshift-client/src/lib.rs | 281 +++++++++++++++++++++++--- 3 files changed, 297 insertions(+), 32 deletions(-) diff --git a/crates/frameshift-cli/src/main.rs b/crates/frameshift-cli/src/main.rs index 8ae93c2..88ca5f9 100644 --- a/crates/frameshift-cli/src/main.rs +++ b/crates/frameshift-cli/src/main.rs @@ -148,6 +148,7 @@ enum RunError { NotImplemented(String), } +/// Human-readable rendering used when printing the error to stderr. impl std::fmt::Display for RunError { /// Format the run error for printing to stderr. fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { @@ -157,6 +158,8 @@ impl std::fmt::Display for RunError { } } +/// Bridge from the command layer's `CliError` into the process-level +/// `RunError`. impl From for RunError { /// Convert a `CliError` into a `RunError`, preserving the exit-code /// distinction for `NotImplemented` vs all other errors. @@ -473,10 +476,12 @@ fn current_dir() -> Result { /// /// Returning a message rather than printing directly keeps this testable /// without capturing stderr. This is purely informational: `Client::install` -/// has already committed the install by the time the caller prints this, so -/// nothing here can or should block anything -- see -/// `evaluate_conformance_upgrade` in `frameshift-client/src/lib.rs` for the -/// blocking-semantics rationale. +/// has already committed the install by the time the caller prints this. +/// An `IntegrityFailure` normally fails the install with +/// `ClientError::ConformanceIntegrityFailure` before any report exists, so +/// its arm here is only reached when the operator overrode the block with +/// `FRAMESHIFT_ALLOW_CONFORMANCE_INTEGRITY_FAILURE=1` -- see +/// `enforce_conformance_integrity` in `frameshift-client/src/lib.rs`. fn conformance_upgrade_warning( persona: &str, decision: &frameshift_client::CrossVersionDecision, @@ -493,7 +498,8 @@ fn conformance_upgrade_warning( actual_hash, } => Some(format!( "{persona}'s shipped conformance baseline failed integrity verification \ - (declared hash {declared_hash}, actual {actual_hash:?}); install not blocked" + (declared hash {declared_hash}, actual {actual_hash:?}); installed anyway \ + because FRAMESHIFT_ALLOW_CONFORMANCE_INTEGRITY_FAILURE=1 is set" )), CrossVersionDecision::InvalidScore => Some(format!( "{persona}'s conformance baseline score is invalid; cannot evaluate this \ @@ -502,6 +508,7 @@ fn conformance_upgrade_warning( } } +/// Unit tests for `conformance_upgrade_warning`'s per-variant messaging. #[cfg(test)] mod conformance_warning_tests { use super::*; @@ -522,8 +529,10 @@ mod conformance_warning_tests { .is_none()); } - /// Every non-clean variant produces a message naming the persona and - /// stating that the install was not blocked. + /// Every non-clean variant produces a message naming the persona. + /// Regression/InvalidScore state the install was not blocked; + /// IntegrityFailure (only printable under the operator override, since + /// it otherwise fails the install) names the override variable. #[test] fn message_for_every_non_clean_variant() { let regression = @@ -541,7 +550,7 @@ mod conformance_warning_tests { ) .unwrap(); assert!(integrity.contains("rust")); - assert!(integrity.contains("not blocked")); + assert!(integrity.contains("FRAMESHIFT_ALLOW_CONFORMANCE_INTEGRITY_FAILURE")); let invalid = conformance_upgrade_warning("rust", &CrossVersionDecision::InvalidScore).unwrap(); diff --git a/crates/frameshift-client/src/error.rs b/crates/frameshift-client/src/error.rs index 3fbb23a..c3d34f3 100644 --- a/crates/frameshift-client/src/error.rs +++ b/crates/frameshift-client/src/error.rs @@ -1,5 +1,8 @@ use std::path::PathBuf; +/// Every failure the client library can surface to callers (CLI, daemon, +/// MCP server, desktop app). Variants carry enough context to render an +/// actionable message without any additional lookup. #[derive(Debug, thiserror::Error)] pub enum ClientError { #[error("failed to read or write {path}: {source}")] @@ -155,6 +158,26 @@ pub enum ClientError { #[error("pack {0:?} exists in the registry but has no published version")] NoPublishedVersion(String), + /// An install-over-existing was refused because the incoming pack's shipped + /// conformance baseline failed integrity verification: the bundle hash the + /// baseline declares does not match the hash of the conformance bundle the + /// pack actually ships, so its conformance evidence cannot be trusted. + #[error( + "refusing to install persona {persona:?}: its shipped conformance baseline failed \ + integrity verification (declared bundle hash {declared_hash}, actual {actual_hash}); \ + the pack's conformance evidence may have been tampered with. Set \ + FRAMESHIFT_ALLOW_CONFORMANCE_INTEGRITY_FAILURE=1 to install anyway" + )] + ConformanceIntegrityFailure { + /// The persona whose upgrade was refused. + persona: String, + /// The bundle hash the shipped baseline declares. + declared_hash: String, + /// The hash of the conformance bundle the pack actually ships, or + /// `"missing"` when the pack ships no bundle at all. + actual_hash: String, + }, + /// The persona's pack manifest declares `memory_required = "hard"` but the /// project declares no `[memory]` adapter in its config.toml. #[error( diff --git a/crates/frameshift-client/src/lib.rs b/crates/frameshift-client/src/lib.rs index 229c22d..e5b28fd 100644 --- a/crates/frameshift-client/src/lib.rs +++ b/crates/frameshift-client/src/lib.rs @@ -64,6 +64,7 @@ pub struct Client { config_root: Option, } +/// Inherent methods implementing the Frameshift client's public API surface. impl Client { /// Construct a `Client` from the given options. pub fn new(options: ClientOptions) -> Self { @@ -81,6 +82,7 @@ impl Client { })) } + /// Return the root of the managed Frameshift data directory. pub fn data_root(&self) -> &Path { &self.data_root } @@ -146,6 +148,13 @@ impl Client { registry::resolve_latest_version(name) } + /// Resolve the project id for `project_root`. + /// + /// If the `FRAMESHIFT_PROJECT_ID` env var is set to a non-empty value, it + /// is validated via [`validate_explicit_project_id`] and returned + /// directly, overriding the default derivation. Otherwise the id is + /// derived by hashing the canonicalized project root path via + /// [`hashed_project_id`]. pub fn project_id(&self, project_root: &Path) -> Result { if let Ok(explicit) = std::env::var(PROJECT_ID_ENV) { if !explicit.is_empty() { @@ -157,6 +166,13 @@ impl Client { hashed_project_id(project_root) } + /// Compute the central-store paths for `project_root`. + /// + /// Derives the project id, then builds `config_path`/`lock_path`/ + /// `cache_dir`/`active_path`/`personas_dir` under the data root. As a + /// side effect, runs [`migrate_legacy_project_files`] to migrate any + /// pre-WS-1 `frameshift.toml`/`frameshift.lock` found at the project root + /// into the central store. pub fn project_paths(&self, project_root: &Path) -> Result { let project_id = self.project_id(project_root)?; let cache_dir = self.data_root.join("cache"); @@ -178,6 +194,19 @@ impl Client { Ok(paths) } + /// Install a persona into `request.project_root` from either a local pack + /// directory or the HTTP registry. + /// + /// Verifies the pack's manifest matches the requested spec and its + /// Ed25519 signature (when present) for a local install, or fetches, + /// extracts, and verifies via the registry. Compares the incoming pack's + /// conformance baseline against any previously-installed version of the + /// same persona before overwriting it; an integrity-verification failure + /// hard-blocks the install unless + /// `FRAMESHIFT_ALLOW_CONFORMANCE_INTEGRITY_FAILURE=1` is set (a mere + /// score regression only warns). On success, upserts the lockfile and + /// re-materializes project state, returning the resulting + /// [`InstallReport`]. pub fn install(&self, request: InstallRequest) -> Result { ensure_exists(&request.project_root)?; @@ -198,17 +227,32 @@ impl Client { } }; - // Best-effort, warn-only comparison of shipped conformance baselines - // against any version of this persona already installed for the - // project. Must be evaluated before `finish_install` overwrites the - // previously-installed persona directory. Never fails the install -- - // see `evaluate_conformance_upgrade`. + // Comparison of shipped conformance baselines against any version of + // this persona already installed for the project. Must be evaluated + // before `finish_install` overwrites the previously-installed persona + // directory. Regression/MissingBaseline/InvalidScore are warn-only; + // IntegrityFailure hard-blocks the install (overridable via + // `FRAMESHIFT_ALLOW_CONFORMANCE_INTEGRITY_FAILURE=1`) -- see + // `enforce_conformance_integrity`. let conformance_upgrade = evaluate_conformance_upgrade(&paths, &locked); + enforce_conformance_integrity( + &locked.name, + conformance_upgrade.as_ref(), + integrity_override_enabled(), + )?; // Shared tail: upsert into lockfile and materialize project state. finish_install(self, &paths, locked, conformance_upgrade) } + /// Mark `persona` as the active persona for `project_root`. + /// + /// First re-syncs the project (see [`Client::sync`]), then errors with + /// [`ClientError::PersonaNotInstalled`] if `persona` is not among the + /// synced personas. Refuses to activate (returns + /// [`ClientError::MemoryRequirementUnmet`]) when the persona's memory + /// contract is hard-required but the project declares no memory adapter. + /// On success, writes `persona` into the project's `active` marker file. pub fn activate(&self, project_root: &Path, persona: &str) -> Result<(), ClientError> { let report = self.sync(project_root)?; if !report.personas.iter().any(|installed| installed == persona) { @@ -420,6 +464,12 @@ impl Client { } } + /// Re-materialize project state from the project's lockfile. + /// + /// Reads `projects//lock.toml`; if it does not exist yet, returns an + /// empty [`SyncReport`] without touching disk further. Otherwise calls + /// [`Client::materialize_project_state`] to rebuild `personas/` from the + /// cache and returns the list of locked persona names. pub fn sync(&self, project_root: &Path) -> Result { let paths = self.project_paths(project_root)?; let Some((raw_lock, lockfile)) = load_lockfile_with_raw(&paths.lock_path)? else { @@ -440,6 +490,11 @@ impl Client { }) } + /// Remove cache entries not referenced by any project's lockfile. + /// + /// Scans every `projects//lock.toml` to build the set of referenced + /// pack hashes, then deletes any `cache/` directory not in that + /// set. Returns a [`GcReport`] listing the hashes that were removed. pub fn gc(&self) -> Result { let mut referenced_hashes = BTreeSet::new(); let projects_root = self.data_root.join("projects"); @@ -591,6 +646,16 @@ impl Client { Ok(paths.project_state_dir) } + /// Rebuild `personas_dir` on disk to exactly match `lockfile`. + /// + /// Validates every persona name (guarding against path traversal), writes + /// `raw_lock` to `lock_path`, removes any `personas/` directory not + /// present in `lockfile`, and for each locked persona re-copies its + /// source from the content-addressed cache and re-renders its output via + /// `materialize_persona_rendered_outputs`. Returns + /// [`ClientError::MissingCacheEntry`] if a locked persona's hash is not + /// present in the cache. Also clears the `active` marker file if it + /// points at a persona no longer present in `lockfile`. fn materialize_project_state( &self, paths: &ProjectPaths, @@ -777,6 +842,11 @@ impl Client { } } +/// Resolve the default Frameshift data root. +/// +/// Uses `XDG_DATA_HOME/frameshift` when `XDG_DATA_HOME` is set and +/// non-empty, otherwise falls back to `$HOME/.local/share/frameshift`. +/// Errors when neither is available. fn default_data_root() -> Result { if let Ok(xdg_data_home) = std::env::var("XDG_DATA_HOME") { if !xdg_data_home.is_empty() { @@ -815,6 +885,10 @@ fn default_config_root() -> Result { Ok(home.join(".config")) } +/// Validate an explicit project id supplied via `FRAMESHIFT_PROJECT_ID`. +/// +/// Rejects an empty string, `.`, `..`, and any id containing `/` or `\`, +/// since the id is joined as a single path component under the data root. fn validate_explicit_project_id(project_id: &str) -> Result<(), ClientError> { if project_id.is_empty() || project_id == "." || project_id == ".." || project_id.contains('/') { @@ -945,6 +1019,10 @@ fn migrate_legacy_project_files(project_root: &Path, paths: &ProjectPaths) { } } +/// Derive a project id by SHA-256 hashing the canonicalized project root path. +/// +/// Fails if the path cannot be canonicalized (e.g. it does not exist) or if +/// the canonicalized path is not valid UTF-8. fn hashed_project_id(project_root: &Path) -> Result { let canonical_root = fs::canonicalize(project_root).map_err(|source| ClientError::Io { path: project_root.to_path_buf(), @@ -971,6 +1049,13 @@ pub(crate) fn validate_pack_request(pack: &Pack, spec: &PersonaSpec) -> Result<( Ok(()) } +/// Verify `pack`'s Ed25519 signature against its declared author pubkey, if +/// the pack carries a signature at all. +/// +/// A pack with no signature is accepted unchanged (local installs are not +/// required to be signed). Returns [`ClientError::InvalidAuthorPublicKey`] +/// if the pubkey cannot be decoded, or [`ClientError::SignatureVerification`] +/// if the signature does not verify. fn verify_pack_signature_if_present(pack: &Pack) -> Result<(), ClientError> { if !pack.has_signature() { return Ok(()); @@ -983,6 +1068,11 @@ fn verify_pack_signature_if_present(pack: &Pack) -> Result<(), ClientError> { .map_err(|_| ClientError::SignatureVerification) } +/// Decode a 32-byte Ed25519 public key from `encoded`, trying hex, then +/// base64 URL-safe (no padding), then base64 standard (no padding) in turn. +/// +/// Returns [`ClientError::InvalidAuthorPublicKey`] if none of the encodings +/// produce exactly 32 bytes. fn parse_verifying_key_bytes(encoded: &str) -> Result<[u8; 32], ClientError> { if let Ok(bytes) = hex::decode(encoded) { if let Ok(array) = <[u8; 32]>::try_from(bytes.as_slice()) { @@ -1017,6 +1107,10 @@ pub(crate) fn locked_persona_from_pack(pack: &Pack) -> LockedPersona { } } +/// Insert or replace `persona` in `lockfile.personas` by name. +/// +/// If a persona with the same name is already present, it is replaced in +/// place; otherwise `persona` is appended and the list is re-sorted by name. fn upsert_locked_persona(lockfile: &mut Lockfile, persona: LockedPersona) { if let Some(existing) = lockfile .personas @@ -1033,10 +1127,19 @@ fn upsert_locked_persona(lockfile: &mut Lockfile, persona: LockedPersona) { .sort_by(|left, right| left.name.cmp(&right.name)); } +/// Load and parse the lockfile at `path`, discarding the raw TOML text. +/// +/// Returns `Ok(None)` when the file does not exist. See +/// [`load_lockfile_with_raw`]. fn load_lockfile(path: &Path) -> Result, ClientError> { load_lockfile_with_raw(path).map(|maybe| maybe.map(|(_, lockfile)| lockfile)) } +/// Read and parse the lockfile at `path`, returning both the raw TOML text +/// and the parsed [`Lockfile`]. +/// +/// Returns `Ok(None)` when `path` does not exist. Returns +/// [`ClientError::TomlDeserialize`] if the file exists but fails to parse. fn load_lockfile_with_raw(path: &Path) -> Result, ClientError> { if !path.exists() { return Ok(None); @@ -1131,6 +1234,12 @@ fn compose_rendered_content( composed } +/// Locate the markdown file to render for a pack directory. +/// +/// Tries `RENDER_CANDIDATES` in priority order (`AGENTS.md`, `CLAUDE.md`, +/// `GEMINI.md`, `README.md`), then falls back to the first `.md` file found +/// in `pack_dir` (sorted). Returns [`ClientError::MissingRenderSource`] if +/// no candidate exists. fn find_render_source(pack_dir: &Path) -> Result { for candidate in RENDER_CANDIDATES { let path = pack_dir.join(candidate); @@ -1149,6 +1258,7 @@ fn find_render_source(pack_dir: &Path) -> Result { Err(ClientError::MissingRenderSource(pack_dir.to_path_buf())) } +/// Return an error unless `path` exists on disk. fn ensure_exists(path: &Path) -> Result<(), ClientError> { if path.exists() { return Ok(()); @@ -1160,6 +1270,8 @@ fn ensure_exists(path: &Path) -> Result<(), ClientError> { }) } +/// Create `path` and any missing parent directories, wrapping failures as +/// [`ClientError::Io`]. fn ensure_dir(path: &Path) -> Result<(), ClientError> { fs::create_dir_all(path).map_err(|source| ClientError::Io { path: path.to_path_buf(), @@ -1167,6 +1279,7 @@ fn ensure_dir(path: &Path) -> Result<(), ClientError> { }) } +/// Create an empty file at `path` if one does not already exist. fn touch_empty(path: &Path) -> Result<(), ClientError> { if path.exists() { return Ok(()); @@ -1174,6 +1287,7 @@ fn touch_empty(path: &Path) -> Result<(), ClientError> { write_file(path, b"") } +/// Write `bytes` to `path`, creating any missing parent directories first. fn write_file(path: &Path, bytes: &[u8]) -> Result<(), ClientError> { if let Some(parent) = path.parent() { ensure_dir(parent)?; @@ -1184,6 +1298,8 @@ fn write_file(path: &Path, bytes: &[u8]) -> Result<(), ClientError> { }) } +/// Read the entire contents of `path` as a UTF-8 string, wrapping failures +/// as [`ClientError::Io`]. fn read_to_string(path: &Path) -> Result { fs::read_to_string(path).map_err(|source| ClientError::Io { path: path.to_path_buf(), @@ -1191,6 +1307,8 @@ fn read_to_string(path: &Path) -> Result { }) } +/// Recursively remove the directory at `path`, wrapping failures as +/// [`ClientError::Io`]. fn remove_dir_all(path: &Path) -> Result<(), ClientError> { fs::remove_dir_all(path).map_err(|source| ClientError::Io { path: path.to_path_buf(), @@ -1198,6 +1316,7 @@ fn remove_dir_all(path: &Path) -> Result<(), ClientError> { }) } +/// Remove the file at `path`, treating a missing file as success. fn remove_file_if_exists(path: &Path) -> Result<(), ClientError> { match fs::remove_file(path) { Ok(()) => Ok(()), @@ -1209,6 +1328,10 @@ fn remove_file_if_exists(path: &Path) -> Result<(), ClientError> { } } +/// Recursively copy every file and subdirectory from `source` into +/// `destination`, creating directories as needed. +/// +/// Skips entries that are neither files nor directories (e.g. symlinks). fn copy_dir_recursive(source: &Path, destination: &Path) -> Result<(), ClientError> { ensure_dir(destination)?; for entry in read_dir_sorted(source)? { @@ -1232,6 +1355,8 @@ fn copy_dir_recursive(source: &Path, destination: &Path) -> Result<(), ClientErr Ok(()) } +/// Read the entries of `path` and return them sorted by file name, for +/// deterministic iteration order. fn read_dir_sorted(path: &Path) -> Result, ClientError> { let mut entries = fs::read_dir(path) .map_err(|source| ClientError::Io { @@ -1287,9 +1412,9 @@ fn install_from_registry( registry::fetch_and_install(spec, paths) } -/// Best-effort, non-blocking comparison of `locked`'s shipped conformance -/// baseline against the baseline of any version of the same persona already -/// installed for this project. +/// Comparison of `locked`'s shipped conformance baseline against the +/// baseline of any version of the same persona already installed for this +/// project. /// /// Returns `None` when there is no previously-installed version of /// `locked.name` (a fresh install, not an upgrade) -- this is the only @@ -1297,9 +1422,13 @@ fn install_from_registry( /// comparison applies at all. When there is a previous version, delegates /// to `frameshift_conformance::RegressionGate::evaluate_cross_version` and /// logs the result via `tracing`: `Regression`/`IntegrityFailure`/ -/// `InvalidScore` at `warn!` (all warn-only -- installation is never -/// blocked), `MissingBaseline` at `debug!` (an expected, non-fatal outcome -/// for packs that ship no baseline). +/// `InvalidScore` at `warn!`, `MissingBaseline` at `debug!` (an expected, +/// non-fatal outcome for packs that ship no baseline). +/// +/// This function only observes and logs; enforcement is the caller's job. +/// `Regression`/`MissingBaseline`/`InvalidScore` are warn-only, while +/// `IntegrityFailure` hard-blocks the install via +/// [`enforce_conformance_integrity`]. /// /// Must be called *before* `finish_install`/`materialize_project_state` /// overwrites the previously-installed persona directory, since this reads @@ -1352,8 +1481,7 @@ fn evaluate_conformance_upgrade( version = %locked.version, declared_hash, actual_hash = ?actual_hash, - "incoming pack's conformance baseline failed integrity verification \ - (warn-only, install not blocked)" + "incoming pack's conformance baseline failed integrity verification" ); } frameshift_conformance::CrossVersionDecision::MissingBaseline { @@ -1380,6 +1508,57 @@ fn evaluate_conformance_upgrade( Some(decision) } +/// Environment variable that lets an operator explicitly bypass the +/// conformance-integrity hard block on install. Only the exact value `"1"` +/// counts as set. +const ALLOW_INTEGRITY_FAILURE_ENV: &str = "FRAMESHIFT_ALLOW_CONFORMANCE_INTEGRITY_FAILURE"; + +/// Whether the operator has explicitly opted to install packs whose shipped +/// conformance baseline fails integrity verification. +fn integrity_override_enabled() -> bool { + std::env::var(ALLOW_INTEGRITY_FAILURE_ENV).is_ok_and(|v| v == "1") +} + +/// Hard gate on the cross-version conformance decision: refuse an +/// install-over-existing whose shipped baseline fails integrity verification. +/// +/// A baseline whose declared bundle hash does not match the bundle the pack +/// actually ships means the pack's conformance evidence cannot be trusted -- +/// unlike a mere score regression, this indicates tampering or a broken +/// publish pipeline, so it blocks the install. `override_enabled` (from +/// [`integrity_override_enabled`]) downgrades the block to a `warn!` so an +/// operator can consciously proceed. Every other decision variant (and a +/// fresh install's `None`) passes through untouched. +fn enforce_conformance_integrity( + persona: &str, + decision: Option<&frameshift_conformance::CrossVersionDecision>, + override_enabled: bool, +) -> Result<(), ClientError> { + let Some(frameshift_conformance::CrossVersionDecision::IntegrityFailure { + declared_hash, + actual_hash, + }) = decision + else { + return Ok(()); + }; + let actual = actual_hash.clone().unwrap_or_else(|| "missing".to_string()); + if override_enabled { + warn!( + persona, + declared_hash, + actual_hash = %actual, + "conformance-integrity failure overridden by {ALLOW_INTEGRITY_FAILURE_ENV}=1; \ + installing anyway" + ); + return Ok(()); + } + Err(ClientError::ConformanceIntegrityFailure { + persona: persona.to_string(), + declared_hash: declared_hash.clone(), + actual_hash: actual, + }) +} + /// Read and parse `manifest_path` as a [`frameshift_pack::PackManifest`], /// returning its `conformance_baseline` if present. /// @@ -1394,10 +1573,47 @@ fn read_pack_conformance_baseline( manifest.conformance_baseline } +/// Unit tests for install/activate/sync/gc, lockfile handling, persona-name +/// validation, conformance-upgrade evaluation, and rendered-output +/// composition. #[cfg(test)] mod tests { use super::*; + /// enforce_conformance_integrity blocks IntegrityFailure, honors the + /// operator override, and passes every other decision (and fresh + /// installs) through untouched. + #[test] + fn conformance_integrity_gate_blocks_and_overrides() { + use frameshift_conformance::CrossVersionDecision as D; + let failure = D::IntegrityFailure { + declared_hash: "aa".into(), + actual_hash: Some("bb".into()), + }; + let err = enforce_conformance_integrity("rust", Some(&failure), false).unwrap_err(); + assert!(matches!( + err, + ClientError::ConformanceIntegrityFailure { .. } + )); + assert!(enforce_conformance_integrity("rust", Some(&failure), true).is_ok()); + let regression = D::Regression { delta: 0.5 }; + assert!(enforce_conformance_integrity("rust", Some(®ression), false).is_ok()); + assert!(enforce_conformance_integrity("rust", Some(&D::Pass), false).is_ok()); + assert!(enforce_conformance_integrity("rust", None, false).is_ok()); + } + + /// A pack that ships no conformance bundle renders its actual hash as + /// "missing" in the blocking error. + #[test] + fn conformance_integrity_error_renders_missing_actual_hash() { + let failure = frameshift_conformance::CrossVersionDecision::IntegrityFailure { + declared_hash: "aa".into(), + actual_hash: None, + }; + let err = enforce_conformance_integrity("rust", Some(&failure), false).unwrap_err(); + assert!(err.to_string().contains("actual missing")); + } + /// validate_persona_name accepts clean slugs and rejects traversal/separators. #[test] fn validate_persona_name_guards_traversal() { @@ -1639,6 +1855,7 @@ mod tests { ); } + /// PersonaSpec parsing rejects strings missing a name or version. #[test] fn rejects_invalid_persona_specs() { assert!("cryptographic".parse::().is_err()); @@ -1646,6 +1863,8 @@ mod tests { assert!("cryptographic@".parse::().is_err()); } + /// validate_explicit_project_id rejects path separators and accepts a + /// plain id. #[test] fn explicit_project_id_rejects_path_separators() { assert!(validate_explicit_project_id("team/alpha").is_err()); @@ -1653,6 +1872,8 @@ mod tests { assert!(validate_explicit_project_id("valid-id").is_ok()); } + /// Rendered output composes the infrastructure overlay before the + /// persona content when `config_root` provides one. #[test] fn rendered_output_includes_infra_overlay() { let tmp = tempfile::tempdir().unwrap(); @@ -1730,6 +1951,8 @@ mod tests { ); } + /// Rendered output contains only the persona content when no + /// infrastructure overlay is configured. #[test] fn rendered_output_works_without_infra_overlay() { let tmp = tempfile::tempdir().unwrap(); @@ -1955,9 +2178,11 @@ mod tests { } /// An incoming pack that declares a baseline but ships no - /// `conformance/bundle.toml` to verify it against reports - /// `IntegrityFailure` with `actual_hash: None`, never `Pass` or - /// `Regression` -- and the install still succeeds (warn-only). + /// `conformance/bundle.toml` to verify it against is an integrity + /// failure, and integrity failures hard-block the install: the second + /// install must fail with `ClientError::ConformanceIntegrityFailure` + /// (rendering the unshipped bundle's hash as "missing") and must leave + /// the previously-installed version untouched. #[test] fn upgrade_with_unshipped_bundle_reports_integrity_failure() { let tmp = tempfile::tempdir().unwrap(); @@ -1985,26 +2210,34 @@ mod tests { // conformance/bundle.toml is written to back it up. let new_pack_dir = tmp.path().join("new-pack"); write_pack_with_baseline(&new_pack_dir, "conform-noverify", "2.0.0", 0.9, false); - let report = client + let err = client .install(InstallRequest { - project_root, + project_root: project_root.clone(), spec: PersonaSpec { name: "conform-noverify".to_string(), version: "2.0.0".to_string(), }, source: InstallSource::LocalPath(new_pack_dir), }) - .unwrap(); + .expect_err("integrity failure must hard-block the install"); - match report.conformance_upgrade { - Some(frameshift_conformance::CrossVersionDecision::IntegrityFailure { + match &err { + ClientError::ConformanceIntegrityFailure { + persona, actual_hash, .. - }) => { - assert_eq!(actual_hash, None); + } => { + assert_eq!(persona, "conform-noverify"); + assert_eq!(actual_hash, "missing"); } - other => panic!("expected Some(IntegrityFailure), got {other:?}"), + other => panic!("expected ConformanceIntegrityFailure, got {other:?}"), } + + // The blocked upgrade must not have clobbered the installed version. + let lockfile_raw = + fs::read_to_string(client.project_paths(&project_root).unwrap().lock_path).unwrap(); + assert!(lockfile_raw.contains("1.0.0")); + assert!(!lockfile_raw.contains("2.0.0")); } /// Upgrading a persona that has never shipped a baseline (old or new) From bdffc9fae48dce6780d0f6f0c2ed416dbe40aaec Mon Sep 17 00:00:00 2001 From: GhostFrame Date: Sat, 11 Jul 2026 19:06:15 -0400 Subject: [PATCH 10/10] feat(server): admin tombstone endpoint POST /v1/admin/packs/{name}/{version}/tombstone exposes the previously-unwired catalog tombstone (one-way Active -> Tombstone, idempotent per the Postgres adapter). Auth reuses the Ed25519 signed-request middleware plus a new FRAMESHIFT_ADMIN_PUBKEYS allowlist (comma-separated base64url keys); an empty allowlist disables the endpoint entirely and answers 404 so its existence is not revealed, a verified non-admin signer gets a generic 403. Body carries the tombstone reason (author-request, tos-violation, dmca). Covered by six integration tests through the real router. --- crates/frameshift-server/src/config.rs | 56 +++- crates/frameshift-server/src/lib.rs | 3 + crates/frameshift-server/src/router.rs | 23 +- crates/frameshift-server/src/routes/admin.rs | 153 +++++++++ crates/frameshift-server/src/routes/mod.rs | 3 + .../tests/admin_tombstone.rs | 297 ++++++++++++++++++ .../frameshift-server/tests/authors_write.rs | 1 + crates/frameshift-server/tests/integration.rs | 2 + .../frameshift-server/tests/mocks/catalog.rs | 34 +- crates/frameshift-server/tests/publish.rs | 1 + crates/frameshift-server/tests/telemetry.rs | 1 + 11 files changed, 560 insertions(+), 14 deletions(-) create mode 100644 crates/frameshift-server/src/routes/admin.rs create mode 100644 crates/frameshift-server/tests/admin_tombstone.rs diff --git a/crates/frameshift-server/src/config.rs b/crates/frameshift-server/src/config.rs index 1d03d5b..21727cd 100644 --- a/crates/frameshift-server/src/config.rs +++ b/crates/frameshift-server/src/config.rs @@ -31,10 +31,14 @@ //! | `R2_SECRET_ACCESS_KEY` | `""` | Secret access key (supplied via a secrets manager in production) | //! | `TRUST_FORWARDED_FOR` | `false` | Trust `X-Forwarded-For` for rate-limit key extraction; set `true` only behind a trusted proxy | //! | `SIGNED_REQUEST_MAX_SKEW_SECS` | `300` | Max allowed clock skew (seconds) between a signed write request's timestamp and server time; also bounds the replay-nonce retention window | +//! | `FRAMESHIFT_ADMIN_PUBKEYS` | `""` | Comma-separated base64url-no-pad Ed25519 public keys allowed to call `/v1/admin/*` endpoints; empty disables all admin endpoints (404) | //! //! Env var names match the struct field names verbatim (figment maps //! `download_secret` <-> `DOWNLOAD_SECRET`); shorter aliases would require an -//! explicit remap step which we don't have yet. +//! explicit remap step which we don't have yet. `FRAMESHIFT_ADMIN_PUBKEYS` is +//! the one deliberate exception: it carries the `FRAMESHIFT_` prefix so it +//! cannot be confused with an unrelated `ADMIN_PUBKEYS` variable that some +//! other tool in the deployment environment might already own. use std::net::SocketAddr; use std::path::PathBuf; @@ -205,6 +209,19 @@ pub struct ServerConfig { /// Default: 300 seconds (5 minutes). pub signed_request_max_skew: Duration, + /// Base64url-no-pad Ed25519 public keys authorized to call the admin + /// endpoints (e.g. `POST /v1/admin/packs/{name}/{version}/tombstone`). + /// + /// Parsed from the comma-separated `FRAMESHIFT_ADMIN_PUBKEYS` env var. + /// Stored in the same base64url-no-pad string representation produced by + /// `Ed25519PublicKey`'s `Display` impl, so callers compare with a plain + /// string equality against + /// `verified_signer.pubkey.to_string()` (see + /// [`crate::auth::VerifiedSigner`]). Default: empty (admin endpoints + /// disabled; handlers return `404` rather than `403` so the route's + /// existence is not disclosed). + pub admin_pubkeys: Vec, + /// Memory backend selector: `"none"` (default), `"http"`, or `"sqlite"`. /// /// - `"none"` -- no memory adapter; personas that require memory will fail @@ -306,6 +323,10 @@ impl std::fmt::Debug for ServerConfig { .field("r2_secret_access_key", &"[REDACTED]") .field("trust_forwarded_for", &self.trust_forwarded_for) .field("signed_request_max_skew", &self.signed_request_max_skew) + .field( + "admin_pubkeys", + &format!("[{} key(s)]", self.admin_pubkeys.len()), + ) .field("memory_backend", &self.memory_backend) .field("memory_http_endpoint", &self.memory_http_endpoint) .field("memory_http_auth", &"[REDACTED]") @@ -387,6 +408,11 @@ struct RawConfig { /// `SIGNED_REQUEST_MAX_SKEW_SECS`). signed_request_max_skew_secs: u64, + /// Comma-separated base64url-no-pad Ed25519 admin public keys (raw + /// string, split into `Vec` on convert). Maps to + /// `FRAMESHIFT_ADMIN_PUBKEYS`. + admin_pubkeys: String, + /// Memory backend selector. memory_backend: String, /// HTTP memory endpoint URL. @@ -399,6 +425,21 @@ struct RawConfig { memory_sqlite_path: String, } +/// Split a comma-separated raw string into a `Vec`, trimming +/// whitespace around each entry and skipping empty segments. +/// +/// Mirrors the parsing convention already used by +/// [`ServerConfig::cors_origins`], but eagerly collects into an owned +/// `Vec` instead of returning a lazy iterator, since +/// `admin_pubkeys` is compared on every admin-route request. +fn split_comma_list(raw: &str) -> Vec { + raw.split(',') + .map(str::trim) + .filter(|s| !s.is_empty()) + .map(str::to_string) + .collect() +} + impl RawConfig { /// Convert this raw configuration into a [`ServerConfig`]. /// @@ -428,6 +469,7 @@ impl RawConfig { r2_secret_access_key: SecretString::new(self.r2_secret_access_key), trust_forwarded_for: self.trust_forwarded_for, signed_request_max_skew: Duration::from_secs(self.signed_request_max_skew_secs), + admin_pubkeys: split_comma_list(&self.admin_pubkeys), memory_backend: self.memory_backend, memory_http_endpoint: self.memory_http_endpoint, memory_http_auth: self.memory_http_auth, @@ -465,6 +507,7 @@ fn default_raw_config() -> RawConfig { r2_secret_access_key: String::new(), trust_forwarded_for: false, signed_request_max_skew_secs: 300, + admin_pubkeys: String::new(), memory_backend: "none".to_string(), memory_http_endpoint: String::new(), memory_http_auth: "none".to_string(), @@ -479,7 +522,11 @@ impl ServerConfig { /// /// Environment variables are read with no prefix (e.g. `BIND_ADDR` not /// `FRAMESHIFT_BIND_ADDR`). See the module-level documentation for the full - /// mapping. + /// mapping. `admin_pubkeys` is the sole exception: it is read from the + /// prefixed `FRAMESHIFT_ADMIN_PUBKEYS` var via a second, narrowly-scoped + /// `Env::prefixed(...).only(...)` merge layered on top of the unprefixed + /// provider, so every other field keeps the established no-prefix + /// convention. /// /// # Errors /// @@ -491,6 +538,7 @@ impl ServerConfig { pub fn from_env() -> Result> { let raw: RawConfig = Figment::from(Serialized::defaults(default_raw_config())) .merge(Env::raw()) + .merge(Env::prefixed("FRAMESHIFT_").only(&["admin_pubkeys"])) .extract() .map_err(Box::new)?; Ok(raw.into_server_config()) @@ -531,6 +579,7 @@ mod tests { r2_secret_access_key: SecretString::new(String::new()), trust_forwarded_for: false, signed_request_max_skew: Duration::from_secs(300), + admin_pubkeys: Vec::new(), memory_backend: "none".to_string(), memory_http_endpoint: String::new(), memory_http_auth: "none".to_string(), @@ -570,6 +619,7 @@ mod tests { r2_secret_access_key: SecretString::new(String::new()), trust_forwarded_for: false, signed_request_max_skew: Duration::from_secs(300), + admin_pubkeys: Vec::new(), memory_backend: "none".to_string(), memory_http_endpoint: String::new(), memory_http_auth: "none".to_string(), @@ -605,6 +655,7 @@ mod tests { r2_secret_access_key: SecretString::new(String::new()), trust_forwarded_for: false, signed_request_max_skew: Duration::from_secs(300), + admin_pubkeys: Vec::new(), memory_backend: "none".to_string(), memory_http_endpoint: String::new(), memory_http_auth: "none".to_string(), @@ -667,6 +718,7 @@ mod tests { r2_secret_access_key: SecretString::new(String::new()), trust_forwarded_for: false, signed_request_max_skew: Duration::from_secs(300), + admin_pubkeys: Vec::new(), memory_backend: "none".to_string(), memory_http_endpoint: String::new(), memory_http_auth: "none".to_string(), diff --git a/crates/frameshift-server/src/lib.rs b/crates/frameshift-server/src/lib.rs index 0dfe123..5f1cb24 100644 --- a/crates/frameshift-server/src/lib.rs +++ b/crates/frameshift-server/src/lib.rs @@ -33,6 +33,9 @@ //! content signature against the handle's currently-registered key. //! - Signed download URLs: `POST /v1/downloads` (mint) and `GET /dl/{hash}`. //! - Operational endpoints: `/healthz`, `/metrics` (real Prometheus registry). +//! - Admin: `POST /v1/admin/packs/{name}/{version}/tombstone`, gated by the +//! same signed-request middleware plus an operator-controlled pubkey +//! allowlist (`FRAMESHIFT_ADMIN_PUBKEYS`; see [`crate::routes::admin`]). //! - MCP placeholder: `/mcp/*` returns 501. //! //! Deferred (M5+): OAuth 2.1, transparency log, full MCP surface, and a shared diff --git a/crates/frameshift-server/src/router.rs b/crates/frameshift-server/src/router.rs index fd47835..65929af 100644 --- a/crates/frameshift-server/src/router.rs +++ b/crates/frameshift-server/src/router.rs @@ -33,11 +33,14 @@ //! layer (governor) applied only to its sub-router, so the rest of the //! API (including the verifier `/dl/{hash}`) is not impacted. //! -//! The mutating endpoints -- `POST /v1/packs`, `POST /v1/authors`, and -//! `POST /v1/authors/{handle}/rotate` -- carry the Ed25519 signed-request -//! `route_layer` ([`crate::middleware::auth::require_signed_request`]). It is -//! applied only to those method-routers, so anonymous reads on the same paths -//! (e.g. `GET /v1/packs`) never buffer a body or require a signature. +//! The mutating endpoints -- `POST /v1/packs`, `POST /v1/authors`, +//! `POST /v1/authors/{handle}/rotate`, and +//! `POST /v1/admin/packs/{name}/{version}/tombstone` -- carry the Ed25519 +//! signed-request `route_layer` ([`crate::middleware::auth::require_signed_request`]). +//! It is applied only to those method-routers, so anonymous reads on the same +//! paths (e.g. `GET /v1/packs`) never buffer a body or require a signature. +//! The admin router additionally enforces an allowlist on top of signature +//! verification -- see [`crate::routes::admin`]. use std::num::NonZeroU32; use std::sync::Arc; @@ -59,6 +62,7 @@ use crate::middleware::auth::require_signed_request; use crate::middleware::metrics::MetricsLayer; use crate::middleware::request_id::RequestIdGenerator; use crate::middleware::tracing::make_trace_layer; +use crate::routes::admin::admin_router; use crate::routes::authors::{authors_router, authors_write_router}; use crate::routes::downloads::{dl_router, pack_download_url_router}; use crate::routes::handles::handles_router; @@ -82,6 +86,7 @@ use crate::state::AppState; /// /handles -- handle lookup /// /telemetry -- POST /selection opt-in selection telemetry sink /// /memory -- GET /health read-only memory backend health +/// /admin -- POST /packs/{name}/{version}/tombstone (signed + allowlist) /// /mcp -- MCP placeholder (501 for all methods) /// ``` /// @@ -124,12 +129,18 @@ pub fn app(state: AppState) -> Router { // (registration + key rotation). let authors = authors_router().merge(authors_write_router().route_layer(signed.clone())); + // Admin: every route in this sub-router is mutating and allowlist-gated, + // so the whole router carries the signed-request layer (unlike `packs` + // and `authors`, there is no anonymous-read counterpart to merge with). + let admin = admin_router().route_layer(signed.clone()); + let v1 = Router::new() .nest("/packs", packs) .nest("/authors", authors) .nest("/handles", handles_router()) .nest("/telemetry", telemetry_router()) - .nest("/memory", memory_router()); + .nest("/memory", memory_router()) + .nest("/admin", admin); let x_request_id = axum::http::HeaderName::from_static("x-request-id"); diff --git a/crates/frameshift-server/src/routes/admin.rs b/crates/frameshift-server/src/routes/admin.rs new file mode 100644 index 0000000..c11ce99 --- /dev/null +++ b/crates/frameshift-server/src/routes/admin.rs @@ -0,0 +1,153 @@ +//! Administrative endpoints under `/v1/admin`. +//! +//! # Endpoints +//! +//! | Method | Path | Handler | +//! |---|---|---| +//! | POST | `/v1/admin/packs/{name}/{version}/tombstone` | [`tombstone_pack_route`] | +//! +//! # Authentication and authorization +//! +//! Every route in this module carries the Ed25519 signed-request layer +//! ([`crate::middleware::auth::require_signed_request`]), wired in +//! [`crate::router::app`] exactly like the other mutating routers. That layer +//! proves the request was produced by the holder of *some* Ed25519 key, but it +//! does not by itself grant admin authority -- +//! [`CatalogBackend`](frameshift_catalog::CatalogBackend) deliberately does +//! not know about callers, so this module is the only place that enforces +//! the admin allowlist: +//! +//! - `state.config.admin_pubkeys` empty -- the admin surface is administratively +//! disabled. Every request (even a validly signed one) gets a plain `404`, the +//! same status an unmapped path would produce, so the route's existence is +//! never disclosed while the allowlist is empty. +//! - Allowlist non-empty but the verified signer is not a member -- `403` with a +//! fixed, generic body. The allowlist contents are never echoed anywhere. +//! - Allowlist non-empty and the verified signer is a member -- the request +//! proceeds to the catalog call. + +use axum::extract::{Path, State}; +use axum::http::StatusCode; +use axum::response::{IntoResponse, Response}; +use axum::routing::post; +use axum::{Extension, Json, Router}; +use chrono::Utc; +use frameshift_catalog::{TombstoneReason, TombstoneRecord}; +use serde::{Deserialize, Serialize}; + +use crate::auth::VerifiedSigner; +use crate::error::AppError; +use crate::routes::packs::{validate_pack_name, validate_pack_version}; +use crate::state::AppState; + +/// Build the admin sub-router, mounted at `/v1/admin`. +/// +/// Returned without any auth layer; [`crate::router::app`] applies the +/// signed-request `route_layer` before nesting this router, mirroring +/// [`crate::routes::authors::authors_write_router`]. +/// +/// Routes: +/// - `POST /packs/{name}/{version}/tombstone` -> [`tombstone_pack_route`] +pub fn admin_router() -> Router { + Router::new().route( + "/packs/{name}/{version}/tombstone", + post(tombstone_pack_route), + ) +} + +/// Request body for `POST /v1/admin/packs/{name}/{version}/tombstone`. +#[derive(Debug, Deserialize)] +pub struct TombstoneRequest { + /// Why this version is being taken down. Serialized/deserialized as one of + /// `"author-request"`, `"tos-violation"`, `"dmca"` (see + /// [`TombstoneReason`]). + pub reason: TombstoneReason, +} + +/// Response body for a successful tombstone. +#[derive(Debug, Serialize)] +pub struct TombstoneResponse { + /// The pack name that was tombstoned. + pub name: String, + /// The version string that was tombstoned. + pub version: String, + /// Always the fixed string `"tombstoned"` on success. + pub status: String, +} + +/// `POST /v1/admin/packs/{name}/{version}/tombstone` +/// +/// Mark a pack version as tombstoned (removed from public availability). This +/// is a one-way transition (`Active` -> `Tombstone`); see +/// [`frameshift_catalog::CatalogBackend::tombstone_pack`] for the trait-level +/// contract. Re-tombstoning an already-tombstoned version is accepted as +/// idempotent (last-writer-wins on `reason`/`recorded_at`), matching the +/// Postgres adapter's documented choice -- this endpoint never surfaces a +/// `409` from the tombstone operation itself. +/// +/// # Authorization +/// +/// The signed-request middleware has already verified the caller controls +/// `signer.pubkey` by the time this handler runs. This handler additionally +/// checks that key against `state.config.admin_pubkeys` -- see the module +/// documentation for the exact disable/forbid/allow semantics. +/// +/// # Response +/// +/// `200 OK` with body [`TombstoneResponse`]. +/// +/// # Errors +/// +/// - `404 Not Found` -- the admin allowlist is empty (endpoint disabled), or +/// the pack version does not exist. +/// - `403 Forbidden` -- the verified signer is not on the admin allowlist. +/// - `400 Bad Request` -- `name`/`version` fail path validation. +/// - `500 Internal Server Error` -- catalog backend failure. +pub async fn tombstone_pack_route( + State(state): State, + Extension(signer): Extension, + Path((name, version)): Path<(String, String)>, + Json(body): Json, +) -> Result { + validate_pack_name(&name)?; + validate_pack_version(&version)?; + + // Disabled surface: an empty allowlist must be indistinguishable from a + // route that does not exist, so this check comes before anything else and + // always returns 404, never 403. + if state.config.admin_pubkeys.is_empty() { + return Err(AppError::NotFound("not found".to_string())); + } + + // Compare in the same base64url-no-pad string representation the config + // was parsed into; `VerifiedSigner::pubkey`'s `Display` impl produces the + // identical encoding. + let signer_b64 = signer.pubkey.to_string(); + if !state.config.admin_pubkeys.iter().any(|k| k == &signer_b64) { + tracing::warn!( + signer = %signer.pubkey, + "tombstone attempt by a verified key that is not on the admin allowlist" + ); + return Err(AppError::Forbidden( + "signer is not an authorized admin".to_string(), + )); + } + + let record = TombstoneRecord { + reason: body.reason, + recorded_at: Utc::now(), + }; + + state + .catalog + .tombstone_pack(&name, &version, record) + .await + .map_err(|e| AppError::from_catalog(e, "pack_version"))?; + + let resp = TombstoneResponse { + name, + version, + status: "tombstoned".to_string(), + }; + Ok((StatusCode::OK, Json(resp)).into_response()) +} diff --git a/crates/frameshift-server/src/routes/mod.rs b/crates/frameshift-server/src/routes/mod.rs index 204599a..7f41efb 100644 --- a/crates/frameshift-server/src/routes/mod.rs +++ b/crates/frameshift-server/src/routes/mod.rs @@ -9,7 +9,10 @@ //! - [`ops`] -- `GET /healthz` and `GET /metrics` operational endpoints. //! - [`telemetry`] -- `POST /v1/telemetry/selection` opt-in selection telemetry sink. //! - [`memory`] -- `GET /v1/memory/health` read-only memory backend health. +//! - [`admin`] -- `POST /v1/admin/packs/{name}/{version}/tombstone` and other +//! allowlist-gated operator endpoints. +pub mod admin; pub mod authors; pub mod downloads; pub mod handles; diff --git a/crates/frameshift-server/tests/admin_tombstone.rs b/crates/frameshift-server/tests/admin_tombstone.rs new file mode 100644 index 0000000..c1c28b0 --- /dev/null +++ b/crates/frameshift-server/tests/admin_tombstone.rs @@ -0,0 +1,297 @@ +//! Integration tests for the signed-request, allowlist-gated admin route +//! `POST /v1/admin/packs/{name}/{version}/tombstone`. +//! +//! Every request is driven through the real router +//! ([`frameshift_server::app`]) via `tower::ServiceExt::oneshot`, exactly like +//! the other integration test files. All catalog interaction goes through the +//! in-memory [`MockCatalog`], whose `tombstone_pack` mirrors the Postgres +//! adapter's documented idempotent (last-writer-wins) semantics. +//! +//! # Coverage +//! +//! - unsigned request -> `401` (signed-request middleware rejects before the +//! handler's allowlist check ever runs) +//! - signed by a key NOT on the allowlist -> `403` +//! - signed by a key ON the allowlist -> `200`, catalog state updated +//! - unknown pack/version -> `404` +//! - empty allowlist (feature disabled) -> `404` even with a valid signature +//! - repeat tombstone of an already-tombstoned version -> `200` (idempotent, +//! matching the Postgres adapter's documented last-writer-wins choice) + +mod mocks; + +use std::path::PathBuf; +use std::sync::Arc; +use std::time::Duration; + +use axum::body::Body; +use axum::http::{Request, StatusCode}; +use chrono::Utc; +use ed25519_dalek::SigningKey; +use http_body_util::BodyExt as _; +use secrecy::SecretString; +use tower::ServiceExt as _; + +use frameshift_catalog::identity::Ed25519PublicKey; +use frameshift_catalog::records::PackVersionRecord; +use frameshift_catalog::status::PackStatus; +use frameshift_objects::ObjectHash; + +use frameshift_server::metrics::Metrics; +use frameshift_server::{app, AppState, LogFormat, ServerConfig}; + +use mocks::catalog::MockCatalog; +use mocks::objects::MockPackStore; + +/// Build a minimal [`ServerConfig`] for these tests, with `admin_pubkeys` set +/// to `admins`. Pass an empty `Vec` to exercise the disabled-endpoint path. +fn test_config(admins: Vec) -> Arc { + Arc::new(ServerConfig { + bind_addr: "127.0.0.1:0".parse().unwrap(), + postgres_url: SecretString::new("postgres://test".into()), + object_store_root: PathBuf::from("/tmp"), + log_level: "off".into(), + log_format: LogFormat::Text, + max_request_bytes: 1_048_576, + max_search_limit: 100, + trust_forwarded_for: false, + signed_request_max_skew: Duration::from_secs(300), + admin_pubkeys: admins, + shutdown_grace: Duration::from_secs(1), + cors_allowed_origins: String::new(), + download_secret: SecretString::new(String::new()), + download_token_ttl: Duration::from_secs(300), + download_max_token_ttl: Duration::from_secs(1800), + download_rate_per_min: 0, + object_store_backend: "fs".to_string(), + r2_endpoint: String::new(), + r2_bucket: String::new(), + r2_prefix: "objects".to_string(), + r2_region: "auto".to_string(), + r2_access_key_id: String::new(), + r2_secret_access_key: SecretString::new(String::new()), + memory_backend: "none".to_string(), + memory_http_endpoint: String::new(), + memory_http_auth: "none".to_string(), + memory_http_timeout_secs: 30, + memory_sqlite_path: String::new(), + }) +} + +/// Build an [`AppState`] backed by `catalog`, with the admin allowlist set to +/// `admins`. +fn mk_state(catalog: MockCatalog, admins: Vec) -> AppState { + AppState { + catalog: Arc::new(catalog), + objects: Arc::new(MockPackStore::new()), + runtime: None, + memory: None, + config: test_config(admins), + metrics: Arc::new(Metrics::new()), + auth_nonces: Arc::new(frameshift_server::auth::NonceCache::new( + Duration::from_secs(600), + )), + } +} + +/// base64url-no-pad encoding of a signing key's public key -- the same +/// representation `admin_pubkeys` entries and `VerifiedSigner::pubkey`'s +/// `Display` impl use. +fn pubkey_b64(key: &SigningKey) -> String { + Ed25519PublicKey(key.verifying_key().to_bytes()).to_string() +} + +/// Insert a minimal, `Active` [`PackVersionRecord`] for `(name, version)` +/// directly into `catalog`'s in-memory state, bypassing the publish flow. +fn seed_active_version(catalog: &MockCatalog, name: &str, version: &str) { + let mut state = catalog.state.write().unwrap(); + let record = PackVersionRecord { + pack_name: name.to_string(), + version: version.to_string(), + content_hash: ObjectHash::of(b"test-pack-bytes"), + signature: vec![0u8; 64], + author_pubkey: Ed25519PublicKey([7u8; 32]), + parent_hash: None, + capability_manifest_json: "{}".to_string(), + schema_version: 1, + license: "MIT".to_string(), + published_at: Utc::now(), + status: PackStatus::Active, + size_bytes: 16, + }; + state + .versions + .insert((name.to_string(), version.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( + state: AppState, + path: &str, + json: serde_json::Value, + key: Option<&SigningKey>, +) -> axum::http::Response { + let body = serde_json::to_vec(&json).unwrap(); + let mut req = Request::builder() + .method("POST") + .uri(path) + .header("content-type", "application/json"); + if let Some(k) = key { + for h in mocks::signing::signed_headers(k, "POST", path, &body) { + req = req.header(h.name, h.value); + } + } + let req = req.body(Body::from(body)).unwrap(); + app(state).oneshot(req).await.unwrap() +} + +/// The standard tombstone request body used across these tests. +fn tombstone_body() -> serde_json::Value { + serde_json::json!({ "reason": "author-request" }) +} + +/// An unsigned request is rejected by the signed-request middleware before +/// the handler's allowlist check ever runs. +#[tokio::test] +async fn unsigned_request_returns_401() { + let admin = SigningKey::from_bytes(&[50u8; 32]); + let catalog = MockCatalog::new(); + seed_active_version(&catalog, "my-pack", "1.0.0"); + let state = mk_state(catalog, vec![pubkey_b64(&admin)]); + + let resp = post_signed_json( + state, + "/v1/admin/packs/my-pack/1.0.0/tombstone", + tombstone_body(), + None, + ) + .await; + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); +} + +/// A correctly signed request from a key that is NOT on the allowlist -> 403. +#[tokio::test] +async fn non_admin_signer_returns_403() { + let admin = SigningKey::from_bytes(&[51u8; 32]); + let non_admin = SigningKey::from_bytes(&[52u8; 32]); + let catalog = MockCatalog::new(); + seed_active_version(&catalog, "my-pack", "1.0.0"); + let state = mk_state(catalog, vec![pubkey_b64(&admin)]); + + let resp = post_signed_json( + state, + "/v1/admin/packs/my-pack/1.0.0/tombstone", + tombstone_body(), + Some(&non_admin), + ) + .await; + assert_eq!(resp.status(), StatusCode::FORBIDDEN); +} + +/// A correctly signed request from an admin key tombstones the version and +/// returns 200 with the expected body shape. +#[tokio::test] +async fn admin_signer_returns_200_and_tombstones() { + let admin = SigningKey::from_bytes(&[53u8; 32]); + let catalog = MockCatalog::new(); + seed_active_version(&catalog, "my-pack", "1.0.0"); + let state = mk_state(catalog.clone(), vec![pubkey_b64(&admin)]); + + let resp = post_signed_json( + state, + "/v1/admin/packs/my-pack/1.0.0/tombstone", + tombstone_body(), + Some(&admin), + ) + .await; + assert_eq!(resp.status(), StatusCode::OK, "admin tombstone should 200"); + + let body = resp.into_body().collect().await.unwrap().to_bytes(); + let json: serde_json::Value = serde_json::from_slice(&body).unwrap(); + assert_eq!(json["name"], "my-pack"); + assert_eq!(json["version"], "1.0.0"); + assert_eq!(json["status"], "tombstoned"); + + let s = catalog.state.read().unwrap(); + let record = s + .versions + .get(&("my-pack".to_string(), "1.0.0".to_string())) + .expect("version must still exist"); + assert!( + matches!(record.status, PackStatus::Tombstone { .. }), + "version status must transition to Tombstone" + ); +} + +/// Tombstoning an unknown pack/version -> 404. +#[tokio::test] +async fn unknown_version_returns_404() { + let admin = SigningKey::from_bytes(&[54u8; 32]); + let catalog = MockCatalog::new(); + let state = mk_state(catalog, vec![pubkey_b64(&admin)]); + + let resp = post_signed_json( + state, + "/v1/admin/packs/no-such-pack/9.9.9/tombstone", + tombstone_body(), + Some(&admin), + ) + .await; + assert_eq!(resp.status(), StatusCode::NOT_FOUND); +} + +/// An empty admin allowlist disables the endpoint entirely -> 404, even for a +/// validly signed request and an existing version. This must NOT be 403: an +/// empty allowlist means the feature is off, indistinguishable from an +/// unmapped route. +#[tokio::test] +async fn empty_allowlist_returns_404_even_with_valid_signature() { + let signer = SigningKey::from_bytes(&[55u8; 32]); + let catalog = MockCatalog::new(); + seed_active_version(&catalog, "my-pack", "1.0.0"); + let state = mk_state(catalog, vec![]); + + let resp = post_signed_json( + state, + "/v1/admin/packs/my-pack/1.0.0/tombstone", + tombstone_body(), + Some(&signer), + ) + .await; + assert_eq!(resp.status(), StatusCode::NOT_FOUND); +} + +/// Repeat-tombstoning an already-tombstoned version is idempotent (200), not +/// a conflict -- matching the Postgres adapter's documented last-writer-wins +/// choice (`crates/frameshift-catalog-postgres/src/backend.rs`). +#[tokio::test] +async fn repeat_tombstone_is_idempotent_200() { + let admin = SigningKey::from_bytes(&[56u8; 32]); + let catalog = MockCatalog::new(); + seed_active_version(&catalog, "my-pack", "1.0.0"); + let state = mk_state(catalog.clone(), vec![pubkey_b64(&admin)]); + + let r1 = post_signed_json( + state, + "/v1/admin/packs/my-pack/1.0.0/tombstone", + tombstone_body(), + Some(&admin), + ) + .await; + assert_eq!(r1.status(), StatusCode::OK); + + let state2 = mk_state(catalog, vec![pubkey_b64(&admin)]); + let r2 = post_signed_json( + state2, + "/v1/admin/packs/my-pack/1.0.0/tombstone", + serde_json::json!({ "reason": "tos-violation" }), + Some(&admin), + ) + .await; + assert_eq!( + r2.status(), + StatusCode::OK, + "re-tombstoning must be idempotent, not a conflict" + ); +} diff --git a/crates/frameshift-server/tests/authors_write.rs b/crates/frameshift-server/tests/authors_write.rs index ff88ac4..86ba21e 100644 --- a/crates/frameshift-server/tests/authors_write.rs +++ b/crates/frameshift-server/tests/authors_write.rs @@ -39,6 +39,7 @@ fn test_config() -> Arc { max_search_limit: 100, trust_forwarded_for: false, signed_request_max_skew: Duration::from_secs(300), + admin_pubkeys: Vec::new(), shutdown_grace: Duration::from_secs(1), cors_allowed_origins: String::new(), download_secret: SecretString::new(String::new()), diff --git a/crates/frameshift-server/tests/integration.rs b/crates/frameshift-server/tests/integration.rs index 501dd18..7f8d4fd 100644 --- a/crates/frameshift-server/tests/integration.rs +++ b/crates/frameshift-server/tests/integration.rs @@ -77,6 +77,7 @@ fn test_config() -> Arc { r2_secret_access_key: SecretString::new(String::new()), trust_forwarded_for: false, signed_request_max_skew: Duration::from_secs(300), + admin_pubkeys: Vec::new(), memory_backend: "none".to_string(), memory_http_endpoint: String::new(), memory_http_auth: "none".to_string(), @@ -939,6 +940,7 @@ fn dl_state_with_rate(catalog: MockCatalog, objects: MockPackStore, rate: u32) - // false (peer-IP only); this test opts in deliberately. trust_forwarded_for: true, signed_request_max_skew: Duration::from_secs(300), + admin_pubkeys: Vec::new(), memory_backend: "none".to_string(), memory_http_endpoint: String::new(), memory_http_auth: "none".to_string(), diff --git a/crates/frameshift-server/tests/mocks/catalog.rs b/crates/frameshift-server/tests/mocks/catalog.rs index e64a781..b71f013 100644 --- a/crates/frameshift-server/tests/mocks/catalog.rs +++ b/crates/frameshift-server/tests/mocks/catalog.rs @@ -21,7 +21,7 @@ use frameshift_catalog::error::{CatalogError, HealthStatus}; use frameshift_catalog::filters::{PackSearchFilters, PackSearchResult}; use frameshift_catalog::identity::Ed25519PublicKey; use frameshift_catalog::records::{AuthorRecord, PackRecord, PackVersionRecord}; -use frameshift_catalog::status::TombstoneRecord; +use frameshift_catalog::status::{PackStatus, TombstoneRecord}; /// Shared mutable state for [`MockCatalog`]. /// @@ -293,14 +293,36 @@ impl CatalogBackend for MockCatalog { Ok(*count) } - /// Tombstone a pack version (no-op in mock). + /// Tombstone a pack version, mirroring the Postgres adapter's documented + /// choice (`crates/frameshift-catalog-postgres/src/backend.rs`): + /// re-tombstoning an already-tombstoned version is idempotent + /// (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. async fn tombstone_pack( &self, - _name: &str, - _version: &str, - _record: TombstoneRecord, + name: &str, + version: &str, + record: TombstoneRecord, ) -> Result<(), CatalogError> { - Ok(()) + let mut state = self + .state + .write() + .map_err(|e| CatalogError::BackendError(e.to_string().into()))?; + let key = (name.to_string(), version.to_string()); + match state.versions.get_mut(&key) { + Some(v) => { + v.status = PackStatus::Tombstone { + reason: record.reason, + recorded_at: record.recorded_at, + }; + Ok(()) + } + None => Err(CatalogError::NotFound { + kind: "pack_version", + key: format!("{name}@{version}"), + }), + } } /// Get the public key for a handle. diff --git a/crates/frameshift-server/tests/publish.rs b/crates/frameshift-server/tests/publish.rs index 4e0cbea..4ab20bd 100644 --- a/crates/frameshift-server/tests/publish.rs +++ b/crates/frameshift-server/tests/publish.rs @@ -56,6 +56,7 @@ fn test_config() -> Arc { max_search_limit: 100, trust_forwarded_for: false, signed_request_max_skew: Duration::from_secs(300), + admin_pubkeys: Vec::new(), shutdown_grace: Duration::from_secs(1), cors_allowed_origins: String::new(), download_secret: SecretString::new(String::new()), diff --git a/crates/frameshift-server/tests/telemetry.rs b/crates/frameshift-server/tests/telemetry.rs index 2572929..f5b95e5 100644 --- a/crates/frameshift-server/tests/telemetry.rs +++ b/crates/frameshift-server/tests/telemetry.rs @@ -45,6 +45,7 @@ fn test_config() -> Arc { r2_secret_access_key: SecretString::new(String::new()), trust_forwarded_for: false, signed_request_max_skew: Duration::from_secs(300), + admin_pubkeys: Vec::new(), memory_backend: "none".to_string(), memory_http_endpoint: String::new(), memory_http_auth: "none".to_string(),