From ebfd312f705492130272eb2674384f01a64c78b2 Mon Sep 17 00:00:00 2001 From: Linus Beckhaus Date: Sat, 7 Mar 2026 12:18:55 +0100 Subject: [PATCH 01/15] chore: run basic commands --- .claude/skills/gitnexus/gitnexus-cli/SKILL.md | 164 +++++++++--------- .../skills/gitnexus/gitnexus-guide/SKILL.md | 128 +++++++------- AGENTS.md | 12 +- CLAUDE.md | 12 +- gitnexus/package-lock.json | 11 +- 5 files changed, 167 insertions(+), 160 deletions(-) diff --git a/.claude/skills/gitnexus/gitnexus-cli/SKILL.md b/.claude/skills/gitnexus/gitnexus-cli/SKILL.md index 3ae9c18e5d..1318567d55 100644 --- a/.claude/skills/gitnexus/gitnexus-cli/SKILL.md +++ b/.claude/skills/gitnexus/gitnexus-cli/SKILL.md @@ -1,82 +1,82 @@ ---- -name: gitnexus-cli -description: "Use when the user needs to run GitNexus CLI commands like analyze/index a repo, check status, clean the index, generate a wiki, or list indexed repos. Examples: \"Index this repo\", \"Reanalyze the codebase\", \"Generate a wiki\"" ---- - -# GitNexus CLI Commands - -All commands work via `npx` — no global install required. - -## Commands - -### analyze — Build or refresh the index - -```bash -npx gitnexus analyze -``` - -Run from the project root. This parses all source files, builds the knowledge graph, writes it to `.gitnexus/`, and generates CLAUDE.md / AGENTS.md context files. - -| Flag | Effect | -| -------------- | ---------------------------------------------------------------- | -| `--force` | Force full re-index even if up to date | -| `--embeddings` | Enable embedding generation for semantic search (off by default) | - -**When to run:** First time in a project, after major code changes, or when `gitnexus://repo/{name}/context` reports the index is stale. - -### status — Check index freshness - -```bash -npx gitnexus status -``` - -Shows whether the current repo has a GitNexus index, when it was last updated, and symbol/relationship counts. Use this to check if re-indexing is needed. - -### clean — Delete the index - -```bash -npx gitnexus clean -``` - -Deletes the `.gitnexus/` directory and unregisters the repo from the global registry. Use before re-indexing if the index is corrupt or after removing GitNexus from a project. - -| Flag | Effect | -| --------- | ------------------------------------------------- | -| `--force` | Skip confirmation prompt | -| `--all` | Clean all indexed repos, not just the current one | - -### wiki — Generate documentation from the graph - -```bash -npx gitnexus wiki -``` - -Generates repository documentation from the knowledge graph using an LLM. Requires an API key (saved to `~/.gitnexus/config.json` on first use). - -| Flag | Effect | -| ------------------- | ----------------------------------------- | -| `--force` | Force full regeneration | -| `--model ` | LLM model (default: minimax/minimax-m2.5) | -| `--base-url ` | LLM API base URL | -| `--api-key ` | LLM API key | -| `--concurrency ` | Parallel LLM calls (default: 3) | -| `--gist` | Publish wiki as a public GitHub Gist | - -### list — Show all indexed repos - -```bash -npx gitnexus list -``` - -Lists all repositories registered in `~/.gitnexus/registry.json`. The MCP `list_repos` tool provides the same information. - -## After Indexing - -1. **Read `gitnexus://repo/{name}/context`** to verify the index loaded -2. Use the other GitNexus skills (`exploring`, `debugging`, `impact-analysis`, `refactoring`) for your task - -## Troubleshooting - -- **"Not inside a git repository"**: Run from a directory inside a git repo -- **Index is stale after re-analyzing**: Restart Claude Code to reload the MCP server -- **Embeddings slow**: Omit `--embeddings` (it's off by default) or set `OPENAI_API_KEY` for faster API-based embedding +--- +name: gitnexus-cli +description: "Use when the user needs to run GitNexus CLI commands like analyze/index a repo, check status, clean the index, generate a wiki, or list indexed repos. Examples: \"Index this repo\", \"Reanalyze the codebase\", \"Generate a wiki\"" +--- + +# GitNexus CLI Commands + +All commands work via `npx` — no global install required. + +## Commands + +### analyze — Build or refresh the index + +```bash +npx gitnexus analyze +``` + +Run from the project root. This parses all source files, builds the knowledge graph, writes it to `.gitnexus/`, and generates CLAUDE.md / AGENTS.md context files. + +| Flag | Effect | +| -------------- | ---------------------------------------------------------------- | +| `--force` | Force full re-index even if up to date | +| `--embeddings` | Enable embedding generation for semantic search (off by default) | + +**When to run:** First time in a project, after major code changes, or when `gitnexus://repo/{name}/context` reports the index is stale. + +### status — Check index freshness + +```bash +npx gitnexus status +``` + +Shows whether the current repo has a GitNexus index, when it was last updated, and symbol/relationship counts. Use this to check if re-indexing is needed. + +### clean — Delete the index + +```bash +npx gitnexus clean +``` + +Deletes the `.gitnexus/` directory and unregisters the repo from the global registry. Use before re-indexing if the index is corrupt or after removing GitNexus from a project. + +| Flag | Effect | +| --------- | ------------------------------------------------- | +| `--force` | Skip confirmation prompt | +| `--all` | Clean all indexed repos, not just the current one | + +### wiki — Generate documentation from the graph + +```bash +npx gitnexus wiki +``` + +Generates repository documentation from the knowledge graph using an LLM. Requires an API key (saved to `~/.gitnexus/config.json` on first use). + +| Flag | Effect | +| ------------------- | ----------------------------------------- | +| `--force` | Force full regeneration | +| `--model ` | LLM model (default: minimax/minimax-m2.5) | +| `--base-url ` | LLM API base URL | +| `--api-key ` | LLM API key | +| `--concurrency ` | Parallel LLM calls (default: 3) | +| `--gist` | Publish wiki as a public GitHub Gist | + +### list — Show all indexed repos + +```bash +npx gitnexus list +``` + +Lists all repositories registered in `~/.gitnexus/registry.json`. The MCP `list_repos` tool provides the same information. + +## After Indexing + +1. **Read `gitnexus://repo/{name}/context`** to verify the index loaded +2. Use the other GitNexus skills (`exploring`, `debugging`, `impact-analysis`, `refactoring`) for your task + +## Troubleshooting + +- **"Not inside a git repository"**: Run from a directory inside a git repo +- **Index is stale after re-analyzing**: Restart Claude Code to reload the MCP server +- **Embeddings slow**: Omit `--embeddings` (it's off by default) or set `OPENAI_API_KEY` for faster API-based embedding diff --git a/.claude/skills/gitnexus/gitnexus-guide/SKILL.md b/.claude/skills/gitnexus/gitnexus-guide/SKILL.md index 937ac73d16..6e58ef2fba 100644 --- a/.claude/skills/gitnexus/gitnexus-guide/SKILL.md +++ b/.claude/skills/gitnexus/gitnexus-guide/SKILL.md @@ -1,64 +1,64 @@ ---- -name: gitnexus-guide -description: "Use when the user asks about GitNexus itself — available tools, how to query the knowledge graph, MCP resources, graph schema, or workflow reference. Examples: \"What GitNexus tools are available?\", \"How do I use GitNexus?\"" ---- - -# GitNexus Guide - -Quick reference for all GitNexus MCP tools, resources, and the knowledge graph schema. - -## Always Start Here - -For any task involving code understanding, debugging, impact analysis, or refactoring: - -1. **Read `gitnexus://repo/{name}/context`** — codebase overview + check index freshness -2. **Match your task to a skill below** and **read that skill file** -3. **Follow the skill's workflow and checklist** - -> If step 1 warns the index is stale, run `npx gitnexus analyze` in the terminal first. - -## Skills - -| Task | Skill to read | -| -------------------------------------------- | ------------------- | -| Understand architecture / "How does X work?" | `gitnexus-exploring` | -| Blast radius / "What breaks if I change X?" | `gitnexus-impact-analysis` | -| Trace bugs / "Why is X failing?" | `gitnexus-debugging` | -| Rename / extract / split / refactor | `gitnexus-refactoring` | -| Tools, resources, schema reference | `gitnexus-guide` (this file) | -| Index, status, clean, wiki CLI commands | `gitnexus-cli` | - -## Tools Reference - -| Tool | What it gives you | -| ---------------- | ------------------------------------------------------------------------ | -| `query` | Process-grouped code intelligence — execution flows related to a concept | -| `context` | 360-degree symbol view — categorized refs, processes it participates in | -| `impact` | Symbol blast radius — what breaks at depth 1/2/3 with confidence | -| `detect_changes` | Git-diff impact — what do your current changes affect | -| `rename` | Multi-file coordinated rename with confidence-tagged edits | -| `cypher` | Raw graph queries (read `gitnexus://repo/{name}/schema` first) | -| `list_repos` | Discover indexed repos | - -## Resources Reference - -Lightweight reads (~100-500 tokens) for navigation: - -| Resource | Content | -| ---------------------------------------------- | ----------------------------------------- | -| `gitnexus://repo/{name}/context` | Stats, staleness check | -| `gitnexus://repo/{name}/clusters` | All functional areas with cohesion scores | -| `gitnexus://repo/{name}/cluster/{clusterName}` | Area members | -| `gitnexus://repo/{name}/processes` | All execution flows | -| `gitnexus://repo/{name}/process/{processName}` | Step-by-step trace | -| `gitnexus://repo/{name}/schema` | Graph schema for Cypher | - -## Graph Schema - -**Nodes:** File, Function, Class, Interface, Method, Community, Process -**Edges (via CodeRelation.type):** CALLS, IMPORTS, EXTENDS, IMPLEMENTS, DEFINES, MEMBER_OF, STEP_IN_PROCESS - -```cypher -MATCH (caller)-[:CodeRelation {type: 'CALLS'}]->(f:Function {name: "myFunc"}) -RETURN caller.name, caller.filePath -``` +--- +name: gitnexus-guide +description: "Use when the user asks about GitNexus itself — available tools, how to query the knowledge graph, MCP resources, graph schema, or workflow reference. Examples: \"What GitNexus tools are available?\", \"How do I use GitNexus?\"" +--- + +# GitNexus Guide + +Quick reference for all GitNexus MCP tools, resources, and the knowledge graph schema. + +## Always Start Here + +For any task involving code understanding, debugging, impact analysis, or refactoring: + +1. **Read `gitnexus://repo/{name}/context`** — codebase overview + check index freshness +2. **Match your task to a skill below** and **read that skill file** +3. **Follow the skill's workflow and checklist** + +> If step 1 warns the index is stale, run `npx gitnexus analyze` in the terminal first. + +## Skills + +| Task | Skill to read | +| -------------------------------------------- | ------------------- | +| Understand architecture / "How does X work?" | `gitnexus-exploring` | +| Blast radius / "What breaks if I change X?" | `gitnexus-impact-analysis` | +| Trace bugs / "Why is X failing?" | `gitnexus-debugging` | +| Rename / extract / split / refactor | `gitnexus-refactoring` | +| Tools, resources, schema reference | `gitnexus-guide` (this file) | +| Index, status, clean, wiki CLI commands | `gitnexus-cli` | + +## Tools Reference + +| Tool | What it gives you | +| ---------------- | ------------------------------------------------------------------------ | +| `query` | Process-grouped code intelligence — execution flows related to a concept | +| `context` | 360-degree symbol view — categorized refs, processes it participates in | +| `impact` | Symbol blast radius — what breaks at depth 1/2/3 with confidence | +| `detect_changes` | Git-diff impact — what do your current changes affect | +| `rename` | Multi-file coordinated rename with confidence-tagged edits | +| `cypher` | Raw graph queries (read `gitnexus://repo/{name}/schema` first) | +| `list_repos` | Discover indexed repos | + +## Resources Reference + +Lightweight reads (~100-500 tokens) for navigation: + +| Resource | Content | +| ---------------------------------------------- | ----------------------------------------- | +| `gitnexus://repo/{name}/context` | Stats, staleness check | +| `gitnexus://repo/{name}/clusters` | All functional areas with cohesion scores | +| `gitnexus://repo/{name}/cluster/{clusterName}` | Area members | +| `gitnexus://repo/{name}/processes` | All execution flows | +| `gitnexus://repo/{name}/process/{processName}` | Step-by-step trace | +| `gitnexus://repo/{name}/schema` | Graph schema for Cypher | + +## Graph Schema + +**Nodes:** File, Function, Class, Interface, Method, Community, Process +**Edges (via CodeRelation.type):** CALLS, IMPORTS, EXTENDS, IMPLEMENTS, DEFINES, MEMBER_OF, STEP_IN_PROCESS + +```cypher +MATCH (caller)-[:CodeRelation {type: 'CALLS'}]->(f:Function {name: "myFunc"}) +RETURN caller.name, caller.filePath +``` diff --git a/AGENTS.md b/AGENTS.md index b0e0f76969..613b768cb5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,7 +1,7 @@ # GitNexus — Code Intelligence -This project is indexed by GitNexus as **GitNexus** (1573 symbols, 4146 relationships, 120 execution flows). Use the GitNexus MCP tools to understand code, assess impact, and navigate safely. +This project is indexed by GitNexus as **intelligent_skills** (1617 symbols, 4272 relationships, 123 execution flows). Use the GitNexus MCP tools to understand code, assess impact, and navigate safely. > If any GitNexus tool warns the index is stale, run `npx gitnexus analyze` in terminal first. @@ -17,7 +17,7 @@ This project is indexed by GitNexus as **GitNexus** (1573 symbols, 4146 relation 1. `gitnexus_query({query: ""})` — find execution flows related to the issue 2. `gitnexus_context({name: ""})` — see all callers, callees, and process participation -3. `READ gitnexus://repo/GitNexus/process/{processName}` — trace the full execution flow step by step +3. `READ gitnexus://repo/intelligent_skills/process/{processName}` — trace the full execution flow step by step 4. For regressions: `gitnexus_detect_changes({scope: "compare", base_ref: "main"})` — see what your branch changed ## When Refactoring @@ -56,10 +56,10 @@ This project is indexed by GitNexus as **GitNexus** (1573 symbols, 4146 relation | Resource | Use for | |----------|---------| -| `gitnexus://repo/GitNexus/context` | Codebase overview, check index freshness | -| `gitnexus://repo/GitNexus/clusters` | All functional areas | -| `gitnexus://repo/GitNexus/processes` | All execution flows | -| `gitnexus://repo/GitNexus/process/{name}` | Step-by-step execution trace | +| `gitnexus://repo/intelligent_skills/context` | Codebase overview, check index freshness | +| `gitnexus://repo/intelligent_skills/clusters` | All functional areas | +| `gitnexus://repo/intelligent_skills/processes` | All execution flows | +| `gitnexus://repo/intelligent_skills/process/{name}` | Step-by-step execution trace | ## Self-Check Before Finishing diff --git a/CLAUDE.md b/CLAUDE.md index b0e0f76969..613b768cb5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,7 +1,7 @@ # GitNexus — Code Intelligence -This project is indexed by GitNexus as **GitNexus** (1573 symbols, 4146 relationships, 120 execution flows). Use the GitNexus MCP tools to understand code, assess impact, and navigate safely. +This project is indexed by GitNexus as **intelligent_skills** (1617 symbols, 4272 relationships, 123 execution flows). Use the GitNexus MCP tools to understand code, assess impact, and navigate safely. > If any GitNexus tool warns the index is stale, run `npx gitnexus analyze` in terminal first. @@ -17,7 +17,7 @@ This project is indexed by GitNexus as **GitNexus** (1573 symbols, 4146 relation 1. `gitnexus_query({query: ""})` — find execution flows related to the issue 2. `gitnexus_context({name: ""})` — see all callers, callees, and process participation -3. `READ gitnexus://repo/GitNexus/process/{processName}` — trace the full execution flow step by step +3. `READ gitnexus://repo/intelligent_skills/process/{processName}` — trace the full execution flow step by step 4. For regressions: `gitnexus_detect_changes({scope: "compare", base_ref: "main"})` — see what your branch changed ## When Refactoring @@ -56,10 +56,10 @@ This project is indexed by GitNexus as **GitNexus** (1573 symbols, 4146 relation | Resource | Use for | |----------|---------| -| `gitnexus://repo/GitNexus/context` | Codebase overview, check index freshness | -| `gitnexus://repo/GitNexus/clusters` | All functional areas | -| `gitnexus://repo/GitNexus/processes` | All execution flows | -| `gitnexus://repo/GitNexus/process/{name}` | Step-by-step execution trace | +| `gitnexus://repo/intelligent_skills/context` | Codebase overview, check index freshness | +| `gitnexus://repo/intelligent_skills/clusters` | All functional areas | +| `gitnexus://repo/intelligent_skills/processes` | All execution flows | +| `gitnexus://repo/intelligent_skills/process/{name}` | Step-by-step execution trace | ## Self-Check Before Finishing diff --git a/gitnexus/package-lock.json b/gitnexus/package-lock.json index f8a98f8603..bfcb5d185f 100644 --- a/gitnexus/package-lock.json +++ b/gitnexus/package-lock.json @@ -1,12 +1,12 @@ { "name": "gitnexus", - "version": "1.3.9", + "version": "1.3.10", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "gitnexus", - "version": "1.3.9", + "version": "1.3.10", "hasInstallScript": true, "license": "PolyForm-Noncommercial-1.0.0", "dependencies": { @@ -3084,6 +3084,7 @@ "resolved": "https://registry.npmjs.org/express/-/express-4.22.1.tgz", "integrity": "sha512-F2X8g9P1X7uCPZMA3MVf9wcTqlyNp7IhH5qPCI0izhaOIYXaW9L535tGA3qmjRzpH+bZczqq7hVKxTR4NWnu+g==", "license": "MIT", + "peer": true, "dependencies": { "accepts": "~1.3.8", "array-flatten": "1.1.1", @@ -4381,6 +4382,7 @@ "integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==", "dev": true, "license": "MIT", + "peer": true, "engines": { "node": ">=12" }, @@ -5222,6 +5224,7 @@ "integrity": "sha512-7dxoA6kYvtgWw80265MyqJlkRl4yawIjO7S5MigytjELkX43fV2WsAXzsNfO7sBpPPCF5Gp0+XzHk0DwLCq3xQ==", "hasInstallScript": true, "license": "MIT", + "peer": true, "dependencies": { "node-addon-api": "^8.0.0", "node-gyp-build": "^4.8.0" @@ -5594,6 +5597,7 @@ "integrity": "sha512-5C1sg4USs1lfG0GFb2RLXsdpXqBSEhAaA/0kPL01wxzpMqLILNxIxIOKiILz+cdg/pLnOUxFYOR5yhHU666wbw==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "esbuild": "~0.27.0", "get-tsconfig": "^4.7.5" @@ -5720,6 +5724,7 @@ "integrity": "sha512-w+N7Hifpc3gRjZ63vYBXA56dvvRlNWRczTdmCBBa+CotUzAPf5b7YMdMR/8CQoeYE5LX3W4wj6RYTgonm1b9DA==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "esbuild": "^0.27.0", "fdir": "^6.5.0", @@ -5795,6 +5800,7 @@ "integrity": "sha512-hOQuK7h0FGKgBAas7v0mSAsnvrIgAvWmRFjmzpJ7SwFHH3g1k2u37JtYwOwmEKhK6ZO3v9ggDBBm0La1LCK4uQ==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@vitest/expect": "4.0.18", "@vitest/mocker": "4.0.18", @@ -6134,6 +6140,7 @@ "resolved": "https://registry.npmjs.org/zod/-/zod-4.3.6.tgz", "integrity": "sha512-rftlrkhHZOcjDwkGlnUtZZkvaPHCsDATp4pGpuOOMDaTdDDXF91wuVDJoWoPsKX/3YPQ5fHuF3STjcYyKr+Qhg==", "license": "MIT", + "peer": true, "funding": { "url": "https://github.com/sponsors/colinhacks" } From 02eb465c6504e14e89b2a462ff0e58c8b1f0fdd1 Mon Sep 17 00:00:00 2001 From: Linus Beckhaus Date: Sat, 7 Mar 2026 12:45:35 +0100 Subject: [PATCH 02/15] docs: create initial summary and tracking document --- docs/pr-skill-installation-refactor.md | 134 +++++++++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100644 docs/pr-skill-installation-refactor.md diff --git a/docs/pr-skill-installation-refactor.md b/docs/pr-skill-installation-refactor.md new file mode 100644 index 0000000000..b6106635b5 --- /dev/null +++ b/docs/pr-skill-installation-refactor.md @@ -0,0 +1,134 @@ +# Refactor: Unify Skill Installation in `setup`, Remove from `analyze` + +## Current Situation + +GitNexus has two CLI commands that both install agent skills (static markdown files that teach AI agents GitNexus workflows): + +### `gitnexus analyze` + +- Primary purpose: index a git repository into a KuzuDB knowledge graph +- Secondary side effects: + - Updates `CLAUDE.md` and `AGENTS.md` with dynamic stats (node count, edge count, process count, etc.) + - Installs 6 skills to **project-local** `/.claude/skills/gitnexus//SKILL.md` +- Runs frequently (after every significant code change, or automatically via post-commit hooks) +- Skill installation lives in `ai-context.ts::installSkills()` — a simpler, older implementation + +### `gitnexus setup` + +- Primary purpose: one-time global configuration (MCP server entries, hooks) +- Also installs 6 skills to **global** `~/.claude/skills//SKILL.md` +- Runs once per machine +- Skill installation lives in `setup.ts::installSkillsTo()` — a more robust implementation that handles both flat files and directory-based skills with recursive copy + +### The skills themselves + +There are **7** skill files on disk in `gitnexus/skills/`: + +| Skill File | Installed by `analyze` | Installed by `setup` | +|---|---|---| +| `gitnexus-exploring.md` | Yes | Yes | +| `gitnexus-debugging.md` | Yes | Yes | +| `gitnexus-impact-analysis.md` | Yes | Yes | +| `gitnexus-refactoring.md` | Yes | Yes | +| `gitnexus-guide.md` | Yes | Yes | +| `gitnexus-cli.md` | Yes | Yes | +| `gitnexus-pr-review.md` | **No** | **No** | + +Both commands install the same 6 skills. `gitnexus-pr-review` exists on disk but is not installed by either command. + +## Why This Is a Problem + +### 1. Duplicate skills in Claude Code + +When a user runs both `setup` and `analyze`, the same skills exist in two locations: +- `~/.claude/skills/gitnexus-exploring/SKILL.md` (global, from `setup`) +- `/.claude/skills/gitnexus/gitnexus-exploring/SKILL.md` (project-local, from `analyze`) + +This is a **known Claude Code bug** ([#25209](https://github.com/anthropics/claude-code/issues/25209)): when the same skill name exists in both global and project-local directories, both appear in the skill list instead of one shadowing the other. Users see each GitNexus skill listed twice. + +### 2. Wrong responsibility boundary + +Skills are static markdown files. They don't depend on the repository index, don't contain dynamic stats, and don't change between `analyze` runs. Installing them on every `analyze` invocation is unnecessary work in the wrong place. + +The `analyze` command's purpose is indexing and generating dynamic context. Skill installation is static configuration — it belongs in `setup`. + +### 3. Two implementations of the same logic + +- `ai-context.ts::installSkills()` (lines 194-264) — simpler, only handles flat files, has hardcoded fallback descriptions +- `setup.ts::installSkillsTo()` (lines 254-289) — more robust, handles directory-based skills with recursive copy + +Having two code paths for the same operation is a maintenance burden. + +### 4. README inaccuracies + +The README currently states: +- "**4 agent skills** installed to `.claude/skills/` automatically" (line 185) — there are actually 6 (missing `gitnexus-guide` and `gitnexus-cli`), plus 1 unlisted (`gitnexus-pr-review`) +- The README implies `analyze` handles everything: "This indexes the codebase, installs agent skills, registers Claude Code hooks, and creates `AGENTS.md` / `CLAUDE.md` context files — all in one command." (line 73) — but hooks are actually installed by `setup`, not `analyze` + +## Options Considered + +### Option A: Remove skills from `analyze`, keep only in `setup` +- Cleanest separation of concerns +- Risk: users who never run `setup` get no skills +- **Chosen approach** (with migration handling) + +### Option B: Remove skills from `setup`, keep only in `analyze` +- Rejected: skills are static config, not index-dependent — wrong home + +### Option C: Keep both but add deduplication logic +- Rejected: adds complexity to work around a bug that shouldn't exist in our code + +### Option D: Add `--skills` flag to `analyze` for opt-in local installation +- Rejected by consensus (Gemini, Codex, and us): reintroduces the dual-location problem, adds a flag and code path for an edge case that contradicts the intended architecture + +## Chosen Strategy + +### 1. Remove skill installation from `analyze` + +- Delete `installSkills()` from `ai-context.ts` +- Remove its call in `generateAIContextFiles()` +- `generateAIContextFiles()` becomes focused: only generates `CLAUDE.md` and `AGENTS.md` with dynamic index stats + +### 2. `setup` is the single owner of skill installation + +- Already has the better implementation (`installSkillsTo()`) +- Installs globally for Claude Code, Cursor, and OpenCode +- No changes needed to the installation logic itself + +### 3. Migration: handle stale project-local skills + +Two-part approach that respects command boundaries: + +- **In `analyze`**: if `/.claude/skills/gitnexus/` exists, print a deprecation notice: + `"Note: Skills are no longer installed by analyze. Run 'gitnexus setup' to manage skills globally."` + No deletion — `analyze` doesn't own skills anymore and shouldn't destructively modify them. + +- **In `setup`**: after installing global skills, check if the current working directory is a git repo with `.claude/skills/gitnexus/`. If so, remove it and print a notice: + `"Removed project-local skills (now installed globally)"` + This is safe because `setup` is actively taking ownership and replacing the local copy with a global one. + +### 4. Update the `analyze` tip + +The existing tip at `analyze.ts:363` says "Run `gitnexus setup` to configure MCP for your editor." Update it to also mention skills: +`"Run 'gitnexus setup' to configure MCP and install agent skills for your editor."` + +### 5. Update README (separate commit) + +- Fix skill count: 6 skills, not 4 +- List all skills including `gitnexus-guide` and `gitnexus-cli` +- Clarify that `analyze` handles indexing + dynamic context, `setup` handles MCP + skills + hooks +- Correct line 73 which incorrectly attributes hooks to `analyze` + +### 6. Future work (out of scope for this PR) + +- Add `gitnexus-pr-review` to the `SKILL_NAMES` list in `setup.ts` +- Unify the skill name constants into a single shared location + +## Files Changed + +| File | Change | +|---|---| +| `gitnexus/src/cli/ai-context.ts` | Remove `installSkills()` function and its call | +| `gitnexus/src/cli/analyze.ts` | Add deprecation notice for stale local skills; update setup tip | +| `gitnexus/src/cli/setup.ts` | Add cleanup of project-local skills during global install | +| `README.md` | Fix skill count, clarify command responsibilities | From b246c8783bdb93fcda3fe7dee892fa121cb4ced5 Mon Sep 17 00:00:00 2001 From: Linus Beckhaus Date: Sat, 7 Mar 2026 12:52:27 +0100 Subject: [PATCH 03/15] test: add test suite for skill installation refactor Design and implement tests before the actual refactor to establish acceptance criteria and regression guards. - Export `installSkillsTo` and `SKILL_NAMES` from setup.ts for testability - Add regression tests to ai-context.test.ts (dynamic stats, idempotency, content preservation) + todo placeholders for post-refactor behavior - New setup-skills.test.ts: validates installSkillsTo installs all 6 skills correctly, with idempotency + todo placeholders for cleanup logic - New analyze-skills-notice.test.ts: tests stale project-local skills deprecation notice (warn-only, no deletion) - Update PR tracking doc with progress log All 852 unit tests pass, no regressions. --- docs/pr-skill-installation-refactor.md | 42 +++++- docs/test-plan-skill-installation-refactor.md | 111 +++++++++++++++ gitnexus/src/cli/setup.ts | 4 +- gitnexus/test/unit/ai-context.test.ts | 70 ++++++++++ .../test/unit/analyze-skills-notice.test.ts | 96 +++++++++++++ gitnexus/test/unit/setup-skills.test.ts | 128 ++++++++++++++++++ 6 files changed, 448 insertions(+), 3 deletions(-) create mode 100644 docs/test-plan-skill-installation-refactor.md create mode 100644 gitnexus/test/unit/analyze-skills-notice.test.ts create mode 100644 gitnexus/test/unit/setup-skills.test.ts diff --git a/docs/pr-skill-installation-refactor.md b/docs/pr-skill-installation-refactor.md index b6106635b5..40025e34d2 100644 --- a/docs/pr-skill-installation-refactor.md +++ b/docs/pr-skill-installation-refactor.md @@ -130,5 +130,45 @@ The existing tip at `analyze.ts:363` says "Run `gitnexus setup` to configure MCP |---|---| | `gitnexus/src/cli/ai-context.ts` | Remove `installSkills()` function and its call | | `gitnexus/src/cli/analyze.ts` | Add deprecation notice for stale local skills; update setup tip | -| `gitnexus/src/cli/setup.ts` | Add cleanup of project-local skills during global install | +| `gitnexus/src/cli/setup.ts` | Export `installSkillsTo` and `SKILL_NAMES`; add cleanup of project-local skills during global install | +| `gitnexus/test/unit/ai-context.test.ts` | Add regression guards + post-refactor acceptance placeholders | +| `gitnexus/test/unit/setup-skills.test.ts` | New — tests `installSkillsTo` core logic + setup cleanup placeholders | +| `gitnexus/test/unit/analyze-skills-notice.test.ts` | New — tests stale skills deprecation notice helper | | `README.md` | Fix skill count, clarify command responsibilities | + +--- + +## Progress Log + +### Phase 1: Research & Design (completed) + +- Analyzed both commands (`analyze` and `setup`) to understand how each installs skills +- Identified the duplicate installation problem and its interaction with Claude Code bug [#25209](https://github.com/anthropics/claude-code/issues/25209) +- Consulted Gemini and Codex for architectural review — both agreed on the approach +- Evaluated 4 options (A–D), chose Option A with migration handling +- Documented the full strategy in this file + +### Phase 2: Test Design & Implementation (completed) + +- Designed test plan covering 21 test cases across 3 files (`docs/test-plan-skill-installation-refactor.md`) +- Exported `installSkillsTo` and `SKILL_NAMES` from `setup.ts` to enable direct testing +- Implemented tests in 3 files: + - `test/unit/ai-context.test.ts` — 9 passing + 2 todo (post-refactor acceptance) + - `test/unit/setup-skills.test.ts` — 5 passing + 6 todo (post-refactor acceptance) + - `test/unit/analyze-skills-notice.test.ts` — 4 passing (notice helper works now) +- All 852 unit tests pass (39 files), no regressions +- Safety: all tests use `os.tmpdir()` temp directories, never touch real `~/.claude/` or working repo + +### Phase 3: Implementation (pending) + +- Remove `installSkills()` from `ai-context.ts` +- Add stale skills notice to `analyze` +- Add project-local cleanup to `setup` +- Update `analyze` tip to mention skills +- Flip `todo` tests to active, remove old skills test + +### Phase 4: README Update (pending) + +- Fix skill count (6, not 4) +- List all skills including `gitnexus-guide` and `gitnexus-cli` +- Clarify command responsibilities diff --git a/docs/test-plan-skill-installation-refactor.md b/docs/test-plan-skill-installation-refactor.md new file mode 100644 index 0000000000..c2edd77526 --- /dev/null +++ b/docs/test-plan-skill-installation-refactor.md @@ -0,0 +1,111 @@ +# Test Plan: Skill Installation Refactor + +## Test Infrastructure + +- **Framework**: Vitest (already used, v4.x) +- **Location**: `gitnexus/test/unit/` for unit tests, `gitnexus/test/integration/` for integration +- **Run**: `npm test` (unit only), `npm run test:all` (unit + integration) +- **Isolation**: All tests use `os.tmpdir()` temp directories — no real `~/.claude/` or repo modification + +## Safety Approach + +All filesystem operations happen inside `fs.mkdtemp()` temp directories, cleaned up in `afterAll`. Tests never touch: +- The real `~/.claude/` directory +- The real working repository +- Any global state + +For functions that reference `os.homedir()`, we mock the return value to point at a temp directory. + +## Existing Tests to Update + +### `test/unit/ai-context.test.ts` + +The existing test at line 67-79 (`it('installs skills files')`) tests that `generateAIContextFiles` installs skills. After the refactor, this behavior is removed. This test must be updated. + +| # | Test | Assertion | Notes | +|---|------|-----------|-------| +| 1 | `generateAIContextFiles` does NOT install skills | Skills directory `.claude/skills/gitnexus/` should not exist after calling `generateAIContextFiles` | Inverts existing test | +| 2 | Return value does not mention skills | `result.files` should not contain any skill-related entries | | + +## New Unit Tests: `test/unit/ai-context.test.ts` (additions) + +### `generateAIContextFiles` — post-refactor behavior + +| # | Test | Setup | Assertion | +|---|------|-------|-----------| +| 3 | Still generates CLAUDE.md with dynamic stats | Call with `{ nodes: 100, edges: 200, processes: 10 }` | CLAUDE.md contains `100 symbols`, `200 relationships`, `10 execution flows` | +| 4 | Still generates AGENTS.md | Call with stats | AGENTS.md exists with gitnexus markers | +| 5 | Idempotent — no duplicate sections on re-run | Call twice with different stats | Only one `gitnexus:start` marker, second stats overwrite first | +| 6 | Preserves existing non-GitNexus content in CLAUDE.md | Pre-create CLAUDE.md with custom content, then call | Custom content still present alongside gitnexus section | + +## New Unit Tests: `test/unit/setup-skills.test.ts` + +These test the skill installation logic in `setup.ts`. Since `setupCommand` does many things (MCP config, hooks, skills), we need to test the skill-related functions. The key functions are not currently exported, so we'll either: +- Extract and export `installSkillsTo` (it's already a standalone function) +- Or test via `setupCommand` with mocked filesystem + +### `installSkillsTo` — core skill installation + +| # | Test | Setup | Assertion | +|---|------|-------|-----------| +| 7 | Installs all 6 skills to target directory | Call with temp dir as target | Each of the 6 skill directories exists with a `SKILL.md` file | +| 8 | Each SKILL.md has non-empty content | Call with temp dir | Every `SKILL.md` has length > 0 | +| 9 | Idempotent — re-running overwrites cleanly | Call twice | Same 6 skills, no duplicates, no errors | +| 10 | Handles missing source skill gracefully | Mock one skill file as missing | Other 5 install successfully, missing one is skipped | + +### `setupCommand` — cleanup of project-local skills + +| # | Test | Setup | Assertion | +|---|------|-------|-----------| +| 11 | Removes project-local skills when installing globally | Create `.claude/skills/gitnexus/` in a fake repo dir, mock `process.cwd()` to that dir | Directory removed after setup | +| 12 | Prints notice when removing project-local skills | Same as above, capture console output | Output contains migration notice | +| 13 | Does nothing when no project-local skills exist | Mock cwd to a dir without `.claude/skills/gitnexus/` | No errors, no removal attempted | +| 14 | Does not remove project-local skills if not in a git repo | Create `.claude/skills/gitnexus/` but no `.git` directory | Skills directory left intact (not a repo, don't touch) | + +## New Unit Tests: `test/unit/analyze-skills-notice.test.ts` + +These test the deprecation notice in `analyze` when stale project-local skills are detected. + +Since `analyzeCommand` is heavy (requires KuzuDB, pipeline, etc.), we don't test the full command. Instead, we test the notice logic as an extracted helper or verify it through console output mocking. + +| # | Test | Setup | Assertion | +|---|------|-------|-----------| +| 15 | Prints deprecation notice when `.claude/skills/gitnexus/` exists | Create the directory in temp repo, capture console | Output contains "no longer installed by analyze" or similar | +| 16 | No notice when `.claude/skills/gitnexus/` does not exist | Don't create the directory | No skill-related console output | +| 17 | Notice does NOT delete the directory | Create directory, run the check | Directory still exists after | + +## Edge Cases + +| # | Test | Location | Setup | Assertion | +|---|------|----------|-------|-----------| +| 18 | Existing CLAUDE.md with gitnexus section but no skills dir | ai-context test | Pre-create CLAUDE.md with markers, no skills dir | Updates section, doesn't create skills dir | +| 19 | Skills dir exists but is empty | setup test | Create empty `.claude/skills/gitnexus/` | Cleanup still removes it | +| 20 | Skills dir contains extra files (user-modified) | setup test | Add custom file alongside skill files | Still removes entire gitnexus skills dir (it's our namespace) | +| 21 | Read-only skills directory | setup test | Create dir with restricted permissions | Graceful error, doesn't crash | + +## Test File Summary + +| File | Tests | Type | +|------|-------|------| +| `test/unit/ai-context.test.ts` | #1-6, #18 | Unit — update existing + add new | +| `test/unit/setup-skills.test.ts` | #7-14, #19-21 | Unit — new file | +| `test/unit/analyze-skills-notice.test.ts` | #15-17 | Unit — new file | + +## Running Tests Pre-Implementation + +These tests can be written and run **before** the actual refactor. Here's the strategy: + +- **Tests that verify NEW behavior** (#1, #2, #15-17): Write them now, expect them to **fail**. They become the acceptance criteria — when they pass, the refactor is correct. +- **Tests that verify EXISTING behavior we're keeping** (#3-6, #18): Write them now, expect them to **pass** both before and after the refactor. They're regression guards. +- **Tests for setup skills** (#7-14, #19-21): These test `installSkillsTo` which already exists in `setup.ts`. They can pass now if we export the function. They verify the setup path works correctly regardless of the refactor. + +## Execution + +```bash +# Run just the skill-related tests +cd gitnexus +npx vitest run test/unit/ai-context.test.ts test/unit/setup-skills.test.ts test/unit/analyze-skills-notice.test.ts + +# Run all unit tests (verify no regressions) +npm test +``` diff --git a/gitnexus/src/cli/setup.ts b/gitnexus/src/cli/setup.ts index 79cd6eba55..8adf6bb9a6 100644 --- a/gitnexus/src/cli/setup.ts +++ b/gitnexus/src/cli/setup.ts @@ -240,7 +240,7 @@ async function setupOpenCode(result: SetupResult): Promise { // ─── Skill Installation ─────────────────────────────────────────── -const SKILL_NAMES = ['gitnexus-exploring', 'gitnexus-debugging', 'gitnexus-impact-analysis', 'gitnexus-refactoring', 'gitnexus-guide', 'gitnexus-cli']; +export const SKILL_NAMES = ['gitnexus-exploring', 'gitnexus-debugging', 'gitnexus-impact-analysis', 'gitnexus-refactoring', 'gitnexus-guide', 'gitnexus-cli']; /** * Install GitNexus skills to a target directory. @@ -251,7 +251,7 @@ const SKILL_NAMES = ['gitnexus-exploring', 'gitnexus-debugging', 'gitnexus-impac * - Flat file: skills/{name}.md → copied as SKILL.md * - Directory: skills/{name}/SKILL.md → copied recursively (includes references/, etc.) */ -async function installSkillsTo(targetDir: string): Promise { +export async function installSkillsTo(targetDir: string): Promise { const installed: string[] = []; const skillsRoot = path.join(__dirname, '..', '..', 'skills'); diff --git a/gitnexus/test/unit/ai-context.test.ts b/gitnexus/test/unit/ai-context.test.ts index 6eb47b78fb..f6210e3011 100644 --- a/gitnexus/test/unit/ai-context.test.ts +++ b/gitnexus/test/unit/ai-context.test.ts @@ -77,4 +77,74 @@ describe('generateAIContextFiles', () => { // Skills dir may not be created if skills source doesn't exist in test context } }); + + // ── POST-REFACTOR: these will replace the above test ───────────── + // After the refactor, generateAIContextFiles should NOT install skills. + // Uncomment these and remove the test above once the refactor lands. + + it.todo('does NOT install skills after refactor'); + // const stats = { nodes: 10 }; + // await generateAIContextFiles(tmpDir, storagePath, 'TestProject', stats); + // const skillsDir = path.join(tmpDir, '.claude', 'skills', 'gitnexus'); + // await expect(fs.stat(skillsDir)).rejects.toThrow(); // Should not exist + + it.todo('return value does not mention skills after refactor'); + // const stats = { nodes: 10 }; + // const result = await generateAIContextFiles(tmpDir, storagePath, 'TestProject', stats); + // const hasSkillEntry = result.files.some(f => f.includes('skills')); + // expect(hasSkillEntry).toBe(false); + + // ── Regression guards (should pass before AND after refactor) ──── + + it('generates CLAUDE.md with dynamic stats', async () => { + const stats = { nodes: 42, edges: 84, processes: 7 }; + await generateAIContextFiles(tmpDir, storagePath, 'StatsProject', stats); + + const content = await fs.readFile(path.join(tmpDir, 'CLAUDE.md'), 'utf-8'); + expect(content).toContain('42 symbols'); + expect(content).toContain('84 relationships'); + expect(content).toContain('7 execution flows'); + }); + + it('generates AGENTS.md alongside CLAUDE.md', async () => { + const stats = { nodes: 10, edges: 20, processes: 3 }; + await generateAIContextFiles(tmpDir, storagePath, 'TestProject', stats); + + const agentsPath = path.join(tmpDir, 'AGENTS.md'); + const content = await fs.readFile(agentsPath, 'utf-8'); + expect(content).toContain('gitnexus:start'); + expect(content).toContain('gitnexus:end'); + }); + + it('preserves existing non-GitNexus content in CLAUDE.md', async () => { + // Pre-create CLAUDE.md with custom content + const claudePath = path.join(tmpDir, 'CLAUDE.md'); + await fs.writeFile(claudePath, '# My Custom Instructions\n\nDo not remove this.\n', 'utf-8'); + + const stats = { nodes: 10 }; + await generateAIContextFiles(tmpDir, storagePath, 'TestProject', stats); + + const content = await fs.readFile(claudePath, 'utf-8'); + expect(content).toContain('My Custom Instructions'); + expect(content).toContain('Do not remove this.'); + expect(content).toContain('gitnexus:start'); + }); + + it('existing CLAUDE.md with gitnexus section but no skills dir works', async () => { + // Pre-create CLAUDE.md with an existing gitnexus section + const claudePath = path.join(tmpDir, 'CLAUDE.md'); + await fs.writeFile(claudePath, '\nold content\n\n', 'utf-8'); + + const stats = { nodes: 99 }; + await generateAIContextFiles(tmpDir, storagePath, 'UpdatedProject', stats); + + const content = await fs.readFile(claudePath, 'utf-8'); + // Old content replaced + expect(content).not.toContain('old content'); + // New content present + expect(content).toContain('99 symbols'); + // Still only one section + const starts = (content.match(/gitnexus:start/g) || []).length; + expect(starts).toBe(1); + }); }); diff --git a/gitnexus/test/unit/analyze-skills-notice.test.ts b/gitnexus/test/unit/analyze-skills-notice.test.ts new file mode 100644 index 0000000000..c7d5c3dcd3 --- /dev/null +++ b/gitnexus/test/unit/analyze-skills-notice.test.ts @@ -0,0 +1,96 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import fs from 'fs/promises'; +import path from 'path'; +import os from 'os'; + +/** + * Tests for the deprecation notice in analyze when stale project-local skills exist. + * + * After the refactor, `analyze` no longer installs skills but should warn + * if it detects a leftover `.claude/skills/gitnexus/` directory from a prior run. + * + * The notice logic will be extracted as a small helper from analyze.ts (or ai-context.ts) + * so we can test it without running the full analyze pipeline. + */ + +// Placeholder for the function that will be extracted during refactor. +// For now we define the expected interface and test against it. +// Once implemented, update the import to point to the real function. + +/** + * Check for stale project-local skills and print a deprecation notice. + * Returns true if stale skills were detected. + */ +async function checkStaleProjectSkills(repoPath: string): Promise { + const skillsDir = path.join(repoPath, '.claude', 'skills', 'gitnexus'); + try { + const stat = await fs.stat(skillsDir); + if (stat.isDirectory()) { + console.log(` Note: Skills are no longer installed by analyze. Run 'gitnexus setup' to manage skills globally.`); + return true; + } + } catch { + // Directory doesn't exist — nothing to warn about + } + return false; +} + +describe('analyze — stale project-local skills notice', () => { + let tmpDir: string; + let consoleOutput: string[]; + + beforeEach(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-analyze-notice-test-')); + consoleOutput = []; + vi.spyOn(console, 'log').mockImplementation((...args: any[]) => { + consoleOutput.push(args.map(String).join(' ')); + }); + }); + + afterEach(async () => { + vi.restoreAllMocks(); + try { + await fs.rm(tmpDir, { recursive: true, force: true }); + } catch { /* best-effort */ } + }); + + it('prints deprecation notice when .claude/skills/gitnexus/ exists', async () => { + // Create stale project-local skills directory with a skill file inside + const skillSubDir = path.join(tmpDir, '.claude', 'skills', 'gitnexus', 'gitnexus-exploring'); + await fs.mkdir(skillSubDir, { recursive: true }); + await fs.writeFile(path.join(skillSubDir, 'SKILL.md'), 'stale'); + + const detected = await checkStaleProjectSkills(tmpDir); + + expect(detected).toBe(true); + const notice = consoleOutput.find(line => line.includes('no longer installed by analyze')); + expect(notice).toBeDefined(); + }); + + it('prints no notice when .claude/skills/gitnexus/ does not exist', async () => { + const detected = await checkStaleProjectSkills(tmpDir); + + expect(detected).toBe(false); + const notice = consoleOutput.find(line => line.includes('no longer installed by analyze')); + expect(notice).toBeUndefined(); + }); + + it('does NOT delete the directory — only warns', async () => { + const skillsDir = path.join(tmpDir, '.claude', 'skills', 'gitnexus'); + await fs.mkdir(skillsDir, { recursive: true }); + + await checkStaleProjectSkills(tmpDir); + + // Directory must still exist + const stat = await fs.stat(skillsDir); + expect(stat.isDirectory()).toBe(true); + }); + + it('handles .claude dir existing without skills/gitnexus/', async () => { + // .claude exists but no skills subdirectory + await fs.mkdir(path.join(tmpDir, '.claude'), { recursive: true }); + + const detected = await checkStaleProjectSkills(tmpDir); + expect(detected).toBe(false); + }); +}); diff --git a/gitnexus/test/unit/setup-skills.test.ts b/gitnexus/test/unit/setup-skills.test.ts new file mode 100644 index 0000000000..dfdebf2282 --- /dev/null +++ b/gitnexus/test/unit/setup-skills.test.ts @@ -0,0 +1,128 @@ +import { describe, it, expect, vi, beforeEach, afterEach, afterAll, beforeAll } from 'vitest'; +import fs from 'fs/promises'; +import path from 'path'; +import os from 'os'; +import { installSkillsTo, SKILL_NAMES } from '../../src/cli/setup.js'; + +describe('installSkillsTo', () => { + let tmpDir: string; + + beforeEach(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-setup-skills-test-')); + }); + + afterEach(async () => { + try { + await fs.rm(tmpDir, { recursive: true, force: true }); + } catch { /* best-effort */ } + }); + + it('installs all 6 skills to target directory', async () => { + const installed = await installSkillsTo(tmpDir); + expect(installed).toHaveLength(SKILL_NAMES.length); + expect(installed.sort()).toEqual([...SKILL_NAMES].sort()); + }); + + it('creates SKILL.md for each skill', async () => { + await installSkillsTo(tmpDir); + + for (const name of SKILL_NAMES) { + const skillFile = path.join(tmpDir, name, 'SKILL.md'); + const stat = await fs.stat(skillFile); + expect(stat.isFile()).toBe(true); + } + }); + + it('each SKILL.md has non-empty content', async () => { + await installSkillsTo(tmpDir); + + for (const name of SKILL_NAMES) { + const skillFile = path.join(tmpDir, name, 'SKILL.md'); + const content = await fs.readFile(skillFile, 'utf-8'); + expect(content.length).toBeGreaterThan(0); + } + }); + + it('is idempotent — re-running overwrites cleanly', async () => { + await installSkillsTo(tmpDir); + const firstRun = await installSkillsTo(tmpDir); + + expect(firstRun).toHaveLength(SKILL_NAMES.length); + + // Verify no duplicate directories + const entries = await fs.readdir(tmpDir); + const skillDirs = entries.filter(e => e.startsWith('gitnexus-')); + expect(skillDirs).toHaveLength(SKILL_NAMES.length); + }); + + it('handles missing source skill gracefully', async () => { + // installSkillsTo reads from the package skills/ directory. + // If a skill source file doesn't exist, it silently skips. + // We can't easily mock the filesystem here, but we can verify + // that the function doesn't throw even when called normally. + const installed = await installSkillsTo(tmpDir); + // At minimum, should not throw and should return an array + expect(Array.isArray(installed)).toBe(true); + }); +}); + +describe('setupCommand — project-local skill cleanup', () => { + let tmpHome: string; + let tmpRepo: string; + let originalCwd: string; + let consoleOutput: string[]; + + beforeEach(async () => { + tmpHome = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-setup-home-')); + tmpRepo = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-setup-repo-')); + originalCwd = process.cwd(); + consoleOutput = []; + + // Capture console.log output + vi.spyOn(console, 'log').mockImplementation((...args: any[]) => { + consoleOutput.push(args.map(String).join(' ')); + }); + vi.spyOn(console, 'warn').mockImplementation((...args: any[]) => { + consoleOutput.push(args.map(String).join(' ')); + }); + }); + + afterEach(async () => { + process.chdir(originalCwd); + vi.restoreAllMocks(); + try { + await fs.rm(tmpHome, { recursive: true, force: true }); + await fs.rm(tmpRepo, { recursive: true, force: true }); + } catch { /* best-effort */ } + }); + + // These tests document expected POST-REFACTOR behavior. + // They will FAIL until the refactor is implemented, serving as acceptance criteria. + + it.todo('removes project-local skills when installing globally'); + // Setup: create /.claude/skills/gitnexus/ with skill files + // Mock process.cwd() to tmpRepo, os.homedir() to tmpHome + // Run setupCommand + // Assert: /.claude/skills/gitnexus/ no longer exists + + it.todo('prints notice when removing project-local skills'); + // Same setup as above + // Assert: consoleOutput contains migration notice + + it.todo('does nothing when no project-local skills exist'); + // Mock cwd to tmpRepo (no .claude/skills/gitnexus/) + // Run setupCommand + // Assert: no errors, no removal-related output + + it.todo('does not remove project-local skills if not in a git repo'); + // Create .claude/skills/gitnexus/ but no .git directory + // Assert: skills directory left intact + + it.todo('removes empty project-local skills directory'); + // Create empty .claude/skills/gitnexus/ + // Assert: cleaned up + + it.todo('removes project-local skills dir even with extra files'); + // Create .claude/skills/gitnexus/ with a custom extra file + // Assert: entire gitnexus/ dir removed (it's our namespace) +}); From c3f699513ec583e159a4b15e5d8f7f9fdfe220c1 Mon Sep 17 00:00:00 2001 From: Linus Beckhaus Date: Sat, 7 Mar 2026 13:07:13 +0100 Subject: [PATCH 04/15] test(skills): activate pre-refactor acceptance suite Turn skill-installation refactor acceptance tests from todo placeholders into active checks. Harden weak assertions and isolate temp dirs per test. Document the test-phase changelog and expected red-state before implementation. --- docs/pr-skill-installation-refactor.md | 27 ++-- docs/test-plan-skill-installation-refactor.md | 24 +++- gitnexus/test/unit/ai-context.test.ts | 49 +++----- .../test/unit/analyze-skills-notice.test.ts | 42 +++---- gitnexus/test/unit/setup-skills.test.ts | 118 +++++++++++++----- 5 files changed, 161 insertions(+), 99 deletions(-) diff --git a/docs/pr-skill-installation-refactor.md b/docs/pr-skill-installation-refactor.md index 40025e34d2..59b2d99bd7 100644 --- a/docs/pr-skill-installation-refactor.md +++ b/docs/pr-skill-installation-refactor.md @@ -131,10 +131,12 @@ The existing tip at `analyze.ts:363` says "Run `gitnexus setup` to configure MCP | `gitnexus/src/cli/ai-context.ts` | Remove `installSkills()` function and its call | | `gitnexus/src/cli/analyze.ts` | Add deprecation notice for stale local skills; update setup tip | | `gitnexus/src/cli/setup.ts` | Export `installSkillsTo` and `SKILL_NAMES`; add cleanup of project-local skills during global install | -| `gitnexus/test/unit/ai-context.test.ts` | Add regression guards + post-refactor acceptance placeholders | -| `gitnexus/test/unit/setup-skills.test.ts` | New — tests `installSkillsTo` core logic + setup cleanup placeholders | -| `gitnexus/test/unit/analyze-skills-notice.test.ts` | New — tests stale skills deprecation notice helper | +| `gitnexus/test/unit/ai-context.test.ts` | Regression guards + active acceptance tests that assert `analyze` no longer installs skills | +| `gitnexus/test/unit/setup-skills.test.ts` | `installSkillsTo` core tests + active `setupCommand` cleanup acceptance tests | +| `gitnexus/test/unit/analyze-skills-notice.test.ts` | Contract tests bound to production export (`checkStaleProjectSkills`) instead of local helper | | `README.md` | Fix skill count, clarify command responsibilities | +| `docs/test-plan-skill-installation-refactor.md` | Add test-phase changelog and rationale for active acceptance tests | +| `docs/pr-skill-installation-refactor.md` | Add progress changelog for test hardening | --- @@ -152,20 +154,27 @@ The existing tip at `analyze.ts:363` says "Run `gitnexus setup` to configure MCP - Designed test plan covering 21 test cases across 3 files (`docs/test-plan-skill-installation-refactor.md`) - Exported `installSkillsTo` and `SKILL_NAMES` from `setup.ts` to enable direct testing -- Implemented tests in 3 files: - - `test/unit/ai-context.test.ts` — 9 passing + 2 todo (post-refactor acceptance) - - `test/unit/setup-skills.test.ts` — 5 passing + 6 todo (post-refactor acceptance) - - `test/unit/analyze-skills-notice.test.ts` — 4 passing (notice helper works now) -- All 852 unit tests pass (39 files), no regressions +- Implemented tests in 3 files and hardened weak assertions (no swallowed failures, isolated temp dirs per test) +- Converted acceptance placeholders to active tests for post-refactor behavior +- Replaced the analyze notice test-only helper with a production-contract test that requires `analyze.ts` export wiring +- Current intent: acceptance tests stay red until Phase 3 implementation is complete; regression guards stay green - Safety: all tests use `os.tmpdir()` temp directories, never touch real `~/.claude/` or working repo +### Phase 2A: Test Hardening Changelog (2026-03-07) + +- Activated acceptance tests in `ai-context.test.ts` (#1, #2) to enforce "no skills from analyze" behavior. +- Activated migration cleanup acceptance tests in `setup-skills.test.ts` (#11, #12, #19, #20). +- Kept `setup-skills` non-cleanup assertions green and made missing-source behavior explicit via targeted FS mocking. +- Converted `analyze-skills-notice.test.ts` from local placeholder implementation to production export contract checks. +- Deferred cleanup edge case #21 (read-only directory) until cleanup implementation exists to avoid false signal. + ### Phase 3: Implementation (pending) - Remove `installSkills()` from `ai-context.ts` - Add stale skills notice to `analyze` - Add project-local cleanup to `setup` - Update `analyze` tip to mention skills -- Flip `todo` tests to active, remove old skills test +- Make active acceptance tests pass ### Phase 4: README Update (pending) diff --git a/docs/test-plan-skill-installation-refactor.md b/docs/test-plan-skill-installation-refactor.md index c2edd77526..1b5eb8a682 100644 --- a/docs/test-plan-skill-installation-refactor.md +++ b/docs/test-plan-skill-installation-refactor.md @@ -95,9 +95,10 @@ Since `analyzeCommand` is heavy (requires KuzuDB, pipeline, etc.), we don't test These tests can be written and run **before** the actual refactor. Here's the strategy: -- **Tests that verify NEW behavior** (#1, #2, #15-17): Write them now, expect them to **fail**. They become the acceptance criteria — when they pass, the refactor is correct. +- **Tests that verify NEW behavior** (#1, #2, #11, #12, #15-17, #19, #20): Keep them active and expect them to **fail** until implementation lands. They are the acceptance criteria. - **Tests that verify EXISTING behavior we're keeping** (#3-6, #18): Write them now, expect them to **pass** both before and after the refactor. They're regression guards. -- **Tests for setup skills** (#7-14, #19-21): These test `installSkillsTo` which already exists in `setup.ts`. They can pass now if we export the function. They verify the setup path works correctly regardless of the refactor. +- **Tests for setup skills** (#7-10): These test `installSkillsTo` directly and should pass now. +- **Deferred edge case** (#21): Keep as planned while cleanup implementation is pending. ## Execution @@ -109,3 +110,22 @@ npx vitest run test/unit/ai-context.test.ts test/unit/setup-skills.test.ts test/ # Run all unit tests (verify no regressions) npm test ``` + +## Changelog + +### 2026-03-07 — Pre-refactor test hardening + +- Switched acceptance checks from placeholder `it.todo(...)` to active tests for: + - `generateAIContextFiles` no longer installing skills (#1, #2) + - `setupCommand` migration cleanup behavior (#11, #12, #19, #20) +- Reworked `analyze` notice tests to target production-code contract instead of a local test-only helper: + - Tests now require `analyze.ts` to export `checkStaleProjectSkills(repoPath)` and validate behavior through that symbol (#15-#17). + - This removes false confidence where tests could pass without any production integration. +- Strengthened weak-path assertions: + - `ai-context` tests now use per-test temp directories (`beforeEach`/`afterEach`) to remove shared state leakage. + - Replaced the permissive "installs skills files" try/catch test with strict assertions. + - `installSkillsTo` missing-source test now simulates one missing skill via mocked `fs.readFile` and verifies partial install outcome (#10). +- Intentional status in this phase: + - Acceptance tests are expected to fail until refactor implementation lands. + - Regression/behavior-preservation tests should continue to pass. + - Read-only cleanup scenario (#21) remains planned and should be finalized when cleanup implementation exists. diff --git a/gitnexus/test/unit/ai-context.test.ts b/gitnexus/test/unit/ai-context.test.ts index f6210e3011..3d6162f005 100644 --- a/gitnexus/test/unit/ai-context.test.ts +++ b/gitnexus/test/unit/ai-context.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeAll, afterAll } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import fs from 'fs/promises'; import path from 'path'; import os from 'os'; @@ -8,13 +8,13 @@ describe('generateAIContextFiles', () => { let tmpDir: string; let storagePath: string; - beforeAll(async () => { + beforeEach(async () => { tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-ai-ctx-test-')); storagePath = path.join(tmpDir, '.gitnexus'); await fs.mkdir(storagePath, { recursive: true }); }); - afterAll(async () => { + afterEach(async () => { try { await fs.rm(tmpDir, { recursive: true, force: true }); } catch { /* best-effort */ } @@ -50,11 +50,12 @@ describe('generateAIContextFiles', () => { }); it('updates existing CLAUDE.md without duplicating', async () => { - const stats = { nodes: 10 }; + const firstStats = { nodes: 10, edges: 20, processes: 2 }; + const secondStats = { nodes: 11, edges: 22, processes: 3 }; // Run twice - await generateAIContextFiles(tmpDir, storagePath, 'TestProject', stats); - await generateAIContextFiles(tmpDir, storagePath, 'TestProject', stats); + await generateAIContextFiles(tmpDir, storagePath, 'TestProject', firstStats); + await generateAIContextFiles(tmpDir, storagePath, 'TestProject', secondStats); const claudeMdPath = path.join(tmpDir, 'CLAUDE.md'); const content = await fs.readFile(claudeMdPath, 'utf-8'); @@ -62,37 +63,24 @@ describe('generateAIContextFiles', () => { // Should only have one gitnexus section const starts = (content.match(/gitnexus:start/g) || []).length; expect(starts).toBe(1); + expect(content).toContain('11 symbols'); + expect(content).not.toContain('10 symbols'); }); - it('installs skills files', async () => { + it('does NOT install skills after refactor', async () => { const stats = { nodes: 10 }; - const result = await generateAIContextFiles(tmpDir, storagePath, 'TestProject', stats); + await generateAIContextFiles(tmpDir, storagePath, 'TestProject', stats); - // Should have installed skill files const skillsDir = path.join(tmpDir, '.claude', 'skills', 'gitnexus'); - try { - const entries = await fs.readdir(skillsDir, { recursive: true }); - expect(entries.length).toBeGreaterThan(0); - } catch { - // Skills dir may not be created if skills source doesn't exist in test context - } + await expect(fs.stat(skillsDir)).rejects.toThrow(); }); - // ── POST-REFACTOR: these will replace the above test ───────────── - // After the refactor, generateAIContextFiles should NOT install skills. - // Uncomment these and remove the test above once the refactor lands. - - it.todo('does NOT install skills after refactor'); - // const stats = { nodes: 10 }; - // await generateAIContextFiles(tmpDir, storagePath, 'TestProject', stats); - // const skillsDir = path.join(tmpDir, '.claude', 'skills', 'gitnexus'); - // await expect(fs.stat(skillsDir)).rejects.toThrow(); // Should not exist - - it.todo('return value does not mention skills after refactor'); - // const stats = { nodes: 10 }; - // const result = await generateAIContextFiles(tmpDir, storagePath, 'TestProject', stats); - // const hasSkillEntry = result.files.some(f => f.includes('skills')); - // expect(hasSkillEntry).toBe(false); + it('return value does not mention skills after refactor', async () => { + const stats = { nodes: 10 }; + const result = await generateAIContextFiles(tmpDir, storagePath, 'TestProject', stats); + const hasSkillEntry = result.files.some(f => f.includes('skills')); + expect(hasSkillEntry).toBe(false); + }); // ── Regression guards (should pass before AND after refactor) ──── @@ -146,5 +134,6 @@ describe('generateAIContextFiles', () => { // Still only one section const starts = (content.match(/gitnexus:start/g) || []).length; expect(starts).toBe(1); + await expect(fs.stat(path.join(tmpDir, '.claude', 'skills', 'gitnexus'))).rejects.toThrow(); }); }); diff --git a/gitnexus/test/unit/analyze-skills-notice.test.ts b/gitnexus/test/unit/analyze-skills-notice.test.ts index c7d5c3dcd3..912b521387 100644 --- a/gitnexus/test/unit/analyze-skills-notice.test.ts +++ b/gitnexus/test/unit/analyze-skills-notice.test.ts @@ -4,35 +4,19 @@ import path from 'path'; import os from 'os'; /** - * Tests for the deprecation notice in analyze when stale project-local skills exist. - * - * After the refactor, `analyze` no longer installs skills but should warn - * if it detects a leftover `.claude/skills/gitnexus/` directory from a prior run. - * - * The notice logic will be extracted as a small helper from analyze.ts (or ai-context.ts) - * so we can test it without running the full analyze pipeline. + * Contract tests for analyze stale-skills notice. + * These are intentionally active pre-refactor acceptance tests: + * they should fail until analyze exports and uses checkStaleProjectSkills(). */ -// Placeholder for the function that will be extracted during refactor. -// For now we define the expected interface and test against it. -// Once implemented, update the import to point to the real function. - -/** - * Check for stale project-local skills and print a deprecation notice. - * Returns true if stale skills were detected. - */ -async function checkStaleProjectSkills(repoPath: string): Promise { - const skillsDir = path.join(repoPath, '.claude', 'skills', 'gitnexus'); - try { - const stat = await fs.stat(skillsDir); - if (stat.isDirectory()) { - console.log(` Note: Skills are no longer installed by analyze. Run 'gitnexus setup' to manage skills globally.`); - return true; - } - } catch { - // Directory doesn't exist — nothing to warn about - } - return false; +async function getCheckStaleProjectSkills(): Promise<(repoPath: string) => Promise> { + const analyzeModule = await import('../../src/cli/analyze.js'); + const candidate = (analyzeModule as any).checkStaleProjectSkills; + expect( + typeof candidate, + 'analyze.ts must export checkStaleProjectSkills(repoPath) for unit testing', + ).toBe('function'); + return candidate as (repoPath: string) => Promise; } describe('analyze — stale project-local skills notice', () => { @@ -60,6 +44,7 @@ describe('analyze — stale project-local skills notice', () => { await fs.mkdir(skillSubDir, { recursive: true }); await fs.writeFile(path.join(skillSubDir, 'SKILL.md'), 'stale'); + const checkStaleProjectSkills = await getCheckStaleProjectSkills(); const detected = await checkStaleProjectSkills(tmpDir); expect(detected).toBe(true); @@ -68,6 +53,7 @@ describe('analyze — stale project-local skills notice', () => { }); it('prints no notice when .claude/skills/gitnexus/ does not exist', async () => { + const checkStaleProjectSkills = await getCheckStaleProjectSkills(); const detected = await checkStaleProjectSkills(tmpDir); expect(detected).toBe(false); @@ -79,6 +65,7 @@ describe('analyze — stale project-local skills notice', () => { const skillsDir = path.join(tmpDir, '.claude', 'skills', 'gitnexus'); await fs.mkdir(skillsDir, { recursive: true }); + const checkStaleProjectSkills = await getCheckStaleProjectSkills(); await checkStaleProjectSkills(tmpDir); // Directory must still exist @@ -90,6 +77,7 @@ describe('analyze — stale project-local skills notice', () => { // .claude exists but no skills subdirectory await fs.mkdir(path.join(tmpDir, '.claude'), { recursive: true }); + const checkStaleProjectSkills = await getCheckStaleProjectSkills(); const detected = await checkStaleProjectSkills(tmpDir); expect(detected).toBe(false); }); diff --git a/gitnexus/test/unit/setup-skills.test.ts b/gitnexus/test/unit/setup-skills.test.ts index dfdebf2282..a110a29baa 100644 --- a/gitnexus/test/unit/setup-skills.test.ts +++ b/gitnexus/test/unit/setup-skills.test.ts @@ -1,8 +1,8 @@ -import { describe, it, expect, vi, beforeEach, afterEach, afterAll, beforeAll } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import fs from 'fs/promises'; import path from 'path'; import os from 'os'; -import { installSkillsTo, SKILL_NAMES } from '../../src/cli/setup.js'; +import { installSkillsTo, SKILL_NAMES, setupCommand } from '../../src/cli/setup.js'; describe('installSkillsTo', () => { let tmpDir: string; @@ -56,13 +56,28 @@ describe('installSkillsTo', () => { }); it('handles missing source skill gracefully', async () => { - // installSkillsTo reads from the package skills/ directory. - // If a skill source file doesn't exist, it silently skips. - // We can't easily mock the filesystem here, but we can verify - // that the function doesn't throw even when called normally. + const missingSkill = SKILL_NAMES[SKILL_NAMES.length - 1]; + const originalReadFile = fs.readFile.bind(fs); + const readFileSpy = vi.spyOn(fs, 'readFile').mockImplementation(async (...args: any[]) => { + const filePath = String(args[0]); + if (filePath.endsWith(`${missingSkill}.md`)) { + const err = new Error('ENOENT'); + (err as NodeJS.ErrnoException).code = 'ENOENT'; + throw err; + } + return originalReadFile(...args as any); + }); + const installed = await installSkillsTo(tmpDir); - // At minimum, should not throw and should return an array - expect(Array.isArray(installed)).toBe(true); + readFileSpy.mockRestore(); + + expect(installed).toHaveLength(SKILL_NAMES.length - 1); + expect(installed).not.toContain(missingSkill); + for (const name of SKILL_NAMES.filter(n => n !== missingSkill)) { + const skillFile = path.join(tmpDir, name, 'SKILL.md'); + const stat = await fs.stat(skillFile); + expect(stat.isFile()).toBe(true); + } }); }); @@ -77,6 +92,9 @@ describe('setupCommand — project-local skill cleanup', () => { tmpRepo = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-setup-repo-')); originalCwd = process.cwd(); consoleOutput = []; + process.chdir(tmpRepo); + + vi.spyOn(os, 'homedir').mockReturnValue(tmpHome); // Capture console.log output vi.spyOn(console, 'log').mockImplementation((...args: any[]) => { @@ -96,33 +114,71 @@ describe('setupCommand — project-local skill cleanup', () => { } catch { /* best-effort */ } }); - // These tests document expected POST-REFACTOR behavior. - // They will FAIL until the refactor is implemented, serving as acceptance criteria. + it('removes project-local skills when installing globally', async () => { + await fs.mkdir(path.join(tmpHome, '.claude'), { recursive: true }); + await fs.mkdir(path.join(tmpRepo, '.git'), { recursive: true }); + const localSkillsDir = path.join(tmpRepo, '.claude', 'skills', 'gitnexus'); + await fs.mkdir(path.join(localSkillsDir, 'gitnexus-exploring'), { recursive: true }); + await fs.writeFile(path.join(localSkillsDir, 'gitnexus-exploring', 'SKILL.md'), 'legacy skill'); + + await setupCommand(); + + await expect(fs.stat(localSkillsDir)).rejects.toThrow(); + }); - it.todo('removes project-local skills when installing globally'); - // Setup: create /.claude/skills/gitnexus/ with skill files - // Mock process.cwd() to tmpRepo, os.homedir() to tmpHome - // Run setupCommand - // Assert: /.claude/skills/gitnexus/ no longer exists + it('prints notice when removing project-local skills', async () => { + await fs.mkdir(path.join(tmpHome, '.claude'), { recursive: true }); + await fs.mkdir(path.join(tmpRepo, '.git'), { recursive: true }); + const localSkillsDir = path.join(tmpRepo, '.claude', 'skills', 'gitnexus'); + await fs.mkdir(localSkillsDir, { recursive: true }); - it.todo('prints notice when removing project-local skills'); - // Same setup as above - // Assert: consoleOutput contains migration notice + await setupCommand(); - it.todo('does nothing when no project-local skills exist'); - // Mock cwd to tmpRepo (no .claude/skills/gitnexus/) - // Run setupCommand - // Assert: no errors, no removal-related output + const migrationNotice = consoleOutput.find(line => line.includes('Removed project-local skills')); + expect(migrationNotice).toBeDefined(); + }); - it.todo('does not remove project-local skills if not in a git repo'); - // Create .claude/skills/gitnexus/ but no .git directory - // Assert: skills directory left intact + it('does nothing when no project-local skills exist', async () => { + await fs.mkdir(path.join(tmpHome, '.claude'), { recursive: true }); + await fs.mkdir(path.join(tmpRepo, '.git'), { recursive: true }); - it.todo('removes empty project-local skills directory'); - // Create empty .claude/skills/gitnexus/ - // Assert: cleaned up + await setupCommand(); - it.todo('removes project-local skills dir even with extra files'); - // Create .claude/skills/gitnexus/ with a custom extra file - // Assert: entire gitnexus/ dir removed (it's our namespace) + const migrationNotice = consoleOutput.find(line => line.includes('Removed project-local skills')); + expect(migrationNotice).toBeUndefined(); + }); + + it('does not remove project-local skills if not in a git repo', async () => { + await fs.mkdir(path.join(tmpHome, '.claude'), { recursive: true }); + const localSkillsDir = path.join(tmpRepo, '.claude', 'skills', 'gitnexus'); + await fs.mkdir(localSkillsDir, { recursive: true }); + + await setupCommand(); + + const stat = await fs.stat(localSkillsDir); + expect(stat.isDirectory()).toBe(true); + }); + + it('removes empty project-local skills directory', async () => { + await fs.mkdir(path.join(tmpHome, '.claude'), { recursive: true }); + await fs.mkdir(path.join(tmpRepo, '.git'), { recursive: true }); + const localSkillsDir = path.join(tmpRepo, '.claude', 'skills', 'gitnexus'); + await fs.mkdir(localSkillsDir, { recursive: true }); + + await setupCommand(); + + await expect(fs.stat(localSkillsDir)).rejects.toThrow(); + }); + + it('removes project-local skills dir even with extra files', async () => { + await fs.mkdir(path.join(tmpHome, '.claude'), { recursive: true }); + await fs.mkdir(path.join(tmpRepo, '.git'), { recursive: true }); + const localSkillsDir = path.join(tmpRepo, '.claude', 'skills', 'gitnexus'); + await fs.mkdir(localSkillsDir, { recursive: true }); + await fs.writeFile(path.join(localSkillsDir, 'README.txt'), 'user custom note'); + + await setupCommand(); + + await expect(fs.stat(localSkillsDir)).rejects.toThrow(); + }); }); From c2ffb76c667626a7b588dc4553f715d6562d5577 Mon Sep 17 00:00:00 2001 From: Linus Beckhaus Date: Sat, 7 Mar 2026 13:10:23 +0100 Subject: [PATCH 05/15] refactor: unify skill installation in setup, remove from analyze MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove installSkills() from ai-context.ts — generateAIContextFiles now only produces CLAUDE.md and AGENTS.md with dynamic index stats - Clean up unused fileURLToPath/__dirname imports from ai-context.ts - Export checkStaleProjectSkills() from analyze.ts that warns users about stale project-local skills without deleting them - Add cleanupProjectLocalSkills() to setup.ts that removes .claude/skills/gitnexus/ when running in a git repo, since skills are now installed globally - Update analyze setup tip to mention skills alongside MCP Skills are static assets that don't depend on the repository index. Installing them on every analyze run was unnecessary and caused duplicate entries in Claude Code when both setup and analyze had been run (known Claude Code bug #25209). Setup is now the single owner of skill installation. All 859 unit tests pass. --- docs/pr-skill-installation-refactor.md | 15 ++--- gitnexus/src/cli/ai-context.ts | 87 -------------------------- gitnexus/src/cli/analyze.ts | 23 ++++++- gitnexus/src/cli/setup.ts | 37 +++++++++++ 4 files changed, 67 insertions(+), 95 deletions(-) diff --git a/docs/pr-skill-installation-refactor.md b/docs/pr-skill-installation-refactor.md index 59b2d99bd7..e370999a78 100644 --- a/docs/pr-skill-installation-refactor.md +++ b/docs/pr-skill-installation-refactor.md @@ -168,13 +168,14 @@ The existing tip at `analyze.ts:363` says "Run `gitnexus setup` to configure MCP - Converted `analyze-skills-notice.test.ts` from local placeholder implementation to production export contract checks. - Deferred cleanup edge case #21 (read-only directory) until cleanup implementation exists to avoid false signal. -### Phase 3: Implementation (pending) - -- Remove `installSkills()` from `ai-context.ts` -- Add stale skills notice to `analyze` -- Add project-local cleanup to `setup` -- Update `analyze` tip to mention skills -- Make active acceptance tests pass +### Phase 3: Implementation (completed) + +- Removed `installSkills()` function and its call from `ai-context.ts`; cleaned up unused `fileURLToPath`/`__dirname` imports +- Added `checkStaleProjectSkills(repoPath)` export to `analyze.ts` — warns but does not delete +- Called `checkStaleProjectSkills` in analyze flow after generating AI context files +- Updated setup tip in `analyze.ts` to mention skills alongside MCP +- Added `cleanupProjectLocalSkills()` to `setup.ts` — removes `.claude/skills/gitnexus/` if cwd is a git repo +- All 25 skill-related tests pass; all 859 unit tests pass, no regressions ### Phase 4: README Update (pending) diff --git a/gitnexus/src/cli/ai-context.ts b/gitnexus/src/cli/ai-context.ts index 087320472d..9125255b12 100644 --- a/gitnexus/src/cli/ai-context.ts +++ b/gitnexus/src/cli/ai-context.ts @@ -8,11 +8,6 @@ import fs from 'fs/promises'; import path from 'path'; -import { fileURLToPath } from 'url'; - -// ESM equivalent of __dirname -const __filename = fileURLToPath(import.meta.url); -const __dirname = path.dirname(__filename); interface RepoStats { files?: number; @@ -187,82 +182,6 @@ async function upsertGitNexusSection( return 'appended'; } -/** - * Install GitNexus skills to .claude/skills/gitnexus/ - * Works natively with Claude Code, Cursor, and GitHub Copilot - */ -async function installSkills(repoPath: string): Promise { - const skillsDir = path.join(repoPath, '.claude', 'skills', 'gitnexus'); - const installedSkills: string[] = []; - - // Skill definitions bundled with the package - const skills = [ - { - name: 'gitnexus-exploring', - description: 'Use when the user asks how code works, wants to understand architecture, trace execution flows, or explore unfamiliar parts of the codebase. Examples: "How does X work?", "What calls this function?", "Show me the auth flow"', - }, - { - name: 'gitnexus-debugging', - description: 'Use when the user is debugging a bug, tracing an error, or asking why something fails. Examples: "Why is X failing?", "Where does this error come from?", "Trace this bug"', - }, - { - name: 'gitnexus-impact-analysis', - description: 'Use when the user wants to know what will break if they change something, or needs safety analysis before editing code. Examples: "Is it safe to change X?", "What depends on this?", "What will break?"', - }, - { - name: 'gitnexus-refactoring', - description: 'Use when the user wants to rename, extract, split, move, or restructure code safely. Examples: "Rename this function", "Extract this into a module", "Refactor this class", "Move this to a separate file"', - }, - { - name: 'gitnexus-guide', - description: 'Use when the user asks about GitNexus itself — available tools, how to query the knowledge graph, MCP resources, graph schema, or workflow reference. Examples: "What GitNexus tools are available?", "How do I use GitNexus?"', - }, - { - name: 'gitnexus-cli', - description: 'Use when the user needs to run GitNexus CLI commands like analyze/index a repo, check status, clean the index, generate a wiki, or list indexed repos. Examples: "Index this repo", "Reanalyze the codebase", "Generate a wiki"', - }, - ]; - - for (const skill of skills) { - const skillDir = path.join(skillsDir, skill.name); - const skillPath = path.join(skillDir, 'SKILL.md'); - - try { - // Create skill directory - await fs.mkdir(skillDir, { recursive: true }); - - // Try to read from package skills directory - const packageSkillPath = path.join(__dirname, '..', '..', 'skills', `${skill.name}.md`); - let skillContent: string; - - try { - skillContent = await fs.readFile(packageSkillPath, 'utf-8'); - } catch { - // Fallback: generate minimal skill content - skillContent = `--- -name: ${skill.name} -description: ${skill.description} ---- - -# ${skill.name.charAt(0).toUpperCase() + skill.name.slice(1)} - -${skill.description} - -Use GitNexus tools to accomplish this task. -`; - } - - await fs.writeFile(skillPath, skillContent, 'utf-8'); - installedSkills.push(skill.name); - } catch (err) { - // Skip on error, don't fail the whole process - console.warn(`Warning: Could not install skill ${skill.name}:`, err); - } - } - - return installedSkills; -} - /** * Generate AI context files after indexing */ @@ -285,12 +204,6 @@ export async function generateAIContextFiles( const claudeResult = await upsertGitNexusSection(claudePath, content); createdFiles.push(`CLAUDE.md (${claudeResult})`); - // Install skills to .claude/skills/gitnexus/ - const installedSkills = await installSkills(repoPath); - if (installedSkills.length > 0) { - createdFiles.push(`.claude/skills/gitnexus/ (${installedSkills.length} skills)`); - } - return { files: createdFiles }; } diff --git a/gitnexus/src/cli/analyze.ts b/gitnexus/src/cli/analyze.ts index c6a7c3b07c..224c2ae095 100644 --- a/gitnexus/src/cli/analyze.ts +++ b/gitnexus/src/cli/analyze.ts @@ -42,6 +42,24 @@ function ensureHeap(): boolean { return true; } +/** + * Check for stale project-local skills left by a previous `analyze` run. + * Prints a deprecation notice but does NOT delete — cleanup is handled by `setup`. + */ +export async function checkStaleProjectSkills(repoPath: string): Promise { + const skillsDir = path.join(repoPath, '.claude', 'skills', 'gitnexus'); + try { + const stat = await fs.stat(skillsDir); + if (stat.isDirectory()) { + console.log(` Note: Skills are no longer installed by analyze. Run 'gitnexus setup' to manage skills globally.`); + return true; + } + } catch { + // Directory doesn't exist — nothing to warn about + } + return false; +} + export interface AnalyzeOptions { force?: boolean; embeddings?: boolean; @@ -348,6 +366,9 @@ export const analyzeCommand = async ( console.log(` Context: ${aiContext.files.join(', ')}`); } + // Warn if stale project-local skills exist from a previous analyze run + await checkStaleProjectSkills(repoPath); + // Show a quiet summary if some edge types needed fallback insertion if (kuzuWarnings.length > 0) { const totalFallback = kuzuWarnings.reduce((sum, w) => { @@ -360,7 +381,7 @@ export const analyzeCommand = async ( try { await fs.access(getGlobalRegistryPath()); } catch { - console.log('\n Tip: Run `gitnexus setup` to configure MCP for your editor.'); + console.log('\n Tip: Run `gitnexus setup` to configure MCP and install agent skills for your editor.'); } console.log(''); diff --git a/gitnexus/src/cli/setup.ts b/gitnexus/src/cli/setup.ts index 8adf6bb9a6..4c34edf82c 100644 --- a/gitnexus/src/cli/setup.ts +++ b/gitnexus/src/cli/setup.ts @@ -341,6 +341,40 @@ async function installOpenCodeSkills(result: SetupResult): Promise { } } +// ─── Project-local skill cleanup ─────────────────────────────────── + +/** + * Remove stale project-local skills left by previous `analyze` runs. + * Only cleans up if cwd is a git repo (has .git directory). + */ +async function cleanupProjectLocalSkills(result: SetupResult): Promise { + const cwd = process.cwd(); + + // Only clean up inside git repos + const gitDir = path.join(cwd, '.git'); + try { + const stat = await fs.stat(gitDir); + if (!stat.isDirectory()) return; + } catch { + return; // Not a git repo + } + + const localSkillsDir = path.join(cwd, '.claude', 'skills', 'gitnexus'); + try { + const stat = await fs.stat(localSkillsDir); + if (!stat.isDirectory()) return; + } catch { + return; // No project-local skills + } + + try { + await fs.rm(localSkillsDir, { recursive: true, force: true }); + result.configured.push('Removed project-local skills (now installed globally)'); + } catch (err: any) { + result.errors.push(`Project-local skill cleanup: ${err.message}`); + } +} + // ─── Main command ────────────────────────────────────────────────── export const setupCommand = async () => { @@ -370,6 +404,9 @@ export const setupCommand = async () => { await installCursorSkills(result); await installOpenCodeSkills(result); + // Clean up stale project-local skills left by previous `analyze` runs + await cleanupProjectLocalSkills(result); + // Print results if (result.configured.length > 0) { console.log(' Configured:'); From f15b0aaa4bed4fa2e4ebf7c7d3bdfba0dd167f1d Mon Sep 17 00:00:00 2001 From: Linus Beckhaus Date: Sat, 7 Mar 2026 13:19:55 +0100 Subject: [PATCH 06/15] fix(skills): cover worktree cleanup and no-op analyze notice Resolve project-local skill cleanup from nested cwd by locating repo root via .git marker file/dir. Emit stale-skill migration notice even when analyze exits early as already up to date. Add regression tests and refresh refactor docs/status after Phase 3b fixes. --- docs/pr-skill-installation-refactor.md | 24 ++++++++++- docs/test-plan-skill-installation-refactor.md | 26 +++++++++++- gitnexus/src/cli/analyze.ts | 2 + gitnexus/src/cli/setup.ts | 40 ++++++++++++------ .../test/unit/analyze-skills-notice.test.ts | 41 +++++++++++++++++++ gitnexus/test/unit/setup-skills.test.ts | 26 ++++++++++++ 6 files changed, 145 insertions(+), 14 deletions(-) diff --git a/docs/pr-skill-installation-refactor.md b/docs/pr-skill-installation-refactor.md index e370999a78..8174e0b190 100644 --- a/docs/pr-skill-installation-refactor.md +++ b/docs/pr-skill-installation-refactor.md @@ -134,7 +134,7 @@ The existing tip at `analyze.ts:363` says "Run `gitnexus setup` to configure MCP | `gitnexus/test/unit/ai-context.test.ts` | Regression guards + active acceptance tests that assert `analyze` no longer installs skills | | `gitnexus/test/unit/setup-skills.test.ts` | `installSkillsTo` core tests + active `setupCommand` cleanup acceptance tests | | `gitnexus/test/unit/analyze-skills-notice.test.ts` | Contract tests bound to production export (`checkStaleProjectSkills`) instead of local helper | -| `README.md` | Fix skill count, clarify command responsibilities | +| `README.md` | **Pending (Phase 4)** — no README edits in this branch yet | | `docs/test-plan-skill-installation-refactor.md` | Add test-phase changelog and rationale for active acceptance tests | | `docs/pr-skill-installation-refactor.md` | Add progress changelog for test hardening | @@ -177,6 +177,28 @@ The existing tip at `analyze.ts:363` says "Run `gitnexus setup` to configure MCP - Added `cleanupProjectLocalSkills()` to `setup.ts` — removes `.claude/skills/gitnexus/` if cwd is a git repo - All 25 skill-related tests pass; all 859 unit tests pass, no regressions +### Post-Implementation Review (2026-03-07) + +Validation rerun in this branch: +- `npx vitest run test/unit/ai-context.test.ts test/unit/setup-skills.test.ts test/unit/analyze-skills-notice.test.ts` → **25/25 passing** +- `npm test` (unit suite) → **859/859 passing** + +Residual edge cases found in review (not addressed in this branch): +- The updated `analyze` setup tip is effectively unreachable after successful indexing because `registerRepo()` creates the global registry before the tip check. +- `setup` cleanup runs regardless of whether any global skill install actually succeeded; on machines with no supported editor directories present, stale local skills could be removed without replacement. + +### Phase 3b: Follow-up fixes for review findings #1 and #2 (completed) + +- Updated project-local cleanup in `setup.ts` to resolve repo root by walking upward and detecting `.git` as either directory or file. +- Cleanup now works from nested directories and worktree/submodule-style `.git` files. +- Updated `analyze.ts` early-return branch (`Already up to date`) to still call `checkStaleProjectSkills(repoPath)` so migration notice is not skipped. +- Added targeted regression tests: + - `setup-skills.test.ts`: nested-subdirectory cleanup and `.git` file marker cleanup. + - `analyze-skills-notice.test.ts`: stale-skill notice appears on up-to-date early return. +- Validation rerun after patch: + - `npx vitest run test/unit/setup-skills.test.ts test/unit/analyze-skills-notice.test.ts` → **18/18 passing** + - `npm test` (unit suite) → **862/862 passing** + ### Phase 4: README Update (pending) - Fix skill count (6, not 4) diff --git a/docs/test-plan-skill-installation-refactor.md b/docs/test-plan-skill-installation-refactor.md index 1b5eb8a682..3bc2441d2f 100644 --- a/docs/test-plan-skill-installation-refactor.md +++ b/docs/test-plan-skill-installation-refactor.md @@ -9,7 +9,7 @@ ## Safety Approach -All filesystem operations happen inside `fs.mkdtemp()` temp directories, cleaned up in `afterAll`. Tests never touch: +All filesystem operations happen inside `fs.mkdtemp()` temp directories, cleaned up in test teardown (`afterEach`/`afterAll` depending on file). Tests never touch: - The real `~/.claude/` directory - The real working repository - Any global state @@ -100,6 +100,20 @@ These tests can be written and run **before** the actual refactor. Here's the st - **Tests for setup skills** (#7-10): These test `installSkillsTo` directly and should pass now. - **Deferred edge case** (#21): Keep as planned while cleanup implementation is pending. +## Current Status (Post-Implementation) + +- Skill-focused suite (`ai-context`, `setup-skills`, `analyze-skills-notice`): **28/28 passing** +- Full unit suite (`npm test`): **862/862 passing** + +## Residual Coverage Gaps + +The following edge cases were identified in review and are not yet covered by automated tests: + +| # | Gap | Why it matters | +|---|-----|----------------| +| 25 | Cleanup behavior when no global skill target is installed/configured | Local skills may be removed even if no replacement global install succeeded | +| 26 | Setup tip visibility in `analyze` after successful indexing | Tip check happens after registry write, so the new MCP+skills tip may never surface in normal success path | + ## Execution ```bash @@ -129,3 +143,13 @@ npm test - Acceptance tests are expected to fail until refactor implementation lands. - Regression/behavior-preservation tests should continue to pass. - Read-only cleanup scenario (#21) remains planned and should be finalized when cleanup implementation exists. + +### 2026-03-07 — Phase 3b follow-up coverage + +- Added `setup-skills` regression coverage for: + - cleanup from nested subdirectory inside repo (fixes gap #23) + - cleanup when repo marker is `.git` file (worktree/submodule style, fixes gap #22) +- Added `analyze-skills-notice` regression coverage ensuring stale-skill notice still appears on `Already up to date` early return (fixes gap #24) +- Updated suite totals after new tests: + - skill-focused suite: 28 tests + - full unit suite: 862 tests diff --git a/gitnexus/src/cli/analyze.ts b/gitnexus/src/cli/analyze.ts index 224c2ae095..b2f5a4dc2f 100644 --- a/gitnexus/src/cli/analyze.ts +++ b/gitnexus/src/cli/analyze.ts @@ -116,6 +116,8 @@ export const analyzeCommand = async ( const existingMeta = await loadMeta(storagePath); if (existingMeta && !options?.force && existingMeta.lastCommit === currentCommit) { + // Keep migration notice visible even on no-op analyze runs + await checkStaleProjectSkills(repoPath); console.log(' Already up to date\n'); return; } diff --git a/gitnexus/src/cli/setup.ts b/gitnexus/src/cli/setup.ts index 4c34edf82c..c4d9a1ef11 100644 --- a/gitnexus/src/cli/setup.ts +++ b/gitnexus/src/cli/setup.ts @@ -344,22 +344,38 @@ async function installOpenCodeSkills(result: SetupResult): Promise { // ─── Project-local skill cleanup ─────────────────────────────────── /** - * Remove stale project-local skills left by previous `analyze` runs. - * Only cleans up if cwd is a git repo (has .git directory). + * Find the nearest git repository root by walking upward and checking for + * a .git marker (directory for standard repos, file for worktrees/submodules). */ -async function cleanupProjectLocalSkills(result: SetupResult): Promise { - const cwd = process.cwd(); +async function findRepoRoot(startPath: string): Promise { + let current = path.resolve(startPath); + const root = path.parse(current).root; - // Only clean up inside git repos - const gitDir = path.join(cwd, '.git'); - try { - const stat = await fs.stat(gitDir); - if (!stat.isDirectory()) return; - } catch { - return; // Not a git repo + while (true) { + const gitMarker = path.join(current, '.git'); + try { + const stat = await fs.stat(gitMarker); + if (stat.isDirectory() || stat.isFile()) { + return current; + } + } catch { + // Keep walking up + } + + if (current === root) return null; + current = path.dirname(current); } +} + +/** + * Remove stale project-local skills left by previous `analyze` runs. + * Cleans up at repo root and supports both .git directories and files. + */ +async function cleanupProjectLocalSkills(result: SetupResult): Promise { + const repoRoot = await findRepoRoot(process.cwd()); + if (!repoRoot) return; // Not inside a git repo - const localSkillsDir = path.join(cwd, '.claude', 'skills', 'gitnexus'); + const localSkillsDir = path.join(repoRoot, '.claude', 'skills', 'gitnexus'); try { const stat = await fs.stat(localSkillsDir); if (!stat.isDirectory()) return; diff --git a/gitnexus/test/unit/analyze-skills-notice.test.ts b/gitnexus/test/unit/analyze-skills-notice.test.ts index 912b521387..98a9a3181e 100644 --- a/gitnexus/test/unit/analyze-skills-notice.test.ts +++ b/gitnexus/test/unit/analyze-skills-notice.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import fs from 'fs/promises'; import path from 'path'; import os from 'os'; +import { execSync } from 'child_process'; /** * Contract tests for analyze stale-skills notice. @@ -81,4 +82,44 @@ describe('analyze — stale project-local skills notice', () => { const detected = await checkStaleProjectSkills(tmpDir); expect(detected).toBe(false); }); + + it('prints stale-skills notice on analyze early return (Already up to date)', async () => { + // Real git repo so analyzeCommand can resolve repo + commit + execSync('git init', { cwd: tmpDir, stdio: 'ignore' }); + await fs.writeFile(path.join(tmpDir, 'README.md'), 'hello\n', 'utf-8'); + execSync('git add README.md', { cwd: tmpDir, stdio: 'ignore' }); + execSync('git -c user.name="Test" -c user.email="test@example.com" commit -m "init"', { + cwd: tmpDir, + stdio: 'ignore', + }); + + const commit = execSync('git rev-parse HEAD', { cwd: tmpDir }).toString().trim(); + + // Mark index metadata as current so analyze exits early + const storagePath = path.join(tmpDir, '.gitnexus'); + await fs.mkdir(storagePath, { recursive: true }); + await fs.writeFile( + path.join(storagePath, 'meta.json'), + JSON.stringify({ repoPath: tmpDir, lastCommit: commit, indexedAt: new Date().toISOString() }, null, 2), + 'utf-8', + ); + + // Leave stale project-local skills in place + await fs.mkdir(path.join(tmpDir, '.claude', 'skills', 'gitnexus'), { recursive: true }); + + const originalNodeOptions = process.env.NODE_OPTIONS; + process.env.NODE_OPTIONS = `${originalNodeOptions || ''} --max-old-space-size=8192`.trim(); + try { + const analyzeModule = await import('../../src/cli/analyze.js'); + const analyzeCommand = (analyzeModule as any).analyzeCommand as (inputPath?: string) => Promise; + await analyzeCommand(tmpDir); + } finally { + process.env.NODE_OPTIONS = originalNodeOptions; + } + + const upToDate = consoleOutput.find(line => line.includes('Already up to date')); + const notice = consoleOutput.find(line => line.includes('no longer installed by analyze')); + expect(upToDate).toBeDefined(); + expect(notice).toBeDefined(); + }); }); diff --git a/gitnexus/test/unit/setup-skills.test.ts b/gitnexus/test/unit/setup-skills.test.ts index a110a29baa..7b0034d554 100644 --- a/gitnexus/test/unit/setup-skills.test.ts +++ b/gitnexus/test/unit/setup-skills.test.ts @@ -159,6 +159,32 @@ describe('setupCommand — project-local skill cleanup', () => { expect(stat.isDirectory()).toBe(true); }); + it('removes project-local skills when run from a nested subdirectory', async () => { + await fs.mkdir(path.join(tmpHome, '.claude'), { recursive: true }); + await fs.mkdir(path.join(tmpRepo, '.git'), { recursive: true }); + const localSkillsDir = path.join(tmpRepo, '.claude', 'skills', 'gitnexus'); + await fs.mkdir(localSkillsDir, { recursive: true }); + + const nestedDir = path.join(tmpRepo, 'packages', 'app'); + await fs.mkdir(nestedDir, { recursive: true }); + process.chdir(nestedDir); + + await setupCommand(); + + await expect(fs.stat(localSkillsDir)).rejects.toThrow(); + }); + + it('treats .git file as a valid repo marker (worktree/submodule style)', async () => { + await fs.mkdir(path.join(tmpHome, '.claude'), { recursive: true }); + await fs.writeFile(path.join(tmpRepo, '.git'), 'gitdir: /tmp/fake-worktree.git'); + const localSkillsDir = path.join(tmpRepo, '.claude', 'skills', 'gitnexus'); + await fs.mkdir(localSkillsDir, { recursive: true }); + + await setupCommand(); + + await expect(fs.stat(localSkillsDir)).rejects.toThrow(); + }); + it('removes empty project-local skills directory', async () => { await fs.mkdir(path.join(tmpHome, '.claude'), { recursive: true }); await fs.mkdir(path.join(tmpRepo, '.git'), { recursive: true }); From c0f12a87959c8766ae5a67c36747ddcfd6e5dcf4 Mon Sep 17 00:00:00 2001 From: Linus Beckhaus Date: Sat, 7 Mar 2026 13:21:42 +0100 Subject: [PATCH 07/15] docs: update README to reflect skill installation changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix skill count: 4 → 6, add Guide and CLI to the list - Clarify that analyze indexes + creates context files, while setup handles MCP config, skills, and hooks - Update skill install location from .claude/skills/ to ~/.claude/skills/ - Update CLI command descriptions --- README.md | 12 +++++++----- docs/pr-skill-installation-refactor.md | 12 +++++++----- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 77d8e5967b..9c5bd6279f 100644 --- a/README.md +++ b/README.md @@ -70,13 +70,13 @@ The CLI indexes your repository and runs an MCP server that gives AI agents deep npx gitnexus analyze ``` -That's it. This indexes the codebase, installs agent skills, registers Claude Code hooks, and creates `AGENTS.md` / `CLAUDE.md` context files — all in one command. +That's it. This indexes the codebase and creates `AGENTS.md` / `CLAUDE.md` context files — all in one command. -To configure MCP for your editor, run `npx gitnexus setup` once — or set it up manually below. +To configure MCP, install agent skills, and register hooks for your editor, run `npx gitnexus setup` once — or set it up manually below. ### MCP Setup -`gitnexus setup` auto-detects your editors and writes the correct global MCP config. You only need to run it once. +`gitnexus setup` auto-detects your editors and writes the correct global MCP config, installs agent skills, and registers hooks. You only need to run it once. ### Editor Support @@ -132,7 +132,7 @@ claude mcp add gitnexus -- npx -y gitnexus@latest mcp ### CLI Commands ```bash -gitnexus setup # Configure MCP for your editors (one-time) +gitnexus setup # Configure MCP, skills, and hooks (one-time) gitnexus analyze [path] # Index a repository (or update stale index) gitnexus analyze --force # Force full re-index gitnexus analyze --skip-embeddings # Skip embedding generation (faster) @@ -182,12 +182,14 @@ gitnexus wiki --base-url # Wiki with custom LLM API base URL | `detect_impact` | Pre-commit change analysis — scope, affected processes, risk level | | `generate_map` | Architecture documentation from the knowledge graph with mermaid diagrams | -**4 agent skills** installed to `.claude/skills/` automatically: +**6 agent skills** installed to `~/.claude/skills/` via `gitnexus setup`: - **Exploring** — Navigate unfamiliar code using the knowledge graph - **Debugging** — Trace bugs through call chains - **Impact Analysis** — Analyze blast radius before changes - **Refactoring** — Plan safe refactors using dependency mapping +- **Guide** — GitNexus tool reference, graph schema, and workflow guidance +- **CLI** — Run GitNexus CLI commands (analyze, status, clean, wiki, etc.) --- diff --git a/docs/pr-skill-installation-refactor.md b/docs/pr-skill-installation-refactor.md index 8174e0b190..a13f6612ff 100644 --- a/docs/pr-skill-installation-refactor.md +++ b/docs/pr-skill-installation-refactor.md @@ -134,7 +134,7 @@ The existing tip at `analyze.ts:363` says "Run `gitnexus setup` to configure MCP | `gitnexus/test/unit/ai-context.test.ts` | Regression guards + active acceptance tests that assert `analyze` no longer installs skills | | `gitnexus/test/unit/setup-skills.test.ts` | `installSkillsTo` core tests + active `setupCommand` cleanup acceptance tests | | `gitnexus/test/unit/analyze-skills-notice.test.ts` | Contract tests bound to production export (`checkStaleProjectSkills`) instead of local helper | -| `README.md` | **Pending (Phase 4)** — no README edits in this branch yet | +| `README.md` | Fix skill count (6 not 4), clarify command responsibilities | | `docs/test-plan-skill-installation-refactor.md` | Add test-phase changelog and rationale for active acceptance tests | | `docs/pr-skill-installation-refactor.md` | Add progress changelog for test hardening | @@ -199,8 +199,10 @@ Residual edge cases found in review (not addressed in this branch): - `npx vitest run test/unit/setup-skills.test.ts test/unit/analyze-skills-notice.test.ts` → **18/18 passing** - `npm test` (unit suite) → **862/862 passing** -### Phase 4: README Update (pending) +### Phase 4: README Update (completed) -- Fix skill count (6, not 4) -- List all skills including `gitnexus-guide` and `gitnexus-cli` -- Clarify command responsibilities +- Fixed skill count: 4 → 6, added Guide and CLI skills to the list +- Changed install location from `.claude/skills/` to `~/.claude/skills/` with `via gitnexus setup` +- Clarified Quick Start: `analyze` indexes + creates context files; `setup` handles MCP + skills + hooks +- Updated MCP Setup description to mention skills and hooks +- Updated CLI command comment for `setup` From e132f77eb67e5c16dd4a1abbf08dd727bb59fba0 Mon Sep 17 00:00:00 2001 From: Linus Beckhaus Date: Sat, 7 Mar 2026 13:31:25 +0100 Subject: [PATCH 08/15] docs: plan auto-discovery of skill names from disk Replace the "future work" section with a concrete Phase 5 plan to auto-discover skill names from the skills/ source directory instead of hardcoding them. Add 7 new test cases (#27-33) to the test plan covering discovery logic, prefix filtering, and mixed layouts. --- docs/pr-skill-installation-refactor.md | 27 ++++++++++++++++--- docs/test-plan-skill-installation-refactor.md | 16 ++++++++++- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/docs/pr-skill-installation-refactor.md b/docs/pr-skill-installation-refactor.md index a13f6612ff..689cc103c4 100644 --- a/docs/pr-skill-installation-refactor.md +++ b/docs/pr-skill-installation-refactor.md @@ -119,10 +119,20 @@ The existing tip at `analyze.ts:363` says "Run `gitnexus setup` to configure MCP - Clarify that `analyze` handles indexing + dynamic context, `setup` handles MCP + skills + hooks - Correct line 73 which incorrectly attributes hooks to `analyze` -### 6. Future work (out of scope for this PR) +### 6. Auto-discover skill names from disk -- Add `gitnexus-pr-review` to the `SKILL_NAMES` list in `setup.ts` -- Unify the skill name constants into a single shared location +Replace the hardcoded `SKILL_NAMES` array in `setup.ts` with a `discoverSkillNames()` function that reads the `skills/` source directory at install time. This: + +- Automatically picks up `gitnexus-pr-review` (previously missed because it wasn't in the hardcoded list) +- Prevents future breakage when skill files are renamed or added +- Removes the need for a shared constant — the filesystem is the source of truth + +Discovery logic: +- **Flat files**: `gitnexus-exploring.md` → skill name `gitnexus-exploring` +- **Directories**: `gitnexus-cli/SKILL.md` → skill name `gitnexus-cli` +- Only entries matching `gitnexus-*` are included (avoids picking up stray files like `README.md`) + +`SKILL_NAMES` remains exported (now computed dynamically) for test compatibility. ## Files Changed @@ -130,7 +140,7 @@ The existing tip at `analyze.ts:363` says "Run `gitnexus setup` to configure MCP |---|---| | `gitnexus/src/cli/ai-context.ts` | Remove `installSkills()` function and its call | | `gitnexus/src/cli/analyze.ts` | Add deprecation notice for stale local skills; update setup tip | -| `gitnexus/src/cli/setup.ts` | Export `installSkillsTo` and `SKILL_NAMES`; add cleanup of project-local skills during global install | +| `gitnexus/src/cli/setup.ts` | Replace hardcoded `SKILL_NAMES` with `discoverSkillNames()`; export `installSkillsTo` and `SKILL_NAMES`; add cleanup of project-local skills during global install | | `gitnexus/test/unit/ai-context.test.ts` | Regression guards + active acceptance tests that assert `analyze` no longer installs skills | | `gitnexus/test/unit/setup-skills.test.ts` | `installSkillsTo` core tests + active `setupCommand` cleanup acceptance tests | | `gitnexus/test/unit/analyze-skills-notice.test.ts` | Contract tests bound to production export (`checkStaleProjectSkills`) instead of local helper | @@ -199,6 +209,15 @@ Residual edge cases found in review (not addressed in this branch): - `npx vitest run test/unit/setup-skills.test.ts test/unit/analyze-skills-notice.test.ts` → **18/18 passing** - `npm test` (unit suite) → **862/862 passing** +### Phase 5: Auto-discover skill names (planned) + +- Replace hardcoded `SKILL_NAMES` array with `discoverSkillNames()` that reads `gitnexus/skills/` at install time +- Picks up `gitnexus-pr-review` automatically (previously missed) +- Discovery: flat `.md` files → strip extension; directories with `SKILL.md` → use dir name; filter to `gitnexus-*` prefix +- `SKILL_NAMES` export becomes the result of `discoverSkillNames()` (async, called once per `installSkillsTo` invocation) +- New tests: `discoverSkillNames` discovery logic, prefix filtering, mixed layout handling +- Existing `installSkillsTo` tests adapt automatically (they reference `SKILL_NAMES.length`) + ### Phase 4: README Update (completed) - Fixed skill count: 4 → 6, added Guide and CLI skills to the list diff --git a/docs/test-plan-skill-installation-refactor.md b/docs/test-plan-skill-installation-refactor.md index 3bc2441d2f..8d7a253025 100644 --- a/docs/test-plan-skill-installation-refactor.md +++ b/docs/test-plan-skill-installation-refactor.md @@ -88,7 +88,7 @@ Since `analyzeCommand` is heavy (requires KuzuDB, pipeline, etc.), we don't test | File | Tests | Type | |------|-------|------| | `test/unit/ai-context.test.ts` | #1-6, #18 | Unit — update existing + add new | -| `test/unit/setup-skills.test.ts` | #7-14, #19-21 | Unit — new file | +| `test/unit/setup-skills.test.ts` | #7-14, #19-21, #27-33 | Unit — new file | | `test/unit/analyze-skills-notice.test.ts` | #15-17 | Unit — new file | ## Running Tests Pre-Implementation @@ -105,6 +105,20 @@ These tests can be written and run **before** the actual refactor. Here's the st - Skill-focused suite (`ai-context`, `setup-skills`, `analyze-skills-notice`): **28/28 passing** - Full unit suite (`npm test`): **862/862 passing** +## New Unit Tests: `test/unit/setup-skills.test.ts` — skill discovery + +These test the `discoverSkillNames()` function that replaces the hardcoded `SKILL_NAMES` array. + +| # | Test | Setup | Assertion | +|---|------|-------|-----------| +| 27 | Discovers all skills from the real source directory | Call `discoverSkillNames()` with the real skills root | Returns at least 7 names including `gitnexus-pr-review` | +| 28 | Only includes `gitnexus-*` prefixed entries | Create temp dir with `gitnexus-foo.md`, `README.md`, `notes.txt` | Returns only `gitnexus-foo` | +| 29 | Discovers flat `.md` files | Create temp dir with `gitnexus-test.md` | Returns `['gitnexus-test']` | +| 30 | Discovers directory-based skills | Create temp dir with `gitnexus-test/SKILL.md` | Returns `['gitnexus-test']` | +| 31 | Handles mixed layouts (flat + directory) | Create both `gitnexus-a.md` and `gitnexus-b/SKILL.md` | Returns both `gitnexus-a` and `gitnexus-b` | +| 32 | Returns empty array for empty directory | Create empty temp dir | Returns `[]` | +| 33 | Directories without SKILL.md are ignored | Create `gitnexus-broken/` with no SKILL.md | Returns `[]` | + ## Residual Coverage Gaps The following edge cases were identified in review and are not yet covered by automated tests: From 4a88ff65b63a0f0b7da3f11b0bc637be916785a5 Mon Sep 17 00:00:00 2001 From: Linus Beckhaus Date: Sat, 7 Mar 2026 13:32:40 +0100 Subject: [PATCH 09/15] test: add acceptance tests for discoverSkillNames (#27-33) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 8 tests covering auto-discovery of skill names from the skills/ source directory: real source smoke test, prefix filtering, flat file discovery, directory-based discovery, mixed layouts, empty dir, and directories without SKILL.md. All 8 fail as expected — discoverSkillNames() is not yet exported from setup.ts. Existing 13 setup-skills tests remain green. --- gitnexus/test/unit/setup-skills.test.ts | 102 ++++++++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/gitnexus/test/unit/setup-skills.test.ts b/gitnexus/test/unit/setup-skills.test.ts index 7b0034d554..1882b32d34 100644 --- a/gitnexus/test/unit/setup-skills.test.ts +++ b/gitnexus/test/unit/setup-skills.test.ts @@ -2,8 +2,25 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import fs from 'fs/promises'; import path from 'path'; import os from 'os'; +import { fileURLToPath } from 'url'; import { installSkillsTo, SKILL_NAMES, setupCommand } from '../../src/cli/setup.js'; +const __filename = fileURLToPath(import.meta.url); +const __dirname = path.dirname(__filename); + +/** Resolve the real skills source directory used by setup.ts */ +const SKILLS_ROOT = path.join(__dirname, '..', '..', 'skills'); + +/** + * Dynamically import discoverSkillNames from setup.ts. + * Returns null if the export doesn't exist yet (pre-implementation). + */ +async function getDiscoverSkillNames(): Promise<((skillsRoot: string) => Promise) | null> { + const mod = await import('../../src/cli/setup.js'); + const candidate = (mod as any).discoverSkillNames; + return typeof candidate === 'function' ? candidate : null; +} + describe('installSkillsTo', () => { let tmpDir: string; @@ -208,3 +225,88 @@ describe('setupCommand — project-local skill cleanup', () => { await expect(fs.stat(localSkillsDir)).rejects.toThrow(); }); }); + +describe('discoverSkillNames', () => { + let tmpDir: string; + let discoverSkillNames: ((skillsRoot: string) => Promise) | null; + + beforeEach(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-discover-skills-')); + discoverSkillNames = await getDiscoverSkillNames(); + }); + + afterEach(async () => { + try { + await fs.rm(tmpDir, { recursive: true, force: true }); + } catch { /* best-effort */ } + }); + + it('is exported from setup.ts', () => { + expect(discoverSkillNames, 'setup.ts must export discoverSkillNames()').not.toBeNull(); + }); + + it('discovers all skills from the real source directory', async () => { + if (!discoverSkillNames) return expect.fail('discoverSkillNames not exported'); + const names = await discoverSkillNames(SKILLS_ROOT); + expect(names.length).toBeGreaterThanOrEqual(7); + expect(names).toContain('gitnexus-pr-review'); + expect(names).toContain('gitnexus-exploring'); + expect(names).toContain('gitnexus-cli'); + }); + + it('only includes gitnexus-* prefixed entries', async () => { + if (!discoverSkillNames) return expect.fail('discoverSkillNames not exported'); + // Create a mix of valid and invalid entries + await fs.writeFile(path.join(tmpDir, 'gitnexus-foo.md'), 'skill content'); + await fs.writeFile(path.join(tmpDir, 'README.md'), 'not a skill'); + await fs.writeFile(path.join(tmpDir, 'notes.txt'), 'not a skill'); + + const names = await discoverSkillNames(tmpDir); + expect(names).toEqual(['gitnexus-foo']); + }); + + it('discovers flat .md files', async () => { + if (!discoverSkillNames) return expect.fail('discoverSkillNames not exported'); + await fs.writeFile(path.join(tmpDir, 'gitnexus-test.md'), 'skill content'); + + const names = await discoverSkillNames(tmpDir); + expect(names).toEqual(['gitnexus-test']); + }); + + it('discovers directory-based skills', async () => { + if (!discoverSkillNames) return expect.fail('discoverSkillNames not exported'); + const skillDir = path.join(tmpDir, 'gitnexus-test'); + await fs.mkdir(skillDir, { recursive: true }); + await fs.writeFile(path.join(skillDir, 'SKILL.md'), 'skill content'); + + const names = await discoverSkillNames(tmpDir); + expect(names).toEqual(['gitnexus-test']); + }); + + it('handles mixed layouts (flat + directory)', async () => { + if (!discoverSkillNames) return expect.fail('discoverSkillNames not exported'); + await fs.writeFile(path.join(tmpDir, 'gitnexus-a.md'), 'flat skill'); + const dirSkill = path.join(tmpDir, 'gitnexus-b'); + await fs.mkdir(dirSkill, { recursive: true }); + await fs.writeFile(path.join(dirSkill, 'SKILL.md'), 'dir skill'); + + const names = await discoverSkillNames(tmpDir); + expect(names.sort()).toEqual(['gitnexus-a', 'gitnexus-b']); + }); + + it('returns empty array for empty directory', async () => { + if (!discoverSkillNames) return expect.fail('discoverSkillNames not exported'); + const names = await discoverSkillNames(tmpDir); + expect(names).toEqual([]); + }); + + it('ignores directories without SKILL.md', async () => { + if (!discoverSkillNames) return expect.fail('discoverSkillNames not exported'); + const brokenSkill = path.join(tmpDir, 'gitnexus-broken'); + await fs.mkdir(brokenSkill, { recursive: true }); + await fs.writeFile(path.join(brokenSkill, 'README.md'), 'not a skill'); + + const names = await discoverSkillNames(tmpDir); + expect(names).toEqual([]); + }); +}); From 011b2c10e22f4ea3c3be7eca8e55135998901f5a Mon Sep 17 00:00:00 2001 From: Linus Beckhaus Date: Sat, 7 Mar 2026 13:35:48 +0100 Subject: [PATCH 10/15] feat(skills): auto-discover skill names from disk Replace hardcoded SKILL_NAMES array with discoverSkillNames() that reads the skills/ source directory at install time. This: - Automatically picks up gitnexus-pr-review (7 skills, up from 6) - Prevents breakage when skill files are renamed or added - Uses filesystem as source of truth instead of a hardcoded list Discovery supports flat files (*.md) and directory-based skills (*/SKILL.md), filtered to gitnexus-* prefix only. SKILL_NAMES export preserved (lazily populated) for backward compatibility. 870/870 unit tests passing. --- docs/pr-skill-installation-refactor.md | 13 ++--- docs/test-plan-skill-installation-refactor.md | 13 ++++- gitnexus/src/cli/setup.ts | 53 +++++++++++++++++-- 3 files changed, 68 insertions(+), 11 deletions(-) diff --git a/docs/pr-skill-installation-refactor.md b/docs/pr-skill-installation-refactor.md index 689cc103c4..5fb89622ec 100644 --- a/docs/pr-skill-installation-refactor.md +++ b/docs/pr-skill-installation-refactor.md @@ -209,14 +209,15 @@ Residual edge cases found in review (not addressed in this branch): - `npx vitest run test/unit/setup-skills.test.ts test/unit/analyze-skills-notice.test.ts` → **18/18 passing** - `npm test` (unit suite) → **862/862 passing** -### Phase 5: Auto-discover skill names (planned) +### Phase 5: Auto-discover skill names (completed) -- Replace hardcoded `SKILL_NAMES` array with `discoverSkillNames()` that reads `gitnexus/skills/` at install time -- Picks up `gitnexus-pr-review` automatically (previously missed) -- Discovery: flat `.md` files → strip extension; directories with `SKILL.md` → use dir name; filter to `gitnexus-*` prefix -- `SKILL_NAMES` export becomes the result of `discoverSkillNames()` (async, called once per `installSkillsTo` invocation) -- New tests: `discoverSkillNames` discovery logic, prefix filtering, mixed layout handling +- Replaced hardcoded `SKILL_NAMES` array with `discoverSkillNames(skillsRoot)` that reads the skills source directory +- `gitnexus-pr-review` now automatically discovered and installed (7 skills total, up from 6) +- Discovery logic: flat `.md` files → strip extension; directories with `SKILL.md` → use dir name; filter to `gitnexus-*` prefix +- `SKILL_NAMES` export preserved for test compatibility (lazily populated on first `installSkillsTo` call) +- 8 new tests (#27-33 + export check): all passing - Existing `installSkillsTo` tests adapt automatically (they reference `SKILL_NAMES.length`) +- Validation: **870/870 unit tests passing** ### Phase 4: README Update (completed) diff --git a/docs/test-plan-skill-installation-refactor.md b/docs/test-plan-skill-installation-refactor.md index 8d7a253025..fb23efe74a 100644 --- a/docs/test-plan-skill-installation-refactor.md +++ b/docs/test-plan-skill-installation-refactor.md @@ -102,8 +102,8 @@ These tests can be written and run **before** the actual refactor. Here's the st ## Current Status (Post-Implementation) -- Skill-focused suite (`ai-context`, `setup-skills`, `analyze-skills-notice`): **28/28 passing** -- Full unit suite (`npm test`): **862/862 passing** +- Skill-focused suite (`ai-context`, `setup-skills`, `analyze-skills-notice`): **36/36 passing** +- Full unit suite (`npm test`): **870/870 passing** ## New Unit Tests: `test/unit/setup-skills.test.ts` — skill discovery @@ -167,3 +167,12 @@ npm test - Updated suite totals after new tests: - skill-focused suite: 28 tests - full unit suite: 862 tests + +### 2026-03-07 — Phase 5: Auto-discover skill names + +- Added `discoverSkillNames` describe block with 8 tests (#27-33 + export check) +- Tests cover: real source discovery, prefix filtering, flat files, directory-based skills, mixed layouts, empty dir, directories without SKILL.md +- All 8 tests initially failed ("discoverSkillNames not exported"), then passed after implementation +- Updated suite totals: + - skill-focused suite: 36 tests + - full unit suite: 870 tests diff --git a/gitnexus/src/cli/setup.ts b/gitnexus/src/cli/setup.ts index c4d9a1ef11..76f9263789 100644 --- a/gitnexus/src/cli/setup.ts +++ b/gitnexus/src/cli/setup.ts @@ -240,7 +240,53 @@ async function setupOpenCode(result: SetupResult): Promise { // ─── Skill Installation ─────────────────────────────────────────── -export const SKILL_NAMES = ['gitnexus-exploring', 'gitnexus-debugging', 'gitnexus-impact-analysis', 'gitnexus-refactoring', 'gitnexus-guide', 'gitnexus-cli']; +/** + * Discover skill names from a skills source directory. + * + * Scans for two layouts: + * - Flat file: gitnexus-{name}.md → skill name "gitnexus-{name}" + * - Directory: gitnexus-{name}/SKILL.md → skill name "gitnexus-{name}" + * + * Only entries prefixed with "gitnexus-" are included. + * Directories without a SKILL.md are ignored. + */ +export async function discoverSkillNames(skillsRoot: string): Promise { + const entries = await fs.readdir(skillsRoot, { withFileTypes: true }); + const names: string[] = []; + + for (const entry of entries) { + if (!entry.name.startsWith('gitnexus-')) continue; + + if (entry.isFile() && entry.name.endsWith('.md')) { + names.push(entry.name.replace(/\.md$/, '')); + } else if (entry.isDirectory()) { + try { + await fs.access(path.join(skillsRoot, entry.name, 'SKILL.md')); + names.push(entry.name); + } catch { /* no SKILL.md — skip */ } + } + } + + return names.sort(); +} + +/** Default skills source directory (resolved relative to this file). */ +const DEFAULT_SKILLS_ROOT = path.join(__dirname, '..', '..', 'skills'); + +/** + * Skill names discovered from the skills/ source directory. + * Computed lazily on first access; exported for test compatibility. + */ +export let SKILL_NAMES: string[] = []; +let _skillNamesResolved = false; + +async function ensureSkillNames(): Promise { + if (!_skillNamesResolved) { + SKILL_NAMES = await discoverSkillNames(DEFAULT_SKILLS_ROOT); + _skillNamesResolved = true; + } + return SKILL_NAMES; +} /** * Install GitNexus skills to a target directory. @@ -253,9 +299,10 @@ export const SKILL_NAMES = ['gitnexus-exploring', 'gitnexus-debugging', 'gitnexu */ export async function installSkillsTo(targetDir: string): Promise { const installed: string[] = []; - const skillsRoot = path.join(__dirname, '..', '..', 'skills'); + const skillsRoot = DEFAULT_SKILLS_ROOT; + const skillNames = await ensureSkillNames(); - for (const skillName of SKILL_NAMES) { + for (const skillName of skillNames) { const skillDir = path.join(targetDir, skillName); try { From 70a9f03bcf3d984f370f1c8ae3575c0e5c4fd63c Mon Sep 17 00:00:00 2001 From: Linus Beckhaus Date: Sat, 7 Mar 2026 13:41:13 +0100 Subject: [PATCH 11/15] docs: add pr description --- docs/pr-description.md | 84 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 docs/pr-description.md diff --git a/docs/pr-description.md b/docs/pr-description.md new file mode 100644 index 0000000000..6aba9692f6 --- /dev/null +++ b/docs/pr-description.md @@ -0,0 +1,84 @@ +# PR: Unify skill installation in `setup`, remove from `analyze` + +## Title + +`refactor(skills): unify installation in setup, auto-discover from disk` + +## Description + +### Summary + +- Removes skill installation from `analyze` (was a side effect of indexing) +- Makes `setup` the single owner of skill installation (global only) +- Auto-discovers skill names from the `skills/` directory instead of hardcoding them +- Adds migration handling for users with stale project-local skills + +### Problem + +When users run both `gitnexus setup` and `gitnexus analyze`, the same skills get installed to two locations: + +| Location | Installed by | +|----------|-------------| +| `~/.claude/skills/gitnexus-exploring/SKILL.md` | `setup` (global) | +| `/.claude/skills/gitnexus/gitnexus-exploring/SKILL.md` | `analyze` (project-local) | + +This triggers [Claude Code bug #25209](https://github.com/anthropics/claude-code/issues/25209) — both copies appear in the skill list instead of one shadowing the other. Users see every GitNexus skill listed twice. + +Beyond the duplication bug, skills are static markdown files that don't depend on the repository index. Installing them on every `analyze` run is unnecessary work in the wrong place. There were also two separate implementations of the same install logic. + +### Solution + +**Single owner:** `setup` installs skills globally to `~/.claude/skills/`, `~/.cursor/skills/`, and `~/.config/opencode/skill/`. `analyze` no longer touches skills. + +**Migration:** `analyze` prints a deprecation notice if stale project-local skills exist. `setup` cleans them up after installing globally. + +**Auto-discovery:** Skill names are discovered from the `skills/` source directory at install time instead of being hardcoded. This automatically picks up `gitnexus-pr-review` (previously missing) and prevents future breakage when skills are added or renamed. + +### Changes + +| File | What changed | +|------|-------------| +| `gitnexus/src/cli/ai-context.ts` | Removed `installSkills()` function and its call; cleaned up unused imports | +| `gitnexus/src/cli/analyze.ts` | Added `checkStaleProjectSkills()` deprecation notice; updated setup tip to mention skills | +| `gitnexus/src/cli/setup.ts` | Added `discoverSkillNames()` replacing hardcoded list; added `cleanupProjectLocalSkills()` with worktree/submodule support | +| `gitnexus/test/unit/ai-context.test.ts` | Acceptance tests: `analyze` no longer installs skills; regression guards for dynamic context generation | +| `gitnexus/test/unit/setup-skills.test.ts` | `installSkillsTo` tests, `setupCommand` cleanup tests, `discoverSkillNames` discovery tests | +| `gitnexus/test/unit/analyze-skills-notice.test.ts` | Contract tests for `checkStaleProjectSkills` export and behavior | +| `README.md` | Fixed skill count (4 -> 7), corrected command responsibility descriptions | + +### Design decisions + +1. **Why not keep both locations?** Adds complexity to work around a bug that shouldn't exist in our code. Skills are static config — one canonical location is cleaner. + +2. **Why doesn't `analyze` delete stale skills?** After the refactor, `analyze` no longer owns skills. Deleting them would cross the responsibility boundary. It warns; `setup` cleans up. + +3. **Why auto-discover instead of hardcode?** The hardcoded `SKILL_NAMES` list was the root cause of `gitnexus-pr-review` being silently excluded. Discovery from disk means adding a new skill is just dropping a file — no code change required. + +4. **Worktree/submodule support:** `cleanupProjectLocalSkills` walks upward to find the repo root, handling `.git` as either a directory (standard) or a file (worktrees/submodules). + +### Test plan + +- [x] `analyze` no longer creates `.claude/skills/gitnexus/` (acceptance tests #1-2) +- [x] `analyze` prints deprecation notice when stale skills exist (#15-17) +- [x] `analyze` shows notice even on "Already up to date" early return +- [x] `setup` installs all 7 discovered skills with non-empty SKILL.md (#7-9) +- [x] `setup` removes project-local skills in git repos (#11-12) +- [x] `setup` handles nested dirs, worktrees, non-git dirs, empty dirs (#13-14, nested/worktree tests) +- [x] `discoverSkillNames` discovers flat files, directories, mixed layouts (#27-33) +- [x] `discoverSkillNames` filters to `gitnexus-*` prefix only (#28) +- [x] `discoverSkillNames` ignores directories without SKILL.md (#33) +- [x] All existing tests unaffected — **870/870 passing** + +### Commits + +| Commit | Description | +|--------|-------------| +| `02eb465` | docs: create initial summary and tracking document | +| `b246c87` | test: add test suite for skill installation refactor | +| `c3f6995` | test(skills): activate pre-refactor acceptance suite | +| `c2ffb76` | refactor: unify skill installation in setup, remove from analyze | +| `f15b0aa` | fix(skills): cover worktree cleanup and no-op analyze notice | +| `c0f12a8` | docs: update README to reflect skill installation changes | +| `e132f77` | docs: plan auto-discovery of skill names from disk | +| `4a88ff6` | test: add acceptance tests for discoverSkillNames | +| `011b2c1` | feat(skills): auto-discover skill names from disk | From 2fa3dd1f78c34293fe4a2f7ce3155270b8a7fccf Mon Sep 17 00:00:00 2001 From: Linus Beckhaus Date: Sat, 7 Mar 2026 13:43:38 +0100 Subject: [PATCH 12/15] test(skills): add discovery edge-case coverage Cover duplicate-name collision, missing root and EACCES propagation, and lazy SKILL_NAMES population contract. Update refactor docs/test plan with new case IDs (#34-#37) and updated passing totals. --- docs/pr-skill-installation-refactor.md | 9 ++++ docs/test-plan-skill-installation-refactor.md | 21 ++++++-- gitnexus/test/unit/setup-skills.test.ts | 54 +++++++++++++++++++ 3 files changed, 81 insertions(+), 3 deletions(-) diff --git a/docs/pr-skill-installation-refactor.md b/docs/pr-skill-installation-refactor.md index 5fb89622ec..4655295cc7 100644 --- a/docs/pr-skill-installation-refactor.md +++ b/docs/pr-skill-installation-refactor.md @@ -219,6 +219,15 @@ Residual edge cases found in review (not addressed in this branch): - Existing `installSkillsTo` tests adapt automatically (they reference `SKILL_NAMES.length`) - Validation: **870/870 unit tests passing** +### Phase 5A: Discovery edge-case test hardening (completed) + +- Added 4 high-value tests in `setup-skills.test.ts`: + - Collision case where both `gitnexus-x.md` and `gitnexus-x/SKILL.md` exist + - Missing skills-root error propagation (`ENOENT`) + - Permission/read failure propagation (`EACCES` via mocked `fs.readdir`) + - `SKILL_NAMES` lazy-population export contract (empty before first install, populated after) +- Validation: **874/874 unit tests passing** + ### Phase 4: README Update (completed) - Fixed skill count: 4 → 6, added Guide and CLI skills to the list diff --git a/docs/test-plan-skill-installation-refactor.md b/docs/test-plan-skill-installation-refactor.md index fb23efe74a..b72610e987 100644 --- a/docs/test-plan-skill-installation-refactor.md +++ b/docs/test-plan-skill-installation-refactor.md @@ -88,7 +88,7 @@ Since `analyzeCommand` is heavy (requires KuzuDB, pipeline, etc.), we don't test | File | Tests | Type | |------|-------|------| | `test/unit/ai-context.test.ts` | #1-6, #18 | Unit — update existing + add new | -| `test/unit/setup-skills.test.ts` | #7-14, #19-21, #27-33 | Unit — new file | +| `test/unit/setup-skills.test.ts` | #7-14, #19-21, #27-37 | Unit — new file | | `test/unit/analyze-skills-notice.test.ts` | #15-17 | Unit — new file | ## Running Tests Pre-Implementation @@ -102,8 +102,8 @@ These tests can be written and run **before** the actual refactor. Here's the st ## Current Status (Post-Implementation) -- Skill-focused suite (`ai-context`, `setup-skills`, `analyze-skills-notice`): **36/36 passing** -- Full unit suite (`npm test`): **870/870 passing** +- Skill-focused suite (`ai-context`, `setup-skills`, `analyze-skills-notice`): **40/40 passing** +- Full unit suite (`npm test`): **874/874 passing** ## New Unit Tests: `test/unit/setup-skills.test.ts` — skill discovery @@ -118,6 +118,10 @@ These test the `discoverSkillNames()` function that replaces the hardcoded `SKIL | 31 | Handles mixed layouts (flat + directory) | Create both `gitnexus-a.md` and `gitnexus-b/SKILL.md` | Returns both `gitnexus-a` and `gitnexus-b` | | 32 | Returns empty array for empty directory | Create empty temp dir | Returns `[]` | | 33 | Directories without SKILL.md are ignored | Create `gitnexus-broken/` with no SKILL.md | Returns `[]` | +| 34 | Colliding flat + directory entries are explicitly covered | Create both `gitnexus-collision.md` and `gitnexus-collision/SKILL.md` | Documents current duplicate-name behavior | +| 35 | Missing skills root behavior is explicit | Call with non-existent skills root | Rejects with `ENOENT` | +| 36 | Readdir permission failure behavior is explicit | Mock `fs.readdir` to throw `EACCES` for one root | Rejects with `EACCES` | +| 37 | `SKILL_NAMES` lazy export contract is covered | Fresh-import module, then call `installSkillsTo` | Starts empty, populated after first install | ## Residual Coverage Gaps @@ -176,3 +180,14 @@ npm test - Updated suite totals: - skill-focused suite: 36 tests - full unit suite: 870 tests + +### 2026-03-07 — Phase 5A: Discovery edge-case hardening + +- Added 4 additional discovery tests (#34-#37) for: + - flat+directory name collision handling (current behavior documentation) + - missing root (`ENOENT`) propagation + - permission failure (`EACCES`) propagation + - `SKILL_NAMES` lazy-population contract after first install +- Updated suite totals: + - skill-focused suite: 40 tests + - full unit suite: 874 tests diff --git a/gitnexus/test/unit/setup-skills.test.ts b/gitnexus/test/unit/setup-skills.test.ts index 1882b32d34..53ed56f92d 100644 --- a/gitnexus/test/unit/setup-skills.test.ts +++ b/gitnexus/test/unit/setup-skills.test.ts @@ -309,4 +309,58 @@ describe('discoverSkillNames', () => { const names = await discoverSkillNames(tmpDir); expect(names).toEqual([]); }); + + it('handles colliding flat+directory entries with same skill name', async () => { + if (!discoverSkillNames) return expect.fail('discoverSkillNames not exported'); + await fs.writeFile(path.join(tmpDir, 'gitnexus-collision.md'), 'flat skill'); + const dirSkill = path.join(tmpDir, 'gitnexus-collision'); + await fs.mkdir(dirSkill, { recursive: true }); + await fs.writeFile(path.join(dirSkill, 'SKILL.md'), 'dir skill'); + + const names = await discoverSkillNames(tmpDir); + expect(names).toEqual(['gitnexus-collision', 'gitnexus-collision']); + }); + + it('throws when skills root does not exist', async () => { + if (!discoverSkillNames) return expect.fail('discoverSkillNames not exported'); + const missingRoot = path.join(tmpDir, 'does-not-exist'); + + await expect(discoverSkillNames(missingRoot)).rejects.toMatchObject({ code: 'ENOENT' }); + }); + + it('propagates readdir permission failures', async () => { + if (!discoverSkillNames) return expect.fail('discoverSkillNames not exported'); + const protectedRoot = path.join(tmpDir, 'protected'); + await fs.mkdir(protectedRoot, { recursive: true }); + + const originalReaddir = fs.readdir.bind(fs); + const readdirSpy = vi.spyOn(fs, 'readdir').mockImplementation(async (...args: any[]) => { + const target = String(args[0]); + if (target === protectedRoot) { + const err = new Error('EACCES'); + (err as NodeJS.ErrnoException).code = 'EACCES'; + throw err; + } + return originalReaddir(...args as any); + }); + + await expect(discoverSkillNames(protectedRoot)).rejects.toMatchObject({ code: 'EACCES' }); + readdirSpy.mockRestore(); + }); + + it('SKILL_NAMES export is lazily populated on first install', async () => { + vi.resetModules(); + const freshSetup = await import('../../src/cli/setup.js'); + const targetDir = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-lazy-skill-names-')); + + try { + expect(freshSetup.SKILL_NAMES).toEqual([]); + + const installed = await freshSetup.installSkillsTo(targetDir); + expect(freshSetup.SKILL_NAMES.length).toBeGreaterThan(0); + expect(installed.sort()).toEqual([...freshSetup.SKILL_NAMES].sort()); + } finally { + await fs.rm(targetDir, { recursive: true, force: true }); + } + }); }); From 774ac8f7bd6327c5929012789b0b15e0b9181c82 Mon Sep 17 00:00:00 2001 From: Linus Beckhaus Date: Sat, 7 Mar 2026 13:51:26 +0100 Subject: [PATCH 13/15] =?UTF-8?q?fix(skills):=20address=20review=20feedbac?= =?UTF-8?q?k=20=E2=80=94=20guard=20cleanup,=20fix=20README=20count?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Guard cleanupProjectLocalSkills on successful global install: don't remove local skills if no global replacement was actually installed - Update README skill count from 6 to 7, add gitnexus-pr-review entry - Stop swallowing non-ENOENT errors in installSkillsTo — only skip missing source skills, propagate real I/O failures 874/874 unit tests passing. --- README.md | 3 ++- gitnexus/src/cli/setup.ts | 14 ++++++++++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 9c5bd6279f..45c42fc44c 100644 --- a/README.md +++ b/README.md @@ -182,12 +182,13 @@ gitnexus wiki --base-url # Wiki with custom LLM API base URL | `detect_impact` | Pre-commit change analysis — scope, affected processes, risk level | | `generate_map` | Architecture documentation from the knowledge graph with mermaid diagrams | -**6 agent skills** installed to `~/.claude/skills/` via `gitnexus setup`: +**7 agent skills** installed to `~/.claude/skills/` via `gitnexus setup` (auto-discovered from disk): - **Exploring** — Navigate unfamiliar code using the knowledge graph - **Debugging** — Trace bugs through call chains - **Impact Analysis** — Analyze blast radius before changes - **Refactoring** — Plan safe refactors using dependency mapping +- **PR Review** — Review pull requests using the knowledge graph - **Guide** — GitNexus tool reference, graph schema, and workflow guidance - **CLI** — Run GitNexus CLI commands (analyze, status, clean, wiki, etc.) diff --git a/gitnexus/src/cli/setup.ts b/gitnexus/src/cli/setup.ts index 76f9263789..61aab42d47 100644 --- a/gitnexus/src/cli/setup.ts +++ b/gitnexus/src/cli/setup.ts @@ -327,8 +327,9 @@ export async function installSkillsTo(targetDir: string): Promise { await fs.writeFile(path.join(skillDir, 'SKILL.md'), content, 'utf-8'); installed.push(skillName); } - } catch { - // Source skill not found — skip + } catch (err: any) { + if (err?.code === 'ENOENT') continue; // Source skill not found — skip + throw err; // Real I/O errors should propagate } } @@ -467,8 +468,13 @@ export const setupCommand = async () => { await installCursorSkills(result); await installOpenCodeSkills(result); - // Clean up stale project-local skills left by previous `analyze` runs - await cleanupProjectLocalSkills(result); + // Clean up stale project-local skills left by previous `analyze` runs, + // but only if at least one global skill install succeeded (don't remove + // local skills without replacement). + const globalSkillsInstalled = result.configured.some(c => c.includes('skills (')); + if (globalSkillsInstalled) { + await cleanupProjectLocalSkills(result); + } // Print results if (result.configured.length > 0) { From 0e7301de4255546c098d15d4109dd1ab1277061a Mon Sep 17 00:00:00 2001 From: Linus Beckhaus Date: Sat, 7 Mar 2026 13:52:20 +0100 Subject: [PATCH 14/15] docs: update pr description --- docs/pr-description.md | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/docs/pr-description.md b/docs/pr-description.md index 6aba9692f6..9d4e33ac69 100644 --- a/docs/pr-description.md +++ b/docs/pr-description.md @@ -58,16 +58,19 @@ Beyond the duplication bug, skills are static markdown files that don't depend o ### Test plan -- [x] `analyze` no longer creates `.claude/skills/gitnexus/` (acceptance tests #1-2) -- [x] `analyze` prints deprecation notice when stale skills exist (#15-17) +- [x] `analyze` no longer creates `.claude/skills/gitnexus/` (acceptance tests 1-2) +- [x] `analyze` prints deprecation notice when stale skills exist (15-17) - [x] `analyze` shows notice even on "Already up to date" early return -- [x] `setup` installs all 7 discovered skills with non-empty SKILL.md (#7-9) -- [x] `setup` removes project-local skills in git repos (#11-12) -- [x] `setup` handles nested dirs, worktrees, non-git dirs, empty dirs (#13-14, nested/worktree tests) -- [x] `discoverSkillNames` discovers flat files, directories, mixed layouts (#27-33) -- [x] `discoverSkillNames` filters to `gitnexus-*` prefix only (#28) -- [x] `discoverSkillNames` ignores directories without SKILL.md (#33) -- [x] All existing tests unaffected — **870/870 passing** +- [x] `setup` installs all 7 discovered skills with non-empty SKILL.md (7-9) +- [x] `setup` removes project-local skills in git repos (11-12) +- [x] `setup` handles nested dirs, worktrees, non-git dirs, empty dirs (13-14, nested/worktree tests) +- [x] `discoverSkillNames` discovers flat files, directories, mixed layouts (27-33) +- [x] `discoverSkillNames` filters to `gitnexus-*` prefix only (28) +- [x] `discoverSkillNames` ignores directories without SKILL.md (33) +- [x] `discoverSkillNames` documents collision behavior for same-name flat+directory entries (34) +- [x] `discoverSkillNames` propagates errors: missing root (`ENOENT`), permission failure (`EACCES`) (35-36) +- [x] `SKILL_NAMES` lazy-population contract: starts empty, populated after first install (37) +- [x] All existing tests unaffected — **874/874 passing** ### Commits @@ -82,3 +85,4 @@ Beyond the duplication bug, skills are static markdown files that don't depend o | `e132f77` | docs: plan auto-discovery of skill names from disk | | `4a88ff6` | test: add acceptance tests for discoverSkillNames | | `011b2c1` | feat(skills): auto-discover skill names from disk | +| `2fa3dd1` | test(skills): add discovery edge-case coverage (#34-37) | From 2920f075b81542a29d9fb10c34d7e7dbb2350c0d Mon Sep 17 00:00:00 2001 From: Linus Beckhaus Date: Sat, 7 Mar 2026 13:56:47 +0100 Subject: [PATCH 15/15] =?UTF-8?q?chore:=20address=20review=20nitpicks=20?= =?UTF-8?q?=E2=80=94=20docs,=20unused=20variable?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Update PR doc: mark cleanup guard edge case as fixed, clarify remaining tip visibility note - Update test plan: mark gap #25 as resolved with guard reference - Remove unused dirSkillFile variable in installSkillsTo --- docs/pr-skill-installation-refactor.md | 6 +++--- docs/test-plan-skill-installation-refactor.md | 8 ++++---- gitnexus/src/cli/setup.ts | 1 - 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/docs/pr-skill-installation-refactor.md b/docs/pr-skill-installation-refactor.md index 4655295cc7..ff5b0ef120 100644 --- a/docs/pr-skill-installation-refactor.md +++ b/docs/pr-skill-installation-refactor.md @@ -193,9 +193,9 @@ Validation rerun in this branch: - `npx vitest run test/unit/ai-context.test.ts test/unit/setup-skills.test.ts test/unit/analyze-skills-notice.test.ts` → **25/25 passing** - `npm test` (unit suite) → **859/859 passing** -Residual edge cases found in review (not addressed in this branch): -- The updated `analyze` setup tip is effectively unreachable after successful indexing because `registerRepo()` creates the global registry before the tip check. -- `setup` cleanup runs regardless of whether any global skill install actually succeeded; on machines with no supported editor directories present, stale local skills could be removed without replacement. +Residual edge cases found in review: +- The updated `analyze` setup tip is effectively unreachable after successful indexing because `registerRepo()` creates the global registry before the tip check. (Unchanged — cosmetic, not a correctness issue.) +- ~~`setup` cleanup runs regardless of whether any global skill install actually succeeded.~~ **Fixed** in review feedback commit: cleanup now guarded by `globalSkillsInstalled` check — only runs if at least one global skill install succeeded. ### Phase 3b: Follow-up fixes for review findings #1 and #2 (completed) diff --git a/docs/test-plan-skill-installation-refactor.md b/docs/test-plan-skill-installation-refactor.md index b72610e987..ade1169b7b 100644 --- a/docs/test-plan-skill-installation-refactor.md +++ b/docs/test-plan-skill-installation-refactor.md @@ -127,10 +127,10 @@ These test the `discoverSkillNames()` function that replaces the hardcoded `SKIL The following edge cases were identified in review and are not yet covered by automated tests: -| # | Gap | Why it matters | -|---|-----|----------------| -| 25 | Cleanup behavior when no global skill target is installed/configured | Local skills may be removed even if no replacement global install succeeded | -| 26 | Setup tip visibility in `analyze` after successful indexing | Tip check happens after registry write, so the new MCP+skills tip may never surface in normal success path | +| # | Gap | Status | +|---|-----|--------| +| 25 | Cleanup behavior when no global skill target is installed/configured | **Resolved** — cleanup now guarded by `globalSkillsInstalled` in `setup.ts`; local skills are only removed if a global install succeeded | +| 26 | Setup tip visibility in `analyze` after successful indexing | Open — tip check happens after registry write, so the MCP+skills tip may never surface in the normal success path (cosmetic) | ## Execution diff --git a/gitnexus/src/cli/setup.ts b/gitnexus/src/cli/setup.ts index 61aab42d47..25b3c00014 100644 --- a/gitnexus/src/cli/setup.ts +++ b/gitnexus/src/cli/setup.ts @@ -308,7 +308,6 @@ export async function installSkillsTo(targetDir: string): Promise { try { // Try directory-based skill first (skills/{name}/SKILL.md) const dirSource = path.join(skillsRoot, skillName); - const dirSkillFile = path.join(dirSource, 'SKILL.md'); let isDirectory = false; try {