fix(create_project): idempotent rename write completes a stray write+delete split (#53)

The rename path (write new path, then delete old path) is two separate,
non-atomic API calls. If a prior partial run's write succeeded but the paired
delete failed — plausible under the same infra#179 flakiness resume (#50)
exists to work around — a resume would recompute the identical rename and
blind-create (no sha) at a path that already exists, hitting a conflict and
getting stuck needing another resume cycle just to re-report the same thing.

renameEntry now reads the new path first: if it already holds the correct
content (prior write succeeded), the write is skipped and only the
outstanding delete of the old path runs; if the new path exists but differs,
it's updated with the fetched sha instead of blind-created; if the old path
is already gone by delete time, that's treated as done, not a failure.
Mirrors injectDispatchAllow's (#51) read-before-write idempotency pattern.

Test: TestCreateProject_Resume_StrayRenamedOldPath_CompletesCleanly — a stray
old path plus an already-correct new path resolves to a single delete, zero
redundant writes, no partial_failure. All prior create_project tests
unaffected (backward compatible).

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

internal/tools/create_project_from_template.go

@@ -374,21 +374,7 @@ func (t *CreateProjectFromTemplate) substituteEntry(ctx context.Context, owner,
 	enc := base64.StdEncoding.EncodeToString([]byte(newContent))
 
 	if renamed {
-		if err := t.upsertRetry(ctx, owner, name, newPath, gitea.UpsertFileArgs{
+		return t.renameEntry(ctx, owner, name, branch, path, newPath, enc, newContent, fc.Sha)
-			Branch:  branch,
-			Content: enc,
-			Message: fmt.Sprintf("template: substitute + rename %s -> %s", path, newPath),
-		}); err != nil {
-			return "", fmt.Sprintf("write %s: %v", newPath, err)
-		}
-		if _, err := t.c.DeleteFile(ctx, owner, name, path, gitea.DeleteFileArgs{
-			Branch:  branch,
-			Sha:     fc.Sha,
-			Message: fmt.Sprintf("template: drop placeholder path %s", path),
-		}); err != nil {
-			return "", fmt.Sprintf("delete %s: %v", path, err)
-		}
-		return path + " -> " + newPath, ""
 	}
 
 	if err := t.upsertRetry(ctx, owner, name, path, gitea.UpsertFileArgs{
@@ -401,3 +387,45 @@ func (t *CreateProjectFromTemplate) substituteEntry(ctx context.Context, owner,
 	}
 	return path, ""
 }
+
+// renameEntry writes newPath then deletes oldPath. Both halves are idempotent
+// so a resume that hits a prior write-succeeded/delete-failed rename (the two
+// are separate, non-atomic API calls) completes cleanly instead of erroring on
+// a blind re-create at a path that already exists (gitea-mcp#53): if newPath
+// already holds the correct content, the write is skipped and only the
+// outstanding delete of oldPath runs; if oldPath is already gone, the delete
+// is a no-op too.
+func (t *CreateProjectFromTemplate) renameEntry(ctx context.Context, owner, name, branch, oldPath, newPath, enc, newContent, oldSha string) (substituted, failure string) {
+	existing, err := t.c.GetFileContents(ctx, owner, name, newPath, branch)
+	switch {
+	case err == nil:
+		decoded, derr := base64.StdEncoding.DecodeString(existing.Content)
+		if derr == nil && string(decoded) == newContent {
+			break // already correct from a prior partial run — skip the write
+		}
+		if writeErr := t.upsertRetry(ctx, owner, name, newPath, gitea.UpsertFileArgs{
+			Branch: branch, Content: enc, Sha: existing.Sha,
+			Message: fmt.Sprintf("template: substitute + rename %s -> %s", oldPath, newPath),
+		}); writeErr != nil {
+			return "", fmt.Sprintf("write %s: %v", newPath, writeErr)
+		}
+	case errors.Is(err, gitea.ErrNotFound):
+		if writeErr := t.upsertRetry(ctx, owner, name, newPath, gitea.UpsertFileArgs{
+			Branch: branch, Content: enc,
+			Message: fmt.Sprintf("template: substitute + rename %s -> %s", oldPath, newPath),
+		}); writeErr != nil {
+			return "", fmt.Sprintf("write %s: %v", newPath, writeErr)
+		}
+	default:
+		return "", fmt.Sprintf("read %s: %v", newPath, err)
+	}
+
+	if _, err := t.c.DeleteFile(ctx, owner, name, oldPath, gitea.DeleteFileArgs{
+		Branch:  branch,
+		Sha:     oldSha,
+		Message: fmt.Sprintf("template: drop placeholder path %s", oldPath),
+	}); err != nil && !errors.Is(err, gitea.ErrNotFound) {
+		return "", fmt.Sprintf("delete %s: %v", oldPath, err)
+	}
+	return oldPath + " -> " + newPath, ""
+}

internal/tools/create_project_from_template_test.go

@@ -329,6 +329,29 @@ func TestCreateProject_Resume_AlreadyFullyDone_IsSuccess(t *testing.T) {
 	assert.Empty(t, out.PartialFailure, "nothing left to do on resume must be success, not a loud failure")
 }
 
+// A resume where a prior partial run's rename write SUCCEEDED but its paired
+// delete FAILED (both are separate, non-atomic API calls) must complete
+// cleanly: recognize the new path is already correct, skip re-writing it, and
+// just finish the outstanding delete of the stray old path (gitea-mcp#53).
+func TestCreateProject_Resume_StrayRenamedOldPath_CompletesCleanly(t *testing.T) {
+	files := map[string]string{
+		// stray: delete never completed in the prior run
+		"cmd/__PROJECT_NAME__/main.go": "package main\nimport \"__MODULE_PATH__/pkg/litellm\"\nconst n = \"__PROJECT_NAME__\"\n",
+		// already correct: the write half of the same prior rename DID complete
+		"cmd/new-svc/main.go": "package main\nimport \"git.d-ma.be/mathias/new-svc/pkg/litellm\"\nconst n = \"new-svc\"\n",
+	}
+	f := newFakeTemplateServerResumed(files, "main")
+	srv := httptest.NewServer(f.handler(t, "template-go-agent", "new-svc"))
+	defer srv.Close()
+
+	out := callTool(t, srv.URL, "template-go-agent", `{"owner":"mathias","name":"new-svc","resume":true}`)
+
+	assert.Empty(t, out.PartialFailure)
+	assert.Contains(t, out.FilesSubstituted, "cmd/__PROJECT_NAME__/main.go -> cmd/new-svc/main.go")
+	assert.NotContains(t, f.puts, "cmd/new-svc/main.go", "already-correct new path must not be rewritten")
+	assert.Contains(t, f.deletes, "cmd/__PROJECT_NAME__/main.go", "the outstanding delete must still happen")
+}
+
 // dispatch_allow injection is idempotent on resume: if .dispatch-allow already
 // has the correct content (from an earlier successful injection), re-invoking
 // must not attempt another write — and must not error the way a naive

internal/tools/issue_label.go

@@ -29,10 +29,10 @@ func (t *IssueLabel) Descriptor() registry.ToolDescriptor {
 				"owner":{"type":"string"},
 				"repo":{"type":"string"},
 				"number":{"type":"integer","minimum":1},
-				"labels":{"type":"array","items":{"type":"string"}},
+				"labels":{"type":"array","items":{"type":"string"},"description":"Label names to resolve and apply. Either labels or label_ids is required."},
-				"label_ids":{"type":"array","items":{"type":"integer"}}
+				"label_ids":{"type":"array","items":{"type":"integer"},"description":"Label IDs to apply directly, skipping name resolution. Either labels or label_ids is required."}
 			},
-			"required":["owner","repo","number","labels"]
+			"required":["owner","repo","number"]
 		}`),
 	}
 }

internal/tools/issue_label_test.go

@@ -53,6 +53,52 @@ func TestIssueLabelAppliesByName(t *testing.T) {
 	assert.Contains(t, string(out), `"name":"enhancement"`)
 }
 
+// label_ids alone (no labels) must work end-to-end without hitting ListLabels
+// at all — this is the schema-level "either labels or label_ids" contract, and
+// it must never require a GET to the label list when the caller already has IDs.
+func TestIssueLabelAppliesByIDOnly(t *testing.T) {
+	var captured []byte
+	var listCalled bool
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch {
+		case r.Method == http.MethodGet && r.URL.Path == "/api/v1/repos/o/r/labels":
+			listCalled = true
+			w.Header().Set("Content-Type", "application/json")
+			_, _ = w.Write([]byte(labelListFixture))
+		case r.Method == http.MethodPost && r.URL.Path == "/api/v1/repos/o/r/issues/42/labels":
+			var err error
+			captured, err = io.ReadAll(r.Body)
+			require.NoError(t, err)
+			w.Header().Set("Content-Type", "application/json")
+			_, _ = w.Write([]byte(labelListFixture))
+		default:
+			t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path)
+		}
+	}))
+	defer srv.Close()
+
+	tool := tools.NewIssueLabel(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"o"}))
+	out, err := tool.Call(context.Background(), json.RawMessage(`{"owner":"o","repo":"r","number":42,"label_ids":[1,2]}`))
+	require.NoError(t, err)
+
+	assert.False(t, listCalled, "label_ids-only must not call ListLabels")
+	var payload map[string]any
+	require.NoError(t, json.Unmarshal(captured, &payload))
+	ids, ok := payload["labels"].([]any)
+	require.True(t, ok)
+	assert.ElementsMatch(t, []any{float64(1), float64(2)}, ids)
+	assert.Contains(t, string(out), `"name":"bug"`)
+}
+
+// #52 review finding: the advertised schema wrongly required "labels", making
+// label_ids-only calls fail JSON-Schema validation before reaching Call at all.
+// Lock the fixed contract: neither is individually required.
+func TestIssueLabelSchema_NeitherLabelsNorLabelIDsRequired(t *testing.T) {
+	sch := string(tools.NewIssueLabel(gitea.NewClient("http://unused", ""), allowlist.New([]string{"o"})).Descriptor().InputSchema)
+	assert.NotContains(t, sch, `"required":["owner","repo","number","labels"]`)
+	assert.Contains(t, sch, `"required":["owner","repo","number"]`)
+}
+
 func TestIssueLabelUnknownNameNamesTheMissingLabel(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set("Content-Type", "application/json")