fix: resolve four bugs from issue #258#10
Merged
Merged
Conversation
0xmrpeter
added a commit
that referenced
this pull request
May 25, 2026
Addresses three findings from independent review of PR #10: 1. **Tautology test (blocking)**: the regression test for bug 4 inlined the handler logic instead of importing the production code, so any divergence in `adapter.ts` would not be caught. Extracted the handler into an exported pure function `makeThreadReadyHandler(deps)` and updated the test to import and exercise it directly. Verified the test now catches regressions by temporarily mutating the production code — 3 of 4 tests fail as expected. 2. **Silent failure when eventBus missing**: previously used optional chaining (`this.core.eventBus?.on(...)`), so a host core without eventBus would silently re-introduce bug 4. Now logs a loud warning at startup if eventBus is absent. 3. **Stale comment in createSessionThread**: comment claimed the non-empty-sessionId branch was used by the startup reuse path, but that path actually uses `createThread:false` and bypasses this method entirely. Comment clarified; also added `threadId` to the patchRecord payload in the non-empty branch for symmetry, so any future direct caller gets a complete platform record. Added a 4th regression test covering `onError` invocation when patchRecord rejects, to lock down the error-path contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
1. Plugin name mismatch (Bug 1): self-reported `@openacp/slack` did not
match npm package `@openacp/slack-adapter`, breaking settingsPath,
env-var overrides, legacy config migration, and status reporting.
Now matches package name.
2. Broken `security.allowedUserIds` reference (Bug 2): adapter read
`core.configManager.get().security.allowedUserIds`, but core config
has no top-level `security` field — it threw at start. Removed the
read; allowlist now comes solely from the plugin's own config.
3. Dual `allowedUserIds` sources (Bug 3): event-router previously merged
`slackConfig.allowedUserIds` with the (broken) core security list.
Consolidated to a single source from `slackConfig.allowedUserIds`;
empty means "allow all" (matches setup wizard prompt).
4. "No Slack channel for session" after restart (Bug 4): core invokes
`createSessionThread("", name)` before `session.id` exists, so the
old `patchRecord("", ...)` was a no-op and `channelId` was never
persisted. Now stash the meta in `_pendingChannelsBySlug` and re-key
onto the real sessionId via a `SESSION_THREAD_READY` listener, then
persist `platform.channelId` so `tryRestoreSessionFromRecord` works
after restart.
Bump to 2026.525.1.
Closes #258
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses three findings from independent review of PR #10: 1. **Tautology test (blocking)**: the regression test for bug 4 inlined the handler logic instead of importing the production code, so any divergence in `adapter.ts` would not be caught. Extracted the handler into an exported pure function `makeThreadReadyHandler(deps)` and updated the test to import and exercise it directly. Verified the test now catches regressions by temporarily mutating the production code — 3 of 4 tests fail as expected. 2. **Silent failure when eventBus missing**: previously used optional chaining (`this.core.eventBus?.on(...)`), so a host core without eventBus would silently re-introduce bug 4. Now logs a loud warning at startup if eventBus is absent. 3. **Stale comment in createSessionThread**: comment claimed the non-empty-sessionId branch was used by the startup reuse path, but that path actually uses `createThread:false` and bypasses this method entirely. Comment clarified; also added `threadId` to the patchRecord payload in the non-empty branch for symmetry, so any future direct caller gets a complete platform record. Added a 4th regression test covering `onError` invocation when patchRecord rejects, to lock down the error-path contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
0xmrpeter
added a commit
that referenced
this pull request
May 25, 2026
…ering Two blockers from Phase 2 review: 1. **Reverted Task 1's @openacp/security promotion.** The adapter does NOT consume the security plugin — allowedUserIds is read directly from slackConfig.allowedUserIds (consolidated in PR #10). Promoting to required dependency would break installs that don't have the security plugin without justification. Moved back to optional. 2. **/openacp-archive: post ephemeral BEFORE archiving.** Slack rejects chat.postEphemeral against an archived channel with is_archived, so users would never see the confirmation. Swapped order; added a regression test that verifies postEphemeral fires before archiveChannel. Also: bumped to 2026.525.2 and committed the phase 2 plan document. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
0xmrpeter
added a commit
that referenced
this pull request
May 25, 2026
* feat(slack): promote @openacp/security to required pluginDependencies Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(slack): implement /openacp-archive slash command handler The Slack manifest at src/setup.ts:33 advertises /openacp-archive, but no handler was registered. Users invoking the command saw a 'timeout' error every time. Semantics match deleteSessionThread: archive the current session's channel and tell the user. Exported makeArchiveCommandHandler as a factory so the production handler is testable directly without spinning up Bolt — mirrors the same pattern as makeThreadReadyHandler. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(slack): ping notification channel on permission requests Matches Telegram pattern from permissions.ts:78-84 — users watching only the notification channel now see a 🔐 alert with a click-through to the session channel where the buttons are. Best-effort so a notification failure can't break the underlying permission flow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(slack): persist startupChannelId so restarts reuse the same channel Before: each restart called _createStartupSession which created a fresh channel — the previous one was orphaned. The TODO comment at line 561 said 'configManager.save not available in external plugin context' and gave up. Now: index.ts gets a settingsAPI scoped to this plugin (mirrors the discord-adapter pattern at index.ts:140-143) and passes it to the SlackAdapter constructor. _createStartupSession persists the channel ID via settingsAPI.set() so the existing reuse path at lines 405-422 finds it on next boot. Falls back gracefully with a clear log if settingsAPI is unavailable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * review: revert wrong dependency promotion + fix archive ephemeral ordering Two blockers from Phase 2 review: 1. **Reverted Task 1's @openacp/security promotion.** The adapter does NOT consume the security plugin — allowedUserIds is read directly from slackConfig.allowedUserIds (consolidated in PR #10). Promoting to required dependency would break installs that don't have the security plugin without justification. Moved back to optional. 2. **/openacp-archive: post ephemeral BEFORE archiving.** Slack rejects chat.postEphemeral against an archived channel with is_archived, so users would never see the confirmation. Swapped order; added a regression test that verifies postEphemeral fires before archiveChannel. Also: bumped to 2026.525.2 and committed the phase 2 plan document. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Closes #258. Bumps to
2026.525.1.Each of the four bugs reported in the issue is verified and fixed:
Bug 1 — Plugin name mismatch
src/index.ts:8self-reported@openacp/slack, but npm package is@openacp/slack-adapter.settingsPath(<root>/@openacp/slack/settings.json), env-var overrides (OPENACP_SLACK_*), legacychannels.slackconfig migration, and status reporting to all fail silently.@openacp/slack-adapterto match the package.Bug 2 — Broken
security.allowedUserIdsreferencesrc/adapter.ts:317readcore.configManager.get().security.allowedUserIds, but core config schema has no top-levelsecurity(onlyworkspace.securityfor paths).TypeErroratstart()on any fresh install. Addingsecurity: { allowedUserIds: [] }toconfig.jsondid not help — Zod strips unknown keys.slackConfig.allowedUserIds.Bug 3 — Dual
allowedUserIdssourceschannel-manager.ts:34usedthis.config.allowedUserIds(Slack plugin config) whileadapter.ts:317usedcore.security.allowedUserIds(broken). The event-router merged them with fallback logic that was never reachable.slackConfig.allowedUserIds. Empty array means "allow all" — matches the wizard prompt (Allowed Slack User IDs (comma-separated, or Enter to allow all)).Bug 4 — "No Slack channel for session" after restart
adapter.createSessionThread("", name)beforesession.idis assigned. The old code didthis.sessions.set("", meta)andpatchRecord("", { platform: { channelId } })— both no-ops against the real session record.tryRestoreSessionFromRecordreturned early becauseplatform.channelIdwas missing → warning fires on everysendMessage._pendingChannelsBySlug, subscribe tocore.eventBussession:threadReady, then re-key intosessionsand persistplatform.channelIdonce the real sessionId is known. Listener cleaned up instop().Test plan
pnpm buildsucceedspnpm test— 112 pass / 1 skipped (was 109/1)src/__tests__/thread-ready-persistence.test.tscovers Bug 4 (3 cases: happy path, ignore other adapters, no-op when slug not pending)event-router.test.tsto drop the now-removed global allowlist parameter; "allows all users when both Slack and global lists are empty" still passes<root>/@openacp/slack-adapter/settings.jsonand that a restart preserves the session channelFollow-ups (out of scope, separate PRs)
The full Slack ↔ Telegram parity audit surfaced ~14 additional gaps (assistant integration, control-card persistence,
/openacp-archivehandler, missing event-bus subscriptions, etc.). Plan already drafted indocs/superpowers/plans/2026-04-10-slack-telegram-parity.md.🤖 Generated with Claude Code