Conversation
🦋 Changeset detectedLatest commit: 351f6f0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Thank you for following the naming conventions! 🙏 |
Contributor
|
Warning Review limit reached
Next review available in: 15 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Eve integration now targets Eve 0.30, captures expanded turn and session metadata, supports configurable message modes, tracks lifecycle events and costs, and optionally emits session rollups. ChangesEve event lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Eve
participant EvlogHook
participant TurnState
participant SessionState
participant SessionEvent
Eve->>EvlogHook: lifecycle and message events
EvlogHook->>TurnState: accumulate turn metadata
TurnState->>SessionState: update rollup metrics
Eve->>EvlogHook: session completion or failure
EvlogHook->>SessionState: finalize open turns
SessionState->>SessionEvent: emit optional session rollup
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/evlog/package.json`:
- Line 466: The eve peer dependency floor and release classification must
express the same compatibility decision. In packages/evlog/package.json at lines
466-466, confirm whether >=0.30.0 is required; if so, retain it and update
.changeset/eve-030-event-surface.md at lines 2-2 from minor to major, otherwise
lower the peer range and keep 0.30 event handlers optional. Ensure both sites
follow the patch/minor/major release guidelines.
- Line 441: Update the eve dependency entry in package.json to the required
0.30.8 export-compatible version, then replace any as never casts used for eve
events with explicit event payload shapes so invalid or mismatched fields are
caught by typechecking.
In `@packages/evlog/src/eve/index.ts`:
- Around line 111-119: Update EveRuntimeInfo and the runtime construction around
invocation handling so version, agentId, and model are optional and only
included when Eve reports non-empty values; do not store empty-string
placeholders. Adjust buildLineage to emit eve.runtime only when the resulting
runtime object contains at least one reported field, while preserving parent and
other lineage data for subagent sessions.
- Around line 821-838: Update packages/evlog/test/eve.test.ts lines 707-731
first by adding a sessionEvent regression test that parks a turn on incomplete
authorization, emits session.failed, and asserts the session event increments
failedTurns. Then update packages/evlog/src/eve/index.ts lines 821-838 and its
callers to pass the terminal httpStatus into accumulateSessionTotals; increment
cancelledTurns only for CANCELLED_STATUS and failedTurns for every other status
>= 400, rather than classifying from phase.
- Around line 20-28: Update the `EveMessageMode` JSDoc preview description to
state that attachments are reduced to their type (and media type if applicable),
removing the inaccurate reference to attachment size; keep the
`summarizeMessageParts` behavior and other mode descriptions unchanged.
- Around line 688-712: Update buildLineage to remove the local cast around
ctx.session and read parent directly from the callback context’s typed session
shape. Preserve the existing parent lineage fields and conditional
turnId/subagent properties while relying on the declared SessionParent API type.
- Around line 1074-1092: Update the compaction state handled by the
`'compaction.requested'` and `'compaction.completed'` event handlers to track
requested compactions separately from completed compactions. Ensure
`flushEveMetadata` emits `eve.compaction` when a request has recorded a model or
input-token count even if `acc.compactions` remains zero, while preserving the
existing completion count behavior.
- Line 98: Remove the unused SessionRollup.startedAt field from its type and
delete the corresponding initialization in freshRollup(). Keep
emitSessionEvent() and the existing rollup behavior unchanged, since session
duration is not being added.
In `@packages/evlog/test/eve.test.ts`:
- Line 691: Remove the duplicate const message declaration in the test block,
keeping a single declaration of message with its existing type assertion so the
TypeScript build and tests compile.
- Line 62: Remove the unused complete?: boolean property from the compactions
entry type in eve.test.ts and delete the corresponding dead handling that checks
compaction.completed, preserving the existing behavior for modelId and
usageInputTokens.
- Around line 707-731: Add session-rollup regression coverage alongside the
existing sessionEvent test: configure defineEvlogHook with sessionEvent enabled,
park a turn on an authorization, emit session.failed, and wait for the drain.
Locate the session event via findEventViaDrain and assert its session
scope/identity and failedTurns count, ensuring the phase-derived failed-turn
aggregation is exercised before the implementation fix.
- Line 85: Replace the `as never` casts on the fake eve payloads in the
`message.received`, `authorization.completed`, and `subagent.started` test cases
with the appropriate eve `HookEventMap`/`ProtocolEvent` types, adding the
required imports. Preserve the existing payload values while restoring
compile-time validation of their fields.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a40aa49b-c79c-47e3-aa6d-201b768e275e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
.changeset/eve-030-event-surface.md.changeset/eve-terminate-cancelled-turns.mdpackages/evlog/package.jsonpackages/evlog/src/ai/index.tspackages/evlog/src/eve/index.tspackages/evlog/test/eve.test.ts
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
2 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/evlog/test/eve.test.ts (1)
733-736: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winDuplicate
const messagedeclaration breaks typecheck.The annotated file shows the same declaration twice at line 734:
const message = event?.message as { received: string, parts: unknown[] } const message = event?.message as { received: string, parts: unknown[] }TypeScript rejects a redeclared
constin the same block scope, so typecheck and this test file fail to run. Delete the second line.🐛 Proposed fix
const message = event?.message as { received: string, parts: unknown[] } - const message = event?.message as { received: string, parts: unknown[] } expect(message.received).toHaveLength(600)#!/bin/bash # Confirm whether the duplicate const declaration is still present. rg -n --type=ts -C3 "const message = event\?\.message as" packages/evlog/test/eve.test.ts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/evlog/test/eve.test.ts` around lines 733 - 736, Remove the duplicate const message declaration in the test block around findEventViaDrain, leaving a single typed assignment before the received-length and parts assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/eve-030-event-surface.md:
- Line 17: Update the preview-mode description in eve-030-event-surface.md to
state that attachments are reduced to both their type and media type, matching
summarizeMessageParts and the EveMessageMode documentation.
In `@packages/evlog/src/eve/index.ts`:
- Around line 674-687: Add a test option to the runTurn helper in eve.test.ts
that emits compaction.requested without compaction.completed, then add coverage
asserting eve.compaction contains count: 0, requested: 1, model, and
inputTokensAtTrigger. Keep existing tests and default runTurn behavior
unchanged.
- Around line 1086-1096: Update the compaction.requested handler to preserve the
first trigger’s compactionModel and compactionInputTokens instead of overwriting
them on later requests; only assign these accumulator fields when no prior
compaction trigger has been recorded, while continuing to increment
compactionsRequested for every request.
- Around line 881-888: Update accumulateSessionTotals to include each turn’s
ai.estimatedCost when folding session totals, while preserving reported
usage.costUsd as the preferred cost. In the session ai payload, emit
estimatedCost when no reported cost was accumulated instead of omitting cost
information, and keep the existing costUsd behavior for reported costs.
In `@packages/evlog/test/eve.test.ts`:
- Around line 444-448: Replace the `as never` payload casts at the
`session.completed` calls in the affected tests with
`HookEventMap['session.completed']`-typed payloads. Supply all required `data`
fields according to the eve event shape, preserving the existing hook invocation
behavior and allowing type checking to catch future payload changes.
---
Duplicate comments:
In `@packages/evlog/test/eve.test.ts`:
- Around line 733-736: Remove the duplicate const message declaration in the
test block around findEventViaDrain, leaving a single typed assignment before
the received-length and parts assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 77e4d52e-35ba-4242-8c9d-d038a4376e37
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
.changeset/eve-030-event-surface.md.changeset/eve-terminate-cancelled-turns.mdpackages/evlog/package.jsonpackages/evlog/src/ai/index.tspackages/evlog/src/eve/index.tspackages/evlog/test/eve.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/evlog/src/eve/index.ts`:
- Around line 889-893: Preserve mixed cost sources in session totals: update the
session aggregation around packages/evlog/src/eve/index.ts:835 to add
estimatedCost only for turns without Eve-reported cost, update the emission
around packages/evlog/src/eve/index.ts:889-893 to report both costUsd and
estimatedCost when both exist, and add a regression test at
packages/evlog/test/eve.test.ts:854-870 asserting both fields for a session
containing one reported-cost turn and one estimated-cost turn.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ba7b74cf-2f3b-46fe-aae7-6e2a39f6a651
📒 Files selected for processing (3)
.changeset/eve-030-event-surface.mdpackages/evlog/src/eve/index.tspackages/evlog/test/eve.test.ts
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/evlog/test/eve.test.ts (1)
763-777: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRemove the repeated
const messagedeclaration.The snippet shows
const message = event?.message as { received: string, parts: unknown[] }twice in the same block. TypeScript rejects a redeclaredconstin one block scope, so typecheck fails and this test file does not run. Keep one declaration.🐛 Proposed fix
const message = event?.message as { received: string, parts: unknown[] } - const message = event?.message as { received: string, parts: unknown[] } expect(message.received).toHaveLength(600)#!/bin/bash # Confirm whether the duplicate declaration is still present on the PR branch. rg -nP --type=ts -B 2 -A 4 'const message = event\?\.message as' packages/evlog/test/eve.test.ts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/evlog/test/eve.test.ts` around lines 763 - 777, Remove the duplicate const message declaration in the affected test block, keeping a single typed declaration before the received-length and parts assertions so the test typechecks and retains both validations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/evlog/test/eve.test.ts`:
- Around line 706-744: Extend the subagent lifecycle test around the existing
subagent.started/completed scenario with a separate turn that emits
subagent.called and subagent.started but no subagent.completed, then complete
that turn and assert the recorded subagent entry has status "started". Keep the
existing completed-status and duration assertions, and ensure the new test
covers the started transition in the eve event handling flow.
---
Duplicate comments:
In `@packages/evlog/test/eve.test.ts`:
- Around line 763-777: Remove the duplicate const message declaration in the
affected test block, keeping a single typed declaration before the
received-length and parts assertions so the test typechecks and retains both
validations.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 91a51c37-4f65-4075-bf68-532717459da2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
.changeset/eve-030-event-surface.mdpackages/evlog/package.jsonpackages/evlog/src/ai/index.tspackages/evlog/src/eve/index.tspackages/evlog/test/eve.test.ts
2 tasks
2 tasks
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.
🔗 Linked issue
📚 Description
📝 Checklist
Summary by CodeRabbit
New Features
Documentation