fix(github): harden Variable + Comment reconcile + add lifecycle convergence tests

packages/alchemy/src/GitHub/Comment.ts

@@ -1,5 +1,6 @@
 import { Octokit } from "@octokit/rest";
 import * as Effect from "effect/Effect";
+import { isResolved } from "../Diff.ts";
 import * as Provider from "../Provider.ts";
 import { Resource } from "../Resource.ts";
 import { dedent } from "../Util/dedent.ts";
@@ -163,6 +164,21 @@ export const CommentProvider = () =>
   Provider.succeed(Comment, {
     stables: ["commentId"],
 
+    // The `(owner, repository, issueNumber)` tuple identifies the
+    // host issue/PR. A comment can't be moved between issues — change
+    // any of them and we must replace.
+    diff: Effect.fn(function* ({ news, olds }) {
+      if (!isResolved(news)) return undefined;
+      if (
+        news.owner !== olds.owner ||
+        news.repository !== olds.repository ||
+        news.issueNumber !== olds.issueNumber
+      ) {
+        return { action: "replace" } as const;
+      }
+      return undefined;
+    }),
+
     reconcile: Effect.fn(function* ({ news, output }) {
       const octokit = createClient(news);
       const body = dedent(news.body);
@@ -171,7 +187,7 @@ export const CommentProvider = () =>
       // state via the cached id; a 404 (deleted out-of-band, or never
       // created) collapses to "no observed comment" so we converge by
       // posting a fresh one.
-      const observedId = output?.commentId
+      const observed = output?.commentId
         ? yield* Effect.tryPromise({
             try: async () => {
               try {
@@ -180,7 +196,12 @@ export const CommentProvider = () =>
                   repo: news.repository,
                   comment_id: output.commentId,
                 });
-                return data.id;
+                return {
+                  id: data.id,
+                  body: data.body ?? "",
+                  htmlUrl: data.html_url,
+                  updatedAt: data.updated_at,
+                };
               } catch (error: any) {
                 if (error.status === 404) return undefined;
                 throw error;
@@ -191,7 +212,7 @@ export const CommentProvider = () =>
         : undefined;
 
       // Ensure — when no live comment exists, POST creates one.
-      if (observedId === undefined) {
+      if (observed === undefined) {
         const { data } = yield* Effect.tryPromise(() =>
           octokit.rest.issues.createComment({
             owner: news.owner,
@@ -207,21 +228,28 @@ export const CommentProvider = () =>
         };
       }
 
-      // Sync — PATCH the existing comment with the desired body. GitHub's
-      // updateComment is idempotent for identical bodies (returns same
-      // updatedAt), so we always issue the call rather than diffing.
-      const { data } = yield* Effect.tryPromise(() =>
-        octokit.rest.issues.updateComment({
-          owner: news.owner,
-          repo: news.repository,
-          comment_id: observedId,
-          body,
-        }),
-      );
+      // Sync — PATCH the existing comment only when the observed body
+      // drifted from desired. Skipping the call on no-op keeps redeploys
+      // with unchanged props quiet and preserves the prior `updatedAt`.
+      if (observed.body !== body) {
+        const { data } = yield* Effect.tryPromise(() =>
+          octokit.rest.issues.updateComment({
+            owner: news.owner,
+            repo: news.repository,
+            comment_id: observed.id,
+            body,
+          }),
+        );
+        return {
+          commentId: data.id,
+          htmlUrl: data.html_url,
+          updatedAt: data.updated_at,
+        };
+      }
       return {
-        commentId: data.id,
-        htmlUrl: data.html_url,
-        updatedAt: data.updated_at,
+        commentId: observed.id,
+        htmlUrl: observed.htmlUrl,
+        updatedAt: observed.updatedAt,
       };
     }),
 
@@ -232,6 +260,8 @@ export const CommentProvider = () =>
 
       const octokit = createClient(olds);
 
+      // Idempotent delete: 404 = already gone (deleted in the GitHub UI
+      // or by another process), which matches the desired terminal state.
       yield* Effect.tryPromise(async () => {
         try {
           await octokit.rest.issues.deleteComment({

packages/alchemy/src/GitHub/Variable.ts

@@ -1,4 +1,5 @@
 import * as Effect from "effect/Effect";
+import { isResolved } from "../Diff.ts";
 import * as Provider from "../Provider.ts";
 import { Resource } from "../Resource.ts";
 import { GitHubCredentials } from "./Credentials.ts";
@@ -109,7 +110,22 @@ const getOctokit = Effect.gen(function* () {
 
 export const VariableProvider = () =>
   Provider.succeed(Variable, {
-    reconcile: Effect.fn(function* ({ news }) {
+    // `(owner, repository, name)` is the GitHub-side identifier for a
+    // repo variable. Mutating any of them isn't an in-place rename —
+    // GitHub will refuse, so the engine must replace.
+    diff: Effect.fn(function* ({ news, olds }) {
+      if (!isResolved(news)) return undefined;
+      if (
+        news.owner !== olds.owner ||
+        news.repository !== olds.repository ||
+        news.name !== olds.name
+      ) {
+        return { action: "replace" } as const;
+      }
+      return undefined;
+    }),
+
+    reconcile: Effect.fn(function* ({ news, output }) {
       const octokit = yield* getOctokit;
 
       // Observe — `name` is the path identifier for repo variables; ask
@@ -133,22 +149,40 @@ export const VariableProvider = () =>
         catch: (e) => e as Error,
       });
 
-      // Ensure — POST creates the variable.
+      // Ensure — POST creates the variable. Tolerate a 422 race: another
+      // caller (or a concurrent reconcile) may have created the variable
+      // between our observe and ensure; in that case we fall through to
+      // the sync step below to make the value match.
       if (observed === undefined) {
-        yield* Effect.tryPromise(() =>
-          octokit.rest.actions.createRepoVariable({
-            owner: news.owner,
-            repo: news.repository,
-            name: news.name,
-            value: news.value,
-          }),
-        );
-        return { updatedAt: new Date().toISOString() };
+        const created = yield* Effect.tryPromise({
+          try: async () => {
+            try {
+              await octokit.rest.actions.createRepoVariable({
+                owner: news.owner,
+                repo: news.repository,
+                name: news.name,
+                value: news.value,
+              });
+              return true as const;
+            } catch (error: any) {
+              // 422 = "variable already exists" race. Anything else is
+              // a real error (auth, validation on name format, …).
+              if (error.status === 422) return false as const;
+              throw error;
+            }
+          },
+          catch: (e) => e as Error,
+        });
+        if (created) {
+          return { updatedAt: new Date().toISOString() };
+        }
       }
 
-      // Sync — PATCH the value if it drifted; skip the call when the
-      // observed value already matches to keep the API quiet.
-      if (observed.value !== news.value) {
+      // Sync — PATCH the value when the observed cloud value drifted
+      // from desired (or when we just lost the create race above). Skip
+      // the API call on a no-op so the timestamp doesn't churn — this
+      // keeps redeploys with the same props as a true no-op.
+      if (observed === undefined || observed.value !== news.value) {
         yield* Effect.tryPromise(() =>
           octokit.rest.actions.updateRepoVariable({
             owner: news.owner,
@@ -157,13 +191,21 @@ export const VariableProvider = () =>
             value: news.value,
           }),
         );
+        return { updatedAt: new Date().toISOString() };
       }
-      return { updatedAt: new Date().toISOString() };
+      // Observed value already matches — preserve prior timestamp so
+      // a redeploy with unchanged props is a true no-op for downstream
+      // consumers reading `updatedAt`.
+      return {
+        updatedAt: output?.updatedAt ?? new Date().toISOString(),
+      };
     }),
 
     delete: Effect.fn(function* ({ olds }) {
       const octokit = yield* getOctokit;
 
+      // Idempotent delete: 404 = already gone (deleted out-of-band or
+      // never created), which is the desired terminal state.
       yield* Effect.tryPromise(async () => {
         try {
           await octokit.rest.actions.deleteRepoVariable({