From fad8532809464dd645952d867ef63c32dedb7c9c Mon Sep 17 00:00:00 2001 From: Loongphy Date: Sun, 8 Nov 2026 12:25:17 +0800 Subject: [PATCH 1/5] fix: harden proxy, cleanup, daemon and import paths from history review Proxy handling: - Split bare WinHTTP PAC failover lists (`host:port; host2:port`) on ';' and use the first entry instead of passing an invalid proxy to curl. - Map WinINET protocol keys to curl schemes: socks->socks4, socks4, socks5, socks5h; keep http/https. - Translate '*.domain' bypass entries to curl noproxy suffixes and stop leaking the proxy URL when noproxy allocation fails. - Replace the system-proxy probe spin loop with a blocking Io.Mutex. - scutil --proxy: keep only the nine keys the parser consumes so unrelated fields can no longer evict HTTP/HTTPS/SOCKS settings. Reliability: - `codex-auth app` falls back to the cached codext CLI when the release lookup fails instead of hard-failing offline. - `clean` preserves the managed `codext-cli` cache and default `backup` export directory; stale entries are collected before deletion so iterating-while-deleting cannot skip siblings (same fix for removed account backup pruning). - Rewrites to the user's `config.toml` go through a temp-file rename so a crash cannot leave a truncated file. - The daemon executable resolver skips empty PATH segments and reports not-found instead of falling back to a bare name the OS would resolve against the working directory; a missing executable maps to the silent not_running contract rather than an 'unconfirmed' warning. - The app-running probe distinguishes 'not running' from probe failures so a failed check no longer risks launching a second instance. - Live TUI Enter on a non-selectable row keeps the view open. Validation & safety: - `import --alias` now shares the `alias set` rules (format + conflicts) and validates before writing snapshots or mutating the registry. - Display text from API/auth sources is sanitized (control characters and invalid UTF-8 become spaces) before reaching the terminal; email normalization is shared instead of duplicated. - New app error codes are registered in the preflight handled-error table so failures report friendly messages. - PATH executable checks require the execute bit on POSIX so a data file cannot shadow a real tool. Tests cover PAC failover, socks mapping, bypass translation, scutil overflow, clean whitelist, collect-then-delete, import alias rules, and display sanitization. --- src/api/account.zig | 3 +- src/api/http_executable.zig | 8 +- src/api/http_proxy.zig | 53 +++++++++--- src/api/http_proxy_scutil.zig | 26 +++++- src/api/me.zig | 13 +-- src/auth/auth.zig | 8 +- src/cli/live_view.zig | 9 +- src/registry/account_ops.zig | 13 ++- src/registry/clean.zig | 26 ++++-- src/registry/common.zig | 103 ++++++++++++++++++++++- src/registry/export.zig | 2 +- src/registry/import.zig | 31 ++++++- src/registry/import_helpers.zig | 2 + src/registry/parse.zig | 4 +- src/registry/root.zig | 7 ++ src/workflows/alias.zig | 56 +++++-------- src/workflows/app.zig | 114 ++++++++++++++++++++----- src/workflows/codex_daemon.zig | 7 +- src/workflows/live_runtime.zig | 3 + src/workflows/preflight.zig | 8 +- src/workflows/switch.zig | 7 +- tests/api_proxy_test.zig | 61 +++++++++++++- tests/cli_integration_test.zig | 11 ++- tests/registry_test.zig | 143 ++++++++++++++++++++++++++++++++ 24 files changed, 601 insertions(+), 117 deletions(-) diff --git a/src/api/account.zig b/src/api/account.zig index e447b55f..8c3c1d94 100644 --- a/src/api/account.zig +++ b/src/api/account.zig @@ -1,5 +1,6 @@ const std = @import("std"); const chatgpt_http = @import("http.zig"); +const registry = @import("../registry/root.zig"); pub const default_account_endpoint = "https://chatgpt.com/backend-api/accounts"; @@ -104,5 +105,5 @@ fn parseAccountNameAlloc(allocator: std.mem.Allocator, value: ?std.json.Value) ! else => return null, }; if (raw.len == 0) return null; - return try allocator.dupe(u8, raw); + return try registry.sanitizeDisplayAlloc(allocator, raw); } diff --git a/src/api/http_executable.zig b/src/api/http_executable.zig index 1c9b7ebf..7deb5329 100644 --- a/src/api/http_executable.zig +++ b/src/api/http_executable.zig @@ -82,12 +82,16 @@ pub fn resolveExecutablePathEntryForLaunchAlloc( return null; } fn accessPath(path: []const u8) bool { + // A PATH entry that exists but cannot be launched (e.g. a data file named + // `curl`) must not shadow later, usable candidates. Windows has no execute + // permission bit, so existence stays the check there. + const options: std.Io.Dir.AccessOptions = if (builtin.target.os.tag == .windows) .{} else .{ .execute = true }; if (std.Io.Dir.path.isAbsolute(path)) { - std.Io.Dir.accessAbsolute(app_runtime.io(), path, .{}) catch return false; + std.Io.Dir.accessAbsolute(app_runtime.io(), path, options) catch return false; return true; } - std.Io.Dir.cwd().access(app_runtime.io(), path, .{}) catch return false; + std.Io.Dir.cwd().access(app_runtime.io(), path, options) catch return false; return true; } diff --git a/src/api/http_proxy.zig b/src/api/http_proxy.zig index 4e8e0962..94328198 100644 --- a/src/api/http_proxy.zig +++ b/src/api/http_proxy.zig @@ -17,6 +17,7 @@ const builtin = @import("builtin"); const std = @import("std"); +const app_runtime = @import("../core/runtime.zig"); const env = @import("http_env.zig"); const no_system_proxy = struct { @@ -66,9 +67,9 @@ const no_proxy_env_keys = [_][]const u8{ "NO_PROXY", "no_proxy" }; /// A proxy entry selected for one scheme. pub const Picked = struct { hostport: []const u8, - /// `curl` needs an explicit scheme to treat an entry as SOCKS; `socks5h` - /// keeps DNS resolution on the proxy side. - socks: bool, + /// `curl` needs an explicit scheme prefix to treat an entry as SOCKS; an + /// empty prefix means the default HTTP-proxy scheme. + scheme: []const u8 = "", }; /// Resolve the proxy to use for `endpoint` in this process. @@ -108,11 +109,13 @@ pub fn decide( var url: std.ArrayList(u8) = .empty; errdefer url.deinit(allocator); - if (picked.socks) try url.appendSlice(allocator, "socks5h://"); + try url.appendSlice(allocator, picked.scheme); try url.appendSlice(allocator, picked.hostport); + const url_owned = try url.toOwnedSlice(allocator); + errdefer allocator.free(url_owned); return Proxy{ - .url = try url.toOwnedSlice(allocator), + .url = url_owned, .noproxy = try noproxyValue(allocator, env_map, system.bypass), }; } @@ -138,7 +141,17 @@ fn noproxyValue( pub fn pickProxyServer(scheme: Scheme, raw: []const u8) ?Picked { const trimmed = std.mem.trim(u8, raw, " \t"); if (trimmed.len == 0) return null; - if (std.mem.findScalar(u8, trimmed, '=') == null) return .{ .hostport = trimmed, .socks = false }; + if (std.mem.findScalar(u8, trimmed, '=') == null) { + // WinHTTP reports a PAC failover list as a bare `host:port;host:port` + // string; only the first entry can be handed to curl, and WinHTTP has + // already ordered them by preference. + var it = std.mem.splitScalar(u8, trimmed, ';'); + while (it.next()) |entry| { + const item = std.mem.trim(u8, entry, " \t"); + if (item.len != 0) return .{ .hostport = item }; + } + return null; + } const wanted = switch (scheme) { .https => "https", @@ -155,10 +168,10 @@ pub fn pickProxyServer(scheme: Scheme, raw: []const u8) ?Picked { if (value.len == 0) continue; const picked = Picked{ .hostport = value, - .socks = std.ascii.eqlIgnoreCase(key, "socks") or std.ascii.eqlIgnoreCase(key, "socks5"), + .scheme = socksSchemeForKey(key) orelse "", }; if (std.ascii.eqlIgnoreCase(key, wanted)) return picked; - if (picked.socks) { + if (picked.scheme.len != 0) { if (socks_fallback == null) socks_fallback = picked; } else if (fallback == null and (std.ascii.eqlIgnoreCase(key, "https") or std.ascii.eqlIgnoreCase(key, "http"))) @@ -169,6 +182,18 @@ pub fn pickProxyServer(scheme: Scheme, raw: []const u8) ?Picked { return fallback orelse socks_fallback; } +/// WinINET's legacy `socks` entry was SOCKS4, but every modern consumer of the +/// shared format (the macOS `SOCKSProxy` key included) speaks SOCKS5, so it +/// maps to `socks5h`; explicit `socks4`/`socks5`/`socks5h` keys are honoured +/// literally. `socks5h` keeps DNS resolution on the proxy side. +fn socksSchemeForKey(key: []const u8) ?[]const u8 { + if (std.ascii.eqlIgnoreCase(key, "socks4")) return "socks4://"; + if (std.ascii.eqlIgnoreCase(key, "socks") or + std.ascii.eqlIgnoreCase(key, "socks5") or + std.ascii.eqlIgnoreCase(key, "socks5h")) return "socks5h://"; + return null; +} + /// Translate a platform bypass list into curl's comma separated `noproxy` list. /// The WinINET `` shorthand becomes `localhost`, duplicates are dropped, /// and empty input yields `null`. @@ -185,6 +210,9 @@ pub fn translateBypass(allocator: std.mem.Allocator, raw: []const u8) !?[]u8 { if (std.ascii.eqlIgnoreCase(part, "") or std.ascii.eqlIgnoreCase(part, "")) { part = "localhost"; } + // `curl`'s noproxy list matches by domain suffix and does not read the + // WinINET `*.` wildcard prefix, so `*.internal` becomes `internal`. + if (std.mem.startsWith(u8, part, "*.")) part = part[2..]; if (containsEntry(seen.items, part)) continue; if (out.items.len != 0) try out.append(allocator, ','); try out.appendSlice(allocator, part); @@ -215,7 +243,7 @@ fn envValue(env_map: *const std.process.Environ.Map, key: []const u8) ?[]const u return value; } -var system_proxy_mutex: std.atomic.Mutex = .unlocked; +var system_proxy_mutex: std.Io.Mutex = .init; var system_proxy_cached: ?*const SystemProxy = null; /// Platform proxy settings, read once per process. Every endpoint codex-auth @@ -226,10 +254,9 @@ var system_proxy_cached: ?*const SystemProxy = null; pub fn systemProxy(endpoint: []const u8) SystemProxy { if (@atomicLoad(?*const SystemProxy, &system_proxy_cached, .acquire)) |cached| return cached.*; - while (!system_proxy_mutex.tryLock()) { - std.Thread.yield() catch {}; - } - defer system_proxy_mutex.unlock(); + const io = app_runtime.io(); + system_proxy_mutex.lockUncancelable(io); + defer system_proxy_mutex.unlock(io); if (@atomicLoad(?*const SystemProxy, &system_proxy_cached, .acquire)) |cached| return cached.*; // The cached struct and the server/bypass slices inside it live for the diff --git a/src/api/http_proxy_scutil.zig b/src/api/http_proxy_scutil.zig index 4b580736..f11614ac 100644 --- a/src/api/http_proxy_scutil.zig +++ b/src/api/http_proxy_scutil.zig @@ -13,9 +13,15 @@ pub const Values = struct { }; /// Fields are kept as slices into the input text, which is valid for the whole -/// parse. `scutil` emits far fewer keys than this, so the fixed table avoids an -/// allocator round trip per line. -const max_fields = 12; +/// parse. Only the keys `appendProtocol` reads are stored, so the fixed table +/// stays small and unrelated keys can never crowd the wanted ones out. +const max_fields = 9; + +const wanted_keys = [_][]const u8{ + "HTTPEnable", "HTTPProxy", "HTTPPort", + "HTTPSEnable", "HTTPSProxy", "HTTPSPort", + "SOCKSEnable", "SOCKSProxy", "SOCKSPort", +}; const Fields = struct { keys: [max_fields][]const u8 = @splat(""), @@ -23,6 +29,20 @@ const Fields = struct { count: usize = 0, fn put(self: *Fields, key: []const u8, value: []const u8) void { + for (wanted_keys) |wanted| { + if (std.mem.eql(u8, key, wanted)) break; + } else return; + // A repeated key replaces its earlier value, matching "last wins" plist + // semantics and keeping the table bounded even for hostile input. + if (self.get(key) != null) { + for (self.keys[0..self.count], self.values[0..self.count]) |*found, *stored| { + if (std.mem.eql(u8, found.*, key)) { + stored.* = value; + return; + } + } + unreachable; + } if (self.count == max_fields) return; self.keys[self.count] = key; self.values[self.count] = value; diff --git a/src/api/me.zig b/src/api/me.zig index e0f7f701..74caeb39 100644 --- a/src/api/me.zig +++ b/src/api/me.zig @@ -1,5 +1,6 @@ const std = @import("std"); const http = @import("http.zig"); +const registry = @import("../registry/root.zig"); pub const default_me_endpoint = "https://api.openai.com/v1/me"; @@ -52,9 +53,9 @@ pub fn parseMeResponse(allocator: std.mem.Allocator, body: []const u8) !MeResult const owned_user_id = try allocator.dupe(u8, id); errdefer allocator.free(owned_user_id); - const owned_email = try normalizeEmailAlloc(allocator, email); + const owned_email = try registry.normalizeEmailAlloc(allocator, email); errdefer allocator.free(owned_email); - const owned_name = if (name) |value| try allocator.dupe(u8, value) else null; + const owned_name = if (name) |value| try registry.sanitizeDisplayAlloc(allocator, value) else null; errdefer if (owned_name) |value| allocator.free(value); return .{ @@ -71,11 +72,3 @@ fn nonEmptyStringField(obj: std.json.ObjectMap, key: []const u8) ?[]const u8 { else => null, }; } - -fn normalizeEmailAlloc(allocator: std.mem.Allocator, email: []const u8) ![]u8 { - var buf = try allocator.alloc(u8, email.len); - for (email, 0..) |ch, i| { - buf[i] = std.ascii.toLower(ch); - } - return buf; -} diff --git a/src/auth/auth.zig b/src/auth/auth.zig index b34e7ad0..5a18410e 100644 --- a/src/auth/auth.zig +++ b/src/auth/auth.zig @@ -47,13 +47,7 @@ const CpaAuthJson = struct { last_refresh: ?[]const u8, }; -fn normalizeEmailAlloc(allocator: std.mem.Allocator, email: []const u8) ![]u8 { - var buf = try allocator.alloc(u8, email.len); - for (email, 0..) |ch, i| { - buf[i] = std.ascii.toLower(ch); - } - return buf; -} +const normalizeEmailAlloc = registry.normalizeEmailAlloc; fn recordKeyAlloc( allocator: std.mem.Allocator, diff --git a/src/cli/live_view.zig b/src/cli/live_view.zig index f762937a..4bff43f7 100644 --- a/src/cli/live_view.zig +++ b/src/cli/live_view.zig @@ -249,10 +249,15 @@ pub fn selectAccountWithLiveUpdates( }, .enter => { if (parsedDisplayedIndex(number_buf[0..number_len], total_accounts)) |displayed_idx| { - return try dupSelectedAccountKeyForDisplayedAccount(allocator, rows, borrowed.reg, displayed_idx); + if (try dupSelectedAccountKeyForDisplayedAccount(allocator, rows, borrowed.reg, displayed_idx)) |account_key| return account_key; + // The numbered row is not selectable (for + // example an errored account): keep the + // view open instead of treating it as quit. + continue; } if (selected_idx) |idx| return try dupSelectedAccountKey(allocator, rows, borrowed.reg, idx); - return null; + // Nothing selected to confirm; ignore. + continue; }, .quit => return null, .backspace => { diff --git a/src/registry/account_ops.zig b/src/registry/account_ops.zig index 95496fe1..0d2289fe 100644 --- a/src/registry/account_ops.zig +++ b/src/registry/account_ops.zig @@ -367,6 +367,13 @@ pub fn deleteRemovedAccountBackups( }; defer dir.close(app_runtime.io()); + // Collect matching entries first: deleting while the directory iterator is + // live can skip siblings depending on the filesystem. + var stale_names: std.ArrayList([]u8) = .empty; + defer { + for (stale_names.items) |name| allocator.free(name); + stale_names.deinit(allocator); + } var it = dir.iterate(); while (try it.next(app_runtime.io())) |entry| { if (entry.kind != .file and entry.kind != .sym_link) continue; @@ -381,7 +388,11 @@ pub fn deleteRemovedAccountBackups( const record_key = info.record_key orelse continue; if (!isRemovedAccountKey(reg, removed, record_key)) continue; - dir.deleteFile(app_runtime.io(), entry.name) catch {}; + try stale_names.append(allocator, try allocator.dupe(u8, entry.name)); + } + + for (stale_names.items) |name| { + dir.deleteFile(app_runtime.io(), name) catch {}; } } diff --git a/src/registry/clean.zig b/src/registry/clean.zig index 32e1627e..606e9a55 100644 --- a/src/registry/clean.zig +++ b/src/registry/clean.zig @@ -189,6 +189,8 @@ pub fn isAllowedCurrentSnapshot(reg: *const Registry, entry_name: []const u8) bo pub fn isAllowedAccountsEntry(reg: *const Registry, entry_name: []const u8) bool { if (std.mem.eql(u8, entry_name, "registry.json")) return true; if (std.mem.eql(u8, entry_name, "backups")) return true; + if (std.mem.eql(u8, entry_name, common.default_export_dir_name)) return true; + if (std.mem.eql(u8, entry_name, common.codext_cache_dir_name)) return true; return isAllowedCurrentSnapshot(reg, entry_name); } @@ -225,19 +227,33 @@ pub fn cleanAccountsBackupsWithLoader(allocator: std.mem.Allocator, codex_home: var reg = try load_registry(allocator, codex_home); defer reg.deinit(allocator); - var stale_snapshot_files_removed: usize = 0; + // Collect the stale entries first: deleting while the directory iterator is + // live can skip siblings depending on the filesystem. + var stale_names: std.ArrayList(struct { name: []u8, kind: std.Io.File.Kind }) = .empty; + defer { + for (stale_names.items) |item| allocator.free(item.name); + stale_names.deinit(allocator); + } var accounts_dir = try std.Io.Dir.cwd().openDir(app_runtime.io(), dir, .{ .iterate = true }); defer accounts_dir.close(app_runtime.io()); var it = accounts_dir.iterate(); while (try it.next(app_runtime.io())) |entry| { + switch (entry.kind) { + .file, .sym_link, .directory => {}, + else => continue, + } if (isAllowedAccountsEntry(®, entry.name)) { continue; } + try stale_names.append(allocator, .{ .name = try allocator.dupe(u8, entry.name), .kind = entry.kind }); + } - switch (entry.kind) { - .file, .sym_link => try accounts_dir.deleteFile(app_runtime.io(), entry.name), - .directory => try accounts_dir.deleteTree(app_runtime.io(), entry.name), - else => continue, + var stale_snapshot_files_removed: usize = 0; + for (stale_names.items) |item| { + switch (item.kind) { + .file, .sym_link => try accounts_dir.deleteFile(app_runtime.io(), item.name), + .directory => try accounts_dir.deleteTree(app_runtime.io(), item.name), + else => unreachable, } stale_snapshot_files_removed += 1; } diff --git a/src/registry/common.zig b/src/registry/common.zig index ae19a0fd..b8292172 100644 --- a/src/registry/common.zig +++ b/src/registry/common.zig @@ -24,9 +24,45 @@ pub fn getEnvVarOwned(allocator: std.mem.Allocator, name: []const u8) ![]u8 { return app_runtime.getEnvVarOwned(allocator, name); } +/// Terminal-untrusted text (API fields and auth-file values rendered into +/// table cells) must not carry control characters: C0/C1/DEL codepoints and +/// invalid UTF-8 bytes become a plain space so nothing stored can emit an +/// escape sequence or a line break when it is displayed. +pub fn sanitizeDisplayAlloc(allocator: std.mem.Allocator, text: []const u8) ![]u8 { + var out: std.ArrayList(u8) = .empty; + errdefer out.deinit(allocator); + try out.ensureTotalCapacity(allocator, text.len); + var index: usize = 0; + while (index < text.len) { + const cp_len = std.unicode.utf8ByteSequenceLength(text[index]) catch { + out.appendAssumeCapacity(' '); + index += 1; + continue; + }; + if (index + cp_len > text.len) { + out.appendAssumeCapacity(' '); + break; + } + const cp = std.unicode.utf8Decode(text[index..][0..cp_len]) catch { + out.appendAssumeCapacity(' '); + index += 1; + continue; + }; + if (cp < 0x20 or cp == 0x7f or (cp >= 0x80 and cp <= 0x9f)) { + out.appendAssumeCapacity(' '); + } else { + out.appendSliceAssumeCapacity(text[index .. index + cp_len]); + } + index += cp_len; + } + return try out.toOwnedSlice(allocator); +} + pub fn normalizeEmailAlloc(allocator: std.mem.Allocator, email: []const u8) ![]u8 { - var buf = try allocator.alloc(u8, email.len); - for (email, 0..) |ch, i| { + const sanitized = try sanitizeDisplayAlloc(allocator, email); + defer allocator.free(sanitized); + var buf = try allocator.alloc(u8, sanitized.len); + for (sanitized, 0..) |ch, i| { buf[i] = std.ascii.toLower(ch); } return buf; @@ -565,3 +601,66 @@ pub fn writeFile(path: []const u8, data: []const u8) !void { } pub const max_backups: usize = 5; + +/// Directories under `accounts/` managed by features other than the account +/// registry; `clean` must preserve them alongside `backups` and the live +/// snapshots. +pub const default_export_dir_name = "backup"; +pub const codext_cache_dir_name = "codext-cli"; + +/// Why an alias value is unusable; shared by `alias set` and `import --alias` +/// so both reject the same selector-conflicting or unsafe values. +pub const AliasIssue = enum { + empty, + digit_only, + flag_like, + colon_prefix, + control_char, + duplicate_alias, + account_key_collision, +}; + +pub const AliasConflict = struct { + issue: AliasIssue, + index: usize, +}; + +fn aliasIsDigitOnly(value: []const u8) bool { + if (value.len == 0) return false; + for (value) |ch| { + if (ch < '0' or ch > '9') return false; + } + return true; +} + +/// Format rules shared by `alias set` and `import --alias`: non-empty, never a +/// number (digits select displayed rows), never flag- or colon-prefixed +/// (selectors and completion protocols reserve those), and free of control +/// characters. +pub fn aliasFormatIssue(alias_value: []const u8) ?AliasIssue { + if (alias_value.len == 0) return .empty; + if (aliasIsDigitOnly(alias_value)) return .digit_only; + if (std.mem.startsWith(u8, alias_value, "-")) return .flag_like; + if (std.mem.startsWith(u8, alias_value, ":")) return .colon_prefix; + for (alias_value) |ch| { + if (ch < 0x20 or ch == 0x7f) return .control_char; + } + return null; +} + +/// `alias_value` conflicts when another account already carries it +/// (case-insensitive) or when it equals an account key, because keys resolve +/// before aliases. `exclude_idx` skips the slot being updated — a record may +/// keep the alias it already has. +pub fn findAliasConflict(reg: *const Registry, alias_value: []const u8, exclude_idx: ?usize) ?AliasConflict { + for (reg.accounts.items, 0..) |rec, idx| { + if (exclude_idx != null and idx == exclude_idx.?) continue; + if (rec.alias.len != 0 and std.ascii.eqlIgnoreCase(rec.alias, alias_value)) { + return .{ .issue = .duplicate_alias, .index = idx }; + } + if (std.ascii.eqlIgnoreCase(rec.account_key, alias_value)) { + return .{ .issue = .account_key_collision, .index = idx }; + } + } + return null; +} diff --git a/src/registry/export.zig b/src/registry/export.zig index ba9bef4f..3ae54a9f 100644 --- a/src/registry/export.zig +++ b/src/registry/export.zig @@ -23,7 +23,7 @@ pub const ExportSummary = struct { }; pub fn defaultExportDirectory(allocator: std.mem.Allocator, codex_home: []const u8) ![]u8 { - return try std.Io.Dir.path.join(allocator, &[_][]const u8{ codex_home, "accounts", "backup" }); + return try std.Io.Dir.path.join(allocator, &[_][]const u8{ codex_home, "accounts", common.default_export_dir_name }); } pub fn exportAccounts( diff --git a/src/registry/import.zig b/src/registry/import.zig index 6ae18dda..a7359cb5 100644 --- a/src/registry/import.zig +++ b/src/registry/import.zig @@ -311,6 +311,21 @@ fn importCpaFile( return try importConvertedAuthInfo(allocator, codex_home, reg, explicit_alias, &info, converted); } +/// `import --alias` applies the same rules as `alias set`, checked before the +/// auth snapshot is written or the registry record is touched. The record +/// being replaced keeps its slot exempt, so re-importing with the same alias +/// is not a false duplicate. +fn checkImportAlias(reg: *const Registry, explicit_alias: ?[]const u8, replace_idx: ?usize) !void { + const alias_value = explicit_alias orelse return; + if (common.aliasFormatIssue(alias_value) != null) return error.InvalidAlias; + if (common.findAliasConflict(reg, alias_value, replace_idx)) |conflict| { + return switch (conflict.issue) { + .duplicate_alias => error.DuplicateAlias, + else => error.InvalidAlias, + }; + } +} + fn importConvertedAuthInfo( allocator: std.mem.Allocator, codex_home: []const u8, @@ -327,7 +342,9 @@ fn importConvertedAuthInfo( const record_key = info.record_key orelse return error.MissingChatgptUserId; const alias = explicit_alias orelse ""; - const existed = findAccountIndexByAccountKey(reg, record_key) != null; + const replace_idx = findAccountIndexByAccountKey(reg, record_key); + const existed = replace_idx != null; + try checkImportAlias(reg, explicit_alias, replace_idx); const dest = try accountAuthPath(allocator, codex_home, record_key); defer allocator.free(dest); @@ -380,7 +397,9 @@ fn importAuthInfo( const record_key = info.record_key orelse return error.MissingChatgptUserId; const alias = explicit_alias orelse ""; - const existed = findAccountIndexByAccountKey(reg, record_key) != null; + const replace_idx = findAccountIndexByAccountKey(reg, record_key); + const existed = replace_idx != null; + try checkImportAlias(reg, explicit_alias, replace_idx); const dest = try accountAuthPath(allocator, codex_home, record_key); defer allocator.free(dest); @@ -408,7 +427,9 @@ fn importApiKeyAuthData( const record_key = try account_ops.apiKeyAccountKeyAlloc(allocator, me.user_id, api_key); defer allocator.free(record_key); const alias = explicit_alias orelse ""; - const existed = findAccountIndexByAccountKey(reg, record_key) != null; + const replace_idx = findAccountIndexByAccountKey(reg, record_key); + const existed = replace_idx != null; + try checkImportAlias(reg, explicit_alias, replace_idx); const dest = try accountAuthPath(allocator, codex_home, record_key); defer allocator.free(dest); @@ -436,7 +457,9 @@ fn importApiKeyAuthFile( const record_key = try account_ops.apiKeyAccountKeyAlloc(allocator, me.user_id, api_key); defer allocator.free(record_key); const alias = explicit_alias orelse ""; - const existed = findAccountIndexByAccountKey(reg, record_key) != null; + const replace_idx = findAccountIndexByAccountKey(reg, record_key); + const existed = replace_idx != null; + try checkImportAlias(reg, explicit_alias, replace_idx); const dest = try accountAuthPath(allocator, codex_home, record_key); defer allocator.free(dest); diff --git a/src/registry/import_helpers.zig b/src/registry/import_helpers.zig index 36f11116..93fc3b61 100644 --- a/src/registry/import_helpers.zig +++ b/src/registry/import_helpers.zig @@ -39,6 +39,8 @@ pub fn isImportValidationError(err: anyerror) bool { error.AccountIdMismatch, error.InvalidJwt, error.InvalidBase64, + error.InvalidAlias, + error.DuplicateAlias, => true, else => false, }; diff --git a/src/registry/parse.zig b/src/registry/parse.zig index c92ac929..ccb89b62 100644 --- a/src/registry/parse.zig +++ b/src/registry/parse.zig @@ -121,7 +121,9 @@ fn parseResetCreditCard(allocator: std.mem.Allocator, obj: std.json.ObjectMap) ! errdefer allocator.free(status); const expires_at = optionalStringAlloc(allocator, obj.get("expires_at")); errdefer if (expires_at) |value| allocator.free(value); - const title = optionalStringAlloc(allocator, obj.get("title")); + const title_raw = optionalStringAlloc(allocator, obj.get("title")); + defer if (title_raw) |value| allocator.free(value); + const title = if (title_raw) |raw| try common.sanitizeDisplayAlloc(allocator, raw) else null; errdefer if (title) |value| allocator.free(value); return .{ .id = id, diff --git a/src/registry/root.zig b/src/registry/root.zig index 13c90e84..7b6ff8c6 100644 --- a/src/registry/root.zig +++ b/src/registry/root.zig @@ -18,6 +18,7 @@ pub const private_file_permissions = common.private_file_permissions; pub const getEnvMap = common.getEnvMap; pub const getEnvVarOwned = common.getEnvVarOwned; pub const normalizeEmailAlloc = common.normalizeEmailAlloc; +pub const sanitizeDisplayAlloc = common.sanitizeDisplayAlloc; pub const readFileAlloc = common.readFileAlloc; pub const RateLimitWindow = common.RateLimitWindow; pub const CreditsSnapshot = common.CreditsSnapshot; @@ -66,6 +67,12 @@ pub const copyManagedFile = common.copyManagedFile; pub const replaceFilePreservingPermissions = common.replaceFilePreservingPermissions; pub const writeFile = common.writeFile; pub const max_backups = common.max_backups; +pub const default_export_dir_name = common.default_export_dir_name; +pub const codext_cache_dir_name = common.codext_cache_dir_name; +pub const AliasIssue = common.AliasIssue; +pub const AliasConflict = common.AliasConflict; +pub const aliasFormatIssue = common.aliasFormatIssue; +pub const findAliasConflict = common.findAliasConflict; pub const CleanSummary = clean.CleanSummary; pub const backupAuthIfChanged = clean.backupAuthIfChanged; diff --git a/src/workflows/alias.zig b/src/workflows/alias.zig index b5f01715..ed6467bf 100644 --- a/src/workflows/alias.zig +++ b/src/workflows/alias.zig @@ -68,46 +68,28 @@ fn replaceAlias(allocator: std.mem.Allocator, rec: *registry.AccountRecord, alia rec.alias = owned_alias; } -fn isDigitOnly(value: []const u8) bool { - if (value.len == 0) return false; - for (value) |ch| { - if (ch < '0' or ch > '9') return false; - } - return true; -} - fn validateAlias(reg: *registry.Registry, alias_value: []const u8, selected_idx: usize) !void { - if (alias_value.len == 0) { - try cli.output.printInvalidAliasError("alias cannot be empty; use `codex-auth alias clear ` to remove one."); - return error.InvalidAlias; - } - if (isDigitOnly(alias_value)) { - try cli.output.printInvalidAliasError("alias cannot be only digits because numbers select displayed rows."); - return error.InvalidAlias; - } - if (std.mem.startsWith(u8, alias_value, "-")) { - try cli.output.printInvalidAliasError("alias cannot start with '-' because flag-like words are never read as selectors."); - return error.InvalidAlias; - } - if (std.mem.startsWith(u8, alias_value, ":")) { - try cli.output.printInvalidAliasError("alias cannot start with ':' because completion protocols reserve leading colons."); - return error.InvalidAlias; - } - for (alias_value) |ch| { - if (ch < 0x20 or ch == 0x7f) { - try cli.output.printInvalidAliasError("alias cannot contain control characters."); - return error.InvalidAlias; + if (registry.aliasFormatIssue(alias_value)) |issue| { + switch (issue) { + .empty => try cli.output.printInvalidAliasError("alias cannot be empty; use `codex-auth alias clear ` to remove one."), + .digit_only => try cli.output.printInvalidAliasError("alias cannot be only digits because numbers select displayed rows."), + .flag_like => try cli.output.printInvalidAliasError("alias cannot start with '-' because flag-like words are never read as selectors."), + .colon_prefix => try cli.output.printInvalidAliasError("alias cannot start with ':' because completion protocols reserve leading colons."), + .control_char => try cli.output.printInvalidAliasError("alias cannot contain control characters."), + else => unreachable, } + return error.InvalidAlias; } - for (reg.accounts.items, 0..) |rec, idx| { - if (idx == selected_idx) continue; - if (rec.alias.len != 0 and std.ascii.eqlIgnoreCase(rec.alias, alias_value)) { - try cli.output.printDuplicateAliasError(alias_value, rec.email); - return error.DuplicateAlias; - } - if (std.ascii.eqlIgnoreCase(rec.account_key, alias_value)) { - try cli.output.printInvalidAliasError("alias cannot equal another account's key because keys resolve before aliases."); - return error.InvalidAlias; + if (registry.findAliasConflict(reg, alias_value, selected_idx)) |conflict| { + switch (conflict.issue) { + .duplicate_alias => { + try cli.output.printDuplicateAliasError(alias_value, reg.accounts.items[conflict.index].email); + return error.DuplicateAlias; + }, + else => { + try cli.output.printInvalidAliasError("alias cannot equal another account's key because keys resolve before aliases."); + return error.InvalidAlias; + }, } } } diff --git a/src/workflows/app.zig b/src/workflows/app.zig index 2949bfe7..6e686ad2 100644 --- a/src/workflows/app.zig +++ b/src/workflows/app.zig @@ -18,7 +18,7 @@ const desktop_section_name = "desktop"; const wsl_desktop_setting_key = "runCodexInWindowsSubsystemForLinux"; const codext_repo_latest_url = "https://api.github.com/repos/Loongphy/codext/releases/latest"; const codext_repo_url = "https://github.com/Loongphy/codext"; -const codext_cache_dir_name = "codext-cli"; +const codext_cache_dir_name = registry.codext_cache_dir_name; const windows_app_id_resolver_script = "function Resolve-CodexAppPackage { param([string]$AppId) " ++ "if ([string]::IsNullOrWhiteSpace($AppId)) { throw 'Codex App ID is empty' }; " ++ @@ -82,7 +82,14 @@ pub fn handleApp(allocator: std.mem.Allocator, resolved_codex_home: []const u8, defer effective_app_id.deinit(allocator); try requireAppId(effective_app_id); try validateConfiguredOptions(allocator, effective_platform.value, effective_app_id, opts); - if (try isCodexAppRunning(allocator, effective_platform.value, effective_app_id)) { + const app_running = try isCodexAppRunning(allocator, effective_platform.value, effective_app_id); + if (app_running == null) { + // A failed probe is not evidence the app is absent; launching is still + // attempted because a packaged second launch usually just activates the + // existing instance, but the user should know the check was skipped. + try writeAppStep("Could not confirm whether Codex App is running; launching anyway."); + } + if (app_running == true) { try writeAppAlreadyRunning(); return; } @@ -362,18 +369,20 @@ fn platformLabel(platform: types.AppPlatform) []const u8 { }; } -fn isCodexAppRunning(allocator: std.mem.Allocator, platform: ?types.AppPlatform, app_id: ResolvedValue) !bool { - const value = app_id.value orelse return false; +/// `null` means the running-state probe itself failed, which is different from +/// a confirmed "not running" and must not silently take the launch path. +fn isCodexAppRunning(allocator: std.mem.Allocator, platform: ?types.AppPlatform, app_id: ResolvedValue) !?bool { + const value = app_id.value orelse return null; return switch (builtin.target.os.tag) { .windows => try isCodexAppRunningOnWindows(allocator, value), .macos => try isCodexAppRunningOnMac(allocator, value), - else => switch (platform orelse return false) { + else => switch (platform orelse return null) { .win, .wsl, .mac => false, }, }; } -fn isCodexAppRunningOnWindows(allocator: std.mem.Allocator, app_id: []const u8) !bool { +fn isCodexAppRunningOnWindows(allocator: std.mem.Allocator, app_id: []const u8) !?bool { const script = try windowsAppIdRunningScriptAlloc(allocator, app_id); defer allocator.free(script); @@ -382,13 +391,13 @@ fn isCodexAppRunningOnWindows(allocator: std.mem.Allocator, app_id: []const u8) &[_][]const u8{ "pwsh.exe", "-NoProfile", "-Command", script }, 3000, null, - ) catch return false; + ) catch return null; defer result.deinit(allocator); - if (result.timed_out) return false; + if (result.timed_out) return null; return std.mem.trim(u8, result.stdout, " \t\r\n").len != 0; } -fn isCodexAppRunningOnMac(allocator: std.mem.Allocator, app_id: []const u8) !bool { +fn isCodexAppRunningOnMac(allocator: std.mem.Allocator, app_id: []const u8) !?bool { var result = http_child.runChildCapture( allocator, &[_][]const u8{ @@ -403,9 +412,9 @@ fn isCodexAppRunningOnMac(allocator: std.mem.Allocator, app_id: []const u8) !boo }, 3000, null, - ) catch return false; + ) catch return null; defer result.deinit(allocator); - if (result.timed_out or !childExitedSuccessfully(result.term)) return false; + if (result.timed_out or !childExitedSuccessfully(result.term)) return null; return std.mem.eql(u8, std.mem.trim(u8, result.stdout, " \t\r\n"), "true"); } @@ -479,9 +488,47 @@ fn writeAppPlatformSetting(allocator: std.mem.Allocator, home: []const u8, value const updated = try updateDesktopWslSettingTomlAlloc(allocator, data, use_wsl); defer allocator.free(updated); - var file = try std.Io.Dir.cwd().createFile(app_runtime.io(), config_path, .{ .truncate = true }); - defer file.close(app_runtime.io()); - try file.writeStreamingAll(app_runtime.io(), updated); + try writeConfigFileAtomic(config_path, updated); +} + +/// config.toml is user-edited state shared with Codex itself; a crash mid-write +/// must leave either the old or the new file, never a truncated one. Mirrors +/// the registry replace dance so it works on Windows, where rename does not +/// overwrite an existing destination. +fn writeConfigFileAtomic(path: []const u8, data: []const u8) !void { + const allocator = app_runtime.scratchAllocator(); + const temp_path = try allocator.print("{s}.tmp.{d}", .{ path, @as(i128, std.Io.Timestamp.now(app_runtime.io(), .real).toNanoseconds()) }); + defer allocator.free(temp_path); + const backup_path = try allocator.print("{s}.bak.{d}", .{ path, @as(i128, std.Io.Timestamp.now(app_runtime.io(), .real).toNanoseconds()) }); + defer allocator.free(backup_path); + + { + var file = try std.Io.Dir.cwd().createFile(app_runtime.io(), temp_path, .{ .truncate = true }); + defer file.close(app_runtime.io()); + try file.writeStreamingAll(app_runtime.io(), data); + try file.sync(app_runtime.io()); + } + + const had_original = blk: { + std.Io.Dir.cwd().rename(path, std.Io.Dir.cwd(), backup_path, app_runtime.io()) catch |err| switch (err) { + error.FileNotFound => break :blk false, + else => return err, + }; + break :blk true; + }; + errdefer { + std.Io.Dir.cwd().deleteFile(app_runtime.io(), temp_path) catch {}; + if (had_original) { + std.Io.Dir.cwd().rename(backup_path, std.Io.Dir.cwd(), path, app_runtime.io()) catch {}; + } + } + try std.Io.Dir.cwd().rename(temp_path, std.Io.Dir.cwd(), path, app_runtime.io()); + if (had_original) { + std.Io.Dir.cwd().deleteFile(app_runtime.io(), backup_path) catch |err| switch (err) { + error.FileNotFound => {}, + else => return err, + }; + } } const TomlLocation = struct { @@ -828,19 +875,34 @@ fn cachedCodextCliPath(allocator: std.mem.Allocator, home: []const u8, platform: fn downloadDefaultCodextCli(allocator: std.mem.Allocator, home: []const u8, platform: types.AppPlatform, quiet: bool) !CodextInstallResult { if (!quiet) try writeAppStep("Checking latest " ++ codext_repo_url ++ " release..."); - const release = try fetchLatestCodextRelease(allocator); + const release = fetchLatestCodextRelease(allocator) catch |err| { + // The freshness check is the only step that needs the network, so an + // already-installed codext keeps working offline. + if (try cachedCodextCliPath(allocator, home, platform)) |cached| { + if (!quiet) try writeAppStep("Release check failed; using the cached Codext CLI."); + return .{ .path = cached, .source = .cached }; + } + if (err == error.InvalidCodextReleaseResponse) { + try writeAppError("Codext release lookup returned an unexpected response.\n"); + } + return err; + }; defer release.deinit(allocator); const cache_root = try std.Io.Dir.path.join(allocator, &.{ home, "accounts", codext_cache_dir_name }); defer allocator.free(cache_root); try std.Io.Dir.cwd().createDirPath(app_runtime.io(), cache_root); - const asset = release.assetFor(platform) orelse return error.CodextReleaseAssetNotFound; + const asset = release.assetFor(platform) orelse { + try writeAppError("No Codext release asset matches this platform.\n"); + return error.CodextReleaseAssetNotFound; + }; const target_downloaded = try ensureCodextAssetInstalled(allocator, cache_root, release.tag, platform, asset, quiet); const installed = try managedCodextExecutablePath(allocator, home, platform); if (!fileExists(installed)) { allocator.free(installed); + try writeAppError("Codext CLI install did not produce the expected executable.\n"); return error.CodextReleaseInstallFailed; } return .{ @@ -1042,7 +1104,15 @@ fn downloadAndInstallCodextAsset( 120000, ); defer download.deinit(allocator); - try checkedChildResult(download); + checkedChildResult(download) catch |err| { + try writeAppStep("Codext download failed."); + const detail = std.mem.trim(u8, download.stderr, " \t\r\n"); + if (detail.len != 0) { + try writeAppError(detail); + try writeAppError("\n"); + } + return err; + }; if (platform == .win) { const archive_quoted = try psSingleQuoteAlloc(allocator, archive_path); defer allocator.free(archive_quoted); @@ -1050,9 +1120,15 @@ fn downloadAndInstallCodextAsset( defer allocator.free(dest_quoted); const script = try allocator.print("Expand-Archive -LiteralPath {s} -DestinationPath {s} -Force", .{ archive_quoted, dest_quoted }); defer allocator.free(script); - try runChecked(allocator, &[_][]const u8{ "pwsh.exe", "-NoProfile", "-Command", script }, 120000); + runChecked(allocator, &[_][]const u8{ "pwsh.exe", "-NoProfile", "-Command", script }, 120000) catch |err| { + try writeAppError("Codext archive extraction failed.\n"); + return err; + }; } else { - try runChecked(allocator, &[_][]const u8{ tarExecutable(), "-xzf", archive_path, "-C", extract_dir }, 120000); + runChecked(allocator, &[_][]const u8{ tarExecutable(), "-xzf", archive_path, "-C", extract_dir }, 120000) catch |err| { + try writeAppError("Codext archive extraction failed.\n"); + return err; + }; } try installManagedCodextExecutable(allocator, cache_root, extract_dir, platform); try writeManagedCodextVersion(allocator, cache_root, tag, platform, asset); diff --git a/src/workflows/codex_daemon.zig b/src/workflows/codex_daemon.zig index 5a78b5ff..c705eae0 100644 --- a/src/workflows/codex_daemon.zig +++ b/src/workflows/codex_daemon.zig @@ -243,6 +243,9 @@ fn resolveExecutable(allocator: std.mem.Allocator, home: []const u8) ![]u8 { defer env.deinit(); var paths = std.mem.splitScalar(u8, env.get("PATH") orelse "", if (builtin.target.os.tag == .windows) ';' else ':'); while (paths.next()) |dir| { + // Empty PATH segments resolve relative to the current directory, which + // must never be searched for a lifecycle helper. + if (dir.len == 0) continue; const path = try std.Io.Dir.path.join(allocator, &.{ dir, name }); if (isExecutable(path)) { defer allocator.free(path); @@ -250,7 +253,9 @@ fn resolveExecutable(allocator: std.mem.Allocator, home: []const u8) ![]u8 { } allocator.free(path); } - return try allocator.dupe(u8, name); + // Spawning a bare name would let the OS search the current directory, so + // an unresolvable daemon executable stays an init failure instead. + return error.ExecutableRequired; } fn isExecutable(path: []const u8) bool { diff --git a/src/workflows/live_runtime.zig b/src/workflows/live_runtime.zig index 588a7ca5..38b4e102 100644 --- a/src/workflows/live_runtime.zig +++ b/src/workflows/live_runtime.zig @@ -566,6 +566,9 @@ fn runDaemonRound(worker: *DaemonWorker) void { const text: ?[]const u8 = blk: { var client = daemon.Client.init(allocator, worker.codex_home) catch |err| { if (err == error.OutOfMemory) break :blk "out of memory"; + // No executable means there is nothing to coordinate, matching the + // silent absence of a control socket. + if (err == error.ExecutableRequired) break :blk null; break :blk "daemon state unconfirmed"; }; defer client.deinit(); diff --git a/src/workflows/preflight.zig b/src/workflows/preflight.zig index 0053a65c..4587bf70 100644 --- a/src/workflows/preflight.zig +++ b/src/workflows/preflight.zig @@ -50,7 +50,13 @@ pub fn isHandledCliError(err: anyerror) bool { err == error.WindowsAppLaunchRequiresWindows or err == error.WindowsAppPlatformRequiresWindows or err == error.MacAppPlatformRequiresMacOS or - err == error.WindowsPassthroughArgsUnsupported; + err == error.WindowsPassthroughArgsUnsupported or + err == error.UnsupportedPlatform or + err == error.ChildProcessFailed or + err == error.ChildProcessTimedOut or + err == error.InvalidCodextReleaseResponse or + err == error.CodextReleaseAssetNotFound or + err == error.CodextReleaseInstallFailed; } pub fn ensureLiveTty(target: LiveTtyTarget) !void { diff --git a/src/workflows/switch.zig b/src/workflows/switch.zig index ee77499d..f6fd9082 100644 --- a/src/workflows/switch.zig +++ b/src/workflows/switch.zig @@ -254,9 +254,12 @@ fn activateSelection( if (!opts.json) try cli.output.printSwitchedAccount(allocator, reg, account_key); var client = daemon.Client.init(allocator, codex_home) catch |err| { if (err == error.OutOfMemory) return err; - cli.output.printCodexDaemonDiagnostic(codex_home, "codex", .unknown) catch {}; + // No daemon executable is absence, not an unconfirmed state: stay on + // the silent not_running contract a missing control socket produces. + const outcome: daemon.Outcome = if (err == error.ExecutableRequired) .not_running else .unknown; + cli.output.printCodexDaemonDiagnostic(codex_home, "codex", outcome) catch {}; if (!opts.json) return; - var result = results.buildSwitchResult(allocator, reg, account_key, .unknown) catch |build_err| return printJsonWorkflowError(build_err); + var result = results.buildSwitchResult(allocator, reg, account_key, outcome) catch |build_err| return printJsonWorkflowError(build_err); defer result.deinit(allocator); try cli.json_output.printSwitchResult(&result); return; diff --git a/tests/api_proxy_test.zig b/tests/api_proxy_test.zig index 259ba511..1ac94229 100644 --- a/tests/api_proxy_test.zig +++ b/tests/api_proxy_test.zig @@ -73,12 +73,20 @@ test "scheme of endpoint follows the url prefix" { test "bare proxy server applies to every scheme" { const picked_https = proxy.pickProxyServer(.https, "127.0.0.1:7890").?; try std.testing.expectEqualStrings("127.0.0.1:7890", picked_https.hostport); - try std.testing.expect(!picked_https.socks); + try std.testing.expectEqualStrings("", picked_https.scheme); const picked_http = proxy.pickProxyServer(.http, " 127.0.0.1:7890 ").?; try std.testing.expectEqualStrings("127.0.0.1:7890", picked_http.hostport); } +test "bare pac failover list yields the first entry only" { + const picked = proxy.pickProxyServer(.https, "proxy1.example:8080; proxy2.example:8080").?; + try std.testing.expectEqualStrings("proxy1.example:8080", picked.hostport); + try std.testing.expectEqualStrings("", picked.scheme); + + try std.testing.expect(proxy.pickProxyServer(.https, " ; ; ") == null); +} + test "per protocol proxy server picks the matching entry" { const raw = "http=127.0.0.1:7890;https=127.0.0.1:7891;socks=127.0.0.1:7892"; try std.testing.expectEqualStrings("127.0.0.1:7891", proxy.pickProxyServer(.https, raw).?.hostport); @@ -103,13 +111,24 @@ test "opposite http scheme outranks socks as a fallback" { test "socks only system proxy is still usable" { const picked = proxy.pickProxyServer(.https, "socks=127.0.0.1:7892").?; try std.testing.expectEqualStrings("127.0.0.1:7892", picked.hostport); - try std.testing.expect(picked.socks); + try std.testing.expectEqualStrings("socks5h://", picked.scheme); +} + +test "explicit socks protocol keys map to their curl schemes" { + const picked_v4 = proxy.pickProxyServer(.https, "socks4=127.0.0.1:7892").?; + try std.testing.expectEqualStrings("socks4://", picked_v4.scheme); + + const picked_v5 = proxy.pickProxyServer(.https, "socks5=127.0.0.1:7892").?; + try std.testing.expectEqualStrings("socks5h://", picked_v5.scheme); + + const picked_v5h = proxy.pickProxyServer(.https, "socks5h=127.0.0.1:7892").?; + try std.testing.expectEqualStrings("socks5h://", picked_v5h.scheme); } test "exact scheme match is not flagged as socks" { const picked = proxy.pickProxyServer(.https, "https=127.0.0.1:7890;socks=127.0.0.1:7892").?; try std.testing.expectEqualStrings("127.0.0.1:7890", picked.hostport); - try std.testing.expect(!picked.socks); + try std.testing.expectEqualStrings("", picked.scheme); } test "empty proxy server yields nothing" { @@ -122,7 +141,7 @@ test "bypass list translates the wininet local shorthand" { const allocator = std.testing.allocator; const translated = (try proxy.translateBypass(allocator, ";*.internal;127.0.0.1")).?; defer allocator.free(translated); - try std.testing.expectEqualStrings("localhost,*.internal,127.0.0.1", translated); + try std.testing.expectEqualStrings("localhost,internal,127.0.0.1", translated); } test "bypass list drops duplicates and accepts either separator" { @@ -289,6 +308,40 @@ test "resolved proxy is written into the curl config document" { try std.testing.expect(std.mem.find(u8, config.items, "noproxy = \"localhost,*.internal\"") != null); } +test "scutil parser keeps wanted keys past the unrelated field budget" { + const allocator = std.testing.allocator; + const text = + \\ { + \\ ExceptionsList : { + \\ 0 : *.local + \\ } + \\ FTPPassive : 1 + \\ ProxyAutoConfigEnable : 0 + \\ ProxyAutoConfigURLString : + \\ ProxyAutoDiscoveryEnable : 0 + \\ ExcludeSimpleHostnames : 0 + \\ FTPEnable : 0 + \\ FTPProxy : + \\ FTPPort : 0 + \\ GopherEnable : 0 + \\ GopherProxy : + \\ GopherPort : 0 + \\ HTTPEnable : 0 + \\ HTTPSProxy : 10.0.0.2 + \\ HTTPSPort : 8888 + \\ HTTPSEnable : 1 + \\} + \\ + ; + + const values = try scutil.parse(allocator, text); + defer if (values.server) |server| allocator.free(server); + defer if (values.bypass) |bypass| allocator.free(bypass); + + try std.testing.expectEqualStrings("https=10.0.0.2:8888", values.server.?); + try std.testing.expectEqualStrings("*.local", values.bypass.?); +} + test "curl config document omits proxy options when none resolved" { const allocator = std.testing.allocator; diff --git a/tests/cli_integration_test.zig b/tests/cli_integration_test.zig index 5c466742..5a90d0e2 100644 --- a/tests/cli_integration_test.zig +++ b/tests/cli_integration_test.zig @@ -942,7 +942,7 @@ test "Scenario: Given a reload-capable daemon then a plain switch applies creden } } -test "Scenario: Given no managed executable then daemon restart resolves relative and empty PATH entries" { +test "Scenario: Given no managed executable then daemon restart resolves relative PATH entries but never the current directory" { if (builtin.target.os.tag == .windows) return error.SkipZigTest; const gpa = std.testing.allocator; const project_root = try projectRootAlloc(gpa); @@ -973,6 +973,15 @@ test "Scenario: Given no managed executable then daemon restart resolves relativ defer gpa.free(result.stdout); defer gpa.free(result.stderr); try expectSuccess(result); + if (path.len == 0) { + // An empty PATH segment would resolve `codex` relative to the + // working directory; the resolver must refuse that implicit + // search path, so the switch skips daemon coordination silently. + try std.testing.expect(std.mem.indexOf(u8, result.stderr, "daemon") == null); + try std.testing.expectError(error.FileNotFound, fake_daemon.read(dir, "lifecycle-calls")); + try std.testing.expectError(error.FileNotFound, dir.openFile("restarted-home", .{})); + continue; + } try std.testing.expect(std.mem.find(u8, result.stderr, "daemon restart completed") != null); const calls = try fake_daemon.read(dir, "lifecycle-calls"); defer gpa.free(calls); diff --git a/tests/registry_test.zig b/tests/registry_test.zig index cbc50233..f697c628 100644 --- a/tests/registry_test.zig +++ b/tests/registry_test.zig @@ -1440,6 +1440,39 @@ test "clean uses a whitelist and only removes non-current entries under accounts preserved_backup.close(); } +test "clean preserves the default export and codext cache directories" { + const gpa = std.testing.allocator; + var tmp = fs.tmpDir(.{}); + defer tmp.cleanup(); + + const codex_home = try tmp.dir.realpathAlloc(gpa, "."); + defer gpa.free(codex_home); + try tmp.dir.makePath("accounts"); + + var reg = makeEmptyRegistry(); + defer reg.deinit(gpa); + try reg.accounts.append(gpa, try makeAccountRecord(gpa, "keep@example.com", "", .business, .chatgpt, 1)); + try registry.saveRegistry(gpa, codex_home, ®); + + try tmp.dir.makePath("accounts/backup"); + try tmp.dir.writeFile(.{ .sub_path = "accounts/backup/user@example.com.auth.json", .data = "exported" }); + try tmp.dir.makePath("accounts/codext-cli"); + try tmp.dir.writeFile(.{ .sub_path = "accounts/codext-cli/codext", .data = "bin" }); + try tmp.dir.writeFile(.{ .sub_path = "accounts/junk.txt", .data = "junk" }); + try tmp.dir.makePath("accounts/stray-dir"); + try tmp.dir.writeFile(.{ .sub_path = "accounts/stray-dir/old.txt", .data = "junk" }); + + const summary = try registry.cleanAccountsBackups(gpa, codex_home); + try std.testing.expect(summary.stale_snapshot_files_removed == 2); + + var exported = try tmp.dir.openFile("accounts/backup/user@example.com.auth.json", .{}); + exported.close(); + var codext = try tmp.dir.openFile("accounts/codext-cli/codext", .{}); + codext.close(); + try std.testing.expectError(error.FileNotFound, tmp.dir.openFile("accounts/junk.txt", .{})); + try std.testing.expectError(error.FileNotFound, tmp.dir.openDir("accounts/stray-dir", .{})); +} + test "clean preserves account snapshots when registry is missing" { const gpa = std.testing.allocator; var tmp = fs.tmpDir(.{}); @@ -1756,6 +1789,95 @@ test "import auth path with repeated single file reports updated on second impor try std.testing.expect(second.events.items[0].outcome == .updated); } +test "import auth path rejects explicit aliases that conflict with selector rules" { + const gpa = std.testing.allocator; + var tmp = fs.tmpDir(.{}); + defer tmp.cleanup(); + + const codex_home = try tmp.dir.realpathAlloc(gpa, "."); + defer gpa.free(codex_home); + try tmp.dir.makePath("imports"); + + const auth_json = try authJsonWithEmailPlan(gpa, "aliased@example.com", "plus"); + defer gpa.free(auth_json); + try tmp.dir.writeFile(.{ .sub_path = "imports/aliased.json", .data = auth_json }); + + const auth_path = try fs.path.join(gpa, &[_][]const u8{ codex_home, "imports", "aliased.json" }); + defer gpa.free(auth_path); + + const bad_aliases = [_][]const u8{ "", "123", "-flag", ":colon", "bad\talias", "bad\x01alias" }; + for (bad_aliases) |bad_alias| { + var reg = makeEmptyRegistry(); + defer reg.deinit(gpa); + + var report = try registry.importAuthPath(gpa, codex_home, ®, auth_path, bad_alias); + defer report.deinit(gpa); + try std.testing.expectEqual(@as(usize, 0), report.appliedCount()); + try std.testing.expectEqual(@as(usize, 1), report.skipped); + const failure = report.failure orelse return error.TestExpectedEqual; + try std.testing.expect(failure == error.InvalidAlias); + try std.testing.expectEqual(@as(usize, 0), reg.accounts.items.len); + } +} + +test "import auth path rejects an explicit alias already taken by another account" { + const gpa = std.testing.allocator; + var tmp = fs.tmpDir(.{}); + defer tmp.cleanup(); + + const codex_home = try tmp.dir.realpathAlloc(gpa, "."); + defer gpa.free(codex_home); + try tmp.dir.makePath("imports"); + + const auth_json = try authJsonWithEmailPlan(gpa, "incoming@example.com", "plus"); + defer gpa.free(auth_json); + try tmp.dir.writeFile(.{ .sub_path = "imports/incoming.json", .data = auth_json }); + + const auth_path = try fs.path.join(gpa, &[_][]const u8{ codex_home, "imports", "incoming.json" }); + defer gpa.free(auth_path); + + var reg = makeEmptyRegistry(); + defer reg.deinit(gpa); + try reg.accounts.append(gpa, try makeAccountRecord(gpa, "other@example.com", "taken", .plus, .chatgpt, 1)); + + var report = try registry.importAuthPath(gpa, codex_home, ®, auth_path, "taken"); + defer report.deinit(gpa); + try std.testing.expectEqual(@as(usize, 0), report.appliedCount()); + const failure = report.failure orelse return error.TestExpectedEqual; + try std.testing.expect(failure == error.DuplicateAlias); + try std.testing.expectEqual(@as(usize, 1), reg.accounts.items.len); +} + +test "import auth path accepts re-import with the same explicit alias on the same account" { + const gpa = std.testing.allocator; + var tmp = fs.tmpDir(.{}); + defer tmp.cleanup(); + + const codex_home = try tmp.dir.realpathAlloc(gpa, "."); + defer gpa.free(codex_home); + try tmp.dir.makePath("imports"); + + const auth_json = try authJsonWithEmailPlan(gpa, "same@example.com", "plus"); + defer gpa.free(auth_json); + try tmp.dir.writeFile(.{ .sub_path = "imports/same.json", .data = auth_json }); + + const auth_path = try fs.path.join(gpa, &[_][]const u8{ codex_home, "imports", "same.json" }); + defer gpa.free(auth_path); + + var reg = makeEmptyRegistry(); + defer reg.deinit(gpa); + + var first = try registry.importAuthPath(gpa, codex_home, ®, auth_path, "mine"); + defer first.deinit(gpa); + try std.testing.expect(first.imported == 1); + + var second = try registry.importAuthPath(gpa, codex_home, ®, auth_path, "mine"); + defer second.deinit(gpa); + try std.testing.expect(second.updated == 1); + try std.testing.expect(second.skipped == 0); + try std.testing.expectEqualStrings("mine", reg.accounts.items[0].alias); +} + test "import auth path with invalid single file keeps failure for non-zero exit handling" { const gpa = std.testing.allocator; var tmp = fs.tmpDir(.{}); @@ -2014,3 +2136,24 @@ test "export accounts writes cpa token files to default backup directory" { try std.testing.expect(std.mem.find(u8, exported, "\"refresh_token\": \"refresh-export-cpa@example.com\"") != null); try std.testing.expect(std.mem.find(u8, exported, "\"tokens\"") == null); } + +test "sanitizeDisplayAlloc replaces control characters and invalid utf-8" { + const gpa = std.testing.allocator; + + const plain = try registry.sanitizeDisplayAlloc(gpa, "Alice Example"); + defer gpa.free(plain); + try std.testing.expectEqualStrings("Alice Example", plain); + + const controls = try registry.sanitizeDisplayAlloc(gpa, "line\nbreak\x1b[31m red\x7f\x80"); + defer gpa.free(controls); + try std.testing.expect(std.mem.indexOfScalar(u8, controls, '\n') == null); + try std.testing.expect(std.mem.indexOfScalar(u8, controls, 0x1b) == null); + try std.testing.expect(std.mem.indexOfScalar(u8, controls, 0x7f) == null); + try std.testing.expect(std.mem.indexOfScalar(u8, controls, 0x80) == null); + try std.testing.expectEqualStrings("line break [31m red ", controls); + + const truncated = try registry.sanitizeDisplayAlloc(gpa, "emoji \xf0\x9f"); + defer gpa.free(truncated); + try std.testing.expect(std.mem.indexOfScalar(u8, truncated, 0xf0) == null); + try std.testing.expect(std.ascii.isPrint(truncated[truncated.len - 1])); +} From d7cd4f855fc251cd5b20f780678b383dc30cacf8 Mon Sep 17 00:00:00 2001 From: Loongphy Date: Sun, 8 Nov 2026 12:31:19 +0800 Subject: [PATCH 2/5] fix: address greptile findings on atomicity, sanitization, and error paths - config.toml rewrite resolves symlinks to their target and uses a single atomic rename on POSIX so an interrupted write can no longer leave the settings file missing; Windows keeps the backup-swap dance. - Stored account_name and alias values are cleaned on registry load: names get control characters stripped, and a control-containing alias is dropped instead of silently renamed. - installManagedCodextExecutable prints the package-layout error before returning CodextReleaseInstallFailed so the handled error is not mute. - The stale-entry collectors free the duplicated name when list append fails, closing an error-path leak in clean and account backup pruning. - The Windows app-running probe now also requires a successful exit before trusting the PowerShell output, matching the macOS branch. --- src/registry/account_ops.zig | 6 +++++- src/registry/clean.zig | 6 +++++- src/registry/storage_parse.zig | 17 +++++++++++++++-- src/workflows/app.zig | 34 +++++++++++++++++++++++++++++----- 4 files changed, 54 insertions(+), 9 deletions(-) diff --git a/src/registry/account_ops.zig b/src/registry/account_ops.zig index 0d2289fe..b7c28cfc 100644 --- a/src/registry/account_ops.zig +++ b/src/registry/account_ops.zig @@ -388,7 +388,11 @@ pub fn deleteRemovedAccountBackups( const record_key = info.record_key orelse continue; if (!isRemovedAccountKey(reg, removed, record_key)) continue; - try stale_names.append(allocator, try allocator.dupe(u8, entry.name)); + const owned_name = try allocator.dupe(u8, entry.name); + stale_names.append(allocator, owned_name) catch |err| { + allocator.free(owned_name); + return err; + }; } for (stale_names.items) |name| { diff --git a/src/registry/clean.zig b/src/registry/clean.zig index 606e9a55..c0374aca 100644 --- a/src/registry/clean.zig +++ b/src/registry/clean.zig @@ -245,7 +245,11 @@ pub fn cleanAccountsBackupsWithLoader(allocator: std.mem.Allocator, codex_home: if (isAllowedAccountsEntry(®, entry.name)) { continue; } - try stale_names.append(allocator, .{ .name = try allocator.dupe(u8, entry.name), .kind = entry.kind }); + const owned_name = try allocator.dupe(u8, entry.name); + stale_names.append(allocator, .{ .name = owned_name, .kind = entry.kind }) catch |err| { + allocator.free(owned_name); + return err; + }; } var stale_snapshot_files_removed: usize = 0; diff --git a/src/registry/storage_parse.zig b/src/registry/storage_parse.zig index 30d5a5fe..b075d18a 100644 --- a/src/registry/storage_parse.zig +++ b/src/registry/storage_parse.zig @@ -39,7 +39,20 @@ pub fn parseAccountRecord(allocator: std.mem.Allocator, obj: std.json.ObjectMap, else => return error.MissingChatgptUserId, }, .email = try normalizeEmailAlloc(allocator, email), - .alias = try allocator.dupe(u8, alias), + // Older builds stored aliases without validation. A control-containing + // alias can never be typed as a selector, so drop it rather than + // silently renaming it to a different value. + .alias = blk: { + const cleaned = try common.sanitizeDisplayAlloc(allocator, alias); + if (!std.mem.eql(u8, cleaned, alias)) { + allocator.free(cleaned); + break :blk try allocator.dupe(u8, ""); + } + break :blk cleaned; + }, + // Stored display names predate the entry-point sanitization, so they + // still get cleaned here: every render path consumes the loaded + // record, not the raw JSON. .account_name = try parseOptionalStoredStringAlloc(allocator, obj.get("account_name")), .plan = null, .auth_mode = null, @@ -81,5 +94,5 @@ fn parseOptionalStoredStringAlloc(allocator: std.mem.Allocator, value: ?std.json else => return null, }; if (text.len == 0) return null; - return try allocator.dupe(u8, text); + return try common.sanitizeDisplayAlloc(allocator, text); } diff --git a/src/workflows/app.zig b/src/workflows/app.zig index 6e686ad2..3a515b31 100644 --- a/src/workflows/app.zig +++ b/src/workflows/app.zig @@ -393,7 +393,7 @@ fn isCodexAppRunningOnWindows(allocator: std.mem.Allocator, app_id: []const u8) null, ) catch return null; defer result.deinit(allocator); - if (result.timed_out) return null; + if (result.timed_out or !childExitedSuccessfully(result.term)) return null; return std.mem.trim(u8, result.stdout, " \t\r\n").len != 0; } @@ -492,10 +492,31 @@ fn writeAppPlatformSetting(allocator: std.mem.Allocator, home: []const u8, value } /// config.toml is user-edited state shared with Codex itself; a crash mid-write -/// must leave either the old or the new file, never a truncated one. Mirrors -/// the registry replace dance so it works on Windows, where rename does not -/// overwrite an existing destination. +/// must leave either the old or the new file, never a truncated or missing one. +/// A symlinked config stays linked: the rewrite targets the real path, so the +/// link keeps pointing at the updated file instead of becoming a plain copy. +/// POSIX gets a single atomic rename; Windows keeps the backup-and-swap dance +/// because its rename cannot overwrite a destination. fn writeConfigFileAtomic(path: []const u8, data: []const u8) !void { + const allocator = app_runtime.scratchAllocator(); + const target = app_runtime.realPathFileAlloc(allocator, std.Io.Dir.cwd(), path) catch try allocator.dupe(u8, path); + defer allocator.free(target); + + if (builtin.target.os.tag != .windows) { + var buf: [4096]u8 = undefined; + var atomic_file = try std.Io.Dir.cwd().createFileAtomic(app_runtime.io(), target, .{ .replace = true }); + defer atomic_file.deinit(app_runtime.io()); + var file_writer = atomic_file.file.writer(app_runtime.io(), &buf); + try file_writer.interface.writeAll(data); + try file_writer.interface.flush(); + try atomic_file.replace(app_runtime.io()); + return; + } + + return writeConfigFileWindowsReplace(target, data); +} + +fn writeConfigFileWindowsReplace(path: []const u8, data: []const u8) !void { const allocator = app_runtime.scratchAllocator(); const temp_path = try allocator.print("{s}.tmp.{d}", .{ path, @as(i128, std.Io.Timestamp.now(app_runtime.io(), .real).toNanoseconds()) }); defer allocator.free(temp_path); @@ -1237,7 +1258,10 @@ fn installManagedCodextExecutable(allocator: std.mem.Allocator, cache_root: []co // codex-package.json. const metadata_path = try std.Io.Dir.path.join(allocator, &.{ extract_dir, "codex-package.json" }); defer allocator.free(metadata_path); - if (!fileExists(metadata_path)) return error.CodextReleaseInstallFailed; + if (!fileExists(metadata_path)) { + try writeAppError("codext release archive did not contain the expected package layout (codex-package.json is missing).\n"); + return error.CodextReleaseInstallFailed; + } const archive_name = if (platform == .win) "codext.zip" else "codext.tar.gz"; const archive_path = try std.Io.Dir.path.join(allocator, &.{ extract_dir, archive_name }); From 4a617b12a23d19bcb1a5aef9669faa3eb293a865 Mon Sep 17 00:00:00 2001 From: Loongphy Date: Sun, 8 Nov 2026 12:35:03 +0800 Subject: [PATCH 3/5] fix: reject c1 controls and malformed utf-8 in aliases like the loader --- src/registry/common.zig | 14 ++++++++++---- tests/registry_test.zig | 9 +++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/registry/common.zig b/src/registry/common.zig index b8292172..02d039ad 100644 --- a/src/registry/common.zig +++ b/src/registry/common.zig @@ -635,15 +635,21 @@ fn aliasIsDigitOnly(value: []const u8) bool { /// Format rules shared by `alias set` and `import --alias`: non-empty, never a /// number (digits select displayed rows), never flag- or colon-prefixed -/// (selectors and completion protocols reserve those), and free of control -/// characters. +/// (selectors and completion protocols reserve those), and free of the same +/// control characters `sanitizeDisplayAlloc` strips — accepting a codepoint +/// the loader would remove would let a saved alias silently disappear. pub fn aliasFormatIssue(alias_value: []const u8) ?AliasIssue { if (alias_value.len == 0) return .empty; if (aliasIsDigitOnly(alias_value)) return .digit_only; if (std.mem.startsWith(u8, alias_value, "-")) return .flag_like; if (std.mem.startsWith(u8, alias_value, ":")) return .colon_prefix; - for (alias_value) |ch| { - if (ch < 0x20 or ch == 0x7f) return .control_char; + var index: usize = 0; + while (index < alias_value.len) { + const cp_len = std.unicode.utf8ByteSequenceLength(alias_value[index]) catch return .control_char; + if (index + cp_len > alias_value.len) return .control_char; + const cp = std.unicode.utf8Decode(alias_value[index..][0..cp_len]) catch return .control_char; + if (cp < 0x20 or cp == 0x7f or (cp >= 0x80 and cp <= 0x9f)) return .control_char; + index += cp_len; } return null; } diff --git a/tests/registry_test.zig b/tests/registry_test.zig index f697c628..fe5df838 100644 --- a/tests/registry_test.zig +++ b/tests/registry_test.zig @@ -2157,3 +2157,12 @@ test "sanitizeDisplayAlloc replaces control characters and invalid utf-8" { try std.testing.expect(std.mem.indexOfScalar(u8, truncated, 0xf0) == null); try std.testing.expect(std.ascii.isPrint(truncated[truncated.len - 1])); } + +test "aliasFormatIssue rejects c1 controls and malformed utf-8 like the loader does" { + const c1 = "bad\xc2\x85alias"; + try std.testing.expect(registry.aliasFormatIssue(c1) == .control_char); + try std.testing.expect(registry.aliasFormatIssue("bad\xf0\x9f") == .control_char); + try std.testing.expect(registry.aliasFormatIssue("bad\x80alias") == .control_char); + try std.testing.expect(registry.aliasFormatIssue("fine-alias_1") == null); + try std.testing.expect(registry.aliasFormatIssue("emoji-\xe2\x9c\x93") == null); +} From 5e5f111561ec732644ae8b6c885ec99461e01ed9 Mon Sep 17 00:00:00 2001 From: Loongphy Date: Sun, 8 Nov 2026 12:39:18 +0800 Subject: [PATCH 4/5] fix: keep rejected array aliases fatal and probe the socket on missing helpers - A rejected --alias on the single aliased array item now fails the import, so a purge run never saves a registry missing that account. - ExecutableRequired no longer maps blindly to not_running: when the control socket still exists the daemon state stays unconfirmed in both the switch path and the live daemon worker. --- src/registry/import.zig | 4 ++++ src/workflows/codex_daemon.zig | 20 +++++++++++++++++++- src/workflows/live_runtime.zig | 7 ++++--- src/workflows/switch.zig | 10 +++++++--- tests/cli_integration_test.zig | 6 ++++-- 5 files changed, 38 insertions(+), 9 deletions(-) diff --git a/src/registry/import.zig b/src/registry/import.zig index a7359cb5..66e1fdfe 100644 --- a/src/registry/import.zig +++ b/src/registry/import.zig @@ -270,6 +270,10 @@ fn appendAuthArrayData( const outcome = importConvertedAuthInfo(allocator, codex_home, reg, item_alias, &info, item_data) catch |err| { if (!isImportValidationError(err)) return err; try report.addItemEvent(allocator, label, item_index, .skipped, importReasonLabel(err), detail); + // An explicit --alias rejection on the single aliased item fails + // the import: without a failure, a purge run would still save the + // rebuilt registry and drop the rejected account for good. + if (item_alias != null) report.failure = err; continue; }; try report.addItemEvent(allocator, label, item_index, outcome, null, detail); diff --git a/src/workflows/codex_daemon.zig b/src/workflows/codex_daemon.zig index c705eae0..5659a799 100644 --- a/src/workflows/codex_daemon.zig +++ b/src/workflows/codex_daemon.zig @@ -36,11 +36,29 @@ pub const Client = struct { self.allocator.free(self.executable); } + fn controlSocketPathAlloc(allocator: std.mem.Allocator, codex_home: []const u8) ![]u8 { + return std.Io.Dir.path.join(allocator, &.{ codex_home, "app-server-control", "app-server-control.sock" }); + } + + /// The control endpoint is the only presence signal that survives a + /// missing or moved helper executable: callers that cannot resolve one + /// must still consult the socket before reporting the daemon absent. A + /// probe error is conservatively presence, not absence. + pub fn controlEndpointPresent(codex_home: []const u8) !bool { + const socket = try controlSocketPathAlloc(runtime.scratchAllocator(), codex_home); + defer runtime.scratchAllocator().free(socket); + std.Io.Dir.cwd().access(runtime.io(), socket, .{}) catch |err| switch (err) { + error.FileNotFound => return false, + else => return true, + }; + return true; + } + /// A socket path alone never establishes responsiveness. A missing endpoint /// is a best-effort absence check; errors and stale endpoints stay unknown. pub fn probe(self: *const Client) !Outcome { if (!supported()) return .unsupported; - const socket = try std.Io.Dir.path.join(self.allocator, &.{ self.codex_home, "app-server-control", "app-server-control.sock" }); + const socket = try controlSocketPathAlloc(self.allocator, self.codex_home); defer self.allocator.free(socket); std.Io.Dir.cwd().access(runtime.io(), socket, .{}) catch |err| switch (err) { error.FileNotFound => return .not_running, diff --git a/src/workflows/live_runtime.zig b/src/workflows/live_runtime.zig index 38b4e102..66d9735c 100644 --- a/src/workflows/live_runtime.zig +++ b/src/workflows/live_runtime.zig @@ -566,9 +566,10 @@ fn runDaemonRound(worker: *DaemonWorker) void { const text: ?[]const u8 = blk: { var client = daemon.Client.init(allocator, worker.codex_home) catch |err| { if (err == error.OutOfMemory) break :blk "out of memory"; - // No executable means there is nothing to coordinate, matching the - // silent absence of a control socket. - if (err == error.ExecutableRequired) break :blk null; + // No executable is only absence when the control socket is gone + // too; a live endpoint with an unlaunchable helper stays + // unconfirmed. + if (err == error.ExecutableRequired and !(daemon.Client.controlEndpointPresent(worker.codex_home) catch true)) break :blk null; break :blk "daemon state unconfirmed"; }; defer client.deinit(); diff --git a/src/workflows/switch.zig b/src/workflows/switch.zig index f6fd9082..86e267ea 100644 --- a/src/workflows/switch.zig +++ b/src/workflows/switch.zig @@ -254,9 +254,13 @@ fn activateSelection( if (!opts.json) try cli.output.printSwitchedAccount(allocator, reg, account_key); var client = daemon.Client.init(allocator, codex_home) catch |err| { if (err == error.OutOfMemory) return err; - // No daemon executable is absence, not an unconfirmed state: stay on - // the silent not_running contract a missing control socket produces. - const outcome: daemon.Outcome = if (err == error.ExecutableRequired) .not_running else .unknown; + // A missing executable is only absence when the control socket is + // gone too; a live endpoint with an unlaunchable helper stays + // unconfirmed, because that daemon may still serve old credentials. + const outcome: daemon.Outcome = if (err == error.ExecutableRequired) + (if (try daemon.Client.controlEndpointPresent(codex_home)) .unknown else .not_running) + else + .unknown; cli.output.printCodexDaemonDiagnostic(codex_home, "codex", outcome) catch {}; if (!opts.json) return; var result = results.buildSwitchResult(allocator, reg, account_key, outcome) catch |build_err| return printJsonWorkflowError(build_err); diff --git a/tests/cli_integration_test.zig b/tests/cli_integration_test.zig index 5a90d0e2..9cb1d93f 100644 --- a/tests/cli_integration_test.zig +++ b/tests/cli_integration_test.zig @@ -976,8 +976,10 @@ test "Scenario: Given no managed executable then daemon restart resolves relativ if (path.len == 0) { // An empty PATH segment would resolve `codex` relative to the // working directory; the resolver must refuse that implicit - // search path, so the switch skips daemon coordination silently. - try std.testing.expect(std.mem.indexOf(u8, result.stderr, "daemon") == null); + // search path. The control socket still exists, so the switch + // reports the daemon state as unconfirmed rather than absent — + // but no lifecycle helper may ever run. + try std.testing.expect(std.mem.indexOf(u8, result.stderr, "daemon responsiveness could not be confirmed") != null); try std.testing.expectError(error.FileNotFound, fake_daemon.read(dir, "lifecycle-calls")); try std.testing.expectError(error.FileNotFound, dir.openFile("restarted-home", .{})); continue; From 49e03cf77c1d66a04ec4714ec938d6bb21f53f2d Mon Sep 17 00:00:00 2001 From: Loongphy Date: Sun, 8 Nov 2026 12:48:03 +0800 Subject: [PATCH 5/5] test: seed a managed stub executable for missing-endpoint daemon tests --- tests/workflows_daemon_test.zig | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/workflows_daemon_test.zig b/tests/workflows_daemon_test.zig index 220ccc77..2f09235e 100644 --- a/tests/workflows_daemon_test.zig +++ b/tests/workflows_daemon_test.zig @@ -5,9 +5,25 @@ const fs = @import("codex_auth").core.compat_fs; const runtime = @import("codex_auth").core.runtime; const fake = @import("support/codex_daemon.zig"); +/// A managed executable placeholder with no control endpoint: enough for +/// Client.init to resolve the helper while every probe still reports +/// not_running, so no lifecycle command can run. +fn installStubExecutable(dir: fs.Dir) !void { + const name = if (builtin.target.os.tag == .windows) "codex.exe" else "codex"; + const sub_path = try fs.path.join(std.testing.allocator, &.{ "packages", "app-server-daemon", "current", "bin", name }); + defer std.testing.allocator.free(sub_path); + try dir.makePath("packages/app-server-daemon/current/bin"); + try dir.writeFile(.{ .sub_path = sub_path, .data = "stub" }); + if (builtin.target.os.tag == .windows) return; + var file = try dir.openFile(sub_path, .{ .mode = .read_write }); + defer file.close(); + try file.chmod(0o700); +} + test "Scenario: Given a missing daemon endpoint then lifecycle commands are not invoked" { var tmp = fs.tmpDir(.{}); defer tmp.cleanup(); + try installStubExecutable(tmp.dir); const home = try tmp.dir.realpathAlloc(std.testing.allocator, "."); defer std.testing.allocator.free(home); var client = try daemon.Client.init(std.testing.allocator, home); @@ -153,6 +169,7 @@ test "Scenario: Given a slow reload then ensure reports unconfirmed without rest test "Scenario: Given a missing daemon endpoint then the default reload stays a no-op" { var tmp = fs.tmpDir(.{}); defer tmp.cleanup(); + try installStubExecutable(tmp.dir); const home = try tmp.dir.realpathAlloc(std.testing.allocator, "."); defer std.testing.allocator.free(home); var client = try daemon.Client.init(std.testing.allocator, home);