diff --git a/lib/logger.ts b/lib/logger.ts index 5916c7c..24d4f06 100644 --- a/lib/logger.ts +++ b/lib/logger.ts @@ -74,7 +74,8 @@ export function logRequest(stage: string, data: Record): void { stage, ...data, }; - const filePath = persistRequestStage(stage, payload); + const shouldPersist = LOGGING_ENABLED || DEBUG_ENABLED; + const filePath = shouldPersist ? persistRequestStage(stage, payload) : undefined; const extra: Record = { stage, requestId: payload.requestId, @@ -117,7 +118,10 @@ function emit(level: LogLevel, message: string, extra?: Record) message, extra: sanitizedExtra, }; - appendRollingLog(entry); + + if (LOGGING_ENABLED || DEBUG_ENABLED) { + appendRollingLog(entry); + } if (loggerClient?.app) { void loggerClient.app diff --git a/lib/session/session-manager.ts b/lib/session/session-manager.ts index 88793ba..3d2eaf1 100644 --- a/lib/session/session-manager.ts +++ b/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) { diff --git a/lib/utils/clone.ts b/lib/utils/clone.ts index ffc7623..8cc8bec 100644 --- a/lib/utils/clone.ts +++ b/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(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(items: T[]): T[] { - if (!Array.isArray(items) || items.length === 0) { +export function cloneInputItems(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)); diff --git a/lib/utils/input-item-utils.ts b/lib/utils/input-item-utils.ts index cc99127..8fee2e4 100644 --- a/lib/utils/input-item-utils.ts +++ b/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; } /** diff --git a/spec/pr-33-coderabbit-review.md b/spec/pr-33-coderabbit-review.md new file mode 100644 index 0000000..77d615a --- /dev/null +++ b/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. diff --git a/test/logger.test.ts b/test/logger.test.ts index 3bd62ad..5189854 100644 --- a/test/logger.test.ts +++ b/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; +let warnSpy: ReturnType; +let errorSpy: ReturnType; 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");