feat: summarizer surface on LCMEngine + lcm_synthesize_around adapter — closes #156 (#164 PR-2)#166
Open
100yenadmin wants to merge 2 commits into
Open
feat: summarizer surface on LCMEngine + lcm_synthesize_around adapter — closes #156 (#164 PR-2)#166100yenadmin wants to merge 2 commits into
100yenadmin wants to merge 2 commits into
Conversation
added 2 commits
May 19, 2026 23:10
#164 PR-2: build the summarizer surface on LCMEngine and wire the 8th dispatch adapter, closing #156 at 8/8 tool-dispatch coverage. Summarizer surface (engine/__init__.py + engine/lifecycle.py): on_session_start now constructs a HermesSummarizerDeps (PR-1's shim) and an LcmSummarizer from it, exposing engine.deps (SummarizerDeps), engine.summarize (the bound SummarizeFn), and engine._summarizer (the LcmSummarizer object the build_llm_call factory needs for .candidates). Built once per DB-open inside the `if self._db is None:` block — config-scoped, not session-scoped — and nulled in on_session_end for symmetry with the stores. This also activates /lcm doctor apply's real summarizer path: commands/doctor.py's getattr(engine, "deps") probe now resolves, so apply_scoped_doctor_repair only renders "unavailable" when the candidate chain is genuinely empty rather than unconditionally. lcm_synthesize_around adapter (tools/_adapters.py): _adapt_lcm_synthesize _around builds a frozen _SynthesizeAroundCtx and a build_llm_call factory that wires the synthesis dispatcher's async LlmCall to the engine's configured summarizer chain — a faithful port of TS buildLlmCallFrom Summarizer (lcm-synthesize-around-tool.ts:608-618 @ 1f07fbd) plus the TS handler's summarizer wiring (:1019-1037). The factory bridges sync LcmSummarizer.summarize -> async LlmCall and records the resolved primary candidate model (Wave-12 F8 audit-honesty). Registered into TOOL_DISPATCH in engine/__init__.py. #156 close-out: _NOT_YET_ADAPTED in test_dispatch_registry_coverage.py is now empty; the advertised-surface test additionally asserts set(TOOL_ DISPATCH) equals the advertised surface (8/8). test_tool_dispatch.py's exact-membership test updated to include lcm_synthesize_around. Closes #156
…ADME knob Direct tests for #164 PR-2 plus the operator-facing knob doc. tests/test_summarizer_surface.py — pins the summarizer surface on LCMEngine: the three handles (deps / summarize / _summarizer) are None on a bare engine and post-on_session_end, populated between; deps satisfies the SummarizerDeps Protocol; on_session_start is idempotent (the surface is config-scoped, identity stable on re-entry); engine.summarize round-trips against a fake complete(); and the /lcm doctor apply confirmation — _resolve_doctor_apply_summarize resolves the engine's real surface to a usable summarizer (not the "unavailable" arm) and still returns None on a pre-init engine (the arm is conditional, not dead and not always-on). tests/tools/test_synthesize_around_adapter.py — direct coverage for the adapter (the #161 review established an adapter must not ship with zero direct tests; mirrors test_compact_adapter.py): _build_synthesizer_llm _call yields an awaitable LlmCall whose output is the summary and whose actual_model is the resolved primary candidate (Wave-12 F8), and raises on a bare engine / empty candidate chain; _adapt_lcm_synthesize_around degrades to a structured tool-error when engine state is unset, builds SynthesizeAroundContext correctly and dispatches end-to-end (a non-error tool_result + a lcm_synthesis_cache row in status='ready'), and degrades to "No summarization model resolved" when the summarizer is unconfigured; _SynthesizeAroundCtx structurally satisfies the SynthesizeAroundContext Protocol. README.md — documents the optional auxiliary.lcm_summary Hermes config entry (provider/model/timeout for LCM summarization + synthesis), folding in the PR #165 review MINOR; mirrors hermes_llm.py's module docstring into the operator-facing configuration section.
📝 WalkthroughWalkthroughCompletes dispatch adapter coverage by implementing ChangesLCM Summarize-Around Dispatch Adapter & Summarizer Surface
Sequence Diagram(s)sequenceDiagram
participant Engine as LCMEngine
participant Lifecycle as on_session_start
participant Summarizer as LcmSummarizer
participant Adapter as _adapt_lcm_synthesize_around
participant Handler as handle_lcm_synthesize_around
participant DB as lcm_synthesis_cache
Lifecycle->>Summarizer: __init__ with config<br/>(summary_provider hint)
Summarizer-->>Lifecycle: self._summarizer ready<br/>(candidates resolved)
Lifecycle->>Engine: engine.summarize = synthesizer.summarize
Lifecycle->>Engine: engine._summarizer = summarizer
Adapter->>Engine: check _db, _conversation_store
Adapter->>Summarizer: _build_synthesizer_llm_call()<br/>lazy factory
Adapter->>Handler: _SynthesizeAroundCtx with factory
Handler->>Adapter: await build_llm_call()
Adapter->>Summarizer: summarize() + timing
Summarizer-->>Adapter: LlmCallResult (text, actual_model)
Handler->>DB: INSERT lcm_synthesis_cache<br/>status='ready', resolved_model
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lossless_hermes/engine/lifecycle.py (1)
288-345:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against partial initialization when summarizer setup fails.
self._dband stores are assigned before summarizer construction. If that block throws once, the engine is left “DB-open but summarizer-unset”; lateron_session_startcalls short-circuit onself._db is not None, so the summarizer surface never recovers.Suggested fix
- # Wire state onto the shell class. The four stores share the - # one connection — ADR-017 §"Synchronous by design" means each - # store is a thin CRUD wrapper, not a lifecycle owner. - self._db = conn - self._conversation_store = ConversationStore( + # Build collaborators first; only publish to self after all + # construction succeeds to avoid partial-init state. + conversation_store = ConversationStore( conn, fts5_available=features.fts5_available, ) - self._summary_store = SummaryStore( + summary_store = SummaryStore( conn, fts5_available=features.fts5_available, trigram_tokenizer_available=features.fts5_trigram_available, ) - self._telemetry_store = CompactionTelemetryStore(conn) - self._maintenance_store = CompactionMaintenanceStore(conn) + telemetry_store = CompactionTelemetryStore(conn) + maintenance_store = CompactionMaintenanceStore(conn) @@ - self.deps = HermesSummarizerDeps() + deps = HermesSummarizerDeps() @@ - summarizer = LcmSummarizer( - deps=self.deps, + summarizer = LcmSummarizer( + deps=deps, config=self.config, provider_hint=self.config.summary_provider or None, model_hint=self.config.summary_model or None, ) - self._summarizer = summarizer - # ``self.summarize`` is the bound ``LcmSummarizer.summarize`` — - # the ``SummarizeFn``-shaped callable downstream consumers - # (``CompactionEngine.compact_full_sweep``, the - # ``lcm_synthesize_around`` ``build_llm_call`` factory) expect. - self.summarize = summarizer.summarize + summarize = summarizer.summarize + + # Publish once everything succeeded. + self._db = conn + self._conversation_store = conversation_store + self._summary_store = summary_store + self._telemetry_store = telemetry_store + self._maintenance_store = maintenance_store + self.deps = deps + self._summarizer = summarizer + self.summarize = summarize🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lossless_hermes/engine/lifecycle.py` around lines 288 - 345, The code assigns self._db and the various stores before constructing the summarizer, so if HermesSummarizerDeps/LcmSummarizer construction fails the engine remains “DB-open but summarizer-unset” and future on_session_start short-circuits; fix by constructing the summarizer (create HermesSummarizerDeps(), then LcmSummarizer and bind self._summarizer/self.summarize) before assigning self._db and initializing ConversationStore, SummaryStore, CompactionTelemetryStore, and CompactionMaintenanceStore, or alternatively wrap summarizer construction in try/except and on exception undo/reset self._db and any created stores so the object stays in a consistent state (references: HermesSummarizerDeps, LcmSummarizer, self._summarizer, self.summarize, self._db, ConversationStore, SummaryStore, CompactionTelemetryStore, CompactionMaintenanceStore, on_session_start).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lossless_hermes/engine/lifecycle.py`:
- Around line 288-345: The code assigns self._db and the various stores before
constructing the summarizer, so if HermesSummarizerDeps/LcmSummarizer
construction fails the engine remains “DB-open but summarizer-unset” and future
on_session_start short-circuits; fix by constructing the summarizer (create
HermesSummarizerDeps(), then LcmSummarizer and bind
self._summarizer/self.summarize) before assigning self._db and initializing
ConversationStore, SummaryStore, CompactionTelemetryStore, and
CompactionMaintenanceStore, or alternatively wrap summarizer construction in
try/except and on exception undo/reset self._db and any created stores so the
object stays in a consistent state (references: HermesSummarizerDeps,
LcmSummarizer, self._summarizer, self.summarize, self._db, ConversationStore,
SummaryStore, CompactionTelemetryStore, CompactionMaintenanceStore,
on_session_start).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 21325fd9-46ed-4871-ad1e-a74e9d654c88
📒 Files selected for processing (8)
README.mdsrc/lossless_hermes/engine/__init__.pysrc/lossless_hermes/engine/lifecycle.pysrc/lossless_hermes/tools/_adapters.pytests/test_dispatch_registry_coverage.pytests/test_summarizer_surface.pytests/test_tool_dispatch.pytests/tools/test_synthesize_around_adapter.py
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.
Summary
PR 2 of the unified 5-PR compaction-P0 sequence (#164). Builds the summarizer surface on
LCMEngineand wires the 8th dispatch adapter, closing #156 at 8/8 tool-dispatch coverage. Builds on PR 1 (#165, merged) which landedHermesSummarizerDeps.Follows the #164 unified plan's §4 (PR-2), Q-F (construction point), and Q-G (
lcm_synthesize_aroundwiring).1 · The summarizer surface (
engine/__init__.py+engine/lifecycle.py)on_session_startnow constructs aHermesSummarizerDeps(PR-1's shim) and anLcmSummarizerfrom it, exposing three engine handles:engine.deps— the publicSummarizerDeps.engine.summarize— the boundLcmSummarizer.summarize, theSummarizeFn-shaped callable ((text, aggressive=False, options=None) -> str) thatCompactionEngine.compact_full_sweep(PR 3) and thelcm_synthesize_aroundbuild_llm_callfactory consume.engine._summarizer— theLcmSummarizerobject (thebuild_llm_callfactory reads.candidatesoff it for the Wave-12 F8 audit-honest model name).Built once per DB-open inside the non-re-entrant
if self._db is None:block — config-scoped, not session-scoped (a re-entranton_session_startfor a new session on an already-open DB does not rebuild it; identity stable). Nulled inon_session_endfor symmetry with the four stores. ThreeOptionalstate fields added toLCMEngine.__init__, defaultingNone;SummarizerDeps/LcmSummarizer/SummarizeFnimported underTYPE_CHECKING(annotation-only here — construction is lazy inlifecycle.py) to dodge any engine↔summarize cycle.2 · The
lcm_synthesize_arounddispatch adapter (tools/_adapters.py)_adapt_lcm_synthesize_around— the deferred #156 PR-3 work, unblocked by the summarizer surface. Unlike the four PR-1 attribute-bag adapters,SynthesizeAroundContext.build_llm_callis a factory (() -> tuple[LlmCall, str])._SynthesizeAroundCtx— a frozen dataclass structurally satisfyingSynthesizeAroundContext(conn/conversation_store/timezone/build_llm_call).conn/conversation_store/timezonemap from engine state like the other adapters;depsreuses the existing_build_depshelper._build_synthesizer_llm_call— theBuildLlmCallfactory body, a faithful port of the TSbuildLlmCallFromSummarizer(lcm-synthesize-around-tool.ts:608-618@1f07fbd) composed with the TS handler's own summarizer wiring (:1019-1037). It closes overengine._summarizer, bridges sync→async (LlmCallisasync,LcmSummarizer.summarizeis sync per ADR-017), passesLcmSummarizeOptions(is_condensed=True)(synthesis is a condensed-tier rollup — TS{isCondensed: true}), measures latency, and recordsactual_modelas the resolved primary candidate (Wave-12 F8 audit-honesty). It raisesRuntimeErroron an unset/empty summarizer; the handler's existingtry/exceptaroundctx.build_llm_call()converts that into the structured "No summarization model resolved" tool-error.TOOL_DISPATCH["lcm_synthesize_around"] = _adapt_lcm_synthesize_around.3 · #156 close-out — dispatch coverage 8/8
sorted(TOOL_DISPATCH)is now:lcm_compact, lcm_describe, lcm_doctor, lcm_get_entity, lcm_grep, lcm_search_entities, lcm_status, lcm_synthesize_around— 8 tools, equal to the advertisedget_tool_schemas()surface.lcm_expandstays deferred per ADR-037 (absent from bothTOOL_DISPATCHandTOOL_SCHEMAS)._NOT_YET_ADAPTEDintests/test_dispatch_registry_coverage.pyis now empty; the xfail ratchet is fully discharged (noXPASSfailures — confirmed locally). The advertised-surface test additionally assertsset(TOOL_DISPATCH)equals the advertised surface.tests/test_tool_dispatch.py's exact-membership test (test_tool_dispatch_holds_diagnostic_and_pr1_pr2_adapted_tools) asserted the un-wired state — it explicitly expectedlcm_synthesize_aroundabsent ("lands in [P0] Core lcm_* agent tools never dispatch — dispatch-adapter layer never built #156 PR-3"). Renamed totest_tool_dispatch_holds_all_eight_adapted_toolsand updated to include it, preserving its real intent (a tight membership pin that fails if a tool is wired before its prerequisites or dropped).4 ·
/lcm doctor applyside-effect (confirmed, not regressed)commands/doctor.py:261-262probesgetattr(engine, "deps"/"summarize", None). Before this PR both always returnedNone, soapply_scoped_doctor_repairhit itskind="unavailable"arm unconditionally. Nowengine.summarizeis a non-Nonecallable andengine.depsis a realSummarizerDeps—_resolve_doctor_apply_summarizeresolves a usable summarizer, so the "unavailable" arm no longer fires. A test pins both directions: the resolver returns a usable summarizer from the engine's surface, and still returnsNoneon a pre-init engine (the arm is conditional, not dead, not always-on).5 · README knob-docs
Documents the optional
auxiliary.lcm_summaryHermes config entry (provider/model/timeout for LCM summarization + synthesis), folding in the PR #165 review MINOR — mirrorshermes_llm.py's module docstring into the operator-facing configuration section.Test plan
New test files (mirror
tests/tools/test_compact_adapter.py; the #161 review established an adapter must not ship with zero direct coverage):tests/test_summarizer_surface.py— surface presence/null-out,SummarizerDepsconformance,on_session_startidempotence,engine.summarizeround-trip against a fakecomplete(), both doctor-apply confirmations.tests/tools/test_synthesize_around_adapter.py—_build_synthesizer_llm_call(awaitableLlmCall,actual_model= primary candidate, raises on bare/empty-chain engine);_adapt_lcm_synthesize_around(engine-state-unset structured error, end-to-end dispatch writing alcm_synthesis_cacherow, degrade-to-tool-error when unconfigured);_SynthesizeAroundCtxstructural conformance.tests/test_dispatch_registry_coverage.py+tests/test_tool_dispatch.py— ratchet flipped to 8/8.Verified locally (LEXAR repo): the new/changed test files +
test_dispatch_registry_coverage.py+test_tool_dispatch.py+test_compact_adapter.py(104 passed), plustest_lcm_synthesize_around.py/test_hermes_llm.py/test_handle_tool_call_ingest.py/test_lifecycle.py/test_engine_state_fields.py(no regression).ruff check/ruff format/ty check srcclean (the one pre-existingtyunused-ignore-commentwarning indb/connection.pyis untouched by this PR andtyexits 0). Full suite verification is via GitHub CI ({macOS, ubuntu} × {py3.11, 3.12, 3.13}).Part of #164.
Closes #156.Summary by CodeRabbit
New Features
lcm_synthesize_aroundtool implementation with full dispatch adapter support (8/8 coverage).Documentation
Tests