ci: harden CLI Draft Release reconciliation

[MLittleprince]

Summary

• harden the CLI Draft Release state machine against the same delayed-visibility class seen in OpenClaw Cloud Plugin run 30791226792
• poll and verify the pushed immutable tag, Draft Release metadata, and expected build assets with bounded backoff
• reconcile ambiguous tag-push and Release-create responses without issuing a duplicate push or duplicate create request
• safely resume an incomplete existing Draft by retrying asset upload and metadata edit, then verifying the final Draft
• require the remote tag with gh release create --verify-tag

Validation

• node --test .github/scripts/prepare-cli-release.test.mjs — 45 passed
• bash -n .github/scripts/publish-cli-release.sh
• node --check .github/scripts/prepare-cli-release.mjs
• bash -n scripts/build-binary.sh
• git diff --check
• sensitive diff scan: no real token, internal IP/URL, or local user path

Safety

This PR does not change the CLI version, create or publish a tag/Release, publish npm, change Doc Agent mapping, or change changelog generation. It only hardens the real-run GitHub Tag/Draft Release boundary.

[Memtensor-AI]

🤖 Open Code Review

Target: PR #26
Task: c438e9d34fe97522
Base: main
Head: docs-sync/memos-cli-release-reliability

🔍 OpenCodeReview found 6 issue(s) in this PR.

⚠️ 1 warning(s) occurred during review.

---

1. .github/scripts/prepare-cli-release.test.mjs (L236-L243)

When releaseCreateStatus: 1 and releaseCreateMaterializes: true, the mock writes the state file and resets MOCK_RELEASE_LOOKUP_COUNT to 0 before returning the non-zero exit code. If the production script were to call gh release create a second time (the exact idempotency failure this test is guarding against), the counter would be reset again, restarting the visibility clock. While the current production code is correct, this mock behaviour makes a second accidental create call harder to detect: the second call would appear to succeed and the lookup would start fresh. Adding a guard such as if [[ ! -s "${MOCK_RELEASE_STATE_FILE}" ]] around the state-file write would cause a second create call to produce a detectable no-op, making the existing assertion assert.equal(...gh release create..., 1) the only defence needed.

💡 Suggested Change

Before:

"elif [[ \"${1:-}\" == \"release\" && \"${2:-}\" == \"create\" ]]; then",
"  if [[ \"${MOCK_RELEASE_CREATE_MATERIALIZES}\" == \"true\" ]]; then",
"    created_state='draft'",
"    if [[ \" $* \" == *' --prerelease '* ]]; then created_state='prerelease-draft'; fi",
"    printf '%s\\n' \"${created_state}\" > \"${MOCK_RELEASE_STATE_FILE}\"",
"    printf '0\\n' > \"${MOCK_RELEASE_LOOKUP_COUNT}\"",
"  fi",
"  exit \"${MOCK_RELEASE_CREATE_STATUS}\"",

After:

  if [[ "${MOCK_RELEASE_CREATE_MATERIALIZES}" == "true" ]]; then
      "    # Only materialize on the first create call; a second call should be a detectable no-op."
      "    if [[ ! -s \"${MOCK_RELEASE_STATE_FILE}\" ]]; then"
      "      created_state='draft'"
      "      if [[ \" $* \" == *' --prerelease '* ]]; then created_state='prerelease-draft'; fi"
      "      printf '%s\\n' \"${created_state}\" > \"${MOCK_RELEASE_STATE_FILE}\""
      "      printf '0\\n' > \"${MOCK_RELEASE_LOOKUP_COUNT}\""
      "    fi"
      "  fi"
      "  exit \"${MOCK_RELEASE_CREATE_STATUS}\""

---

2. .github/scripts/prepare-cli-release.test.mjs (L1558-L1572)

The "reconciles an ambiguous create response" test does not assert that no second gh release create call was issued in the reconciled case — only that create appears exactly once in total. Because the production code is correct, this currently passes, but if a regression caused the code to retry the create (resetting the mock counter), the test would still see exactly one create in the log if the mock never incremented it. Adding assert.doesNotMatch(reconciled.callLog, /gh release create.*\ngh release create/) or checking total count before and after the ambiguous path would make the guard explicit.

💡 Suggested Change

Before:

  const reconciled = runPublishFixture({
    releaseCreateStatus: 1,
    releaseCreateMaterializes: true,
    releaseVisibilityDelay: 1,
  });
  assert.equal(reconciled.failure, undefined);
  assert.equal((reconciled.callLog.match(/gh release create/g) || []).length, 1);

  const missing = runPublishFixture({
    releaseCreateStatus: 1,
    releaseCreateMaterializes: false,
  });
  assert.ok(missing.failure);
  assert.equal((missing.callLog.match(/gh release create/g) || []).length, 1);
  assert.match(missing.output, /no matching Draft became visible/);

After:

  assert.equal(reconciled.failure, undefined);
  assert.equal((reconciled.callLog.match(/gh release create/g) || []).length, 1);
  // Explicitly confirm the reconcile path did NOT issue a second create.
  assert.doesNotMatch(reconciled.callLog, /gh release upload.*\ngh release create/);

---

3. .github/scripts/prepare-cli-release.test.mjs (L1575-L1580)

assetVisibilityDelay: 100 is used here to ensure no assets are shown during reconciliation before the upload runs. However, as described in the mock logic, this delay is overridden the moment MOCK_ASSETS_VISIBLE_FILE is created by the upload handler — so any value ≥ 1 would have the same effect given RELEASE_RETRY_ATTEMPTS: "4". The magic number 100 strongly implies a time delay and misleads readers into thinking it controls post-upload asset polling. A comment explaining the intent (or a lower value like 5 matching the retry budget) would make the fixture self-documenting.

💡 Suggested Change

Before:

test("publish state machine resumes an incomplete Draft instead of creating a second Release", () => {
  const result = runPublishFixture({
    releaseCreateStatus: 1,
    releaseCreateMaterializes: true,
    assetVisibilityDelay: 100,
  });

After:

  const result = runPublishFixture({
    releaseCreateStatus: 1,
    releaseCreateMaterializes: true,
    // Use a large delay to guarantee assets appear 'absent' before upload runs;
    // the upload mock itself marks assets visible, overriding this counter.
    assetVisibilityDelay: 100,
  });

---

4. .github/scripts/publish-cli-release.sh (L51-L54)

The run_retryable_command function only captures stderr (2>"${retry_log}"), so any stdout output from the wrapped command (e.g., gh release upload progress lines) is passed straight to the caller's stdout. This is intentional for normal output, but if a command writes its actionable error message to stdout rather than stderr, the error will not appear in the log and will be silently lost on retry (only the last attempt's stderr is printed). gh CLI typically uses stderr for errors, so this is low-risk today, but it is worth documenting the assumption or capturing both streams.

Additionally, the same fixed log path is shared across every invocation of run_retryable_command within a single process ($$ is the PID, not per-call). Inside upload_and_update_draft, the upload's log is silently overwritten by the edit invocation. If the upload succeeds but the edit fails on its last attempt, the upload's earlier stderr is gone — only the edit's last-attempt stderr is printed. This is harmless today because the upload would have already exited on failure, but the shared log makes the retry logging fragile.

💡 Suggested Change

Before:

  local retry_log="${RUNNER_TEMP}/memos-cli-release-command-$$.log"
  for ((attempt = 1; attempt <= release_retry_attempts; attempt += 1)); do
    set +e
    "$@" 2>"${retry_log}"

After:

  # Use a per-call unique log path to avoid overwriting logs between consecutive
  # run_retryable_command invocations within the same process.
  local retry_log="${RUNNER_TEMP}/memos-cli-release-command-$$-${RANDOM}.log"
  for ((attempt = 1; attempt <= release_retry_attempts; attempt += 1)); do
    set +e
    "$@" 2>"${retry_log}"

---

5. .github/scripts/publish-cli-release.sh (L115-L120)

When wait_for_visibility=true and ls-remote succeeds but returns an empty SHA on every attempt (i.e., the tag never becomes visible within release_retry_attempts polls), the for loop exhausts all retries and falls through to return 0 at line 119 — returning success with empty output. The caller at line 307 treats empty output as "tag not found" and emits a hard error, so the end-to-end behaviour is correct. However, the function itself returns 0 in a situation that is not a success (visibility timeout), making it harder to distinguish a clean "tag absent" result from a polling timeout. Consider returning a distinct non-zero status (or at minimum a comment) so the semantics are explicit.

💡 Suggested Change

Before:

    if [[ "${attempt}" != "${release_retry_attempts}" ]]; then
      retry_sleep "${attempt}"
    fi
  done
  return 0
}

After:

    if [[ "${attempt}" != "${release_retry_attempts}" ]]; then
      retry_sleep "${attempt}"
    fi
  done
  # Visibility timeout: tag was not observed within the retry budget.
  # Return empty output with success so callers check -z on the output;
  # a dedicated exit code could make the timeout case explicit if needed.
  return 0
}

---

6. .github/scripts/publish-cli-release.sh (L353)

validate_draft_release (and the inline check at line 353) relies on set -e to propagate its non-zero exit rather than an explicit || exit 1. Inside a complex if/else chain that has already seen several set +e/set -e toggles, relying on implicit set -e propagation is brittle: any future wrapping of this call in an if, &&, ||, or ! expression would silently suppress the exit. An explicit guard makes intent clear.

💡 Suggested Change

Before:

    validate_draft_release

After:

    if ! validate_draft_release; then
      echo "::error::Draft Release ${CURRENT_TAG} could not be verified after asset resume."
      exit 1
    fi

---

🧹 Filtered 1 low-confidence OCR finding(s) before posting/fix-loop (existing_code_mismatch: 1).

Generated by cloud-assistant via Open Code Review.

[Memtensor-AI]

✅ Automated Test Results: PASSED

All tests passed (1/1 executed). memos_github_open_source/smoke: 1/1. Duration: 1s

Branch: docs-sync/memos-cli-release-reliability