#13881: fix: Address Greptile feedback - test isolation and channel resolution
channel: slack
stale
Cluster:
Session Management and Fixes
This PR addresses specific feedback from Greptile code review on PR #13872.
## 🔧 Issues Fixed
### 1. Test Isolation in `session-checkpoint.test.ts`
- **Problem**: Tests were writing to actual user `~/.openclaw/checkpoints/` directory
- **Impact**: Could interfere with user data or cause test failures in parallel runs
- **Solution**: Mock `os.homedir()` to redirect to isolated test directory
- **Additional**: Added proper mock cleanup in `afterEach` to prevent test pollution
### 2. Channel Resolution Logic in `message.ts`
- **Problem**: Flawed logic missed plain channel names (e.g., `general` without `#`)
- **Issue**: Condition `!input.startsWith("C")` incorrectly treated channel IDs as names
- **Solution**: Replaced with robust regex pattern `/^[CG][A-Z0-9]+$/i` to properly identify channel IDs
- **Benefit**: Now handles all Slack channel name variations correctly
## 🧪 Testing
- All existing tests continue to pass
- Test isolation verified - no more writes to user directories
- Channel resolution logic tested with various input formats
## 📝 Code Quality
- Maintains backward compatibility
- No breaking changes to public APIs
- Follows existing code patterns and style
Addresses: #13872 (Greptile review feedback)
<!-- greptile_comment -->
<h2>Greptile Overview</h2>
<h3>Greptile Summary</h3>
This PR follows up on prior review feedback by (1) isolating session-checkpoint tests from the real user home directory via an `os.homedir()` mock, and (2) adjusting Slack `sendMessage` channel resolution to better distinguish channel IDs from names.
The changes integrate with the outbound messaging path in `src/infra/outbound/message.ts` (pre-processing `params.to` before delegating to delivery), and with the session checkpoint implementation (`src/infra/session-checkpoint.ts`) by ensuring tests don’t write to `~/.openclaw/checkpoints`.
Two issues should be addressed before merge:
- `session-checkpoint.test.ts` now restores *all* mocks in `afterEach`, but individual tests also manually restore their spies, which can lead to double-restore / inconsistent test behavior.
- The new Slack channel ID regex is broad enough to misclassify some valid channel names as IDs, skipping name resolution and likely breaking delivery for those names.
<h3>Confidence Score: 3/5</h3>
- This PR is close to safe to merge, but it contains a couple of behavior/test-stability issues that should be fixed first.
- Core changes are small and localized, but the Slack channel ID detection can mis-handle some channel names, and the test suite change introduces potentially brittle mock restoration order (global restore plus per-test restores).
- src/infra/outbound/message.ts, src/infra/session-checkpoint.test.ts
<!-- greptile_other_comments_section -->
<!-- /greptile_comment -->
Most Similar PRs
#13882: feat: Enhance session checkpoint system with better types and valid...
by trevorgordon981 · 2026-02-11
85.5%
#8024: fix(slack): resolve channel names via directory for cross-account m...
by emma-digital-assistant · 2026-02-03
84.4%
#13889: feat: Slack channel cache, session cost alerts & checkpoint/recover...
by trevorgordon981 · 2026-02-11
83.1%
#4878: fix: string/type handling and API fixes (#4537, #4380, #4373, #4547...
by lailoo · 2026-01-30
82.6%
#11048: fix: address repository issues (env, author, CI comments, security ...
by cavula · 2026-02-07
82.5%
#19083: Slack: preserve per-thread context and consistent thread replies
by jkimbo · 2026-02-17
82.1%
#15050: fix: transcript corruption resilience — strip aborted tool_use bloc...
by yashchitneni · 2026-02-12
81.2%
#22433: Slack: fix thread context loss after session reset
by stgarrity · 2026-02-21
80.7%
#7719: fix(slack): thread replies with @mentions dropped in requireMention...
by SocialNerd42069 · 2026-02-03
80.1%
#8684: fix(slack): add title param and channel resolution for file upload
by shuans · 2026-02-04
80.0%