Skip to content

fix(langchain): forward session properties to the engine - #2692

Open
AmirF194 wants to merge 1 commit into
Canner:mainfrom
AmirF194:fix/2638-langchain-session-properties
Open

fix(langchain): forward session properties to the engine#2692
AmirF194 wants to merge 1 commit into
Canner:mainfrom
AmirF194:fix/2638-langchain-session-properties

Conversation

@AmirF194

@AmirF194 AmirF194 commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

What changes

WrenToolkit.query(), dry_plan() and dry_run() now accept properties
and forward it to the engine, so a caller can supply MDL session properties
per call, the same fix already needed on the wren-pydantic side (#2684).

WrenEngine.query/dry_plan/dry_run already accept properties
(core/wren/src/wren/engine.py); the langchain toolkit simply never passed
it through, so a model guarded by row-level access control could not be
read through this SDK at all: planning fails when a required session
property is missing, and there was no way to supply one.

Fixes #2691 (companion issue for wren-langchain; see #2638 for the identical wren-pydantic gap)

Why the LangChain tools are unchanged

wren_query / wren_dry_plan keep their current signatures on purpose.
Turning a session property into a model-fillable tool argument would let the
LLM choose the identity that RLAC is keyed on, which is a separate design
question (toolkit- or run-scoped binding), so this PR only opens the direct
Python API.

How this was verified

  • 3 new tests assert properties reaches WrenEngine.query/dry_plan/dry_run.
  • 3 existing delegation tests updated for the new properties=None default
    call contract.
  • Confirmed the new tests fail against the unmodified toolkit (6 failed) and
    pass with the fix (6 passed), same Docker image both times.
  • Full suite: pytest -v on Python 3.11 and 3.12 (this module's CI matrix),
    119 passed / 2 deselected (the slow marker) on both. ruff check . and
    ruff format --check . clean on both.
  • Not verified: an actual RLAC-protected project end to end. The engine-side
    behavior is the existing, already-tested path; this PR only closes the gap
    in what the toolkit forwards to it.

Summary by CodeRabbit

  • New Features

    • Added optional session properties to query, plan, and validation operations.
    • Supports row-level access-control predicates through session properties.
  • Tests

    • Added coverage for default and custom session properties across query, planning, and validation workflows.

WrenToolkit.query/dry_plan/dry_run never accepted or forwarded session
properties, even though WrenEngine already threads them through its
planning path. A model guarded by row-level access control needs the
session property its rule declares required, so every call through the
SDK failed with a missing-property planning error, with no way to
supply one. wren-pydantic has the identical gap open on the same issue
(Canner#2638).

Closes Canner#2691
Refs Canner#2638
@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: eb7eee9f-595b-4a4c-bd3c-2c9837787c66

📥 Commits

Reviewing files that changed from the base of the PR and between f2841bc and 5bbe920.

📒 Files selected for processing (2)
  • sdk/wren-langchain/src/wren_langchain/_toolkit.py
  • sdk/wren-langchain/tests/unit/test_toolkit_runtime.py

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


Walkthrough

WrenToolkit.query, dry_plan, and dry_run now accept optional MDL session properties and forward them to the engine. Unit tests cover default None values and custom property dictionaries.

Changes

LangChain session properties

Layer / File(s) Summary
Session properties forwarding
sdk/wren-langchain/src/wren_langchain/_toolkit.py, sdk/wren-langchain/tests/unit/test_toolkit_runtime.py
The three toolkit methods accept optional properties values and pass them to the corresponding engine calls. Tests verify default and custom property forwarding.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 5bbe9

The direct toolkit APIs now forward per-call session properties to the engine, enabling guarded queries without changing the LangChain tool interfaces. No actionable merge-blocking risk remains after normal checks and review.

Poem

I’m a rabbit with properties tucked tight,
Passing session clues through the query’s flight.
Plans and dry runs now carry them too,
Tests check the defaults and custom values through.
Hop, hop—RLAC paths come into view!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the primary change: forwarding session properties from the LangChain toolkit to the engine.
Description check ✅ Passed The description explains the change, user impact, linked issue, testing, and the deliberate tool-signature constraint.
Linked Issues check ✅ Passed The changes satisfy issue #2691 by adding and forwarding properties on all three direct toolkit APIs while preserving LangChain tool signatures.
Out of Scope Changes check ✅ Passed The modified APIs and tests remain within the linked issue scope, with no unrelated code changes identified.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

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.

wren-langchain: session properties are not forwarded, same gap as #2638

1 participant