valknar/llmx
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: remove unnecessary isLoggingEnabled() checks (#420)

Browse Source 
It appears that use of `isLoggingEnabled()` was erroneously copypasta'd
in many places. This PR updates its docstring to clarify that should
only be used to avoid constructing a potentially expensive docstring.
With this change, the only function that merits/uses this check is
`execCommand()`.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/420).
* #423
* __->__ #420
* #419
This commit is contained in:
Michael Bolin
2025-04-20 09:58:06 -07:00
committed by GitHub
parent 81cf47e591
commit b554b522f7
10 changed files with 199 additions and 192 deletions
Download Patch File Download Diff File Expand all files Collapse all files

58
codex-cli/src/utils/agent/agent-loop.ts
View File

@@ -8,7 +8,7 @@ import type {
} from "openai/resources/responses/responses.mjs";
import type { Reasoning } from "openai/resources.mjs";
import { log, isLoggingEnabled } from "./log.js";
import { log } from "./log.js";
import { OPENAI_BASE_URL, OPENAI_TIMEOUT_MS } from "../config.js";
import { parseToolCallArguments } from "../parsers.js";
import {
@@ -116,15 +116,13 @@ export class AgentLoop {
// Reset the current stream to allow new requests
this.currentStream = null;
if (isLoggingEnabled()) {
log(
`AgentLoop.cancel() invoked – currentStream=${Boolean(
this.currentStream,
)} execAbortController=${Boolean(
this.execAbortController,
)} generation=${this.generation}`,
);
}
log(
`AgentLoop.cancel() invoked – currentStream=${Boolean(
this.currentStream,
)} execAbortController=${Boolean(this.execAbortController)} generation=${
this.generation
}`,
);
(
this.currentStream as { controller?: { abort?: () => void } } | null
)?.controller?.abort?.();
@@ -136,9 +134,7 @@ export class AgentLoop {
// Create a new abort controller for future tool calls
this.execAbortController = new AbortController();
if (isLoggingEnabled()) {
log("AgentLoop.cancel(): execAbortController.abort() called");
}
log("AgentLoop.cancel(): execAbortController.abort() called");
// NOTE: We intentionally do *not* clear `lastResponseId` here. If the
// stream produced a `function_call` before the user cancelled, OpenAI now
@@ -174,9 +170,7 @@ export class AgentLoop {
// this.onItem(cancelNotice);
this.generation += 1;
if (isLoggingEnabled()) {
log(`AgentLoop.cancel(): generation bumped to ${this.generation}`);
}
log(`AgentLoop.cancel(): generation bumped to ${this.generation}`);
}
/**
@@ -315,13 +309,11 @@ export class AgentLoop {
const callId: string = (item as any).call_id ?? (item as any).id;
const args = parseToolCallArguments(rawArguments ?? "{}");
if (isLoggingEnabled()) {
log(
`handleFunctionCall(): name=${
name ?? "undefined"
} callId=${callId} args=${rawArguments}`,
);
}
log(
`handleFunctionCall(): name=${
name ?? "undefined"
} callId=${callId} args=${rawArguments}`,
);
if (args == null) {
const outputItem: ResponseInputItem.FunctionCallOutput = {
@@ -407,11 +399,9 @@ export class AgentLoop {
// Create a fresh AbortController for this run so that tool calls from a
// previous run do not accidentally get signalled.
this.execAbortController = new AbortController();
if (isLoggingEnabled()) {
log(
`AgentLoop.run(): new execAbortController created (${this.execAbortController.signal}) for generation ${this.generation}`,
);
}
log(
`AgentLoop.run(): new execAbortController created (${this.execAbortController.signal}) for generation ${this.generation}`,
);
// NOTE: We no longer (re‑)attach an `abort` listener to `hardAbort` here.
// A single listener that forwards the `abort` to the current
// `execAbortController` is installed once in the constructor. Re‑adding a
@@ -502,11 +492,9 @@ export class AgentLoop {
const mergedInstructions = [prefix, this.instructions]
.filter(Boolean)
.join("\n");
if (isLoggingEnabled()) {
log(
`instructions (length ${mergedInstructions.length}): ${mergedInstructions}`,
);
}
log(
`instructions (length ${mergedInstructions.length}): ${mergedInstructions}`,
);
// eslint-disable-next-line no-await-in-loop
stream = await this.oai.responses.create({
model: this.model,
@@ -733,9 +721,7 @@ export class AgentLoop {
try {
// eslint-disable-next-line no-await-in-loop
for await (const event of stream) {
if (isLoggingEnabled()) {
log(`AgentLoop.run(): response event ${event.type}`);
}
log(`AgentLoop.run(): response event ${event.type}`);
// process and surface each item (no‑op until we can depend on streaming events)
if (event.type === "response.output_item.done") {

33
codex-cli/src/utils/agent/handle-exec-command.ts
View File

@@ -223,23 +223,22 @@ async function execCommand(
workdir = process.cwd();
}
}
if (isLoggingEnabled()) {
if (applyPatchCommand != null) {
log("EXEC running apply_patch command");
} else {
const { cmd, timeoutInMillis } = execInput;
// Seconds are a bit easier to read in log messages and most timeouts
// are specified as multiples of 1000, anyway.
const timeout =
timeoutInMillis != null
? Math.round(timeoutInMillis / 1000).toString()
: "undefined";
log(
`EXEC running \`${formatCommandForDisplay(
cmd,
)}\` in workdir=${workdir} with timeout=${timeout}s`,
);
}
if (applyPatchCommand != null) {
log("EXEC running apply_patch command");
} else if (isLoggingEnabled()) {
const { cmd, timeoutInMillis } = execInput;
// Seconds are a bit easier to read in log messages and most timeouts
// are specified as multiples of 1000, anyway.
const timeout =
timeoutInMillis != null
? Math.round(timeoutInMillis / 1000).toString()
: "undefined";
log(
`EXEC running \`${formatCommandForDisplay(
cmd,
)}\` in workdir=${workdir} with timeout=${timeout}s`,
);
}
// Note execApplyPatch() and exec() are coded defensively and should not

8
codex-cli/src/utils/agent/log.ts
View File

@@ -124,6 +124,14 @@ export function log(message: string): void {
(logger ?? initLogger()).log(message);
}
/**
* USE SPARINGLY! This function should only be used to guard a call to log() if
* the log message is large and you want to avoid constructing it if logging is
* disabled.
*
* `log()` is already a no-op if DEBUG is not set, so an extra
* `isLoggingEnabled()` check is unnecessary.
*/
export function isLoggingEnabled(): boolean {
return (logger ?? initLogger()).isLoggingEnabled();
}

10
codex-cli/src/utils/agent/platform-commands.ts
View File

@@ -2,7 +2,7 @@
* Utility functions for handling platform-specific commands
*/
import { log, isLoggingEnabled } from "./log.js";
import { log } from "./log.js";
/**
* Map of Unix commands to their Windows equivalents
@@ -59,9 +59,7 @@ export function adaptCommandForPlatform(command: Array<string>): Array<string> {
return command;
}
if (isLoggingEnabled()) {
log(`Adapting command '${cmd}' for Windows platform`);
}
log(`Adapting command '${cmd}' for Windows platform`);
// Create a new command array with the adapted command
const adaptedCommand = [...command];
@@ -78,9 +76,7 @@ export function adaptCommandForPlatform(command: Array<string>): Array<string> {
}
}
if (isLoggingEnabled()) {
log(`Adapted command: ${adaptedCommand.join(" ")}`);
}
log(`Adapted command: ${adaptedCommand.join(" ")}`);
return adaptedCommand;
}

149
codex-cli/src/utils/agent/sandbox/raw-exec.ts
View File

@@ -7,12 +7,15 @@ import type {
StdioPipe,
} from "child_process";
import { log, isLoggingEnabled } from "../log.js";
import { log } from "../log.js";
import { adaptCommandForPlatform } from "../platform-commands.js";
import { spawn } from "child_process";
import * as os from "os";
const MAX_BUFFER = 1024 * 100; // 100 KB
// Maximum output cap: either MAX_OUTPUT_LINES lines or MAX_OUTPUT_BYTES bytes,
// whichever limit is reached first.
const MAX_OUTPUT_BYTES = 1024 * 10; // 10 KB
const MAX_OUTPUT_LINES = 256;
/**
* This function should never return a rejected promise: errors should be
@@ -27,10 +30,7 @@ export function exec(
// Adapt command for the current platform (e.g., convert 'ls' to 'dir' on Windows)
const adaptedCommand = adaptCommandForPlatform(command);
if (
isLoggingEnabled() &&
JSON.stringify(adaptedCommand) !== JSON.stringify(command)
) {
if (JSON.stringify(adaptedCommand) !== JSON.stringify(command)) {
log(
`Command adapted for platform: ${command.join(
" ",
@@ -95,9 +95,7 @@ export function exec(
// timely fashion.
if (abortSignal) {
const abortHandler = () => {
if (isLoggingEnabled()) {
log(`raw-exec: abort signal received – killing child ${child.pid}`);
}
log(`raw-exec: abort signal received – killing child ${child.pid}`);
const killTarget = (signal: NodeJS.Signals) => {
if (!child.pid) {
return;
@@ -148,37 +146,14 @@ export function exec(
// resolve the promise and translate the failure into a regular
// ExecResult object so the rest of the agent loop can carry on gracefully.
const stdoutChunks: Array<Buffer> = [];
const stderrChunks: Array<Buffer> = [];
let numStdoutBytes = 0;
let numStderrBytes = 0;
let hitMaxStdout = false;
let hitMaxStderr = false;
return new Promise<ExecResult>((resolve) => {
child.stdout?.on("data", (data: Buffer) => {
if (!hitMaxStdout) {
numStdoutBytes += data.length;
if (numStdoutBytes <= MAX_BUFFER) {
stdoutChunks.push(data);
} else {
hitMaxStdout = true;
}
}
});
child.stderr?.on("data", (data: Buffer) => {
if (!hitMaxStderr) {
numStderrBytes += data.length;
if (numStderrBytes <= MAX_BUFFER) {
stderrChunks.push(data);
} else {
hitMaxStderr = true;
}
}
});
// Collect stdout and stderr up to configured limits.
const stdoutCollector = createTruncatingCollector(child.stdout!);
const stderrCollector = createTruncatingCollector(child.stderr!);
child.on("exit", (code, signal) => {
const stdout = Buffer.concat(stdoutChunks).toString("utf8");
const stderr = Buffer.concat(stderrChunks).toString("utf8");
const stdout = stdoutCollector.getString();
const stderr = stderrCollector.getString();
// Map (code, signal) to an exit code. We expect exactly one of the two
// values to be non-null, but we code defensively to handle the case where
@@ -194,24 +169,104 @@ export function exec(
exitCode = 1;
}
if (isLoggingEnabled()) {
log(
`raw-exec: child ${child.pid} exited code=${exitCode} signal=${signal}`,
);
}
resolve({
log(
`raw-exec: child ${child.pid} exited code=${exitCode} signal=${signal}`,
);
const execResult = {
stdout,
stderr,
exitCode,
});
};
resolve(
addTruncationWarningsIfNecessary(
execResult,
stdoutCollector.hit,
stderrCollector.hit,
),
);
});
child.on("error", (err) => {
resolve({
const execResult = {
stdout: "",
stderr: String(err),
exitCode: 1,
});
};
resolve(
addTruncationWarningsIfNecessary(
execResult,
stdoutCollector.hit,
stderrCollector.hit,
),
);
});
});
}
/**
* Creates a collector that accumulates data Buffers from a stream up to
* specified byte and line limits. After either limit is exceeded, further
* data is ignored.
*/
function createTruncatingCollector(
stream: NodeJS.ReadableStream,
byteLimit: number = MAX_OUTPUT_BYTES,
lineLimit: number = MAX_OUTPUT_LINES,
) {
const chunks: Array<Buffer> = [];
let totalBytes = 0;
let totalLines = 0;
let hitLimit = false;
stream?.on("data", (data: Buffer) => {
if (hitLimit) {
return;
}
totalBytes += data.length;
for (let i = 0; i < data.length; i++) {
if (data[i] === 0x0a) {
totalLines++;
}
}
if (totalBytes <= byteLimit && totalLines <= lineLimit) {
chunks.push(data);
} else {
hitLimit = true;
}
});
return {
getString() {
return Buffer.concat(chunks).toString("utf8");
},
/** True if either byte or line limit was exceeded */
get hit(): boolean {
return hitLimit;
},
};
}
/**
* Adds a truncation warnings to stdout and stderr, if appropriate.
*/
function addTruncationWarningsIfNecessary(
execResult: ExecResult,
hitMaxStdout: boolean,
hitMaxStderr: boolean,
): ExecResult {
if (!hitMaxStdout && !hitMaxStderr) {
return execResult;
} else {
const { stdout, stderr, exitCode } = execResult;
return {
stdout: hitMaxStdout
? stdout + "\n\n[Output truncated: too many lines or bytes]"
: stdout,
stderr: hitMaxStderr
? stderr + "\n\n[Output truncated: too many lines or bytes]"
: stderr,
exitCode,
};
}
}

14
codex-cli/src/utils/config.ts
View File

@@ -8,7 +8,7 @@
import type { FullAutoErrorMode } from "./auto-approval-mode.js";
import { log, isLoggingEnabled } from "./agent/log.js";
import { log } from "./agent/log.js";
import { AutoApprovalMode } from "./auto-approval-mode.js";
import { existsSync, mkdirSync, readFileSync, writeFileSync } from "fs";
import { load as loadYaml, dump as dumpYaml } from "js-yaml";
@@ -245,15 +245,11 @@ export const loadConfig = (
? resolvePath(cwd, options.projectDocPath)
: discoverProjectDocPath(cwd);
if (projectDocPath) {
if (isLoggingEnabled()) {
log(
`[codex] Loaded project doc from ${projectDocPath} (${projectDoc.length} bytes)`,
);
}
log(
`[codex] Loaded project doc from ${projectDocPath} (${projectDoc.length} bytes)`,
);
} else {
if (isLoggingEnabled()) {
log(`[codex] No project doc found in ${cwd}`);
}
log(`[codex] No project doc found in ${cwd}`);
}
}