Add OnAuthenticate to ITwilioSessionHook，ITwilioCallStatusHook#1379
Open
yileicn wants to merge 3 commits into
Open
Add OnAuthenticate to ITwilioSessionHook，ITwilioCallStatusHook#1379yileicn wants to merge 3 commits into
yileicn wants to merge 3 commits into
Conversation
Contributor
PR Summary by QodoAdd synchronous OnAuthenticate hook for Twilio session and call status flows
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
Contributor
Code Review by Qodo
1. Breaking hook interface change
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+84
to
+92
| // No onebrain identity arrives on the Twilio media stream. Authenticate synchronously | ||
| // (so AsyncLocal identity writes survive) before the streaming session hooks run. | ||
| var request = new ConversationalVoiceRequest | ||
| { | ||
| AgentId = agentId, | ||
| ConversationId = conversationId | ||
| }; | ||
| HookEmitter.Emit<ITwilioSessionHook>(services, hook => hook.OnAuthenticate(request), agentId); | ||
|
|
Contributor
There was a problem hiding this comment.
1. onauthenticate missing agentid validation 📘 Rule violation ☼ Reliability
New synchronous OnAuthenticate hook emissions use request.AgentId/agentId without validating null/empty values at this integration boundary. This can cause unexpected exceptions in hook resolution or incorrect identity propagation when inbound requests/WebSocket paths are malformed.
Agent Prompt
## Issue description
The newly added synchronous `HookEmitter.Emit(...OnAuthenticate...)` calls use `request.AgentId` / `agentId` without validating null/empty inputs.
## Issue Context
These calls are at API/integration boundaries (HTTP endpoints and WebSocket path-derived values). Per compliance, boundary inputs should be validated and failures handled safely.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.Twilio/TwilioStreamMiddleware.cs[84-92]
- src/Plugins/BotSharp.Plugin.Twilio/Controllers/TwilioInboundController.cs[56-63]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+10
to
+16
| /// <summary> | ||
| /// Runs synchronously before any session hook, so identity writes (AsyncLocal) | ||
| /// set here survive into the subsequent async hooks. | ||
| /// </summary> | ||
| /// <param name="request"></param> | ||
| void OnAuthenticate(ConversationalVoiceRequest request); | ||
|
|
Contributor
There was a problem hiding this comment.
2. Breaking hook interface change 🐞 Bug ⚙ Maintainability
ITwilioSessionHook/ITwilioCallStatusHook add a new non-default OnAuthenticate method, which is a source-breaking API change for any external hook implementations. Because the new method is synchronous and mandatory, any auth work needing async I/O will be forced into blocking patterns.
Agent Prompt
### Issue description
Adding `void OnAuthenticate(...)` as a required interface member breaks compilation for all existing implementations of `ITwilioSessionHook` / `ITwilioCallStatusHook`.
### Issue Context
These are extension-point interfaces; even if this repo has no implementers, downstream projects will.
### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.Twilio/Interfaces/ITwilioSessionHook.cs[10-16]
- src/Plugins/BotSharp.Plugin.Twilio/Interfaces/ITwilioCallStatusHook.cs[9-10]
### Suggested fix
- Prefer backward-compatible default interface implementations:
- `void OnAuthenticate(ConversationalVoiceRequest request) { }`
- Or introduce a new optional interface (e.g., `ITwilioSessionAuthHook`) and have emitters call it when present.
- If you intend a hard break, bump major version and document the migration.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+65
to
+67
| // Authenticate (synchronous, so identity writes survive into the async session hooks) | ||
| HookEmitter.Emit<ITwilioSessionHook>(_services, hook => hook.OnAuthenticate(request), request.AgentId); | ||
|
|
Contributor
There was a problem hiding this comment.
3. Auth not run for hooks 🐞 Bug ≡ Correctness
OnAuthenticate is only invoked on some Twilio entrypoints, but many other endpoints still emit ITwilioSessionHook actions without authenticating in that request, so hooks can run without the intended AsyncLocal identity context. This contradicts the new contract comment (“before any session hook”) and will cause inconsistent behavior across Twilio callbacks.
Agent Prompt
### Issue description
`OnAuthenticate` is not invoked before all `ITwilioSessionHook` emissions, so identity context can be missing on later Twilio callbacks (separate HTTP requests).
### Issue Context
Your new interface comment states authentication must run synchronously before any session hook so AsyncLocal identity flows into async hooks.
### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.Twilio/Controllers/TwilioVoiceController.cs[65-67]
### Suggested fix
- Ensure every controller action (and service method, if callable independently) that emits `ITwilioSessionHook` first calls:
- `HookEmitter.Emit<ITwilioSessionHook>(..., h => h.OnAuthenticate(request), request.AgentId);`
- Concretely add this at the start of at least:
- `ReceiveCallerMessage`
- `ReplyCallerMessage`
- any other endpoints that call `HookEmitter.Emit<ITwilioSessionHook>`
- Consider centralizing via an action filter/base controller helper to avoid drift.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+56
to
+58
| // Authenticate (synchronous, so identity writes survive into the async session hooks) | ||
| HookEmitter.Emit<ITwilioSessionHook>(_services, hook => hook.OnAuthenticate(request), request.AgentId); | ||
|
|
Contributor
There was a problem hiding this comment.
4. Authentication failures ignored 🐞 Bug ⛨ Security
OnAuthenticate is emitted via HookEmitter.Emit(Action<T>), which catches and logs exceptions and then continues, so authentication failures cannot stop the controller flow and the request can proceed without identity. This is particularly risky because the method name implies a security boundary but the execution semantics are “best-effort.”
Agent Prompt
### Issue description
Authentication is executed through `HookEmitter.Emit(...)`, which swallows hook exceptions, so an auth hook cannot abort processing.
### Issue Context
`OnAuthenticate` is intended to establish identity context; if it fails, continuing can lead to unauthenticated state mutations or incorrect authorization/auditing.
### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.Twilio/Controllers/TwilioInboundController.cs[56-58]
### Suggested fix
- Add a dedicated fail-fast auth emission path, e.g.:
- `HookEmitter.EmitOrThrow<T>(...)` (no try/catch), or
- extend `HookEmitOption` with `StopOnException` / `ThrowOnException`.
- Alternatively, change the contract to return a result:
- `bool OnAuthenticate(...)` or `AuthenticationResult OnAuthenticate(...)`
- controllers must check result and return 401/403.
- Do not use the current best-effort emitter for security-critical auth.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+84
to
+92
| // No onebrain identity arrives on the Twilio media stream. Authenticate synchronously | ||
| // (so AsyncLocal identity writes survive) before the streaming session hooks run. | ||
| var request = new ConversationalVoiceRequest | ||
| { | ||
| AgentId = agentId, | ||
| ConversationId = conversationId | ||
| }; | ||
| HookEmitter.Emit<ITwilioSessionHook>(services, hook => hook.OnAuthenticate(request), agentId); | ||
|
|
Contributor
There was a problem hiding this comment.
5. Stream auth has limited data 🐞 Bug ☼ Reliability
TwilioStreamMiddleware authenticates a WebSocket stream using a ConversationalVoiceRequest containing only AgentId and ConversationId, so any auth implementation that requires call-specific fields (e.g., CallSid) or request metadata cannot validate at this stage. This makes stream authentication brittle and likely to diverge from the HTTP webhook authentication behavior.
Agent Prompt
### Issue description
Stream authentication is invoked before Twilio "start" event data is available and with a minimally populated request.
### Issue Context
The middleware later parses CallSid from the stream "start" event and stores it on the connection.
### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.Twilio/TwilioStreamMiddleware.cs[84-92]
### Suggested fix
- Option A: defer `OnAuthenticate` until after receiving/parsing the "start" event and then populate the request with CallSid (and any other required fields) before authenticating.
- Option B: introduce a dedicated stream auth hook contract that explicitly documents what identifiers are available (agentId, conversationId) and avoid implying CallSid-based auth.
- Option C: perform two-phase auth: pre-auth by agent/conversation, then enrich/verify once CallSid arrives.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
No description provided.