Skip to content

Update openai and langgraph adapters to support events - #650

Open
malithaYL wants to merge 2 commits into
feature/523-agui-3-integrationfrom
feature/523-agui-4-openai-langgraph
Open

Update openai and langgraph adapters to support events#650
malithaYL wants to merge 2 commits into
feature/523-agui-3-integrationfrom
feature/523-agui-4-openai-langgraph

Conversation

@malithaYL

Copy link
Copy Markdown
Contributor

Description

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test update
  • CI/CD update
  • Other (please describe):

Related Issues

Fixes #
Relates to #

Changes Made

Testing

  • Unit tests pass locally
  • Integration tests pass locally
  • Manual testing completed
  • New tests added for changes

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published

Screenshots (if applicable)

Additional Notes

@malithaYL
malithaYL force-pushed the feature/523-agui-4-openai-langgraph branch from cadbe49 to d78590b Compare August 20, 2026 12:28
@malithaYL
malithaYL force-pushed the feature/523-agui-4-openai-langgraph branch from d78590b to e5bbbf8 Compare August 20, 2026 13:05
@malithaYL
malithaYL marked this pull request as ready for review August 20, 2026 13:23
@malithaYL
malithaYL requested a review from amithad as a code owner August 20, 2026 13:23
@malithaYL
malithaYL force-pushed the feature/523-agui-4-openai-langgraph branch from e5bbbf8 to 8c0767e Compare August 20, 2026 13:29
@malithaYL
malithaYL marked this pull request as draft August 20, 2026 13:36
@malithaYL
malithaYL force-pushed the feature/523-agui-4-openai-langgraph branch from 8c0767e to 7f061f4 Compare August 20, 2026 13:41
@malithaYL
malithaYL marked this pull request as ready for review August 20, 2026 13:42

@amithad amithad left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Overall: a focused, high-quality PR 4 of the #523 stack. Both adapters match spec.md §10 as corrected in this PR, and the tests cover exactly the failure modes that would otherwise be silent (the empty-assistant-bubble regression, nested run_id pairing, the unserialisable-args path, the no-call_id item, and message_output_created staying ignored so messages are not doubled). CI is fully green (55 checks).

Spec documents (reviewed first): the spec.md and plan.md deltas are self-correcting and verifiable. §10 now separates correlation ids from boundary derivation and records that LangGraph, not only ADK, must derive boundaries; plan.md iteration 4 records every implementation-time decision with its reason, including the deviation from its own step 2. The claims I re-verified against the code all hold: started: set[str] is a local passed into _map_event (never on self), on_chat_model_start maps to nothing with MessageStart deferred to the first non-empty delta, OpenAI reads every id off the SDK's own events and remembers nothing, tool arguments are emitted whole, and _tool_arguments' broad except logs and leaves the call bracketed. One internal inconsistency introduced by the correction is inline (spec.md:794).

Spec conformance (dimension 6, against the PR 4 checklist): all implemented and tested. OpenAI stops filtering to ResponseTextDeltaEvent and maps tool_called/tool_output plus reasoning; LangGraph adds the on_chat_model_end/on_tool_start/on_tool_end branches with run_id correlation; both runner test suites moved from assert deltas == ["hi"] to event-sequence assertions while the framework_context round-trip assertions stay. The existing-test blast radius matches the spec's table exactly: only the two runner test files change, and the must-not-change list is untouched. The logger names follow the ak.<framework>.runner convention the other adapters use.

Findings: 1 suggestion and 1 question, both inline.

Summary-only:

  • [suggestion] design.md has gone stale again, in a new spot this PR's learning created: its "Adapter boundary support" table says LangGraph's boundaries are "Explicit" via on_chat_model_start/on_chat_model_end and the work is "handle more branches of an if it already has", but this PR established those events are unusable for message boundaries and added derivation with per-run memory. Fold this row into the design.md sync pass already requested on PR 649 (the comment on plan.md:210 there), so the sync fixes all four stale spots in one edit.
  • [suggestion] The PR body is again the unfilled template: no description and no issue link. Worth noting in the description that the "Breaking change" checkbox really belongs to PR 1's runner-contract change; what this PR itself changes observably is that OpenAI and LangGraph streams now carry real message boundaries, reasoning, and tool calls instead of the synthetic transitional pair.

No existing feedback on the PR, so nothing was skipped as a duplicate.

- **LangGraph** has an id but its `on_chat_model_start` / `on_chat_model_end` fire around *every*
model call whether or not prose was streamed, so mapping them directly brackets a tool-calling
turn into a message with nothing in it — the empty assistant bubble §4 rule 4 exists to prevent.
It therefore ignores `on_chat_model_start` entirely, emits `MessageStart` on the first non-empty

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[suggestion] This correction did not reach the §10 table above it, so the section now contradicts itself.

  • The table row (spec.md:763) still instructs: "Add branches for on_chat_model_start/on_chat_model_end/on_tool_start/on_tool_end to the if", while this bullet says the adapter "ignores on_chat_model_start entirely" and the implementation has no such branch.
  • plan.md handles the same drift by annotating its step 2 inline ("The on_chat_model_start branch is gone, which deviates from step 2 above"); the table row deserves the same one-line touch, e.g. dropping on_chat_model_start from the list or adding "(start is derived, see below)".
  • Matters because ak-dev-review-pr extracts requirements from these tables: a reviewer of PR 5 or 6 walking §10 would flag the missing branch as an omission.

:param item: The `RunItem` the event wraps.
:return: The AK events this item produces, empty when it maps to nothing.
"""
if name not in ("tool_called", "tool_output"):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[question] Handoff run items stay unmapped: deliberate non-goal, or a gap worth an event?

  • handoff_requested/handoff_occured fall through here and test_item_created_run_events_are_ignored_so_messages_are_not_doubled pins one of them as ignored, but neither spec.md §9/§10 nor plan.md records the decision, unlike every other ignore in this PR.
  • design.md's Motivation cites handoffs as information the old filter discards ("tool-call items and handoffs are dropped by the filter"), so a reader of the spec set would expect them to survive once the filter goes.
  • The AK event model has a natural carrier: StepStart/StepEnd (name = the target agent), which LangGraph declined for its own reasons (on_chain_* is too noisy) that do not apply here — a handoff is exactly the user-recognisable boundary that decision said steps should be.
  • If the answer is "non-goal for now": a line in spec.md §10 and in PR 7's fidelity matrix keeps the next reviewer from re-asking; if not, the mapping looks cheap while the raw items are already in hand.

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.

2 participants