fix(routing): resolve channel default account instead of hardcoded 'default'#15727
fix(routing): resolve channel default account instead of hardcoded 'default'#15727FuseWeb wants to merge 1 commit intoopenclaw:mainfrom
Conversation
| import { resolveChannelDefaultAccountId } from "../channels/plugins/helpers.js"; | ||
| import { getChannelPlugin } from "../channels/plugins/index.js"; | ||
| import { listBindings } from "./bindings.js"; |
There was a problem hiding this comment.
Heavy plugin registry import
resolve-route.ts now imports getChannelPlugin from src/channels/plugins/index.ts, which (per its own header comment) depends on the active plugin registry and is intentionally “heavy”. resolveAgentRoute() is used in tests/config-only contexts and can run before plugin registry initialization, so calling getChannelPlugin() here can throw (via requireActivePluginRegistry()) or pull in heavy runtime code during routing. Consider switching to a config-only default-account resolution path that doesn’t require plugin registry, or ensure the plugin registry is guaranteed initialized before any routing resolution runs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/routing/resolve-route.ts
Line: 5:7
Comment:
**Heavy plugin registry import**
`resolve-route.ts` now imports `getChannelPlugin` from `src/channels/plugins/index.ts`, which (per its own header comment) depends on the *active plugin registry* and is intentionally “heavy”. `resolveAgentRoute()` is used in tests/config-only contexts and can run before plugin registry initialization, so calling `getChannelPlugin()` here can throw (via `requireActivePluginRegistry()`) or pull in heavy runtime code during routing. Consider switching to a config-only default-account resolution path that doesn’t require plugin registry, or ensure the plugin registry is guaranteed initialized before any routing resolution runs.
How can I resolve this? If you propose a fix, please make it concise.|
Thanks for the feedback @greptile-apps! The try-catch fallback in the latest commit handles the runtime case - if the plugin registry isn't initialized, we gracefully fall back to You're right that this adds a "heavy" import to resolve-route.ts. A cleaner architecture would be a config-only account resolution path, but that would require:
For this bugfix PR, the try-catch approach seems like a reasonable tradeoff - it fixes the issue without major refactoring. Happy to discuss if maintainers prefer a different approach. The key point: in early-init/config-only contexts, falling back to |
|
Can you suggest a fix for this @greptileai? |
|
I see the issue now. Your reasoning about the try-catch is correct, but there's a subtle problem with the fallback behavior that undermines your fix. When the plugin registry isn't initialized and you fall back to In
This means the bug you're fixing reappears in config-only contexts. The real tradeoff here:
A more robust fallback would inspect function resolveChannelDefaultAccountIdForRoute(cfg: OpenClawConfig, channel: string): string {
try {
const plugin = getChannelPlugin(channel);
if (!plugin) {
return DEFAULT_ACCOUNT_ID;
}
return normalizeAccountIdFromSessionKey(resolveChannelDefaultAccountId({ plugin, cfg }));
} catch {
// Plugin registry not initialized - fall back to config introspection
const channelCfg = cfg.channels?.[channel];
if (channelCfg?.accounts && typeof channelCfg.accounts === 'object') {
const accountIds = Object.keys(channelCfg.accounts);
if (accountIds.length === 1) {
return normalizeAccountIdFromSessionKey(accountIds[0]);
}
}
return DEFAULT_ACCOUNT_ID;
}
}This preserves your fix even when the plugin registry isn't ready yet. |
|
Thanks @greptileai! Implemented your suggestion - now falls back to config introspection when plugin registry isn't available. If there's exactly one account in This preserves the fix even in early-init contexts. 🙏 |
|
Perfect! The implementation looks solid. You've correctly added the config fallback in
This preserves the fix for the non-literal default account issue (#9198) even during early initialization when the plugin registry isn't available yet. The regression test also confirms the behavior works correctly. Nice work on the defensive implementation! 🚀 |
bfc1ccb to
f92900f
Compare
c81cfc6 to
ae58c7e
Compare
When a channel has a single account that isn't literally named 'default', bindings with no accountId match should still route to that account. This change: - Adds resolveChannelDefaultAccountIdForRoute to dynamically determine the channel's default account - Updates matchesAccountId to compare against the resolved default instead of hardcoded 'default' - Includes channelDefaultAccountId in the binding cache key for correctness Fixes openclaw#9198
ae58c7e to
18cfffd
Compare
Summary
Fixes #9198 — binding routing fails for multi-account channels where the default account id is not literally
"default".Root Cause
matchesAccountId()insrc/routing/resolve-route.tscompared the inbound account against the hardcodedDEFAULT_ACCOUNT_ID("default") when a binding omittedaccountId. This meant bindings like:…would never match on a channel whose sole (and thus default) account is named
"biz".Fix
resolveChannelDefaultAccountIdfromsrc/channels/plugins/helpers.tsto determine the channel's actual default account id.normalizeAccountIdfromsession-key.ts.Testing
pnpm test src/routing/resolve-route.test.ts)Greptile Overview
Greptile Summary
This PR fixes account matching for bindings that omit
accountIdby resolving the channel’s actual default account id (instead of comparing against the hardcoded"default"), and adds a regression test for channels whose default account id is non-literal.The change is implemented in
src/routing/resolve-route.tsby introducing a helper that looks up the channel plugin and asks it (viaresolveChannelDefaultAccountId) for the default account id, then uses that value when deciding whether a binding with noaccountIdmatches. The test suite is extended insrc/routing/resolve-route.test.tsto cover a single-account channel named"biz".Confidence Score: 2/5
resolveAgentRoute()now imports/callsgetChannelPlugin()from the runtime plugin registry module, which can throw if the registry isn’t initialized and also pulls heavy runtime dependencies into a shared routing path. That’s a behavior change that can surface at runtime outside the test case.Last reviewed commit: ee90398
(3/5) Reply to the agent's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!