Advance remove-recursive-submodules to the response_messages fix - #56
Closed
iskakaushik wants to merge 11 commits into
Closed
Advance remove-recursive-submodules to the response_messages fix#56iskakaushik wants to merge 11 commits into
iskakaushik wants to merge 11 commits into
Conversation
#40) * fix: preserve conversation history across multi-step tool calls Two bugs caused multi-step tool calling to repeatedly re-issue the same tool calls and drop the system prompt: - MultiStepCoordinator::execute_multi_step rebuilt step inputs from initial_options + only the most recent step's tool calls/results, so earlier turns were lost between iterations. Refactor to mirror the Vercel AI SDK pattern (packages/ai/src/generate-text/generate-text.ts): immutable initial_messages plus a response_messages accumulator; each step's input is the concatenation of the two. - OpenAIRequestBuilder dropped options.system whenever the caller supplied a non-empty messages array (the Chat Completions API has no top-level system field; system must be a leading role=system message). Anthropic's builder already handled this correctly via request["system"]. Validated by the existing MultiStepDuplicateExecutionTest cases, which were known-failing per issue #26 and now pass for both providers. Adds tests/cmake_test_discovery_*.json to .gitignore (build artifact). * style: wrap long comment line to satisfy clang-format
…#41) * feat(models): refresh OpenAI / Anthropic model identifiers (May 2026) OpenAI: - Replace the gpt-4o / gpt-4 / gpt-3.5 lineup with the GPT-5 family as current (gpt-5.4 / -pro / -mini / -nano, gpt-5-mini, gpt-5-nano, gpt-4.1 / -mini). Older identifiers retained but marked DEPRECATED. - Default model -> kGpt54. - Drop unverified gpt-5.5 / gpt-5.5-pro entries (not in OpenAI's public catalog or in the Vercel AI SDK's curated list). Anthropic: - Promote claude-opus-4-7 / claude-sonnet-4-6 / claude-opus-4-6 / claude-haiku-4-5 to current; default model -> kClaudeSonnet46. - supported_models() now lists both the friendly aliases (e.g. claude-sonnet-4-5) and the dated snapshots (e.g. -20250929) so the identifiers exposed by ai::anthropic::models::* validate via supports_model(). Tests / examples / mocks / README updated to the new defaults. Fixes the previously-failing AnthropicIntegrationTest.DefaultModelGeneration which still asserted against the old claude-sonnet-4-5 default. * style: apply clang-format
#42) * feat(langfuse): add Langfuse tracing module for ai::Client agent calls Adds an opt-in ai::langfuse module that wraps generate_text calls and emits one trace per logical operation with: - one Generation observation (model, modelParameters, input messages, output text, aggregated usage, finish_reason / step count metadata), - one Span per tool invocation, parented to the generation, with the tool's args and result (or error) recorded as input/output. The module hooks into the existing on_tool_call_start / on_tool_call_finish callbacks in GenerateOptions so user-installed callbacks are preserved (chained). After generate_text returns, the caller invokes Trace::end() to flush the accumulated batch to POST /api/public/ingestion via the vendored httplib + Basic auth. Layout - include/ai/langfuse.h: public surface (Config, Tracer, Trace, generate_text helper). - src/langfuse/tracer.cpp: HTTP / batching / event-shape implementation. - examples/langfuse_tracing.cpp: end-to-end demo using OpenAI with lookup_user / get_weather tools across multiple steps. - .env.local.example: credentials template (the populated .env.local is gitignored). - CMakeLists.txt: new ai-sdk-cpp-langfuse target (alias ai::langfuse), AI_SDK_HAS_LANGFUSE=1, included in ai::sdk and the install/export set. Existing targets unchanged. Verified end-to-end: example produced trace with the expected nested generation+tool observations against us.cloud.langfuse.com. * style: apply clang-format
Langfuse module (`include/ai/langfuse.h`, `src/langfuse/tracer.cpp`):
- Single Trace::to_iso8601(time_point) helper replaces three
duplicated gmtime_r/snprintf blocks (now_iso8601 plus two inline
lambdas in finish_generation / build_trace_event).
- Capture timestamp once per record_tool_call_{start,finish} event
(previously two clock reads per span event).
- Anonymous-namespace constants for event type discriminators
(trace-create / span-create / span-update / generation-create) and
level / unit strings, replacing scattered magic strings.
- Extract make_span_create + wrap_event helpers; the orphan-span
fallback in record_tool_call_finish reuses them instead of inlining
20+ lines of duplicated body construction.
- Drop the over-engineered ParsedHost/parse_host: httplib::Client's
URL constructor handles scheme/host/port already; we only need a
base-path extractor for Langfuse instances served under a sub-path.
- Cache httplib::Client + Basic-auth headers on the Tracer (lazily
constructed under mu_) so repeat send_batch calls reuse the
connection / TLS session instead of paying a fresh handshake.
- Replace 6-arg `start_trace(name, input, user, session, metadata,
tags)` sprawl with `start_trace(name, TraceOptions{})` to match the
rest of the SDK's struct-of-options style.
- Default Config::error_policy = ErrorPolicy::kStrict (was
best_effort=true) so misconfigurations surface at integration time
instead of being silently swallowed.
- Reject post-end mutations: set_*/instrument/finish_generation/record_*
bail out if Trace::end() has fired.
- Replace hand-rolled UUID v4 with stduuid (vendored under
third_party/stduuid-header-only/).
Multi-step coordinator (`src/tools/multi_step_coordinator.cpp`):
- Avoid re-copying initial_messages every loop iteration. step_messages
is grown in place: erase back to the immutable prefix + insert the
running response_messages accumulator, instead of `step_options =
initial_options; step_options.messages = initial_messages;
step_options.messages.insert(...)` per step.
- Remove the dead create_next_step_options stub from header + impl
(not part of any external ABI; was only kept as scaffolding during
the refactor).
- Trim narrating comment that referenced an external repo path.
Vendoring:
- Add third_party/stduuid-header-only/ (uuid.h + LICENSE) and
third_party/stduuid-cmake/ wrapper exposing a stduuid::stduuid
INTERFACE target. Wired into ai-sdk-cpp-langfuse via PRIVATE link.
Verified: full ctest suite (221/227 pass; 6 failing are
ClickHouseIntegrationTest cases that need a local ClickHouse server,
unchanged from main). End-to-end Langfuse example produces the
expected trace with 1 generation + N tool spans nested correctly.
The installed package was unusable from a downstream CMake project: targets were exported under their internal ai-sdk-cpp-* names while the config file referenced ai::core and friends, the langfuse component was missing from the component list, and the OpenSSL, Threads, and ZLIB dependencies of the static libraries were neither linked for install-tree consumers nor resolved via find_dependency(). Export every target under its ai:: alias name, declare the dependencies on the install interface, and find them in the package config. The umbrella ai::sdk target is now added only when the consumer requested no explicit components. Switch httplib compression from brotli to zlib and drop the brotli submodule; zlib was already a dependency and nothing needed brotli. Mark the zlib and googletest subprojects EXCLUDE_FROM_ALL so their targets stop polluting the default build. Also fix the developer scripts: the format and lint sweeps now exclude only build-* directories instead of any matching path component (a file named build-info.cpp would have been skipped silently), clang-tidy receives the compile database's directory as -p expects, and build.py prints the debug-suffixed example paths the examples build actually produces. Add ANTHROPIC_API_KEY to .env.local.example.
std::atomic<std::shared_ptr> in logger.h does not compile on standard libraries that have not implemented P0718 (notably libc++, which this project selects on Apple), so any macOS build of the SDK or of downstream code including <ai/logger.h> failed. Feature-test __cpp_lib_atomic_shared_ptr, fall back to the atomic_load and atomic_store free functions elsewhere, and document the lifetime of the reference logger() returns. Treat a set-but-empty OPENAI_API_KEY or ANTHROPIC_API_KEY as absent through a shared utils::non_empty_env() helper; an empty value previously produced a client that failed only at request time. Also size the langfuse ISO-8601 buffer for snprintf's worst case, drop a std::move on nlohmann json subscripts that suppressed no copy, silence unused-parameter warnings in the tool factories and the base stream_text stub, and fix the stale designated-initializer example in core.h that no longer compiles.
The model constants predate the current provider lineups: the GPT-5.6 family and GPT-5.5 were missing, as were Claude Fable 5, Opus 5, Opus 4.8, and Sonnet 5, while long-retired identifiers (GPT-4o, GPT-4, GPT-3.5, the o-series, Claude Sonnet/Opus 4.0) were still advertised. Add the current identifiers with explicit dated snapshot constants, remove the retired ones, and move the defaults to gpt-5.6 and claude-sonnet-5. Removing the retired constants is a source-compatibility change for callers still referencing them. Two request-building accommodations come with the new models. GPT-5.6 defaults to reasoning on Chat Completions, which does not support function tools and can consume the whole completion budget without producing user-visible text, so requests pin reasoning_effort to "none". Recent Claude models reject sampling controls, so temperature and top_p are omitted for those model IDs with a warning log. Update supported_models(), mocks, fixtures, unit and integration tests, examples, and documentation accordingly. Tests that need a neutral model now use the literal "test-model" instead of a real identifier.
The OpenAI and Anthropic stream implementations had drifted into near-identical copies of the same threading, queueing, and SSE parsing code, and both mishandled several streaming cases: tool-call deltas were never assembled into events, finish events carried no usage or finish reason, API error bodies on non-200 responses were lost, and a stream could emit several terminal events. Introduce providers::streaming::HttpSseStream, which owns the stream thread, event queue, HTTP request and SSE line splitting, pending tool-call assembly, and emit-once finish semantics. Each provider implementation now only translates its SSE lines into stream events. Behavioral fixes that come with the rework: - Streamed tool calls are accumulated per index and emitted as tool-call events, including when a gateway closes the stream without [DONE] or content_block_stop. - Exactly one finish event terminates every stream, carrying usage and the real finish reason on success and kFinishReasonError with no usage on failure. OpenAI streams now request stream_options.include_usage, without which the API omits usage from streaming responses entirely. - Error bodies of non-200 responses are captured via a response handler (httplib routes them through the content receiver, leaving response.body empty) so auth and rate-limit failures stay diagnosable. A user-initiated stop is no longer reported as a network error. - Anthropic SSE parsing tolerates present-but-null JSON fields from gateways, keeps a tool_use block's initial input separate from streamed input_json_delta fragments, completes on message_stop instead of waiting for the connection to close, and no longer races or terminates on a second start_stream call.
The OpenAI factory already accepts a retry::RetryConfig so callers can tune retry behavior for unreliable networks, but the Anthropic factory offered no equivalent, leaving its clients pinned to the default policy. Add the matching create_client(api_key, base_url, retry_config) overload. The two public AnthropicClient constructors now delegate to one private constructor taking std::optional<retry::RetryConfig> rather than duplicating the ProviderConfig block.
GenerateResult::response_messages documents that it includes the assistant's response so callers can continue the conversation, but the multi-step coordinator only appended assistant turns on the tool-call feedback path. A run whose last step finished with stop (or any non-tool_calls reason) returned the step-1 assistant/tool exchange without the final answer. This regressed in the multi-step history fix (f9d8b40) and survived the coordinator refactor (09c423c); flagged by review on ClickHouse/ClickHouse#113959. Append the terminal step's assistant text to the accumulator on the non-tool_calls exits, and cover the contract with unit tests for the multi-step coordinator (which previously had none).
ClickHouse consumes this repository as a flat submodule and builds it with its own contrib CMake, so the nested zlib, googletest, and clickhouse-cpp submodules (used only for this repository's own tests and the langfuse module) must not appear as gitlinks. Same shape as the previous heads of this integration branch.
Collaborator
Author
|
Superseded by #57: with the nested submodules replaced by pinned FetchContent, ClickHouse can pin main directly and the remove-recursive-submodules integration branch is no longer needed. |
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.
Rebuilds the ClickHouse integration branch on current main (through #53) plus the response_messages terminal-reply fix (#55), with the usual flatten commit removing the nested zlib/googletest/clickhouse-cpp gitlinks.
Head commit b30aaef is the intended new submodule pin for ClickHouse/ClickHouse#113959.