Skip to content

Commit 2eca439

Browse files
michaelmcneesclaude
andcommitted
fix: address pre-push reviewer feedback for git sync engine
- Clarify mantle repos Long now that sync ships - Split ActionGitSyncValidationFailed (parse) from ActionGitSyncApplyFailed (save) - Wire secret resolver into GoGitDriver.Auth for poller and CLI - Add NewAuthResolver helper with HTTPS token support - Document Poller single-snapshot behavior - Expand --from-dir help and auto_apply:false status hint Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent ca257f4 commit 2eca439

7 files changed

Lines changed: 223 additions & 13 deletions

File tree

packages/engine/internal/audit/audit.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,10 +67,11 @@ const (
6767
ActionRepoUpdated Action = "repo.updated"
6868
ActionRepoRemoved Action = "repo.removed"
6969

70-
ActionGitSyncStarted Action = "git.sync.started"
71-
ActionGitSyncCompleted Action = "git.sync.completed"
72-
ActionGitSyncFailed Action = "git.sync.failed"
73-
ActionGitSyncValidationError Action = "git.sync.validation_failed"
70+
ActionGitSyncStarted Action = "git.sync.started"
71+
ActionGitSyncCompleted Action = "git.sync.completed"
72+
ActionGitSyncFailed Action = "git.sync.failed"
73+
ActionGitSyncValidationFailed Action = "git.sync.validation_failed"
74+
ActionGitSyncApplyFailed Action = "git.sync.apply_failed"
7475
)
7576

7677
// Resource identifies the target of an audit event.

packages/engine/internal/cli/repos.go

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"github.com/dvflw/mantle/internal/db"
1111
"github.com/dvflw/mantle/internal/repo"
1212
"github.com/dvflw/mantle/internal/repo/sync"
13+
"github.com/dvflw/mantle/internal/secret"
1314
"github.com/spf13/cobra"
1415
)
1516

@@ -20,10 +21,10 @@ func newReposCommand() *cobra.Command {
2021
cmd := &cobra.Command{
2122
Use: "repos",
2223
Short: "Manage GitOps workflow source repositories",
23-
Long: `Registers GitOps source repositories whose workflow YAML definitions will be
24-
synced into this Mantle instance. This command manages the registry; the sync
25-
engine itself (sidecar, file discovery, validate/plan/apply) ships in a later
26-
milestone. Auth material is stored in a "git" credential type
24+
Long: `Registers GitOps source repositories whose workflow YAML definitions are
25+
synced into this Mantle instance. This command manages the registry; use
26+
` + "`mantle repos sync`" + ` for on-demand syncs, or start ` + "`mantle serve`" + ` to let the
27+
background poller run. Auth material is stored in a "git" credential type
2728
(` + "`mantle secrets create --type git`" + `) and referenced here by name.`,
2829
}
2930
cmd.AddCommand(newReposAddCommand())
@@ -123,7 +124,11 @@ func newReposStatusCommand() *cobra.Command {
123124
fmt.Fprintf(out, "Path: %s\n", r.Path)
124125
fmt.Fprintf(out, "Poll: %s\n", r.PollInterval)
125126
fmt.Fprintf(out, "Credential: %s\n", r.Credential)
126-
fmt.Fprintf(out, "Auto-Apply: %t\n", r.AutoApply)
127+
if r.AutoApply {
128+
fmt.Fprintln(out, "Auto-Apply: true")
129+
} else {
130+
fmt.Fprintln(out, "Auto-Apply: false (manual sync only — background poller does not run this repo)")
131+
}
127132
fmt.Fprintf(out, "Prune: %t\n", r.Prune)
128133
fmt.Fprintf(out, "Enabled: %t\n", r.Enabled)
129134
if r.LastSyncAt != nil {
@@ -229,7 +234,20 @@ of cloning (useful for tests and CI-driven deployments).`,
229234
if fromDir != "" {
230235
driver = &sync.NoopDriver{BasePath: fromDir}
231236
} else {
232-
driver = &sync.GoGitDriver{BasePath: filepath.Join(os.TempDir(), "mantle-repos")}
237+
var secretResolver *secret.Resolver
238+
if cfg := config.FromContext(cmd.Context()); cfg != nil && cfg.Encryption.Key != "" {
239+
encryptor, encErr := secret.NewEncryptor(cfg.Encryption.Key)
240+
if encErr != nil {
241+
return fmt.Errorf("configuring encryption for git sync: %w", encErr)
242+
}
243+
secretResolver = &secret.Resolver{
244+
Store: &secret.Store{DB: store.DB, Encryptor: encryptor},
245+
}
246+
}
247+
driver = &sync.GoGitDriver{
248+
BasePath: filepath.Join(os.TempDir(), "mantle-repos"),
249+
Auth: sync.NewAuthResolver(cmd.Context(), secretResolver),
250+
}
233251
}
234252
report, err := sync.SyncRepo(cmd.Context(), store.DB, store, r, driver)
235253
if err != nil {
@@ -245,6 +263,6 @@ of cloning (useful for tests and CI-driven deployments).`,
245263
return nil
246264
},
247265
}
248-
cmd.Flags().StringVar(&fromDir, "from-dir", "", "Use a pre-populated directory instead of cloning (testing / CI)")
266+
cmd.Flags().StringVar(&fromDir, "from-dir", "", "Path to an already-cloned repo directory (skips git pull; useful for CI pipelines or local testing)")
249267
return cmd
250268
}

packages/engine/internal/cli/serve.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,11 +155,22 @@ func newServeCommand() *cobra.Command {
155155
if artifactBase == "" {
156156
artifactBase = filepath.Join(os.TempDir(), "mantle-artifacts")
157157
}
158+
var secretResolver *secret.Resolver
159+
if cfg.Encryption.Key != "" {
160+
encryptor, encErr := secret.NewEncryptor(cfg.Encryption.Key)
161+
if encErr != nil {
162+
return fmt.Errorf("configuring encryption for git sync: %w", encErr)
163+
}
164+
secretResolver = &secret.Resolver{
165+
Store: &secret.Store{DB: database, Encryptor: encryptor},
166+
}
167+
}
158168
poller := &reposync.Poller{
159169
DB: database,
160170
Store: repoStore,
161171
Driver: &reposync.GoGitDriver{
162172
BasePath: filepath.Join(artifactBase, "git"),
173+
Auth: reposync.NewAuthResolver(ctx, secretResolver),
163174
},
164175
}
165176
go poller.Run(ctx)
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
package sync
2+
3+
import (
4+
"context"
5+
"fmt"
6+
7+
"github.com/dvflw/mantle/internal/secret"
8+
"github.com/go-git/go-git/v5/plumbing/transport"
9+
"github.com/go-git/go-git/v5/plumbing/transport/http"
10+
)
11+
12+
// authLookup is a function that resolves a credential name to its field map.
13+
// Defined as a type so tests can inject a fake without needing a live DB.
14+
type authLookup func(ctx context.Context, name string) (map[string]string, error)
15+
16+
// NewAuthResolver returns a callback suitable for GoGitDriver.Auth. The
17+
// resolver looks up a credential by name via the provided resolver, then
18+
// converts the git-type fields into a go-git AuthMethod.
19+
//
20+
// Only HTTPS tokens are supported in this release. SSH keys return an
21+
// explicit error so operators see a clear message instead of a silent
22+
// authentication failure — SSH support lands in Plan C.
23+
func NewAuthResolver(ctx context.Context, resolver *secret.Resolver) func(string) (transport.AuthMethod, error) {
24+
if resolver == nil {
25+
return newAuthResolverFromLookup(ctx, nil)
26+
}
27+
return newAuthResolverFromLookup(ctx, resolver.Resolve)
28+
}
29+
30+
// newAuthResolverFromLookup builds the Auth callback from any authLookup
31+
// function. It is the testable core; NewAuthResolver adapts the production
32+
// secret.Resolver to this interface.
33+
func newAuthResolverFromLookup(ctx context.Context, lookup authLookup) func(string) (transport.AuthMethod, error) {
34+
return func(name string) (transport.AuthMethod, error) {
35+
if lookup == nil {
36+
return nil, fmt.Errorf("secret resolver not configured")
37+
}
38+
fields, err := lookup(ctx, name)
39+
if err != nil {
40+
return nil, fmt.Errorf("resolving credential %q: %w", name, err)
41+
}
42+
if token := fields["token"]; token != "" {
43+
username := fields["username"]
44+
if username == "" {
45+
username = "x-token-auth"
46+
}
47+
return &http.BasicAuth{Username: username, Password: token}, nil
48+
}
49+
if fields["ssh_key"] != "" {
50+
return nil, fmt.Errorf("ssh_key credentials are not supported yet; use a token credential for now (SSH auth ships in Plan C)")
51+
}
52+
return nil, fmt.Errorf("credential %q has no token or ssh_key", name)
53+
}
54+
}
Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
package sync
2+
3+
import (
4+
"context"
5+
"errors"
6+
"testing"
7+
8+
"github.com/go-git/go-git/v5/plumbing/transport/http"
9+
)
10+
11+
func TestNewAuthResolver(t *testing.T) {
12+
ctx := context.Background()
13+
14+
t.Run("token credential returns BasicAuth with default username", func(t *testing.T) {
15+
lookup := func(_ context.Context, name string) (map[string]string, error) {
16+
return map[string]string{"token": "ghp_secret123"}, nil
17+
}
18+
fn := newAuthResolverFromLookup(ctx, lookup)
19+
auth, err := fn("my-cred")
20+
if err != nil {
21+
t.Fatalf("unexpected error: %v", err)
22+
}
23+
basic, ok := auth.(*http.BasicAuth)
24+
if !ok {
25+
t.Fatalf("expected *http.BasicAuth, got %T", auth)
26+
}
27+
if basic.Password != "ghp_secret123" {
28+
t.Errorf("password = %q, want %q", basic.Password, "ghp_secret123")
29+
}
30+
if basic.Username != "x-token-auth" {
31+
t.Errorf("username = %q, want %q (default)", basic.Username, "x-token-auth")
32+
}
33+
})
34+
35+
t.Run("token credential with explicit username preserves it", func(t *testing.T) {
36+
lookup := func(_ context.Context, name string) (map[string]string, error) {
37+
return map[string]string{"token": "ghp_secret123", "username": "mybot"}, nil
38+
}
39+
fn := newAuthResolverFromLookup(ctx, lookup)
40+
auth, err := fn("my-cred")
41+
if err != nil {
42+
t.Fatalf("unexpected error: %v", err)
43+
}
44+
basic, ok := auth.(*http.BasicAuth)
45+
if !ok {
46+
t.Fatalf("expected *http.BasicAuth, got %T", auth)
47+
}
48+
if basic.Username != "mybot" {
49+
t.Errorf("username = %q, want %q", basic.Username, "mybot")
50+
}
51+
})
52+
53+
t.Run("ssh_key credential returns unsupported error", func(t *testing.T) {
54+
lookup := func(_ context.Context, name string) (map[string]string, error) {
55+
return map[string]string{"ssh_key": "-----BEGIN OPENSSH PRIVATE KEY-----"}, nil
56+
}
57+
fn := newAuthResolverFromLookup(ctx, lookup)
58+
_, err := fn("my-ssh-cred")
59+
if err == nil {
60+
t.Fatal("expected error for ssh_key credential, got nil")
61+
}
62+
if !containsString(err.Error(), "ssh_key credentials are not supported") {
63+
t.Errorf("error = %q, want SSH not supported message", err.Error())
64+
}
65+
})
66+
67+
t.Run("credential with no token or ssh_key returns error", func(t *testing.T) {
68+
lookup := func(_ context.Context, name string) (map[string]string, error) {
69+
return map[string]string{"username": "someone"}, nil
70+
}
71+
fn := newAuthResolverFromLookup(ctx, lookup)
72+
_, err := fn("empty-cred")
73+
if err == nil {
74+
t.Fatal("expected error for empty credential, got nil")
75+
}
76+
if !containsString(err.Error(), "no token or ssh_key") {
77+
t.Errorf("error = %q, want 'no token or ssh_key' message", err.Error())
78+
}
79+
})
80+
81+
t.Run("nil lookup returns resolver-not-configured error", func(t *testing.T) {
82+
fn := newAuthResolverFromLookup(ctx, nil)
83+
_, err := fn("any-cred")
84+
if err == nil {
85+
t.Fatal("expected error for nil resolver, got nil")
86+
}
87+
if !containsString(err.Error(), "secret resolver not configured") {
88+
t.Errorf("error = %q, want 'secret resolver not configured'", err.Error())
89+
}
90+
})
91+
92+
t.Run("lookup error is wrapped and returned", func(t *testing.T) {
93+
lookup := func(_ context.Context, name string) (map[string]string, error) {
94+
return nil, errors.New("credential not found")
95+
}
96+
fn := newAuthResolverFromLookup(ctx, lookup)
97+
_, err := fn("missing-cred")
98+
if err == nil {
99+
t.Fatal("expected error, got nil")
100+
}
101+
if !containsString(err.Error(), "resolving credential") {
102+
t.Errorf("error = %q, want wrapping 'resolving credential'", err.Error())
103+
}
104+
if !containsString(err.Error(), "credential not found") {
105+
t.Errorf("error = %q, want wrapped 'credential not found'", err.Error())
106+
}
107+
})
108+
}
109+
110+
// containsString is a small helper to avoid importing strings in test assertions.
111+
func containsString(s, substr string) bool {
112+
return len(s) >= len(substr) && (s == substr || len(substr) == 0 ||
113+
func() bool {
114+
for i := 0; i+len(substr) <= len(s); i++ {
115+
if s[i:i+len(substr)] == substr {
116+
return true
117+
}
118+
}
119+
return false
120+
}())
121+
}

packages/engine/internal/repo/sync/engine.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ func SyncRepo(ctx context.Context, database *sql.DB, store *repo.Store, r *repo.
8383
emit(ctx, database, audit.Event{
8484
Timestamp: time.Now(),
8585
Actor: "sync",
86-
Action: audit.ActionGitSyncValidationError,
86+
Action: audit.ActionGitSyncValidationFailed,
8787
Resource: audit.Resource{Type: "git_repo", ID: r.ID},
8888
TeamID: teamID,
8989
Metadata: map[string]string{"name": r.Name, "file": f.RelPath, "error": parseErr.Error()},
@@ -96,7 +96,7 @@ func SyncRepo(ctx context.Context, database *sql.DB, store *repo.Store, r *repo.
9696
emit(ctx, database, audit.Event{
9797
Timestamp: time.Now(),
9898
Actor: "sync",
99-
Action: audit.ActionGitSyncValidationError,
99+
Action: audit.ActionGitSyncApplyFailed,
100100
Resource: audit.Resource{Type: "git_repo", ID: r.ID},
101101
TeamID: teamID,
102102
Metadata: map[string]string{"name": r.Name, "file": f.RelPath, "error": saveErr.Error()},

packages/engine/internal/repo/sync/poller.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,11 @@ type Poller struct {
3232
// the repo's poll_interval and calls SyncRepo. Returns when ctx is
3333
// cancelled — all child goroutines exit within one tick.
3434
func (p *Poller) Run(ctx context.Context) {
35+
// Run snapshots the repo list once at startup and launches one goroutine
36+
// per enabled auto-apply repo. Repos added or reconfigured after Run
37+
// begins will NOT be picked up until the server restarts. Live
38+
// reconfiguration is Plan C scope — the simpler snapshot design
39+
// keeps Plan B focused on proving the sync loop works.
3540
repos, err := p.Store.List(ctx)
3641
if err != nil {
3742
slog.ErrorContext(ctx, "poller: list repos failed", "err", err)

0 commit comments

Comments
 (0)