fix(message): make channel capability probing secret-safe#47520
Closed
vrknetha wants to merge 1 commit into
Closed
fix(message): make channel capability probing secret-safe#47520vrknetha wants to merge 1 commit into
vrknetha wants to merge 1 commit into
Conversation
Contributor
Greptile SummaryThis PR hardens the Key changes and observations:
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/channels/plugins/message-actions.ts
Line: 101
Comment:
**Module-level error log Set is never cleared**
`loggedActionProbeErrors` is process-lifetime state that persists across the entire test suite run. The `afterEach` in `message-actions.test.ts` resets the plugin registry but not this Set. While the current tests pass (they don't spy on `defaultRuntime.error`), any future test that asserts `defaultRuntime.error` is called after a probe failure will silently fail if a previous test already inserted the same key.
Consider exporting a reset helper (behind a test flag or as a separate module export for test utilities), or clearing the Set as part of the test teardown:
```ts
// Exported only for test teardown
export function _resetActionProbeErrorLogForTest() {
loggedActionProbeErrors.clear();
}
```
This keeps the deduplication intact for production while preserving test isolation.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/channels/plugins/message-actions.ts
Line: 103-115
Comment:
**Catch-all swallows non-SecretRef programming errors during probing**
`runSafeActionProbe` catches every thrown value, including bugs in plugin code (e.g. `TypeError: cannot read properties of undefined`). After the first occurrence, subsequent identical errors are silently dropped (deduplication key is in the module-level Set). This means a plugin developer could introduce a real programming error in `listActions` / `supportsButtons` / `supportsCards` and never see it again after the first restart.
Since the only intended tolerance is for unresolved `SecretRef` errors, consider narrowing the catch to a known error type or marker, and re-throwing unrecognised errors:
```ts
function isToleratedProbeError(err: unknown): boolean {
const msg = err instanceof Error ? err.message : String(err);
return msg.includes("SecretRef") || msg.includes("unresolved");
}
function runSafeActionProbe<T>(params: { ... }): T {
try {
return params.fn();
} catch (err) {
if (!isToleratedProbeError(err)) {
throw err; // still surface real bugs
}
logActionProbeError(params.pluginId, params.probe, err);
return params.fallback;
}
}
```
This keeps the fix scoped to its stated purpose and avoids silently swallowing unexpected plugin errors.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 055251d |
8b35708 to
0666c84
Compare
Contributor
Author
|
Rebased onto latest
All 20 tests in the changed files pass locally. The second fix commit ( |
Contributor
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
2 participants
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
messagetool could crash during schema construction when an unrelated channel plugin eagerly resolved account secrets and hit an unresolvedSecretRef.channel=whatsappcould fail because Telegram/Slack capability probing ran first and threw before the send path.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
messagesends no longer fail just because another configured channel has an unresolved token SecretRef.messagetool schema/capability probing now falls back safely instead of crashing the tool.Security Impact (required)
No)Yes)No)No)No)Yes, explain risk + mitigation:Repro + Verification
Environment
file:localcache:refs that were unresolved in the active runtime snapshotSteps
SecretReftoken.messagetool for an explicit send on the healthy channel.Expected
messagetool.Actual
Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
pnpm test -- src/infra/outbound/channel-selection.test.ts src/channels/plugins/message-actions.test.tspnpm buildmessage.sendWhatsApp dry-run succeeded after the fix while unresolved SecretRef warnings remained on unrelated channelspnpm testcurrently hits an unrelated existing failure insrc/media/store.redirect.test.tson file-mode expectations (384vs420)Review Conversations
Compatibility / Migration
Yes)No)No)Failure Recovery (if this breaks)
src/channels/plugins/message-actions.ts,src/infra/outbound/channel-selection.tsRisks and Mitigations