refactor(llmist): unify llmist backend onto shared executeWithBackend adapter path#540
Conversation
|
🤖 Just a sec, looking into that PR for you Progress: [████░░░░░░] 44% (iteration 31/70) 🔍 Code Review Update (1 min) Last updated: iteration 31 · review |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Well-structured refactor that correctly unifies the llmist backend onto the shared executeWithBackend adapter path. The registry simplification and elimination of the special-case bypass are clean. However, there are two behavioral regressions where the new implementation doesn't wire up integrations that the old code path provided.
Code Issues
Should Fix
-
src/backends/llmist/index.ts:75-76 —
process.env.LLMIST_LOG_FILEis set to a temp path inos.tmpdir(), but the outer pipeline'sfileLogger.llmistLogPath(created bycreateFileLoggerinexecutionPipeline.ts) points to the workspace directory. During run finalization,storeRunLogsreads fromfileLogger.llmistLogPath(inrunTracking.ts:62-63), andgetZippedBuffer()also looks there. Since the llmist SDK writes to the temp path, the structured llmist log is orphaned — it won't appear in run records or log bundles. The oldlifecycle.ts:259code correctly set this tofileLogger.llmistLogPath. Fix: either exposellmistLogPathonAgentBackendInput, or have the adapter'sexecutecallback setprocess.env.LLMIST_LOG_FILE = ctx.fileLogger.llmistLogPathbefore callingbackend.execute(). -
src/backends/llmist/index.ts:82-107 —
createConfiguredBuilderis called without aprogressMonitor. The adapter creates aProgressMonitor(atadapter.ts:210-231) and passes it asinput.progressReporter, but the llmist backend never readsprogressReporterand doesn't pass it tocreateConfiguredBuilder. This means the observer hooks increateObserverHooks(athooks.ts:112-114) won't callprogressMonitor.onIteration(), so the progress accumulator will have zero data — progress updates to Trello/GitHub will be empty. The old code path inlifecycle.ts:267passedprogressMonitorthrough to the builder. Fix: passinput.progressReporteras theprogressMonitoroption tocreateConfiguredBuilder(theProgressMonitorclass already implements the right interface).
|
|
||
| // Point llmist at a temp log file for its internal structured log | ||
| const llmistLogPath = path.join(os.tmpdir(), `llmist-${agentType}-${Date.now()}.log`); | ||
| process.env.LLMIST_LOG_FILE = llmistLogPath; |
There was a problem hiding this comment.
[SHOULD_FIX] This sets LLMIST_LOG_FILE to a temp path, but the outer pipeline's fileLogger.llmistLogPath points to the workspace dir. During finalization (runTracking.ts:62-63), the log is read from fileLogger.llmistLogPath and included in getZippedBuffer(). Since the SDK writes to the temp path, the structured llmist log is lost from run records and log bundles.
The old lifecycle.ts:259 code set this to fileLogger.llmistLogPath. Since the backend doesn't have access to fileLogger, consider either:
- Adding a
llmistLogPathfield toAgentBackendInput, or - Having the adapter's
executecallback set the env var before callingbackend.execute()
There was a problem hiding this comment.
Fixed! Added llmistLogPath field to AgentBackendInput and passed fileLogger.llmistLogPath from the adapter. The llmist backend now sets process.env.LLMIST_LOG_FILE to this workspace path instead of a temp path, ensuring the structured llmist log is included in run records and log bundles.
| const gadgets = profile.getLlmistGadgets(agentType); | ||
|
|
||
| // Build the configured agent builder with all llmist-specific features: | ||
| // rate limiting, retry, compaction, iteration hints, observer hooks |
There was a problem hiding this comment.
[SHOULD_FIX] Missing progressMonitor parameter. The adapter creates a ProgressMonitor and passes it as input.progressReporter, but this backend never passes it to createConfiguredBuilder. As a result, createObserverHooks won't call progressMonitor.onIteration() (hooks.ts:112-114), and the progress accumulator will have zero state — progress updates to Trello/GitHub will be empty.
The old code path (lifecycle.ts:267) passed the progress monitor through to the builder. Fix:
progressMonitor: input.progressReporter as ProgressMonitor,(or pass it through a more type-safe way)
There was a problem hiding this comment.
Fixed! Now passing input.progressReporter to createConfiguredBuilder as the progressMonitor parameter. This ensures createObserverHooks can call onIteration(), onToolCall(), and onText() to populate the progress accumulator, enabling progress updates to Trello/GitHub.
Review Feedback Addressed ✅Thank you for the thorough review! Both behavioral regressions have been fixed: 1. llmist structured log integration ✅
2. Progress monitor integration ✅
Verification
Committed as: |
Fixes two behavioral regressions in the llmist backend unification: 1. **llmistLogPath integration** — The llmist SDK now writes its structured log to the workspace directory (provided via `input.llmistLogPath` from the outer pipeline's `fileLogger`). Previously it wrote to a temp path, which caused the log to be orphaned and excluded from run records and log bundles. 2. **progressMonitor integration** — The backend now passes `input.progressReporter` to `createConfiguredBuilder` as the `progressMonitor` parameter. This ensures `createObserverHooks` can call `onIteration()`/`onToolCall()`/`onText()`, enabling progress updates to Trello/GitHub. Previously the monitor was created by the adapter but never wired through to the builder. Changes: - Added `llmistLogPath` field to `AgentBackendInput` interface - Adapter passes `fileLogger.llmistLogPath` when building backend input - Llmist backend sets `process.env.LLMIST_LOG_FILE` to the provided path - Llmist backend passes `progressReporter` as `progressMonitor` to builder - Added tests verifying both integrations Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
🤖 Just a sec, looking into that PR for you Progress: [█████░░░░░] 46% (iteration 32/70) 🔍 Code Review Update (1 min) I've completed a review of how the new Last updated: iteration 32 · review |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean, well-structured refactoring that eliminates the special-case bypass in registry.ts and unifies both backends onto the shared executeWithBackend adapter path. The design decisions are sound, CI passes, and tests are thorough.
Architecture & Design
The key design choice — having LlmistBackend.execute() call runAgentLoop directly instead of executeAgentLifecycle — correctly avoids nesting two pipeline scaffolds. The outer executeAgentPipeline (via adapter.ts) provides the lifecycle (repo setup, CWD, env, watchdog, logging, finalization), while the inner execute() only handles llmist-specific concerns (builder configuration, synthetic call injection, agent loop). This is the right split.
The removal of the specialized executor dispatch table (specializedExecutors) is justified — getAgentProfile().getLlmistGadgets() now serves as the single source of truth for per-agent-type tool sets, replacing the per-executor imports from base.ts, review.ts, respond-to-ci.ts, etc.
Minor Observations (non-blocking)
LLM call file logging to temp dir: The new LlmistBackend.execute() creates its own createLLMCallLogger(os.tmpdir(), ...) rather than using the outer fileLogger.llmCallLogger. This means per-call request/response text files won't be included in the ZIP bundle (fileLogger.getZippedBuffer()). In practice this is benign because: (1) real-time DB logging via storeLlmCall is active when runId is present, so the data is accessible from the dashboard; (2) the cascade.log and llmist.log files are included in the ZIP correctly via llmistLogPath. If you want parity with the old lifecycle path's ZIP contents, you could pass fileLogger.llmCallLogger through the AgentBackendInput in a follow-up, but this is not a regression that affects functionality.
process.env.LLMIST_LOG_FILE not cleaned up: The env var is set but never restored after execution. This matches the existing behavior in lifecycle.ts:259-260, so it's not a regression — just a pre-existing pattern worth noting for future cleanup.
Summary
Unifies the llmist backend onto the shared
executeWithBackendadapter path that the claude-code backend already uses, eliminating the special-case bypass inregistry.tsthat caused the two execution paths to diverge.Card: https://trello.com/c/FvhG64kh/112-find-top-candidate-for-refactoring-and-plan-clean-refactoring-of-it-look-for-god-classes-modules-functions-files-code-duplicatio
What changed
src/backends/llmist/index.ts— RewroteLlmistBackend.execute()to receive a fully pre-resolvedAgentBackendInputand run the llmist SDK directly (no delegation to legacy per-agent executors). UsesgetAgentProfile().getLlmistGadgets()for gadget instantiation, convertsContextInjection[]to synthetic gadget calls viainjectSyntheticCall, and builds the agent withcreateConfiguredBuilderwhich preserves all llmist-specific features (loop detection, compaction, iteration hints, rate limiting, retry).src/agents/registry.ts— Removed the 27-line special-case that bypassedexecuteWithBackendfor the llmist backend and passed a minimal stub input. All backends now go throughexecuteWithBackend, which provides: repo setup, CWD change/restore, env var loading, run record creation, log finalization, progress monitor, and watchdog.tests/unit/backends/llmist.test.ts— Rewrote tests to match the new implementation. Tests now verify the unified path: gadget loading from profiles, synthetic call injection fromcontextInjections, loop termination handling, PR URL extraction, and correct parameter passing tocreateConfiguredBuilder.tests/unit/agents/registry.test.ts— Updated test for the llmist path to verify it now callsexecuteWithBackend(like all other backends), removing the assertion that it bypassed the adapter.Key design decisions
execute()method does NOT callexecuteAgentLifecycle— that would nest two pipeline scaffolds. Instead, it runs the llmist SDK directly, leveraging the outerexecuteAgentPipeline(fromadapter.ts) for lifecycle management.createLLMCallLoggerintoos.tmpdir()since the outer pipeline'sFileLoggeris not exposed to backends. Real-time per-call metrics still work via therunIdpassed tocreateObserverHooksinsidecreateConfiguredBuilder.getAgentProfile().getLlmistGadgets()— previously the llmist executors inbase.ts,review.ts,respond-to-ci.ts, etc. each had their own gadget lists; now one function covers all.Preserved llmist features
runAgentLoop+createObserverHooks)createConfiguredBuilder+getCompactionConfig)createConfiguredBuilder+getIterationTrailingMessage)createConfiguredBuilder)injectSyntheticCall)Test plan
npm test)npm run typecheck)npm run lint)executeWithBackend🤖 Generated with Claude Code