feat(agent-manager): auto-generate conversation titles from the first message - #122
Conversation
|
Thanks for the implementation — the overall direction looks good, but I found a few blockers before I can approve. First, this branch needs to be rebased onto the current A few additional issues to address after the rebase:
After the branch is rebased and these points are addressed, I’d be happy to review it again. |
Address review on #122: - infrastructure/titles.py: build_titler() was forwarding only provider/name/max_tokens, dropping region/temperature/top_p. A Bedrock deployment with model.region set in YAML (not via env fallback) would crash the title model at startup while its agents worked fine. Forward the deployment's full model config; keep max_tokens as our own override, since a title is a handful of words regardless of what the agents are configured to answer with. - widget: refresh the open Threads panel after every completed turn. Titling runs concurrently with the turn and is bounded to 24 tokens, so it's almost always done before the turn's own stream finishes — this reflects a generated title without a manual reopen, at the cost of one cheap GET, and without a dedicated push channel.
36d4a55 to
3283a90
Compare
|
Thanks for the review — addressed 3 of the 4, and want to push back on the 4th with reasoning rather than just complying. 1. Rebase — done. Rebased onto current 2. Dropped 3. UI not refreshing while the panel stays open — fixed, without a push channel. Added 4. Short messages skipping the LLM — keeping this one, pushing back. None of the acceptance criteria actually say "must always call an LLM" — they say the title is automatically generated, based on the first message, short, and meaningful. A message like "אפס לי את הסיסמה" (16 chars) is already all three: it's a better title than anything a model would paraphrase it into, and it costs nothing. Removing the shortcut means burning a real LLM call on an estimated 30-40% of conversations for a strictly worse result in the common case, and better only in the (also common) case where the opening message legitimately needs summarizing — which the 48-char threshold already routes to the LLM. Would rather keep the threshold as documented, intentional behavior than "fix" it into paying for and sometimes downgrading titles that already work. Happy to tune the threshold if 48 chars feels wrong, but don't think the LLM should run unconditionally. Ready for another pass. |
|
Pushed a follow-up architectural change on top of the previous fixes, worth flagging explicitly since it changes the shape of the diff:
Net effect: All gates green ( |
|
Thanks — this is much closer now. The rebase/refactor alignment and model-config handling look good, and the CI is green. I still see one blocking issue in the UI update flow: The generated title is still produced in a background task, while the UI refreshes the thread list only once when the main turn finishes. That creates a race like:
So the current solution works only if title generation happens to finish before the main turn. The comment even says this is “usually” the case, but I think this needs to be deterministic. We need a mechanism that guarantees the UI eventually receives the generated title after generation actually completes — for example an explicit update/event, polling while the title is pending, or another approach consistent with the existing architecture. Two smaller points to align as well:
Once the UI race is fixed and these contract/details are aligned, I’m good with another pass. |
|
All three addressed. 1. The UI race — you were right, fixed properly. My previous answer defended it with "titling almost always finishes first," which is a probability where you correctly asked for a guarantee. It also wasn't even reliably true: a fast turn against a slow provider loses the race, up to the generator's 5s timeout. The turn's own SSE stream now carries the result, which turned out to have exact precedent —
Cost is nil in practice: naming starts with the turn, so by the time the stream ends it has normally already resolved and the await returns immediately. Worst case is bounded by the timeout that already existed, and only on a conversation's first turn. The event carries the settled title — generated, or the trim standing in for a failed generation — so a client relaying it never has to distinguish the two. Covered by 2. Short messages skipping the LLM — documented as intentional. Added a dedicated section to the PR description explaining the reasoning rather than leaving it implicit. 3. Stale On the configurability criterion specifically:
|
… message Generates a short title from a conversation's opening message using the system's configured LLM (or an override via EXTRA_TITLE_MODEL), once per conversation. The trimmed opening message is written synchronously as an immediate fallback title; the LLM-generated title overwrites it from a background task that never blocks or fails the turn it names. - domain/titles.py: TitleGenerator port (ABC), matching the Repository/ CallbackProvider convention. - infrastructure/titles.py: ConversationTitler adapter built on build_chat_model, provider/model agnostic. Total over any model output — bounded, unquoted across scripts, and never empty. - application/service.py: wires titling into the existing once-per- conversation hook; the call is isolated from the conversation's own history and token budget. - agent_engine/logging_config.py: log() now threads exc_info through, fixing a pre-existing bug where warning tracebacks were silently dropped. Closes #117
build_titler() forwarded only provider/name/max_tokens, dropping region/temperature/top_p. A Bedrock deployment with model.region set in YAML (not via env fallback) would crash the title model at startup while its agents worked fine. Forward the deployment's full model config; keep max_tokens as our own override, since a title is a handful of words regardless of what the agents are configured to answer with.
…gine agent_manager talked to a model directly for titling — the only place it ever did, everywhere else it depends on Engine for message -> response. Move that capability into agent_engine instead, matching how RunStatusEngine and ApprovalEngine already expose optional runtime capabilities: - agent_engine/engine/text_completion_engine.py: TextCompletionEngine, an optional Protocol for one stateless model call outside the compiled graph — no nodes, no tools, no prompt rendering. - LangGraphEngine implements it via defaults.model (region/temperature/ top_p included, via the now-public model_factory_kwargs helper shared with node model construction) and the engine's own callbacks. Built fresh per call rather than cached, so a per-call max_tokens reaches the provider through its own constructor kwarg instead of relying on every provider integration honoring an invoke-time override. - ConversationTitler now depends on TextCompletionEngine, not BaseChatModel — agent_manager no longer imports langchain anywhere. build_titler/parse_model_ref and EXTRA_TITLE_MODEL are gone: the composition root asks the already-built engine whether it can complete text and uses it if so, checked once at startup rather than per turn. This also drops the region/temperature/top_p forwarding fixed in the previous commit — the deployment's own model-construction path handles it now, the same way it already does for every agent's model.
…ing field, make complete() take a model Three fixes on top of the last commit's TextCompletionEngine: - build_model(): GraphBuilder._build_model and Engine.complete() both did factory(provider, name, temperature, **kwargs) by hand. One function in graph_builder.py now, used by both; the kwargs-filtering helper it wraps goes back to being module-private since nothing outside build_model calls it directly anymore. - Dropped RunnableConfig["tags"] from complete() — it had no precedent anywhere in the codebase, I added it without a reason. run_name stays (matches _thread_config's existing use for conversation turns) and is now commented as the external LangChain field it is. - complete() takes an optional `model: BaseModelConfig | None`, defaulting to defaults.model rather than being hardwired to it. The capability was otherwise unusable for a future caller that needs a specific model rather than the system's default; the data structure to express that already exists, so the parameter was the missing piece, not new plumbing.
…acing it Titling runs alongside the turn, but the widget refreshed the thread list exactly once when the turn's stream closed. If the title landed after that — a fast turn, a slow provider, anything up to the generator's 5s timeout — the UI kept the fallback until the panel was reopened. The previous fix reasoned that titling "almost always" finishes first, which is a probability, not the guarantee this needs. The turn's own SSE stream now carries the result, following the existing precedent that manager-level events (`turn_started`) are emitted by the route rather than the engine: - PreparedConversationTurn carries the background naming task, so the handle dies with the turn instead of outliving it in service-level state, and whoever delivers the turn can also deliver its title. - ConversationService.wait_for_generated_title() resolves once naming actually completes. Free for a live turn — naming starts with the turn, so it has usually finished by the time the stream ends — and bounded by the generator's own timeout otherwise. - The stream route emits `event: title` after `final`; the widget applies it directly and no longer guesses by refreshing on turn completion. The event carries the settled title, generated or the trim standing in for a failed generation, so a client relaying it never has to tell the two apart.
0130b6b to
71f332e
Compare
|
Thanks — this is much closer now. The previous UI race is addressed properly with the explicit I still see three issues I’d like to address before approving:
After the engine emits title = await service.wait_for_generated_title(turn)and only then allows the SSE stream to finish. Since title generation can wait up to 5 seconds, the frontend can still consider the execution active even though the actual agent response has already completed. In the widget, I think
If the client disconnects while the route is waiting for the title, cancellation of the request task can propagate to the title task as well. That means closing the browser/widget or losing the connection after the agent already finished could prevent the title from ever being persisted. The title generation should be independent background application work. At minimum the await should be cancellation-safe (
Currently await self._rename(conversation_id, title)
return titlebut So if The The deliberate short-message optimization is clear now in the PR description, so I don’t consider that a technical blocker anymore as long as that behavior is intentional. Once these lifecycle/consistency issues are addressed, I think this will be in very good shape for approval. |
|
Thanks — I verified all three points against the full request/application/UI lifecycle, and they were valid. Fixed in
The deeper pass found and fixed two adjacent correctness issues as well:
Validation:
I also rewrote the PR description so the documented lifecycle and test plan match the implementation now. |
Summary
Closes #117. When the first user message is accepted in a new conversation, the manager writes a trimmed fallback immediately and generates a short title with the system's configured default LLM in independent background work.
Layering
agent_engine/engine/text_completion_engine.pydefinesTextCompletionEngine, an optional capability Protocol for one stateless model call outside the compiled graph (no nodes, tools, or prompt rendering), following the existing optional engine-capability pattern.LangGraphEngineimplements it.complete()accepts an optionalBaseModelConfig, defaults todefaults.model, and uses the same shared model-construction path as graph nodes, including provider, region, temperature,top_p, and per-callmax_tokens.agent_manager/domain/titles.pydefines theTitleGeneratorport.agent_manager/infrastructure/titles.pyowns the prompt, timeout, normalization, and fallback.agent_managerdoes not import LangChain or construct models directly.defaults.modelkeeps the trimmed opening message as its title.Delivery and lifecycle
ConversationService, not by the SSE request.asyncio.shield, so disconnecting after the answer cannot cancel title generation or persistence.titleevent only after the title has settled and been persisted. If the generated-title write fails, no contradictory event is sent and the UI remains on the persisted fallback.finalandpending_approvalremain terminal for the main UI execution: the widget releases the composer immediately, while continuing to consume the same response for a latertitleevent. A slow title therefore does not block the next interaction or approval decision.Robustness
Deliberate: short opening messages skip the LLM
An opening message already short enough to be a title (≤48 characters after whitespace collapsing) is used as-is. This is intentional: a message such as
Reset my passwordis already short and meaningful, and a model paraphrase would add cost without reliably improving it. Longer openings use the LLM.Test plan
make format-check,make lint,make typecheck, andmake generate-checkorigin/mainsnapshot (test_agentctl_run_without_session_prints_reusable_generated_session)npm run typecheck:widget,npm run typecheck:e2e,npm run build:widget, andnpm run test:widgetfinal, a second turn before title delivery, UI thread update, request cancellation, persistence failure, secondary delivery failure, exact-once behavior after first-message edits, full model config, timeouts, normalization, budget/history isolation, and no-model fallbackAcceptance criteria
BaseModelConfigwithout changing the title feature