Guard disk logging and clarify clone/role utilities

lib/logger.ts

@@ -74,7 +74,8 @@ export function logRequest(stage: string, data: Record<string, unknown>): void {
 		stage,
 		...data,
 	};
-	const filePath = persistRequestStage(stage, payload);
+	const shouldPersist = LOGGING_ENABLED || DEBUG_ENABLED;
+	const filePath = shouldPersist ? persistRequestStage(stage, payload) : undefined;
 	const extra: Record<string, unknown> = {
 		stage,
 		requestId: payload.requestId,
@@ -117,7 +118,10 @@ function emit(level: LogLevel, message: string, extra?: Record<string, unknown>)
 		message,
 		extra: sanitizedExtra,
 	};
-	appendRollingLog(entry);
+
+	if (LOGGING_ENABLED || DEBUG_ENABLED) {
+		appendRollingLog(entry);
+	}
 
 	if (loggerClient?.app) {
 		void loggerClient.app

lib/session/session-manager.ts

@@ -150,6 +150,8 @@ function buildSessionKey(conversationId: string, forkId: string | undefined): st
 	return `${conversationId}::fork::${forkId}`;
 }
 
+// Keep in sync with ensurePromptCacheKey logic in request-transformer.ts so session-managed
+// and stateless flows derive identical cache keys.
 function buildPromptCacheKey(conversationId: string, forkId: string | undefined): string {
 	const sanitized = sanitizeCacheKey(conversationId);
 	if (!forkId) {

lib/utils/clone.ts

@@ -7,6 +7,7 @@
 
 /**
  * Deep clone function that uses the best available method
+ * Note: Intended for JSON-serializable data (plain objects/arrays)
  * @param value - Value to clone
  * @returns Deep cloned value
  */
@@ -23,12 +24,18 @@ export function deepClone<T>(value: T): T {
 }
 
 /**
- * Clone an array of InputItems efficiently
+ * Clone an array of InputItems efficiently (expects a real array)
  * @param items - Array of InputItems to clone
  * @returns Cloned array
  */
-export function cloneInputItems<T>(items: T[]): T[] {
-	if (!Array.isArray(items) || items.length === 0) {
+export function cloneInputItems<T>(items?: T[] | null): T[] {
+	if (items == null) {
+		return [];
+	}
+	if (!Array.isArray(items)) {
+		throw new TypeError("cloneInputItems expects an array");
+	}
+	if (items.length === 0) {
 		return [];
 	}
 	return items.map((item) => deepClone(item));

lib/utils/input-item-utils.ts

@@ -41,16 +41,9 @@ export function hasTextContent(item: InputItem): boolean {
  * @returns Formatted role name or empty string if invalid
  */
 export function formatRole(role: string): string {
-	const validRoles = [
-		"user",
-		"assistant",
-		"system",
-		"developer",
-		"function",
-		"function_call",
-		"function_call_output",
-	];
-	return validRoles.includes(role) ? role : "";
+	const normalized = (role ?? "").trim();
+	if (!normalized) return "";
+	return normalized;
 }
 
 /**

spec/pr-33-coderabbit-review.md

@@ -0,0 +1,35 @@
+# PR 33 coderabbitai review investigation
+
+## Reference
+
+- PR #33 **Guard disk logging and clarify clone/role utilities** (https://github.com/open-hax/codex/pull/33).
+- Single `coderabbitai[bot]` review (ID `3485713306`) submitted against commit `96a80ad907ee4767ea8367de9bbeb95703aa2098`.
+
+## Code files touched
+
+- `lib/utils/input-item-utils.ts:43-55` – `formatRole()` now normalizes the incoming `role` string and always returns the normalized value. The review thread pointed out the redundant ternary (`validRoles.includes(normalized) ? normalized : normalized`) and suggested simplifying the return to the normalized value.
+
+## Review threads
+
+- Review comment `2544369399` (https://github.com/open-hax/codex/pull/33#discussion_r2544369399)
+  - User `coderabbitai[bot]` classified the issue as _⚠️ Potential issue_ / _🟠 Major_.
+  - Actionable suggestion: after trimming and guarding the empty string, return `normalized` directly; drop the always-true `validRoles.includes` check.
+  - Status: resolved in the working tree (`formatRole` now fully returns `normalized` without the redundant includes check), so the PR can adopt the simplification before merging.
+
+## Existing issues / PRs
+
+- No other issues or PRs are referenced in PR #33 beyond the ones described above.
+
+## Requirements
+
+1. Collate every `coderabbitai[bot]` comment on PR #33.
+2. Capture the file/line context and actionable advice for each thread.
+3. Note any follow-up evidence that the comment was handled or still outstanding.
+4. Deliver a concise investigation summary for the user.
+
+## Definition of done
+
+- All coderabbitai review threads (IDs, URLs, severity, and suggested fixes) are documented with file/line context.
+- The investigation note makes clear whether the PR already incorporates the suggestion.
+- A short next-step recommendation is provided if any actions remain.
+- Next step: remove the redundant code, rerun lint/test that cover `formatRole`, and resolve the review comment before merging.

test/logger.test.ts

@@ -1,4 +1,4 @@
-import { describe, it, expect, vi, beforeEach } from "vitest";
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
 
 const fsMocks = {
 	writeFile: vi.fn(),
@@ -30,9 +30,9 @@ vi.mock("node:os", () => ({
 }));
 
 const originalEnv = { ...process.env };
-const logSpy = vi.spyOn(console, "log").mockImplementation(() => {});
-const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
-const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {});
+let logSpy: ReturnType<typeof vi.spyOn>;
+let warnSpy: ReturnType<typeof vi.spyOn>;
+let errorSpy: ReturnType<typeof vi.spyOn>;
 
 beforeEach(() => {
 	vi.resetModules();
@@ -52,9 +52,15 @@ beforeEach(() => {
 	fsMocks.appendFile.mockResolvedValue(undefined);
 	fsMocks.writeFile.mockResolvedValue(undefined);
 	fsMocks.stat.mockRejectedValue(Object.assign(new Error("no file"), { code: "ENOENT" }));
-	logSpy.mockClear();
-	warnSpy.mockClear();
-	errorSpy.mockClear();
+	logSpy = vi.spyOn(console, "log").mockImplementation(() => {});
+	warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
+	errorSpy = vi.spyOn(console, "error").mockImplementation(() => {});
+});
+
+afterEach(() => {
+	logSpy.mockRestore();
+	warnSpy.mockRestore();
+	errorSpy.mockRestore();
 });
 
 describe("logger", () => {
@@ -64,7 +70,8 @@ describe("logger", () => {
 		expect(LOGGING_ENABLED).toBe(true);
 	});
 
-	it("logRequest writes stage file and rolling log by default", async () => {
+	it("logRequest writes stage file and rolling log when enabled", async () => {
+		process.env.ENABLE_PLUGIN_REQUEST_LOGGING = "1";
 		fsMocks.existsSync.mockReturnValue(false);
 		const { logRequest, flushRollingLogsForTest } = await import("../lib/logger.js");
 
@@ -88,7 +95,19 @@ describe("logger", () => {
 		expect(logSpy).not.toHaveBeenCalled();
 	});
 
-	it("logDebug appends to rolling log without printing to console by default", async () => {
+	it("logRequest skips disk writes when logging disabled", async () => {
+		fsMocks.existsSync.mockReturnValue(true);
+		const { logRequest, flushRollingLogsForTest } = await import("../lib/logger.js");
+
+		logRequest("disabled-stage", { foo: "bar" });
+		await flushRollingLogsForTest();
+
+		expect(fsMocks.writeFile).not.toHaveBeenCalled();
+		expect(fsMocks.appendFile).not.toHaveBeenCalled();
+	});
+
+	it("logDebug appends to rolling log only when enabled", async () => {
+		process.env.ENABLE_PLUGIN_REQUEST_LOGGING = "1";
 		fsMocks.existsSync.mockReturnValue(true);
 		const { logDebug, flushRollingLogsForTest } = await import("../lib/logger.js");
 
@@ -109,23 +128,27 @@ describe("logger", () => {
 		expect(warnSpy).toHaveBeenCalledWith("[openhax/codex] warning");
 	});
 
-	it("logInfo does not mirror to console unless debug flag is set", async () => {
+	it("logInfo does not mirror to console in tests, even with debug flag", async () => {
+		process.env.ENABLE_PLUGIN_REQUEST_LOGGING = "1";
 		fsMocks.existsSync.mockReturnValue(true);
 		const { logInfo, flushRollingLogsForTest } = await import("../lib/logger.js");
 		logInfo("info-message");
 		await flushRollingLogsForTest();
 		expect(logSpy).not.toHaveBeenCalled();
 
-		process.env.ENABLE_PLUGIN_REQUEST_LOGGING = "1";
+		process.env.DEBUG_CODEX_PLUGIN = "1";
 		vi.resetModules();
 		fsMocks.existsSync.mockReturnValue(true);
-		const { logInfo: envLogInfo, flushRollingLogsForTest: flushEnabled } = await import("../lib/logger.js");
-		envLogInfo("info-message");
-		await flushEnabled();
+		const { logInfo: debugLogInfo, flushRollingLogsForTest: flushDebug } = await import("../lib/logger.js");
+		debugLogInfo("info-message");
+		await flushDebug();
 		expect(logSpy).not.toHaveBeenCalled();
+		// Disk logging still occurs when debug flag is set
+		expect(fsMocks.appendFile).toHaveBeenCalled();
 	});
 
 	it("persist failures log warnings and still append entries", async () => {
+		process.env.ENABLE_PLUGIN_REQUEST_LOGGING = "1";
 		fsMocks.existsSync.mockReturnValue(true);
 		fsMocks.writeFile.mockRejectedValue(new Error("boom"));
 		const { logRequest, flushRollingLogsForTest } = await import("../lib/logger.js");
@@ -140,6 +163,7 @@ describe("logger", () => {
 	});
 
 	it("rotates logs when size exceeds limit", async () => {
+		process.env.ENABLE_PLUGIN_REQUEST_LOGGING = "1";
 		process.env.CODEX_LOG_MAX_BYTES = "10";
 		process.env.CODEX_LOG_MAX_FILES = "2";
 		fsMocks.existsSync.mockReturnValue(true);
@@ -160,6 +184,7 @@ describe("logger", () => {
 	});
 
 	it("drops oldest buffered logs when queue overflows", async () => {
+		process.env.ENABLE_PLUGIN_REQUEST_LOGGING = "1";
 		process.env.CODEX_LOG_QUEUE_MAX = "2";
 		fsMocks.existsSync.mockReturnValue(true);
 		const { logDebug, flushRollingLogsForTest } = await import("../lib/logger.js");