fix(repo_mirror_push): resolve mirror credential from server env, not the payload (#49)

The mirror credential no longer has to ride the tool-call payload (which is
persisted to transcript → claudewatcher → brain → gitea history). Adds
remote_password_env: the name of a server-side env var the tool resolves at call
time, so the secret stays in the server process. An env name that resolves to
empty errors loudly rather than silently sending an empty password. Raw
remote_password still works but the schema/description now mark it DISCOURAGED.

Tests: password resolved from the env var (never in output); unset env var →
ErrValidation.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

.gitea/workflows/cd.yml

@@ -60,7 +60,11 @@ jobs:
         run: |
           REGISTRY="localhost:5000"
           REF="${REGISTRY}/${{ env.IMAGE }}:${{ steps.meta.outputs.sha-tag }}"
+          # Stamp the real build version: the git tag on a v* build, else the short sha.
+          VERSION="${{ steps.meta.outputs.version-tag }}"
+          [ -z "$VERSION" ] && VERSION="${{ steps.meta.outputs.sha-tag }}"
           buildah build \
+            --build-arg VERSION="${VERSION}" \
             --label "org.opencontainers.image.revision=${{ github.sha }}" \
             --label "org.opencontainers.image.source=${{ github.repositoryUrl }}" \
             -t ${REF} \

Dockerfile

@@ -14,7 +14,10 @@ ENV GOSUMDB=off
 COPY go.mod go.sum ./
 RUN go mod download
 COPY . .
-RUN CGO_ENABLED=0 go build -trimpath -ldflags='-s -w' -o /out/gitea-mcp ./cmd/gitea-mcp
+# Build version stamped in by CI (--build-arg VERSION=<tag|sha>); defaults to
+# "dev" for a plain `docker build` (#47).
+ARG VERSION=dev
+RUN CGO_ENABLED=0 go build -trimpath -ldflags="-s -w -X main.version=${VERSION}" -o /out/gitea-mcp ./cmd/gitea-mcp
 
 FROM gcr.io/distroless/static-debian12:nonroot
 COPY --from=build /out/gitea-mcp /gitea-mcp

internal/tools/registry_test.go

@@ -54,5 +54,5 @@ func TestEveryRegisteredToolIsDispatchable(t *testing.T) {
 // Lock the tool count so an accidental drop of a registration in RegisterAll
 // (the single source main.go and this test share) fails loudly.
 func TestRegisteredToolCount(t *testing.T) {
-	assert.Len(t, buildRegistry().Tools(), 38)
+	assert.Len(t, buildRegistry().Tools(), 39)
 }

internal/tools/repo_mirror_push.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"os"
 
 	"git.d-ma.be/mathias/gitea-mcp/internal/allowlist"
 	"git.d-ma.be/mathias/gitea-mcp/internal/gitea"
@@ -22,7 +23,7 @@ func NewRepoMirrorPush(c *gitea.Client, a *allowlist.Allowlist) *RepoMirrorPush
 func (t *RepoMirrorPush) Descriptor() registry.ToolDescriptor {
 	return registry.ToolDescriptor{
 		Name:        "repo_mirror_push",
-		Description: "Manage push mirrors for a repository: add, list, or delete.",
+		Description: "Manage push mirrors for a repository: add, list, or delete. For the mirror credential, PREFER remote_password_env (the name of an env var the server reads) so the secret never rides the tool-call payload/transcript; remote_password (raw) is discouraged and will be persisted in logs.",
 		InputSchema: json.RawMessage(`{
 			"type":"object",
 			"properties":{
@@ -31,7 +32,8 @@ func (t *RepoMirrorPush) Descriptor() registry.ToolDescriptor {
 				"action":{"type":"string","enum":["add","list","delete"]},
 				"remote_address":{"type":"string","description":"Mirror target URL (required for add)."},
 				"remote_username":{"type":"string"},
-				"remote_password":{"type":"string","description":"Never logged or returned."},
+				"remote_password_env":{"type":"string","description":"PREFERRED: name of a server-side env var holding the mirror credential; the server resolves it, so the secret is never in this call. Errors if the var is unset."},
+				"remote_password":{"type":"string","description":"DISCOURAGED: raw credential — lands in the tool-call transcript/logs. Use remote_password_env instead."},
 				"interval":{"type":"string","description":"Sync interval, e.g. '8h0m0s'."},
 				"sync_on_commit":{"type":"boolean"},
 				"mirror_name":{"type":"string","description":"Remote name to delete (required for delete)."}
@@ -42,15 +44,16 @@ func (t *RepoMirrorPush) Descriptor() registry.ToolDescriptor {
 }
 
 type repoMirrorPushArgs struct {
-	Owner          string `json:"owner"`
-	Repo           string `json:"repo"`
-	Action         string `json:"action"`
-	RemoteAddress  string `json:"remote_address"`
-	RemoteUsername string `json:"remote_username"`
-	RemotePassword string `json:"remote_password"`
-	Interval       string `json:"interval"`
-	SyncOnCommit   bool   `json:"sync_on_commit"`
-	MirrorName     string `json:"mirror_name"`
+	Owner             string `json:"owner"`
+	Repo              string `json:"repo"`
+	Action            string `json:"action"`
+	RemoteAddress     string `json:"remote_address"`
+	RemoteUsername    string `json:"remote_username"`
+	RemotePassword    string `json:"remote_password"`
+	RemotePasswordEnv string `json:"remote_password_env"`
+	Interval          string `json:"interval"`
+	SyncOnCommit      bool   `json:"sync_on_commit"`
+	MirrorName        string `json:"mirror_name"`
 }
 
 // safeMirror omits remote_password so it is never returned to the caller.
@@ -72,6 +75,22 @@ func toSafeMirror(m *gitea.PushMirror) safeMirror {
 	}
 }
 
+// resolveMirrorPassword prefers remote_password_env — the name of a server-side
+// env var — so the credential never appears in the tool-call payload (#49). It
+// falls back to the raw (discouraged) remote_password. An env name that resolves
+// to empty is a loud error, not a silent empty password.
+func resolveMirrorPassword(args repoMirrorPushArgs) (string, error) {
+	if args.RemotePasswordEnv != "" {
+		pw := os.Getenv(args.RemotePasswordEnv)
+		if pw == "" {
+			return "", fmt.Errorf("remote_password_env %q is unset or empty in the server environment: %w",
+				args.RemotePasswordEnv, gitea.ErrValidation)
+		}
+		return pw, nil
+	}
+	return args.RemotePassword, nil
+}
+
 func (t *RepoMirrorPush) Call(ctx context.Context, raw json.RawMessage) (json.RawMessage, error) {
 	var args repoMirrorPushArgs
 	if err := parseArgs(raw, &args); err != nil {
@@ -82,10 +101,14 @@ func (t *RepoMirrorPush) Call(ctx context.Context, raw json.RawMessage) (json.Ra
 	}
 	switch args.Action {
 	case "add":
+		password, err := resolveMirrorPassword(args)
+		if err != nil {
+			return nil, err
+		}
 		m, err := t.c.AddPushMirror(ctx, args.Owner, args.Repo, gitea.AddPushMirrorArgs{
 			RemoteAddress:  args.RemoteAddress,
 			RemoteUsername: args.RemoteUsername,
-			RemotePassword: args.RemotePassword,
+			RemotePassword: password,
 			Interval:       args.Interval,
 			SyncOnCommit:   args.SyncOnCommit,
 		})

internal/tools/repo_mirror_push_test.go

@@ -3,6 +3,7 @@ package tools_test
 import (
 	"context"
 	"encoding/json"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"testing"
@@ -14,6 +15,45 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// #49: remote_password_env names a server-side env var; the secret is resolved
+// from the server environment and never rides the tool-call payload.
+func TestRepoMirrorPushTool_PasswordFromEnv(t *testing.T) {
+	t.Setenv("TEST_MIRROR_PW", "env-secret")
+	var gotPw string
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		body, _ := io.ReadAll(r.Body)
+		var m map[string]any
+		_ = json.Unmarshal(body, &m)
+		gotPw, _ = m["remote_password"].(string)
+		w.WriteHeader(http.StatusCreated)
+		_, _ = w.Write([]byte(`{"id":1,"remote_name":"m","remote_address":"a"}`))
+	}))
+	defer srv.Close()
+
+	tool := tools.NewRepoMirrorPush(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"mathias"}))
+	out, err := tool.Call(context.Background(), json.RawMessage(`{
+		"owner":"mathias","name":"infra","action":"add",
+		"remote_address":"https://github.com/mathias/infra.git",
+		"remote_username":"mathias","remote_password_env":"TEST_MIRROR_PW"
+	}`))
+	require.NoError(t, err)
+	assert.Equal(t, "env-secret", gotPw, "password must be resolved from the server env var")
+	assert.NotContains(t, string(out), "env-secret")
+}
+
+// remote_password_env pointing at an unset var must fail loudly, not silently
+// send an empty password.
+func TestRepoMirrorPushTool_EnvUnsetErrors(t *testing.T) {
+	tool := tools.NewRepoMirrorPush(gitea.NewClient("http://unused", ""), allowlist.New([]string{"mathias"}))
+	_, err := tool.Call(context.Background(), json.RawMessage(`{
+		"owner":"mathias","name":"infra","action":"add",
+		"remote_address":"https://github.com/x/y.git","remote_username":"u",
+		"remote_password_env":"DEFINITELY_UNSET_MIRROR_VAR_XYZ"
+	}`))
+	require.Error(t, err)
+	assert.ErrorIs(t, err, gitea.ErrValidation)
+}
+
 func TestRepoMirrorPushTool_Add(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		assert.Equal(t, http.MethodPost, r.Method)

internal/tools/tbd_ship.go

@@ -171,25 +171,34 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
 		return nil, fmt.Errorf("branch protection probe: %w", err)
 	}
 
-	// Branch from base, write the change, open the PR.
-	if err := t.c.CreateBranch(ctx, args.Owner, args.Repo, branch, base); err != nil {
+	// Branch from base. A conflict means the branch already exists — resume on it
+	// (idempotent re-invoke, #48) rather than failing.
+	if err := t.c.CreateBranch(ctx, args.Owner, args.Repo, branch, base); err != nil && !errors.Is(err, gitea.ErrConflict) {
 		return nil, fmt.Errorf("create branch %s: %w", branch, err)
 	}
 
-	// A pre-existing file at path needs its blob sha to update; a new file does not.
+	// A pre-existing file at path needs its blob sha to update; a new file does
+	// not. If the content already on the branch is identical, skip the write so a
+	// resume doesn't produce a redundant empty-diff commit.
 	sha := ""
+	needWrite := true
 	if fc, ferr := t.c.GetFileContents(ctx, args.Owner, args.Repo, args.Path, branch); ferr == nil {
 		sha = fc.Sha
+		if decoded, derr := base64.StdEncoding.DecodeString(fc.Content); derr == nil && string(decoded) == args.Content {
+			needWrite = false
+		}
 	} else if !errors.Is(ferr, gitea.ErrNotFound) {
 		return nil, fmt.Errorf("read %s on %s: %w", args.Path, branch, ferr)
 	}
-	if _, err := t.c.UpsertFile(ctx, args.Owner, args.Repo, args.Path, gitea.UpsertFileArgs{
-		Branch:  branch,
-		Content: base64.StdEncoding.EncodeToString([]byte(args.Content)),
-		Message: args.Message,
-		Sha:     sha,
-	}); err != nil {
-		return nil, fmt.Errorf("write %s on %s: %w", args.Path, branch, err)
+	if needWrite {
+		if _, err := t.c.UpsertFile(ctx, args.Owner, args.Repo, args.Path, gitea.UpsertFileArgs{
+			Branch:  branch,
+			Content: base64.StdEncoding.EncodeToString([]byte(args.Content)),
+			Message: args.Message,
+			Sha:     sha,
+		}); err != nil {
+			return nil, fmt.Errorf("write %s on %s: %w", args.Path, branch, err)
+		}
 	}
 
 	prTitle := args.PRTitle
@@ -203,7 +212,13 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
 		Base:  base,
 	})
 	if err != nil {
-		return nil, fmt.Errorf("open PR: %w", err)
+		// An open PR for this head already exists → resume it (#48).
+		if errors.Is(err, gitea.ErrConflict) || errors.Is(err, gitea.ErrValidation) {
+			pr, err = t.findOpenPR(ctx, args.Owner, args.Repo, branch)
+		}
+		if err != nil {
+			return nil, fmt.Errorf("open PR: %w", err)
+		}
 	}
 
 	// CI gate on the PR head commit.
@@ -267,6 +282,21 @@ func (t *TBDShip) pollRuns(ctx context.Context, owner, repo, headSHA string, tim
 	}
 }
 
+// findOpenPR returns the open PR whose head is the given branch — used to resume
+// an existing PR when CreatePullRequest reports one already exists (#48).
+func (t *TBDShip) findOpenPR(ctx context.Context, owner, repo, branch string) (*gitea.PullRequest, error) {
+	prs, err := t.c.ListPullRequests(ctx, owner, repo, "open", branch, 1, 50)
+	if err != nil {
+		return nil, err
+	}
+	for i := range prs {
+		if prs[i].Head.Ref == branch {
+			return &prs[i], nil
+		}
+	}
+	return nil, fmt.Errorf("no open PR found for head %s: %w", branch, gitea.ErrNotFound)
+}
+
 func (t *TBDShip) listRuns(ctx context.Context, owner, repo, headSHA string) []gitea.WorkflowRun {
 	resp, err := t.c.ListWorkflowRuns(ctx, owner, repo, gitea.ListWorkflowRunsArgs{HeadSHA: headSHA, Limit: 50})
 	if err != nil || resp == nil {

internal/tools/tbd_ship_test.go

@@ -19,12 +19,23 @@ import (
 // shipFake serves the whole tbd_ship flow; runsJSON is the workflow_runs array
 // for the head commit, letting each test drive the CI gate.
 func shipFake(t *testing.T, runsJSON string, merged, deleted *atomic.Bool) *httptest.Server {
+	t.Helper()
+	return shipFakeOpts(t, runsJSON, 0, http.StatusOK, merged, deleted)
+}
+
+// shipFakeOpts adds protection (requiredApprovals>0 → protected) and a merge
+// status code, so tests can drive the review-protected and conflict paths.
+func shipFakeOpts(t *testing.T, runsJSON string, requiredApprovals, mergeStatus int, merged, deleted *atomic.Bool) *httptest.Server {
 	t.Helper()
 	return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		p := r.URL.Path
 		w.Header().Set("Content-Type", "application/json")
 		switch {
 		case r.Method == http.MethodGet && strings.Contains(p, "/branch_protections/"):
+			if requiredApprovals > 0 {
+				_, _ = w.Write([]byte(`{"required_approvals":` + itoa(requiredApprovals) + `}`))
+				return
+			}
 			w.WriteHeader(http.StatusNotFound) // unprotected
 			_, _ = w.Write([]byte(`{"message":"not found"}`))
 		case r.Method == http.MethodPost && strings.HasSuffix(p, "/branches"):
@@ -43,7 +54,7 @@ func shipFake(t *testing.T, runsJSON string, merged, deleted *atomic.Bool) *http
 			_, _ = w.Write([]byte(`{"total_count":0,"workflow_runs":` + runsJSON + `}`))
 		case r.Method == http.MethodPost && strings.HasSuffix(p, "/pulls/7/merge"):
 			merged.Store(true)
-			w.WriteHeader(http.StatusOK)
+			w.WriteHeader(mergeStatus)
 			_, _ = w.Write([]byte(`{}`))
 		case r.Method == http.MethodDelete && strings.Contains(p, "/branches/"):
 			deleted.Store(true)
@@ -109,6 +120,90 @@ func TestTBDShip_RedCI_FailsClosed(t *testing.T) {
 	assert.False(t, merged.Load(), "merge must NOT be called when CI is red")
 }
 
+func itoa(n int) string { b, _ := json.Marshal(n); return string(b) }
+
+// Green CI but the base branch requires review → fail closed (no auto-merge).
+func TestTBDShip_ReviewProtected_FailsClosed(t *testing.T) {
+	var merged, deleted atomic.Bool
+	srv := shipFakeOpts(t, `[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]`, 1, http.StatusOK, &merged, &deleted)
+	defer srv.Close()
+
+	res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x"}`)
+
+	assert.Equal(t, false, res["merged"])
+	assert.NotEmpty(t, res["reason"])
+	assert.Contains(t, res["reason"], "review")
+	assert.False(t, merged.Load(), "must NOT merge a review-protected base")
+}
+
+// Green CI but the merge is unclean (409) → fail closed, PR left open.
+func TestTBDShip_MergeConflict_FailsClosed(t *testing.T) {
+	var merged, deleted atomic.Bool
+	srv := shipFakeOpts(t, `[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]`, 0, http.StatusConflict, &merged, &deleted)
+	defer srv.Close()
+
+	res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x"}`)
+
+	assert.Equal(t, false, res["merged"])
+	assert.NotEmpty(t, res["reason"])
+	assert.True(t, merged.Load(), "merge is attempted")
+	assert.False(t, deleted.Load(), "branch is NOT deleted on a failed merge")
+}
+
+// Re-invoking with the same change must resume the existing branch/PR (not
+// error on "branch exists"), skip the redundant write when content is
+// unchanged, and merge once CI is green (#48).
+func TestTBDShip_Resume_ExistingBranchAndPR(t *testing.T) {
+	var merged, deleted, wrote atomic.Bool
+	branch := "tbd/resume-me"
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		p := r.URL.Path
+		w.Header().Set("Content-Type", "application/json")
+		switch {
+		case r.Method == http.MethodGet && strings.Contains(p, "/branch_protections/"):
+			w.WriteHeader(http.StatusNotFound)
+			_, _ = w.Write([]byte(`{"message":"not found"}`))
+		case r.Method == http.MethodPost && strings.HasSuffix(p, "/branches"):
+			w.WriteHeader(http.StatusConflict) // branch already exists
+			_, _ = w.Write([]byte(`{"message":"branch already exists"}`))
+		case r.Method == http.MethodGet && strings.Contains(p, "/contents/"):
+			// existing file with identical content ("hi") → write should be skipped
+			w.WriteHeader(http.StatusOK)
+			_, _ = w.Write([]byte(`{"path":"docs/x.md","sha":"s","content":"aGk=","encoding":"base64"}`))
+		case (r.Method == http.MethodPost || r.Method == http.MethodPut) && strings.Contains(p, "/contents/"):
+			wrote.Store(true)
+			w.WriteHeader(http.StatusCreated)
+			_, _ = w.Write([]byte(`{"content":{"path":"x","sha":"s2"},"commit":{"sha":"c"}}`))
+		case r.Method == http.MethodPost && strings.HasSuffix(p, "/pulls"):
+			w.WriteHeader(http.StatusConflict) // PR already exists for this head
+			_, _ = w.Write([]byte(`{"message":"pull request already exists"}`))
+		case r.Method == http.MethodGet && strings.HasSuffix(p, "/pulls"):
+			_, _ = w.Write([]byte(`[{"number":7,"html_url":"http://x/pulls/7","state":"open","head":{"ref":"` + branch + `","sha":"abc"},"base":{"ref":"main"}}]`))
+		case r.Method == http.MethodGet && strings.Contains(p, "/actions/runs"):
+			_, _ = w.Write([]byte(`{"total_count":1,"workflow_runs":[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]}`))
+		case r.Method == http.MethodPost && strings.HasSuffix(p, "/pulls/7/merge"):
+			merged.Store(true)
+			w.WriteHeader(http.StatusOK)
+			_, _ = w.Write([]byte(`{}`))
+		case r.Method == http.MethodDelete && strings.Contains(p, "/branches/"):
+			deleted.Store(true)
+			w.WriteHeader(http.StatusOK)
+			_, _ = w.Write([]byte(`{}`))
+		default:
+			t.Errorf("unexpected request: %s %s", r.Method, p)
+			w.WriteHeader(http.StatusNotFound)
+		}
+	}))
+	defer srv.Close()
+
+	res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x","branch":"`+branch+`"}`)
+
+	assert.Equal(t, true, res["merged"], "resume must merge when CI is green")
+	assert.Equal(t, float64(7), res["pr_number"], "must reuse the existing PR #7")
+	assert.False(t, wrote.Load(), "identical content must not trigger a redundant write")
+	assert.True(t, merged.Load())
+}
+
 func TestTBDShip_AllowlistRejects(t *testing.T) {
 	tool := tools.NewTBDShip(gitea.NewClient("http://unused", ""), allowlist.New([]string{"mathias"}))
 	_, err := tool.Call(context.Background(), json.RawMessage(`{"owner":"evil","repo":"r","path":"p","content":"c","message":"m"}`))