fix(docs-sync): emit review_items_file output, fix auto_push fast path, tighten gates

Author: jpr5

.github/workflows/showcase_docs-sync.yml

@@ -59,16 +59,13 @@ jobs:
           elif [ $EXIT_CODE -eq 3 ]; then
             echo "Has review items + clean transforms"
             echo "action=push_and_pr" >> "$GITHUB_OUTPUT"
-            if [ -f review-items.txt ]; then
-              REVIEW_ITEMS=$(cat review-items.txt)
-            else
+            # The sync script writes review-items.txt and emits
+            # review_items_file=<abs-path> directly to GITHUB_OUTPUT.
+            # Verify the file actually exists so downstream steps don't
+            # silently read an empty path.
+            if [ ! -f review-items.txt ]; then
               echo "::warning::review-items.txt not found despite exit code 3 — sync script may have a bug"
-              REVIEW_ITEMS="(review-items.txt not found — check sync script output)"
             fi
-            # Persist review items to a file for downstream jq-based payload building
-            # (avoids unsafe string interpolation of untrusted filenames into JSON).
-            printf '%s' "$REVIEW_ITEMS" > review-items-raw.txt
-            echo "review_items_file=review-items-raw.txt" >> "$GITHUB_OUTPUT"
           else
             echo "Sync failed with exit code $EXIT_CODE"
             exit 1
@@ -119,12 +116,29 @@ jobs:
             # GitHub's PR search syntax has no `head:` qualifier, so that
             # query always returns empty. Use jq on the full list instead
             # to filter by headRefName prefix client-side.
-            EXISTING_PR=$(gh pr list \
-              --state open \
-              --base "$SHOWCASE_BRANCH" \
-              --json number,url,headRefName \
-              --jq '[.[] | select(.headRefName | startswith("docs-sync/needs-review/"))] | .[0].url' \
-              2>/dev/null || echo "")
+            # Fail the step on gh API failure rather than swallowing it —
+            # `|| echo ""` would silently treat a transient 5xx as "no
+            # existing PR" and open a duplicate. Retry once with backoff
+            # to absorb blips; if both attempts fail, exit non-zero.
+            gh_pr_list_existing() {
+              gh pr list \
+                --state open \
+                --base "$SHOWCASE_BRANCH" \
+                --json number,url,headRefName \
+                --jq '[.[] | select(.headRefName | startswith("docs-sync/needs-review/"))] | .[0].url'
+            }
+            if ! EXISTING_PR=$(gh_pr_list_existing 2>gh-err.txt); then
+              echo "::warning::gh pr list failed on first attempt — retrying after 5s"
+              cat gh-err.txt || true
+              sleep 5
+              if ! EXISTING_PR=$(gh_pr_list_existing 2>gh-err.txt); then
+                echo "::error::gh pr list failed twice — aborting to avoid duplicate PR creation"
+                cat gh-err.txt || true
+                rm -f gh-err.txt
+                exit 1
+              fi
+            fi
+            rm -f gh-err.txt
             if [ -n "$EXISTING_PR" ]; then
               echo "Open needs-review PR already exists: $EXISTING_PR"
               echo "Skipping new PR — Slack alert will point at the existing one."
@@ -180,6 +194,15 @@ jobs:
             # Explicit signal: no PR opened, and this is the intended outcome
             # (not an error path). Downstream alerts key on this.
             echo "pr_opened=false" >> "$GITHUB_OUTPUT"
+            # Set needs_review explicitly so downstream `needs_review != 'true'`
+            # gates (notify-auto-sync) don't misfire on this deliberate
+            # no-op. Value depends on why we got here: push_and_pr path =
+            # review still pending; auto_push path = no review needed.
+            if [ "$ACTION" = "push_and_pr" ]; then
+              echo "needs_review=true" >> "$GITHUB_OUTPUT"
+            else
+              echo "needs_review=false" >> "$GITHUB_OUTPUT"
+            fi
             exit 0
           fi
 

showcase/scripts/sync-docs-from-main.ts

@@ -617,15 +617,17 @@ function main(): SyncResult {
     }
   }
 
-  // Update sync marker only when NOTHING had local modifications (no
-  // merge conflicts AND no auto-merged-with-local-mods files). If any file
-  // had local mods — even an auto-merged one — keep the old sha so the
-  // next run still has the correct base context for 3-way merging until
-  // a human has reviewed/merged the needs-review PR.
+  // Update sync marker whenever all files were successfully resolved:
+  // clean transforms, clean auto-merges, or no changes at all. A successful
+  // 3-way auto-merge means the file IS resolved — its content on disk
+  // reflects the merged state, so the next run should treat HEAD as the
+  // new base. Only outstanding merge conflicts (upstream-wins written to
+  // PR branch only) or deletions (not auto-applied) keep the old sha so
+  // future runs still flag them until a human merges the needs-review PR.
   if (
     !dryRun &&
-    result.needsReview.length === 0 &&
     result.mergeConflict.length === 0 &&
+    result.deleted.length === 0 &&
     (result.copied.length > 0 ||
       result.transformed.length > 0 ||
       result.autoMerged.length > 0)
@@ -678,14 +680,16 @@ const hasAutoApplied =
   result.copied.length > 0 ||
   result.transformed.length > 0 ||
   result.autoMerged.length > 0;
-// Any file that had showcase-local modifications (needsReview) must route
-// through push_and_pr so a human can review — even if the 3-way merge was
-// clean (autoMerged). Silent auto-merging files with local mods would
-// clobber intentional showcase divergence.
+// Only files that still need human attention force push_and_pr:
+// - mergeConflict: 3-way merge failed, upstream-wins content staged to PR
+//   branch, human must reconcile
+// - deleted: files gone on main, not auto-deleted in showcase, human decides
+// Auto-merged files (clean 3-way merge) are considered RESOLVED — they go
+// through the auto_push fast path and the marker advances. `needsReview`
+// is the superset tracker (local-mods detected pre-merge) and is NOT a
+// gating condition on its own.
 const hasReviewItems =
-  result.mergeConflict.length > 0 ||
-  result.needsReview.length > 0 ||
-  result.deleted.length > 0;
+  result.mergeConflict.length > 0 || result.deleted.length > 0;
 
 if (!hasAutoApplied && !hasReviewItems) {
   process.exit(2);
@@ -720,10 +724,18 @@ if (!hasAutoApplied && !hasReviewItems) {
   // ROOT makes the contract explicit: whoever invokes the script picks
   // where these artifacts land, and the workflow invokes from repo root.
   const artifactDir = process.cwd();
-  fs.writeFileSync(
-    path.join(artifactDir, "review-items.txt"),
-    reviewLines.join("\n") + "\n",
-  );
+  const reviewItemsPath = path.join(artifactDir, "review-items.txt");
+  fs.writeFileSync(reviewItemsPath, reviewLines.join("\n") + "\n");
+  // Emit the absolute path to GITHUB_OUTPUT so the workflow's PR-body and
+  // Slack payload steps can read the file without hardcoding a location.
+  // (Downstream steps use `steps.sync.outputs.review_items_file`.) Skipped
+  // outside CI — process.env.GITHUB_OUTPUT is unset for local runs.
+  if (process.env.GITHUB_OUTPUT) {
+    fs.appendFileSync(
+      process.env.GITHUB_OUTPUT,
+      `review_items_file=${reviewItemsPath}\n`,
+    );
+  }
   // Persist the conflict manifest so the workflow can apply upstream-wins
   // content to the PR branch (NOT the current worktree — see
   // conflictManifest comment above).