diff --git a/internal/api/core/errors.go b/internal/api/core/errors.go index 37b68eaa7..5328a9662 100644 --- a/internal/api/core/errors.go +++ b/internal/api/core/errors.go @@ -526,6 +526,8 @@ func StatusForSkillMarketplaceError(err error) int { return http.StatusNotFound case errors.Is(err, skillmarketplace.ErrNotMarketplace): return http.StatusUnprocessableEntity + case errors.Is(err, skillmarketplace.ErrUnavailable): + return http.StatusServiceUnavailable case errors.Is(err, skillmarketplace.ErrNotConfigured): return http.StatusServiceUnavailable default: diff --git a/internal/api/core/skills.go b/internal/api/core/skills.go index 1a5daddfd..6a4a3545d 100644 --- a/internal/api/core/skills.go +++ b/internal/api/core/skills.go @@ -316,6 +316,11 @@ func (h *BaseHandlers) InstallSkillMarketplace(c *gin.Context) { h.respondError(c, http.StatusInternalServerError, err) return } + if err := skillmarketplace.VerifyInstallVisible(h.SkillsRegistry, result); err != nil { + h.logSkillMarketplaceInstallVerificationFailure(result, err) + h.respondError(c, StatusForSkillMarketplaceError(err), err) + return + } c.JSON(http.StatusOK, contract.SkillMarketplaceInstallResponse{ Skill: SkillMarketplaceInstallPayloadFromResult(result), @@ -491,3 +496,17 @@ func (h *BaseHandlers) refreshSkillsAfterMarketplaceMutation(c *gin.Context) err } return nil } + +func (h *BaseHandlers) logSkillMarketplaceInstallVerificationFailure( + result skillmarketplace.InstallResult, + err error, +) { + h.Logger.Warn( + "skills marketplace: installed skill is not discoverable", + "name", result.Name, + "source", result.Registry, + "slug", result.Slug, + "path", result.Path, + "reason", err.Error(), + ) +} diff --git a/internal/api/core/skills_test.go b/internal/api/core/skills_test.go index b89fb0fbf..cc8680e59 100644 --- a/internal/api/core/skills_test.go +++ b/internal/api/core/skills_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "net/http" + "strings" "testing" "time" @@ -364,6 +365,11 @@ func TestStatusForSkillMarketplaceError(t *testing.T) { skillmarketplace.ErrNotConfigured, http.StatusServiceUnavailable, }, + { + "Should return 503 when marketplace install is unavailable after discovery", + skillmarketplace.ErrUnavailable, + http.StatusServiceUnavailable, + }, {"Should return 500 for unknown error", http.ErrServerClosed, http.StatusInternalServerError}, } @@ -516,11 +522,29 @@ func TestSkillMarketplaceHandlers(t *testing.T) { t.Parallel() refreshed := false + installedSkill := &skills.Skill{ + Meta: skills.SkillMeta{ + Name: "review", + }, + Source: skills.SourceMarketplace, + Enabled: true, + Provenance: &skills.Provenance{ + Slug: "@agh/review", + Registry: "clawhub", + Version: "1.2.0", + }, + } registry := &stubSkillsRegistry{ RefreshGlobalFn: func(context.Context) error { refreshed = true return nil }, + GetFn: func(name string) (*skills.Skill, bool) { + if name == "review" { + return installedSkill, true + } + return nil, false + }, } marketplace := &stubSkillMarketplaceService{ InstallFn: func(_ context.Context, slug string, version string) (skillmarketplace.InstallResult, error) { @@ -572,6 +596,59 @@ func TestSkillMarketplaceHandlers(t *testing.T) { } }) + t.Run("Should reject marketplace install when refreshed registry cannot see skill", func(t *testing.T) { + t.Parallel() + + registry := &stubSkillsRegistry{ + RefreshGlobalFn: func(context.Context) error { + return nil + }, + GetFn: func(string) (*skills.Skill, bool) { + return nil, false + }, + } + marketplace := &stubSkillMarketplaceService{ + InstallFn: func(context.Context, string, string) (skillmarketplace.InstallResult, error) { + return skillmarketplace.InstallResult{ + Name: "review", + Slug: "@agh/review", + Version: "1.2.0", + Registry: "clawhub", + Path: "/tmp/agh/skills/review", + Hash: "sha256:abc", + Status: "installed", + }, nil + }, + } + engine := newSkillsHandlerFixtureWithMarketplace( + t, + registry, + testutil.StubWorkspaceService{}, + marketplace, + ) + rec := testutil.PerformRequest( + t, + engine, + http.MethodPost, + "/api/skills/marketplace/install", + testutil.MustJSONBody(t, contract.SkillMarketplaceInstallRequest{ + Slug: "@agh/review", + }), + ) + + if rec.Code != http.StatusServiceUnavailable { + t.Fatalf( + "status = %d, want %d; body=%s", + rec.Code, + http.StatusServiceUnavailable, + rec.Body.String(), + ) + } + if !strings.Contains(rec.Body.String(), "not visible after skill discovery") { + t.Fatalf("body = %s, want registry visibility reason", rec.Body.String()) + } + }) + t.Run("Should not refresh registry for update check only", func(t *testing.T) { t.Parallel() diff --git a/internal/cli/skill_marketplace.go b/internal/cli/skill_marketplace.go index f4a194ecc..05d07fd5d 100644 --- a/internal/cli/skill_marketplace.go +++ b/internal/cli/skill_marketplace.go @@ -15,6 +15,7 @@ import ( registryclawhub "github.com/compozy/agh/internal/registry/clawhub" "github.com/compozy/agh/internal/skills" skillmarketplace "github.com/compozy/agh/internal/skills/marketplace" + skillbundled "github.com/compozy/agh/skills" ) const ( @@ -97,6 +98,9 @@ func installMarketplaceSkill( if err != nil { return skillInstallItem{}, err } + if err := verifyInstalledMarketplaceSkill(ctx, runtime, result); err != nil { + return skillInstallItem{}, err + } return skillInstallItem{ Name: result.Name, Slug: result.Slug, @@ -108,6 +112,43 @@ func installMarketplaceSkill( }, nil } +func verifyInstalledMarketplaceSkill( + ctx context.Context, + runtime *runtimeContext, + result skillmarketplace.InstallResult, +) error { + if runtime == nil { + return errors.New("cli: skill runtime context is required") + } + + registry := skills.NewRegistry(skills.RegistryConfig{ + BundledFS: skillbundled.FS(), + UserAgentsDir: runtime.HomePaths.AgentsDir, + UserSkillsDir: runtime.HomePaths.SkillsDir, + DisabledSkills: append([]string(nil), runtime.Config.Skills.DisabledSkills...), + }, skills.WithLogger(slog.Default())) + if err := registry.LoadAll(ctx); err != nil { + return fmt.Errorf("cli: reload skill registry after marketplace install %q: %w", result.Name, err) + } + + if err := skillmarketplace.VerifyInstallVisible(registry, result); err != nil { + logSkillMarketplaceInstallVerificationFailure(result, err) + return fmt.Errorf("cli: %w", err) + } + return nil +} + +func logSkillMarketplaceInstallVerificationFailure(result skillmarketplace.InstallResult, err error) { + slog.Warn( + "skills marketplace: installed skill is not discoverable", + "name", result.Name, + "source", result.Registry, + "slug", result.Slug, + "path", result.Path, + "reason", err.Error(), + ) +} + func updateMarketplaceSkills( ctx context.Context, runtime *runtimeContext, diff --git a/internal/cli/skill_test.go b/internal/cli/skill_test.go index 76e3d504f..19382ff07 100644 --- a/internal/cli/skill_test.go +++ b/internal/cli/skill_test.go @@ -22,6 +22,7 @@ import ( aghconfig "github.com/compozy/agh/internal/config" registrypkg "github.com/compozy/agh/internal/registry" "github.com/compozy/agh/internal/skills" + skillmarketplace "github.com/compozy/agh/internal/skills/marketplace" "github.com/compozy/agh/internal/store/globaldb" "github.com/compozy/agh/internal/testutil" workspacepkg "github.com/compozy/agh/internal/workspace" @@ -1514,6 +1515,53 @@ func TestSkillMarketplaceHelpers(t *testing.T) { } }) + t.Run("Should install-marketplace-skill-rejects-disabled-discovery", func(t *testing.T) { + server := newMarketplaceTestServer(t, marketplaceServerFixture{ + downloads: map[string]marketplaceDownloadFixture{ + "@agh/review": { + version: "1.1.0", + files: map[string]string{ + "review/SKILL.md": skillDocument("review", "Review helper", "body"), + }, + }, + }, + }) + defer server.Close() + + env := newSkillTestEnv(t, func(cfg *aghconfig.Config) { + cfg.Skills.DisabledSkills = []string{"review"} + cfg.Skills.Marketplace = aghconfig.MarketplaceConfig{ + Registry: "clawhub", + BaseURL: server.URL(), + } + }) + runtime, registry, err := loadSkillRegistry(env.deps) + if err != nil { + t.Fatalf("loadSkillRegistry() error = %v", err) + } + defer func() { + if closeErr := registry.Close(); closeErr != nil { + t.Fatalf("registry.Close() error = %v", closeErr) + } + }() + + _, err = installMarketplaceSkill( + testutil.Context(t), + runtime, + registry, + "@agh/review", + "", + "", + env.deps.now, + ) + if err == nil { + t.Fatal("installMarketplaceSkill(disabled discovery) error = nil, want failure") + } + if !errors.Is(err, skillmarketplace.ErrUnavailable) { + t.Fatalf("installMarketplaceSkill(disabled discovery) error = %v, want ErrUnavailable", err) + } + }) + t.Run("Should install-marketplace-skill-rejects-nil-archive", func(t *testing.T) { env := newSkillTestEnv(t, nil) diff --git a/internal/skills/marketplace/service.go b/internal/skills/marketplace/service.go index b37c27f21..7e87d5bbd 100644 --- a/internal/skills/marketplace/service.go +++ b/internal/skills/marketplace/service.go @@ -48,6 +48,8 @@ var ( ErrNotFound = errors.New("skills marketplace: not found") // ErrNotMarketplace classifies installed skills that lack marketplace provenance. ErrNotMarketplace = errors.New("skills marketplace: not marketplace") + // ErrUnavailable classifies installs that are not visible through skill discovery. + ErrUnavailable = errors.New("skills marketplace: unavailable") // ErrNotConfigured classifies missing or unsupported marketplace registry setup. ErrNotConfigured = errors.New("skills marketplace: not configured") @@ -99,6 +101,11 @@ type NamedRegistry interface { SourceNamed(name string) registrypkg.Source } +// SkillResolver resolves installed skills from the loaded AGH skill catalog. +type SkillResolver interface { + Get(name string) (*skills.Skill, bool) +} + // SourceLoader resolves configured marketplace sources. type SourceLoader func(aghconfig.MarketplaceConfig) ([]registrypkg.Source, error) @@ -984,6 +991,75 @@ func ResolveMarketplaceInstallTarget( return registrypkg.PathWithinRoot(skillsDir, name) } +// VerifyInstallVisible verifies that a completed marketplace install is visible +// through AGH skill discovery with the marketplace provenance that agents rely on. +func VerifyInstallVisible(resolver SkillResolver, result InstallResult) error { + if resolver == nil { + return classifiedf( + ErrUnavailable, + "installed marketplace skill %q cannot be verified because skill discovery is not configured", + result.Name, + ) + } + skill, ok := resolver.Get(result.Name) + if !ok { + return classifiedf( + ErrUnavailable, + "installed marketplace skill %q is not visible after skill discovery; inspect %s and retry the install", + result.Name, + result.Path, + ) + } + if skill.Source != skills.SourceMarketplace { + return classifiedf( + ErrUnavailable, + "installed marketplace skill %q resolved as %s%s after skill discovery; "+ + "remove or disable that declaration and retry the install", + result.Name, + skills.SkillSourceName(skill.Source), + installVisibilityLocation(skill), + ) + } + if skill.Provenance == nil { + return classifiedf( + ErrUnavailable, + "installed marketplace skill %q is missing provenance after skill discovery; inspect %s and retry the install", + result.Name, + result.Path, + ) + } + if strings.TrimSpace(skill.Provenance.Slug) != strings.TrimSpace(result.Slug) { + return classifiedf( + ErrUnavailable, + "installed marketplace skill %q resolved slug %q after skill discovery, want %q; "+ + "remove the conflicting skill and retry the install", + result.Name, + skill.Provenance.Slug, + result.Slug, + ) + } + if !skill.Enabled { + return classifiedf( + ErrUnavailable, + "installed marketplace skill %q is visible but disabled after skill discovery; "+ + "enable the skill and retry discovery", + result.Name, + ) + } + return nil +} + +func installVisibilityLocation(skill *skills.Skill) string { + if skill == nil { + return "" + } + path := firstNonEmpty(skill.FilePath, skill.Dir) + if path == "" { + return "" + } + return fmt.Sprintf(" from %s", path) +} + func (s *Service) loadRegistry() (*registrypkg.MultiRegistry, error) { sources, err := s.sourceLoader(s.skillsConfig.Marketplace) if err != nil { diff --git a/internal/skills/marketplace/service_test.go b/internal/skills/marketplace/service_test.go index bfa88b581..fa3d8c76b 100644 --- a/internal/skills/marketplace/service_test.go +++ b/internal/skills/marketplace/service_test.go @@ -9,6 +9,7 @@ import ( "io" "os" "path/filepath" + "strings" "testing" "time" @@ -27,6 +28,15 @@ type fakeInstallRegistry struct { checkUpdateErr error } +type fakeSkillResolver struct { + skills map[string]*skills.Skill +} + +func (r fakeSkillResolver) Get(name string) (*skills.Skill, bool) { + skill, ok := r.skills[name] + return skill, ok +} + func (r fakeInstallRegistry) Download( context.Context, string, @@ -253,6 +263,126 @@ func TestUpdateSkillClassifiesRegistryLookupFailures(t *testing.T) { }) } +func TestVerifyInstallVisible(t *testing.T) { + t.Parallel() + + result := InstallResult{ + Name: "review", + Slug: "@agh/review", + Version: "1.2.0", + Registry: "clawhub", + Path: "/tmp/agh/skills/review", + Hash: "sha256:abc", + Status: "installed", + } + visibleSkill := func() *skills.Skill { + return &skills.Skill{ + Meta: skills.SkillMeta{Name: "review"}, + Source: skills.SourceMarketplace, + FilePath: filepath.Join( + "tmp", + "agh", + "skills", + "review", + SkillMarkdownFileName, + ), + Enabled: true, + Provenance: &skills.Provenance{ + Slug: "@agh/review", + Registry: "clawhub", + Version: "1.2.0", + }, + } + } + + t.Run("Should accept visible marketplace skill with matching provenance", func(t *testing.T) { + t.Parallel() + + err := VerifyInstallVisible(fakeSkillResolver{skills: map[string]*skills.Skill{ + "review": visibleSkill(), + }}, result) + if err != nil { + t.Fatalf("VerifyInstallVisible() error = %v", err) + } + }) + + cases := []struct { + name string + resolver SkillResolver + wantText string + }{ + { + name: "Should classify missing discovery as unavailable", + resolver: fakeSkillResolver{skills: map[string]*skills.Skill{}}, + wantText: "not visible after skill discovery", + }, + { + name: "Should classify shadowing source as unavailable", + resolver: fakeSkillResolver{skills: map[string]*skills.Skill{ + "review": { + Meta: skills.SkillMeta{Name: "review"}, + Source: skills.SourceUser, + FilePath: "/tmp/agh/skills/review/SKILL.md", + Enabled: true, + }, + }}, + wantText: "resolved as user from /tmp/agh/skills/review/SKILL.md", + }, + { + name: "Should classify missing provenance as unavailable", + resolver: fakeSkillResolver{skills: map[string]*skills.Skill{ + "review": func() *skills.Skill { + skill := visibleSkill() + skill.Provenance = nil + return skill + }(), + }}, + wantText: "missing provenance after skill discovery", + }, + { + name: "Should classify slug mismatch as unavailable", + resolver: fakeSkillResolver{skills: map[string]*skills.Skill{ + "review": func() *skills.Skill { + skill := visibleSkill() + skill.Provenance.Slug = "@other/review" + return skill + }(), + }}, + wantText: "resolved slug \"@other/review\" after skill discovery, want \"@agh/review\"", + }, + { + name: "Should classify disabled skill as unavailable", + resolver: fakeSkillResolver{skills: map[string]*skills.Skill{ + "review": func() *skills.Skill { + skill := visibleSkill() + skill.Enabled = false + return skill + }(), + }}, + wantText: "visible but disabled after skill discovery", + }, + { + name: "Should classify missing resolver as unavailable", + resolver: nil, + wantText: "skill discovery is not configured", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + err := VerifyInstallVisible(tc.resolver, result) + if !errors.Is(err, ErrUnavailable) { + t.Fatalf("VerifyInstallVisible() error = %v, want ErrUnavailable", err) + } + if !strings.Contains(err.Error(), tc.wantText) { + t.Fatalf("VerifyInstallVisible() error = %v, want %q", err, tc.wantText) + } + }) + } +} + func marketplaceSkillArchive(t *testing.T, name string, description string, body string) []byte { t.Helper() diff --git a/packages/site/content/runtime/core/configuration/config-toml.mdx b/packages/site/content/runtime/core/configuration/config-toml.mdx index 060489d42..c6e408154 100644 --- a/packages/site/content/runtime/core/configuration/config-toml.mdx +++ b/packages/site/content/runtime/core/configuration/config-toml.mdx @@ -1240,15 +1240,15 @@ run-enqueue APIs are the execution boundary. Coordinator launch only applies to workspace-scoped coordinated runs with a stable `coordination_channel_id`. Global runs and intent-only tasks do not start coordinators in the MVP. -| Field | Type | Default | Valid values | Description | -| ----------------------------------- | -------- | ------------- | ----------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `enabled` | boolean | `false` | `true` or `false` | Allows coordinator bootstrap after a coordinated workspace run is enqueued. | -| `agent_name` | string | `coordinator` | Non-empty when enabled. | Agent definition used for managed coordinator sessions. | -| `provider` | string | empty | Empty or a configured provider key. | Optional provider override for the coordinator agent. | -| `model` | string | empty | Empty or provider-supported model string. | Optional model override. If set, `provider` or provider default resolution must exist. | -| `default_ttl` | duration | `2h` | `1m` through `24h`. | Lifetime assigned to managed coordinator sessions. | -| `max_children` | integer | `5` | `1` through `5`. | Maximum safe-spawn children a coordinator may hold at once. | -| `max_active_sessions_per_workspace` | integer | `5` | Positive integer. | Caps concurrent autonomy-managed sessions (coordinator + spawned workers) per workspace. Coordinator uniqueness is enforced by the daemon singleton check. | +| Field | Type | Default | Valid values | Description | +| ----------------------------------- | -------- | ------------- | ----------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `enabled` | boolean | `false` | `true` or `false` | Allows coordinator bootstrap after a coordinated workspace run is enqueued. | +| `agent_name` | string | `coordinator` | Non-empty when enabled. | Agent definition used for managed coordinator sessions. | +| `provider` | string | empty | Empty or a configured provider key. | Optional provider override for the coordinator agent. | +| `model` | string | empty | Empty or provider-supported model string. | Optional model override. If set, `provider` or provider default resolution must exist. | +| `default_ttl` | duration | `2h` | `1m` through `24h`. | Lifetime assigned to managed coordinator sessions. | +| `max_children` | integer | `5` | `1` through `5`. | Maximum safe-spawn children a coordinator may hold at once. | +| `max_active_sessions_per_workspace` | integer | `5` | Positive integer. | Caps concurrent autonomy-managed sessions (coordinator + spawned workers) per workspace. Coordinator uniqueness is enforced by the daemon singleton check. | Provider and model resolution is intentionally narrow. The daemon applies: diff --git a/packages/site/content/runtime/core/skills/marketplace.mdx b/packages/site/content/runtime/core/skills/marketplace.mdx index 063015464..bb026c9c2 100644 --- a/packages/site/content/runtime/core/skills/marketplace.mdx +++ b/packages/site/content/runtime/core/skills/marketplace.mdx @@ -87,6 +87,12 @@ exists in a lower source, the higher-precedence source wins according to the nor When installation runs through the daemon API, AGH refreshes the global skills registry before returning so `/api/skills` and the web Installed tab can reflect the new skill immediately. +After writing the files, AGH verifies that skill discovery can resolve the installed skill as a +marketplace skill with matching provenance and an enabled state. If that verification fails, the CLI +returns a classified marketplace unavailable error and the daemon API returns an unavailable +response. Treat this as a local state problem, not a retry signal: enable a disabled skill, remove or +rename a higher-precedence declaration, or remove the broken install directory and install again. + ## Daemon API The daemon exposes marketplace skill operations under `/api/skills/marketplace` on both HTTP and @@ -190,14 +196,14 @@ Update behavior: ## Version Pinning -The internal install pipeline can request a specific archive version, and the marketplace provenance -stores the installed version. The current user-facing CLI does not expose `agh skill install ---version`, nor does it accept version syntax in slugs. +Use `--version` to request a specific marketplace archive version: + +```bash +agh skill install @author/refactor-checklist --version 1.2.0 +``` -For now, treat marketplace installs as "install latest" and use the recorded `.agh-meta.json` -version for audit and update checks. If you need a fixed skill version today, vendor the skill as a -manual local skill in `$AGH_HOME/skills/` or `/.agh/skills/` and review updates -manually. +AGH records the installed version in `.agh-meta.json` for audit and update checks. Version suffixes +are still not accepted in slugs; keep the slug as `@author/name` and pass the version with the flag. ## Remove diff --git a/skills/agh/references/tools-and-skills.md b/skills/agh/references/tools-and-skills.md index 8893fb150..c48b2316d 100644 --- a/skills/agh/references/tools-and-skills.md +++ b/skills/agh/references/tools-and-skills.md @@ -63,6 +63,13 @@ The response shape is `SkillShadowsRecord` / `SkillShadowsResponse`: `winner` is Do not diagnose skill drift from filesystem paths alone. Use the resolver view so workspace, agent-local, bundled, marketplace, extension, and additional-path precedence are all considered. +Marketplace install can write files and still fail discovery verification when the effective skill is +disabled, shadowed by a higher-precedence declaration, missing marketplace provenance, or reporting a +different slug. Treat a marketplace unavailable or not-discoverable install result as terminal until +local state changes. Use `agh skill where `, inspect the winning source and path, then enable +the installed skill, remove or rename the shadowing declaration, or remove the broken install +directory before retrying. + ## Native AGH Tool Map Agents running inside AGH should read references/native-tools.md before choosing a tool or CLI fallback. That file lists the daemon-native toolsets and stable `agh__*` IDs, but the source of truth for parameters and availability is always the live descriptor returned by `agh__tool_info`.