Skip to content

feat: Invoked IText2SqlHook.SqlExecuting in SqlSelectFn and ExecuteQueryFn #1392

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Oceania2018 merged 1 commit into from
Aug 4, 2026
+10 −2
Merged

feat: Invoked IText2SqlHook.SqlExecuting in SqlSelectFn and ExecuteQueryFn #1392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/Plugins/BotSharp.Plugin.SqlDriver/Functions/ExecuteQueryFn.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,13 @@ public ExecuteQueryFn(IServiceProvider services,

public async Task<bool> Execute(RoleDialogModel message)
{
var dbHook = _services.GetRequiredService<IText2SqlHook>();
// The hook may rewrite message.FunctionArgs (e.g. tag the statements for
// traceability), so it must run before the args are deserialized.
await dbHook.SqlExecuting(message);
Comment on lines +25 to +28

@qodo-code-review qodo-code-review Bot Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Hook exceptions escape handling 🐞 Bug ☼ Reliability

SqlExecuting is invoked before existing error-handling paths (and SqlSelectFn has no local
handling at all), so a throwing hook can abort execution without producing the usual controlled
message.Content/StopCompletion outcomes. This creates a new unhandled-exception path introduced
by this PR.
Agent Prompt 
### Issue description
`IText2SqlHook.SqlExecuting` is now awaited before the function’s existing try/catch handling (and `SqlSelectFn` has none), so any hook exception can bubble out and bypass normal failure reporting.

### Issue Context
The codebase already has a standard safe hook-dispatch mechanism (`HookEmitter.Emit`) that catches and logs hook exceptions per hook.

### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.SqlDriver/Functions/ExecuteQueryFn.cs[25-35]
- src/Plugins/BotSharp.Plugin.SqlDriver/Functions/SqlSelectFn.cs[16-34]
- src/Infrastructure/BotSharp.Core/Infrastructures/HookEmitter.cs[38-64]

### Proposed fix
Wrap `SqlExecuting` invocation with error handling consistent with the rest of the system. Prefer using `HookEmitter.Emit<IText2SqlHook>(...)` (which already catches/logs hook exceptions) or add a local try/catch around `SqlExecuting` that sets `message.Content`, `message.StopCompletion`, and returns `false` in a controlled way.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


var args = JsonSerializer.Deserialize<ExecuteQueryArgs>(message.FunctionArgs) ?? new();
//var refinedArgs = await RefineSqlStatement(message, args);
var dbHook = _services.GetRequiredService<IText2SqlHook>();
var dbType = dbHook.GetDatabaseType(message);
var connectionString = _setting.Connections.FirstOrDefault(x => x.Name.Equals(args.DataSource, StringComparison.OrdinalIgnoreCase))?.ConnectionString;
var dbConnectionString = dbHook.GetConnectionString(message, args.DataSource) ?? connectionString ?? throw new Exception("database connection is not found");
Expand Down
6 changes: 5 additions & 1 deletion src/Plugins/BotSharp.Plugin.SqlDriver/Functions/SqlSelectFn.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ public SqlSelectFn(IServiceProvider services,

public async Task<bool> Execute(RoleDialogModel message)
{
var dbHook = _services.GetRequiredService<IText2SqlHook>();
// The hook may rewrite message.FunctionArgs (e.g. tag the statement for
// traceability), so it must run before the args are deserialized.
await dbHook.SqlExecuting(message);

Comment on lines +18 to +22

@qodo-code-review qodo-code-review Bot Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

2. Multi-hook dispatch bypassed 🐞 Bug ≡ Correctness

SqlExecuting is invoked on a single IText2SqlHook resolved via GetRequiredService, so other
registered/matching hooks won’t receive SqlExecuting and cannot rewrite FunctionArgs. This is
inconsistent with the repo’s hook infrastructure which supports multiple hooks per agent via
GetHooks/HookEmitter.Emit.
Agent Prompt 
### Issue description
`SqlExecuting` is called on a single resolved `IText2SqlHook`, but the repo supports multiple hooks per agent. As a result, only one registered hook receives the new `SqlExecuting` event, and others cannot apply SQL rewrites/tags.

### Issue Context
- `HookProvider.GetHooks<T>` is built on `GetServices<T>()`, enabling multiple hooks.
- `SqlDriverPlanningHook` already uses `HookEmitter.Emit<IText2SqlHook>` for `SqlGenerated`.

### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.SqlDriver/Functions/SqlSelectFn.cs[16-34]
- src/Plugins/BotSharp.Plugin.SqlDriver/Functions/ExecuteQueryFn.cs[25-35]
- src/Infrastructure/BotSharp.Abstraction/Hooks/HookProvider.cs[8-12]
- src/Plugins/BotSharp.Plugin.SqlDriver/Hooks/SqlDriverPlanningHook.cs[25-28]

### Proposed fix
Dispatch `SqlExecuting` through `HookEmitter.Emit<IText2SqlHook>(_services, hook => hook.SqlExecuting(message), message.CurrentAgentId)` (or iterate `_services.GetHooks<IText2SqlHook>(message.CurrentAgentId)`). Keep the single-hook resolution only for `GetDatabaseType` / `GetConnectionString` if the design requires a single authoritative provider.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

var args = JsonSerializer.Deserialize<SqlStatement>(message.FunctionArgs);

if (args.GeneratedWithoutTableDefinition)
Expand All @@ -24,7 +29,6 @@ public async Task<bool> Execute(RoleDialogModel message)
}

// check if need to instantely
var dbHook = _services.GetRequiredService<IText2SqlHook>();
var dbType = dbHook.GetDatabaseType(message);
var dbConnectionString = dbHook.GetConnectionString(message) ??
throw new Exception("database connectdion is not found");
Expand Down
Loading