fix: defects found by a full review of internal/ and main.go (#48)

* fix(deps): patch the high severity brace-expansion advisory

The Azure DevOps extension pinned brace-expansion to 5.0.8 through an
overrides entry, and 5.0.8 is the version the current advisory covers: a
denial of service through unbounded intermediate arrays that bypasses the
earlier CVE-2026-14257 mitigation. The pin that once fixed a problem had
become the problem, and nothing would move it because an override is
absolute.

Bumped to 5.0.9, the first patched release and the current latest, with the
lockfile regenerated to match.

* chore(ci): update the actions CI exercises to their current majors

Every run warned that actions/checkout and actions/setup-go target Node 20,
which GitHub now force-runs on Node 24 and will stop supporting. Moved to
checkout v7, setup-go v7, and upload-artifact v7 in ci.yml and
action-smoke.yml, which this pull request exercises.

The release, helm publish, and Azure DevOps extension workflows are left
alone deliberately. They run only on a tag or a manual dispatch, so no pull
request proves a bump there, and a broken publish workflow is discovered at
the worst possible moment. They are worth updating in a change that can be
dispatched and watched.

* fix: repository paths and shared state in destination pull request sync

Three defects that each made a whole path unusable.

The engine handed prsync the rendered destination name, which is owner
relative because CloneURL and EnsureRepo prepend the owner themselves.
Every destination pull request and comment endpoint interpolates that value
as a fully qualified path, so a GitHub destination built "/repos/widget/pulls"
instead of "/repos/acme/widget/pulls" and answered 404 for every call.
mirror_objects could not work at all against GitHub, GitLab, or Bitbucket.
Remote gains QualifiedPath, each provider implements the convention its own
ListRepos reports, and the engine qualifies the name before passing it on.
The existing tests passed because their fake used the broken convention.

Azure DevOps went wrong the other way: ListRepos reports project/name and
every URL already carries the project as its own segment, so interpolating
the qualified path whole produced a duplicated project and 404ed on every
repository. Its pull request routes now use the repository segment alone.

internal/prsync wrote pull request records into the shared GitState from
inside the engine's worker pool, bypassing the lock the engine uses for its
own access and documents as necessary. Two repositories finishing together
was a concurrent map write, which aborts the process and cannot be
recovered by the worker that caused it. GitState now guards its own maps,
so no caller has to remember.

* fix(registry): use the Docker Hub session token instead of basic auth

Authenticate logged in and threw the response away, so the JWT the Hub API
authenticates with was never kept. Every later listing fell back to HTTP
basic auth, which that API does not accept: a private repository with
watch_tags on answered 404 after three retries, reported against the image
rather than against the credentials.

The session token is stored and sent as a bearer credential, ahead of a
configured personal access token, with basic auth left as a last resort for
any deployment still relying on it. An anonymous registry sends nothing.

* fix(prsync): close vanished pull requests, and stop losing work on failure

Sources are listed open only, so a merged or closed pull request does not
arrive with a finished state: it stops appearing. The close path was
therefore unreachable, and a destination pull request stayed open forever
while the mirror push pruned the branch under it. Records the source no
longer lists are now reconciled and closed with a note that says only what
is knowable from an open listing.

GitHub keeps discussion comments and review comments in separate id spaces
addressed through different routes, and both were updated through the
issues route. Every pull request with a mirrored inline comment failed its
conversation sync with a 404 on every run after the first, or edited an
unrelated comment if the ids collided. The id now carries which space it
belongs to, and a bare id from an older state file still resolves to the
discussion route.

A conversation that failed part way discarded the ids of the comments it
had already posted, because the state write came after the error return, so
the next run reposted all of them, forever. What succeeded is recorded
before giving up.

The repository fingerprint was recorded before the pull request pass ran,
so a failed pass was marked done and skipped on the next run until an
unrelated branch or tag happened to move. It is recorded after, and only
when the pass had no failures.

A pull request head that failed to fetch was dropped from the keep set,
which pruned it from the cache and then deleted it at the destination. One
transient network error removed a correct branch and pulled the head out
from under its mirrored pull request. The previous copy is kept instead.

* fix: cron overlap, signal handling, glob validation, and Slack block limits

The image sync cron had no SkipIfStillRunning, which the git cron has along
with a comment explaining the hazard. A run that outlives its interval
overlapped itself on a Syncer that keeps per run state and a state map with
no locking: the second run's report was voided by the first, and two
writers on that map abort the process. Both cron loops now also register
the schedule before the first run, so a typo is caught immediately instead
of after a full sync of every image, and install the signal handler before
the first run, so a SIGTERM during it lets the run finish and persist
rather than killing the process with nothing saved.

A malformed exclude glob was silently ignored, so a repository somebody
wrote a rule to keep out was mirrored to the destination. Patterns are
validated at config load, and a pattern that cannot be parsed now drops the
repository rather than mirroring it.

A Slack section block could be emitted empty, and could exceed the size
limit, from a single oversized failure line such as a transport error
carrying a multi kilobyte HTML body. Either one makes Slack answer 400 and
the entire notification is lost, which matters most for the failure alert
that is the reason the integration exists. Sections are bounded at both
ends, and truncate now counts characters rather than bytes so it neither
trims non-ASCII text far short of the limit nor splits a rune.

A failed state save on the fail fast path was discarded, so every image
copied before the abort was re-pulled and re-pushed on the next run with
nothing in the output to say why. The same failure is fatal on the normal
exit path; it is at least reported here.

* docs: record the review fixes in the changelog

---------

Co-authored-by: Anmol Nagpal <ianmolnagpal@gmail.com>

Author: clouddrove-ci

.github/workflows/action-smoke.yml

@@ -9,7 +9,7 @@ jobs:
   action:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v4
+      - uses: actions/checkout@v7
 
       # The uses: ./ steps below cannot assert behaviour, because every
       # scenario we can run without real registries or git remotes fails

.github/workflows/ci.yml

@@ -10,10 +10,10 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - name: Checkout
-        uses: actions/checkout@v4
+        uses: actions/checkout@v7
 
       - name: Set up Go
-        uses: actions/setup-go@v5
+        uses: actions/setup-go@v7
         with:
           go-version: "1.26"
           cache: true
@@ -34,7 +34,7 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - name: Checkout
-        uses: actions/checkout@v4
+        uses: actions/checkout@v7
 
       - name: Set up Helm
         uses: azure/setup-helm@v4

CHANGELOG.md

@@ -7,6 +7,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+- `pull_requests.mirror_objects` could not work against GitHub, GitLab, or Bitbucket. The engine handed the destination side the rendered repository name, which is owner relative because the clone URL and repository creation prepend the owner themselves, while every destination pull request and comment endpoint interpolates that value as a fully qualified path: a GitHub destination asked for `/repos/widget/pulls` rather than `/repos/acme/widget/pulls` and every call answered 404. Azure DevOps failed the other way, duplicating the project into a path that already carried it, so listing pull requests 404d for every repository
+- Mirroring several repositories at once could abort the process. Pull request records were written into the shared git state from inside the engine's worker pool without the lock the engine uses for its own access, and two repositories finishing together is a concurrent map write, which Go turns into an unrecoverable crash. The state type now guards its own maps
+- A private Docker Hub repository with `watch_tags` on failed with a 404 blamed on the image. The login response's session token was discarded, so every listing fell back to HTTP basic auth, which that API does not accept
+- A destination pull request stayed open forever once its source merged or closed. Sources are listed open only, so a finished pull request stops appearing rather than arriving with a final state, and nothing closed the mirror; the branch under it was pruned by the mirror push meanwhile. Records the source no longer lists are reconciled and closed with a note
+- Every pull request carrying a mirrored inline comment failed its conversation sync after the first run. GitHub keeps discussion and review comments in separate id spaces reached through different routes, and both were addressed through the discussion route
+- A conversation that failed part way discarded the ids of the comments it had already posted, so the next run reposted all of them, forever. A repository whose pull request pass failed was also recorded as fully mirrored, so it was skipped on the next run and never retried until an unrelated branch or tag moved
+- A single failed fetch of a pull request head deleted that branch from the destination, pulling the head out from under a mirrored pull request. The copy from the previous run is kept
+- The image sync cron could overlap itself, corrupting the run report and racing on the sync state map. Both cron loops now skip a tick rather than start a second run, validate the schedule before the first run instead of after it, and install the signal handler before the first run so a shutdown during it can still persist state
+- A malformed `exclude` glob was ignored entirely, mirroring a repository that the rule was written to keep out. Filter patterns are validated at config load
+- A Slack notification could be dropped whole. One oversized failure line, such as a transport error carrying a large HTML body, produced a block that was empty or over the size limit, and Slack rejects both
+- A failed state save on the fail-fast path was silent, so everything copied before the abort was copied again on the next run with no explanation
+- `git-sync` could not authenticate to Azure DevOps over git in `pat` mode, fixed in v0.2.1 and described there
+
+### Security
+- Patched a high severity advisory in `brace-expansion`, which the Azure DevOps extension pinned to exactly the affected version through an `overrides` entry
+
 ## [0.2.1] - 2026-08-19
 
 ### Added

azure-devops-extension/SyncerD/package-lock.json

@@ -11,6 +11,6 @@
     "azure-pipelines-task-lib": "^5.278.0"
   },
   "overrides": {
-    "brace-expansion": "5.0.8"
+    "brace-expansion": "5.0.9"
   }
 }

azure-devops-extension/SyncerD/package.json

@@ -3,6 +3,7 @@ package config
 import (
 	"fmt"
 	"os"
+	"path"
 	"strings"
 
 	"github.com/clouddrove/syncerd/internal/vcs"
@@ -360,6 +361,17 @@ func (c *Config) ValidateGitSync() error {
 		// A disabled block is not validated. Its fields have no effect,
 		// and rejecting a stale value in one would block a run that does
 		// not read it.
+		for _, pat := range m.Filters.Include {
+			if _, err := path.Match(pat, "probe"); err != nil {
+				return fmt.Errorf("git.mirrors[%d].filters.include pattern %q is malformed: %w", i, pat, err)
+			}
+		}
+		for _, pat := range m.Filters.Exclude {
+			if _, err := path.Match(pat, "probe"); err != nil {
+				return fmt.Errorf("git.mirrors[%d].filters.exclude pattern %q is malformed: %w; an exclude that cannot be parsed would otherwise be silently ignored", i, pat, err)
+			}
+		}
+
 		if m.PullRequests.MirrorObjects && !m.PullRequests.Enabled {
 			return fmt.Errorf("git.mirrors[%d].pull_requests.mirror_objects is set but enabled is not; the destination pull request needs the head branch that enabled mirrors", i)
 		}

internal/config/git.go

@@ -394,3 +394,27 @@ func TestPullRequestSwitchDefaults(t *testing.T) {
 		t.Error("an explicit false must win over the default")
 	}
 }
+
+func TestValidateGitSyncRejectsMalformedFilterPatterns(t *testing.T) {
+	// A malformed exclude used to be ignored entirely, so a repository
+	// somebody wrote a rule to keep out was mirrored to the destination.
+	cfg := &Config{Git: validGit()}
+	cfg.Git.Mirrors[0].Filters.Exclude = []string{"internal-[secret"}
+	cfg.Git.ApplyDefaults()
+
+	err := cfg.ValidateGitSync()
+	if err == nil || !strings.Contains(err.Error(), "malformed") {
+		t.Fatalf("expected a malformed pattern error, got %v", err)
+	}
+}
+
+func TestValidateGitSyncAcceptsWellFormedFilterPatterns(t *testing.T) {
+	cfg := &Config{Git: validGit()}
+	cfg.Git.Mirrors[0].Filters.Include = []string{"svc-*", "app-?"}
+	cfg.Git.Mirrors[0].Filters.Exclude = []string{"*-legacy"}
+	cfg.Git.ApplyDefaults()
+
+	if err := cfg.ValidateGitSync(); err != nil {
+		t.Fatalf("validate: %v", err)
+	}
+}

internal/config/git_test.go

@@ -561,27 +561,18 @@ func (e *Engine) mirrorRepo(ctx context.Context, cancel context.CancelFunc, m Mi
 		}
 	}
 
-	if !e.opts.DryRun {
-		e.stateMark(m.Name, repo.Path, destName, fp)
-
-		if setter, ok := m.Dest.(vcs.DefaultBranchSetter); ok && repo.DefaultBranch != "" && created {
-			if err := setter.SetDefaultBranch(ctx, destName, repo.DefaultBranch); err != nil {
-				redacted := e.redact(err.Error())
-				logging.Warn(fmt.Sprintf("mirror %s: could not set default branch on %s: %s", m.Name, destName, redacted),
-					"mirror", m.Name, "destination", destName, "error", redacted)
-			}
-		}
-	}
-
 	// Pull request objects come after the push, because a destination pull
 	// request cannot reference commits that have not arrived yet.
 	var prRes prsync.Result
 	if m.PullRequests.Enabled && m.PullRequests.MirrorObjects && m.DestPRs != nil {
 		var perr error
 		prRes, perr = prsync.Sync(ctx, prs, prsync.Options{
-			Mirror:       m.Name,
-			SourceRepo:   repo.Path,
-			DestRepo:     destName,
+			Mirror:     m.Name,
+			SourceRepo: repo.Path,
+			// Qualified, not the rendered name: the rendered name is owner
+			// relative, and every destination API addresses a repository by
+			// its full path.
+			DestRepo:     m.Dest.QualifiedPath(destName),
 			BranchPrefix: m.PullRequests.BranchPrefix,
 			Source:       m.SourcePRs,
 			Dest:         m.DestPRs,
@@ -605,6 +596,22 @@ func (e *Engine) mirrorRepo(ctx context.Context, cancel context.CancelFunc, m Mi
 		}
 	}
 
+	// A fingerprint is the claim that this repository is fully mirrored. It
+	// is withheld when the pull request pass failed, because recording it
+	// would skip the repository on the next run and the failure would never
+	// be retried until an unrelated branch or tag happened to move.
+	if !e.opts.DryRun && len(prRes.Failures) == 0 {
+		e.stateMark(m.Name, repo.Path, destName, fp)
+
+		if setter, ok := m.Dest.(vcs.DefaultBranchSetter); ok && repo.DefaultBranch != "" && created {
+			if err := setter.SetDefaultBranch(ctx, destName, repo.DefaultBranch); err != nil {
+				redacted := e.redact(err.Error())
+				logging.Warn(fmt.Sprintf("mirror %s: could not set default branch on %s: %s", m.Name, destName, redacted),
+					"mirror", m.Name, "destination", destName, "error", redacted)
+			}
+		}
+	}
+
 	e.addEvent(MirrorEvent{
 		Mirror:           m.Name,
 		SourceRepo:       repo.Path,
@@ -689,9 +696,14 @@ func (e *Engine) syncPRBranches(ctx context.Context, m Mirror, repo vcs.Repo, pr
 			from = srcURL
 		}
 		if err := e.opts.Runner.FetchPRHead(ctx, cacheDir, from, "refs/heads/"+pr.HeadBranch, branch, srcCred); err != nil {
-			logging.Warn(fmt.Sprintf("mirror %s: %s could not fetch the head of pull request %d, skipping it: %s",
+			logging.Warn(fmt.Sprintf("mirror %s: %s could not fetch the head of pull request %d, keeping the copy from the last run: %s",
 				m.Name, repo.Path, pr.Number, e.redact(err.Error())),
 				"mirror", m.Name, "source", repo.Path, "pull_request", pr.Number)
+			// Keep it anyway. The pull request is still open at the source,
+			// so dropping the branch here would prune it from the cache and
+			// then delete it at the destination, pulling the head out from
+			// under a mirrored pull request over one failed fetch.
+			keep[branch] = true
 			continue
 		}
 		keep[branch] = true

internal/gitsync/sync.go

@@ -20,6 +20,10 @@ type fakeRemote struct {
 }
 
 func (f *fakeRemote) CloneURL(path string) string { return filepath.Join(f.base, path+".git") }
+
+// QualifiedPath mirrors what a real provider does: the rendered name is
+// owner relative, and the qualified form is what an API addresses.
+func (f *fakeRemote) QualifiedPath(name string) string { return "acme/" + name }
 func (f *fakeRemote) GitCredential(context.Context) (vcs.GitCredential, error) {
 	return vcs.GitCredential{}, nil
 }
@@ -699,3 +703,141 @@ func TestWithoutMirrorObjectsASameRepoPullRequestStillGetsNoBranch(t *testing.T)
 		t.Errorf("PRBranchesPushed = %d, want 0", rep.Mirrored[0].PRBranchesPushed)
 	}
 }
+
+// recordingWriter captures the repository path the engine hands the pull
+// request writer, which is the value every destination API interpolates.
+type recordingWriter struct {
+	mu    sync.Mutex
+	paths []string
+}
+
+func (r *recordingWriter) record(p string) {
+	r.mu.Lock()
+	r.paths = append(r.paths, p)
+	r.mu.Unlock()
+}
+
+func (r *recordingWriter) FindPullRequest(_ context.Context, repo, _ string) (vcs.PullRequest, bool, error) {
+	r.record(repo)
+	return vcs.PullRequest{}, false, nil
+}
+func (r *recordingWriter) CreatePullRequest(_ context.Context, repo string, spec vcs.PullRequestSpec) (vcs.PullRequest, error) {
+	r.record(repo)
+	return vcs.PullRequest{Number: 100, State: vcs.PROpen, HeadBranch: spec.HeadBranch}, nil
+}
+func (r *recordingWriter) UpdatePullRequest(_ context.Context, repo string, _ int, _ vcs.PullRequestSpec) error {
+	r.record(repo)
+	return nil
+}
+func (r *recordingWriter) ClosePullRequest(_ context.Context, repo string, _ int) error {
+	r.record(repo)
+	return nil
+}
+
+func TestDestinationPullRequestsUseTheQualifiedRepositoryPath(t *testing.T) {
+	eng, m, _ := newEngineFixture(t)
+
+	source := m.SourceRemote.CloneURL("acme/app")
+	sha := strings.TrimSpace(git(t, source, "rev-parse", "refs/heads/main"))
+
+	writer := &recordingWriter{}
+	enablePRs(&m, &fakePRLister{prs: []vcs.PullRequest{{
+		Number: 7, State: vcs.PROpen, HeadBranch: "main", HeadSHA: sha, BaseBranch: "main",
+	}}})
+	m.PullRequests.MirrorObjects = true
+	m.DestPRs = writer
+
+	rep, err := eng.Run(context.Background(), []Mirror{m})
+	if err != nil {
+		t.Fatalf("run: %v", err)
+	}
+	if len(rep.Failures) != 0 {
+		t.Fatalf("unexpected failures: %+v", rep.Failures)
+	}
+	if len(writer.paths) == 0 {
+		t.Fatal("the writer was never called")
+	}
+
+	// The rendered destination name is owner relative because CloneURL and
+	// EnsureRepo prepend the owner themselves. Handing that bare name to an
+	// API that wants a full path produced "/repos/app/pulls" and 404ed on
+	// every repository.
+	for _, got := range writer.paths {
+		if got != "acme/app" {
+			t.Errorf("writer received %q, want the qualified path acme/app", got)
+		}
+	}
+}
+
+// failingWriter fails every create, standing in for a destination that
+// rejects the pull request pass while the branches mirror fine.
+type failingWriter struct{}
+
+func (failingWriter) FindPullRequest(context.Context, string, string) (vcs.PullRequest, bool, error) {
+	return vcs.PullRequest{}, false, nil
+}
+func (failingWriter) CreatePullRequest(context.Context, string, vcs.PullRequestSpec) (vcs.PullRequest, error) {
+	return vcs.PullRequest{}, errors.New("403 forbidden")
+}
+func (failingWriter) UpdatePullRequest(context.Context, string, int, vcs.PullRequestSpec) error {
+	return nil
+}
+func (failingWriter) ClosePullRequest(context.Context, string, int) error { return nil }
+
+func TestAFailedPullRequestPassIsRetriedOnTheNextRun(t *testing.T) {
+	eng, m, _ := newEngineFixture(t)
+
+	source := m.SourceRemote.CloneURL("acme/app")
+	sha := strings.TrimSpace(git(t, source, "rev-parse", "refs/heads/main"))
+	enablePRs(&m, &fakePRLister{prs: []vcs.PullRequest{{
+		Number: 7, State: vcs.PROpen, HeadBranch: "main", HeadSHA: sha, BaseBranch: "main",
+	}}})
+	m.PullRequests.MirrorObjects = true
+	m.DestPRs = failingWriter{}
+
+	rep, _ := eng.Run(context.Background(), []Mirror{m})
+	if len(rep.Failures) != 1 || rep.Failures[0].Stage != "pr-objects" {
+		t.Fatalf("want one pr-objects failure, got %+v", rep.Failures)
+	}
+
+	// The fingerprint is the claim that the repository is fully mirrored.
+	// Recording it here would skip the repository next run and the failure
+	// would never be retried until an unrelated ref moved.
+	if _, ok := eng.opts.State.Get("fake", "acme/app"); ok {
+		t.Error("a repository whose pull request pass failed must not be recorded as done")
+	}
+
+	rep, _ = eng.Run(context.Background(), []Mirror{m})
+	if rep.Skipped != 0 {
+		t.Errorf("the repository must be retried, Skipped = %d", rep.Skipped)
+	}
+}
+
+func TestAnUnreachableForkHeadKeepsTheBranchAlreadyAtTheDestination(t *testing.T) {
+	eng, m, _ := newEngineFixture(t)
+
+	fork := newForkRepo(t, "feature", "one")
+	forkSHA := strings.TrimSpace(git(t, fork, "rev-parse", "refs/heads/feature"))
+	lister := &fakePRLister{prs: []vcs.PullRequest{{
+		Number: 7, State: vcs.PROpen, HeadBranch: "feature", HeadSHA: forkSHA,
+		HeadRepoCloneURL: fork,
+	}}}
+	enablePRs(&m, lister)
+
+	if _, err := eng.Run(context.Background(), []Mirror{m}); err != nil {
+		t.Fatalf("first: %v", err)
+	}
+	if destRef(t, m, "refs/heads/syncerd/pr/7") == "" {
+		t.Fatal("the branch should exist after the first run")
+	}
+
+	// The fork becomes unreachable while the pull request is still open.
+	lister.prs[0].HeadRepoCloneURL = filepath.Join(t.TempDir(), "gone.git")
+
+	if _, err := eng.Run(context.Background(), []Mirror{m}); err != nil {
+		t.Fatalf("second: %v", err)
+	}
+	if destRef(t, m, "refs/heads/syncerd/pr/7") == "" {
+		t.Error("one failed fetch must not delete a branch the destination already has")
+	}
+}