refactor: extract shared helpers to reduce code complexity across codebase - #2
Closed
terryso wants to merge 60 commits into
Closed
refactor: extract shared helpers to reduce code complexity across codebase#2terryso wants to merge 60 commits into
terryso wants to merge 60 commits into
Conversation
…ication and cyclomatic complexity: SandboxChecker.matchesAllowedPathList(), OpenAIClient.buildRequestBody(), and Agent.closedQueryResult().
…eateTraceRecorder) from duplicated code in Agent.swift's two largest methods, reducing promptImpl from 678→625 lines and stream from 1120→1092 lines, with all 6209 tests passing.
…essionMessages) that eliminated 7 duplicated inline blocks across promptImpl and stream, reducing promptImpl from 641→610 lines and stream from 1108→1078 lines with all 6212 tests passing.
…createSubAgentSpawner) that eliminated duplicated skill resolution logic across executeSkill/executeSkillStream and SubAgentSpawner construction across promptImpl/stream, with all 5682 tests passing.
… executeToolWithLifecycle) from ToolExecutor.swift's executeSingleTool method, eliminating 5× duplicated error-return patterns, 2× ToolStartedEvent emission blocks, and 2× full tool execution lifecycle blocks, reducing executeSingleTool from ~250→99 lines with all 6211 tests passing.
…rver) from MCPClientManager.swift, eliminating 3× duplicated 6-case switch dispatch blocks across connectAll/reconnect/setServers and 2× duplicated 4-line server removal blocks in setServers, reducing the file from 622→610 lines with all 5684+12 tests passing.
…sultFromExecute, executionErrorResult) from ToolBuilder.swift, eliminating duplicated input decoding pipeline across CodableTool/StructuredCodableTool (2×26 identical lines), duplicated result mapping across RawInputTool/StructuredCodableTool (2×13 identical lines), and duplicated error-return catch blocks across 3 tool structs (3×6 identical lines), reducing the file from 430→409 lines with all 5682+12 tests passing.
…ppendFallbackEntry) from Compact.swift, eliminating 3× duplicated LLM response text extraction blocks and 3× duplicated fallback SessionMemoryEntry construction blocks, reducing the file from 485→477 lines with all 5682+12 tests passing.
…oryStore.swift, eliminating 2× duplicated 26-line filter/sort/limit blocks across InMemoryStore.query and FileBasedMemoryStore.query, reducing the file from 391→377 lines with all 5682+12 tests passing.
…ponse, stripCodeFences) into new LLMResponseHelpers.swift, eliminating 3× duplicated extractTextFromResponse implementations and 3× duplicated stripCodeFences implementations across LLMSkillEvolver.swift, PromptEvolverEngine.swift, and LLMExperienceExtractor.swift, reducing those 3 files by 93 lines combined with all 5682+12 tests passing.
…hropicClient.swift (eliminating 2×11-line duplicated URLError catch blocks) and validatePathSafeIdentifier() into EnvUtils.swift (eliminating 3×10-line duplicated path traversal validation methods across SessionStore, FactStore, and MemoryStore), net -18 lines with all 5682+12 tests passing.
…SkillsDir, atomicWriteJSON) into EnvUtils.swift, eliminating 2× duplicated skills directory resolution blocks (~15 lines each) and 2× duplicated atomic JSON file write patterns (~40 lines each) from SkillUsageStore and SkillCuratorStore, net -36 lines with all 5684+12 tests passing.
…ryDir()/defaultMemoryDir shared helpers into EnvUtils.swift, eliminating 9× duplicated 5-line dateFormatter initializations across store files and Logger, plus 2× duplicated ~18-line resolveMemoryDir methods from MemoryStore and FactStore, net -71 lines with all 5682+12 tests passing.
…wift (unifying 3 duplicated JSON parsing implementations from Agent.swift, OpenAIClient.swift, and Streaming.swift) and replaced 4 inline ISO8601DateFormatter constructions in RunTracker, RunRecoveryService, and ExperienceTypes with makeISO8601DateFormatter() calls, net -26 lines with all 6212+12 tests passing.
…lidateLLMHTTPResponse, resolveBaseURL) into LLMClient.swift, unifying duplicated URLError handling, HTTP response validation, and URL resolution between AnthropicClient and OpenAIClient, net -33 lines with all 6212+12 tests passing.
…ted, defaultHomeDir) eliminating 5× duplicated stop+sessionEnd hook blocks, 2× duplicated LLMRequestStartedEvent emissions, and 2× duplicated home directory resolution blocks, net -12 lines with all 5682+12 tests passing.
…BudgetExceededInStream) eliminating 2× duplicated error classification blocks, 3× duplicated budget check patterns, and 2× duplicated ~35-line budget exceeded handling blocks, net -3 lines with all 5682+12 tests passing.
…LMResponseAsObject, parseLLMResponseAsArray) into LLMResponseHelpers.swift, eliminating the identical trim→stripCodeFences→data→JSONSerialization pipeline duplicated across LLMSkillEvolver, PromptEvolverEngine, and LLMExperienceExtractor, with all 6212+12 tests passing.
…keSDKJSONDecoder) into EnvUtils.swift, eliminating 3× duplicated djb2 hash implementations + 1 inline hash + 3× duplicated JSON encoder/decoder initialization blocks + 3× inline decoder constructions across 6 files, net -28 lines with all 5682+12 tests passing.
…ed ~12-line skill reconstructions in review tools, and replaced inline JSONEncoder/JSONDecoder in MemoryBundleExport/ImportService with shared makeSDKJSONEncoder/makeSDKJSONDecoder factories, net -28 lines with all 5682+12 tests passing.
…tInterrupted and emitAgentFailed in Agent) eliminating 7× duplicated Logger+SDKError denial blocks and 8× duplicated if-let-eventBus+AgentTerminalEvent emission patterns, net -6 lines with all 5682+12 tests passing.
…fy) into EnvUtils.swift, eliminating the last remaining #if os(Linux) home directory resolution pattern from SessionStore and unifying 2 duplicated JSON serialization functions from ConfigTool and ReadMcpResourceTool, net -25 lines with all 5682+12 tests passing.
…xt) centralizing 4× duplicated LLMCostEvent constructions and 2× duplicated 24-field ToolContext constructions across promptImpl and stream, with all 5682+12 tests passing.
…orResponse, requireNonEmptyInput) centralizing 4× duplicated createDirectory+error blocks across store files and 14× duplicated error response patterns + 9× duplicated non-empty guard patterns across review tools, net -41 lines with all 5682+12 tests passing.
…ft, centralizing 4× duplicated ~28-line cost-tracking pipelines (TokenUsage parsing → estimateCost → recordUsage → emitLLMCostEvent → recordCostBreakdown) across promptImpl fallback path, promptImpl main loop, stream messageStart, and stream messageDelta, reducing Agent.swift from 3619→3586 lines with all 5682+12 tests passing.
…efaultSubAgentSpawner.swift and refactored simple spawn to delegate to enhanced spawn, reducing the file from 197→169 lines (-28 lines) by eliminating ~45 lines of duplicated tool filtering, Agent creation, and result processing logic across 2 spawn methods, with all 5682+12 tests passing.
…Value, anyToMCPValue, mcpValueToAny) into MCPTypes.swift, eliminating 8 duplicated private methods across InProcessMCPServer, MCPClientManager, and AgentMCPServer, net -54 lines with all 5682+12 tests passing.
…ver, createMCPSession) and moved ToolExecutionError to MCPTypes.swift, eliminating duplicated tool registration loop (~45 lines), session creation (2×5 lines), and error struct (2×8 lines) across InProcessMCPServer and AgentMCPServer, net -28 lines with all 5682+12 tests passing.
… EnvUtils.swift and 2 default directory constants (defaultTracesDir, defaultApiRunsDir), eliminating duplicated YAML escaping from CuratorRunReport and SkillWriter and inline home directory resolution from TraceRecorder and RunPersistenceService, net -6 lines with all 5682+12 tests passing.
…ate) into TestHarness.swift, eliminating 3× duplicated makeTempDir, 3× duplicated cleanup, and 2× duplicated date function definitions across SkillUsageTrackerE2ETests, SkillCuratorE2ETests, and SessionSearchE2ETests, net -28 lines with all 6212+12 tests passing.
…liminating duplicated Process setup code (executable URL, arguments, cwd, environment merging) between launchBackgroundProcess and executeBashProcess, with all 5682+12 tests passing.
…(eliminating 6× duplicated `cwd ?? FileManager.default.currentDirectoryPath` in Agent.swift) and replaced private `stripMarkdownFences` in Compact.swift with the shared `stripCodeFences` from LLMResponseHelpers.swift, net -7 lines with all 5682+12 tests passing.
…izing the duplicated guard+validate+update pattern across 4 state transition methods (startRun, completeRun, failRun, cancelRun), and replaced 4 raw JSONEncoder()/JSONDecoder() constructions in RunPersistenceService.swift with shared makeSDKJSONEncoder()/makeSDKJSONDecoder() factories, net -10 lines with all 5682+12 tests passing.
…erties in AgentEventTypesE2ETests.swift, eliminating 21× duplicated JSONEncoder+iso8601 and 17× duplicated JSONDecoder+iso8601 construction blocks, net -23 lines with all 5682+12 tests passing.
…umentation test files, eliminating 548 lines of duplicated helper methods (projectRoot, examplesDir, fileContent, packageSwiftContent) that were identically copied across all files, with all 5682+12 tests passing.
…es in AgentEventTypesTests.swift, eliminating 37× duplicated JSONEncoder+iso8601 and 64× duplicated JSONDecoder+iso8601 construction blocks, net -85 lines with all 5682+12 tests passing.
…es in SkillEvolutionTypesTests.swift, centralizing 4 iso8601-configured and 10 raw JSONEncoder/JSONDecoder sites into 2 shared properties, while correctly preserving 4 SkillSignal test sites that require the default .deferredToDate date strategy, net -5 lines with all 5682+12 tests passing.
…n MemoryFactTests and TokenStreamingEventTests (centralizing 3 iso8601 encoder+decoder sites), and replaced 2 raw ISO8601DateFormatter constructions in FactStoreTests with makeISO8601DateFormatter(), with all 5682+12 tests passing.
…, eliminating 276 lines of duplicated setUp/tearDown tempDir boilerplate (13× ~21 lines each) into a single 31-line shared base class, net -245 lines with all 5682+12 tests passing.
…Helpers.swift and replaced 436 duplicated guard-let-XCTFail-return blocks across 15 documentation test files with single-line try requireFileContent() calls, saving 1308 lines with all 5682+12 tests passing.
…Tests, FileCacheTests, MemoryReviewHookTests) to inherit from TempDirTestCase instead of XCTestCase, eliminating 125 lines of duplicated setUp/tearDown tempDir boilerplate with all 5682+12 tests passing.
…, FrozenSnapshotTests, SkillLoaderTests) to inherit from TempDirTestCase instead of XCTestCase, eliminating 41 lines of duplicated setUp/tearDown tempDir boilerplate with all 5682+12 tests passing.
…wift to inherit from TempDirTestCase, eliminating 152 lines of duplicated setUp/tearDown tempDir boilerplate with all 5682+12 tests passing.
…ests 2 classes, SessionManagementWiringATDDTests 3 classes, SessionSearchEngineTests 1 class) to inherit from TempDirTestCase instead of XCTestCase, eliminating 64 lines of duplicated setUp/tearDown tempDir boilerplate with all 5682+12 tests passing.
…, CombinedSessionOptionsWiringTests, StreamSessionWiringTests) in SessionManagementWiringATDDTests.swift to inherit from TempDirTestCase instead of XCTestCase, eliminating 18 lines of duplicated setUp/tearDown tempDir boilerplate with all 5682+12 tests passing.
…grated 2 remaining test files (TraceRecorderTests, ProjectDocumentDiscoveryTests) to inherit from TempDirTestCase instead of XCTestCase, eliminating 21 lines of duplicated setUp/tearDown tempDir boilerplate with all 5682+12 tests passing.
…JSONResponse error patterns with reviewErrorResponse() helper, replacing manual Skill reconstruction with skill.withBaseDir(), and replacing manual empty-name guard with requireNonEmptyInput(), reducing the file from 125→105 lines (-20 lines) with all 5682+12 tests passing.
…makeMockURLSession) into MockURLProtocolHelpers.swift, eliminating 10 duplicated readBodyFromStream method definitions across 8 files and 17 duplicated URLSessionConfiguration.ephemeral patterns across 15 files, net -158 lines with all 5682+12 tests passing.
…eTempGitRepo, cleanupTempDir) and makeTestToolContext() shared test helpers, eliminating 2×48-line createTemplateGitRepo method definitions and 6×6-line makeContextWithoutStore definitions across 7 test files, net -28 lines with all 5682+12 tests passing.
…Id parameters and removed 14 private makeContext/makeTestToolContext method definitions across 14 test files, net -105 lines with all 5682+12 tests passing.
…t, eliminating ~35 lines of duplicated pause/resume/timeout logic between setupPauseHandler (stream) and setupPromptPauseHandler (prompt), reducing Agent.swift from 3586→3581 lines with all 5682+12 tests passing.
…rs in SandboxChecker.swift (eliminating 6 duplicated backslash/quote-stripping blocks across extractSubshellCommand and extractCommandBasename) and simplified PersistedSSEEvent.init(from:) in APITypes.swift (replacing 34-line verbose switch with 15-line nil-first-then-assign pattern), net -22 lines with all 5682+12 tests passing.
…Data, SystemData in SDKMessage.swift and OutputFormat in AgentTypes.swift) that compared all stored properties identically to what Swift synthesizes, net -35 lines with all 5682+12 tests passing.
…s (teamStore, taskStore, planStore, cronStore, todoStore, worktreeStore, mailboxStore, senderName, mcpConnections) and removed 8 private makeContext/makeTestToolContext method definitions across 8 test files, net -64 lines with all 5682+12 tests passing.
…g 15 duplicated "store not available" guard-else-return patterns across 12 tool files into single-line try calls, net -5 lines with all 5682+12 tests passing.
…n 5 test files (APITypesTests, TaskTypesExtendedTests, TokenUsageTests, TaskTypesTests, PromptEvolutionTypesTests), centralizing 67 JSONEncoder/JSONDecoder construction sites into 10 shared static properties with all 6209 tests passing.
…swift, eliminating 4 duplicated private makeSkill() method definitions across ExecuteSkillTests, ExecuteSkillStreamTests, SkillToolTests, and SkillRegistryTests, net -54 lines with all 17028+12 tests passing.
… into GitTestHelpers.swift, eliminating 3 duplicated seedSkill method definitions (17 lines each) and 2 duplicated date(daysAgo:) method definitions (3 lines each) across SkillUsageTrackerTests, SkillCuratorTests, and IntelligentCuratorTests, with all 5682+12 tests passing.
…s.swift, replacing 12 identical 9-line callTool method definitions across 12 test files with thin 3-line delegations, net -26 lines with all 5682+12 tests passing.
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
64 iterations of complexity reduction refactoring (56 successful), extracting shared helpers to eliminate code duplication across 177 files, net -3,561 lines (4,297 insertions / 7,858 deletions).
New shared infrastructure files
Production code:
Utils/LLMResponseHelpers.swift(95 lines) — LLM response parsing: extractFirstTextFromResponse, stripCodeFences, parseJSONToDict, parseLLMResponseAsObject/ArrayUtils/EnvUtils.swiftexpanded (335 lines) — centralized: directory resolution, file I/O, validation, factory functions (makeISO8601DateFormatter, makeSDKJSONEncoder, makeSDKJSONDecoder), djb2Hash, YAML/JSON utilitiesMCPTypes.swiftexpanded (268 lines) — shared MCP value conversion, tool registration, ToolExecutionErrorLLMClient.swiftexpanded — shared LLM client infrastructure (performLLMRequest, validateLLMHTTPResponse, buildJSONPostRequest)ReviewTools.swiftexpanded — reviewErrorResponse, requireNonEmptyInputToolTypes.swiftexpanded — StoreUnavailableError, ToolContext.requireStore()AgentTypes.swiftexpanded — resolvedCwd computed propertySkillTypes.swiftexpanded — Skill.withBaseDir()Test code:
TempDirTestCase.swift(36 lines) — base class for temp directory management (47+ test files migrated)GitTestHelpers.swift(242 lines) — makeTestToolContext, makeTestSkill, seedSkill, callToolForTest, etc.MockURLProtocolHelpers.swift(38 lines) — readRequestBodyFromStream, makeMockURLSessionDocumentationTestHelpers.swift— shared documentation test helpersKey patterns extracted
Verification
🤖 Generated with Claude Code