fix: verify marketplace skill installs#244
Conversation
|
@Viniciuscarvalho is attempting to deploy a commit to the Compozy Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds post-install visibility verification for marketplace skills: a new ErrUnavailable sentinel, SkillResolver and VerifyInstallVisible checks, API and CLI calls to verify installed skills are discoverable with matching provenance/slug and enabled, plus tests exercising failure modes. ChangesMarketplace Install Visibility Verification
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
pedronauck
left a comment
pedronauck
left a comment
There was a problem hiding this comment.
Cross-LLM peer review — PR #244 "fix: verify marketplace skill installs"
Two independent implementation peer reviews were run against the PR head (e9ca643f, base main), each reading the full post-change files in an isolated worktree, not just the hunks:
- Reviewer A — Claude Opus (
--ide claude --model opus --reasoning-effort xhigh) → verdict SHIP (0 blockers, 7 risks, 3 nits) - Reviewer B — Codex (
--ide codex --model gpt-5.5 --reasoning-effort xhigh) → verdict FIX_BEFORE_SHIP (2 blockers, 2 risks, 0 nits)
Aggregate verdict: 🟡 FIX_BEFORE_SHIP (one must-fix; everything else is recommended follow-up).
The change is directionally correct, nil-safe, and well-tested — no correctness, security, concurrency, or persistence defects were found by either reviewer. Codex independently ran go test ./internal/api/core ./internal/cli ./internal/skills/marketplace → 1757 passed. The reviewers disagree only on severity of one issue (below), and one Codex blocker did not survive verification.
✅ Cross-validated finding (both reviewers caught it independently) — recommend fixing before merge
CLI install-verification failures don't return the new ErrUnavailable sentinel; HTTP/CLI error contracts diverge.
internal/cli/skill_marketplace.go (~L136–174)
The HTTP path wraps every visibility failure with %w … skillmarketplace.ErrUnavailable, so StatusForSkillMarketplaceError classifies it to 503. The CLI helper verifyInstalledMarketplaceSkill returns plain fmt.Errorf(...) for the same five failure modes (not-visible, shadowed source, missing provenance, slug mismatch, disabled), so the identical logical failure is programmatically classifiable on one surface and opaque text on the other. This breaks the HTTP/CLI parity the PR itself establishes and undercuts the agent-manageable / deterministic-error-identity posture (SD-011) for an agent-facing surface.
- Codex rated this a blocker; Claude rated it a risk (correctly noting it is not a
%wviolation — these are origin errors — so the question is classification, not wrapping discipline). - Reconciliation: it is the PR's own stated parity contract, on a CLI surface agents drive, so it should be closed before merge.
- Fix: wrap
skillmarketplace.ErrUnavailablein every CLI visibility branch (including thefindSkillByNamemiss), then update the CLI regression test to asserterrors.Is(err, skillmarketplace.ErrUnavailable)instead ofstrings.Contains(err.Error(), "visible but disabled"). This also resolves the valid half of Codex B-002.
📋 Recommended follow-ups (risks — strong consensus that these are non-blocking)
-
Duplicated verification invariant, no shared owner (Claude R-001, N-003) — the five-step "installed skill is discoverable" check is implemented verbatim twice (
verifyMarketplaceInstallVisibleininternal/api/core/skills.go:499andverifyInstalledMarketplaceSkillininternal/cli/skill_marketplace.go:115), with two different source-label helpers. Extract one owner ininternal/skills(/marketplace)returning a%w-wrappedErrUnavailable; route both API and CLI through it. This single extraction also fixes the must-fix above and unifies the divergent helper names. (Greenfield: prefer one owner over parallel copies that will drift.) -
503 conflates five non-transient failures (Claude R-003) —
ErrUnavailable → 503(internal/api/core/errors.go:529), but four of the five modes (shadowed, disabled, missing-provenance, slug-mismatch) are terminal until the operator acts. 503 invites clients/agents to retry-with-backoff against a condition retrying can't fix. Consider splitting (409 Conflict for shadow/slug, 422 for disabled/missing-provenance) or pick a terminal code and document it as non-retryable. -
Install is non-atomic on failed verification (Claude R-004) — order is
Install(writes dir + sidecar) →RefreshGlobal→ verify. On failure the files remain on disk and the daemon registry has already been refreshed (a disabled/shadowed skill is partially observable viaGET /api/skills) while the response reports failure. Either reword as "installed but not discoverable" or remove the just-installed dir for the unrecoverable subset so disk state matches the reported outcome — and document the choice. -
Thin predicate test coverage vs. the new contract (Codex R-001 + Claude R-006) — the API test exercises only "not visible after refresh"; the CLI test only "disabled". The helper also enforces source, provenance, slug-match, and the implicit
InstallResult.Name == Skill.Meta.Nameresolution key. Add focused table coverage for the remaining predicates (with sentinel-identity assertions), and pin the name-resolution coupling with a test. -
No structured event on verification failure (Claude R-005) — an install that lands on disk yet is undiscoverable is exactly the operational anomaly to surface. Add a
slog.Warn(handler + CLI) withname,source,slug,path, and reason before returning. -
Docs / official AGH skill not co-shipped (Claude R-007) —
agh skill install/ the install endpoint can now fail with a brand-new terminal outcome. Per the AGH Impact Audit (Web/Docs + Official AGH skill rows), co-ship agent-facing guidance (react by enable/remove-shadow, not retry) or record an explicit "no doc impact" rationale in the PR body. -
Pre-existing brittle classifier nearby (Codex R-002) —
classifyRegistryLookup(internal/skills/marketplace/service.go:1047) falls back tostrings.Contains(err.Error(), " not found"). Not introduced by this PR (it only added theErrUnavailablevar to that file), but adjacent and worth replacing with a typed/sentinel error from the registry boundary.
🔹 Nits (Claude)
- N-001
internal/api/core/skills.go:510— "remove the shadowing skill" wording can point operators at the wrong artifact; include the resolved source/path of the winning declaration in the message. - N-002
internal/cli/skill_marketplace.go:124— the freshskills.NewRegistryfor verification is built withoutskills.WithLogger, so load-time verify warnings fall toslog.Default()instead of the command logger. - N-003 divergent helper names (
verifyInstalledMarketplaceSkillvsverifyMarketplaceInstallVisible) — unify when extracting the shared helper (R-001).
⚠️ Reviewer claim that did NOT survive verification (excluded from the aggregate)
Codex B-002 — "new CLI subtest omits required t.Parallel()". Verified against the source: no subtest inside TestSkillMarketplaceHelpers calls t.Parallel() (only the parent at skill_test.go:1171); all ~18 siblings run serially, almost certainly due to shared filesystem/registry state via newSkillTestEnv/loadSkillRegistry. The new subtest follows the established local convention, so this is not a test-shape violation. Only the string-assertion half of B-002 is valid, and it is already folded into the must-fix above.
Evidence & method
- Both reviewers read all 6 changed files in full plus cross-cutting context (
internal/skills/registry.go,installer.go,types.go,provenance.go,internal/api/testutil), repoCLAUDE.mdset,standing_directives.md, anddocs/_memory/lessons/. - Codex executed the unit suites for the three touched packages → 1757 tests passed. Neither reviewer ran full
make verify(read-only review). - Worktree source was confirmed unmutated before and after both runs; each reviewer wrote only its own findings file.
- Per-reviewer raw findings:
.compozy/pr-244/claude/impl-review-findings-round1.md(SHIP) and.compozy/pr-244/codex/impl-review-findings-round1.md(FIX_BEFORE_SHIP). Both passed thecy-impl-peer-reviewstructural validator.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cli/skill_marketplace.go (1)
141-150: 💤 Low valueOptional: use
slog.WarnContext(ctx, ...)to preserve context-scoped attributes.The helper logs via
slog.Warnwithout a context. Passingctxthrough and usingslog.WarnContextkeeps trace/request attributes attached, consistent with context propagation through the call stack.♻️ Proposed change
- if err := skillmarketplace.VerifyInstallVisible(registry, result); err != nil { - logSkillMarketplaceInstallVerificationFailure(result, err) + if err := skillmarketplace.VerifyInstallVisible(registry, result); err != nil { + logSkillMarketplaceInstallVerificationFailure(ctx, result, err) return fmt.Errorf("cli: %w", err) }-func logSkillMarketplaceInstallVerificationFailure(result skillmarketplace.InstallResult, err error) { - slog.Warn( +func logSkillMarketplaceInstallVerificationFailure(ctx context.Context, result skillmarketplace.InstallResult, err error) { + slog.WarnContext( + ctx, "skills marketplace: installed skill is not discoverable", "name", result.Name, "source", result.Registry, "slug", result.Slug, "path", result.Path, "reason", err.Error(), ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/skill_marketplace.go` around lines 141 - 150, The function logSkillMarketplaceInstallVerificationFailure currently uses slog.Warn without a context; change its signature to accept a context.Context (e.g., add parameter ctx) and replace slog.Warn with slog.WarnContext(ctx, ...) so context-scoped attributes are preserved; update all call sites that invoke logSkillMarketplaceInstallVerificationFailure to pass the appropriate ctx through (preserving existing result/err usage) and ensure the log key/values remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/cli/skill_marketplace.go`:
- Around line 141-150: The function
logSkillMarketplaceInstallVerificationFailure currently uses slog.Warn without a
context; change its signature to accept a context.Context (e.g., add parameter
ctx) and replace slog.Warn with slog.WarnContext(ctx, ...) so context-scoped
attributes are preserved; update all call sites that invoke
logSkillMarketplaceInstallVerificationFailure to pass the appropriate ctx
through (preserving existing result/err usage) and ensure the log key/values
remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ea89981-7fab-4010-b78f-97f6fd310e9a
⛔ Files ignored due to path filters (3)
packages/site/content/runtime/core/configuration/config-toml.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/skills/marketplace.mdxis excluded by!**/*.mdxskills/agh/references/tools-and-skills.mdis excluded by!**/*.md
📒 Files selected for processing (6)
internal/api/core/skills.gointernal/api/core/skills_test.gointernal/cli/skill_marketplace.gointernal/cli/skill_test.gointernal/skills/marketplace/service.gointernal/skills/marketplace/service_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/api/core/skills_test.go
- internal/cli/skill_test.go
2 participants
Summary
Changes
RefreshGlobal()and map failed visibility checks to503 Service Unavailable.agh skill installand validate marketplace source, provenance slug, and enabled state before printing success.Release Notes
Bug Fixes
Test plan
PATH=/opt/homebrew/bin:$PATH go test ./internal/api/core ./internal/cli ./internal/skills/marketplace -count=1PATH=/opt/homebrew/bin:$PATH TURBO_CONCURRENCY=1 VITEST_MAX_WORKERS=1 GOMAXPROCS=2 make verifycompleted on the pre-sync base in/tmp/aghv2.vggp8p.PATH=/opt/homebrew/bin:$PATH TURBO_CONCURRENCY=1 VITEST_MAX_WORKERS=1 GOMAXPROCS=2 MAGE=/tmp/agh-mage make verifywas retried after rebasing ontoa71b6a7f, passed installer check,bun-lint, andbun-typecheck, then the local process was killed during@agh/ui:test(Error: signal: killed). This appears to be local resource pressure; CI should run the full gate.Refs Viniciuscarvalho#2.
Summary by CodeRabbit
New Features
Bug Fixes
Tests