fix(core): make config, credential, installer and workspace-init writes crash-safe (#954) (#1084)

* fix(core): make config, credential, installer and workspace-init writes crash-safe (#954)

Five in-place writes that truncate their target before writing a byte, so a
crash or a full disk mid-save destroys the previous contents.

New `codeframe/core/atomic_io.py` — headless, stdlib-only — does the durable
replace properly: unique temp file in the same directory, fsync the file,
os.replace, then fsync the directory (the rename itself is not durable
without that). `ui/routers/_helpers.atomic_write_json` re-exports it so the
two existing router callers are unchanged.

Routed through it: `save_environment_config`, `ConfigManager.save`,
`record_installation`, `clear_installation_history`, and
`CredentialStore._save_encrypted_store` — which had its own temp/replace copy
that fsynced nothing (`mode=0o600` applies the permission before the rename,
so the ciphertext is never briefly world-readable at its final name).

The worst bug here was on the READ side, not the write side.
`_load_encrypted_store` returned `{}` on a failed decrypt, which reads as "no
credentials stored". The write paths do load -> mutate -> save, so the very
next `store()` re-encrypted that empty dict plus one new entry over the top of
the real file — permanently destroying every other provider key, in exactly
the CODEFRAME_CREDENTIAL_SECRET / machine-id-change scenario CLAUDE.md
documents. It now raises `CredentialStoreUnreadableError`; read-only callers
(`retrieve`, `list_providers`) degrade through an explicit
`_load_encrypted_store_for_read`, write callers let it propagate so the
ciphertext survives and stays recoverable.

`record_installation` also crashed a *successful* install when
environment.json held anything but the expected dict-of-dicts (a list or a
string raised TypeError on the index). Any unexpected shape is now treated as
"no usable history".

Workspace init is transactional: the DB is built at a temp path, the workspace
row committed, the WAL checkpointed, and only then os.replace'd into
state.db. Before, a crash between `_init_database` and the INSERT left a
schema-complete but rowless DB, and every later call hit the
`db_path.exists()` fast path and raised "contains no workspace record" —
forever, with no way out but deleting .codeframe by hand.

Closes #954

* fix(workspace): keep state.db's umask permissions when building via mkstemp (#954)

mkstemp forces 0600 and os.replace preserves it, so the transactional-init
change silently tightened state.db from the umask-derived mode sqlite used to
create it (0644 on a stock 0022 umask). That is an unrequested behavior change
that could break a shared-group deployment. Reproduce a normal file creation
instead — permissions are not this change's business.

* fix(core): correct permission handling and CLI error surfacing (#954 review)

Four defects raised by the GLM and claude reviewers on PR #1084.

1. state.db was group-WRITABLE under a 002/007 umask. My previous commit
   reproduced permissions with `0o666 & ~umask`, but sqlite creates with an
   0644 base, not open()'s 0666. Verified: umask 002 -> sqlite 0644, my
   formula 0664. Fixed by not doing permission math at all — drop mkstemp and
   let sqlite create the temp DB itself (a uuid4 name gives the per-writer
   uniqueness mkstemp was providing). My test for this was tautological, using
   the same formula on both sides, so it passed while being wrong; it now
   compares against a reference database sqlite creates, across three umasks.

2. `os.umask(0)` + restore per write toggles a process global, so in a
   threaded server another thread could create a world-writable file in that
   window. The umask is now read once at import into DEFAULT_FILE_MODE.

3. atomic_write_text/atomic_write_json silently tightened config.yaml,
   config.json and environment.json to 0600, because mkstemp forces that mode
   and os.replace carries it onto the target. Only the credential store passed
   an explicit mode. They now default to DEFAULT_FILE_MODE — what
   `open(path, "w")` produced before this change.

4. `cf auth setup` / `cf auth rotate` printed a raw traceback when the store
   was unreadable, since CredentialStoreUnreadableError now propagates and
   cli/app.py has no top-level handler. That is precisely the user this
   exception's recovery text was written for. Both now catch and print it, the
   way `cf auth remove` already did.

Every fix has a regression test, and both permission tests are mutation-checked
(reinstating either bug fails 6 tests).

Not fixed, flagged as a follow-up by the reviewer: record_installation still
does an unlocked read-modify-write, so two concurrent installs can lose an
entry. Pre-existing and outside this issue's scope.

* fix(atomic_io): preserve an existing file's permissions on save (#954 review)

Third instance of the same bug class on this branch. os.replace points the
target *name* at the temp inode, carrying the temp's mode with it, whereas
open(path, "w") truncated the existing inode and left its mode alone. So a
fixed DEFAULT_FILE_MODE silently undid an operator's
`chmod 600 .codeframe/config.yaml` on the next save — through the web UI's
engine toggle, for instance.

When no explicit mode is given, the target's current permissions are now
preserved and only a not-yet-existing file gets the umask default. An explicit
mode (the credential store's 0600) still wins.

Raised by the GLM reviewer with the exact failure scenario. Mutation-checked:
reverting to the fixed default fails 4 tests.

* fix(installer): give get_installation_history the same shape guard as the writer (#954 review)

record_installation was hardened against a malformed environment.json;
get_installation_history one function below was not. `data.get("installations")`
raises AttributeError for any valid-JSON-but-not-an-object file (`["a"]`,
`"str"`, `42`, `null`), and `except (json.JSONDecodeError, IOError)` does not
catch AttributeError — so it propagated raw.

The PR description claimed this method already degraded to {} for a malformed
file. That was only true of a parse failure, not the wrong-shape case this
change had just added explicit write-side coverage for. The claim is corrected
in the PR body.

Also pins UTF-8 on the read to match the writer (#1029's convention).

Caught by the claude-review bot, which verified the claim instead of taking it.
Mutation-checked: removing the guard fails 5 tests.

Author: frankbria

codeframe/cli/auth_commands.py

@@ -55,6 +55,7 @@
     CredentialManager,
     CredentialProvider,
     CredentialSource,
+    CredentialStoreUnreadableError,
 )
 from codeframe.core.api_key_service import ApiKeyService
 from codeframe.platform_store.database import Database
@@ -668,8 +669,16 @@ def setup_credential(
         console.print("Please check the value and try again.")
         raise typer.Exit(1)
 
-    # Store credential
-    manager.set_credential(provider_enum, value)
+    # Store credential. An unreadable store now raises rather than silently
+    # overwriting itself (#954), and that lands on precisely the users this
+    # command exists for — someone re-running `cf auth setup` after adding
+    # CODEFRAME_CREDENTIAL_SECRET or moving machines. Show the exception's
+    # recovery text instead of a raw traceback (raised by the claude reviewer).
+    try:
+        manager.set_credential(provider_enum, value)
+    except CredentialStoreUnreadableError as e:
+        console.print(f"[red]Error:[/red] {e}")
+        raise typer.Exit(1)
     console.print(f"[green]Successfully stored credential for {provider_enum.display_name}[/green]")
 
 
@@ -845,8 +854,12 @@ def rotate_credential(
             console.print("Use --force to skip validation.")
             raise typer.Exit(1)
 
-    # Rotate credential
-    manager.rotate_credential(provider_enum, value)
+    # Rotate credential (same unreadable-store handling as `setup` above).
+    try:
+        manager.rotate_credential(provider_enum, value)
+    except CredentialStoreUnreadableError as e:
+        console.print(f"[red]Error:[/red] {e}")
+        raise typer.Exit(1)
     console.print(f"[green]Successfully rotated credential for {provider_enum.display_name}[/green]")
 
 

codeframe/core/atomic_io.py

@@ -0,0 +1,150 @@
+"""Atomic, crash-safe file writes.
+
+Headless by construction — stdlib only, no FastAPI/UI imports, so every layer
+(core, CLI, routers) can share one implementation.
+
+Why this exists: `open(path, "w")` truncates the target *first*. A crash, a
+full disk or a killed process between that truncation and the last byte leaves
+the file empty or half-written, and the previous contents are gone. For
+`config.yaml`, `environment.json` and the encrypted credential store, that is
+silent data loss on the user's machine (#954).
+
+The sequence here is the standard durable-replace dance:
+
+1. write into a temp file **in the same directory** (``os.replace`` is only
+   atomic within a filesystem),
+2. ``fsync`` it so the bytes are on disk before anything points at them,
+3. ``os.replace`` onto the target — atomic, so a reader sees either the whole
+   old file or the whole new one, never a mixture,
+4. best-effort ``fsync`` of the directory so the rename itself survives a crash.
+
+The temp name is unique per call: a shared ``.tmp`` suffix let two concurrent
+writers collide, with the loser's cleanup deleting the winner's in-flight file
+(#920).
+"""
+
+import json
+import os
+import tempfile
+from pathlib import Path
+from typing import Any, Union
+
+__all__ = [
+    "atomic_write_bytes",
+    "atomic_write_text",
+    "atomic_write_json",
+    "fsync_directory",
+    "DEFAULT_FILE_MODE",
+]
+
+# ``tempfile.mkstemp`` forces 0600 and ``os.replace`` carries that onto the
+# target, so writing through this module would silently tighten config.yaml /
+# environment.json from the umask-derived mode ``open(path, "w")`` gave them.
+# Default to what a plain create would produce instead; callers that want
+# something stricter (the credential store) pass ``mode`` explicitly.
+#
+# Read once at import, not per call: ``os.umask`` is a read-modify-write of a
+# process-global, so doing it on every write opens a window where another
+# thread creates a world-writable file (raised by the GLM reviewer).
+_UMASK = os.umask(0)
+os.umask(_UMASK)
+DEFAULT_FILE_MODE = 0o666 & ~_UMASK
+
+
+def fsync_directory(path: Union[str, Path]) -> None:
+    """Best-effort ``fsync`` of a directory so a rename inside it is durable.
+
+    ``os.replace`` is atomic but not automatically durable: on POSIX
+    filesystems a power loss right after the rename can lose the new directory
+    entry even though the file's own contents were synced. Anything that
+    renames a finished file into place — including the workspace ``state.db``
+    swap, which does not go through ``atomic_write_bytes`` — needs this.
+
+    Silently does nothing where directories cannot be opened (Windows).
+    """
+    try:
+        dir_fd = os.open(path, os.O_RDONLY)
+    except OSError:  # pragma: no cover - platform dependent
+        return
+    try:
+        os.fsync(dir_fd)
+    except OSError:  # pragma: no cover - platform dependent
+        pass
+    finally:
+        os.close(dir_fd)
+
+
+def atomic_write_bytes(
+    path: Union[str, Path], data: bytes, mode: int | None = None
+) -> None:
+    """Durably replace ``path`` with ``data``.
+
+    Args:
+        path: Target file. Parent directories are created if missing.
+        data: Bytes to write.
+        mode: Permission bits applied to the file before it is moved into
+            place, so it never briefly carries the wrong mode at the target
+            name (the credential store passes 0600). When omitted, the target's
+            existing permissions are preserved, and a file that does not exist
+            yet gets ``DEFAULT_FILE_MODE`` — matching ``open(path, "w")`` in
+            both cases, rather than mkstemp's 0600.
+
+    Raises:
+        OSError: If the write, fsync or rename fails. The existing file at
+            ``path`` is left untouched in that case.
+    """
+    path = Path(path)
+    path.parent.mkdir(parents=True, exist_ok=True)
+
+    fd, tmp_name = tempfile.mkstemp(
+        prefix=f".{path.name}.", suffix=".tmp", dir=path.parent
+    )
+    try:
+        with os.fdopen(fd, "wb") as f:
+            f.write(data)
+            f.flush()
+            os.fsync(f.fileno())
+        if mode is not None:
+            file_mode = mode
+        else:
+            # os.replace points the target name at the temp *inode*, carrying
+            # its mode along, whereas `open(path, "w")` truncated the existing
+            # inode and left its mode alone. So a fixed default would silently
+            # undo an operator's `chmod 600 .codeframe/config.yaml` on the next
+            # save. Preserve what is already there; only a file that does not
+            # exist yet gets the umask default.
+            try:
+                file_mode = path.stat().st_mode & 0o777
+            except OSError:
+                file_mode = DEFAULT_FILE_MODE
+        os.chmod(tmp_name, file_mode)
+        os.replace(tmp_name, path)
+    except BaseException:
+        # Never leak the temp file — a failed save must not litter the
+        # workspace with .config.yaml.*.tmp droppings.
+        try:
+            os.unlink(tmp_name)
+        except OSError:
+            pass
+        raise
+
+    # Persist the directory entry too, or the rename itself can be lost on a
+    # hard crash even though the file contents were synced.
+    fsync_directory(path.parent)
+
+
+def atomic_write_text(
+    path: Union[str, Path], text: str, encoding: str = "utf-8", mode: int | None = None
+) -> None:
+    """Durably replace ``path`` with ``text``.
+
+    The encoding is explicit and defaults to UTF-8 rather than the locale
+    default, which would otherwise write cp1252 on stock Windows and then be
+    rejected by our own UTF-8 readers (#931/#1029).
+    """
+    atomic_write_bytes(path, text.encode(encoding), mode=mode)
+
+
+def atomic_write_json(path: Union[str, Path], payload: Any, mode: int | None = None) -> None:
+    """Durably replace ``path`` with ``payload`` serialized as indented JSON."""
+    atomic_write_text(path, json.dumps(payload, indent=2), mode=mode)

codeframe/core/config.py

@@ -27,6 +27,8 @@
 from pydantic import BaseModel, Field, field_validator
 from pydantic_settings import BaseSettings, SettingsConfigDict
 
+from codeframe.core.atomic_io import atomic_write_json, atomic_write_text
+
 logger = logging.getLogger(__name__)
 
 
@@ -579,14 +581,16 @@ def save_environment_config(workspace_path: Path, config: EnvironmentConfig) ->
     # Must match the reader's encoding (#931). allow_unicode=True emits non-ASCII
     # verbatim, so with the locale default here a value like "café" would be
     # written as cp1252 on stock Windows and then rejected by our own UTF-8 read.
-    with open(config_file, "w", encoding="utf-8") as f:
-        yaml.dump(
-            config.to_dict(),
-            f,
-            default_flow_style=False,
-            sort_keys=False,
-            allow_unicode=True,
-        )
+    rendered = yaml.dump(
+        config.to_dict(),
+        default_flow_style=False,
+        sort_keys=False,
+        allow_unicode=True,
+    )
+    # Atomic: `open(..., "w")` truncated config.yaml before writing a byte, so a
+    # crash or a full disk mid-save left the workspace with an empty or
+    # half-written config and no way back (#954).
+    atomic_write_text(config_file, rendered)
 
 
 def get_default_environment_config() -> EnvironmentConfig:
@@ -854,8 +858,8 @@ def load(self) -> ProjectConfig:
     def save(self, config: ProjectConfig) -> None:
         """Save project configuration."""
         self.config_dir.mkdir(parents=True, exist_ok=True)
-        with open(self.config_file, "w") as f:
-            json.dump(config.model_dump(), f, indent=2)
+        # Atomic (#954) — see save_environment_config.
+        atomic_write_json(self.config_file, config.model_dump())
         self._project_config = config
 
     def get_global(self) -> GlobalConfig: