Harden automated candidate promotion

ilysenko · ilysenko · commit 27049bd9e7f4 · 2026-07-11T20:40:12.000+03:00
diff --git a/AGENTS.md b/AGENTS.md
@@ -117,6 +117,11 @@ Primary human docs: [architecture](docs/architecture.md),
   `systemd --user` updater service on a best-effort basis.
 - Failed privileged updater installs stay failed until a newer rebuild or an
   explicit retry path; avoid auto-retrying every reconcile cycle.
+- Automated user-local updater paths must force acceptance and running-app
+  overrides off. They may build alongside a running app, but promotion must
+  wait for exit or fail without replacing the installed runtime.
+- Transactional app promotion retains only the immediately previous managed
+  app backup; older exact managed backups are pruned under the promotion lock.
 - Manual rollback uses the last-known-good package recorded in updater state
   and the same format-specific command layer as normal installs.
 - Local installs, updater rebuilds, and scheduled CI use the same upstream DMG
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -23,6 +23,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 
 ### Fixed
 
+- Automated user-local updates no longer inherit or set developer overrides
+  that could replace a running Electron app or bypass DMG acceptance. Manual
+  and timer rebuilds now fail safely at promotion, transactional installs retain
+  only the immediately previous app backup, and drift issue automation mutates
+  only issues carrying its valid fingerprint marker.
 - The Add Project folder picker is no longer parented to the Codex window on
   Linux X11. This avoids a GNOME Shell modal input grab that could lock desktop
   input and flood system logs, while preserving parented dialogs on Wayland,
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/contrib/user-local-install/README.md b/contrib/user-local-install/README.md
@@ -94,3 +94,5 @@ codex-desktop-version
 - The helper scripts are copied into `~/.local/opt` and do not run from the git checkout directly.
 - The X11/XWayland preference is stored in `~/.config/codex-desktop-linux/user-local.env` and is preserved across updater refreshes.
 - The weekly timer runs `codex-desktop-update --quiet`. It is opt-in: pass `--enable-timer` to `install-user-local.sh` to activate it, or run `systemctl --user enable --now codex-desktop-update.timer` manually after install.
+- Automated rebuilds never bypass the running-app or DMG acceptance gates. They may build a candidate while ChatGPT Desktop is open, but promotion waits for the in-app after-exit flow or fails safely for a manual/timer run. Retry after closing the app.
+- A successful transactional update retains only the immediately previous app backup. Older exact managed backups are pruned; manually named paths, files, and symlinks are left alone.
diff --git a/contrib/user-local-install/files/.local/bin/codex-desktop-update b/contrib/user-local-install/files/.local/bin/codex-desktop-update
@@ -126,7 +126,7 @@ log "Rebuilding wrapper..."
         export CODEX_LINUX_FEATURES_CONFIG="$SOURCE_REPO_DIR/linux-features/features.json"
     fi
     TMPDIR="$STATE_DIR/tmp" \
-    CODEX_INSTALL_ALLOW_RUNNING=1 \
+    CODEX_INSTALL_ALLOW_RUNNING=0 \
         CODEX_INSTALL_ROOT="$OPT_ROOT" \
         CODEX_INSTALL_DIR="$APP_DIR" \
         ./install.sh
diff --git a/docs/updater.md b/docs/updater.md
@@ -144,6 +144,20 @@ configuration. Drift in any selected feature rejects the candidate; disable
 that feature and retry if receiving the upstream update is more important than
 retaining it.
 
+Automated user-local rebuilds always force
+`CODEX_INSTALL_ALLOW_RUNNING=0` and `CODEX_ACCEPTANCE_OVERRIDE=0`, even if the
+service or invoking shell inherited developer overrides. The in-app update path
+continues through its after-exit hook and relaunches after a successful update.
+A manual command or timer may build while the app is open, but final promotion
+is refused and the working app remains unchanged until Electron exits. Failed
+promotion candidates are disposable by default; opt in to diagnostic retention
+with `CODEX_KEEP_REJECTED_CANDIDATE=1`.
+
+Transactional user-local installs retain one previous-app directory for manual
+recovery. Each successful promotion replaces that retained backup with the
+version that was working immediately beforehand; older exact managed backup
+directories are pruned under the promotion lock.
+
 Updater downloads are streamed to unique temporary files and published as
 `Codex-<sha256>.dmg` only after the file and parent directory are synced. The
 content-addressed path stays immutable while daemon and wrapper rebuild flows
diff --git a/docs/upstream-dmg-acceptance.md b/docs/upstream-dmg-acceptance.md
@@ -36,11 +36,22 @@ app. First installation remains a single atomic sibling rename.
 
 Each build evaluates reports from its own transaction directory, and a
 per-target promotion lock serializes the short final replacement and recovery
-window.
+window. Automated user-local update paths explicitly disable the developer-only
+running-app override. A timer or manual helper may finish building a candidate
+while Electron is open, but promotion is refused until that exact installed
+Electron process exits; the existing app and evidence remain unchanged.
+
+After a successful promotion or journal recovery, cleanup retains exactly one
+managed `<app>.backup-TIMESTAMP[-N]` directory: the app version that was working
+immediately before the accepted candidate. Older managed backups are removed
+under the promotion lock, including read-only local copies derived from Nix.
+Symlinks, regular files, and non-exact/manual backup names are never removed.
+Cleanup failures only warn and are retried by the next install or recovery.
 
 `--fresh` refreshes the DMG and candidate without deleting the working app
-early. Set `CODEX_KEEP_REJECTED_CANDIDATE=1` to retain a rejected candidate for
-debugging. `CODEX_ACCEPTANCE_OVERRIDE=1` is a developer-only emergency escape
+early. Set `CODEX_KEEP_REJECTED_CANDIDATE=1` to retain a rejected or safely
+unpromoted candidate for debugging; otherwise disposable candidates are
+removed. `CODEX_ACCEPTANCE_OVERRIDE=1` is a developer-only emergency escape
 hatch for a completely built candidate; CI and the updater do not set it.
 
 The updater also publishes downloads to immutable, content-addressed paths
@@ -61,6 +72,9 @@ headers so rerunning an obsolete workflow cannot reopen an old issue. The
 identity must contain an ETag or both Last-Modified and Content-Length. If
 either the tested or current identity is unavailable, reconciliation makes no
 issue changes.
+Only issues carrying both the label and a valid hidden 64-character fingerprint
+marker are managed. Manually created labeled issues and malformed markers are
+never updated, reopened, superseded, or closed by the workflow.
 
 ## Manual Validation
 
diff --git a/install.sh b/install.sh
@@ -187,7 +187,14 @@ transactional_install() {
 
     mkdir -p "$candidate_dir/.codex-linux"
     cp "$decision_path" "$candidate_dir/.codex-linux/upstream-dmg-decision.json"
-    promote_candidate_install "$candidate_dir" "$final_dir"
+    if ! promote_candidate_install "$candidate_dir" "$final_dir"; then
+        if [ "${CODEX_KEEP_REJECTED_CANDIDATE:-0}" != "1" ]; then
+            remove_tree_safely "$candidate_dir"
+        else
+            warn "Unpromoted candidate retained for diagnostics: $candidate_dir"
+        fi
+        error "Accepted candidate could not be promoted; the existing app was not changed"
+    fi
     info "Acceptance transaction reports: $report_dir"
     info "Acceptance decision: $published_decision_path"
     if [ -n "${PROMOTED_BACKUP_APP_DIR:-}" ]; then
diff --git a/scripts/ci/upstream-dmg-issue.js b/scripts/ci/upstream-dmg-issue.js
@@ -51,7 +51,10 @@ async function listTrackingIssues(github, repo) {
   const issues = github.paginate
     ? await github.paginate(github.rest.issues.listForRepo, params)
     : (await github.rest.issues.listForRepo(params)).data;
-  return issues.filter((issue) => issue.pull_request == null);
+  // The label is public repository metadata and may also be useful on a
+  // maintainer-created issue. Only the hidden fingerprint marker proves that
+  // this automation owns an issue and may mutate its lifecycle.
+  return issues.filter((issue) => issue.pull_request == null && issueFingerprint(issue) !== null);
 }
 
 async function ensureLabel(github, repo) {
diff --git a/scripts/ci/upstream-dmg-issue.test.js b/scripts/ci/upstream-dmg-issue.test.js
@@ -117,3 +117,63 @@ test("does not mutate accepted or rejected issues when either HTTP identity is m
     }
   }
 });
+
+test("accepted candidates ignore labeled issues without a valid automation marker", async () => {
+  const fixture = fakeGithub([
+    { number: 20, state: "open", body: "Manually tracked upstream investigation" },
+    { number: 21, state: "open", body: "<!-- upstream-dmg-fingerprint:not-a-sha -->" },
+  ]);
+
+  const result = await reconcileUpstreamDmgIssue({
+    github: fixture.github,
+    repo: { owner: "o", repo: "r" },
+    decision: decision("accepted", "5".repeat(64)),
+    currentHttpIdentityKey: "current",
+  });
+
+  assert.equal(result.action, "closed-resolved");
+  assert.equal(result.count, 0);
+  assert.equal(fixture.calls.length, 0);
+});
+
+test("rejected candidates create a managed issue without mutating an unowned labeled issue", async () => {
+  const fixture = fakeGithub([
+    { number: 22, state: "open", body: "Manually tracked upstream investigation" },
+  ]);
+
+  const result = await reconcileUpstreamDmgIssue({
+    github: fixture.github,
+    repo: { owner: "o", repo: "r" },
+    decision: decision("rejected", "6".repeat(64)),
+    currentHttpIdentityKey: "current",
+  });
+
+  assert.equal(result.action, "created");
+  assert.equal(fixture.calls.filter(([name]) => name === "create").length, 1);
+  assert.equal(
+    fixture.calls.some(([, args]) => args.issue_number === 22),
+    false,
+  );
+});
+
+test("mixed reconciliation mutates only issues carrying a valid fingerprint marker", async () => {
+  const fixture = fakeGithub([
+    { number: 23, state: "open", body: "Manually tracked upstream investigation" },
+    { number: 24, state: "open", body: fingerprintMarker("7".repeat(64)) },
+  ]);
+
+  await reconcileUpstreamDmgIssue({
+    github: fixture.github,
+    repo: { owner: "o", repo: "r" },
+    decision: decision("rejected", "8".repeat(64)),
+    currentHttpIdentityKey: "current",
+  });
+
+  assert.equal(
+    fixture.calls.some(([, args]) => args.issue_number === 23),
+    false,
+  );
+  assert.ok(fixture.calls.some(([name, args]) => (
+    name === "update" && args.issue_number === 24 && args.state === "closed"
+  )));
+});
diff --git a/scripts/lib/candidate-install.sh b/scripts/lib/candidate-install.sh
@@ -30,6 +30,45 @@ candidate_promotion_journal_path() {
     printf '%s/.%s.promotion.json\n' "$(dirname "$final_dir")" "$(basename "$final_dir")"
 }
 
+remove_managed_candidate_backup() {
+    local path="$1"
+    [ -d "$path" ] && [ ! -L "$path" ] || return 0
+    # Nix/store-derived app copies may retain read-only directory modes.
+    # `find -P` also avoids dereferencing a path replaced with a symlink.
+    find -P "$path" -type d -exec chmod u+w {} + 2>/dev/null || true
+    [ -d "$path" ] && [ ! -L "$path" ] || return 0
+    rm -rf -- "$path"
+}
+
+cleanup_managed_candidate_backups_locked() {
+    local final_dir="$1"
+    local protected_backup="${2:-}"
+    local keep_latest="${3:-0}"
+    local stale_backups=""
+    local stale_backup
+    local args=(list-stale-backups --final "$final_dir")
+
+    if [ -n "$protected_backup" ]; then
+        args+=(--protect "$protected_backup")
+    elif [ "$keep_latest" = "1" ]; then
+        args+=(--keep-latest)
+    fi
+    if [ "${CODEX_PROMOTION_TEST_FAIL_BACKUP_CLEANUP:-0}" = "1" ]; then
+        warn "Could not prune old app backups (simulated failure); cleanup will be retried"
+        return 0
+    fi
+    if ! stale_backups="$(python3 "$CANDIDATE_PROMOTION_HELPER" "${args[@]}")"; then
+        warn "Could not enumerate old app backups; cleanup will be retried"
+        return 0
+    fi
+    while IFS= read -r stale_backup; do
+        [ -n "$stale_backup" ] || continue
+        if ! remove_managed_candidate_backup "$stale_backup"; then
+            warn "Could not remove old app backup; cleanup will be retried: $stale_backup"
+        fi
+    done <<<"$stale_backups"
+}
+
 recover_pending_candidate_promotion_locked() {
     local final_dir="$1"
     local journal_file
@@ -45,6 +84,7 @@ recover_pending_candidate_promotion_locked() {
         export PROMOTED_BACKUP_APP_DIR
         info "Recovered previous app backup: $recovered_backup"
     fi
+    cleanup_managed_candidate_backups_locked "$final_dir" "$recovered_backup" 1
 }
 
 recover_pending_candidate_promotion() {
@@ -87,7 +127,17 @@ promote_candidate_install() {
     # The long build is allowed while the app runs. Only the short atomic
     # promotion window requires the installed executable to be stopped.
     INSTALL_DIR="$final_dir"
-    assert_install_target_not_running
+    if declare -F install_target_is_stopped >/dev/null 2>&1; then
+        if ! install_target_is_stopped; then
+            warn "ChatGPT Desktop is still running from $final_dir (pid $RUNNING_INSTALL_TARGET_PID); accepted candidate was not promoted"
+            INSTALL_DIR="$previous_install_dir"
+            flock -u "$promotion_lock_fd"
+            exec {promotion_lock_fd}>&-
+            return 1
+        fi
+    else
+        assert_install_target_not_running
+    fi
     INSTALL_DIR="$previous_install_dir"
 
     if [ -e "$final_dir" ]; then
@@ -132,10 +182,12 @@ promote_candidate_install() {
             exec {promotion_lock_fd}>&-
             return 1
         fi
+        cleanup_managed_candidate_backups_locked "$final_dir" "$backup"
     else
         info "Promoting accepted candidate: $final_dir"
         mv "$candidate_dir" "$final_dir"
         backup=""
+        cleanup_managed_candidate_backups_locked "$final_dir"
     fi
 
     if [ -n "$backup" ] && [ ! -d "$backup" ]; then
diff --git a/scripts/lib/candidate-promotion.py b/scripts/lib/candidate-promotion.py
@@ -9,6 +9,8 @@
 import json
 import os
 from pathlib import Path
+import re
+import stat
 import sys
 import uuid
 
@@ -246,6 +248,42 @@ def recover(args: argparse.Namespace) -> None:
     raise RuntimeError(f"Cannot safely recover interrupted promotion: {journal_path}")
 
 
+def list_stale_backups(args: argparse.Namespace) -> None:
+    final = Path(args.final).absolute()
+    protected = Path(args.protect).absolute() if args.protect else None
+    pattern = re.compile(rf"^{re.escape(final.name)}\.backup-(\d{{14}})(?:-([1-9]\d*))?$")
+    managed: list[tuple[tuple[str, int], Path]] = []
+
+    try:
+        siblings = list(final.parent.iterdir())
+    except FileNotFoundError:
+        return
+
+    for sibling in siblings:
+        match = pattern.fullmatch(sibling.name)
+        if match is None:
+            continue
+        try:
+            metadata = sibling.lstat()
+        except FileNotFoundError:
+            continue
+        # Never follow or remove a symlink, regular file, or similarly named
+        # maintainer-owned path.
+        if not stat.S_ISDIR(metadata.st_mode):
+            continue
+        managed.append(((match.group(1), int(match.group(2) or 0)), sibling.absolute()))
+
+    keep: Path | None = None
+    if protected is not None and any(path == protected for _, path in managed):
+        keep = protected
+    elif args.keep_latest and managed:
+        keep = max(managed, key=lambda item: item[0])[1]
+
+    for _, path in managed:
+        if path != keep:
+            print(path)
+
+
 def parser() -> argparse.ArgumentParser:
     value = argparse.ArgumentParser()
     subcommands = value.add_subparsers(dest="command", required=True)
@@ -272,6 +310,12 @@ def parser() -> argparse.ArgumentParser:
     recover_parser.add_argument("--journal", required=True)
     recover_parser.add_argument("--final", required=True)
     recover_parser.set_defaults(function=recover)
+
+    stale_parser = subcommands.add_parser("list-stale-backups")
+    stale_parser.add_argument("--final", required=True)
+    stale_parser.add_argument("--protect")
+    stale_parser.add_argument("--keep-latest", action="store_true")
+    stale_parser.set_defaults(function=list_stale_backups)
     return value
 
 
diff --git a/scripts/lib/install-helpers.sh b/scripts/lib/install-helpers.sh
@@ -87,7 +87,8 @@ Environment variables:
                       Developer-only promotion override for a completely built
                       candidate rejected by the shared acceptance profile
   CODEX_KEEP_REJECTED_CANDIDATE=1
-                      Keep a rejected sibling candidate for diagnostics
+                      Keep a rejected or safely unpromoted sibling candidate
+                      for diagnostics
 
 After install, launch with:
   ./codex-app/start.sh
diff --git a/scripts/lib/process-detection.sh b/scripts/lib/process-detection.sh
@@ -78,18 +78,29 @@ find_running_install_target_pid() {
 }
 
 assert_install_target_not_running() {
+    if ! install_target_is_stopped; then
+        error "ChatGPT Desktop is currently running from $INSTALL_DIR (pid $RUNNING_INSTALL_TARGET_PID).
+Close that app before rebuilding this install directory, or build into a separate path:
+  CODEX_INSTALL_DIR=/tmp/codex-app-build ./install.sh
+
+Set CODEX_INSTALL_ALLOW_RUNNING=1 only if you intentionally want to overwrite a running app."
+    fi
+}
+
+install_target_is_stopped() {
     local pid
+    RUNNING_INSTALL_TARGET_PID=""
 
     if [ "${CODEX_INSTALL_ALLOW_RUNNING:-0}" = "1" ]; then
         warn "CODEX_INSTALL_ALLOW_RUNNING=1 set; installer may overwrite a running ChatGPT app"
         return 0
     fi
 
     if pid="$(find_running_install_target_pid)"; then
-        error "ChatGPT Desktop is currently running from $INSTALL_DIR (pid $pid).
-Close that app before rebuilding this install directory, or build into a separate path:
-  CODEX_INSTALL_DIR=/tmp/codex-app-build ./install.sh
-
-Set CODEX_INSTALL_ALLOW_RUNNING=1 only if you intentionally want to overwrite a running app."
+        RUNNING_INSTALL_TARGET_PID="$pid"
+        export RUNNING_INSTALL_TARGET_PID
+        return 1
     fi
+
+    return 0
 }
diff --git a/tests/scripts_smoke.sh b/tests/scripts_smoke.sh
diff --git a/updater/Cargo.toml b/updater/Cargo.toml
diff --git a/updater/src/wrapper_apply.rs b/updater/src/wrapper_apply.rs
