[agent/claude:opus] Fix CodeRabbit findings from PR review. Verify each against ... #4

Closed
Virgil wants to merge 1 commit from agent/fix-coderabbit-findings-from-pr-review into main
12 changed files with 63 additions and 23 deletions

View file

@ -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)
}

View file

@ -45,7 +45,7 @@ func runExport(dir string) error {
BuiltBy: "core scm export",
})
if err != nil {
return err
return cli.WrapVerb(err, "compile", "manifest")
}
}

View file

@ -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
}

View file

@ -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.

View file

@ -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,

2
go.mod
View file

@ -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

View file

@ -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
}
}

View file

@ -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"
)

View file

@ -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.

View file

@ -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)

View file

@ -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)
}

View file

@ -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{