fix: close post-merge test gaps in hooks + Phase-2 history - #140
Merged
Conversation
Post-merge audit of #137/#138 found four untested behaviors; all now covered, no product code changes: - canonical-sdk-compat.test.ts: toGeminiContent output validated by the REAL @google/generative-ai validateChatHistory (startChat constructor, offline) with a negative control proving the validator fires — pins the role:"function" decision to the SDK, not to a code comment. toOpenAIMessages output assigned to ChatCompletionMessageParam[] WITHOUT a cast so typecheck enforces shape compatibility (the provider call site casts, which would hide drift). - session-hooks H8: hook input mutation composed with MANUAL approval — the approval UI delta shows the hook-rewritten input and the approved merge base is the mutated input, not the model's original. - session-hooks H9/H10/H11: provider_switched, rotated, and session_end observe emits (previously only session_start/after_turn were covered). Also hardens the observe-hook file waits: wait for parseable JSON, not file existence — 'cat > file' creates the file before the payload lands.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #140 +/- ##
=======================================
Coverage 82.07% 82.07%
=======================================
Files 97 97
Lines 16867 16867
=======================================
Hits 13843 13843
Misses 3024 3024
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
provider-gemini.test.ts installs a process-global
mock.module("@google/generative-ai"), so whether an in-process import
sees the real SDK depends on test-file execution order — passed locally,
failed in CI (startChat returned the mock's {sendMessageStream} stub and
the negative control stopped throwing). Move the validation into a
spawned fixture (fake-pi pattern): a fresh process is mock-proof and
order-independent.
Collaborator
Author
|
/gemini-review |
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
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.
What
Post-merge testing audit of #137 (hook bus) and #138 (Phase-2 history). The merged combination passes the full suite (1233 tests) and the new files measure 99–100% line coverage, but four behaviors were untested. This PR covers them — tests only, no product code changes.
New coverage
canonical-sdk-compat.test.ts— pins the riskiest converter integration points to the actual installed SDKs instead of code comments:toGeminiContentoutput is fed through the real@google/generative-aistartChat(), whose constructor runsvalidateChatHistorysynchronously (offline, zero network). A negative control (functionResponse on auserturn — the design doc's original sketch) proves the validator actually fires, so the role-"function"decision can't silently regress on an SDK bump.toOpenAIMessagesoutput is assigned toChatCompletionMessageParam[]without a cast, sobun run typecheckenforces structural compatibility — the provider call site uses a cast, which would hide drift.session-hooks.test.tsH8–H11:provider_switched,rotated, andsession_endobserve emits, none of which were exercised before (onlysession_start/after_turnwere).Test-harness fix
The observe-hook assertions now wait for parseable JSON, not file existence —
cat > filecreates the file before the payload bytes land, which made H9/H11 flaky. H8 also waits for the mutation broadcast rather thanwaiting_approval(the tool_start handler flips that status from its side-effect-free peek before the hook gate runs, so status alone races the hook).bun run test(1233 pass),typecheck,lintgreen.🤖 Generated with Claude Code