From 61b881d4e51a0e41e0bdce89feb6390f722a946c Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 12 May 2025 17:24:44 -0700 Subject: [PATCH 1/2] fix: agent instructions were not being included when ~/.codex/instructions.md was empty (#908) I had seen issues where `codex-rs` would not always write files without me pressuring it to do so, and between that and the report of https://github.com/openai/codex/issues/900, I decided to look into this further. I found two serious issues with agent instructions: (1) We were only sending agent instructions on the first turn, but looking at the TypeScript code, we should be sending them on every turn. (2) There was a serious issue where the agent instructions were frequently lost: * The TypeScript CLI appears to keep writing `~/.codex/instructions.md`: https://github.com/openai/codex/blob/55142e3e6caddd1e613b71bcb89385ce5cc708bf/codex-cli/src/utils/config.ts#L586 * If `instructions.md` is present, the Rust CLI uses the contents of it INSTEAD OF the default prompt, even if `instructions.md` is empty: https://github.com/openai/codex/blob/55142e3e6caddd1e613b71bcb89385ce5cc708bf/codex-rs/core/src/config.rs#L202-L203 The combination of these two things means that I have been using `codex-rs` without these key instructions: https://github.com/openai/codex/blob/main/codex-rs/core/prompt.md Looking at the TypeScript code, it appears we should be concatenating these three items every time (if they exist): * `prompt.md` * `~/.codex/instructions.md` * nearest `AGENTS.md` This PR fixes things so that: * `Config.instructions` is `None` if `instructions.md` is empty * `Payload.instructions` is now `&'a str` instead of `Option<&'a String>` because we should always have _something_ to send * `Prompt` now has a `get_full_instructions()` helper that returns a `Cow` that will always include the agent instructions first. --- codex-rs/core/src/chat_completions.rs | 5 ++--- codex-rs/core/src/client.rs | 3 ++- codex-rs/core/src/client_common.rs | 23 ++++++++++++++++++++--- codex-rs/core/src/config.rs | 20 ++++++++++---------- 4 files changed, 34 insertions(+), 17 deletions(-) diff --git a/codex-rs/core/src/chat_completions.rs b/codex-rs/core/src/chat_completions.rs index 8e818c2f03..7760c48fbf 100644 --- a/codex-rs/core/src/chat_completions.rs +++ b/codex-rs/core/src/chat_completions.rs @@ -38,9 +38,8 @@ pub(crate) async fn stream_chat_completions( // Build messages array let mut messages = Vec::::new(); - if let Some(instr) = &prompt.instructions { - messages.push(json!({"role": "system", "content": instr})); - } + let full_instructions = prompt.get_full_instructions(); + messages.push(json!({"role": "system", "content": full_instructions})); for item in &prompt.input { if let ResponseItem::Message { role, content } = item { diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index f8f303911e..7316e90456 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -166,9 +166,10 @@ impl ModelClient { debug!("tools_json: {}", serde_json::to_string_pretty(&tools_json)?); + let full_instructions = prompt.get_full_instructions(); let payload = Payload { model: &self.model, - instructions: prompt.instructions.as_ref(), + instructions: &full_instructions, input: &prompt.input, tools: &tools_json, tool_choice: "auto", diff --git a/codex-rs/core/src/client_common.rs b/codex-rs/core/src/client_common.rs index fcdac71d5a..8eb8074b1e 100644 --- a/codex-rs/core/src/client_common.rs +++ b/codex-rs/core/src/client_common.rs @@ -2,12 +2,17 @@ use crate::error::Result; use crate::models::ResponseItem; use futures::Stream; use serde::Serialize; +use std::borrow::Cow; use std::collections::HashMap; use std::pin::Pin; use std::task::Context; use std::task::Poll; use tokio::sync::mpsc; +/// The `instructions` field in the payload sent to a model should always start +/// with this content. +const BASE_INSTRUCTIONS: &str = include_str!("../prompt.md"); + /// API request payload for a single model turn. #[derive(Default, Debug, Clone)] pub struct Prompt { @@ -15,7 +20,8 @@ pub struct Prompt { pub input: Vec, /// Optional previous response ID (when storage is enabled). pub prev_id: Option, - /// Optional initial instructions (only sent on first turn). + /// Optional instructions from the user to amend to the built-in agent + /// instructions. pub instructions: Option, /// Whether to store response on server side (disable_response_storage = !store). pub store: bool, @@ -26,6 +32,18 @@ pub struct Prompt { pub extra_tools: HashMap, } +impl Prompt { + pub(crate) fn get_full_instructions(&self) -> Cow { + match &self.instructions { + Some(instructions) => { + let instructions = format!("{BASE_INSTRUCTIONS}\n{instructions}"); + Cow::Owned(instructions) + } + None => Cow::Borrowed(BASE_INSTRUCTIONS), + } + } +} + #[derive(Debug)] pub enum ResponseEvent { OutputItemDone(ResponseItem), @@ -54,8 +72,7 @@ pub(crate) enum Summary { #[derive(Debug, Serialize)] pub(crate) struct Payload<'a> { pub(crate) model: &'a str, - #[serde(skip_serializing_if = "Option::is_none")] - pub(crate) instructions: Option<&'a String>, + pub(crate) instructions: &'a str, // TODO(mbolin): ResponseItem::Other should not be serialized. Currently, // we code defensively to avoid this case, but perhaps we should use a // separate enum for serialization. diff --git a/codex-rs/core/src/config.rs b/codex-rs/core/src/config.rs index 6a71a45e4d..4c815ad047 100644 --- a/codex-rs/core/src/config.rs +++ b/codex-rs/core/src/config.rs @@ -10,11 +10,6 @@ use serde::Deserialize; use std::collections::HashMap; use std::path::PathBuf; -/// Embedded fallback instructions that mirror the TypeScript CLI’s default -/// system prompt. These are compiled into the binary so a clean install behaves -/// correctly even if the user has not created `~/.codex/instructions.md`. -const EMBEDDED_INSTRUCTIONS: &str = include_str!("../prompt.md"); - /// Maximum number of bytes of the documentation that will be embedded. Larger /// files are *silently truncated* to this size so we do not take up too much of /// the context window. @@ -42,7 +37,7 @@ pub struct Config { /// who have opted into Zero Data Retention (ZDR). pub disable_response_storage: bool, - /// System instructions. + /// User-provided instructions from instructions.md. pub instructions: Option, /// Optional external notifier command. When set, Codex will spawn this @@ -198,9 +193,7 @@ impl Config { cfg: ConfigToml, overrides: ConfigOverrides, ) -> std::io::Result { - // Instructions: user-provided instructions.md > embedded default. - let instructions = - Self::load_instructions().or_else(|| Some(EMBEDDED_INSTRUCTIONS.to_string())); + let instructions = Self::load_instructions(); // Destructure ConfigOverrides fully to ensure all overrides are applied. let ConfigOverrides { @@ -289,7 +282,14 @@ impl Config { fn load_instructions() -> Option { let mut p = codex_dir().ok()?; p.push("instructions.md"); - std::fs::read_to_string(&p).ok() + std::fs::read_to_string(&p).ok().and_then(|s| { + let s = s.trim(); + if s.is_empty() { + None + } else { + Some(s.to_string()) + } + }) } /// Meant to be used exclusively for tests: `load_with_overrides()` should From 33ddfabd1fad85b5dbc21ae85afd77b4747fc9fd Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 12 May 2025 18:01:44 -0700 Subject: [PATCH 2/2] fix: always load version from package.json at runtime --- codex-cli/build.mjs | 3 + codex-cli/src/app.tsx | 2 +- .../components/chat/terminal-chat-input.tsx | 4 +- .../chat/terminal-chat-past-rollout.tsx | 2 +- .../src/components/chat/terminal-chat.tsx | 2 +- codex-cli/src/session.ts | 60 +++++++++++++++++++ codex-cli/src/utils/agent/agent-loop.ts | 14 ++--- codex-cli/src/utils/check-updates.ts | 2 +- codex-cli/tests/check-updates.test.ts | 2 +- 9 files changed, 77 insertions(+), 14 deletions(-) create mode 100644 codex-cli/src/session.ts diff --git a/codex-cli/build.mjs b/codex-cli/build.mjs index 465e8b9244..16664d76fc 100644 --- a/codex-cli/build.mjs +++ b/codex-cli/build.mjs @@ -72,6 +72,9 @@ if (isDevBuild) { esbuild .build({ entryPoints: ["src/cli.tsx"], + // Do not bundle the contents of package.json at build time: always read it + // at runtime. + external: ["../package.json"], bundle: true, format: "esm", platform: "node", diff --git a/codex-cli/src/app.tsx b/codex-cli/src/app.tsx index 5d859db576..8c634a1243 100644 --- a/codex-cli/src/app.tsx +++ b/codex-cli/src/app.tsx @@ -5,7 +5,7 @@ import type { ResponseItem } from "openai/resources/responses/responses"; import TerminalChat from "./components/chat/terminal-chat"; import TerminalChatPastRollout from "./components/chat/terminal-chat-past-rollout"; import { checkInGit } from "./utils/check-in-git"; -import { CLI_VERSION, type TerminalChatSession } from "./utils/session.js"; +import { CLI_VERSION, type TerminalChatSession } from "./session.js"; import { onExit } from "./utils/terminal"; import { ConfirmInput } from "@inkjs/ui"; import { Box, Text, useApp, useStdin } from "ink"; diff --git a/codex-cli/src/components/chat/terminal-chat-input.tsx b/codex-cli/src/components/chat/terminal-chat-input.tsx index 819b8ea3eb..dbbb38a60f 100644 --- a/codex-cli/src/components/chat/terminal-chat-input.tsx +++ b/codex-cli/src/components/chat/terminal-chat-input.tsx @@ -10,12 +10,12 @@ import type { import MultilineTextEditor from "./multiline-editor"; import { TerminalChatCommandReview } from "./terminal-chat-command-review.js"; import TextCompletions from "./terminal-chat-completions.js"; +import { setSessionId } from "../../session.js"; import { loadConfig } from "../../utils/config.js"; import { getFileSystemSuggestions } from "../../utils/file-system-suggestions.js"; import { expandFileTags } from "../../utils/file-tag-utils"; 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 { loadCommandHistory, @@ -584,7 +584,7 @@ export default function TerminalChatInput({ try { const os = await import("node:os"); - const { CLI_VERSION } = await import("../../utils/session.js"); + const { CLI_VERSION } = await import("../../session.js"); const { buildBugReportUrl } = await import( "../../utils/bug-report.js" ); diff --git a/codex-cli/src/components/chat/terminal-chat-past-rollout.tsx b/codex-cli/src/components/chat/terminal-chat-past-rollout.tsx index f041f36f76..d822c0e49e 100644 --- a/codex-cli/src/components/chat/terminal-chat-past-rollout.tsx +++ b/codex-cli/src/components/chat/terminal-chat-past-rollout.tsx @@ -1,4 +1,4 @@ -import type { TerminalChatSession } from "../../utils/session.js"; +import type { TerminalChatSession } from "../../session.js"; import type { ResponseItem } from "openai/resources/responses/responses"; import TerminalChatResponseItem from "./terminal-chat-response-item"; diff --git a/codex-cli/src/components/chat/terminal-chat.tsx b/codex-cli/src/components/chat/terminal-chat.tsx index 998a190cf1..ce200be14d 100644 --- a/codex-cli/src/components/chat/terminal-chat.tsx +++ b/codex-cli/src/components/chat/terminal-chat.tsx @@ -10,6 +10,7 @@ import TerminalMessageHistory from "./terminal-message-history.js"; import { formatCommandForDisplay } from "../../format-command.js"; import { useConfirmation } from "../../hooks/use-confirmation.js"; import { useTerminalSize } from "../../hooks/use-terminal-size.js"; +import { CLI_VERSION } from "../../session.js"; import { AgentLoop } from "../../utils/agent/agent-loop.js"; import { ReviewDecision } from "../../utils/agent/review.js"; import { generateCompactSummary } from "../../utils/compact-summary.js"; @@ -24,7 +25,6 @@ import { uniqueById, } from "../../utils/model-utils.js"; import { createOpenAIClient } from "../../utils/openai-client.js"; -import { CLI_VERSION } from "../../utils/session.js"; import { shortCwd } from "../../utils/short-path.js"; import { saveRollout } from "../../utils/storage/save-rollout.js"; import ApprovalModeOverlay from "../approval-mode-overlay.js"; diff --git a/codex-cli/src/session.ts b/codex-cli/src/session.ts new file mode 100644 index 0000000000..6139c2d717 --- /dev/null +++ b/codex-cli/src/session.ts @@ -0,0 +1,60 @@ +// Note that "../package.json" is marked external in build.mjs. This ensures +// that the contents of package.json will always be read at runtime, which is +// preferable so we do not have to make a temporary change to package.json in +// the source tree to update the version number in the code. +import pkg from "../package.json" with { type: "json" }; + +// Read the version directly from package.json. +export const CLI_VERSION: string = (pkg as { version: string }).version; +export const ORIGIN = "codex_cli_ts"; + +export type TerminalChatSession = { + /** Globally unique session identifier */ + id: string; + /** The OpenAI username associated with this session */ + user: string; + /** Version identifier of the Codex CLI that produced the session */ + version: string; + /** The model used for the conversation */ + model: string; + /** ISO timestamp noting when the session was persisted */ + timestamp: string; + /** Optional custom instructions that were active for the run */ + instructions: string; +}; + +let sessionId = ""; + +/** + * Update the globally tracked session identifier. + * Passing an empty string clears the current session. + */ +export function setSessionId(id: string): void { + sessionId = id; +} + +/** + * Retrieve the currently active session identifier, or an empty string when + * no session is active. + */ +export function getSessionId(): string { + return sessionId; +} + +let currentModel = ""; + +/** + * Record the model that is currently being used for the conversation. + * Setting an empty string clears the record so the next agent run can update it. + */ +export function setCurrentModel(model: string): void { + currentModel = model; +} + +/** + * Return the model that was last supplied to {@link setCurrentModel}. + * If no model has been recorded yet, an empty string is returned. + */ +export function getCurrentModel(): string { + return currentModel; +} diff --git a/codex-cli/src/utils/agent/agent-loop.ts b/codex-cli/src/utils/agent/agent-loop.ts index 60749a2389..16e0a3428a 100644 --- a/codex-cli/src/utils/agent/agent-loop.ts +++ b/codex-cli/src/utils/agent/agent-loop.ts @@ -11,6 +11,13 @@ import type { } from "openai/resources/responses/responses.mjs"; import type { Reasoning } from "openai/resources.mjs"; +import { + ORIGIN, + CLI_VERSION, + getSessionId, + setCurrentModel, + setSessionId, +} from "../../session.js"; import { OPENAI_TIMEOUT_MS, OPENAI_ORGANIZATION, @@ -22,13 +29,6 @@ import { import { log } from "../logger/log.js"; import { parseToolCallArguments } from "../parsers.js"; import { responsesCreateViaChatCompletions } from "../responses.js"; -import { - ORIGIN, - CLI_VERSION, - getSessionId, - setCurrentModel, - setSessionId, -} from "../session.js"; import { handleExecCommand } from "./handle-exec-command.js"; import { HttpsProxyAgent } from "https-proxy-agent"; import { randomUUID } from "node:crypto"; diff --git a/codex-cli/src/utils/check-updates.ts b/codex-cli/src/utils/check-updates.ts index 5e326c1c93..c6ce703a0d 100644 --- a/codex-cli/src/utils/check-updates.ts +++ b/codex-cli/src/utils/check-updates.ts @@ -1,7 +1,7 @@ import type { AgentName } from "package-manager-detector"; import { detectInstallerByPath } from "./package-manager-detector"; -import { CLI_VERSION } from "./session"; +import { CLI_VERSION } from "../session"; import boxen from "boxen"; import chalk from "chalk"; import { getLatestVersion } from "fast-npm-meta"; diff --git a/codex-cli/tests/check-updates.test.ts b/codex-cli/tests/check-updates.test.ts index 75ec8aaf4e..e9f62c60c6 100644 --- a/codex-cli/tests/check-updates.test.ts +++ b/codex-cli/tests/check-updates.test.ts @@ -9,7 +9,7 @@ import { renderUpdateCommand, } from "../src/utils/check-updates"; import { detectInstallerByPath } from "../src/utils/package-manager-detector"; -import { CLI_VERSION } from "../src/utils/session"; +import { CLI_VERSION } from "../src/session"; // In-memory FS mock let memfs: Record = {};