From 071eca4214a10bb06da129824b020fcee6cdb5db Mon Sep 17 00:00:00 2001 From: Snider Date: Tue, 17 Mar 2026 10:02:37 +0000 Subject: [PATCH] fix(coderabbit): address review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. forge/labels: add ErrLabelNotFound sentinel, EnsureLabel only creates on not-found 2. handlers/tick_parent: add comment explaining coreerr alias for go-log 3. repos/registry: wrap raw os errors with coreerr.E() in FindRegistry 4. go.mod: bump go 1.26.0 → 1.26.1 5. cmd/scm: pass locales.FS to cli.RegisterCommands 6. marketplace/builder: add Medium field, replace coreio.Local in loadFromDir/WriteIndex 7. marketplace/installer: capture and return DeleteAll error in Remove 8. cmd/scm/export: wrap manifest.Compile error with cli.WrapVerb 9. handlers/dispatch: add RepoOwner/RepoName/EpicNumber to early-success return 10. cmd/scm/compile: validate ed25519 key length before casting Co-Authored-By: Virgil --- cmd/scm/cmd_compile.go | 7 +++++++ cmd/scm/cmd_export.go | 2 +- cmd/scm/cmd_index.go | 3 ++- cmd/scm/cmd_scm.go | 3 ++- forge/labels.go | 11 ++++++++++- go.mod | 2 +- jobrunner/handlers/dispatch.go | 12 ++++++++---- jobrunner/handlers/tick_parent.go | 2 +- marketplace/builder.go | 31 +++++++++++++++++++++++-------- marketplace/builder_test.go | 5 +++-- marketplace/installer.go | 4 +++- repos/registry.go | 4 ++-- 12 files changed, 63 insertions(+), 23 deletions(-) diff --git a/cmd/scm/cmd_compile.go b/cmd/scm/cmd_compile.go index 1e5333f..44eb754 100644 --- a/cmd/scm/cmd_compile.go +++ b/cmd/scm/cmd_compile.go @@ -3,6 +3,7 @@ package scm import ( "crypto/ed25519" "encoding/hex" + "fmt" "os/exec" "strings" @@ -56,6 +57,12 @@ func runCompile(dir, signKeyHex, builtBy string) error { if err != nil { return cli.WrapVerb(err, "decode", "sign key") } + if len(keyBytes) != ed25519.PrivateKeySize { + return cli.WrapVerb( + fmt.Errorf("expected %d bytes, got %d", ed25519.PrivateKeySize, len(keyBytes)), + "validate", "sign key length", + ) + } opts.SignKey = ed25519.PrivateKey(keyBytes) } diff --git a/cmd/scm/cmd_export.go b/cmd/scm/cmd_export.go index dd4286c..58c69f2 100644 --- a/cmd/scm/cmd_export.go +++ b/cmd/scm/cmd_export.go @@ -45,7 +45,7 @@ func runExport(dir string) error { BuiltBy: "core scm export", }) if err != nil { - return err + return cli.WrapVerb(err, "compile", "manifest") } } diff --git a/cmd/scm/cmd_index.go b/cmd/scm/cmd_index.go index 2f1db2f..22b322a 100644 --- a/cmd/scm/cmd_index.go +++ b/cmd/scm/cmd_index.go @@ -5,6 +5,7 @@ import ( "path/filepath" "forge.lthn.ai/core/cli/pkg/cli" + coreio "forge.lthn.ai/core/go-io" "forge.lthn.ai/core/go-scm/marketplace" ) @@ -48,7 +49,7 @@ func runIndex(dirs []string, output, baseURL, org string) error { } absOutput, _ := filepath.Abs(output) - if err := marketplace.WriteIndex(absOutput, idx); err != nil { + if err := marketplace.WriteIndex(coreio.Local, absOutput, idx); err != nil { return err } diff --git a/cmd/scm/cmd_scm.go b/cmd/scm/cmd_scm.go index 4a7ad1b..ffc3820 100644 --- a/cmd/scm/cmd_scm.go +++ b/cmd/scm/cmd_scm.go @@ -9,10 +9,11 @@ package scm import ( "forge.lthn.ai/core/cli/pkg/cli" + "forge.lthn.ai/core/go-scm/locales" ) func init() { - cli.RegisterCommands(AddScmCommands) + cli.RegisterCommands(AddScmCommands, locales.FS) } // Style aliases from shared package. diff --git a/forge/labels.go b/forge/labels.go index 77a7b85..1910ced 100644 --- a/forge/labels.go +++ b/forge/labels.go @@ -1,6 +1,7 @@ package forge import ( + "errors" "strings" forgejo "codeberg.org/mvdkleijn/forgejo-sdk/forgejo/v2" @@ -8,6 +9,9 @@ import ( "forge.lthn.ai/core/go-log" ) +// ErrLabelNotFound is returned when a label cannot be found by name. +var ErrLabelNotFound = errors.New("label not found") + // ListOrgLabels returns all labels for repos in the given organisation. // Note: The Forgejo SDK does not have a dedicated org-level labels endpoint. // This lists labels from the first repo found, which works when orgs use shared label sets. @@ -74,15 +78,20 @@ func (c *Client) GetLabelByName(owner, repo, name string) (*forgejo.Label, error } } - return nil, log.E("forge.GetLabelByName", "label "+name+" not found in "+owner+"/"+repo, nil) + return nil, log.E("forge.GetLabelByName", "label "+name+" not found in "+owner+"/"+repo, ErrLabelNotFound) } // EnsureLabel checks if a label exists, and creates it if it doesn't. +// Only creates a new label when the error is ErrLabelNotFound; other +// errors (e.g. network failures) are returned immediately. func (c *Client) EnsureLabel(owner, repo, name, color string) (*forgejo.Label, error) { label, err := c.GetLabelByName(owner, repo, name) if err == nil { return label, nil } + if !errors.Is(err, ErrLabelNotFound) { + return nil, err + } return c.CreateRepoLabel(owner, repo, forgejo.CreateLabelOption{ Name: name, diff --git a/go.mod b/go.mod index c4ebb0b..fd822cd 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module forge.lthn.ai/core/go-scm -go 1.26.0 +go 1.26.1 require ( code.gitea.io/sdk/gitea v0.23.2 diff --git a/jobrunner/handlers/dispatch.go b/jobrunner/handlers/dispatch.go index 0ea7372..e6ff20e 100644 --- a/jobrunner/handlers/dispatch.go +++ b/jobrunner/handlers/dispatch.go @@ -109,10 +109,14 @@ func (h *DispatchHandler) Execute(ctx context.Context, signal *jobrunner.Pipelin if l.Name == LabelInProgress || l.Name == LabelAgentComplete { coreerr.Info("issue already processed, skipping", "issue", signal.ChildNumber, "label", l.Name) return &jobrunner.ActionResult{ - Action: "dispatch", - Success: true, - Timestamp: time.Now(), - Duration: time.Since(start), + Action: "dispatch", + RepoOwner: safeOwner, + RepoName: safeRepo, + EpicNumber: signal.EpicNumber, + ChildNumber: signal.ChildNumber, + Success: true, + Timestamp: time.Now(), + Duration: time.Since(start), }, nil } } diff --git a/jobrunner/handlers/tick_parent.go b/jobrunner/handlers/tick_parent.go index 6ed0b26..cc32152 100644 --- a/jobrunner/handlers/tick_parent.go +++ b/jobrunner/handlers/tick_parent.go @@ -8,7 +8,7 @@ import ( forgejosdk "codeberg.org/mvdkleijn/forgejo-sdk/forgejo/v2" - coreerr "forge.lthn.ai/core/go-log" + coreerr "forge.lthn.ai/core/go-log" // aliased coreerr: go-log exports E() for structured error creation "forge.lthn.ai/core/go-scm/forge" "forge.lthn.ai/core/go-scm/jobrunner" ) diff --git a/marketplace/builder.go b/marketplace/builder.go index 586bce5..ed5c3bb 100644 --- a/marketplace/builder.go +++ b/marketplace/builder.go @@ -25,6 +25,18 @@ type Builder struct { // Org is the default organisation used when constructing Repo URLs. Org string + + // Medium is the filesystem abstraction used for reading and writing. + // Falls back to coreio.Local if nil. + Medium coreio.Medium +} + +// medium returns the builder's Medium or falls back to coreio.Local. +func (b *Builder) medium() coreio.Medium { + if b.Medium != nil { + return b.Medium + } + return coreio.Local } // BuildFromDirs scans each directory for subdirectories containing either @@ -34,8 +46,9 @@ func (b *Builder) BuildFromDirs(dirs ...string) (*Index, error) { var modules []Module seen := make(map[string]bool) + m := b.medium() for _, dir := range dirs { - entries, err := os.ReadDir(dir) + entries, err := m.List(dir) if err != nil { if os.IsNotExist(err) { continue @@ -113,22 +126,24 @@ func BuildFromManifests(manifests []*manifest.Manifest) *Index { } // WriteIndex serialises an Index to JSON and writes it to the given path. -func WriteIndex(path string, idx *Index) error { - if err := coreio.Local.EnsureDir(filepath.Dir(path)); err != nil { +func WriteIndex(m coreio.Medium, path string, idx *Index) error { + if err := m.EnsureDir(filepath.Dir(path)); err != nil { return coreerr.E("marketplace.WriteIndex", "mkdir failed", err) } data, err := json.MarshalIndent(idx, "", " ") if err != nil { return coreerr.E("marketplace.WriteIndex", "marshal failed", err) } - return coreio.Local.Write(path, string(data)) + return m.Write(path, string(data)) } // loadFromDir tries core.json first, then falls back to .core/manifest.yaml. func (b *Builder) loadFromDir(dir string) (*manifest.Manifest, error) { + m := b.medium() + // Prefer compiled manifest (core.json). coreJSON := filepath.Join(dir, "core.json") - if raw, err := coreio.Local.Read(coreJSON); err == nil { + if raw, err := m.Read(coreJSON); err == nil { cm, err := manifest.ParseCompiled([]byte(raw)) if err != nil { return nil, coreerr.E("marketplace.Builder.loadFromDir", "parse core.json", err) @@ -138,16 +153,16 @@ func (b *Builder) loadFromDir(dir string) (*manifest.Manifest, error) { // Fall back to source manifest. manifestYAML := filepath.Join(dir, ".core", "manifest.yaml") - raw, err := coreio.Local.Read(manifestYAML) + raw, err := m.Read(manifestYAML) if err != nil { return nil, nil // No manifest — skip silently. } - m, err := manifest.Parse([]byte(raw)) + mf, err := manifest.Parse([]byte(raw)) if err != nil { return nil, coreerr.E("marketplace.Builder.loadFromDir", "parse manifest.yaml", err) } - return m, nil + return mf, nil } // repoURL constructs a module repository URL from the builder config. diff --git a/marketplace/builder_test.go b/marketplace/builder_test.go index 13e237e..6a84663 100644 --- a/marketplace/builder_test.go +++ b/marketplace/builder_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "testing" + coreio "forge.lthn.ai/core/go-io" "forge.lthn.ai/core/go-scm/manifest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -206,7 +207,7 @@ func TestWriteIndex_Good(t *testing.T) { }, } - err := WriteIndex(path, idx) + err := WriteIndex(coreio.Local, path, idx) require.NoError(t, err) data, err := os.ReadFile(path) @@ -231,7 +232,7 @@ func TestWriteIndex_Good_RoundTrip(t *testing.T) { idx, err := b.BuildFromDirs(root) require.NoError(t, err) - require.NoError(t, WriteIndex(path, idx)) + require.NoError(t, WriteIndex(coreio.Local, path, idx)) data, err := os.ReadFile(path) require.NoError(t, err) diff --git a/marketplace/installer.go b/marketplace/installer.go index e581bba..93b20c3 100644 --- a/marketplace/installer.go +++ b/marketplace/installer.go @@ -110,7 +110,9 @@ func (i *Installer) Remove(code string) error { } dest := filepath.Join(i.modulesDir, code) - _ = i.medium.DeleteAll(dest) + if err := i.medium.DeleteAll(dest); err != nil { + return coreerr.E("marketplace.Installer.Remove", "delete "+code, err) + } return i.store.Delete(storeGroup, code) } diff --git a/repos/registry.go b/repos/registry.go index 88f5808..ec698dd 100644 --- a/repos/registry.go +++ b/repos/registry.go @@ -106,7 +106,7 @@ func FindRegistry(m io.Medium) (string, error) { // Check current directory and parents dir, err := os.Getwd() if err != nil { - return "", err + return "", coreerr.E("repos.FindRegistry", "failed to get working directory", err) } for { @@ -131,7 +131,7 @@ func FindRegistry(m io.Medium) (string, error) { // Check home directory common locations home, err := os.UserHomeDir() if err != nil { - return "", err + return "", coreerr.E("repos.FindRegistry", "failed to get home directory", err) } commonPaths := []string{ -- 2.45.3