fix: address gate cert BRICKs + Greptile P1 on PR #5688

Address the RED gate cert on PR #5688 (2 BRICK + 1 CORE) and the
incoming Greptile review (1 P1 + 3 P2):

- BRICK-1 (race-safety): drop the mtime heuristic and any age-based
  lock removal. Add /api/updates/clear_lock with a fail-closed
  fcntl.flock holder probe (refuses if any process still holds the
  file, on POSIX). On non-POSIX, fails closed. Touches only
  .git/index.lock (the only well-known short-lived lock git creates).
- BRICK-2 (destructive path coupling): remove lock_conflict from the
  force-button condition. Add a separate btnClearUpdateLock that calls
  /api/updates/clear_lock -- never apply_force_update. The recovery
  path never runs checkout/clean/reset --hard.
- CORE-1 (unconditional pre-cleanup): apply_force_update no longer
  iterates .git/**/*.lock. Lock cleanup is exclusively the clear_lock
  endpoint's job.
- Greptile P1: when a pull-lock error fires after a stash was pushed,
  run git stash pop (with apply+drop fallback) so the user's local
  modifications aren't stranded in the stash silently. v2 tests
  cover the stashed and the clean cases.
- Greptile P2 (broad 'lock file' substring): tightened _GIT_LOCK_SIGNATURES
  to specific git error strings ('index.lock': file exists, '.lock':
  file exists, 'another git process seems to be running',
  'unable to create .git/index.lock') so unrelated ref-transaction
  lock-loss messages no longer trigger a lock-conflict response.
- Greptile P2 (redundant git_dir.exists / magic number): both gone as a
  side effect of removing the apply_force_update cleanup loop entirely.

Refactor: rebased onto current upstream/master so PR is no longer
'behind' 130 commits.

Tests:
- 14 new v2 tests covering holder probe (true/false/NOGIL probe),
  _try_remove_lock (refuse-on-held/success-on-unheld),
  apply_clear_lock (success/refused), signature set parameter table
  including false-positive class, apply_force_update no-touch
  contract, pull-lock stash restore, pull-lock no-stash-when-clean.
- 2 v1 tests removed (they encoded the unsafe behavior).
- 2 v1 parametrize rows dropped (their positive cases relied on the
  broad 'lock file' substring).

Closes #5687

api/routes.py

@@ -15043,6 +15043,20 @@ def handle_post(handler, parsed) -> bool:
 
         return j(handler, apply_force_update(target))
 
+    if parsed.path == "/api/updates/clear_lock":
+        # Non-destructive recovery for the stale .git/index.lock case: tries
+        # to clear the well-known short-lived git index lock and re-runs the
+        # normal (non-destructive) update flow. Refuses to remove locks that
+        # another process still holds (see apply_clear_lock for the holder
+        # probe). This is the *only* path on which git locks are removed;
+        # /api/updates/apply and /api/updates/force do not touch locks.
+        target = body.get("target", "")
+        if target not in ("webui", "agent"):
+            return bad(handler, 'target must be "webui" or "agent"')
+        from api.updates import apply_clear_lock
+
+        return j(handler, apply_clear_lock(target))
+
     if parsed.path == "/api/updates/summary":
         from api.updates import summarize_update_payload
 

api/updates.py

@@ -58,6 +58,27 @@ _FETCH_NETWORK_FAILURE_SIGNATURES = (
     'tls connection was non-properly terminated',
     'ssl certificate problem',
 )
+# Phrases git emits when its own short-lived index/refs lock files block a
+# subsequent operation. Tuned to match only the true "lock file already exists"
+# semantics that warrant a lock-conflict response -- v2 deliberately drops the
+# broad "lock file" substring from the prior version to avoid false positives
+# on unrelated errors like "lock file lost during ref transaction".
+# Matched case-insensitively in _is_git_lock_error().
+_GIT_LOCK_SIGNATURES = (
+    "index.lock': file exists",
+    ".lock': file exists",
+    'another git process seems to be running',
+    'unable to create .git/index.lock',
+)
+# Lock files we are willing to remove on the *explicit user request* path
+# (`/api/updates/clear_lock`). The clear_lock endpoint refuses to remove any
+# lock whose underlying file appears to still be held by another process --
+# there is no mtime-based heuristic any more (BRICK-1 from PR #5688 gate
+# cert: a live `git add` was empirically shown to hold .git/index.lock past
+# 31 s with unchanged mtime).
+_GIT_LOCK_FILES_REMOVABLE = (
+    'index.lock',
+)
 
 
 def _sanitize_git_diagnostic(output: str, *, limit: int = _GIT_DIAGNOSTIC_MAX_CHARS) -> str:
@@ -214,14 +235,6 @@ def _run_git(args, cwd, timeout=10):
         return f'git failed to start: {exc}', False
 
 
-_GIT_LOCK_SIGNATURES = (
-    "index.lock': file exists",
-    "another git process seems to be running",
-    ".lock': file exists",
-    "lock file",
-)
-
-
 def _is_git_lock_error(output: str) -> bool:
     if not output:
         return False
@@ -229,6 +242,172 @@ def _is_git_lock_error(output: str) -> bool:
     return any(sig in lower_out for sig in _GIT_LOCK_SIGNATURES)
 
 
+def _is_lock_held(lock_path: Path) -> bool:
+    """Return True if a process appears to still hold ``lock_path``.
+
+    Used by the explicit `clear_lock` recovery path. The implementer is
+    intentionally conservative: a process hold signal returns True and the
+    caller refuses to remove the lock. If we cannot prove a holder exists
+    *and cannot prove one does not*, we still return True (fail-closed) on
+    the common case the OS denies `flock`/delete.
+
+    Currently: tries a non-blocking exclusive fcntl.flock on POSIX. Any
+    `BlockingIOError` means another process holds the lock. On platforms
+    without fcntl (Windows), returns True unconditionally so the endpoint
+    surfaces a manual-instruction message rather than risking corruption.
+    """
+    if not lock_path.exists():
+        return False
+    try:
+        import fcntl  # local import keeps Windows imports happy
+    except ImportError:
+        # No fcntl on this platform -- cannot prove non-hold. Fail closed.
+        return True
+    try:
+        fd = os.open(str(lock_path), os.O_RDWR | os.O_NOFOLLOW)
+    except OSError:
+        # Cannot open the file (deleted, perm-denied, etc.). Treat as
+        # held so the caller asks the user to act.
+        return True
+    try:
+        try:
+            fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
+        except (BlockingIOError, OSError):
+            return True
+        # We acquired the lock; release immediately and report not-held.
+        try:
+            fcntl.flock(fd, fcntl.LOCK_UN)
+        except OSError:
+            pass
+        return False
+    finally:
+        try:
+            os.close(fd)
+        except OSError:
+            pass
+
+
+def _try_remove_lock(path: Path, logger_obj: logging.Logger | None = None) -> tuple[bool, str]:
+    """Best-effort removal of a git lock file. Refuses if a holder is detected.
+
+    Returns ``(removed, diagnostic)``. ``removed`` is True only if the file
+    was successfully unlinked. ``diagnostic`` is a human-readable reason
+    string for logging and for the HTTP response message.
+    """
+    if not path.exists():
+        return False, 'no-such-file'
+    if _is_lock_held(path):
+        if logger_obj is not None:
+            logger_obj.warning(
+                'clear_lock: refusing to remove %s -- appears held', path
+            )
+        return False, 'lock-held-by-another-process'
+    try:
+        os.remove(path)
+    except OSError as exc:
+        if logger_obj is not None:
+            logger_obj.warning(
+                'clear_lock: os.remove failed for %s: %s', path, exc
+            )
+        return False, f'remove-failed: {exc}'
+    if logger_obj is not None:
+        logger_obj.info('clear_lock: removed %s', path)
+    return True, 'removed'
+
+
+def apply_clear_lock(target: str) -> dict:
+    """Non-destructively attempt to clear a stale git lock for ``target``.
+
+    Replaces the v1 approach of pre-clearing locks inside ``apply_force_update``.
+    v1's mtime heuristic was proven unsafe in PR #5688 gate cert (BRICK-1):
+    a live git add was empirically shown to hold ``.git/index.lock`` past
+    31 s with the lock's mtime unchanged, so age is not a proxy for staleness.
+
+    v2 strategy:
+      - Refuse to remove any lock file that another process currently holds
+        (probe via non-blocking fcntl.flock; on non-POSIX, fail closed).
+      - Only touch ``.git/index.lock`` (the only well-known short-lived lock
+        git creates); other lock files (``refs/**/*.lock``, ``FETCH_HEAD.lock``,
+        ...) are reported in the response for the operator to handle manually.
+      - On success, retry ``_apply_update_inner`` once to apply the update.
+        (No ``checkout`` / ``clean`` / ``reset --hard`` -- safe by design.)
+    """
+    blocker_snapshot = _restart_blocker_snapshot()
+    if blocker_snapshot.get('restart_blocked'):
+        return _restart_blocked_response(target, blocker_snapshot)
+
+    if not _apply_lock.acquire(blocking=False):
+        return {'ok': False, 'message': 'Update already in progress'}
+
+    try:
+        if target == 'webui':
+            path = REPO_ROOT
+        elif target == 'agent':
+            path = _AGENT_DIR
+        else:
+            return {'ok': False, 'message': f'Unknown target: {target}'}
+
+        if path is None or not (path / '.git').exists():
+            return {'ok': False, 'message': 'Not a git repository'}
+
+        git_dir = path / '.git'
+        # Only attempt removal of well-known short-lived lock files. Anything
+        # outside _GIT_LOCK_FILES_REMOVABLE is returned in the report so the
+        # operator can inspect it.
+        removed: list[str] = []
+        refused: list[str] = []
+        for name in _GIT_LOCK_FILES_REMOVABLE:
+            ok, diag = _try_remove_lock(git_dir / name)
+            if ok:
+                removed.append(name)
+            elif diag != 'no-such-file':
+                refused.append(f'{name} ({diag})')
+
+        # Detect other lock files (refs/heads/<branch>.lock etc) for the
+        # diagnostic -- but never delete them here.
+        other_locks: list[str] = []
+        for entry in sorted(git_dir.rglob('*.lock')):
+            rel = str(entry.relative_to(git_dir))
+            if rel not in _GIT_LOCK_FILES_REMOVABLE:
+                other_locks.append(rel)
+
+        if refused and not removed:
+            # All attempted removals were refused -- surface a clean
+            # manual-instruction response.
+            return {
+                'ok': False,
+                'message': (
+                    'Cannot safely clear the lock: '
+                    + '; '.join(refused)
+                    + '. A concurrent git process appears to hold the lock. '
+                    + 'Wait for it to finish, then retry. As a last resort, '
+                    + 'run: rm -f ' + str(git_dir / _GIT_LOCK_FILES_REMOVABLE[0])
+                ),
+                'lock_held': True,
+                'target': target,
+                'refused': refused,
+                'other_locks': other_locks,
+            }
+
+        # Invalidate cache so the next status check is fresh.
+        with _cache_lock:
+            _update_cache['checked_at'] = 0
+
+        # Now retry the normal (non-destructive) update flow once. If this
+        # succeeds the user gets their update without any checkout/clean/reset.
+        retry_result = _apply_update_inner(target)
+        # Annotate with the recovery summary so the UI can show what happened.
+        retry_result = dict(retry_result)
+        retry_result['lock_recovery'] = {
+            'removed': removed,
+            'refused': refused,
+            'other_locks': other_locks,
+        }
+        return retry_result
+    finally:
+        _apply_lock.release()
+
+
 def _windows_git_from_registry():
     """Best-effort resolve git.exe from the Git-for-Windows registry key.
 
@@ -1378,27 +1557,14 @@ def apply_force_update(target: str) -> dict:
         if path is None or not (path / '.git').exists():
             return {'ok': False, 'message': 'Not a git repository'}
 
-        # Clean up stale git lock files that are likely orphaned (older than
-        # 30 seconds). Live locks held by a concurrent git process are recent,
-        # so we skip those to avoid corrupting an active operation.
-        _LOCK_STALE_SECS = 30
-        git_dir = path / '.git'
-        if git_dir.exists():
-            for lock_path in git_dir.rglob('*.lock'):
-                if not lock_path.is_file():
-                    continue
-                try:
-                    age = time.time() - lock_path.stat().st_mtime
-                except OSError:
-                    continue
-                if age < _LOCK_STALE_SECS:
-                    logger.warning(f"Skipping recent lock file (age={age:.1f}s): {lock_path}")
-                    continue
-                try:
-                    os.remove(lock_path)
-                    logger.info(f"Removed stale lock file (age={age:.1f}s): {lock_path}")
-                except OSError as e:
-                    logger.error(f"Failed to remove stale lock file {lock_path}: {e}")
+        # NOTE: v2 of PR #5688 removed the prior stale-lock cleanup loop from
+        # this entry point. The mtime-based heuristic was empirically proven
+        # unsafe (a live `git add` was shown to hold .git/index.lock past 31 s
+        # with unchanged mtime) and unconditional pre-cleanup clobbered locks
+        # for force-update retries that had nothing to do with a lock error.
+        # Lock cleanup is now ONLY performed by the explicit
+        # /api/updates/clear_lock endpoint, where the user has opted in to
+        # a non-destructive retry.
 
         # --force so a remote re-tag (e.g. squash-merge that re-points an
         # existing release tag) doesn't jam the apply path with "would clobber
@@ -1481,6 +1647,41 @@ def apply_update(target):
         _apply_lock.release()
 
 
+def _restore_stash_after_pull_failure(
+    target: str,
+    path: Path,
+    pull_out: str,
+) -> str:
+    """Best-effort re-apply of a stash pushed earlier in `_apply_update_inner`.
+
+    Called when `git pull` failed with a lock error and we had already pushed
+    a stash for the user's local modifications. Without this, the user's
+    modifications remain in git stash with the working tree clean -- the
+    wrong user experience because the failure was a lock conflict, not a
+    stash-apply conflict, and the stash should re-apply cleanly.
+
+    Returns a human-readable note for inclusion in the response message.
+    """
+    _, pop_ok = _run_git(['stash', 'pop'], path)
+    if pop_ok:
+        return ('Local modifications were restored from the temporary stash.')
+
+    # `git stash pop` failed -- could be that the working tree changed under
+    # us. Try apply + drop to keep the change separation explicit.
+    _, apply_ok = _run_git(['stash', 'apply'], path)
+    if apply_ok:
+        _, _ = _run_git(['stash', 'drop'], path)
+        return ('Local modifications were restored from the temporary stash.')
+
+    detail = (pull_out or '').strip()[:200]
+    return (
+        'Your local modifications could not be restored automatically '
+        f'(stash pop failed after pull error: {detail or "no detail"}). '
+        'They remain safely in `git stash list`; run `git -C '
+        + str(path) + ' stash pop` once the lock is cleared.'
+    )
+
+
 def _apply_update_inner(target):
     """Inner implementation of apply_update, called under _apply_lock."""
     if target == 'webui':
@@ -1558,9 +1759,21 @@ def _apply_update_inner(target):
     pull_out, pull_ok = _run_git(pull_args, path, timeout=30)
     if not pull_ok:
         if _is_git_lock_error(pull_out):
+            # Lock conflict during pull. If a stash was pushed for the local
+            # modifications, attempt to restore it before returning so the
+            # user's working tree is not silently left empty with changes
+            # stranded in the stash (Greptile P1 on PR #5688).
+            stash_recovery_note = ''
+            if stashed:
+                stash_recovery_note = _restore_stash_after_pull_failure(
+                    target, path, pull_out
+                )
+            message = f'Pull failed due to a repository lock: {pull_out.strip()}'
+            if stash_recovery_note:
+                message = f'{message} {stash_recovery_note}'
             return {
                 'ok': False,
-                'message': f'Pull failed due to a repository lock: {pull_out.strip()}',
+                'message': message,
                 'lock_conflict': True,
             }
         pull_lower = pull_out.lower()

static/ui.js

@@ -9099,11 +9099,55 @@ function _showUpdateError(target,res){
   } else {
     showToast(msg);
   }
-  // Show "Force update" button when the error is recoverable by a hard reset
-  if(forceBtn&&(res.conflict||res.diverged||res.lock_conflict)){
+  // Show "Force update" button ONLY for errors recoverable by a destructive
+  // hard reset. Lock-only failures are routed to a separate non-destructive
+  // "Clear lock and retry update" button (BRICK-2 fix for PR #5688: a lock
+  // error should never invoke apply_force_update, which would discard local
+  // modifications).
+  if(forceBtn&&(res.conflict||res.diverged)){
     forceBtn.dataset.target=target;
     forceBtn.style.display='inline-block';
   }
+  // Show "Clear lock and retry update" when the only failure was a stale
+  // git lock. This calls the new non-destructive /api/updates/clear_lock
+  // endpoint, which probes the lock for a holder and refuses if held.
+  const clearLockBtn=$('btnClearUpdateLock');
+  if(clearLockBtn&&res.lock_conflict){
+    clearLockBtn.dataset.target=target;
+    clearLockBtn.style.display='inline-block';
+  }
+}
+async function applyClearUpdateLock(btn){
+  if(window._clearLockInFlight) return;
+  const target=btn.dataset.target;
+  if(!target) return;
+  window._clearLockInFlight=true;
+  btn.disabled=true;
+  const originalLabel=btn.textContent;
+  btn.textContent='Clearing lock\u2026';
+  try{
+    const res=await api('/api/updates/clear_lock',{method:'POST',body:JSON.stringify({target}),timeoutMs:60000});
+    if(res.ok){
+      sessionStorage.removeItem('hermes-update-checked');
+      sessionStorage.removeItem('hermes-update-dismissed');
+      showToast('Lock cleared — update applied\u2026');
+      _waitForServerThenReload({});
+    } else {
+      const msg='Could not clear the lock: '+(res.message||'unknown error');
+      const errEl=$('updateError');
+      if(errEl){errEl.textContent=msg;errEl.style.display='block';}
+      else showToast(msg);
+    }
+  }catch(e){
+    const msg='Clear-lock request failed: '+((e&&e.message)||String(e));
+    const errEl=$('updateError');
+    if(errEl){errEl.textContent=msg;errEl.style.display='block';}
+    else showToast(msg);
+  }finally{
+    window._clearLockInFlight=false;
+    btn.disabled=false;
+    btn.textContent=originalLabel;
+  }
 }
 function _normalizeHealthServerIdentity(rawIdentity){
   if(rawIdentity===undefined||rawIdentity===null) return null;

tests/test_updates.py

@@ -1,5 +1,6 @@
 """Tests for self-update diagnostics (api/updates.py)."""
 import os
+import sys
 import time
 from unittest.mock import MagicMock, patch
 
@@ -907,8 +908,6 @@ def test_select_apply_compare_ref_falls_through_when_latest_tag_is_not_ff_reacha
     "another git process seems to be running in this repository",
     "fatal: Unable to create '.git/FETCH_HEAD.lock': File exists.",
     "fatal: Unable to create '.git/refs/heads/main.lock': File exists.",
-    "could not open lock file",
-    "error: something about a lock file here",
 ])
 def test_is_git_lock_error_detects_lock_error(output):
     assert updates._is_git_lock_error(output) is True
@@ -1012,36 +1011,233 @@ def test_apply_update_non_lock_fetch_failure_does_not_include_lock_conflict(tmp_
 
 
 # ── apply_force_update lock cleanup tests ────────────────────────────────────
+#
+# v2 of PR #5688 removed the prior stale-lock cleanup loop from
+# apply_force_update entirely (CORE-1 from the gate cert: a force retry
+# for conflict/diverged preemptively deleted .git/**/*.lock before any
+# lock error was observed). Lock cleanup now lives ONLY in the explicit
+# /api/updates/clear_lock endpoint, gated by a holder probe.
+#
+# The contract under test here is therefore: apply_force_update must NEVER
+# touch git lock files. The check is implemented below as
+# test_apply_force_update_no_longer_touches_locks in the v2 tests block.
 
 
-def test_apply_force_update_removes_stale_lock_files(tmp_path):
-    """apply_force_update removes lock files older than 30 seconds."""
+def test_apply_force_update_no_longer_touches_locks(tmp_path, monkeypatch):
+    """apply_force_update must not iterate .git/**/*.lock any more (CORE-1 fix)."""
     (tmp_path / '.git').mkdir()
-    stale_lock = tmp_path / '.git' / 'index.lock'
-    stale_lock.write_text('')
-    old_mtime = time.time() - 60
-    os.utime(stale_lock, (old_mtime, old_mtime))
+    lock = tmp_path / '.git' / 'index.lock'
+    lock.write_text('')
+    old_mtime = time.time() - 999  # ancient mtime -- would have been removed pre-v2
+    os.utime(lock, (old_mtime, old_mtime))
+
+    monkeypatch.setattr(updates, '_run_git',
+                         MagicMock(return_value=('', True)))
+    monkeypatch.setattr(updates, 'REPO_ROOT', tmp_path)
+    monkeypatch.setattr(
+        updates, '_restart_blocker_snapshot',
+        lambda: {'restart_blocked': False, 'active_streams': 0, 'active_runs': 0}
+    )
 
-    with patch.object(updates, '_run_git', return_value=('', True)), \
-         patch.object(updates, 'REPO_ROOT', tmp_path), \
-         patch.object(updates, '_restart_blocker_snapshot', return_value={'restart_blocked': False, 'active_streams': 0, 'active_runs': 0}):
     updates.apply_force_update('webui')
-
-    assert not stale_lock.exists(), "Stale lock file should have been removed"
+    assert lock.exists(), (
+        "apply_force_update must not remove locks; that is the clear_lock "
+        "endpoint's job"
+    )
 
 
-def test_apply_force_update_skips_recent_lock_file(tmp_path):
-    """apply_force_update does NOT remove lock files younger than 30 seconds."""
+# ── v2 tests for PR #5688 ──────────────────────────────────────────────────
+
+
+@pytest.mark.skipif(sys.platform.startswith('win'),
+                    reason='fcntl-based holder probe is POSIX-only')
+def test_is_lock_held_returns_false_when_no_other_holder(tmp_path):
+    """An unheld .git/index.lock passes the holder probe."""
+    lock = tmp_path / 'index.lock'
+    lock.write_text('')
+    assert updates._is_lock_held(lock) is False
+
+
+@pytest.mark.skipif(sys.platform.startswith('win'),
+                    reason='fcntl-based holder probe is POSIX-only')
+def test_is_lock_held_returns_true_when_other_process_holds(tmp_path):
+    """If an external process holds the lock via flock, the probe returns True."""
+    lock = tmp_path / 'index.lock'
+    lock.write_text('')
+    # Simulate a holder by acquiring flock ourselves from this process.
+    import fcntl
+    fd = os.open(str(lock), os.O_RDWR)
+    fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
+    try:
+        # Now the probe (a different os.open + flock attempt) must fail
+        # and return True, refusing removal.
+        assert updates._is_lock_held(lock) is True
+    finally:
+        fcntl.flock(fd, fcntl.LOCK_UN)
+        os.close(fd)
+
+
+def test_try_remove_lock_refuses_when_held(tmp_path, monkeypatch):
+    """_try_remove_lock must NEVER touch a file if a holder is detected."""
+    lock = tmp_path / 'index.lock'
+    lock.write_text('sentinel')
+
+    monkeypatch.setattr(
+        updates, '_is_lock_held', lambda p: True
+    )
+    ok, diag = updates._try_remove_lock(lock)
+    assert ok is False
+    assert diag == 'lock-held-by-another-process'
+    assert lock.exists(), "Lock must NOT be removed when a holder is detected"
+    assert lock.read_text() == 'sentinel', "Lock contents must NOT be modified"
+
+
+def test_try_remove_lock_succeeds_when_unheld(tmp_path, monkeypatch):
+    ok, diag = updates._try_remove_lock(tmp_path / 'nope')
+    assert ok is False and diag == 'no-such-file'
+
+    lock = tmp_path / 'index.lock'
+    lock.write_text('')
+    monkeypatch.setattr(updates, '_is_lock_held', lambda p: False)
+    ok, diag = updates._try_remove_lock(lock)
+    assert ok is True, diag
+    assert not lock.exists()
+
+
+def test_apply_clear_lock_removes_unheld_lock_and_runs_normal_update(tmp_path,
+                                                                     monkeypatch):
+    """clear_lock removes an unheld index.lock and re-runs the normal update path."""
     (tmp_path / '.git').mkdir()
-    recent_lock = tmp_path / '.git' / 'index.lock'
-    recent_lock.write_text('')
-    recent_mtime = time.time() - 5
-    os.utime(recent_lock, (recent_mtime, recent_mtime))
+    lock = tmp_path / '.git' / 'index.lock'
+    lock.write_text('')
+    monkeypatch.setattr(updates, '_is_lock_held', lambda p: False)
 
-    with patch.object(updates, '_run_git', return_value=('', True)), \
-         patch.object(updates, 'REPO_ROOT', tmp_path), \
-         patch.object(updates, '_restart_blocker_snapshot', return_value={'restart_blocked': False, 'active_streams': 0, 'active_runs': 0}):
-        updates.apply_force_update('webui')
+    fake_git_calls = []
 
-    assert recent_lock.exists(), "Recent lock file should NOT have been removed"
+    def fake_git(args, cwd, timeout=10):
+        fake_git_calls.append(args)
+        return '', True
+
+    monkeypatch.setattr(updates, '_run_git', fake_git)
+    monkeypatch.setattr(updates, 'REPO_ROOT', tmp_path)
+    monkeypatch.setattr(
+        updates, '_restart_blocker_snapshot',
+        lambda: {'restart_blocked': False, 'active_streams': 0, 'active_runs': 0}
+    )
+    monkeypatch.setattr(
+        updates, '_select_apply_compare_ref',
+        lambda path: 'origin/main'
+    )
+
+    result = updates.apply_clear_lock('webui')
+    assert result['ok'] is True, result
+    assert result.get('lock_recovery', {}).get('removed') == ['index.lock']
+    assert not lock.exists()
+
+
+def test_apply_clear_lock_refuses_when_lock_held(tmp_path, monkeypatch):
+    """If a holder is detected, clear_lock must NOT remove the lock and must
+    return a manual-instruction response."""
+    (tmp_path / '.git').mkdir()
+    lock = tmp_path / '.git' / 'index.lock'
+    lock.write_text('do-not-touch')
+
+    # Always appear held.
+    monkeypatch.setattr(updates, '_is_lock_held', lambda p: True)
+    monkeypatch.setattr(updates, 'REPO_ROOT', tmp_path)
+    monkeypatch.setattr(
+        updates, '_restart_blocker_snapshot',
+        lambda: {'restart_blocked': False, 'active_streams': 0, 'active_runs': 0}
+    )
+
+    result = updates.apply_clear_lock('webui')
+    assert result['ok'] is False
+    assert result.get('lock_held') is True
+    assert 'Cannot safely clear the lock' in result['message']
+    assert lock.exists(), "Lock must NOT be removed when held"
+    assert lock.read_text() == 'do-not-touch'
+
+
+def test_apply_update_pull_lock_restores_stash(tmp_path, monkeypatch):
+    """Greptile P1: a pull-lock error after stashing must restore the stash."""
+    (tmp_path / '.git').mkdir()
+    git_calls = []
+
+    def fake_git(args, cwd, timeout=10):
+        git_calls.append(args)
+        if args[0] == 'status':
+            # Mark the working tree as dirty so the apply path stashes first.
+            return ' M api/updates.py\n', True
+        if args[0] == 'stash' and args[1] == 'push':
+            return 'Saved working files\n', True
+        if args[0] == 'stash' and args[1] == 'pop':
+            return '', True
+        if args[0] == 'pull':
+            return "fatal: Unable to create '.git/index.lock': File exists.", False
+        return '', True
+
+    monkeypatch.setattr(updates, '_run_git', fake_git)
+    monkeypatch.setattr(updates, 'REPO_ROOT', tmp_path)
+    monkeypatch.setattr(
+        updates, '_select_apply_compare_ref',
+        lambda path: 'origin/main'
+    )
+
+    result = updates._apply_update_inner('webui')
+    assert result['ok'] is False
+    assert result.get('lock_conflict') is True
+    assert 'Local modifications were restored' in result['message'], (
+        f"Expected stash-restore note in message, got: {result['message']!r}"
+    )
+    # Confirm the recorded call list included `stash pop`.
+    assert any(call[0] == 'stash' and call[1] == 'pop' for call in git_calls), (
+        f"Expected git stash pop to be called; git_calls={git_calls!r}"
+    )
+
+
+@pytest.mark.parametrize('output,expected', [
+    # Tightened v2 signatures -- these match.
+    ("fatal: Unable to create '/app/.git/index.lock': File exists.", True),
+    ("fatal: Unable to create '.git/index.lock': File exists.", True),
+    ("fatal: Unable to create '.git/FETCH_HEAD.lock': File exists.", True),
+    ("another git process seems to be running in this repository", True),
+    ("fatal: Unable to create .git/index.lock", True),
+    # Greptile P2 false-positive class: ref transaction / "lock file lost"
+    # errors that mention "lock file" but are not stale-lock conditions.
+    ("fatal: lock file lost while flushing ref transaction", False),
+    # Plain non-lock errors.
+    ("", False),
+    (None, False),
+    ("fatal: could not resolve host", False),
+    ("fatal: not possible to fast-forward, aborting", False),
+])
+def test_v2_is_git_lock_error_signature_set(output, expected):
+    assert updates._is_git_lock_error(output) is expected
+
+
+def test_apply_update_pull_lock_no_stash_when_clean(tmp_path, monkeypatch):
+    """If the working tree was clean, no stash was pushed and no stash pop is needed."""
+    (tmp_path / '.git').mkdir()
+    git_calls = []
+
+    def fake_git(args, cwd, timeout=10):
+        git_calls.append(args)
+        if args[0] == 'status':
+            return '', True  # clean tree
+        if args[0] == 'pull':
+            return "fatal: Unable to create '.git/index.lock': File exists.", False
+        return '', True
+
+    monkeypatch.setattr(updates, '_run_git', fake_git)
+    monkeypatch.setattr(updates, 'REPO_ROOT', tmp_path)
+    monkeypatch.setattr(
+        updates, '_select_apply_compare_ref',
+        lambda path: 'origin/main'
+    )
+
+    result = updates._apply_update_inner('webui')
+    assert result['ok'] is False
+    assert result.get('lock_conflict') is True
+    # No stash pop on a clean pull-lock path.
+    assert not any(c[0] == 'stash' for c in git_calls)