diff --git a/CHANGELOG.md b/CHANGELOG.md index fb704dd..5a87c47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,16 @@ All notable changes to creed are documented here. The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) and the project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Added + +- **Layered context sources.** Compose cached git organization context with + per-repository `.creed/` files; `sync`, `diff`, `validate`, and `doctor` now + share the same resolved source path. +- **Layered migration guide.** Document manifest configuration, pinned refs, + authentication, CI drift gating, and non-clobbering pull/push behavior. + ## [0.3.0] — 2026-08-19 The first post-reset feature release. Everything shipped since v0.1.0 lands diff --git a/README.md b/README.md index 98b2b03..3694955 100644 --- a/README.md +++ b/README.md @@ -164,20 +164,42 @@ Paths in `skills` and `config` are relative to `.creed/`. `output_dir` is relati to the project root and is guarded so it cannot escape the project with `..` or an absolute path. -## Source models - -Local source is the default: Creed reads `.creed/` from the current project. -Git-backed sharing is available through the service `Pull` path: the git remote -is cloned or reused from cache, then read with the same manifest, skill, and -config semantics as a local source. The manifest can record the remote URL: +For organization-wide context, use an ordered layered source. Shared layers are +read first and the consumer's local `.creed/` layer is always read last: ```yaml source: - type: git + type: layered path: .creed - remote: https://github.com/example/context.git + layers: + - name: org + type: git + remote: https://github.com/TechGodHQ/agent-context.git + path: .creed + ref: 0123456789abcdef0123456789abcdef01234567 ``` +A later layer with the same skill or config name overrides the earlier entry. +Use distinct names when both entries should be emitted. See +[`docs/layered-context-migration.md`](docs/layered-context-migration.md) for +migration and CI guidance. + +## Source models + +Local source is the default: Creed reads `.creed/` from the current project. +A direct git source remains supported for compatibility. Layered sharing is the +v0.4 path: the configured git layers are cloned or reused from cache, then +composed with the local source through the same SourceReader used by `sync`, +`diff`, `validate`, and `doctor`. + +`creed pull ` records the remote as an `org` layer and composes it; it +never replaces local `.creed/` files. `creed push` is rejected for layered +sources so shared context changes go through review instead of clobbering the +central repository. + +See [`docs/layered-context-migration.md`](docs/layered-context-migration.md) +for the full manifest and migration guide. + Git remotes support public HTTPS URLs, private HTTPS URLs with the configured service token, and SSH URLs through either `SSH_AUTH_SOCK` or an explicit `CREED_GIT_SSH_KEY` path. If the key is passphrase-protected, set @@ -227,6 +249,7 @@ Creed uses a ports-and-adapters layout: - `internal/ports`: source-reader and target-emitter interfaces. - `internal/adapters/localfs`: reads `.creed/` and writes target files locally. - `internal/adapters/gitremote`: reads `.creed/` from a git remote clone/cache. +- `internal/adapters/layered`: composes ordered local/git source readers. - `internal/usecase`: the sync engine and result model. - `internal/service`: the canonical API shared by generated CLI, MCP, and HTTP surfaces. - `internal/codegen`: parses the service interface and emits operation descriptors plus diff --git a/docs/architecture.md b/docs/architecture.md index 92ae5a3..91dbf09 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -106,6 +106,8 @@ Implemented adapters: - `localfs.Source`: reads `.creed/` in the current project. - `gitremote.Source`: clones or reuses a cached git repository, then delegates reads to the local filesystem adapter. +- `layered.Source`: merges ordered local/git readers into one SourceReader; the + service appends the consumer's local reader after shared layers. ## Target emitters @@ -124,12 +126,13 @@ and skips files whose content is already identical. `internal/usecase.SyncEngine` performs one sync run: -1. Read `.creed/manifest.yaml`. -2. Resolve either a requested target (`--target`) or all enabled targets. -3. Validate `output_dir` so emitted paths cannot escape the project root. -4. Read all manifest-declared skills and config files. -5. For each target, prepare emitted files from target path metadata. -6. Emit files, collecting per-file and per-target result data. +1. Read `.creed/manifest.yaml` and resolve its local/git/layered source graph. +2. Read every ordered shared layer, then the consumer's local source. +3. Resolve either a requested target (`--target`) or all enabled targets. +4. Validate `output_dir` so emitted paths cannot escape the project root. +5. Read all composed skills and config files. +6. For each target, prepare emitted files from target path metadata. +7. Emit files, collecting per-file and per-target result data. Partial target failures are isolated: one failed target does not prevent the next target from running. A top-level error is reserved for failures that prevent the diff --git a/docs/layered-context-migration.md b/docs/layered-context-migration.md new file mode 100644 index 0000000..ef83f0f --- /dev/null +++ b/docs/layered-context-migration.md @@ -0,0 +1,92 @@ +# Layered context migration + +Creed v0.4 can compose a shared organization context repository with the +repository's own `.creed/` files. The shared layer is emitted first and the +local repository layer is emitted second, using the same `---` separator that +Creed uses for ordinary config aggregation. + +## Manifest contract + +Add an ordered `source.layers` list to the consumer repository's manifest: + +```yaml +version: 1 +source: + type: layered + path: .creed + layers: + - name: org + type: git + remote: https://github.com/TechGodHQ/agent-context.git + path: .creed + # Prefer a full commit SHA for reproducible CI. + ref: 0123456789abcdef0123456789abcdef01234567 + +targets: + - name: codex + enabled: true + output_dir: . +config: + - name: repo + path: config/repo.md +skills: + - name: repo-review + path: skills/repo-review.md +``` + +`source.layers` is ordered. Creed always appends the consumer's local source +as the final layer. A layer can be `type: local` for a second local source, or +`type: git` for a cloned source. `path` is relative to the layer root and +defaults to `.creed`. `ref` may pin a branch, tag reference, or commit SHA. + +The consumer's local `source.path` remains `.creed`; custom `path` values are +for git or secondary local layers. Pull rejects URLs with embedded credentials, +queries, or fragments—configure HTTPS tokens or SSH authentication separately. + +If two layers declare the same skill or config name, the later layer wins. Use +distinct names when both pieces of context should be emitted; distinct config +names are normally preferable for organization rules and repository rules. + +## Migration steps + +1. Create a central repository containing organization-wide config and skills + under `.creed/`, with its own `manifest.yaml`. +2. Remove duplicated organization entries from each consumer repository only + after the central repository has been pushed and its commit SHA recorded. +3. Add the layered `source` block above to each consumer manifest, retaining + the consumer's targets and repository-specific entries. +4. Run `creed validate`. It fetches every configured layer and checks the + referenced remote files as well as local files. +5. Run `creed sync` and review the generated target files. +6. Add `creed diff` to CI. It uses the same composed source and exits `1` when + generated output drifts, so central-context changes are gated too. + +## Pull behavior + +`creed pull ` now records the remote as an `org` layer and composes it +with the local source. It never replaces local `.creed/config/*` or +`.creed/skills/*`. If the consumer has no manifest yet, pull creates a minimal +layered manifest so `validate`, `doctor`, and `diff` remain usable afterward. + +`creed push` is intentionally rejected for layered sources. Shared context +should be changed in the central repository through the normal review/PR path; +blindly copying a consumer `.creed/` directory back to the organization +repository would reintroduce the v0.3 clobbering failure mode. + +## Authentication and caching + +- Public and private HTTPS remotes use go-git HTTPS authentication. Configure + the service token with the existing `WithGitToken` integration path; tokens + are not written into URLs or reports. +- SSH remotes use `SSH_AUTH_SOCK`, or `CREED_GIT_SSH_KEY` plus + `CREED_GIT_SSH_PASSPHRASE` for an explicit key. +- `WithCacheDir` enables commit-aware clone caching. A pinned commit reuses its + cached clone; an unpinned branch is refreshed when its remote HEAD changes. +- `creed doctor` reports the configured remote with embedded passwords removed. + +For CI-secret-backed end-to-end verification, set +`CREED_RUN_GITHUB_AUTH_INTEGRATION=1` together with +`CREED_GITHUB_HTTPS_REMOTE`/`CREED_GITHUB_HTTPS_TOKEN` and/or +`CREED_GITHUB_SSH_REMOTE`. The integration test exercises the complete layered +service path and never prints or stores the token. SSH mode uses the runner's +`SSH_AUTH_SOCK` or `CREED_GIT_SSH_KEY` configuration. diff --git a/internal/adapters/gitremote/source.go b/internal/adapters/gitremote/source.go index 9d4f3b9..dbe58d2 100644 --- a/internal/adapters/gitremote/source.go +++ b/internal/adapters/gitremote/source.go @@ -11,6 +11,7 @@ import ( "encoding/json" "errors" "fmt" + "net/url" "os" "path/filepath" "strings" @@ -44,8 +45,12 @@ var errCacheMiss = errors.New("git remote cache miss") // It implements ports.SourceReader by cloning the repository to a directory // and delegating reads to a LocalFS adapter. type Source struct { - // remoteURL is the git clone URL (HTTPS). + // remoteURL is the git clone URL. remoteURL string + // sourcePath is the source directory relative to the cloned repository. + sourcePath string + // ref optionally pins the clone to a branch, tag, or commit SHA. + ref string // token is an optional authentication token for private repos. token string // cacheDir is an optional persistent directory for commit-cache behavior. @@ -62,30 +67,57 @@ type Source struct { cloneCount int // test hook: number of actual clone operations performed } +// SourceOptions configures a GitRemote source reader. +type SourceOptions struct { + // RemoteURL is the git clone URL. + RemoteURL string + // SourcePath is the source directory relative to the cloned repository. + SourcePath string + // Ref optionally pins the clone to a branch, tag, or commit SHA. + Ref string + // Token is an optional HTTPS authentication token. + Token string + // CacheDir enables persistent clone caching when non-empty. + CacheDir string +} + // NewSource creates a GitRemote source reader for the given remote URL. // An optional token can be provided for private repository access. // Clones go to a temp directory with no persistent caching. func NewSource(remoteURL, token string) *Source { - return &Source{ - remoteURL: remoteURL, - token: token, - } + return NewSourceWithOptions(SourceOptions{RemoteURL: remoteURL, Token: token}) } // NewSourceWithCache creates a GitRemote source reader with persistent commit // caching. The cacheDir stores clone directories and SHA metadata so that // subsequent reads on unchanged remote HEAD skip the clone entirely. func NewSourceWithCache(remoteURL, token, cacheDir string) *Source { + return NewSourceWithOptions(SourceOptions{RemoteURL: remoteURL, Token: token, CacheDir: cacheDir}) +} + +// NewSourceWithOptions creates a GitRemote source reader with an explicit +// source subdirectory, optional ref pin, authentication token, and cache. +func NewSourceWithOptions(options SourceOptions) *Source { + sourcePath := options.SourcePath + if strings.TrimSpace(sourcePath) == "" { + sourcePath = ".creed" + } return &Source{ - remoteURL: remoteURL, - token: token, - cacheDir: cacheDir, + remoteURL: options.RemoteURL, + sourcePath: sourcePath, + ref: strings.TrimSpace(options.Ref), + token: options.Token, + cacheDir: options.CacheDir, } } // cacheKey returns a deterministic cache key derived from the remote URL. func (s *Source) cacheKey() string { - h := sha256.Sum256([]byte(s.remoteURL)) + material := s.remoteURL + if s.ref != "" { + material += "\x00" + s.ref + } + h := sha256.Sum256([]byte(material)) return hex.EncodeToString(h[:]) } @@ -99,8 +131,56 @@ func (s *Source) cacheFilePath() string { return filepath.Join(s.cacheDir, "refs", s.cacheKey()+".json") } +func (s *Source) ensureCacheLayout(create bool) error { + if s.cacheDir == "" { + return nil + } + for _, path := range []string{s.cacheDir, filepath.Join(s.cacheDir, "clones"), filepath.Join(s.cacheDir, "refs")} { + if err := ensureDirectoryNoSymlink(path, create); err != nil { + return err + } + } + return nil +} + +func ensureDirectoryNoSymlink(path string, create bool) error { + absolute, err := filepath.Abs(path) + if err != nil { + return err + } + volume := filepath.VolumeName(absolute) + current := volume + string(filepath.Separator) + rest := strings.TrimPrefix(absolute, current) + for _, part := range strings.Split(rest, string(filepath.Separator)) { + if part == "" || part == "." { + continue + } + current = filepath.Join(current, part) + info, statErr := os.Lstat(current) + if errors.Is(statErr, os.ErrNotExist) { + if !create { + return statErr + } + if err := os.Mkdir(current, 0755); err != nil && !errors.Is(err, os.ErrExist) { + return err + } + info, statErr = os.Lstat(current) + } + if statErr != nil { + return statErr + } + if info.Mode()&os.ModeSymlink != 0 || !info.IsDir() { + return fmt.Errorf("cache component %q must be a non-symlink directory", current) + } + } + return nil +} + // writeCache persists the current SHA and clone directory to the cache file. func (s *Source) writeCache() error { + if err := s.ensureCacheLayout(true); err != nil { + return fmt.Errorf("prepare cache layout: %w", err) + } entry := cacheEntry{ SHA: s.cachedSHA, Dir: s.clonedDir, @@ -109,11 +189,25 @@ func (s *Source) writeCache() error { if err != nil { return fmt.Errorf("marshal cache entry: %w", err) } - if err := os.MkdirAll(filepath.Dir(s.cacheFilePath()), 0755); err != nil { - return fmt.Errorf("create cache dir: %w", err) + tmp, err := os.CreateTemp(filepath.Dir(s.cacheFilePath()), ".creed-cache-*") + if err != nil { + return fmt.Errorf("create cache temp file: %w", err) + } + tmpName := tmp.Name() + defer os.Remove(tmpName) + if err := tmp.Chmod(0644); err != nil { + tmp.Close() + return fmt.Errorf("chmod cache temp file: %w", err) + } + if _, err := tmp.Write(data); err != nil { + tmp.Close() + return fmt.Errorf("write cache temp file: %w", err) + } + if err := tmp.Close(); err != nil { + return fmt.Errorf("close cache temp file: %w", err) } - if err := os.WriteFile(s.cacheFilePath(), data, 0644); err != nil { - return fmt.Errorf("write cache file: %w", err) + if err := os.Rename(tmpName, s.cacheFilePath()); err != nil { + return fmt.Errorf("replace cache file: %w", err) } return nil } @@ -124,23 +218,37 @@ func (s *Source) InvalidateCache() error { s.mu.Lock() defer s.mu.Unlock() if s.cacheDir == "" { + s.clonedDir, s.cachedSHA, s.localSource, s.cloned = "", "", nil, false return nil } + if err := s.ensureCacheLayout(false); err != nil { + if errors.Is(err, os.ErrNotExist) { + s.clonedDir, s.cachedSHA, s.localSource, s.cloned = "", "", nil, false + return nil + } + return fmt.Errorf("validate cache layout: %w", err) + } if err := os.RemoveAll(s.clonePath()); err != nil { return fmt.Errorf("remove cached clone: %w", err) } if err := os.Remove(s.cacheFilePath()); err != nil && !errors.Is(err, os.ErrNotExist) { return fmt.Errorf("remove cache metadata: %w", err) } - s.clonedDir = "" - s.cachedSHA = "" - s.localSource = nil - s.cloned = false + s.clonedDir, s.cachedSHA, s.localSource, s.cloned = "", "", nil, false return nil } // readCache reads the cache entry for this remote, if it exists. func (s *Source) readCache() (*cacheEntry, error) { + if err := s.ensureCacheLayout(false); err != nil { + if errors.Is(err, os.ErrNotExist) { + return nil, fmt.Errorf("%w: cache metadata missing", errCacheMiss) + } + return nil, err + } + if info, err := os.Lstat(s.cacheFilePath()); err == nil && info.Mode()&os.ModeSymlink != 0 { + return nil, fmt.Errorf("%w: cache metadata is a symlink", errCacheMiss) + } data, err := os.ReadFile(s.cacheFilePath()) if err != nil { if errors.Is(err, os.ErrNotExist) { @@ -158,31 +266,12 @@ func (s *Source) readCache() (*cacheEntry, error) { // remoteHeadSHA queries the remote repository for the current HEAD commit SHA // without cloning. Uses go-git's ls-remote via an in-memory repository. func (s *Source) remoteHeadSHA(ctx context.Context) (string, error) { - repo, err := git.Init(memory.NewStorage(), nil) - if err != nil { - return "", fmt.Errorf("init temp repo for ls-remote: %w", err) - } - - remoteCfg := &config.RemoteConfig{ - Name: "origin", - URLs: []string{s.remoteURL}, - } - - remote, err := repo.CreateRemote(remoteCfg) - if err != nil { - return "", fmt.Errorf("create remote: %w", err) - } - - auth, err := s.authMethod() + refs, err := s.remoteRefs(ctx) if err != nil { return "", err } - - listOpts := &git.ListOptions{Auth: auth} - - refs, err := remote.ListContext(ctx, listOpts) - if err != nil { - return "", classifyGitError("list remote refs", s.remoteURL, err) + if s.ref != "" { + return resolveRemoteRef(refs, s.ref) } // Prefer the HEAD reference. In ls-remote output, HEAD may be a @@ -197,21 +286,75 @@ func (s *Source) remoteHeadSHA(ctx context.Context) (string, error) { } } - // Fall back to main branch. - for _, ref := range refs { - if ref.Name().IsBranch() && ref.Name().Short() == "main" { - return ref.Hash().String(), nil + for _, branch := range []string{"main", "master"} { + for _, ref := range refs { + if ref.Name().IsBranch() && ref.Name().Short() == branch { + return ref.Hash().String(), nil + } } } + return "", fmt.Errorf("no HEAD, master, or main reference found in remote") +} - // Fall back to master branch. - for _, ref := range refs { - if ref.Name().IsBranch() && ref.Name().Short() == "master" { - return ref.Hash().String(), nil - } +func (s *Source) remoteRefs(ctx context.Context) ([]*plumbing.Reference, error) { + repo, err := git.Init(memory.NewStorage(), nil) + if err != nil { + return nil, fmt.Errorf("init temp repo for ls-remote: %w", err) + } + remote, err := repo.CreateRemote(&config.RemoteConfig{Name: "origin", URLs: []string{s.remoteURL}}) + if err != nil { + return nil, fmt.Errorf("create remote: %w", err) + } + auth, err := s.authMethod() + if err != nil { + return nil, err + } + refs, err := remote.ListContext(ctx, &git.ListOptions{Auth: auth}) + if err != nil { + return nil, classifyGitError("list remote refs", s.remoteURL, err) } + return refs, nil +} - return "", fmt.Errorf("no HEAD, master, or main reference found in remote") +func resolveRemoteRef(refs []*plumbing.Reference, requested string) (string, error) { + sha, _, err := resolveRemoteRefForFetch(refs, requested) + return sha, err +} + +func resolveRemoteRefForFetch(refs []*plumbing.Reference, requested string) (string, string, error) { + if isCommitSHA(requested) { + // The fetch step verifies that this object is actually reachable from + // the remote; syntactic SHA validation alone is not sufficient. + return strings.ToLower(requested), requested, nil + } + names := []string{} + if strings.HasPrefix(requested, "refs/") { + names = append(names, requested) + } else { + // Prefer branches when a name is ambiguous, then tags. + names = append(names, plumbing.NewBranchReferenceName(requested).String(), plumbing.NewTagReferenceName(requested).String()) + } + for _, name := range names { + var direct *plumbing.Reference + for _, ref := range refs { + if ref.Name().String() == name { + direct = ref + break + } + } + if direct == nil { + continue + } + // Annotated tags may have a peeled ^{} ref. Prefer the commit hash. + peeledName := plumbing.ReferenceName(name + "^{}").String() + for _, ref := range refs { + if ref.Name().String() == peeledName { + return ref.Hash().String(), name, nil + } + } + return direct.Hash().String(), name, nil + } + return "", "", fmt.Errorf("reference %q not found in remote", requested) } // ensureCloned clones the repository on first access. Subsequent calls within @@ -259,28 +402,49 @@ func (s *Source) tryCache(ctx context.Context) error { return err } - // Check if the cached clone directory still exists. - if _, err := os.Stat(entry.Dir); err != nil { + // Cache metadata is untrusted. Only accept the clone path owned by this + // Source instance and reject symlinked/escaped cache directories. + expectedDir := filepath.Clean(s.clonePath()) + if filepath.Clean(entry.Dir) != expectedDir { + return fmt.Errorf("%w: cached clone path is outside the expected cache", errCacheMiss) + } + cacheRoot, err := filepath.EvalSymlinks(s.cacheDir) + if err != nil { + return fmt.Errorf("%w: cache root unavailable", errCacheMiss) + } + resolvedDir, err := filepath.EvalSymlinks(expectedDir) + if err != nil { if errors.Is(err, os.ErrNotExist) { return fmt.Errorf("%w: cached clone dir missing", errCacheMiss) } - return fmt.Errorf("stat cached clone dir: %w", err) + return fmt.Errorf("%w: cached clone cannot be resolved", errCacheMiss) } - - // Query remote HEAD to see if it has changed. - remoteSHA, err := s.remoteHeadSHA(ctx) - if err != nil { - return fmt.Errorf("cannot determine remote HEAD: %w", err) + if relative, relErr := filepath.Rel(cacheRoot, resolvedDir); relErr != nil || relative == ".." || strings.HasPrefix(relative, ".."+string(filepath.Separator)) { + return fmt.Errorf("%w: cached clone escapes cache root", errCacheMiss) + } + if info, statErr := os.Lstat(expectedDir); statErr != nil || info.Mode()&os.ModeSymlink != 0 || !info.IsDir() { + return fmt.Errorf("%w: cached clone must be a non-symlink directory", errCacheMiss) } - if remoteSHA != entry.SHA { - return fmt.Errorf("%w: remote HEAD changed (was %s, now %s)", errCacheMiss, shortSHA(entry.SHA), shortSHA(remoteSHA)) + if isCommitSHA(s.ref) { + if !strings.EqualFold(entry.SHA, s.ref) { + return fmt.Errorf("%w: cached SHA does not match pinned ref", errCacheMiss) + } + } else { + // Query remote HEAD or the mutable branch/tag ref to see if it changed. + remoteSHA, err := s.remoteHeadSHA(ctx) + if err != nil { + return fmt.Errorf("cannot determine remote HEAD: %w", err) + } + if !strings.EqualFold(remoteSHA, entry.SHA) { + return fmt.Errorf("%w: remote HEAD changed (was %s, now %s)", errCacheMiss, shortSHA(entry.SHA), shortSHA(remoteSHA)) + } } // Cache hit — reuse the existing clone directory. s.clonedDir = entry.Dir s.cachedSHA = entry.SHA - s.localSource = localfs.NewSource(entry.Dir) + s.localSource = localfs.NewSourceWithPath(entry.Dir, s.sourcePath) s.cloned = true return nil } @@ -296,10 +460,18 @@ func (s *Source) clone(ctx context.Context) error { if s.cacheDir != "" { // Persistent clone directory for caching. + if err := s.ensureCacheLayout(true); err != nil { + return fmt.Errorf("prepare cache layout: %w", err) + } cloneDir = s.clonePath() + if info, statErr := os.Lstat(cloneDir); statErr == nil && info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("cached clone path must not be a symlink") + } // Remove any stale clone from a previous run. - os.RemoveAll(cloneDir) - if err := os.MkdirAll(cloneDir, 0755); err != nil { + if err := os.RemoveAll(cloneDir); err != nil { + return fmt.Errorf("remove stale clone: %w", err) + } + if err := os.Mkdir(cloneDir, 0755); err != nil { return fmt.Errorf("create clone dir: %w", err) } } else { @@ -317,6 +489,24 @@ func (s *Source) clone(ctx context.Context) error { Depth: 1, Auth: auth, } + var checkoutSHA, fetchRef string + if s.ref != "" { + refs, refsErr := s.remoteRefs(ctx) + if refsErr != nil { + os.RemoveAll(cloneDir) + return refsErr + } + checkoutSHA, fetchRef, err = resolveRemoteRefForFetch(refs, s.ref) + if err != nil { + os.RemoveAll(cloneDir) + return err + } + // Do not rely on the default branch being the source of the pinned + // object. The explicit fetch below brings the requested ref/object in. + cloneOpts.Tags = git.AllTags + cloneOpts.Depth = 0 + cloneOpts.NoCheckout = true + } repo, err := git.PlainCloneContext(ctx, cloneDir, false, cloneOpts) if err != nil { @@ -325,6 +515,47 @@ func (s *Source) clone(ctx context.Context) error { return classifyGitError("clone repository", s.remoteURL, err) } + if s.ref != "" { + fetchOptions := &git.FetchOptions{ + RemoteName: "origin", + RefSpecs: []config.RefSpec{config.RefSpec("+" + fetchRef + ":refs/creed/pinned")}, + Depth: 0, + Auth: auth, + Tags: git.AllTags, + Force: true, + } + fetchErr := repo.FetchContext(ctx, fetchOptions) + if fetchErr != nil && !errors.Is(fetchErr, git.NoErrAlreadyUpToDate) { + // Some servers reject exact SHA refspecs even when the object is + // reachable. Fetch all heads/tags as a safe fallback. + fallback := &git.FetchOptions{ + RemoteName: "origin", + RefSpecs: []config.RefSpec{ + config.RefSpec("+refs/heads/*:refs/remotes/origin/*"), + config.RefSpec("+refs/tags/*:refs/tags/*"), + }, + Depth: 0, + Auth: auth, + Tags: git.AllTags, + Force: true, + } + if fallbackErr := repo.FetchContext(ctx, fallback); fallbackErr != nil && !errors.Is(fallbackErr, git.NoErrAlreadyUpToDate) { + os.RemoveAll(cloneDir) + return classifyGitError("fetch reference", s.remoteURL, fallbackErr) + } + } + worktree, err := repo.Worktree() + if err != nil { + os.RemoveAll(cloneDir) + return fmt.Errorf("open cloned worktree: %w", err) + } + checkout := &git.CheckoutOptions{Hash: plumbing.NewHash(checkoutSHA)} + if err := worktree.Checkout(checkout); err != nil { + os.RemoveAll(cloneDir) + return classifyGitError("checkout reference", s.remoteURL, err) + } + } + head, err := repo.Head() if err != nil { os.RemoveAll(cloneDir) @@ -333,7 +564,7 @@ func (s *Source) clone(ctx context.Context) error { s.clonedDir = cloneDir s.cachedSHA = head.Hash().String() - s.localSource = localfs.NewSource(cloneDir) + s.localSource = localfs.NewSourceWithPath(cloneDir, s.sourcePath) s.cloned = true return nil @@ -444,19 +675,32 @@ func (s *Source) authMethod() (transport.AuthMethod, error) { } method, err := ssh.NewSSHAgentAuth("git") if err != nil { - return nil, fmt.Errorf("SSH remote %q requires SSH auth: set SSH_AUTH_SOCK or CREED_GIT_SSH_KEY: %w", s.remoteURL, err) + return nil, fmt.Errorf("SSH remote %q requires SSH auth: set SSH_AUTH_SOCK or CREED_GIT_SSH_KEY: %w", sanitizeRemoteURL(s.remoteURL), err) } return method, nil } func isHTTPSRemote(remoteURL string) bool { - return strings.HasPrefix(remoteURL, "https://") + return strings.HasPrefix(strings.ToLower(strings.TrimSpace(remoteURL)), "https://") } func isSSHRemote(remoteURL string) bool { + remoteURL = strings.ToLower(strings.TrimSpace(remoteURL)) return strings.HasPrefix(remoteURL, "git@") || strings.HasPrefix(remoteURL, "ssh://") } +func isCommitSHA(ref string) bool { + if len(ref) != 40 { + return false + } + for _, r := range ref { + if !(r >= '0' && r <= '9') && !(r >= 'a' && r <= 'f') && !(r >= 'A' && r <= 'F') { + return false + } + } + return true +} + func classifyGitError(operation, remoteURL string, err error) error { if err == nil { return nil @@ -485,14 +729,20 @@ func sanitizeErrorMessage(message string) string { } func sanitizeRemoteURL(remoteURL string) string { - if !strings.HasPrefix(remoteURL, "https://") || !strings.Contains(remoteURL, "@") { - return remoteURL - } - parts := strings.SplitN(strings.TrimPrefix(remoteURL, "https://"), "@", 2) - if len(parts) != 2 { - return remoteURL + remoteURL = strings.TrimSpace(remoteURL) + parsed, err := url.Parse(remoteURL) + if err == nil && parsed.Scheme != "" { + parsed.User = nil + parsed.RawQuery = "" + parsed.Fragment = "" + return parsed.String() + } + if at := strings.LastIndex(remoteURL, "@"); at >= 0 { + if colon := strings.Index(remoteURL, ":"); colon > 0 && colon < at { + return remoteURL[at+1:] + } } - return "https://" + parts[1] + return remoteURL } // injectToken injects an authentication token into an HTTPS git URL. diff --git a/internal/adapters/gitremote/source_test.go b/internal/adapters/gitremote/source_test.go index 22ab8ea..4e9cd60 100644 --- a/internal/adapters/gitremote/source_test.go +++ b/internal/adapters/gitremote/source_test.go @@ -2,6 +2,7 @@ package gitremote import ( "context" + "encoding/json" "errors" "os" "os/exec" @@ -419,3 +420,140 @@ func TestClassifyGitErrorSanitizesRemoteURL(t *testing.T) { t.Fatalf("classified error did not label auth failure: %v", err) } } + +func TestGitRemotePinnedCommitCacheWorksOffline(t *testing.T) { + if testing.Short() { + t.Skip("skipping git integration test in short mode") + } + bareURL := createBareRepo(t) + output, err := exec.Command("git", "--git-dir", bareURL, "rev-parse", "HEAD").CombinedOutput() + if err != nil { + t.Fatalf("git rev-parse: %v: %s", err, output) + } + ref := strings.TrimSpace(string(output)) + cacheDir := t.TempDir() + options := SourceOptions{RemoteURL: bareURL, Ref: ref, CacheDir: cacheDir} + first := NewSourceWithOptions(options) + if _, err := first.ReadManifest(context.Background()); err != nil { + t.Fatalf("first pinned read: %v", err) + } + if first.CachedSHA() != ref { + t.Fatalf("first pinned SHA = %q, want %q", first.CachedSHA(), ref) + } + if err := os.RemoveAll(bareURL); err != nil { + t.Fatalf("remove remote: %v", err) + } + second := NewSourceWithOptions(options) + if _, err := second.ReadManifest(context.Background()); err != nil { + t.Fatalf("offline pinned cache read: %v", err) + } + if second.CloneCount() != 0 { + t.Fatalf("offline pinned cache cloned %d times, want 0", second.CloneCount()) + } +} + +func TestGitRemoteAnnotatedTagRef(t *testing.T) { + if testing.Short() { + t.Skip("skipping git integration test in short mode") + } + bareURL := createBareRepo(t) + workDir := t.TempDir() + commands := [][]string{ + {"git", "clone", bareURL, workDir}, + {"git", "-C", workDir, "config", "user.name", "Tag Test"}, + {"git", "-C", workDir, "config", "user.email", "tag@example.invalid"}, + {"git", "-C", workDir, "tag", "-a", "v1", "-m", "release"}, + {"git", "-C", workDir, "push", "origin", "refs/tags/v1"}, + } + for _, args := range commands { + if output, err := exec.Command(args[0], args[1:]...).CombinedOutput(); err != nil { + t.Fatalf("%s: %v\n%s", strings.Join(args, " "), err, output) + } + } + src := NewSourceWithOptions(SourceOptions{RemoteURL: bareURL, Ref: "v1"}) + if _, err := src.ReadManifest(context.Background()); err != nil { + t.Fatalf("annotated tag read: %v", err) + } + defer src.Cleanup() +} + +func TestGitRemoteRejectsEscapedCacheMetadata(t *testing.T) { + if testing.Short() { + t.Skip("skipping git integration test in short mode") + } + bareURL := createBareRepo(t) + cacheDir := t.TempDir() + src := NewSourceWithCache(bareURL, "", cacheDir) + external := t.TempDir() + if err := os.MkdirAll(filepath.Join(external, ".creed"), 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(external, ".creed", "manifest.yaml"), []byte("version: 1\nsource:\n type: local\n path: .creed\n"), 0644); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(src.cacheFilePath()), 0755); err != nil { + t.Fatal(err) + } + data, err := json.Marshal(cacheEntry{SHA: "0123456789012345678901234567890123456789", Dir: external}) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(src.cacheFilePath(), data, 0644); err != nil { + t.Fatal(err) + } + if err := os.RemoveAll(bareURL); err != nil { + t.Fatal(err) + } + if _, err := src.ReadManifest(context.Background()); err == nil { + t.Fatal("ReadManifest trusted cache metadata outside the configured cache") + } +} + +func TestGitRemoteNonDefaultBranchAndSHARefs(t *testing.T) { + if testing.Short() { + t.Skip("skipping git integration test in short mode") + } + bareURL := createBareRepo(t) + workDir := t.TempDir() + for _, args := range [][]string{ + {"git", "clone", bareURL, workDir}, + {"git", "-C", workDir, "config", "user.name", "Branch Test"}, + {"git", "-C", workDir, "config", "user.email", "branch@example.invalid"}, + {"git", "-C", workDir, "checkout", "-b", "feature-only"}, + } { + if output, err := exec.Command(args[0], args[1:]...).CombinedOutput(); err != nil { + t.Fatalf("%s: %v\n%s", strings.Join(args, " "), err, output) + } + } + featureSkill := filepath.Join(workDir, ".creed", "skills", "code-review.md") + if err := os.WriteFile(featureSkill, []byte("# Feature-only branch"), 0644); err != nil { + t.Fatal(err) + } + for _, args := range [][]string{ + {"git", "-C", workDir, "add", ".creed/skills/code-review.md"}, + {"git", "-C", workDir, "commit", "-m", "feature-only context"}, + {"git", "-C", workDir, "push", "origin", "HEAD:refs/heads/feature-only"}, + } { + if output, err := exec.Command(args[0], args[1:]...).CombinedOutput(); err != nil { + t.Fatalf("%s: %v\n%s", strings.Join(args, " "), err, output) + } + } + output, err := exec.Command("git", "-C", workDir, "rev-parse", "HEAD").CombinedOutput() + if err != nil { + t.Fatalf("git rev-parse: %v: %s", err, output) + } + sha := strings.TrimSpace(string(output)) + for _, ref := range []string{"feature-only", sha} { + src := NewSourceWithOptions(SourceOptions{RemoteURL: bareURL, Ref: ref}) + skill, readErr := src.ReadSkill(context.Background(), "code-review") + if readErr != nil { + t.Fatalf("read ref %q: %v", ref, readErr) + } + if string(skill.Content) != "# Feature-only branch" { + t.Fatalf("ref %q content = %q, want feature-only branch", ref, skill.Content) + } + if cleanupErr := src.Cleanup(); cleanupErr != nil { + t.Fatalf("cleanup ref %q: %v", ref, cleanupErr) + } + } +} diff --git a/internal/adapters/layered/source.go b/internal/adapters/layered/source.go new file mode 100644 index 0000000..d52c873 --- /dev/null +++ b/internal/adapters/layered/source.go @@ -0,0 +1,172 @@ +// Package layered composes ordered Creed source readers into one deterministic +// source. It keeps infrastructure-specific readers behind the SourceReader port. +package layered + +import ( + "context" + "fmt" + + "github.com/techgodhq/creed/internal/domain" + "github.com/techgodhq/creed/internal/ports" +) + +// Source reads several source layers as one composed SourceReader. Layers are +// ordered from shared context to repository context; later entries with the +// same name replace earlier entries while retaining deterministic order. +type Source struct { + layers []ports.SourceReader +} + +var _ ports.SourceReader = (*Source)(nil) + +// NewSource creates a layered source from one or more ordered readers. +func NewSource(layers ...ports.SourceReader) *Source { + copied := append([]ports.SourceReader(nil), layers...) + return &Source{layers: copied} +} + +type snapshot struct { + manifests []*domain.Manifest +} + +func (s *Source) readSnapshot(ctx context.Context) (*snapshot, error) { + if len(s.layers) == 0 { + return nil, fmt.Errorf("layered source requires at least one layer") + } + result := &snapshot{manifests: make([]*domain.Manifest, 0, len(s.layers))} + for i, layer := range s.layers { + if layer == nil { + return nil, fmt.Errorf("layered source layer %d is nil", i) + } + manifest, err := layer.ReadManifest(ctx) + if err != nil { + return nil, fmt.Errorf("read layer %d manifest: %w", i, err) + } + if manifest == nil { + return nil, fmt.Errorf("layer %d returned a nil manifest", i) + } + result.manifests = append(result.manifests, manifest) + } + return result, nil +} + +// ReadManifest returns the composed manifest. Targets come from the last +// layer that declares them, while skills and configs are merged in layer order. +func (s *Source) ReadManifest(ctx context.Context) (*domain.Manifest, error) { + state, err := s.readSnapshot(ctx) + if err != nil { + return nil, err + } + last := state.manifests[len(state.manifests)-1] + combined := &domain.Manifest{ + Version: last.Version, + Source: domain.SourceConfig{ + Type: "layered", + Path: last.Source.Path, + }, + Skills: mergeSkills(state.manifests), + Configs: mergeConfigs(state.manifests), + } + for i := len(state.manifests) - 1; i >= 0; i-- { + if state.manifests[i].Targets != nil { + combined.Targets = append([]domain.TargetConfig(nil), state.manifests[i].Targets...) + break + } + } + if combined.Version == 0 { + combined.Version = 1 + } + return combined, nil +} + +func mergeSkills(manifests []*domain.Manifest) []domain.SkillEntry { + merged := []domain.SkillEntry{} + positions := map[string]int{} + for _, manifest := range manifests { + for _, entry := range manifest.Skills { + if position, ok := positions[entry.Name]; ok { + merged[position] = entry + continue + } + positions[entry.Name] = len(merged) + merged = append(merged, entry) + } + } + return merged +} + +func mergeConfigs(manifests []*domain.Manifest) []domain.ConfigEntry { + merged := []domain.ConfigEntry{} + positions := map[string]int{} + for _, manifest := range manifests { + for _, entry := range manifest.Configs { + if position, ok := positions[entry.Name]; ok { + merged[position] = entry + continue + } + positions[entry.Name] = len(merged) + merged = append(merged, entry) + } + } + return merged +} + +// ReadSkill reads the last declaration of a named skill, allowing repository +// context to override a shared skill with the same name. +func (s *Source) ReadSkill(ctx context.Context, name string) (*domain.Skill, error) { + state, err := s.readSnapshot(ctx) + if err != nil { + return nil, err + } + for i := len(state.manifests) - 1; i >= 0; i-- { + for j := len(state.manifests[i].Skills) - 1; j >= 0; j-- { + if state.manifests[i].Skills[j].Name == name { + return s.layers[i].ReadSkill(ctx, name) + } + } + } + return nil, fmt.Errorf("skill not found in layered source: %s", name) +} + +// ListSkills returns the composed skill declarations. +func (s *Source) ListSkills(ctx context.Context) ([]domain.SkillInfo, error) { + manifest, err := s.ReadManifest(ctx) + if err != nil { + return nil, err + } + result := make([]domain.SkillInfo, 0, len(manifest.Skills)) + for _, entry := range manifest.Skills { + result = append(result, domain.SkillInfo(entry)) + } + return result, nil +} + +// ReadConfig reads the last declaration of a named config, allowing repository +// context to override a shared config with the same name. +func (s *Source) ReadConfig(ctx context.Context, name string) (*domain.ConfigFile, error) { + state, err := s.readSnapshot(ctx) + if err != nil { + return nil, err + } + for i := len(state.manifests) - 1; i >= 0; i-- { + for j := len(state.manifests[i].Configs) - 1; j >= 0; j-- { + if state.manifests[i].Configs[j].Name == name { + return s.layers[i].ReadConfig(ctx, name) + } + } + } + return nil, fmt.Errorf("config not found in layered source: %s", name) +} + +// ListConfigs returns the composed config declarations. +func (s *Source) ListConfigs(ctx context.Context) ([]domain.ConfigInfo, error) { + manifest, err := s.ReadManifest(ctx) + if err != nil { + return nil, err + } + result := make([]domain.ConfigInfo, 0, len(manifest.Configs)) + for _, entry := range manifest.Configs { + result = append(result, domain.ConfigInfo(entry)) + } + return result, nil +} diff --git a/internal/adapters/localfs/emitter.go b/internal/adapters/localfs/emitter.go index 6e54fb1..718bd1b 100644 --- a/internal/adapters/localfs/emitter.go +++ b/internal/adapters/localfs/emitter.go @@ -38,6 +38,11 @@ func NewEmitter(baseDir string) *Emitter { // Partial failures do not abort the remaining files. func (e *Emitter) Emit(ctx context.Context, target domain.Target, files []ports.EmittedFile) ([]ports.EmitResult, error) { results := make([]ports.EmitResult, 0, len(files)) + for _, f := range files { + if _, err := e.safeOutputPath(f.Path); err != nil { + return nil, fmt.Errorf("validate output %q: %w", f.Path, err) + } + } for _, f := range files { result := e.emitFile(f) @@ -185,7 +190,7 @@ func (e *Emitter) safeOutputPath(relPath string) (string, error) { current = filepath.Join(current, part) info, err := os.Lstat(current) if os.IsNotExist(err) { - return current, nil + return filepath.Join(e.baseDir, filepath.FromSlash(clean)), nil } if err != nil { return "", err @@ -199,7 +204,10 @@ func (e *Emitter) safeOutputPath(relPath string) (string, error) { // emitFile writes a single file atomically, returning the result. func (e *Emitter) emitFile(f ports.EmittedFile) ports.EmitResult { - fullPath := filepath.Join(e.baseDir, f.Path) + fullPath, pathErr := e.safeOutputPath(f.Path) + if pathErr != nil { + return ports.EmitResult{Path: f.Path, Status: ports.EmitStatusError, Error: pathErr} + } // Check if the file already exists with identical content. existing, err := os.ReadFile(fullPath) @@ -282,7 +290,10 @@ func (e *Emitter) emitFile(f ports.EmittedFile) ports.EmitResult { func (e *Emitter) Preview(_ context.Context, _ domain.Target, files []ports.EmittedFile) ([]ports.EmitResult, error) { results := make([]ports.EmitResult, 0, len(files)) for _, f := range files { - fullPath := filepath.Join(e.baseDir, f.Path) + fullPath, err := e.safeOutputPath(f.Path) + if err != nil { + return nil, fmt.Errorf("validate output %q: %w", f.Path, err) + } existing, err := os.ReadFile(fullPath) if err == nil && bytes.Equal(existing, f.Content) { results = append(results, ports.EmitResult{Path: f.Path, Status: ports.EmitStatusSkipped}) @@ -346,7 +357,10 @@ func (e *Emitter) Clean(ctx context.Context, target domain.Target) error { return nil } for _, relPath := range target.EmitPaths("") { - fullPath := filepath.Join(e.baseDir, relPath) + fullPath, err := e.safeOutputPath(relPath) + if err != nil { + return fmt.Errorf("clean %s: %w", relPath, err) + } // RemoveAll handles both files and directories gracefully. if err := os.RemoveAll(fullPath); err != nil { return fmt.Errorf("clean %s: %w", relPath, err) diff --git a/internal/adapters/localfs/source.go b/internal/adapters/localfs/source.go index c5d2ff0..f4c52d6 100644 --- a/internal/adapters/localfs/source.go +++ b/internal/adapters/localfs/source.go @@ -5,8 +5,12 @@ package localfs import ( "context" "fmt" + "io" "os" "path/filepath" + "runtime" + "strconv" + "strings" "gopkg.in/yaml.v3" @@ -24,10 +28,21 @@ type manifestFile struct { Configs []domain.ConfigEntry `yaml:"config"` } -type sourceConfigYAML struct { +type sourceLayerYAML struct { + Name string `yaml:"name"` Type string `yaml:"type"` Path string `yaml:"path"` Remote string `yaml:"remote"` + Ref string `yaml:"ref"` +} + +type sourceConfigYAML struct { + Type string `yaml:"type"` + Path string `yaml:"path"` + Remote string `yaml:"remote"` + Ref string `yaml:"ref"` + Layers []sourceLayerYAML `yaml:"layers"` + Overlays []sourceLayerYAML `yaml:"overlays"` } type targetConfigYAML struct { @@ -39,8 +54,14 @@ type targetConfigYAML struct { // Source reads creed data from a local filesystem directory. // It implements ports.SourceReader. type Source struct { - // creedDir is the absolute path to the .creed/ directory. + // rootDir is the project/clone root used to contain the source directory. + rootDir string + // creedDir is the absolute path to the source directory. creedDir string + // pathErr records an invalid configured source path for deferred reporting + // from the SourceReader methods (constructors intentionally do not return + // errors to preserve the adapter's existing API). + pathErr error } // Compile-time assertion that Source implements ports.SourceReader. @@ -49,26 +70,44 @@ var _ ports.SourceReader = (*Source)(nil) // NewSource creates a LocalFS source reader for the given project root. // The creed directory is resolved as root/.creed. func NewSource(root string) *Source { - return &Source{ - creedDir: filepath.Join(root, ".creed"), + return NewSourceWithPath(root, ".creed") +} + +// NewSourceWithPath creates a LocalFS source reader rooted at a directory +// relative to root. Invalid paths are reported by the first read operation. +func NewSourceWithPath(root, sourcePath string) *Source { + if strings.TrimSpace(sourcePath) == "" { + sourcePath = ".creed" } + clean, err := safeRelativePath(sourcePath) + if err != nil { + return &Source{pathErr: fmt.Errorf("invalid source path %q: %w", sourcePath, err)} + } + return &Source{rootDir: root, creedDir: filepath.Join(root, clean)} } -// newSourceWithDir creates a LocalFS source reader with an explicit creed directory. +// newSourceWithDir creates a LocalFS source reader with an explicit source directory. // Used internally and by GitRemote to read from a cloned repo. // //nolint:unused // consumed by gitremote adapter in stacked PR #2 func newSourceWithDir(creedDir string) *Source { - return &Source{ - creedDir: creedDir, - } + return &Source{rootDir: creedDir, creedDir: creedDir} } // ReadManifest reads and parses the manifest.yaml from the .creed/ directory. func (s *Source) ReadManifest(ctx context.Context) (*domain.Manifest, error) { - manifestPath := filepath.Join(s.creedDir, "manifest.yaml") + if err := ctx.Err(); err != nil { + return nil, err + } + if err := s.ready(); err != nil { + return nil, err + } + manifestPath, err := s.sourceFilePath("manifest.yaml") + if err != nil { + return nil, fmt.Errorf("invalid manifest path: %w", err) + } - data, err := os.ReadFile(manifestPath) + data, err := readContainedFile(s.rootDir, manifestPath) if err != nil { if os.IsNotExist(err) { return nil, fmt.Errorf("manifest not found at %s", manifestPath) @@ -81,13 +120,24 @@ func (s *Source) ReadManifest(ctx context.Context) (*domain.Manifest, error) { return nil, fmt.Errorf("failed to parse manifest: %w", err) } + source := domain.SourceConfig{ + Type: mf.Source.Type, + Path: mf.Source.Path, + Remote: mf.Source.Remote, + Ref: mf.Source.Ref, + } + for _, layer := range append(append([]sourceLayerYAML{}, mf.Source.Layers...), mf.Source.Overlays...) { + source.Layers = append(source.Layers, domain.SourceLayer{ + Name: layer.Name, + Type: layer.Type, + Path: layer.Path, + Remote: layer.Remote, + Ref: layer.Ref, + }) + } m := &domain.Manifest{ Version: mf.Version, - Source: domain.SourceConfig{ - Type: mf.Source.Type, - Path: mf.Source.Path, - Remote: mf.Source.Remote, - }, + Source: source, Skills: mf.Skills, Configs: mf.Configs, } @@ -108,6 +158,19 @@ func (s *Source) ReadManifest(ctx context.Context) (*domain.Manifest, error) { return m, nil } +// ReadManifestBytes reads the manifest through the same contained-file +// boundary as normal source reads. It is intended for strict schema validators. +func (s *Source) ReadManifestBytes(ctx context.Context) ([]byte, error) { + if err := ctx.Err(); err != nil { + return nil, err + } + manifestPath, err := s.sourceFilePath("manifest.yaml") + if err != nil { + return nil, err + } + return readContainedFile(s.rootDir, manifestPath) +} + // ReadSkill reads a skill's full content by name. func (s *Source) ReadSkill(ctx context.Context, name string) (*domain.Skill, error) { manifest, err := s.ReadManifest(ctx) @@ -115,10 +178,14 @@ func (s *Source) ReadSkill(ctx context.Context, name string) (*domain.Skill, err return nil, err } - for _, entry := range manifest.Skills { + for i := len(manifest.Skills) - 1; i >= 0; i-- { + entry := manifest.Skills[i] if entry.Name == name { - skillPath := filepath.Join(s.creedDir, entry.Path) - content, err := os.ReadFile(skillPath) + skillPath, err := s.sourceFilePath(entry.Path) + if err != nil { + return nil, fmt.Errorf("invalid skill path %s: %w", entry.Path, err) + } + content, err := readContainedFile(s.rootDir, skillPath) if err != nil { return nil, fmt.Errorf("failed to read skill file %s: %w", skillPath, err) } @@ -154,10 +221,14 @@ func (s *Source) ReadConfig(ctx context.Context, name string) (*domain.ConfigFil return nil, err } - for _, entry := range manifest.Configs { + for i := len(manifest.Configs) - 1; i >= 0; i-- { + entry := manifest.Configs[i] if entry.Name == name { - configPath := filepath.Join(s.creedDir, entry.Path) - content, err := os.ReadFile(configPath) + configPath, err := s.sourceFilePath(entry.Path) + if err != nil { + return nil, fmt.Errorf("invalid config path %s: %w", entry.Path, err) + } + content, err := readContainedFile(s.rootDir, configPath) if err != nil { return nil, fmt.Errorf("failed to read config file %s: %w", configPath, err) } @@ -185,3 +256,174 @@ func (s *Source) ListConfigs(ctx context.Context) ([]domain.ConfigInfo, error) { } return configs, nil } + +func readContainedFile(rootDir, path string) ([]byte, error) { + file, err := os.Open(path) + if err != nil { + return nil, err + } + defer file.Close() + info, err := file.Stat() + if err != nil { + return nil, err + } + if !info.Mode().IsRegular() { + return nil, fmt.Errorf("source path must be a regular file") + } + if runtime.GOOS == "linux" { + fdPath := filepath.Join("/proc/self/fd", strconv.FormatUint(uint64(file.Fd()), 10)) + resolvedFD, err := os.Readlink(fdPath) + if err != nil { + return nil, fmt.Errorf("resolve opened source file: %w", err) + } + if strings.HasSuffix(resolvedFD, " (deleted)") { + return nil, fmt.Errorf("opened source file was deleted") + } + resolvedRoot, err := filepath.EvalSymlinks(rootDir) + if err != nil { + return nil, err + } + resolvedRoot, err = filepath.Abs(resolvedRoot) + if err != nil { + return nil, err + } + resolvedPath, err := filepath.EvalSymlinks(resolvedFD) + if err != nil { + return nil, err + } + requestedPath, err := filepath.EvalSymlinks(path) + if err != nil { + return nil, err + } + resolvedPath, err = filepath.Abs(resolvedPath) + if err != nil { + return nil, err + } + requestedPath, err = filepath.Abs(requestedPath) + if err != nil { + return nil, err + } + if filepath.Clean(resolvedPath) != filepath.Clean(requestedPath) { + return nil, fmt.Errorf("source file changed through a symlink during open") + } + relative, err := filepath.Rel(resolvedRoot, resolvedPath) + if err != nil || relative == ".." || strings.HasPrefix(relative, ".."+string(filepath.Separator)) { + return nil, fmt.Errorf("opened source file escaped source root") + } + } + return io.ReadAll(file) +} + +func (s *Source) ready() error { + if s.pathErr != nil { + return s.pathErr + } + if s.creedDir == "" { + return fmt.Errorf("source directory is not configured") + } + return nil +} + +func (s *Source) sourceFilePath(sourcePath string) (string, error) { + if err := s.ready(); err != nil { + return "", err + } + clean, err := safeRelativePath(sourcePath) + if err != nil { + return "", err + } + path := filepath.Join(s.creedDir, clean) + rootInfo, err := os.Lstat(s.rootDir) + if err != nil { + if os.IsNotExist(err) { + return path, nil + } + return "", err + } + if rootInfo.Mode()&os.ModeSymlink != 0 || !rootInfo.IsDir() { + return "", fmt.Errorf("source root must be a non-symlink directory") + } + + // Reject symlink components between the project/clone root and the + // configured source directory, not just symlinks in the declared file path. + relativeSource, err := filepath.Rel(s.rootDir, s.creedDir) + if err != nil || relativeSource == ".." || strings.HasPrefix(relativeSource, ".."+string(filepath.Separator)) { + return "", fmt.Errorf("source directory must remain inside source root") + } + current := s.rootDir + if relativeSource != "." { + for _, part := range strings.Split(relativeSource, string(filepath.Separator)) { + current = filepath.Join(current, part) + info, statErr := os.Lstat(current) + if os.IsNotExist(statErr) { + return path, nil + } + if statErr != nil { + return "", statErr + } + if info.Mode()&os.ModeSymlink != 0 { + return "", fmt.Errorf("source directory %q traverses a symlink", s.creedDir) + } + } + } + + sourceInfo, err := os.Lstat(s.creedDir) + if os.IsNotExist(err) { + return path, nil + } + if err != nil { + return "", err + } + if sourceInfo.Mode()&os.ModeSymlink != 0 || !sourceInfo.IsDir() { + return "", fmt.Errorf("source directory must be a non-symlink directory") + } + current = s.creedDir + for _, part := range strings.Split(clean, string(filepath.Separator)) { + current = filepath.Join(current, part) + info, statErr := os.Lstat(current) + if os.IsNotExist(statErr) { + return path, nil + } + if statErr != nil { + return "", statErr + } + if info.Mode()&os.ModeSymlink != 0 { + return "", fmt.Errorf("path %q traverses a symlink", sourcePath) + } + } + + resolvedRoot, err := filepath.EvalSymlinks(s.rootDir) + if err != nil { + return "", err + } + resolvedSourceRoot, err := filepath.EvalSymlinks(s.creedDir) + if err != nil { + return "", err + } + if relative, relErr := filepath.Rel(resolvedRoot, resolvedSourceRoot); relErr != nil || relative == ".." || strings.HasPrefix(relative, ".."+string(filepath.Separator)) { + return "", fmt.Errorf("source directory must remain inside source root") + } + resolvedPath, err := filepath.EvalSymlinks(path) + if err != nil { + return "", err + } + if relative, relErr := filepath.Rel(resolvedSourceRoot, resolvedPath); relErr != nil || relative == ".." || strings.HasPrefix(relative, ".."+string(filepath.Separator)) { + return "", fmt.Errorf("path must remain inside source directory") + } + return path, nil +} + +func safeRelativePath(sourcePath string) (string, error) { + if strings.TrimSpace(sourcePath) == "" { + return "", fmt.Errorf("path is required") + } + path := filepath.FromSlash(sourcePath) + if filepath.IsAbs(path) || filepath.VolumeName(path) != "" { + return "", fmt.Errorf("path must be relative") + } + clean := filepath.Clean(path) + if clean == "." || clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator)) { + return "", fmt.Errorf("path must remain inside source directory") + } + return clean, nil +} diff --git a/internal/adapters/localfs/source_test.go b/internal/adapters/localfs/source_test.go index 2e20ca2..ed4d02d 100644 --- a/internal/adapters/localfs/source_test.go +++ b/internal/adapters/localfs/source_test.go @@ -202,3 +202,51 @@ func TestListConfigs(t *testing.T) { t.Errorf("expected configs[0].Name == \"project-context\", got %q", configs[0].Name) } } + +func TestReadConfigRejectsSymlinkEscape(t *testing.T) { + root := createTestProject(t) + external := filepath.Join(t.TempDir(), "outside.md") + if err := os.WriteFile(external, []byte("secret"), 0644); err != nil { + t.Fatal(err) + } + link := filepath.Join(root, ".creed", "config", "leak.md") + if err := os.Symlink(external, link); err != nil { + t.Fatal(err) + } + manifestPath := filepath.Join(root, ".creed", "manifest.yaml") + manifest := mustReadLocal(t, manifestPath) + manifest += " - name: leak\n path: config/leak.md\n" + if err := os.WriteFile(manifestPath, []byte(manifest), 0644); err != nil { + t.Fatal(err) + } + if _, err := NewSource(root).ReadConfig(context.Background(), "leak"); err == nil { + t.Fatal("ReadConfig followed a symlink outside the source directory") + } +} + +func mustReadLocal(t *testing.T, path string) string { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + return string(data) +} + +func TestReadManifestRejectsAncestorSymlinkSourcePath(t *testing.T) { + root := t.TempDir() + outside := t.TempDir() + if err := os.MkdirAll(filepath.Join(outside, ".creed"), 0755); err != nil { + t.Fatal(err) + } + manifest := "version: 1\nsource:\n type: local\n path: .creed\n" + if err := os.WriteFile(filepath.Join(outside, ".creed", "manifest.yaml"), []byte(manifest), 0644); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, filepath.Join(root, "link")); err != nil { + t.Fatal(err) + } + if _, err := NewSourceWithPath(root, "link/.creed").ReadManifest(context.Background()); err == nil { + t.Fatal("ReadManifest traversed an ancestor symlink") + } +} diff --git a/internal/domain/types.go b/internal/domain/types.go index d756ed1..7ff9c40 100644 --- a/internal/domain/types.go +++ b/internal/domain/types.go @@ -111,14 +111,36 @@ type TargetInfo struct { Outputs []TargetOutput } +// SourceLayer describes one ordered context layer in a layered source. +// Layers are read in declaration order and the project's local layer is read +// last, so repository-specific context follows the shared context. +type SourceLayer struct { + // Name is a stable human-readable layer identifier. + Name string + // Type is the layer backend type: "local" or "git". + Type string + // Path is the source directory relative to the layer root. + Path string + // Remote is the git clone URL for a git layer. + Remote string + // Ref optionally pins the git layer to a branch, tag, or commit SHA. + Ref string +} + // SourceConfig configures the source backend for reading creed data. type SourceConfig struct { - // Type is the source backend type: "local" or "git". + // Type is the source backend type: "local", "git", or "layered". + // A local source may also declare Layers for backwards-compatible overlay + // configuration; layered is the canonical type when overlays are present. Type string - // Path is the directory path (for local source, typically ".creed"). + // Path is the source directory path (for local source, typically ".creed"). Path string - // Remote is the git clone URL (for git source). Empty for local. + // Remote is the git clone URL (for a direct git source). Empty for local. Remote string + // Ref optionally pins a direct git source to a branch, tag, or commit SHA. + Ref string + // Layers are ordered shared/overlay sources read before the local project. + Layers []SourceLayer } // TargetConfig represents a target entry in the manifest. diff --git a/internal/integration/github_auth_test.go b/internal/integration/github_auth_test.go new file mode 100644 index 0000000..0317005 --- /dev/null +++ b/internal/integration/github_auth_test.go @@ -0,0 +1,74 @@ +package integration + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/techgodhq/creed/internal/service" +) + +// TestLayeredGitHubAuthIntegration is opt-in because it requires credentials +// owned by the CI/user environment. It exercises the complete layered service +// path rather than only testing the adapter's auth-method type. +func TestLayeredGitHubAuthIntegration(t *testing.T) { + if os.Getenv("CREED_RUN_GITHUB_AUTH_INTEGRATION") != "1" { + t.Skip("set CREED_RUN_GITHUB_AUTH_INTEGRATION=1 for GitHub auth integration") + } + + httpsRemote := strings.TrimSpace(os.Getenv("CREED_GITHUB_HTTPS_REMOTE")) + httpsToken := os.Getenv("CREED_GITHUB_HTTPS_TOKEN") + sshRemote := strings.TrimSpace(os.Getenv("CREED_GITHUB_SSH_REMOTE")) + if httpsRemote == "" && sshRemote == "" { + t.Fatal("set CREED_GITHUB_HTTPS_REMOTE and/or CREED_GITHUB_SSH_REMOTE") + } + if httpsRemote != "" { + if !strings.HasPrefix(strings.ToLower(httpsRemote), "https://github.com/") { + t.Fatal("CREED_GITHUB_HTTPS_REMOTE must point to github.com over HTTPS") + } + if httpsToken == "" { + t.Fatal("CREED_GITHUB_HTTPS_TOKEN is required for the HTTPS integration") + } + runLayeredGitHubValidation(t, httpsRemote, httpsToken) + } + if sshRemote != "" { + lower := strings.ToLower(sshRemote) + if !strings.HasPrefix(lower, "git@github.com:") && !strings.HasPrefix(lower, "ssh://git@github.com/") { + t.Fatal("CREED_GITHUB_SSH_REMOTE must point to github.com over SSH") + } + runLayeredGitHubValidation(t, sshRemote, "") + } +} + +func runLayeredGitHubValidation(t *testing.T, remote, token string) { + t.Helper() + root := t.TempDir() + manifest := fmt.Sprintf(`version: 1 +source: + type: layered + path: .creed + layers: + - name: org + type: git + remote: %q + path: .creed +targets: [] +`, remote) + creedDir := filepath.Join(root, ".creed") + if err := os.MkdirAll(creedDir, 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(creedDir, "manifest.yaml"), []byte(manifest), 0644); err != nil { + t.Fatal(err) + } + result, err := service.New(root, service.WithGitToken(token), service.WithCacheDir(filepath.Join(t.TempDir(), "cache"))).Validate(context.Background()) + if err != nil { + t.Fatalf("layered GitHub validation returned error: %v", err) + } + if !result.Valid { + t.Fatalf("layered GitHub validation failed: %#v", result.Errors) + } +} diff --git a/internal/service/doctor.go b/internal/service/doctor.go index 8db613f..f94a956 100644 --- a/internal/service/doctor.go +++ b/internal/service/doctor.go @@ -2,6 +2,7 @@ package service import ( "context" + "fmt" "net/url" "os" "os/exec" @@ -116,9 +117,21 @@ func (s *Implementation) Doctor(ctx context.Context) (DoctorReport, error) { } // --- Source type / remote (from manifest, best-effort) --- + requiresGit := false if manifest, err := s.readManifest(); err == nil { report.SourceType = manifest.Source.Type - report.SourceRemote = redactRemoteURL(manifest.Source.Remote) + requiresGit = manifest.Source.Type == "git" + remotes := []string{} + if manifest.Source.Remote != "" { + remotes = append(remotes, redactRemoteURL(manifest.Source.Remote)) + } + for _, layer := range manifest.Source.Layers { + requiresGit = requiresGit || layer.Type == "git" + if layer.Remote != "" { + remotes = append(remotes, redactRemoteURL(layer.Remote)) + } + } + report.SourceRemote = strings.Join(remotes, ", ") } // --- Canonical validation --- @@ -173,7 +186,7 @@ func (s *Implementation) Doctor(ctx context.Context) (DoctorReport, error) { Detail: gitPath, }) } else { - if report.SourceType == "git" { + if requiresGit { report.Checks = append(report.Checks, DoctorCheck{ Kind: "error", Code: "git_missing_for_remote_source", @@ -220,20 +233,46 @@ func (s *Implementation) resolveRoot() string { // parsed, it falls back to a conservative split-based approach that removes // anything between the scheme and the last @ before the host. func redactRemoteURL(remote string) string { + remote = strings.TrimSpace(remote) if remote == "" { return "" } - // SSH-style URLs (git@host:path) have no URL userinfo; safe to return. - if !strings.HasPrefix(remote, "https://") && !strings.HasPrefix(remote, "http://") { - return remote + parsed, err := url.Parse(remote) + if err == nil && parsed.Scheme != "" { + // Drop all userinfo. Usernames can themselves be bearer tokens. + parsed.User = nil + parsed.RawQuery = "" + parsed.Fragment = "" + return parsed.String() + } + // scp-style SSH remotes have no URL scheme. If parsing failed but the + // value still contains userinfo, strip everything through the last @. + if at := strings.LastIndex(remote, "@"); at >= 0 { + if colon := strings.Index(remote, ":"); colon > 0 && colon < at { + return remote[at+1:] + } + } + return remote +} + +func normalizePullRemoteURL(remote string) (string, error) { + remote = strings.TrimSpace(remote) + if remote == "" { + return remote, nil } parsed, err := url.Parse(remote) - if err != nil || parsed.User == nil { - return remote + if err != nil { + return "", fmt.Errorf("invalid remote URL: %w", err) + } + if parsed.User != nil { + return "", fmt.Errorf("remote URL must not contain embedded credentials; configure HTTPS/SSH authentication separately") + } + if parsed.Scheme == "" { + // scp-style SSH remotes (git@host:path) are intentionally accepted. + return remote, nil } - // Preserve the username (safe to display), drop the password. - if _, hasPassword := parsed.User.Password(); hasPassword { - parsed.User = url.User(parsed.User.Username()) + if parsed.RawQuery != "" || parsed.Fragment != "" { + return "", fmt.Errorf("remote URL must not contain a query or fragment") } - return parsed.String() + return parsed.String(), nil } diff --git a/internal/service/impl.go b/internal/service/impl.go index 5e7c907..6f2c247 100644 --- a/internal/service/impl.go +++ b/internal/service/impl.go @@ -117,15 +117,25 @@ func (s *Implementation) Init(ctx context.Context, projectName string) error { return s.writeManifest(manifest) } -// Sync syncs local Creed context from .creed/ to configured targets. +// Sync syncs the resolved Creed source context to configured targets. func (s *Implementation) Sync(ctx context.Context, opts usecase.SyncOptions) (*usecase.SyncResult, error) { - engine := usecase.NewSyncEngine(localfs.NewSource(s.root), localfs.NewEmitter(s.root)) + source, err := s.openSource(ctx) + if err != nil { + return nil, err + } + defer source.close() + engine := usecase.NewSyncEngine(source.reader, localfs.NewEmitter(s.root)) return engine.Sync(ctx, opts) } -// Diff compares rendered local Creed context with its target outputs. +// Diff compares rendered resolved Creed context with its target outputs. func (s *Implementation) Diff(ctx context.Context, opts usecase.DiffOptions) (*usecase.DiffResult, error) { - engine := usecase.NewSyncEngine(localfs.NewSource(s.root), localfs.NewEmitter(s.root)) + source, err := s.openSource(ctx) + if err != nil { + return nil, err + } + defer source.close() + engine := usecase.NewSyncEngine(source.reader, localfs.NewEmitter(s.root)) return engine.Diff(ctx, opts) } @@ -173,9 +183,14 @@ func (s *Implementation) RemoveSkill(ctx context.Context, name string) error { return fmt.Errorf("skill not found: %s", name) } -// ListSkills lists all manifest-registered skills. +// ListSkills lists all skills in the resolved source, including shared layers. func (s *Implementation) ListSkills(ctx context.Context) ([]domain.SkillInfo, error) { - return localfs.NewSource(s.root).ListSkills(ctx) + source, err := s.openSource(ctx) + if err != nil { + return nil, err + } + defer source.close() + return source.reader.ListSkills(ctx) } // AddConfig registers a configuration file path in the manifest. @@ -222,9 +237,14 @@ func (s *Implementation) RemoveConfig(ctx context.Context, name string) error { return fmt.Errorf("config not found: %s", name) } -// ListConfigs lists all manifest-registered configuration files. +// ListConfigs lists all configuration files in the resolved source, including shared layers. func (s *Implementation) ListConfigs(ctx context.Context) ([]domain.ConfigInfo, error) { - return localfs.NewSource(s.root).ListConfigs(ctx) + source, err := s.openSource(ctx) + if err != nil { + return nil, err + } + defer source.close() + return source.reader.ListConfigs(ctx) } // ListTargets lists all known targets and annotates them with manifest state. @@ -236,6 +256,18 @@ func (s *Implementation) ListTargets(ctx context.Context) ([]domain.TargetInfo, if err != nil { return nil, err } + if manifest.Source.Type == "git" || manifest.Source.Type == "layered" || len(manifest.Source.Layers) > 0 { + source, sourceErr := s.openSource(ctx) + if sourceErr != nil { + return nil, sourceErr + } + defer source.close() + resolved, resolveErr := source.reader.ReadManifest(ctx) + if resolveErr != nil { + return nil, resolveErr + } + manifest = resolved + } configured := make(map[string]domain.TargetConfig, len(manifest.Targets)) for _, tc := range manifest.Targets { configured[tc.Name] = tc @@ -280,28 +312,38 @@ func (s *Implementation) DisableTarget(ctx context.Context, name string) error { return s.setTargetEnabled(ctx, name, false) } -// Pull reads Creed context from a git remote and syncs it into this service's -// root using the same SyncEngine path as local sync. +// Pull records a shared git layer and syncs the composed source into this +// project's targets. It never replaces the local .creed source files. func (s *Implementation) Pull(ctx context.Context, remoteURL string) error { - if remoteURL == "" { - manifest, err := s.readManifest() - if err != nil { + if err := ctx.Err(); err != nil { + return err + } + if remoteURL != "" { + normalizedRemote, normalizeErr := normalizePullRemoteURL(remoteURL) + if normalizeErr != nil { + return normalizeErr + } + if err := s.ensureLayeredManifest(ctx, normalizedRemote); err != nil { return err } - remoteURL = manifest.Source.Remote - } - if remoteURL == "" { - return fmt.Errorf("remote URL is required") - } - source := gitremote.NewSource(remoteURL, s.token) - if s.cacheDir != "" { - source = gitremote.NewSourceWithCache(remoteURL, s.token, s.cacheDir) } else { - defer func() { - _ = source.Cleanup() - }() + manifest, manifestErr := s.readManifest() + if manifestErr != nil { + return manifestErr + } + if manifest.Source.Type != "git" && manifest.Source.Type != "layered" && len(manifest.Source.Layers) == 0 { + return fmt.Errorf("remote URL is required for pull") + } + if manifest.Source.Type == "git" && strings.TrimSpace(manifest.Source.Remote) == "" { + return fmt.Errorf("remote URL is required for pull") + } + } + source, err := s.openSource(ctx) + if err != nil { + return err } - engine := usecase.NewSyncEngine(source, localfs.NewEmitter(s.root)) + defer source.close() + engine := usecase.NewSyncEngine(source.reader, localfs.NewEmitter(s.root)) result, err := engine.Sync(ctx, usecase.SyncOptions{}) if err != nil { return err @@ -312,6 +354,67 @@ func (s *Implementation) Pull(ctx context.Context, remoteURL string) error { return nil } +func (s *Implementation) ensureLayeredManifest(ctx context.Context, remoteURL string) error { + manifest, err := s.readManifest() + if err != nil { + if _, statErr := os.Stat(s.manifestPath()); statErr != nil && os.IsNotExist(statErr) { + remote, cleanup, openErr := s.openGitSource(gitremote.SourceOptions{RemoteURL: remoteURL}) + if openErr != nil { + return openErr + } + remoteManifest, readErr := remote.ReadManifest(ctx) + cleanup() + if readErr != nil { + return readErr + } + manifest = &domain.Manifest{ + Version: 1, + Source: domain.SourceConfig{ + Type: "layered", + Path: ".creed", + Layers: []domain.SourceLayer{{ + Name: "org", + Type: "git", + Path: ".creed", + Remote: remoteURL, + }}, + }, + Targets: remoteManifest.Targets, + } + return s.writeManifest(manifest) + } + return err + } + manifest.Source.Type = "layered" + manifest.Source.Path = sourcePathOrDefault(manifest.Source.Path) + updated := false + for i := range manifest.Source.Layers { + if manifest.Source.Layers[i].Name == "org" || manifest.Source.Layers[i].Remote == remoteURL { + layer := manifest.Source.Layers[i] + if layer.Name == "" { + layer.Name = "org" + } + layer.Type = "git" + if layer.Path == "" { + layer.Path = ".creed" + } + layer.Remote = remoteURL + manifest.Source.Layers[i] = layer + updated = true + break + } + } + if !updated { + manifest.Source.Layers = append(manifest.Source.Layers, domain.SourceLayer{ + Name: "org", + Type: "git", + Path: ".creed", + Remote: remoteURL, + }) + } + return s.writeManifest(manifest) +} + // Push publishes local .creed source changes to a git remote using the system // git executable. It is intentionally isolated here until a writable git port // exists; callers still interact through the stable Service contract. @@ -319,11 +422,14 @@ func (s *Implementation) Push(ctx context.Context, remoteURL string) error { if err := ctx.Err(); err != nil { return err } + manifest, err := s.readManifest() + if err != nil { + return err + } + if manifest.Source.Type == "layered" || len(manifest.Source.Layers) > 0 { + return fmt.Errorf("push is not supported for layered sources; update the shared repository through a pull request") + } if remoteURL == "" { - manifest, err := s.readManifest() - if err != nil { - return err - } remoteURL = manifest.Source.Remote } if remoteURL == "" { @@ -529,10 +635,20 @@ type manifestYAML struct { Configs []domain.ConfigEntry `yaml:"config,omitempty"` } -type sourceConfigYAML struct { +type sourceLayerYAML struct { + Name string `yaml:"name"` Type string `yaml:"type"` Path string `yaml:"path"` Remote string `yaml:"remote,omitempty"` + Ref string `yaml:"ref,omitempty"` +} + +type sourceConfigYAML struct { + Type string `yaml:"type"` + Path string `yaml:"path"` + Remote string `yaml:"remote,omitempty"` + Ref string `yaml:"ref,omitempty"` + Layers []sourceLayerYAML `yaml:"layers,omitempty"` } type targetConfigYAML struct { @@ -548,10 +664,20 @@ func toManifestYAML(manifest *domain.Manifest) manifestYAML { Type: manifest.Source.Type, Path: manifest.Source.Path, Remote: manifest.Source.Remote, + Ref: manifest.Source.Ref, }, Skills: manifest.Skills, Configs: manifest.Configs, } + for _, layer := range manifest.Source.Layers { + mf.Source.Layers = append(mf.Source.Layers, sourceLayerYAML{ + Name: layer.Name, + Type: layer.Type, + Path: layer.Path, + Remote: layer.Remote, + Ref: layer.Ref, + }) + } for _, tc := range manifest.Targets { mf.Targets = append(mf.Targets, targetConfigYAML{ Name: tc.Name, @@ -612,7 +738,12 @@ func (s *Implementation) Watch(ctx context.Context, opts usecase.WatchOptions, s }() syncFn := func(ctx context.Context, syncOpts usecase.SyncOptions) (*usecase.SyncResult, error) { - engine := usecase.NewSyncEngine(localfs.NewSource(s.root), localfs.NewEmitter(s.root)) + source, err := s.openSource(ctx) + if err != nil { + return nil, err + } + defer source.close() + engine := usecase.NewSyncEngine(source.reader, localfs.NewEmitter(s.root)) return engine.Sync(ctx, syncOpts) } engine := usecase.NewWatchEngine(watcher, syncFn) diff --git a/internal/service/impl_test.go b/internal/service/impl_test.go index 048cc9d..dfbe11f 100644 --- a/internal/service/impl_test.go +++ b/internal/service/impl_test.go @@ -748,9 +748,9 @@ config: if strings.Contains(report.SourceRemote, "hunter2") { t.Errorf("SourceRemote leaked URL password: %s", report.SourceRemote) } - // The username should be preserved (safe to display). - if !strings.Contains(report.SourceRemote, "ci-user") { - t.Errorf("SourceRemote should preserve username; got: %s", report.SourceRemote) + // URL userinfo must be removed completely; usernames may be tokens. + if strings.Contains(report.SourceRemote, "ci-user") { + t.Errorf("SourceRemote leaked URL username: %s", report.SourceRemote) } // The host and path should be intact. if !strings.Contains(report.SourceRemote, "git.example.com/repo.git") { diff --git a/internal/service/layered_test.go b/internal/service/layered_test.go new file mode 100644 index 0000000..5e67e14 --- /dev/null +++ b/internal/service/layered_test.go @@ -0,0 +1,389 @@ +package service + +import ( + "context" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/techgodhq/creed/internal/usecase" +) + +func TestLayeredSyncConcatenatesOrgBeforeRepoContext(t *testing.T) { + remote := newLayeredRemote(t, `version: 1 +source: + type: local + path: .creed +targets: + - name: codex + enabled: true + output_dir: . +config: + - name: org + path: config/org.md +`, map[string]string{ + ".creed/config/org.md": "# Org rules\n\nCommit style: conventional.\n", + }) + root := newLayeredConsumer(t, remote, `config: + - name: repo + path: config/repo.md +`, map[string]string{ + ".creed/config/repo.md": "# Repo rules\n\nRun the focused tests first.\n", + }) + + result, err := New(root, WithCacheDir(filepath.Join(t.TempDir(), "cache"))).Sync(context.Background(), usecase.SyncOptions{Target: "codex"}) + if err != nil { + t.Fatalf("layered sync: %v", err) + } + if result.HasErrors() { + t.Fatalf("layered sync target errors: %#v", result.Targets) + } + + content := mustRead(t, filepath.Join(root, "AGENTS.md")) + orgIndex := strings.Index(content, "# Org rules") + repoIndex := strings.Index(content, "# Repo rules") + if orgIndex < 0 || repoIndex < 0 || orgIndex >= repoIndex { + t.Fatalf("layered AGENTS.md order = %q, want org content before repo content", content) + } + if !strings.Contains(content, "\n---\n") { + t.Fatalf("layered AGENTS.md = %q, want separator", content) + } +} + +func TestLayeredDiffIsCleanForComposedOutputAndDetectsDrift(t *testing.T) { + remote := newLayeredRemote(t, `version: 1 +source: + type: local + path: .creed +config: + - name: org + path: config/org.md +`, map[string]string{ + ".creed/config/org.md": "# Org rules\n", + }) + root := newLayeredConsumer(t, remote, `targets: + - name: codex + enabled: true + output_dir: . +config: + - name: repo + path: config/repo.md +`, map[string]string{ + ".creed/config/repo.md": "# Repo rules\n", + "AGENTS.md": "# Org rules\n\n---\n\n# Repo rules\n", + }) + + ctx := context.Background() + clean, err := New(root, WithCacheDir(filepath.Join(t.TempDir(), "cache"))).Diff(ctx, usecase.DiffOptions{Target: "codex"}) + if err != nil { + t.Fatalf("clean layered diff: %v", err) + } + if clean.HasDifferences() { + t.Fatalf("clean layered diff reported drift: %q", clean.UnifiedDiff()) + } + + if err := os.WriteFile(filepath.Join(root, "AGENTS.md"), []byte("# Org rules\n\n---\n\nchanged\n"), 0644); err != nil { + t.Fatal(err) + } + drift, err := New(root, WithCacheDir(filepath.Join(t.TempDir(), "cache"))).Diff(ctx, usecase.DiffOptions{Target: "codex"}) + if err != nil { + t.Fatalf("drifted layered diff: %v", err) + } + if !drift.HasDifferences() || !strings.Contains(drift.UnifiedDiff(), "+# Repo rules") { + t.Fatalf("drifted layered diff = %q, want repo-layer replacement", drift.UnifiedDiff()) + } +} + +func TestLayeredValidateChecksRemoteLayerFiles(t *testing.T) { + remote := newLayeredRemote(t, `version: 1 +source: + type: local + path: .creed +config: + - name: org + path: config/missing.md +`, nil) + root := newLayeredConsumer(t, remote, `config: + - name: repo + path: config/repo.md +`, map[string]string{ + ".creed/config/repo.md": "# Repo rules\n", + }) + + result, err := New(root, WithCacheDir(filepath.Join(t.TempDir(), "cache"))).Validate(context.Background()) + if err != nil { + t.Fatalf("layered validate: %v", err) + } + if result.Valid || !hasDiagnostic(result.Errors, "missing_layer_source_file") { + t.Fatalf("layered validation = %#v, want missing_layer_source_file", result) + } +} + +func TestLayeredDoctorUsesRemoteValidationPath(t *testing.T) { + remote := newLayeredRemote(t, `version: 1 +source: + type: local + path: .creed +config: + - name: org + path: config/missing.md +`, nil) + root := newLayeredConsumer(t, remote, `config: + - name: repo + path: config/repo.md +`, map[string]string{ + ".creed/config/repo.md": "# Repo rules\n", + }) + + report, err := New(root, WithCacheDir(filepath.Join(t.TempDir(), "cache"))).Doctor(context.Background()) + if err != nil { + t.Fatalf("layered doctor: %v", err) + } + if !hasDoctorCheck(report.Checks, "error", "missing_layer_source_file") { + t.Fatalf("layered doctor checks = %#v, want missing_layer_source_file", report.Checks) + } +} + +func TestPullPersistsLayeredManifestWithoutClobberingLocalSource(t *testing.T) { + remote := newLayeredRemote(t, `version: 1 +source: + type: local + path: .creed +targets: + - name: codex + enabled: true + output_dir: . +config: + - name: org + path: config/org.md +`, map[string]string{ + ".creed/config/org.md": "# Org rules\n", + }) + root := newLayeredConsumer(t, remote, `targets: + - name: codex + enabled: true + output_dir: . +config: + - name: repo + path: config/repo.md +`, map[string]string{ + ".creed/config/repo.md": "# Repo rules\n", + }) + + if err := New(root, WithCacheDir(filepath.Join(t.TempDir(), "cache"))).Pull(context.Background(), remote); err != nil { + t.Fatalf("layered pull: %v", err) + } + content := mustRead(t, filepath.Join(root, "AGENTS.md")) + if !strings.Contains(content, "# Org rules") || !strings.Contains(content, "# Repo rules") { + t.Fatalf("pulled AGENTS.md = %q, want both layers", content) + } + if got := mustRead(t, filepath.Join(root, ".creed", "config", "repo.md")); got != "# Repo rules\n" { + t.Fatalf("pull clobbered local source: %q", got) + } + manifest := mustRead(t, filepath.Join(root, ".creed", "manifest.yaml")) + if !strings.Contains(manifest, "type: layered") || !strings.Contains(manifest, "remote:") { + t.Fatalf("pull manifest = %q, want persisted layered source", manifest) + } +} + +func TestValidateRejectsSymlinkedManifest(t *testing.T) { + root := t.TempDir() + outside := t.TempDir() + if err := os.WriteFile(filepath.Join(outside, "manifest.yaml"), []byte("version: 1\nsource:\n type: local\n path: .creed\n"), 0644); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(root, ".creed"), 0755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(outside, "manifest.yaml"), filepath.Join(root, ".creed", "manifest.yaml")); err != nil { + t.Fatal(err) + } + result, err := New(root).Validate(context.Background()) + if err != nil { + t.Fatalf("Validate(): %v", err) + } + if result.Valid || !hasDiagnostic(result.Errors, "invalid_manifest") { + t.Fatalf("Validate() followed symlinked manifest: %#v", result) + } +} + +func TestLayeredValidateChecksRemoteTargets(t *testing.T) { + remote := newLayeredRemote(t, `version: 1 +source: + type: local + path: .creed +targets: + - name: codex + enabled: true + output_dir: ../outside +`, nil) + root := newLayeredConsumer(t, remote, `targets: [] +`, nil) + result, err := New(root, WithCacheDir(filepath.Join(t.TempDir(), "cache"))).Validate(context.Background()) + if err != nil { + t.Fatalf("Validate(): %v", err) + } + if result.Valid || !hasDiagnostic(result.Errors, "unsafe_target_output_dir") { + t.Fatalf("Validate() accepted remote output_dir traversal: %#v", result) + } +} + +func TestLayeredValidateRejectsDuplicateEntriesWithinRemoteLayer(t *testing.T) { + remote := newLayeredRemote(t, `version: 1 +source: + type: local + path: .creed +config: + - name: shared + path: config/one.md + - name: shared + path: config/two.md +`, map[string]string{ + ".creed/config/one.md": "one\n", + ".creed/config/two.md": "two\n", + }) + root := newLayeredConsumer(t, remote, `targets: [] +`, nil) + result, err := New(root, WithCacheDir(filepath.Join(t.TempDir(), "cache"))).Validate(context.Background()) + if err != nil { + t.Fatalf("Validate(): %v", err) + } + if result.Valid || !hasDiagnostic(result.Errors, "duplicate_source_name") { + t.Fatalf("Validate() accepted duplicate remote entry: %#v", result) + } +} + +func TestPullRejectsCredentialBearingRemoteVariants(t *testing.T) { + for _, remote := range []string{ + " HTTPS://user:secret@example.com/org.git", + "HTTPS://user:secret@example.com/org.git", + "ftp://user:secret@example.com/org.git", + } { + t.Run(remote, func(t *testing.T) { + if err := New(t.TempDir()).Pull(context.Background(), remote); err == nil || !strings.Contains(err.Error(), "embedded credentials") { + t.Fatalf("Pull(%q) error = %v, want credential rejection", remote, err) + } + }) + } +} + +func TestLayeredValidateChecksOverriddenRemoteEntry(t *testing.T) { + remote := newLayeredRemote(t, `version: 1 +source: + type: local + path: .creed +config: + - name: shared + path: config/missing.md +`, nil) + root := newLayeredConsumer(t, remote, `config: + - name: shared + path: config/local.md +`, map[string]string{ + ".creed/config/local.md": "# Local override\n", + }) + result, err := New(root, WithCacheDir(filepath.Join(t.TempDir(), "cache"))).Validate(context.Background()) + if err != nil { + t.Fatalf("layered validate: %v", err) + } + if !hasDiagnostic(result.Errors, "missing_layer_source_file") { + t.Fatalf("validation skipped overridden remote entry: %#v", result) + } +} + +func TestPullRejectsCredentialBearingRemoteBeforeWritingManifest(t *testing.T) { + root := t.TempDir() + err := New(root).Pull(context.Background(), "https://user:secret@example.com/org.git") + if err == nil || !strings.Contains(err.Error(), "embedded credentials") { + t.Fatalf("Pull() error = %v, want embedded-credential rejection", err) + } + if _, statErr := os.Stat(filepath.Join(root, ".creed", "manifest.yaml")); !os.IsNotExist(statErr) { + t.Fatalf("Pull() wrote a manifest after rejecting credentials: %v", statErr) + } +} + +func TestPullPreservesExistingLayerPathAndRef(t *testing.T) { + remoteManifest := `version: 1 +source: + type: local + path: .creed +` + remote := newLayeredRemote(t, remoteManifest, map[string]string{"context/manifest.yaml": remoteManifest}) + root := newLayeredConsumer(t, remote, `targets: [] +`, nil) + manifestPath := filepath.Join(root, ".creed", "manifest.yaml") + manifest := mustRead(t, manifestPath) + manifest = strings.Replace(manifest, " path: .creed\n", " path: context\n ref: main\n", 1) + if err := os.WriteFile(manifestPath, []byte(manifest), 0644); err != nil { + t.Fatal(err) + } + if err := New(root).Pull(context.Background(), remote); err != nil { + t.Fatalf("Pull(): %v", err) + } + got := mustRead(t, manifestPath) + if !strings.Contains(got, "path: context") || !strings.Contains(got, "ref: main") { + t.Fatalf("Pull() discarded layer path/ref: %q", got) + } +} + +func newLayeredRemote(t *testing.T, manifest string, files map[string]string) string { + t.Helper() + root := t.TempDir() + writeLayeredFile(t, root, ".creed/manifest.yaml", manifest) + for path, content := range files { + writeLayeredFile(t, root, path, content) + } + initLayeredGitRepo(t, root) + return root +} + +func newLayeredConsumer(t *testing.T, remote, manifestSuffix string, files map[string]string) string { + t.Helper() + root := t.TempDir() + manifest := fmt.Sprintf(`version: 1 +source: + type: layered + path: .creed + layers: + - name: org + type: git + remote: %q + path: .creed +`, remote) + manifest += manifestSuffix + writeLayeredFile(t, root, ".creed/manifest.yaml", manifest) + for path, content := range files { + writeLayeredFile(t, root, path, content) + } + return root +} + +func writeLayeredFile(t *testing.T, root, relative, content string) { + t.Helper() + path := filepath.Join(root, filepath.FromSlash(relative)) + if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(content), 0644); err != nil { + t.Fatal(err) + } +} + +func initLayeredGitRepo(t *testing.T, root string) { + t.Helper() + commands := [][]string{ + {"git", "init", "-b", "main", root}, + {"git", "-C", root, "config", "user.name", "Creed Layer Test"}, + {"git", "-C", root, "config", "user.email", "creed-layer@example.invalid"}, + {"git", "-C", root, "add", "."}, + {"git", "-C", root, "commit", "-m", "add layered fixture"}, + } + for _, args := range commands { + if output, err := exec.Command(args[0], args[1:]...).CombinedOutput(); err != nil { + t.Fatalf("%s: %v\n%s", strings.Join(args, " "), err, output) + } + } +} diff --git a/internal/service/source.go b/internal/service/source.go new file mode 100644 index 0000000..935694d --- /dev/null +++ b/internal/service/source.go @@ -0,0 +1,124 @@ +package service + +import ( + "context" + "fmt" + + "github.com/techgodhq/creed/internal/adapters/gitremote" + "github.com/techgodhq/creed/internal/adapters/layered" + "github.com/techgodhq/creed/internal/adapters/localfs" + "github.com/techgodhq/creed/internal/domain" + "github.com/techgodhq/creed/internal/ports" +) + +type sourceHandle struct { + reader ports.SourceReader + layers []ports.SourceReader + cleanups []func() +} + +func (h *sourceHandle) close() { + for i := len(h.cleanups) - 1; i >= 0; i-- { + h.cleanups[i]() + } +} + +// openSource resolves the project's configured source into the same +// SourceReader used by sync, diff, list, validate, and doctor operations. +func (s *Implementation) openSource(ctx context.Context) (*sourceHandle, error) { + control := localfs.NewSource(s.root) + manifest, err := control.ReadManifest(ctx) + if err != nil { + return nil, err + } + return s.openSourceForManifest(manifest) +} + +func (s *Implementation) openSourceForManifest(manifest *domain.Manifest) (*sourceHandle, error) { + if manifest == nil { + return nil, fmt.Errorf("source manifest is nil") + } + sourceType := manifest.Source.Type + if sourceType == "" { + sourceType = "local" + } + + switch sourceType { + case "local": + local := localfs.NewSource(s.root) + if len(manifest.Source.Layers) == 0 { + return &sourceHandle{reader: local}, nil + } + return s.openLayeredSource(manifest.Source.Layers, local) + case "layered": + if len(manifest.Source.Layers) == 0 { + return nil, fmt.Errorf("layered source requires at least one layer") + } + local := localfs.NewSource(s.root) + return s.openLayeredSource(manifest.Source.Layers, local) + case "git": + if len(manifest.Source.Layers) > 0 { + return nil, fmt.Errorf("git source cannot declare source layers; use type: layered") + } + remote, cleanup, err := s.openGitSource(gitremote.SourceOptions{ + RemoteURL: manifest.Source.Remote, + SourcePath: sourcePathOrDefault(manifest.Source.Path), + Ref: manifest.Source.Ref, + }) + if err != nil { + return nil, err + } + return &sourceHandle{reader: remote, layers: []ports.SourceReader{remote}, cleanups: []func(){cleanup}}, nil + default: + return nil, fmt.Errorf("source type %q is unsupported", manifest.Source.Type) + } +} + +func (s *Implementation) openLayeredSource(layers []domain.SourceLayer, local ports.SourceReader) (*sourceHandle, error) { + readers := make([]ports.SourceReader, 0, len(layers)+1) + cleanups := make([]func(), 0, len(layers)) + for i, layer := range layers { + switch layer.Type { + case "local": + readers = append(readers, localfs.NewSourceWithPath(s.root, sourcePathOrDefault(layer.Path))) + case "git": + remote, cleanup, err := s.openGitSource(gitremote.SourceOptions{ + RemoteURL: layer.Remote, + SourcePath: sourcePathOrDefault(layer.Path), + Ref: layer.Ref, + }) + if err != nil { + for j := len(cleanups) - 1; j >= 0; j-- { + cleanups[j]() + } + return nil, fmt.Errorf("open source layer %d: %w", i, err) + } + readers = append(readers, remote) + cleanups = append(cleanups, cleanup) + default: + for j := len(cleanups) - 1; j >= 0; j-- { + cleanups[j]() + } + return nil, fmt.Errorf("source layer %d has unsupported type %q", i, layer.Type) + } + } + readers = append(readers, local) + return &sourceHandle{reader: layered.NewSource(readers...), layers: readers, cleanups: cleanups}, nil +} + +func (s *Implementation) openGitSource(options gitremote.SourceOptions) (*gitremote.Source, func(), error) { + options.Token = s.token + options.CacheDir = s.cacheDir + remote := gitremote.NewSourceWithOptions(options) + if s.cacheDir == "" { + return remote, func() { _ = remote.Cleanup() }, nil + } + return remote, func() {}, nil +} + +func sourcePathOrDefault(path string) string { + if path == "" { + return ".creed" + } + return path +} diff --git a/internal/service/validate.go b/internal/service/validate.go index a14b4f6..401f326 100644 --- a/internal/service/validate.go +++ b/internal/service/validate.go @@ -11,6 +11,7 @@ import ( "gopkg.in/yaml.v3" + "github.com/techgodhq/creed/internal/adapters/localfs" "github.com/techgodhq/creed/internal/domain" ) @@ -41,9 +42,20 @@ type validationManifest struct { } type validationSource struct { + Type string `yaml:"type"` + Path string `yaml:"path"` + Remote string `yaml:"remote"` + Ref string `yaml:"ref"` + Layers []validationLayer `yaml:"layers"` + Overlays []validationLayer `yaml:"overlays"` +} + +type validationLayer struct { + Name string `yaml:"name"` Type string `yaml:"type"` Path string `yaml:"path"` Remote string `yaml:"remote"` + Ref string `yaml:"ref"` } type validationTarget struct { @@ -52,14 +64,15 @@ type validationTarget struct { OutputDir string `yaml:"output_dir"` } -// Validate checks the local manifest, enabled targets, and every registered -// source without writing target outputs. Errors are reported in the result; -// a returned error is reserved for an unreadable or unparsable manifest. +// Validate checks the local manifest, all configured source layers, enabled +// targets, and every registered source file without writing target outputs. +// Errors are reported in the result; a returned error is reserved for an +// unreadable or unparsable manifest. func (s *Implementation) Validate(ctx context.Context) (ValidationResult, error) { if err := ctx.Err(); err != nil { return ValidationResult{}, err } - manifest, err := s.readValidationManifest() + manifest, err := s.readValidationManifest(ctx) if err != nil { result := ValidationResult{} result.addError("invalid_manifest", "manifest is unreadable or does not match the supported schema: "+err.Error(), "manifest.yaml") @@ -71,48 +84,224 @@ func (s *Implementation) Validate(ctx context.Context) (ValidationResult, error) } else if *manifest.Version != 1 { result.addError("unsupported_manifest_version", fmt.Sprintf("manifest version %d is unsupported", *manifest.Version), "manifest.yaml") } - if manifest.Source.Type != "local" && manifest.Source.Type != "git" { + + sourceType := manifest.Source.Type + if sourceType == "" { + sourceType = "local" + } + if manifest.Source.Type != "local" && manifest.Source.Type != "git" && manifest.Source.Type != "layered" { result.addError("unknown_source_type", fmt.Sprintf("source type %q is unsupported", manifest.Source.Type), "manifest.yaml") } - if manifest.Source.Type == "git" && strings.TrimSpace(manifest.Source.Remote) == "" { + if manifest.Source.Path != "" { + if _, err := safeSourcePath(manifest.Source.Path); err != nil { + result.addError("unsafe_source_path", fmt.Sprintf("source path %q is invalid: %v", manifest.Source.Path, err), "source.path") + } + } + if (sourceType == "local" || sourceType == "layered") && manifest.Source.Path != "" && manifest.Source.Path != ".creed" { + result.addError("unsupported_local_source_path", "local and layered consumer sources must use .creed", "source.path") + } + if sourceType == "git" && strings.TrimSpace(manifest.Source.Remote) == "" { result.addError("missing_source_remote", "git source requires a remote URL", "manifest.yaml") } + if strings.TrimSpace(manifest.Source.Remote) != "" { + if _, err := normalizePullRemoteURL(manifest.Source.Remote); err != nil { + result.addError("invalid_source_remote", err.Error(), "source.remote") + } + } + layers := append(append([]validationLayer{}, manifest.Source.Layers...), manifest.Source.Overlays...) + hasLayers := len(layers) > 0 + if sourceType == "git" && hasLayers { + result.addError("incompatible_source_layers", "git source cannot declare source layers; use type: layered", "manifest.yaml") + } + if sourceType == "layered" && !hasLayers { + result.addError("missing_source_layers", "layered source requires at least one layer", "manifest.yaml") + } + seenLayerNames := map[string]struct{}{} + for i, layer := range layers { + validateLayer(&result, i, layer, seenLayerNames) + } - seenTargets := make(map[string]struct{}, len(manifest.Targets)) - for _, target := range manifest.Targets { + validateTargetConfigs(&result, manifest.Targets, "manifest.yaml") + + localNames := map[string]struct{}{} + if sourceType != "git" { + seenNames := map[string]string{} + seenPaths := map[string]string{} + for _, entry := range manifest.Skills { + localNames[entry.Name] = struct{}{} + s.validateEntryAt(&result, s.localSourceRoot(manifest.Source.Path), "skill", entry.Name, entry.Path, seenNames, seenPaths) + } + for _, entry := range manifest.Configs { + localNames[entry.Name] = struct{}{} + s.validateEntryAt(&result, s.localSourceRoot(manifest.Source.Path), "config", entry.Name, entry.Path, seenNames, seenPaths) + } + } + + // The same resolved SourceReader powers sync/diff and is intentionally used + // here so a remote layer cannot validate successfully while sync reads a + // different source graph. + if sourceType == "local" || sourceType == "layered" || sourceType == "git" { + resolvedLayered := sourceType == "layered" || hasLayers + if source, openErr := s.openSource(ctx); openErr != nil { + if resolvedLayered || sourceType == "git" { + result.addError("layered_source_unavailable", openErr.Error(), "source") + } + } else { + defer source.close() + _, readErr := source.reader.ReadManifest(ctx) + if readErr != nil && (resolvedLayered || sourceType == "git") { + result.addError("layered_source_unavailable", readErr.Error(), "source") + } + if resolvedLayered || sourceType == "git" { + for i, layerReader := range source.layers { + if !sourceTypeIsDirectRemote(sourceType) && i == len(source.layers)-1 { + continue + } + layerManifest, layerErr := layerReader.ReadManifest(ctx) + if layerErr != nil { + result.addError("layered_source_unavailable", layerErr.Error(), fmt.Sprintf("source.layers[%d]", i)) + continue + } + s.validateResolvedEntries(ctx, &result, layerReader, layerManifest) + } + } + } + } + + result.Valid = len(result.Errors) == 0 + return result, nil +} + +func validateLayer(result *ValidationResult, index int, layer validationLayer, seenNames map[string]struct{}) { + path := fmt.Sprintf("source.layers[%d]", index) + name := strings.TrimSpace(layer.Name) + if name == "" { + result.addError("empty_source_layer_name", "source layer name is required", path) + } else if _, ok := seenNames[name]; ok { + result.addError("duplicate_source_layer_name", fmt.Sprintf("source layer %q is declared more than once", name), path) + } else { + seenNames[name] = struct{}{} + } + layerType := layer.Type + if layerType != "local" && layerType != "git" { + result.addError("unknown_source_layer_type", fmt.Sprintf("source layer %q type %q is unsupported", name, layer.Type), path) + } + layerPath := layer.Path + if layerPath == "" { + layerPath = ".creed" + } + if _, err := safeSourcePath(layerPath); err != nil { + result.addError("unsafe_source_layer_path", fmt.Sprintf("source layer %q path %q is invalid: %v", name, layerPath, err), path+".path") + } + if layerType == "git" && strings.TrimSpace(layer.Remote) == "" { + result.addError("missing_source_layer_remote", fmt.Sprintf("source layer %q requires a remote URL", name), path) + } + if strings.TrimSpace(layer.Remote) != "" { + if _, err := normalizePullRemoteURL(layer.Remote); err != nil { + result.addError("invalid_source_layer_remote", err.Error(), path+".remote") + } + } + if strings.ContainsAny(layer.Ref, "\r\n") { + result.addError("invalid_source_layer_ref", fmt.Sprintf("source layer %q ref must be a single line", name), path+".ref") + } +} + +func sourceTypeIsDirectRemote(sourceType string) bool { + return sourceType == "git" +} + +func validateTargetConfigs(result *ValidationResult, targets []validationTarget, pathPrefix string) { + seen := map[string]struct{}{} + for _, target := range targets { + path := pathPrefix + ".targets" if target.Name == "" { - result.addError("empty_target_name", "target name is required", "manifest.yaml") + result.addError("empty_target_name", "target name is required", path) continue } - if _, ok := seenTargets[target.Name]; ok { - result.addError("duplicate_target_name", fmt.Sprintf("target %q is declared more than once", target.Name), "manifest.yaml") + if _, ok := seen[target.Name]; ok { + result.addError("duplicate_target_name", fmt.Sprintf("target %q is declared more than once", target.Name), path) } else { - seenTargets[target.Name] = struct{}{} + seen[target.Name] = struct{}{} } known, lookupErr := domain.LookupTarget(target.Name) if lookupErr != nil { - result.addError("unknown_target", lookupErr.Error(), "manifest.yaml") + result.addError("unknown_target", lookupErr.Error(), path) continue } if target.Enabled && len(known.Outputs("")) == 0 { - result.addWarning("enabled_target_has_no_outputs", fmt.Sprintf("enabled target %q has no configured outputs", target.Name), "manifest.yaml") + result.addWarning("enabled_target_has_no_outputs", fmt.Sprintf("enabled target %q has no configured outputs", target.Name), path) + } + if target.OutputDir == "" || target.OutputDir == "." { + continue + } + clean := filepath.Clean(filepath.FromSlash(target.OutputDir)) + if filepath.IsAbs(target.OutputDir) || clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator)) { + result.addError("unsafe_target_output_dir", fmt.Sprintf("target %q output_dir %q escapes the project root", target.Name, target.OutputDir), path) } } +} +func (s *Implementation) validateResolvedEntries(ctx context.Context, result *ValidationResult, source interface { + ReadSkill(context.Context, string) (*domain.Skill, error) + ReadConfig(context.Context, string) (*domain.ConfigFile, error) +}, manifest *domain.Manifest) { + if manifest == nil { + result.addError("invalid_layer_manifest", "resolved source returned a nil manifest", "source") + return + } + validateResolvedTargets(result, manifest.Targets) seenNames := map[string]string{} seenPaths := map[string]string{} for _, entry := range manifest.Skills { - s.validateEntry(&result, "skill", entry.Name, entry.Path, seenNames, seenPaths) + recordResolvedEntry(result, "skill", entry.Name, entry.Path, seenNames, seenPaths) + if err := validateSkillIdentifier(entry.Name); err != nil { + continue + } + skill, err := source.ReadSkill(ctx, entry.Name) + if err != nil || skill == nil { + result.addError("missing_layer_source_file", fmt.Sprintf("skill %q cannot be read from the resolved source: %v", entry.Name, err), entry.Path) + } } for _, entry := range manifest.Configs { - s.validateEntry(&result, "config", entry.Name, entry.Path, seenNames, seenPaths) + recordResolvedEntry(result, "config", entry.Name, entry.Path, seenNames, seenPaths) + config, err := source.ReadConfig(ctx, entry.Name) + if err != nil || config == nil { + result.addError("missing_layer_source_file", fmt.Sprintf("config %q cannot be read from the resolved source: %v", entry.Name, err), entry.Path) + } } - result.Valid = len(result.Errors) == 0 - return result, nil } -func (s *Implementation) readValidationManifest() (*validationManifest, error) { - data, err := os.ReadFile(s.manifestPath()) +func validateResolvedTargets(result *ValidationResult, targets []domain.TargetConfig) { + converted := make([]validationTarget, 0, len(targets)) + for _, target := range targets { + converted = append(converted, validationTarget{Name: target.Name, Enabled: target.Enabled, OutputDir: target.OutputDir}) + } + validateTargetConfigs(result, converted, "source layer") +} + +func recordResolvedEntry(result *ValidationResult, kind, name, sourcePath string, seenNames, seenPaths map[string]string) { + label := kind + " " + fmt.Sprintf("%q", name) + if previous, ok := seenNames[name]; ok { + result.addError("duplicate_source_name", fmt.Sprintf("%s duplicates %s within one source layer", label, previous), sourcePath) + } else { + seenNames[name] = label + } + cleanPath, err := safeSourcePath(sourcePath) + if err != nil { + result.addError("unsafe_source_path", fmt.Sprintf("%s path %q is invalid: %v", label, sourcePath, err), sourcePath) + return + } + if previous, ok := seenPaths[cleanPath]; ok { + result.addError("duplicate_source_path", fmt.Sprintf("%s reuses source path %q already used by %s", label, cleanPath, previous), cleanPath) + } else { + seenPaths[cleanPath] = label + } +} + +func (s *Implementation) readValidationManifest(ctx context.Context) (*validationManifest, error) { + // Read through LocalFS first so a symlinked manifest cannot bypass source + // containment before the strict YAML decoder below examines its fields. + data, err := localfs.NewSource(s.root).ReadManifestBytes(ctx) if err != nil { if os.IsNotExist(err) { return nil, fmt.Errorf("manifest.yaml does not exist") @@ -134,14 +323,21 @@ func (s *Implementation) readValidationManifest() (*validationManifest, error) { return &manifest, nil } -func (s *Implementation) validateEntry(result *ValidationResult, kind, name, sourcePath string, seenNames, seenPaths map[string]string) { +func (s *Implementation) validateEntryAt(result *ValidationResult, sourceRoot, kind, name, sourcePath string, seenNames, seenPaths map[string]string) { label := kind + " " + fmt.Sprintf("%q", name) if strings.TrimSpace(name) == "" { result.addError("empty_source_name", kind+" name is required", sourcePath) - } else if previous, ok := seenNames[name]; ok { - result.addError("duplicate_source_name", fmt.Sprintf("%s duplicates %s", label, previous), sourcePath) } else { - seenNames[name] = label + if kind == "skill" { + if err := validateSkillIdentifier(name); err != nil { + result.addError("unsafe_source_name", fmt.Sprintf("%s name %q is invalid: %v", label, name, err), sourcePath) + } + } + if previous, ok := seenNames[name]; ok { + result.addError("duplicate_source_name", fmt.Sprintf("%s duplicates %s", label, previous), sourcePath) + } else { + seenNames[name] = label + } } cleanPath, err := safeSourcePath(sourcePath) if err != nil { @@ -154,7 +350,37 @@ func (s *Implementation) validateEntry(result *ValidationResult, kind, name, sou seenPaths[cleanPath] = label } - path := filepath.Join(s.creedDir(), cleanPath) + path := filepath.Join(sourceRoot, cleanPath) + rootInfo, rootErr := os.Lstat(sourceRoot) + if rootErr != nil { + result.addError("unreadable_source_file", fmt.Sprintf("%s source directory cannot be inspected", label), cleanPath) + return + } + if rootInfo.Mode()&os.ModeSymlink != 0 { + result.addError("symlink_source_file", fmt.Sprintf("%s source directory must not be a symlink", label), cleanPath) + return + } + current := sourceRoot + parts := strings.Split(cleanPath, string(filepath.Separator)) + for i, part := range parts { + current = filepath.Join(current, part) + component, componentErr := os.Lstat(current) + if os.IsNotExist(componentErr) { + break + } + if componentErr != nil { + result.addError("unreadable_source_file", fmt.Sprintf("%s source path cannot be inspected", label), cleanPath) + return + } + if component.Mode()&os.ModeSymlink != 0 { + result.addError("symlink_source_file", fmt.Sprintf("%s source must not traverse a symlink", label), cleanPath) + return + } + if i < len(parts)-1 && !component.IsDir() { + result.addError("unreadable_source_file", fmt.Sprintf("%s source parent is not a directory", label), cleanPath) + return + } + } info, err := os.Lstat(path) if err != nil { if os.IsNotExist(err) { @@ -168,14 +394,14 @@ func (s *Implementation) validateEntry(result *ValidationResult, kind, name, sou result.addError("symlink_source_file", fmt.Sprintf("%s source must be a regular file, not a symlink", label), cleanPath) return } - resolvedRoot, rootErr := filepath.EvalSymlinks(s.creedDir()) + resolvedRoot, resolvedRootErr := filepath.EvalSymlinks(sourceRoot) resolvedPath, pathErr := filepath.EvalSymlinks(path) - if rootErr != nil || pathErr != nil { + if resolvedRootErr != nil || pathErr != nil { result.addError("unreadable_source_file", fmt.Sprintf("%s source path cannot be resolved", label), cleanPath) return } if relative, relErr := filepath.Rel(resolvedRoot, resolvedPath); relErr != nil || relative == ".." || strings.HasPrefix(relative, ".."+string(filepath.Separator)) { - result.addError("escaped_source_path", fmt.Sprintf("%s resolves outside .creed", label), cleanPath) + result.addError("escaped_source_path", fmt.Sprintf("%s resolves outside source directory", label), cleanPath) return } if !info.Mode().IsRegular() { @@ -196,16 +422,34 @@ func (s *Implementation) validateEntry(result *ValidationResult, kind, name, sou } } +func validateSkillIdentifier(name string) error { + if strings.TrimSpace(name) == "" { + return fmt.Errorf("name is required") + } + if filepath.IsAbs(filepath.FromSlash(name)) || strings.ContainsAny(name, "/\\") { + return fmt.Errorf("name must be a single relative path component") + } + clean := filepath.Clean(filepath.FromSlash(name)) + if clean == "." || clean == ".." || clean != filepath.FromSlash(name) { + return fmt.Errorf("name must be a single relative path component") + } + return nil +} + +func (s *Implementation) localSourceRoot(_ string) string { + return s.creedDir() +} + func safeSourcePath(sourcePath string) (string, error) { if strings.TrimSpace(sourcePath) == "" { return "", fmt.Errorf("path is required") } if filepath.IsAbs(sourcePath) || filepath.VolumeName(sourcePath) != "" { - return "", fmt.Errorf("path must be relative to .creed") + return "", fmt.Errorf("path must be relative") } - clean := filepath.Clean(sourcePath) + clean := filepath.Clean(filepath.FromSlash(sourcePath)) if clean == "." || clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator)) { - return "", fmt.Errorf("path must remain inside .creed") + return "", fmt.Errorf("path must remain inside source") } return clean, nil } diff --git a/internal/usecase/sync.go b/internal/usecase/sync.go index 225a27d..da880c9 100644 --- a/internal/usecase/sync.go +++ b/internal/usecase/sync.go @@ -358,6 +358,9 @@ func renderContextOutput(output domain.TargetOutput, inputs renderInputs) ([]por func renderSkillDirOutput(output domain.TargetOutput, inputs renderInputs) ([]ports.EmittedFile, error) { files := make([]ports.EmittedFile, 0, len(inputs.skills)) for _, skill := range inputs.skills { + if err := validateSkillName(skill.Name); err != nil { + return nil, fmt.Errorf("skill %q: %w", skill.Name, err) + } files = append(files, ports.EmittedFile{ Path: output.Path + skill.Name + ".md", Content: skill.Content, @@ -413,6 +416,20 @@ func isDirPath(path string) bool { return strings.HasSuffix(path, "/") } +func validateSkillName(name string) error { + if strings.TrimSpace(name) == "" { + return fmt.Errorf("name is required") + } + if filepath.IsAbs(filepath.FromSlash(name)) || strings.ContainsAny(name, "/\\") { + return fmt.Errorf("name must be a single relative path component") + } + clean := filepath.Clean(filepath.FromSlash(name)) + if clean == "." || clean == ".." || clean != filepath.FromSlash(name) { + return fmt.Errorf("name must be a single relative path component") + } + return nil +} + // aggregateConfigs concatenates all config file contents, separated by // a markdown horizontal rule for readability. func aggregateConfigs(configs []domain.ConfigFile) []byte { diff --git a/internal/usecase/sync_test.go b/internal/usecase/sync_test.go index fc5f76e..021dde2 100644 --- a/internal/usecase/sync_test.go +++ b/internal/usecase/sync_test.go @@ -838,3 +838,21 @@ func fileExists(path string) bool { _, err := os.Stat(path) return err == nil } + +func TestSyncRejectsSkillNameTraversalBeforeEmit(t *testing.T) { + src := newTestSource() + const maliciousName = "../../escape" + src.manifest.Skills = append(src.manifest.Skills, domain.SkillEntry{Name: maliciousName, Path: "skills/escape.md"}) + src.skills[maliciousName] = &domain.Skill{Name: maliciousName, Path: "skills/escape.md", Content: []byte("must not write")} + root := t.TempDir() + result, err := NewSyncEngine(src, localfs.NewEmitter(root)).Sync(context.Background(), SyncOptions{Target: "claude"}) + if err != nil { + t.Fatalf("Sync() returned top-level error: %v", err) + } + if !result.HasErrors() { + t.Fatalf("Sync() accepted malicious skill name: %#v", result) + } + if _, err := os.Stat(filepath.Join(root, "..", "escape.md")); !os.IsNotExist(err) { + t.Fatalf("malicious skill created an outside file: %v", err) + } +} diff --git a/openspec/changes/creed-v04-layered-context/.openspec.yaml b/openspec/changes/creed-v04-layered-context/.openspec.yaml new file mode 100644 index 0000000..18ecc23 --- /dev/null +++ b/openspec/changes/creed-v04-layered-context/.openspec.yaml @@ -0,0 +1,2 @@ +name: creed-v04-layered-context +status: implemented diff --git a/openspec/changes/creed-v04-layered-context/design.md b/openspec/changes/creed-v04-layered-context/design.md new file mode 100644 index 0000000..e2e182c --- /dev/null +++ b/openspec/changes/creed-v04-layered-context/design.md @@ -0,0 +1,39 @@ +# Design: layered context sources + +## Manifest + +`domain.SourceConfig` gains `Ref` and ordered `Layers`. Each layer has a name, +backend type, source path, remote URL, and optional ref. The parser accepts +`source.layers` as the canonical spelling and `source.overlays` as a compatible +alias; serializers write `layers`. + +`local`, `git`, and `layered` source types remain supported. A local source with +layers is treated as layered for compatibility. A direct git source remains a +single-layer legacy mode. + +## Composition + +`internal/adapters/layered.Source` implements `ports.SourceReader` and wraps +local or git readers. It merges manifests deterministically, with the last +entry of a duplicate skill/config name replacing the earlier entry. Targets +come from the last layer that declares them, with the consumer manifest taking +precedence when present. The service always appends the consumer local reader +last. + +`internal/service/source.go` is the single source-graph factory. Sync, diff, +list, validate, doctor, watch, and pull use it, so those surfaces cannot silently +choose different source semantics. Git readers retain their cache/auth behavior +and now support a source subdirectory and optional ref pin. + +## Validation and safety + +Validation strictly parses the consumer manifest, validates every layer's type, +name, path, remote, and ref, then opens the resolved source graph. It reads all +remote-declared entries through the same composed reader used by sync. Local +files retain symlink, traversal, regular-file, permission, and empty-content +checks. LocalFS also rejects traversal paths before reading remote files. + +`doctor` reuses `Validate` and includes sanitized layered remotes in its report. +`pull` writes only the consumer manifest metadata needed to remember the shared +layer; it refuses `push` for layered sources to prevent central-repository +clobbering. diff --git a/openspec/changes/creed-v04-layered-context/proposal.md b/openspec/changes/creed-v04-layered-context/proposal.md new file mode 100644 index 0000000..4897f6e --- /dev/null +++ b/openspec/changes/creed-v04-layered-context/proposal.md @@ -0,0 +1,24 @@ +# Proposal: layered context sources + +## Problem + +Creed v0.3 treats local and git-backed context as separate operations. `sync`, +`diff`, `validate`, and `doctor` do not share a source-resolution path, and +`pull` replaces the consumer output instead of composing shared and +repository-specific context. + +## Proposal + +Add an ordered layered source model. A consumer manifest declares shared +`source.layers` and Creed appends its local `.creed/` layer. The composed source +is used by sync, diff, validate, doctor, list operations, and pull. Shared +layers are read first; local context is read last. Pull records metadata and +never overwrites local source files. + +## Acceptance + +- A manifest can compose a git org layer and local repository configs/skills. +- `creed diff` compares the composed output and returns exit status 1 on drift. +- HTTPS token and SSH-agent/key authentication remain on the existing go-git + auth paths, with no raw credentials in errors or reports. +- A migration guide documents the central repository and CI workflow. diff --git a/openspec/changes/creed-v04-layered-context/specs/layered-context/spec.md b/openspec/changes/creed-v04-layered-context/specs/layered-context/spec.md new file mode 100644 index 0000000..5a71f63 --- /dev/null +++ b/openspec/changes/creed-v04-layered-context/specs/layered-context/spec.md @@ -0,0 +1,46 @@ +# Layered context source specification + +## Requirements + +### Requirement: Ordered source composition +The system SHALL read declared shared layers in manifest order and the local +consumer source last. + +#### Scenario: Org and repo configs are emitted together +- Given a git layer with `org.md` and a local layer with `repo.md` +- When `creed sync` renders a context target +- Then both files are present in deterministic order +- And the org content precedes the repo content with a `---` separator + +### Requirement: Shared source overrides are deterministic +The system SHALL use the later declaration when two layers use the same skill or +config name. + +#### Scenario: Repository-specific skill override +- Given a shared and local skill with the same name +- When the composed source reads that skill +- Then the local skill content is used + +### Requirement: Drift gating uses the composed source +`creed diff` SHALL compare target files with the fully composed source and SHALL +return a non-zero drift status when any owned output differs. + +#### Scenario: Central context drift +- Given target output matching the org and repo layers +- When the central content changes +- Then `creed diff` reports the changed output and exits `1` + +### Requirement: Layer health is validated +`creed validate` and `creed doctor` SHALL fetch/read every configured layer and +report unavailable or missing remote entries as structured diagnostics. + +#### Scenario: Missing remote config +- Given a remote manifest referencing a missing config file +- When `creed validate` runs +- Then validation is invalid with a layer-source diagnostic +- And `creed doctor` includes the same error code + +### Requirement: Pull is non-clobbering +`creed pull` SHALL persist the remote layer metadata and compose the pull with +local source files without replacing those files. Layered `push` SHALL be +rejected with an actionable message. diff --git a/openspec/changes/creed-v04-layered-context/tasks.md b/openspec/changes/creed-v04-layered-context/tasks.md new file mode 100644 index 0000000..e088eeb --- /dev/null +++ b/openspec/changes/creed-v04-layered-context/tasks.md @@ -0,0 +1,10 @@ +# Implementation tasks + +- [x] Extend the domain and YAML manifest model with ordered layers and ref pins. +- [x] Add a composed `SourceReader` adapter with deterministic merge semantics. +- [x] Route sync, diff, validate, doctor, list, watch, and pull through one source factory. +- [x] Preserve HTTPS token and SSH agent/key auth paths and add ref-aware caching. +- [x] Prevent layered push from replacing the shared repository. +- [x] Add layered service tests for sync, diff drift, validate, doctor, and pull. +- [x] Add migration documentation and CI guidance. +- [x] Run race tests, vet, formatting, and generated-surface checks.