fix(credentials): complete key-store durability follow-ups

src/cli.zig

@@ -251,7 +251,7 @@ pub const usage_text =
     \\  graff login codex [--refresh]    ChatGPT/Codex OAuth login (PKCE)
     \\  graff login kimi                 Kimi Code OAuth login (device-code)
     \\  graff login xai                  Grok/SuperGrok OAuth login (device-code)
-    \\  graff key set <provider> <key>   store a key (macOS Keychain, else 0600 file)
+    \\  graff key set <provider> <key>   store a key (Keychain; POSIX 0600 file; Windows home ACL)
     \\  graff key list                   show which providers have keys
     \\  graff models [refresh]           list the live catalog; refresh Codex + models.dev metadata
     \\  graff route <model>…             dry-run which provider/billing a model lands on (no API call)

src/credential_store.zig

@@ -7,11 +7,10 @@
 //! silent logout with nothing left to recover from. Write a per-writer-unique
 //! temp file in the target's own directory, fsync it, then rename over the
 //! target, so a reader sees either the whole old file or the whole new one.
-//!
-//! The guarantee is against a crashed process, not against power loss: the temp
-//! file is fsynced but the containing directory is not, so some filesystems can
-//! still lose the rename across a hard power cut. Same shape — and same limit —
-//! as learn_store.writeAtomicReplace / eval_memory.writeNotes.
+//! On POSIX, fsync the containing directory after the rename so that directory
+//! entry is also durable across power loss. Zig 0.17 cannot portably flush
+//! Windows directory handles, so Windows keeps the atomic replacement and
+//! synced file contents but has a weaker power-loss guarantee.
 
 const std = @import("std");
 const builtin = @import("builtin");
@@ -83,6 +82,19 @@ fn writeDestination(io: Io, dir: Io.Dir, sub_path: []const u8, allocator: Alloca
     }
 }
 
+fn syncDirectory(io: Io, dir: Io.Dir) !void {
+    if (builtin.os.tag == .windows) return;
+    // Linux openDir handles may use O_PATH, which fsync rejects. Reopen the
+    // exact createFileAtomic destination directory read-only as a File.
+    const file = try dir.openFile(io, ".", .{
+        .allow_directory = true,
+        .follow_symlinks = false,
+        .resolve_beneath = true,
+    });
+    defer file.close(io);
+    try file.sync(io);
+}
+
 /// Replace `dir`/`sub_path` with `bytes` atomically. `sub_path` may carry
 /// directory components; createFileAtomic keeps the temp file in the target's
 /// own directory, so the rename never crosses a filesystem. If `sub_path` is a
@@ -116,6 +128,7 @@ pub fn replaceFile(io: Io, dir: Io.Dir, sub_path: []const u8, bytes: []const u8,
     try atomic.file.writeStreamingAll(io, bytes);
     try atomic.file.sync(io);
     try atomic.replace(io);
+    try syncDirectory(io, atomic.dir);
 }
 
 /// #477: the process $HOME, pinned once at startup (startup.zig, beside
@@ -203,6 +216,47 @@ test "replaceFile: renames a whole new file into place, never truncating the tar
     try std.testing.expectEqual(@as(usize, 1), try entryCount(io, tmp.dir));
 }
 
+test "replaceFile: nested target directories keep the replacement atomic" {
+    if (builtin.os.tag == .windows) return;
+    const io = std.testing.io;
+    var tmp = std.testing.tmpDir(.{ .iterate = true });
+    defer tmp.cleanup();
+    try tmp.dir.createDir(io, "nested", private_dir);
+    try tmp.dir.createDir(io, "nested/credentials", private_dir);
+    try tmp.dir.writeFile(io, .{ .sub_path = "nested/credentials/key.json", .data = "old" });
+
+    const original = try tmp.dir.openFile(io, "nested/credentials/key.json", .{});
+    defer original.close(io);
+    try replaceFile(io, tmp.dir, "nested/credentials/key.json", "new", private_file);
+
+    var read_buffer: [16]u8 = undefined;
+    var reader = original.reader(io, &read_buffer);
+    const before = try reader.interface.allocRemaining(std.testing.allocator, .limited(64));
+    defer std.testing.allocator.free(before);
+    try std.testing.expectEqualStrings("old", before);
+    const after = try tmp.dir.readFileAlloc(io, "nested/credentials/key.json", std.testing.allocator, .limited(64));
+    defer std.testing.allocator.free(after);
+    try std.testing.expectEqualStrings("new", after);
+
+    // createFileAtomic must consume its sibling temporary file in the actual
+    // containing directory, not leave it beside the nested target.
+    var parent = try tmp.dir.openDir(io, "nested/credentials", .{ .iterate = true });
+    defer parent.close(io);
+    try std.testing.expectEqual(@as(usize, 1), try entryCount(io, parent));
+}
+
+test "syncDirectory: an opened nested destination directory is syncable" {
+    if (builtin.os.tag == .windows) return;
+    const io = std.testing.io;
+    var tmp = std.testing.tmpDir(.{});
+    defer tmp.cleanup();
+    try tmp.dir.createDir(io, "nested", private_dir);
+    try tmp.dir.createDir(io, "nested/credentials", private_dir);
+    const parent = try tmp.dir.openDir(io, "nested/credentials", .{});
+    defer parent.close(io);
+    try syncDirectory(io, parent);
+}
+
 // The #477 g_home fallback test lives in startup_tests.zig (with the other
 // credential-scope regressions): exactly ONE test in the suite may mutate
 // g_home, because the parallel test runner makes two mutators a coin flip.
@@ -357,6 +411,18 @@ test "#405: replaceFile rejects a symlink cycle without replacing either link" {
     try std.testing.expectEqual(@as(usize, 2), try entryCount(io, tmp.dir));
 }
 
+test "replaceFile: private mode stays 0600 through repeated replacements" {
+    if (!Io.File.Permissions.has_executable_bit) return;
+    const io = std.testing.io;
+    var tmp = std.testing.tmpDir(.{ .iterate = true });
+    defer tmp.cleanup();
+
+    try replaceFile(io, tmp.dir, "secret.json", "first", private_file);
+    try std.testing.expectEqual(@as(std.posix.mode_t, 0o600), (try tmp.dir.statFile(io, "secret.json", .{})).permissions.toMode() & 0o777);
+    try replaceFile(io, tmp.dir, "secret.json", "second", private_file);
+    try std.testing.expectEqual(@as(std.posix.mode_t, 0o600), (try tmp.dir.statFile(io, "secret.json", .{})).permissions.toMode() & 0o777);
+}
+
 test "writeOAuth: the credential file is 0600 inside 0700 directories" {
     if (builtin.os.tag == .windows) return;
     const io = std.testing.io;

src/keys_cli.zig

@@ -3,8 +3,10 @@
 //!
 //! Safe API-key store: on macOS, keys live in the login Keychain (service
 //! "simple-harness", account=provider id) via the `security` CLI — never on
-//! disk in plaintext. Elsewhere they fall back to a 0600 file
-//! (~/.simple-harness-keys.json). `harness key set <provider> <key>` writes;
+//! disk in plaintext. Other platforms use ~/.simple-harness-keys.json: mode
+//! 0600 on POSIX, while Windows inherits the home directory's ACL because Zig
+//! 0.17 exposes no portable API for installing an owner-only DACL.
+//! `harness key set <provider> <key>` writes;
 //! startup reads the selected provider first (env always wins), then fills the
 //! remaining providers when a picker, switch, resume, or fallback needs them.
 //!
@@ -109,7 +111,8 @@ pub fn storeKey(io: Io, gpa: Allocator, arena: Allocator, home: []const u8, prov
         const term = child.wait(io) catch return false;
         return term == .exited and term.exited == 0;
     }
-    // Linux/other: merge into a 0600 JSON file.
+    // Non-macOS: merge into one JSON file. POSIX uses 0600; Windows inherits
+    // the home directory ACL because Io.File.Permissions has no ACL semantics.
     _ = gpa;
     const path = std.fmt.allocPrint(arena, "{s}/{s}", .{ home, keys_file }) catch return false;
     var obj: std.json.ObjectMap = .empty;
@@ -122,10 +125,10 @@ pub fn storeKey(io: Io, gpa: Allocator, arena: Allocator, home: []const u8, prov
     var aw: Io.Writer.Allocating = .init(arena);
     var s: std.json.Stringify = .{ .writer = &aw.writer };
     s.write(Value{ .object = obj }) catch return false;
-    // Atomic + 0600: this is a read-modify-write of EVERY provider's key, so a
-    // truncate-in-place that dies mid-write does not lose one key, it loses all
-    // of them at once. 0600 also makes the "0600 JSON file" above true: the old
-    // createFile took the umask default, i.e. world-readable API keys.
+    // Atomic + private on POSIX: this is a read-modify-write of EVERY
+    // provider's key, so a truncate-in-place that dies mid-write does not lose
+    // one key, it loses all of them at once. The old POSIX createFile took the
+    // umask default, which could leave API keys world-readable.
     credential_store.replaceFile(io, Io.Dir.cwd(), path, aw.writer.buffered(), credential_store.private_file) catch return false;
     return true;
 }
@@ -154,6 +157,64 @@ pub fn loadStoredKey(io: Io, arena: Allocator, home: []const u8, provider: []con
     return null;
 }
 
+test "file-backed key store merges and loads keys on its live platforms" {
+    if (builtin.os.tag == .macos) return;
+    const io = std.testing.io;
+    var tmp = std.testing.tmpDir(.{});
+    defer tmp.cleanup();
+    var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator);
+    defer arena_state.deinit();
+    const arena = arena_state.allocator();
+    var home_buf: [std.fs.max_path_bytes]u8 = undefined;
+    const home = home_buf[0..try tmp.dir.realPath(io, &home_buf)];
+
+    try std.testing.expect(storeKey(io, std.testing.allocator, arena, home, "openai", "sk-first"));
+    try std.testing.expect(storeKey(io, std.testing.allocator, arena, home, "anthropic", "sk-second"));
+    try std.testing.expectEqualStrings("sk-first", loadStoredKey(io, arena, home, "openai").?);
+    try std.testing.expectEqualStrings("sk-second", loadStoredKey(io, arena, home, "anthropic").?);
+
+    if (Io.File.Permissions.has_executable_bit) {
+        const path = try std.fmt.allocPrint(arena, "{s}/{s}", .{ home, keys_file });
+        const mode = (try Io.Dir.cwd().statFile(io, path, .{})).permissions.toMode() & 0o777;
+        try std.testing.expectEqual(@as(std.posix.mode_t, 0o600), mode);
+    }
+}
+
+test "file-backed key store preserves keys across repeated read-modify-writes" {
+    if (builtin.os.tag == .macos) return;
+    const io = std.testing.io;
+    var tmp = std.testing.tmpDir(.{ .iterate = true });
+    defer tmp.cleanup();
+    var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator);
+    defer arena_state.deinit();
+    const arena = arena_state.allocator();
+    var home_buf: [std.fs.max_path_bytes]u8 = undefined;
+    const home = home_buf[0..try tmp.dir.realPath(io, &home_buf)];
+
+    try std.testing.expect(storeKey(io, std.testing.allocator, arena, home, "openai", "sk-openai-initial"));
+    try std.testing.expect(storeKey(io, std.testing.allocator, arena, home, "anthropic", "sk-anthropic-initial"));
+    try std.testing.expect(storeKey(io, std.testing.allocator, arena, home, "deepseek", "sk-deepseek-initial"));
+
+    // Each update rereads the file produced by the preceding update. Checking
+    // after every pass catches a lost unrelated provider at the exact write
+    // that drops it, rather than only observing the final state.
+    for (0..8) |round| {
+        const openai_key = try std.fmt.allocPrint(arena, "sk-openai-{d}", .{round});
+        const anthropic_key = try std.fmt.allocPrint(arena, "sk-anthropic-{d}", .{round});
+        try std.testing.expect(storeKey(io, std.testing.allocator, arena, home, "openai", openai_key));
+        try std.testing.expect(storeKey(io, std.testing.allocator, arena, home, "anthropic", anthropic_key));
+        try std.testing.expectEqualStrings(openai_key, loadStoredKey(io, arena, home, "openai").?);
+        try std.testing.expectEqualStrings(anthropic_key, loadStoredKey(io, arena, home, "anthropic").?);
+        try std.testing.expectEqualStrings("sk-deepseek-initial", loadStoredKey(io, arena, home, "deepseek").?);
+    }
+
+    if (Io.File.Permissions.has_executable_bit) {
+        const path = try std.fmt.allocPrint(arena, "{s}/{s}", .{ home, keys_file });
+        const mode = (try Io.Dir.cwd().statFile(io, path, .{})).permissions.toMode() & 0o777;
+        try std.testing.expectEqual(@as(std.posix.mode_t, 0o600), mode);
+    }
+}
+
 pub const StoredKeyScope = union(enum) {
     all,
     provider: []const u8,

src/startup_keys.zig

@@ -205,7 +205,7 @@ pub fn resolveKeys(io: Io, gpa: Allocator, arena: Allocator, environ_map: anytyp
         std.process.fatal(
             \\no API key found. quickest fixes:
             \\  graff login                         free codegraff key (device-code OAuth)
-            \\  graff key set <provider> <key>      store a key (macOS Keychain, else 0600 file)
+            \\  graff key set <provider> <key>      store a key (Keychain; POSIX 0600 file; Windows home ACL)
             \\  export ANTHROPIC_API_KEY=sk-ant-…   or CODEGRAFF/DEEPSEEK/OPENAI/MINIMAX/XIAOMI/KIMI/MOONSHOT/XAI/ZAI _API_KEY
             \\a Codex CLI login (~/.codex/auth.json) is also picked up automatically.
         , .{});