Fix CodeRabbit review issues from PR #29

lib/auth/server.ts

@@ -10,9 +10,14 @@ const __dirname = path.dirname(fileURLToPath(import.meta.url));
 const successHtml = fs.readFileSync(path.join(__dirname, "..", "oauth-success.html"), "utf-8");
 
 /**
- * Start a small local HTTP server that waits for /auth/callback and returns the code
- * @param options - OAuth state for validation
- * @returns Promise that resolves to server info
+ * Start a local HTTP listener that captures the OAuth authorization code from /auth/callback.
+ *
+ * @param options - Configuration object.
+ * @param options.state - Expected `state` query parameter value used to validate the callback.
+ * @returns An object containing:
+ *  - `port`: the bound port number (1455),
+ *  - `close()`: a function that closes the server,
+ *  - `waitForCode(...)`: waits up to ~60 seconds for an authorization code; validation always uses the configured `options.state` (the optional argument is accepted only for API symmetry); returns `{ code: string }` when a code matching that state is captured, or `null` on timeout.
  */
 export function startLocalOAuthServer({ state }: { state: string }): Promise<OAuthServerInfo> {
 	const server = http.createServer((req, res) => {
@@ -53,7 +58,7 @@ export function startLocalOAuthServer({ state }: { state: string }): Promise<OAu
 				resolve({
 					port: 1455,
 					close: () => server.close(),
-					waitForCode: async () => {
+					waitForCode: async (_expectedState?: string) => {
 						const poll = () => new Promise<void>((r) => setTimeout(r, 100));
 						for (let i = 0; i < 600; i++) {
 							const lastCode = (server as http.Server & { _lastCode?: string })._lastCode;

lib/logger.ts

@@ -23,6 +23,21 @@
 	directory?: string;
 };
 
+type OpencodeClientWithTui = OpencodeClient & {
+	tui?: {
+		showToast?: (args: { message: string; variant?: "success" | "error" | "warning" | "info" }) => void;
+	};
+};
+
+function hasTuiShowToast(client: OpencodeClient): client is OpencodeClientWithTui {
+	return (
+		"tui" in client &&
+		typeof client.tui === "object" &&
+		client.tui !== null &&
+		typeof client.tui?.showToast === "function"
+	);
+}
+
 type RollingLogEntry = {
 	timestamp: string;
 	service: string;
@@ -123,7 +138,7 @@
 		appendRollingLog(entry);
 	}
 
-	if (loggerClient?.app) {
+	if (loggerClient?.app?.log) {
 		void loggerClient.app
 			.log({
 				body: entry,
@@ -143,44 +158,64 @@
 	logToConsole(level, message, sanitizedExtra);
 }
 
+/**
+ * Sends a user-facing notification (toast) through the configured logger client, if available.
+ *
+ * Constructs a payload with a title derived from the log level, the provided message as the body,
+ * and optional extra metadata, then attempts to call `app.notify` or `app.toast`. If no app or
+ * compatible send method is present, the function returns without action. Failures to send are
+ * recorded as a warning via console logging.
+ *
+ * @param level - The severity level for the notification (`"debug" | "info" | "warn" | "error"`). A value of `"error"` produces an "error" title; other values produce a "warning" title.
+ * @param message - The primary text to show in the notification body.
+ * @param extra - Optional metadata to include with the notification payload.
+ */
 function notifyToast(level: LogLevel, message: string, extra?: Record<string, unknown>): void {
-	const app = (loggerClient as any)?.app;
-	if (!app) return;
+	if (!loggerClient?.tui?.showToast) return;
 
-	const payload = {
-		title: level === "error" ? `${PLUGIN_NAME} error` : `${PLUGIN_NAME} warning`,
-		body: message,
-		level,
-		extra,
-	};
-	// For Opencode SDK compatibility, also allow notify({ title, body, level }) shape
+	const variant = level === "error" ? "error" : "warning";
 
-	const notify = typeof app.notify === "function" ? app.notify.bind(app) : undefined;
-	const toast = typeof app.toast === "function" ? app.toast.bind(app) : undefined;
-	const send = notify ?? toast;
-	if (!send) return;
-
-	void send(payload).catch((err: unknown) => {
+	try {
+		void loggerClient.tui.showToast({
+			body: {
+				title: level === "error" ? `${PLUGIN_NAME} error` : `${PLUGIN_NAME} warning`,
+				message: `${PLUGIN_NAME}: ${message}`,
+				variant,
+			},
+		});
+	} catch (err: unknown) {
 		logToConsole("warn", "Failed to send plugin toast", { error: toErrorMessage(err) });
-	});
+	}
 }
 
-function logToConsole(
-	level: LogLevel,
-	message: string,
-	extra?: Record<string, unknown>,
-	error?: unknown,
-): void {
+/**
+ * Writes a plugin-prefixed log message to the console when the log level is applicable.
+ *
+ * Logs warnings and errors unconditionally; debug and info messages are written only when console logging is enabled. The message is prefixed with the plugin name and, if provided, `extra` is JSON-stringified and appended; on JSON serialization failure, `String(extra)` is appended instead.
+ *
+ * @param level - Log level determining severity and console method
+ * @param message - Primary log message text
+ * @param extra - Additional context appended to the message; values are JSON-stringified when possible
+ */
+function logToConsole(level: LogLevel, message: string, extra?: Record<string, unknown>): void {
 	const isWarnOrError = level === "warn" || level === "error";
 	const shouldLogDebugOrInfo = CONSOLE_LOGGING_ENABLED && (level === "debug" || level === "info");
 	const shouldLog = isWarnOrError || shouldLogDebugOrInfo;
 	if (!shouldLog) {
 		return;
 	}
 	const prefix = `[${PLUGIN_NAME}] ${message}`;
-	const details = extra ? `${prefix} ${JSON.stringify(extra)}` : prefix;
+	let details = prefix;
+	if (extra) {
+		try {
+			details = `${prefix} ${JSON.stringify(extra)}`;
+		} catch {
+			// Fallback to a best-effort representation instead of throwing from logging
+			details = `${prefix} ${String(extra)}`;
+		}
+	}
 	if (level === "error") {
-		console.error(details, error ?? "");
+		console.error(details);
 		return;
 	}
 	if (level === "warn") {

lib/prompts/codex.ts

@@ -3,7 +3,7 @@
 import { fileURLToPath } from "node:url";
 import { recordCacheHit, recordCacheMiss } from "../cache/cache-metrics.js";
 import { codexInstructionsCache, getCodexCacheKey } from "../cache/session-cache.js";
-import { logError } from "../logger.js";
+import { logError, logWarn } from "../logger.js";
 import type { CacheMetadata, GitHubRelease } from "../types.js";
 import { CACHE_FILES, CACHE_TTL_MS } from "../utils/cache-config.js";
 import {
@@ -46,7 +46,7 @@
  * Rate limit protection: Only checks GitHub if cache is older than 15 minutes
  * @returns Codex instructions
  */
 export async function getCodexInstructions(): Promise<string> {
 	const sessionEntry = codexInstructionsCache.get("latest");
 	if (sessionEntry) {
 		recordCacheHit("codexInstructions");
@@ -87,7 +87,18 @@
 		return fileContent;
 	}
 
-	const latestTag = await getLatestReleaseTag();
+	let latestTag: string | undefined;
+	try {
+		latestTag = await getLatestReleaseTag();
+	} catch (error) {
+		// If we can't get the latest tag, fall back to cache or bundled version
+		logWarn("Failed to get latest release tag, falling back to cache/bundled", { error });
+		// Fall back to bundled instructions
+		const bundledContent = readFileSync(join(__dirname, "codex-instructions.md"), "utf8");
+		cacheSessionEntry(bundledContent, undefined, undefined);
+		return bundledContent;
+	}
+
 	const cacheKeyForLatest = getCodexCacheKey(cachedETag ?? undefined, latestTag);
 	const sessionForLatest = codexInstructionsCache.get(cacheKeyForLatest);
 	if (sessionForLatest) {

lib/request/fetch-helpers.ts

@@ -108,7 +108,7 @@
  * @param codexMode - Enable CODEX_MODE (bridge prompt instead of tool remap)
  * @returns Transformed body and updated init, or undefined if no body
  */
 export async function transformRequestForCodex(
 	init: RequestInit | undefined,
 	url: string,
 	codexInstructions: string,
@@ -248,11 +248,12 @@
 }
 
 /**
- * Handles error responses from the Codex API
- * @param response - Error response from API
- * @returns Response with error details
+ * Enriches a Codex API error Response with structured error details and rate-limit metadata.
+ *
+ * @param response - The original error Response from a Codex API request
+ * @returns A Response with the same status and statusText whose body is either the original raw body or a JSON object containing an `error` object with `message`, optional `friendly_message`, optional `rate_limits`, and `status`. When the body is enriched, the response `Content-Type` is set to `application/json; charset=utf-8`.
  */
 export async function handleErrorResponse(response: Response): Promise<Response> {
 	const raw = await response.text();
 
 	let enriched = raw;
@@ -323,7 +324,11 @@
 	logError(`${response.status} error`, { body: enriched });
 
 	const headers = new Headers(response.headers);
-	headers.set("content-type", "application/json; charset=utf-8");
+	// Only set JSON content-type if we successfully enriched the response
+	// Otherwise preserve the original content-type for non-JSON responses
+	if (enriched !== raw) {
+		headers.set("content-type", "application/json; charset=utf-8");
+	}
 	return new Response(enriched, {
 		status: response.status,
 		statusText: response.statusText,
@@ -364,4 +369,4 @@
 	if (v == null) return undefined;
 	const n = parseInt(v, 10);
 	return Number.isFinite(n) ? n : undefined;
-}
+}

lib/types.ts

@@ -70,7 +70,7 @@ export interface ReasoningConfig {
 export interface OAuthServerInfo {
 	port: number;
 	close: () => void;
-	waitForCode: (state: string) => Promise<{ code: string } | null>;
+	waitForCode: (_state?: string) => Promise<{ code: string } | null>;
 }
 
 /**

spec/pr-29-review-analysis.md

@@ -0,0 +1,82 @@
+# PR #29 Review Thread Analysis
+
+## Summary
+
+PR #29 has **1 unresolved review thread** from `coderabbitai` containing **19 actionable comments** across multiple categories.
+
+## Action Items by Category
+
+### 🚨 **BLOCKER Issues (Must Fix)**
+
+1. **Content-Type Header Bug** - `lib/request/fetch-helpers.ts:302-308`
+   - **Issue**: `handleErrorResponse` unconditionally sets JSON content-type on potentially non-JSON bodies
+   - **Impact**: Misleads callers, causes `response.json()` parse errors on HTML responses
+   - **Fix**: Preserve original content-type or wrap raw body in JSON envelope
+
+2. **Cache Bypass Bug** - `lib/prompts/codex.ts:90`
+   - **Issue**: `getLatestReleaseTag()` failure bypasses cache/bundled fallbacks
+   - **Impact**: Network failures break the entire fallback chain
+   - **Fix**: Wrap entire setup in try/catch to ensure fallback path
+
+### 🧪 **Test Improvements**
+
+3. **Remove Unused Mocks** - `test/cache-warming.test.ts:118-165`
+   - Remove `mockGetCodexInstructions`/`mockGetOpenCodeCodexPrompt` from `areCachesWarm` tests
+
+4. **Fix Mock Leakage** - `test/index.test.ts:22-28, 93-121`
+   - Reset `sessionManager` instance mocks in `beforeEach` to prevent cross-test leakage
+
+5. **Add Missing Test Case** - `test/codex-fetcher.test.ts`
+   - Add direct `compactionDecision` test case coverage
+
+6. **Fix Redundant Tests** - `test/codex-fetcher.test.ts:272-287`
+   - Either provide distinct inputs for short/long text scenarios or remove redundant test
+
+### 🔧 **Code Quality Improvements**
+
+7. **Logger Hardening** - `lib/logger.ts:138-159`
+   - Add try/catch around `JSON.stringify(extra)` to prevent logging failures
+   - Remove unused `error` parameter from `logToConsole`
+
+### 📊 **Coverage Issues**
+
+8. **Docstring Coverage** - Overall: 46.28% (Required: 80%)
+   - Multiple files need docstring improvements to meet coverage requirements
+
+## Files Requiring Changes
+
+### Critical Files (Blockers)
+
+- `lib/request/fetch-helpers.ts` - Content-type header fix
+- `lib/prompts/codex.ts` - Cache fallback fix
+
+### Test Files
+
+- `test/cache-warming.test.ts` - Remove unused mocks
+- `test/index.test.ts` - Fix mock leakage
+- `test/codex-fetcher.test.ts` - Add missing test case, fix redundancy
+
+### Code Quality
+
+- `lib/logger.ts` - Harden JSON.stringify, remove unused parameter
+
+### Multiple Files (Docstring Coverage)
+
+- Various files need docstring additions to reach 80% coverage
+
+## Priority Order
+
+1. **Blocker fixes** (content-type, cache fallback)
+2. **Test improvements** (mock leakage, missing coverage)
+3. **Code quality** (logger hardening)
+4. **Documentation** (docstring coverage)
+
+## Definition of Done
+
+- [x] Content-type header bug fixed and tested
+- [x] Cache fallback properly handles network failures
+- [x] All test issues resolved
+- [x] Logger hardened against JSON failures
+- [x] Docstring coverage reaches acceptable levels
+- [x] All tests pass (398 passed, 2 skipped)
+- [ ] Code review thread resolved

spec/wait-for-code-doc.md

@@ -0,0 +1,37 @@
+# waitForCode JSDoc Clarification
+
+## Context
+
+`startLocalOAuthServer` exposes `waitForCode(expectedState?)` but the optional argument is ignored; documentation currently implies it validates state. Need to clarify docs so readers know validation uses `options.state` and the parameter exists only for API symmetry.
+
+## Code References
+
+- `lib/auth/server.ts:12-21` – JSDoc describing `waitForCode` with `expectedState?` bullet.
+- `lib/auth/server.ts:61-69` – `waitForCode` implementation ignoring `_expectedState` and returning `{ code } | null`.
+
+## Existing Issues / PRs
+
+- Open issues (`gh issue list --limit 5` on 2025-11-20): #26, #25, #24, #23, #22 – none cover waitForCode docs.
+- Open PRs (`gh pr list --limit 5` on 2025-11-20): #34, #29 – unrelated to waitForCode docs.
+
+## Requirements
+
+1. Update JSDoc to state state validation uses the configured `options.state`; the optional argument is accepted only for API symmetry (or omit its name from the bullet).
+2. Adjust return/behavior description to say it returns `{ code }` when a code matching the configured state is captured or `null` on timeout.
+3. Keep implementation unchanged.
+
+## Definition of Done
+
+- JSDoc accurately reflects state validation source and return behavior for `waitForCode`.
+- No code logic changes; only documentation updates in `lib/auth/server.ts`.
+- Quick reread confirms wording is clear and non-misleading.
+
+## Plan
+
+### Phase 1 – Implementation
+
+- Edit `lib/auth/server.ts` JSDoc to clarify state validation source and optional argument purpose; update return description accordingly.
+
+### Phase 2 – Verification
+
+- Re-read updated JSDoc for clarity and correctness; ensure no code changes introduced.