feat: git sync engine (Plan B of issue #16)#134
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 45 minutes and 4 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis pull request introduces a complete GitOps repository sync engine that enables automatic and manual synchronization of workflow repositories. It implements driver abstractions for flexible repository pulling, integrates background polling for auto-apply repos, reconciles configuration into the database, and provides CLI commands plus daemon startup integration. ChangesGitOps Repository Sync Engine
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as repos sync
participant Engine as SyncRepo
participant Driver as GoGitDriver/NoopDriver
participant Discover as Discover
participant Parser as Workflow Parser
participant Store as repo.Store
participant Audit as Audit
User->>CLI: mantle repos sync acme
CLI->>Store: Load repo config
CLI->>Engine: SyncRepo(ctx, db, store, repo, driver)
Engine->>Audit: emit: git.sync.started
Engine->>Driver: Pull(ctx, repo)
Driver->>Driver: clone/fetch/reset or return dir
Driver-->>Engine: PullResult{LocalPath, SHA}
Engine->>Discover: Discover(LocalPath, path)
Discover-->>Engine: []DiscoveredFile{RelPath, Bytes, Hash}
Engine->>Parser: Parse each YAML file
Parser->>Store: Save workflow if valid
Store-->>Parser: saved or unchanged
Engine->>Engine: count applied/unchanged/failures
Engine->>Store: UpdateSyncState(SHA, error summary)
Engine->>Audit: emit: git.sync.completed
Engine-->>CLI: Report{SHA, Applied, Unchanged, Failures}
CLI-->>User: Synced acme, 2 applied, 0 unchanged, 0 failures
sequenceDiagram
participant Startup as serve startup
participant Config as config
participant Reconcile as Reconcile
participant Store as repo.Store
participant Poller as Poller
participant Engine as SyncRepo
Startup->>Config: Load git_sync.repos config
Startup->>Reconcile: Reconcile(ctx, store, repos)
Reconcile->>Store: Get/Create/Update per config
Store-->>Reconcile: complete
Startup->>Poller: New Poller(DB, Store, Driver)
Startup->>Poller: Run(ctx) in goroutine
Poller->>Poller: Snapshot enabled auto_apply repos
Poller->>Poller: Start ticker goroutine per repo
loop On each tick
Poller->>Engine: SyncRepo(ctx, db, store, repo, driver)
Engine->>Engine: Pull/Discover/Parse/Save
Engine-->>Poller: Report
Poller->>Poller: OnSync(repo, report, err)
end
Poller-->>Startup: (runs in background)
Startup-->>Startup: Continue server initialization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes This PR introduces substantial new functionality across the git sync subsystem: multiple driver implementations, authentication handling, file discovery, the core sync orchestration engine, background polling, configuration reconciliation, and CLI/daemon integration. The changes span diverse responsibilities (drivers, auth, discovery, storage, orchestration, scheduling) with moderate logic density and cross-cutting dependencies. Review requires understanding the driver contract, sync flow through the engine, polling mechanics, config reconciliation, and integration with CLI and daemon startup. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
9 tasks
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds UpdateSyncState(ctx, id, sha, syncErr) to the repo Store so the sync engine (Task 7) can record the SHA, timestamp, and error summary into git_repos.last_sync_* after each attempt. Empty syncErr is stored as NULL, clearing any previous failure on a successful sync. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
On `mantle serve`, after the lifecycle context is established: reconcile all git_sync.repos entries from config into the DB (fatal on mismatch), then launch a Poller with GoGitDriver rooted at <storage.path>/git (or a tmp fallback). The poller goroutine exits cleanly when ctx is cancelled. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- 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>
filepath.WalkDir can encounter symlinks; reading via the symlink path is TOCTOU-prone (CWE-367, gosec G122). Checking d.Type().IsRegular() before os.ReadFile skips symlinks, device files, and other non-regular entries, which are not valid workflow YAML anyway. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
michaelmcnees
force-pushed
the
feat/git-sync-engine
branch
from
36aa741 to
37f6147
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/engine/internal/repo/sync/poller.go (1)
83-96: 💤 Low value
time.NewTickerdelays the first sync by a full interval.With the production 60s default (or 10s floor), no sync runs until the first interval elapses after startup. If a prompt initial sync on boot is desired, fire once before entering the loop.
🤖 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 `@packages/engine/internal/repo/sync/poller.go` around lines 83 - 96, The poller currently creates a time.NewTicker and waits a full interval before the first SyncRepo call, delaying initial sync; to fix, invoke SyncRepo(ctx, p.DB, p.Store, r, p.Driver) once immediately after creating the ticker (and before the for/select) and call p.OnSync(r, report, syncErr) if set, while still honoring ctx.Done; keep the existing ticker loop intact for subsequent periodic syncs so NewTicker/ticker.Stop and the select remain unchanged.packages/engine/internal/cli/serve.go (2)
168-179: 💤 Low valuePoller goroutine isn't joined on shutdown.
go poller.Run(ctx)is fire-and-forget; whensrv.Start(ctx)returns after ctx cancellation, the process can exit before the poller's in-flightSyncRepocompletes. If clean drain of an in-progress sync matters, wait forRunto return (e.g., via a channel/WaitGroup) before returning fromRunE.🤖 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 `@packages/engine/internal/cli/serve.go` around lines 168 - 179, The poller goroutine started with go poller.Run(ctx) is not awaited, so when srv.Start(ctx) returns on cancellation the process may exit before reposync.Poller.Run completes; modify the serve command (RunE) to start Run in a managed goroutine and wait for it to finish before returning: create a sync.WaitGroup or a done channel, increment it before calling poller.Run(ctx) (or run poller.Run in a goroutine that signals the done channel), and defer or select to wait for that signal after srv.Start(ctx) returns (and/or when ctx is canceled) so reposync.Poller.Run has completed; reference reposync.Poller, poller.Run, and srv.Start in your changes.
158-167: 💤 Low valueEncryptor is constructed twice from the same key.
Lines 82-90 already build
eng.Resolver(with its ownsecret.Encryptor) whencfg.Encryption.Key != "". This block repeats the sameNewEncryptor+Resolverconstruction. Reuse a single resolver to avoid the duplicate decryption setup and keep the two wirings from drifting.🤖 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 `@packages/engine/internal/cli/serve.go` around lines 158 - 167, The code is creating a second secret.Encryptor and secret.Resolver when cfg.Encryption.Key != "" (using secret.NewEncryptor and instantiating secret.Resolver/secret.Store) which duplicates the earlier eng.Resolver construction; instead reuse the already-created eng.Resolver (or its Store/Encryptor) rather than calling secret.NewEncryptor again — replace the duplicate NewEncryptor + secret.Resolver/secret.Store construction with an assignment from the existing eng.Resolver (e.g., secretResolver = eng.Resolver or secretResolver = &eng.Resolver.Store as appropriate) so only one Encryptor/Resolver is instantiated and wired throughout.packages/engine/internal/repo/store.go (1)
279-296: ⚖️ Poor tradeoff
UpdateSyncStatewrites state without an in-transaction audit event, diverging from the Store's stated invariant.The Store doc (lines 15-17) promises every state-changing method emits an audit event in the same transaction "so audit log and state never drift."
UpdateSyncStatemutatesgit_reposbut emits nothing — the engine emitsgit.sync.*events in a separate transaction via its ownemithelper, so this particular write isn't covered by the same-tx guarantee. If that's an intentional exception (sync state is operational, not security-relevant), consider tightening the Store comment to scope the invariant; otherwise fold the audit emission into this write.As per coding guidelines: "Emit audit events via the AuditEmitter interface for every state-changing operation."
🤖 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 `@packages/engine/internal/repo/store.go` around lines 279 - 296, UpdateSyncState currently updates git_repos without emitting an audit event in the same DB transaction, violating the Store invariant; change it to perform the update and the audit emission inside a single transaction: start a tx (e.g., s.DB.BeginTx(ctx,...)), run the UPDATE using tx.ExecContext, call the Store's AuditEmitter method (AuditEmitter.EmitAudit or the concrete emit helper) to record the git.sync.* audit event with repo id, sha and syncErr using the same tx, rollback on any error and commit on success, and preserve the existing error-wrapping behavior (keep function name UpdateSyncState and use the same parameters). Ensure the audit emission uses the AuditEmitter interface so the audit and state change cannot drift.
🤖 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 `@packages/engine/internal/cli/repos_test.go`:
- Line 179: The test calls reposCtx incorrectly expecting two returns;
reposCtx(t) returns a single *config.Config. Replace the tuple assignment "_,
cfg := reposCtx(t)" with a single assignment "cfg := reposCtx(t)" (update the
variable name usage in the test to match cfg) so the function return count
matches the assignment and the package compiles.
---
Nitpick comments:
In `@packages/engine/internal/cli/serve.go`:
- Around line 168-179: The poller goroutine started with go poller.Run(ctx) is
not awaited, so when srv.Start(ctx) returns on cancellation the process may exit
before reposync.Poller.Run completes; modify the serve command (RunE) to start
Run in a managed goroutine and wait for it to finish before returning: create a
sync.WaitGroup or a done channel, increment it before calling poller.Run(ctx)
(or run poller.Run in a goroutine that signals the done channel), and defer or
select to wait for that signal after srv.Start(ctx) returns (and/or when ctx is
canceled) so reposync.Poller.Run has completed; reference reposync.Poller,
poller.Run, and srv.Start in your changes.
- Around line 158-167: The code is creating a second secret.Encryptor and
secret.Resolver when cfg.Encryption.Key != "" (using secret.NewEncryptor and
instantiating secret.Resolver/secret.Store) which duplicates the earlier
eng.Resolver construction; instead reuse the already-created eng.Resolver (or
its Store/Encryptor) rather than calling secret.NewEncryptor again — replace the
duplicate NewEncryptor + secret.Resolver/secret.Store construction with an
assignment from the existing eng.Resolver (e.g., secretResolver = eng.Resolver
or secretResolver = &eng.Resolver.Store as appropriate) so only one
Encryptor/Resolver is instantiated and wired throughout.
In `@packages/engine/internal/repo/store.go`:
- Around line 279-296: UpdateSyncState currently updates git_repos without
emitting an audit event in the same DB transaction, violating the Store
invariant; change it to perform the update and the audit emission inside a
single transaction: start a tx (e.g., s.DB.BeginTx(ctx,...)), run the UPDATE
using tx.ExecContext, call the Store's AuditEmitter method
(AuditEmitter.EmitAudit or the concrete emit helper) to record the git.sync.*
audit event with repo id, sha and syncErr using the same tx, rollback on any
error and commit on success, and preserve the existing error-wrapping behavior
(keep function name UpdateSyncState and use the same parameters). Ensure the
audit emission uses the AuditEmitter interface so the audit and state change
cannot drift.
In `@packages/engine/internal/repo/sync/poller.go`:
- Around line 83-96: The poller currently creates a time.NewTicker and waits a
full interval before the first SyncRepo call, delaying initial sync; to fix,
invoke SyncRepo(ctx, p.DB, p.Store, r, p.Driver) once immediately after creating
the ticker (and before the for/select) and call p.OnSync(r, report, syncErr) if
set, while still honoring ctx.Done; keep the existing ticker loop intact for
subsequent periodic syncs so NewTicker/ticker.Stop and the select remain
unchanged.
🪄 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: 4134c6b7-8f29-4b3d-8d4d-b9aff7a739bd
⛔ Files ignored due to path filters (1)
packages/engine/go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
CLAUDE.mdpackages/engine/go.modpackages/engine/internal/audit/audit.gopackages/engine/internal/cli/repos.gopackages/engine/internal/cli/repos_test.gopackages/engine/internal/cli/serve.gopackages/engine/internal/repo/store.gopackages/engine/internal/repo/store_test.gopackages/engine/internal/repo/sync/auth.gopackages/engine/internal/repo/sync/auth_test.gopackages/engine/internal/repo/sync/discover.gopackages/engine/internal/repo/sync/discover_test.gopackages/engine/internal/repo/sync/driver.gopackages/engine/internal/repo/sync/engine.gopackages/engine/internal/repo/sync/engine_test.gopackages/engine/internal/repo/sync/gogit_driver.gopackages/engine/internal/repo/sync/gogit_driver_test.gopackages/engine/internal/repo/sync/noop_driver.gopackages/engine/internal/repo/sync/noop_driver_test.gopackages/engine/internal/repo/sync/poller.gopackages/engine/internal/repo/sync/poller_test.gopackages/engine/internal/repo/sync/reconcile.gopackages/engine/internal/repo/sync/reconcile_test.go
…heck
Gosec G122 (CWE-367) fires on os.ReadFile(path) inside filepath.WalkDir
because the string path is race-prone (symlink TOCTOU). The fix opens the
target directory via os.OpenRoot (Go 1.24+) which uses openat-style
syscalls, then reads each file relative to that root — eliminating the
TOCTOU window without changing the traversal logic.
Also adds 10 go-git transitive dependency vulnerabilities to the
govulncheck exclusion list. govulncheck confirms the call chains are not
reachable from application code ("your code doesn't appear to call").
Track upstream at github.com/go-git/go-git and remove exclusions when
go-git ships fixed dependency versions.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
Plan B of 3 for issue #16. Stacked on #133 (Plan A foundation). Ships the auto-apply sync engine end-to-end.
internal/repo/syncpackage:Driverinterface withNoopDriver(tests / CI-driven deployments) andGoGitDriver(standalone, viagithub.com/go-git/go-git/v5).Discoverwalks YAML files and hashes them.SyncRepoorchestrates pull → discover → parse + save per file.Reconcilematerializesgit_sync.reposconfig intogit_reposrows at startup.Pollerruns one goroutine per enabledauto_apply: truerepo.Store.UpdateSyncStatewriteslast_sync_sha,last_sync_at,last_sync_errorafter each sync attempt.mantle repos sync <name>CLI with--from-dirflag for CI / test scenarios (usesNoopDriver).mantle serve: reconciles thegit_sync.reposconfig block, then launches the poller goroutine tied to the signal-aware context.NewAuthResolverlooks upgit-type credentials and converts HTTPS tokens intohttp.BasicAuthfor go-git. SSH keys return an explicit "not supported yet" error (ships in Plan C).git.sync.started,git.sync.completed,git.sync.failed,git.sync.validation_failed(parse errors),git.sync.apply_failed(workflow.Save errors).What operators can do today (A + B together)
mantle repos addorgit_sync.repos:inmantle.yamlmantle serve— the poller syncs eachauto_apply: truerepo everypoll_interval, applying changed workflow YAML as new immutable versionsmantle repos sync <name>mantle repos status <name>— SHA, timestamp, truncated error summaryPre-push review fixes (commit 2eca439)
Four reviewer agents ran before push (Technical Writer, Product Manager, Legal Compliance, Reality Checker). Consolidated fixes:
mantle reposhelp now that it's actually hereActionGitSyncValidationFailed(parse errors) fromActionGitSyncApplyFailed(save errors) — one audit consumer, two root-cause categoriesNewAuthResolverinto both the poller and therepos syncCLI so private repos authenticate instead of silently failingauto_apply: falsehint tomantle repos statusoutput so operators know the background poller skips those reposPoller.Runsnapshots the repo list at startup (live reconfig ships in Plan C)--from-dirflag helpPlan C TODOs (tracked, not blocking)
<artifact>/git/<repo-id>/working tree whenmantle repos removeis called (SOC 2 CC6.5 disposal)err.Error()before placing it ingit.sync.failedaudit metadata in case go-git echoes the remote URLstorage.pathshould be explicit in production (theos.TempDir()fallback has umask-dependent permissions)git_reposchangesmantle repos updateCLI (store hasUpdate; only the CLI surface is missing)auto_apply: falseplan/apply flow, webhook receiver, REST API, metricsTest plan
go test ./...inpackages/engine— all greengo test ./internal/repo/sync/— 14 tests (Driver, Discover, SyncRepo, Reconcile, Poller, NewAuthResolver)go test ./internal/repo/— 15 tests (Plan A CRUD + UpdateSyncState)go test ./internal/cli/— includesTestReposSync_UsesNoopDriverWithFromDirgo build ./cmd/mantle— cleanmantle serveagainst a repo withgit_sync.repos:inmantle.yaml, watchlast_sync_atpopulate on each poll tickmantle repos sync <name>against a private GitHub repo with a PAT credentialCloses part of #16. Plan C follows.
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
mantle repos sync <name>command to trigger immediate repository synchronizationmantle servefor repositories with auto-apply enabled