Fix CodeRabbit review issues from PR #29#34
Conversation
## Critical Bug Fixes - Fix content-type header bug in fetch-helpers.ts - preserve original content-type for non-JSON responses - Fix cache fallback bug in codex.ts - wrap getLatestReleaseTag() in try/catch to ensure fallback chain works ## Test Improvements - Remove unused mocks in cache-warming.test.ts areCachesWarm tests - Fix mock leakage in index.test.ts by resetting sessionManager instance mocks - Add missing compactionDecision test case in codex-fetcher.test.ts - Remove redundant test case in codex-fetcher.test.ts ## Code Quality - Harden logger against JSON.stringify failures with try/catch fallback - Remove unused error parameter from logToConsole function - Update type signatures to match new function signatures ## Documentation - Add comprehensive PR analysis document in spec/pr-29-review-analysis.md All tests pass (398 passed, 2 skipped) with 82.73% coverage.
|
Caution Review failedThe pull request is closed. Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRelaxed the OAuth server API (waitForCode now optionally accepts a state param), added app-facing toast notifications and refactored console logging, added codex instruction fallback, enriched API error responses with rate-limit info, and updated related tests and mocks. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Browser as Browser
participant LocalServer as startLocalOAuthServer
participant OAuth as OAuth Provider
participant App as App TUI
note right of LocalServer `#eef2ff`: startLocalOAuthServer opens a local HTTP listener\nreturns { port, close, waitForCode(_expectedState?) }
Dev->>LocalServer: startLocalOAuthServer(opts{state})
Browser->>OAuth: user authorizes (redirect to callback)
OAuth-->>Browser: GET /auth/callback?code=ABC&state=XYZ
Browser->>LocalServer: Request /auth/callback (code, state)
LocalServer->>LocalServer: validate state (closure-held) and store code
LocalServer-->>Browser: serve success HTML
Dev->>LocalServer: await waitForCode(_expectedState?)
LocalServer-->>Dev: return { code: "ABC" } or null on timeout
alt error emitted
LocalServer->>App: emit error -> notifyToast() -> app.tui.showToast
App-->>LocalServer: toast displayed / error handled
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas needing extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
Comment |
|
Note Docstrings generation - SUCCESS |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/codex-fetcher.test.ts (1)
61-78: Add mock reset for finalizeCompactionResponseMock.The
finalizeCompactionResponseMockis not being reset in thebeforeEachblock, which could lead to test isolation issues if mock state leaks between test cases.Apply this diff to add the missing reset:
maybeHandleCodexCommandMock.mockReset(); maybeHandleCodexCommandMock.mockReturnValue(null); logRequestMock.mockClear(); recordSessionResponseMock.mockReset(); + finalizeCompactionResponseMock.mockReset(); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
spec/pr-29-review-analysis.mdis excluded by none and included by none
📒 Files selected for processing (10)
lib/auth/server.ts(1 hunks)lib/logger.ts(1 hunks)lib/prompts/codex.ts(2 hunks)lib/request/fetch-helpers.ts(1 hunks)lib/types.ts(1 hunks)test/auth.test.ts(1 hunks)test/cache-warming.test.ts(0 hunks)test/codex-fetcher.test.ts(3 hunks)test/index.test.ts(1 hunks)test/prompts-codex.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- test/cache-warming.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
lib/prompts/codex.ts (1)
lib/logger.ts (1)
logWarn(97-99)
lib/logger.ts (1)
lib/constants.ts (1)
PLUGIN_NAME(7-7)
test/codex-fetcher.test.ts (1)
lib/request/codex-fetcher.ts (1)
createCodexFetcher(32-115)
🪛 GitHub Check: Lint & Typecheck
lib/auth/server.ts
[warning] 56-56:
'expectedState' is defined but never used. Allowed unused args must match /^_/u
🔇 Additional comments (9)
lib/logger.ts (1)
168-194: LGTM! Defensive error handling in logger.The changes improve robustness:
- Removed unused optional error parameter from the signature
- Added try/catch around
JSON.stringify()to prevent the logger from throwing when serializing circular references or other problematic objects- Fallback to
String(extra)ensures logging always succeedslib/request/fetch-helpers.ts (1)
326-330: LGTM! Preserves original content-type for non-JSON errors.The conditional logic correctly sets the JSON content-type only when the error response was successfully enriched with additional fields. Non-JSON error bodies retain their original content-type.
test/index.test.ts (1)
119-121: LGTM! Proper test cleanup.Adding mock resets for sessionManager methods ensures clean state between tests and prevents test pollution.
test/auth.test.ts (1)
220-222: LGTM! Test assertions aligned with logger changes.Console.error expectations updated to match the single-argument signature from the logger changes. This correctly reflects the composed details string logged by
logToConsole.Also applies to: 233-235, 245-247, 286-288, 295-297, 308-310, 319-321
test/prompts-codex.test.ts (1)
246-246: LGTM! Console assertion aligned with logger changes.The expectation now correctly matches the single-argument
console.errorcall from the updated logger.lib/prompts/codex.ts (1)
90-100: LGTM! Good error handling with fallback.The try/catch around
getLatestReleaseTag()ensures network failures fall back gracefully to bundled instructions. The early return at line 99 ensureslatestTagis always defined in subsequent code paths.lib/types.ts (1)
73-73: All call sites are compatible with the optional parameter change.Both invocations in the test file pass the
stateparameter explicitly ("state-123"and"expected"), so they remain fully compatible with the optional signature. The change is backward compatible—existing callers continue to work, and the API is now more flexible for future use cases where the state parameter might not be needed.test/codex-fetcher.test.ts (2)
20-20: LGTM!The mock declaration follows the established pattern in the file.
49-52: LGTM!The module mock is correctly structured and consistent with other mocks in the test suite.
Docstrings generation was requested by @riatzukiza. * #34 (comment) The following files were modified: * `lib/auth/server.ts` * `lib/logger.ts` * `lib/request/fetch-helpers.ts`
📝 Add docstrings to `release/review-comments`
## Test Improvements - Enhance compaction decision test in codex-fetcher.test.ts to validate full flow: - Verify recordSessionResponseFromHandledResponse called with compacted response - Verify fetcher returns the compacted response with correct status/body - Ensure complete end-to-end compaction flow validation ## Code Quality - Fix linter warning in lib/auth/server.ts by prefixing unused parameter with underscore - Update corresponding type definition in lib/types.ts to match All tests continue to pass (398 passed, 2 skipped).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/codex-fetcher.test.ts (1)
291-310: Strengthen assertions to verify the complete compaction flow.The test verifies that
finalizeCompactionResponseis called but doesn't confirm the compacted response flows through torecordSessionResponseFromHandledResponseor is returned by the fetcher. Based on the implementation inlib/request/codex-fetcher.ts(lines 73-82), the compacted response should be passed downstream and returned.Apply this diff to add comprehensive assertions:
it("handles compaction decision when present", async () => { const mockDecision = { type: "compact" as const, reason: "test" }; transformRequestForCodexMock.mockResolvedValue({ body: { model: "gpt-5" }, sessionContext: { sessionId: "s-3", enabled: true }, compactionDecision: mockDecision, }); handleSuccessResponseMock.mockResolvedValue(new Response("payload", { status: 200 })); finalizeCompactionResponseMock.mockResolvedValue(new Response("compacted", { status: 200 })); const fetcher = createCodexFetcher(baseDeps()); - await fetcher("https://api.openai.com", {}); + const response = await fetcher("https://api.openai.com", {}); expect(finalizeCompactionResponseMock).toHaveBeenCalledWith({ response: expect.any(Response), decision: mockDecision, sessionManager, sessionContext: { sessionId: "s-3", enabled: true }, }); + expect(recordSessionResponseMock).toHaveBeenCalledWith({ + sessionManager, + sessionContext: { sessionId: "s-3", enabled: true }, + handledResponse: expect.objectContaining({ + status: 200, + }), + }); + expect(await response.text()).toBe("compacted"); });lib/auth/server.ts (1)
61-61: Prefix unused parameter with underscore.The
expectedStateparameter is unused and triggers a lint warning. Prefix it with an underscore to indicate it's intentionally unused.Apply this diff:
- waitForCode: async (expectedState?: string) => { + waitForCode: async (_expectedState?: string) => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
spec/pr-29-review-analysis.mdis excluded by none and included by none
📒 Files selected for processing (10)
lib/auth/server.ts(3 hunks)lib/logger.ts(3 hunks)lib/prompts/codex.ts(2 hunks)lib/request/fetch-helpers.ts(3 hunks)lib/types.ts(1 hunks)test/auth.test.ts(1 hunks)test/cache-warming.test.ts(0 hunks)test/codex-fetcher.test.ts(3 hunks)test/index.test.ts(1 hunks)test/prompts-codex.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- test/cache-warming.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
lib/prompts/codex.ts (1)
lib/logger.ts (1)
logWarn(97-99)
test/codex-fetcher.test.ts (1)
lib/request/codex-fetcher.ts (1)
createCodexFetcher(32-115)
lib/logger.ts (1)
lib/constants.ts (1)
PLUGIN_NAME(7-7)
🪛 GitHub Check: Lint & Typecheck
lib/auth/server.ts
[warning] 61-61:
'expectedState' is defined but never used. Allowed unused args must match /^_/u
🔇 Additional comments (7)
lib/types.ts (1)
73-73: LGTM!Making the
stateparameter optional maintains backwards compatibility while supporting the updated implementation inlib/auth/server.ts.lib/request/fetch-helpers.ts (1)
327-331: LGTM!The conditional Content-Type handling correctly preserves the original Content-Type when enrichment fails, addressing the critical bug fix mentioned in the PR objectives.
test/index.test.ts (1)
119-121: LGTM!Resetting session manager instance mocks in
beforeEachprevents mock leakage between tests, ensuring test isolation.test/prompts-codex.test.ts (1)
246-246: LGTM!Removing the empty string argument aligns with the updated logging behavior in
lib/logger.ts.test/auth.test.ts (1)
220-222: LGTM!Removing empty string arguments from
console.errorcalls aligns with the logging behavior updates inlib/logger.ts.lib/prompts/codex.ts (1)
90-100: LGTM!The try/catch wrapper around
getLatestReleaseTag()ensures network failures gracefully fall back to bundled instructions, addressing a critical bug fix from the PR objectives.lib/logger.ts (1)
189-215: Well-designed defensive logging with clean signature.The signature change (removing the unused
errorparameter) and the addition of try/catch around JSON.stringify are solid improvements. The fallback toString(extra)handles circular references and non-serializable objects gracefully, and the unifieddetailsstring approach is more consistent than the previous multi-argument pattern.
## Type Safety Improvements - Add OpencodeApp type with proper notify/toast method signatures - Add OpencodeClientWithApp intersection type for type-safe app access - Create isOpencodeClientWithApp type guard function - Replace (loggerClient as any)?.app with type-safe guarded access - Update emit function to use type guard for loggerClient.app access ## Benefits - Eliminates unsafe any type casting - Provides compile-time type checking for app property access - Maintains backward compatibility with existing OpencodeClient interface - Follows TypeScript best practices for type guards All tests continue to pass (398 passed, 2 skipped).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/codex-fetcher.test.ts (1)
20-52: Reset compaction mock inbeforeEachto keep tests isolated.
finalizeCompactionResponseMockis created hoisted but never reset inbeforeEach, unlike the other mocks. If any test changes its implementation or relies on call counts, that state will leak into subsequent tests. Adding a reset keeps the setup consistent and avoids subtle test coupling.beforeEach(() => { vi.resetModules(); globalThis.fetch = fetchMock as typeof fetch; fetchMock.mockReset(); fetchMock.mockResolvedValue(new Response("ok", { status: 200 })); shouldRefreshTokenMock.mockReset(); shouldRefreshTokenMock.mockReturnValue(false); refreshAndUpdateTokenMock.mockReset(); transformRequestForCodexMock.mockReset(); createCodexHeadersMock.mockReset(); handleErrorResponseMock.mockReset(); handleSuccessResponseMock.mockReset(); handleSuccessResponseMock.mockResolvedValue(new Response("handled", { status: 200 })); maybeHandleCodexCommandMock.mockReset(); maybeHandleCodexCommandMock.mockReturnValue(null); logRequestMock.mockClear(); recordSessionResponseMock.mockReset(); + finalizeCompactionResponseMock.mockReset(); });lib/logger.ts (1)
165-199: Consider handling async failures fromapp.notify/app.toast, not just sync throws.
notifyToastwrapssend(payload)in atry/catch, but that only catches synchronous exceptions. Ifnotify/toastare (now or later) implemented as async functions that return a Promise, rejections will be unhandled.Once
OpencodeApp.notify/toastare typed asvoid | Promise<unknown>, you can handle both sync and async failures:const result = send(payload); if (result && typeof (result as Promise<unknown>).catch === "function") { (result as Promise<unknown>).catch((err) => { logToConsole("warn", "Failed to send plugin toast", { error: toErrorMessage(err) }); }); }This keeps notifications best‑effort while ensuring any failure path is surfaced via the logger.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
lib/auth/server.ts(3 hunks)lib/logger.ts(4 hunks)lib/types.ts(1 hunks)test/codex-fetcher.test.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/codex-fetcher.test.ts (1)
lib/request/codex-fetcher.ts (1)
createCodexFetcher(32-115)
lib/logger.ts (1)
lib/constants.ts (1)
PLUGIN_NAME(7-7)
🔇 Additional comments (3)
test/codex-fetcher.test.ts (1)
291-324: Compaction decision test now validates the full end‑to‑end flow.This test cleanly exercises the compaction branch: it sets a
compactionDecision, stubsfinalizeCompactionResponseto a distinctcompactedResponse, asserts the finalizer is called with the right context, confirms the recorder sees the compacted response, and verifies the fetcher returns that same compacted payload. This aligns well with the productioncreateCodexFetcherlogic.lib/logger.ts (1)
201-236: Console logging refactor looks correct and safer.The new
logToConsolebehavior (always logging warn/error, gating debug/info onCONSOLE_LOGGING_ENABLED, prefixing withPLUGIN_NAME, and JSON‑stringifyingextrawith a safe fallback) is coherent and defensive. I don’t see functional issues here.lib/types.ts (1)
70-74: waitForCode’s optional_stateparameter aligns cleanly with the server API.Making the
stateargument optional and prefixing it with_keeps the public type compatible with callers while matching the implementation instartLocalOAuthServer. No issues from a typing or behavioral standpoint.
- Replace unsafe type casting with proper optional chaining - Update notifyToast to use correct Opencode SDK API structure - Use client.tui.showToast with proper body object format - Remove unnecessary type guard function - All tests pass and TypeScript compilation succeeds
* device/stealth: commit local changes in submodule * Docs: fix test README package name (#18) * docs: reference @openhax/codex in test README * Delete spec/issue-11-docs-package.md * Add fork-aware prompt_cache_key derivation (#19) * Refactor: Eliminate code duplication and improve maintainability - Create shared clone utility (lib/utils/clone.ts) to eliminate 3+ duplicate implementations - Create InputItemUtils (lib/utils/input-item-utils.ts) for centralized text extraction - Centralize magic numbers in constants with SESSION_CONFIG, CONVERSATION_CONFIG, PERFORMANCE_CONFIG - Add ESLint cognitive complexity rules (max: 15) to prevent future issues - Refactor large functions to use shared utilities, reducing complexity - Update all modules to use centralized utilities and constants - Remove dead code and unused imports - All 123 tests pass, no regressions introduced Code quality improved from B+ to A- with better maintainability. * testing! * Address review: plugin config errors, compaction gating, cache key fallback * Style and metrics cleanup for auth, cache, and sessions * linting and formatting * Finalize PR 20 review: shared clone utils and tests * Fix CI workflow YAML syntax and quoting * update lint rules * allow lint warnings without masking errors * seperate linting and formatting ci * seperated formatting workflow from ci yaml and gave it permissions to edit workflow files * opencode can respond to all pr comments * Fix test/README.md documentation: update test counts and config file paths - Update stale test counts to reflect actual numbers: * auth.test.ts: 16 → 27 tests * config.test.ts: 13 → 16 tests * request-transformer.test.ts: 30 → 123 tests * logger.test.ts: 5 → 7 tests * response-handler.test.ts: unchanged at 10 tests - Fix broken configuration file paths: * config/minimal-opencode.json (was config/minimal-opencode.json) * config/full-opencode.json (was config/full-opencode.json) Both configuration files exist in the config/ directory at repository root. * 0.1.0 * 0.2.0 * docs: update AGENTS.md for gpt-5.1-codex-max support - Update overview to reflect new gpt-5.1-codex-max model as default - Add note about xhigh reasoning effort exclusivity to gpt-5.1-codex-max - Document expanded model lineup matching Codex CLI * chore: add v3.3.0 changelog entry for gpt-5.1-codex-max - Document new Codex Max support with xhigh reasoning - Note configuration changes and sample updates - Record automatic reasoning effort downgrade fix for compatibility * docs: update README for gpt-5.1-codex-max integration - Add gpt-5.1-codex-max configuration with xhigh reasoning support - Update model count from 20 to 21 variants - Expand model comparison table with Codex Max as flagship default - Add note about xhigh reasoning exclusivity and auto-downgrade behavior * config: add gpt-5.1-codex-max to full-opencode.json - Add flagship Codex Max model with 400k context and 128k output limits - Configure with medium reasoning effort as default - Include encrypted_content for stateless operation - Set store: false for ChatGPT backend compatibility * config: update minimal-opencode.json default to gpt-5.1-codex-max - Change default model from gpt-5.1-codex to gpt-5.1-codex-max - Align minimal config with new flagship Codex Max model - Provide users with best-in-class default experience * docs: update CONFIG_FIELDS.md for gpt-5.1-codex-max - Add gpt-5.1-codex-max example configuration - Document xhigh reasoning effort exclusivity and auto-clamping - Remove outdated duplicate key example section - Clean up reasoning effort notes with new xhigh behavior * docs: add persistent logging note to TESTING.md - Document new per-request JSON logging and rolling log files - Note environment variables for enabling live console output - Help developers debug with comprehensive logging capabilities * feat: implement persistent rolling logging in logger.ts - Add rolling log file under ~/.opencode/logs/codex-plugin/ - Write structured JSON entries with timestamps for all log levels - Maintain per-request stage files for detailed debugging - Improve error handling and log forwarding to OpenCode app - Separate console logging controls from file logging * feat: add gpt-5.1-codex-max support to request transformer - Add model normalization for all codex-max variants - Implement xhigh reasoning effort with auto-downgrade for non-max models - Add Codex Max specific reasoning effort validation and normalization - Ensure compatibility with existing model configurations * types: add xhigh reasoning effort to TypeScript interfaces - Add xhigh to ConfigOptions.reasoningEffort union type - Add xhigh to ReasoningConfig.effort union type - Enable type-safe usage of extra high reasoning for gpt-5.1-codex-max * test: add gpt-5.1-codex-max to test-all-models.sh - Add test case for new flagship Codex Max model - Verify medium reasoning effort with auto summary and medium verbosity - Ensure comprehensive testing coverage for all model variants * test: fix codex-fetcher test headers mock - Add default Authorization header to createCodexHeaders mock - Prevent test failures due to missing required headers - Ensure consistent test environment across all test runs * test: update logger tests for persistent rolling logging - Add tests for rolling log file functionality - Update test structure to handle module caching properly - Test console logging behavior with environment variables - Verify error handling for file write failures - Ensure appendFileSync is called for all log entries * test: add appendFileSync mock to plugin-config tests - Add missing appendFileSync mock to prevent test failures - Ensure all file system operations are properly mocked - Maintain test isolation and consistency * test: add appendFileSync mock to prompts-codex tests - Add appendFileSync mock to prevent test failures from logger changes - Clear all mocks properly in beforeEach setup - Ensure test isolation and consistency across test runs * test: add comprehensive fs mocks to prompts-opencode-codex tests - Add existsSync, appendFileSync, writeFileSync, mkdirSync mocks - Clear all mocks in beforeEach for proper test isolation - Prevent test failures from logger persistent logging changes - Ensure consistent test environment across all test files * test: add comprehensive gpt-5.1-codex-max test coverage - Add model normalization tests for all codex-max variants - Test xhigh reasoning effort behavior for codex-max vs other models - Verify reasoning effort downgrade logic (minimal/none → low, xhigh → high) - Add integration tests for transformRequestBody with xhigh reasoning - Ensure complete test coverage for new Codex Max functionality * docs: add specification files for gpt-5.1-codex-max and persistent logging - Add comprehensive spec for Codex Max integration with xhigh reasoning - Document persistent logging requirements and implementation plan - Track requirements, references, and change logs for both features * fix failing tests * fix: implement cache isolation to resolve OAuth plugin conflicts Resolves Issue #25 - Plugin fails with confusing errors if started with the other oauth plugin's cache files **Root Cause**: Both opencode-openai-codex-auth and @openhax/codex plugins used identical cache file names in ~/.opencode/cache/, causing conflicts when switching between plugins. **Solution**: 1. **Cache Isolation** (lib/utils/cache-config.ts): - Added PLUGIN_PREFIX = "openhax-codex" for unique cache namespace - Updated cache files to use plugin-specific prefixes: - openhax-codex-instructions.md (was codex-instructions.md) - openhax-codex-opencode-prompt.txt (was opencode-codex.txt) - Corresponding metadata files with -meta.json suffix 2. **Migration Logic** (lib/prompts/opencode-codex.ts): - migrateLegacyCache(): Automatically detects and migrates old cache files - validateCacheFormat(): Detects incompatible cache formats from other plugins - Enhanced error messages with actionable guidance for cache conflicts 3. **Test Updates**: - Updated all test files to use new cache file names - All 123 tests passing ✅ **User Experience**: - Seamless migration: Users switching plugins get automatic cache migration - Clear error messages: When cache conflicts occur, users get actionable guidance - No data loss: Existing cache content is preserved during migration Files modified: - lib/utils/cache-config.ts - Cache isolation configuration - lib/prompts/opencode-codex.ts - Migration and validation logic - test/prompts-opencode-codex.test.ts - Updated cache file paths - test/prompts-codex.test.ts - Updated cache file paths - spec/issue-25-oauth-cache-conflicts.md - Implementation spec * fixed minor type error * test: remove redundant env reset and header mock * Reduce console logging to debug flag * fix: filter ENOENT errors from cache logging to reduce noise - Add ENOENT filtering in getOpenCodeCodexPrompt cache read error handling - Add ENOENT filtering in getCachedPromptPrefix error handling - Prevents noisy error logs for expected first-run scenarios - Preserves visibility into genuine I/O/parsing problems - Addresses CodeRabbit review feedback on PR #28 * Use openhax/codex as plugin identifier * Add fallback sources for OpenCode codex prompt * Refresh OpenCode prompt cache metadata * Refactor metrics response helpers and fix JWT decoding * Tighten session tail slice to user/assistant only * Improve compaction resilience and cache metrics safety * Add release process documentation and open issues triage guide - Add comprehensive RELEASE_PROCESS.md with step-by-step release workflow - Add open-issues-triage.md for systematic issue management - Both documents support better project governance and maintenance * Strengthen tests for cache keys and gpt-5.1 cases * Soften first-session cache warnings and sync transformed body * Preseed session prompt cache keys * Memoize config loading and keep bridge prompts stable * Code cleanup: removed redundancy, improved tests Co-authored-by: riatzukiza <riatzukiza@users.noreply.github.com> * Fixed test shallow copy issue with deep copy. Co-authored-by: riatzukiza <riatzukiza@users.noreply.github.com> * chore(codex-max): memoize config loading, stabilize bridge prompts, and prep for Codex Max release; update request transforms and auth/session utilities; add lint-warnings spec * stabilize oauth server tests by completing mocks * fix: improve token refresh error handling and add debug logging - Add debug logging to token refresh process in session-manager - Improve error handling in codex-fetcher for 401 responses - Fix fetch helper error handling for failed token refresh - Add comprehensive test coverage for token refresh scenarios - Add refresh-access-token spec documentation * Fix test to assert on returned auth object instead of in-place mutation - Update test/fetch-helpers.test.ts to properly validate refreshAndUpdateToken return value - Add type guard for OAuth auth type checking - Aligns test expectations with function's design of returning updated auth object - All 396 tests pass with no TypeScript errors * test: add negative test for host-provided prompt_cache_key; fix: ensure explicit Content-Type headers in OAuth server responses - Add test to verify host-provided prompt_cache_key is preserved over session cache key - Update OAuth server send helper to always include default Content-Type: text/plain; charset=utf-8 - Change headers parameter type to http.OutgoingHttpHeaders for stronger typing - Preserve existing HTML response Content-Type override behavior * fix: clone auth refresh and tighten console logging * chore: guard disk logging and strengthen clones/roles * fix: simplify role validation in formatRole function - Remove hardcoded role whitelist in formatRole() - Return normalized role directly without validation - Add PR review documentation for CodeRabbit feedback * Fix all CodeRabbit review issues from PR #29 ## Critical Bug Fixes - Fix content-type header bug in fetch-helpers.ts - preserve original content-type for non-JSON responses - Fix cache fallback bug in codex.ts - wrap getLatestReleaseTag() in try/catch to ensure fallback chain works ## Test Improvements - Remove unused mocks in cache-warming.test.ts areCachesWarm tests - Fix mock leakage in index.test.ts by resetting sessionManager instance mocks - Add missing compactionDecision test case in codex-fetcher.test.ts - Remove redundant test case in codex-fetcher.test.ts ## Code Quality - Harden logger against JSON.stringify failures with try/catch fallback - Remove unused error parameter from logToConsole function - Update type signatures to match new function signatures ## Documentation - Add comprehensive PR analysis document in spec/pr-29-review-analysis.md All tests pass (398 passed, 2 skipped) with 82.73% coverage. * 📝 Add docstrings to `release/review-comments` Docstrings generation was requested by @riatzukiza. * #34 (comment) The following files were modified: * `lib/auth/server.ts` * `lib/logger.ts` * `lib/request/fetch-helpers.ts` * Enhance compaction test coverage and fix linter warning ## Test Improvements - Enhance compaction decision test in codex-fetcher.test.ts to validate full flow: - Verify recordSessionResponseFromHandledResponse called with compacted response - Verify fetcher returns the compacted response with correct status/body - Ensure complete end-to-end compaction flow validation ## Code Quality - Fix linter warning in lib/auth/server.ts by prefixing unused parameter with underscore - Update corresponding type definition in lib/types.ts to match All tests continue to pass (398 passed, 2 skipped). * Replace unsafe any cast with type-safe client access in logger ## Type Safety Improvements - Add OpencodeApp type with proper notify/toast method signatures - Add OpencodeClientWithApp intersection type for type-safe app access - Create isOpencodeClientWithApp type guard function - Replace (loggerClient as any)?.app with type-safe guarded access - Update emit function to use type guard for loggerClient.app access ## Benefits - Eliminates unsafe any type casting - Provides compile-time type checking for app property access - Maintains backward compatibility with existing OpencodeClient interface - Follows TypeScript best practices for type guards All tests continue to pass (398 passed, 2 skipped). * Fix type safety in logger module - Replace unsafe type casting with proper optional chaining - Update notifyToast to use correct Opencode SDK API structure - Use client.tui.showToast with proper body object format - Remove unnecessary type guard function - All tests pass and TypeScript compilation succeeds * Clarify waitForCode state validation docs --------- Co-authored-by: opencode-agent[bot] <opencode-agent[bot]@users.noreply.github.com> Co-authored-by: riatzukiza <riatzukiza@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary
This PR addresses all 19 actionable comments from the CodeRabbit review on PR #29, focusing on critical bug fixes, test improvements, and code quality enhancements.
Critical Bug Fixes 🚨
handleErrorResponseinlib/request/fetch-helpers.tsto preserve original content-type for non-JSON responses, preventing parse errors on HTML error responsesgetLatestReleaseTag()in try/catch inlib/prompts/codex.tsto ensure network failures properly fall back to cache/bundled instructionsTest Improvements 🧪
areCachesWarmtests intest/cache-warming.test.tsby removing unused prompt mockstest/index.test.tsto prevent cross-test contaminationtest/codex-fetcher.test.tsto cover the finalization pathtest/codex-fetcher.test.tsCode Quality 🔧
JSON.stringify()inlib/logger.tsto prevent logging failures from non-serializable dataerrorparameter fromlogToConsoleand updated all call sitesDocumentation 📊
spec/pr-29-review-analysis.mdwith detailed breakdown of all issues and fixesTest Results ✅
Files Modified
lib/request/fetch-helpers.ts- Content-type header fixlib/prompts/codex.ts- Cache fallback fixlib/logger.ts- JSON.stringify hardeninglib/auth/server.ts- Type signature updateslib/types.ts- Type signature updatesspec/pr-29-review-analysis.md- DocumentationThis PR resolves all blocking issues from the CodeRabbit review and ensures the codebase is more robust and well-tested.