[PR #141] [MERGED] refactor: decompose activities into services layer with structured error handling #142

New issue

Closed

opened 2026-02-27 08:09:26 +03:00 by kerem · 0 comments

kerem commented

2026-02-27 08:09:26 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/KeygraphHQ/shannon/pull/141
Author: @ajmallesh
Created: 2/17/2026
Status: ✅ Merged
Merged: 2/17/2026
Merged by: @ajmallesh

Base: main ← Head: refactor/architecture

📝 Commits (10+)

7fce261 refactor: remove ~750 lines of dead code across 12 files
4239c1c refactor: remove ~275 lines of dead code and enable stricter tsconfig
9fb9d97 refactor: consolidate duplicate types and file I/O utilities
1ef8d37 refactor: extract services layer, Result type, and ErrorCode classification
7d24c52 refactor: replace console.log/chalk with ActivityLogger across services
81faa87 feat: add resume header to workflow.log showing previous workflow ID and checkpoint
62967c2 refactor: consolidate file layout and break circular dependencies
9eb9162 refactor: remove ~70 low-value comments across 13 files
1c940ae docs: update CLAUDE.md and commands for services-layer architecture
7d59d4d refactor: extract helpers from long functions in client, workflows, and agent-execution

📊 Changes

56 files changed (+2880 additions, -2894 deletions)

View changed files

📝 .claude/commands/debug.md (+22 -14)
📝 .claude/commands/review.md (+11 -0)
📝 CLAUDE.md (+35 -13)
📝 package-lock.json (+0 -1)
📝 package.json (+0 -1)
📝 src/ai/claude-executor.ts (+53 -208)
📝 src/ai/message-handlers.ts (+30 -43)
📝 src/ai/output-formatters.ts (+286 -41)
📝 src/ai/router-utils.ts (+0 -6)
📝 src/ai/types.ts (+0 -38)
📝 src/audit/audit-session.ts (+46 -29)
📝 src/audit/index.ts (+0 -4)
➕ src/audit/log-stream.ts (+127 -0)
📝 src/audit/logger.ts (+14 -54)
📝 src/audit/metrics-tracker.ts (+30 -26)
📝 src/audit/utils.ts (+7 -102)
📝 src/audit/workflow-logger.ts (+57 -71)
➖ src/cli/input-validator.ts (+0 -59)
➖ src/cli/ui.ts (+0 -49)
📝 src/config-parser.ts (+297 -92)

...and 36 more files

📄 Description

Motivation

Community code review by @PopoviciGabriel (#78) identified structural issues as the project has grown:

activities.ts had grown to ~500 lines mixing Temporal orchestration, git lifecycle, audit logging, and Claude SDK invocation — making it hard to extend or reason about individually
Error handling diverged across modules — some paths throw, some return error objects, some log and continue — with no way to distinguish retryable vs non-retryable failures by type
Config validation stopped on the first error with raw AJV output, requiring users to fix issues one at a time
Spending cap detection was duplicated across 5 locations with 3 independently maintained pattern lists that had drifted out of sync

Summary

Structural refactor that establishes clear architecture boundaries without changing any external behavior. The five-phase pipeline, Temporal orchestration, and all agent behavior remain identical.

Services layer extraction — Extracted three focused services (AgentExecutionService, ConfigLoaderService, ExploitationCheckerService) wired through a manual DI container scoped per-workflow. Activities become thin Temporal wrappers (~150 lines) that own heartbeats and error classification while services own domain logic with zero Temporal imports.

Codebase restructuring — Reorganized src/ into a clear module hierarchy. Moved error-handling, git-manager, prompt-manager, queue-validation, and reporting into src/services/. Consolidated scattered type definitions into src/types/ (result.ts, errors.ts, metrics.ts, audit.ts, activity-logger.ts). Deleted src/constants.ts — absorbed agent validators and mappings into session-manager.ts alongside agent definitions to create a single AGENTS registry as the source of truth. Broke a circular dependency between temporal/ and services/ by extracting the ActivityLogger interface into src/types/. Decomposed long functions in client.ts, workflows.ts, and agent-execution.ts into focused helpers — extracting CLI argument parsing, workspace resolution, sequential phase execution, and failure handling into single-responsibility functions.

Structured error handling — Replaced scattered string-matching error classification with an ErrorCode enum and Result<T,E> type. The error classifier now matches on typed codes for internal errors and falls back to string matching only for external SDK/network errors. Config validation upgraded from fail-on-first to collect-all-errors with human-readable messages. Spending cap detection consolidated from 5 locations into a shared billing-detection.ts utility.

Logging separation — Replaced console.log/chalk calls across services with an ActivityLogger interface threaded via DI. Workflows use Temporal's replay-safe log import. CLI display code keeps console.log — that's user-facing terminal output, not operational logging.

Removed ~1,025 lines of dead code across two passes and ~70 low-value comments. Enabled stricter tsconfig (noUnusedLocals, noUnusedParameters).

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/KeygraphHQ/shannon/pull/141 **Author:** [@ajmallesh](https://github.com/ajmallesh) **Created:** 2/17/2026 **Status:** ✅ Merged **Merged:** 2/17/2026 **Merged by:** [@ajmallesh](https://github.com/ajmallesh) **Base:** `main` ← **Head:** `refactor/architecture` --- ### 📝 Commits (10+) - [`7fce261`](https://github.com/KeygraphHQ/shannon/commit/7fce26185205843dd5df262bf0aef0bc35602b52) refactor: remove ~750 lines of dead code across 12 files - [`4239c1c`](https://github.com/KeygraphHQ/shannon/commit/4239c1c3f7cf7da21b30c2e173206a7cb3f70410) refactor: remove ~275 lines of dead code and enable stricter tsconfig - [`9fb9d97`](https://github.com/KeygraphHQ/shannon/commit/9fb9d97d5a8e40307c123fc3a261b8aa6b947a39) refactor: consolidate duplicate types and file I/O utilities - [`1ef8d37`](https://github.com/KeygraphHQ/shannon/commit/1ef8d37682dd75ceea386848e0068d15ddbd4d87) refactor: extract services layer, Result type, and ErrorCode classification - [`7d24c52`](https://github.com/KeygraphHQ/shannon/commit/7d24c52b27780d773f20e26ac514454f1608a694) refactor: replace console.log/chalk with ActivityLogger across services - [`81faa87`](https://github.com/KeygraphHQ/shannon/commit/81faa87fde5bc5dac93b6a8e015143d83f68fffc) feat: add resume header to workflow.log showing previous workflow ID and checkpoint - [`62967c2`](https://github.com/KeygraphHQ/shannon/commit/62967c20f8460f6808a1ede19e4ab7c9ea336b2f) refactor: consolidate file layout and break circular dependencies - [`9eb9162`](https://github.com/KeygraphHQ/shannon/commit/9eb9162fef2962aa1bf003b5f543d350a75fd184) refactor: remove ~70 low-value comments across 13 files - [`1c940ae`](https://github.com/KeygraphHQ/shannon/commit/1c940ae7b52b8a8fe4e9c7c9a926c08dbf1c4b8f) docs: update CLAUDE.md and commands for services-layer architecture - [`7d59d4d`](https://github.com/KeygraphHQ/shannon/commit/7d59d4d49d97f44d752b94e9f97132e1ec399840) refactor: extract helpers from long functions in client, workflows, and agent-execution ### 📊 Changes **56 files changed** (+2880 additions, -2894 deletions) <details> <summary>View changed files</summary> 📝 `.claude/commands/debug.md` (+22 -14) 📝 `.claude/commands/review.md` (+11 -0) 📝 `CLAUDE.md` (+35 -13) 📝 `package-lock.json` (+0 -1) 📝 `package.json` (+0 -1) 📝 `src/ai/claude-executor.ts` (+53 -208) 📝 `src/ai/message-handlers.ts` (+30 -43) 📝 `src/ai/output-formatters.ts` (+286 -41) 📝 `src/ai/router-utils.ts` (+0 -6) 📝 `src/ai/types.ts` (+0 -38) 📝 `src/audit/audit-session.ts` (+46 -29) 📝 `src/audit/index.ts` (+0 -4) ➕ `src/audit/log-stream.ts` (+127 -0) 📝 `src/audit/logger.ts` (+14 -54) 📝 `src/audit/metrics-tracker.ts` (+30 -26) 📝 `src/audit/utils.ts` (+7 -102) 📝 `src/audit/workflow-logger.ts` (+57 -71) ➖ `src/cli/input-validator.ts` (+0 -59) ➖ `src/cli/ui.ts` (+0 -49) 📝 `src/config-parser.ts` (+297 -92) _...and 36 more files_ </details> ### 📄 Description ## Motivation Community code review by @PopoviciGabriel (#78) identified structural issues as the project has grown: - `activities.ts` had grown to ~500 lines mixing Temporal orchestration, git lifecycle, audit logging, and Claude SDK invocation — making it hard to extend or reason about individually - Error handling diverged across modules — some paths throw, some return error objects, some log and continue — with no way to distinguish retryable vs non-retryable failures by type - Config validation stopped on the first error with raw AJV output, requiring users to fix issues one at a time - Spending cap detection was duplicated across 5 locations with 3 independently maintained pattern lists that had drifted out of sync ## Summary Structural refactor that establishes clear architecture boundaries without changing any external behavior. The five-phase pipeline, Temporal orchestration, and all agent behavior remain identical. **Services layer extraction** — Extracted three focused services (`AgentExecutionService`, `ConfigLoaderService`, `ExploitationCheckerService`) wired through a manual DI container scoped per-workflow. Activities become thin Temporal wrappers (~150 lines) that own heartbeats and error classification while services own domain logic with zero Temporal imports. **Codebase restructuring** — Reorganized `src/` into a clear module hierarchy. Moved `error-handling`, `git-manager`, `prompt-manager`, `queue-validation`, and `reporting` into `src/services/`. Consolidated scattered type definitions into `src/types/` (`result.ts`, `errors.ts`, `metrics.ts`, `audit.ts`, `activity-logger.ts`). Deleted `src/constants.ts` — absorbed agent validators and mappings into `session-manager.ts` alongside agent definitions to create a single `AGENTS` registry as the source of truth. Broke a circular dependency between `temporal/` and `services/` by extracting the `ActivityLogger` interface into `src/types/`. Decomposed long functions in `client.ts`, `workflows.ts`, and `agent-execution.ts` into focused helpers — extracting CLI argument parsing, workspace resolution, sequential phase execution, and failure handling into single-responsibility functions. **Structured error handling** — Replaced scattered string-matching error classification with an `ErrorCode` enum and `Result<T,E>` type. The error classifier now matches on typed codes for internal errors and falls back to string matching only for external SDK/network errors. Config validation upgraded from fail-on-first to collect-all-errors with human-readable messages. Spending cap detection consolidated from 5 locations into a shared `billing-detection.ts` utility. **Logging separation** — Replaced `console.log`/chalk calls across services with an `ActivityLogger` interface threaded via DI. Workflows use Temporal's replay-safe `log` import. CLI display code keeps `console.log` — that's user-facing terminal output, not operational logging. Removed ~1,025 lines of dead code across two passes and ~70 low-value comments. Enabled stricter tsconfig (`noUnusedLocals`, `noUnusedParameters`). --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>