Skip to content

feat(extension): add explicit source config - #9

Merged
omarluq merged 10 commits into
mainfrom
feat/extension-manager
May 13, 2026
Merged

feat(extension): add explicit source config#9
omarluq merged 10 commits into
mainfrom
feat/extension-manager

Conversation

@omarluq

@omarluq omarluq commented May 12, 2026

Copy link
Copy Markdown
Owner

Summary

  • add explicit extensions.use source config parsing
  • add version-first extension lockfile/resolver foundation
  • make extension loading resolve configured sources only
  • document extension manager architecture and commands

Validation

  • mise exec -- go test ./...
  • mise exec -- task build
  • mise exec -- task ci

@coderabbitai

coderabbitai Bot commented May 12, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 74626227-93b4-41d2-b052-6e0d23dc878c

📥 Commits

Reviewing files that changed from the base of the PR and between 664a905 and 42b8b53.

📒 Files selected for processing (3)
  • internal/config/extensions_test.go
  • internal/extension/lockfile_test.go
  • internal/extension/manager_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/extension/manager_test.go

📝 Walkthrough

Summary by CodeRabbit

Release Notes

  • New Features

    • Explicit extension dependency declaration via extensions.use configuration model with support for path, official, and GitHub sources
    • Version pinning through lockfile mechanism
    • Enhanced extension status reporting showing loaded/configured state
  • Documentation

    • New comprehensive extension manager specification
    • Updated extension API, runtime, and roadmap documentation
    • CLI reference expanded with complete extension subcommands

Walkthrough

This PR replaces the previous extensions.paths model with an explicit extensions.use configuration, adds parsing and resolution for scheme-based sources (path:, official:, github:) with a lockfile and install-root resolution, records configured vs loaded state in the extension service, updates CLI output to show configured/load status, and updates docs/tests/configs to the new model.

Changes

Extension Dependency Management System

Layer / File(s) Summary
Docs and CLI reference
README.md, config.example.yaml, docs/extension-api.md, docs/extension-manager.md, docs/extension-roadmap.md, docs/extension-runtime.md
Replace default-root prose with extensions.use examples, expand docs links and CLI command list, and document loader/manifest/lockfile behaviors.
CLI output and formatting
cmd/librecode/extension.go, cmd/librecode/config.go
extension list reports configured extensions from service state and prints load status; config show lists extensions.use entries via formatted source@version strings.
Source types and parsing
internal/extension/sources.go, internal/extension/sources_test.go
Introduce ConfiguredSource and SourceRef; ParseSourceRef enforces scheme:value and validates official, github, and path forms; tests cover accepted/rejected patterns and key/version outputs.
Lockfile model and persistence
internal/extension/lockfile.go, tests
Add LockFile and LockedExtension types with ReadLockFile/WriteLockFile helpers; missing lockfile treated as empty; atomic secure writes and round-trip tests.
Resolution and path derivation
internal/extension/resolver.go, internal/extension/paths.go, tests
Implement ResolveConfiguredSources producing ResolvedSource entries (Name, LoadPath, Status), scheme resolvers for path/official/github, and LocalLoadPaths with dedupe logic; tests validate resolution and unknown-scheme errors.
Config schema and unmarshalling
internal/config/config.go, internal/config/extensions.go, internal/config/loader.go, tests
ExtensionsConfig now holds Use []ExtensionUse (alias to ConfiguredSource); add unmarshalConfig with decodeExtensionUseHook to accept string/object forms, integrate into LoadResolved, add validation, update defaults, and add loader tests.
DI: ExtensionService integration and state
internal/extension/manager_state.go, internal/di/extension_service.go, internal/extension/manager.go, tests
Add ManagerState tracking configured and loaded extensions; NewExtensionService resolves configured sources via home+lock/install roots, computes canonical deduped load paths, loads manager paths when enabled, and records State; minor manifest display-path change and tests added.
Runtime/tests small updates
internal/assistant/*, internal/terminal/*, assorted tests
Ensure usage fields always emit in JSON for assistant runtime types and update test fixtures to use Extensions.Use fields.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 Extensions filed in tidy rows,
Sources and versions where the YAML shows,
Locks keep installs steady and bright,
Manager notes configured vs loaded right,
A rabbit hums — everything in sight.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat(extension): add explicit source config' directly describes the main change—adding explicit extension source configuration parsing as the primary objective.
Description check ✅ Passed The description clearly outlines the changeset: adding explicit extensions.use source config parsing, version-first lockfile/resolver foundation, configuring resolved source loading, and documenting the extension manager architecture.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/extension-manager

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/config/config.go`:
- Around line 176-184: The current validateExtensions (method
validateExtensions) only checks that each extensionUse.Source is non-empty;
update it to parse and validate the source format so malformed refs fail during
config validation. Inside validateExtensions iterate config.Extensions.Use and
for each extensionUse.Source run a parser/validator (e.g., url.Parse or your
project's extension ref parser) to verify scheme and expected shape; if parsing
fails return a descriptive fmt.Errorf("config: extensions.use source %q invalid:
%w", extensionUse.Source, err). Ensure you reference the validateExtensions
method and the Extensions.Use slice when adding the parse/validation logic.

In `@internal/config/extensions.go`:
- Around line 16-20: The decoder in mapstructure.NewDecoder currently only uses
decodeExtensionUseHook, so duration strings (fields like ConnMaxLifetime,
BaseDelay, MaxDelay, TTL, Timeout) won't be parsed; update the
DecoderConfig.DecodeHook to compose decodeExtensionUseHook with
mapstructure.StringToTimeDurationHookFunc (use
mapstructure.ComposeDecodeHookFunc to combine them) so that both ExtensionUse
decoding and string->time.Duration conversion occur when creating the decoder
variable via mapstructure.NewDecoder.

In `@internal/di/extension_service.go`:
- Around line 49-61: The current resolveExtensionSources function always reads
the global lockfile at core.LibrecodeHome()/extension.LockFileName, bypassing
any project-local lockfile; change it to prefer a project-local lockfile (e.g.,
a lockfile in the repo/working directory) if present, falling back to the home
lockfile otherwise: check for a local path for extension.LockFileName first,
call extension.ReadLockFile with that path when it exists, and only read the
home lockfile via core.LibrecodeHome() if no local lockfile is found before
passing the chosen lockFile into extension.ResolveConfiguredSources.
- Around line 64-71: The function extensionLoadPaths builds a slice of LoadPath
strings from resolvedSources but does not deduplicate them, allowing duplicate
local paths to be returned and causing double-loading; update extensionLoadPaths
to filter duplicates by tracking seen LoadPath values (e.g., using a map keyed
by resolvedSources[i].LoadPath) and only append when not already seen, so the
returned paths slice contains each LoadPath once.

In `@internal/extension/lockfile.go`:
- Around line 47-63: The WriteLockFile function currently writes directly to the
target path which can leave a partially written lockfile on crash; change it to
write atomically by writing the marshaled content to a temporary file in the
same directory (use os.CreateTemp or ioutil.TempFile with filepath.Dir(path)),
set the file mode to 0o600, close it, then atomically replace the target with
os.Rename(tempPath, cleanPath); keep the existing MkdirAll call, ensure you use
filepath.Clean(path) for the final rename target, and propagate any errors from
temp file creation, write, Chmod/Close, or Rename back to the caller and remove
the temp file on errors.

In `@internal/extension/sources.go`:
- Around line 86-94: The validateGitHubSource function currently only checks
subdir for path traversal but not the owner/repo components; update
validateGitHubSource (variables repo, parts, subdir) to reject any owner or repo
segment that equals ".." or contains ".." (e.g., "github:../repo" or
"github:owner/.."), returning the same error used for invalid source format;
ensure the new check runs after splitting parts but before accepting the value
so owner/repo traversal segments are refused.

In `@README.md`:
- Line 229: The README uses lowercase "github:" in the user-facing sentence
about extension sources; update the text to use the product-correct
capitalization "GitHub:" (e.g., change "github:" to "GitHub:") so the phrase
alongside "official:" and "path:" reads "official:, GitHub:, path:" and
maintains consistent casing in the user-facing docs.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: caf07552-a021-4fcd-9ab7-0818af91566e

📥 Commits

Reviewing files that changed from the base of the PR and between 6aafdcb and 35aa2ec.

📒 Files selected for processing (23)
  • README.md
  • cmd/librecode/config.go
  • cmd/librecode/extension.go
  • config.example.yaml
  • docs/extension-api.md
  • docs/extension-manager.md
  • docs/extension-roadmap.md
  • docs/extension-runtime.md
  • internal/assistant/runtime_test.go
  • internal/config/config.go
  • internal/config/extensions.go
  • internal/config/loader.go
  • internal/config/loader_test.go
  • internal/di/extension_service.go
  • internal/extension/lockfile.go
  • internal/extension/manager.go
  • internal/extension/manager_state.go
  • internal/extension/manager_test.go
  • internal/extension/paths.go
  • internal/extension/resolver.go
  • internal/extension/sources.go
  • internal/extension/sources_test.go
  • internal/terminal/render_parity_test.go

Comment thread internal/config/config.go
Comment thread internal/config/extensions.go
Comment thread internal/di/extension_service.go Outdated
Comment thread internal/di/extension_service.go
Comment thread internal/extension/lockfile.go Outdated
Comment thread internal/extension/sources.go
Comment thread README.md
@omarluq
omarluq force-pushed the feat/extension-manager branch from 35aa2ec to 60d47de Compare May 13, 2026 16:19
@omarluq
omarluq marked this pull request as draft May 13, 2026 16:21
@omarluq
omarluq force-pushed the feat/extension-manager branch from 60d47de to 3181dbd Compare May 13, 2026 18:28
@codecov-commenter

codecov-commenter commented May 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.33083% with 47 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.88%. Comparing base (216e1d4) to head (42b8b53).

Files with missing lines Patch % Lines
internal/extension/lockfile.go 54.34% 13 Missing and 8 partials ⚠️
internal/config/extensions.go 71.05% 9 Missing and 2 partials ⚠️
internal/di/extension_service.go 77.77% 5 Missing and 5 partials ⚠️
internal/config/loader.go 72.72% 2 Missing and 1 partial ⚠️
internal/extension/resolver.go 96.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main       #9      +/-   ##
==========================================
+ Coverage   55.16%   55.88%   +0.71%     
==========================================
  Files         152      156       +4     
  Lines       15030    15283     +253     
==========================================
+ Hits         8292     8541     +249     
+ Misses       5818     5809       -9     
- Partials      920      933      +13     
Flag Coverage Δ
unittests 55.88% <82.33%> (+0.71%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@omarluq
omarluq marked this pull request as ready for review May 13, 2026 19:25

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
internal/config/loader_test.go (1)

57-127: ⚡ Quick win

Prefer one table-driven test for these extensions.use parse/validation variants.

The three new cases share the same fixture/setup flow; consolidating them into subtests will reduce duplication and make it easier to add new schemes/edge cases.

As per coding guidelines "**/*_test.go: Prefer table-driven tests for core behavior and regression tests for terminal rendering bugs".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/config/loader_test.go` around lines 57 - 127, The three tests
(TestLoadParsesExtensionUseForms, TestLoadRejectsEmptyExtensionUseObject,
TestLoadRejectsInvalidExtensionUseSource) duplicate setup and exercise the same
parsing/validation behavior for extensions.use; refactor them into a single
table-driven test with subtests (e.g., TestLoadExtensionsUseCases) that iterates
cases containing: name, config YAML string, expected error boolean, expected
error substring (if any), and expected parsed fields for successful cases; reuse
the shared setup code (temp dirs, t.Setenv, t.Chdir, writeConfig) once per
subtest, call config.Load("") and assert using the case expectations (check
result.IsError()/result.Error() or inspect cfg.Extensions.Use entries and
duration strings) to replace the three separate test functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/di/extension_service_test.go`:
- Around line 42-45: The test creates a symlink with os.Symlink which fails on
Windows without elevation; update the test to guard and skip on Windows by
importing runtime and adding an early check like if runtime.GOOS == "windows" {
t.Skip("symlink requires elevated permissions on Windows") } before creating
symlinkPath; apply the same guard to the analogous test in
internal/extension/manager_test.go and leave the rest of the setup
(extensionDir, symlinkPath, require.NoError) unchanged.
- Line 32: The test cleanup currently discards the result of
container.ShutdownWithContext(t.Context()).Succeed which hides teardown
failures; change the cleanup to assert the shutdown succeeded by calling
assert.True(t, container.ShutdownWithContext(t.Context()).Succeed) inside the
t.Cleanup closure (e.g., t.Cleanup(func() { assert.True(t,
container.ShutdownWithContext(t.Context()).Succeed) })), and ensure the
testify/assert package is imported.

---

Nitpick comments:
In `@internal/config/loader_test.go`:
- Around line 57-127: The three tests (TestLoadParsesExtensionUseForms,
TestLoadRejectsEmptyExtensionUseObject,
TestLoadRejectsInvalidExtensionUseSource) duplicate setup and exercise the same
parsing/validation behavior for extensions.use; refactor them into a single
table-driven test with subtests (e.g., TestLoadExtensionsUseCases) that iterates
cases containing: name, config YAML string, expected error boolean, expected
error substring (if any), and expected parsed fields for successful cases; reuse
the shared setup code (temp dirs, t.Setenv, t.Chdir, writeConfig) once per
subtest, call config.Load("") and assert using the case expectations (check
result.IsError()/result.Error() or inspect cfg.Extensions.Use entries and
duration strings) to replace the three separate test functions.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4c991faa-f199-4c0f-a226-e3e748bc6ec6

📥 Commits

Reviewing files that changed from the base of the PR and between 60d47de and 0c0ad29.

📒 Files selected for processing (29)
  • README.md
  • cmd/librecode/config.go
  • cmd/librecode/extension.go
  • config.example.yaml
  • docs/extension-api.md
  • docs/extension-manager.md
  • docs/extension-roadmap.md
  • docs/extension-runtime.md
  • internal/assistant/client.go
  • internal/assistant/runtime.go
  • internal/assistant/runtime_test.go
  • internal/config/config.go
  • internal/config/extensions.go
  • internal/config/loader.go
  • internal/config/loader_test.go
  • internal/di/config_service.go
  • internal/di/config_service_test.go
  • internal/di/export_test.go
  • internal/di/extension_service.go
  • internal/di/extension_service_test.go
  • internal/extension/lockfile.go
  • internal/extension/manager.go
  • internal/extension/manager_state.go
  • internal/extension/manager_test.go
  • internal/extension/paths.go
  • internal/extension/resolver.go
  • internal/extension/sources.go
  • internal/extension/sources_test.go
  • internal/terminal/render_parity_test.go
✅ Files skipped from review due to trivial changes (4)
  • internal/assistant/runtime.go
  • internal/extension/manager_state.go
  • docs/extension-roadmap.md
  • internal/assistant/client.go
🚧 Files skipped from review as they are similar to previous changes (10)
  • cmd/librecode/config.go
  • config.example.yaml
  • internal/assistant/runtime_test.go
  • internal/extension/manager.go
  • internal/extension/sources_test.go
  • internal/config/config.go
  • cmd/librecode/extension.go
  • internal/extension/lockfile.go
  • internal/extension/paths.go
  • internal/extension/resolver.go

Comment thread internal/di/extension_service_test.go Outdated
Comment thread internal/di/extension_service_test.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (4)
internal/config/extensions_test.go (1)

14-49: ⚡ Quick win

Consolidate these parsing cases into a table-driven test.

These three tests validate the same core behavior with different fixtures; table-driving them will reduce duplication and make new source-shape cases easier to add.

♻️ Suggested refactor
-func TestLoadParsesExtensionUseStringForm(t *testing.T) {
-	t.Parallel()
-
-	cfg := loadExtensionConfigFromContent(t, `extensions:
-  use:
-    - " path:.librecode/extensions "
-`)
-
-	require.Len(t, cfg.Extensions.Use, 1)
-	assert.Equal(t, config.ExtensionUse{Source: "path:.librecode/extensions", Version: ""}, cfg.Extensions.Use[0])
-}
-
-func TestLoadParsesExtensionUseObjectForm(t *testing.T) {
-	t.Parallel()
-
-	cfg := loadExtensionConfigFromContent(t, `extensions:
-  use:
-    - source: " official:vim-mode "
-      version: " v0.1.0 "
-`)
-
-	require.Len(t, cfg.Extensions.Use, 1)
-	assert.Equal(t, config.ExtensionUse{Source: "official:vim-mode", Version: "v0.1.0"}, cfg.Extensions.Use[0])
-}
-
-func TestLoadParsesExtensionUseInlineObjectForm(t *testing.T) {
-	t.Parallel()
-
-	cfg := loadExtensionConfigFromContent(t, `extensions:
-  use:
-    - {source: "github:owner/repo", version: "v1.0.0"}
-`)
-
-	require.Len(t, cfg.Extensions.Use, 1)
-	assert.Equal(t, config.ExtensionUse{Source: "github:owner/repo", Version: "v1.0.0"}, cfg.Extensions.Use[0])
-}
+func TestLoadParsesExtensionUseForms(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name     string
+		content  string
+		expected config.ExtensionUse
+	}{
+		{
+			name: "string form",
+			content: `extensions:
+  use:
+    - " path:.librecode/extensions "
+`,
+			expected: config.ExtensionUse{Source: "path:.librecode/extensions", Version: ""},
+		},
+		{
+			name: "object form",
+			content: `extensions:
+  use:
+    - source: " official:vim-mode "
+      version: " v0.1.0 "
+`,
+			expected: config.ExtensionUse{Source: "official:vim-mode", Version: "v0.1.0"},
+		},
+		{
+			name: "inline object form",
+			content: `extensions:
+  use:
+    - {source: "github:owner/repo", version: "v1.0.0"}
+`,
+			expected: config.ExtensionUse{Source: "github:owner/repo", Version: "v1.0.0"},
+		},
+	}
+
+	for _, tc := range tests {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			cfg := loadExtensionConfigFromContent(t, tc.content)
+			require.Len(t, cfg.Extensions.Use, 1)
+			assert.Equal(t, tc.expected, cfg.Extensions.Use[0])
+		})
+	}
+}

As per coding guidelines, **/*_test.go: Prefer table-driven tests for core behavior and regression tests for terminal rendering bugs.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/config/extensions_test.go` around lines 14 - 49, Replace the three
separate tests TestLoadParsesExtensionUseStringForm,
TestLoadParsesExtensionUseObjectForm, and
TestLoadParsesExtensionUseInlineObjectForm with a single table-driven test that
iterates cases containing a name, YAML input, and expected config.ExtensionUse;
for each case call t.Run(case.name, func(t *testing.T){ t.Parallel(); cfg :=
loadExtensionConfigFromContent(t, case.input); require.Len(t,
cfg.Extensions.Use, 1); assert.Equal(t, case.expected, cfg.Extensions.Use[0])
}), referencing loadExtensionConfigFromContent, cfg.Extensions.Use, require.Len
and assert.Equal to keep the same assertions and behavior.
internal/extension/manager_test.go (1)

423-444: ⚡ Quick win

Prefer a table-driven test for the LocalLoadPaths core behavior cases.

Line 423 and Line 438 are tightly related success/error-path checks and are better maintained as a single table-driven test.

♻️ Suggested refactor
-func TestLocalLoadPathsParsesPathSources(t *testing.T) {
-	t.Parallel()
-
-	paths, err := extension.LocalLoadPaths([]extension.ConfiguredSource{
-		{Source: " " + testPathExtensionSource + " ", Version: ""},
-		{Source: "path:./custom", Version: ""},
-		{Source: testManagerVimModeSource, Version: ""},
-		{Source: "github:example/extension", Version: "v1.2.3"},
-		{Source: testPathExtensionSource, Version: ""},
-	})
-
-	require.NoError(t, err)
-	assert.Equal(t, []string{".librecode/extensions", "./custom"}, paths)
-}
-
-func TestLocalLoadPathsRejectsUnknownScheme(t *testing.T) {
-	t.Parallel()
-
-	_, err := extension.LocalLoadPaths([]extension.ConfiguredSource{{Source: "npm:thing", Version: ""}})
-
-	assert.ErrorContains(t, err, "unsupported source scheme")
-}
+func TestLocalLoadPaths(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name       string
+		input      []extension.ConfiguredSource
+		wantPaths  []string
+		wantErrSub string
+	}{
+		{
+			name: "parses path sources",
+			input: []extension.ConfiguredSource{
+				{Source: " " + testPathExtensionSource + " ", Version: ""},
+				{Source: "path:./custom", Version: ""},
+				{Source: testManagerVimModeSource, Version: ""},
+				{Source: "github:example/extension", Version: "v1.2.3"},
+				{Source: testPathExtensionSource, Version: ""},
+			},
+			wantPaths: []string{".librecode/extensions", "./custom"},
+		},
+		{
+			name:       "rejects unknown scheme",
+			input:      []extension.ConfiguredSource{{Source: "npm:thing", Version: ""}},
+			wantErrSub: "unsupported source scheme",
+		},
+	}
+
+	for _, tc := range tests {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			paths, err := extension.LocalLoadPaths(tc.input)
+			if tc.wantErrSub != "" {
+				assert.ErrorContains(t, err, tc.wantErrSub)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, tc.wantPaths, paths)
+		})
+	}
+}

As per coding guidelines, "Prefer table-driven tests for core behavior and regression tests for terminal rendering bugs".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/extension/manager_test.go` around lines 423 - 444, Combine the two
tests into a single table-driven test that iterates cases calling
extension.LocalLoadPaths and asserts either the expected []string result or an
expected error substring; replace TestLocalLoadPathsParsesPathSources and
TestLocalLoadPathsRejectsUnknownScheme with e.g. TestLocalLoadPaths (or a
renamed function) that defines cases for the successful parse (inputs including
" "+testPathExtensionSource+" ", "path:./custom", testManagerVimModeSource,
"github:example/extension", testPathExtensionSource expecting
[]string{".librecode/extensions","./custom"}) and for the failure case (input
"npm:thing" expecting an error containing "unsupported source scheme"), run each
case as t.Run and use require.NoError/assert.Equal for success cases and
assert.ErrorContains for error cases.
internal/extension/lockfile_test.go (2)

3-6: ⚡ Quick win

Make permission assertion OS-aware to avoid Windows test fragility.

The strict 0o600 equality check is POSIX-specific and may be unreliable on Windows. Gate it for non-Windows platforms (or assert a Windows-safe equivalent).

Suggested diff
 import (
 	"os"
 	"path/filepath"
+	"runtime"
 	"testing"
@@
 	info, err := os.Stat(path)
 	require.NoError(t, err)
-	assert.Equal(t, os.FileMode(0o600), info.Mode().Perm())
+	if runtime.GOOS != "windows" {
+		assert.Equal(t, os.FileMode(0o600), info.Mode().Perm())
+	}
 }

Also applies to: 66-69

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/extension/lockfile_test.go` around lines 3 - 6, Update the
POSIX-only permission assertion that compares file mode to 0o600 so it only runs
on non-Windows platforms: add an import for runtime in
internal/extension/lockfile_test.go and wrap the equality check (the assertion
that file mode == 0o600) in a conditional like if runtime.GOOS != "windows" {
... }, and for Windows either skip the strict equality or assert a Windows-safe
alternative (e.g., that the file exists or is readable); apply the same change
to the second occurrence of the 0o600 assertion in the file (the other
permission check at lines referenced in the review).

14-82: ⚡ Quick win

Prefer table-driven tests for lockfile behavior matrix.

These are core behavior checks and currently split into many near-identical single-case tests; converting to table-driven cases would reduce duplication and make scenario expansion simpler.

As per coding guidelines, **/*_test.go: Prefer table-driven tests for core behavior and regression tests for terminal rendering bugs.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/extension/lockfile_test.go` around lines 14 - 82, Combine the
several single-case tests into one table-driven test that iterates scenarios
describing input setup, the call (extension.ReadLockFile or
extension.WriteLockFile), and expected outcomes; create a test table with
entries for "missing file returns empty", "rejects invalid YAML", "normalizes
nil extensions", "reports read errors", "write initializes nil extensions and
sets 0600", and "write fails when parent is file", then loop with
t.Run(entry.name, func(t *testing.T){ t.Parallel(); setup temp dir, write
preconditions (e.g. file contents or parent-as-file), call the appropriate
function (ReadLockFile/WriteLockFile using extension.LockFileName), and assert
expected error strings and state (lockFile.Extensions, file perms) — this
centralizes logic, reduces duplication, and keeps the same assertions and use of
require/assert while preserving per-subtest parallelism.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/config/extensions_test.go`:
- Around line 14-49: Replace the three separate tests
TestLoadParsesExtensionUseStringForm, TestLoadParsesExtensionUseObjectForm, and
TestLoadParsesExtensionUseInlineObjectForm with a single table-driven test that
iterates cases containing a name, YAML input, and expected config.ExtensionUse;
for each case call t.Run(case.name, func(t *testing.T){ t.Parallel(); cfg :=
loadExtensionConfigFromContent(t, case.input); require.Len(t,
cfg.Extensions.Use, 1); assert.Equal(t, case.expected, cfg.Extensions.Use[0])
}), referencing loadExtensionConfigFromContent, cfg.Extensions.Use, require.Len
and assert.Equal to keep the same assertions and behavior.

In `@internal/extension/lockfile_test.go`:
- Around line 3-6: Update the POSIX-only permission assertion that compares file
mode to 0o600 so it only runs on non-Windows platforms: add an import for
runtime in internal/extension/lockfile_test.go and wrap the equality check (the
assertion that file mode == 0o600) in a conditional like if runtime.GOOS !=
"windows" { ... }, and for Windows either skip the strict equality or assert a
Windows-safe alternative (e.g., that the file exists or is readable); apply the
same change to the second occurrence of the 0o600 assertion in the file (the
other permission check at lines referenced in the review).
- Around line 14-82: Combine the several single-case tests into one table-driven
test that iterates scenarios describing input setup, the call
(extension.ReadLockFile or extension.WriteLockFile), and expected outcomes;
create a test table with entries for "missing file returns empty", "rejects
invalid YAML", "normalizes nil extensions", "reports read errors", "write
initializes nil extensions and sets 0600", and "write fails when parent is
file", then loop with t.Run(entry.name, func(t *testing.T){ t.Parallel(); setup
temp dir, write preconditions (e.g. file contents or parent-as-file), call the
appropriate function (ReadLockFile/WriteLockFile using extension.LockFileName),
and assert expected error strings and state (lockFile.Extensions, file perms) —
this centralizes logic, reduces duplication, and keeps the same assertions and
use of require/assert while preserving per-subtest parallelism.

In `@internal/extension/manager_test.go`:
- Around line 423-444: Combine the two tests into a single table-driven test
that iterates cases calling extension.LocalLoadPaths and asserts either the
expected []string result or an expected error substring; replace
TestLocalLoadPathsParsesPathSources and TestLocalLoadPathsRejectsUnknownScheme
with e.g. TestLocalLoadPaths (or a renamed function) that defines cases for the
successful parse (inputs including " "+testPathExtensionSource+" ",
"path:./custom", testManagerVimModeSource, "github:example/extension",
testPathExtensionSource expecting []string{".librecode/extensions","./custom"})
and for the failure case (input "npm:thing" expecting an error containing
"unsupported source scheme"), run each case as t.Run and use
require.NoError/assert.Equal for success cases and assert.ErrorContains for
error cases.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3bb093c8-63fe-4822-941e-561647a95190

📥 Commits

Reviewing files that changed from the base of the PR and between 0c0ad29 and 664a905.

📒 Files selected for processing (8)
  • internal/config/extensions_test.go
  • internal/config/loader_test.go
  • internal/di/export_test.go
  • internal/di/extension_service_test.go
  • internal/extension/lockfile_test.go
  • internal/extension/manager_test.go
  • internal/extension/resolver_test.go
  • internal/extension/sources_test.go
✅ Files skipped from review due to trivial changes (1)
  • internal/extension/resolver_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
  • internal/di/export_test.go
  • internal/config/loader_test.go
  • internal/di/extension_service_test.go
  • internal/extension/sources_test.go

coderabbitai[bot]
coderabbitai Bot previously approved these changes May 13, 2026
@omarluq
omarluq force-pushed the feat/extension-manager branch from b1bc76b to 42b8b53 Compare May 13, 2026 20:22
@sonarqubecloud

Copy link
Copy Markdown

@omarluq
omarluq merged commit 5dada19 into main May 13, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants