fix: post-release audit — hook safety, polling resilience, cache merge, env warnings#77
Merged
Merged
Conversation
…e, env warnings P0: Wrap all plugin hook handlers (event, experimental.text.complete, tool.execute.before/after) in try/catch to prevent host CLI crashes. P1: Fix polling-manager — move authResolver inside try block, add .catch() to tick().then() chains to prevent silent polling death. P1: Change usage-aggregator fromJSON to merge (upsert) instead of replace, preventing early in-memory samples from being lost on cache load. P1: Wrap coexistence promptClient call in try/catch to isolate prompt channel errors from propagating to plugin hooks. P2: Add console.warn for invalid env var values with valid options. P2: Fix CONFIG_DEFAULTS.md toast remnant (toast-only → prompt+publisher). P2: Add clarifying JSDoc to MAX_SESSION_ENTRIES constant. P3: Use per-save unique tmp paths in disk-cache to prevent concurrent write collisions.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Walkthrough다양한 모듈에 환경 변수 검증, 에러 처리 개선, 동시 저장 충돌 감소 로직이 추가되었습니다. 또한 설정 기본값이 업데이트되고, 사용량 샘플 병합 방식이 교체에서 업서트로 변경되었습니다. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/usage-aggregator.test.ts (1)
426-451: 병합 테스트에 “동일messageID갱신” 케이스를 한 줄 더 추가하면 회귀 방지가 더 탄탄해집니다.현재 테스트는 “신규 추가” 경로를 잘 검증합니다. 같은
messageID입력 시 값이 덮어써지는 경로까지 포함하면upsert의미를 완전히 고정할 수 있습니다.테스트 보강 예시
aggregator.fromJSON(newSamples) + aggregator.fromJSON([{ ...newSamples[0], cost: 0.07 }]) const samples = aggregator.allSamples() expect(samples).toHaveLength(2) expect(samples.map((s) => s.messageID).sort()).toEqual(["new_msg", "old_msg"]) + expect(samples.find((s) => s.messageID === "new_msg")?.cost).toBe(0.07)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/usage-aggregator.test.ts` around lines 426 - 451, Add one more case to the "fromJSON merges samples with existing (upsert behavior)" test that verifies an incoming sample with the same messageID updates the existing sample: after calling aggregator.addSample(oldSample) and then aggregator.fromJSON with a new sample that has messageID "old_msg" but different fields (e.g., updated completedMs, contextUsedTokens, cost), assert that aggregator.allSamples() still has the same length but that the sample with messageID "old_msg" reflects the updated fields (use messageID lookup or find on samples array). This ensures aggregator.fromJSON upserts existing entries rather than only appending new ones.src/plugin.ts (2)
520-521: 세션 상한 주석에 실제 상한값을 함께 명시하면 더 명확합니다.주석에 현재 상한 숫자(예:
50)를 같이 넣어두면, 운영/튜닝 시maxStateEntries와의 차이를 더 빠르게 파악할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugin.ts` around lines 520 - 521, Update the comment for the constant MAX_SESSION_ENTRIES to explicitly include its numeric value (50) and clarify it is distinct from config.maxStateEntries; e.g., modify the docstring above MAX_SESSION_ENTRIES so it reads something like "Maximum concurrent sessions tracked (50). Distinct from config.maxStateEntries (reducer history depth)." to make the hard limit obvious during tuning/operations.
835-918: 훅 무중단 전략은 좋지만, 완전 무음catch에는 최소 디버그 로그 경로를 두는 편이 운영 추적에 유리합니다.지금 구조는 안정성 목적에 부합합니다. 다만 모든 예외를 완전히 삼키면 현장 장애 분석이 어려워집니다. 기본은 무음 유지, 디버그 플래그에서만 로그를 남기는 방식이 균형이 좋습니다.
경량 디버그 로깅 예시
+function reportHookError(scope: string, error: unknown): void { + if (process.env.OPENCODE_STATUS_HUD_DEBUG === "1") { + console.warn(`[opencode-status-hud] ${scope} hook failed`, error) + } +} ... - } catch { - // Plugin hooks must never throw — silently ignore errors + } catch (error) { + reportHookError("event", error) } ... - } catch { - // Plugin hooks must never throw — silently ignore errors + } catch (error) { + reportHookError("experimental.text.complete", error) }Also applies to: 948-1009, 1013-1026, 1030-1044
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugin.ts` around lines 835 - 918, The try/catch currently swallows all errors silently; change the final catch to capture the error (catch (err)) and, while keeping silent by default, emit a debug-level log when a debug flag is enabled (use the existing runtimeConfig for a debug/verbose flag check). Specifically, inside the catch block reference the error and call a debug logger (e.g., ctx.logger?.debug(...) or console.debug(...)) only when runtimeConfig.debug (or similar) is true so normal operation stays silent but failures are visible when debugging; update the same pattern for the other hook blocks noted (lines marked 948-1009, 1013-1026, 1030-1044) to use the same conditional logging approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/plugin.ts`:
- Around line 520-521: Update the comment for the constant MAX_SESSION_ENTRIES
to explicitly include its numeric value (50) and clarify it is distinct from
config.maxStateEntries; e.g., modify the docstring above MAX_SESSION_ENTRIES so
it reads something like "Maximum concurrent sessions tracked (50). Distinct from
config.maxStateEntries (reducer history depth)." to make the hard limit obvious
during tuning/operations.
- Around line 835-918: The try/catch currently swallows all errors silently;
change the final catch to capture the error (catch (err)) and, while keeping
silent by default, emit a debug-level log when a debug flag is enabled (use the
existing runtimeConfig for a debug/verbose flag check). Specifically, inside the
catch block reference the error and call a debug logger (e.g.,
ctx.logger?.debug(...) or console.debug(...)) only when runtimeConfig.debug (or
similar) is true so normal operation stays silent but failures are visible when
debugging; update the same pattern for the other hook blocks noted (lines marked
948-1009, 1013-1026, 1030-1044) to use the same conditional logging approach.
In `@tests/usage-aggregator.test.ts`:
- Around line 426-451: Add one more case to the "fromJSON merges samples with
existing (upsert behavior)" test that verifies an incoming sample with the same
messageID updates the existing sample: after calling
aggregator.addSample(oldSample) and then aggregator.fromJSON with a new sample
that has messageID "old_msg" but different fields (e.g., updated completedMs,
contextUsedTokens, cost), assert that aggregator.allSamples() still has the same
length but that the sample with messageID "old_msg" reflects the updated fields
(use messageID lookup or find on samples array). This ensures
aggregator.fromJSON upserts existing entries rather than only appending new
ones.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/CONFIG_DEFAULTS.mdsrc/disk-cache.tssrc/plugin.tssrc/polling-manager.tssrc/runtime/coexistence.tssrc/usage-aggregator.tstests/usage-aggregator.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Post-release hardening pass addressing 9 issues identified by deep codebase audit (5× Explore agents + Oracle architecture review). All fixes are minimal, targeted, and backwards-compatible.
Changes
P0 — Critical
src/plugin.tsevent,tool.execute.before/after,experimental.text.complete) in try/catch — unhandled exceptions crashed host CLIP1 — High
src/polling-manager.tsauthResolver()call inside try block — was unguarded outsidesrc/polling-manager.ts.catch()to bothtick().then()chains — rejected tick killed polling permanentlysrc/usage-aggregator.tsfromJSON()now merges viaupsertSample()instead of wiping in-memory datasrc/runtime/coexistence.tspromptClientdispatch in try/catchP2 — Medium
src/plugin.tswarnInvalidEnv()helper — logs warnings for invalid env var values instead of silent nulldocs/CONFIG_DEFAULTS.mdsrc/plugin.tsMAX_SESSION_ENTRIESexplaining the 200-entry capP3 — Low
src/disk-cache.tssaveCountertotmpPath— prevents concurrent write collision within same processVerification
npm run test)npm run typecheck)npm run build)Not in Scope (Future)
Oracle identified additional architectural items deferred to future PRs:
plugin.tsSRP extraction (session store, renderer, cache coordinator)pruneMapover200kcost rate activationSummary by CodeRabbit
릴리스 노트
버그 수정
문서