fix: make serve lifecycle work on Windows (process + permission semantics)

CHANGELOG.md

@@ -18,6 +18,31 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   are set), and `auth set gemini` is accepted and stores the key like any other keyed
   provider. Default model is `gemini-2.5-flash`.
 
+### Fixed
+
+- **`serve status` / `stop` / `restart` no longer misbehave on Windows.** Three
+  POSIX assumptions in the serve lifecycle were wrong on Windows and are now
+  branched explicitly:
+  - `os.kill(pid, 0)` was used as a benign liveness probe. On Windows it is not
+    one: CPython maps `CTRL_C_EVENT` and `CTRL_BREAK_EVENT` to
+    `GenerateConsoleCtrlEvent` and every other signal to `TerminateProcess`, and
+    because `CTRL_C_EVENT == 0`, signal 0 delivers a real console Ctrl+C to the
+    target's process group. `serve status` could therefore interrupt the very
+    server it was reporting on. Liveness is now checked with
+    `OpenProcess`/`WaitForSingleObject`, which observes without signalling.
+  - `signal.SIGKILL` does not exist on Windows and raised `AttributeError` when
+    escalating a stop; the escalation path now falls back to `SIGTERM`, which
+    CPython routes to `TerminateProcess`.
+  - `subprocess.Popen(start_new_session=True)` is a POSIX-only way to detach the
+    restarted server; Windows now uses `DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP`.
+
+  The state file's 0600/0700 permission *verification* is also now skipped on
+  Windows, where NTFS has no POSIX mode bits and the check could never pass —
+  previously this refused to write `serve.json` at all. `chmod` is still applied;
+  only the POSIX-specific assertion is dropped, and the protection boundary there
+  is the user-profile ACL. POSIX behaviour is unchanged: enforcement still fails
+  closed rather than writing a plaintext `api_key` at a looser mode.
+
 ## [0.13.0] — 2026-08-18
 
 ### Added

docs/spec/http-face.md

@@ -55,6 +55,13 @@ Written atomically, and never observable at a looser mode than 0600: the permiss
 and verified before any payload is written. The parent directory is forced to 0700 with the same
 verification. Both fail with an error rather than write a plaintext `api_key` at a looser mode.
 
+On Windows the verification is skipped, because the guarantee it asserts is not one the platform
+makes: NTFS has no POSIX mode bits, `chmod` cannot produce 0600/0700, and `stat` reports 0666/0777,
+so the comparison could never pass and the server could never persist its state. The protection
+boundary there is the ACL on `%USERPROFILE%\.amplifier-agent`, which by default denies other
+standard users. `chmod` is still applied; only the POSIX-specific assertion is dropped. The
+enforcement above is therefore a guarantee on POSIX and a best effort on Windows.
+
 The file is removed on shutdown and also from SIGTERM/SIGINT handlers, so a kill during startup does
 not leave a stale file behind.
 

src/amplifier_agent_cli/admin/serve_lifecycle.py

@@ -41,6 +41,24 @@
 _STATE_FILE_MODE = 0o600
 _STATE_DIR_MODE = 0o700
 
+# Windows differs from POSIX in three ways that matter to this module:
+#
+#   - os.kill(pid, 0) is not a liveness probe. CPython's posixmodule maps
+#     CTRL_C_EVENT and CTRL_BREAK_EVENT to GenerateConsoleCtrlEvent and every
+#     other signal to TerminateProcess. Because wincon.h defines CTRL_C_EVENT
+#     as 0, signal 0 takes the *first* branch: it delivers a real Ctrl+C to the
+#     target's console process group rather than testing for existence. So the
+#     "benign probe" is not benign -- it can kill the server, and can hit other
+#     processes sharing that console group. See _is_pid_alive_windows.
+#   - signal.SIGKILL does not exist (AttributeError on access). See _hard_kill.
+#   - Detaching a child needs DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP;
+#     start_new_session (setsid) is POSIX-only. See restart_command.
+#
+# File permissions are deliberately NOT keyed off this flag -- whether chmod
+# can enforce a mode is a property of the filesystem, not the OS name. See
+# _enforce_mode.
+_IS_WINDOWS = os.name == "nt"
+
 
 def _state_dir() -> Path:
     """Return the directory that holds ``serve.json``.
@@ -61,6 +79,42 @@ def _state_file() -> Path:
 # ---------------------------------------------------------------------------
 
 
+def _enforce_mode(path: Path, mode: int, *, cannot_set: str, not_applied: str) -> None:
+    """Apply ``mode`` to ``path`` and verify it stuck, else raise ``PermissionError``.
+
+    The state file holds a plaintext ``api_key``, so on POSIX we refuse to
+    write it unless the mode is genuinely enforced: chmod, then stat back and
+    compare. Some networked and virtual filesystems silently ignore chmod, and
+    a mode that did not stick is worse than useless -- it looks like
+    protection that is not there.
+
+    Windows is exempt from the *verification*, not from the intent. NTFS has
+    no POSIX mode bits: chmod cannot produce 0o700/0o600, stat reports
+    0o777/0o666, so the comparison could never pass and ``serve`` could never
+    persist its state file on any Windows machine. There the protection
+    boundary is the ACL on the user profile directory
+    (``%USERPROFILE%\\.amplifier-agent``), which by default denies other
+    standard users. We still call chmod -- it sets the owner-write bit and is
+    harmless -- we just do not assert a POSIX guarantee the platform does not
+    make.
+
+    ``cannot_set`` and ``not_applied`` are the caller's message tails so each
+    call site can name what it was protecting.
+    """
+    try:
+        os.chmod(path, mode)
+    except NotImplementedError as exc:
+        if _IS_WINDOWS:
+            return
+        raise PermissionError(cannot_set) from exc
+    if _IS_WINDOWS:
+        return
+    # Verify the mode was actually applied (some networked/virtual FSes ignore chmod).
+    actual = stat.S_IMODE(os.stat(path).st_mode)
+    if actual != mode:
+        raise PermissionError(not_applied.format(actual=oct(actual)))
+
+
 def _ensure_state_dir() -> Path:
     """Create the state directory with mode 0700, return its path.
 
@@ -69,23 +123,21 @@ def _ensure_state_dir() -> Path:
     """
     d = _state_dir()
     d.mkdir(parents=True, exist_ok=True)
-    try:
-        d.chmod(_STATE_DIR_MODE)
-    except NotImplementedError as exc:
-        raise PermissionError(
+    _enforce_mode(
+        d,
+        _STATE_DIR_MODE,
+        cannot_set=(
             f"Cannot set directory permissions on {d}. "
             "Your filesystem may not support Unix mode bits. "
             "The state file (which contains a sensitive api_key) cannot be "
             "written safely without mode 0700 on the parent directory."
-        ) from exc
-    # Verify the mode was actually applied (some networked/virtual FSes ignore chmod).
-    actual = stat.S_IMODE(d.stat().st_mode)
-    if actual != _STATE_DIR_MODE:
-        raise PermissionError(
-            f"Failed to set mode 0700 on {d} (got {oct(actual)}). "
+        ),
+        not_applied=(
+            f"Failed to set mode 0700 on {d} (got {{actual}}). "
             "The state file contains a sensitive api_key and cannot be written "
             "safely without enforced directory permissions."
-        )
+        ),
+    )
     return d
 
 
@@ -111,21 +163,21 @@ def write_state_file(payload: dict[str, Any]) -> None:
     # Write into a tempfile in the same directory so os.replace is atomic.
     fd, tmp_path = tempfile.mkstemp(dir=d, prefix=".serve-", suffix=".json.tmp")
     try:
-        try:
-            os.chmod(tmp_path, _STATE_FILE_MODE)
-        except NotImplementedError as exc:
-            raise PermissionError(
+        # Restrict the mode *before* the sensitive payload is written, so the
+        # api_key is never on disk at a more-permissive mode.
+        _enforce_mode(
+            Path(tmp_path),
+            _STATE_FILE_MODE,
+            cannot_set=(
                 f"Cannot set mode 0600 on {tmp_path}. "
                 "Your filesystem may not support Unix mode bits. "
                 "Refusing to write api_key in plaintext without permission enforcement."
-            ) from exc
-        # Verify enforcement before writing the sensitive payload.
-        actual = stat.S_IMODE(os.stat(tmp_path).st_mode)
-        if actual != _STATE_FILE_MODE:
-            raise PermissionError(
-                f"Failed to set mode 0600 on {tmp_path} (got {oct(actual)}). "
+            ),
+            not_applied=(
+                f"Failed to set mode 0600 on {tmp_path} (got {{actual}}). "
                 "Refusing to write api_key in plaintext without enforced file permissions."
-            )
+            ),
+        )
         os.write(fd, encoded)
     finally:
         os.close(fd)
@@ -171,13 +223,80 @@ def remove_state_file() -> None:
 # ---------------------------------------------------------------------------
 
 
+def _is_pid_alive_windows(pid: int) -> bool:
+    """Windows liveness check that does NOT signal or terminate the process.
+
+    ``os.kill(pid, 0)`` cannot be used here. CPython maps CTRL_C_EVENT and
+    CTRL_BREAK_EVENT to ``GenerateConsoleCtrlEvent`` and every other signal to
+    ``TerminateProcess``; since ``CTRL_C_EVENT == 0``, signal 0 takes the first
+    branch and delivers a real Ctrl+C to the target's console process group.
+    Either branch can kill what we only meant to observe.
+
+    So we open a handle and poll it instead: ``WaitForSingleObject(handle, 0)``
+    returns WAIT_TIMEOUT while the process is alive and WAIT_OBJECT_0 once it
+    has exited. ACCESS_DENIED from OpenProcess means the process exists but is
+    owned by another user, mirroring the POSIX ``PermissionError`` -> alive case.
+    """
+    import ctypes
+    from ctypes import wintypes
+
+    SYNCHRONIZE = 0x00100000
+    WAIT_TIMEOUT = 0x00000102
+    ERROR_ACCESS_DENIED = 5
+
+    # use_last_error routes GetLastError through ctypes' own thread-local copy,
+    # captured immediately after the call. Calling kernel32.GetLastError()
+    # directly would read the error state *after* ctypes' own bookkeeping and
+    # can report a stale or cleared value.
+    kernel32 = ctypes.WinDLL("kernel32", use_last_error=True)  # type: ignore[attr-defined]
+
+    # Declare signatures explicitly. Without restype, ctypes assumes c_int and
+    # truncates the 64-bit HANDLE, so CloseHandle would be handed a bogus value
+    # (leaking the real handle) and a valid handle could read as falsy.
+    kernel32.OpenProcess.argtypes = (wintypes.DWORD, wintypes.BOOL, wintypes.DWORD)
+    kernel32.OpenProcess.restype = wintypes.HANDLE
+    kernel32.WaitForSingleObject.argtypes = (wintypes.HANDLE, wintypes.DWORD)
+    kernel32.WaitForSingleObject.restype = wintypes.DWORD
+    kernel32.CloseHandle.argtypes = (wintypes.HANDLE,)
+    kernel32.CloseHandle.restype = wintypes.BOOL
+
+    handle = kernel32.OpenProcess(SYNCHRONIZE, False, pid)
+    if not handle:
+        return ctypes.get_last_error() == ERROR_ACCESS_DENIED  # type: ignore[attr-defined]
+    try:
+        return kernel32.WaitForSingleObject(handle, 0) == WAIT_TIMEOUT
+    finally:
+        kernel32.CloseHandle(handle)
+
+
+def _hard_kill(pid: int) -> None:
+    """Forcefully terminate ``pid`` (the SIGKILL equivalent).
+
+    Windows has no ``signal.SIGKILL`` -- accessing the attribute raises
+    ``AttributeError``, so it must be probed with ``hasattr`` rather than
+    caught. SIGTERM is the right fallback there: it is neither CTRL_C_EVENT
+    nor CTRL_BREAK_EVENT, so CPython routes it to ``TerminateProcess``, which
+    is the unconditional kill this function promises. On POSIX the two signals
+    keep their usual meanings and SIGKILL is used directly.
+    """
+    sig = signal.SIGKILL if hasattr(signal, "SIGKILL") else signal.SIGTERM
+    os.kill(pid, sig)
+
+
 def is_pid_alive(pid: int) -> bool:
     """Return True if process ``pid`` exists and is signalable.
 
-    Uses ``os.kill(pid, 0)`` (signal 0 checks existence without delivering
-    a signal). ``PermissionError`` means the process exists but we don't
-    own it — still alive. ``ProcessLookupError`` means it is gone.
+    On POSIX, uses ``os.kill(pid, 0)`` (signal 0 checks existence without
+    delivering a signal). ``PermissionError`` means the process exists but we
+    don't own it — still alive. ``ProcessLookupError`` means it is gone.
+
+    On Windows, delegates to :func:`_is_pid_alive_windows`: signal 0 is not a
+    no-op probe there (it is CTRL_C_EVENT, which CPython delivers as a real
+    console Ctrl+C), so the POSIX idiom would signal the process it is meant
+    to be silently observing.
     """
+    if _IS_WINDOWS:
+        return _is_pid_alive_windows(pid)
     try:
         os.kill(pid, 0)
         return True
@@ -301,7 +420,7 @@ def stop_command(force: bool, timeout_s: float) -> None:
         raise SystemExit(0)
 
     if force:
-        os.kill(pid, signal.SIGKILL)
+        _hard_kill(pid)
         wait_for_exit(pid, timeout=2.0)
         remove_state_file()
         click.echo(f"amplifier-agent serve: stopped (SIGKILL, PID {pid})")
@@ -317,7 +436,7 @@ def stop_command(force: bool, timeout_s: float) -> None:
         raise SystemExit(0)
 
     # Graceful window expired — escalate.
-    os.kill(pid, signal.SIGKILL)
+    _hard_kill(pid)
     wait_for_exit(pid, timeout=2.0)
     remove_state_file()
     click.echo(
@@ -376,7 +495,7 @@ def restart_command() -> None:
     os.kill(old_pid, signal.SIGTERM) if is_pid_alive(old_pid) else None
     if not wait_for_exit(old_pid, timeout=5.0):
         if is_pid_alive(old_pid):
-            os.kill(old_pid, signal.SIGKILL)
+            _hard_kill(old_pid)
             wait_for_exit(old_pid, timeout=2.0)
     remove_state_file()
 
@@ -397,14 +516,23 @@ def restart_command() -> None:
     if host_config_path:
         cmd.extend(["--config", host_config_path])
 
-    # Launch detached — stdout/stderr go to /dev/null; the server writes its
-    # own logs via uvicorn's log machinery.
+    # Launch detached — stdout/stderr go to the null device; the server writes
+    # its own logs via uvicorn's log machinery. Detach differently per platform:
+    # start_new_session (setsid) is a POSIX no-op on Windows, where real
+    # detachment requires DETACHED_PROCESS + a new process group.
     devnull = open(os.devnull, "wb")
+    detach_kwargs: dict[str, Any] = {}
+    if _IS_WINDOWS:
+        detach_kwargs["creationflags"] = (
+            subprocess.DETACHED_PROCESS | subprocess.CREATE_NEW_PROCESS_GROUP  # type: ignore[attr-defined]
+        )
+    else:
+        detach_kwargs["start_new_session"] = True
     subprocess.Popen(
         cmd,
-        start_new_session=True,
         stdout=devnull,
         stderr=devnull,
+        **detach_kwargs,
     )
 
     # Wait for the new state file to appear, indicating a successful lifespan.