← Back to PRs

#13881: fix: Address Greptile feedback - test isolation and channel resolution

by trevorgordon981 open 2026-02-11 04:40 View on GitHub →
channel: slack stale
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