refactor: unify duplicated agent execution lifecycles into shared pipeline#509
Conversation
|
🤔 Just a sec, looking into that PR refactor now |
|
👋 I'm monitoring this PR. Currently waiting for:
Once the checks pass and reviews are submitted, I'll address any feedback. The refactoring looks clean - nice work extracting the shared execution pipeline! ✨ |
|
🔍 Reviewing code — Examining the PR changes for quality and correctness... |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean refactoring that extracts ~250 lines of duplicated orchestration code into a shared executeAgentPipeline(). The change is behavior-preserving, well-tested, and correctly structured.
Verification Notes
Behavior-preserving: Traced all key execution paths (success, failure, watchdog timeout, cleanup) and confirmed semantic equivalence:
- CWD save/restore ✓
- Env snapshot load/unload ✓
- Squint DB setup/teardown ✓
- Run tracking creation ✓
- Finalize run on all exit paths (success, failure, watchdog) ✓
- Cleanup in
finallyblock withskipRepoDeletion✓ - Log buffer priority (effectively unchanged — both backends return undefined
logBuffer, so fileLogger fallback is always used) ✓
Intentional behavioral changes (minor, improvements):
- Sentry
sourcetag unified frombackend_execution/agent_lifecycle→agent_execution(noted in PR, test updated) - Adapter watchdog now also reports to Sentry via
captureException(previously only lifecycle did this) — an improvement backendtag removed from adapter Sentry error reports (minor observability reduction, agent identifier still provides context)
Type duplication: LogWriter type now exists in 3 places (backends/types.ts, agents/utils/hooks.ts, agents/shared/executionPipeline.ts). The first two pre-date this PR. Consider consolidating to a single canonical export in a future pass.
Circular dependency chain: executionPipeline.ts → cleanup.ts → lifecycle.ts → executionPipeline.ts. This is safe because cleanup.ts and runTracking.ts only import types from lifecycle.ts, which get erased at runtime. No circular runtime dependency exists. Consider having cleanup.ts and runTracking.ts import FileLogger directly from executionPipeline.ts to make the dependency graph cleaner.
All 53 tests pass. CI is green. The strategy callback pattern is a good fit for this problem.
Summary
executeAgentPipeline()function insrc/agents/shared/executionPipeline.tsthat unifies the ~250 lines of identical orchestration code duplicated betweenadapter.ts(Claude Code backend) andlifecycle.ts(LLMist backend)executeWithBackend(adapter.ts) andexecuteAgentLifecycle(lifecycle.ts) to delegate the outer scaffold to the shared pipelinecreateLogWriterhelper (previously duplicated in adapter.ts) into the shared moduleexecuteAgentPipelinecovering: CWD restoration, watchdog integration, cleanup behavior, error handling, run tracking viasetRunId, andfinalizeMetadatapropagationcaptureExceptiontest to reflect that the shared pipeline usessource: 'agent_execution'(previouslybackend_executionin adapter.ts)What changed
New file:
src/agents/shared/executionPipeline.tsAgentPipelineOptionsinterface with anexecutestrategy callbackPipelineContextprovidingrepoDir,fileLogger,logWriter,runId,setRunIdexecuteAgentPipeline()— the 16-step shared scaffold: FileLogger → Watchdog → Repo setup → Env snapshot → Squint DB → Run tracking → CWD change → Execute → Restore CWD → Finalize run → CleanupcreateLogWriter()— shared helper extracted from adapter.tsRefactored:
src/backends/adapter.tsexecuteWithBackendnow delegates outer scaffold toexecuteAgentPipelinebuildBackendInput) moved insideexecutecallback usingctx.setRunId()Refactored:
src/agents/shared/lifecycle.tsexecuteAgentLifecyclenow delegates outer scaffold toexecuteAgentPipelinellmIterations,gadgetCalls) handled viaFinalizeRunOutcome.metadataNew file:
tests/unit/agents/shared/executionPipeline.test.tsTest plan
npm test)npm run typecheck)npm run lint)Card: https://trello.com/c/BR9j1dGp/93-refactor-unify-duplicated-agent-execution-lifecycles-in-adapterts-and-lifecyclets
🤖 Generated with Claude Code