From 911b06017b509be7df1c11618524d3dee7cd2af3 Mon Sep 17 00:00:00 2001 From: Erdem Tuna Date: Mon, 9 Mar 2026 20:49:16 +0000 Subject: [PATCH 01/10] Initialize PAW workflow for Flat Install Layout Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .paw/work/flat-install-layout/WorkShaping.md | 178 ++++++++++++++++++ .../flat-install-layout/WorkflowContext.md | 30 +++ 2 files changed, 208 insertions(+) create mode 100644 .paw/work/flat-install-layout/WorkShaping.md create mode 100644 .paw/work/flat-install-layout/WorkflowContext.md diff --git a/.paw/work/flat-install-layout/WorkShaping.md b/.paw/work/flat-install-layout/WorkShaping.md new file mode 100644 index 0000000..f0121f2 --- /dev/null +++ b/.paw/work/flat-install-layout/WorkShaping.md @@ -0,0 +1,178 @@ +# Work Shaping: Flat Install Layout for Agent Directories + +## Problem Statement + +**Who benefits:** Users who install craft skills globally for AI agents (Claude Code, GitHub Copilot). + +**What problem is solved:** Nested `github.com/org/repo/skill/` directory paths break agent skill discovery. Agents expect flat directories under their skills root (e.g., `~/.claude/skills/my-skill/`), not deeply nested trees. The current composite-key layout, while excellent for project-scoped `forge/` vendoring, creates paths that agents cannot traverse. + +**Example of the problem:** +``` +# Current (broken for agents): +~/.claude/skills/github.com/lossyrob/phased-agent-workflow/paw-implement/SKILL.md + +# Desired (agent-discoverable): +~/.claude/skills/github-com--lossyrob--phased-agent-workflow--paw-implement/SKILL.md +``` + +## Work Breakdown + +### Core Functionality + +1. **`FlatKey()` function** in `internal/install/installer.go` + - Input: composite key `github.com/org/repo/skill` + - Output: flat key `github-com--org--repo--skill` + - Rules: + - `/` → `--` (double-dash separates path components) + - `.` → `-` (dots become single dashes, applied to ALL segments) + - Casing preserved (no lowercasing) + - `--` separator is collision-safe: GitHub org/repo names and directory-derived skill names cannot contain `--` + +2. **`InstallFlat()` method** in `internal/install/installer.go` + - Same atomic staging + swap semantics as `Install()` + - Same path traversal security validation + - Transforms composite keys via `FlatKey()` before writing + - Encapsulates the flat layout decision within the install package + +3. **CLI wiring** in `install.go`, `get.go`, `update.go` + - Global installs → call `InstallFlat()` instead of `Install()` + - Project installs → continue calling `Install()` (nested layout in `forge/`) + +4. **Remove cleanup** in `remove.go` + - Global removes: use `FlatKey()` to build directory name, `os.RemoveAll()`, skip `cleanEmptyParents` (no parents to clean in flat layout) + - Project removes: keep existing nested cleanup with `cleanEmptyParents` + +### Supporting Work + +5. **Tests** in `installer_test.go` + - 5+ new tests for `InstallFlat()`/`FlatKey()` + - Existing tests (including `TestInstallCompositeKeys`) remain unchanged + +6. **Documentation** updates to `README.md` and `E2E_REAL_WORLD_TEST.md` + - Update expected directory structures for global installs + - Document flat key format + +## Edge Cases + +| Edge Case | Expected Handling | +|-----------|-------------------| +| Dots in non-host segments (e.g., `github.com/my.org/repo/skill`) | All dots → dashes: `github-com--my-org--repo--skill` | +| Skill names with dashes (e.g., `paw-implement`) | Preserved as-is; `--` separator is unambiguous | +| Mixed casing (e.g., `GitHub.com/MyOrg/Repo/Skill`) | Preserved: `GitHub-com--MyOrg--Repo--Skill` | +| Existing nested global installs | N/A | No existing installs use nested layout — clean-slate change | +| Project-scoped installs (`forge/`) | Unchanged — continue using nested composite-key layout | +| Remove after flat install | Pinfile has original composite keys; apply `FlatKey()` to derive directory name | +| `cleanEmptyParents` on global remove | No-op — flat dirs have no parents to clean | + +## Architecture + +### Component Interactions + +``` +CLI Layer (install.go / get.go / update.go / remove.go) + │ + ├── Global scope ──→ InstallFlat(agentSkillsDir, skills) + │ │ + │ ├── FlatKey(compositeKey) → flat directory name + │ ├── Staging + atomic swap (same as Install) + │ └── Path traversal validation (same as Install) + │ + └── Project scope ──→ Install(forgeDir, skills) [unchanged] +``` + +### Data Flow for Global Install + +``` +craft.pin.yaml CLI (install.go) installer.go +┌──────────────┐ ┌──────────────────┐ ┌───────────────────┐ +│ skills: │ │ │ │ InstallFlat() │ +│ github.com/ │ ──→ │ collectSkillFiles│ ──→ │ for each skill: │ +│ org/repo/ │ │ (composite keys) │ │ FlatKey(key) │ +│ skill │ │ │ │ stage + swap │ +└──────────────┘ └──────────────────┘ └───────────────────┘ + │ + ▼ + ~/.claude/skills/ + github-com--org--repo--skill/ + SKILL.md +``` + +### Data Flow for Global Remove + +``` +craft.pin.yaml CLI (remove.go) filesystem +┌──────────────┐ ┌──────────────────┐ ┌────────────────────────┐ +│ skills: │ │ for each skill: │ │ │ +│ github.com/ │ ──→ │ FlatKey(key) │ ──→ │ os.RemoveAll( │ +│ org/repo/ │ │ build path │ │ target/flat-key/) │ +│ skill │ │ (no parent │ │ (no parent cleanup) │ +└──────────────┘ │ cleanup) │ └────────────────────────┘ + └──────────────────┘ +``` + +## Critical Analysis + +### Value Assessment + +- **High value**: This is a blocking issue — global installs are currently non-functional for agent discovery +- **Low risk**: The change is well-scoped — `Install()` is untouched, `InstallFlat()` is additive +- **Clean separation**: Project (nested) vs global (flat) is a clear boundary + +### Build vs Modify Tradeoffs + +- **New code**: `FlatKey()` (~10 lines), `InstallFlat()` (~15 lines wrapping `Install()` with key transform) +- **Modified code**: 4 CLI files (routing to `InstallFlat` for global), 1 file (`remove.go` for flat cleanup) +- **Total new code**: Minimal — most logic is reused from existing `Install()` + +### No Backward Compatibility Concern + +- No existing global installs use the nested layout — clean-slate change +- No migration, no orphaned directories, no stale-remove risk + +## Codebase Fit + +### Reuse Opportunities + +- `InstallFlat()` can delegate to `Install()` after transforming keys, reusing all atomic staging, path validation, and file-writing logic +- `FlatKey()` is a pure function — easy to test in isolation +- Remove logic already looks up skills from pinfile — just needs `FlatKey()` applied for global scope + +### Similar Patterns + +- The existing `Install()` → staging → atomic swap pattern is well-established and proven +- CLI already branches on `globalFlag` for project vs global behavior — adding `InstallFlat()` call follows existing pattern + +## Risk Assessment + +| Risk | Likelihood | Impact | Mitigation | +|------|-----------|--------|------------| +| N/A — no existing nested global installs | — | — | Clean-slate; no migration needed | +| `--` separator collision | Very Low | High | Validated: GitHub disallows `--` in org/repo names | +| Dot-to-dash creates ambiguous keys | Very Low | Medium | In practice, dots in org/repo/skill names are extremely rare | +| `InstallFlat()` diverges from `Install()` over time | Low | Medium | `InstallFlat()` should delegate to `Install()` internally | + +## Implementation Phases (from brief) + +| Phase | Files | What | +|-------|-------|------| +| 1 | `installer.go` | Add `FlatKey()` + `InstallFlat()` | +| 2 | `install.go`, `get.go`, `update.go` | Wire `InstallFlat()` for global paths | +| 3 | `remove.go` | Use `FlatKey()` for global cleanup, keep nested for project | +| 4 | `installer_test.go` | 5+ new tests (FlatKey, basic, multi-package, same-name, overwrite) + FlatKey edge case tests | +| 5 | `README.md`, `E2E_REAL_WORLD_TEST.md` | Update expected directory structures | + +## Open Questions for Downstream Stages + +1. **Should `InstallFlat()` be a wrapper that transforms keys then calls `Install()`, or a parallel implementation?** (Recommendation: wrapper/delegate to minimize code duplication) +2. **Should `craft list -g` / `craft tree -g` display flat keys or reconstructed composite keys?** (Display is cosmetic; doesn't affect this feature but worth deciding) + +## Session Notes + +- **Key decision**: No backward compatibility / no migration. Clean break. +- **Key decision**: `--` separator validated as collision-safe by user. +- **Key decision**: Dots replaced in ALL segments, not just host. +- **Key decision**: Casing preserved (no lowercasing). +- **Key decision**: `FlatKey()` + `InstallFlat()` encapsulated in install package (not CLI-layer transform). +- **Key decision**: Existing tests unchanged; new tests added alongside. +- **Key decision**: `cleanEmptyParents` is a no-op for flat global removes — just `os.RemoveAll` the single flat directory. +- **Discovery**: Pinfile always has original composite keys, so `FlatKey()` can be applied at remove time without needing to reverse the transformation. diff --git a/.paw/work/flat-install-layout/WorkflowContext.md b/.paw/work/flat-install-layout/WorkflowContext.md new file mode 100644 index 0000000..466b5da --- /dev/null +++ b/.paw/work/flat-install-layout/WorkflowContext.md @@ -0,0 +1,30 @@ +# WorkflowContext + +Work Title: Flat Install Layout +Work ID: flat-install-layout +Base Branch: main +Target Branch: feature/flat-install-layout +Workflow Mode: full +Review Strategy: local +Review Policy: milestones +Session Policy: continuous +Final Agent Review: enabled +Final Review Mode: multi-model +Final Review Interactive: smart +Final Review Models: gpt-5.2, gemini-3-pro-preview, claude-opus-4.6 +Final Review Specialists: all +Final Review Interaction Mode: parallel +Final Review Specialist Models: none +Plan Generation Mode: single-model +Plan Generation Models: gpt-5.2, gemini-3-pro-preview, claude-opus-4.6 +Planning Docs Review: enabled +Planning Review Mode: multi-model +Planning Review Interactive: smart +Planning Review Models: gpt-5.2, gemini-3-pro-preview, claude-opus-4.6 +Custom Workflow Instructions: none +Initial Prompt: Flat install layout for agent directories. Nested github.com/org/repo/skill/ paths break agent skill discovery. Use host--org--repo--skill format for global installs. Project forge/ keeps nested layout. +Issue URL: none +Remote: origin +Artifact Lifecycle: commit-and-clean +Artifact Paths: auto-derived +Additional Inputs: none From f93fd3d958882c74de618fedb09933a1e6eca2ef Mon Sep 17 00:00:00 2001 From: Erdem Tuna Date: Mon, 9 Mar 2026 20:54:01 +0000 Subject: [PATCH 02/10] Add specification for flat install layout Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .paw/work/flat-install-layout/Spec.md | 151 ++++++++++++++++++++++++++ 1 file changed, 151 insertions(+) create mode 100644 .paw/work/flat-install-layout/Spec.md diff --git a/.paw/work/flat-install-layout/Spec.md b/.paw/work/flat-install-layout/Spec.md new file mode 100644 index 0000000..334ee65 --- /dev/null +++ b/.paw/work/flat-install-layout/Spec.md @@ -0,0 +1,151 @@ +# Feature Specification: Flat Install Layout for Agent Directories + +**Branch**: feature/flat-install-layout | **Created**: 2026-03-09 | **Status**: Draft +**Input Brief**: Flatten global skill install paths so AI agents can discover them + +## Overview + +When craft installs skills globally for AI agents like Claude Code or GitHub Copilot, it currently writes them into deeply nested directory trees mirroring the source repository path — for example, `~/.claude/skills/github.com/lossyrob/phased-agent-workflow/paw-implement/`. These agents expect skills to appear as immediate children of their skills directory. The nested layout makes globally installed skills invisible to agent discovery. + +This feature introduces a flat directory naming scheme for global installs. Each skill's composite key (host/owner/repo/skill) is transformed into a single flat directory name using `--` as a component separator and replacing dots with dashes — producing names like `github-com--lossyrob--phased-agent-workflow--paw-implement`. The flat format is unique, collision-safe, and places every skill as a direct child of the agent's skills root. + +Project-scoped installs (`forge/` directory) remain unchanged. The nested composite-key layout is well-suited for vendored dependencies where agent discovery is not a concern. This separation keeps the change surgical: global installs get flat layout, project installs keep nested layout. + +The `list` and `tree` commands for global scope will display skills using their original composite key format (e.g., `github.com/org/repo/skill`) rather than the flat directory name, preserving readability. + +## Objectives + +- Enable AI agents to discover globally installed skills by placing them as direct children of the skills root directory +- Prevent skill name collisions across different source repositories through a deterministic flat naming scheme +- Maintain the existing nested layout for project-scoped installs, preserving vendoring semantics +- Keep removal operations correct by deriving flat directory names from pinfile composite keys +- Display globally installed skills in `list`/`tree` output using readable composite key format + +## User Scenarios & Testing + +### User Story P1 – Global Skill Installation with Agent Discovery + +Narrative: A developer runs `craft get github.com/acme/tools@v1.0.0` to install a skill for their AI agent. The skill appears as a flat directory under the agent's skills root, and the agent can immediately discover and use it. + +Independent Test: After `craft get`, verify the skill exists at `~/.claude/skills/github-com--acme--tools--my-skill/SKILL.md` (not nested under `github.com/acme/tools/`). + +Acceptance Scenarios: +1. Given a user runs `craft get github.com/acme/tools@v1.0.0`, When the install completes, Then skills appear as flat directories directly under the agent's skills root +2. Given a user runs `craft install -g`, When the install completes, Then all global skills use flat directory names +3. Given a skill `github.com/org/repo/my-skill`, When installed globally, Then the directory name is `github-com--org--repo--my-skill` + +### User Story P2 – Global Skill Update Preserves Flat Layout + +Narrative: A developer updates a globally installed skill to a newer version. The updated files appear in the same flat directory without creating nested paths. + +Independent Test: After `craft update -g`, verify the skill directory is still flat and contains updated files. + +Acceptance Scenarios: +1. Given a globally installed skill at version v1.0.0, When the user runs `craft update -g`, Then the updated skill overwrites the same flat directory +2. Given multiple globally installed skills, When the user runs `craft update -g alias`, Then only the target skill is updated and all directories remain flat + +### User Story P3 – Global Skill Removal Cleans Flat Directory + +Narrative: A developer removes a globally installed skill. The flat directory is deleted completely, with no orphaned parent directories. + +Independent Test: After `craft remove -g alias`, verify the flat directory no longer exists and no empty parent directories were created. + +Acceptance Scenarios: +1. Given a globally installed skill, When the user runs `craft remove -g alias`, Then the flat skill directory is removed +2. Given a global removal, When cleanup completes, Then no empty intermediate directories remain (there are none to begin with in flat layout) + +### User Story P4 – Project Install Unchanged + +Narrative: A developer runs `craft install` in a project. Skills are vendored into `forge/` using the existing nested composite-key layout, unaffected by the flat layout change. + +Independent Test: After `craft install`, verify skills exist at `forge/github.com/org/repo/skill/` (nested, not flat). + +Acceptance Scenarios: +1. Given a project with dependencies, When the user runs `craft install`, Then skills appear under `forge/` in nested composite-key paths +2. Given `craft remove alias` in project scope, Then nested directories and their empty parents are cleaned up as before + +### User Story P5 – Global List and Tree Display + +Narrative: A developer runs `craft list -g` or `craft tree -g` to see their globally installed skills. Skills are displayed using the original composite key format for readability, not the flat directory name. + +Independent Test: After installing skills globally, `craft list -g` shows `github.com/org/repo/skill` (not `github-com--org--repo--skill`). + +Acceptance Scenarios: +1. Given globally installed skills, When the user runs `craft list -g`, Then skills display as `github.com/org/repo/skill` +2. Given globally installed skills, When the user runs `craft tree -g`, Then the tree shows composite key paths + +### Edge Cases +- Composite key with dots in non-host segments (e.g., `github.com/my.org/repo/skill`) → flat key: `github-com--my-org--repo--skill` +- Skill names containing single dashes (e.g., `paw-implement`) → preserved in flat key; `--` separator remains unambiguous +- Mixed casing in composite keys → casing preserved in flat key +- Empty skill map → `InstallFlat()` succeeds with no-op (same as `Install()`) + +## Requirements + +### Functional Requirements + +- FR-001: A `FlatKey()` function converts composite keys to flat directory names by replacing `/` with `--` and `.` with `-`, preserving casing (Stories: P1, P2, P3) +- FR-002: An `InstallFlat()` method installs skills using flat directory names with the same atomic staging, security validation, and overwrite semantics as `Install()` (Stories: P1, P2) +- FR-003: Global install commands (`craft install -g`, `craft get`, `craft update -g`) use `InstallFlat()` for writing skills to agent directories (Stories: P1, P2) +- FR-004: Global remove (`craft remove -g`) uses `FlatKey()` to derive directory names for cleanup, skipping parent directory cleanup (Stories: P3) +- FR-005: Project install commands continue using `Install()` with nested composite-key layout (Stories: P4) +- FR-006: Project remove continues using nested paths with parent directory cleanup (Stories: P4) +- FR-007: `craft list -g` and `craft tree -g` display skills using original composite key format from the pinfile (Stories: P5) + +### Key Entities + +- **Composite Key**: The `host/owner/repo/skill` string used internally (e.g., `github.com/org/repo/my-skill`) +- **Flat Key**: The transformed directory name for global installs (e.g., `github-com--org--repo--my-skill`) + +### Cross-Cutting / Non-Functional + +- Path traversal protection applies equally to flat keys (same validation as nested keys) +- Flat install behavior must remain consistent with nested install behavior (atomicity, security validation, overwrite semantics) + +## Success Criteria + +- SC-001: After global install, every skill directory is a direct child of the agent skills root (no nested subdirectories) (FR-002, FR-003) +- SC-002: `FlatKey()` output is deterministic — same input always produces same output (FR-001) +- SC-003: Two skills with the same leaf name from different repos produce different flat keys (FR-001) +- SC-004: After global remove, the flat skill directory no longer exists (FR-004) +- SC-005: Project installs produce nested paths under `forge/` unchanged from current behavior (FR-005) +- SC-006: `craft list -g` displays composite key format, not flat directory names (FR-007) +- SC-007: All existing tests continue to pass without modification (FR-005, FR-006) + +## Assumptions + +- GitHub does not allow `--` in organization or repository names, making `--` a collision-safe separator +- Dots in organization, repository, and skill names are extremely rare on GitHub, minimizing ambiguity risk from dot-to-dash conversion +- AI agents (Claude Code, GitHub Copilot) discover skills by scanning immediate children of their skills directory +- No existing global installs use the nested layout — this is a clean-slate change with no migration needed + +## Scope + +In Scope: +- `FlatKey()` pure function and `InstallFlat()` method in installer +- Wiring global install/update commands to use `InstallFlat()` +- Wiring global remove to use `FlatKey()` for cleanup +- Updating `list -g` and `tree -g` display to use composite key format +- New tests for `FlatKey()` and `InstallFlat()` +- Documentation updates (README.md, E2E_REAL_WORLD_TEST.md) + +Out of Scope: +- Changes to project-scoped install/remove behavior (`forge/` layout) +- Migration of hypothetical pre-existing nested global installs +- Changes to pinfile format or resolution logic +- Changes to `Install()` function + +## Dependencies + +- Existing `Install()` function — `InstallFlat()` delegates to it +- Pinfile composite keys — used by remove to derive flat directory names +- Agent detection (`internal/agent/`) — determines global install target paths + +## Risks & Mitigations + +- **Dot-to-dash ambiguity**: Two different composite keys could theoretically produce the same flat key if they differ only in dots vs dashes. Impact: skill collision. Mitigation: This is extremely unlikely in practice; GitHub org/repo names rarely contain dots. +- **`InstallFlat()` divergence from `Install()`**: If `InstallFlat()` is implemented as a copy rather than a wrapper, future changes to `Install()` might not be reflected. Mitigation: Implement `InstallFlat()` as a thin wrapper that transforms keys then delegates to `Install()`. + +## References + +- WorkShaping: .paw/work/flat-install-layout/WorkShaping.md From 707832d6a658dc600d527b1f7e98f9873821503c Mon Sep 17 00:00:00 2001 From: Erdem Tuna Date: Mon, 9 Mar 2026 20:57:26 +0000 Subject: [PATCH 03/10] Add code research for flat install layout Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .paw/work/flat-install-layout/CodeResearch.md | 502 ++++++++++++++++++ 1 file changed, 502 insertions(+) create mode 100644 .paw/work/flat-install-layout/CodeResearch.md diff --git a/.paw/work/flat-install-layout/CodeResearch.md b/.paw/work/flat-install-layout/CodeResearch.md new file mode 100644 index 0000000..0df950a --- /dev/null +++ b/.paw/work/flat-install-layout/CodeResearch.md @@ -0,0 +1,502 @@ +# Code Research: Flat Install Layout + +## 1. installer.go — `Install()` Function + +**File**: `internal/install/installer.go` + +### Signature +[installer.go:17](internal/install/installer.go#L17) +```go +func Install(target string, skills map[string]map[string][]byte) error +``` + +- `target`: root directory to install into (e.g., `~/.claude/skills/` or `forge/`) +- `skills`: map of composite key → (relative file path → content) + +### Full Function Body (lines 17–88) + +**Target directory creation and resolution** [installer.go:18–25](internal/install/installer.go#L18): +```go +if err := os.MkdirAll(target, 0o700); err != nil { + return fmt.Errorf("creating target directory: %w", err) +} +absTarget, err := filepath.Abs(target) +if err != nil { + return fmt.Errorf("resolving target path: %w", err) +} +``` + +**Skill iteration loop** [installer.go:27](internal/install/installer.go#L27): +```go +for skillName, files := range skills { +``` +- `skillName` is the composite key (e.g., `github.com/org/repo/my-skill`) +- `files` is `map[string][]byte` of relative paths to content + +**Directory creation via `filepath.Join`** [installer.go:28](internal/install/installer.go#L28): +```go +skillDir := filepath.Join(target, skillName) +``` +- When `skillName` is `github.com/org/repo/my-skill`, this creates nested dirs automatically via OS path semantics. + +**Path traversal validation** [installer.go:29–35](internal/install/installer.go#L29): +```go +absSkillDir, err := filepath.Abs(skillDir) +if err != nil { + return fmt.Errorf("resolving skill path: %w", err) +} +if !strings.HasPrefix(absSkillDir, absTarget+string(filepath.Separator)) { + return fmt.Errorf("skill name %q escapes target directory", skillName) +} +``` + +**Staging directory** [installer.go:38–44](internal/install/installer.go#L38): +```go +stagingDir := skillDir + ".staging" +_ = os.RemoveAll(stagingDir) +if err := os.MkdirAll(stagingDir, 0o700); err != nil { + return fmt.Errorf("creating staging directory for %q: %w", skillName, err) +} +``` + +**File writing with per-file path traversal check** [installer.go:47–68](internal/install/installer.go#L47): +```go +writeErr := func() error { + for relPath, content := range files { + fullPath := filepath.Join(stagingDir, relPath) + absFullPath, err := filepath.Abs(fullPath) + // ...validate against staging dir... + if !strings.HasPrefix(absFullPath, filepath.Clean(stagingDir)+string(filepath.Separator)) { + return fmt.Errorf("file path %q escapes skill directory", relPath) + } + // ...MkdirAll + WriteFile... + } + return nil +}() +``` + +**Atomic swap** [installer.go:76–84](internal/install/installer.go#L76): +```go +if err := os.RemoveAll(skillDir); err != nil { ... } +if err := os.Rename(stagingDir, skillDir); err != nil { ... } +``` + +### Key Observations for `InstallFlat()` +- `Install()` already handles composite keys as `skillName` — it just passes them through `filepath.Join(target, skillName)` which creates nested directories. +- `InstallFlat()` can transform keys via `FlatKey()` then call `Install()` with modified map, OR duplicate the loop with flat path construction. +- The spec says `InstallFlat()` should delegate to `Install()` — transforming keys then passing through. This reuses all staging, validation, and atomicity logic. + +--- + +## 2. install.go — Global vs Project Install Calls + +**File**: `internal/cli/install.go` + +### Global Install Path + +**`runInstall` dispatches on `globalFlag`** [install.go:43–48](internal/cli/install.go#L43): +```go +func runInstall(cmd *cobra.Command, args []string) error { + if globalFlag { + return runInstallGlobal(cmd) + } + return runInstallProject(cmd) +} +``` + +**Target resolution for global** [install.go:106–109](internal/cli/install.go#L106): +```go +targetPaths, err := resolveInstallTargets(installTarget) +if err != nil { + return err +} +``` +- `targetPaths` is `[]string` — one or more agent install directories (e.g., `~/.claude/skills/`). +- Resolved by `resolveInstallTargets()` at [install.go:256](internal/cli/install.go#L256). + +**Skill files collection** [install.go:111–115](internal/cli/install.go#L111): +```go +skillFiles, err := collectSkillFiles(fetcher, result) +``` +- Returns `map[string]map[string][]byte` — composite key → files. + +**`Install()` call for global** [install.go:123–128](internal/cli/install.go#L123): +```go +for _, targetPath := range targetPaths { + if err := installlib.Install(targetPath, skillFiles); err != nil { + progress.Fail("Installation failed") + return fmt.Errorf("installation failed: %w", err) + } +} +``` +→ **This loop needs to change to call `installlib.InstallFlat(targetPath, skillFiles)` for global installs.** + +### Project Install Path + +**forge path** [install.go:206](internal/cli/install.go#L206): +```go +forgePath := filepath.Join(root, "forge") +``` + +**`Install()` call for project** [install.go:222–226](internal/cli/install.go#L222): +```go +if err := installlib.Install(forgePath, skillFiles); err != nil { + progress.Fail("Vendoring failed") + return fmt.Errorf("vendoring failed: %w", err) +} +``` +→ **This stays as `Install()` — project layout unchanged.** + +### `collectSkillFiles` — Composite Key Construction + +[install.go:351–397](internal/cli/install.go#L351) + +Composite key is built at [install.go:391](internal/cli/install.go#L391): +```go +compositeKey := prefix + "/" + skillName +skills[compositeKey] = files +``` +Where `prefix` = `parsed.PackageIdentity()` (e.g., `github.com/org/repo`) and `skillName` is the skill's leaf name (e.g., `my-skill`). Result: `github.com/org/repo/my-skill`. + +### Import Alias + +[install.go:14](internal/cli/install.go#L14): +```go +installlib "github.com/erdemtuna/craft/internal/install" +``` + +--- + +## 3. get.go — `Install()` Call + +**File**: `internal/cli/get.go` + +**Import** [get.go:10](internal/cli/get.go#L10): +```go +installlib "github.com/erdemtuna/craft/internal/install" +``` + +**Target resolution** [get.go:220](internal/cli/get.go#L220): +```go +targetPaths, err := resolveInstallTargets(getTarget) +``` + +**Skill files collection** [get.go:226](internal/cli/get.go#L226): +```go +skillFiles, err := collectSkillFiles(fetcher, result) +``` + +**`Install()` call** [get.go:240–245](internal/cli/get.go#L240): +```go +for _, targetPath := range targetPaths { + if err := installlib.Install(targetPath, skillFiles); err != nil { + progress.Fail("Installation failed") + return fmt.Errorf("installation failed: %w\n note: ...", err) + } +} +``` +→ **Needs to change to `installlib.InstallFlat()`** — `get` is always global scope. + +--- + +## 4. update.go — `Install()` Call + +**File**: `internal/cli/update.go` + +**Import** [update.go:12](internal/cli/update.go#L12): +```go +installlib "github.com/erdemtuna/craft/internal/install" +``` + +**Global branch** [update.go:214–231](internal/cli/update.go#L214): +```go +if globalFlag { + targetPaths, err := resolveInstallTargets(updateTarget) + // ... + for _, targetPath := range targetPaths { + if err := installlib.Install(targetPath, skillFiles); err != nil { + progress.Fail("Installation failed") + return fmt.Errorf("installation failed: %w", err) + } + } +``` +→ **`Install()` at [update.go:227](internal/cli/update.go#L227) needs to change to `InstallFlat()` for global scope.** + +**Project branch** [update.go:244–254](internal/cli/update.go#L244): +```go +forgePath := filepath.Join(root, "forge") +// ... +if err := installlib.Install(forgePath, skillFiles); err != nil { +``` +At [update.go:251](internal/cli/update.go#L251) — **stays as `Install()`**. + +--- + +## 5. remove.go — Deletion Logic + +**File**: `internal/cli/remove.go` + +### Scope Branching + +**Global vs project** [remove.go:37–53](internal/cli/remove.go#L37): +```go +if globalFlag { + manifestPath, err = GlobalManifestPath() + // ... + pfPath, err = GlobalPinfilePath() +} else { + root, err = os.Getwd() + // ... + manifestPath = filepath.Join(root, "craft.yaml") + pfPath = filepath.Join(root, "craft.pin.yaml") +} +``` + +### Skill Name Source + +**Removed skill names from pinfile** [remove.go:78–82](internal/cli/remove.go#L78): +```go +if pfErr == nil { + if entry, ok := pf.Resolved[depURL]; ok { + removedSkills = entry.Skills + } +} +``` +- `removedSkills` contains leaf skill names (e.g., `["my-skill", "other-skill"]`), NOT composite keys. + +### Target Resolution for Cleanup + +[remove.go:111–121](internal/cli/remove.go#L111): +```go +if globalFlag { + targetPath, err = resolveInstallTargets(removeTarget) +} else if removeTarget != "" { + targetPath = []string{removeTarget} +} else { + targetPath = []string{filepath.Join(root, "forge")} +} +``` + +### Path Construction for Deletion + +**Namespace prefix** [remove.go:129–135](internal/cli/remove.go#L129): +```go +parsed, parseErr := resolve.ParseDepURL(depURL) +// ... +nsPrefix := parsed.PackageIdentity() +``` +- `nsPrefix` = `github.com/org/repo` + +**Skill directory path** [remove.go:141](internal/cli/remove.go#L141): +```go +skillDir := filepath.Join(tp, nsPrefix, skillName) +``` +- Produces nested path: `~/.claude/skills/github.com/org/repo/my-skill` +→ **For global scope, this needs to use `FlatKey(nsPrefix + "/" + skillName)` instead, producing `~/.claude/skills/github-com--org--repo--my-skill`.** + +### Path Traversal Protection + +[remove.go:143–154](internal/cli/remove.go#L143): +```go +absSkillDir, err := filepath.Abs(skillDir) +// ... +absTarget, err := filepath.Abs(tp) +// ... +if !strings.HasPrefix(absSkillDir, absTarget+string(filepath.Separator)) { + cmd.PrintErrf(" warning: skill name %q escapes target directory, skipping\n", skillName) + continue +} +``` + +### Deletion + Parent Cleanup + +[remove.go:156–162](internal/cli/remove.go#L156): +```go +if _, err := os.Stat(skillDir); err == nil { + if err := os.RemoveAll(skillDir); err != nil { + cmd.PrintErrf(" warning: could not remove %s: %v\n", skillDir, err) + } else { + removedFromAny = true + cleanEmptyParents(tp, filepath.Dir(skillDir)) + } +} +``` +→ **For global flat layout, `cleanEmptyParents` should be skipped (no nested parents exist).** + +### `cleanEmptyParents` Function + +[remove.go:181–196](internal/cli/remove.go#L181): +```go +func cleanEmptyParents(root, dir string) { + absRoot, err := filepath.Abs(root) + if err != nil { + return + } + for { + absDir, err := filepath.Abs(dir) + if err != nil || absDir == absRoot || !strings.HasPrefix(absDir, absRoot+string(filepath.Separator)) { + break + } + if err := os.Remove(dir); err != nil { + break // not empty or permission error + } + dir = filepath.Dir(dir) + } +} +``` +- Walks up from `dir` to `root`, removing empty dirs. Safe — `os.Remove` fails on non-empty. +- For flat layout, `filepath.Dir(flatSkillDir)` == `target`, so `cleanEmptyParents` would be a no-op anyway. But spec says to skip the call entirely for clarity. + +--- + +## 6. list.go / tree.go — Skill Name Display + +### list.go + +**File**: `internal/cli/list.go` + +**Data source**: pinfile entries [list.go:59–77](internal/cli/list.go#L59): +```go +for key, entry := range pf.Resolved { + parsed, err := resolve.ParseDepURL(key) + // ... + alias := urlToAlias[parsed.PackageIdentity()] + if alias == "" { + alias = parsed.Repo + } + deps = append(deps, depInfo{ + alias: alias, + version: parsed.RefString(), + url: parsed.PackageIdentity(), + skills: entry.Skills, + }) +} +``` + +**Display — default (tabular)** [list.go:94–101](internal/cli/list.go#L94): +```go +_, _ = fmt.Fprintf(w, "%s\t%s\t(%d %s)\n", sanitize(d.alias), d.version, len(d.skills), skillWord) +``` +- Shows alias, version, skill count. Does NOT show individual skill names in default mode. + +**Display — detailed** [list.go:84–92](internal/cli/list.go#L84): +```go +cmd.Printf("%s %s %s\n", sanitize(d.alias), d.version, sanitize(d.url)) +if len(d.skills) > 0 { + cmd.Printf(" skills: %s\n", sanitize(strings.Join(d.skills, ", "))) +} +``` +- `d.url` = `parsed.PackageIdentity()` = `github.com/org/repo` +- `d.skills` = `entry.Skills` = leaf skill names from pinfile (e.g., `["my-skill"]`) +- **Skill names displayed are leaf names from pinfile**, not composite keys and not flat keys. + +### tree.go + +**File**: `internal/cli/tree.go` + +**Data source**: pinfile entries [tree.go:47–65](internal/cli/tree.go#L47): +```go +for key, entry := range pf.Resolved { + parsed, err := resolve.ParseDepURL(key) + // ... + alias := urlToAlias[parsed.PackageIdentity()] + if alias == "" { + alias = parsed.Repo + } + deps = append(deps, ui.DepNode{ + Alias: alias, + URL: key, + Skills: entry.Skills, + }) +} +``` + +**Rendering** via `ui.RenderTree` at [tree.go:67](internal/cli/tree.go#L67): +```go +ui.RenderTree(cmd.OutOrStdout(), packageName, localSkills, deps) +``` + +### ui/tree.go — RenderTree + +**File**: `internal/ui/tree.go` + +**Dep node display** [tree.go:58](internal/ui/tree.go#L58): +```go +_, _ = fmt.Fprintf(w, "%s%s (%s)\n", connector, dep.Alias, dep.URL) +``` +- Shows alias and full dep URL (e.g., `github.com/org/repo@v1.0.0`) + +**Skill display** [tree.go:60–65](internal/ui/tree.go#L60): +```go +for j, skill := range dep.Skills { + // ... + _, _ = fmt.Fprintf(w, "%s%s%s\n", childPrefix, skillConn, skill) +} +``` +- `skill` = leaf name from pinfile (e.g., `my-skill`). + +### Key Observation for Spec FR-007 + +The spec says `list -g` and `tree -g` should display skills using **composite key format** (`github.com/org/repo/skill`). Currently: + +- **list.go default**: shows alias + version + skill count — no change needed (no skill names displayed). +- **list.go --detailed**: shows `d.url` (= `PackageIdentity()` = `github.com/org/repo`) on the header line, then leaf skill names. The spec wants composite key format for skills → change `d.skills` display to prefix each with `d.url + "/"`. +- **tree.go**: shows leaf skill names under each dep node. The spec wants composite key format → prefix each skill with the package identity. + +Both commands get skill names from **pinfile** (`entry.Skills`), not from filesystem scanning. No filesystem scan is involved — this is purely a display concern. + +--- + +## 7. installer_test.go — Test Structure + +**File**: `internal/install/installer_test.go` + +### Test Inventory + +| Test | Lines | What it tests | +|------|-------|---------------| +| `TestInstallCreatesStructure` | [10–30](internal/install/installer_test.go#L10) | Basic install creates dir + files | +| `TestInstallOverwrites` | [32–53](internal/install/installer_test.go#L32) | Second install replaces content | +| `TestInstallEmpty` | [55–60](internal/install/installer_test.go#L55) | Empty skills map is no-op | +| `TestInstallRejectsTraversalSkillName` | [62–76](internal/install/installer_test.go#L62) | `../../etc/malicious` → error | +| `TestInstallRejectsTraversalFilePath` | [78–92](internal/install/installer_test.go#L78) | File path traversal → error | +| `TestInstallAllowsNormalSkillNames` | [94–108](internal/install/installer_test.go#L94) | Normal names + nested files OK | +| `TestInstallRejectsDotSkillName` | [110–121](internal/install/installer_test.go#L110) | `.` skill name → error | +| `TestInstallRejectsEmptySkillName` | [123–134](internal/install/installer_test.go#L123) | Empty skill name → error | +| `TestInstallCleansUpStagingOnError` | [136–151](internal/install/installer_test.go#L136) | Staging dir cleaned on failure | +| `TestInstallAtomicOverwrite` | [153–177](internal/install/installer_test.go#L153) | Overwrite is atomic (old → new) | +| `TestInstallCompositeKeys` | [179–210](internal/install/installer_test.go#L179) | Composite keys create nested dirs | + +### Test Patterns + +**Temp directories**: All tests use `t.TempDir()` [installer_test.go:11](internal/install/installer_test.go#L11): +```go +target := filepath.Join(t.TempDir(), "skills") +``` + +**Skills map construction**: Inline `map[string]map[string][]byte{ ... }` literals. + +**Assertions**: Standard `t.Fatalf`/`t.Errorf` with `os.ReadFile`, `os.Stat`, `os.ReadDir`, `strings.Contains`. + +**No test helpers**: Each test is self-contained with inline setup. No shared helper functions. + +**No subtests**: All top-level test functions, no `t.Run()` subtests. + +### Key Test for Reference: `TestInstallCompositeKeys` + +[installer_test.go:179–210](internal/install/installer_test.go#L179) — Tests that composite keys like `github.com/org/repo/my-skill` create nested directory structures. This test should remain passing unchanged after adding `InstallFlat()`. + +--- + +## Summary: Required Changes by File + +| File | Change | Lines Affected | +|------|--------|----------------| +| `internal/install/installer.go` | Add `FlatKey()` + `InstallFlat()` | New code after line 88 | +| `internal/cli/install.go` | Global: `Install()` → `InstallFlat()` | [124](internal/cli/install.go#L124) | +| `internal/cli/get.go` | `Install()` → `InstallFlat()` | [241](internal/cli/get.go#L241) | +| `internal/cli/update.go` | Global: `Install()` → `InstallFlat()` | [227](internal/cli/update.go#L227) | +| `internal/cli/remove.go` | Global: flat path + skip `cleanEmptyParents` | [141](internal/cli/remove.go#L141), [161](internal/cli/remove.go#L161) | +| `internal/cli/list.go` | Detailed: prefix skills with composite key | [87](internal/cli/list.go#L87) | +| `internal/cli/tree.go` | Prefix skills with package identity | [60–65](internal/ui/tree.go#L60) | +| `internal/install/installer_test.go` | Add `TestFlatKey*`, `TestInstallFlat*` tests | New code after line 210 | From b27ba16e9766942a271b485b2e7b021fd57d742f Mon Sep 17 00:00:00 2001 From: Erdem Tuna Date: Mon, 9 Mar 2026 20:59:03 +0000 Subject: [PATCH 04/10] Add implementation plan for flat install layout Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../flat-install-layout/ImplementationPlan.md | 181 ++++++++++++++++++ 1 file changed, 181 insertions(+) create mode 100644 .paw/work/flat-install-layout/ImplementationPlan.md diff --git a/.paw/work/flat-install-layout/ImplementationPlan.md b/.paw/work/flat-install-layout/ImplementationPlan.md new file mode 100644 index 0000000..380b71c --- /dev/null +++ b/.paw/work/flat-install-layout/ImplementationPlan.md @@ -0,0 +1,181 @@ +# Flat Install Layout Implementation Plan + +## Overview + +Introduce flat directory naming for globally installed skills so AI agents can discover them. The composite key `github.com/org/repo/skill` becomes the flat directory name `github-com--org--repo--skill`. Project-scoped installs (`forge/`) remain nested. Display commands (`list -g`, `tree -g`) show original composite key format for readability. + +## Current State Analysis + +- `Install()` at [internal/install/installer.go:17](internal/install/installer.go#L17) takes composite keys and creates nested directories via `filepath.Join(target, skillName)`. All atomic staging, path traversal, and overwrite logic lives here. +- Global install calls at [internal/cli/install.go:124](internal/cli/install.go#L124), [internal/cli/get.go:241](internal/cli/get.go#L241), and [internal/cli/update.go:227](internal/cli/update.go#L227) all call `installlib.Install()`. +- Remove at [internal/cli/remove.go:141](internal/cli/remove.go#L141) constructs nested path `filepath.Join(tp, nsPrefix, skillName)` and calls `cleanEmptyParents` at line 161. +- Display: `list.go` detailed mode shows leaf skill names at line 87; `tree.go` via `ui/tree.go:65` shows leaf names. Neither distinguishes global vs project scope for display format. +- 11 existing tests in `installer_test.go`, all self-contained with `t.TempDir()`. + +## Desired End State + +- Global installs produce flat directories as immediate children of the agent skills root +- Project installs produce nested directories under `forge/` (unchanged) +- Global removes delete flat directories without parent cleanup +- `list -g --detailed` and `tree -g` show composite key format for skills +- All existing tests pass; new tests cover `FlatKey()` and `InstallFlat()` +- Documentation updated with flat layout examples + +## What We're NOT Doing + +- Changing `Install()` function behavior +- Migrating pre-existing global installs (clean-slate) +- Changing pinfile format or resolution logic +- Modifying project-scoped install/remove/display behavior +- Adding reverse `FlatKey()` → composite key conversion (pinfile is source of truth) + +## Phase Status +- [ ] **Phase 1: Core installer** - Add `FlatKey()` + `InstallFlat()` with tests +- [ ] **Phase 2: CLI wiring** - Route global install/get/update through `InstallFlat()` +- [ ] **Phase 3: Remove cleanup** - Use `FlatKey()` for global removes +- [ ] **Phase 4: Display commands** - Show composite keys in `list -g`/`tree -g` +- [ ] **Phase 5: Documentation** - Update README.md, E2E doc, and Docs.md + +## Phase Candidates + + +--- + +## Phase 1: Core Installer + +Add `FlatKey()` and `InstallFlat()` to the install package, plus comprehensive tests. + +### Changes Required + +- **`internal/install/installer.go`**: Add two exported functions after the existing `Install()` (after line 88): + - `FlatKey(compositeKey string) string` — Pure function. Replace `.` → `-`, then `/` → `--`. Return the result. Casing preserved. + - `InstallFlat(target string, skills map[string]map[string][]byte) error` — Build a new skill map with `FlatKey(k)` as key for each entry `k`, then delegate to `Install()`. This reuses all staging, validation, and atomicity logic. + +- **`internal/install/installer_test.go`**: Add new tests after existing tests (after line 210): + - `TestFlatKey` — Table-driven: standard key, dots in all segments, dashes in skill name, mixed casing, single-segment edge + - `TestFlatKeyEdgeCases` — Dots in non-host segments, keys with many components, empty-ish edge behavior + - `TestInstallFlatCreatesStructure` — Basic flat install; verify directories are direct children of target + - `TestInstallFlatMultiPackage` — Two skills from different repos; verify both flat, no nesting + - `TestInstallFlatSameName` — Two skills with same leaf name from different repos produce different flat directories (collision safety) + - `TestInstallFlatOverwrites` — Second flat install replaces content atomically + +### Success Criteria + +#### Automated Verification: +- [ ] Tests pass: `go test ./internal/install/ -v -run "TestFlatKey|TestInstallFlat"` +- [ ] Full suite: `go test ./...` +- [ ] Vet: `go vet ./...` + +#### Manual Verification: +- [ ] `FlatKey("github.com/org/repo/skill")` returns `"github-com--org--repo--skill"` +- [ ] `InstallFlat()` creates flat directories (not nested) in target + +--- + +## Phase 2: CLI Wiring + +Route global installs through `InstallFlat()` in all three commands. + +### Changes Required + +- **`internal/cli/install.go`**: At [line 124](internal/cli/install.go#L124), change `installlib.Install(targetPath, skillFiles)` to `installlib.InstallFlat(targetPath, skillFiles)`. Project path at [line 223](internal/cli/install.go#L223) remains `Install()`. + +- **`internal/cli/get.go`**: At [line 241](internal/cli/get.go#L241), change `installlib.Install(targetPath, skillFiles)` to `installlib.InstallFlat(targetPath, skillFiles)`. `get` is always global scope. + +- **`internal/cli/update.go`**: At [line 227](internal/cli/update.go#L227), change `installlib.Install(targetPath, skillFiles)` to `installlib.InstallFlat(targetPath, skillFiles)`. Project path at [line 251](internal/cli/update.go#L251) remains `Install()`. + +### Success Criteria + +#### Automated Verification: +- [ ] Build: `go build ./...` +- [ ] Vet: `go vet ./...` +- [ ] Tests: `go test ./...` + +#### Manual Verification: +- [ ] `craft get github.com/anthropics/courses@branch:main --target /tmp/test-skills` creates flat directories under `/tmp/test-skills/` +- [ ] `craft install` in a project still creates nested `forge/` structure + +--- + +## Phase 3: Remove Cleanup + +Use `FlatKey()` for global removes, skip parent cleanup. + +### Changes Required + +- **`internal/cli/remove.go`**: + - Add import for `installlib "github.com/erdemtuna/craft/internal/install"` + - At [line 141](internal/cli/remove.go#L141): branch on `globalFlag`: + - Global: `skillDir := filepath.Join(tp, installlib.FlatKey(nsPrefix+"/"+skillName))` + - Project: keep existing `filepath.Join(tp, nsPrefix, skillName)` + - At [line 161](internal/cli/remove.go#L161): only call `cleanEmptyParents` for project scope (wrap in `if !globalFlag`) + +### Success Criteria + +#### Automated Verification: +- [ ] Build: `go build ./...` +- [ ] Vet: `go vet ./...` +- [ ] Tests: `go test ./...` + +#### Manual Verification: +- [ ] After `craft remove -g alias --target /tmp/test-skills`, flat skill directory is gone +- [ ] No empty parent directories left behind +- [ ] Project `craft remove alias` still cleans nested dirs + empty parents + +--- + +## Phase 4: Display Commands + +Show composite key format in `list -g --detailed` and `tree -g`. + +### Changes Required + +- **`internal/cli/list.go`**: At [line 87](internal/cli/list.go#L87), in the detailed display branch: when `globalFlag` is true, prefix each skill name with `d.url + "/"` to form composite keys. When project scope, display leaf names as-is. + +- **`internal/ui/tree.go`**: At [line 65](internal/ui/tree.go#L65), the `RenderTree` function currently shows raw skill names. Add a `ShowCompositeKeys bool` field to `DepNode` struct (or add a parameter). When true, prefix each skill with the package identity from `dep.URL` (extracted before the `@` version). Wire this from `tree.go` CLI command based on `globalFlag`. + + Alternative simpler approach: the CLI command in `internal/cli/tree.go` can transform `dep.Skills` to composite key format before passing to `RenderTree`, avoiding changes to the ui package interface. + +### Success Criteria + +#### Automated Verification: +- [ ] Build: `go build ./...` +- [ ] Vet: `go vet ./...` +- [ ] Tests: `go test ./...` + +#### Manual Verification: +- [ ] `craft list -g --detailed` shows skills as `github.com/org/repo/skill-name` +- [ ] `craft tree -g` shows composite key skill names under each dep +- [ ] `craft list --detailed` (project) shows leaf skill names as before +- [ ] `craft tree` (project) shows leaf skill names as before + +--- + +## Phase 5: Documentation + +Update documentation to reflect flat install layout for global installs. + +### Changes Required + +- **`.paw/work/flat-install-layout/Docs.md`**: Technical reference covering `FlatKey()` format, `InstallFlat()` delegation pattern, CLI routing, and remove cleanup. Load `paw-docs-guidance` for template. + +- **`README.md`**: Update the global install directory structure examples to show flat layout. Update agent auto-detection table if it shows nested paths. Keep project/forge examples unchanged. + +- **`E2E_REAL_WORLD_TEST.md`**: Update Part 9 (Consumer/global install) expected directory structures from nested to flat format. Update any verification steps that check for nested global directories. + +### Success Criteria + +#### Automated Verification: +- [ ] No build/test regressions: `go test ./...` + +#### Manual Verification: +- [ ] README global install example shows flat directory names +- [ ] E2E doc Part 9 reflects flat layout expectations +- [ ] Docs.md captures implementation details accurately + +--- + +## References +- Spec: `.paw/work/flat-install-layout/Spec.md` +- Research: `.paw/work/flat-install-layout/CodeResearch.md` +- WorkShaping: `.paw/work/flat-install-layout/WorkShaping.md` From a02355ea7d6a993e2d240bcfad916ac261e35398 Mon Sep 17 00:00:00 2001 From: Erdem Tuna Date: Mon, 9 Mar 2026 21:05:09 +0000 Subject: [PATCH 05/10] Implement flat install layout for agent directories Add FlatKey() and InstallFlat() to the install package. FlatKey converts composite keys (github.com/org/repo/skill) to flat directory names (github-com--org--repo--skill) by replacing / with -- and . with -. Wire InstallFlat() for global installs in install, get, and update commands. Project installs continue using nested layout via Install(). Update remove to use FlatKey() for global cleanup and skip parent directory cleanup (flat layout has no parents to clean). Update list --detailed and tree to show composite key format for globally installed skills. Add 6 new tests covering FlatKey edge cases, flat install structure, multi-package, same-name collision safety, and overwrite semantics. Update README.md and E2E_REAL_WORLD_TEST.md with flat layout documentation and expected directory structures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- README.md | 15 ++- internal/cli/get.go | 2 +- internal/cli/install.go | 2 +- internal/cli/list.go | 9 +- internal/cli/remove.go | 12 ++- internal/cli/tree.go | 10 +- internal/cli/update.go | 2 +- internal/install/installer.go | 23 ++++ internal/install/installer_test.go | 164 +++++++++++++++++++++++++++++ 9 files changed, 230 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 2da99d9..a47d00f 100644 --- a/README.md +++ b/README.md @@ -363,8 +363,19 @@ craft auto-detects your AI agent and installs skills to the correct directory: When both agents are detected, craft prompts you to choose. Use `--target ` to override auto-detection. -**Global installs** (`craft get`, `craft install -g`) write to agent skill directories. -**Project installs** (`craft install`) vendor to `forge/` in the project root (gitignored). +**Global installs** (`craft get`, `craft install -g`) write to agent skill directories using a flat naming scheme so agents can discover skills. Each skill becomes a direct child of the skills root: + +``` +~/.claude/skills/ +├── github-com--acme--company-standards--coding-style/ +│ └── SKILL.md +└── github-com--acme--company-standards--review-checklist/ + └── SKILL.md +``` + +The flat directory name is derived from the composite key: slashes become `--`, dots become `-` (e.g., `github.com/acme/company-standards/coding-style` → `github-com--acme--company-standards--coding-style`). + +**Project installs** (`craft install`) vendor to `forge/` in the project root (gitignored), using nested composite-key paths. ## Known Limitations diff --git a/internal/cli/get.go b/internal/cli/get.go index 4d73aa4..5cc6626 100644 --- a/internal/cli/get.go +++ b/internal/cli/get.go @@ -238,7 +238,7 @@ func runGet(cmd *cobra.Command, args []string) error { // Install to agent directories progress.Update("Installing skills...") for _, targetPath := range targetPaths { - if err := installlib.Install(targetPath, skillFiles); err != nil { + if err := installlib.InstallFlat(targetPath, skillFiles); err != nil { progress.Fail("Installation failed") return fmt.Errorf("installation failed: %w\n note: dependencies were added to the global manifest but installation could not complete\n hint: run 'craft install -g' to retry, or 'craft remove -g ' to undo", err) } diff --git a/internal/cli/install.go b/internal/cli/install.go index 595a8c4..4f80404 100644 --- a/internal/cli/install.go +++ b/internal/cli/install.go @@ -121,7 +121,7 @@ func runInstallGlobal(cmd *cobra.Command) error { progress.Update("Installing skills...") for _, targetPath := range targetPaths { - if err := installlib.Install(targetPath, skillFiles); err != nil { + if err := installlib.InstallFlat(targetPath, skillFiles); err != nil { progress.Fail("Installation failed") return fmt.Errorf("installation failed: %w", err) } diff --git a/internal/cli/list.go b/internal/cli/list.go index a5843e6..b06b241 100644 --- a/internal/cli/list.go +++ b/internal/cli/list.go @@ -84,7 +84,14 @@ func runList(cmd *cobra.Command, args []string) error { for _, d := range deps { cmd.Printf("%s %s %s\n", sanitize(d.alias), d.version, sanitize(d.url)) if len(d.skills) > 0 { - cmd.Printf(" skills: %s\n", sanitize(strings.Join(d.skills, ", "))) + displaySkills := d.skills + if globalFlag { + displaySkills = make([]string, len(d.skills)) + for i, s := range d.skills { + displaySkills[i] = d.url + "/" + s + } + } + cmd.Printf(" skills: %s\n", sanitize(strings.Join(displaySkills, ", "))) } else { cmd.Printf(" skills: (none)\n") } diff --git a/internal/cli/remove.go b/internal/cli/remove.go index 2df197a..fab700f 100644 --- a/internal/cli/remove.go +++ b/internal/cli/remove.go @@ -8,6 +8,7 @@ import ( "sort" "strings" + installlib "github.com/erdemtuna/craft/internal/install" "github.com/erdemtuna/craft/internal/manifest" "github.com/erdemtuna/craft/internal/pinfile" "github.com/erdemtuna/craft/internal/resolve" @@ -138,7 +139,12 @@ func runRemove(cmd *cobra.Command, args []string) error { for _, skillName := range orphaned { removedFromAny := false for _, tp := range targetPath { - skillDir := filepath.Join(tp, nsPrefix, skillName) + var skillDir string + if globalFlag { + skillDir = filepath.Join(tp, installlib.FlatKey(nsPrefix+"/"+skillName)) + } else { + skillDir = filepath.Join(tp, nsPrefix, skillName) + } // Path traversal protection absSkillDir, err := filepath.Abs(skillDir) if err != nil { @@ -158,7 +164,9 @@ func runRemove(cmd *cobra.Command, args []string) error { cmd.PrintErrf(" warning: could not remove %s: %v\n", skillDir, err) } else { removedFromAny = true - cleanEmptyParents(tp, filepath.Dir(skillDir)) + if !globalFlag { + cleanEmptyParents(tp, filepath.Dir(skillDir)) + } } } } diff --git a/internal/cli/tree.go b/internal/cli/tree.go index 48da460..c66190c 100644 --- a/internal/cli/tree.go +++ b/internal/cli/tree.go @@ -57,10 +57,18 @@ func runTree(cmd *cobra.Command, args []string) error { alias = parsed.Repo } + skills := entry.Skills + if globalFlag { + skills = make([]string, len(entry.Skills)) + for i, s := range entry.Skills { + skills[i] = parsed.PackageIdentity() + "/" + s + } + } + deps = append(deps, ui.DepNode{ Alias: alias, URL: key, - Skills: entry.Skills, + Skills: skills, }) } diff --git a/internal/cli/update.go b/internal/cli/update.go index 0f29248..5858de7 100644 --- a/internal/cli/update.go +++ b/internal/cli/update.go @@ -224,7 +224,7 @@ func runUpdate(cmd *cobra.Command, args []string) error { } for _, targetPath := range targetPaths { - if err := installlib.Install(targetPath, skillFiles); err != nil { + if err := installlib.InstallFlat(targetPath, skillFiles); err != nil { progress.Fail("Installation failed") return fmt.Errorf("installation failed: %w", err) } diff --git a/internal/install/installer.go b/internal/install/installer.go index ef89969..92f55e0 100644 --- a/internal/install/installer.go +++ b/internal/install/installer.go @@ -86,3 +86,26 @@ func Install(target string, skills map[string]map[string][]byte) error { return nil } + +// FlatKey converts a composite skill key (host/owner/repo/skill) into a flat +// directory name suitable for agent skill discovery. Slashes become "--" and +// dots become "-". Casing is preserved. +// +// Example: "github.com/org/repo/my-skill" → "github-com--org--repo--my-skill" +func FlatKey(compositeKey string) string { + flat := strings.ReplaceAll(compositeKey, ".", "-") + flat = strings.ReplaceAll(flat, "/", "--") + return flat +} + +// InstallFlat installs skills using flat directory names so that each skill +// is a direct child of the target directory. This is used for global installs +// where AI agents need to discover skills by scanning immediate children. +// It transforms composite keys via FlatKey then delegates to Install. +func InstallFlat(target string, skills map[string]map[string][]byte) error { + flat := make(map[string]map[string][]byte, len(skills)) + for k, v := range skills { + flat[FlatKey(k)] = v + } + return Install(target, flat) +} diff --git a/internal/install/installer_test.go b/internal/install/installer_test.go index a026db6..afe162f 100644 --- a/internal/install/installer_test.go +++ b/internal/install/installer_test.go @@ -208,3 +208,167 @@ func TestInstallCompositeKeys(t *testing.T) { t.Errorf("Unexpected content for other/tools: %q", content2) } } + +func TestFlatKey(t *testing.T) { + tests := []struct { + input string + want string + }{ + {"github.com/org/repo/my-skill", "github-com--org--repo--my-skill"}, + {"github.com/lossyrob/phased-agent-workflow/paw-implement", "github-com--lossyrob--phased-agent-workflow--paw-implement"}, + {"github.com/anthropics/skills/skill-creator", "github-com--anthropics--skills--skill-creator"}, + {"simple-skill", "simple-skill"}, + {"host.name/owner/repo/skill", "host-name--owner--repo--skill"}, + } + for _, tt := range tests { + got := FlatKey(tt.input) + if got != tt.want { + t.Errorf("FlatKey(%q) = %q, want %q", tt.input, got, tt.want) + } + } +} + +func TestFlatKeyEdgeCases(t *testing.T) { + tests := []struct { + name string + input string + want string + }{ + {"dots in org", "github.com/my.org/repo/skill", "github-com--my-org--repo--skill"}, + {"dots in repo", "github.com/org/my.repo/skill", "github-com--org--my-repo--skill"}, + {"dots in skill", "github.com/org/repo/my.skill", "github-com--org--repo--my-skill"}, + {"dots everywhere", "git.hub.com/my.org/my.repo/my.skill", "git-hub-com--my-org--my-repo--my-skill"}, + {"mixed casing", "GitHub.com/MyOrg/Repo/Skill", "GitHub-com--MyOrg--Repo--Skill"}, + {"many components", "a/b/c/d/e", "a--b--c--d--e"}, + {"no slashes no dots", "plain-name", "plain-name"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := FlatKey(tt.input) + if got != tt.want { + t.Errorf("FlatKey(%q) = %q, want %q", tt.input, got, tt.want) + } + }) + } +} + +func TestInstallFlatCreatesStructure(t *testing.T) { + target := filepath.Join(t.TempDir(), "skills") + skills := map[string]map[string][]byte{ + "github.com/org/repo/my-skill": { + "SKILL.md": []byte("flat skill"), + }, + } + + if err := InstallFlat(target, skills); err != nil { + t.Fatalf("InstallFlat error: %v", err) + } + + // Should be flat, not nested + flatDir := filepath.Join(target, "github-com--org--repo--my-skill") + content, err := os.ReadFile(filepath.Join(flatDir, "SKILL.md")) + if err != nil { + t.Fatalf("ReadFile error: %v", err) + } + if string(content) != "flat skill" { + t.Errorf("Unexpected content: %q", content) + } + + // Nested path should NOT exist + nestedDir := filepath.Join(target, "github.com", "org", "repo", "my-skill") + if _, err := os.Stat(nestedDir); err == nil { + t.Error("nested directory should not exist after InstallFlat") + } +} + +func TestInstallFlatMultiPackage(t *testing.T) { + target := filepath.Join(t.TempDir(), "skills") + skills := map[string]map[string][]byte{ + "github.com/org/repo/skill-a": { + "SKILL.md": []byte("a"), + }, + "github.com/other/tools/skill-b": { + "SKILL.md": []byte("b"), + }, + } + + if err := InstallFlat(target, skills); err != nil { + t.Fatalf("InstallFlat error: %v", err) + } + + // Both should be direct children of target + entries, err := os.ReadDir(target) + if err != nil { + t.Fatalf("ReadDir error: %v", err) + } + if len(entries) != 2 { + t.Fatalf("expected 2 entries, got %d", len(entries)) + } + + names := make(map[string]bool) + for _, e := range entries { + names[e.Name()] = true + } + if !names["github-com--org--repo--skill-a"] { + t.Error("missing github-com--org--repo--skill-a") + } + if !names["github-com--other--tools--skill-b"] { + t.Error("missing github-com--other--tools--skill-b") + } +} + +func TestInstallFlatSameName(t *testing.T) { + target := filepath.Join(t.TempDir(), "skills") + skills := map[string]map[string][]byte{ + "github.com/org/repo/my-skill": { + "SKILL.md": []byte("from org"), + }, + "github.com/other/tools/my-skill": { + "SKILL.md": []byte("from other"), + }, + } + + if err := InstallFlat(target, skills); err != nil { + t.Fatalf("InstallFlat error: %v", err) + } + + // Same leaf name, different flat keys — no collision + c1, err := os.ReadFile(filepath.Join(target, "github-com--org--repo--my-skill", "SKILL.md")) + if err != nil { + t.Fatalf("ReadFile org: %v", err) + } + if string(c1) != "from org" { + t.Errorf("org content: %q", c1) + } + + c2, err := os.ReadFile(filepath.Join(target, "github-com--other--tools--my-skill", "SKILL.md")) + if err != nil { + t.Fatalf("ReadFile other: %v", err) + } + if string(c2) != "from other" { + t.Errorf("other content: %q", c2) + } +} + +func TestInstallFlatOverwrites(t *testing.T) { + target := filepath.Join(t.TempDir(), "skills") + + skills1 := map[string]map[string][]byte{ + "github.com/org/repo/my-skill": {"SKILL.md": []byte("v1")}, + } + if err := InstallFlat(target, skills1); err != nil { + t.Fatalf("first InstallFlat: %v", err) + } + + skills2 := map[string]map[string][]byte{ + "github.com/org/repo/my-skill": {"SKILL.md": []byte("v2")}, + } + if err := InstallFlat(target, skills2); err != nil { + t.Fatalf("second InstallFlat: %v", err) + } + + content, _ := os.ReadFile(filepath.Join(target, "github-com--org--repo--my-skill", "SKILL.md")) + if string(content) != "v2" { + t.Errorf("expected v2, got %q", content) + } +} From e0c1631c0537d90fe26c582ea50b4a0d6f4427a0 Mon Sep 17 00:00:00 2001 From: Erdem Tuna Date: Tue, 10 Mar 2026 05:56:12 +0000 Subject: [PATCH 06/10] Stop tracking PAW artifacts for flat-install-layout Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .paw/work/flat-install-layout/CodeResearch.md | 502 ------------------ .../flat-install-layout/ImplementationPlan.md | 181 ------- .paw/work/flat-install-layout/Spec.md | 151 ------ .paw/work/flat-install-layout/WorkShaping.md | 178 ------- .../flat-install-layout/WorkflowContext.md | 30 -- 5 files changed, 1042 deletions(-) delete mode 100644 .paw/work/flat-install-layout/CodeResearch.md delete mode 100644 .paw/work/flat-install-layout/ImplementationPlan.md delete mode 100644 .paw/work/flat-install-layout/Spec.md delete mode 100644 .paw/work/flat-install-layout/WorkShaping.md delete mode 100644 .paw/work/flat-install-layout/WorkflowContext.md diff --git a/.paw/work/flat-install-layout/CodeResearch.md b/.paw/work/flat-install-layout/CodeResearch.md deleted file mode 100644 index 0df950a..0000000 --- a/.paw/work/flat-install-layout/CodeResearch.md +++ /dev/null @@ -1,502 +0,0 @@ -# Code Research: Flat Install Layout - -## 1. installer.go — `Install()` Function - -**File**: `internal/install/installer.go` - -### Signature -[installer.go:17](internal/install/installer.go#L17) -```go -func Install(target string, skills map[string]map[string][]byte) error -``` - -- `target`: root directory to install into (e.g., `~/.claude/skills/` or `forge/`) -- `skills`: map of composite key → (relative file path → content) - -### Full Function Body (lines 17–88) - -**Target directory creation and resolution** [installer.go:18–25](internal/install/installer.go#L18): -```go -if err := os.MkdirAll(target, 0o700); err != nil { - return fmt.Errorf("creating target directory: %w", err) -} -absTarget, err := filepath.Abs(target) -if err != nil { - return fmt.Errorf("resolving target path: %w", err) -} -``` - -**Skill iteration loop** [installer.go:27](internal/install/installer.go#L27): -```go -for skillName, files := range skills { -``` -- `skillName` is the composite key (e.g., `github.com/org/repo/my-skill`) -- `files` is `map[string][]byte` of relative paths to content - -**Directory creation via `filepath.Join`** [installer.go:28](internal/install/installer.go#L28): -```go -skillDir := filepath.Join(target, skillName) -``` -- When `skillName` is `github.com/org/repo/my-skill`, this creates nested dirs automatically via OS path semantics. - -**Path traversal validation** [installer.go:29–35](internal/install/installer.go#L29): -```go -absSkillDir, err := filepath.Abs(skillDir) -if err != nil { - return fmt.Errorf("resolving skill path: %w", err) -} -if !strings.HasPrefix(absSkillDir, absTarget+string(filepath.Separator)) { - return fmt.Errorf("skill name %q escapes target directory", skillName) -} -``` - -**Staging directory** [installer.go:38–44](internal/install/installer.go#L38): -```go -stagingDir := skillDir + ".staging" -_ = os.RemoveAll(stagingDir) -if err := os.MkdirAll(stagingDir, 0o700); err != nil { - return fmt.Errorf("creating staging directory for %q: %w", skillName, err) -} -``` - -**File writing with per-file path traversal check** [installer.go:47–68](internal/install/installer.go#L47): -```go -writeErr := func() error { - for relPath, content := range files { - fullPath := filepath.Join(stagingDir, relPath) - absFullPath, err := filepath.Abs(fullPath) - // ...validate against staging dir... - if !strings.HasPrefix(absFullPath, filepath.Clean(stagingDir)+string(filepath.Separator)) { - return fmt.Errorf("file path %q escapes skill directory", relPath) - } - // ...MkdirAll + WriteFile... - } - return nil -}() -``` - -**Atomic swap** [installer.go:76–84](internal/install/installer.go#L76): -```go -if err := os.RemoveAll(skillDir); err != nil { ... } -if err := os.Rename(stagingDir, skillDir); err != nil { ... } -``` - -### Key Observations for `InstallFlat()` -- `Install()` already handles composite keys as `skillName` — it just passes them through `filepath.Join(target, skillName)` which creates nested directories. -- `InstallFlat()` can transform keys via `FlatKey()` then call `Install()` with modified map, OR duplicate the loop with flat path construction. -- The spec says `InstallFlat()` should delegate to `Install()` — transforming keys then passing through. This reuses all staging, validation, and atomicity logic. - ---- - -## 2. install.go — Global vs Project Install Calls - -**File**: `internal/cli/install.go` - -### Global Install Path - -**`runInstall` dispatches on `globalFlag`** [install.go:43–48](internal/cli/install.go#L43): -```go -func runInstall(cmd *cobra.Command, args []string) error { - if globalFlag { - return runInstallGlobal(cmd) - } - return runInstallProject(cmd) -} -``` - -**Target resolution for global** [install.go:106–109](internal/cli/install.go#L106): -```go -targetPaths, err := resolveInstallTargets(installTarget) -if err != nil { - return err -} -``` -- `targetPaths` is `[]string` — one or more agent install directories (e.g., `~/.claude/skills/`). -- Resolved by `resolveInstallTargets()` at [install.go:256](internal/cli/install.go#L256). - -**Skill files collection** [install.go:111–115](internal/cli/install.go#L111): -```go -skillFiles, err := collectSkillFiles(fetcher, result) -``` -- Returns `map[string]map[string][]byte` — composite key → files. - -**`Install()` call for global** [install.go:123–128](internal/cli/install.go#L123): -```go -for _, targetPath := range targetPaths { - if err := installlib.Install(targetPath, skillFiles); err != nil { - progress.Fail("Installation failed") - return fmt.Errorf("installation failed: %w", err) - } -} -``` -→ **This loop needs to change to call `installlib.InstallFlat(targetPath, skillFiles)` for global installs.** - -### Project Install Path - -**forge path** [install.go:206](internal/cli/install.go#L206): -```go -forgePath := filepath.Join(root, "forge") -``` - -**`Install()` call for project** [install.go:222–226](internal/cli/install.go#L222): -```go -if err := installlib.Install(forgePath, skillFiles); err != nil { - progress.Fail("Vendoring failed") - return fmt.Errorf("vendoring failed: %w", err) -} -``` -→ **This stays as `Install()` — project layout unchanged.** - -### `collectSkillFiles` — Composite Key Construction - -[install.go:351–397](internal/cli/install.go#L351) - -Composite key is built at [install.go:391](internal/cli/install.go#L391): -```go -compositeKey := prefix + "/" + skillName -skills[compositeKey] = files -``` -Where `prefix` = `parsed.PackageIdentity()` (e.g., `github.com/org/repo`) and `skillName` is the skill's leaf name (e.g., `my-skill`). Result: `github.com/org/repo/my-skill`. - -### Import Alias - -[install.go:14](internal/cli/install.go#L14): -```go -installlib "github.com/erdemtuna/craft/internal/install" -``` - ---- - -## 3. get.go — `Install()` Call - -**File**: `internal/cli/get.go` - -**Import** [get.go:10](internal/cli/get.go#L10): -```go -installlib "github.com/erdemtuna/craft/internal/install" -``` - -**Target resolution** [get.go:220](internal/cli/get.go#L220): -```go -targetPaths, err := resolveInstallTargets(getTarget) -``` - -**Skill files collection** [get.go:226](internal/cli/get.go#L226): -```go -skillFiles, err := collectSkillFiles(fetcher, result) -``` - -**`Install()` call** [get.go:240–245](internal/cli/get.go#L240): -```go -for _, targetPath := range targetPaths { - if err := installlib.Install(targetPath, skillFiles); err != nil { - progress.Fail("Installation failed") - return fmt.Errorf("installation failed: %w\n note: ...", err) - } -} -``` -→ **Needs to change to `installlib.InstallFlat()`** — `get` is always global scope. - ---- - -## 4. update.go — `Install()` Call - -**File**: `internal/cli/update.go` - -**Import** [update.go:12](internal/cli/update.go#L12): -```go -installlib "github.com/erdemtuna/craft/internal/install" -``` - -**Global branch** [update.go:214–231](internal/cli/update.go#L214): -```go -if globalFlag { - targetPaths, err := resolveInstallTargets(updateTarget) - // ... - for _, targetPath := range targetPaths { - if err := installlib.Install(targetPath, skillFiles); err != nil { - progress.Fail("Installation failed") - return fmt.Errorf("installation failed: %w", err) - } - } -``` -→ **`Install()` at [update.go:227](internal/cli/update.go#L227) needs to change to `InstallFlat()` for global scope.** - -**Project branch** [update.go:244–254](internal/cli/update.go#L244): -```go -forgePath := filepath.Join(root, "forge") -// ... -if err := installlib.Install(forgePath, skillFiles); err != nil { -``` -At [update.go:251](internal/cli/update.go#L251) — **stays as `Install()`**. - ---- - -## 5. remove.go — Deletion Logic - -**File**: `internal/cli/remove.go` - -### Scope Branching - -**Global vs project** [remove.go:37–53](internal/cli/remove.go#L37): -```go -if globalFlag { - manifestPath, err = GlobalManifestPath() - // ... - pfPath, err = GlobalPinfilePath() -} else { - root, err = os.Getwd() - // ... - manifestPath = filepath.Join(root, "craft.yaml") - pfPath = filepath.Join(root, "craft.pin.yaml") -} -``` - -### Skill Name Source - -**Removed skill names from pinfile** [remove.go:78–82](internal/cli/remove.go#L78): -```go -if pfErr == nil { - if entry, ok := pf.Resolved[depURL]; ok { - removedSkills = entry.Skills - } -} -``` -- `removedSkills` contains leaf skill names (e.g., `["my-skill", "other-skill"]`), NOT composite keys. - -### Target Resolution for Cleanup - -[remove.go:111–121](internal/cli/remove.go#L111): -```go -if globalFlag { - targetPath, err = resolveInstallTargets(removeTarget) -} else if removeTarget != "" { - targetPath = []string{removeTarget} -} else { - targetPath = []string{filepath.Join(root, "forge")} -} -``` - -### Path Construction for Deletion - -**Namespace prefix** [remove.go:129–135](internal/cli/remove.go#L129): -```go -parsed, parseErr := resolve.ParseDepURL(depURL) -// ... -nsPrefix := parsed.PackageIdentity() -``` -- `nsPrefix` = `github.com/org/repo` - -**Skill directory path** [remove.go:141](internal/cli/remove.go#L141): -```go -skillDir := filepath.Join(tp, nsPrefix, skillName) -``` -- Produces nested path: `~/.claude/skills/github.com/org/repo/my-skill` -→ **For global scope, this needs to use `FlatKey(nsPrefix + "/" + skillName)` instead, producing `~/.claude/skills/github-com--org--repo--my-skill`.** - -### Path Traversal Protection - -[remove.go:143–154](internal/cli/remove.go#L143): -```go -absSkillDir, err := filepath.Abs(skillDir) -// ... -absTarget, err := filepath.Abs(tp) -// ... -if !strings.HasPrefix(absSkillDir, absTarget+string(filepath.Separator)) { - cmd.PrintErrf(" warning: skill name %q escapes target directory, skipping\n", skillName) - continue -} -``` - -### Deletion + Parent Cleanup - -[remove.go:156–162](internal/cli/remove.go#L156): -```go -if _, err := os.Stat(skillDir); err == nil { - if err := os.RemoveAll(skillDir); err != nil { - cmd.PrintErrf(" warning: could not remove %s: %v\n", skillDir, err) - } else { - removedFromAny = true - cleanEmptyParents(tp, filepath.Dir(skillDir)) - } -} -``` -→ **For global flat layout, `cleanEmptyParents` should be skipped (no nested parents exist).** - -### `cleanEmptyParents` Function - -[remove.go:181–196](internal/cli/remove.go#L181): -```go -func cleanEmptyParents(root, dir string) { - absRoot, err := filepath.Abs(root) - if err != nil { - return - } - for { - absDir, err := filepath.Abs(dir) - if err != nil || absDir == absRoot || !strings.HasPrefix(absDir, absRoot+string(filepath.Separator)) { - break - } - if err := os.Remove(dir); err != nil { - break // not empty or permission error - } - dir = filepath.Dir(dir) - } -} -``` -- Walks up from `dir` to `root`, removing empty dirs. Safe — `os.Remove` fails on non-empty. -- For flat layout, `filepath.Dir(flatSkillDir)` == `target`, so `cleanEmptyParents` would be a no-op anyway. But spec says to skip the call entirely for clarity. - ---- - -## 6. list.go / tree.go — Skill Name Display - -### list.go - -**File**: `internal/cli/list.go` - -**Data source**: pinfile entries [list.go:59–77](internal/cli/list.go#L59): -```go -for key, entry := range pf.Resolved { - parsed, err := resolve.ParseDepURL(key) - // ... - alias := urlToAlias[parsed.PackageIdentity()] - if alias == "" { - alias = parsed.Repo - } - deps = append(deps, depInfo{ - alias: alias, - version: parsed.RefString(), - url: parsed.PackageIdentity(), - skills: entry.Skills, - }) -} -``` - -**Display — default (tabular)** [list.go:94–101](internal/cli/list.go#L94): -```go -_, _ = fmt.Fprintf(w, "%s\t%s\t(%d %s)\n", sanitize(d.alias), d.version, len(d.skills), skillWord) -``` -- Shows alias, version, skill count. Does NOT show individual skill names in default mode. - -**Display — detailed** [list.go:84–92](internal/cli/list.go#L84): -```go -cmd.Printf("%s %s %s\n", sanitize(d.alias), d.version, sanitize(d.url)) -if len(d.skills) > 0 { - cmd.Printf(" skills: %s\n", sanitize(strings.Join(d.skills, ", "))) -} -``` -- `d.url` = `parsed.PackageIdentity()` = `github.com/org/repo` -- `d.skills` = `entry.Skills` = leaf skill names from pinfile (e.g., `["my-skill"]`) -- **Skill names displayed are leaf names from pinfile**, not composite keys and not flat keys. - -### tree.go - -**File**: `internal/cli/tree.go` - -**Data source**: pinfile entries [tree.go:47–65](internal/cli/tree.go#L47): -```go -for key, entry := range pf.Resolved { - parsed, err := resolve.ParseDepURL(key) - // ... - alias := urlToAlias[parsed.PackageIdentity()] - if alias == "" { - alias = parsed.Repo - } - deps = append(deps, ui.DepNode{ - Alias: alias, - URL: key, - Skills: entry.Skills, - }) -} -``` - -**Rendering** via `ui.RenderTree` at [tree.go:67](internal/cli/tree.go#L67): -```go -ui.RenderTree(cmd.OutOrStdout(), packageName, localSkills, deps) -``` - -### ui/tree.go — RenderTree - -**File**: `internal/ui/tree.go` - -**Dep node display** [tree.go:58](internal/ui/tree.go#L58): -```go -_, _ = fmt.Fprintf(w, "%s%s (%s)\n", connector, dep.Alias, dep.URL) -``` -- Shows alias and full dep URL (e.g., `github.com/org/repo@v1.0.0`) - -**Skill display** [tree.go:60–65](internal/ui/tree.go#L60): -```go -for j, skill := range dep.Skills { - // ... - _, _ = fmt.Fprintf(w, "%s%s%s\n", childPrefix, skillConn, skill) -} -``` -- `skill` = leaf name from pinfile (e.g., `my-skill`). - -### Key Observation for Spec FR-007 - -The spec says `list -g` and `tree -g` should display skills using **composite key format** (`github.com/org/repo/skill`). Currently: - -- **list.go default**: shows alias + version + skill count — no change needed (no skill names displayed). -- **list.go --detailed**: shows `d.url` (= `PackageIdentity()` = `github.com/org/repo`) on the header line, then leaf skill names. The spec wants composite key format for skills → change `d.skills` display to prefix each with `d.url + "/"`. -- **tree.go**: shows leaf skill names under each dep node. The spec wants composite key format → prefix each skill with the package identity. - -Both commands get skill names from **pinfile** (`entry.Skills`), not from filesystem scanning. No filesystem scan is involved — this is purely a display concern. - ---- - -## 7. installer_test.go — Test Structure - -**File**: `internal/install/installer_test.go` - -### Test Inventory - -| Test | Lines | What it tests | -|------|-------|---------------| -| `TestInstallCreatesStructure` | [10–30](internal/install/installer_test.go#L10) | Basic install creates dir + files | -| `TestInstallOverwrites` | [32–53](internal/install/installer_test.go#L32) | Second install replaces content | -| `TestInstallEmpty` | [55–60](internal/install/installer_test.go#L55) | Empty skills map is no-op | -| `TestInstallRejectsTraversalSkillName` | [62–76](internal/install/installer_test.go#L62) | `../../etc/malicious` → error | -| `TestInstallRejectsTraversalFilePath` | [78–92](internal/install/installer_test.go#L78) | File path traversal → error | -| `TestInstallAllowsNormalSkillNames` | [94–108](internal/install/installer_test.go#L94) | Normal names + nested files OK | -| `TestInstallRejectsDotSkillName` | [110–121](internal/install/installer_test.go#L110) | `.` skill name → error | -| `TestInstallRejectsEmptySkillName` | [123–134](internal/install/installer_test.go#L123) | Empty skill name → error | -| `TestInstallCleansUpStagingOnError` | [136–151](internal/install/installer_test.go#L136) | Staging dir cleaned on failure | -| `TestInstallAtomicOverwrite` | [153–177](internal/install/installer_test.go#L153) | Overwrite is atomic (old → new) | -| `TestInstallCompositeKeys` | [179–210](internal/install/installer_test.go#L179) | Composite keys create nested dirs | - -### Test Patterns - -**Temp directories**: All tests use `t.TempDir()` [installer_test.go:11](internal/install/installer_test.go#L11): -```go -target := filepath.Join(t.TempDir(), "skills") -``` - -**Skills map construction**: Inline `map[string]map[string][]byte{ ... }` literals. - -**Assertions**: Standard `t.Fatalf`/`t.Errorf` with `os.ReadFile`, `os.Stat`, `os.ReadDir`, `strings.Contains`. - -**No test helpers**: Each test is self-contained with inline setup. No shared helper functions. - -**No subtests**: All top-level test functions, no `t.Run()` subtests. - -### Key Test for Reference: `TestInstallCompositeKeys` - -[installer_test.go:179–210](internal/install/installer_test.go#L179) — Tests that composite keys like `github.com/org/repo/my-skill` create nested directory structures. This test should remain passing unchanged after adding `InstallFlat()`. - ---- - -## Summary: Required Changes by File - -| File | Change | Lines Affected | -|------|--------|----------------| -| `internal/install/installer.go` | Add `FlatKey()` + `InstallFlat()` | New code after line 88 | -| `internal/cli/install.go` | Global: `Install()` → `InstallFlat()` | [124](internal/cli/install.go#L124) | -| `internal/cli/get.go` | `Install()` → `InstallFlat()` | [241](internal/cli/get.go#L241) | -| `internal/cli/update.go` | Global: `Install()` → `InstallFlat()` | [227](internal/cli/update.go#L227) | -| `internal/cli/remove.go` | Global: flat path + skip `cleanEmptyParents` | [141](internal/cli/remove.go#L141), [161](internal/cli/remove.go#L161) | -| `internal/cli/list.go` | Detailed: prefix skills with composite key | [87](internal/cli/list.go#L87) | -| `internal/cli/tree.go` | Prefix skills with package identity | [60–65](internal/ui/tree.go#L60) | -| `internal/install/installer_test.go` | Add `TestFlatKey*`, `TestInstallFlat*` tests | New code after line 210 | diff --git a/.paw/work/flat-install-layout/ImplementationPlan.md b/.paw/work/flat-install-layout/ImplementationPlan.md deleted file mode 100644 index 380b71c..0000000 --- a/.paw/work/flat-install-layout/ImplementationPlan.md +++ /dev/null @@ -1,181 +0,0 @@ -# Flat Install Layout Implementation Plan - -## Overview - -Introduce flat directory naming for globally installed skills so AI agents can discover them. The composite key `github.com/org/repo/skill` becomes the flat directory name `github-com--org--repo--skill`. Project-scoped installs (`forge/`) remain nested. Display commands (`list -g`, `tree -g`) show original composite key format for readability. - -## Current State Analysis - -- `Install()` at [internal/install/installer.go:17](internal/install/installer.go#L17) takes composite keys and creates nested directories via `filepath.Join(target, skillName)`. All atomic staging, path traversal, and overwrite logic lives here. -- Global install calls at [internal/cli/install.go:124](internal/cli/install.go#L124), [internal/cli/get.go:241](internal/cli/get.go#L241), and [internal/cli/update.go:227](internal/cli/update.go#L227) all call `installlib.Install()`. -- Remove at [internal/cli/remove.go:141](internal/cli/remove.go#L141) constructs nested path `filepath.Join(tp, nsPrefix, skillName)` and calls `cleanEmptyParents` at line 161. -- Display: `list.go` detailed mode shows leaf skill names at line 87; `tree.go` via `ui/tree.go:65` shows leaf names. Neither distinguishes global vs project scope for display format. -- 11 existing tests in `installer_test.go`, all self-contained with `t.TempDir()`. - -## Desired End State - -- Global installs produce flat directories as immediate children of the agent skills root -- Project installs produce nested directories under `forge/` (unchanged) -- Global removes delete flat directories without parent cleanup -- `list -g --detailed` and `tree -g` show composite key format for skills -- All existing tests pass; new tests cover `FlatKey()` and `InstallFlat()` -- Documentation updated with flat layout examples - -## What We're NOT Doing - -- Changing `Install()` function behavior -- Migrating pre-existing global installs (clean-slate) -- Changing pinfile format or resolution logic -- Modifying project-scoped install/remove/display behavior -- Adding reverse `FlatKey()` → composite key conversion (pinfile is source of truth) - -## Phase Status -- [ ] **Phase 1: Core installer** - Add `FlatKey()` + `InstallFlat()` with tests -- [ ] **Phase 2: CLI wiring** - Route global install/get/update through `InstallFlat()` -- [ ] **Phase 3: Remove cleanup** - Use `FlatKey()` for global removes -- [ ] **Phase 4: Display commands** - Show composite keys in `list -g`/`tree -g` -- [ ] **Phase 5: Documentation** - Update README.md, E2E doc, and Docs.md - -## Phase Candidates - - ---- - -## Phase 1: Core Installer - -Add `FlatKey()` and `InstallFlat()` to the install package, plus comprehensive tests. - -### Changes Required - -- **`internal/install/installer.go`**: Add two exported functions after the existing `Install()` (after line 88): - - `FlatKey(compositeKey string) string` — Pure function. Replace `.` → `-`, then `/` → `--`. Return the result. Casing preserved. - - `InstallFlat(target string, skills map[string]map[string][]byte) error` — Build a new skill map with `FlatKey(k)` as key for each entry `k`, then delegate to `Install()`. This reuses all staging, validation, and atomicity logic. - -- **`internal/install/installer_test.go`**: Add new tests after existing tests (after line 210): - - `TestFlatKey` — Table-driven: standard key, dots in all segments, dashes in skill name, mixed casing, single-segment edge - - `TestFlatKeyEdgeCases` — Dots in non-host segments, keys with many components, empty-ish edge behavior - - `TestInstallFlatCreatesStructure` — Basic flat install; verify directories are direct children of target - - `TestInstallFlatMultiPackage` — Two skills from different repos; verify both flat, no nesting - - `TestInstallFlatSameName` — Two skills with same leaf name from different repos produce different flat directories (collision safety) - - `TestInstallFlatOverwrites` — Second flat install replaces content atomically - -### Success Criteria - -#### Automated Verification: -- [ ] Tests pass: `go test ./internal/install/ -v -run "TestFlatKey|TestInstallFlat"` -- [ ] Full suite: `go test ./...` -- [ ] Vet: `go vet ./...` - -#### Manual Verification: -- [ ] `FlatKey("github.com/org/repo/skill")` returns `"github-com--org--repo--skill"` -- [ ] `InstallFlat()` creates flat directories (not nested) in target - ---- - -## Phase 2: CLI Wiring - -Route global installs through `InstallFlat()` in all three commands. - -### Changes Required - -- **`internal/cli/install.go`**: At [line 124](internal/cli/install.go#L124), change `installlib.Install(targetPath, skillFiles)` to `installlib.InstallFlat(targetPath, skillFiles)`. Project path at [line 223](internal/cli/install.go#L223) remains `Install()`. - -- **`internal/cli/get.go`**: At [line 241](internal/cli/get.go#L241), change `installlib.Install(targetPath, skillFiles)` to `installlib.InstallFlat(targetPath, skillFiles)`. `get` is always global scope. - -- **`internal/cli/update.go`**: At [line 227](internal/cli/update.go#L227), change `installlib.Install(targetPath, skillFiles)` to `installlib.InstallFlat(targetPath, skillFiles)`. Project path at [line 251](internal/cli/update.go#L251) remains `Install()`. - -### Success Criteria - -#### Automated Verification: -- [ ] Build: `go build ./...` -- [ ] Vet: `go vet ./...` -- [ ] Tests: `go test ./...` - -#### Manual Verification: -- [ ] `craft get github.com/anthropics/courses@branch:main --target /tmp/test-skills` creates flat directories under `/tmp/test-skills/` -- [ ] `craft install` in a project still creates nested `forge/` structure - ---- - -## Phase 3: Remove Cleanup - -Use `FlatKey()` for global removes, skip parent cleanup. - -### Changes Required - -- **`internal/cli/remove.go`**: - - Add import for `installlib "github.com/erdemtuna/craft/internal/install"` - - At [line 141](internal/cli/remove.go#L141): branch on `globalFlag`: - - Global: `skillDir := filepath.Join(tp, installlib.FlatKey(nsPrefix+"/"+skillName))` - - Project: keep existing `filepath.Join(tp, nsPrefix, skillName)` - - At [line 161](internal/cli/remove.go#L161): only call `cleanEmptyParents` for project scope (wrap in `if !globalFlag`) - -### Success Criteria - -#### Automated Verification: -- [ ] Build: `go build ./...` -- [ ] Vet: `go vet ./...` -- [ ] Tests: `go test ./...` - -#### Manual Verification: -- [ ] After `craft remove -g alias --target /tmp/test-skills`, flat skill directory is gone -- [ ] No empty parent directories left behind -- [ ] Project `craft remove alias` still cleans nested dirs + empty parents - ---- - -## Phase 4: Display Commands - -Show composite key format in `list -g --detailed` and `tree -g`. - -### Changes Required - -- **`internal/cli/list.go`**: At [line 87](internal/cli/list.go#L87), in the detailed display branch: when `globalFlag` is true, prefix each skill name with `d.url + "/"` to form composite keys. When project scope, display leaf names as-is. - -- **`internal/ui/tree.go`**: At [line 65](internal/ui/tree.go#L65), the `RenderTree` function currently shows raw skill names. Add a `ShowCompositeKeys bool` field to `DepNode` struct (or add a parameter). When true, prefix each skill with the package identity from `dep.URL` (extracted before the `@` version). Wire this from `tree.go` CLI command based on `globalFlag`. - - Alternative simpler approach: the CLI command in `internal/cli/tree.go` can transform `dep.Skills` to composite key format before passing to `RenderTree`, avoiding changes to the ui package interface. - -### Success Criteria - -#### Automated Verification: -- [ ] Build: `go build ./...` -- [ ] Vet: `go vet ./...` -- [ ] Tests: `go test ./...` - -#### Manual Verification: -- [ ] `craft list -g --detailed` shows skills as `github.com/org/repo/skill-name` -- [ ] `craft tree -g` shows composite key skill names under each dep -- [ ] `craft list --detailed` (project) shows leaf skill names as before -- [ ] `craft tree` (project) shows leaf skill names as before - ---- - -## Phase 5: Documentation - -Update documentation to reflect flat install layout for global installs. - -### Changes Required - -- **`.paw/work/flat-install-layout/Docs.md`**: Technical reference covering `FlatKey()` format, `InstallFlat()` delegation pattern, CLI routing, and remove cleanup. Load `paw-docs-guidance` for template. - -- **`README.md`**: Update the global install directory structure examples to show flat layout. Update agent auto-detection table if it shows nested paths. Keep project/forge examples unchanged. - -- **`E2E_REAL_WORLD_TEST.md`**: Update Part 9 (Consumer/global install) expected directory structures from nested to flat format. Update any verification steps that check for nested global directories. - -### Success Criteria - -#### Automated Verification: -- [ ] No build/test regressions: `go test ./...` - -#### Manual Verification: -- [ ] README global install example shows flat directory names -- [ ] E2E doc Part 9 reflects flat layout expectations -- [ ] Docs.md captures implementation details accurately - ---- - -## References -- Spec: `.paw/work/flat-install-layout/Spec.md` -- Research: `.paw/work/flat-install-layout/CodeResearch.md` -- WorkShaping: `.paw/work/flat-install-layout/WorkShaping.md` diff --git a/.paw/work/flat-install-layout/Spec.md b/.paw/work/flat-install-layout/Spec.md deleted file mode 100644 index 334ee65..0000000 --- a/.paw/work/flat-install-layout/Spec.md +++ /dev/null @@ -1,151 +0,0 @@ -# Feature Specification: Flat Install Layout for Agent Directories - -**Branch**: feature/flat-install-layout | **Created**: 2026-03-09 | **Status**: Draft -**Input Brief**: Flatten global skill install paths so AI agents can discover them - -## Overview - -When craft installs skills globally for AI agents like Claude Code or GitHub Copilot, it currently writes them into deeply nested directory trees mirroring the source repository path — for example, `~/.claude/skills/github.com/lossyrob/phased-agent-workflow/paw-implement/`. These agents expect skills to appear as immediate children of their skills directory. The nested layout makes globally installed skills invisible to agent discovery. - -This feature introduces a flat directory naming scheme for global installs. Each skill's composite key (host/owner/repo/skill) is transformed into a single flat directory name using `--` as a component separator and replacing dots with dashes — producing names like `github-com--lossyrob--phased-agent-workflow--paw-implement`. The flat format is unique, collision-safe, and places every skill as a direct child of the agent's skills root. - -Project-scoped installs (`forge/` directory) remain unchanged. The nested composite-key layout is well-suited for vendored dependencies where agent discovery is not a concern. This separation keeps the change surgical: global installs get flat layout, project installs keep nested layout. - -The `list` and `tree` commands for global scope will display skills using their original composite key format (e.g., `github.com/org/repo/skill`) rather than the flat directory name, preserving readability. - -## Objectives - -- Enable AI agents to discover globally installed skills by placing them as direct children of the skills root directory -- Prevent skill name collisions across different source repositories through a deterministic flat naming scheme -- Maintain the existing nested layout for project-scoped installs, preserving vendoring semantics -- Keep removal operations correct by deriving flat directory names from pinfile composite keys -- Display globally installed skills in `list`/`tree` output using readable composite key format - -## User Scenarios & Testing - -### User Story P1 – Global Skill Installation with Agent Discovery - -Narrative: A developer runs `craft get github.com/acme/tools@v1.0.0` to install a skill for their AI agent. The skill appears as a flat directory under the agent's skills root, and the agent can immediately discover and use it. - -Independent Test: After `craft get`, verify the skill exists at `~/.claude/skills/github-com--acme--tools--my-skill/SKILL.md` (not nested under `github.com/acme/tools/`). - -Acceptance Scenarios: -1. Given a user runs `craft get github.com/acme/tools@v1.0.0`, When the install completes, Then skills appear as flat directories directly under the agent's skills root -2. Given a user runs `craft install -g`, When the install completes, Then all global skills use flat directory names -3. Given a skill `github.com/org/repo/my-skill`, When installed globally, Then the directory name is `github-com--org--repo--my-skill` - -### User Story P2 – Global Skill Update Preserves Flat Layout - -Narrative: A developer updates a globally installed skill to a newer version. The updated files appear in the same flat directory without creating nested paths. - -Independent Test: After `craft update -g`, verify the skill directory is still flat and contains updated files. - -Acceptance Scenarios: -1. Given a globally installed skill at version v1.0.0, When the user runs `craft update -g`, Then the updated skill overwrites the same flat directory -2. Given multiple globally installed skills, When the user runs `craft update -g alias`, Then only the target skill is updated and all directories remain flat - -### User Story P3 – Global Skill Removal Cleans Flat Directory - -Narrative: A developer removes a globally installed skill. The flat directory is deleted completely, with no orphaned parent directories. - -Independent Test: After `craft remove -g alias`, verify the flat directory no longer exists and no empty parent directories were created. - -Acceptance Scenarios: -1. Given a globally installed skill, When the user runs `craft remove -g alias`, Then the flat skill directory is removed -2. Given a global removal, When cleanup completes, Then no empty intermediate directories remain (there are none to begin with in flat layout) - -### User Story P4 – Project Install Unchanged - -Narrative: A developer runs `craft install` in a project. Skills are vendored into `forge/` using the existing nested composite-key layout, unaffected by the flat layout change. - -Independent Test: After `craft install`, verify skills exist at `forge/github.com/org/repo/skill/` (nested, not flat). - -Acceptance Scenarios: -1. Given a project with dependencies, When the user runs `craft install`, Then skills appear under `forge/` in nested composite-key paths -2. Given `craft remove alias` in project scope, Then nested directories and their empty parents are cleaned up as before - -### User Story P5 – Global List and Tree Display - -Narrative: A developer runs `craft list -g` or `craft tree -g` to see their globally installed skills. Skills are displayed using the original composite key format for readability, not the flat directory name. - -Independent Test: After installing skills globally, `craft list -g` shows `github.com/org/repo/skill` (not `github-com--org--repo--skill`). - -Acceptance Scenarios: -1. Given globally installed skills, When the user runs `craft list -g`, Then skills display as `github.com/org/repo/skill` -2. Given globally installed skills, When the user runs `craft tree -g`, Then the tree shows composite key paths - -### Edge Cases -- Composite key with dots in non-host segments (e.g., `github.com/my.org/repo/skill`) → flat key: `github-com--my-org--repo--skill` -- Skill names containing single dashes (e.g., `paw-implement`) → preserved in flat key; `--` separator remains unambiguous -- Mixed casing in composite keys → casing preserved in flat key -- Empty skill map → `InstallFlat()` succeeds with no-op (same as `Install()`) - -## Requirements - -### Functional Requirements - -- FR-001: A `FlatKey()` function converts composite keys to flat directory names by replacing `/` with `--` and `.` with `-`, preserving casing (Stories: P1, P2, P3) -- FR-002: An `InstallFlat()` method installs skills using flat directory names with the same atomic staging, security validation, and overwrite semantics as `Install()` (Stories: P1, P2) -- FR-003: Global install commands (`craft install -g`, `craft get`, `craft update -g`) use `InstallFlat()` for writing skills to agent directories (Stories: P1, P2) -- FR-004: Global remove (`craft remove -g`) uses `FlatKey()` to derive directory names for cleanup, skipping parent directory cleanup (Stories: P3) -- FR-005: Project install commands continue using `Install()` with nested composite-key layout (Stories: P4) -- FR-006: Project remove continues using nested paths with parent directory cleanup (Stories: P4) -- FR-007: `craft list -g` and `craft tree -g` display skills using original composite key format from the pinfile (Stories: P5) - -### Key Entities - -- **Composite Key**: The `host/owner/repo/skill` string used internally (e.g., `github.com/org/repo/my-skill`) -- **Flat Key**: The transformed directory name for global installs (e.g., `github-com--org--repo--my-skill`) - -### Cross-Cutting / Non-Functional - -- Path traversal protection applies equally to flat keys (same validation as nested keys) -- Flat install behavior must remain consistent with nested install behavior (atomicity, security validation, overwrite semantics) - -## Success Criteria - -- SC-001: After global install, every skill directory is a direct child of the agent skills root (no nested subdirectories) (FR-002, FR-003) -- SC-002: `FlatKey()` output is deterministic — same input always produces same output (FR-001) -- SC-003: Two skills with the same leaf name from different repos produce different flat keys (FR-001) -- SC-004: After global remove, the flat skill directory no longer exists (FR-004) -- SC-005: Project installs produce nested paths under `forge/` unchanged from current behavior (FR-005) -- SC-006: `craft list -g` displays composite key format, not flat directory names (FR-007) -- SC-007: All existing tests continue to pass without modification (FR-005, FR-006) - -## Assumptions - -- GitHub does not allow `--` in organization or repository names, making `--` a collision-safe separator -- Dots in organization, repository, and skill names are extremely rare on GitHub, minimizing ambiguity risk from dot-to-dash conversion -- AI agents (Claude Code, GitHub Copilot) discover skills by scanning immediate children of their skills directory -- No existing global installs use the nested layout — this is a clean-slate change with no migration needed - -## Scope - -In Scope: -- `FlatKey()` pure function and `InstallFlat()` method in installer -- Wiring global install/update commands to use `InstallFlat()` -- Wiring global remove to use `FlatKey()` for cleanup -- Updating `list -g` and `tree -g` display to use composite key format -- New tests for `FlatKey()` and `InstallFlat()` -- Documentation updates (README.md, E2E_REAL_WORLD_TEST.md) - -Out of Scope: -- Changes to project-scoped install/remove behavior (`forge/` layout) -- Migration of hypothetical pre-existing nested global installs -- Changes to pinfile format or resolution logic -- Changes to `Install()` function - -## Dependencies - -- Existing `Install()` function — `InstallFlat()` delegates to it -- Pinfile composite keys — used by remove to derive flat directory names -- Agent detection (`internal/agent/`) — determines global install target paths - -## Risks & Mitigations - -- **Dot-to-dash ambiguity**: Two different composite keys could theoretically produce the same flat key if they differ only in dots vs dashes. Impact: skill collision. Mitigation: This is extremely unlikely in practice; GitHub org/repo names rarely contain dots. -- **`InstallFlat()` divergence from `Install()`**: If `InstallFlat()` is implemented as a copy rather than a wrapper, future changes to `Install()` might not be reflected. Mitigation: Implement `InstallFlat()` as a thin wrapper that transforms keys then delegates to `Install()`. - -## References - -- WorkShaping: .paw/work/flat-install-layout/WorkShaping.md diff --git a/.paw/work/flat-install-layout/WorkShaping.md b/.paw/work/flat-install-layout/WorkShaping.md deleted file mode 100644 index f0121f2..0000000 --- a/.paw/work/flat-install-layout/WorkShaping.md +++ /dev/null @@ -1,178 +0,0 @@ -# Work Shaping: Flat Install Layout for Agent Directories - -## Problem Statement - -**Who benefits:** Users who install craft skills globally for AI agents (Claude Code, GitHub Copilot). - -**What problem is solved:** Nested `github.com/org/repo/skill/` directory paths break agent skill discovery. Agents expect flat directories under their skills root (e.g., `~/.claude/skills/my-skill/`), not deeply nested trees. The current composite-key layout, while excellent for project-scoped `forge/` vendoring, creates paths that agents cannot traverse. - -**Example of the problem:** -``` -# Current (broken for agents): -~/.claude/skills/github.com/lossyrob/phased-agent-workflow/paw-implement/SKILL.md - -# Desired (agent-discoverable): -~/.claude/skills/github-com--lossyrob--phased-agent-workflow--paw-implement/SKILL.md -``` - -## Work Breakdown - -### Core Functionality - -1. **`FlatKey()` function** in `internal/install/installer.go` - - Input: composite key `github.com/org/repo/skill` - - Output: flat key `github-com--org--repo--skill` - - Rules: - - `/` → `--` (double-dash separates path components) - - `.` → `-` (dots become single dashes, applied to ALL segments) - - Casing preserved (no lowercasing) - - `--` separator is collision-safe: GitHub org/repo names and directory-derived skill names cannot contain `--` - -2. **`InstallFlat()` method** in `internal/install/installer.go` - - Same atomic staging + swap semantics as `Install()` - - Same path traversal security validation - - Transforms composite keys via `FlatKey()` before writing - - Encapsulates the flat layout decision within the install package - -3. **CLI wiring** in `install.go`, `get.go`, `update.go` - - Global installs → call `InstallFlat()` instead of `Install()` - - Project installs → continue calling `Install()` (nested layout in `forge/`) - -4. **Remove cleanup** in `remove.go` - - Global removes: use `FlatKey()` to build directory name, `os.RemoveAll()`, skip `cleanEmptyParents` (no parents to clean in flat layout) - - Project removes: keep existing nested cleanup with `cleanEmptyParents` - -### Supporting Work - -5. **Tests** in `installer_test.go` - - 5+ new tests for `InstallFlat()`/`FlatKey()` - - Existing tests (including `TestInstallCompositeKeys`) remain unchanged - -6. **Documentation** updates to `README.md` and `E2E_REAL_WORLD_TEST.md` - - Update expected directory structures for global installs - - Document flat key format - -## Edge Cases - -| Edge Case | Expected Handling | -|-----------|-------------------| -| Dots in non-host segments (e.g., `github.com/my.org/repo/skill`) | All dots → dashes: `github-com--my-org--repo--skill` | -| Skill names with dashes (e.g., `paw-implement`) | Preserved as-is; `--` separator is unambiguous | -| Mixed casing (e.g., `GitHub.com/MyOrg/Repo/Skill`) | Preserved: `GitHub-com--MyOrg--Repo--Skill` | -| Existing nested global installs | N/A | No existing installs use nested layout — clean-slate change | -| Project-scoped installs (`forge/`) | Unchanged — continue using nested composite-key layout | -| Remove after flat install | Pinfile has original composite keys; apply `FlatKey()` to derive directory name | -| `cleanEmptyParents` on global remove | No-op — flat dirs have no parents to clean | - -## Architecture - -### Component Interactions - -``` -CLI Layer (install.go / get.go / update.go / remove.go) - │ - ├── Global scope ──→ InstallFlat(agentSkillsDir, skills) - │ │ - │ ├── FlatKey(compositeKey) → flat directory name - │ ├── Staging + atomic swap (same as Install) - │ └── Path traversal validation (same as Install) - │ - └── Project scope ──→ Install(forgeDir, skills) [unchanged] -``` - -### Data Flow for Global Install - -``` -craft.pin.yaml CLI (install.go) installer.go -┌──────────────┐ ┌──────────────────┐ ┌───────────────────┐ -│ skills: │ │ │ │ InstallFlat() │ -│ github.com/ │ ──→ │ collectSkillFiles│ ──→ │ for each skill: │ -│ org/repo/ │ │ (composite keys) │ │ FlatKey(key) │ -│ skill │ │ │ │ stage + swap │ -└──────────────┘ └──────────────────┘ └───────────────────┘ - │ - ▼ - ~/.claude/skills/ - github-com--org--repo--skill/ - SKILL.md -``` - -### Data Flow for Global Remove - -``` -craft.pin.yaml CLI (remove.go) filesystem -┌──────────────┐ ┌──────────────────┐ ┌────────────────────────┐ -│ skills: │ │ for each skill: │ │ │ -│ github.com/ │ ──→ │ FlatKey(key) │ ──→ │ os.RemoveAll( │ -│ org/repo/ │ │ build path │ │ target/flat-key/) │ -│ skill │ │ (no parent │ │ (no parent cleanup) │ -└──────────────┘ │ cleanup) │ └────────────────────────┘ - └──────────────────┘ -``` - -## Critical Analysis - -### Value Assessment - -- **High value**: This is a blocking issue — global installs are currently non-functional for agent discovery -- **Low risk**: The change is well-scoped — `Install()` is untouched, `InstallFlat()` is additive -- **Clean separation**: Project (nested) vs global (flat) is a clear boundary - -### Build vs Modify Tradeoffs - -- **New code**: `FlatKey()` (~10 lines), `InstallFlat()` (~15 lines wrapping `Install()` with key transform) -- **Modified code**: 4 CLI files (routing to `InstallFlat` for global), 1 file (`remove.go` for flat cleanup) -- **Total new code**: Minimal — most logic is reused from existing `Install()` - -### No Backward Compatibility Concern - -- No existing global installs use the nested layout — clean-slate change -- No migration, no orphaned directories, no stale-remove risk - -## Codebase Fit - -### Reuse Opportunities - -- `InstallFlat()` can delegate to `Install()` after transforming keys, reusing all atomic staging, path validation, and file-writing logic -- `FlatKey()` is a pure function — easy to test in isolation -- Remove logic already looks up skills from pinfile — just needs `FlatKey()` applied for global scope - -### Similar Patterns - -- The existing `Install()` → staging → atomic swap pattern is well-established and proven -- CLI already branches on `globalFlag` for project vs global behavior — adding `InstallFlat()` call follows existing pattern - -## Risk Assessment - -| Risk | Likelihood | Impact | Mitigation | -|------|-----------|--------|------------| -| N/A — no existing nested global installs | — | — | Clean-slate; no migration needed | -| `--` separator collision | Very Low | High | Validated: GitHub disallows `--` in org/repo names | -| Dot-to-dash creates ambiguous keys | Very Low | Medium | In practice, dots in org/repo/skill names are extremely rare | -| `InstallFlat()` diverges from `Install()` over time | Low | Medium | `InstallFlat()` should delegate to `Install()` internally | - -## Implementation Phases (from brief) - -| Phase | Files | What | -|-------|-------|------| -| 1 | `installer.go` | Add `FlatKey()` + `InstallFlat()` | -| 2 | `install.go`, `get.go`, `update.go` | Wire `InstallFlat()` for global paths | -| 3 | `remove.go` | Use `FlatKey()` for global cleanup, keep nested for project | -| 4 | `installer_test.go` | 5+ new tests (FlatKey, basic, multi-package, same-name, overwrite) + FlatKey edge case tests | -| 5 | `README.md`, `E2E_REAL_WORLD_TEST.md` | Update expected directory structures | - -## Open Questions for Downstream Stages - -1. **Should `InstallFlat()` be a wrapper that transforms keys then calls `Install()`, or a parallel implementation?** (Recommendation: wrapper/delegate to minimize code duplication) -2. **Should `craft list -g` / `craft tree -g` display flat keys or reconstructed composite keys?** (Display is cosmetic; doesn't affect this feature but worth deciding) - -## Session Notes - -- **Key decision**: No backward compatibility / no migration. Clean break. -- **Key decision**: `--` separator validated as collision-safe by user. -- **Key decision**: Dots replaced in ALL segments, not just host. -- **Key decision**: Casing preserved (no lowercasing). -- **Key decision**: `FlatKey()` + `InstallFlat()` encapsulated in install package (not CLI-layer transform). -- **Key decision**: Existing tests unchanged; new tests added alongside. -- **Key decision**: `cleanEmptyParents` is a no-op for flat global removes — just `os.RemoveAll` the single flat directory. -- **Discovery**: Pinfile always has original composite keys, so `FlatKey()` can be applied at remove time without needing to reverse the transformation. diff --git a/.paw/work/flat-install-layout/WorkflowContext.md b/.paw/work/flat-install-layout/WorkflowContext.md deleted file mode 100644 index 466b5da..0000000 --- a/.paw/work/flat-install-layout/WorkflowContext.md +++ /dev/null @@ -1,30 +0,0 @@ -# WorkflowContext - -Work Title: Flat Install Layout -Work ID: flat-install-layout -Base Branch: main -Target Branch: feature/flat-install-layout -Workflow Mode: full -Review Strategy: local -Review Policy: milestones -Session Policy: continuous -Final Agent Review: enabled -Final Review Mode: multi-model -Final Review Interactive: smart -Final Review Models: gpt-5.2, gemini-3-pro-preview, claude-opus-4.6 -Final Review Specialists: all -Final Review Interaction Mode: parallel -Final Review Specialist Models: none -Plan Generation Mode: single-model -Plan Generation Models: gpt-5.2, gemini-3-pro-preview, claude-opus-4.6 -Planning Docs Review: enabled -Planning Review Mode: multi-model -Planning Review Interactive: smart -Planning Review Models: gpt-5.2, gemini-3-pro-preview, claude-opus-4.6 -Custom Workflow Instructions: none -Initial Prompt: Flat install layout for agent directories. Nested github.com/org/repo/skill/ paths break agent skill discovery. Use host--org--repo--skill format for global installs. Project forge/ keeps nested layout. -Issue URL: none -Remote: origin -Artifact Lifecycle: commit-and-clean -Artifact Paths: auto-derived -Additional Inputs: none From b00357d200542a29337881adaf76989ccbc38682 Mon Sep 17 00:00:00 2001 From: Erdem Tuna Date: Tue, 10 Mar 2026 06:09:59 +0000 Subject: [PATCH 07/10] Address SoT review findings: collision guard and test improvements - Add collision detection in InstallFlat: returns error when distinct composite keys produce the same flat key (MF-2) - Add TestInstallFlatCollisionDetection for dot-vs-dash collision case - Improve TestFlatKey to use t.Run subtests for CI diagnostics (C-3) - Document collision limitation in FlatKey docstring and README (SF-6) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- README.md | 2 +- internal/install/installer.go | 8 +++++++ internal/install/installer_test.go | 38 +++++++++++++++++++++++------- 3 files changed, 38 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index a47d00f..4e587e7 100644 --- a/README.md +++ b/README.md @@ -373,7 +373,7 @@ When both agents are detected, craft prompts you to choose. Use `--target └── SKILL.md ``` -The flat directory name is derived from the composite key: slashes become `--`, dots become `-` (e.g., `github.com/acme/company-standards/coding-style` → `github-com--acme--company-standards--coding-style`). +The flat directory name is derived from the composite key: slashes become `--`, dots become `-` (e.g., `github.com/acme/company-standards/coding-style` → `github-com--acme--company-standards--coding-style`). Note: composite keys differing only in dots vs hyphens (e.g., `my.repo` vs `my-repo`) would collide — craft detects this and returns an error. **Project installs** (`craft install`) vendor to `forge/` in the project root (gitignored), using nested composite-key paths. diff --git a/internal/install/installer.go b/internal/install/installer.go index 92f55e0..ca052cc 100644 --- a/internal/install/installer.go +++ b/internal/install/installer.go @@ -91,6 +91,10 @@ func Install(target string, skills map[string]map[string][]byte) error { // directory name suitable for agent skill discovery. Slashes become "--" and // dots become "-". Casing is preserved. // +// Note: This encoding is lossy — composite keys differing only in dots vs +// hyphens (e.g., "my.repo" vs "my-repo") produce the same flat key. +// InstallFlat detects such collisions and returns an error. +// // Example: "github.com/org/repo/my-skill" → "github-com--org--repo--my-skill" func FlatKey(compositeKey string) string { flat := strings.ReplaceAll(compositeKey, ".", "-") @@ -102,10 +106,14 @@ func FlatKey(compositeKey string) string { // is a direct child of the target directory. This is used for global installs // where AI agents need to discover skills by scanning immediate children. // It transforms composite keys via FlatKey then delegates to Install. +// Returns an error if two distinct composite keys produce the same flat key. func InstallFlat(target string, skills map[string]map[string][]byte) error { flat := make(map[string]map[string][]byte, len(skills)) for k, v := range skills { flat[FlatKey(k)] = v } + if len(flat) != len(skills) { + return fmt.Errorf("flat key collision: %d skills mapped to %d unique keys; composite keys with dots vs hyphens in the same position will collide", len(skills), len(flat)) + } return Install(target, flat) } diff --git a/internal/install/installer_test.go b/internal/install/installer_test.go index afe162f..7399a7c 100644 --- a/internal/install/installer_test.go +++ b/internal/install/installer_test.go @@ -211,20 +211,23 @@ func TestInstallCompositeKeys(t *testing.T) { func TestFlatKey(t *testing.T) { tests := []struct { + name string input string want string }{ - {"github.com/org/repo/my-skill", "github-com--org--repo--my-skill"}, - {"github.com/lossyrob/phased-agent-workflow/paw-implement", "github-com--lossyrob--phased-agent-workflow--paw-implement"}, - {"github.com/anthropics/skills/skill-creator", "github-com--anthropics--skills--skill-creator"}, - {"simple-skill", "simple-skill"}, - {"host.name/owner/repo/skill", "host-name--owner--repo--skill"}, + {"standard composite key", "github.com/org/repo/my-skill", "github-com--org--repo--my-skill"}, + {"real PAW key", "github.com/lossyrob/phased-agent-workflow/paw-implement", "github-com--lossyrob--phased-agent-workflow--paw-implement"}, + {"anthropic skills", "github.com/anthropics/skills/skill-creator", "github-com--anthropics--skills--skill-creator"}, + {"simple skill name", "simple-skill", "simple-skill"}, + {"custom host with dots", "host.name/owner/repo/skill", "host-name--owner--repo--skill"}, } for _, tt := range tests { - got := FlatKey(tt.input) - if got != tt.want { - t.Errorf("FlatKey(%q) = %q, want %q", tt.input, got, tt.want) - } + t.Run(tt.name, func(t *testing.T) { + got := FlatKey(tt.input) + if got != tt.want { + t.Errorf("FlatKey(%q) = %q, want %q", tt.input, got, tt.want) + } + }) } } @@ -372,3 +375,20 @@ func TestInstallFlatOverwrites(t *testing.T) { t.Errorf("expected v2, got %q", content) } } + +func TestInstallFlatCollisionDetection(t *testing.T) { + target := filepath.Join(t.TempDir(), "skills") + // These two keys collide under FlatKey: dots and hyphens map to the same char + skills := map[string]map[string][]byte{ + "github.com/org/my.repo/skill": {"SKILL.md": []byte("dot")}, + "github.com/org/my-repo/skill": {"SKILL.md": []byte("dash")}, + } + + err := InstallFlat(target, skills) + if err == nil { + t.Fatal("expected collision error, got nil") + } + if !strings.Contains(err.Error(), "flat key collision") { + t.Errorf("unexpected error message: %v", err) + } +} From d3d70f381766ffa849755d063934d2d34a9add38 Mon Sep 17 00:00:00 2001 From: Erdem Tuna Date: Tue, 10 Mar 2026 06:38:45 +0000 Subject: [PATCH 08/10] Address all remaining SoT findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SF-1: Make FlatKey injective — only replace slashes with --, preserve dots as-is. Dots are valid directory characters on all platforms, eliminating the lossy dot-to-dash collision risk entirely. MF-1: Add TestRunRemoveGlobal_FlatCleanup integration test that verifies flat directory cleanup for global remove operations. SF-2: Extract CompositeKey() helper in install package for consistent key construction. Remove.go now uses CompositeKey instead of manual string concatenation, eliminating implicit coupling. SF-3/SF-4: Extract QualifySkillNames() helper in install package, used by both list.go and tree.go to eliminate duplicated display logic. Both commands now use the shared helper instead of inline loops. SF-5: No migration needed — no prior releases used nested global layout. TestInstallFlatDistinguishesDotFromDash: New test proving dots and dashes produce distinct flat keys with injective encoding. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- README.md | 6 +-- internal/cli/list.go | 6 +-- internal/cli/remove.go | 2 +- internal/cli/remove_test.go | 67 ++++++++++++++++++++++++++++++ internal/cli/tree.go | 6 +-- internal/install/installer.go | 36 +++++++++------- internal/install/installer_test.go | 53 ++++++++++++----------- 7 files changed, 126 insertions(+), 50 deletions(-) diff --git a/README.md b/README.md index 4e587e7..97d2dd5 100644 --- a/README.md +++ b/README.md @@ -367,13 +367,13 @@ When both agents are detected, craft prompts you to choose. Use `--target ``` ~/.claude/skills/ -├── github-com--acme--company-standards--coding-style/ +├── github.com--acme--company-standards--coding-style/ │ └── SKILL.md -└── github-com--acme--company-standards--review-checklist/ +└── github.com--acme--company-standards--review-checklist/ └── SKILL.md ``` -The flat directory name is derived from the composite key: slashes become `--`, dots become `-` (e.g., `github.com/acme/company-standards/coding-style` → `github-com--acme--company-standards--coding-style`). Note: composite keys differing only in dots vs hyphens (e.g., `my.repo` vs `my-repo`) would collide — craft detects this and returns an error. +The flat directory name is derived from the composite key: slashes become `--`, dots and casing are preserved (e.g., `github.com/acme/company-standards/coding-style` → `github.com--acme--company-standards--coding-style`). The encoding is injective — distinct composite keys always produce distinct flat keys. **Project installs** (`craft install`) vendor to `forge/` in the project root (gitignored), using nested composite-key paths. diff --git a/internal/cli/list.go b/internal/cli/list.go index b06b241..6e780ff 100644 --- a/internal/cli/list.go +++ b/internal/cli/list.go @@ -6,6 +6,7 @@ import ( "strings" "text/tabwriter" + installlib "github.com/erdemtuna/craft/internal/install" "github.com/erdemtuna/craft/internal/resolve" "github.com/spf13/cobra" ) @@ -86,10 +87,7 @@ func runList(cmd *cobra.Command, args []string) error { if len(d.skills) > 0 { displaySkills := d.skills if globalFlag { - displaySkills = make([]string, len(d.skills)) - for i, s := range d.skills { - displaySkills[i] = d.url + "/" + s - } + displaySkills = installlib.QualifySkillNames(d.url, d.skills) } cmd.Printf(" skills: %s\n", sanitize(strings.Join(displaySkills, ", "))) } else { diff --git a/internal/cli/remove.go b/internal/cli/remove.go index fab700f..63ae7e6 100644 --- a/internal/cli/remove.go +++ b/internal/cli/remove.go @@ -141,7 +141,7 @@ func runRemove(cmd *cobra.Command, args []string) error { for _, tp := range targetPath { var skillDir string if globalFlag { - skillDir = filepath.Join(tp, installlib.FlatKey(nsPrefix+"/"+skillName)) + skillDir = filepath.Join(tp, installlib.FlatKey(installlib.CompositeKey(nsPrefix, skillName))) } else { skillDir = filepath.Join(tp, nsPrefix, skillName) } diff --git a/internal/cli/remove_test.go b/internal/cli/remove_test.go index 9c1bd66..6bd1d6d 100644 --- a/internal/cli/remove_test.go +++ b/internal/cli/remove_test.go @@ -362,3 +362,70 @@ func TestCleanEmptyParents_StopsAtRoot(t *testing.T) { t.Error("root should not be deleted") } } + +func TestRunRemoveGlobal_FlatCleanup(t *testing.T) { + // Set up isolated HOME for global manifest/pinfile + fakeHome := t.TempDir() + origHome := os.Getenv("HOME") + t.Setenv("HOME", fakeHome) + defer func() { _ = os.Setenv("HOME", origHome) }() + + // Reset globalFlag after test (cobra persistent flags leak between tests) + defer func() { globalFlag = false }() + + craftDir := filepath.Join(fakeHome, ".craft") + _ = os.MkdirAll(craftDir, 0755) + + manifestContent := `schema_version: 1 +name: global-pkg +dependencies: + my-dep: github.com/org/repo@v1.0.0 +` + _ = os.WriteFile(filepath.Join(craftDir, "craft.yaml"), []byte(manifestContent), 0644) + + pinContent := `pin_version: 1 +resolved: + github.com/org/repo@v1.0.0: + commit: abc123 + integrity: sha256-test + skills: + - my-skill +` + _ = os.WriteFile(filepath.Join(craftDir, "craft.pin.yaml"), []byte(pinContent), 0644) + + // Create flat skill directory (as InstallFlat would produce) + targetDir := filepath.Join(fakeHome, "agent-skills") + flatSkillDir := filepath.Join(targetDir, "github.com--org--repo--my-skill") + _ = os.MkdirAll(flatSkillDir, 0755) + _ = os.WriteFile(filepath.Join(flatSkillDir, "SKILL.md"), []byte("skill"), 0644) + + var buf bytes.Buffer + rootCmd.SetOut(&buf) + rootCmd.SetErr(&buf) + rootCmd.SetArgs([]string{"remove", "-g", "--target", targetDir, "my-dep"}) + err := rootCmd.Execute() + + if err != nil { + t.Fatalf("unexpected error: %v\noutput: %s", err, buf.String()) + } + + output := buf.String() + if !strings.Contains(output, "Removed") { + t.Errorf("expected 'Removed' message, got: %s", output) + } + + // Verify flat skill directory was removed + if _, err := os.Stat(flatSkillDir); err == nil { + t.Error("flat skill directory should have been removed") + } + + // Verify no nested parent directories were created + if _, err := os.Stat(filepath.Join(targetDir, "github.com")); err == nil { + t.Error("no nested parent directories should exist") + } + + // Verify target dir itself still exists + if _, err := os.Stat(targetDir); err != nil { + t.Error("target directory should still exist") + } +} diff --git a/internal/cli/tree.go b/internal/cli/tree.go index c66190c..8c135f9 100644 --- a/internal/cli/tree.go +++ b/internal/cli/tree.go @@ -3,6 +3,7 @@ package cli import ( "strings" + installlib "github.com/erdemtuna/craft/internal/install" "github.com/erdemtuna/craft/internal/resolve" "github.com/erdemtuna/craft/internal/ui" "github.com/spf13/cobra" @@ -59,10 +60,7 @@ func runTree(cmd *cobra.Command, args []string) error { skills := entry.Skills if globalFlag { - skills = make([]string, len(entry.Skills)) - for i, s := range entry.Skills { - skills[i] = parsed.PackageIdentity() + "/" + s - } + skills = installlib.QualifySkillNames(parsed.PackageIdentity(), entry.Skills) } deps = append(deps, ui.DepNode{ diff --git a/internal/install/installer.go b/internal/install/installer.go index ca052cc..912e90b 100644 --- a/internal/install/installer.go +++ b/internal/install/installer.go @@ -88,32 +88,40 @@ func Install(target string, skills map[string]map[string][]byte) error { } // FlatKey converts a composite skill key (host/owner/repo/skill) into a flat -// directory name suitable for agent skill discovery. Slashes become "--" and -// dots become "-". Casing is preserved. +// directory name suitable for agent skill discovery. Slashes become "--". +// Dots and casing are preserved. The encoding is injective — distinct +// composite keys always produce distinct flat keys. // -// Note: This encoding is lossy — composite keys differing only in dots vs -// hyphens (e.g., "my.repo" vs "my-repo") produce the same flat key. -// InstallFlat detects such collisions and returns an error. -// -// Example: "github.com/org/repo/my-skill" → "github-com--org--repo--my-skill" +// Example: "github.com/org/repo/my-skill" → "github.com--org--repo--my-skill" func FlatKey(compositeKey string) string { - flat := strings.ReplaceAll(compositeKey, ".", "-") - flat = strings.ReplaceAll(flat, "/", "--") - return flat + return strings.ReplaceAll(compositeKey, "/", "--") } // InstallFlat installs skills using flat directory names so that each skill // is a direct child of the target directory. This is used for global installs // where AI agents need to discover skills by scanning immediate children. // It transforms composite keys via FlatKey then delegates to Install. -// Returns an error if two distinct composite keys produce the same flat key. func InstallFlat(target string, skills map[string]map[string][]byte) error { flat := make(map[string]map[string][]byte, len(skills)) for k, v := range skills { flat[FlatKey(k)] = v } - if len(flat) != len(skills) { - return fmt.Errorf("flat key collision: %d skills mapped to %d unique keys; composite keys with dots vs hyphens in the same position will collide", len(skills), len(flat)) - } return Install(target, flat) } + +// CompositeKey builds a composite skill key from a package identity and skill +// name. This is the canonical way to construct the key used by both Install +// and remove operations, ensuring format consistency. +func CompositeKey(packageIdentity, skillName string) string { + return packageIdentity + "/" + skillName +} + +// QualifySkillNames prefixes each skill name with its package identity to form +// composite key display names. Used by list and tree commands for global scope. +func QualifySkillNames(packageIdentity string, skills []string) []string { + qualified := make([]string, len(skills)) + for i, s := range skills { + qualified[i] = CompositeKey(packageIdentity, s) + } + return qualified +} diff --git a/internal/install/installer_test.go b/internal/install/installer_test.go index 7399a7c..ab9435f 100644 --- a/internal/install/installer_test.go +++ b/internal/install/installer_test.go @@ -215,11 +215,11 @@ func TestFlatKey(t *testing.T) { input string want string }{ - {"standard composite key", "github.com/org/repo/my-skill", "github-com--org--repo--my-skill"}, - {"real PAW key", "github.com/lossyrob/phased-agent-workflow/paw-implement", "github-com--lossyrob--phased-agent-workflow--paw-implement"}, - {"anthropic skills", "github.com/anthropics/skills/skill-creator", "github-com--anthropics--skills--skill-creator"}, + {"standard composite key", "github.com/org/repo/my-skill", "github.com--org--repo--my-skill"}, + {"real PAW key", "github.com/lossyrob/phased-agent-workflow/paw-implement", "github.com--lossyrob--phased-agent-workflow--paw-implement"}, + {"anthropic skills", "github.com/anthropics/skills/skill-creator", "github.com--anthropics--skills--skill-creator"}, {"simple skill name", "simple-skill", "simple-skill"}, - {"custom host with dots", "host.name/owner/repo/skill", "host-name--owner--repo--skill"}, + {"custom host with dots", "host.name/owner/repo/skill", "host.name--owner--repo--skill"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -237,11 +237,11 @@ func TestFlatKeyEdgeCases(t *testing.T) { input string want string }{ - {"dots in org", "github.com/my.org/repo/skill", "github-com--my-org--repo--skill"}, - {"dots in repo", "github.com/org/my.repo/skill", "github-com--org--my-repo--skill"}, - {"dots in skill", "github.com/org/repo/my.skill", "github-com--org--repo--my-skill"}, - {"dots everywhere", "git.hub.com/my.org/my.repo/my.skill", "git-hub-com--my-org--my-repo--my-skill"}, - {"mixed casing", "GitHub.com/MyOrg/Repo/Skill", "GitHub-com--MyOrg--Repo--Skill"}, + {"dots in org", "github.com/my.org/repo/skill", "github.com--my.org--repo--skill"}, + {"dots in repo", "github.com/org/my.repo/skill", "github.com--org--my.repo--skill"}, + {"dots in skill", "github.com/org/repo/my.skill", "github.com--org--repo--my.skill"}, + {"dots everywhere", "git.hub.com/my.org/my.repo/my.skill", "git.hub.com--my.org--my.repo--my.skill"}, + {"mixed casing", "GitHub.com/MyOrg/Repo/Skill", "GitHub.com--MyOrg--Repo--Skill"}, {"many components", "a/b/c/d/e", "a--b--c--d--e"}, {"no slashes no dots", "plain-name", "plain-name"}, } @@ -268,7 +268,7 @@ func TestInstallFlatCreatesStructure(t *testing.T) { } // Should be flat, not nested - flatDir := filepath.Join(target, "github-com--org--repo--my-skill") + flatDir := filepath.Join(target, "github.com--org--repo--my-skill") content, err := os.ReadFile(filepath.Join(flatDir, "SKILL.md")) if err != nil { t.Fatalf("ReadFile error: %v", err) @@ -312,11 +312,11 @@ func TestInstallFlatMultiPackage(t *testing.T) { for _, e := range entries { names[e.Name()] = true } - if !names["github-com--org--repo--skill-a"] { - t.Error("missing github-com--org--repo--skill-a") + if !names["github.com--org--repo--skill-a"] { + t.Error("missing github.com--org--repo--skill-a") } - if !names["github-com--other--tools--skill-b"] { - t.Error("missing github-com--other--tools--skill-b") + if !names["github.com--other--tools--skill-b"] { + t.Error("missing github.com--other--tools--skill-b") } } @@ -336,7 +336,7 @@ func TestInstallFlatSameName(t *testing.T) { } // Same leaf name, different flat keys — no collision - c1, err := os.ReadFile(filepath.Join(target, "github-com--org--repo--my-skill", "SKILL.md")) + c1, err := os.ReadFile(filepath.Join(target, "github.com--org--repo--my-skill", "SKILL.md")) if err != nil { t.Fatalf("ReadFile org: %v", err) } @@ -344,7 +344,7 @@ func TestInstallFlatSameName(t *testing.T) { t.Errorf("org content: %q", c1) } - c2, err := os.ReadFile(filepath.Join(target, "github-com--other--tools--my-skill", "SKILL.md")) + c2, err := os.ReadFile(filepath.Join(target, "github.com--other--tools--my-skill", "SKILL.md")) if err != nil { t.Fatalf("ReadFile other: %v", err) } @@ -370,25 +370,30 @@ func TestInstallFlatOverwrites(t *testing.T) { t.Fatalf("second InstallFlat: %v", err) } - content, _ := os.ReadFile(filepath.Join(target, "github-com--org--repo--my-skill", "SKILL.md")) + content, _ := os.ReadFile(filepath.Join(target, "github.com--org--repo--my-skill", "SKILL.md")) if string(content) != "v2" { t.Errorf("expected v2, got %q", content) } } -func TestInstallFlatCollisionDetection(t *testing.T) { +func TestInstallFlatDistinguishesDotFromDash(t *testing.T) { target := filepath.Join(t.TempDir(), "skills") - // These two keys collide under FlatKey: dots and hyphens map to the same char + // Dots and hyphens are distinct — no collision with injective encoding skills := map[string]map[string][]byte{ "github.com/org/my.repo/skill": {"SKILL.md": []byte("dot")}, "github.com/org/my-repo/skill": {"SKILL.md": []byte("dash")}, } - err := InstallFlat(target, skills) - if err == nil { - t.Fatal("expected collision error, got nil") + if err := InstallFlat(target, skills); err != nil { + t.Fatalf("InstallFlat error: %v", err) } - if !strings.Contains(err.Error(), "flat key collision") { - t.Errorf("unexpected error message: %v", err) + + c1, _ := os.ReadFile(filepath.Join(target, "github.com--org--my.repo--skill", "SKILL.md")) + if string(c1) != "dot" { + t.Errorf("dot repo content: %q", c1) + } + c2, _ := os.ReadFile(filepath.Join(target, "github.com--org--my-repo--skill", "SKILL.md")) + if string(c2) != "dash" { + t.Errorf("dash repo content: %q", c2) } } From 09eaf09c9ed9e2f3d70dd8667f3a98f850171e5f Mon Sep 17 00:00:00 2001 From: Erdem Tuna Date: Tue, 10 Mar 2026 06:49:45 +0000 Subject: [PATCH 09/10] Address consider-level SoT findings C-2: QualifySkillNames skips empty skill names to avoid trailing-slash display artifacts in list/tree output. C-4: InstallFlat returns a clear error for empty composite keys instead of falling through to Install's path traversal check with a misleading 'escapes target directory' message. Add TestInstallFlatRejectsEmptyKey test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/install/installer.go | 16 ++++++++++++---- internal/install/installer_test.go | 14 ++++++++++++++ 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/internal/install/installer.go b/internal/install/installer.go index 912e90b..1efcff4 100644 --- a/internal/install/installer.go +++ b/internal/install/installer.go @@ -104,7 +104,11 @@ func FlatKey(compositeKey string) string { func InstallFlat(target string, skills map[string]map[string][]byte) error { flat := make(map[string]map[string][]byte, len(skills)) for k, v := range skills { - flat[FlatKey(k)] = v + fk := FlatKey(k) + if fk == "" { + return fmt.Errorf("empty composite key produces empty flat key") + } + flat[fk] = v } return Install(target, flat) } @@ -118,10 +122,14 @@ func CompositeKey(packageIdentity, skillName string) string { // QualifySkillNames prefixes each skill name with its package identity to form // composite key display names. Used by list and tree commands for global scope. +// Empty skill names are skipped to avoid trailing-slash display artifacts. func QualifySkillNames(packageIdentity string, skills []string) []string { - qualified := make([]string, len(skills)) - for i, s := range skills { - qualified[i] = CompositeKey(packageIdentity, s) + qualified := make([]string, 0, len(skills)) + for _, s := range skills { + if s == "" { + continue + } + qualified = append(qualified, CompositeKey(packageIdentity, s)) } return qualified } diff --git a/internal/install/installer_test.go b/internal/install/installer_test.go index ab9435f..b91a0ae 100644 --- a/internal/install/installer_test.go +++ b/internal/install/installer_test.go @@ -397,3 +397,17 @@ func TestInstallFlatDistinguishesDotFromDash(t *testing.T) { t.Errorf("dash repo content: %q", c2) } } + +func TestInstallFlatRejectsEmptyKey(t *testing.T) { + target := filepath.Join(t.TempDir(), "skills") + skills := map[string]map[string][]byte{ + "": {"SKILL.md": []byte("bad")}, + } + err := InstallFlat(target, skills) + if err == nil { + t.Fatal("expected error for empty composite key") + } + if !strings.Contains(err.Error(), "empty") { + t.Errorf("expected 'empty' in error, got: %v", err) + } +} From 868a56b84629c3128960a15e312138f94a46d5e5 Mon Sep 17 00:00:00 2001 From: Erdem Tuna Date: Tue, 10 Mar 2026 06:51:27 +0000 Subject: [PATCH 10/10] Sanitize tree command output against control characters Apply sanitize() to all user-derived strings (alias, URL, skill names, local skills) before passing to RenderTree. Previously list.go sanitized its output but tree.go did not, creating an inconsistency. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/cli/tree.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/internal/cli/tree.go b/internal/cli/tree.go index 8c135f9..a987428 100644 --- a/internal/cli/tree.go +++ b/internal/cli/tree.go @@ -31,7 +31,7 @@ func runTree(cmd *cobra.Command, args []string) error { var localSkills []string for _, s := range m.Skills { parts := strings.Split(strings.TrimRight(s, "/"), "/") - localSkills = append(localSkills, parts[len(parts)-1]) + localSkills = append(localSkills, sanitize(parts[len(parts)-1])) } // Build alias lookup from manifest @@ -63,10 +63,16 @@ func runTree(cmd *cobra.Command, args []string) error { skills = installlib.QualifySkillNames(parsed.PackageIdentity(), entry.Skills) } + // Sanitize all user-derived strings before rendering + sanitizedSkills := make([]string, len(skills)) + for i, s := range skills { + sanitizedSkills[i] = sanitize(s) + } + deps = append(deps, ui.DepNode{ - Alias: alias, - URL: key, - Skills: skills, + Alias: sanitize(alias), + URL: sanitize(key), + Skills: sanitizedSkills, }) }