feat(create_project): resume=true makes substitution durable against infra#179 (#50, #51)

parallax#1 was the real-world evidence: infra#179's branch-writability stall (>40s observed) blew past the tool's 5s budget, substitution ran zero files, and the repo shipped genuinely broken (go.mod still read `module __MODULE_PATH__`) — recoverable only via a manual clone/sed/git-mv/push. Adds `resume: bool`. When true, skips template lookup and repo generation entirely — the destination must already exist — and jumps straight to the tree-walk substitution. This is safe to re-invoke repeatedly because substituteEntry already compares against the branch's CURRENT state on every call (GetTree is re-fetched fresh each time): a file already fixed in a prior partial pass shows up already-correct or already-renamed and is a no-op. So the actual blocker to resumability was purely the "destination must NOT exist" guard on the create path — flipped it for resume, no change needed to the substitution logic itself. Consequences of resume existing: - infra179FinalizeMessage (#46) now points at the concrete recovery — "call this tool again with resume=true" — instead of manual clone/sed guidance. - "no placeholders substituted" only loud-fails on a FRESH create; on resume, finding nothing left to do is the expected steady state (success). - dispatch_allow injection (#43) is now idempotent (read-before-write, update with sha if present-but-different, skip if already correct) so a resumed call with dispatch_allow=true doesn't error re-creating a path that already exists (#51's root cause). - #51's other ask: dispatch_allow failures now land in their own dispatch_allow_failure field, never conflated with substitution's partial_failure — the two can independently succeed/fail. Tests: resume with no destination (error, names "nothing to resume"), resume continuing a partial substitution (asserts /generate is never re-called, only the still-wrong file is rewritten), resume when already fully done (success, not the fresh-create loud-fail), dispatch_allow idempotent re-injection (two-phase test capturing the real written content, no test-visible knowledge of the internal constant), and dispatch_allow_failure as a distinct field. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-04 13:49:38 +02:00
parent 82823aad1e
commit b288462a1c
3 changed files with 305 additions and 92 deletions
@@ -29,20 +29,30 @@ func templateRepoJSON(name string, isTemplate bool) string {
 // fakeTemplateServer serves the whole create-from-template flow off an in-memory
 // file map, driving the tool's tree-walk. Records writes/deletes/put-bodies.
 type fakeTemplateServer struct {
-	mu           sync.Mutex
-	files        map[string]string // path -> raw (un-substituted) content
-	genBranch    string            // default_branch returned by /generate ("" to force fallback)
-	generated    bool
-	puts         []string
-	deletes      []string
-	putBodies    map[string]string // path -> decoded written content
-	repoGetsPost int               // GET dest after generate (branch fallback)
+	mu            sync.Mutex
+	files         map[string]string // path -> raw (un-substituted) content
+	genBranch     string            // default_branch returned by /generate ("" to force fallback)
+	generated     bool
+	generateCalls int // # times POST .../generate was hit — resume must never increment this
+	puts          []string
+	deletes       []string
+	putBodies     map[string]string // path -> decoded written content
+	repoGetsPost  int               // GET dest after generate (branch fallback)
 }

 func newFakeTemplateServer(files map[string]string, genBranch string) *fakeTemplateServer {
 	return &fakeTemplateServer{files: files, genBranch: genBranch, putBodies: map[string]string{}}
 }

+// newFakeTemplateServerResumed simulates a repo that already exists from a
+// prior (real) generate call — GET dest succeeds immediately, without a
+// /generate call first. Used for resume:true tests.
+func newFakeTemplateServerResumed(files map[string]string, genBranch string) *fakeTemplateServer {
+	f := newFakeTemplateServer(files, genBranch)
+	f.generated = true
+	return f
+}
+
 func (f *fakeTemplateServer) handler(t *testing.T, tmpl, dest string) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
 		f.mu.Lock()
@@ -65,6 +75,7 @@ func (f *fakeTemplateServer) handler(t *testing.T, tmpl, dest string) http.Handl

 		case r.Method == http.MethodPost && p == "/api/v1/repos/mathias/"+tmpl+"/generate":
 			f.generated = true
+			f.generateCalls++
 			w.WriteHeader(http.StatusCreated)
 			_, _ = fmt.Fprintf(w, `{"name":%q,"full_name":"mathias/%s","default_branch":%q,"clone_url":"http://gitea.example.com/mathias/%s.git","html_url":"http://gitea.example.com/mathias/%s","template":false}`,
 				dest, dest, f.genBranch, dest, dest)
@@ -135,10 +146,11 @@ func callTool(t *testing.T, srvURL, tmpl, argsJSON string) createOut {
 }

 type createOut struct {
-	FullName         string   `json:"full_name"`
-	DefaultBranch    string   `json:"default_branch"`
-	FilesSubstituted []string `json:"files_substituted"`
-	PartialFailure   string   `json:"partial_failure,omitempty"`
+	FullName             string   `json:"full_name"`
+	DefaultBranch        string   `json:"default_branch"`
+	FilesSubstituted     []string `json:"files_substituted"`
+	PartialFailure       string   `json:"partial_failure,omitempty"`
+	DispatchAllowFailure string   `json:"dispatch_allow_failure,omitempty"`
 }

 // Happy path: whole-tree substitution, content + path rename, correct module host.
@@ -258,6 +270,126 @@ func TestCreateProject_DispatchAllow(t *testing.T) {
 	}
 }

+// ── resume: durable substitution against infra#179 (gitea-mcp#50) ───────────
+
+// resume:true requires an ALREADY-CREATED destination — there is nothing to
+// resume otherwise.
+func TestCreateProject_Resume_NoDestination_Errors(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusNotFound)
+		_, _ = w.Write([]byte(`{"message":"not found"}`))
+	}))
+	defer srv.Close()
+
+	_, err := newTool(srv.URL, "template-go-agent").Call(context.Background(),
+		json.RawMessage(`{"owner":"mathias","name":"new-svc","resume":true}`))
+	require.Error(t, err)
+	assert.ErrorIs(t, err, gitea.ErrValidation)
+	assert.Contains(t, err.Error(), "nothing to resume")
+}
+
+// resume:true on an existing repo continues substitution — fixes only what's
+// still wrong, leaves already-correct files untouched, and never calls
+// /generate again (the whole point: no re-creation, just continuation).
+func TestCreateProject_Resume_ContinuesPartialSubstitution(t *testing.T) {
+	files := map[string]string{
+		"go.mod":    "module git.d-ma.be/mathias/new-svc\n", // already correct from a prior partial run
+		"README.md": "# __PROJECT_NAME__\n",                 // still needs substitution
+	}
+	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.Equal(t, 0, f.generateCalls, "resume must never call /generate")
+	assert.Contains(t, out.FilesSubstituted, "README.md")
+	assert.NotContains(t, out.FilesSubstituted, "go.mod", "already-correct file must not be re-reported")
+	assert.NotContains(t, f.puts, "go.mod", "already-correct file must not be rewritten")
+	assert.Contains(t, f.puts, "README.md")
+}
+
+// resume:true when everything is already substituted is the expected steady
+// state (a prior resume already finished the job, or this is a redundant
+// re-invoke) — success, NOT the "no placeholders substituted" loud failure
+// that a fresh (non-resume) create would trigger.
+func TestCreateProject_Resume_AlreadyFullyDone_IsSuccess(t *testing.T) {
+	files := map[string]string{
+		"go.mod":    "module git.d-ma.be/mathias/new-svc\n",
+		"README.md": "# 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.FilesSubstituted)
+	assert.Empty(t, out.PartialFailure, "nothing left to do on resume must be success, not a loud failure")
+}
+
+// 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
+// create-only write would (gitea 409/422 on an existing path with no sha).
+func TestCreateProject_Resume_DispatchAllowIdempotent(t *testing.T) {
+	// Phase 1: a normal (non-resume) call captures the REAL content the tool
+	// writes for .dispatch-allow, without the test needing to know the exact
+	// unexported constant.
+	seedFiles := map[string]string{"go.mod": "module __MODULE_PATH__\n"}
+	seedSrv := newFakeTemplateServer(seedFiles, "main")
+	srv1 := httptest.NewServer(seedSrv.handler(t, "template-go-agent", "new-svc"))
+	out1 := callTool(t, srv1.URL, "template-go-agent", `{"owner":"mathias","name":"new-svc","dispatch_allow":true}`)
+	srv1.Close()
+	require.Empty(t, out1.PartialFailure)
+	realContent := seedSrv.putBodies[".dispatch-allow"]
+	require.NotEmpty(t, realContent, "phase 1 must have written .dispatch-allow")
+
+	// Phase 2: resume with .dispatch-allow ALREADY at that exact content, plus
+	// one file still needing substitution.
+	files := map[string]string{
+		".dispatch-allow": realContent,
+		"README.md":       "# __PROJECT_NAME__\n",
+	}
+	f := newFakeTemplateServerResumed(files, "main")
+	srv2 := httptest.NewServer(f.handler(t, "template-go-agent", "new-svc"))
+	defer srv2.Close()
+
+	out2 := callTool(t, srv2.URL, "template-go-agent", `{"owner":"mathias","name":"new-svc","resume":true,"dispatch_allow":true}`)
+
+	assert.Empty(t, out2.PartialFailure)
+	assert.Empty(t, out2.DispatchAllowFailure)
+	assert.NotContains(t, f.puts, ".dispatch-allow", "already-correct .dispatch-allow must not be rewritten")
+	assert.NotContains(t, out2.FilesSubstituted, ".dispatch-allow", "unchanged file must not be reported as substituted")
+	assert.Contains(t, out2.FilesSubstituted, "README.md")
+}
+
+// dispatch_allow injection failing is reported in its OWN field, distinct from
+// PartialFailure (gitea-mcp#51) — substitution can succeed while dispatch
+// eligibility still fails, and the caller must be able to tell them apart.
+func TestCreateProject_DispatchAllowFailure_IsDistinctField(t *testing.T) {
+	files := map[string]string{"go.mod": "module __MODULE_PATH__\n"}
+	f := newFakeTemplateServer(files, "main")
+	base := f.handler(t, "template-go-agent", "new-svc")
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if strings.Contains(r.URL.Path, "/contents/.dispatch-allow") {
+			w.WriteHeader(http.StatusInternalServerError)
+			_, _ = w.Write([]byte(`{"message":"boom"}`))
+			return
+		}
+		base(w, r)
+	}))
+	defer srv.Close()
+
+	out := callTool(t, srv.URL, "template-go-agent", `{"owner":"mathias","name":"new-svc","dispatch_allow":true}`)
+
+	assert.Empty(t, out.PartialFailure, "substitution itself succeeded — must not be conflated with the dispatch failure")
+	assert.NotEmpty(t, out.DispatchAllowFailure)
+	assert.Contains(t, out.DispatchAllowFailure, ".dispatch-allow")
+	assert.Contains(t, out.FilesSubstituted, "go.mod", "substitution must still be reported despite the separate dispatch failure")
+}
+
 // ── guardrails unchanged by the rewrite ──────────────────────────────────────

 func TestCreateProject_NameRegexFailure(t *testing.T) {