fix(hydrator): hydrated sha missing on no-ops (#25694) (#25695)

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

commitserver/commit/commit.go

@@ -187,7 +187,7 @@ func (s *Service) handleCommitRequest(logCtx *log.Entry, r *apiclient.CommitHydr
 	// short-circuit if already hydrated
 	if isHydrated {
 		logCtx.Debugf("this dry sha %s is already hydrated", r.DrySha)
-		return "", "", nil
+		return "", hydratedSha, nil
 	}
 
 	logCtx.Debug("Writing manifests")
@@ -197,13 +197,14 @@ func (s *Service) handleCommitRequest(logCtx *log.Entry, r *apiclient.CommitHydr
 		return "", "", fmt.Errorf("failed to write manifests: %w", err)
 	}
 	if !shouldCommit {
-		// add the note and return
+		// Manifests did not change, so we don't need to create a new commit.
+		// Add a git note to track that this dry SHA has been processed, and return the existing hydrated SHA.
 		logCtx.Debug("Adding commit note")
 		err = AddNote(gitClient, r.DrySha, hydratedSha)
 		if err != nil {
 			return "", "", fmt.Errorf("failed to add commit note: %w", err)
 		}
-		return "", "", nil
+		return "", hydratedSha, nil
 	}
 	logCtx.Debug("Committing and pushing changes")
 	out, err = gitClient.CommitAndPush(r.TargetBranch, r.CommitMessage)

commitserver/commit/commit_test.go

@@ -108,7 +108,7 @@ func Test_CommitHydratedManifests(t *testing.T) {
 		resp, err := service.CommitHydratedManifests(t.Context(), validRequest)
 		require.NoError(t, err)
 		require.NotNil(t, resp)
-		assert.Empty(t, resp.HydratedSha) // changes introduced by commit note. hydration won't happen if there are no new manifest|s to commit
+		assert.Equal(t, "it-worked!", resp.HydratedSha, "Should return existing hydrated SHA for no-op")
 	})
 
 	t.Run("root path with dot and blank - no directory removal", func(t *testing.T) {
@@ -283,12 +283,13 @@ func Test_CommitHydratedManifests(t *testing.T) {
 			TargetBranch:  "main",
 			SyncBranch:    "env/test",
 			CommitMessage: "test commit message",
+			DrySha:        "dry-sha-456",
 		}
 
 		resp, err := service.CommitHydratedManifests(t.Context(), requestWithEmptyPaths)
 		require.NoError(t, err)
 		require.NotNil(t, resp)
-		assert.Empty(t, resp.HydratedSha) // changes introduced by commit note. hydration won't happen if there are no new manifest|s to commit
+		assert.Equal(t, "empty-paths-sha", resp.HydratedSha, "Should return existing hydrated SHA for no-op")
 	})
 
 	t.Run("duplicate request already hydrated", func(t *testing.T) {
@@ -329,7 +330,7 @@ func Test_CommitHydratedManifests(t *testing.T) {
 		resp, err := service.CommitHydratedManifests(t.Context(), request)
 		require.NoError(t, err)
 		require.NotNil(t, resp)
-		assert.Empty(t, resp.HydratedSha) // changes introduced by commit note. hydration won't happen if there are no new manifest|s to commit
+		assert.Equal(t, "dupe-test-sha", resp.HydratedSha, "Should return existing hydrated SHA when already hydrated")
 	})
 
 	t.Run("root path with dot - no changes to manifest - should commit note only", func(t *testing.T) {
@@ -355,6 +356,7 @@ func Test_CommitHydratedManifests(t *testing.T) {
 			TargetBranch:  "main",
 			SyncBranch:    "env/test",
 			CommitMessage: "test commit message",
+			DrySha:        "dry-sha-123",
 			Paths: []*apiclient.PathDetails{
 				{
 					Path: ".",
@@ -370,7 +372,8 @@ func Test_CommitHydratedManifests(t *testing.T) {
 		resp, err := service.CommitHydratedManifests(t.Context(), requestWithRootAndBlank)
 		require.NoError(t, err)
 		require.NotNil(t, resp)
-		assert.Empty(t, resp.HydratedSha)
+		// BUG FIX: When manifests don't change (no-op), the existing hydrated SHA should be returned.
+		assert.Equal(t, "root-and-blank-sha", resp.HydratedSha, "Should return existing hydrated SHA for no-op")
 	})
 }
 

commitserver/commit/hydratorhelper_test.go

@@ -401,3 +401,69 @@ func TestAddNote(t *testing.T) {
 	err = AddNote(mockGitClient, drySha, commitShaErr)
 	require.Error(t, err)
 }
+
+// TestWriteForPaths_NoOpScenario tests that when manifests don't change between two hydrations,
+// shouldCommit returns false. This reproduces the bug where a new DRY commit that doesn't affect
+// manifests should not create a new hydrated commit.
+func TestWriteForPaths_NoOpScenario(t *testing.T) {
+	root := tempRoot(t)
+
+	repoURL := "https://github.com/example/repo"
+	drySha1 := "abc123"
+	drySha2 := "def456" // Different dry SHA
+	paths := []*apiclient.PathDetails{
+		{
+			Path: "guestbook",
+			Manifests: []*apiclient.HydratedManifestDetails{
+				{ManifestJSON: `{"apiVersion":"v1","kind":"Service","metadata":{"name":"guestbook-ui"}}`},
+				{ManifestJSON: `{"apiVersion":"apps/v1","kind":"Deployment","metadata":{"name":"guestbook-ui"}}`},
+			},
+			Commands: []string{"kustomize build ."},
+		},
+	}
+
+	now1 := metav1.NewTime(time.Now())
+	metadata1 := &appsv1.RevisionMetadata{
+		Author:  "test-author",
+		Date:    &now1,
+		Message: "Initial commit",
+	}
+
+	// First hydration - manifests are new, so HasFileChanged should return true
+	mockGitClient1 := gitmocks.NewClient(t)
+	mockGitClient1.On("HasFileChanged", "guestbook/manifest.yaml").Return(true, nil).Once()
+
+	shouldCommit1, err := WriteForPaths(root, repoURL, drySha1, metadata1, paths, mockGitClient1)
+	require.NoError(t, err)
+	require.True(t, shouldCommit1, "First hydration should commit because manifests are new")
+
+	// Second hydration - same manifest content but different dry SHA and metadata
+	// Simulate adding a README.md to the dry source (which doesn't affect manifests)
+	now2 := metav1.NewTime(time.Now().Add(1 * time.Hour)) // Different timestamp
+	metadata2 := &appsv1.RevisionMetadata{
+		Author:  "test-author",
+		Date:    &now2,
+		Message: "Add README.md", // Different commit message
+	}
+
+	// The manifests are identical, so HasFileChanged should return false
+	mockGitClient2 := gitmocks.NewClient(t)
+	mockGitClient2.On("HasFileChanged", "guestbook/manifest.yaml").Return(false, nil).Once()
+
+	shouldCommit2, err := WriteForPaths(root, repoURL, drySha2, metadata2, paths, mockGitClient2)
+	require.NoError(t, err)
+	require.False(t, shouldCommit2, "Second hydration should NOT commit because manifests didn't change")
+
+	// Verify that the root-level metadata WAS updated (even though we're not committing)
+	// The files get written to the working directory, but since shouldCommit is false, they won't be committed
+	topMetadataPath := filepath.Join(root.Name(), "hydrator.metadata")
+	topMetadataBytes, err := os.ReadFile(topMetadataPath)
+	require.NoError(t, err)
+
+	var topMetadata hydratorMetadataFile
+	err = json.Unmarshal(topMetadataBytes, &topMetadata)
+	require.NoError(t, err)
+	// The top-level metadata should have the NEW dry SHA (files are written, just not committed)
+	assert.Equal(t, drySha2, topMetadata.DrySHA)
+	assert.Equal(t, metadata2.Date.Format(time.RFC3339), topMetadata.Date)
+}

test/e2e/hydrator_test.go

@@ -298,3 +298,52 @@ func TestHydratorWithPlugin(t *testing.T) {
 			require.Equal(t, "inline-plugin-value", output)
 		})
 }
+
+func TestHydratorNoOp(t *testing.T) {
+	// Test that when hydration is run for a no-op (manifests do not change),
+	// the hydrated SHA is persisted to the app's source hydrator status instead of an empty string.
+	var firstHydratedSHA string
+	var firstDrySHA string
+
+	Given(t).
+		DrySourcePath("guestbook").
+		DrySourceRevision("HEAD").
+		SyncSourcePath("guestbook").
+		SyncSourceBranch("env/test").
+		When().
+		CreateApp().
+		Refresh(RefreshTypeNormal).
+		Wait("--hydrated").
+		Then().
+		Expect(HydrationPhaseIs(HydrateOperationPhaseHydrated)).
+		And(func(app *Application) {
+			require.NotEmpty(t, app.Status.SourceHydrator.CurrentOperation.HydratedSHA, "First hydration should have a hydrated SHA")
+			require.NotEmpty(t, app.Status.SourceHydrator.CurrentOperation.DrySHA, "First hydration should have a dry SHA")
+			firstHydratedSHA = app.Status.SourceHydrator.CurrentOperation.HydratedSHA
+			firstDrySHA = app.Status.SourceHydrator.CurrentOperation.DrySHA
+			t.Logf("First hydration - drySHA: %s, hydratedSHA: %s", firstDrySHA, firstHydratedSHA)
+		}).
+		When().
+		// Make a change to the dry source that doesn't affect the generated manifests.
+		AddFile("guestbook/README.md", "# Guestbook\n\nThis is documentation.").
+		Refresh(RefreshTypeNormal).
+		Wait("--hydrated").
+		Then().
+		Expect(HydrationPhaseIs(HydrateOperationPhaseHydrated)).
+		And(func(app *Application) {
+			require.NotEmpty(t, app.Status.SourceHydrator.CurrentOperation.HydratedSHA,
+				"Hydrated SHA must not be empty")
+			require.NotEmpty(t, app.Status.SourceHydrator.CurrentOperation.DrySHA)
+
+			// The dry SHA should be different (new commit in the dry source)
+			require.NotEqual(t, firstDrySHA, app.Status.SourceHydrator.CurrentOperation.DrySHA,
+				"Dry SHA should change after pushing a new commit")
+
+			t.Logf("Second hydration - drySHA: %s, hydratedSHA: %s",
+				app.Status.SourceHydrator.CurrentOperation.DrySHA,
+				app.Status.SourceHydrator.CurrentOperation.HydratedSHA)
+
+			require.Equal(t, firstHydratedSHA, app.Status.SourceHydrator.CurrentOperation.HydratedSHA,
+				"Hydrated SHA should remain the same for no-op hydration")
+		})
+}