diff --git a/.changepacks/changepack_log_skills_fetch_first.json b/.changepacks/changepack_log_skills_fetch_first.json new file mode 100644 index 0000000..933a8b1 --- /dev/null +++ b/.changepacks/changepack_log_skills_fetch_first.json @@ -0,0 +1,7 @@ +{ + "changes": { + "crates/devup-mcp/Cargo.toml": "Minor" + }, + "note": "devup_skills install now prefers each embedded skill's current upstream documents and falls back to the copy in the binary when fetching fails. A bare machine is the reason skill delivery exists, and is also where a download is least likely to work, so freshness cannot become an installation prerequisite. The CLI already had an ETag-aware fetcher; it now accepts a URL so the MCP installer can derive each document's address from its manifest repository and path instead of accidentally fetching devup-ui for every skill. The CLI still installs devup-ui to its existing HOME targets. MCP writes stay project-local and use the server's existing OutputPolicy and one OutputTransaction. A four-second budget covers the entire install's fetch work, rather than multiplying a timeout by the number of skills. One failed document falls back to the whole embedded skill, so an updated entry is not paired with old references. Network errors, HTTP errors and missing ETags report an embedded source and the fallback reason without warnings; a 404 additionally warns that the manifest path may have moved. Fetched files cannot claim the vendored commit: their entry note and response reuse sourceUrl, etag, fetchedAt and sha256 provenance, with the digest describing upstream bytes before annotation. The note stays after frontmatter so the installed skill still loads. Own skills still come only from the binary, and external skills are never fetched, written or installed by a shell command. DEVUP_MCP_SKILLS_OFFLINE is read once and bypasses the HTTP client. MCP integration tests set it in child processes before startup, while injected upstreams exercise fetched and failure paths without opening a socket. Status, resource reads and self-check cannot enter the fetch path.", + "date": "2026-09-14T13:26:37.677275+00:00" +} diff --git a/README.md b/README.md index 12e310a..cfee845 100644 --- a/README.md +++ b/README.md @@ -215,7 +215,7 @@ devup-mcp가 돌려주는 TSX는 **devup-ui 코드**입니다. devup-mcp만 깔 | 스킬 | 출처 | devup-mcp가 하는 일 | |---|---|---| -| `devup-ui` · `vespera` · `vespertide` | dev-five-git (우리 것) | **바이너리에 내장.** `{"action":"install"}`이 네트워크 없이 스킬 디렉터리에 씁니다 | +| `devup-ui` · `vespera` · `vespertide` | dev-five-git (우리 것) | **바이너리에 내장.** `{"action":"install"}`이 upstream 최신 문서를 우선 가져오고, 실패하면 내장본을 스킬 디렉터리에 씁니다 | | `vercel-react-best-practices` · `vercel-react-view-transitions` | vercel-labs/agent-skills | **내장하지 않음.** 설치 명령 `npx skills add vercel-labs/agent-skills`를 넘길 뿐, 실행하지 않습니다 | vercel 것을 내장하지 않는 이유는 두 가지입니다. **`vercel-labs/agent-skills`에는 LICENSE 파일이 없어** 재배포할 권리가 없고, 그 스킬들은 단일 파일이 아니라 `SKILL.md` + `AGENTS.md` + 규칙 파일 수십 개(합쳐 ~350 KB)라서 애초에 던져줄 물건이 아니라 설치할 물건입니다. @@ -224,6 +224,12 @@ vercel 것을 내장하지 않는 이유는 두 가지입니다. **`vercel-labs/ 설치 위치는 프로젝트 안입니다 — 이미 있는 것을 우선해 `.claude/skills`, `.opencode/skill`, `.agents/skills` 순으로 고릅니다. 프로젝트 루트는 devup-mcp가 쓸 수 있는 유일한 곳이라 새 권한이 필요 없고, 스킬이 저장소를 따라다닙니다. 이미 깔려 있으면 다시 쓰지 않습니다. +설치 응답의 `installed[].source`는 `fetched` 또는 `embedded`이며, 내장본을 썼다면 `reason`도 반환합니다. 가져오기는 manifest의 저장소와 문서 경로에서 만든 `https://raw.githubusercontent.com/REPO/HEAD/PATH`를 사용하며, 호출 전체의 네트워크 대기는 최대 4초입니다. 네트워크 오류, HTTP 오류, ETag 누락은 내장본으로 돌아가고, 404는 upstream 경로가 바뀌었을 수 있으므로 `warnings`에도 알립니다. 여러 문서 중 하나라도 실패하면 그 스킬 전체를 내장본으로 설치합니다. + +`DEVUP_MCP_SKILLS_OFFLINE=1`로 서버를 시작하면 가져오기를 생략합니다. 이 설정은 프로세스에서 한 번 읽으며 `0`, `false`, 빈 값은 가져오기를 허용합니다. `own` 스킬은 항상 바이너리에서, `external` 스킬은 여전히 설치 명령 안내만 합니다. `--self-check`와 스킬 resource 읽기, `status`는 가져오기를 실행하지 않습니다. 별도 CLI인 `--install-skills`의 devup-ui/HOME 설치 계약은 그대로입니다. + +가져온 문서의 주석은 고정 커밋을 주장하지 않고 URL, 가져온 시각(Unix seconds), ETag, 주석 추가 전 SHA-256을 기록합니다. 응답의 `installed[].documents[].provenance`도 같은 정보를 담습니다. 내장본을 설치하거나 resource로 읽을 때는 기존 커밋 정보를 유지합니다. + 내장본은 각 레포의 `SKILL.md`를 그대로 복사한 것이고, 응답과 설치된 파일 모두 **어느 커밋인지와 최신본 URL**을 함께 답니다. 사본은 낡습니다 — 그게 내장의 정직한 비용이고, `node scripts/refresh-skills.mjs`가 그걸 갱신하는 방법입니다(`--check`는 쓰지 않고 드리프트만 보고). 주석은 YAML frontmatter **뒤에** 들어갑니다. `---`는 0번째 바이트에 있어야 로더가 읽습니다. 설치하지 않고 읽기만 하려면 `devup://skill/devup-ui` 리소스도 있습니다. 다만 그건 fallback입니다 — 설치해야 로더가 알아서 꺼내 줍니다. diff --git a/crates/devup-mcp/src/server/mod.rs b/crates/devup-mcp/src/server/mod.rs index a575cdd..425cb05 100644 --- a/crates/devup-mcp/src/server/mod.rs +++ b/crates/devup-mcp/src/server/mod.rs @@ -913,7 +913,7 @@ impl DevupServer { description = "Report which agent skills the code devup-mcp emits depends on and whether this workspace has them, then install the ones devup-mcp carries (action: status | install). \ The TSX devup_figma_export returns is devup-ui code, and an agent that has never seen devup-ui does not know its components are compile-time placeholders, that $token means devup.json, or that a style prop takes a responsive array - it guesses, and this server cannot see the guesses. \ Call status before writing or editing that code. Anything reported missing is a gap you can close in one step. \ - install writes the vendored SKILL.md for devup-ui, vespera and vespertide into the workspace skill root (.claude/skills, .opencode/skill or .agents/skills - an existing one is preferred), with no network. Load them afterwards the way your runtime loads a project skill; an installed skill keeps applying to later sessions, which reading a document once does not. \ + install fetches current documents for embedded skills, falling back to the binary on failure, and writes own skills from the binary into the workspace skill root (.claude/skills, .opencode/skill or .agents/skills - an existing one is preferred), reporting source and any fallback reason (DEVUP_MCP_SKILLS_OFFLINE=1 disables fetching). Load them afterwards the way your runtime loads a project skill; an installed skill keeps applying to later sessions, which reading a document once does not. \ External skills are reported, never written: devup-mcp hands over its publisher's install command and does not run it.", output_schema = permissive_object_output_schema() )] @@ -926,8 +926,9 @@ impl DevupServer { self.output_policy.primary_root(), ))), "install" => { - let outcome = - skills::install(&self.output_policy, &input.names).map_err(to_mcp_error)?; + let outcome = skills::install(&self.output_policy, &input.names) + .await + .map_err(to_mcp_error)?; // The state after the write, from the same reader `status` // uses. An install that reports what it meant to do rather than // what is now on disk is the report that cannot be trusted. diff --git a/crates/devup-mcp/src/server/skills.rs b/crates/devup-mcp/src/server/skills.rs index 2ea09c9..42c3a9f 100644 --- a/crates/devup-mcp/src/server/skills.rs +++ b/crates/devup-mcp/src/server/skills.rs @@ -20,7 +20,8 @@ //! //! `embedded` skills are DevFive's own but live in another repository - //! devup-ui, vespera, vespertide. Their canonical `SKILL.md` is vendored into -//! the binary, so installing them needs no network. That matters because the +//! the binary as an offline fallback; install prefers current upstream documents. +//! The fallback matters because the //! situation this exists for, a bare machine, is the one in which a download is //! least likely to work. A vendored copy can fall behind its repository, so //! each one pins the commit it copied and `scripts/refresh-skills.mjs` is how @@ -47,8 +48,47 @@ use std::path::{Path, PathBuf}; use std::sync::LazyLock; +use crate::skills::{RawGithubSkillUpstream, SkillFetchError, SkillProvenance, SkillUpstream}; use serde::Deserialize; +const FETCH_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(4); +const OFFLINE_ENV: &str = "DEVUP_MCP_SKILLS_OFFLINE"; + +// Read once per process. Client construction and fetching happen only on an +// explicit install, never while constructing a server or running self-check. +static OFFLINE: LazyLock = LazyLock::new(|| { + std::env::var(OFFLINE_ENV).is_ok_and(|value| { + !matches!( + value.trim().to_ascii_lowercase().as_str(), + "" | "0" | "false" + ) + }) +}); + +struct InstallUpstream; + +#[async_trait::async_trait] +impl SkillUpstream for InstallUpstream { + async fn fetch( + &self, + source_url: &str, + ) -> Result { + if *OFFLINE { + return Err(SkillFetchError::Network(format!( + "{OFFLINE_ENV} disables fetching" + ))); + } + static UPSTREAM: LazyLock> = LazyLock::new(|| { + RawGithubSkillUpstream::with_timeout(FETCH_TIMEOUT).map_err(|error| error.to_string()) + }); + UPSTREAM + .as_ref() + .map_err(|error| SkillFetchError::Network(error.clone()))? + .fetch(source_url) + .await + } +} + pub const MIME_TYPE: &str = "text/markdown"; /// The registry. Parsed rather than duplicated into consts so that refreshing a @@ -336,7 +376,84 @@ pub fn installed_paths(project: &Path, name: &str) -> Vec { .collect() } +struct FetchedDocuments { + documents: Vec<(&'static str, String)>, + provenance: Vec, +} + impl Skill { + /// The manifest's path is either the entry file or the skill directory. + /// References live beside that entry, even for nested upstream skills. + fn upstream_url(&self, relative: &str) -> String { + let path = self.record.path.trim_end_matches('/'); + let directory = if path == ENTRY_DOCUMENT { + "" + } else { + path.strip_suffix("/SKILL.md").unwrap_or(path) + }; + let document = if directory.is_empty() { + relative.to_owned() + } else { + format!("{directory}/{relative}") + }; + format!( + "https://raw.githubusercontent.com/{}/HEAD/{document}", + self.record.repo + ) + } + + /// Resolve the complete set before staging any of it. One failed reference + /// must not leave a new entry document next to old embedded references. + async fn fetch_documents( + &self, + upstream: &dyn SkillUpstream, + deadline: tokio::time::Instant, + ) -> Result { + let mut documents = Vec::new(); + let mut provenance = Vec::new(); + for (relative, _) in self.texts.expect("only carried skills can be fetched") { + let url = self.upstream_url(relative); + let timeout_error = || { + SkillFetchError::Network( + "the four-second skill install fetch budget elapsed".to_owned(), + ) + }; + if tokio::time::Instant::now() >= deadline { + return Err((url, timeout_error())); + } + let fetched = tokio::time::timeout_at(deadline, upstream.fetch(&url)) + .await + .unwrap_or_else(|_| Err(timeout_error())) + .map_err(|error| (url.clone(), error))?; + let text = std::str::from_utf8(&fetched.contents).map_err(|error| { + ( + url.clone(), + SkillFetchError::Network(format!("upstream document is not UTF-8: {error}")), + ) + })?; + let record = SkillProvenance { + source_url: url, + etag: fetched.etag, + fetched_at: std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_secs(), + sha256: crate::skills::sha256(&fetched.contents), + }; + let contents = if *relative == ENTRY_DOCUMENT { + self.annotated_with(text, Some(&record)) + } else { + text.to_owned() + }; + documents.push((*relative, contents)); + provenance.push(serde_json::json!({"path": relative, "provenance": record})); + } + Ok(FetchedDocuments { + documents, + provenance, + }) + } + /// The provenance a reader needs alongside the text: which revision this /// is, and where the current one lives. Carried in the body rather than /// returned beside it, because the installed file outlives this response @@ -390,7 +507,11 @@ impl Skill { /// The document with its provenance note placed after any frontmatter. fn annotated(&self, text: &str) -> String { - let note = self.provenance_note(); + self.annotated_with(text, None) + } + + fn annotated_with(&self, text: &str, fetched: Option<&SkillProvenance>) -> String { + let note = self.provenance_note(fetched); match frontmatter_end(text) { Some(end) => format!("{}\n{note}\n{}", &text[..end], &text[end..]), None => format!("{note}\n\n{text}"), @@ -403,8 +524,18 @@ impl Skill { /// An `own` skill gets a different note, not a filled-in version of the /// vendored one: it has no upstream commit, and printing "at unknown" would /// read as a lost revision rather than as one that never existed. - fn provenance_note(&self) -> String { + fn provenance_note(&self, fetched: Option<&SkillProvenance>) -> String { let r = &self.record; + if let Some(p) = fetched { + return format!( + "", + p.source_url, + p.fetched_at, + p.etag.replace("--", "--"), + p.sha256, + r.used_for, + ); + } if r.origin == Origin::Own { return format!( "