feat: ext registry#20
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a registry-backed marketplace and installer pipeline for extensions and skills, registry source abstractions/adapters (GitHub, ClawHub, MultiRegistry), CLI commands for marketplace operations, extension provenance persistence and DB migrations, safe archive extraction/move utilities, many tests, and a nil-returning DeliveryMetrics bridge method for tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI
participant Loader as RegistrySourceLoader
participant Multi as MultiRegistry
participant Source as RegistrySource
participant Installer as Installer
participant LocalReg as LocalRegistry
participant FS as Filesystem
User->>CLI: extension install `@org/name` or path
CLI->>Loader: load registry sources / detect local path
Loader->>Multi: build multi-registry
Multi->>Source: Info / Download (concurrent across sources)
Source-->>Multi: Detail / DownloadResult
Multi-->>CLI: resolved source & metadata
CLI->>Installer: Install(slug, dlOpts, stagingTarget)
Installer->>Source: Download(ctx, slug, opts)
Source-->>Installer: tar.gz stream
Installer->>Installer: ExtractArchive (safety checks, manifest)
Installer->>FS: stage files in temp dir
Installer->>FS: MoveInstalledDir(temp, final, replace?)
Installer-->>CLI: InstallResult (path, checksum, name/version)
CLI->>LocalReg: Install(manifest, path, checksum, WithInstallRegistryMetadata...)
LocalReg-->>CLI: persisted record (with provenance)
CLI-->>User: success (and restart guidance if needed)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/store/globaldb/global_db.go (1)
92-105:⚠️ Potential issue | 🔴 CriticalAdd migration for new extensions table columns to prevent upgrade failures.
The
extensionstable usesregistry_slug,registry_name, andremote_versioncolumns in active queries (internal/extension/registry.go lines 166, 204, 374, 389-391), butCREATE TABLE IF NOT EXISTSdoes not modify existing tables. Databases upgraded from older versions will lack these columns, causing runtime failures.Add an
ALTER TABLE extensions ADD COLUMNmigration inmigrateGlobalSchemafollowing the pattern used inmigrateSessionColumns(lines 142-165 of migrate_workspace.go):Suggested pattern
func migrateExtensionsColumns(ctx context.Context, db *sql.DB) error { columns, err := tableColumns(ctx, db, "extensions") if err != nil { return err } columnsToAdd := map[string]string{ "registry_slug": "TEXT", "registry_name": "TEXT", "remote_version": "TEXT", } for col, colType := range columnsToAdd { if _, ok := columns[col]; !ok { if _, err := db.ExecContext(ctx, fmt.Sprintf(`ALTER TABLE extensions ADD COLUMN %s %s`, col, colType)); err != nil { return fmt.Errorf("store: add extensions.%s column: %w", col, err) } } } return nil }Call this from
migrateGlobalSchemaafter line 139.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/store/globaldb/global_db.go` around lines 92 - 105, Add a migration that ensures the extensions table has registry_slug, registry_name, and remote_version columns to avoid upgrade breakage: implement a migrateExtensionsColumns(ctx, db *sql.DB) function that uses tableColumns to check existing columns and runs ALTER TABLE extensions ADD COLUMN <name> <type> for any missing columns (registry_slug TEXT, registry_name TEXT, remote_version TEXT), returning wrapped errors like "store: add extensions.<column> column" on failure; then call migrateExtensionsColumns from migrateGlobalSchema (after the session/migration calls) so the columns are added during schema migration.
🧹 Nitpick comments (5)
internal/config/config_test.go (1)
753-772: Globalslogstate mutation prevents parallel execution.The
ShouldWarnForHTTPBaseURLsubtest manipulatesslog.Default(), which is global state. This is intentional for testing warning output but means the parent test cannot uset.Parallel(). Consider adding a comment explaining why parallel execution is not used here.📝 Suggested documentation
func TestExtensionsConfigValidateMarketplaceConfig(t *testing.T) { + // Not parallel: ShouldWarnForHTTPBaseURL mutates global slog.Default(). + t.Run("ShouldAcceptValidMarketplaceConfig", func(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config_test.go` around lines 753 - 772, The subtest ShouldWarnForHTTPBaseURL mutates global slog state (slog.SetDefault) which prevents safe use of t.Parallel; add a brief comment immediately above the t.Run (or at the top of the test function) stating that the test intentionally mutates global slog for capture and therefore must not run in parallel, referencing slog.Default/slog.SetDefault and the test name ShouldWarnForHTTPBaseURL so future maintainers understand why t.Parallel is omitted.internal/extension/registry.go (1)
117-128: Redundant options processing acrossInstallandinstallWithSource.Options are applied twice: first in
Install()(lines 121-125), then again ininstallWithSource()(lines 299-303). While this is functionally correct since options are idempotent setters, it's slightly inefficient. Consider passing the already-built config toinstallWithSourceinstead of re-processing opts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extension/registry.go` around lines 117 - 128, The Install method currently builds installConfig by iterating opts and then calls installWithSource which re-processes opts again; change installWithSource to accept an installConfig (or add a new helper like installWithSourceConfig) so Install can build config once and pass it through (pass manifest, path, checksum, config.source and the built installConfig) and remove the redundant opts loop inside installWithSource; update calls to installWithSource/installWithSourceConfig accordingly and keep the existing InstallOption usage only where configuration is originally built.internal/registry/version_test.go (1)
26-32: Addt.Parallel()to subtests for independent execution.Per coding guidelines, independent subtests should call
t.Parallel()to enable concurrent execution.♻️ Proposed fix
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() if got := VersionIsNewer(tt.current, tt.latest); got != tt.want { t.Fatalf("VersionIsNewer(%q, %q) = %v, want %v", tt.current, tt.latest, got, tt.want) } }) }As per coding guidelines: "Use t.Parallel() for independent subtests in Go tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/registry/version_test.go` around lines 26 - 32, The subtests created in the loop over tests don't call t.Parallel(), so add t.Parallel() as the first statement inside the subtest closure passed to t.Run to enable independent concurrent execution; modify the anonymous func(t *testing.T) used with t.Run (the subtest that calls VersionIsNewer) to invoke t.Parallel() before running the assertion.internal/registry/installer_test.go (1)
223-253: Replace the sleep-based cancellation trigger with explicit synchronization.Lines 230-233 rely on
time.Sleep(50 * time.Millisecond)to guess when the install path has started reading. That makes this test timing-sensitive and prone to flakes under CI load. It would be more stable to haveblockingReadClosersignal whenReadis entered and cancel from that signal instead.As per coding guidelines, Never use
time.Sleep()in orchestration — use proper synchronization primitives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/registry/installer_test.go` around lines 223 - 253, The test uses time.Sleep to guess when the install begins reading; change it to use explicit synchronization by having blockingReadCloser expose a signal (e.g., a started channel or sync.Cond/WaitGroup) that is closed or signaled when its Read method is entered, then in TestInstallerInstallWithContextCancellationClosesReaderAndCleansUp wait on that signal instead of time.Sleep and then call cancel(); remove the sleep goroutine and ensure blockingReadCloser still closes the reader on cancel so the rest of the assertions (reader.closed.Load() and assertNoTempInstallDirs) remain valid.internal/registry/installer.go (1)
590-604: Stream regular files into the checksum hash.
os.ReadFile()loads each file fully into memory before hashing it. Marketplace packages can still contain large regular files under the decompressed-size cap, so this creates avoidable peak allocations in a hot path.os.Open+io.Copykeeps checksuming bounded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/registry/installer.go` around lines 590 - 604, The code currently uses os.ReadFile to load regular files into memory before hashing; change it to open the file with os.Open(absPath), defer close, and stream the file into the hasher with io.Copy(hasher, file) instead of reading the whole content. Keep the existing writeInstallChecksumString(hasher, fmt.Sprintf("file:%s\nmode:%#o\n", normalizedPath, info.Mode().Perm())) call and still write the separator byte via hasher.Write([]byte{0}) after streaming. Ensure any os.Open or io.Copy errors are wrapped/returned similarly to the existing error messages (e.g., "registry: open checksum path %q: %w" or "registry: hash regular file %q: %w"), and make sure the file is closed even on error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cli/extension_marketplace.go`:
- Around line 363-385: configuredExtensionRegistrySources currently loads all
RegistrySource from loader(runtime) but only wraps the filtered slice in a
MultiRegistry, leaking unselected sources. After loader(runtime) returns, ensure
you call Close() on any source that is not included in filtered (i.e., removed
by filterExtensionRegistrySources) and also call Close() on all loaded sources
before returning on error paths. Update the function so that after computing
filtered := filterExtensionRegistrySources(sources, sourceFilter) you iterate
sources and Close() those not in filtered, and on any early return (err or
len(sources)==0 or len(filtered)==0) close all loaded sources before returning
(preserving/combining the original error as needed); reference
functions/variables configuredExtensionRegistrySources,
loader/defaultExtensionRegistrySourceLoader, filterExtensionRegistrySources,
RegistrySource.Close, and MultiRegistry to locate the code to change.
- Around line 406-434: selectMarketplaceExtensionsForUpdate currently indexes
args[0] without checking length which panics when updateAll is false and no name
is provided; update the function to first guard that len(args) > 0 and return a
clear CLI error (e.g., fmt.Errorf("cli: no extension name provided")) before
calling registry.Get or marketplaceExtensionInstalled, so that the code path
using args[0] is only executed when an argument is present; adjust the error
return to match existing error style and keep the rest of the logic
(registry.Get, marketplaceExtensionInstalled, and returning the slice)
unchanged.
In `@internal/cli/skill_marketplace.go`:
- Around line 214-255: The update flow must use the original registry recorded
in installed.Provenance.Registry rather than the aggregated registry; before
calling registry.CheckUpdate and installMarketplaceSkill in
updateMarketplaceSkill, look up or filter the aggregated skillRegistry to get
the specific backend matching installed.Provenance.Registry (return an error if
not found) and call that specificRegistry.CheckUpdate(ctx, slug, currentVersion)
and specificRegistry.Install/ or pass specificRegistry into
installMarketplaceSkill so the update and reinstall come from the same
provenance; update calls referencing registry.CheckUpdate and
installMarketplaceSkill to use the selected specific registry instead.
In `@internal/registry/extract.go`:
- Around line 91-129: Reject extraction paths that would traverse existing
symlinks under destRoot by verifying the real filesystem components after
PathWithinRoot: in ExtractArchive, after computing targetPath (using
CleanArchiveEntryPath and PathWithinRoot), walk the path from destRoot to
targetPath and use os.Lstat on each intermediate component to detect any
symlink; if any component exists and is a symlink, return an error and do not
create directories or open files. Apply the same check before creating
directories (case tar.TypeDir) and before creating/opening files (case
tar.TypeReg/0), and duplicate the check at the other extraction block mentioned
(the similar logic around the other occurrence) so existing symlinked parents
cannot cause extraction escapes.
In `@internal/registry/github/client.go`:
- Around line 248-284: The Download flow currently proceeds to
validateContentType even when c.doRequest returned an HTTP error response (e.g.,
401/404/5xx), masking the real failure; update the logic in Download (the switch
handling selection.asset / selection.useTarball where response is assigned from
c.doRequest) to check response.StatusCode and treat non-2xx codes as errors
(closing response.Body) before calling validateDownloadContentType, returning a
descriptive error (including status code and URL) so callers of Download get the
actual HTTP failure instead of "unexpected download content type"; apply the
same change in the other mirrored block (around lines noted 417-477) that also
returns a response from c.doRequest.
In `@internal/registry/installer.go`:
- Around line 439-460: manifestPathAtRoot currently accepts manifests via
fileExists (which uses os.Stat), allowing symlinks that can point outside the
extracted archive; change the check to require a regular non-symlink file:
replace or supplement fileExists usage in manifestPathAtRoot (and the other
similar manifest-check block) with a function that uses os.Lstat and validates
that the path exists and its FileMode indicates a regular file (not ModeSymlink,
dir, or other non-regular types) and return an error when a manifest is a
symlink or non-regular file; ensure errors reference
installerExtensionManifestName and installerSkillManifestName and preserve
existing error-wrapping behavior so parseInstalledPackageMetadata cannot read
outside targets.
In `@internal/registry/multi_test.go`:
- Around line 320-334: The test currently discards the error from
result.Reader.Close() which hides cleanup failures; update the tail of the Test
(where result is obtained) to call result.Reader.Close(), check its returned
error, and fail the test (e.g., t.Fatalf or t.Fatalf-like assertion) if Close()
returns a non-nil error so cleanup errors on the delegated source are surfaced;
locate the call that currently does `_ = result.Reader.Close()` and replace it
with explicit error handling using the existing t instance and result variable.
In `@internal/registry/multi.go`:
- Around line 93-124: After the fan-out completes (after wg.Wait()), check the
incoming context (ctx) for cancellation before processing results/returning
merged listings: if ctx.Err() != nil return an empty []Listing and ctx.Err() (or
wrap ctx.Err() with any collected errs), so that Search()/resolveSource() honors
cancellations and does not return partial data; update the logic in the function
containing wg.Wait(), merged := mergeListings(results), successes/errs handling
to early-return on ctx cancellation (and apply same pattern to the other similar
block around lines 205-258).
In `@internal/registry/source_test.go`:
- Around line 9-25: Refactor the two tests TestSourceCapsZeroValue and
TestErrNotSupportedMatchesWrappedError into table-driven subtests using the
t.Run("Should...") pattern: create a slice of test cases (with name and
inputs/expected) for each test, iterate over them and call t.Run("Should
<describe expectation>", func(t *testing.T) { t.Parallel(); ...assertions... }),
keeping the existing checks (zero-value Search boolean and errors.Is behavior)
but moving them into the subtest bodies; reference the existing test identifiers
TestSourceCapsZeroValue and TestErrNotSupportedMatchesWrappedError to locate
where to replace the direct checks with the table-driven t.Run subtests.
---
Outside diff comments:
In `@internal/store/globaldb/global_db.go`:
- Around line 92-105: Add a migration that ensures the extensions table has
registry_slug, registry_name, and remote_version columns to avoid upgrade
breakage: implement a migrateExtensionsColumns(ctx, db *sql.DB) function that
uses tableColumns to check existing columns and runs ALTER TABLE extensions ADD
COLUMN <name> <type> for any missing columns (registry_slug TEXT, registry_name
TEXT, remote_version TEXT), returning wrapped errors like "store: add
extensions.<column> column" on failure; then call migrateExtensionsColumns from
migrateGlobalSchema (after the session/migration calls) so the columns are added
during schema migration.
---
Nitpick comments:
In `@internal/config/config_test.go`:
- Around line 753-772: The subtest ShouldWarnForHTTPBaseURL mutates global slog
state (slog.SetDefault) which prevents safe use of t.Parallel; add a brief
comment immediately above the t.Run (or at the top of the test function) stating
that the test intentionally mutates global slog for capture and therefore must
not run in parallel, referencing slog.Default/slog.SetDefault and the test name
ShouldWarnForHTTPBaseURL so future maintainers understand why t.Parallel is
omitted.
In `@internal/extension/registry.go`:
- Around line 117-128: The Install method currently builds installConfig by
iterating opts and then calls installWithSource which re-processes opts again;
change installWithSource to accept an installConfig (or add a new helper like
installWithSourceConfig) so Install can build config once and pass it through
(pass manifest, path, checksum, config.source and the built installConfig) and
remove the redundant opts loop inside installWithSource; update calls to
installWithSource/installWithSourceConfig accordingly and keep the existing
InstallOption usage only where configuration is originally built.
In `@internal/registry/installer_test.go`:
- Around line 223-253: The test uses time.Sleep to guess when the install begins
reading; change it to use explicit synchronization by having blockingReadCloser
expose a signal (e.g., a started channel or sync.Cond/WaitGroup) that is closed
or signaled when its Read method is entered, then in
TestInstallerInstallWithContextCancellationClosesReaderAndCleansUp wait on that
signal instead of time.Sleep and then call cancel(); remove the sleep goroutine
and ensure blockingReadCloser still closes the reader on cancel so the rest of
the assertions (reader.closed.Load() and assertNoTempInstallDirs) remain valid.
In `@internal/registry/installer.go`:
- Around line 590-604: The code currently uses os.ReadFile to load regular files
into memory before hashing; change it to open the file with os.Open(absPath),
defer close, and stream the file into the hasher with io.Copy(hasher, file)
instead of reading the whole content. Keep the existing
writeInstallChecksumString(hasher, fmt.Sprintf("file:%s\nmode:%#o\n",
normalizedPath, info.Mode().Perm())) call and still write the separator byte via
hasher.Write([]byte{0}) after streaming. Ensure any os.Open or io.Copy errors
are wrapped/returned similarly to the existing error messages (e.g., "registry:
open checksum path %q: %w" or "registry: hash regular file %q: %w"), and make
sure the file is closed even on error.
In `@internal/registry/version_test.go`:
- Around line 26-32: The subtests created in the loop over tests don't call
t.Parallel(), so add t.Parallel() as the first statement inside the subtest
closure passed to t.Run to enable independent concurrent execution; modify the
anonymous func(t *testing.T) used with t.Run (the subtest that calls
VersionIsNewer) to invoke t.Parallel() before running the assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 990eeede-5b85-46b1-a446-1a3e4322a7f2
⛔ Files ignored due to path filters (13)
.compozy/tasks/extension-registry/_meta.mdis excluded by!**/*.md.compozy/tasks/extension-registry/_tasks.mdis excluded by!**/*.md.compozy/tasks/extension-registry/memory/MEMORY.mdis excluded by!**/*.md.compozy/tasks/extension-registry/memory/task_01.mdis excluded by!**/*.md.compozy/tasks/extension-registry/memory/task_02.mdis excluded by!**/*.md.compozy/tasks/extension-registry/memory/task_03.mdis excluded by!**/*.md.compozy/tasks/extension-registry/memory/task_04.mdis excluded by!**/*.md.compozy/tasks/extension-registry/memory/task_05.mdis excluded by!**/*.md.compozy/tasks/extension-registry/task_01.mdis excluded by!**/*.md.compozy/tasks/extension-registry/task_02.mdis excluded by!**/*.md.compozy/tasks/extension-registry/task_03.mdis excluded by!**/*.md.compozy/tasks/extension-registry/task_04.mdis excluded by!**/*.md.compozy/tasks/extension-registry/task_05.mdis excluded by!**/*.md
📒 Files selected for processing (36)
internal/cli/cli_integration_test.gointernal/cli/extension.gointernal/cli/extension_marketplace.gointernal/cli/extension_marketplace_integration_test.gointernal/cli/extension_marketplace_test.gointernal/cli/root.gointernal/cli/skill_commands.gointernal/cli/skill_marketplace.gointernal/cli/skill_marketplace_integration_test.gointernal/cli/skill_output.gointernal/cli/skill_test.gointernal/config/config.gointernal/config/config_test.gointernal/config/merge.gointernal/extension/registry.gointernal/extension/registry_test.gointernal/registry/clawhub/client.gointernal/registry/clawhub/client_test.gointernal/registry/extract.gointernal/registry/extract_test.gointernal/registry/github/client.gointernal/registry/github/client_test.gointernal/registry/installer.gointernal/registry/installer_integration_test.gointernal/registry/installer_test.gointernal/registry/multi.gointernal/registry/multi_test.gointernal/registry/source.gointernal/registry/source_test.gointernal/registry/types.gointernal/registry/version.gointernal/registry/version_test.gointernal/skills/marketplace/registry.gointernal/skills/marketplace/types.gointernal/store/globaldb/global_db.gointernal/store/globaldb/global_db_test.go
💤 Files with no reviewable changes (2)
- internal/skills/marketplace/types.go
- internal/skills/marketplace/registry.go
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
internal/registry/multi.go (1)
178-191: Redundant nil check ondetail.At line 180,
detailis guaranteed non-nil becausem.Info()(line 173) returns an error when the detail cannot be resolved, and the error is already handled at lines 174-176. Theif detail != nilblock can be simplified.♻️ Suggested simplification
- latestVersion := "" - source := "" - if detail != nil { - latestVersion = strings.TrimSpace(detail.Version) - source = strings.TrimSpace(detail.Source) - } + latestVersion := strings.TrimSpace(detail.Version) + source := strings.TrimSpace(detail.Source)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/registry/multi.go` around lines 178 - 191, The nil check on detail is redundant because m.Info() already returns an error when detail can't be resolved; remove the if detail != nil block and directly assign latestVersion := strings.TrimSpace(detail.Version) and source := strings.TrimSpace(detail.Source) before constructing the UpdateInfo (keeping references to detail, m.Info(), latestVersion, source, UpdateInfo and VersionIsNewer to locate the code). Ensure the behavior and return value remain unchanged aside from the simplified assignment.internal/registry/multi_test.go (2)
431-462: Consider using "Should..." naming pattern in subtests.Per coding guidelines, subtests should use
t.Run("Should...")pattern. The current naming ("newer version available", "equal version") is descriptive but doesn't follow the prescribed pattern.♻️ Suggested naming update
- t.Run("newer version available", func(t *testing.T) { + t.Run("Should report update when newer version available", func(t *testing.T) { info, err := makeRegistry("1.2.0").CheckUpdate(context.Background(), "pkg", "1.1.0") ... - t.Run("equal version", func(t *testing.T) { + t.Run("Should report no update when versions are equal", func(t *testing.T) { info, err := makeRegistry("1.2.0").CheckUpdate(context.Background(), "pkg", "1.2.0")As per coding guidelines, "MUST use t.Run("Should...") pattern for ALL test cases".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/registry/multi_test.go` around lines 431 - 462, The subtest names in TestMultiRegistryCheckUpdate don't follow the required "Should..." pattern; update the two t.Run calls inside TestMultiRegistryCheckUpdate (the ones exercising makeRegistry(...).CheckUpdate) to descriptive "Should..." names (e.g., "Should report an update when a newer version is available" and "Should not report an update when versions are equal") so the subtest names follow the coding guideline while keeping the same assertions and behavior.
491-537: Subtests should use "Should..." naming pattern.The subtests here ("search respects canceled context", "info requires slug", etc.) are descriptive but don't follow the
t.Run("Should...")pattern specified in coding guidelines.As per coding guidelines, "MUST use t.Run("Should...") pattern for ALL test cases".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/registry/multi_test.go` around lines 491 - 537, The four subtest names inside TestMultiRegistryValidationAndFallbackErrors need to follow the "Should..." pattern: update the t.Run strings so they start with "Should" (e.g., change "search respects canceled context" to "Should respect canceled context for search", "info requires slug" to "Should return error for blank slug in Info", "info returns not found when no source resolves slug" to "Should return not found when no source resolves slug", and "download rejects nil result" to "Should reject nil result from Download"); no behavior or logic changes to the tests (which exercise checkMultiRegistryContext, NewMultiRegistry(...).Info, and NewMultiRegistry(...).Download) — only rename the t.Run descriptions.internal/registry/github/client.go (1)
688-696: Good pattern:closeResponseBodyproperly wraps close errors.This helper correctly handles and wraps close errors. Consider using this helper consistently throughout the file instead of
_ = response.Body.Close()to maintain uniform error handling.The codebase already has
closeResponseBodythat properly handles close errors. Lines 272, 465, and 501 should use this helper or similar pattern for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/registry/github/client.go` around lines 688 - 696, Replace direct silent closes like `_ = response.Body.Close()` with the helper `closeResponseBody` to uniformly handle and wrap close errors; locate usages of `response.Body.Close()` (e.g., the places currently discarding the error) and change them to call `if err := closeResponseBody(response.Body, "<context>"); err != nil { return fmt.Errorf("github: %s close: %w", "<operation>", err) }` or log the error consistently depending on surrounding function behavior, using descriptive context strings (e.g., "list tags response", "get release response") so `closeResponseBody` is used everywhere for consistent error handling.internal/registry/extract_test.go (1)
111-113: Prefer typed/sentinel error assertions here.These checks are pinned to free-form error text, so harmless context changes will break the tests. The unsafe-entry paths should expose sentinel or typed errors so these cases can assert with
errors.Is/errors.Asinstead ofstrings.Contains(err.Error(), ...). As per coding guidelines, "Use errors.Is() and errors.As() for error matching — never compare error strings" and "MUST have specific error assertions (ErrorContains, ErrorAs)".Also applies to: 125-127, 148-150, 163-165, 173-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/registry/extract_test.go` around lines 111 - 113, The test currently asserts on err.Error() text when calling ExtractArchive (and similar cases at the other noted lines); change these to use typed/sentinel error checks (errors.Is or errors.As) instead of string matching: introduce or use an exported sentinel error (e.g., registry.ErrUnsupportedArchiveEntry) or a concrete error type returned by ExtractArchive/unsafe-entry paths, then update the test assertions to use errors.Is(err, registry.ErrUnsupportedArchiveEntry) or errors.As to match the concrete error type for each failing case (apply the same change for the other occurrences at the referenced line ranges) so the tests assert via errors.Is/errors.As rather than strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/extension/registry.go`:
- Around line 347-359: The ExtensionInfo construction in registry.go always sets
Enabled: true which causes WithInstallReplaceExisting() updates to re-enable
previously disabled extensions; change the replacement logic to preserve the
existing Enabled state instead of forcing true: when building the ExtensionInfo
for install/replace, look up the current stored extension (by Name/RegistrySlug
or using r.get.../r.findExistingExtension helper) and set Enabled to that
existing value if present, otherwise default to true for new installs; update
the conflict-upsert path that writes the ExtensionInfo (the code around the
ExtensionInfo literal and the upsert/write-back on conflict) to use this
preserved Enabled value so replaces do not change user-disabled extensions.
- Around line 399-403: The branch that returns raw DB errors when
config.replaceExisting is true should wrap the error with context before
returning; update the return in that branch to use fmt.Errorf to include a
descriptive message mentioning info.Name and wrap the original err (same pattern
used elsewhere and similar to mapRegistryConstraintError usage) so callers know
which install failed.
- Around line 162-165: The extensions table needs a migration to add the new
provenance columns referenced by List(), Get(), and Install(); update
migrateGlobalSchema() to perform an idempotent schema change that adds
registry_slug, registry_name, and remote_version to the extensions table (use
ALTER TABLE … ADD COLUMN if-not-exists semantics or a safe existence check) so
older databases are upgraded before queries run; ensure the migration runs
alongside the other table migrations and that the new columns have appropriate
types/defaults to match how registry.go uses them.
In `@internal/registry/extract.go`:
- Around line 250-267: PathWithinRoot currently lets an empty or whitespace-only
root proceed because filepath.Abs turns it into the working directory; update
PathWithinRoot to explicitly validate the trimmed root (e.g., call
strings.TrimSpace(root) and check for empty) and return a clear error (e.g.,
"root path must not be empty") before calling filepath.Abs; follow the same
validation style as resolvePathWithinRoot/extractArchive and ensure error
messages reference PathWithinRoot for easier debugging and add/adjust unit tests
that exercise empty/whitespace root inputs.
- Around line 137-146: The cleanup call os.Remove(targetPath) currently ignores
any error (using _), so modify the error handling in the write failure branch to
capture the removal error and join it with the existing writeErr (using
errors.Join) before returning; specifically, in the block where writeErr is
created after io.Copy failure (and after attempting file.Close()), replace the
ignored os.Remove with code that stores the removal error (e.g., rmErr :=
os.Remove(targetPath)) and if rmErr != nil set writeErr = errors.Join(writeErr,
fmt.Errorf("remove archive file %q after write failure: %w", targetPath, rmErr))
so the returned error includes both the original write/close error and any
removal failure.
In `@internal/registry/github/client.go`:
- Around line 498-508: In the rate-limit handling block, stop ignoring errors:
when parsing X-RateLimit-Remaining (variable remaining) handle the strconv.Atoi
error instead of discarding it — if err != nil and c.logger != nil call
c.logger.Warn (include the header value and err) and decide to treat remaining
as 0 or skip rate checks; likewise replace `_ = response.Body.Close()` with
proper error handling (if err := response.Body.Close(); err != nil and c.logger
!= nil call c.logger.Warn or c.logger.Error with the error and context including
response.Request.URL.String()); reference symbols: response, remaining,
rateLimitWarnThreshold, c.logger. Ensure logs include the error variable so
parsing and close failures are observable while keeping the prior control flow.
- Around line 270-274: The code currently discards the error from
response.Body.Close() when validateDownloadContentType fails; update the error
handling around response.Body.Close() (the block near
validateDownloadContentType and the response variable) to capture and handle the
close error instead of using `_`, e.g., call response.Body.Close(), check the
returned error, and return or wrap that error (or log it with context) along
with the original validation error so no Close() error is ignored.
- Around line 463-471: The retry path currently discards the error from
response.Body.Close() (the `_ = response.Body.Close()` line); change this to
capture the error and handle it instead of ignoring it: call err :=
response.Body.Close() and if err != nil then log or surface it (e.g., use the
client logger on c if available to Warnf/Debugf with context "github: failed to
close response body before retry" and include err), otherwise decide to
wrap/return the error; keep the existing retry logic around attempt,
c.maxRetries, c.sleep, backoff and nextBackoff unchanged.
---
Nitpick comments:
In `@internal/registry/extract_test.go`:
- Around line 111-113: The test currently asserts on err.Error() text when
calling ExtractArchive (and similar cases at the other noted lines); change
these to use typed/sentinel error checks (errors.Is or errors.As) instead of
string matching: introduce or use an exported sentinel error (e.g.,
registry.ErrUnsupportedArchiveEntry) or a concrete error type returned by
ExtractArchive/unsafe-entry paths, then update the test assertions to use
errors.Is(err, registry.ErrUnsupportedArchiveEntry) or errors.As to match the
concrete error type for each failing case (apply the same change for the other
occurrences at the referenced line ranges) so the tests assert via
errors.Is/errors.As rather than strings.
In `@internal/registry/github/client.go`:
- Around line 688-696: Replace direct silent closes like `_ =
response.Body.Close()` with the helper `closeResponseBody` to uniformly handle
and wrap close errors; locate usages of `response.Body.Close()` (e.g., the
places currently discarding the error) and change them to call `if err :=
closeResponseBody(response.Body, "<context>"); err != nil { return
fmt.Errorf("github: %s close: %w", "<operation>", err) }` or log the error
consistently depending on surrounding function behavior, using descriptive
context strings (e.g., "list tags response", "get release response") so
`closeResponseBody` is used everywhere for consistent error handling.
In `@internal/registry/multi_test.go`:
- Around line 431-462: The subtest names in TestMultiRegistryCheckUpdate don't
follow the required "Should..." pattern; update the two t.Run calls inside
TestMultiRegistryCheckUpdate (the ones exercising makeRegistry(...).CheckUpdate)
to descriptive "Should..." names (e.g., "Should report an update when a newer
version is available" and "Should not report an update when versions are equal")
so the subtest names follow the coding guideline while keeping the same
assertions and behavior.
- Around line 491-537: The four subtest names inside
TestMultiRegistryValidationAndFallbackErrors need to follow the "Should..."
pattern: update the t.Run strings so they start with "Should" (e.g., change
"search respects canceled context" to "Should respect canceled context for
search", "info requires slug" to "Should return error for blank slug in Info",
"info returns not found when no source resolves slug" to "Should return not
found when no source resolves slug", and "download rejects nil result" to
"Should reject nil result from Download"); no behavior or logic changes to the
tests (which exercise checkMultiRegistryContext, NewMultiRegistry(...).Info, and
NewMultiRegistry(...).Download) — only rename the t.Run descriptions.
In `@internal/registry/multi.go`:
- Around line 178-191: The nil check on detail is redundant because m.Info()
already returns an error when detail can't be resolved; remove the if detail !=
nil block and directly assign latestVersion := strings.TrimSpace(detail.Version)
and source := strings.TrimSpace(detail.Source) before constructing the
UpdateInfo (keeping references to detail, m.Info(), latestVersion, source,
UpdateInfo and VersionIsNewer to locate the code). Ensure the behavior and
return value remain unchanged aside from the simplified assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27afad64-7d03-4ccb-b126-2f374e0f8b88
⛔ Files ignored due to path filters (16)
.compozy/tasks/extension-registry/reviews-001/_meta.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_001.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_002.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_003.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_004.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_005.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_006.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_007.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_008.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_009.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_010.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_011.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_012.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_013.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_014.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_015.mdis excluded by!**/*.md
📒 Files selected for processing (16)
internal/cli/extension_marketplace.gointernal/cli/extension_marketplace_test.gointernal/cli/skill_marketplace.gointernal/cli/skill_test.gointernal/config/config_test.gointernal/extension/registry.gointernal/registry/extract.gointernal/registry/extract_test.gointernal/registry/github/client.gointernal/registry/github/client_test.gointernal/registry/installer.gointernal/registry/installer_test.gointernal/registry/multi.gointernal/registry/multi_test.gointernal/registry/source_test.gointernal/registry/version_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/registry/installer_test.go
- internal/cli/skill_marketplace.go
- internal/registry/source_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/registry/version_test.go
- internal/config/config_test.go
- internal/registry/github/client_test.go
- internal/cli/skill_test.go
- internal/registry/installer.go
- internal/cli/extension_marketplace_test.go
- internal/cli/extension_marketplace.go
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/extension/registry.go (1)
199-210:⚠️ Potential issue | 🟡 MinorWrap
Getscan failures with extension context.Non-
ErrNoRowsfailures fromscanExtensionInfoare returned raw here, so schema/scan issues lose the operation and extension name.💡 Proposed fix
info, err := scanExtensionInfo(row) if errors.Is(err, sql.ErrNoRows) { return nil, &ExtensionNotFoundError{Name: trimmedName} } if err != nil { - return nil, err + return nil, fmt.Errorf("extension: get %q: %w", trimmedName, err) }As per coding guidelines, "Use explicit error returns with wrapped context:
fmt.Errorf("context: %w", err)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extension/registry.go` around lines 199 - 210, The scan failure from scanExtensionInfo in the Get path returns raw errors losing context; update the error return (for non-sql.ErrNoRows cases) to wrap the original error with context about the operation and extension name (e.g., include "scanning extension" and trimmedName) using fmt.Errorf("%s: %w", ...) so callers get both the scan error and which extension/query failed; keep the ErrNoRows handling that returns &ExtensionNotFoundError{Name: trimmedName} unchanged.
🧹 Nitpick comments (1)
internal/extension/registry.go (1)
271-277: RemoveinstallWithSourcemethod and update tests to use public API.The method is only called from test files (
registry_test.go,registry_integration_test.go,manager_test.go) and duplicates functionality already available through the publicInstallmethod with theWithInstallSourceoption. Replace test calls withInstall(manifest, path, checksum, WithInstallSource(source))to avoid maintaining test-only production surface code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extension/registry.go` around lines 271 - 277, Remove the test-only helper method installWithSource on type Registry and update all tests that call r.installWithSource(...) to use the public API: r.Install(manifest, path, checksum, WithInstallSource(source)). Ensure any references to installWithSource are deleted (including its definition) and that tests import or reference WithInstallSource correctly; if installWithConfig is still used internally, leave it as-is but do not expose a new helper—the public Install + WithInstallSource option should provide equivalent behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/registry/extract.go`:
- Around line 249-250: The removal of backupDir via os.RemoveAll(backupDir) is
currently returned as an install failure even though the replacement and
os.Rename calls already succeeded; change this to treat backup deletion as
post-install cleanup by not returning the error to callers—replace the returning
fmt.Errorf with a non-fatal handling (e.g., log a warning/error including the
backupDir and err, and continue/return nil) so installer logic in installer.go
doesn't abort after a successful replacement; optionally capture or emit the
cleanup error for telemetry but do not propagate it as a fatal install error.
- Around line 106-108: Wrap validation errors from CleanArchiveEntryPath with
archive and entry context instead of returning err directly: replace the bare
return err after calling CleanArchiveEntryPath(header.Name) with a wrapped error
using fmt.Errorf that includes the archive identifier (e.g. archivePath or
archiveName), the original header.Name (entry) and %w to wrap the original error
(so ErrArchiveEntryPathRequired remains inspectable); for example return
fmt.Errorf("archive %s: entry %q: %w", archivePath, header.Name, err). Ensure
fmt is imported if not already.
In `@internal/registry/github/client.go`:
- Around line 323-334: The branch handling http.StatusNotFound after calling
closeResponseBody incorrectly returns an error even when fetchReleasePage(ctx,
repo) returns published releases; modify the http.StatusNotFound case so that
after calling c.fetchReleasePage(ctx, repo) it returns the first publishable
release (from the releases slice) instead of erroring: call fetchReleasePage,
check len(releases)==0 and only then return the "no published releases" error,
otherwise return that first release (the same shape returned by the normal
latest-release path) so Info() and versionless Download() succeed; changes touch
the http.StatusNotFound branch and use fetchReleasePage, closeResponseBody, and
repo.full to locate code.
- Around line 489-507: In checkRateLimit, don't treat remaining == 0 as a hard
error unconditionally; first inspect response.StatusCode and only
construct/return the rate-limit error (using rateLimitErr, joinErrors and
closeResponseBody) when the status code is 403 or 429; for other status codes
with X-RateLimit-Remaining == "0" simply log a warning (via
c.logger.Debug/Warning) including the status and request URL and return nil so
successful 2xx responses aren't treated as failures. Ensure you still handle
parsing errors and nil response as before and refer to the existing symbols
checkRateLimit, remaining, requestURLString, closeResponseBody and joinErrors
when making the change.
---
Outside diff comments:
In `@internal/extension/registry.go`:
- Around line 199-210: The scan failure from scanExtensionInfo in the Get path
returns raw errors losing context; update the error return (for
non-sql.ErrNoRows cases) to wrap the original error with context about the
operation and extension name (e.g., include "scanning extension" and
trimmedName) using fmt.Errorf("%s: %w", ...) so callers get both the scan error
and which extension/query failed; keep the ErrNoRows handling that returns
&ExtensionNotFoundError{Name: trimmedName} unchanged.
---
Nitpick comments:
In `@internal/extension/registry.go`:
- Around line 271-277: Remove the test-only helper method installWithSource on
type Registry and update all tests that call r.installWithSource(...) to use the
public API: r.Install(manifest, path, checksum, WithInstallSource(source)).
Ensure any references to installWithSource are deleted (including its
definition) and that tests import or reference WithInstallSource correctly; if
installWithConfig is still used internally, leave it as-is but do not expose a
new helper—the public Install + WithInstallSource option should provide
equivalent behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a7011e1a-4653-4d71-9733-853e5bee3022
⛔ Files ignored due to path filters (31)
.agents/skills/compozy/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/compozy/references/cli-reference.mdis excluded by!**/*.md,!.agents/**.agents/skills/compozy/references/config-reference.mdis excluded by!**/*.md,!.agents/**.compozy/tasks/extension-registry/reviews-002/_meta.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_001.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_002.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_003.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_004.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_005.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_006.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_007.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_008.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_009.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_010.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_011.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_012.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_013.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/_meta.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_001.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_002.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_003.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_004.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_005.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_006.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_007.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_008.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_009.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_010.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_011.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_012.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_013.mdis excluded by!**/*.md
📒 Files selected for processing (8)
internal/extension/registry.gointernal/extension/registry_test.gointernal/registry/extract.gointernal/registry/extract_test.gointernal/registry/github/client.gointernal/registry/github/client_test.gointernal/store/globaldb/global_db_test.gointernal/store/globaldb/migrate_workspace.go
✅ Files skipped from review due to trivial changes (2)
- internal/registry/github/client_test.go
- internal/registry/extract_test.go
pedronauck
force-pushed
the
ext-registry
branch
from
39d10f0 to
9cb1a73
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/skill_marketplace.go (1)
266-269:⚠️ Potential issue | 🔴 CriticalGuard the empty-args path before reading
args[0].Calling
updateMarketplaceSkills(..., nil, false, ...)panics here withindex out of rangeinstead of returning a CLI validation error.Suggested fix
- name, err := normalizeSkillName(args[0]) + if len(args) == 0 { + return nil, errors.New("cli: skill name is required unless --all is set") + } + + name, err := normalizeSkillName(args[0]) if err != nil { return nil, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/skill_marketplace.go` around lines 266 - 269, The code reads args[0] without checking args length, causing a panic; before calling normalizeSkillName(args[0]) (and in callers like updateMarketplaceSkills), add a guard that verifies len(args) > 0 and return a CLI validation error (non-nil error) when empty, so normalizeSkillName is only invoked with a valid argument; update any related call sites to propagate the validation error rather than passing nil into updateMarketplaceSkills.
♻️ Duplicate comments (2)
internal/registry/extract.go (2)
106-108:⚠️ Potential issue | 🟡 MinorWrap entry-path validation failures with archive context.
Returning
errdirectly loses context about which archive entry caused the failure.ErrArchiveEntryPathRequiredin particular carries no entry name, making debugging difficult.🔧 Proposed fix
entryName, err := CleanArchiveEntryPath(header.Name) if err != nil { - return err + return fmt.Errorf("clean archive entry %q: %w", header.Name, err) }As per coding guidelines, "Use explicit error returns with wrapped context:
fmt.Errorf("context: %w", err)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/registry/extract.go` around lines 106 - 108, Wrap the error returned from CleanArchiveEntryPath with archive-entry context instead of returning err directly; change the return to something like fmt.Errorf("archive entry %q: %w", header.Name, err) (and if an archive identifier is available in scope include it, e.g. fmt.Errorf("archive %q entry %q: %w", archiveName, header.Name, err)) so ErrArchiveEntryPathRequired and other validation errors carry the offending entry name; update the code around CleanArchiveEntryPath and the entryName handling to return this wrapped error.
249-251:⚠️ Potential issue | 🟠 MajorDon't report install as failed after successful replacement.
By line 249, both
os.Renamecalls have succeeded—the new package is already in place attargetDir. Returning an error here causes the installer to report failure even though the install completed, potentially leaving on-disk state out of sync with persisted provenance metadata.Consider treating backup cleanup as non-fatal post-install work.
🔧 Proposed fix
if err := os.RemoveAll(backupDir); err != nil { - return fmt.Errorf("registry: remove backup package directory %q: %w", backupDir, err) + // Log warning but don't fail - install succeeded, backup cleanup is best-effort + // The backup will be cleaned up on next install or can be removed manually + return nil } return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/registry/extract.go` around lines 249 - 251, Change the backup cleanup to be non-fatal: after the os.Rename calls that moved the new package into targetDir, do not return an error from the os.RemoveAll(backupDir) failure; instead log or emit a warning and continue so a failed cleanup does not mark the install as failed. Replace the current "if err := os.RemoveAll(backupDir); return fmt.Errorf(...)" behavior with a non-fatal handler (e.g., log.Printf or the package's logger) that records the error and the backupDir path but does not return it, referencing the existing os.RemoveAll(backupDir) call and the earlier os.Rename operations that placed content into targetDir.
🧹 Nitpick comments (2)
internal/registry/installer.go (1)
243-245: Consider making stale temp cleanup non-fatal.Failing the entire install because a cleanup of old stale temp directories failed seems overly strict. A user could be blocked from installing a package because an unrelated old temp directory can't be removed (e.g., permission issues on a leftover directory). Consider logging this as a warning instead of returning an error.
♻️ Proposed change
- if err := i.cleanupStaleTempDirs(installParent); err != nil { - return nil, err - } + if err := i.cleanupStaleTempDirs(installParent); err != nil { + // Log but don't fail - old temp cleanup is best-effort + // Consider injecting a logger for proper slog usage + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/registry/installer.go` around lines 243 - 245, The cleanup of stale temp dirs currently aborts the install when i.cleanupStaleTempDirs(installParent) returns an error; change this to non-fatal handling by logging a warning with the error and context (e.g., include installParent and the error returned) and continue execution instead of returning nil, err; locate the call to i.cleanupStaleTempDirs(installParent) and replace the return-path with a warning log (using the existing logger in this package) so cleanup failures don't block installs but are still recorded for diagnostics.internal/registry/extract.go (1)
176-179: Clarify unlimited write behavior when limit is zero or negative.When
limit <= 0, the writer simply counts bytes without enforcing any limit. This may be intentional (for testing or optional limit enforcement), but a comment would help clarify this design choice.📝 Suggested comment
func (w *countingLimitWriter) Write(p []byte) (int, error) { if w.total == nil { return 0, errors.New("total counter is required") } + // When limit is zero or negative, counting proceeds without enforcement. if w.limit <= 0 { *w.total += int64(len(p)) return len(p), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/registry/extract.go` around lines 176 - 179, Add a short clarifying comment above the if block that checks w.limit <= 0 explaining that a non‑positive limit disables enforcement (treats the writer as unlimited), that the code still accumulates bytes into *w.total and returns the full write length, and that this behavior is intentional for cases like testing or optional limit disabling; refer to the variables w.limit and *w.total in the comment so future readers know this branch is the "unlimited" path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cli/extension_marketplace.go`:
- Around line 113-116: The call to multi.Info may return (nil, nil) and the code
then dereferences detail (e.g., detail.Version/detail.Source) causing a panic;
update the handling after calling multi.Info in the install flow (where detail
variable is used and in the other similar spot around the second Info call) to
explicitly check if detail == nil and return a descriptive error (e.g.,
fmt.Errorf("extension info is nil for %s", slug) or a typed error) before
accessing any fields or mapping into ExtensionRecord, ensuring both occurrences
validate the returned pointer.
- Around line 154-161: The install currently computes registryName via
firstNonEmpty(detail.Source, strings.TrimSpace(sourceFilter)) which ignores
result.Source; change the selection to include result.Source first (e.g.,
firstNonEmpty(result.Source, detail.Source, strings.TrimSpace(sourceFilter))) so
registry.Install (and its extensionpkg.WithInstallRegistryMetadata call that
supplies slug, registryName, remoteVersion) persists the resolved
InstallResult.Source when present; update the same logic in the other install
sites (the analogous firstNonEmpty usages around lines ~250-253) so
updateMarketplaceExtension has info.RegistryName populated.
In `@internal/cli/skill_marketplace.go`:
- Around line 186-188: The defer currently swallows the os.RemoveAll(tempRoot)
error; change the defer to capture and handle the error instead of assigning to
_. Replace the anonymous defer with a check like: call os.RemoveAll(tempRoot)
inside the defer, assign its return to err, and if err != nil log or report it
(use the package's logger or fmt.Errorf/ log.Printf consistently with the file)
so failed cleanup is visible; reference tempRoot, os.RemoveAll and the defer
anonymous function when making the change.
---
Outside diff comments:
In `@internal/cli/skill_marketplace.go`:
- Around line 266-269: The code reads args[0] without checking args length,
causing a panic; before calling normalizeSkillName(args[0]) (and in callers like
updateMarketplaceSkills), add a guard that verifies len(args) > 0 and return a
CLI validation error (non-nil error) when empty, so normalizeSkillName is only
invoked with a valid argument; update any related call sites to propagate the
validation error rather than passing nil into updateMarketplaceSkills.
---
Duplicate comments:
In `@internal/registry/extract.go`:
- Around line 106-108: Wrap the error returned from CleanArchiveEntryPath with
archive-entry context instead of returning err directly; change the return to
something like fmt.Errorf("archive entry %q: %w", header.Name, err) (and if an
archive identifier is available in scope include it, e.g. fmt.Errorf("archive %q
entry %q: %w", archiveName, header.Name, err)) so ErrArchiveEntryPathRequired
and other validation errors carry the offending entry name; update the code
around CleanArchiveEntryPath and the entryName handling to return this wrapped
error.
- Around line 249-251: Change the backup cleanup to be non-fatal: after the
os.Rename calls that moved the new package into targetDir, do not return an
error from the os.RemoveAll(backupDir) failure; instead log or emit a warning
and continue so a failed cleanup does not mark the install as failed. Replace
the current "if err := os.RemoveAll(backupDir); return fmt.Errorf(...)" behavior
with a non-fatal handler (e.g., log.Printf or the package's logger) that records
the error and the backupDir path but does not return it, referencing the
existing os.RemoveAll(backupDir) call and the earlier os.Rename operations that
placed content into targetDir.
---
Nitpick comments:
In `@internal/registry/extract.go`:
- Around line 176-179: Add a short clarifying comment above the if block that
checks w.limit <= 0 explaining that a non‑positive limit disables enforcement
(treats the writer as unlimited), that the code still accumulates bytes into
*w.total and returns the full write length, and that this behavior is
intentional for cases like testing or optional limit disabling; refer to the
variables w.limit and *w.total in the comment so future readers know this branch
is the "unlimited" path.
In `@internal/registry/installer.go`:
- Around line 243-245: The cleanup of stale temp dirs currently aborts the
install when i.cleanupStaleTempDirs(installParent) returns an error; change this
to non-fatal handling by logging a warning with the error and context (e.g.,
include installParent and the error returned) and continue execution instead of
returning nil, err; locate the call to i.cleanupStaleTempDirs(installParent) and
replace the return-path with a warning log (using the existing logger in this
package) so cleanup failures don't block installs but are still recorded for
diagnostics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e2263ab2-a66a-4562-87e4-5e34417981bf
⛔ Files ignored due to path filters (60)
.agents/skills/compozy/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/compozy/references/cli-reference.mdis excluded by!**/*.md,!.agents/**.agents/skills/compozy/references/config-reference.mdis excluded by!**/*.md,!.agents/**.compozy/tasks/extension-registry/_meta.mdis excluded by!**/*.md.compozy/tasks/extension-registry/_tasks.mdis excluded by!**/*.md.compozy/tasks/extension-registry/memory/MEMORY.mdis excluded by!**/*.md.compozy/tasks/extension-registry/memory/task_01.mdis excluded by!**/*.md.compozy/tasks/extension-registry/memory/task_02.mdis excluded by!**/*.md.compozy/tasks/extension-registry/memory/task_03.mdis excluded by!**/*.md.compozy/tasks/extension-registry/memory/task_04.mdis excluded by!**/*.md.compozy/tasks/extension-registry/memory/task_05.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/_meta.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_001.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_002.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_003.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_004.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_005.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_006.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_007.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_008.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_009.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_010.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_011.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_012.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_013.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_014.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-001/issue_015.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/_meta.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_001.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_002.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_003.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_004.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_005.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_006.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_007.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_008.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_009.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_010.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_011.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_012.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-002/issue_013.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/_meta.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_001.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_002.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_003.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_004.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_005.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_006.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_007.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_008.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_009.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_010.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_011.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_012.mdis excluded by!**/*.md.compozy/tasks/extension-registry/reviews-003/issue_013.mdis excluded by!**/*.md.compozy/tasks/extension-registry/task_01.mdis excluded by!**/*.md.compozy/tasks/extension-registry/task_02.mdis excluded by!**/*.md.compozy/tasks/extension-registry/task_03.mdis excluded by!**/*.md.compozy/tasks/extension-registry/task_04.mdis excluded by!**/*.md.compozy/tasks/extension-registry/task_05.mdis excluded by!**/*.md
📒 Files selected for processing (37)
internal/cli/cli_integration_test.gointernal/cli/extension.gointernal/cli/extension_marketplace.gointernal/cli/extension_marketplace_integration_test.gointernal/cli/extension_marketplace_test.gointernal/cli/root.gointernal/cli/skill_commands.gointernal/cli/skill_marketplace.gointernal/cli/skill_marketplace_integration_test.gointernal/cli/skill_output.gointernal/cli/skill_test.gointernal/config/config.gointernal/config/config_test.gointernal/config/merge.gointernal/extension/registry.gointernal/extension/registry_test.gointernal/registry/clawhub/client.gointernal/registry/clawhub/client_test.gointernal/registry/extract.gointernal/registry/extract_test.gointernal/registry/github/client.gointernal/registry/github/client_test.gointernal/registry/installer.gointernal/registry/installer_integration_test.gointernal/registry/installer_test.gointernal/registry/multi.gointernal/registry/multi_test.gointernal/registry/source.gointernal/registry/source_test.gointernal/registry/types.gointernal/registry/version.gointernal/registry/version_test.gointernal/skills/marketplace/registry.gointernal/skills/marketplace/types.gointernal/store/globaldb/global_db.gointernal/store/globaldb/global_db_test.gointernal/store/globaldb/migrate_workspace.go
💤 Files with no reviewable changes (2)
- internal/skills/marketplace/registry.go
- internal/skills/marketplace/types.go
✅ Files skipped from review due to trivial changes (7)
- internal/registry/source_test.go
- internal/config/merge.go
- internal/registry/installer_test.go
- internal/registry/source.go
- internal/registry/types.go
- internal/registry/github/client_test.go
- internal/cli/extension_marketplace_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/cli/root.go
- internal/cli/cli_integration_test.go
- internal/registry/installer_integration_test.go
- internal/config/config_test.go
- internal/store/globaldb/migrate_workspace.go
- internal/registry/multi_test.go
- internal/registry/extract_test.go
- internal/registry/version_test.go
- internal/registry/github/client.go
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
internal/cli/extension_marketplace.go (2)
157-165:⚠️ Potential issue | 🟠 MajorPersist the actual source returned by the installer too.
registryNamestill ignoresresult.Source. If the adapter only resolves the source during download/install, the record is saved without source metadata and laterextension updatefails with “missing registry source metadata”.Proposed fix
- remoteVersion := firstNonEmpty(result.Version, detail.Version, manifest.Version) - registryName := firstNonEmpty(detail.Source, strings.TrimSpace(sourceFilter)) + remoteVersion := firstNonEmpty(result.Version, detail.Version, manifest.Version) + registryName := firstNonEmpty(result.Source, detail.Source, strings.TrimSpace(sourceFilter)) + if registryName == "" { + return ExtensionRecord{}, fmt.Errorf("cli: extension registry returned no source for %q", slug) + } if err := registry.Install( manifest, finalDir,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/extension_marketplace.go` around lines 157 - 165, The code builds registryName from firstNonEmpty(detail.Source, strings.TrimSpace(sourceFilter)) and thus drops result.Source returned by the installer; update the selection so the actual source returned by the installer is persisted by using result.Source first (e.g. pick firstNonEmpty(result.Source, detail.Source, strings.TrimSpace(sourceFilter))) before calling registry.Install, and ensure extensionpkg.WithInstallRegistryMetadata(slug, registryName, remoteVersion) receives that registryName so the installed record includes the installer-resolved source.
116-119:⚠️ Potential issue | 🔴 CriticalReject a nil
Info()result before using it.A
(nil, nil)frommulti.Infostill falls through here and later dereferencesdetail.Version/detail.Source, so install panics instead of returning a CLI error.Proposed fix
detail, err := multi.Info(ctx, slug) if err != nil { return ExtensionRecord{}, err } + if detail == nil { + return ExtensionRecord{}, fmt.Errorf("cli: extension registry returned no detail for %q", slug) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/extension_marketplace.go` around lines 116 - 119, multi.Info(ctx, slug) can return (nil, nil) and the code currently assumes detail is non-nil; add a nil-check after calling multi.Info to reject a nil detail and return a clear CLI error instead of proceeding to use detail.Version/detail.Source. Specifically, after the call that assigns detail, err := multi.Info(ctx, slug), check if err != nil as now and also if detail == nil then return an appropriate error (e.g., fmt.Errorf or the CLI error type) so callers of the install path do not dereference a nil pointer.
🧹 Nitpick comments (1)
internal/config/config_test.go (1)
733-809: Use table-driven pattern for the validation cases; keep the HTTP warning subtest separate.This test repeats setup/assertion patterns across four cases (valid config, empty config, invalid base URL, unknown registry). Consolidate these into a table-driven loop per coding guidelines. However, keep the "ShouldWarnForHTTPBaseURL" subtest outside the loop since it manipulates
slog.Default()and must run sequentially to avoid cross-test interference.♻️ Suggested refactor shape
func TestExtensionsConfigValidateMarketplaceConfig(t *testing.T) { + testCases := []struct { + name string + cfg ExtensionsConfig + wantErr string + }{ + { + name: "ShouldAcceptValidMarketplaceConfig", + cfg: ExtensionsConfig{Marketplace: ExtensionsMarketplaceConfig{ + Registry: "github", BaseURL: "https://api.github.example.test", + }}, + }, + { + name: "ShouldAcceptEmptyMarketplaceConfig", + cfg: ExtensionsConfig{}, + }, + { + name: "ShouldRejectMarketplaceBaseURLWithoutHost", + cfg: ExtensionsConfig{Marketplace: ExtensionsMarketplaceConfig{ + Registry: "github", BaseURL: "https://", + }}, + wantErr: "extensions.marketplace.base_url", + }, + { + name: "ShouldRejectUnknownMarketplaceRegistry", + cfg: ExtensionsConfig{Marketplace: ExtensionsMarketplaceConfig{ + Registry: "unknown", BaseURL: "https://api.github.example.test", + }}, + wantErr: "extensions.marketplace.registry", + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + err := tc.cfg.Validate() + if tc.wantErr == "" && err != nil { + t.Fatalf("ExtensionsConfig.Validate() error = %v", err) + } + if tc.wantErr != "" { + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("ExtensionsConfig.Validate() error = %v, want %q", err, tc.wantErr) + } + } + }) + } + t.Run("ShouldWarnForHTTPBaseURL", func(t *testing.T) { // This subtest swaps slog.Default(), so the parent test must remain // non-parallel to avoid cross-test interference. // ... existing code ... }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config_test.go` around lines 733 - 809, TestExtensionsConfigValidateMarketplaceConfig repeats four similar subtests; refactor by converting the valid/empty/invalid-base-url/unknown-registry subtests into a table-driven loop: create a slice of cases with fields (name, cfg ExtensionsConfig, wantErr bool, wantErrContains string), iterate and run t.Run(case.name, func(t *testing.T) { err := case.cfg.Validate(); assert presence/absence of error and check strings.Contains(err.Error(), case.wantErrContains) when applicable }), while keeping the existing ShouldWarnForHTTPBaseURL subtest unchanged and outside the table because it mutates slog.Default(); preserve the original assertions and non-parallel execution for that subtest.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cli/extension_marketplace.go`:
- Around line 204-205: The code derives installDir with
filepath.Dir(info.ManifestPath) without validating info.ManifestPath, so an
empty or malformed ManifestPath yields "." and can cause remove/update to target
the CWD; before calling filepath.Dir (in the block that calls stage(installDir)
and the similar block at lines ~326-333), validate info.ManifestPath (e.g.,
ensure it is non-empty and looks like a valid path — optionally check
filepath.IsAbs or that filepath.Clean(info.ManifestPath) != ".") and return an
error or skip the row if invalid; update the code that uses installDir for
stage(...), remove, and update to only proceed when ManifestPath passes
validation.
In `@internal/registry/extract_test.go`:
- Around line 62-177: The subtests in internal/registry/extract_test.go (e.g.,
the t.Run closures named "decompressed-size", "file-count", "symlink",
"path-traversal", "symlinked-parent", "empty-destination", "invalid-gzip" and
the other t.Run groups noted in the comment) are independent and should call
t.Parallel() inside each subtest body; update each t.Run(...) func(t *testing.T)
{ ... } to begin with t.Parallel() so each subtest runs in parallel and keep the
rest of the assertions unchanged.
- Around line 24-57: The test function
TestExtractArchive_ValidArchiveProducesDirectoryStructure should be refactored
into a subtest using t.Run with a "Should..." description and use table-driven
subtests for each check; specifically, wrap the overall scenario in
t.Run("Should extract valid tar.gz into expected directory structure", func(t
*testing.T) { ... }) and convert the loop over checks into subtests like
t.Run(fmt.Sprintf("Should have %s", check.path), func(t *testing.T){ ... }),
keeping existing setup (mustTarGz, ExtractArchive) and assertions (os.ReadFile
and content compare) intact; apply the same pattern to other top-level tests
referenced (lines ~180-199, 235-274, 369-439) so all cases follow the
t.Run("Should...") convention.
- Around line 131-139: Add platform-specific skips for Windows to avoid running
os.Symlink and os.Chmod-dependent tests: in the t.Run("symlinked-parent", ...)
block add a runtime.GOOS == "windows" check and call t.Skip with a message about
symlink/chmod semantics, and likewise add the same runtime check and t.Skip at
the start of the TestCleanupArchiveFileJoinsRemoveFailure test; follow the
existing pattern used in internal/fileutil/atomic_test.go to detect windows and
skip these platform-specific tests.
---
Duplicate comments:
In `@internal/cli/extension_marketplace.go`:
- Around line 157-165: The code builds registryName from
firstNonEmpty(detail.Source, strings.TrimSpace(sourceFilter)) and thus drops
result.Source returned by the installer; update the selection so the actual
source returned by the installer is persisted by using result.Source first (e.g.
pick firstNonEmpty(result.Source, detail.Source,
strings.TrimSpace(sourceFilter))) before calling registry.Install, and ensure
extensionpkg.WithInstallRegistryMetadata(slug, registryName, remoteVersion)
receives that registryName so the installed record includes the
installer-resolved source.
- Around line 116-119: multi.Info(ctx, slug) can return (nil, nil) and the code
currently assumes detail is non-nil; add a nil-check after calling multi.Info to
reject a nil detail and return a clear CLI error instead of proceeding to use
detail.Version/detail.Source. Specifically, after the call that assigns detail,
err := multi.Info(ctx, slug), check if err != nil as now and also if detail ==
nil then return an appropriate error (e.g., fmt.Errorf or the CLI error type) so
callers of the install path do not dereference a nil pointer.
---
Nitpick comments:
In `@internal/config/config_test.go`:
- Around line 733-809: TestExtensionsConfigValidateMarketplaceConfig repeats
four similar subtests; refactor by converting the
valid/empty/invalid-base-url/unknown-registry subtests into a table-driven loop:
create a slice of cases with fields (name, cfg ExtensionsConfig, wantErr bool,
wantErrContains string), iterate and run t.Run(case.name, func(t *testing.T) {
err := case.cfg.Validate(); assert presence/absence of error and check
strings.Contains(err.Error(), case.wantErrContains) when applicable }), while
keeping the existing ShouldWarnForHTTPBaseURL subtest unchanged and outside the
table because it mutates slog.Default(); preserve the original assertions and
non-parallel execution for that subtest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff005c14-ded2-460f-985b-da57bc542dc2
⛔ Files ignored due to path filters (87)
.agents/skills/qa-execution/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-execution/assets/issue-template.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-execution/assets/verification-report-template.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-execution/references/checklist.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-execution/references/project-signals.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-execution/references/web-ui-qa.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-execution/scripts/discover-project-contract.pyis excluded by!.agents/**.agents/skills/qa-report/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-report/assets/issue-template.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-report/references/bug_report_templates.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-report/references/figma_validation.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-report/references/regression_testing.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-report/references/test_case_templates.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-report/scripts/create_bug_report.shis excluded by!.agents/**.agents/skills/qa-report/scripts/generate_test_cases.shis excluded by!.agents/**.agents/skills/systematic-qa/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/systematic-qa/assets/issue-template.mdis excluded by!**/*.md,!.agents/**.agents/skills/systematic-qa/assets/verification-report-template.mdis excluded by!**/*.md,!.agents/**.compozy/tasks/extension-registry/qa/case-matrix.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/issues/BUG-001.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/runtime/agh-home/agh.dbis excluded by!**/*.db.compozy/tasks/extension-registry/qa/runtime/agh-home/config.tomlis excluded by!**/*.toml.compozy/tasks/extension-registry/qa/runtime/agh-home/daemon.lockis excluded by!**/*.lock.compozy/tasks/extension-registry/qa/runtime/agh-home/logs/agh.logis excluded by!**/*.log.compozy/tasks/extension-registry/qa/test-cases/SMOKE-001.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/SMOKE-002.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/SMOKE-003.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/SMOKE-004.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/SMOKE-005.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-001.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-002.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-003.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-004.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-005.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-006.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-007.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-008.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-009.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-010.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-011.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-012.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-013.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-014.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-015.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-016.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-017.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-018.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-019.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-020.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-021.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-022.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-023.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-024.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-025.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-026.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-027.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-028.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-029.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-FUNC-030.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-INT-001.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-INT-002.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-INT-003.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-INT-004.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-INT-005.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-INT-006.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-INT-007.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-INT-008.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-INT-009.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-INT-010.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-REG-001.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-REG-002.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-REG-003.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-REG-004.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-REG-005.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-REG-006.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-SEC-001.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-SEC-002.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-SEC-003.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-SEC-004.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-SEC-005.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-SEC-006.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-SEC-007.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-cases/TC-SEC-008.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-plans/extension-registry-regression.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/test-plans/extension-registry-test-plan.mdis excluded by!**/*.md.compozy/tasks/extension-registry/qa/verification-report.mdis excluded by!**/*.mdskills-lock.jsonis excluded by!**/*.json
📒 Files selected for processing (12)
internal/cli/extension.gointernal/cli/extension_marketplace.gointernal/cli/extension_marketplace_integration_test.gointernal/cli/extension_marketplace_test.gointernal/cli/extension_test.gointernal/config/config_test.gointernal/daemon/boot.gointernal/daemon/daemon_test.gointernal/daemon/extensions.gointernal/extension/install_managed.gointernal/extensiontest/bridge_adapter_harness_integration_test.gointernal/registry/extract_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/cli/extension_marketplace_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/cli/extension.go
- internal/cli/extension_marketplace_integration_test.go
This was referenced Apr 17, 2026
Closed
Closed
Closed
This was referenced May 23, 2026
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Merged
pedronauck
added a commit
that referenced
this pull request
May 26, 2026
## Release v0.0.1 This PR prepares the release of version v0.0.1. ### Changelog ## 0.0.1 - 2026-05-26 ### Other Changes - Lessons learned ### ♻️ Refactoring - Project structure (#7) - Kb improvements (#12) - Rename spaces to channels (#17) - Add extensions gaps (#21) - Improve tool calls ui (#22) - Remove web app header - Module improvements (#29) - Memory improvements (#35) - Storybook for web and ui (#38) - Enable AGH network by default for new installs (#57) - Hermes adjustments (#69) - Badges design (#84) - Storybook scenario and logos gallery - Migrate typescript tests (#114) - Internal go packages (#120) - Ui patterns (#127) - Improve e2e tests (#130) - Ui redesign - Workspace isolation across runtime surfaces (#145) - Prod ready applies (#162) - Tool card ui (#164) - Alpha on logo - Prod ready features (#167) - Thread sheet (#202) ### 🎉 Features - Implement config foundation packages - Implement sqlite store package - Add ACP client package - Add session lifecycle manager - Implement observe package - Add daemon composition root - Add uds api server - Implement cli package - Add http api server - Add system design - Add foundation types, schemas, and layout shell for web client - Add daemon health polling and agent sidebar systems for web client - Add session system CRUD, streaming core, and session store for web client - Add chat view, messages, and composer tests for web client - Add tool cards and renderers for web client - Add file-backed memory store core - Scaffold memory session seams - Add memory dream consolidation service - Wire memory assembler into daemon - Add memory api and cli - New skills system (#1) - Add workspace entity (#5) - Add new skill capabilities (#8) - Web ui v2 (#9) - Improve hooks system (#10) - Session resilience (#11) - Add extensability (#13) - Add automation (#16) - Add channels (#14) - Add network implementation (#15) - Add network, bridges and automations web pages (#18) - Ext registry (#20) - Add core tasks (#19) - Bridge adapters (#23) - Add site (#26) - Add ext refac and sandbox (#25) - Settings ui (#37) - Tasks ui (#36) - Harness improvements (#44) - Agent capabilities (#49) - Redesign ui (#48) - Unify capability (#53) - Redesign network workspace (#59) - Add task deletion and split session delete from stop (#58) - Session provider selection (#60) - Production grade adjustments (#66) - Autonomous system (#75) - Add agent session route (#80) - Tools registry (#85) - Agents soul (#88) - Add network threads (#105) - Orchestration improvements (#106) - Memory v2 (#108) - Agent categories (#113) - Providers model (#118) - Add canonical AGH bundled skill (#143) - Onboarding and improvements (#198) - Onboarding and improvements (#201) ### 🐛 Bug Fixes - Review round - Review rounds - Resolve memory extensibility review batch - Embed web into daemon - Defaults agents - Acp integration (#4) - Lint errors - Prd folder - Remove orphan web actions and dead surfaces (#55) - Qa testing and fixes (#73) - New review rounds (#82) - Security audit (#90) - Release qa round (#95) - Add missing tools (#141) - New qa round (#147) - Advanced qa round (#149) - Homebrew tap - Final review round (#151) - Daemon healthy - Reasoning models (#158) - Lint errors (#160) - Review round (#168) - Release adjustments (#171) - Stabilize release ci fixtures - Stabilize release integration gate - Stabilize release verify gates - Stabilize release integration flows - Stabilize release verify gates - Stabilize main verify shutdown - Ignore stale acpmock cancel - Marketplace search focus and filtering (#193) - Website video - Workspace command select ### 📚 Documentation - Update agents.md - Update prd - Update skills - Update compozy tasks - Update compozy - Update compozy - Add new skills - Archive prd - Update prds - Update rfc - Update prds - Update prds - Add automation prd - Channels prd - Update prd - Update prd - New prds - Archive prds - Bridges adapters prd - Sandbox prd - Update - Archive prd - Update - Add new prd - New design - Update prd - Archive prds - Update prds - Tasks-ui prd tasks - Update prd - Update design docs - Agent capabilities prd - Improve site docs - Remove old design references - Udpate - Autonomous prd - Update skills - Blog design - Agent sould prd - Final qa plan - Update - Remove codex ledgers from gitignore - Remove not needed files - Udpate ledger - Update cy-codex-loop skill - Orchestration improves prd - Update prds - Orch improvs prd - Memv2 prd - Providers model prd - Update refacs prd - New design proposal - Update rules - Update skills - New blog posts (#173) - Format docs - Remove old design files - Remove old - Skeeper update ### 📦 Build System - Initial structure - Commitlint - Frontend base structure - Update vscode settings - Add subagents - Coderabbit - Prd and tooling - Bun lock - Lint tooling - Copy.md and tooling adjusts - Add repoclone rc - Upgrade skeeper to v0.2.0 - Update go.mod - Adopt task artifacts into skeeper - Sync codex plans with skeeper - Skeeper lock - Skeeper lock - New skills - Skeeper lock - Skeeper lock - Skeeper lock - Update deps and go - Regenerate daytona sidecar assets for go 1.26.3 - Fix cliff - Ignore docs on fmt - Build web assets before goreleaser - Extend release dry-run timeout ### 🔧 CI/CD - Lint errors - Fint release pr - Fix goreleaser ### 🧪 Testing - Add e2e tests (#27) - Qa rounds (#78) - Improve test suite (#138) - Harden daemon-served restart reloads - Harden daemon-served readiness waits - Stabilize dashboard focus assertion - Stabilize release integration gates - Stabilize release e2e markers - Stabilize release e2e flows Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This was referenced May 26, 2026
Closed
Merged
pedronauck
added a commit
that referenced
this pull request
May 26, 2026
## Release v0.0.1 This PR prepares the release of version v0.0.1. ### Changelog ## 0.0.1 - 2026-05-26 ### Other Changes - Lessons learned ### ♻️ Refactoring - Project structure (#7) - Kb improvements (#12) - Rename spaces to channels (#17) - Add extensions gaps (#21) - Improve tool calls ui (#22) - Remove web app header - Module improvements (#29) - Memory improvements (#35) - Storybook for web and ui (#38) - Enable AGH network by default for new installs (#57) - Hermes adjustments (#69) - Badges design (#84) - Storybook scenario and logos gallery - Migrate typescript tests (#114) - Internal go packages (#120) - Ui patterns (#127) - Improve e2e tests (#130) - Ui redesign - Workspace isolation across runtime surfaces (#145) - Prod ready applies (#162) - Tool card ui (#164) - Alpha on logo - Prod ready features (#167) - Thread sheet (#202) ### 🎉 Features - Implement config foundation packages - Implement sqlite store package - Add ACP client package - Add session lifecycle manager - Implement observe package - Add daemon composition root - Add uds api server - Implement cli package - Add http api server - Add system design - Add foundation types, schemas, and layout shell for web client - Add daemon health polling and agent sidebar systems for web client - Add session system CRUD, streaming core, and session store for web client - Add chat view, messages, and composer tests for web client - Add tool cards and renderers for web client - Add file-backed memory store core - Scaffold memory session seams - Add memory dream consolidation service - Wire memory assembler into daemon - Add memory api and cli - New skills system (#1) - Add workspace entity (#5) - Add new skill capabilities (#8) - Web ui v2 (#9) - Improve hooks system (#10) - Session resilience (#11) - Add extensability (#13) - Add automation (#16) - Add channels (#14) - Add network implementation (#15) - Add network, bridges and automations web pages (#18) - Ext registry (#20) - Add core tasks (#19) - Bridge adapters (#23) - Add site (#26) - Add ext refac and sandbox (#25) - Settings ui (#37) - Tasks ui (#36) - Harness improvements (#44) - Agent capabilities (#49) - Redesign ui (#48) - Unify capability (#53) - Redesign network workspace (#59) - Add task deletion and split session delete from stop (#58) - Session provider selection (#60) - Production grade adjustments (#66) - Autonomous system (#75) - Add agent session route (#80) - Tools registry (#85) - Agents soul (#88) - Add network threads (#105) - Orchestration improvements (#106) - Memory v2 (#108) - Agent categories (#113) - Providers model (#118) - Add canonical AGH bundled skill (#143) - Onboarding and improvements (#198) - Onboarding and improvements (#201) ### 🐛 Bug Fixes - Review round - Review rounds - Resolve memory extensibility review batch - Embed web into daemon - Defaults agents - Acp integration (#4) - Lint errors - Prd folder - Remove orphan web actions and dead surfaces (#55) - Qa testing and fixes (#73) - New review rounds (#82) - Security audit (#90) - Release qa round (#95) - Add missing tools (#141) - New qa round (#147) - Advanced qa round (#149) - Homebrew tap - Final review round (#151) - Daemon healthy - Reasoning models (#158) - Lint errors (#160) - Review round (#168) - Release adjustments (#171) - Stabilize release ci fixtures - Stabilize release integration gate - Stabilize release verify gates - Stabilize release integration flows - Stabilize release verify gates - Stabilize main verify shutdown - Ignore stale acpmock cancel - Marketplace search focus and filtering (#193) - Website video - Workspace command select ### 📚 Documentation - Update agents.md - Update prd - Update skills - Update compozy tasks - Update compozy - Update compozy - Add new skills - Archive prd - Update prds - Update rfc - Update prds - Update prds - Add automation prd - Channels prd - Update prd - Update prd - New prds - Archive prds - Bridges adapters prd - Sandbox prd - Update - Archive prd - Update - Add new prd - New design - Update prd - Archive prds - Update prds - Tasks-ui prd tasks - Update prd - Update design docs - Agent capabilities prd - Improve site docs - Remove old design references - Udpate - Autonomous prd - Update skills - Blog design - Agent sould prd - Final qa plan - Update - Remove codex ledgers from gitignore - Remove not needed files - Udpate ledger - Update cy-codex-loop skill - Orchestration improves prd - Update prds - Orch improvs prd - Memv2 prd - Providers model prd - Update refacs prd - New design proposal - Update rules - Update skills - New blog posts (#173) - Format docs - Remove old design files - Remove old - Skeeper update ### 📦 Build System - Initial structure - Commitlint - Frontend base structure - Update vscode settings - Add subagents - Coderabbit - Prd and tooling - Bun lock - Lint tooling - Copy.md and tooling adjusts - Add repoclone rc - Upgrade skeeper to v0.2.0 - Update go.mod - Adopt task artifacts into skeeper - Sync codex plans with skeeper - Skeeper lock - Skeeper lock - New skills - Skeeper lock - Skeeper lock - Skeeper lock - Update deps and go - Regenerate daytona sidecar assets for go 1.26.3 - Fix cliff - Ignore docs on fmt - Build web assets before goreleaser - Extend release dry-run timeout ### 🔧 CI/CD - Lint errors - Fint release pr - Fix goreleaser - Fix release ### 🧪 Testing - Add e2e tests (#27) - Qa rounds (#78) - Improve test suite (#138) - Harden daemon-served restart reloads - Harden daemon-served readiness waits - Stabilize dashboard focus assertion - Stabilize release integration gates - Stabilize release e2e markers - Stabilize release e2e flows Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Merged
pedronauck
added a commit
that referenced
this pull request
May 26, 2026
## Release v0.0.2 This PR prepares the release of version v0.0.2. ### Changelog ## 0.0.2 - 2026-05-26 ### Other Changes - Lessons learned ### ♻️ Refactoring - Project structure (#7) - Kb improvements (#12) - Rename spaces to channels (#17) - Add extensions gaps (#21) - Improve tool calls ui (#22) - Remove web app header - Module improvements (#29) - Memory improvements (#35) - Storybook for web and ui (#38) - Enable AGH network by default for new installs (#57) - Hermes adjustments (#69) - Badges design (#84) - Storybook scenario and logos gallery - Migrate typescript tests (#114) - Internal go packages (#120) - Ui patterns (#127) - Improve e2e tests (#130) - Ui redesign - Workspace isolation across runtime surfaces (#145) - Prod ready applies (#162) - Tool card ui (#164) - Alpha on logo - Prod ready features (#167) - Thread sheet (#202) ### 🎉 Features - Implement config foundation packages - Implement sqlite store package - Add ACP client package - Add session lifecycle manager - Implement observe package - Add daemon composition root - Add uds api server - Implement cli package - Add http api server - Add system design - Add foundation types, schemas, and layout shell for web client - Add daemon health polling and agent sidebar systems for web client - Add session system CRUD, streaming core, and session store for web client - Add chat view, messages, and composer tests for web client - Add tool cards and renderers for web client - Add file-backed memory store core - Scaffold memory session seams - Add memory dream consolidation service - Wire memory assembler into daemon - Add memory api and cli - New skills system (#1) - Add workspace entity (#5) - Add new skill capabilities (#8) - Web ui v2 (#9) - Improve hooks system (#10) - Session resilience (#11) - Add extensability (#13) - Add automation (#16) - Add channels (#14) - Add network implementation (#15) - Add network, bridges and automations web pages (#18) - Ext registry (#20) - Add core tasks (#19) - Bridge adapters (#23) - Add site (#26) - Add ext refac and sandbox (#25) - Settings ui (#37) - Tasks ui (#36) - Harness improvements (#44) - Agent capabilities (#49) - Redesign ui (#48) - Unify capability (#53) - Redesign network workspace (#59) - Add task deletion and split session delete from stop (#58) - Session provider selection (#60) - Production grade adjustments (#66) - Autonomous system (#75) - Add agent session route (#80) - Tools registry (#85) - Agents soul (#88) - Add network threads (#105) - Orchestration improvements (#106) - Memory v2 (#108) - Agent categories (#113) - Providers model (#118) - Add canonical AGH bundled skill (#143) - Onboarding and improvements (#198) - Onboarding and improvements (#201) ### 🐛 Bug Fixes - Review round - Review rounds - Resolve memory extensibility review batch - Embed web into daemon - Defaults agents - Acp integration (#4) - Lint errors - Prd folder - Remove orphan web actions and dead surfaces (#55) - Qa testing and fixes (#73) - New review rounds (#82) - Security audit (#90) - Release qa round (#95) - Add missing tools (#141) - New qa round (#147) - Advanced qa round (#149) - Homebrew tap - Final review round (#151) - Daemon healthy - Reasoning models (#158) - Lint errors (#160) - Review round (#168) - Release adjustments (#171) - Stabilize release ci fixtures - Stabilize release integration gate - Stabilize release verify gates - Stabilize release integration flows - Stabilize release verify gates - Stabilize main verify shutdown - Ignore stale acpmock cancel - Marketplace search focus and filtering (#193) - Website video - Workspace command select ### 📚 Documentation - Update agents.md - Update prd - Update skills - Update compozy tasks - Update compozy - Update compozy - Add new skills - Archive prd - Update prds - Update rfc - Update prds - Update prds - Add automation prd - Channels prd - Update prd - Update prd - New prds - Archive prds - Bridges adapters prd - Sandbox prd - Update - Archive prd - Update - Add new prd - New design - Update prd - Archive prds - Update prds - Tasks-ui prd tasks - Update prd - Update design docs - Agent capabilities prd - Improve site docs - Remove old design references - Udpate - Autonomous prd - Update skills - Blog design - Agent sould prd - Final qa plan - Update - Remove codex ledgers from gitignore - Remove not needed files - Udpate ledger - Update cy-codex-loop skill - Orchestration improves prd - Update prds - Orch improvs prd - Memv2 prd - Providers model prd - Update refacs prd - New design proposal - Update rules - Update skills - New blog posts (#173) - Format docs - Remove old design files - Remove old - Skeeper update ### 📦 Build System - Initial structure - Commitlint - Frontend base structure - Update vscode settings - Add subagents - Coderabbit - Prd and tooling - Bun lock - Lint tooling - Copy.md and tooling adjusts - Add repoclone rc - Upgrade skeeper to v0.2.0 - Update go.mod - Adopt task artifacts into skeeper - Sync codex plans with skeeper - Skeeper lock - Skeeper lock - New skills - Skeeper lock - Skeeper lock - Skeeper lock - Update deps and go - Regenerate daytona sidecar assets for go 1.26.3 - Fix cliff - Ignore docs on fmt - Build web assets before goreleaser - Extend release dry-run timeout ### 🔧 CI/CD - Lint errors - Fint release pr - Fix goreleaser - Fix release - Fix release process ### 🧪 Testing - Add e2e tests (#27) - Qa rounds (#78) - Improve test suite (#138) - Harden daemon-served restart reloads - Harden daemon-served readiness waits - Stabilize dashboard focus assertion - Stabilize release integration gates - Stabilize release e2e markers - Stabilize release e2e flows - Improve suite speed Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This was referenced May 26, 2026
Closed
Closed
Merged
pedronauck
added a commit
that referenced
this pull request
May 27, 2026
## Release v0.0.2 This PR prepares the release of version v0.0.2. ### Changelog ## 0.0.2 - 2026-05-26 ### Other Changes - Lessons learned ### ♻️ Refactoring - Project structure (#7) - Kb improvements (#12) - Rename spaces to channels (#17) - Add extensions gaps (#21) - Improve tool calls ui (#22) - Remove web app header - Module improvements (#29) - Memory improvements (#35) - Storybook for web and ui (#38) - Enable AGH network by default for new installs (#57) - Hermes adjustments (#69) - Badges design (#84) - Storybook scenario and logos gallery - Migrate typescript tests (#114) - Internal go packages (#120) - Ui patterns (#127) - Improve e2e tests (#130) - Ui redesign - Workspace isolation across runtime surfaces (#145) - Prod ready applies (#162) - Tool card ui (#164) - Alpha on logo - Prod ready features (#167) - Thread sheet (#202) ### 🎉 Features - Implement config foundation packages - Implement sqlite store package - Add ACP client package - Add session lifecycle manager - Implement observe package - Add daemon composition root - Add uds api server - Implement cli package - Add http api server - Add system design - Add foundation types, schemas, and layout shell for web client - Add daemon health polling and agent sidebar systems for web client - Add session system CRUD, streaming core, and session store for web client - Add chat view, messages, and composer tests for web client - Add tool cards and renderers for web client - Add file-backed memory store core - Scaffold memory session seams - Add memory dream consolidation service - Wire memory assembler into daemon - Add memory api and cli - New skills system (#1) - Add workspace entity (#5) - Add new skill capabilities (#8) - Web ui v2 (#9) - Improve hooks system (#10) - Session resilience (#11) - Add extensability (#13) - Add automation (#16) - Add channels (#14) - Add network implementation (#15) - Add network, bridges and automations web pages (#18) - Ext registry (#20) - Add core tasks (#19) - Bridge adapters (#23) - Add site (#26) - Add ext refac and sandbox (#25) - Settings ui (#37) - Tasks ui (#36) - Harness improvements (#44) - Agent capabilities (#49) - Redesign ui (#48) - Unify capability (#53) - Redesign network workspace (#59) - Add task deletion and split session delete from stop (#58) - Session provider selection (#60) - Production grade adjustments (#66) - Autonomous system (#75) - Add agent session route (#80) - Tools registry (#85) - Agents soul (#88) - Add network threads (#105) - Orchestration improvements (#106) - Memory v2 (#108) - Agent categories (#113) - Providers model (#118) - Add canonical AGH bundled skill (#143) - Onboarding and improvements (#198) - Onboarding and improvements (#201) ### 🐛 Bug Fixes - Review round - Review rounds - Resolve memory extensibility review batch - Embed web into daemon - Defaults agents - Acp integration (#4) - Lint errors - Prd folder - Remove orphan web actions and dead surfaces (#55) - Qa testing and fixes (#73) - New review rounds (#82) - Security audit (#90) - Release qa round (#95) - Add missing tools (#141) - New qa round (#147) - Advanced qa round (#149) - Homebrew tap - Final review round (#151) - Daemon healthy - Reasoning models (#158) - Lint errors (#160) - Review round (#168) - Release adjustments (#171) - Stabilize release ci fixtures - Stabilize release integration gate - Stabilize release verify gates - Stabilize release integration flows - Stabilize release verify gates - Stabilize main verify shutdown - Ignore stale acpmock cancel - Marketplace search focus and filtering (#193) - Website video - Workspace command select ### 📚 Documentation - Update agents.md - Update prd - Update skills - Update compozy tasks - Update compozy - Update compozy - Add new skills - Archive prd - Update prds - Update rfc - Update prds - Update prds - Add automation prd - Channels prd - Update prd - Update prd - New prds - Archive prds - Bridges adapters prd - Sandbox prd - Update - Archive prd - Update - Add new prd - New design - Update prd - Archive prds - Update prds - Tasks-ui prd tasks - Update prd - Update design docs - Agent capabilities prd - Improve site docs - Remove old design references - Udpate - Autonomous prd - Update skills - Blog design - Agent sould prd - Final qa plan - Update - Remove codex ledgers from gitignore - Remove not needed files - Udpate ledger - Update cy-codex-loop skill - Orchestration improves prd - Update prds - Orch improvs prd - Memv2 prd - Providers model prd - Update refacs prd - New design proposal - Update rules - Update skills - New blog posts (#173) - Format docs - Remove old design files - Remove old - Skeeper update ### 📦 Build System - Initial structure - Commitlint - Frontend base structure - Update vscode settings - Add subagents - Coderabbit - Prd and tooling - Bun lock - Lint tooling - Copy.md and tooling adjusts - Add repoclone rc - Upgrade skeeper to v0.2.0 - Update go.mod - Adopt task artifacts into skeeper - Sync codex plans with skeeper - Skeeper lock - Skeeper lock - New skills - Skeeper lock - Skeeper lock - Skeeper lock - Update deps and go - Regenerate daytona sidecar assets for go 1.26.3 - Fix cliff - Ignore docs on fmt - Build web assets before goreleaser - Extend release dry-run timeout ### 🔧 CI/CD - Lint errors - Fint release pr - Fix goreleaser - Fix release - Fix release process - Fix release sync - Decouple release dry-run npm auth - Persist web assets git auth ### 🧪 Testing - Add e2e tests (#27) - Qa rounds (#78) - Improve test suite (#138) - Harden daemon-served restart reloads - Harden daemon-served readiness waits - Stabilize dashboard focus assertion - Stabilize release integration gates - Stabilize release e2e markers - Stabilize release e2e flows - Improve suite speed <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated web assets dependency to a newer version for improved stability and performance. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/compozy/agh/pull/211?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
](https://app.coderabbit.ai/change-stack/compozy/agh/pull/211?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
This was referenced May 27, 2026
Closed
Closed
Merged
pedronauck
added a commit
that referenced
this pull request
May 27, 2026
## Release v0.0.2 This PR prepares the release of version v0.0.2. ### Changelog ## 0.0.2 - 2026-05-27 ### Other Changes - Lessons learned ### ♻️ Refactoring - Project structure (#7) - Kb improvements (#12) - Rename spaces to channels (#17) - Add extensions gaps (#21) - Improve tool calls ui (#22) - Remove web app header - Module improvements (#29) - Memory improvements (#35) - Storybook for web and ui (#38) - Enable AGH network by default for new installs (#57) - Hermes adjustments (#69) - Badges design (#84) - Storybook scenario and logos gallery - Migrate typescript tests (#114) - Internal go packages (#120) - Ui patterns (#127) - Improve e2e tests (#130) - Ui redesign - Workspace isolation across runtime surfaces (#145) - Prod ready applies (#162) - Tool card ui (#164) - Alpha on logo - Prod ready features (#167) - Thread sheet (#202) ### 🎉 Features - Implement config foundation packages - Implement sqlite store package - Add ACP client package - Add session lifecycle manager - Implement observe package - Add daemon composition root - Add uds api server - Implement cli package - Add http api server - Add system design - Add foundation types, schemas, and layout shell for web client - Add daemon health polling and agent sidebar systems for web client - Add session system CRUD, streaming core, and session store for web client - Add chat view, messages, and composer tests for web client - Add tool cards and renderers for web client - Add file-backed memory store core - Scaffold memory session seams - Add memory dream consolidation service - Wire memory assembler into daemon - Add memory api and cli - New skills system (#1) - Add workspace entity (#5) - Add new skill capabilities (#8) - Web ui v2 (#9) - Improve hooks system (#10) - Session resilience (#11) - Add extensability (#13) - Add automation (#16) - Add channels (#14) - Add network implementation (#15) - Add network, bridges and automations web pages (#18) - Ext registry (#20) - Add core tasks (#19) - Bridge adapters (#23) - Add site (#26) - Add ext refac and sandbox (#25) - Settings ui (#37) - Tasks ui (#36) - Harness improvements (#44) - Agent capabilities (#49) - Redesign ui (#48) - Unify capability (#53) - Redesign network workspace (#59) - Add task deletion and split session delete from stop (#58) - Session provider selection (#60) - Production grade adjustments (#66) - Autonomous system (#75) - Add agent session route (#80) - Tools registry (#85) - Agents soul (#88) - Add network threads (#105) - Orchestration improvements (#106) - Memory v2 (#108) - Agent categories (#113) - Providers model (#118) - Add canonical AGH bundled skill (#143) - Onboarding and improvements (#198) - Onboarding and improvements (#201) ### 🐛 Bug Fixes - Review round - Review rounds - Resolve memory extensibility review batch - Embed web into daemon - Defaults agents - Acp integration (#4) - Lint errors - Prd folder - Remove orphan web actions and dead surfaces (#55) - Qa testing and fixes (#73) - New review rounds (#82) - Security audit (#90) - Release qa round (#95) - Add missing tools (#141) - New qa round (#147) - Advanced qa round (#149) - Homebrew tap - Final review round (#151) - Daemon healthy - Reasoning models (#158) - Lint errors (#160) - Review round (#168) - Release adjustments (#171) - Stabilize release ci fixtures - Stabilize release integration gate - Stabilize release verify gates - Stabilize release integration flows - Stabilize release verify gates - Stabilize main verify shutdown - Ignore stale acpmock cancel - Marketplace search focus and filtering (#193) - Website video - Workspace command select ### 📚 Documentation - Update agents.md - Update prd - Update skills - Update compozy tasks - Update compozy - Update compozy - Add new skills - Archive prd - Update prds - Update rfc - Update prds - Update prds - Add automation prd - Channels prd - Update prd - Update prd - New prds - Archive prds - Bridges adapters prd - Sandbox prd - Update - Archive prd - Update - Add new prd - New design - Update prd - Archive prds - Update prds - Tasks-ui prd tasks - Update prd - Update design docs - Agent capabilities prd - Improve site docs - Remove old design references - Udpate - Autonomous prd - Update skills - Blog design - Agent sould prd - Final qa plan - Update - Remove codex ledgers from gitignore - Remove not needed files - Udpate ledger - Update cy-codex-loop skill - Orchestration improves prd - Update prds - Orch improvs prd - Memv2 prd - Providers model prd - Update refacs prd - New design proposal - Update rules - Update skills - New blog posts (#173) - Format docs - Remove old design files - Remove old - Skeeper update ### 📦 Build System - Initial structure - Commitlint - Frontend base structure - Update vscode settings - Add subagents - Coderabbit - Prd and tooling - Bun lock - Lint tooling - Copy.md and tooling adjusts - Add repoclone rc - Upgrade skeeper to v0.2.0 - Update go.mod - Adopt task artifacts into skeeper - Sync codex plans with skeeper - Skeeper lock - Skeeper lock - New skills - Skeeper lock - Skeeper lock - Skeeper lock - Update deps and go - Regenerate daytona sidecar assets for go 1.26.3 - Fix cliff - Ignore docs on fmt - Build web assets before goreleaser - Extend release dry-run timeout - Fix release dry-run token contract ### 🔧 CI/CD - Lint errors - Fint release pr - Fix goreleaser - Fix release - Fix release process - Fix release sync - Decouple release dry-run npm auth - Persist web assets git auth - Require npm auth before release merge ### 🧪 Testing - Add e2e tests (#27) - Qa rounds (#78) - Improve test suite (#138) - Harden daemon-served restart reloads - Harden daemon-served readiness waits - Stabilize dashboard focus assertion - Stabilize release integration gates - Stabilize release e2e markers - Stabilize release e2e flows - Improve suite speed <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated dependencies to latest versions. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/compozy/agh/pull/214?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
](https://app.coderabbit.ai/change-stack/compozy/agh/pull/214?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Tests