Skip to content

Conversation

@nianiaJR
Copy link

@nianiaJR nianiaJR commented Jan 28, 2026

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

Summary by CodeRabbit

  • Refactor

    • Simplified graph invocation handling by removing an unnecessary wrapper while preserving streaming behavior and tracer naming; no user-facing behavior changes.
  • Tests

    • Added a test to ensure run persistence is triggered during graph invocation and verified trace IDs and related logs are recorded.

✏️ Tip: You can customize this high-level summary in your review settings.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @nianiaJR, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves a bug where the invoke method of the CompiledStateGraphObject was subject to redundant wrapping. By eliminating this superfluous interception, the change aims to optimize the execution flow of the graph and prevent any unintended side effects or performance overhead caused by duplicate processing.

Highlights

  • Removed Redundant Method Wrapping: The invoke method within the CompiledStateGraphObject was being unnecessarily wrapped by this.wrapGraphMethod. This change removes that specific wrapping, ensuring that only the stream method remains wrapped, addressing a potential duplication issue.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@coderabbitai
Copy link

coderabbitai bot commented Jan 28, 2026

📝 Walkthrough

Walkthrough

The PR removes the wrapper that replaced graph.invoke with this.wrapGraphMethod in CompiledStateGraphObject, leaving graph.invoke untouched; graph.stream remains wrapped and tracer name assignment is unchanged. A new test asserts persistRun is triggered when graph.invoke is called.

Changes

Cohort / File(s) Summary
Method wrapping removal
plugin/langchain/lib/graph/CompiledStateGraphObject.ts
Removed the override that wrapped graph.invoke with this.wrapGraphMethod; left graph.stream wrapping and tracer name setup unchanged.
Tests added
plugin/langchain/test/llm.test.ts
Added a test "should persistRun be triggered when graph.invoke is called" that monkey-patches LangGraphTracer.prototype.persistRun and Tracer.prototype.traceId, issues GET /llm/graph, and asserts persistRun was invoked and trace_id propagated.

Sequence Diagram(s)

(Skipped — changes are small and primarily test + single-method unwrapping; no multi-component new control flow requiring visualization.)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Poem

🐇 I hopped through code at break of dawn,

Unwrapped a call that lingered long;
Stream still dressed, invoke set free,
Logs and runs now dance with glee. ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: duplicate wrap tracer' directly addresses the main change: removing a duplicate wrapping of the graph.invoke method. This is the primary modification in the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses an issue with duplicate tracer wrapping for the invoke method. By removing the manual wrapping, the code is cleaner and avoids redundant tracing. The change is a good improvement. I've identified one minor follow-up action to further clean up the code as a result of this change.

I am having trouble creating individual review comments. Click here to see my feedback.

plugin/langchain/lib/graph/CompiledStateGraphObject.ts (47-48)

medium

With the removal of the graph.invoke wrapper, the originalInvoke variable, declared on line 40, is no longer used. It should be removed to avoid dead code.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@plugin/langchain/test/llm.test.ts`:
- Line 99: The test currently performs await app.httpRequest().get('/llm/graph')
without asserting the HTTP status; change it to capture the response (e.g.,
const res = await app.httpRequest().get('/llm/graph')) and add an assertion that
the status is expected (for example expect(res.status).toBe(200)) so failures
show a clear status-code error; update the same call site where
app.httpRequest().get('/llm/graph') is used.

app.mockLog();
mm(Tracer.prototype, 'traceId', 'test-persist-run-trace-id');

await app.httpRequest().get('/llm/graph');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add status code assertion for consistency and clearer failure messages.

The HTTP request should verify the response status like the other tests in this file (e.g., line 77-79). Without this, if the endpoint fails, the test may pass incorrectly or produce confusing assertion errors.

Proposed fix
-      await app.httpRequest().get('/llm/graph');
+      await app.httpRequest().get('/llm/graph').expect(200);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await app.httpRequest().get('/llm/graph');
await app.httpRequest().get('/llm/graph').expect(200);
🤖 Prompt for AI Agents
In `@plugin/langchain/test/llm.test.ts` at line 99, The test currently performs
await app.httpRequest().get('/llm/graph') without asserting the HTTP status;
change it to capture the response (e.g., const res = await
app.httpRequest().get('/llm/graph')) and add an assertion that the status is
expected (for example expect(res.status).toBe(200)) so failures show a clear
status-code error; update the same call site where
app.httpRequest().get('/llm/graph') is used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant