diff --git a/.changepacks/changepack_log_echo_the_client_name.json b/.changepacks/changepack_log_echo_the_client_name.json new file mode 100644 index 00000000..27de5664 --- /dev/null +++ b/.changepacks/changepack_log_echo_the_client_name.json @@ -0,0 +1,7 @@ +{ + "changes": { + "crates/devup-mcp/Cargo.toml": "Patch" + }, + "note": "Half the runtime table is matched against client names observed in the wild rather than read out of source, because those clients are closed. A wrong guess there does not fail: the match simply never fires, the runtime reads `unknown`, every convention is written, and the result is safe and completely silent - indistinguishable from a client devup-mcp has genuinely never heard of. The report now echoes the name verbatim as `runtime.clientName` beside the verdict drawn from it, and when nothing matched it says what `unknown` means, that it is safe rather than wrong, and that a client with its own skill directory whose name is not in the table is exactly what should be reported. That is the only path by which the table gets corrected, and it is the same class of silent failure the rest of this branch exists to remove. `Lookup::new` takes the client name and derives the runtime from it rather than being handed a verdict, so the name and the conclusion cannot disagree.", + "date": "2026-09-22T15:31:59.219473100Z" +} \ No newline at end of file diff --git a/.changepacks/changepack_log_installed_stops_meaning_is_file.json b/.changepacks/changepack_log_installed_stops_meaning_is_file.json new file mode 100644 index 00000000..04f3f807 --- /dev/null +++ b/.changepacks/changepack_log_installed_stops_meaning_is_file.json @@ -0,0 +1,7 @@ +{ + "changes": { + "crates/devup-mcp/Cargo.toml": "Minor" + }, + "note": "`installed` meant `is_file()` and nothing more, so a `SKILL.md` written by a devup-mcp from six months ago was indistinguishable from the current document, and so was a one-line placeholder someone dropped in that directory. The practical sting was on the other side of the same check: `install` skipped anything already present, which meant there was no way to update a skill at all - the first install a machine ever did was the last one it would get, and every later call reported that stale copy as installed. Install state is now decided by comparing the bytes on disk against exactly what this build would write, and reported as `installState.revision`: `current` (identical, or fetched from upstream, which is at least as new as the vendored copy and cannot be compared further without the network), `older` (carries devup-mcp provenance but is not this build's), `incomplete` (the entry document is this build's but a file it links to is missing or differs - the shape that looks installed and whose links go nowhere), or `foreign` (no devup-mcp provenance note at all). An install now rewrites `older` and `incomplete` in place, at the roots where the stale copies actually are rather than where a fresh install would have chosen, and never touches `foreign`, because replacing a document this server did not write would destroy someone's work and an install is not the moment to decide that was a mistake; it is reported under `leftAlone` instead. `report` counts `outdated` apart from `missing` because the two need different words from the caller - one is a skill the agent has never seen, the other is one it is reading right now and being taught rules this binary no longer emits. The runtime table grew from three clients to twelve, with Cursor, Zed, Windsurf, VS Code/Copilot, Gemini CLI, Cline, Continue, Amp and Goose added from their own source or official documentation. Cline is the entry that justifies the table: it reads `.agents/skills` only from the home directory, so the shared project-local convention is one directory it never opens, and the previous fallback would have written exactly there. JetBrains AI Assistant is deliberately absent - it has no auto-discovered directory and a human must register one, so guessing a path would be worse than the fallback. An unidentified client now reads every convention devup-mcp knows while still writing only the three broad ones: reading narrowly was making a Cursor user who had installed once into `~/.cursor/skills` be told the skill was missing, with a second copy as the remedy. The gap moved into `devup_project_context` as well, scoped to what each scope actually found, because `devup.json` is devup-ui's theme format, the `openapi.json` under scope api is generated by vespera routes and the `models/*.json` under scope db are vespertide schemas, and an agent about to edit one of them had no way to learn it needed those rules. Three guide rules moved into `devup_figma_export`'s own description - screenshots verify rather than author, do not hand-interpret the node tree, never guess a UI value - because those three produce wrong code rather than a worse response when violated, and they lived only in a resource that Codex collapses into a single namespace description. `report` also hands over the machine-wide skill directory for this runtime under `machineWide`, as a path to run yourself: the home directory stays outside the allowed write root, and widening that so a design-to-code server can write to `$HOME` is not a trade worth making for convenience. Finally `autoUpdate.installedPathNote` explains that a version-named install directory belongs to the host and is fixed at install time, so a directory reading 0.9.0-dev around a binary reporting 0.10.1 is self-update working rather than a mismatch.", + "date": "2026-09-22T14:59:24.071914600Z" +} \ No newline at end of file diff --git a/.changepacks/changepack_log_skills_reach_the_runtime_that_asked.json b/.changepacks/changepack_log_skills_reach_the_runtime_that_asked.json new file mode 100644 index 00000000..0afa7424 --- /dev/null +++ b/.changepacks/changepack_log_skills_reach_the_runtime_that_asked.json @@ -0,0 +1,7 @@ +{ + "changes": { + "crates/devup-mcp/Cargo.toml": "Minor" + }, + "note": "A skill installed where the connected runtime never looks reports success, sits on disk, and still never loads, and that silent failure is what the reports from Codex were. Codex reads project skills from `.agents/skills` and does not look at `.claude/skills`, but a fresh project has no skill root at all, so the choice fell through to the first known convention - which is `.claude/skills` - and the agent was told devup-ui was installed while it went on writing devup-ui from guesses. opencode never reported it because opencode scans `.claude/skills` for compatibility and so happened to work. The runtime is now read from the MCP `clientInfo` name sent at initialize and decides the directory; a client that does not name itself has every convention written, because with no name and no existing root there is nothing to choose on and picking one of three blind is a two-in-three chance of writing where nothing looks, against a cost of a few kilobytes inside a directory the project already owns. Install state now also reads the machine-wide roots - `~/.claude/skills`, `~/.codex/skills`, `~/.config/opencode/skill`, `~/.agents/skills` - because a skill there really is loaded: a workspace holding devup-ui in all four, every one of them live, was answering `installedCount: 0`, which the README itself calls the noise that teaches a reader to ignore the field. Those roots are read and never written, so the allowed-write-root boundary is unchanged. The symmetric mistake is refused too, and it is the quieter one: a file in a directory this runtime never opens is not counted as installed. `devup_skills` takes `projectRoot` like every other tool, because without it the report described the server's own write root, which is not the project unless the two were configured alike - a host granting one shared parent, an Orca worktree pool or a monorepo checkout, had every call reporting on that parent and would have installed there. The inducement moved to where agents actually read. The strongest sentence used to live on `devup_skills`'s own description, which is the tool an agent has no reason to call; it is now in `devup_figma_export`'s description, the one channel that reaches an agent which never sees the server `instructions` - a subagent handed tool schemas alone, and Codex, where `instructions` becomes a single namespace description rather than prompt text. The same gap is on the export response as `skillGap`, and deliberately not gated on `projectRoot`, since the caller who does not know devup-ui is precisely the caller who does not send it; `devup_ui_validate` keeps its own copy for the case where the code was already written and refused. Install state is resolved through one `Lookup` carrying project, home and runtime rather than three threaded parameters, which also makes the home injectable - reading the real one had these assertions passing in CI and failing on the machine of anyone who has devup-ui installed in their own home, which is everyone working on this repository.", + "date": "2026-09-22T14:09:05.375113900Z" +} \ No newline at end of file diff --git a/README.md b/README.md index 7ef74d84..f591e712 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ Figma 쪽 4개, 프로젝트 쪽 5개, 스킬 1개, 모두 10개입니다. - `devup_figma_search`: page, section, frame, component를 이름으로 탐색. URL에 `node-id`가 있으면 **그 노드와 그 아래로 범위를 좁히고**, 없으면 파일 전체를 검색합니다. 둘 중 무엇을 했는지는 응답의 `scope`가 알려줍니다 - `devup_figma_explore`: 링크된 요구사항/라벨 주변의 실제 화면 후보를 공간 순서로 탐색 - `devup_figma_auth`: 연결 상태 확인, 브라우저 OAuth 로그인, 로그아웃, 사전 등록 자격증명 주입(`configure`), 연결 실패 원인을 실측해 보고하는 `doctor` -- `devup_skills`: devup-mcp가 내놓는 코드에 필요한 에이전트 스킬이 이 워크스페이스에 있는지 보고(`status`)하고, devup-mcp가 품고 있는 것을 설치(`install`). **텍스트를 응답에 실어 보내는 게 아니라 스킬 디렉터리에 설치해서 에이전트 자신의 로더가 읽게 합니다** — 한 번 읽은 문서는 한 번 쓰이지만, 설치된 스킬은 이후 모든 세션에 계속 적용됩니다 +- `devup_skills`: devup-mcp가 내놓는 코드에 필요한 에이전트 스킬이 이 워크스페이스에 있는지 보고(`status`)하고, devup-mcp가 품고 있는 것을 설치(`install`). **텍스트를 응답에 실어 보내는 게 아니라 스킬 디렉터리에 설치해서 에이전트 자신의 로더가 읽게 합니다** — 한 번 읽은 문서는 한 번 쓰이지만, 설치된 스킬은 이후 모든 세션에 계속 적용됩니다. 어느 디렉터리인지는 `clientInfo`로 판별한 런타임이 정하고, `projectRoot`로 어느 프로젝트인지 지정합니다 - `devup_project_context`: 프로젝트의 실제 `devup.json` 토큰, `openapi.json` 엔드포인트, Vespertide 모델을 읽음. 중첩 체크아웃과 빌드 산출물 디렉터리는 스캔에서 제외하고 무엇을 제외했는지 보고 - `devup_ui_validate`: 생성한 TSX를 프로젝트의 실제 `devup.json`에 대조해 검증. `ok`는 개수가 아니라 심각도로 판정 - `devup_stack_diff`: DB 모델부터 생성된 API 클라이언트까지의 층간 드리프트 탐지. 모든 발견은 명시적 `confidence`를 가짐 @@ -261,7 +261,68 @@ vercel 것을 내장하지 않는 이유는 두 가지입니다. **`vercel-labs/ **devup-mcp는 그 명령을 대신 실행하지 않습니다.** 디자인→코드 서버가 패키지 설치기를 실행하면, 레지스트리 항목 하나가 오염됐을 때 화면을 export한 모든 기계에서 임의 실행이 됩니다. -설치 위치는 프로젝트 안입니다 — 이미 있는 것을 우선해 `.claude/skills`, `.opencode/skill`, `.agents/skills` 순으로 고릅니다. 프로젝트 루트는 devup-mcp가 쓸 수 있는 유일한 곳이라 새 권한이 필요 없고, 스킬이 저장소를 따라다닙니다. 이미 깔려 있으면 다시 쓰지 않습니다. +설치 위치는 프로젝트 안입니다. 프로젝트 루트는 devup-mcp가 쓸 수 있는 유일한 곳이라 새 권한이 필요 없고, 스킬이 저장소를 따라다닙니다. 이미 깔려 있으면 다시 쓰지 않습니다. + +**어느 디렉터리인지는 붙어 있는 런타임이 정합니다.** 런타임마다 읽는 곳이 다르고, 읽지 않는 곳에 설치하는 것은 **조용한 실패**이기 때문입니다 — 호출은 성공을 보고하고, 파일은 디스크에 있고, 스킬은 끝내 로드되지 않습니다. Codex에서 올라온 보고가 정확히 이 모양이었습니다. 새 프로젝트에는 스킬 루트가 아예 없으니 선택이 목록의 첫 항목인 `.claude/skills`로 떨어졌고, Codex는 그곳을 읽지 않습니다. + +누구인지는 MCP `initialize`가 보내는 `clientInfo` 이름으로 압니다. + +| 런타임 | 설치 위치 | 머신 전체(읽기만) | +|---|---|---| +| Claude Code | `.claude/skills` | `~/.claude/skills` | +| **Codex** | **`.agents/skills`** | `~/.agents/skills`, `~/.codex/skills` | +| opencode | `.opencode/skill` | `~/.config/opencode/skill` | +| Cursor | `.cursor/skills` | `~/.cursor/skills` | +| Zed | `.agents/skills` | `~/.agents/skills` | +| Windsurf | `.windsurf/skills` | `~/.codeium/windsurf/skills` | +| VS Code / Copilot | `.github/skills` | `~/.copilot/skills` | +| Gemini CLI | `.gemini/skills` | `~/.gemini/skills` | +| Cline | `.cline/skills` | `~/.cline/skills`, `~/.agents/skills` | +| Continue | `.continue/skills` | `~/.continue/skills` | +| Amp | `.agents/skills` | `~/.config/agents/skills` | +| Goose | `.agents/skills` | `~/.agents/skills` | +| 이름을 안 밝힌 클라이언트 | **세 곳 모두** (`.claude/skills`·`.opencode/skill`·`.agents/skills`) | 전부 읽습니다 | + +각 런타임은 표의 한 곳만이 아니라 **자기가 읽는 곳 전부**를 검사합니다(대부분 `.agents/skills`를 호환으로 읽습니다). 표에 적은 것은 **새로 설치할 때 고르는 자리**입니다. + +**Cline이 이 표가 필요한 이유입니다.** 다른 대부분은 `.agents/skills`를 읽어서 공용 규약이 통하는데, Cline은 그걸 **홈에서만** 읽습니다. 프로젝트의 `.agents/skills`는 Cline이 열지 않는 디렉터리입니다. + +**JetBrains AI Assistant는 일부러 넣지 않았습니다.** 자동 탐색 디렉터리가 없고 사람이 설정에서 직접 등록해야 해서, 아무 경로나 넣는 것은 안전한 fallback보다 나쁩니다. + +**표의 절반은 관측값입니다.** Cursor·Windsurf·VS Code·Amp는 닫힌 소스라 `clientInfo.name`을 소스로 확인할 수 없습니다. 그 추측이 틀리면 매칭이 안 되고 `unknown`으로 떨어지는데 — **안전하지만 완전히 조용합니다.** devup-mcp가 정말 모르는 클라이언트와 구별이 안 됩니다. + +그래서 응답의 `runtime.clientName`이 **받은 이름을 그대로 돌려줍니다.** `detected`가 `unknown`이면 `ifDetectedIsUnknown`이 그게 틀린 게 아니라 안전한 상태라는 것과, 자기 스킬 디렉터리가 있는 클라이언트라면 그 이름을 알려달라는 것을 함께 답니다. 표가 고쳐지는 유일한 경로입니다. + +이름이 없고 기존 루트도 없으면 고를 근거가 없습니다. 셋 중 하나를 찍는 것은 **아무도 안 보는 곳에 쓸 확률이 3분의 2**이고, 사본 하나는 프로젝트가 이미 소유한 디렉터리 안에서 몇 KB입니다. 그래서 전부 씁니다. 다만 **읽을 때는 아는 규약 전부를 봅니다** — 누가 이미 깔아둔 것을 못 찾고 그 옆에 두 번째 사본을 쓰는 것이 더 나쁘기 때문입니다. + +### `installed`는 더 이상 "파일이 있다"가 아닙니다 + +예전에는 `is_file()` 하나로 판정해서, 반년 전 devup-mcp가 쓴 `SKILL.md`도, 누가 한 줄 적어둔 자리표시자도 현재 문서와 구별되지 않았습니다. 더 나빴던 건 **`install`이 이미 있으면 건너뛰어서 갱신할 방법이 아예 없었다**는 점입니다 — 기계가 처음 한 설치가 마지막 설치였습니다. + +지금은 디스크의 바이트를 이 빌드가 쓸 바이트와 대조해 `installState.revision`으로 답합니다. + +| revision | 뜻 | `install`이 하는 일 | +|---|---|---| +| `current` | 지금 설치하면 나올 것과 같음. 또는 upstream에서 받아온 것(내장본보다 최소한 새것이라 네트워크 없이 더 따질 수 없음) | 아무것도 안 함 | +| `older` | devup-mcp의 provenance 주석은 있는데 이 빌드의 것이 아님 | **제자리에서 다시 씁니다** | +| `incomplete` | `SKILL.md`는 이 빌드 것인데 링크한 문서가 없거나 다름 — 설치된 것처럼 보이고 링크는 끊긴 상태 | **다시 씁니다** | +| `foreign` | devup-mcp provenance 주석이 없음. 누가 직접 쓴 것 | **건드리지 않고 보고만 합니다** | + +`foreign`을 덮어쓰지 않는 이유는 하나입니다. 이 서버가 하지 않은 작업을 지우는 일이고, 설치는 그게 실수였는지 판단할 자리가 아닙니다. devup-mcp의 사본을 받고 싶으면 먼저 지우면 됩니다. + +`report`의 `outdated`가 이 상태인 스킬 이름을 모아 줍니다. `missing`과 따로 세는 이유는 둘이 다른 말을 필요로 하기 때문입니다 — 하나는 에이전트가 본 적 없는 스킬이고, 다른 하나는 **지금 읽고 있으면서 낡은 규칙을 배우고 있는** 스킬입니다. + +**머신 전체 루트는 읽기만 합니다.** devup-mcp는 허용된 write root 안에만 쓰고 홈 디렉터리는 거기에 없습니다 — 그 경계는 그대로입니다. 다만 `~/.codex/skills`에 있는 스킬은 **실제로 로드되므로** 없다고 보고하면 안 됩니다. 예전에는 devup-ui를 `~/.agents/skills`·`~/.claude/skills`·`~/.codex/skills`·`~/.config/opencode/skill` 네 곳에 (전부 각자의 런타임이 읽는 상태로) 갖고 있는 기계가 `installedCount: 0`을 받았습니다. + +반대 실수도 똑같이 조용합니다. 이 런타임이 열지 않는 디렉터리에 있는 파일은 로드되지 않으므로 `installed`로 세지 않습니다. + +**`projectRoot`를 넘기세요.** 생략하면 서버 자신의 write root로 떨어지는데, 둘을 같게 설정한 경우가 아니면 그건 프로젝트가 아닙니다. 공용 부모 하나를 write root로 준 호스트(Orca worktree 풀, 모노레포 체크아웃)에서는 모든 호출이 그 부모를 보고했고, 거기엔 어떤 런타임도 스킬을 찾으러 가지 않습니다. + +### 기계에 한 번만 깔고 싶다면 + +프로젝트마다 한 번씩 설치하는 것이 기본이고, 매 export가 빠진 것을 다시 알려주므로 스스로 복구됩니다. 그래도 기계 하나에 한 번이 낫다면 — devup-mcp는 **그 경로를 알려주기만 합니다.** 응답의 `machineWide`가 이 런타임의 홈 디렉터리와 함께 `action: "run-this-yourself"`를 돌려줍니다. + +홈은 허용된 write root 밖이고, 디자인→코드 서버가 `$HOME`에 쓸 수 있도록 그 경계를 넓히는 것은 편의와 바꿀 만한 거래가 아닙니다. 그래서 경로는 넘기고 실행은 사람 몫입니다. 그 디렉터리에 들어간 스킬은 이후 **모든 프로젝트에서** 설치됨으로 보고됩니다. 설치 응답의 `installed[].source`는 `fetched` 또는 `embedded`이며, 내장본을 썼다면 `reason`도 반환합니다. 가져오기는 manifest의 저장소와 문서 경로에서 만든 `https://raw.githubusercontent.com/REPO/HEAD/PATH`를 사용하며, 호출 전체의 네트워크 대기는 최대 4초입니다. 네트워크 오류, HTTP 오류, ETag 누락은 내장본으로 돌아가고, 404는 upstream 경로가 바뀌었을 수 있으므로 `warnings`에도 알립니다. 여러 문서 중 하나라도 실패하면 그 스킬 전체를 내장본으로 설치합니다. @@ -273,7 +334,14 @@ vercel 것을 내장하지 않는 이유는 두 가지입니다. **`vercel-labs/ 설치하지 않고 읽기만 하려면 `devup://skill/devup-ui` 리소스도 있습니다. 다만 그건 fallback입니다 — 설치해야 로더가 알아서 꺼내 줍니다. -유도는 두 곳에서만 합니다. 세션마다 실리는 `instructions`의 한 줄, 그리고 `devup_ui_validate`가 위반을 찾았는데 devup-ui 스킬이 **실제로 없을 때만** 붙는 `skillGap`입니다. 이미 깔려 있는 사람에게 깔라고 하는 것은 그 필드를 무시하게 만드는 소음입니다. +유도는 네 곳에서 합니다. 이미 깔려 있고 최신인 사람에게 깔라고 하는 것은 그 필드를 무시하게 만드는 소음이므로, 넷 다 **이 런타임이 실제로 로드하지 못하거나 낡은 것을 로드할 때만** 뜹니다. + +1. **`devup_figma_export`의 tool description.** 가장 중요한 자리입니다 — **모든 에이전트에게 도달하는 유일한 채널**이기 때문입니다. `instructions`는 그렇지 않습니다: `task()`로 띄운 서브에이전트는 tool schema만 받고 서버 `instructions`는 받지 못하며, Codex에서 `instructions`는 시스템 프롬프트 텍스트가 아니라 네임스페이스 설명 한 줄이 됩니다. 실제로 코드를 쓰는 주체가 그 두 경우인데, 예전에는 가장 강한 문장이 **부를 이유가 없는 도구인 `devup_skills` 자신의 description**에만 있었습니다. + + 같은 이유로 **가이드의 규칙 3·4·6도 이 description으로 옮겼습니다.** 스크린샷은 검증이지 저작이 아니라는 것, 노드 트리를 직접 읽어 손으로 쓰지 말라는 것, 값을 절대 지어내지 말라는 것 — 셋은 어기면 *응답이 나빠지는* 게 아니라 **잘못된 코드가 나오는** 규칙인데, Codex가 blurb 한 줄로 접어 버리는 리소스에만 있었습니다. +2. **`devup_figma_export` 응답의 `skillGap`.** TSX를 돌려준 응답에 붙습니다. `projectRoot`를 안 넘겨도 붙습니다 — devup-ui를 모르는 호출자가 바로 그 인자를 안 보내는 사람이라, 거기에 걸어두면 필요한 사람에게만 정확히 안 보입니다. `missing`과 `outdated`를 나눠 담습니다. +3. **`devup_project_context` 응답의 `skillGap`.** 2번이 devup-mcp가 *쓰는* 코드를 맡는다면 이쪽은 *읽는* 파일을 맡습니다 — `devup.json`은 devup-ui의 테마 형식이고, scope `api`의 `openapi.json`은 vespera 라우트가 만들며, scope `db`의 `models/*.json`은 vespertide 스키마입니다. **그 scope가 실제로 파일을 찾았을 때만** 뜹니다. `openapi.json`이 없는 프로젝트에 vespera를 권하는 건 소음입니다. +4. **`devup_ui_validate`의 `skillGap`.** 위반이 있을 때. 이건 부검입니다: devup-ui를 모르는 에이전트는 validator도 부르지 않으므로 이것만으로는 닿지 않습니다. 1~3번이 **코드를 쓰기 전**을 맡고, 4번은 이미 쓴 코드가 거절당한 순간을 맡습니다. ## Figma 연결 설정 diff --git a/crates/devup-mcp/src/server/export_skill_gap_tests.rs b/crates/devup-mcp/src/server/export_skill_gap_tests.rs new file mode 100644 index 00000000..352afc15 --- /dev/null +++ b/crates/devup-mcp/src/server/export_skill_gap_tests.rs @@ -0,0 +1,231 @@ +//! The export has to name the conventions its own output is written in. +//! +//! `devup_ui_validate` already did, but only for a caller who had written the +//! code and then chosen to validate it - and an agent that has never seen +//! devup-ui does neither. These lock the earlier moment. +//! +//! Its own file rather than a module in `mod.rs` because these tests touch the +//! filesystem, and `tests/module_boundaries.rs` holds the router to having no +//! `std::fs::` in it at all. + +use super::*; +fn scratch(label: &str) -> std::path::PathBuf { + let path = std::env::temp_dir().join(format!( + "devup-export-gap-{label}-{}-{}", + std::process::id(), + rand::random::() + )); + std::fs::create_dir_all(&path).unwrap(); + path +} + +/// The whole point: the gap arrives on the response that carries the code, +/// without the caller having asked anything of it. +#[test] +fn an_export_that_returns_tsx_names_the_skills_it_is_written_in() { + let project = scratch("tsx"); + let checked = with_project_checks( + json!({"status": "complete", "tsx": ""}), + project.to_str(), + &skills::Lookup::project_only(project.clone(), skills::Runtime::Codex), + ); + let gap = &checked["skillGap"]; + assert!(!gap.is_null(), "an export carrying TSX said nothing"); + assert_eq!(gap["runtime"], "codex"); + let named = gap["missing"] + .as_array() + .unwrap() + .iter() + .map(|entry| entry["skill"].as_str().unwrap().to_owned()) + .collect::>(); + assert!(named.contains(&"devup-ui".to_owned())); + assert!(named.contains(&"devfive-frontend".to_owned())); + // The one action that closes it, not advice. + assert_eq!(gap["missing"][0]["install"]["action"], "devup_skills"); + + let _ = std::fs::remove_dir_all(&project); +} + +/// Every Section workflow answers under `frames` and nothing at the top +/// level. Reading only the top level is how the theme check used to miss +/// them, and this must not repeat it. +#[test] +fn a_section_export_is_checked_through_its_frames() { + let project = scratch("frames"); + let checked = with_project_checks( + json!({"status": "complete", "frames": [{"nodeId": "1:2", "tsx": ""}]}), + project.to_str(), + &skills::Lookup::project_only(project.clone(), skills::Runtime::Codex), + ); + assert!(!checked["skillGap"].is_null(), "frames were not read"); + + let _ = std::fs::remove_dir_all(&project); +} + +/// The caller who does not know about devup-ui is exactly the one who +/// sends no `projectRoot`. Gating the gap on it would have hidden it from +/// everyone who needed it. +#[test] +fn the_gap_does_not_need_a_project_root() { + let project = scratch("no-root"); + let checked = with_project_checks( + json!({"status": "complete", "tsx": ""}), + None, + &skills::Lookup::project_only(project.clone(), skills::Runtime::Codex), + ); + assert!(!checked["skillGap"].is_null()); + assert_eq!( + checked["skillGap"]["workspace"], + project.display().to_string() + ); + + let _ = std::fs::remove_dir_all(&project); +} + +/// Telling someone to install what they already have is the noise that +/// teaches them to ignore the field. +#[test] +fn nothing_is_said_when_the_skills_are_already_there() { + let project = scratch("present"); + for name in CODE_SKILLS { + let path = skills::install_path(&project.join(".agents").join("skills"), name); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(&path, "# already here").unwrap(); + } + let checked = with_project_checks( + json!({"status": "complete", "tsx": ""}), + project.to_str(), + &skills::Lookup::project_only(project.clone(), skills::Runtime::Codex), + ); + assert!( + checked["skillGap"].is_null(), + "installed skills were reported as a gap" + ); + + let _ = std::fs::remove_dir_all(&project); +} + +/// An outdated skill is the case `installed: true` used to hide: it loads, it +/// is read, and it teaches rules this build no longer emits. It has to be said +/// apart from missing, because the two need different words. +#[test] +fn an_outdated_skill_is_reported_apart_from_a_missing_one() { + let project = scratch("outdated"); + let root = project.join(".agents").join("skills"); + // devup-ui installed as this build writes it; devfive-frontend from an + // older one, recognisable by its provenance note and nothing else. + for name in CODE_SKILLS { + let skill = skills::find_by_name(name).unwrap(); + for (relative, contents) in skill.installable_documents().unwrap() { + let path = relative + .split('/') + .fold(root.join(name), |path, part| path.join(part)); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(&path, contents).unwrap(); + } + } + let entry = skills::install_path(&root, "devfive-frontend"); + let body = std::fs::read_to_string(&entry).unwrap(); + std::fs::write(&entry, body.replace("Server Components", "an older build")).unwrap(); + + let checked = with_project_checks( + json!({"status": "complete", "tsx": ""}), + project.to_str(), + &skills::Lookup::project_only(project.clone(), skills::Runtime::Codex), + ); + let gap = &checked["skillGap"]; + assert!(!gap.is_null(), "a stale skill read as fine"); + assert!( + gap["missing"].as_array().unwrap().is_empty(), + "nothing is missing here: {gap}" + ); + assert_eq!(gap["outdated"][0]["skill"], "devfive-frontend"); + assert_eq!(gap["outdated"][0]["revision"], "older"); + assert_eq!(gap["outdated"][0]["install"]["action"], "devup_skills"); + + let _ = std::fs::remove_dir_all(&project); +} + +/// A response with no code in it has nothing to be written in. +#[test] +fn a_selection_response_carries_no_gap() { + let project = scratch("selection"); + let checked = with_project_checks( + json!({"status": "selection_required", "selection": {"count": 3}}), + project.to_str(), + &skills::Lookup::project_only(project.clone(), skills::Runtime::Codex), + ); + assert!(checked["skillGap"].is_null()); + + let _ = std::fs::remove_dir_all(&project); +} + +/// The export gap covers the code devup-mcp writes; this covers the files it +/// reads. Scoped, because a project with no openapi.json has no use for the +/// vespera skill and naming it there is the noise that teaches a reader to +/// skip the field. +#[test] +fn project_context_names_only_the_skills_for_what_that_scope_found() { + let project = scratch("context"); + let lookup = skills::Lookup::project_only(project.clone(), skills::Runtime::Codex); + + let api = with_context_skill_gap(json!({"found": true, "scope": "api"}), "api", &lookup); + let named = |gap: &Value| { + gap["missing"] + .as_array() + .unwrap() + .iter() + .map(|entry| entry["skill"].as_str().unwrap().to_owned()) + .collect::>() + }; + assert_eq!(named(&api["skillGap"]), vec!["vespera"]); + + let db = with_context_skill_gap(json!({"found": true, "scope": "db"}), "db", &lookup); + assert_eq!(named(&db["skillGap"]), vec!["vespertide"]); + + // Nothing found: nothing to say. vespera is irrelevant to a project with + // no openapi.json. + let empty = with_context_skill_gap(json!({"found": false}), "api", &lookup); + assert!(empty["skillGap"].is_null(), "{empty}"); + + // `all` nests one object per scope and each one answers for itself. + let all = with_context_skill_gap( + json!({"found": true, "theme": {"found": true}, "api": {"found": false}, "db": {"found": true}}), + "all", + &lookup, + ); + assert_eq!(named(&all["skillGap"]), vec!["devup-ui", "vespertide"]); + + let _ = std::fs::remove_dir_all(&project); +} + +/// The tool description is the only channel that reaches a subagent handed +/// tool schemas alone, or Codex, where server `instructions` becomes one +/// namespace blurb rather than prompt text. If the sentence is dropped +/// from there, the nudge is gone for both. +#[test] +fn the_export_tool_description_still_says_to_call_devup_skills() { + let tool = DevupServer::tool_router() + .list_all() + .into_iter() + .find(|tool| tool.name == "devup_figma_export") + .expect("devup_figma_export is registered"); + let description = tool.description.clone().unwrap_or_default(); + assert!( + description.contains("devup_skills"), + "the export description no longer names the tool that closes the gap" + ); + assert!( + description.contains("devup-ui"), + "the export description no longer says what the TSX is written in" + ); + // The three guide rules whose violation produces wrong code rather than a + // worse response. They lived only in a resource that Codex shows as a + // single namespace blurb, which is to say nowhere the model reads them. + for rule in ["screenshot", "Never guess", "node tree"] { + assert!( + description.contains(rule), + "the rule about {rule} is back to living only in the guide" + ); + } +} diff --git a/crates/devup-mcp/src/server/mod.rs b/crates/devup-mcp/src/server/mod.rs index fdad3c41..1434916e 100644 --- a/crates/devup-mcp/src/server/mod.rs +++ b/crates/devup-mcp/src/server/mod.rs @@ -296,9 +296,25 @@ pub struct DevupServer { artifacts: ArtifactStore, output_policy: OutputPolicy, asset_jobs: asset_jobs::AssetJobs, + /// The MCP client's own name, learned at `initialize` and kept because + /// the tool methods never see the request context. + /// + /// `Arc` so every clone of the server reads the one that was filled in: + /// a per-clone copy would be empty in whichever clone the handshake did + /// not run through, and the skill roots would silently go back to + /// guessing. + client_name: std::sync::Arc>, } impl DevupServer { + /// Which agent runtime is on the other end, for choosing skill roots. + /// + /// `Unknown` before the handshake or from a client that sends no name, + /// and that case installs every convention rather than picking one. + fn client_name(&self) -> Option<&str> { + self.client_name.get().map(String::as_str) + } + pub fn new(services: Services) -> Self { Self::with_output_roots( services, @@ -317,6 +333,7 @@ impl DevupServer { services, artifacts: ArtifactStore::default(), output_policy: OutputPolicy::from_roots(roots)?, + client_name: std::sync::Arc::new(std::sync::OnceLock::new()), }) } @@ -922,19 +939,22 @@ impl DevupServer { &self, Parameters(input): Parameters, ) -> Result { + let lookup = skills::Lookup::new( + input.project_root.as_deref(), + self.output_policy.primary_root(), + self.client_name(), + ); match input.action.as_str() { - "status" => Ok(tool_result(skills::report( - self.output_policy.primary_root(), - ))), + "status" => Ok(tool_result(skills::report(&lookup))), "install" => { - let outcome = skills::install(&self.output_policy, &input.names) + let outcome = skills::install(&self.output_policy, &lookup, &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. let mut result = outcome; - result["state"] = skills::report(self.output_policy.primary_root()); + result["state"] = skills::report(&lookup); Ok(tool_result(result)) } other => Err(to_mcp_error(DevupError::new( @@ -1121,6 +1141,8 @@ impl DevupServer { #[tool( description = "Export a small Figma selection; the Figma-to-code entry point. Asset requests: recommend 1–3 per call, maximum 6; split larger assetRequests before calling. All fresh exports return exportJob (assetJob compatibility alias) within a one-second initial wait when collection is still running, with per-call frame/root IDs, pagination and elapsed time. Slow asset calls also retain per-asset progress. Poll with jobId; use jobAction=resume when paused. Jobs retain accepted reads/bytes across client timeouts for 30 minutes in this server process, not across restart. Identical arguments recover a lost job reply. Completed results are retained for 5 minutes. Recommend 1–3 frames per call; allow at most 6 frames and 12 frame-times-output units. Budget roughly 5–20 seconds per frame-output unit (15–60 seconds per frame for three outputs) as a planning heuristic, not a guarantee: paging, complexity and throttling can exceed it and clients commonly time out at 300 seconds. Oversized selections are refused before screen collection; split frameIds into one-frame calls when isolating latency, or poll jobId. Partial per-frame projection failures retain successful frame outputs. projectionIssues always explains reported approximations, unclassified layout loss and missing generated-property provenance, even without includeDiagnostics. mappingComplete=false identifies mapping gaps; mapping-incomplete is not value loss and cannot be exact. projectionEvidence includes source fields and calculations for generated attributes. outputPathResults lists supported keys and diagnostics; frame file keys are frame::, while outputPaths reports actual committed writes. Resource responses offer nextAction.tool/arguments for same-artifact body comparison and sizeEstimate with explicit unmeasured wire overhead; coupled asset/path arguments remain together. quality.assets grades binary collection; assetSummary.description explains collection state. Merge saved batch responses offline with devup-mcp --merge-asset-batches batch1.json batch2.json for cumulative collection, unrequested, failed and conflict counts. SECTION links use two stages: receive selection_required, then run nextAction.example to export a selected screen. \ + The `tsx` this returns is devup-ui code, not plain React: its components are compile-time placeholders, `$token` names an entry in the project's devup.json, and a style prop takes a responsive array. Call devup_skills before you write or edit it - it reports which of those conventions this workspace is missing and installs them where your runtime loads skills from. An agent that skips this does not know it is guessing, and this server cannot see the guesses; the gap is repeated as `skillGap` on the response that carries the code. \ + This output is the design, already read. Do not rewrite it from a screenshot or from `get_design_context` - pictures and visual reasoning verify, they do not author. Do not read the node tree and write devup-ui by hand, and do not infer layout from coordinates. Never guess a colour, spacing, radius or typography value: if a value did not come back, say so and stop, because an invented one is indistinguishable from a real one in the code and is the single failure this server exists to prevent. A failed call is a fact to report, not something to route around. \ Ask only for what you will read: `tsx` is the deliverable, and the response always carries `status`, `quality`, `cache.artifactId`, `collection` and `source` beside it. \ `outputs` defaults to `[\"tsx\"]`. Add `devupJson` only when the project has no `devup.json` yet, or when you are introducing tokens it does not define - if it already has one, that file is what the code must match, and `devup_project_context` is what reads it. \ If you already know the node ids you want - a brief named them, or an earlier call did - pass them straight to `frameIds` and skip `devup_figma_explore`; exploring to rediscover ids you are already holding spends a Figma call for nothing. Explore is for when a Section link is all you have. \ @@ -1178,9 +1200,14 @@ impl DevupServer { .wait_briefly() .await .map(|result| { - tool_result(with_project_theme_validation( + tool_result(with_project_checks( result, project_root.as_deref(), + &skills::Lookup::new( + project_root.as_deref(), + self.output_policy.primary_root(), + self.client_name(), + ), )) }) .map_err(to_mcp_error); @@ -1417,9 +1444,14 @@ impl DevupServer { ) .await .map_err(to_mcp_error)?; - return Ok(tool_result(with_project_theme_validation( + return Ok(tool_result(with_project_checks( result, input.project_root.as_deref(), + &skills::Lookup::new( + input.project_root.as_deref(), + self.output_policy.primary_root(), + self.client_name(), + ), ))); } validate_artifact_projection( @@ -1456,9 +1488,14 @@ impl DevupServer { ) .await .map_err(to_mcp_error)?; - return Ok(tool_result(with_project_theme_validation( + return Ok(tool_result(with_project_checks( result, input.project_root.as_deref(), + &skills::Lookup::new( + input.project_root.as_deref(), + self.output_policy.primary_root(), + self.client_name(), + ), ))); } @@ -1522,9 +1559,14 @@ impl DevupServer { ) .await .map_err(to_mcp_error)?; - Ok(tool_result(with_project_theme_validation( + Ok(tool_result(with_project_checks( result, input.project_root.as_deref(), + &skills::Lookup::new( + input.project_root.as_deref(), + self.output_policy.primary_root(), + self.client_name(), + ), ))) } @@ -1543,7 +1585,15 @@ impl DevupServer { ) .await .map_err(to_mcp_error)?; - Ok(tool_result(result)) + Ok(tool_result(with_context_skill_gap( + result, + &input.scope, + &skills::Lookup::new( + input.project_root.as_deref(), + self.output_policy.primary_root(), + self.client_name(), + ), + ))) } #[tool( @@ -1669,9 +1719,13 @@ impl DevupServer { // rather than a nudge. Raised only when the skill is actually absent: // telling a caller who already has it to install it is the noise that // teaches them to skip the field. - let workspace = self.output_policy.primary_root(); + let lookup = skills::Lookup::new( + input.project_root.as_deref(), + self.output_policy.primary_root(), + self.client_name(), + ); if !report.violations.is_empty() - && skills::installed_paths(workspace, "devup-ui").is_empty() + && lookup.installed_paths("devup-ui").is_empty() && let Some(skill) = skills::find_by_name("devup-ui") { result["skillGap"] = json!({ @@ -1679,7 +1733,7 @@ impl DevupServer { "why": "This code broke devup-ui rules, and the devup-ui skill is not installed in \ this workspace. Installing it puts the rules in front of you while you \ write, instead of after this tool has already refused the result.", - "install": skill.install_action(workspace), + "install": skill.install_action(&lookup), }); } Ok(tool_result(result)) @@ -1732,6 +1786,128 @@ fn section_index_from_payload(payload: &CollectedPayload) -> Option Value { + if let Some(gap) = skill_gap(CODE_SKILLS.into_iter(), lookup, CODE_GAP_WHY) { + result["skillGap"] = gap; + } + result +} + +const CODE_GAP_WHY: &str = "This response carries devup-ui code, and the conventions it is written in are not what \ + this runtime would load. Without them the code is written from guesses - invented \ + $tokens, an authored CSS file, components treated as runtime React - and this server can \ + neither see nor correct that."; + +/// The gap between the skills a response's subject needs and what this runtime +/// would actually load, or `None` when there is none. +/// +/// Missing and outdated are reported apart because they need different words +/// from the caller. One is a skill the agent has never seen. The other is one +/// it is reading right now, and being taught rules this build no longer emits - +/// which reads exactly like a correctly installed skill until someone compares +/// the bytes. +fn skill_gap<'a>( + names: impl Iterator, + lookup: &skills::Lookup, + why: &str, +) -> Option { + let mut missing = Vec::new(); + let mut outdated = Vec::new(); + for skill in names.filter_map(skills::find_by_name) { + let entry = json!({ + "skill": skill.record.name, + "whyYouNeedIt": skill.record.used_for, + "install": skill.install_action(lookup), + }); + match skill.installed_freshness(lookup) { + None => missing.push(entry), + Some(state) if state.should_refresh() => { + let mut entry = entry; + entry["revision"] = json!(state.as_str()); + outdated.push(entry); + } + Some(_) => {} + } + } + if missing.is_empty() && outdated.is_empty() { + return None; + } + Some(json!({ + "missing": missing, + "outdated": outdated, + "why": why, + "how": "Call devup_skills with action \"install\" and this same projectRoot, then load \ + the skills the way your runtime loads a project skill. Before writing the code, \ + not after. The same call rewrites an outdated copy devup-mcp wrote and leaves a \ + document it did not write alone.", + "runtime": lookup.runtime.as_str(), + "workspace": lookup.project.display().to_string(), + })) +} + +/// Names the conventions for the files this scope just read, when this runtime +/// would not load them. +/// +/// The export gap covers the code devup-mcp writes; this covers the files it +/// reads. `devup.json` is devup-ui's theme format, the `openapi.json` under +/// scope api is generated by vespera routes, and the `models/*.json` under +/// scope db are vespertide schemas - so an agent about to edit any of them +/// needs the same rules, and had no way to learn it needed them. +/// +/// Gated on the scope having found something. A project with no `openapi.json` +/// has no use for the vespera skill, and naming it there is the noise that +/// teaches a reader to skip the field. +fn with_context_skill_gap(mut result: Value, scope: &str, lookup: &skills::Lookup) -> Value { + let found = |key: &str| { + // `all` nests one object per scope; a single scope answers at the top + // level. Both say `found`. + result + .get(key) + .unwrap_or(&result) + .get("found") + .and_then(Value::as_bool) + .unwrap_or(false) + }; + let mut names: Vec<&str> = Vec::new(); + if matches!(scope, "theme" | "all") && found("theme") { + names.push("devup-ui"); + } + if matches!(scope, "api" | "all") && found("api") { + names.push("vespera"); + } + if matches!(scope, "db" | "all") && found("db") { + names.push("vespertide"); + } + if scope == "ui" && found("ui") { + names.extend(CODE_SKILLS); + } + if let Some(gap) = skill_gap( + names.into_iter(), + lookup, + "These are the conventions for the files this scope just read, and they are not what \ + this runtime would load. Without them an agent edits a devup.json, an openapi.json or \ + a Vespertide model by guessing at its shape.", + ) { + result["skillGap"] = gap; + } + result +} + /// Says whether the TSX just generated fits the project it is going into. /// /// Tokens are named after the Figma variables and text styles the screen @@ -1750,11 +1926,14 @@ fn section_index_from_payload(payload: &CollectedPayload) -> Option) -> Value { - let Some(project_root) = project_root else { - return result; - }; +/// Without a root the theme half is skipped and nothing is read from disk, but +/// the skill half still runs: see [`with_skill_gap`] for why it cannot be +/// gated on an argument the unaware caller is the one least likely to send. +fn with_project_checks( + mut result: Value, + project_root: Option<&str>, + lookup: &skills::Lookup, +) -> Value { const OUTPUTS: [&str; 3] = ["tsx", "componentTsx", "responsiveTsx"]; let mut generated: Vec<(String, String)> = OUTPUTS .into_iter() @@ -1784,6 +1963,16 @@ fn with_project_theme_validation(mut result: Value, project_root: Option<&str>) } } } + // Before the theme half, and before its `project_root` gate: a response + // that handed back code has to name the conventions that code is written + // in whether or not the caller told us where the project is. + let produced_code = !generated.is_empty() || result.get("deliverable").is_some(); + if produced_code { + result = with_skill_gap(result, lookup); + } + let Some(project_root) = project_root else { + return result; + }; if generated.is_empty() { // Resource delivery hands the code back as a link, so there is no // source here to check. Saying so is the point: an absent verdict @@ -2009,6 +2198,15 @@ impl ServerHandler for DevupServer { request: rmcp::model::CallToolRequestParams, context: RequestContext, ) -> Result { + // Read here rather than by overriding `initialize`, because the peer + // already recorded it there and this is the first place the value is + // needed. `set` after the first call is a no-op, so one session keeps + // one answer. + if self.client_name.get().is_none() + && let Some(peer) = context.peer.peer_info() + { + let _ = self.client_name.set(peer.client_info.name.clone()); + } let call = rmcp::handler::server::tool::ToolCallContext::new(self, request, context); let mut response = match self.tool_router.call(call).await { Ok(response) => response, @@ -2092,6 +2290,10 @@ impl ServerHandler for DevupServer { } } +#[cfg(test)] +#[path = "export_skill_gap_tests.rs"] +mod export_skill_gap_tests; + #[cfg(test)] mod p3_error_tests { use super::*; diff --git a/crates/devup-mcp/src/server/self_update.rs b/crates/devup-mcp/src/server/self_update.rs index 06d045e8..9b3f87e1 100644 --- a/crates/devup-mcp/src/server/self_update.rs +++ b/crates/devup-mcp/src/server/self_update.rs @@ -418,6 +418,16 @@ pub fn report() -> Value { // to left the reader to find it, and on this machine three different // devup-mcp binaries were installed at once. "installedPath": installed_path(), + // A host that unpacks each install into a versioned directory names it + // once, at install time, and never renames it - so the directory can + // say 0.9.0-dev while the binary inside it reports 0.10.1 after a + // self-update replaced the file in place. That is the mechanism + // working, not a mismatch, and it reads like one. + "installedPathNote": "The path of the file, not a statement about the version. A \ + directory named after a version is the host's, fixed when it \ + installed; self-update replaces the file inside it and never \ + renames the directory. Read server.commit/buildId for what is \ + actually running.", "note": ownership.note(), "disableWith": DISABLE_ENV, }) diff --git a/crates/devup-mcp/src/server/skills.rs b/crates/devup-mcp/src/server/skills.rs index cc33e1cc..d25426c0 100644 --- a/crates/devup-mcp/src/server/skills.rs +++ b/crates/devup-mcp/src/server/skills.rs @@ -154,6 +154,59 @@ const EMBEDDED: &[(&str, Documents)] = &[ /// file is on disk; the rest are what it links to. pub const ENTRY_DOCUMENT: &str = "SKILL.md"; +/// How each provenance note opens. Named so the writer and the reader cannot +/// drift apart: [`Skill::provenance_note`] builds them and +/// [`Skill::freshness_at`] recognises a file by them. +const FETCHED_MARKER: &str = "", + "{}{} at {} (Unix seconds).\n ETag: {}\n SHA-256 of upstream bytes before annotation: {}\n Why this matters here: {} -->", + FETCHED_MARKER, p.source_url, p.fetched_at, p.etag.replace("--", "--"), @@ -542,11 +1006,12 @@ impl Skill { } if r.origin == Origin::Own { return format!( - "", + marker = AUTHORED_MARKER, repo = r.repo, path = r.path, latest = r.latest_url, @@ -554,11 +1019,12 @@ impl Skill { ); } format!( - "", + marker = VENDORED_MARKER, repo = r.repo, path = r.path, commit = r.commit.as_deref().unwrap_or("unknown"), @@ -571,33 +1037,81 @@ impl Skill { /// The one action that closes the gap, in the imperative, with everything /// needed to carry it out. - pub fn install_action(&self, project: &Path) -> serde_json::Value { - let installed = installed_paths(project, &self.record.name); + /// The freshness of the worst copy this runtime would load, or `None` when + /// there is no copy at all. + /// + /// The worst one decides. A machine with a current copy in one root and a + /// stale copy in another is loading both, and the stale one is the one + /// that will be wrong. + pub fn installed_freshness(&self, lookup: &Lookup) -> Option { + lookup + .installed_paths(&self.record.name) + .iter() + .map(|path| self.freshness_at(path)) + .max_by_key(|state| match state { + Freshness::Current => 0, + Freshness::Older => 1, + Freshness::Incomplete => 2, + Freshness::Foreign => 3, + }) + } + + pub fn install_action(&self, lookup: &Lookup) -> serde_json::Value { + let installed = lookup.installed_paths(&self.record.name); if !installed.is_empty() { let paths = installed .iter() .map(|path| path.display().to_string()) .collect::>(); + let revision = self + .installed_freshness(lookup) + .unwrap_or(Freshness::Current); + if revision.should_refresh() { + return serde_json::json!({ + "installed": true, + "paths": paths, + "revision": revision.as_str(), + "action": "devup_skills", + "arguments": {"action": "install", "names": [self.record.name]}, + "why": if revision == Freshness::Incomplete { + "The SKILL.md on disk is this build's, but a document it links to is \ + missing or has different bytes. It looks installed and its links go \ + nowhere." + } else { + "The copy on disk was written by a different devup-mcp build. It still \ + loads, and it describes conventions this binary no longer emits." + }, + "how": "Call devup_skills with action \"install\". A copy devup-mcp wrote is \ + rewritten in place; nothing else is touched.", + }); + } return serde_json::json!({ "installed": true, "paths": paths, + "revision": revision.as_str(), "action": null, - "how": "Already installed. Your skill loader picks it up by its own triggers; \ - nothing to do.", + "how": if revision == Freshness::Foreign { + "Installed, but this document is not one devup-mcp wrote - no provenance \ + note. It is left exactly as it is, because replacing it would destroy work \ + this server did not do. Delete it first if you want devup-mcp's copy." + } else { + "Already installed and current. Your skill loader picks it up by its own \ + triggers; nothing to do." + }, }); } match self.record.origin { Origin::Embedded | Origin::Own => { - let roots = existing_roots(project); - let target = target_root(project); - let writes = self - .record - .documents + let roots = lookup.existing_roots(); + let targets = lookup.target_roots(); + let writes = targets .iter() - .map(|document| { - document_path(&target, &self.record.name, &document.path) - .display() - .to_string() + .flat_map(|target| { + self.record.documents.iter().map(move |document| { + document_path(target, &self.record.name, &document.path) + .display() + .to_string() + }) }) .collect::>(); serde_json::json!({ @@ -608,10 +1122,21 @@ impl Skill { "writesTo": writes, "how": "Call devup_skills with action \"install\". Embedded skills prefer current upstream documents with offline fallback; own skills use the binary. Then load it the way your runtime loads \ a project skill.", - "rootChoice": if roots.is_empty() { - format!("No skill root exists yet, so {} is created.", SKILL_ROOTS[0]) + "runtime": lookup.runtime.as_str(), + "rootChoice": if !roots.is_empty() { + format!("Using the existing root {}.", targets[0].display()) + } else if lookup.runtime == Runtime::Unknown { + "No skill root exists yet and the MCP client did not identify itself, \ + so every known convention is written. One of them is the one your \ + runtime reads." + .to_owned() } else { - format!("Using the existing root {}.", target.display()) + format!( + "No skill root exists yet, so {} is created - the convention {} \ + loads from.", + targets[0].display(), + lookup.runtime.as_str() + ) }, }) } @@ -634,7 +1159,7 @@ impl Skill { /// The shape is deliberately the same for `status` and after `install`, so the /// second call is how the agent confirms the first one worked rather than /// something it has to take on trust. -pub fn report(project: &Path) -> serde_json::Value { +pub fn report(lookup: &Lookup) -> serde_json::Value { let entries = all() .iter() .map(|skill| { @@ -683,7 +1208,7 @@ pub fn report(project: &Path) -> serde_json::Value { if r.origin == Origin::External { entry["license"] = serde_json::json!(r.license); } - let action = skill.install_action(project); + let action = skill.install_action(lookup); entry["installed"] = action["installed"].clone(); entry["installState"] = action; entry @@ -693,31 +1218,125 @@ pub fn report(project: &Path) -> serde_json::Value { .iter() .filter(|entry| entry["installed"] == false) .count(); + // Counted apart from `missing`, because the two need different words from + // the caller: one is a skill the agent has never seen, the other is one it + // is reading right now and getting outdated rules from. + let stale = entries + .iter() + .filter(|entry| { + entry["installState"]["action"] == "devup_skills" && entry["installed"] == true + }) + .map(|entry| entry["name"].clone()) + .collect::>(); let mut report = serde_json::json!({ - "workspace": project.display().to_string(), + "workspace": lookup.project.display().to_string(), + "runtime": { + "detected": lookup.runtime.as_str(), + "clientName": lookup.client_name, + "from": "The MCP clientInfo name sent at initialize, echoed here as `clientName`.", + "why": "Runtimes do not read the same directory. Installing into one this runtime \ + never opens reports success and still never loads, which is the failure this \ + field exists to make visible.", + // A closed-source client's name is matched against what has been + // observed rather than read out of source, so a wrong guess does + // not fail - it just never fires, and reads exactly like a client + // devup-mcp has never heard of. Naming both halves is what lets a + // reader tell those two apart and say which it is. + "ifDetectedIsUnknown": if lookup.runtime == Runtime::Unknown { + serde_json::json!({ + "meaning": "No runtime matched this client name, so every convention is read \ + and the three broad ones are written. That is safe, not wrong.", + "butAlso": "It looks identical to a client devup-mcp does know whose name has \ + since changed. If `clientName` above is a client with its own \ + skill directory, that directory is missing from devup-mcp's table \ + and reporting the name is how it gets added.", + "reportAt": "https://github.com/dev-five-git/devup-mcp/issues", + }) + } else { + serde_json::Value::Null + }, + }, "skillRoots": { - "known": SKILL_ROOTS, - "existing": existing_roots(project) + "known": lookup.runtime.project_roots(), + "existing": lookup.existing_roots() .iter() .map(|path| path.display().to_string()) .collect::>(), - "wouldUse": target_root(project).display().to_string(), + "wouldUse": lookup.target_roots() + .iter() + .map(|path| path.display().to_string()) + .collect::>(), + "knownUser": lookup.runtime.user_roots(), + "existingUser": lookup.user_roots() + .iter() + .map(|path| path.display().to_string()) + .collect::>(), + "userRootsNote": "Machine-wide roots are read to decide `installed`, never written: \ + devup-mcp writes only inside an allowed output root. A skill found \ + in one of these is already loaded by its runtime and needs nothing.", + "writeConventions": lookup.runtime.write_roots(), + "allKnownRoots": EVERY_PROJECT_ROOT, + "scopeNote": "`known` is every root this runtime *reads*, so an install someone else \ + made is found rather than duplicated. `writeConventions` is the \ + narrower list an install may create, and `wouldUse` is what this call \ + would actually write. For an unidentified client the first is every \ + convention and the second is the three broad ones, because writing a \ + copy into each of a dozen editor directories is worse than the problem \ + it solves.", }, "installedCount": entries.len() - missing, "missingCount": missing, + "outdated": stale, + "outdatedNote": "Installed, loading, and not what this build writes. `installed` means the \ + file is there; `installState.revision` says whether it is current, and a \ + skill listed here is teaching an agent rules this binary no longer emits. \ + Installing again rewrites devup-mcp's own copies and leaves a `foreign` \ + one - a document with no devup-mcp provenance note - alone.", "skills": entries, + "machineWide": machine_wide(lookup), "how": "These are the conventions for the code devup-mcp emits and reads. Install the \ missing ones, then let your own skill loader surface them - an installed skill \ keeps applying for every later session, which is the thing reading a document \ once does not do.", "boundary": "devup-mcp installs only carried skills: embedded skills prefer current upstream documents with offline fallback, and own skills use the binary. External skills are never fetched or written; their install command is yours to run.", }); - if let Some(obligation) = changepacks_obligation(project) { + if let Some(obligation) = changepacks_obligation(&lookup.project) { report["repoObligations"] = serde_json::json!({ "changepacks": obligation }); } report } +/// Where a once-per-machine install would go, and why devup-mcp does not do it. +/// +/// Installing per project is correct and self-healing - every export names the +/// gap again - but it is one install per repository, and someone setting up a +/// machine usually wants one install for all of them. The home directory is +/// outside the allowed write root by design, and widening that so a +/// design-to-code server can write to `$HOME` is not a trade worth making for +/// convenience. So the path is handed over and the person runs it. +fn machine_wide(lookup: &Lookup) -> serde_json::Value { + let Some(home) = &lookup.home else { + return serde_json::json!({ + "available": false, + "why": "No home directory is set for this process, so there is nowhere to name.", + }); + }; + let directory = join_root(home, lookup.runtime.user_roots()[0]); + serde_json::json!({ + "available": true, + "directory": display_path(&directory), + "appliesTo": "Every project on this machine, not just this one.", + "action": "run-this-yourself", + "why": "devup-mcp writes only inside an allowed output root, and the home directory is \ + not one. Widening that so a design-to-code server can write to $HOME is not a \ + trade worth making, so the path is yours to fill.", + "how": "Copy the installed skill directories there, or install them with your runtime's \ + own skill tooling. A skill found in this directory is reported as installed by \ + every later call, for every project.", + "readNotWritten": true, + }) +} + /// What a `.changepacks/` directory obliges a pull request in this workspace to /// carry, or `None` when the repository does not use changepacks. /// @@ -794,19 +1413,21 @@ fn display_path(path: &Path) -> String { /// [`OutputTransaction`]: super::output::OutputTransaction pub async fn install( policy: &super::output::OutputPolicy, + lookup: &Lookup, requested: &[String], ) -> Result { - install_with(policy, requested, &InstallUpstream).await + install_with(policy, lookup, requested, &InstallUpstream).await } async fn install_with( policy: &super::output::OutputPolicy, + lookup: &Lookup, requested: &[String], upstream: &dyn SkillUpstream, ) -> Result { use devup_mcp_figma::{DevupError, ErrorCode}; - let project = policy.primary_root().to_path_buf(); + let project = lookup.project.clone(); if let Some(unknown) = requested.iter().find(|name| find_by_name(name).is_none()) { return Err(DevupError::with_details( ErrorCode::DevupInvalidInput, @@ -839,10 +1460,12 @@ async fn install_with( }) .collect::>(); - let root = target_root(&project); + let roots = lookup.target_roots(); let mut transaction = super::output::OutputTransaction::new(); let mut written = Vec::new(); let mut already = Vec::new(); + let mut refreshed = Vec::new(); + let mut left_alone = Vec::new(); let mut warnings = Vec::new(); // One budget for the call, not four seconds multiplied by the registry. let deadline = tokio::time::Instant::now() + FETCH_TIMEOUT; @@ -851,57 +1474,83 @@ async fn install_with( .filter(|skill| skill.record.origin.is_carried()) { let name = &skill.record.name; - if !installed_paths(&project, name).is_empty() { - already.push(name.clone()); - continue; - } - let mut documents = skill - .installable_documents() - .expect("a carried skill always has documents"); - let mut source = "embedded"; - let mut reason = Some("authored in this repository; no upstream".to_owned()); - let mut provenance = Vec::new(); - if skill.record.origin == Origin::Embedded { - match skill.fetch_documents(upstream, deadline).await { - Ok(fetched) => { - documents = fetched.documents; - provenance = fetched.provenance; - source = "fetched"; - reason = None; - } - Err((url, error)) => { - reason = Some(format!("{url}: {error}")); - if error == SkillFetchError::NotFound { - warnings.push(serde_json::json!({ - "name": name, - "sourceUrl": url, - "message": "Upstream returned 404; the document may have moved. Check the skill manifest. The complete embedded copy was installed.", - })); - } + // Presence alone used to end the decision here, which is why there was + // no way to update a skill: the first install a machine ever did was + // the last one it would get. What is on disk decides now. + let present = lookup.installed_paths(name); + if !present.is_empty() { + let states = present + .iter() + .map(|path| (path.clone(), skill.freshness_at(path))) + .collect::>(); + let refreshable = states + .iter() + .filter(|(_, state)| state.should_refresh()) + .map(|(path, _)| path.clone()) + .collect::>(); + for (path, state) in &states { + if *state == Freshness::Foreign { + left_alone.push(serde_json::json!({ + "name": name, + "path": path.display().to_string(), + "revision": state.as_str(), + "why": "No devup-mcp provenance note, so devup-mcp did not write it and \ + will not replace it. Delete it first to take devup-mcp's copy.", + })); } } + if refreshable.is_empty() { + already.push(name.clone()); + continue; + } + // Rewrite where the stale copies actually are, not where a fresh + // install would have chosen - a copy left behind in a root the + // chosen one is not in would keep being loaded and keep being old. + refreshed.push(serde_json::json!({ + "name": name, + "paths": refreshable.iter().map(|p| p.display().to_string()).collect::>(), + })); + stage_documents( + policy, + &mut transaction, + skill, + upstream, + deadline, + &refreshable + .iter() + .filter_map(|path| Some(path.parent()?.parent()?.to_path_buf())) + .collect::>(), + &project, + &mut warnings, + &mut written, + ) + .await?; + continue; } - let mut paths = Vec::with_capacity(documents.len()); - for (relative, contents) in documents { - let target = - policy.resolve(&document_path(&root, name, relative).display().to_string())?; - paths.push(target.display_path().display().to_string()); - transaction.stage( - format!("skill:{name}:{relative}"), - target, - contents.as_bytes(), - )?; - } - written.push(serde_json::json!({"name": name, "paths": paths, "source": source, "reason": reason, "documents": provenance})); + stage_documents( + policy, + &mut transaction, + skill, + upstream, + deadline, + &roots, + &project, + &mut warnings, + &mut written, + ) + .await?; } transaction.commit()?; Ok(serde_json::json!({ "installed": written, + "refreshed": refreshed, "alreadyPresent": already, + "leftAlone": left_alone, "notInstallable": external, "warnings": warnings, - "root": root.display().to_string(), + "roots": roots.iter().map(|root| root.display().to_string()).collect::>(), + "runtime": lookup.runtime.as_str(), "nextAction": if written.is_empty() && external.is_empty() { serde_json::Value::Null } else { @@ -915,6 +1564,96 @@ async fn install_with( })) } +/// Resolves one skill's documents and stages every one of them under `roots`. +/// +/// Shared by the first install and by a refresh so the two cannot drift: a +/// refresh that wrote a different set of files than an install would have is +/// how a skill ends up half one revision and half another. +/// +/// The whole set is staged, never a document at a time, for the reason this +/// module exists: a `SKILL.md` that survived next to four references that did +/// not looks installed and its links go nowhere. +#[allow(clippy::too_many_arguments)] +async fn stage_documents( + policy: &super::output::OutputPolicy, + transaction: &mut super::output::OutputTransaction, + skill: &Skill, + upstream: &dyn SkillUpstream, + deadline: tokio::time::Instant, + roots: &[PathBuf], + project: &Path, + warnings: &mut Vec, + written: &mut Vec, +) -> Result<(), devup_mcp_figma::DevupError> { + use devup_mcp_figma::{DevupError, ErrorCode}; + + let name = &skill.record.name; + let mut documents = skill + .installable_documents() + .expect("a carried skill always has documents"); + let mut source = "embedded"; + let mut reason = Some("authored in this repository; no upstream".to_owned()); + let mut provenance = Vec::new(); + if skill.record.origin == Origin::Embedded { + match skill.fetch_documents(upstream, deadline).await { + Ok(fetched) => { + documents = fetched.documents; + provenance = fetched.provenance; + source = "fetched"; + reason = None; + } + Err((url, error)) => { + reason = Some(format!("{url}: {error}")); + if error == SkillFetchError::NotFound { + warnings.push(serde_json::json!({ + "name": name, + "sourceUrl": url, + "message": "Upstream returned 404; the document may have moved. Check the skill manifest. The complete embedded copy was installed.", + })); + } + } + } + } + let mut paths = Vec::with_capacity(documents.len() * roots.len()); + for root in roots { + for (relative, contents) in &documents { + // The skill root has to sit inside an allowed write root. Said in + // those words, because the caller who hits this passed a + // `projectRoot`, not an `outputPath`, and being told an + // `outputPath` is out of bounds names something they never sent. + let target = policy + .resolve(&document_path(root, name, relative).display().to_string()) + .map_err(|error| { + DevupError::with_details( + ErrorCode::DevupInvalidInput, + format!( + "Cannot install skills into {}: it is outside this server's allowed write roots.", + root.display() + ), + false, + serde_json::json!({ + "projectRoot": project.display().to_string(), + "skillRoot": root.display().to_string(), + "underlying": error.message, + "how": "Start devup-mcp with --allow-write-root pointing at this project, \ + or omit projectRoot to install into the server's own write root.", + }), + ) + })?; + paths.push(target.display_path().display().to_string()); + transaction.stage( + format!("skill:{name}:{}:{relative}", root.display()), + target, + contents.as_bytes(), + )?; + } + } + written.push( + serde_json::json!({"name": name, "paths": paths, "source": source, "reason": reason, "documents": provenance}), + ); + Ok(()) +} + #[cfg(test)] #[path = "skills/fetch_tests.rs"] mod fetch_tests; @@ -928,9 +1667,13 @@ mod tests { async fn an_own_install_reports_the_binary_as_its_source() { let project = scratch("own-source"); let policy = super::super::output::OutputPolicy::from_roots(vec![project.clone()]).unwrap(); - let report = install(&policy, &["devfive-frontend".to_owned()]) - .await - .unwrap(); + let report = install( + &policy, + &unknown_client(&project), + &["devfive-frontend".to_owned()], + ) + .await + .unwrap(); assert_eq!(report["installed"][0]["source"], "embedded"); assert_eq!( report["installed"][0]["reason"], @@ -940,6 +1683,16 @@ mod tests { std::fs::remove_dir_all(project).unwrap(); } + /// A lookup for a client that never named itself, seeing no machine-wide + /// roots. + /// + /// `project_only` is load-bearing: reading the real home would make these + /// assertions depend on whether whoever runs them has devup-ui in + /// `~/.claude/skills`, and most people working on this repository do. + fn unknown_client(project: &Path) -> Lookup { + Lookup::project_only(project.to_path_buf(), Runtime::Unknown) + } + fn scratch(label: &str) -> PathBuf { let path = std::env::temp_dir().join(format!("devup-skills-{label}-{}", std::process::id())); @@ -1110,7 +1863,9 @@ mod tests { let project = scratch("state"); let skill = find_by_name("devup-ui").expect("devup-ui is registered"); - let missing = skill.install_action(&project); + let lookup = unknown_client(&project); + + let missing = skill.install_action(&lookup); assert_eq!(missing["installed"], false); assert_eq!(missing["action"], "devup_skills"); @@ -1120,7 +1875,7 @@ mod tests { std::fs::create_dir_all(path.parent().unwrap()).unwrap(); std::fs::write(&path, "# installed by hand").unwrap(); - let found = skill.install_action(&project); + let found = skill.install_action(&lookup); assert_eq!( found["installed"], true, "a hand-installed skill is installed" @@ -1131,6 +1886,345 @@ mod tests { let _ = std::fs::remove_dir_all(&project); } + /// The client names itself at `initialize`, and that name is the only + /// thing that says which directory to write. + #[test] + fn the_runtime_is_read_from_the_client_name() { + for (name, expected) in [ + ("codex", Runtime::Codex), + ("Codex CLI", Runtime::Codex), + ("claude-code", Runtime::ClaudeCode), + ("Claude Code", Runtime::ClaudeCode), + ("opencode", Runtime::Opencode), + // `cursor-vscode` contains both names; Cursor has to win. + ("cursor-vscode", Runtime::Cursor), + ("Visual Studio Code", Runtime::VsCode), + ("Code - OSS", Runtime::VsCode), + ("Zed", Runtime::Zed), + ("windsurf-client", Runtime::Windsurf), + ("gemini-cli-mcp-client", Runtime::GeminiCli), + ("@cline/core", Runtime::Cline), + ("continue-cli-client", Runtime::Continue), + ("amp-mcp-client", Runtime::Amp), + ("goose", Runtime::Goose), + // No fixed auto-discovered directory, so guessing one would be + // worse than the fallback. + ("JetBrains-IU-copilot-intellij", Runtime::Unknown), + ("some-editor", Runtime::Unknown), + // Short words must not be claimed out of unrelated names. + ("analyzed-client", Runtime::Unknown), + ("example-mcp", Runtime::Unknown), + ] { + assert_eq!( + Runtime::from_client_name(name), + expected, + "{name} was read as the wrong runtime" + ); + } + // `opencode` contains `codex` backwards-of-nowhere, but it does + // contain `code`; the ordering that actually matters is that the + // substring test for opencode runs before the one for codex. + assert_eq!(Runtime::from_client_name("opencode"), Runtime::Opencode); + } + + /// The reported bug, as a test. A fresh project has no skill root at all, + /// so the choice used to fall through to `SKILL_ROOTS[0]` - + /// `.claude/skills` - which Codex never reads. The install reported + /// success and the skill never loaded. + #[test] + fn a_fresh_project_installs_where_this_runtime_actually_reads() { + let project = scratch("fresh"); + let skill = find_by_name("devup-ui").unwrap(); + + let codex = Lookup::project_only(project.clone(), Runtime::Codex); + let writes = skill.install_action(&codex)["writesTo"][0] + .as_str() + .unwrap() + .to_owned(); + assert!( + writes.contains(".agents"), + "Codex would be given {writes}, which it does not read" + ); + assert!(!writes.contains(".claude"), "{writes}"); + + let claude = Lookup::project_only(project.clone(), Runtime::ClaudeCode); + let writes = skill.install_action(&claude)["writesTo"][0] + .as_str() + .unwrap() + .to_owned(); + assert!(writes.contains(".claude"), "{writes}"); + + let _ = std::fs::remove_dir_all(&project); + } + + /// With no client name and no existing root there is nothing to choose on, + /// and picking one of three is a two-in-three chance of writing where + /// nothing looks. + #[test] + fn an_unnamed_client_is_given_every_convention() { + let project = scratch("unnamed"); + let roots = Lookup::project_only(project.clone(), Runtime::Unknown).target_roots(); + assert_eq!(roots.len(), SKILL_ROOTS.len()); + for convention in SKILL_ROOTS { + assert!( + roots + .iter() + .any(|root| root == &join_root(&project, convention)), + "{convention} would not be written, so a runtime reading it gets nothing" + ); + } + + let _ = std::fs::remove_dir_all(&project); + } + + /// The symmetric mistake, and the one that is silent: a file in a + /// directory this runtime never opens is not loaded, and counting it says + /// "installed" about a skill the agent will never see. + #[test] + fn a_skill_in_a_root_this_runtime_never_reads_is_not_installed() { + let project = scratch("wrong-root"); + let path = install_path(&join_root(&project, ".claude/skills"), "devup-ui"); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(&path, "# in the wrong place for Codex").unwrap(); + + assert!( + Lookup::project_only(project.clone(), Runtime::Codex) + .installed_paths("devup-ui") + .is_empty(), + "Codex was told a skill it cannot load is installed" + ); + assert!( + !Lookup::project_only(project.clone(), Runtime::ClaudeCode) + .installed_paths("devup-ui") + .is_empty(), + "Claude Code reads exactly this directory" + ); + + let _ = std::fs::remove_dir_all(&project); + } + + /// Most people install skills once for the machine, not per repository. + /// Reporting those missing is what had a workspace with devup-ui in four + /// loaded directories answering `installedCount: 0`. + #[test] + fn a_machine_wide_install_counts_as_installed() { + let home = scratch("home"); + let project = scratch("home-project"); + let path = install_path(&join_root(&home, ".codex/skills"), "devup-ui"); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(&path, "# installed for the machine").unwrap(); + + let lookup = Lookup { + project: project.clone(), + home: Some(home.clone()), + runtime: Runtime::Codex, + client_name: None, + }; + assert_eq!( + lookup.installed_paths("devup-ui"), + vec![path], + "a skill Codex loads from the home directory was reported missing" + ); + let action = find_by_name("devup-ui").unwrap().install_action(&lookup); + assert_eq!(action["installed"], true); + assert_eq!(action["action"], serde_json::Value::Null, "nothing to do"); + + let _ = std::fs::remove_dir_all(&home); + let _ = std::fs::remove_dir_all(&project); + } + + /// Writes what an install would write, so the freshness check has + /// something honest to compare against. + fn install_by_hand(project: &Path, runtime: Runtime, name: &str) -> PathBuf { + let skill = find_by_name(name).unwrap(); + let root = join_root(project, runtime.write_roots()[0]); + for (relative, contents) in skill.installable_documents().unwrap() { + let path = document_path(&root, name, relative); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(&path, contents).unwrap(); + } + install_path(&root, name) + } + + /// `installed` used to mean `is_file()`, so a one-line placeholder read + /// exactly like the current document. These are the four states that were + /// all reported as one. + #[test] + fn freshness_separates_current_older_foreign_and_incomplete() { + let project = scratch("freshness"); + let skill = find_by_name("devfive-frontend").unwrap(); + + let entry = install_by_hand(&project, Runtime::Codex, "devfive-frontend"); + assert_eq!( + skill.freshness_at(&entry), + Freshness::Current, + "what an install just wrote is not current" + ); + + // A reference gone: the shape that looks installed and whose links go + // nowhere. + let reference = entry + .parent() + .unwrap() + .join("references") + .join("anti-patterns.md"); + assert!( + reference.is_file(), + "this test needs the multi-document skill" + ); + std::fs::remove_file(&reference).unwrap(); + assert_eq!(skill.freshness_at(&entry), Freshness::Incomplete); + let _ = std::fs::remove_dir_all(&project); + + // Our provenance note, someone else's body: a different devup-mcp. + let project = scratch("freshness-older"); + let entry = install_by_hand(&project, Runtime::Codex, "devfive-frontend"); + let body = std::fs::read_to_string(&entry).unwrap(); + std::fs::write( + &entry, + body.replace("Server Components", "Rules from an older build"), + ) + .unwrap(); + assert_eq!(skill.freshness_at(&entry), Freshness::Older); + + // No note at all: not ours, and not ours to replace. + std::fs::write(&entry, "# my own notes about this project\n").unwrap(); + assert_eq!(skill.freshness_at(&entry), Freshness::Foreign); + + let _ = std::fs::remove_dir_all(&project); + } + + /// The practical sting of `is_file()`: `install` skipped anything already + /// present, so the first install a machine did was the last one it got. + #[tokio::test] + async fn install_refreshes_an_older_copy_and_never_touches_a_foreign_one() { + let project = scratch("refresh"); + let policy = super::super::output::OutputPolicy::from_roots(vec![project.clone()]).unwrap(); + let lookup = Lookup::project_only(project.clone(), Runtime::Codex); + + let entry = install_by_hand(&project, Runtime::Codex, "devfive-frontend"); + let current = std::fs::read_to_string(&entry).unwrap(); + std::fs::write(&entry, current.replace("Server Components", "stale")).unwrap(); + + let report = install(&policy, &lookup, &["devfive-frontend".to_owned()]) + .await + .unwrap(); + assert_eq!( + report["refreshed"][0]["name"], "devfive-frontend", + "an outdated copy was skipped instead of rewritten: {report}" + ); + assert_eq!( + std::fs::read_to_string(&entry).unwrap(), + current, + "the refresh did not restore this build's document" + ); + + // Hand-written: reported, never overwritten. + let mine = "# my own notes\n"; + std::fs::write(&entry, mine).unwrap(); + let report = install(&policy, &lookup, &["devfive-frontend".to_owned()]) + .await + .unwrap(); + assert_eq!( + report["leftAlone"][0]["name"], "devfive-frontend", + "{report}" + ); + assert_eq!( + std::fs::read_to_string(&entry).unwrap(), + mine, + "a document devup-mcp did not write was replaced" + ); + + drop(policy); + let _ = std::fs::remove_dir_all(&project); + } + + /// A skill someone installed once for the machine, in their editor's own + /// home directory, was being reported missing - and the fix for "missing" + /// is to write a second copy. + #[test] + fn every_runtime_finds_its_own_home_directory() { + for (runtime, root) in [ + (Runtime::Cursor, ".cursor/skills"), + (Runtime::GeminiCli, ".gemini/skills"), + (Runtime::Cline, ".cline/skills"), + (Runtime::Windsurf, ".codeium/windsurf/skills"), + (Runtime::VsCode, ".copilot/skills"), + ] { + assert!( + runtime.user_roots().contains(&root), + "{} does not look in {root}, where its users install", + runtime.as_str() + ); + assert!( + USER_SKILL_ROOTS.contains(&root), + "an unidentified client would miss {root}" + ); + } + } + + /// Cline is why the table is worth having: it reads `.agents/skills` only + /// from the home directory, so the shared project convention is one + /// directory it never opens. + #[test] + fn a_client_that_does_not_share_the_common_convention_is_not_given_it() { + assert!(!Runtime::Cline.project_roots().contains(&".agents/skills")); + assert!(Runtime::Cline.user_roots().contains(&".agents/skills")); + assert_eq!(Runtime::Cline.write_roots()[0], ".cline/skills"); + } + + /// Half the table is matched against names observed in the wild, because + /// those clients are closed source. A wrong guess does not fail - it just + /// never fires, and reads exactly like a client devup-mcp has never heard + /// of. The name has to come back with the verdict or there is no way to + /// tell those two apart, and no way for the table to get corrected. + #[test] + fn the_report_echoes_the_name_beside_the_verdict_it_drew_from_it() { + let project = scratch("echo"); + let seen = report(&Lookup::new( + project.to_str(), + &project, + Some("some-editor-nobody-added-yet"), + )); + assert_eq!(seen["runtime"]["detected"], "unknown"); + assert_eq!( + seen["runtime"]["clientName"], "some-editor-nobody-added-yet", + "the name that produced the verdict was dropped" + ); + assert!( + !seen["runtime"]["ifDetectedIsUnknown"].is_null(), + "an unknown client was not told what unknown means" + ); + + // A recognised client needs no such note; saying it anyway is the + // noise that teaches a reader to skip the field. + let known = report(&Lookup::new(project.to_str(), &project, Some("codex"))); + assert_eq!(known["runtime"]["detected"], "codex"); + assert_eq!(known["runtime"]["clientName"], "codex"); + assert!(known["runtime"]["ifDetectedIsUnknown"].is_null()); + + // Before the handshake there is no name, and claiming one would be a + // lie about where the verdict came from. + let silent = report(&Lookup::new(project.to_str(), &project, None)); + assert_eq!(silent["runtime"]["detected"], "unknown"); + assert!(silent["runtime"]["clientName"].is_null()); + + let _ = std::fs::remove_dir_all(&project); + } + + /// An unidentified client reads everything and writes the three broad + /// conventions. Reading narrowly would duplicate someone's install; + /// writing broadly would litter a dozen editor directories. + #[test] + fn an_unknown_client_reads_wide_and_writes_narrow() { + assert_eq!(Runtime::Unknown.project_roots(), EVERY_PROJECT_ROOT); + assert_eq!(Runtime::Unknown.write_roots(), SKILL_ROOTS); + assert!(Runtime::Unknown.project_roots().len() > SKILL_ROOTS.len()); + for convention in SKILL_ROOTS { + assert!(EVERY_PROJECT_ROOT.contains(convention)); + } + } + /// An external skill is never answered with a devup-mcp call, because there /// is nothing here to install. It has to hand over its publisher's command /// and say why the bytes are not ours to ship. @@ -1145,7 +2239,7 @@ mod tests { assert!(skill.document().is_none()); assert!(skill.installable_documents().is_none()); - let action = skill.install_action(&project); + let action = skill.install_action(&unknown_client(&project)); assert_eq!(action["action"], "run-this-yourself"); assert_eq!(action["command"], "npx skills add vercel-labs/agent-skills"); assert!( @@ -1171,7 +2265,7 @@ mod tests { std::fs::create_dir_all(&chosen).unwrap(); let skill = find_by_name("devup-ui").unwrap(); - let action = skill.install_action(&project); + let action = skill.install_action(&unknown_client(&project)); let writes_to = action["writesTo"][0].as_str().unwrap().to_owned(); assert!( writes_to.contains(".opencode"), @@ -1268,7 +2362,7 @@ mod tests { let project = scratch("changepacks"); assert!( - report(&project)["repoObligations"].is_null(), + report(&unknown_client(&project))["repoObligations"].is_null(), "a workspace with no .changepacks must carry no obligation" ); @@ -1283,7 +2377,7 @@ mod tests { // Not a changepack log; it must not be counted as one. std::fs::write(directory.join("publish.tgz"), "").unwrap(); - let found = report(&project)["repoObligations"]["changepacks"].clone(); + let found = report(&unknown_client(&project))["repoObligations"]["changepacks"].clone(); assert!(!found.is_null(), "the directory was not detected"); // The command has to be the non-interactive one. Bare `changepacks` diff --git a/crates/devup-mcp/src/server/skills/fetch_tests.rs b/crates/devup-mcp/src/server/skills/fetch_tests.rs index b55cb5c2..15ca97c5 100644 --- a/crates/devup-mcp/src/server/skills/fetch_tests.rs +++ b/crates/devup-mcp/src/server/skills/fetch_tests.rs @@ -36,6 +36,15 @@ fn policy(label: &str) -> (PathBuf, super::super::output::OutputPolicy) { (project, policy) } +/// A scratch project with no machine-wide roots. +/// +/// `project_only` is the point: reading the real home would make these +/// assertions depend on whether the developer running them happens to have +/// devup-ui installed in `~/.claude/skills`, which most of them do. +fn lookup(project: &std::path::Path) -> Lookup { + Lookup::project_only(project.to_path_buf(), Runtime::ClaudeCode) +} + #[tokio::test] async fn fetched_documents_report_their_real_provenance_and_manifest_url() { let (project, policy) = policy("fetched"); @@ -44,9 +53,14 @@ async fn fetched_documents_report_their_real_provenance_and_manifest_url() { contents: text.as_bytes().to_vec(), etag: "\"latest\"".to_owned(), })); - let result = install_with(&policy, &["vespera".to_owned()], &upstream) - .await - .unwrap(); + let result = install_with( + &policy, + &lookup(&project), + &["vespera".to_owned()], + &upstream, + ) + .await + .unwrap(); let written = &result["installed"][0]; assert_eq!(written["source"], "fetched"); assert!(written["reason"].is_null()); @@ -65,9 +79,14 @@ async fn fetched_documents_report_their_real_provenance_and_manifest_url() { assert!(body.contains(&provenance["fetchedAt"].to_string())); assert!(!body.contains("Vendored from")); assert!(body.ends_with("# Current upstream rules\n")); - let again = install_with(&policy, &["vespera".to_owned()], &upstream) - .await - .unwrap(); + let again = install_with( + &policy, + &lookup(&project), + &["vespera".to_owned()], + &upstream, + ) + .await + .unwrap(); assert_eq!(again["alreadyPresent"][0], "vespera"); assert_eq!(upstream.urls.lock().unwrap().len(), 1); drop(policy); @@ -84,9 +103,14 @@ async fn every_fetch_failure_installs_embedded_but_only_404_warns() { ] { let (project, policy) = policy("fallback"); let upstream = FakeUpstream::new(Err(error.clone())); - let result = install_with(&policy, &["devup-ui".to_owned()], &upstream) - .await - .unwrap(); + let result = install_with( + &policy, + &lookup(&project), + &["devup-ui".to_owned()], + &upstream, + ) + .await + .unwrap(); let written = &result["installed"][0]; assert_eq!(written["source"], "embedded"); assert!( @@ -130,6 +154,7 @@ async fn own_external_and_unknown_skills_never_fetch() { let upstream = FakeUpstream::new(Err(SkillFetchError::NotFound)); let result = install_with( &policy, + &lookup(&project), &[ "devfive-frontend".to_owned(), "vercel-react-best-practices".to_owned(), @@ -146,9 +171,14 @@ async fn own_external_and_unknown_skills_never_fetch() { ); assert!(result["warnings"].as_array().unwrap().is_empty()); assert!( - install_with(&policy, &["unknown".to_owned()], &upstream) - .await - .is_err() + install_with( + &policy, + &lookup(&project), + &["unknown".to_owned()], + &upstream + ) + .await + .is_err() ); assert!(upstream.urls.lock().unwrap().is_empty()); drop(policy); @@ -166,7 +196,9 @@ async fn the_entire_install_has_one_four_second_fetch_budget() { } let (project, policy) = policy("timeout"); let start = tokio::time::Instant::now(); - let result = install_with(&policy, &[], &Stalled).await.unwrap(); + let result = install_with(&policy, &lookup(&project), &[], &Stalled) + .await + .unwrap(); assert_eq!(start.elapsed(), std::time::Duration::from_secs(4)); for written in result["installed"].as_array().unwrap() { assert_eq!(written["source"], "embedded"); @@ -245,7 +277,11 @@ async fn a_failed_write_does_not_commit_any_skill() { contents: b"# fetched".to_vec(), etag: "new".to_owned(), })); - assert!(install_with(&policy, &[], &upstream).await.is_err()); + assert!( + install_with(&policy, &lookup(&project), &[], &upstream) + .await + .is_err() + ); assert!(!project.join(".claude/skills/devup-ui/SKILL.md").exists()); assert!( !project diff --git a/crates/devup-mcp/src/server/tools.rs b/crates/devup-mcp/src/server/tools.rs index 62eb156c..71d38b26 100644 --- a/crates/devup-mcp/src/server/tools.rs +++ b/crates/devup-mcp/src/server/tools.rs @@ -24,6 +24,16 @@ pub struct SkillsInput { /// missing, which is the usual case on a machine that has just been set up. #[serde(default)] pub names: Vec, + /// The project whose skills are being reported on or installed into. + /// + /// Omitted, this falls back to the server's own write root, and that is + /// only the project when the two were configured to be the same. A host + /// that granted one shared parent - an Orca worktree pool, a monorepo + /// checkout - made every call report on that parent instead, where no + /// runtime looks for skills. Pass the directory the code is going into, + /// the same one the other tools take. + #[serde(default)] + pub project_root: Option, } fn default_skills_action() -> String { diff --git a/crates/devup-mcp/tests/skills_install.rs b/crates/devup-mcp/tests/skills_install.rs index 5c52ec1d..3889908e 100644 --- a/crates/devup-mcp/tests/skills_install.rs +++ b/crates/devup-mcp/tests/skills_install.rs @@ -43,8 +43,19 @@ where { // Set process configuration before startup, without racing other tests by // mutating this test process's environment. No HTTP or bridge socket opens. + // + // The home is pointed at an empty directory inside the scratch workspace. + // Install state counts the machine-wide skill roots, because a skill in + // `~/.codex/skills` really is loaded and really must not be reported + // missing - but that makes the real home an input, and anyone working on + // this repository has devup-ui installed in theirs. Left alone, these + // assertions pass in CI and fail on the laptop that wrote them. + let home = workspace.join("home"); + std::fs::create_dir_all(&home)?; let mut child = tokio::process::Command::new(env!("CARGO_BIN_EXE_devup-mcp")) .current_dir(workspace) + .env("HOME", &home) + .env("USERPROFILE", &home) .env("DEVUP_MCP_SKILLS_OFFLINE", "1") .env("DEVUP_MCP_NO_UPDATE_CHECK", "1") .env("DEVUP_FIGMA_BRIDGE_PORT", "off")