From b4a2ff551c0fbfc494758e7cc3d5a5725bb84e7b Mon Sep 17 00:00:00 2001 From: Alex Oliveira <4482374+aroff@users.noreply.github.com> Date: Tue, 21 Jul 2026 21:02:53 +0000 Subject: [PATCH 1/7] chore: collapse match guard for newer-nightly clippy (collapsible_match) Newer nightly (1.99.0) clippy flags the nested `if` inside the "version" match arm. Behavior-identical: fold the emptiness check into a match guard. Needed because the nightly bump (for libsqlite3-sys 0.38.1) tightened lints. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/validation/field_validation.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/crates/fastskill-core/src/validation/field_validation.rs b/crates/fastskill-core/src/validation/field_validation.rs index 3f0bcfe8..e0b0b5b1 100644 --- a/crates/fastskill-core/src/validation/field_validation.rs +++ b/crates/fastskill-core/src/validation/field_validation.rs @@ -47,14 +47,12 @@ pub(crate) fn validate_required_fields( ); } } - "version" => { - if skill.version.trim().is_empty() { - result = result.with_error( - "version", - "Skill version cannot be empty", - ErrorSeverity::Critical, - ); - } + "version" if skill.version.trim().is_empty() => { + result = result.with_error( + "version", + "Skill version cannot be empty", + ErrorSeverity::Critical, + ); } _ => {} } From 1aeb690a82975f3ed6abee3012786c2e657bd29d Mon Sep 17 00:00:00 2001 From: Alex Oliveira <4482374+aroff@users.noreply.github.com> Date: Tue, 21 Jul 2026 21:02:53 +0000 Subject: [PATCH 2/7] docs(adr-0005): rename core seam install(origin) -> add_from_origin `install` already means manifest-reconcile-with-dependencies here (CONTEXT.md: "install = Manifest -> skills dir"). The per-Origin single-skill op is `add` semantics -> `add_from_origin(origin, mode)`; note the update preflight gate. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/adr/0005-install-seam-and-origin-model.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/adr/0005-install-seam-and-origin-model.md b/docs/adr/0005-install-seam-and-origin-model.md index 23c7bfa9..76bcff7f 100644 --- a/docs/adr/0005-install-seam-and-origin-model.md +++ b/docs/adr/0005-install-seam-and-origin-model.md @@ -20,7 +20,7 @@ We must decide (a) how the browser write-handlers obtain install/update/reindex Introduce core service methods on `FastSkillService`: -- `install(origin, mode)` where `mode ∈ {Fresh, Update}` — resolve the `Origin` → fetch → validate → write skills dir → update Manifest + Lock → reindex-if-provider. **Install and update are one operation**, differing only in policy (see below). +- `add_from_origin(origin, mode)` where `mode ∈ {Fresh, Update}` — resolve the `Origin` → fetch → validate → write skills dir → update Manifest + Lock → reindex-if-provider. **Add and update are one operation**, differing only in policy (see below). (Named `add_from_origin`, *not* `install`: in this codebase `install` already means the manifest-reconcile-with-dependencies flow — CONTEXT.md "install = Manifest → skills dir". The per-`Origin`, single-skill operation is `add` semantics.) - a **core reindex seam** — reindex is domain logic (skills dir → Vector index) and belongs in core. Both the HTTP handlers **and** the CLI verbs (`execute_add`/`execute_update`/`execute_reindex`) call these methods. The CLI commands become thin wrappers (arg parsing + human output) over the shared core path. @@ -30,7 +30,7 @@ The genuinely CLI-flavoured concerns are **construction-time dependencies, not l - **Embedding provider** — reading the API key from env/config happens at startup (in the CLI, where config resolution belongs); the constructed provider (already a core abstraction, `OpenAIEmbeddingService`) is handed to the service. Reindex then runs iff a provider is present, else **skips silently** (ADR-0002). - **Progress reporting** — a UI concern. The HTTP path returns a structured `Result`; it does not need live progress. The CLI keeps its progress output in its wrapper. -`UpdateService` is **retained but narrowed** to what it already is: the read-only *"is anything newer?"* query (`check_updates`/`resolve_updates`) that backs `check`/`--dry-run`. The *apply* half is `install(origin, Update)`. +`UpdateService` is **retained but narrowed** to what it already is: the read-only *"is anything newer?"* query (`check_updates`/`resolve_updates`) that backs `check`/`--dry-run` (the update *preflight*). The *apply* half is `add_from_origin(origin, Update)`, gated by a `preflight(origin) → {UpToDate | Immutable | Updatable}` step. **Rejected — dependency-inversion / trait injection (core defines `SkillInstaller`/`Reindexer` traits, CLI implements and injects them):** this keeps the orchestration physically in the CLI and adds an indirection layer, but the logic (resolve → fetch → write → lock → reindex) is domain logic that has no reason to live in the CLI. Injection is warranted for the *dependencies* (the embedding provider), not for the *logic*. We inject the provider; we move the logic. From 805a7304a222ce9d75808380e878854c8d64e9ac Mon Sep 17 00:00:00 2001 From: Alex Oliveira <4482374+aroff@users.noreply.github.com> Date: Tue, 21 Jul 2026 21:02:53 +0000 Subject: [PATCH 3/7] =?UTF-8?q?docs(context):=20refine=20Origin=20glossary?= =?UTF-8?q?=20=E2=80=94=20intent-only=20+=20local-absorbs-zip?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-grill refinements: Origin is install intent (resolved facts live in the Lock); `local` covers dir or .zip; repository variant always concrete. Lists all collapsed representations. Co-Authored-By: Claude Opus 4.8 (1M context) --- CONTEXT.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CONTEXT.md b/CONTEXT.md index cfe21c20..1b7d118e 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -70,8 +70,8 @@ A configured remote (or local) source of skills, managed by `repos`. Types: `git _Avoid_: **source**, **registry** — both are deprecated command aliases now folded into `repos`; do not reintroduce them as concepts. **Origin**: -Where a single installed skill came from — both the *input* to an install and the *provenance* recorded on the installed skill (one type, one truth). Variants: `git` (url + ref + subdir), `local` (path, editable), `zip-url`, and `repository` (a *reference into* a configured **Repository**: `{repo, skill, version?}`). The `repository` variant is the only one **Version constraint** / ADR-0004 governs; `git`/`local`/`zip-url` are ref-based and versionless. `Origin` is the single canonical model — it replaces the former `SkillSource` (two colliding types), `SourceType`, and the manifest `"source"` serde tag. -_Avoid_: **source** (banned, see above); do not blur `Origin::repository` (a reference) with **Repository** (the configured place itself). +Where a single installed skill came from — the install **intent** (what the user asked for), recorded as provenance on the installed skill. Variants: `git` (url + ref + subdir), `local` (a filesystem path — directory *or* `.zip` — plus `editable`, dir-only), `zip-url` (a remote zip), and `repository` (a *reference into* a configured **Repository**: `{repo, skill, version?}`). The `repository` variant is the only one **Version constraint** / ADR-0004 governs; `git`/`local`/`zip-url` are ref-based and versionless. `Origin` is intent only: the **resolved** facts (exact commit, resolved version, checksum, timestamps) live in the **Lock**, not in `Origin`. It is the single canonical model — replacing the former `SkillSource` (two colliding types), `SourceType`, `SourceSpecificFields`, and the flat `source_*` fields on the manifest/lock/skill records. +_Avoid_: **source** (banned, see above); do not blur `Origin::repository` (a reference; always names a concrete Repository) with **Repository** (the configured place itself). ### Serving surfaces From bf5b10d0e10d21754df7ed928a98dd532fe45622 Mon Sep 17 00:00:00 2001 From: Alex Oliveira <4482374+aroff@users.noreply.github.com> Date: Tue, 21 Jul 2026 21:07:52 +0000 Subject: [PATCH 4/7] feat(core): add canonical Origin/GitRef/Resolved types (Phase 0 contract) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Origin = install intent (git|local|zip-url|repository); GitRef sum type; Resolved = lock-only resolved facts (version/commit/checksum). Internally-tagged serde with minimal "latest" forms. VersionConstraint gains Display + serde (round-trips through parse, enforcing ADR-0004 at the boundary). 7 serde tests. Not yet wired into manifest/lock/SkillDefinition — that is the next commit. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/fastskill-core/src/core/mod.rs | 4 + crates/fastskill-core/src/core/origin.rs | 184 ++++++++++++++++++++++ crates/fastskill-core/src/core/version.rs | 24 +++ 3 files changed, 212 insertions(+) create mode 100644 crates/fastskill-core/src/core/origin.rs diff --git a/crates/fastskill-core/src/core/mod.rs b/crates/fastskill-core/src/core/mod.rs index 5da73c0a..4e40a494 100644 --- a/crates/fastskill-core/src/core/mod.rs +++ b/crates/fastskill-core/src/core/mod.rs @@ -11,6 +11,7 @@ pub mod frontmatter; pub mod lock; pub mod manifest; pub mod metadata; +pub mod origin; pub mod project; pub mod project_config; pub mod reconciliation; @@ -62,6 +63,9 @@ pub use metadata::{ parse_yaml_frontmatter, MetadataService, MetadataServiceImpl, SkillFrontmatter, SkillMetadata, }; +// origin +pub use origin::{GitRef, Origin, Resolved}; + // project_config pub use project_config::{load_project_config, ProjectConfig}; diff --git a/crates/fastskill-core/src/core/origin.rs b/crates/fastskill-core/src/core/origin.rs new file mode 100644 index 00000000..f498d5c2 --- /dev/null +++ b/crates/fastskill-core/src/core/origin.rs @@ -0,0 +1,184 @@ +//! `Origin` — the single canonical model of *where an installed skill came from*. +//! +//! `Origin` captures install **intent** (what the user asked for). The **resolved** +//! facts a fetch produces — the exact commit, the concrete version, a checksum — +//! live in [`Resolved`], which is stored only in the Lock, never in `Origin`. +//! +//! This type replaces the former six overlapping provenance representations +//! (`SkillSource` ×2, `SourceType`, `SourceSpecificFields`, the flat `source_*` +//! fields on the lock entries, and the nine on `SkillDefinition`). See +//! [ADR-0005](../../../../docs/adr/0005-install-seam-and-origin-model.md) and the +//! `Origin` entry in `CONTEXT.md`. + +use crate::core::version::VersionConstraint; +use serde::{Deserialize, Serialize}; +use std::path::PathBuf; + +/// Where a single installed skill came from (install intent + persisted provenance). +/// +/// Serialized internally-tagged by `type`, so a default-branch git origin is just +/// `{"type":"git","url":"…"}` — the `ref`/`subdir`/`version` fields are omitted when +/// unset, keeping the common "install latest" case minimal. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(tag = "type", rename_all = "kebab-case")] +pub enum Origin { + /// A git repository at a ref (default branch unless pinned), optionally a subdir. + Git { + url: String, + #[serde(default, skip_serializing_if = "GitRef::is_default")] + r#ref: GitRef, + #[serde(default, skip_serializing_if = "Option::is_none")] + subdir: Option, + }, + /// A path on the local filesystem — a directory or a `.zip` archive. + /// `editable` (symlink-in-place) is only valid for a directory. + Local { + path: PathBuf, + #[serde(default, skip_serializing_if = "is_false")] + editable: bool, + }, + /// A remote zip archive fetched over HTTP(S). + ZipUrl { url: String }, + /// A reference *into* a configured [`Repository`](crate::core::manifest). `repo` + /// is the concrete Repository name; `version` is the only place ADR-0004 + /// versioning applies (`None` = newest allowed). + Repository { + repo: String, + skill: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + version: Option, + }, +} + +/// The git ref an [`Origin::Git`] points at. A sum type so illegal combinations +/// (a branch *and* a tag) are unrepresentable; `Default` means the repository's +/// default branch. +#[derive(Debug, Clone, PartialEq, Eq, Default, Serialize, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub enum GitRef { + /// The repository's default branch (whatever `HEAD` points at on clone). + #[default] + Default, + Branch(String), + Tag(String), + Commit(String), +} + +impl GitRef { + /// True for [`GitRef::Default`]; drives `skip_serializing_if` so the ref field + /// is omitted entirely for the common default-branch case. + pub fn is_default(&self) -> bool { + matches!(self, GitRef::Default) + } +} + +/// The concrete facts a fetch resolved an [`Origin`] to. Stored only in the Lock — +/// never in `Origin` — so "what was asked for" stays separate from "what it became". +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct Resolved { + /// The concrete version installed (`SKILL.md` version, or the registry version). + pub version: String, + /// The exact git commit, when the origin is a git clone. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub commit_hash: Option, + /// Content checksum, when the fetch produced one. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub checksum: Option, +} + +fn is_false(b: &bool) -> bool { + !*b +} + +#[cfg(test)] +#[allow(clippy::unwrap_used, clippy::expect_used)] +mod tests { + use super::*; + + fn roundtrip(o: &Origin) -> Origin { + let json = serde_json::to_string(o).unwrap(); + serde_json::from_str(&json).unwrap() + } + + #[test] + fn git_latest_is_minimal() { + let o = Origin::Git { + url: "https://github.com/x/y".into(), + r#ref: GitRef::Default, + subdir: None, + }; + let json = serde_json::to_string(&o).unwrap(); + assert_eq!(json, r#"{"type":"git","url":"https://github.com/x/y"}"#); + assert_eq!(roundtrip(&o), o); + } + + #[test] + fn git_branch_ref_is_subobject() { + let o = Origin::Git { + url: "u".into(), + r#ref: GitRef::Branch("main".into()), + subdir: Some(PathBuf::from("sub")), + }; + let json = serde_json::to_string(&o).unwrap(); + assert!(json.contains(r#""ref":{"branch":"main"}"#), "{json}"); + assert!(json.contains(r#""subdir":"sub""#), "{json}"); + assert_eq!(roundtrip(&o), o); + } + + #[test] + fn local_dir_omits_editable_when_false() { + let o = Origin::Local { + path: PathBuf::from("/tmp/s"), + editable: false, + }; + let json = serde_json::to_string(&o).unwrap(); + assert_eq!(json, r#"{"type":"local","path":"/tmp/s"}"#); + assert_eq!(roundtrip(&o), o); + } + + #[test] + fn zip_url_roundtrips() { + let o = Origin::ZipUrl { + url: "https://x/y.zip".into(), + }; + assert_eq!(roundtrip(&o), o); + } + + #[test] + fn repository_latest_omits_version() { + let o = Origin::Repository { + repo: "main-registry".into(), + skill: "acme/widget".into(), + version: None, + }; + let json = serde_json::to_string(&o).unwrap(); + assert_eq!( + json, + r#"{"type":"repository","repo":"main-registry","skill":"acme/widget"}"# + ); + assert_eq!(roundtrip(&o), o); + } + + #[test] + fn repository_bare_version_normalizes_to_exact_pin() { + // ADR-0004 enforced at the serde boundary via VersionConstraint. + let o = Origin::Repository { + repo: "r".into(), + skill: "s".into(), + version: Some(VersionConstraint::parse("1.2.3").unwrap()), + }; + let json = serde_json::to_string(&o).unwrap(); + assert!(json.contains(r#""version":"=1.2.3""#), "{json}"); + assert_eq!(roundtrip(&o), o); + } + + #[test] + fn resolved_omits_empty_optionals() { + let r = Resolved { + version: "1.0.0".into(), + commit_hash: None, + checksum: None, + }; + assert_eq!(serde_json::to_string(&r).unwrap(), r#"{"version":"1.0.0"}"#); + } +} diff --git a/crates/fastskill-core/src/core/version.rs b/crates/fastskill-core/src/core/version.rs index f2829119..2aba8d69 100644 --- a/crates/fastskill-core/src/core/version.rs +++ b/crates/fastskill-core/src/core/version.rs @@ -86,6 +86,30 @@ impl VersionConstraint { } } +impl std::fmt::Display for VersionConstraint { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + // Delegates to `VersionReq`'s canonical form, so a normalized bare pin + // renders as `=1.2.3` — the exact string `parse` will round-trip. + std::fmt::Display::fmt(&self.req, f) + } +} + +// Persisted form of a constraint is its canonical string, and deserialization +// funnels back through `parse` so the ADR-0004 normalization (bare version = +// exact pin) is enforced at the serde boundary, not just at construction. +impl serde::Serialize for VersionConstraint { + fn serialize(&self, serializer: S) -> Result { + serializer.serialize_str(&self.to_string()) + } +} + +impl<'de> serde::Deserialize<'de> for VersionConstraint { + fn deserialize>(deserializer: D) -> Result { + let s = ::deserialize(deserializer)?; + VersionConstraint::parse(&s).map_err(serde::de::Error::custom) + } +} + /// Compare two version strings pub fn compare_versions(v1: &str, v2: &str) -> Result { let ver1 = Version::parse(v1).map_err(|e| { From 3e95a85a25b272199b4467ea2bd75352647e5fea Mon Sep 17 00:00:00 2001 From: Alex Oliveira <4482374+aroff@users.noreply.github.com> Date: Tue, 21 Jul 2026 21:27:08 +0000 Subject: [PATCH 5/7] feat(core)!: collapse six provenance representations into Origin (Phase 0, core) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reshape fastskill-core onto the canonical Origin (intent) + lock-only Resolved: - manifest: delete SkillSource/SourceSpecificFields/DependencySource; SkillEntry {id, origin, groups}; DependencySpec = Version | Inline{origin,groups}. - skill_manager: delete SourceType; SkillDefinition/SkillUpdate carry `origin` (+ kept resolved facts commit_hash/fetched_at); SkillDefinition::new takes origin. - lock: entries carry {origin, resolved}; version bumped to "3.0" with a pre-parse guard rejecting pre-Origin locks (LockError::UnsupportedVersion); build_skill_source deleted. - consumers updated: reconciliation, service, update, http handlers, output. BREAKING: on-disk manifest/lock serde formats change (greenfield, no shim). Behavior-preserving for CLI outcomes; only new behavior is the lock old-data guard. fastskill-cli intentionally does NOT compile yet — its pass is the next commit. cargo build/test/clippy -p fastskill-core --all-targets all green (335+ tests). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/core/dependency_resolver.rs | 15 +- crates/fastskill-core/src/core/lock.rs | 198 ++++++++-------- crates/fastskill-core/src/core/manifest.rs | 219 ++++-------------- crates/fastskill-core/src/core/mod.rs | 13 +- .../fastskill-core/src/core/reconciliation.rs | 20 +- crates/fastskill-core/src/core/service.rs | 10 +- .../fastskill-core/src/core/skill_manager.rs | 68 ++---- crates/fastskill-core/src/core/update.rs | 8 +- crates/fastskill-core/src/events/event_bus.rs | 4 + .../src/http/handlers/manifest.rs | 94 ++++---- .../src/http/handlers/skills.rs | 8 +- crates/fastskill-core/src/output/mod.rs | 55 +++-- .../src/validation/skill_validator_tests.rs | 4 + .../tests/http_handler_route_tests.rs | 10 +- .../tests/integration_unified_format_test.rs | 15 +- .../tests/skill_project_toml_test.rs | 32 ++- 16 files changed, 314 insertions(+), 459 deletions(-) diff --git a/crates/fastskill-core/src/core/dependency_resolver.rs b/crates/fastskill-core/src/core/dependency_resolver.rs index 93e51909..7c05a333 100644 --- a/crates/fastskill-core/src/core/dependency_resolver.rs +++ b/crates/fastskill-core/src/core/dependency_resolver.rs @@ -199,20 +199,19 @@ impl DependencyResolver { #[allow(clippy::unwrap_used)] mod tests { use super::*; - use crate::core::manifest::{SkillEntry, SkillSource}; + use crate::core::manifest::SkillEntry; + use crate::core::origin::Origin; use std::path::PathBuf; use tempfile::TempDir; fn make_local_entry(id: &str, path: &str) -> SkillEntry { SkillEntry { id: id.to_string(), - source: SkillSource::Local { + origin: Origin::Local { path: PathBuf::from(path), editable: false, }, - version: "*".to_string(), groups: vec![], - editable: false, } } @@ -262,7 +261,7 @@ mod tests { std::fs::write( skill_a_dir.join("skill-project.toml"), r#"[dependencies] -skill-b = { source = "local", path = "local/skill-b" } +skill-b = { origin = { type = "local", path = "local/skill-b" } } "#, ) .unwrap(); @@ -292,7 +291,7 @@ skill-b = { source = "local", path = "local/skill-b" } std::fs::write( skill_a_dir.join("skill-project.toml"), r#"[dependencies] -skill-b = { source = "local", path = "local/skill-b" } +skill-b = { origin = { type = "local", path = "local/skill-b" } } "#, ) .unwrap(); @@ -302,7 +301,7 @@ skill-b = { source = "local", path = "local/skill-b" } std::fs::write( skill_b_dir.join("skill-project.toml"), r#"[dependencies] -skill-c = { source = "local", path = "local/skill-c" } +skill-c = { origin = { type = "local", path = "local/skill-c" } } "#, ) .unwrap(); @@ -354,7 +353,7 @@ skill-c = { source = "local", path = "local/skill-c" } let mut body = String::from("[dependencies]\n"); for dep in deps { body.push_str(&format!( - "{dep} = {{ source = \"local\", path = \"local/{dep}\" }}\n" + "{dep} = {{ origin = {{ type = \"local\", path = \"local/{dep}\" }} }}\n" )); } std::fs::write(skill_dir.join("skill-project.toml"), body).unwrap(); diff --git a/crates/fastskill-core/src/core/lock.rs b/crates/fastskill-core/src/core/lock.rs index d06f6a9b..07bc5ab5 100644 --- a/crates/fastskill-core/src/core/lock.rs +++ b/crates/fastskill-core/src/core/lock.rs @@ -4,12 +4,17 @@ //! - `ProjectSkillsLock`: deterministic, timestamp-free, for `skills.lock` at project root //! - `GlobalSkillsLock`: operational, with timestamps, for `global-skills.lock` in user config dir -use crate::core::manifest::SkillSource; +use crate::core::origin::{Origin, Resolved}; use crate::core::skill_manager::SkillDefinition; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use std::path::{Path, PathBuf}; +/// The lock format version. Bumped to "3.0" for the `Origin`/`Resolved` reshape +/// (Phase 0). Lock files from before this version cannot be read — see +/// [`LockError::UnsupportedVersion`]. +pub const LOCK_FORMAT_VERSION: &str = "3.0"; + // ── Project Lock ───────────────────────────────────────────────────────────── /// Metadata for the project-scoped lock file. @@ -27,24 +32,12 @@ pub struct ProjectLockMetadata { pub struct ProjectLockedSkillEntry { pub id: String, pub name: String, - pub version: String, - pub source: SkillSource, - #[serde(default)] - pub source_name: Option, - #[serde(default)] - pub source_url: Option, - #[serde(default)] - pub source_branch: Option, - #[serde(default)] - pub commit_hash: Option, - #[serde(default)] - pub checksum: Option, + pub origin: Origin, + pub resolved: Resolved, #[serde(default)] pub dependencies: Vec, #[serde(default)] pub groups: Vec, - #[serde(default)] - pub editable: bool, /// Depth in the dependency tree (0 = direct dependency) #[serde(default)] pub depth: u32, @@ -65,7 +58,7 @@ impl ProjectSkillsLock { pub fn new_empty() -> Self { Self { metadata: ProjectLockMetadata { - version: "2.0".to_string(), + version: LOCK_FORMAT_VERSION.to_string(), fastskill_version: Some(env!("CARGO_PKG_VERSION").to_string()), }, skills: Vec::new(), @@ -78,6 +71,7 @@ impl ProjectSkillsLock { } let safe_path = path.canonicalize().map_err(LockError::Io)?; let content = std::fs::read_to_string(&safe_path).map_err(LockError::Io)?; + check_lock_format_version(&content)?; let lock: ProjectSkillsLock = toml::from_str(&content).map_err(|e| LockError::Parse(e.to_string()))?; Ok(lock) @@ -112,20 +106,17 @@ impl ProjectSkillsLock { parent_skill: Option, ) { self.skills.retain(|s| s.id != skill.id.as_str()); - let source = build_skill_source(skill); let entry = ProjectLockedSkillEntry { id: skill.id.to_string(), name: skill.name.clone(), - version: skill.version.clone(), - source, - source_name: skill.installed_from.clone(), - source_url: skill.source_url.clone(), - source_branch: skill.source_branch.clone(), - commit_hash: skill.commit_hash.clone(), - checksum: None, + origin: skill.origin.clone(), + resolved: Resolved { + version: skill.version.clone(), + commit_hash: skill.commit_hash.clone(), + checksum: None, + }, dependencies: skill.dependencies.clone().unwrap_or_default(), groups: Vec::new(), - editable: skill.editable, depth, parent_skill, }; @@ -145,17 +136,17 @@ impl ProjectSkillsLock { let mut mismatches = Vec::new(); for locked in &self.skills { if let Some(installed) = installed_skills.iter().find(|s| s.id.as_str() == locked.id) { - if installed.version != locked.version { + if installed.version != locked.resolved.version { mismatches.push(LockMismatch { skill_id: locked.id.clone(), reason: format!( "Version mismatch: lock={}, installed={}", - locked.version, installed.version + locked.resolved.version, installed.version ), }); } if let (Some(lock_commit), Some(inst_commit)) = - (&locked.commit_hash, &installed.commit_hash) + (&locked.resolved.commit_hash, &installed.commit_hash) { if lock_commit != inst_commit { mismatches.push(LockMismatch { @@ -205,18 +196,8 @@ pub struct GlobalLockMetadata { pub struct GlobalLockedSkillEntry { pub id: String, pub name: String, - pub version: String, - pub source: SkillSource, - #[serde(default)] - pub source_name: Option, - #[serde(default)] - pub source_url: Option, - #[serde(default)] - pub source_branch: Option, - #[serde(default)] - pub commit_hash: Option, - #[serde(default)] - pub checksum: Option, + pub origin: Origin, + pub resolved: Resolved, #[serde(default)] pub dependencies: Vec, #[serde(default)] @@ -241,7 +222,7 @@ impl GlobalSkillsLock { pub fn new_empty() -> Self { Self { metadata: GlobalLockMetadata { - version: "1.0".to_string(), + version: LOCK_FORMAT_VERSION.to_string(), fastskill_version: Some(env!("CARGO_PKG_VERSION").to_string()), }, skills: Vec::new(), @@ -259,6 +240,7 @@ impl GlobalSkillsLock { } let safe_path = path.canonicalize().map_err(LockError::Io)?; let content = std::fs::read_to_string(&safe_path).map_err(LockError::Io)?; + check_lock_format_version(&content)?; let lock: GlobalSkillsLock = toml::from_str(&content).map_err(|e| LockError::Parse(e.to_string()))?; Ok(lock) @@ -276,17 +258,15 @@ impl GlobalSkillsLock { pub fn upsert_skill(&mut self, skill: &SkillDefinition, installed_at: DateTime) { self.skills.retain(|s| s.id != skill.id.as_str()); - let source = build_skill_source(skill); let entry = GlobalLockedSkillEntry { id: skill.id.to_string(), name: skill.name.clone(), - version: skill.version.clone(), - source, - source_name: skill.installed_from.clone(), - source_url: skill.source_url.clone(), - source_branch: skill.source_branch.clone(), - commit_hash: skill.commit_hash.clone(), - checksum: None, + origin: skill.origin.clone(), + resolved: Resolved { + version: skill.version.clone(), + commit_hash: skill.commit_hash.clone(), + checksum: None, + }, dependencies: skill.dependencies.clone().unwrap_or_default(), groups: Vec::new(), installed_at, @@ -350,6 +330,39 @@ pub enum LockError { /// Cannot determine global config directory. #[error("Global config directory unavailable: {0}")] GlobalConfigUnavailable(String), + + /// The lock file predates the `Origin` model (format version < 3.0). There is + /// no migrator — the caller must delete the lock and reinstall. + #[error( + "skills lock format {found} predates the Origin model (3.0); \ + delete the lock file and re-run `fastskill install`" + )] + UnsupportedVersion { found: String }, +} + +/// Lightweight pre-check: read only `metadata.version` out of the raw TOML text, +/// before attempting to deserialize the full lock structure. A pre-Origin lock +/// file's `[[skills]]` entries won't match the current shape at all (e.g. a +/// missing `origin` field), which would otherwise surface as an opaque +/// `LockError::Parse` instead of the actionable `UnsupportedVersion` guard. +fn check_lock_format_version(content: &str) -> Result<(), LockError> { + #[derive(Deserialize)] + struct VersionOnly { + version: String, + } + #[derive(Deserialize)] + struct MetadataOnly { + metadata: VersionOnly, + } + + let parsed: MetadataOnly = + toml::from_str(content).map_err(|e| LockError::Parse(e.to_string()))?; + if parsed.metadata.version != LOCK_FORMAT_VERSION { + return Err(LockError::UnsupportedVersion { + found: parsed.metadata.version, + }); + } + Ok(()) } // ── Routing helpers ─────────────────────────────────────────────────────────── @@ -374,49 +387,12 @@ pub fn global_lock_path() -> Result { }) } -// ── Shared source builder ───────────────────────────────────────────────────── - -fn build_skill_source(skill: &SkillDefinition) -> SkillSource { - if let Some(source_type) = &skill.source_type { - match source_type { - crate::core::skill_manager::SourceType::GitUrl => SkillSource::Git { - url: skill.source_url.clone().unwrap_or_default(), - branch: skill.source_branch.clone(), - tag: skill.source_tag.clone(), - subdir: skill.source_subdir.clone(), - }, - crate::core::skill_manager::SourceType::LocalPath => SkillSource::Local { - path: skill.source_subdir.clone().unwrap_or_else(|| { - std::path::PathBuf::from(skill.source_url.clone().unwrap_or_default()) - }), - editable: skill.editable, - }, - crate::core::skill_manager::SourceType::ZipFile => SkillSource::ZipUrl { - base_url: skill.source_url.clone().unwrap_or_default(), - version: Some(skill.version.clone()), - }, - crate::core::skill_manager::SourceType::Source => SkillSource::Source { - name: skill.installed_from.clone().unwrap_or_default(), - skill: skill.id.to_string(), - version: Some(skill.version.clone()), - }, - } - } else { - SkillSource::Git { - url: skill.source_url.clone().unwrap_or_default(), - branch: None, - tag: None, - subdir: None, - } - } -} - #[cfg(test)] -#[allow(clippy::unwrap_used, clippy::expect_used)] +#[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] mod tests { use super::*; use crate::core::service::SkillId; - use crate::core::skill_manager::{SkillDefinition, SourceType}; + use crate::core::skill_manager::SkillDefinition; use chrono::Utc; use tempfile::TempDir; @@ -436,15 +412,13 @@ mod tests { execution_environment: None, dependencies: None, timeout: None, - source_url: Some("https://github.com/test/repo.git".to_string()), - source_type: Some(SourceType::GitUrl), - source_branch: Some("main".to_string()), - source_tag: None, - source_subdir: None, - installed_from: None, + origin: crate::core::origin::Origin::Git { + url: "https://github.com/test/repo.git".to_string(), + r#ref: crate::core::origin::GitRef::Branch("main".to_string()), + subdir: None, + }, commit_hash: Some("abc123".to_string()), fetched_at: Some(Utc::now()), - editable: false, } } @@ -511,10 +485,12 @@ mod tests { } #[test] - fn test_project_lock_migration_strips_volatile_fields() { - // Simulate loading a v1.0.0 skills.lock that has generated_at and fetched_at + fn test_project_lock_pre_origin_format_is_rejected() { + // A lock file from before the Origin model (any version != "3.0") must be + // rejected with an actionable error rather than silently misparsed — there + // is no migrator (spec decision). let old_format = r#"[metadata] -version = "1.0.0" +version = "2.0" generated_at = "2024-01-01T00:00:00Z" fastskill_version = "0.9.0" @@ -533,22 +509,34 @@ depth = 0 let lock_path = tmp.path().join("skills.lock"); std::fs::write(&lock_path, old_format).unwrap(); - // Load old format - unknown fields (generated_at, fetched_at) are silently ignored + let err = ProjectSkillsLock::load_from_file(&lock_path).unwrap_err(); + match err { + LockError::UnsupportedVersion { found } => assert_eq!(found, "2.0"), + other => panic!("expected UnsupportedVersion, got {other:?}"), + } + } + + #[test] + fn test_project_lock_no_volatile_fields_after_round_trip() { + let tmp = TempDir::new().unwrap(); + let lock_path = tmp.path().join("skills.lock"); + + let mut lock = ProjectSkillsLock::new_empty(); + lock.update_skill(&make_skill("my-skill")); + lock.save_to_file(&lock_path).unwrap(); + let loaded = ProjectSkillsLock::load_from_file(&lock_path).unwrap(); assert_eq!(loaded.skills.len(), 1); - assert_eq!(loaded.skills[0].id, "old-skill"); + assert_eq!(loaded.skills[0].id, "my-skill"); - // Save with new code - loaded.save_to_file(&lock_path).unwrap(); let new_content = std::fs::read_to_string(&lock_path).unwrap(); - assert!( !new_content.contains("generated_at"), - "generated_at must be stripped on save" + "generated_at must not appear" ); assert!( !new_content.contains("fetched_at"), - "fetched_at must be stripped on save" + "fetched_at must not appear" ); } diff --git a/crates/fastskill-core/src/core/manifest.rs b/crates/fastskill-core/src/core/manifest.rs index 2069b3ec..3551bda0 100644 --- a/crates/fastskill-core/src/core/manifest.rs +++ b/crates/fastskill-core/src/core/manifest.rs @@ -1,5 +1,7 @@ //! Skills manifest management for declarative skill control +use crate::core::origin::Origin; +use crate::core::version::VersionConstraint; use serde::{Deserialize, Serialize}; use std::collections::HashMap; use std::path::{Path, PathBuf}; @@ -22,47 +24,9 @@ pub struct ManifestMetadata { #[derive(Debug, Clone, Serialize, Deserialize)] pub struct SkillEntry { pub id: String, - pub source: SkillSource, - pub version: String, + pub origin: Origin, #[serde(default)] pub groups: Vec, - #[serde(default)] - pub editable: bool, -} - -/// Source specification for skills -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] -#[serde(tag = "type")] -pub enum SkillSource { - #[serde(rename = "git")] - Git { - url: String, - #[serde(default)] - branch: Option, - #[serde(default)] - tag: Option, - #[serde(default)] - subdir: Option, - }, - #[serde(rename = "source")] - Source { - name: String, - skill: String, - #[serde(default)] - version: Option, - }, - #[serde(rename = "local")] - Local { - path: PathBuf, - #[serde(default)] - editable: bool, - }, - #[serde(rename = "zip-url")] - ZipUrl { - base_url: String, - #[serde(default)] - version: Option, - }, } impl SkillsManifest { @@ -193,61 +157,20 @@ pub struct DependenciesSection { pub dependencies: HashMap, } -/// Dependency specification - can be a simple version string or inline table with source details +/// Dependency specification - can be a simple version string or inline table with origin details #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(untagged)] pub enum DependencySpec { /// Simple version string: "1.0.0" Version(String), - /// Inline table with source details + /// Inline table with an explicit `Origin` Inline { - source: DependencySource, - #[serde(flatten)] - source_specific: SourceSpecificFields, + origin: Origin, #[serde(default)] groups: Option>, - #[serde(default)] - editable: Option, }, } -/// Dependency source type -#[derive(Debug, Clone, Serialize, Deserialize)] -pub enum DependencySource { - #[serde(rename = "git")] - Git, - #[serde(rename = "local")] - Local, - #[serde(rename = "zip-url")] - ZipUrl, - #[serde(rename = "source")] - Source, -} - -/// Source-specific fields for dependency specifications -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct SourceSpecificFields { - /// For git source - #[serde(default)] - pub url: Option, - #[serde(default)] - pub branch: Option, - /// For local source - #[serde(default)] - pub path: Option, - /// For source source - #[serde(default)] - pub name: Option, - #[serde(default)] - pub skill: Option, - /// For zip-url source - #[serde(default)] - pub zip_url: Option, - /// Version (for source source) - #[serde(default)] - pub version: Option, -} - /// Tool section containing tool-specific configuration #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ToolSection { @@ -566,88 +489,31 @@ impl SkillProjectToml { if let Some(ref deps_section) = self.dependencies { for (skill_id, dep_spec) in &deps_section.dependencies { - let (source, version, groups, editable) = match dep_spec { + let (origin, groups) = match dep_spec { DependencySpec::Version(version_str) => { - // Version-only dependency - treat as source-based + // Version-only dependency: resolved against the "default" repository, + // preserving today's implicit-source behavior. + let constraint = VersionConstraint::parse(version_str).map_err(|e| { + format!("Invalid version '{}' for {}: {}", version_str, skill_id, e) + })?; ( - SkillSource::Source { - name: "default".to_string(), + Origin::Repository { + repo: "default".to_string(), skill: skill_id.clone(), - version: Some(version_str.clone()), + version: Some(constraint), }, - Some(version_str.clone()), Vec::new(), - false, ) } - DependencySpec::Inline { - source, - source_specific, - groups, - editable, - } => { - let source = match source { - DependencySource::Git => { - let url = source_specific.url.clone().ok_or_else(|| { - format!("Git source requires 'url' field for {}", skill_id) - })?; - SkillSource::Git { - url, - branch: source_specific.branch.clone(), - tag: None, - subdir: None, - } - } - DependencySource::Local => { - let path = source_specific.path.clone().ok_or_else(|| { - format!("Local source requires 'path' field for {}", skill_id) - })?; - SkillSource::Local { - path: PathBuf::from(path), - editable: editable.unwrap_or(false), - } - } - DependencySource::ZipUrl => { - let zip_url = source_specific.zip_url.clone().ok_or_else(|| { - format!( - "ZipUrl source requires 'zip_url' field for {}", - skill_id - ) - })?; - SkillSource::ZipUrl { - base_url: zip_url, - version: source_specific.version.clone(), - } - } - DependencySource::Source => { - let name = source_specific.name.clone().ok_or_else(|| { - format!("Source source requires 'name' field for {}", skill_id) - })?; - let skill = source_specific.skill.clone().ok_or_else(|| { - format!("Source source requires 'skill' field for {}", skill_id) - })?; - SkillSource::Source { - name, - skill, - version: source_specific.version.clone(), - } - } - }; - ( - source, - source_specific.version.clone(), - groups.clone().unwrap_or_default(), - editable.unwrap_or(false), - ) + DependencySpec::Inline { origin, groups } => { + (origin.clone(), groups.clone().unwrap_or_default()) } }; entries.push(SkillEntry { id: skill_id.clone(), - source, - version: version.unwrap_or_else(|| "*".to_string()), + origin, groups, - editable, }); } } @@ -737,20 +603,17 @@ mod tests { [[skills]] id = "web-scraper" - source = { type = "git", url = "https://github.com/org/repo.git", branch = "main" } - version = "*" + origin = { type = "git", url = "https://github.com/org/repo.git", ref = { branch = "main" } } [[skills]] id = "dev-tools" - source = { type = "git", url = "https://github.com/org/dev-tools.git" } + origin = { type = "git", url = "https://github.com/org/dev-tools.git" } groups = ["dev"] - version = "*" [[skills]] id = "monitoring" - source = { type = "source", name = "team-tools", skill = "monitoring", version = "2.1.0" } + origin = { type = "repository", repo = "team-tools", skill = "monitoring", version = "=2.1.0" } groups = ["prod"] - version = "2.1.0" "#; let manifest: SkillsManifest = toml::from_str(toml_content).unwrap(); @@ -772,40 +635,38 @@ mod tests { } #[test] - fn test_skill_source_variants() { - // Test Git source - let git_source = SkillSource::Git { + fn test_origin_variants_serialize_as_expected() { + // Test Git origin + let git_origin = Origin::Git { url: "https://github.com/org/repo.git".to_string(), - branch: Some("main".to_string()), - tag: None, + r#ref: crate::core::origin::GitRef::Branch("main".to_string()), subdir: None, }; - // Test Source reference - let source_ref = SkillSource::Source { - name: "team-tools".to_string(), + // Test Repository reference + let repo_origin = Origin::Repository { + repo: "team-tools".to_string(), skill: "monitoring".to_string(), - version: Some("2.1.0".to_string()), + version: Some(VersionConstraint::parse("2.1.0").unwrap()), }; - // Test Local source - let _local_source = SkillSource::Local { + // Test Local origin + let _local_origin = Origin::Local { path: PathBuf::from("./local-skills"), editable: false, }; - // Test ZipUrl source - let _zip_source = SkillSource::ZipUrl { - base_url: "https://skills.example.com/".to_string(), - version: None, + // Test ZipUrl origin + let _zip_origin = Origin::ZipUrl { + url: "https://skills.example.com/".to_string(), }; // Verify they serialize correctly - let git_toml = toml::to_string(&git_source).unwrap(); + let git_toml = toml::to_string(&git_origin).unwrap(); assert!(git_toml.contains("type = \"git\"")); - let source_toml = toml::to_string(&source_ref).unwrap(); - assert!(source_toml.contains("type = \"source\"")); + let repo_toml = toml::to_string(&repo_origin).unwrap(); + assert!(repo_toml.contains("type = \"repository\"")); } #[test] @@ -817,15 +678,13 @@ mod tests { [[skills]] id = "dual-group-skill" - source = { type = "git", url = "https://github.com/org/repo.git" } + origin = { type = "git", url = "https://github.com/org/repo.git" } groups = ["prod", "dev"] - version = "*" [[skills]] id = "only-prod-skill" - source = { type = "git", url = "https://github.com/org/repo2.git" } + origin = { type = "git", url = "https://github.com/org/repo2.git" } groups = ["prod"] - version = "*" "#; let manifest: SkillsManifest = toml::from_str(toml_content).unwrap(); diff --git a/crates/fastskill-core/src/core/mod.rs b/crates/fastskill-core/src/core/mod.rs index 4e40a494..170e1be6 100644 --- a/crates/fastskill-core/src/core/mod.rs +++ b/crates/fastskill-core/src/core/mod.rs @@ -51,11 +51,10 @@ pub use lock::{ // manifest pub use manifest::{ - AuthConfig, AuthType, DependenciesSection, DependencySource, DependencySpec, - EmbeddingConfigToml, EvalConfigToml, FastSkillToolConfig, FileResolutionResult, - HttpServerConfigToml, ManifestError, ManifestMetadata, MetadataSection, ProjectContext, - RepositoryConnection, RepositoryDefinition, RepositoryType, SkillEntry, SkillProjectToml, - SkillSource, SkillsManifest, SourceSpecificFields, ToolSection, + AuthConfig, AuthType, DependenciesSection, DependencySpec, EmbeddingConfigToml, EvalConfigToml, + FastSkillToolConfig, FileResolutionResult, HttpServerConfigToml, ManifestError, + ManifestMetadata, MetadataSection, ProjectContext, RepositoryConnection, RepositoryDefinition, + RepositoryType, SkillEntry, SkillProjectToml, SkillsManifest, ToolSection, }; // metadata @@ -99,9 +98,7 @@ pub use service::{ }; // skill_manager -pub use skill_manager::{ - SkillDefinition, SkillManagementService, SkillManager, SkillUpdate, SourceType, -}; +pub use skill_manager::{SkillDefinition, SkillManagementService, SkillManager, SkillUpdate}; // sources pub use sources::{ diff --git a/crates/fastskill-core/src/core/reconciliation.rs b/crates/fastskill-core/src/core/reconciliation.rs index c22c93ab..166add26 100644 --- a/crates/fastskill-core/src/core/reconciliation.rs +++ b/crates/fastskill-core/src/core/reconciliation.rs @@ -53,11 +53,23 @@ pub struct VersionMismatch { pub locked_version: String, } +use crate::core::origin::Origin; use crate::core::skill_manager::SkillDefinition; use crate::core::version::VersionConstraint; use std::collections::HashMap; use std::path::Path; +/// A short human-readable description of where a skill came from, derived from +/// its `Origin`. Used only for display in the reconciliation report. +fn origin_display(origin: &Origin) -> String { + match origin { + Origin::Git { url, .. } => url.clone(), + Origin::Local { path, .. } => path.display().to_string(), + Origin::ZipUrl { url } => url.clone(), + Origin::Repository { repo, skill, .. } => format!("{repo}/{skill}"), + } +} + /// Build reconciliation report pub fn build_reconciliation_report( installed_skills: &[SkillDefinition], @@ -136,7 +148,7 @@ pub fn build_reconciliation_report( name: skill.name.clone(), version: skill.version.clone(), description: skill.description.clone(), - source: skill.source_url.clone(), + source: Some(origin_display(&skill.origin)), installed_path: skill.skill_file.clone(), installed_at: Some(skill.updated_at), status: status.clone(), @@ -160,7 +172,7 @@ pub fn build_reconciliation_report( name: skill.name.clone(), version: skill.version.clone(), description: skill.description.clone(), - source: skill.source_url.clone(), + source: Some(origin_display(&skill.origin)), installed_path: skill.skill_file.clone(), installed_at: Some(skill.updated_at), status, @@ -188,6 +200,10 @@ mod tests { id.to_string(), "desc".to_string(), version.to_string(), + Origin::Local { + path: std::path::PathBuf::from(format!("./skills/{id}")), + editable: false, + }, ) } diff --git a/crates/fastskill-core/src/core/service.rs b/crates/fastskill-core/src/core/service.rs index 1c666657..947002b0 100644 --- a/crates/fastskill-core/src/core/service.rs +++ b/crates/fastskill-core/src/core/service.rs @@ -526,12 +526,20 @@ impl FastSkillService { .to_string(); let skill_id = SkillId::new(skill_id_str)?; - // Create skill definition from frontmatter + // Create skill definition from frontmatter. This is a directory-scan + // registration path with no real provenance to record — the skill IS a + // local directory on disk, so `Origin::Local` is the accurate (and + // behavior-neutral) origin: previously all source_* fields were simply + // left `None` for this path. let mut skill = crate::core::skill_manager::SkillDefinition::new( skill_id.clone(), frontmatter.name, frontmatter.description, frontmatter.version.unwrap_or_else(|| "1.0.0".to_string()), + crate::core::origin::Origin::Local { + path: skill_dir.to_path_buf(), + editable: false, + }, ); // Set additional fields diff --git a/crates/fastskill-core/src/core/skill_manager.rs b/crates/fastskill-core/src/core/skill_manager.rs index 20db4008..4ac3958f 100644 --- a/crates/fastskill-core/src/core/skill_manager.rs +++ b/crates/fastskill-core/src/core/skill_manager.rs @@ -1,25 +1,16 @@ //! Skill management service implementation +use crate::core::origin::Origin; use crate::core::service::{ServiceError, SkillId}; use async_trait::async_trait; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use std::collections::HashMap; -use std::path::PathBuf; use std::sync::Arc; use tokio::sync::RwLock; // Note: SkillInfoMetadata removed - use skill-project.toml via manifest system instead -/// Source type for skill installation -#[derive(Debug, Clone, Serialize, Deserialize)] -pub enum SourceType { - GitUrl, - LocalPath, - ZipFile, - Source, -} - #[derive(Debug, Clone, Serialize, Deserialize)] pub struct SkillDefinition { pub id: SkillId, @@ -41,20 +32,21 @@ pub struct SkillDefinition { pub dependencies: Option>, pub timeout: Option, - // Source tracking - pub source_url: Option, - pub source_type: Option, - pub source_branch: Option, - pub source_tag: Option, - pub source_subdir: Option, - pub installed_from: Option, + // Provenance (install intent) + pub origin: Origin, + // Resolved facts a fetch produced (read by the lock) pub commit_hash: Option, pub fetched_at: Option>, - pub editable: bool, } impl SkillDefinition { - pub fn new(id: SkillId, name: String, description: String, version: String) -> Self { + pub fn new( + id: SkillId, + name: String, + description: String, + version: String, + origin: Origin, + ) -> Self { let now = Utc::now(); Self { id: id.clone(), @@ -71,15 +63,9 @@ impl SkillDefinition { execution_environment: None, dependencies: None, timeout: None, - source_url: None, - source_type: None, - source_branch: None, - source_tag: None, - source_subdir: None, - installed_from: None, + origin, commit_hash: None, fetched_at: None, - editable: false, } } @@ -125,15 +111,9 @@ pub struct SkillUpdate { pub description: Option, pub version: Option, pub author: Option, - pub source_url: Option, - pub source_type: Option, - pub source_branch: Option, - pub source_tag: Option, - pub source_subdir: Option, - pub installed_from: Option, + pub origin: Option, pub commit_hash: Option, pub fetched_at: Option>, - pub editable: Option, } #[async_trait] @@ -184,23 +164,8 @@ impl SkillManagementService for SkillManager { if let Some(author) = updates.author { skill.author = Some(author); } - if let Some(source_url) = updates.source_url { - skill.source_url = Some(source_url); - } - if let Some(source_type) = updates.source_type { - skill.source_type = Some(source_type); - } - if let Some(source_branch) = updates.source_branch { - skill.source_branch = Some(source_branch); - } - if let Some(source_tag) = updates.source_tag { - skill.source_tag = Some(source_tag); - } - if let Some(source_subdir) = updates.source_subdir { - skill.source_subdir = Some(source_subdir); - } - if let Some(installed_from) = updates.installed_from { - skill.installed_from = Some(installed_from); + if let Some(origin) = updates.origin { + skill.origin = origin; } if let Some(commit_hash) = updates.commit_hash { skill.commit_hash = Some(commit_hash); @@ -208,9 +173,6 @@ impl SkillManagementService for SkillManager { if let Some(fetched_at) = updates.fetched_at { skill.fetched_at = Some(fetched_at); } - if let Some(editable) = updates.editable { - skill.editable = editable; - } skill.updated_at = Utc::now(); Ok(()) } else { diff --git a/crates/fastskill-core/src/core/update.rs b/crates/fastskill-core/src/core/update.rs index 2607efa7..8e6f0d05 100644 --- a/crates/fastskill-core/src/core/update.rs +++ b/crates/fastskill-core/src/core/update.rs @@ -109,7 +109,7 @@ impl UpdateService { } UpdateStrategy::Patch => { // Find highest patch version in same minor - let current = Version::parse(&locked_skill.version).map_err(|e| { + let current = Version::parse(&locked_skill.resolved.version).map_err(|e| { UpdateError::VersionError(VersionError::ParseError(e.to_string())) })?; @@ -137,7 +137,7 @@ impl UpdateService { } UpdateStrategy::Minor => { // Find highest version in same major - let current = Version::parse(&locked_skill.version).map_err(|e| { + let current = Version::parse(&locked_skill.resolved.version).map_err(|e| { UpdateError::VersionError(VersionError::ParseError(e.to_string())) })?; @@ -183,7 +183,7 @@ impl UpdateService { }; // Check if update is actually newer - if !is_newer(&target_version.version, &locked_skill.version)? { + if !is_newer(&target_version.version, &locked_skill.resolved.version)? { return Err(UpdateError::NoUpdateAvailable(locked_skill.id.clone())); } @@ -200,7 +200,7 @@ impl UpdateService { Ok(UpdateInfo { skill_id: locked_skill.id.clone(), - current_version: locked_skill.version.clone(), + current_version: locked_skill.resolved.version.clone(), available_version: target_version.version.clone(), resolution, }) diff --git a/crates/fastskill-core/src/events/event_bus.rs b/crates/fastskill-core/src/events/event_bus.rs index 31258c29..5facb49f 100644 --- a/crates/fastskill-core/src/events/event_bus.rs +++ b/crates/fastskill-core/src/events/event_bus.rs @@ -542,6 +542,10 @@ mod tests { "Sample".to_string(), "A sample skill".to_string(), "1.0.0".to_string(), + crate::core::origin::Origin::Local { + path: std::path::PathBuf::from("./skills/sample-skill"), + editable: false, + }, ) } diff --git a/crates/fastskill-core/src/http/handlers/manifest.rs b/crates/fastskill-core/src/http/handlers/manifest.rs index 0aa3791f..f9898218 100644 --- a/crates/fastskill-core/src/http/handlers/manifest.rs +++ b/crates/fastskill-core/src/http/handlers/manifest.rs @@ -1,8 +1,7 @@ //! Manifest (skill-project.toml) endpoint handlers -use crate::core::manifest::{ - DependenciesSection, DependencySource, DependencySpec, SkillProjectToml, -}; +use crate::core::manifest::{DependenciesSection, DependencySpec, SkillProjectToml}; +use crate::core::origin::Origin; use crate::core::repository::RepositoryManager; use crate::core::sources::{MarketplaceSkill, SourcesManager}; use crate::http::errors::{HttpError, HttpResult}; @@ -48,12 +47,37 @@ pub(crate) fn get_repositories( fn dep_source_type(spec: &DependencySpec) -> &'static str { match spec { DependencySpec::Version(_) => "source", - DependencySpec::Inline { source, .. } => match source { - DependencySource::Git => "git", - DependencySource::Local => "local", - DependencySource::ZipUrl => "zip-url", - DependencySource::Source => "source", + DependencySpec::Inline { origin, .. } => match origin { + Origin::Git { .. } => "git", + Origin::Local { .. } => "local", + Origin::ZipUrl { .. } => "zip-url", + Origin::Repository { .. } => "source", + }, + } +} + +/// Human-readable location string for an `Origin`, used in the `/api/project` view. +fn origin_location(origin: &Origin) -> String { + match origin { + Origin::Git { url, r#ref, .. } => match r#ref { + crate::core::origin::GitRef::Branch(b) => format!("{} (branch: {})", url, b), + crate::core::origin::GitRef::Tag(t) => format!("{} (tag: {})", url, t), + crate::core::origin::GitRef::Commit(c) => format!("{} (commit: {})", url, c), + crate::core::origin::GitRef::Default => url.clone(), }, + Origin::Local { path, .. } => path.to_string_lossy().to_string(), + Origin::ZipUrl { url } => url.clone(), + Origin::Repository { repo, skill, .. } => format!("{} / {}", repo, skill), + } +} + +/// The declared version for a dependency, if any. Only `Origin::Repository` +/// carries a version constraint (ADR-0004); other origins install "whatever +/// is at that location" and have no separate version concept. +fn origin_version(origin: &Origin) -> Option { + match origin { + Origin::Repository { version, .. } => version.as_ref().map(|v| v.to_string()), + _ => None, } } @@ -116,40 +140,8 @@ pub async fn get_project( DependencySpec::Version(v) => { (dep_source_type(spec).to_string(), format!("version {}", v)) } - DependencySpec::Inline { - source, - source_specific, - .. - } => { - let typ = dep_source_type(spec).to_string(); - let location = match source { - DependencySource::Git => source_specific - .url - .clone() - .map(|u| { - source_specific - .branch - .as_ref() - .map(|b| format!("{} (branch: {})", u, b)) - .unwrap_or(u) - }) - .unwrap_or_else(|| "—".to_string()), - DependencySource::Local => source_specific - .path - .clone() - .unwrap_or_else(|| "—".to_string()), - DependencySource::ZipUrl => source_specific - .zip_url - .clone() - .unwrap_or_else(|| "—".to_string()), - DependencySource::Source => source_specific - .name - .as_ref() - .zip(source_specific.skill.as_ref()) - .map(|(n, s)| format!("{} / {}", n, s)) - .unwrap_or_else(|| "—".to_string()), - }; - (typ, location) + DependencySpec::Inline { origin, .. } => { + (dep_source_type(spec).to_string(), origin_location(origin)) } }; serde_json::json!({ "id": id, "type": typ, "location": location }) @@ -191,19 +183,23 @@ pub async fn list_manifest_skills( DependencySpec::Version(v) => { (Some(v.clone()), dep_source_type(spec).to_string()) } - DependencySpec::Inline { - source_specific, .. - } => ( - source_specific.version.clone(), - dep_source_type(spec).to_string(), + DependencySpec::Inline { origin, .. } => { + (origin_version(origin), dep_source_type(spec).to_string()) + } + }; + let (groups, editable) = match spec { + DependencySpec::Inline { groups, origin } => ( + groups.clone().unwrap_or_default(), + matches!(origin, Origin::Local { editable: true, .. }), ), + DependencySpec::Version(_) => (Vec::new(), false), }; ManifestSkillResponse { id: id.clone(), version, - groups: Vec::new(), // TODO: extract from inline spec - editable: false, // TODO: extract from inline spec + groups, + editable, source_type, } }) diff --git a/crates/fastskill-core/src/http/handlers/skills.rs b/crates/fastskill-core/src/http/handlers/skills.rs index 9c3763b5..07fc00a9 100644 --- a/crates/fastskill-core/src/http/handlers/skills.rs +++ b/crates/fastskill-core/src/http/handlers/skills.rs @@ -15,10 +15,7 @@ pub struct UpgradeRequest { } fn skill_metadata_json(skill: &crate::core::skill_manager::SkillDefinition) -> serde_json::Value { - let source_type = skill - .source_type - .as_ref() - .map(|t| serde_json::to_value(t).unwrap_or(serde_json::Value::Null)); + let origin = serde_json::to_value(&skill.origin).unwrap_or(serde_json::Value::Null); serde_json::json!({ "id": skill.id, "name": skill.name, @@ -31,8 +28,7 @@ fn skill_metadata_json(skill: &crate::core::skill_manager::SkillDefinition) -> s "reference_files": skill.reference_files, "script_files": skill.script_files, "asset_files": skill.asset_files, - "source_url": skill.source_url, - "source_type": source_type + "origin": origin }) } diff --git a/crates/fastskill-core/src/output/mod.rs b/crates/fastskill-core/src/output/mod.rs index 5a41875b..f31da3df 100644 --- a/crates/fastskill-core/src/output/mod.rs +++ b/crates/fastskill-core/src/output/mod.rs @@ -3,11 +3,32 @@ //! This module provides shared output formatting capabilities that can be used //! by multiple CLI commands to ensure consistent output styling. +use crate::core::origin::Origin; use crate::core::SkillDefinition; use crate::search::SearchResultItem; use serde_json; use std::fmt; +/// Short origin-type label (git/local/zip-url/repository) for display. +fn origin_type_label(origin: &Origin) -> &'static str { + match origin { + Origin::Git { .. } => "git", + Origin::Local { .. } => "local", + Origin::ZipUrl { .. } => "zip-url", + Origin::Repository { .. } => "repository", + } +} + +/// Location string (URL/path/repo-skill) for display. +fn origin_location_label(origin: &Origin) -> String { + match origin { + Origin::Git { url, .. } => url.clone(), + Origin::Local { path, .. } => path.display().to_string(), + Origin::ZipUrl { url } => url.clone(), + Origin::Repository { repo, skill, .. } => format!("{repo}/{skill}"), + } +} + /// One row for the list table: union of all skills with presence and gap flags. #[derive(Debug, Clone, serde::Serialize)] pub struct ListRow { @@ -539,12 +560,14 @@ fn format_show_table(skills: &[SkillDefinition]) -> Result { output.push_str(&format!(" ID: {}\n", skill.id)); output.push_str(&format!(" Version: {}\n", skill.version)); output.push_str(&format!(" Description: {}\n", skill.description)); - if let Some(source_type) = &skill.source_type { - output.push_str(&format!(" Source Type: {:?}\n", source_type)); - } - if let Some(source_url) = &skill.source_url { - output.push_str(&format!(" Source URL: {}\n", source_url)); - } + output.push_str(&format!( + " Source Type: {}\n", + origin_type_label(&skill.origin) + )); + output.push_str(&format!( + " Source: {}\n", + origin_location_label(&skill.origin) + )); output.push('\n'); } @@ -584,18 +607,14 @@ fn format_show_xml(skills: &[SkillDefinition]) -> Result { " {}\n", escape_xml(&skill.description) )); - if let Some(source_type) = &skill.source_type { - xml.push_str(&format!( - " {:?}\n", - source_type - )); - } - if let Some(source_url) = &skill.source_url { - xml.push_str(&format!( - " {}\n", - escape_xml(source_url) - )); - } + xml.push_str(&format!( + " {}\n", + escape_xml(origin_type_label(&skill.origin)) + )); + xml.push_str(&format!( + " {}\n", + escape_xml(&origin_location_label(&skill.origin)) + )); xml.push_str(" \n"); } diff --git a/crates/fastskill-core/src/validation/skill_validator_tests.rs b/crates/fastskill-core/src/validation/skill_validator_tests.rs index 168c1410..01cd7f8e 100644 --- a/crates/fastskill-core/src/validation/skill_validator_tests.rs +++ b/crates/fastskill-core/src/validation/skill_validator_tests.rs @@ -26,6 +26,10 @@ fn create_test_skill_definition( name.to_string(), description.to_string(), version.to_string(), + crate::core::origin::Origin::Local { + path: skill_dir.clone(), + editable: false, + }, ); skill.skill_file = skill_file; skill diff --git a/crates/fastskill-core/tests/http_handler_route_tests.rs b/crates/fastskill-core/tests/http_handler_route_tests.rs index faad3ef2..6c938c5e 100644 --- a/crates/fastskill-core/tests/http_handler_route_tests.rs +++ b/crates/fastskill-core/tests/http_handler_route_tests.rs @@ -492,10 +492,10 @@ name = "Proj" [dependencies] verdep = "1.2.3" -gitdep = { source = "git", url = "https://example.com/x.git", branch = "main" } -localdep = { source = "local", path = "./local/x" } -zipdep = { source = "zip-url", zip_url = "https://example.com/x.zip" } -srcdep = { source = "source", name = "acme", skill = "widget" } +gitdep = { origin = { type = "git", url = "https://example.com/x.git", ref = { branch = "main" } } } +localdep = { origin = { type = "local", path = "./local/x" } } +zipdep = { origin = { type = "zip-url", url = "https://example.com/x.zip" } } +srcdep = { origin = { type = "repository", repo = "acme", skill = "widget" } } "#; fs::write(&f.project_file_path, toml).unwrap(); let (status, body) = do_get(f.state, "/project").await; @@ -523,7 +523,7 @@ async fn manifest_list_with_deps() { let toml = r#" [dependencies] verdep = "1.0.0" -gitdep = { source = "git", url = "https://example.com/x.git", version = "2.0.0" } +gitdep = { origin = { type = "git", url = "https://example.com/x.git" } } "#; fs::write(&f.project_file_path, toml).unwrap(); let (status, body) = do_get(f.state, "/manifest/skills").await; diff --git a/crates/fastskill-core/tests/integration_unified_format_test.rs b/crates/fastskill-core/tests/integration_unified_format_test.rs index 9dec9c86..94635757 100644 --- a/crates/fastskill-core/tests/integration_unified_format_test.rs +++ b/crates/fastskill-core/tests/integration_unified_format_test.rs @@ -1,6 +1,6 @@ //! Integration tests for unified format context detection -#![allow(clippy::all, clippy::unwrap_used, clippy::expect_used)] +#![allow(clippy::all, clippy::unwrap_used, clippy::expect_used, clippy::panic)] use fastskill_core::core::manifest::ProjectContext; use fastskill_core::core::manifest::SkillProjectToml; @@ -269,7 +269,18 @@ test-skill = "1.0.0" let entries = project.to_skill_entries().unwrap(); assert_eq!(entries.len(), 1); assert_eq!(entries[0].id, "test-skill"); - assert_eq!(entries[0].version, "1.0.0".to_string()); + match &entries[0].origin { + fastskill_core::core::origin::Origin::Repository { + repo, + skill, + version, + } => { + assert_eq!(repo, "default"); + assert_eq!(skill, "test-skill"); + assert_eq!(version.as_ref().unwrap().to_string(), "=1.0.0"); + } + other => panic!("expected Origin::Repository, got {other:?}"), + } // Test 5: Verify skill-project.toml with all sections let full_project = SkillProjectToml { diff --git a/crates/fastskill-core/tests/skill_project_toml_test.rs b/crates/fastskill-core/tests/skill_project_toml_test.rs index bf9595f9..eec8ed77 100644 --- a/crates/fastskill-core/tests/skill_project_toml_test.rs +++ b/crates/fastskill-core/tests/skill_project_toml_test.rs @@ -3,9 +3,9 @@ #![allow(clippy::all, clippy::unwrap_used, clippy::expect_used)] use fastskill_core::core::manifest::{ - DependenciesSection, DependencySource, DependencySpec, MetadataSection, SkillProjectToml, - SourceSpecificFields, + DependenciesSection, DependencySpec, MetadataSection, SkillProjectToml, }; +use fastskill_core::core::origin::Origin; use std::collections::HashMap; #[test] @@ -41,18 +41,14 @@ fn test_skill_project_toml_serialization_with_dependencies_only() { deps.insert( "skill2".to_string(), DependencySpec::Inline { - source: DependencySource::Source, - source_specific: SourceSpecificFields { - name: Some("enterprise".to_string()), - version: Some("2.0.0".to_string()), - url: None, - branch: None, - path: None, - skill: None, - zip_url: None, + origin: Origin::Repository { + repo: "enterprise".to_string(), + skill: "skill2".to_string(), + version: Some( + fastskill_core::core::version::VersionConstraint::parse("2.0.0").unwrap(), + ), }, groups: None, - editable: None, }, ); @@ -66,11 +62,11 @@ fn test_skill_project_toml_serialization_with_dependencies_only() { assert!(toml_string.contains("[dependencies]")); assert!(toml_string.contains("skill1 = \"1.0.0\"")); - // TOML serializer uses table format for inline tables: [dependencies.skill2] - assert!(toml_string.contains("[dependencies.skill2]")); - assert!(toml_string.contains("source = \"source\"")); - assert!(toml_string.contains("name = \"enterprise\"")); - assert!(toml_string.contains("version = \"2.0.0\"")); + // TOML serializer uses table format for inline tables: [dependencies.skill2.origin] + assert!(toml_string.contains("[dependencies.skill2.origin]")); + assert!(toml_string.contains("type = \"repository\"")); + assert!(toml_string.contains("repo = \"enterprise\"")); + assert!(toml_string.contains("version = \"=2.0.0\"")); } #[test] @@ -112,7 +108,7 @@ description = "Test description" [dependencies] skill1 = "1.0.0" -skill2 = { source = "source", name = "enterprise", version = "2.0.0" } +skill2 = { origin = { type = "repository", repo = "enterprise", skill = "skill2", version = "2.0.0" } } "#; let project: SkillProjectToml = toml::from_str(toml_content).unwrap(); From 7fb596e51ebb33da04bffeca6fb15a425e500393 Mon Sep 17 00:00:00 2001 From: Alex Oliveira <4482374+aroff@users.noreply.github.com> Date: Tue, 21 Jul 2026 21:45:14 +0000 Subject: [PATCH 6/7] feat(cli): migrate fastskill-cli onto the Origin model (Phase 0, cli) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BREAKING CHANGE: on-disk lock/manifest formats change (greenfield, no shim). Fix fastskill-cli against the reshaped core: build a concrete Origin at each install site (add_from_git/folder/zip/registry), carry origin through the lock<->manifest reconstructions (previously lossy — dropped source fields), read resolved.version from lock entries, and delete the dead source-detection trio (resolve_source_type/autodetect_source/ResolvedSource). SourceMeta and the four per-path SkillUpdate builders collapse to one {origin} each. Behavior-preserving (CLI outcomes unchanged; net -261 lines). lock_migration_test rewritten: pre-3.0 locks are now rejected (UnsupportedVersion), not migrated. Full workspace green: build + clippy -D warnings + 653 tests, 0 failures. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../fastskill-cli/src/commands/add/install.rs | 18 +- crates/fastskill-cli/src/commands/add/mod.rs | 22 +- .../src/commands/add/skill_def.rs | 34 ++- .../fastskill-cli/src/commands/add/sources.rs | 284 +++++------------- crates/fastskill-cli/src/commands/install.rs | 9 +- crates/fastskill-cli/src/commands/list.rs | 64 ++-- crates/fastskill-cli/src/commands/read.rs | 6 +- crates/fastskill-cli/src/commands/update.rs | 6 +- .../fastskill-cli/src/utils/install_utils.rs | 204 ++++++------- .../fastskill-cli/src/utils/manifest_utils.rs | 136 ++------- .../tests/lock_determinism_test.rs | 15 +- .../tests/lock_migration_test.rs | 70 ++--- .../tests/lock_scope_routing_test.rs | 15 +- 13 files changed, 320 insertions(+), 563 deletions(-) diff --git a/crates/fastskill-cli/src/commands/add/install.rs b/crates/fastskill-cli/src/commands/add/install.rs index 7c866fd2..1cc7b107 100644 --- a/crates/fastskill-cli/src/commands/add/install.rs +++ b/crates/fastskill-cli/src/commands/add/install.rs @@ -88,26 +88,14 @@ pub(super) async fn finish_skill_install( ) -> CliResult<()> { use chrono::Utc; skill_def.skill_file = storage_dir.join("SKILL.md"); - skill_def.source_url = meta.source_url; - skill_def.source_type = meta.source_type; - skill_def.source_branch = meta.source_branch; - skill_def.source_tag = meta.source_tag; - skill_def.source_subdir = meta.source_subdir; - skill_def.installed_from = meta.installed_from; + skill_def.origin = meta.origin; skill_def.fetched_at = Some(Utc::now()); - skill_def.editable = ctx.editable; super::register_skill_once(ctx, &skill_def).await?; let update = fastskill_core::core::skill_manager::SkillUpdate { - source_url: skill_def.source_url.clone(), - source_type: skill_def.source_type.clone(), - source_branch: skill_def.source_branch.clone(), - source_tag: skill_def.source_tag.clone(), - source_subdir: skill_def.source_subdir.clone(), - installed_from: skill_def.installed_from.clone(), + origin: Some(skill_def.origin.clone()), fetched_at: skill_def.fetched_at, - editable: Some(skill_def.editable), ..Default::default() }; ctx.service @@ -133,7 +121,7 @@ pub(super) async fn finish_skill_install( crate::utils::messages::ok("Updated global-skills.lock") ); } else { - super::update_project_files(&skill_def, ctx.groups.clone(), ctx.editable)?; + super::update_project_files(&skill_def, ctx.groups.clone())?; println!( "Successfully added skill: {} (v{})", skill_def.name, version_display diff --git a/crates/fastskill-cli/src/commands/add/mod.rs b/crates/fastskill-cli/src/commands/add/mod.rs index 9b1922db..93e97333 100644 --- a/crates/fastskill-cli/src/commands/add/mod.rs +++ b/crates/fastskill-cli/src/commands/add/mod.rs @@ -13,8 +13,8 @@ use cli_framework::command::{FromArgValueMap, IntoCommandSpec}; use cli_framework::spec::arg_spec::{ArgKind, ArgSpec, ArgValueType, Cardinality}; use cli_framework::spec::command_tree::CommandSpec; use cli_framework::spec::value::ArgValue; +use fastskill_core::core::origin::Origin; use fastskill_core::core::project::resolve_project_file; -use fastskill_core::core::skill_manager::SourceType; use fastskill_core::{FastSkillService, SkillDefinition}; pub use install::copy_dir_recursive; pub use skill_def::create_skill_from_path; @@ -31,14 +31,12 @@ struct AddContext<'a> { global: bool, } -/// Source metadata to record after installing a skill +/// Source metadata to record after installing a skill. +/// +/// `Origin` (install intent) is now the single source of truth for provenance, +/// so this is a thin wrapper rather than a flat bag of source_* fields. struct SourceMeta { - source_url: Option, - source_type: Option, - source_branch: Option, - source_tag: Option, - source_subdir: Option, - installed_from: Option, + origin: Origin, } /// Target for install: where to copy and what to record @@ -48,13 +46,9 @@ struct InstallTarget { version_display: String, } -fn update_project_files( - skill_def: &SkillDefinition, - groups: Vec, - editable: bool, -) -> CliResult<()> { +fn update_project_files(skill_def: &SkillDefinition, groups: Vec) -> CliResult<()> { use crate::utils::manifest_utils; - manifest_utils::add_skill_to_project_toml(skill_def, groups.clone(), editable) + manifest_utils::add_skill_to_project_toml(skill_def, groups.clone()) .map_err(|e| CliError::Config(format!("Failed to update skill-project.toml: {}", e)))?; let current_dir = env::current_dir() diff --git a/crates/fastskill-cli/src/commands/add/skill_def.rs b/crates/fastskill-cli/src/commands/add/skill_def.rs index 1fdae2aa..0949af22 100644 --- a/crates/fastskill-cli/src/commands/add/skill_def.rs +++ b/crates/fastskill-cli/src/commands/add/skill_def.rs @@ -1,6 +1,7 @@ //! Skill definition helpers for the add command. use crate::error::{CliError, CliResult, CliWarning}; +use fastskill_core::core::origin::Origin; use fastskill_core::SkillDefinition; use std::fs; use std::path::Path; @@ -86,8 +87,12 @@ fn generate_fallback_skill_id(skill_path: &Path) -> CliResult { /// Create a skill definition from a path containing SKILL.md. /// The skill ID is read from skill-project.toml if present, otherwise from SKILL.md frontmatter. +/// +/// `origin` is the caller-constructed provenance (install intent) for this skill; +/// `source_type` is only a cosmetic label used for the missing-toml warning display. pub fn create_skill_from_path( skill_path: &Path, + origin: Origin, source_type: &str, editable: bool, ) -> CliResult { @@ -129,8 +134,13 @@ pub fn create_skill_from_path( "1.0.0".to_string() }; - let mut skill = - SkillDefinition::new(skill_id, frontmatter.name, frontmatter.description, version); + let mut skill = SkillDefinition::new( + skill_id, + frontmatter.name, + frontmatter.description, + version, + origin, + ); skill.skill_file = skill_file.clone(); skill.author = frontmatter.author; @@ -145,6 +155,15 @@ mod tests { use std::fs; use tempfile::TempDir; + /// A throwaway `Origin` for tests that only care about the resulting `SkillDefinition` + /// fields derived from SKILL.md / skill-project.toml, not the origin itself. + fn test_origin(path: &Path) -> Origin { + Origin::Local { + path: path.to_path_buf(), + editable: false, + } + } + #[test] fn test_read_project_metadata_missing_file() { let tmp = TempDir::new().unwrap(); @@ -181,7 +200,8 @@ Test skill content "#; fs::write(tmp.path().join("SKILL.md"), skill_md).unwrap(); - let skill = create_skill_from_path(tmp.path(), "local", false).unwrap(); + let skill = + create_skill_from_path(tmp.path(), test_origin(tmp.path()), "local", false).unwrap(); assert_eq!(skill.id.as_str(), "test-skill-no-toml"); } @@ -204,7 +224,8 @@ version = "1.5.0" "#; fs::write(tmp.path().join("skill-project.toml"), project_toml).unwrap(); - let skill = create_skill_from_path(tmp.path(), "local", false).unwrap(); + let skill = + create_skill_from_path(tmp.path(), test_origin(tmp.path()), "local", false).unwrap(); assert_eq!(skill.id.as_str(), "from-toml"); assert_eq!(skill.version, "1.5.0"); } @@ -220,7 +241,8 @@ Test content "#; fs::write(tmp.path().join("SKILL.md"), skill_md).unwrap(); - let skill = create_skill_from_path(tmp.path(), "local", false).unwrap(); + let skill = + create_skill_from_path(tmp.path(), test_origin(tmp.path()), "local", false).unwrap(); assert_eq!(skill.id.as_str(), "fallback-name-skill"); } @@ -235,7 +257,7 @@ Test content "#; fs::write(tmp.path().join("SKILL.md"), skill_md).unwrap(); - let result = create_skill_from_path(tmp.path(), "local", false); + let result = create_skill_from_path(tmp.path(), test_origin(tmp.path()), "local", false); assert!(result.is_err(), "Should fail due to spaces in name"); } } diff --git a/crates/fastskill-cli/src/commands/add/sources.rs b/crates/fastskill-cli/src/commands/add/sources.rs index deee4986..3db6d1de 100644 --- a/crates/fastskill-cli/src/commands/add/sources.rs +++ b/crates/fastskill-cli/src/commands/add/sources.rs @@ -2,69 +2,13 @@ use crate::error::{CliError, CliResult}; use crate::utils::install_utils::{extract_zip, safe_subdir_join}; -use crate::utils::{detect_skill_source, parse_git_url, validate_skill_structure, SkillSource}; -use fastskill_core::core::skill_manager::SourceType; +use crate::utils::{parse_git_url, validate_skill_structure}; +use fastskill_core::core::origin::{GitRef, Origin}; use std::fs; use std::path::{Path, PathBuf}; use tempfile::TempDir; use tracing::info; -/// Resolved source type for the add operation -#[allow(dead_code)] -pub enum ResolvedSource { - /// Local directory path - Local(PathBuf), - /// Git URL - Git(String), - /// ZIP file path - Zip(PathBuf), - /// Registry skill ID - Registry(String), -} - -/// Resolve source type from an explicit flag value or autodetect from the path/URL. -#[allow(dead_code)] -pub fn resolve_source_type(source: &str, source_type: Option<&str>) -> CliResult { - let Some(stype) = source_type else { - return Ok(autodetect_source(source)); - }; - - match stype { - "registry" => Ok(ResolvedSource::Registry(source.to_string())), - "github" | "git" => Ok(ResolvedSource::Git(source.to_string())), - "local" => { - let path = PathBuf::from(source); - if path.extension().and_then(|s| s.to_str()) == Some("zip") { - Ok(ResolvedSource::Zip(path)) - } else { - Ok(ResolvedSource::Local(path)) - } - } - other => Err(CliError::Config(format!( - "Unrecognized --source-type value: '{}'. Expected one of: registry, git, github, local", - other - ))), - } -} - -/// Autodetect source type from path/URL. -#[allow(dead_code)] -pub fn autodetect_source(source: &str) -> ResolvedSource { - match detect_skill_source(source) { - SkillSource::GitUrl(url) => ResolvedSource::Git(url), - SkillSource::ZipFile(path) => ResolvedSource::Zip(path), - SkillSource::Folder(path) => ResolvedSource::Local(path), - SkillSource::SkillId(id) => ResolvedSource::Registry(id), - } -} - -/// Check if a path is a local path that could be a skill directory. -#[allow(dead_code)] -pub fn is_local_path(source: &str) -> bool { - let path = Path::new(source); - path.exists() || source.starts_with('.') || source.starts_with('/') -} - // ── Source installation handlers ────────────────────────────────────────────── /// Find SKILL.md in a directory (root first, then one level of subdirectories). @@ -112,7 +56,12 @@ pub(super) async fn add_from_zip(ctx: &super::AddContext<'_>, zip_path: &Path) - let extract_path = _temp_dir.path(); let skill_path = find_skill_in_directory(extract_path)?; validate_skill_structure(&skill_path)?; - let skill_def = super::skill_def::create_skill_from_path(&skill_path, "zip", false)?; + let origin = Origin::Local { + path: canonical_zip_path.clone(), + editable: false, + }; + let skill_def = + super::skill_def::create_skill_from_path(&skill_path, origin.clone(), "zip", false)?; let version = skill_def.version.clone(); let target = super::InstallTarget { storage_dir: ctx @@ -120,14 +69,7 @@ pub(super) async fn add_from_zip(ctx: &super::AddContext<'_>, zip_path: &Path) - .config() .skill_storage_path .join(skill_def.id.as_str()), - meta: super::SourceMeta { - source_url: Some(canonical_zip_path.to_string_lossy().to_string()), - source_type: Some(SourceType::ZipFile), - source_branch: None, - source_tag: None, - source_subdir: None, - installed_from: None, - }, + meta: super::SourceMeta { origin }, version_display: version, }; super::install::install_via_download(ctx, &skill_path, skill_def, target).await @@ -139,7 +81,6 @@ pub(super) async fn add_from_folder( ) -> CliResult<()> { info!("Adding skill from folder: {}", folder_path.display()); validate_skill_structure(folder_path)?; - let skill_def = super::skill_def::create_skill_from_path(folder_path, "local", ctx.editable)?; let canonical_path = folder_path.canonicalize().map_err(|e| { CliError::InvalidSource(format!( "Failed to resolve absolute path for '{}': {}", @@ -147,20 +88,23 @@ pub(super) async fn add_from_folder( e )) })?; + let origin = Origin::Local { + path: canonical_path.clone(), + editable: ctx.editable, + }; + let skill_def = super::skill_def::create_skill_from_path( + folder_path, + origin.clone(), + "local", + ctx.editable, + )?; let target = super::InstallTarget { storage_dir: ctx .service .config() .skill_storage_path .join(skill_def.id.as_str()), - meta: super::SourceMeta { - source_url: Some(canonical_path.to_string_lossy().to_string()), - source_type: Some(SourceType::LocalPath), - source_branch: None, - source_tag: None, - source_subdir: None, - installed_from: None, - }, + meta: super::SourceMeta { origin }, version_display: skill_def.version.clone(), }; super::install::install_via_local_path(ctx, &canonical_path, skill_def, target).await @@ -170,13 +114,7 @@ async fn clone_and_validate_skill( git_url: &str, branch: Option<&str>, tag: Option<&str>, -) -> CliResult<( - TempDir, - PathBuf, - fastskill_core::SkillDefinition, - Option, - Option, -)> { +) -> CliResult<(TempDir, PathBuf, fastskill_core::SkillDefinition, Origin)> { use fastskill_core::storage::git::{clone_repository, validate_cloned_skill}; let git_info = parse_git_url(git_url)?; let branch = branch.or(git_info.branch.as_deref()); @@ -198,14 +136,20 @@ async fn clone_and_validate_skill( }; let skill_path = validate_cloned_skill(&skill_base_path) .map_err(|e| CliError::SkillValidationFailed(e.to_string()))?; - let skill_def = super::skill_def::create_skill_from_path(&skill_path, "git", false)?; - Ok(( - temp_dir, - skill_path, - skill_def, - branch.map(String::from), - tag.map(String::from), - )) + + let r#ref = match (branch, tag) { + (Some(b), _) => GitRef::Branch(b.to_string()), + (None, Some(t)) => GitRef::Tag(t.to_string()), + (None, None) => GitRef::Default, + }; + let origin = Origin::Git { + url: git_url.to_string(), + r#ref, + subdir: git_info.subdir.clone(), + }; + let skill_def = + super::skill_def::create_skill_from_path(&skill_path, origin.clone(), "git", false)?; + Ok((temp_dir, skill_path, skill_def, origin)) } pub(super) async fn add_from_git( @@ -215,24 +159,16 @@ pub(super) async fn add_from_git( tag: Option<&str>, ) -> CliResult<()> { info!("Adding skill from git URL: {}", git_url); - let (_temp_dir, skill_path, skill_def, branch_opt, tag_opt) = + let (_temp_dir, skill_path, skill_def, origin) = clone_and_validate_skill(git_url, branch, tag).await?; validate_skill_structure(&skill_path)?; - let git_info = parse_git_url(git_url)?; let target = super::InstallTarget { storage_dir: ctx .service .config() .skill_storage_path .join(skill_def.id.as_str()), - meta: super::SourceMeta { - source_url: Some(git_url.to_string()), - source_type: Some(SourceType::GitUrl), - source_branch: branch_opt.or(git_info.branch), - source_tag: tag_opt, - source_subdir: git_info.subdir, - installed_from: None, - }, + meta: super::SourceMeta { origin }, version_display: skill_def.version.clone(), }; super::install::install_via_download(ctx, &skill_path, skill_def, target).await @@ -296,6 +232,7 @@ async fn download_registry_package( repo_client: &(dyn fastskill_core::core::repository::RepositoryClient + Send + Sync), skill_id_full: &str, version: &str, + origin: Origin, ) -> CliResult<(TempDir, PathBuf, fastskill_core::SkillDefinition)> { let zip_data = repo_client .download(skill_id_full, version) @@ -309,7 +246,8 @@ async fn download_registry_package( extract_zip(&temp_zip, &extract_path)?; let skill_path = find_skill_in_directory(&extract_path)?; validate_skill_structure(&skill_path)?; - let skill_def = super::skill_def::create_skill_from_path(&skill_path, "registry", false)?; + let skill_def = + super::skill_def::create_skill_from_path(&skill_path, origin, "registry", false)?; Ok((temp_dir, skill_path, skill_def)) } @@ -343,6 +281,21 @@ pub(super) async fn add_from_registry( .get_client(&default_repo.name) .await .map_err(|e| CliError::Config(format!("Failed to get repository client: {}", e)))?; + + // Origin::Repository.version is the *declared* constraint (from `skill@version`, + // if any) — `None` means "newest allowed" — not the concrete resolved version, + // which stays out of Origin and lives on the lock's `Resolved` instead. + let version_constraint = version_opt + .as_deref() + .map(fastskill_core::core::version::VersionConstraint::parse) + .transpose() + .map_err(|e| CliError::Config(format!("Invalid version constraint: {}", e)))?; + let origin = Origin::Repository { + repo: default_repo.name.clone(), + skill: skill_id_full.clone(), + version: version_constraint, + }; + let version = resolve_registry_version(repo_client.as_ref(), &skill_id_full, version_opt).await?; @@ -362,7 +315,7 @@ pub(super) async fn add_from_registry( skill_id_full, version ); let (_temp_dir, skill_path, skill_def) = - download_registry_package(repo_client.as_ref(), &skill_id_full, &version).await?; + download_registry_package(repo_client.as_ref(), &skill_id_full, &version, origin).await?; if skill_def.id.as_str() != expected_id { return Err(CliError::Config(format!( @@ -379,12 +332,7 @@ pub(super) async fn add_from_registry( .join(&scope) .join(skill_def.id.as_str()), meta: super::SourceMeta { - source_url: None, - source_type: Some(SourceType::Source), - source_branch: None, - source_tag: None, - source_subdir: None, - installed_from: Some(default_repo.name.clone()), + origin: skill_def.origin.clone(), }, version_display: version, }; @@ -418,53 +366,6 @@ mod tests { assert!(parse_registry_scope_id("bad\\scope/id").is_err()); } - #[test] - fn test_resolve_invalid_source_type_returns_error() { - let result = resolve_source_type("/some/path", Some("invalid-type")); - assert!( - result.is_err(), - "invalid source type must return CliError::Config" - ); - if let Err(CliError::Config(msg)) = result { - assert!( - msg.contains("invalid-type"), - "error message must mention the invalid value" - ); - } else { - panic!("Expected CliError::Config"); - } - } - - #[test] - fn test_resolve_registry_source_type() { - let result = resolve_source_type("my-skill@1.0.0", Some("registry")).unwrap(); - assert!(matches!(result, ResolvedSource::Registry(_))); - } - - #[test] - fn test_resolve_git_source_type() { - let result = resolve_source_type("https://github.com/org/skill.git", Some("git")).unwrap(); - assert!(matches!(result, ResolvedSource::Git(_))); - } - - #[test] - fn test_resolve_local_source_type_directory() { - let result = resolve_source_type("/some/local/path", Some("local")).unwrap(); - assert!(matches!(result, ResolvedSource::Local(_))); - } - - #[test] - fn test_resolve_local_source_type_zip() { - let result = resolve_source_type("/some/skill.zip", Some("local")).unwrap(); - assert!(matches!(result, ResolvedSource::Zip(_))); - } - - #[test] - fn test_resolve_no_source_type_autodetects() { - let result = resolve_source_type("my-skill@1.0.0", None).unwrap(); - assert!(matches!(result, ResolvedSource::Registry(_))); - } - #[test] fn test_parse_registry_scope_id_valid() { let (full, scope, id, version) = parse_registry_scope_id("my-scope/my-skill").unwrap(); @@ -493,63 +394,6 @@ mod tests { } } - // ── is_local_path ───────────────────────────────────────────────────────── - - #[test] - fn test_is_local_path_dot_and_slash_prefixes() { - assert!(is_local_path("./relative")); - assert!(is_local_path("/absolute/path")); - assert!(is_local_path("../up")); - } - - #[test] - fn test_is_local_path_existing_path() { - let tmp = tempfile::TempDir::new().unwrap(); - let p = tmp.path().join("thing"); - std::fs::write(&p, "x").unwrap(); - assert!(is_local_path(&p.to_string_lossy())); - } - - #[test] - fn test_is_local_path_bare_name_is_not_local() { - // A bare, non-existent name is treated as a non-local (e.g. registry) ref. - assert!(!is_local_path("some-skill-name-that-does-not-exist")); - } - - // ── autodetect_source ───────────────────────────────────────────────────── - - #[test] - fn test_autodetect_source_registry() { - assert!(matches!( - autodetect_source("scope/skill@1.0.0"), - ResolvedSource::Registry(_) - )); - } - - #[test] - fn test_autodetect_source_git() { - assert!(matches!( - autodetect_source("https://github.com/org/repo.git"), - ResolvedSource::Git(_) - )); - } - - #[test] - fn test_autodetect_source_zip() { - assert!(matches!( - autodetect_source("./bundle.zip"), - ResolvedSource::Zip(_) - )); - } - - #[test] - fn test_autodetect_source_folder() { - assert!(matches!( - autodetect_source("./some/folder"), - ResolvedSource::Local(_) - )); - } - // ── find_skill_in_directory ─────────────────────────────────────────────── #[test] @@ -723,8 +567,13 @@ mod tests { versions: vec!["1.0.0".to_string()], zip_data: zip_bytes, }; + let origin = Origin::Repository { + repo: "default".to_string(), + skill: "scope/test-skill".to_string(), + version: None, + }; let (_temp_dir, skill_path, skill_def) = - download_registry_package(&client, "scope/test-skill", "1.0.0") + download_registry_package(&client, "scope/test-skill", "1.0.0", origin) .await .unwrap(); assert!(skill_path.join("SKILL.md").exists()); @@ -737,7 +586,12 @@ mod tests { versions: vec!["1.0.0".to_string()], zip_data: b"garbage".to_vec(), }; - let result = download_registry_package(&client, "scope/test-skill", "1.0.0").await; + let origin = Origin::Repository { + repo: "default".to_string(), + skill: "scope/test-skill".to_string(), + version: None, + }; + let result = download_registry_package(&client, "scope/test-skill", "1.0.0", origin).await; assert!(result.is_err()); } diff --git a/crates/fastskill-cli/src/commands/install.rs b/crates/fastskill-cli/src/commands/install.rs index e55e5cb8..61d6db7f 100644 --- a/crates/fastskill-cli/src/commands/install.rs +++ b/crates/fastskill-cli/src/commands/install.rs @@ -296,9 +296,7 @@ pub async fn execute_install(args: InstallArgs) -> CliResult<()> { .map(|locked| SkillInstallItem { entry: SkillEntry { id: locked.id, - source: locked.source, - version: locked.version, - editable: locked.editable, + origin: locked.origin, groups: locked.groups, }, depth: locked.depth, @@ -392,7 +390,6 @@ pub async fn execute_install(args: InstallArgs) -> CliResult<()> { installed_skills.push(( skill_def, item.entry.groups.clone(), - item.entry.editable, item.depth, item.parent_skill.clone(), )); @@ -419,7 +416,7 @@ pub async fn execute_install(args: InstallArgs) -> CliResult<()> { } // Update lock file with all installed skills including depth and parent info - for (skill_def, groups, _editable, depth, parent_skill) in installed_skills { + for (skill_def, groups, depth, parent_skill) in installed_skills { manifest_utils::update_lock_file_with_depth( &lock_path, &skill_def, @@ -613,7 +610,7 @@ Description: A test skill for coverage skills_directory = ".claude/skills" [dependencies] -test-skill = { path = "source-skill" } +test-skill = { origin = { type = "local", path = "source-skill" } } "#; fs::write(temp_dir.path().join("skill-project.toml"), manifest_content).unwrap(); diff --git a/crates/fastskill-cli/src/commands/list.rs b/crates/fastskill-cli/src/commands/list.rs index 5c4ab247..e44b8400 100644 --- a/crates/fastskill-cli/src/commands/list.rs +++ b/crates/fastskill-cli/src/commands/list.rs @@ -14,7 +14,8 @@ use cli_framework::spec::arg_spec::{ArgKind, ArgSpec, ArgValueType, Cardinality} use cli_framework::spec::command_tree::CommandSpec; use cli_framework::spec::value::ArgValue; use fastskill_core::core::lock::ProjectSkillsLock; -use fastskill_core::core::manifest::{SkillProjectToml, SkillSource}; +use fastskill_core::core::manifest::SkillProjectToml; +use fastskill_core::core::origin::Origin; use fastskill_core::core::project::resolve_project_file; use fastskill_core::core::service::FastSkillService; use fastskill_core::output::ListRow; @@ -111,20 +112,36 @@ impl FromArgValueMap for ListArgs { } } -/// Extract source path and type from SkillSource -fn format_source_info(source: &SkillSource) -> (Option, Option) { - match source { - SkillSource::Git { url, .. } => (Some(url.clone()), Some("git".to_string())), - SkillSource::Local { path, .. } => { - (Some(path.display().to_string()), Some("local".to_string())) - } - SkillSource::ZipUrl { base_url, .. } => { - (Some(base_url.clone()), Some("zip-url".to_string())) - } - SkillSource::Source { name, .. } => (Some(name.clone()), Some("source".to_string())), +/// Short origin-type label (git/local/zip-url/repository) for display. +/// Mirrors `fastskill_core::output::origin_type_label` (private to that crate). +fn origin_type_label(origin: &Origin) -> &'static str { + match origin { + Origin::Git { .. } => "git", + Origin::Local { .. } => "local", + Origin::ZipUrl { .. } => "zip-url", + Origin::Repository { .. } => "repository", + } +} + +/// Location string (URL/path/repo-skill) for display. +/// Mirrors `fastskill_core::output::origin_location_label` (private to that crate). +fn origin_location_label(origin: &Origin) -> String { + match origin { + Origin::Git { url, .. } => url.clone(), + Origin::Local { path, .. } => path.display().to_string(), + Origin::ZipUrl { url } => url.clone(), + Origin::Repository { repo, skill, .. } => format!("{repo}/{skill}"), } } +/// Extract source path and type from an `Origin`. +fn format_source_info(origin: &Origin) -> (Option, Option) { + ( + Some(origin_location_label(origin)), + Some(origin_type_label(origin).to_string()), + ) +} + /// Execute the list command pub async fn execute_list( service: &FastSkillService, @@ -165,13 +182,13 @@ pub async fn execute_list( }; // Build lock map with additional metadata - let lock_map: HashMap = lock + let lock_map: HashMap = lock .skills .iter() .map(|s| { ( s.id.clone(), - (s.version.clone(), s.name.clone(), s.source.clone()), + (s.resolved.version.clone(), s.name.clone(), s.origin.clone()), ) }) .collect(); @@ -232,22 +249,13 @@ pub async fn execute_list( // Get source path and type let (source_path, source_type) = if let Some(skill) = installed_map.get(&id) { - // For installed skills, use skill_file path and source_type + // For installed skills, use skill_file path and the origin's type label. let path = Some(skill.skill_file.display().to_string()); - let stype = skill.source_type.as_ref().map(|st| match st { - fastskill_core::core::skill_manager::SourceType::GitUrl => "git".to_string(), - fastskill_core::core::skill_manager::SourceType::LocalPath => { - "local".to_string() - } - fastskill_core::core::skill_manager::SourceType::ZipFile => { - "zip-url".to_string() - } - fastskill_core::core::skill_manager::SourceType::Source => "source".to_string(), - }); + let stype = Some(origin_type_label(&skill.origin).to_string()); (path, stype) - } else if let Some((_, _, source)) = lock_map.get(&id) { - // For lock-only skills, extract from SkillSource - format_source_info(source) + } else if let Some((_, _, origin)) = lock_map.get(&id) { + // For lock-only skills, extract from Origin + format_source_info(origin) } else { (None, None) }; diff --git a/crates/fastskill-cli/src/commands/read.rs b/crates/fastskill-cli/src/commands/read.rs index 8280c03c..7a599434 100644 --- a/crates/fastskill-cli/src/commands/read.rs +++ b/crates/fastskill-cli/src/commands/read.rs @@ -286,7 +286,8 @@ pub async fn execute_read(service: Arc, args: ReadArgs) -> Cli skill_id, entry.name.clone(), String::new(), - entry.version.clone(), + entry.resolved.version.clone(), + entry.origin.clone(), ) } else { resolve_skill(&service, &args.skill_id).await? @@ -324,7 +325,8 @@ pub async fn execute_read(service: Arc, args: ReadArgs) -> Cli skill_id, entry.name.clone(), String::new(), - entry.version.clone(), + entry.resolved.version.clone(), + entry.origin.clone(), ); if !entry.dependencies.is_empty() { sd.dependencies = Some(entry.dependencies.clone()); diff --git a/crates/fastskill-cli/src/commands/update.rs b/crates/fastskill-cli/src/commands/update.rs index 6fa40343..e7110726 100644 --- a/crates/fastskill-cli/src/commands/update.rs +++ b/crates/fastskill-cli/src/commands/update.rs @@ -246,7 +246,7 @@ async fn execute_update_global(args: UpdateArgs) -> CliResult<()> { println!("\nGlobal skills (check mode):\n"); for id in &skill_ids { if let Some(entry) = lock.skills.iter().find(|s| s.id == *id) { - println!(" • {} @ {}", entry.id, entry.version); + println!(" • {} @ {}", entry.id, entry.resolved.version); } } if args.check { @@ -403,7 +403,7 @@ async fn execute_update_project(args: UpdateArgs) -> CliResult<()> { } else { println!("\nSkills that would be updated:\n"); for entry in &entries { - println!(" • {} (from {:?})", entry.id, entry.source); + println!(" • {} (from {:?})", entry.id, entry.origin); } } if args.check { @@ -555,7 +555,7 @@ mod tests { fs::write( &skill_project_toml, r#"[dependencies] -test-skill = { source = "git", url = "https://example.com/repo.git" } +test-skill = { origin = { type = "git", url = "https://example.com/repo.git" } } "#, ) .expect("Failed to write skill-project.toml"); diff --git a/crates/fastskill-cli/src/utils/install_utils.rs b/crates/fastskill-cli/src/utils/install_utils.rs index f8930f09..bb72fa2c 100644 --- a/crates/fastskill-cli/src/utils/install_utils.rs +++ b/crates/fastskill-cli/src/utils/install_utils.rs @@ -2,11 +2,11 @@ use crate::error::{CliError, CliResult}; use chrono::Utc; +use fastskill_core::core::manifest::SkillEntry; +use fastskill_core::core::origin::{GitRef, Origin}; use fastskill_core::core::repository::RepositoryManager; +use fastskill_core::core::skill_manager::SkillUpdate; use fastskill_core::core::sources::SourcesManager; -use fastskill_core::core::{ - manifest::SkillEntry, manifest::SkillSource, skill_manager::SourceType, -}; use fastskill_core::{FastSkillService, SkillDefinition}; use std::path::{Path, PathBuf}; @@ -104,33 +104,35 @@ pub async fn install_skill_from_entry( entry: SkillEntry, sources_manager: Option<&SourcesManager>, ) -> CliResult { - match &entry.source { - SkillSource::Git { - url, - branch, - tag, - subdir, - } => { - install_from_git( - service, - url, - branch.as_deref(), - tag.as_deref(), - subdir.as_ref(), - ) - .await - } - SkillSource::Local { path, editable } => install_from_local(service, path, *editable).await, - SkillSource::ZipUrl { base_url, version } => { - install_from_zip_url(service, base_url, version.as_deref()).await + match &entry.origin { + Origin::Git { url, r#ref, subdir } => { + let (branch, tag) = match r#ref { + GitRef::Default => (None, None), + GitRef::Branch(b) => (Some(b.as_str()), None), + GitRef::Tag(t) => (None, Some(t.as_str())), + GitRef::Commit(_) => { + // No clone-by-commit primitive exists yet (`clone_repository` only + // takes branch/tag). Nothing in the current CLI ever *produces* a + // `GitRef::Commit` (no --commit flag), so this is unreachable via the + // supported flows; surface a clear error rather than mishandling it. + return Err(CliError::Config( + "Installing a skill pinned to a git commit is not yet supported" + .to_string(), + )); + } + }; + install_from_git(service, url, branch, tag, subdir.as_ref()).await } - SkillSource::Source { - name, + Origin::Local { path, editable } => install_from_local(service, path, *editable).await, + Origin::ZipUrl { url } => install_from_zip_url(service, url).await, + Origin::Repository { + repo, skill, version, } => { if let Some(sources_mgr) = sources_manager { - install_from_source(service, sources_mgr, name, skill, version.as_deref()).await + let version_str = version.as_ref().map(|v| v.to_string()); + install_from_source(service, sources_mgr, repo, skill, version_str.as_deref()).await } else { Err(CliError::Config( "Sources manager required for source-based installation".to_string(), @@ -205,7 +207,7 @@ async fn copy_skill_to_storage( async fn register_skill( service: &FastSkillService, skill_def: &SkillDefinition, - update: fastskill_core::core::skill_manager::SkillUpdate, + update: SkillUpdate, ) -> CliResult<()> { service .skill_manager() @@ -224,15 +226,36 @@ async fn register_skill( Ok(()) } -fn base_skill_update(def: &SkillDefinition) -> fastskill_core::core::skill_manager::SkillUpdate { - fastskill_core::core::skill_manager::SkillUpdate { - source_url: def.source_url.clone(), - source_type: def.source_type.clone(), +/// The `origin` (install intent) is the single source of truth for provenance now, +/// so every install path can share one `SkillUpdate` builder. +fn base_skill_update(def: &SkillDefinition) -> SkillUpdate { + SkillUpdate { + origin: Some(def.origin.clone()), fetched_at: def.fetched_at, ..Default::default() } } +/// Build the `Origin::Git` for a git-based install, mapping the mutually-exclusive +/// `--branch`/`--tag` flags onto `GitRef`. +fn build_git_origin( + url: &str, + branch: Option<&str>, + tag: Option<&str>, + subdir: Option<&PathBuf>, +) -> Origin { + let r#ref = match (branch, tag) { + (Some(b), _) => GitRef::Branch(b.to_string()), + (None, Some(t)) => GitRef::Tag(t.to_string()), + (None, None) => GitRef::Default, + }; + Origin::Git { + url: url.to_string(), + r#ref, + subdir: subdir.cloned(), + } +} + async fn install_from_git( service: &FastSkillService, url: &str, @@ -250,29 +273,14 @@ async fn install_from_git( }; let (_temp_dir, skill_path) = clone_and_find_skill(&config).await?; - let mut skill_def = create_skill_from_path(&skill_path, "git", false)?; + let origin = build_git_origin(url, branch, tag, subdir); + let mut skill_def = create_skill_from_path(&skill_path, origin, "git", false)?; copy_skill_to_storage(service, &skill_path, &mut skill_def).await?; - skill_def.source_url = Some(url.to_string()); - skill_def.source_type = Some(SourceType::GitUrl); - skill_def.source_branch = branch.map(|s| s.to_string()); - skill_def.source_tag = tag.map(|s| s.to_string()); - skill_def.source_subdir = subdir.cloned(); skill_def.fetched_at = Some(Utc::now()); - skill_def.editable = false; - - register_skill( - service, - &skill_def, - fastskill_core::core::skill_manager::SkillUpdate { - source_branch: skill_def.source_branch.clone(), - source_tag: skill_def.source_tag.clone(), - source_subdir: skill_def.source_subdir.clone(), - ..base_skill_update(&skill_def) - }, - ) - .await?; + + register_skill(service, &skill_def, base_skill_update(&skill_def)).await?; Ok(skill_def) } @@ -319,7 +327,11 @@ async fn install_from_local( ))); } - let mut skill_def = create_skill_from_path(&skill_path, "local", editable)?; + let origin = Origin::Local { + path: skill_path.clone(), + editable, + }; + let mut skill_def = create_skill_from_path(&skill_path, origin, "local", editable)?; let skill_storage_dir = service .config() .skill_storage_path @@ -328,20 +340,9 @@ async fn install_from_local( setup_skill_in_storage(&skill_path, &skill_storage_dir, editable).await?; skill_def.skill_file = skill_storage_dir.join("SKILL.md"); - skill_def.source_url = Some(skill_path.to_string_lossy().to_string()); - skill_def.source_type = Some(SourceType::LocalPath); skill_def.fetched_at = Some(Utc::now()); - skill_def.editable = editable; - - register_skill( - service, - &skill_def, - fastskill_core::core::skill_manager::SkillUpdate { - editable: Some(skill_def.editable), - ..base_skill_update(&skill_def) - }, - ) - .await?; + + register_skill(service, &skill_def, base_skill_update(&skill_def)).await?; Ok(skill_def) } @@ -403,30 +404,20 @@ async fn download_and_extract_zip(url: &str) -> CliResult<(tempfile::TempDir, Pa async fn install_from_zip_url( service: &FastSkillService, base_url: &str, - version: Option<&str>, ) -> CliResult { use crate::commands::add::create_skill_from_path; let (_temp_dir, skill_path) = download_and_extract_zip(base_url).await?; - let mut skill_def = create_skill_from_path(&skill_path, "zip", false)?; + let origin = Origin::ZipUrl { + url: base_url.to_string(), + }; + let mut skill_def = create_skill_from_path(&skill_path, origin, "zip", false)?; copy_skill_to_storage(service, &skill_path, &mut skill_def).await?; - skill_def.source_url = Some(base_url.to_string()); - skill_def.source_type = Some(SourceType::ZipFile); - skill_def.source_tag = version.map(|s| s.to_string()); skill_def.fetched_at = Some(Utc::now()); - skill_def.editable = false; - - register_skill( - service, - &skill_def, - fastskill_core::core::skill_manager::SkillUpdate { - source_tag: skill_def.source_tag.clone(), - ..base_skill_update(&skill_def) - }, - ) - .await?; + + register_skill(service, &skill_def, base_skill_update(&skill_def)).await?; Ok(skill_def) } @@ -460,11 +451,16 @@ async fn create_package_resolver() -> CliResult, ) -> CliResult { match source_config { fastskill_core::core::sources::SourceConfig::Git { @@ -477,7 +473,7 @@ async fn install_from_resolved_source( install_from_local(service, path, false).await } fastskill_core::core::sources::SourceConfig::ZipUrl { base_url, .. } => { - install_from_zip_url(service, base_url, version).await + install_from_zip_url(service, base_url).await } } } @@ -509,7 +505,7 @@ async fn install_from_source( CliError::Config(format!("Failed to resolve skill '{}': {}", skill_name, e)) })?; - install_from_resolved_source(service, &resolution.candidate.source_config, version).await + install_from_resolved_source(service, &resolution.candidate.source_config).await } /// Create SourcesManager from RepositoryManager for marketplace-based repositories @@ -649,19 +645,22 @@ mod tests { let entry = SkillEntry { id: "test-skill".to_string(), - source: SkillSource::Local { + origin: Origin::Local { path: src.clone(), editable: false, }, - version: "1.0.0".to_string(), groups: Vec::new(), - editable: false, }; let def = install_skill_from_entry(&service, entry, None) .await .unwrap(); - assert!(matches!(def.source_type, Some(SourceType::LocalPath))); - assert!(!def.editable); + assert!(matches!( + def.origin, + Origin::Local { + editable: false, + .. + } + )); assert!(def.skill_file.ends_with("SKILL.md")); assert!(def.skill_file.exists()); } @@ -676,18 +675,16 @@ mod tests { let entry = SkillEntry { id: "test-skill".to_string(), - source: SkillSource::Local { + origin: Origin::Local { path: src.clone(), editable: true, }, - version: "1.0.0".to_string(), groups: Vec::new(), - editable: true, }; let def = install_skill_from_entry(&service, entry, None) .await .unwrap(); - assert!(def.editable); + assert!(matches!(def.origin, Origin::Local { editable: true, .. })); let stored = storage.join(def.id.as_str()); assert!(stored.is_symlink(), "editable local install must symlink"); } @@ -699,19 +696,17 @@ mod tests { let service = make_service(&storage).await; let entry = SkillEntry { id: "missing".to_string(), - source: SkillSource::Local { + origin: Origin::Local { path: tmp.path().join("does-not-exist"), editable: false, }, - version: "1.0.0".to_string(), groups: Vec::new(), - editable: false, }; let result = install_skill_from_entry(&service, entry, None).await; assert!(matches!(result, Err(CliError::InvalidSource(_)))); } - // ── install_skill_from_entry: Source without a sources manager ───────────── + // ── install_skill_from_entry: Repository without a sources manager ───────── #[tokio::test] async fn test_install_skill_from_entry_source_requires_manager() { @@ -720,14 +715,12 @@ mod tests { let service = make_service(&storage).await; let entry = SkillEntry { id: "scope/skill".to_string(), - source: SkillSource::Source { - name: "myrepo".to_string(), + origin: Origin::Repository { + repo: "myrepo".to_string(), skill: "scope/skill".to_string(), version: None, }, - version: "1.0.0".to_string(), groups: Vec::new(), - editable: false, }; let result = install_skill_from_entry(&service, entry, None).await; assert!(matches!(result, Err(CliError::Config(_)))); @@ -752,21 +745,18 @@ mod tests { let storage = tmp.path().join("storage"); let service = make_service(&storage).await; + let zip_url = format!("{}/pkg.zip", server.uri()); let entry = SkillEntry { id: "test-skill".to_string(), - source: SkillSource::ZipUrl { - base_url: format!("{}/pkg.zip", server.uri()), - version: Some("1.0.0".to_string()), + origin: Origin::ZipUrl { + url: zip_url.clone(), }, - version: "1.0.0".to_string(), groups: Vec::new(), - editable: false, }; let def = install_skill_from_entry(&service, entry, None) .await .unwrap(); - assert!(matches!(def.source_type, Some(SourceType::ZipFile))); - assert_eq!(def.source_tag, Some("1.0.0".to_string())); + assert!(matches!(def.origin, Origin::ZipUrl { url } if url == zip_url)); assert!(def.skill_file.exists()); } diff --git a/crates/fastskill-cli/src/utils/manifest_utils.rs b/crates/fastskill-cli/src/utils/manifest_utils.rs index 18b37d71..fec7ee72 100644 --- a/crates/fastskill-cli/src/utils/manifest_utils.rs +++ b/crates/fastskill-cli/src/utils/manifest_utils.rs @@ -2,10 +2,8 @@ use fastskill_core::core::{ lock::{global_lock_path, GlobalSkillsLock, LockError, ProjectSkillsLock}, - manifest::{ - DependenciesSection, DependencySource, DependencySpec, SkillProjectToml, - SourceSpecificFields, - }, + manifest::{DependenciesSection, DependencySpec, SkillProjectToml}, + origin::Origin, project::resolve_project_file, skill_manager::SkillDefinition, }; @@ -156,7 +154,6 @@ pub fn remove_from_global_lock_file(skill_id: &str) -> Result<(), Box, - editable: bool, ) -> Result<(), Box> { // Resolve project file from current directory let current_dir = @@ -209,114 +206,31 @@ pub fn add_skill_to_project_toml( .as_mut() .ok_or_else(|| "Failed to initialize dependencies section".to_string())?; - // Convert SkillDefinition to DependencySpec - let dep_spec = if let Some(source_type) = &skill.source_type { - match source_type { - fastskill_core::core::skill_manager::SourceType::GitUrl => { - let source_specific = SourceSpecificFields { - url: skill.source_url.clone(), - branch: skill.source_branch.clone(), - path: None, - name: None, - skill: None, - zip_url: None, - version: None, - }; - DependencySpec::Inline { - source: DependencySource::Git, - source_specific, - groups: if groups.is_empty() { - None - } else { - Some(groups) - }, - editable: if editable { Some(true) } else { None }, - } - } - fastskill_core::core::skill_manager::SourceType::LocalPath => { - let path_buf = skill - .source_subdir - .clone() - .or_else(|| skill.source_url.clone().map(std::path::PathBuf::from)) - .unwrap_or_else(|| std::path::PathBuf::from(skill.id.as_str())); - - // Canonicalize to ensure absolute path (safety net) - let canonical_path = path_buf.canonicalize().map_err(|e| { - format!( - "Failed to canonicalize path '{}': {}", - path_buf.display(), - e - ) - })?; - - let path_str = canonical_path.to_string_lossy().to_string(); - - let source_specific = SourceSpecificFields { - url: None, - branch: None, - path: Some(path_str), - name: None, - skill: None, - zip_url: None, - version: None, - }; - DependencySpec::Inline { - source: DependencySource::Local, - source_specific, - groups: if groups.is_empty() { - None - } else { - Some(groups) - }, - editable: if editable { Some(true) } else { None }, - } - } - fastskill_core::core::skill_manager::SourceType::ZipFile => { - let source_specific = SourceSpecificFields { - url: None, - branch: None, - path: None, - name: None, - skill: None, - zip_url: skill.source_url.clone(), - version: Some(skill.version.clone()), - }; - DependencySpec::Inline { - source: DependencySource::ZipUrl, - source_specific, - groups: if groups.is_empty() { - None - } else { - Some(groups) - }, - editable: None, - } - } - fastskill_core::core::skill_manager::SourceType::Source => { - let source_specific = SourceSpecificFields { - url: None, - branch: None, - path: None, - name: skill.installed_from.clone(), - skill: Some(skill.id.to_string()), - zip_url: None, - version: Some(skill.version.clone()), - }; - DependencySpec::Inline { - source: DependencySource::Source, - source_specific, - groups: if groups.is_empty() { - None - } else { - Some(groups) - }, - editable: None, - } + // Convert SkillDefinition to DependencySpec. `Origin` is now the single source + // of truth for provenance (it already carries `editable` for `Origin::Local`), + // so this is a direct wrap rather than a per-source-type field remap. + let dep_groups = if groups.is_empty() { + None + } else { + Some(groups) + }; + let origin = match &skill.origin { + // Safety net: re-canonicalize a local path to an absolute path before + // persisting it, in case the caller passed a relative one. + Origin::Local { path, editable } => { + let canonical_path = path + .canonicalize() + .map_err(|e| format!("Failed to canonicalize path '{}': {}", path.display(), e))?; + Origin::Local { + path: canonical_path, + editable: *editable, } } - } else { - // Default: version string (source-based) - DependencySpec::Version(skill.version.clone()) + other => other.clone(), + }; + let dep_spec = DependencySpec::Inline { + origin, + groups: dep_groups, }; // Add or update dependency diff --git a/crates/fastskill-cli/tests/lock_determinism_test.rs b/crates/fastskill-cli/tests/lock_determinism_test.rs index e88ddd3e..8f52d79d 100644 --- a/crates/fastskill-cli/tests/lock_determinism_test.rs +++ b/crates/fastskill-cli/tests/lock_determinism_test.rs @@ -10,8 +10,9 @@ use chrono::Utc; use fastskill_core::core::lock::ProjectSkillsLock; +use fastskill_core::core::origin::{GitRef, Origin}; use fastskill_core::core::service::SkillId; -use fastskill_core::core::skill_manager::{SkillDefinition, SourceType}; +use fastskill_core::core::skill_manager::SkillDefinition; use std::fs; use tempfile::TempDir; @@ -31,15 +32,13 @@ fn make_skill(id: &str) -> SkillDefinition { execution_environment: None, dependencies: None, timeout: None, - source_url: Some("https://github.com/test/repo.git".to_string()), - source_type: Some(SourceType::GitUrl), - source_branch: Some("main".to_string()), - source_tag: None, - source_subdir: None, - installed_from: None, + origin: Origin::Git { + url: "https://github.com/test/repo.git".to_string(), + r#ref: GitRef::Branch("main".to_string()), + subdir: None, + }, commit_hash: Some("abc123".to_string()), fetched_at: Some(Utc::now()), - editable: false, } } diff --git a/crates/fastskill-cli/tests/lock_migration_test.rs b/crates/fastskill-cli/tests/lock_migration_test.rs index a0fd8bd0..b9592465 100644 --- a/crates/fastskill-cli/tests/lock_migration_test.rs +++ b/crates/fastskill-cli/tests/lock_migration_test.rs @@ -1,13 +1,19 @@ -#![allow(clippy::unwrap_used, clippy::expect_used)] +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] -//! Integration tests for v1.0.0 → v2.0 lock migration (RFC-056). +//! Integration tests for the lock format version gate (Phase 0 / Origin model). //! -//! Covers acceptance criteria: +//! Formerly (RFC-056) this file covered v1.0.0 → v2.0 migration: //! - AC 11: loading a v1.0.0 lock with `generated_at` / `fetched_at` and immediately -//! re-saving it MUST strip those fields. -//! - AC 18: v1.0.0 format files load without error (backward compat). +//! re-saving it stripped those fields. +//! - AC 18: v1.0.0 format files loaded without error (backward compat). +//! +//! The Origin/Resolved reshape (ADR-0005) removed the migrator entirely: any lock +//! whose `metadata.version` isn't the current `LOCK_FORMAT_VERSION` ("3.0") is now +//! rejected with an actionable `LockError::UnsupportedVersion` telling the caller to +//! delete the lock file and re-run `fastskill install`. These tests now assert that +//! rejection instead of a successful migration. -use fastskill_core::core::lock::ProjectSkillsLock; +use fastskill_core::core::lock::{LockError, ProjectSkillsLock, LOCK_FORMAT_VERSION}; use std::fs; use tempfile::TempDir; @@ -29,54 +35,38 @@ depth = 0 "#; #[test] -fn migration_strips_volatile_fields_on_resave() { - // AC 11: re-saving a v1 lock removes generated_at and fetched_at. +fn pre_origin_lock_is_rejected_not_migrated() { + // Superseded AC 11: there is no migrator anymore. A v1.0.0 lock (predating the + // Origin/Resolved reshape) must be rejected with an actionable error rather + // than silently loaded, stripped, and re-saved. let tmp = TempDir::new().unwrap(); let lock_path = tmp.path().join("skills.lock"); fs::write(&lock_path, V1_LOCK_FIXTURE).unwrap(); - let loaded = ProjectSkillsLock::load_from_file(&lock_path) - .expect("v1.0.0 lock must load (extra fields ignored via serde defaults)"); - assert_eq!(loaded.skills.len(), 1); - assert_eq!(loaded.skills[0].id, "legacy-skill"); - - loaded.save_to_file(&lock_path).unwrap(); - let migrated = fs::read_to_string(&lock_path).unwrap(); - - assert!( - !migrated.contains("generated_at"), - "generated_at must be stripped" - ); - assert!( - !migrated.contains("fetched_at"), - "fetched_at must be stripped" - ); - assert!( - migrated.contains("legacy-skill"), - "skill data must be preserved" - ); - assert!( - migrated.contains("fastskill_version"), - "fastskill_version must remain" - ); + let err = ProjectSkillsLock::load_from_file(&lock_path).unwrap_err(); + match err { + LockError::UnsupportedVersion { found } => assert_eq!(found, "1.0.0"), + other => panic!("expected UnsupportedVersion, got {other:?}"), + } } #[test] -fn v1_lock_loads_without_error() { - // AC 18: backward compat — v1.0.0 file loads cleanly. +fn v1_lock_is_rejected_before_touching_skill_shape() { + // Superseded AC 18: backward compat with pre-3.0 formats is intentionally + // dropped. The lightweight version pre-check must reject the file before any + // attempt to deserialize the (now-incompatible) `[[skills]]` shape. let tmp = TempDir::new().unwrap(); let lock_path = tmp.path().join("skills.lock"); fs::write(&lock_path, V1_LOCK_FIXTURE).unwrap(); - let lock = ProjectSkillsLock::load_from_file(&lock_path).unwrap(); - assert_eq!(lock.skills.len(), 1); - assert_eq!(lock.skills[0].id, "legacy-skill"); - assert_eq!(lock.skills[0].version, "1.0.0"); + let result = ProjectSkillsLock::load_from_file(&lock_path); + assert!(matches!(result, Err(LockError::UnsupportedVersion { .. }))); } #[test] fn project_skills_lock_round_trips() { - // Verify ProjectSkillsLock can be written and reloaded cleanly. + // Verify ProjectSkillsLock can be written and reloaded cleanly at the current + // format version (the Origin/Resolved reshape bumped this to "3.0"). let tmp = TempDir::new().unwrap(); let lock_path = tmp.path().join("skills.lock"); @@ -85,5 +75,5 @@ fn project_skills_lock_round_trips() { let loaded = ProjectSkillsLock::load_from_file(&lock_path).unwrap(); assert!(loaded.skills.is_empty()); - assert_eq!(loaded.metadata.version, "2.0"); + assert_eq!(loaded.metadata.version, LOCK_FORMAT_VERSION); } diff --git a/crates/fastskill-cli/tests/lock_scope_routing_test.rs b/crates/fastskill-cli/tests/lock_scope_routing_test.rs index 88e91120..a5537ac8 100644 --- a/crates/fastskill-cli/tests/lock_scope_routing_test.rs +++ b/crates/fastskill-cli/tests/lock_scope_routing_test.rs @@ -14,8 +14,9 @@ use chrono::Utc; use fastskill_core::core::lock::{GlobalSkillsLock, ProjectSkillsLock}; +use fastskill_core::core::origin::{GitRef, Origin}; use fastskill_core::core::service::SkillId; -use fastskill_core::core::skill_manager::{SkillDefinition, SourceType}; +use fastskill_core::core::skill_manager::SkillDefinition; use std::fs; use tempfile::TempDir; @@ -35,15 +36,13 @@ fn make_skill(id: &str) -> SkillDefinition { execution_environment: None, dependencies: None, timeout: None, - source_url: Some("https://github.com/test/repo.git".to_string()), - source_type: Some(SourceType::GitUrl), - source_branch: Some("main".to_string()), - source_tag: None, - source_subdir: None, - installed_from: None, + origin: Origin::Git { + url: "https://github.com/test/repo.git".to_string(), + r#ref: GitRef::Branch("main".to_string()), + subdir: None, + }, commit_hash: None, fetched_at: Some(Utc::now()), - editable: false, } } From 4b0bb630773a772264f03cdae000b7271c005792 Mon Sep 17 00:00:00 2001 From: Alex Oliveira <4482374+aroff@users.noreply.github.com> Date: Tue, 21 Jul 2026 21:49:11 +0000 Subject: [PATCH 7/7] feat(core): actionable hint when loading a pre-Origin manifest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pre-Origin `[dependencies]` entry (`source = "git"` + flat fields) no longer parses; append a migration hint pointing at the `origin = { type = … }` shape instead of surfacing a raw serde error. Completes the Phase-0 old-data guard (the lock already rejects pre-3.0 formats). Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/fastskill-core/src/core/manifest.rs | 38 ++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/crates/fastskill-core/src/core/manifest.rs b/crates/fastskill-core/src/core/manifest.rs index 3551bda0..bed530fd 100644 --- a/crates/fastskill-core/src/core/manifest.rs +++ b/crates/fastskill-core/src/core/manifest.rs @@ -397,10 +397,24 @@ impl SkillProjectToml { String::new() }; + // Pre-Origin dependency entries used `source = "git|local|zip-url|source"` + // with flat sibling fields; those keys no longer parse. Point the author + // at the new `origin = { type = … }` shape instead of a raw serde error. + let origin_hint = if content.contains("source = \"") { + "\n\nhint: this manifest looks like the pre-Origin `[dependencies]` format \ + (`source = \"git\"` + flat fields). Replace each dependency's `source`/flat \ + fields with `origin = { type = \"git|local|zip-url|repository\", … }`." + } else { + "" + }; + if !line_info.is_empty() { - ManifestError::Parse(format!("TOML syntax error at {}: {}", line_info, error_msg)) + ManifestError::Parse(format!( + "TOML syntax error at {}: {}{}", + line_info, error_msg, origin_hint + )) } else { - ManifestError::Parse(format!("TOML syntax error: {}", error_msg)) + ManifestError::Parse(format!("TOML syntax error: {}{}", error_msg, origin_hint)) } })?; @@ -595,6 +609,26 @@ mod tests { use super::*; use std::path::PathBuf; + #[test] + fn pre_origin_manifest_gives_actionable_hint() { + // A pre-Origin `[dependencies]` entry (`source = "git"` + flat fields) no + // longer parses; the error must point the author at the `origin = {…}` shape. + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("skill-project.toml"); + std::fs::write( + &path, + "[metadata]\nid = \"x\"\nversion = \"1.0.0\"\ndescription = \"d\"\n\n\ + [dependencies.old]\nsource = \"git\"\nurl = \"https://example.com/x.git\"\n", + ) + .unwrap(); + let err = SkillProjectToml::load_from_file(&path).unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains("origin = { type"), + "expected migration hint, got: {msg}" + ); + } + #[test] fn test_manifest_parsing() { let toml_content = r#"