From dc276999a9d63feb305d4de7f176c411cef71ebf Mon Sep 17 00:00:00 2001 From: Thibault Sottiaux Date: Mon, 21 Apr 2025 09:51:34 -0400 Subject: [PATCH] chore: improve storage/ implementation; use log(...) consistently (#473) This PR tidies up primitives under storage/. **Noop changes:** * Promote logger implementation to top-level utility outside of agent/ * Use logger within storage primitives * Cleanup doc strings and comments **Functional changes:** * Increase command history size to 10_000 * Remove unnecessary debounce implementation and ensure a session ID is created only once per agent loop --------- Signed-off-by: Thibault Sottiaux --- codex-cli/src/cli.tsx | 2 +- .../chat/terminal-chat-input-thinking.tsx | 2 +- .../components/chat/terminal-chat-input.tsx | 2 +- .../chat/terminal-chat-new-input.tsx | 2 +- .../src/components/chat/terminal-chat.tsx | 5 +- codex-cli/src/utils/agent/agent-loop.ts | 2 +- .../src/utils/agent/handle-exec-command.ts | 2 +- .../src/utils/agent/platform-commands.ts | 2 +- .../src/utils/agent/sandbox/macos-seatbelt.ts | 2 +- codex-cli/src/utils/agent/sandbox/raw-exec.ts | 2 +- codex-cli/src/utils/config.ts | 2 +- codex-cli/src/utils/{agent => logger}/log.ts | 0 .../src/utils/storage/command-history.ts | 68 +++++++------------ codex-cli/src/utils/storage/save-rollout.ts | 31 +++------ package.json | 1 + 15 files changed, 49 insertions(+), 76 deletions(-) rename codex-cli/src/utils/{agent => logger}/log.ts (100%) diff --git a/codex-cli/src/cli.tsx b/codex-cli/src/cli.tsx index 948c0d3d..e6bed134 100644 --- a/codex-cli/src/cli.tsx +++ b/codex-cli/src/cli.tsx @@ -14,7 +14,6 @@ import type { ResponseItem } from "openai/resources/responses/responses"; import App from "./app"; import { runSinglePass } from "./cli-singlepass"; import { AgentLoop } from "./utils/agent/agent-loop"; -import { initLogger } from "./utils/agent/log"; import { ReviewDecision } from "./utils/agent/review"; import { AutoApprovalMode } from "./utils/auto-approval-mode"; import { checkForUpdates } from "./utils/check-updates"; @@ -25,6 +24,7 @@ import { INSTRUCTIONS_FILEPATH, } from "./utils/config"; import { createInputItem } from "./utils/input-utils"; +import { initLogger } from "./utils/logger/log"; import { isModelSupportedForResponses } from "./utils/model-utils.js"; import { parseToolCall } from "./utils/parsers"; import { onExit, setInkRenderer } from "./utils/terminal"; diff --git a/codex-cli/src/components/chat/terminal-chat-input-thinking.tsx b/codex-cli/src/components/chat/terminal-chat-input-thinking.tsx index fdc8bd21..dd938b06 100644 --- a/codex-cli/src/components/chat/terminal-chat-input-thinking.tsx +++ b/codex-cli/src/components/chat/terminal-chat-input-thinking.tsx @@ -1,4 +1,4 @@ -import { log } from "../../utils/agent/log.js"; +import { log } from "../../utils/logger/log.js"; import { Box, Text, useInput, useStdin } from "ink"; import React, { useState } from "react"; import { useInterval } from "use-interval"; diff --git a/codex-cli/src/components/chat/terminal-chat-input.tsx b/codex-cli/src/components/chat/terminal-chat-input.tsx index ddb9c7a1..8768dd8d 100644 --- a/codex-cli/src/components/chat/terminal-chat-input.tsx +++ b/codex-cli/src/components/chat/terminal-chat-input.tsx @@ -9,10 +9,10 @@ import type { import MultilineTextEditor from "./multiline-editor"; import { TerminalChatCommandReview } from "./terminal-chat-command-review.js"; import TextCompletions from "./terminal-chat-completions.js"; -import { log } from "../../utils/agent/log.js"; import { loadConfig } from "../../utils/config.js"; import { getFileSystemSuggestions } from "../../utils/file-system-suggestions.js"; import { createInputItem } from "../../utils/input-utils.js"; +import { log } from "../../utils/logger/log.js"; import { setSessionId } from "../../utils/session.js"; import { SLASH_COMMANDS, type SlashCommand } from "../../utils/slash-commands"; import { diff --git a/codex-cli/src/components/chat/terminal-chat-new-input.tsx b/codex-cli/src/components/chat/terminal-chat-new-input.tsx index 7dbe130e..95915934 100644 --- a/codex-cli/src/components/chat/terminal-chat-new-input.tsx +++ b/codex-cli/src/components/chat/terminal-chat-new-input.tsx @@ -8,9 +8,9 @@ import type { import MultilineTextEditor from "./multiline-editor"; import { TerminalChatCommandReview } from "./terminal-chat-command-review.js"; -import { log } from "../../utils/agent/log.js"; import { loadConfig } from "../../utils/config.js"; import { createInputItem } from "../../utils/input-utils.js"; +import { log } from "../../utils/logger/log.js"; import { setSessionId } from "../../utils/session.js"; import { loadCommandHistory, diff --git a/codex-cli/src/components/chat/terminal-chat.tsx b/codex-cli/src/components/chat/terminal-chat.tsx index 9ebcfbc7..e20097eb 100644 --- a/codex-cli/src/components/chat/terminal-chat.tsx +++ b/codex-cli/src/components/chat/terminal-chat.tsx @@ -15,13 +15,13 @@ import { formatCommandForDisplay } from "../../format-command.js"; import { useConfirmation } from "../../hooks/use-confirmation.js"; import { useTerminalSize } from "../../hooks/use-terminal-size.js"; import { AgentLoop } from "../../utils/agent/agent-loop.js"; -import { log } from "../../utils/agent/log.js"; import { ReviewDecision } from "../../utils/agent/review.js"; import { generateCompactSummary } from "../../utils/compact-summary.js"; import { OPENAI_BASE_URL, saveConfig } from "../../utils/config.js"; import { extractAppliedPatches as _extractAppliedPatches } from "../../utils/extract-applied-patches.js"; import { getGitDiff } from "../../utils/get-diff.js"; import { createInputItem } from "../../utils/input-utils.js"; +import { log } from "../../utils/logger/log.js"; import { getAvailableModels } from "../../utils/model-utils.js"; import { CLI_VERSION } from "../../utils/session.js"; import { shortCwd } from "../../utils/short-path.js"; @@ -237,6 +237,7 @@ export default function TerminalChat({ // Tear down any existing loop before creating a new one agentRef.current?.terminate(); + const sessionId = crypto.randomUUID(); agentRef.current = new AgentLoop({ model, provider, @@ -249,7 +250,7 @@ export default function TerminalChat({ log(`onItem: ${JSON.stringify(item)}`); setItems((prev) => { const updated = uniqueById([...prev, item as ResponseItem]); - saveRollout(updated); + saveRollout(sessionId, updated); return updated; }); }, diff --git a/codex-cli/src/utils/agent/agent-loop.ts b/codex-cli/src/utils/agent/agent-loop.ts index 311cad96..09bb26f8 100644 --- a/codex-cli/src/utils/agent/agent-loop.ts +++ b/codex-cli/src/utils/agent/agent-loop.ts @@ -10,8 +10,8 @@ import type { } from "openai/resources/responses/responses.mjs"; import type { Reasoning } from "openai/resources.mjs"; -import { log } from "./log.js"; import { OPENAI_TIMEOUT_MS, getApiKey, getBaseUrl } from "../config.js"; +import { log } from "../logger/log.js"; import { parseToolCallArguments } from "../parsers.js"; import { responsesCreateViaChatCompletions } from "../responses.js"; import { diff --git a/codex-cli/src/utils/agent/handle-exec-command.ts b/codex-cli/src/utils/agent/handle-exec-command.ts index 97b78c08..ea87feca 100644 --- a/codex-cli/src/utils/agent/handle-exec-command.ts +++ b/codex-cli/src/utils/agent/handle-exec-command.ts @@ -5,12 +5,12 @@ import type { ApplyPatchCommand, ApprovalPolicy } from "../../approvals.js"; import type { ResponseInputItem } from "openai/resources/responses/responses.mjs"; import { exec, execApplyPatch } from "./exec.js"; -import { isLoggingEnabled, log } from "./log.js"; import { ReviewDecision } from "./review.js"; import { FullAutoErrorMode } from "../auto-approval-mode.js"; import { SandboxType } from "./sandbox/interface.js"; import { canAutoApprove } from "../../approvals.js"; import { formatCommandForDisplay } from "../../format-command.js"; +import { isLoggingEnabled, log } from "../logger/log.js"; import { access } from "fs/promises"; // --------------------------------------------------------------------------- diff --git a/codex-cli/src/utils/agent/platform-commands.ts b/codex-cli/src/utils/agent/platform-commands.ts index 085c575d..161388f9 100644 --- a/codex-cli/src/utils/agent/platform-commands.ts +++ b/codex-cli/src/utils/agent/platform-commands.ts @@ -2,7 +2,7 @@ * Utility functions for handling platform-specific commands */ -import { log } from "./log.js"; +import { log } from "../logger/log.js"; /** * Map of Unix commands to their Windows equivalents diff --git a/codex-cli/src/utils/agent/sandbox/macos-seatbelt.ts b/codex-cli/src/utils/agent/sandbox/macos-seatbelt.ts index 760ba63d..934056d9 100644 --- a/codex-cli/src/utils/agent/sandbox/macos-seatbelt.ts +++ b/codex-cli/src/utils/agent/sandbox/macos-seatbelt.ts @@ -2,7 +2,7 @@ import type { ExecResult } from "./interface.js"; import type { SpawnOptions } from "child_process"; import { exec } from "./raw-exec.js"; -import { log } from "../log.js"; +import { log } from "../../logger/log.js"; function getCommonRoots() { return [ diff --git a/codex-cli/src/utils/agent/sandbox/raw-exec.ts b/codex-cli/src/utils/agent/sandbox/raw-exec.ts index 22add83e..b3d1d8ec 100644 --- a/codex-cli/src/utils/agent/sandbox/raw-exec.ts +++ b/codex-cli/src/utils/agent/sandbox/raw-exec.ts @@ -7,7 +7,7 @@ import type { StdioPipe, } from "child_process"; -import { log } from "../log.js"; +import { log } from "../../logger/log.js"; import { adaptCommandForPlatform } from "../platform-commands.js"; import { spawn } from "child_process"; import * as os from "os"; diff --git a/codex-cli/src/utils/config.ts b/codex-cli/src/utils/config.ts index ada95025..91e0c367 100644 --- a/codex-cli/src/utils/config.ts +++ b/codex-cli/src/utils/config.ts @@ -8,8 +8,8 @@ import type { FullAutoErrorMode } from "./auto-approval-mode.js"; -import { log } from "./agent/log.js"; import { AutoApprovalMode } from "./auto-approval-mode.js"; +import { log } from "./logger/log.js"; import { providers } from "./providers.js"; import { existsSync, mkdirSync, readFileSync, writeFileSync } from "fs"; import { load as loadYaml, dump as dumpYaml } from "js-yaml"; diff --git a/codex-cli/src/utils/agent/log.ts b/codex-cli/src/utils/logger/log.ts similarity index 100% rename from codex-cli/src/utils/agent/log.ts rename to codex-cli/src/utils/logger/log.ts diff --git a/codex-cli/src/utils/storage/command-history.ts b/codex-cli/src/utils/storage/command-history.ts index 997c4872..d774b533 100644 --- a/codex-cli/src/utils/storage/command-history.ts +++ b/codex-cli/src/utils/storage/command-history.ts @@ -1,12 +1,13 @@ +import { log } from "../logger/log.js"; import { existsSync } from "fs"; import fs from "fs/promises"; import os from "os"; import path from "path"; const HISTORY_FILE = path.join(os.homedir(), ".codex", "history.json"); -const DEFAULT_HISTORY_SIZE = 1000; +const DEFAULT_HISTORY_SIZE = 10_000; -// Regex patterns for sensitive commands that should not be saved +// Regex patterns for sensitive commands that should not be saved. const SENSITIVE_PATTERNS = [ /\b[A-Za-z0-9-_]{20,}\b/, // API keys and tokens /\bpassword\b/i, @@ -18,7 +19,7 @@ const SENSITIVE_PATTERNS = [ export interface HistoryConfig { maxSize: number; saveHistory: boolean; - sensitivePatterns: Array; // Array of regex patterns as strings + sensitivePatterns: Array; // Regex patterns. } export interface HistoryEntry { @@ -32,9 +33,6 @@ export const DEFAULT_HISTORY_CONFIG: HistoryConfig = { sensitivePatterns: [], }; -/** - * Loads command history from the history file - */ export async function loadCommandHistory(): Promise> { try { if (!existsSync(HISTORY_FILE)) { @@ -45,26 +43,21 @@ export async function loadCommandHistory(): Promise> { const history = JSON.parse(data) as Array; return Array.isArray(history) ? history : []; } catch (error) { - // Use error logger but for production would use a proper logging system - // eslint-disable-next-line no-console - console.error("Failed to load command history:", error); + log(`error: failed to load command history: ${error}`); return []; } } -/** - * Saves command history to the history file - */ export async function saveCommandHistory( history: Array, config: HistoryConfig = DEFAULT_HISTORY_CONFIG, ): Promise { try { - // Create directory if it doesn't exist + // Create directory if it doesn't exist. const dir = path.dirname(HISTORY_FILE); await fs.mkdir(dir, { recursive: true }); - // Trim history to max size + // Trim history to max size. const trimmedHistory = history.slice(-config.maxSize); await fs.writeFile( @@ -73,14 +66,10 @@ export async function saveCommandHistory( "utf-8", ); } catch (error) { - // eslint-disable-next-line no-console - console.error("Failed to save command history:", error); + log(`error: failed to save command history: ${error}`); } } -/** - * Adds a command to history if it's not sensitive - */ export async function addToHistory( command: string, history: Array, @@ -90,46 +79,41 @@ export async function addToHistory( return history; } - // Check if command contains sensitive information - if (isSensitiveCommand(command, config.sensitivePatterns)) { + // Skip commands with sensitive information. + if (commandHasSensitiveInfo(command, config.sensitivePatterns)) { return history; } - // Check for duplicate (don't add if it's the same as the last command) + // Check for duplicate (don't add if it's the same as the last command). const lastEntry = history[history.length - 1]; if (lastEntry && lastEntry.command === command) { return history; } - // Add new entry - const newEntry: HistoryEntry = { - command, - timestamp: Date.now(), - }; - - const newHistory = [...history, newEntry]; - - // Save to file + // Add new entry. + const newHistory: Array = [ + ...history, + { + command, + timestamp: Date.now(), + }, + ]; await saveCommandHistory(newHistory, config); - return newHistory; } -/** - * Checks if a command contains sensitive information - */ -function isSensitiveCommand( +function commandHasSensitiveInfo( command: string, additionalPatterns: Array = [], ): boolean { - // Check built-in patterns + // Check built-in patterns. for (const pattern of SENSITIVE_PATTERNS) { if (pattern.test(command)) { return true; } } - // Check additional patterns from config + // Check additional patterns from config. for (const patternStr of additionalPatterns) { try { const pattern = new RegExp(patternStr); @@ -137,23 +121,19 @@ function isSensitiveCommand( return true; } } catch (error) { - // Invalid regex pattern, skip it + // Invalid regex pattern, skip it. } } return false; } -/** - * Clears the command history - */ export async function clearCommandHistory(): Promise { try { if (existsSync(HISTORY_FILE)) { await fs.writeFile(HISTORY_FILE, JSON.stringify([]), "utf-8"); } } catch (error) { - // eslint-disable-next-line no-console - console.error("Failed to clear command history:", error); + log(`error: failed to clear command history: ${error}`); } } diff --git a/codex-cli/src/utils/storage/save-rollout.ts b/codex-cli/src/utils/storage/save-rollout.ts index 76d931fd..d6b5a491 100644 --- a/codex-cli/src/utils/storage/save-rollout.ts +++ b/codex-cli/src/utils/storage/save-rollout.ts @@ -1,20 +1,19 @@ -/* eslint-disable no-console */ - import type { ResponseItem } from "openai/resources/responses/responses"; import { loadConfig } from "../config"; +import { log } from "../logger/log.js"; import fs from "fs/promises"; import os from "os"; import path from "path"; const SESSIONS_ROOT = path.join(os.homedir(), ".codex", "sessions"); -async function saveRolloutToHomeSessions( +async function saveRolloutAsync( + sessionId: string, items: Array, ): Promise { await fs.mkdir(SESSIONS_ROOT, { recursive: true }); - const sessionId = crypto.randomUUID(); const timestamp = new Date().toISOString(); const ts = timestamp.replace(/[:.]/g, "-").slice(0, 10); const filename = `rollout-${ts}-${sessionId}.json`; @@ -39,23 +38,15 @@ async function saveRolloutToHomeSessions( "utf8", ); } catch (error) { - console.error(`Failed to save rollout to ${filePath}: `, error); + log(`error: failed to save rollout to ${filePath}: ${error}`); } } -let debounceTimer: NodeJS.Timeout | null = null; -let pendingItems: Array | null = null; - -export function saveRollout(items: Array): void { - pendingItems = items; - if (debounceTimer) { - clearTimeout(debounceTimer); - } - debounceTimer = setTimeout(() => { - if (pendingItems) { - saveRolloutToHomeSessions(pendingItems).catch(() => {}); - pendingItems = null; - } - debounceTimer = null; - }, 2000); +export function saveRollout( + sessionId: string, + items: Array, +): void { + // Best-effort. We also do not log here in case of failure as that should be taken care of + // by `saveRolloutAsync` already. + saveRolloutAsync(sessionId, items).catch(() => {}); } diff --git a/package.json b/package.json index 215616be..b516ce40 100644 --- a/package.json +++ b/package.json @@ -9,6 +9,7 @@ "build": "pnpm --filter @openai/codex run build", "test": "pnpm --filter @openai/codex run test", "lint": "pnpm --filter @openai/codex run lint", + "lint:fix": "pnpm --filter @openai/codex run lint:fix", "typecheck": "pnpm --filter @openai/codex run typecheck", "changelog": "git-cliff --config cliff.toml --output CHANGELOG.ignore.md $LAST_RELEASE_TAG..HEAD", "prepare": "husky",