add .hardno/ignore to skip .pi/ from reviews - #1
Merged
Conversation
royosherove
added a commit
that referenced
this pull request
May 7, 2026
* perf: eliminate redundant file reads + flush model support Fix #1: Cache MemorySnapshot and fileSet in PreparedTurn. In finalizeMemoryForTurn, skip the disk re-read entirely when no tools were used during the turn (turnUsedTools=false). When tools ran, reuse the cached fileSet (avoids re-resolving paths) but still re-read file contents to detect modifications. handleStreaming() now returns { usedTools } flag. Flush model: Add config.compact.flushModel option and AgentAdapter.promptWithModel() method. When configured, memory flush turns use a cheaper/faster model (e.g. Sonnet) instead of the expensive conversation model. Pi adapter implements via session.setModel() with automatic restore after flush. All 217 tests pass, no new type errors. * perf: default flush model to Sonnet (faster) Flush turns are structured tasks (read context → write markdown) that don't need frontier reasoning. Sonnet responds 3-5x faster than Opus for these turns. Default: amazon-bedrock/us.anthropic.claude-sonnet-4-6-20250514-v1:0 Set flushModel: null in config to fall back to conversation model. * fix: address codex review — no model persistence, no complement I/O P2: Replace session.setModel() with in-memory agent.state.model swap. Avoids persisting flush model to settings.json or session log. Crash-safe: if process dies mid-flush, settings remain on user's configured model. P3: Revert complement/unknown mode pre-read. finalizeMemoryForTurn skips digest checks for complement mode, so reading files was wasted I/O. * fix: restore model unconditionally, track only file-modifying tools - Always restore agentState.model in finally (even if undefined) - Only set turnUsedTools=true for write/edit/bash/multi_edit tools (read/grep/ls/find are read-only — no need to re-hash memory files) * fix: invert tool allowlist — use read-only set instead of write set Unknown/extension tools now trigger memory re-read (safe default). Only known read-only tools (read, grep, find, ls, glob) skip re-read. Addresses codex review: custom extension tools that modify memory files would have been missed by the previous write-tool allowlist. * feat: add timing info to compact messages + persistent timing log - Telegram messages now show flush/compact/total time + model used - Timing persisted to ~/.roundhouse/logs/compact-timing.jsonl (JSONL) - Both manual /compact and auto-compact show timing * refactor: extract READ_ONLY_TOOLS + CompactResult to types, add tests - READ_ONLY_TOOLS: shared constant in memory/types.ts (not buried in handleStreaming) - CompactTiming + CompactResult: proper types in types.ts (SRP) - lifecycle.ts imports CompactResult instead of inline type - gateway.ts imports READ_ONLY_TOOLS + CompactResult from types - Tests: READ_ONLY_TOOLS membership, CompactResult with/without timing * feat: use Haiku for compact step too (compactWithModel) Before: flush used Haiku (238s) but compact restored to Opus (152s) = 390s total After: both flush AND compact use Haiku via new compactWithModel adapter method - Added compactWithModel?(threadId, modelId) to AgentAdapter interface - Implemented in pi.ts: in-memory model swap around session.compact() - lifecycle.ts: prefers compactWithModel when available, falls back to compact() * feat: live progress updates during compact (edits Telegram message in-place) - New telegram-progress.ts: createProgressMessage() → editable message handle - Compact shows: 'Flushing memory...' → 'Compacting... (flush took Xs)' → final result - Single message edited in-place (no spam of multiple messages) - Works for both manual /compact and auto-compact - Falls back gracefully for non-Telegram adapters (no-op updates) --------- Co-authored-by: Loki FastStart <loki@faststart.internal>
royosherove
added a commit
that referenced
this pull request
May 14, 2026
…31) (#133) * refactor(session-repair): beautify per code review (7 findings) Addresses all 7 findings from the post-v0.5.30 maintainability review. Pure refactor \u2014 zero behavior change. 536 tests passing (+2 new for the divergent-change fix). #1 + #7 \u2014 DRY: extract matchesErrorPatterns + fix divergent change isContextOverflowError and isToolPairingError shared ~80% of structure (top-level .message, cause-chain walk, stringify-gate, circular try/catch) but with subtle drift: only one walked the cause chain. Extracted matchesErrorPatterns(err, patterns, { stringifyGate }) shared helper. Both classifiers now ~7 lines and BOTH walk cause chains. Added 2 tests proving isToolPairingError now classifies wrapped/Bedrock-nested tool-pairing errors (regression guard for the divergent-change fix). Also extracted looksLikeValidationError() shared gate so the two classifiers don't repeat the 4xx/ValidationException check. #2 \u2014 long method: extracted buildTrimmedEntries(entries, cutIdx) from softResetSessionFile. The orchestration is now linear: validate \u2192 cut \u2192 build \u2192 repair \u2192 write \u2192 report. Each step at the same abstraction level. #3 \u2014 long catch with nested try: extracted attemptSoftResetRecovery() in lifecycle.ts. flushMemoryThenCompact catch block went from ~60 lines with nested try/catch to ~25 lines, linear: classify \u2192 recover \u2192 log \u2192 persist. #4 \u2014 magic number: MAX_CAUSE_CHAIN_DEPTH = 5 named constant. #5 \u2014 file >400 lines: split src/agents/shared/session-repair.ts (574 lines, two domains) into: - session-repair.ts (81 lines) \u2014 public surface, tool-pair repair, re-exports for backward compat - session-soft-reset.ts (120 lines) \u2014 softResetSessionFile + types - error-classifiers.ts (75 lines) \u2014 matchesErrorPatterns + isContextOverflowError + isToolPairingError + MAX_CAUSE_CHAIN_DEPTH - session-repair-internal.ts (239 lines) \u2014 shared filesystem + in-memory repair primitives All public exports preserved via re-exports (Feathers seam pattern). Existing callers compile unchanged. #6 \u2014 naming: introduced RepairResult { entries, report } named type for the previously-anonymous repairEntriesInMemory return shape. Better IDE hover/autocomplete; no inline destructuring noise at call sites. Verification: npm test \u2014 536 passing (was 534, +2 for cause-chain regression tests) wc -l src/agents/shared/session-repair.ts \u2014 81 (was 574) All public API surfaces preserved. * chore: bump to v0.5.31 + CHANGELOG --------- Co-authored-by: Roy Osherove <575051+royosherove@users.noreply.github.com>
royosherove
added a commit
that referenced
this pull request
May 29, 2026
…streaming (#151) * chore(slack-adapter): bump chat SDK to 4.29 + add slack-plan Phase 0 of slack-plan.md: bump chat, @chat-adapter/telegram, @chat-adapter/state-memory from ^4.26.0 to ^4.29.0. All 591 tests pass. Diffed telegram d.ts 4.26 vs 4.29 — only changes that touch us are private→protected (no subclassing) and persistMessageHistory→ persistThreadHistory rename (we don't reference). video_note attachment typing is additive. Also adds .gitignore entry for .claude/ (cron lock, transcripts) and slack-plan.md (4 review iterations, verdict ship-as-is). * feat(slack-adapter): phase 1 — multi-transport refactor Lays the groundwork for Slack to land alongside Telegram in a single gateway. The gateway code path stays uniform (one this.transport.*), but underneath it now routes across delegates via a new CompositeTransportAdapter. Type widening: - GatewayConfig.allowedUserIds and notifyChatIds: number[] → (string | number)[]. Slack ids are strings; telegram are numbers. - ChatThread.post: widened to string | { markdown } | { card, fallbackText? }. - TransportAdapter.notify(chatIds): (string | number)[]. - TransportAdapter.createThread(chatId): widened. - TransportAdapter.enrichPrompt(thread, text): added thread arg. - TransportAdapter.registerCommands(): no token arg; adapter self-sources. - isAllowed: dual lookup (numeric path keeps telegram match working, raw-string path matches slack `Uxxx`). New TransportAdapter methods: - ownsChatId(id) — pure shape check, used to partition notify(). - encodeParentThreadId(chatId) — sub-agent / cron parent thread id. - formatNotifySession(chatId) — startup notification "Session: …" label. - shouldIgnoreMessage?(text, message, thread) — per-transport pre-handler filter. Telegram /start filter moved here from the gateway. - stream(thread, iter, signal?) — per-transport streaming dispatch. CompositeTransportAdapter: - Routes per-thread methods by ownsThread(thread). - Partitions notify() chat ids by ownsChatId() and fans out. - handlePairing returns the first non-null result, tagged with the delegate name so the gateway can mark pairingComplete per-transport. - Exposes ownerOf(thread) and ownerOfChatId(id) so the gateway can do per-transport gating without leaking transport semantics. Per-transport pairingComplete: - Was: single boolean. Stuck at true after the first transport paired, silently blocking the second. - Now: Map<string, boolean> keyed by transport name. Slack pairing can proceed even after telegram pairing completed. Number()-coercion sweep (regressions Slack would have hit silently): - gateway.ts:113 — preserve string ids in handlePendingPairing. - gateway.ts:329-333 — cron notifyFn signature widened. - gateway.ts:352 — drop Number() cast on subagent chatId. - gateway.ts:978-1001 — group/main label moved into transport. formatNotifySession. - gateway.ts:1017-1021 — fireBootTurn partitions by transport, fires one boot turn per transport that owns at least one configured chatId. - subagent-command.ts:30 — detect transport from chatId shape; encode parentThreadId via the matching transport (was hardcoded `telegram:`). - ipc/handler.ts:17-30 — req.session sentinel check now uses transport.ownsChatId() instead of `/^-?\d+$/` regex; slack Cxxx/Dxxx/Gxxx/Uxxx sessions now route to a single target instead of falling through to "send to all". Telegram adapter: - registerCommands self-sources TELEGRAM_BOT_TOKEN. - ownsChatId / encodeParentThreadId / formatNotifySession implementations. - shouldIgnoreMessage swallows /start <nonce> handshake. - createThread.post widened to ChatThreadPost; card path falls back to fallbackText until Phase 2 unifies through SDK postMessage. - handlePairing decorates result with `transport: "telegram"`. New chat-adapters.ts factory registry: lazy imports per platform; throws loudly on a configured-but-uninstalled adapter so typos at config time fail at startup, not silently. Tests: 591 → 614 (all passing). New: - composite-transport.test.ts (10 cases) — routing, partition, race walkthrough, action-id thread routing. - ipc-handler-partition.test.ts (6 cases) — slack vs telegram session routing. - unit.test.ts — 5 new cases for heterogeneous-allowlist isAllowed. * fix(slack-adapter): phase 1 review fixes Critical: gateway.ts:699 was calling enrichPrompt(text) with the post-Phase-1 contract (thread, text). With one positional arg, JS bound thread = string and text = undefined; every real agent turn would silently overwrite the user's prompt with `undefined`. Tests didn't catch it because prepareAgentMessage was never exercised with a fake transport. Fixes: - gateway.ts:699 — pass `thread` to enrichPrompt(thread, text). - util.ts isAllowed — userId can arrive as number from some platforms; widened type to `string | number` and normalized via String(). Old parseInt-based dual lookup missed the userId-as-number case. - commands.ts:25, gateway.ts:757, gateway.ts:795, setup/types.ts:17 — residual `number[]` annotations widened to `(string | number)[]`. New tests: - prepare-agent-message.test.ts — pins enrichPrompt(thread, text) arity so this regression can never recur silently. - unit.test.ts — case for `author.userId` as raw number against both numeric and string allowlist entries. * feat(slack-adapter): phase 2 — Slack TransportAdapter (socket mode) Implements Slack as a first-class TransportAdapter alongside Telegram in a single gateway. v1: single workspace, socket mode only. New module src/transports/slack/: - slack-adapter.ts (TransportAdapter impl with attach(slackSdk) lifecycle) - format.ts (isSlackChatId + Slack 12k markdown_text limit) - pairing.ts (slack-pairing.json + matchPendingPairing — message.im AND assistant_thread_started paths) - notify.ts (REST-only chat.postMessage helper for non-gateway callers; defaults match SDK to keep notify and gateway-emitted messages consistent) - progress.ts (chat.postMessage + chat.update for editable progress) - streaming.ts (post-then-edit fallback with throttled-overflow, abort-signal honoring, init-fail backoff with hard cap and final flush — closes the v3 review polish requirements) - manifest.yaml (Slack app manifest for setup CLI to print) Multi-transport completeness: - richMenuToCard helper (transports/rich-helpers.ts) maps RichMenu to the Chat SDK's transport-agnostic CardElement; Slack's cardToBlockKit and Telegram's extractCard render to platform-native shapes inside the SDK. No per-transport Block Kit converter needed. - stripMarkdownToPlain produces fallbackText for clients that can't render cards (Slack mobile previews etc.). - Gateway constructor now wires SlackAdapter as a delegate when config.chat.adapters.slack is configured (Phase 1's eager throw removed). - After chat.initialize(), gateway: • attaches the Chat SDK Slack adapter to our SlackAdapter delegate. • eagerly calls slackSdk.webClient.auth.test() to populate botUserId before subscriptions activate (closes the bot self-loop race window — Phase 2 risk #1). • registers bot.onAssistantThreadStarted to drive first-DM pairing before the user types anything (chicken-and-egg gap from §2.4). - gateway/streaming.ts now dispatches via transport.stream(thread, iter, signal) — Telegram impl wraps handleTelegramHtmlStream; Slack impl wraps handleSlackStream. Removes the hardcoded isTelegramThread branch. Type plumbing: - Replaced flawed `import { Text } from "chat"` (mdast type, not the JSX factory) with a tiny local TextElement constructor in rich-helpers.ts so we don't pull jsx-runtime into a non-JSX file. - Section/Actions take direct array per chat@4.29.0 jsx-runtime. @chat-adapter/slack@^4.29.0 added to deps. Tests: 617 → 651 (all passing). New: - slack-adapter.test.ts (7 cases) — TransportAdapter contract, postRich uses {card, fallbackText} not {blocks}, notify routes via webClient. - slack-format.test.ts (16 cases) — id shape checks, richMenuToCard output structure (no raw Block Kit), button chunking at 5, stripMarkdownToPlain. - slack-pairing.test.ts (6 cases) — matchPendingPairing dual lookup (userName for message.im, userId for assistant_thread_started). - slack-streaming.test.ts (5 cases) — initial post + edits, retry-with- backoff, abort honoring, empty-stream noop, non-slack-thread guard. * fix(slack-adapter): phase 2 review fixes Critical: createSlackAdapter only env-falls-back SLACK_BOT_TOKEN / SLACK_APP_TOKEN / SLACK_SIGNING_SECRET when called with NO config (zeroConfig = !config). Phase 2 passed `{ mode: "socket" }`, so zeroConfig === false and botToken stayed undefined; every webClient call would throw `AuthenticationError: No bot token available`. Verified against @chat-adapter/slack@4.29.0 dist/index.js:4233-4243. Tests didn't catch it because they all mock the SDK. Fixes: - chat-adapters.ts — explicitly forward env vars to createSlackAdapter (botToken, appToken, signingSecret) so they're populated regardless of whether other config keys are present. - streaming.ts — gate INIT_FAIL_BACKOFF_MS on `initFailures > 0` so successful re-inits after handleOverflow aren't silently skipped when the prior send-initial happened within the backoff window. - streaming.ts — handle overflow on the FIRST chunk too: if a single chunk pushes the uncommitted buffer past 12K before we've sent anything, slice + post directly via chat.postMessage. Without this, Slack would reject the initial post. - types.ts — IncomingMessage.chatId widened to string|number (was number-only); SlackAdapter.handlePairing and gateway's assistant_thread_started synthesizer both pass strings. - gateway.ts — drop the redundant explicit auth.test() call. The SDK's initialize() already calls it (verified against index.js:868-885) and populates botUserId before subscriptions activate. New tests: - slack-streaming.test.ts — overflow path: a 13K chunk produces ≥2 posts with each markdown_text ≤ 12K, combined back to original length. Caught the bug above during implementation. - transport-stream-dispatch.test.ts — pins the gateway/streaming.ts refactor: transport.stream(thread, iter, signal) is called when a transport is provided; falls back to thread.handleStream otherwise. * feat(slack-adapter): phase 3 — roundhouse setup --slack Adds an interactive + non-interactive setup flow for Slack alongside the existing --telegram flow. Mirrors the telegram structure where the underlying step is platform-agnostic; everything Slack-specific lives in new files. CLI: - args.ts: new flags --slack, --slack-bot-token, --slack-app-token, --slack-signing-secret. Env fallback to SLACK_BOT_TOKEN / SLACK_APP_TOKEN / SLACK_SIGNING_SECRET. --slack and --telegram are mutually exclusive (run setup twice if both are wanted). Tokens in argv blocked in --non-interactive mode (avoid argv leakage in process listings). xoxb-/xapp- prefix validation at parse time so paste errors fail fast. - setup.ts: orchestrator routes --slack to the Slack flows. - setup/slack.ts: validateSlackBotToken (auth.test → SlackBotInfo), validateSlackAppTokenShape, redactSlackToken (preserves prefix + last 4 chars so users can identify which token is broken), readBundledManifest. - setup/slack-flows.ts: runInteractiveSlackSetup + runNonInteractiveSlackSetup. Doesn't reuse stepStoreSecrets / stepConfigure from the telegram path because both encode telegram- specific secret names and adapter defaults; uses focused stepWriteSlackEnv / stepWriteSlackConfig / stepWriteSlackPairing helpers that preserve any existing telegram config so multi-transport installs coexist. Pairing: - Interactive flow prints the bundled Slack app manifest inline AND saves a copy to /tmp/roundhouse-slack-manifest.yaml for paste convenience. - Writes ~/.roundhouse/slack-pairing.json with status="pending" before starting the service. The gateway completes pairing on the first message.im or assistant_thread_started event from an allowed user. - Final output explicitly tells the user to OPEN A NEW DM with the bot — the chicken-and-egg gap from slack-plan.md §2.4 (Slack only fires message.im for existing DM channels). Tests: 654 → 665 (all passing). New setup-slack.test.ts covers: - arg parsing (xoxb/xapp prefix gates, env fallback, mutually-exclusive --telegram/--slack, argv-leakage rejection in --non-interactive). - redactSlackToken format. - validateSlackAppTokenShape. - readBundledManifest + privacy check (no users:read.email scope). * fix(slack-adapter): phase 3 review fixes Two P0 regressions vs telegram setup parity, plus three P1 fixes. P0a — agent.configure() was skipped in slack flow. Telegram's stepConfigure invokes agent.configure({ provider, model, cwd, ... }) so pi writes ~/.pi/agent/settings.json with the right provider/model. The slack flow's stepWriteSlackConfig wasn't doing this, so a fresh `roundhouse setup --slack` install left the agent mis-configured. Fix: stepWriteSlackConfig now takes the AgentDefinition, calls agent.configure(...), and merges agent.configDefaults into the gateway config's `agent` block (matches telegram step:416). P0b — --with-psst was silently broken. The slack flow ran `stepInstallPackages` (which installs the psst runtime when --with-psst is set) but never stored secrets in psst — they always went to .env regardless of the flag. Fix: new `stepStoreSlackSecrets` mirrors telegram's stepStoreSecrets. stepWriteSlackEnv now writes only non-secret fields when --with-psst is on, matching telegram's split. P1a — env values weren't envQuote'd. Raw "${value}" interpolation would mis-parse if the bot's display name (or any secret) contained `"`, `$`, backtick, or `\`. Slack lets workspaces pick arbitrary bot display names so this is a real exposure for systemd's EnvironmentFile shell expansion. Use envQuote() like the telegram step does. P1b — --slack-signing-secret wasn't in the argv-leakage gate. Even though signing secret is unused for socket-mode v1, passing it via --non-interactive would leak in `ps aux`. Added to the rejection list alongside --slack-bot-token and --slack-app-token. P1c — pairing hint printed $USER instead of the Slack username. The user just typed their Slack handle into the wizard; we now show opts.users in the "first message from … will complete pairing" line. Cleanup: - Dropped dead `unquoteEnvValue` re-export from slack-flows.ts. - Stripped a dangling `cfgFileExists` import. Tests: - New: --slack-signing-secret in --non-interactive is rejected. - 666 tests pass (was 665). Still deferred to a follow-up (P2/P3 from review): - Round-trip test for stepWriteSlackEnv with special-char botName. - --slack --dry-run output (currently shows telegram-only message). - printSetupHelp() doesn't list --slack flags. - Manifest path resolution for src/dist/ build layout. * feat(slack-adapter): phase 4 — additional test coverage + setup polish Most slack tests landed alongside their feature in Phases 1-3 (composite, ipc, slack-adapter, slack-format, slack-pairing, slack-streaming, setup-slack, transport-stream-dispatch, prepare-agent-message, isAllowed widening). Phase 4 closes the remaining gaps the iter-3 review flagged: Tests (new): - cron-notify-partition.test.ts — exercises the lambda the gateway builds for the cron scheduler (gateway.ts:349) against a real composite. Verifies a heterogeneous (string | number)[] partitions to telegram and slack delegates without dropping ids. - setup-slack-validate-token.test.ts — mocked-fetch tests for validateSlackBotToken. Bar: never leak the raw bot token in any error message; parse user_id/team_id on success; fail loudly on HTTP errors and Slack-side ok:false. Setup polish (P2/P3 items deferred from Phase 3 review): - printDryRun: now branches on opts.slack so --slack --dry-run shows Slack-aware preview (validate xoxb+xapp, write slack-pairing.json, save manifest to /tmp, configure slack adapter) instead of the telegram-only message. - printSetupHelp: now lists --slack, --slack-bot-token, --slack-app-token, --slack-signing-secret, env-var preferences, and notes mutual exclusion. Telegram-only flags (--qr / --bot-token / --notify-chat) annotated. - readBundledManifest: probes two candidate paths so it works for both tsx (src/cli/setup/slack.ts) and a future src/dist/ build layout. Throws with both probed paths listed when neither exists. Tests: 666 → 673 (all passing). * fix(slack-adapter): phase 4 review fixes Two items the iter-4 review flagged: 1. validateSlackBotToken had no AbortSignal.timeout on its fetch — a hung Slack endpoint would block setup forever. Added a 15s ceiling matching the gateway's notify path. 2. New gateway-multi-transport.test.ts: constructs a Gateway with BOTH chat.adapters.telegram AND chat.adapters.slack configured (the case no existing test covered), then walks the composite via the same accessors gateway.start() uses. Closes the slim untested seam in buildTransportDelegates + the slack post-initialize wiring around gateway.ts:348-378. Cases: - constructor doesn't crash with both adapters configured. - composite owns both delegates (names: ["slack", "telegram"]). - ownsChatId routes telegram numeric vs slack Cxxx/Dxxx/Uxxx; rejects garbage. - ownsThread routes by platform prefix, including the adapter.telegramFetch boundary check that real telegram threads carry. - notify partitions a heterogeneous chat-id list to both delegates; unrecognized ids dropped, not silently broadcast. - formatNotifySession routes labels through the correct transport. Tests: 673 → 678 (all passing). * docs(slack-adapter): phase 5 — README, architecture, CLAUDE, CHANGELOG Closes the slack-adapter rollout (Phases 0-5). All 678 tests pass on the slack-adapter branch. README: - New "Slack quick start" section: app manifest, token generation, env vs interactive setup, pairing flow with the explicit "open a NEW DM" instruction (chicken-and-egg gap from slack-plan.md §2.4), and a feature support matrix (what's in v1, what's deferred). - Config reference: documents `chat.adapters.slack`, widens `allowedUserIds`/`notifyChatIds` to mixed Telegram numeric + Slack string ids, calls out `SLACK_BOT_TOKEN`/`SLACK_APP_TOKEN` as the preferred secret-storage path. architecture.md: - New "Transport composition" section with an ASCII diagram of how CompositeTransportAdapter routes across delegates by ownsThread/ownsChatId. Tabulates per-method routing rules (postMessage/postRich/notify/handlePairing/etc.). CLAUDE.md: - Updated "Adding a new chat platform" instructions for the new composite + factory-registry layout. - New "Multi-transport composition" subsection. - New "Slack adapter nuances" subsection covering: thread-id format (slack:CHANNEL:THREAD_TS), AdapterPostableMessage shape (no `blocks` field — use `{ card }`), streaming-vs-cards constraint, bot self-loop filter timing, pairing chicken-and-egg via assistant_thread_started, per-transport boot turn partition, and the createSlackAdapter env-var fallback gotcha (zeroConfig = !config). - New "Type widening for multi-transport" subsection — calls out the Number()-coercion sites caught in Phase 1 so future maintainers don't reintroduce them. CHANGELOG: 0.6.0 release notes covering all of Phases 1-4. Version bumped from 0.5.41 → 0.6.0 in package.json. * fix(slack-adapter): phase 5 review fixes Three concrete factual errors flagged by the iter-5 docs review, plus a migration note for external adapter implementers. README: - "Adding a new chat platform" snippet was OUT OF DATE: showed the old `if (config.slack)` inline-import pattern from before Phase 1's factory registry. Replaced with the real three-step process (register in chatAdapterFactories → implement TransportAdapter → buildTransportDelegates entry). - "Files" table: stale `src/gateway.ts` → `src/gateway/gateway.ts`. Stale test count `311 passing` → `678 passing`. Added src/transports/* entries that didn't exist when the table was written. - Slack quick start step 2: promote the `/tmp` manifest path the setup CLI writes (npm-installed users don't have the source tree handy); cite the in-source path as a reference rather than the primary paste source. CLAUDE.md: - Stale `src/gateway.ts` and `src/agents/pi.ts` paths corrected to current locations. - Added the missing nuance: SlackAdapter.attach(slackSdk) MUST be called after chat.initialize() (otherwise webClient calls throw AuthenticationError). The gateway does this for you in start(); documenting the constraint so a future maintainer doesn't try to hoist the call. architecture.md: - Config-model diagram: `slack: { ... } # (future)` → real shape `slack: { mode: "socket" }`. Slack is no longer future-tense. - Session-thread example: `slack:U12345` was wrong (Slack thread ids are `slack:CHANNEL:THREAD_TS`; U-prefixed ids are users, not channels). Corrected to `slack:D12345:`. CHANGELOG: - Added migration note at top of 0.6.0 section calling out the TransportAdapter interface changes for external adapter implementers. Internal users (telegram + slack only) need no action. Tests: 678 passing. * fix(slack-adapter): remove sensitive test tokens, use placeholders * fix(slack-adapter): per-transport botUsername + pin SDK dependency Blockers fixed: 1. Per-transport bot identity: Store optional botUsername override in adapter config (e.g., chat.adapters.slack.botUsername). Resolve at dispatch time via BotUsernameResolver, with fallback to global chat.botUsername. Prevents Slack setup from breaking Telegram command matching when bot names differ. 2. Pinned Slack SDK: Change @chat-adapter/slack from ^4.29.0 to =4.29.0 to prevent silent breaks from minor version bumps that change SDK internals (post API, thread status, typing handler). Design: Codex-approved resolver pattern (SRP + backward compatible) - BotUsernameResolver: pure resolver, no transport awareness, explicit fallback - gateway.ts: resolve per-thread at dispatch time, pass to matchers - test: updated dispatchInTurnCommand signature to use resolved botUsername All 680 tests passing. * fix(slack-adapter): pre-turn botUsername matching + type-safe overrides + test coverage Fixes 3 medium regressions identified by Codex review: 1. Pre-turn commands lost @botName matching - Issue: /cancel@telegram_bot, /verbose@..., etc. broken in Telegram groups - Root: Pre-turn dispatch used empty string "", in-turn had per-thread resolution - Fix: Extract buildMatchers(botUsername) factory, use at dispatch time for both stages - Result: @botName matching works for pre-turn + in-turn (groups and DMs) 2. Adapter override extraction not type-safe - Issue: Non-string botUsername values crash when helpers.ts calls .toLowerCase() - Root: Constructor didn't validate override types - Fix: Only accept string overrides at extraction time, ignore non-strings - Result: Config errors don't cascade to runtime command matching crashes 3. Test coverage incomplete - Issue: Updated test still used old interface; didn't verify per-adapter resolution - Root: GatewayInternals signature stale, test bypassed resolver with hardcoded value - Fix: Update interface, delete unused matchers plumbing, add resolver unit tests - Tests: Slack override vs Telegram fallback, fallback behavior, no override case - Result: 3 new tests (683 total, +3 resolver tests) All 683 tests passing. Pre-turn and in-turn now use shared matcher factory. * fix(slack-adapter): preserve env keys + support mixed notify ID types Fixes 2 Codex findings for multi-transport coexistence: 1. Preserve existing env keys when rebuilding .env (P1) - Issue: stepConfigure() rebuilds .env from scratch, dropping SLACK_BOT_TOKEN/SLACK_APP_TOKEN - Result: Rerunning setup --telegram after Slack setup silently disables Slack - Fix: Read existing .env upfront, preserve unrelated keys (SLACK_*, custom vars) - Now: Slack tokens persist even when Telegram setup reruns 2. Support both numeric and Slack string IDs in notifyChatIds (P2) - Issue: Setup merged notify IDs with map(Number).filter(!isNaN), dropping D.../C... IDs - Result: Slack notification targets removed on setup rerun - Fix: Preserve both types: numeric IDs (Telegram) + string IDs (Slack) - Type: (number | string)[] both in merge + config storage - Now: Slack D.../C... IDs persist in notifyChatIds Multi-transport coexistence now survives rerunning setup (both adapters + env + notify IDs preserved). Tests: 683/683 passing. * fix(slack-adapter): route pairing dispatch to owning transport only Fixes P2 Codex finding: cross-transport pairing side effects. Issue: CompositeTransportAdapter.handlePairing() invoked all delegates for every message. Slack's handlePairing matches on username alone without checking thread ownership. Result: if Telegram sends a message with a username matching pending Slack pairing, Slack's pairing can fire with an invalid (Telegram) chat ID, corrupting notifyChatIds. Fix: Early-return if delegate doesn't own the thread. Only Slack adapter processes Slack threads, only Telegram processes Telegram threads. Pattern: Applies to pairing logic (ownerOf check required before dispatch). All other per-thread methods already have this guard. Tests: 683 passing (pairing tests verify per-transport isolation). * fix(adapters): add defensive ownsThread checks to handlePairing (all adapters) Belt + suspenders: CompositeTransportAdapter already filters pairing dispatch to owning transport, but individual adapters should self-protect too. Pattern: Consistent with defensive early-returns in other methods. Prevents accidental cross-transport dispatch if adapters are called directly (tests, future changes). Applies to: Telegram + Slack (only adapters with handlePairing). Tests: 683 passing. * fix(pr#151): address Codex CLI review findings (2 issues) Issue 1 — HIGH: Legacy roundhouse pair path breaks mixed-ID config Problem: roundhouse pair coerced notifyChatIds through Number() and filtered NaN, dropping Slack D.../C... IDs. Violates backward compatibility for mixed Telegram+Slack deployments. Fix: Apply same mixed-type coercion from setup/steps.ts to setup.ts - Support (number | string)[] for notifyChatIds - Preserve non-numeric Slack IDs during merge - Only coerce to numeric when pushing new entries Issue 2 — MEDIUM: Matcher refactoring incomplete Problem: Pre-turn dispatch uses buildMatchers() factory, but dispatchInTurnCommand() still rebuilt matchers inline. Two implementations to sync = divergence risk. Fix: Extract buildMatchers() as top-level function, use from both dispatch stages - Eliminates code duplication - Single source of truth for matcher logic - Prevents accidental divergence (same regression can't happen again) Files changed: - src/cli/setup.ts: Mixed-type ID preservation (legacy pair path) - src/gateway/gateway.ts: Extract buildMatchers() to top-level, use in both dispatch stages Tests: 683 passing (no new changes, fixes only) * fix(pr#151): cleanup dead matcher code + align setup.ts with setup/steps.ts Remove dead matcher rebuild in handle() that was never consumed (was building matchers then passing botUsername directly to dispatchInTurnCommand). Also remove redundant Number() coercion in setup.ts to exactly match setup/steps.ts pattern: pairTelegram() returns chatId: number, so explicit coercion adds no value and makes pattern less clear. Both changes align with Codex CLI review findings. * fix(pr#151): remove stale _botUsername global (residual dead code) After extracting BotUsernameResolver, command matching uses resolve() directly. The module-global _botUsername cache is no longer read anywhere. Removes: - Let declaration at module scope - Assignment during gateway init Replaces assignment with comment: BotUsernameResolver handles per-transport identity; no global fallback needed. * fix(pr#151): restore buildMatchers + fix ownsThread tests Two regressions from cleanup: 1. buildMatchers moved inside start() method → not accessible to dispatchInTurnCommand (private class method). Moved to module scope. 2. TelegramAdapter tests lacked adapter.telegramFetch for ownsThread check + ROUNDHOUSE_DIR module cache not cleared between tests. Fixes: - Extract buildMatchers to module scope (before class definition) - Add adapter.telegramFetch stub to test fixtures - Add vi.resetModules() to beforeEach to clear module cache 683/683 tests passing ✅ * fix(pr#151): preserve existing BOT_USERNAME in Slack setup P1 issue: Slack setup was overwriting BOT_USERNAME in .env, breaking Telegram group commands in mixed Telegram+Slack installs. At runtime: 1. applyEnvOverrides reads .env + sets config.chat.botUsername 2. BotUsernameResolver falls back to this global for adapters without overrides 3. Telegram commands like /cancel@telegram_bot fail to match Slack bot name Fix: Only set BOT_USERNAME if not already present (via !has() guard). This preserves the Telegram value during Slack setup. For true per-adapter bot names, we'd use TELEGRAM_BOT_USERNAME + SLACK_BOT_USERNAME env keys (follow-up improvement). 683/683 tests passing ✅ * fix(pr#151): merge ALLOWED_USERS instead of replacing on Slack setup P1 issue: Slack setup was replacing ALLOWED_USERS entirely, dropping previously configured users when Slack is added to an existing install. Effect: Env overrides take precedence at runtime, so the gateway's effective allowlist becomes just the newly provided Slack usernames, potentially locking out prior authorized users (e.g., Telegram users) immediately after setup. Fix: Parse existing ALLOWED_USERS (comma-separated), deduplicate with new users via Set, then write merged value. Preserves all prior users. 683/683 tests passing ✅ * fix(pr#151): unquote ALLOWED_USERS before merge and split P1 issue: parseEnvFile returns quoted values (e.g., "alice,bob"), but merge logic was splitting the raw quoted string directly. Effect: On second setup run with existing .env, entries become "alice/bob" with quote characters, then re-quoted via envQuote → quotes become part of usernames. At runtime quoted usernames don't match incoming authors, silently de-authorizing previously allowed users after setup --slack. Fix: Detect and strip surrounding quotes before splitting and merging. Both non-psst + psst paths updated. 683/683 tests passing ✅ * fix(pr#151): derive per-transport boot session id P2 issue: All boot turns used agentThreadId='main', causing cross-transport session contamination in multi-transport setups. Effect: In Telegram+Slack install, second boot turn reuses first transport's agent session. No transport-specific session is seeded despite looping per transport, so startup context is mixed. Fix: Derive agentThreadId per transport via encodeParentThreadId(chatId). Each transport now seeds its own isolated session. Tests: Run needed (aborted earlier - retry below) * fix(pr#151): preserve psst vault values + align Telegram boot session P1 issue: psst path stored BOT_USERNAME + ALLOWED_USERS without preserving existing vault values → users de-authorized after Slack setup in --with-psst mode. Fix: Read existing vault values for both keys, merge ALLOWED_USERS, preserve BOT_USERNAME if already set. Gracefully handle missing keys (new install). P2 issue: Telegram encodeParentThreadId returned 'telegram:<chatId>:main' but inbound routing uses 'group:<chatId>' for negative IDs. Boot turns seeded different session than live group traffic → startup context not visible. Fix: Align encodeParentThreadId to return group:<chatId> for negative IDs, matching resolveAgentThreadId behavior. DM sessions stay as 'main'. Total fixes now: 18 (14 original + 4 new) --------- Co-authored-by: Roy Osherove <osherove@amazon.com> Co-authored-by: Roy Osherove <575051+royosherove@users.noreply.github.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.
Prevents pi-hard-no from reviewing changes to .pi/ settings files.