refactor: code cleanup Q1 2026

[jonit-dev]

Summary

This PR implements code cleanup tasks from issue #59 to reduce maintenance burden and improve code quality:

1. Extract shared env-var utilities - Created packages/cli/src/commands/shared/env-builder.ts with buildBaseEnvVars and getTelegramStatusWebhooks functions, eliminating ~40 lines of duplication across 4 command files (run.ts, review.ts, qa.ts, slice.ts)

2. Simplify mergeConfigs - Replaced the two-pass manual merge in packages/core/src/config.ts with a generic mergeConfigLayer helper, reducing ~90 lines to ~30 lines while preserving exact semantics

3. Delete stale docs - Removed docs/slack-agent-system-architecture.md which described the deleted packages/slack system

Changes

• New files:

  • packages/cli/src/commands/shared/env-builder.ts - Shared env-var building utilities
  • packages/cli/src/__tests__/commands/shared/env-builder.test.ts - 17 tests for the new module

• Modified files:

  • packages/cli/src/commands/run.ts - Uses shared buildBaseEnvVars
  • packages/cli/src/commands/review.ts - Uses shared buildBaseEnvVars
  • packages/cli/src/commands/qa.ts - Uses shared utilities, removes local getTelegramStatusWebhooks
  • packages/cli/src/commands/slice.ts - Uses shared utilities, removes local getTelegramStatusWebhooks
  • packages/core/src/config.ts - Simplified mergeConfigs with mergeConfigLayer helper

• Deleted files:

  • docs/slack-agent-system-architecture.md - Stale documentation

Test Results

• All 1234 tests pass (470 core + 123 server + 641 CLI including 17 new tests)
• yarn verify passes

Closes #59

[jonit-dev]

Night Watch PR Fix

Changes made:

• Fixed no-dupe-else-if lint error in web/utils/cron.ts: added && hour.includes(',') to distinguish comma-separated hours from single-hour branch

CI Failures Fixed:

• lint: no-dupe-else-if in web/utils/cron.ts — two else if branches had identical conditions; first branch now checks hour.includes(',') to only match comma-separated hours

yarn verify passes locally. Ready for re-review.

Night Watch PR Reviewer

[jonit-dev]

Additional Tests Added

New test coverage:

• mergeConfigLayer semantics (config.test.ts): 5 new tests verifying the merge semantics of the refactored config merge helper — shallow merge for providerEnv/boardProvider, replace semantics for roadmapScanner/jobProviders, and env-layer stacking over file config.

Night Watch PR Reviewer

[github-actions]

DiffGuard AI Analysis

AI Review Summary

🏆 Overall Score: 88/100

This PR delivers a high-quality refactoring that consolidates duplicated environment building logic across CLI commands, adds comprehensive test coverage, fixes a cron parsing bug, and removes deprecated documentation. The changes improve maintainability without altering existing functionality.

✅ Key Strengths

• DRY Principle Applied: Successfully extracts shared buildBaseEnvVars and getTelegramStatusWebhooks utilities, eliminating code duplication across 4 command files (qa.ts, review.ts, run.ts, slice.ts)
• Config Merge Simplification: Consolidates ~86 lines of repetitive conditional assignments in mergeConfigs into a clean ~20-line mergeConfigLayer helper function with explicit merge semantics
• Comprehensive Test Coverage: New test file (env-builder.test.ts) provides 286 lines of thorough tests covering edge cases like empty strings, missing fields, and job-specific provider overrides

⚠️ Areas for Improvement

• Missing Export Validation: The new shared/env-builder.ts module exports two functions, but there's no index.ts barrel export or explicit verification that the imports work correctly in the consuming files (though the test file imports suggest this is functional)

🐛 Bugs Found

Bug Name	Affected Files	Description	Confidence
Cron Hour Parsing Edge Case	web/utils/cron.ts	The original code entered a branch designed for comma-separated hour values even when hour was a single number (e.g., "0"), causing potential issues with array operations. The fix adds && hour.includes(',') to properly gate this logic.	🟢 High

📝 Issues Found

Issue Type	Issue Name	Affected Components	Description	Impact/Severity
Testing	Config Merge Semantics Documentation	packages/core/src/config.ts	New tests added to document expected merge behavior for providerEnv, boardProvider, roadmapScanner, and jobProviders, ensuring future changes preserve the correct semantics	Low - Documentation/validation

🔚 Conclusion
This is a well-executed refactoring PR that significantly improves code maintainability by eliminating duplication. The addition of comprehensive tests and a cron bug fix provide extra value. The PR is ready to merge with no critical issues.

---

Analyzed using z-ai/glm-5

[github-actions]

DiffGuard AI Analysis

AI Review Summary

🏆 Overall Score: 92/100

The PR successfully refactors CLI commands to extract shared environment variable building logic, removes deprecated documentation, and adds comprehensive test coverage. The code is clean, well-documented, and follows TypeScript best practices.

✅ Key Strengths

• DRY Refactoring: Effectively consolidates duplicated buildEnvVars logic from 4 command files (run.ts, review.ts, qa.ts, slice.ts) into a single shared utility with clear documentation
• Comprehensive Testing: Adds 286 lines of thorough test coverage for the new env-builder.ts utilities and validates config merge semantics
• Config Merge Simplification: Replaces 90+ lines of repetitive merge logic with a clean 26-line mergeConfigLayer helper function while preserving all merge semantics

⚠️ Areas for Improvement

• Type Casting Verbosity: The (base as unknown as Record<string, unknown>) pattern in mergeConfigLayer works but is slightly verbose. Consider extracting to a helper or using a more type-safe approach if future refactoring is planned.

🔚 Conclusion
The PR is well-executed with clean code organization and excellent test coverage. The refactoring significantly improves maintainability by eliminating code duplication. Ready to merge.

---

Analyzed using z-ai/glm-5