feat: improve permission_denied handling for better model visibility and user errors - #9
feat: improve permission_denied handling for better model visibility and user errors#9clown145 wants to merge 8 commits into
Conversation
…and user errors - Introduce dedicated formatter (tool-result-formatter.ts) that turns `permission_denied` tool results into clear, actionable messages for the model, including guidance: "If this tool/permission is necessary, clearly tell the user and ask them to grant permission before continuing." - All tools (policy + skill restrictions) now use the same consistent path. - Add run-failure-message.ts helper that prioritizes permission-related failure messages when a run ends with a model error after permission was denied. - Update agent-runner and agent-tool-executor to use the new modules. - Add focused unit test for the formatter. This ensures: - The model can reliably see and react to permission issues. - Users see clear English "insufficient permissions" errors instead of raw model backend errors like "openai_error". - No pollution of base prompts; guidance lives in the tool result itself. Refs: recent incident where permission_denied was masked by later model error.
审阅者指南引入一个格式化器和失败消息助手，使 使用 permission_denied 处理运行失败的时序图sequenceDiagram
actor User
participant AgentRunner as runAgentForMessageInternal
participant FailureHelper as getUserFacingFailureMessage
participant DB as D1Database
participant Sender as sendFinalMessage
User->>AgentRunner: Trigger agent run
AgentRunner->>AgentRunner: Execute tools and model
AgentRunner-->>AgentRunner: Error thrown (originalError)
AgentRunner->>FailureHelper: getUserFacingFailureMessage(runId, originalError, db)
FailureHelper->>DB: hasPermissionDeniedToolCall(runId, db)
DB-->>FailureHelper: { hadPermissionDenied }
alt [hadPermissionDenied and looksLikeModelProviderError(originalError)]
FailureHelper-->>AgentRunner: "Run failed: insufficient permissions for required tools"
else [hadPermissionDenied only]
FailureHelper-->>AgentRunner: "Run failed: insufficient permissions for required tools (originalError)"
else [no permission_denied]
FailureHelper-->>AgentRunner: "Run failed: originalError"
end
AgentRunner->>Sender: sendFinalMessage(env, runId, message, userMessage)
Sender-->>User: Final failure message
AgentRunner->>DB: completeRun(db, runId, "failed")
文件级改动
提示与命令与 Sourcery 交互
自定义你的体验访问你的 dashboard 以：
获取帮助Original review guide in EnglishReviewer's GuideIntroduces a formatter and failure-message helper so that permission_denied tool results are rendered as clear, instructional messages to the model and surfaced to users as explicit permission failures instead of opaque model/provider errors, wiring these helpers into the agent runner and tool executor and covering them with unit tests. Sequence diagram for run failure handling with permission_deniedsequenceDiagram
actor User
participant AgentRunner as runAgentForMessageInternal
participant FailureHelper as getUserFacingFailureMessage
participant DB as D1Database
participant Sender as sendFinalMessage
User->>AgentRunner: Trigger agent run
AgentRunner->>AgentRunner: Execute tools and model
AgentRunner-->>AgentRunner: Error thrown (originalError)
AgentRunner->>FailureHelper: getUserFacingFailureMessage(runId, originalError, db)
FailureHelper->>DB: hasPermissionDeniedToolCall(runId, db)
DB-->>FailureHelper: { hadPermissionDenied }
alt [hadPermissionDenied and looksLikeModelProviderError(originalError)]
FailureHelper-->>AgentRunner: "Run failed: insufficient permissions for required tools"
else [hadPermissionDenied only]
FailureHelper-->>AgentRunner: "Run failed: insufficient permissions for required tools (originalError)"
else [no permission_denied]
FailureHelper-->>AgentRunner: "Run failed: originalError"
end
AgentRunner->>Sender: sendFinalMessage(env, runId, message, userMessage)
Sender-->>User: Final failure message
AgentRunner->>DB: completeRun(db, runId, "failed")
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
嗨，我在这里给出了一些高层次的反馈：
- 在
formatToolResultForModel中，建议用 try/catch 包裹JSON.stringify调用（例如在出错时回退到一个简单字符串），以避免当某个工具返回包含循环引用的值时出现运行时错误。 run-failure-message.ts中的looksLikeModelError启发式规则目前比较宽泛（例如匹配任何包含model或provider的消息）；建议收紧这些提示（比如只匹配已知的错误码/前缀如*_error，或特定服务提供商的错误模式），以减少将无关故障误判为模型错误的可能性。
给 AI Agent 的提示
请根据本次代码审查中的评论进行修改：
## 总体评论
- 在 `formatToolResultForModel` 中，建议用 try/catch 包裹 `JSON.stringify` 调用（例如在出错时回退到一个简单字符串），以避免当某个工具返回包含循环引用的值时出现运行时错误。
- `run-failure-message.ts` 中的 `looksLikeModelError` 启发式规则目前比较宽泛（例如匹配任何包含 `model` 或 `provider` 的消息）；建议收紧这些提示（比如只匹配已知的错误码/前缀如 `*_error`，或特定服务提供商的错误模式），以减少将无关故障误判为模型错误的可能性。帮我变得更有用！请在每条评论上点击 👍 或 👎，我会根据你的反馈来改进后续的审查。
Original comment in English
Hey - I've left some high level feedback:
- In
formatToolResultForModel, consider wrapping theJSON.stringifycall in a try/catch (e.g. falling back to a simple string) to avoid runtime errors if a tool ever returns a value with circular references. - The
looksLikeModelErrorheuristic inrun-failure-message.tsis fairly broad (e.g. matching any message containingmodelorprovider); consider tightening these hints (e.g. to known error codes/prefixes like*_erroror provider-specific patterns) to reduce the chance of misclassifying unrelated failures as model errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `formatToolResultForModel`, consider wrapping the `JSON.stringify` call in a try/catch (e.g. falling back to a simple string) to avoid runtime errors if a tool ever returns a value with circular references.
- The `looksLikeModelError` heuristic in `run-failure-message.ts` is fairly broad (e.g. matching any message containing `model` or `provider`); consider tightening these hints (e.g. to known error codes/prefixes like `*_error` or provider-specific patterns) to reduce the chance of misclassifying unrelated failures as model errors.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a tool result formatter that provides clear guidance to the model when a tool execution is denied due to permissions, and refactors the agent runner to generate cleaner, user-facing failure messages. A review comment points out a bug in hasPermissionDeniedToolCall where querying the tool_calls table misses policy-level and skill-level restrictions, which are instead recorded in the run_steps table, and provides a code suggestion to fix this.
| async function hasPermissionDeniedToolCall(runId: string, db: D1Database): Promise<boolean> { | ||
| try { | ||
| const row = await db | ||
| .prepare( | ||
| `SELECT 1 FROM tool_calls | ||
| WHERE run_id = ? AND status = 'permission_denied' | ||
| LIMIT 1` | ||
| ) | ||
| .bind(runId) | ||
| .first(); | ||
|
|
||
| return !!row; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of hasPermissionDeniedToolCall only queries the tool_calls table for status = 'permission_denied'. However, policy-level and skill-level restrictions (handled in agent-tool-executor.ts before executing the tool) are recorded via recordToolCompletedStep with summaryStatus: "skill_denied" and are never inserted into the tool_calls table. As a result, runs failing due to these restrictions will not have their failure messages mapped correctly.
To fix this, we should query the run_steps table, which uniformly records all completed tool steps (including those blocked early with skill_denied or permission_denied).
async function hasPermissionDeniedToolCall(runId: string, db: D1Database): Promise<boolean> {
try {
const row = await db
.prepare(
"SELECT 1 FROM run_steps WHERE run_id = ? AND kind = 'tool_completed' AND (summary LIKE '%: permission_denied' OR summary LIKE '%: skill_denied') LIMIT 1"
)
.bind(runId)
.first();
return !!row;
} catch {
return false;
}
}- Wrap JSON.stringify in try/catch in tool-result-formatter to handle circular references safely. - Improve hasPermissionDeniedToolCall to also check run_steps table (skill_denied + permission_denied summaries), fixing incomplete detection. - Tighten looksLikeModelError heuristic with more specific patterns instead of broad substring matching.
|
@sourcery-ai review |
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request improves error handling and user-facing messages when agent runs fail due to permission denials. It introduces a new tool result formatter to guide the model on permission issues and a utility to check the database for permission-denied steps. The review feedback identifies a critical bug in the database query where a non-existent column summary_status is referenced, causing the check to fail silently, and suggests executing the queries in parallel to avoid performance bottlenecks.
| async function hasPermissionDeniedToolCall(runId: string, db: D1Database): Promise<boolean> { | ||
| try { | ||
| // Check tool_calls table (most common case, including policy denials) | ||
| const toolCallRow = await db | ||
| .prepare( | ||
| `SELECT 1 FROM tool_calls | ||
| WHERE run_id = ? | ||
| AND (status = 'permission_denied' OR error_code IN ('permission_denied', 'skill_tool_not_allowed')) | ||
| LIMIT 1` | ||
| ) | ||
| .bind(runId) | ||
| .first(); | ||
|
|
||
| if (toolCallRow) return true; | ||
|
|
||
| // Also check run_steps for skill-level and other denied cases | ||
| // (some permission denials are recorded here with summary_status) | ||
| const stepRow = await db | ||
| .prepare( | ||
| `SELECT 1 FROM run_steps | ||
| WHERE run_id = ? | ||
| AND (summary_status = 'skill_denied' | ||
| OR summary LIKE '%permission_denied%' | ||
| OR summary LIKE '%denied%') | ||
| LIMIT 1` | ||
| ) | ||
| .bind(runId) | ||
| .first(); | ||
|
|
||
| return !!stepRow; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
There are two issues in this function:
- Correctness Bug (High): The query on
run_stepsreferencessummary_status, which is not a column in therun_stepstable (as defined inruns-repository.tsandrun-step-recorder.ts). The tool completion status is stored inside thesummarycolumn as a formatted string (e.g.,${toolName}: ${summaryStatus}). Because of thetry/catchblock, this query fails silently and always returnsfalsefor therun_stepscheck. - Performance Bottleneck (Medium): The two database queries are executed sequentially, causing two network round-trips to the D1 database. In Cloudflare Workers, this adds unnecessary latency.
We can resolve both issues by querying the summary column instead of summary_status and executing both queries in parallel using Promise.all.
async function hasPermissionDeniedToolCall(runId: string, db: D1Database): Promise<boolean> {
try {
const [toolCallRow, stepRow] = await Promise.all([
db
.prepare(
`SELECT 1 FROM tool_calls
WHERE run_id = ?
AND (status = 'permission_denied' OR error_code IN ('permission_denied', 'skill_tool_not_allowed'))
LIMIT 1`
)
.bind(runId)
.first(),
db
.prepare(
`SELECT 1 FROM run_steps
WHERE run_id = ?
AND (summary LIKE '%skill_denied%'
OR summary LIKE '%permission_denied%'
OR summary LIKE '%denied%')
LIMIT 1`
)
.bind(runId)
.first()
]);
return !!toolCallRow || !!stepRow;
} catch {
return false;
}
}
There was a problem hiding this comment.
Hey - 我发现了 3 个问题，并留下了一些整体性的反馈：
hasPermissionDeniedToolCall查询使用了宽泛的summary LIKE '%denied%'匹配，这容易产生误报；建议改为使用定义明确的状态 / 错误码，或者更具体的匹配模式，以避免把无关的失败误分类为权限问题。- 在
formatPermissionDeniedForModel中，你只暴露了error.message；如果ToolResult携带了额外的上下文（例如错误码、工具名称或权限范围），建议也把这些信息包含到格式化文本中，以帮助模型为用户提供更精确的指导。
给 AI 代理的提示词
Please address the comments from this code review:
## Overall Comments
- The `hasPermissionDeniedToolCall` query uses broad `summary LIKE '%denied%'` matching, which risks false positives; consider narrowing this to well-defined status/error codes or more specific patterns to avoid misclassifying unrelated failures as permission issues.
- In `formatPermissionDeniedForModel`, you only surface `error.message`; if the `ToolResult` carries additional context (e.g., error codes, tool name, or permission scope), consider including those in the formatted text to help the model give more precise guidance to the user.
## Individual Comments
### Comment 1
<location path="src/core/run-failure-message.ts" line_range="56" />
<code_context>
+ .prepare(
+ `SELECT 1 FROM run_steps
+ WHERE run_id = ?
+ AND (summary_status = 'skill_denied'
+ OR summary LIKE '%permission_denied%'
+ OR summary LIKE '%denied%')
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Broad `LIKE '%denied%'` match on `summary` risks false positives for unrelated failures
This predicate is broad enough to classify unrelated failures as permission issues (any error text containing "denied"). That would cause `getUserFacingFailureMessage` to mislabel arbitrary model errors as permission problems. Consider tightening the pattern (e.g. `permission_denied`, `tool_denied`, etc.) or, ideally, basing this only on explicit statuses / error codes you control.
Suggested implementation:
```typescript
const stepRow = await db
.prepare(
`SELECT 1 FROM run_steps
WHERE run_id = ?
AND (summary_status = 'skill_denied'
OR summary LIKE '%permission_denied%'
OR summary LIKE '%permission denied%')
LIMIT 1`
)
.bind(runId)
.first();
```
If the `run_steps` table already has or later gains structured indicators for permission issues (e.g. `summary_status IN ('skill_denied', 'permission_denied', 'tool_denied')` or an explicit error code column), you should further tighten this predicate to rely on those explicit values instead of the `summary` text matching.
</issue_to_address>
### Comment 2
<location path="src/core/run-failure-message.ts" line_range="35-44" />
<code_context>
+async function hasPermissionDeniedToolCall(runId: string, db: D1Database): Promise<boolean> {
</code_context>
<issue_to_address>
**issue (bug_risk):** Swallowing DB errors in `hasPermissionDeniedToolCall` can silently mask data issues and change behavior
Because the catch block returns `false` on any query failure (schema change, transient DB issue, etc.), `getUserFacingFailureMessage` will treat “DB error” the same as “no permission-denied rows” and fall back to the generic model-error message. This hides the real failure mode and complicates outage diagnosis. It would be better to distinguish “no permission-denied rows” from “query failed” (e.g., rethrow, use a tri-state, or similar) and/or log/emit telemetry instead of silently swallowing the error.
</issue_to_address>
### Comment 3
<location path="src/core/model/tool-result-formatter.ts" line_range="16-21" />
<code_context>
+ }
+}
+
+function isToolResult(value: unknown): value is ToolResult {
+ return (
+ value !== null &&
+ typeof value === "object" &&
+ "status" in value &&
+ typeof (value as any).status === "string"
+ );
+}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `isToolResult` type guard is very permissive and may misclassify arbitrary objects as `ToolResult`
The guard currently accepts any object with a string `status` field, so unrelated objects could be treated as `ToolResult`. This means `formatToolResultForModel` might apply permission-denied formatting to non-tool data if they expose `status === 'permission_denied'`. Please tighten the guard (e.g., validate additional `ToolResult`-specific fields and/or restrict `status` to a known enum of values) to avoid misclassification.
Suggested implementation:
```typescript
const TOOL_RESULT_STATUSES = new Set<string>(["permission_denied"]);
function isToolResult(value: unknown): value is ToolResult {
if (value === null || typeof value !== "object") {
return false;
}
const candidate = value as Partial<ToolResult> & Record<string, unknown>;
if (typeof candidate.status !== "string") {
return false;
}
if (!TOOL_RESULT_STATUSES.has(candidate.status)) {
return false;
}
// Require at least one additional ToolResult-specific structural field
// to avoid matching arbitrary objects that just happen to have a status.
const hasToolIdentifier =
typeof (candidate as any).toolName === "string" ||
typeof (candidate as any).toolId === "string" ||
typeof (candidate as any).name === "string";
if (!hasToolIdentifier) {
return false;
}
return true;
}
```
To fully align this guard with your real `ToolResult` type:
1. Replace the `TOOL_RESULT_STATUSES` contents with the complete set of valid `ToolResult["status"]` values (e.g. `["ok", "error", "permission_denied"]` if those exist).
2. Update the structural checks in `hasToolIdentifier` to match the actual identifier fields on `ToolResult` (for example, if the interface uses `toolName` only, you can simplify to `typeof candidate.toolName === "string"`).
3. If `ToolResult` has other required fields (e.g. `result`, `error`, `metadata`), add corresponding runtime checks here to further tighten the guard.
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据这些反馈改进之后的评审。
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- The
hasPermissionDeniedToolCallquery uses broadsummary LIKE '%denied%'matching, which risks false positives; consider narrowing this to well-defined status/error codes or more specific patterns to avoid misclassifying unrelated failures as permission issues. - In
formatPermissionDeniedForModel, you only surfaceerror.message; if theToolResultcarries additional context (e.g., error codes, tool name, or permission scope), consider including those in the formatted text to help the model give more precise guidance to the user.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `hasPermissionDeniedToolCall` query uses broad `summary LIKE '%denied%'` matching, which risks false positives; consider narrowing this to well-defined status/error codes or more specific patterns to avoid misclassifying unrelated failures as permission issues.
- In `formatPermissionDeniedForModel`, you only surface `error.message`; if the `ToolResult` carries additional context (e.g., error codes, tool name, or permission scope), consider including those in the formatted text to help the model give more precise guidance to the user.
## Individual Comments
### Comment 1
<location path="src/core/run-failure-message.ts" line_range="56" />
<code_context>
+ .prepare(
+ `SELECT 1 FROM run_steps
+ WHERE run_id = ?
+ AND (summary_status = 'skill_denied'
+ OR summary LIKE '%permission_denied%'
+ OR summary LIKE '%denied%')
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Broad `LIKE '%denied%'` match on `summary` risks false positives for unrelated failures
This predicate is broad enough to classify unrelated failures as permission issues (any error text containing "denied"). That would cause `getUserFacingFailureMessage` to mislabel arbitrary model errors as permission problems. Consider tightening the pattern (e.g. `permission_denied`, `tool_denied`, etc.) or, ideally, basing this only on explicit statuses / error codes you control.
Suggested implementation:
```typescript
const stepRow = await db
.prepare(
`SELECT 1 FROM run_steps
WHERE run_id = ?
AND (summary_status = 'skill_denied'
OR summary LIKE '%permission_denied%'
OR summary LIKE '%permission denied%')
LIMIT 1`
)
.bind(runId)
.first();
```
If the `run_steps` table already has or later gains structured indicators for permission issues (e.g. `summary_status IN ('skill_denied', 'permission_denied', 'tool_denied')` or an explicit error code column), you should further tighten this predicate to rely on those explicit values instead of the `summary` text matching.
</issue_to_address>
### Comment 2
<location path="src/core/run-failure-message.ts" line_range="35-44" />
<code_context>
+async function hasPermissionDeniedToolCall(runId: string, db: D1Database): Promise<boolean> {
</code_context>
<issue_to_address>
**issue (bug_risk):** Swallowing DB errors in `hasPermissionDeniedToolCall` can silently mask data issues and change behavior
Because the catch block returns `false` on any query failure (schema change, transient DB issue, etc.), `getUserFacingFailureMessage` will treat “DB error” the same as “no permission-denied rows” and fall back to the generic model-error message. This hides the real failure mode and complicates outage diagnosis. It would be better to distinguish “no permission-denied rows” from “query failed” (e.g., rethrow, use a tri-state, or similar) and/or log/emit telemetry instead of silently swallowing the error.
</issue_to_address>
### Comment 3
<location path="src/core/model/tool-result-formatter.ts" line_range="16-21" />
<code_context>
+ }
+}
+
+function isToolResult(value: unknown): value is ToolResult {
+ return (
+ value !== null &&
+ typeof value === "object" &&
+ "status" in value &&
+ typeof (value as any).status === "string"
+ );
+}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `isToolResult` type guard is very permissive and may misclassify arbitrary objects as `ToolResult`
The guard currently accepts any object with a string `status` field, so unrelated objects could be treated as `ToolResult`. This means `formatToolResultForModel` might apply permission-denied formatting to non-tool data if they expose `status === 'permission_denied'`. Please tighten the guard (e.g., validate additional `ToolResult`-specific fields and/or restrict `status` to a known enum of values) to avoid misclassification.
Suggested implementation:
```typescript
const TOOL_RESULT_STATUSES = new Set<string>(["permission_denied"]);
function isToolResult(value: unknown): value is ToolResult {
if (value === null || typeof value !== "object") {
return false;
}
const candidate = value as Partial<ToolResult> & Record<string, unknown>;
if (typeof candidate.status !== "string") {
return false;
}
if (!TOOL_RESULT_STATUSES.has(candidate.status)) {
return false;
}
// Require at least one additional ToolResult-specific structural field
// to avoid matching arbitrary objects that just happen to have a status.
const hasToolIdentifier =
typeof (candidate as any).toolName === "string" ||
typeof (candidate as any).toolId === "string" ||
typeof (candidate as any).name === "string";
if (!hasToolIdentifier) {
return false;
}
return true;
}
```
To fully align this guard with your real `ToolResult` type:
1. Replace the `TOOL_RESULT_STATUSES` contents with the complete set of valid `ToolResult["status"]` values (e.g. `["ok", "error", "permission_denied"]` if those exist).
2. Update the structural checks in `hasToolIdentifier` to match the actual identifier fields on `ToolResult` (for example, if the interface uses `toolName` only, you can simplify to `typeof candidate.toolName === "string"`).
3. If `ToolResult` has other required fields (e.g. `result`, `error`, `metadata`), add corresponding runtime checks here to further tighten the guard.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@gemini-code-assist review |
1 similar comment
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request improves how tool execution results and agent run failures are handled, specifically focusing on permission denials. It introduces a formatter to provide clear instructions to the model when a tool is denied, and a helper to generate user-facing failure messages. However, a critical issue was identified in src/core/run-failure-message.ts where querying a non-existent summary_status column in the run_steps table will trigger a SQLite error, causing the permission check to always fail. Additionally, the broad LIKE '%denied%' query should be refined to target specific denial patterns.
| const stepRow = await db | ||
| .prepare( | ||
| `SELECT 1 FROM run_steps | ||
| WHERE run_id = ? | ||
| AND (summary_status = 'skill_denied' | ||
| OR summary LIKE '%permission_denied%' | ||
| OR summary LIKE '%denied%') | ||
| LIMIT 1` | ||
| ) |
There was a problem hiding this comment.
The run_steps table does not have a summary_status column (as defined in runs-repository.ts where only id, run_id, agent_id, status, kind, summary, created_at are inserted). Querying summary_status will throw a SQLite error, causing the try-catch block to always return false and fail to detect skill-level permission denials.
Additionally, LIKE '%denied%' is overly broad and could match unrelated terms (e.g., "undeniable"). Since the statuses are recorded as part of the summary string (e.g., toolName: skill_denied or toolName: permission_denied), we should query summary LIKE '%skill_denied%' and summary LIKE '%permission_denied%' instead.
const stepRow = await db
.prepare(
`SELECT 1 FROM run_steps
WHERE run_id = ?
AND (summary LIKE '%skill_denied%'
OR summary LIKE '%permission_denied%')
LIMIT 1`
)
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces structured formatting for tool permission denial results to guide the model on how to handle them, and adds a utility to generate clearer user-facing failure messages by querying the database for permission denials. A critical issue was identified in src/core/run-failure-message.ts where querying a non-existent summary_status column in the run_steps table will cause a silent database error, breaking the fallback check. A more robust query pattern was suggested to resolve this.
| const stepRow = await db | ||
| .prepare( | ||
| `SELECT 1 FROM run_steps | ||
| WHERE run_id = ? | ||
| AND (summary_status = 'skill_denied' | ||
| OR summary LIKE '%permission_denied%' | ||
| OR summary LIKE '%denied%') | ||
| LIMIT 1` | ||
| ) | ||
| .bind(runId) | ||
| .first(); |
There was a problem hiding this comment.
The run_steps table does not have a summary_status column (as defined in runs-repository.ts). Querying summary_status = 'skill_denied' will throw a SQLite error, which is caught by the try/catch block and silently returns false. This completely breaks the fallback check for skill-level permission denials recorded in run_steps.
Additionally, summary LIKE '%permission_denied%' is redundant when summary LIKE '%denied%' is used, and %denied% is too broad and prone to false positives.
Since recordToolCompletedStep records the status in the summary column as ${toolName}: ${summaryStatus}, we should query summary LIKE '%skill_denied%' or summary LIKE '%permission_denied%' instead.
const stepRow = await db
.prepare(
`SELECT 1 FROM run_steps
WHERE run_id = ?
AND (summary LIKE '%skill_denied%'
OR summary LIKE '%permission_denied%')
LIMIT 1`
)
.bind(runId)
.first();
There was a problem hiding this comment.
Code Review
This pull request improves the handling of tool permission denials by formatting permission-denied tool results with explicit instructions for the model, and customizing the final run failure message when permission denials are detected in the database. Feedback focuses on addressing a database query issue in hasPermissionDeniedToolCall where a non-existent column (summary_status) is queried and a shared try-catch block prevents proper fallback execution. Additionally, it is recommended to wrap getUserFacingFailureMessage in a try-catch block within the agent runner to prevent database errors from crashing the main error handler and leaving runs stuck in a running state.
| async function hasPermissionDeniedToolCall(runId: string, db: D1Database): Promise<boolean> { | ||
| try { | ||
| // Check tool_calls table (most common case, including policy denials) | ||
| const toolCallRow = await db | ||
| .prepare( | ||
| `SELECT 1 FROM tool_calls | ||
| WHERE run_id = ? | ||
| AND (status = 'permission_denied' OR error_code IN ('permission_denied', 'skill_tool_not_allowed')) | ||
| LIMIT 1` | ||
| ) | ||
| .bind(runId) | ||
| .first(); | ||
|
|
||
| if (toolCallRow) return true; | ||
|
|
||
| // Also check run_steps for skill-level and other denied cases | ||
| // (some permission denials are recorded here with summary_status) | ||
| const stepRow = await db | ||
| .prepare( | ||
| `SELECT 1 FROM run_steps | ||
| WHERE run_id = ? | ||
| AND (summary_status = 'skill_denied' | ||
| OR summary LIKE '%permission_denied%' | ||
| OR summary LIKE '%denied%') | ||
| LIMIT 1` | ||
| ) | ||
| .bind(runId) | ||
| .first(); | ||
|
|
||
| return !!stepRow; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
There are two issues in hasPermissionDeniedToolCall:
- Non-existent column: The
run_stepstable does not have asummary_statuscolumn (as seen inruns-repository.tsandrun-step-recorder.ts). Instead, the status of the tool completion is recorded inside thesummarycolumn as part of a formatted string (e.g.,"${toolCall.name}: skill_denied"). Queryingsummary_status = 'skill_denied'will throw a database error. - Shared
try-catchblock: Because both queries are wrapped in a singletry-catchblock, any error in the first query (or the column error in the second query) will cause the function to immediately returnfalse, skipping the fallback check entirely.
We should wrap each query in its own try-catch block and update the run_steps query to check summary LIKE '%skill_denied%'.
async function hasPermissionDeniedToolCall(runId: string, db: D1Database): Promise<boolean> {
// Check tool_calls table (most common case, including policy denials)
try {
const toolCallRow = await db
.prepare(
"SELECT 1 FROM tool_calls WHERE run_id = ? AND (status = 'permission_denied' OR error_code IN ('permission_denied', 'skill_tool_not_allowed')) LIMIT 1"
)
.bind(runId)
.first();
if (toolCallRow) return true;
} catch {
// Proceed to fallback check if tool_calls query fails
}
// Also check run_steps for skill-level and other denied cases
try {
const stepRow = await db
.prepare(
"SELECT 1 FROM run_steps WHERE run_id = ? AND (summary LIKE '%skill_denied%' OR summary LIKE '%permission_denied%' OR summary LIKE '%denied%') LIMIT 1"
)
.bind(runId)
.first();
return !!stepRow;
} catch {
return false;
}
}| } | ||
| ); | ||
|
|
||
| const userMessage = await getUserFacingFailureMessage(runId, summary, env.AGENT_DB); |
There was a problem hiding this comment.
If getUserFacingFailureMessage throws an error (e.g., due to database connectivity issues or unexpected query failures), it will crash the outer catch block. This will prevent sendFinalMessage from being sent and, more importantly, prevent completeRun(env.AGENT_DB, runId, "failed") from executing, leaving the run stuck in the running state.
To ensure robust error handling and defensive programming, wrap the call to getUserFacingFailureMessage in a try-catch block and fallback to the default failure message.
let userMessage = "Run failed: " + summary;
try {
userMessage = await getUserFacingFailureMessage(runId, summary, env.AGENT_DB);
} catch {
// Fallback to default message if database query fails
}- hasPermissionDeniedToolCall now returns {hadPermissionDenied, dbError?} to distinguish query failures from 'no denial found' (addresses swallowing DB errors)
- Wrap getUserFacingFailureMessage call in agent-runner catch with its own try/catch + logging so DB helper failures never break failure reporting
- Tighten run_steps matching from broad LIKE '%denied%' to precise 'toolName: skill_denied' / 'toolName: permission_denied' patterns
- Run the tool_calls and run_steps permission checks in parallel with Promise.all
- Strengthen isToolResult type guard: require permission_denied status + at least one identifier field (toolName/name/error)
- Enhance formatPermissionDeniedForModel to include tool name and error code in the guidance sent to the model
- Update unit tests to assert the improved, more informative output strings
- All checks green: prettier, eslint, tsc --noEmit, full vitest suite (279 tests)
Refs: PR #9, Gemini 3rd review
|
@gemini-code-assist review Gemini 第三轮 review 反馈已全部解决 + 全项目 Prettier 格式化完成并成功推送（使用正确 SOCKS5 代理 20170）。 已修复内容：
所有检查已绿：
请重新 review，感谢！ |
There was a problem hiding this comment.
Code Review
This pull request introduces structured formatting for tool results and run failures, specifically focusing on handling permission-denied scenarios. It adds formatToolResultForModel to provide clear guidance to the model when a tool execution is denied, and getUserFacingFailureMessage to return user-friendly error messages when runs fail due to insufficient permissions. The review feedback highlights critical issues in the isToolResult type guard, which could fail to identify permission-denied results due to overly strict checks and incorrect null handling for the error object. Additionally, the feedback points out that ToolResult objects do not natively contain the toolName property, recommending that the tool name be passed explicitly from the executor context to ensure the formatted message includes the correct tool information.
| function isToolResult(value: unknown): value is ToolResult { | ||
| if (value === null || typeof value !== "object") { | ||
| return false; | ||
| } | ||
|
|
||
| const candidate = value as Partial<ToolResult> & Record<string, unknown>; | ||
|
|
||
| if (typeof candidate.status !== "string") { | ||
| return false; | ||
| } | ||
|
|
||
| // Only treat objects that have a known permission_denied status as ToolResult for this formatter | ||
| if (!PERMISSION_DENIED_STATUSES.has(candidate.status)) { | ||
| return false; | ||
| } | ||
|
|
||
| // Require at least one additional identifier to avoid matching random objects | ||
| const hasIdentifier = | ||
| typeof candidate.toolName === "string" || | ||
| typeof (candidate as any).name === "string" || | ||
| typeof candidate.error === "object"; | ||
|
|
||
| return hasIdentifier; | ||
| } |
There was a problem hiding this comment.
The isToolResult function currently requires an additional identifier (toolName, name, or error) to classify an object as a ToolResult. However, according to the ToolResult type definition, error is optional, and toolName/name are not standard properties on the root of ToolResult. If a tool returns a simple { status: "permission_denied" } without an error object, it will fail this check and won't be formatted with the model guidance.
Additionally, typeof candidate.error === "object" will evaluate to true if error is null (since typeof null === "object" in JavaScript), which can lead to unexpected behavior.
Since formatToolResultForModel is only called on tool results in agent-tool-executor.ts, we can simplify this check to only verify the status property.
function isToolResult(value: unknown): value is ToolResult {
if (value === null || typeof value !== "object") {
return false;
}
const candidate = value as Partial<ToolResult> & Record<string, unknown>;
return typeof candidate.status === "string" && PERMISSION_DENIED_STATUSES.has(candidate.status);
}| export function formatToolResultForModel(result: unknown): string { | ||
| if (isToolResult(result) && result.status === "permission_denied") { | ||
| return formatPermissionDeniedForModel(result); | ||
| } | ||
|
|
||
| // Default behavior for other results | ||
| try { | ||
| return JSON.stringify(result ?? null); | ||
| } catch { | ||
| return JSON.stringify({ error: "Failed to serialize tool result" }); | ||
| } | ||
| } | ||
|
|
||
| function formatPermissionDeniedForModel(result: ToolResult): string { | ||
| const error = result.error; | ||
| const reason = error?.message ?? "Permission denied"; | ||
| const code = error?.code ? ` (code: ${error.code})` : ""; | ||
|
|
||
| const toolInfo = (result as any).toolName ? ` for tool "${(result as any).toolName}"` : ""; | ||
|
|
||
| return [ | ||
| `Permission denied${toolInfo}${code}.`, | ||
| `Reason: ${reason}`, | ||
| "", | ||
| "If this tool or permission is necessary to complete the user's request, clearly explain the limitation to the user and ask them to grant the required permission before continuing.", | ||
| "Do not keep attempting the same restricted action." | ||
| ].join("\n"); | ||
| } |
There was a problem hiding this comment.
The formatPermissionDeniedForModel function attempts to read (result as any).toolName to include the tool name in the model prompt. However, ToolResult objects returned by tool executors do not contain the toolName property on their root. As a result, toolInfo will always be empty, and the model will not receive the name of the tool that was denied.
To fix this, we should pass the toolName as an optional parameter to formatToolResultForModel and formatPermissionDeniedForModel so it can be correctly populated from the tool call context.
export function formatToolResultForModel(result: unknown, toolName?: string): string {
if (isToolResult(result) && result.status === "permission_denied") {
return formatPermissionDeniedForModel(result, toolName);
}
// Default behavior for other results
try {
return JSON.stringify(result ?? null);
} catch {
return JSON.stringify({ error: "Failed to serialize tool result" });
}
}
function formatPermissionDeniedForModel(result: ToolResult, toolName?: string): string {
const error = result.error;
const reason = error?.message ?? "Permission denied";
const code = error?.code ? ` (code: ${error.code})` : "";
const name = toolName || (result as any).toolName || (result as any).name;
const toolInfo = name ? ` for tool "${name}"` : "";
return [
`Permission denied${toolInfo}${code}.`,
`Reason: ${reason}`,
"",
"If this tool or permission is necessary to complete the user's request, clearly explain the limitation to the user and ask them to grant the required permission before continuing.",
"Do not keep attempting the same restricted action."
].join("\n");
}| toolCallId: toolCall.id, | ||
| toolName: toolCall.name, | ||
| content: stringifyToolResult(result) | ||
| content: formatToolResultForModel(result) |
There was a problem hiding this comment.
Pass toolCall.name to formatToolResultForModel so that the formatted permission denied message can include the name of the tool that was denied.
| content: formatToolResultForModel(result) | |
| content: formatToolResultForModel(result, toolCall.name) |
…fter previous tightening) - Make formatToolResultForModel accept optional explicit toolName parameter (second arg) - Pass toolCall.name explicitly from createToolResultMessage in agent-tool-executor - Relax isToolResult guard for permission_denied: only require status + non-null error object (ToolResult does not carry toolName) - Handle null error correctly (Gemini concern) - toolName still used for nice output in formatPermissionDeniedForModel when provided - All checks green: prettier, tsc, 279 tests Refs: PR #9, latest Gemini review after 3rd round fixes
|
@gemini-code-assist review 最新修复（针对 Gemini 最新一轮反馈）：
所有检查再次全绿：
请重新 review，感谢！ |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to format tool results and run failure messages, specifically providing clearer instructions to the model and user when a tool execution is denied due to insufficient permissions. Feedback on these changes suggests relaxing the isToolResult type guard to allow an optional error property, adding a corresponding unit test for permission-denied results without an error payload, and restricting the database query on run_steps to kind = 'tool_completed' to prevent false positives.
| // For permission_denied results, require a proper error object (not null) | ||
| const error = candidate.error; | ||
| if (error === null || typeof error !== "object") { | ||
| return false; | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
The type guard isToolResult strictly requires candidate.error to be a non-null object. However, according to the ToolResult definition in src/tools/types.ts, the error property is optional (error?: ...). If a tool returns a permission_denied status without an error payload, isToolResult will return false, causing formatToolResultForModel to fall back to standard JSON serialization and bypass the helpful instructions.
We should allow error to be undefined in the type guard, especially since formatPermissionDeniedForModel is already designed to handle a missing error object gracefully.
| // For permission_denied results, require a proper error object (not null) | |
| const error = candidate.error; | |
| if (error === null || typeof error !== "object") { | |
| return false; | |
| } | |
| return true; | |
| // For permission_denied results, if an error object is present, ensure it is a proper object (not null) | |
| const error = candidate.error; | |
| if (error !== undefined && (error === null || typeof error !== "object")) { | |
| return false; | |
| } | |
| return true; |
| // Check run_steps for recorded denials (skill restrictions etc.) | ||
| // These are written as failed steps with summary in format "toolName: skill_denied" | ||
| db | ||
| .prepare( | ||
| `SELECT 1 FROM run_steps | ||
| WHERE run_id = ? | ||
| AND status = 'failed' | ||
| AND (summary LIKE '%: skill_denied' | ||
| OR summary LIKE '%: permission_denied') | ||
| LIMIT 1` | ||
| ) |
There was a problem hiding this comment.
To prevent potential false positives, we should restrict the query on run_steps to only match steps where kind = 'tool_completed'. This ensures we only match actual tool completion failures and avoid matching other run failure steps that might happen to contain : skill_denied or : permission_denied in their summaries.
| // Check run_steps for recorded denials (skill restrictions etc.) | |
| // These are written as failed steps with summary in format "toolName: skill_denied" | |
| db | |
| .prepare( | |
| `SELECT 1 FROM run_steps | |
| WHERE run_id = ? | |
| AND status = 'failed' | |
| AND (summary LIKE '%: skill_denied' | |
| OR summary LIKE '%: permission_denied') | |
| LIMIT 1` | |
| ) | |
| // Check run_steps for recorded denials (skill restrictions etc.) | |
| // These are written as failed steps with summary in format "toolName: skill_denied" | |
| db | |
| .prepare( | |
| `SELECT 1 FROM run_steps | |
| WHERE run_id = ? | |
| AND kind = 'tool_completed' | |
| AND status = 'failed' | |
| AND (summary LIKE '%: skill_denied' | |
| OR summary LIKE '%: permission_denied') | |
| LIMIT 1` | |
| ) |
| }); | ||
|
|
There was a problem hiding this comment.
Add a unit test to verify that formatToolResultForModel gracefully handles permission_denied results that do not contain an error object.
});
it("handles permission_denied without an error object gracefully", () => {
const result = {
status: "permission_denied" as const
};
const output = formatToolResultForModel(result);
expect(output).toContain("Permission denied");
expect(output).toContain("If this tool or permission is necessary");
});
|
@gemini-code-assist review |
|
@sourcery-ai review |
There was a problem hiding this comment.
Code Review
This pull request introduces structured formatting for tool results and run failure messages, specifically improving how permission denial errors are handled and presented to the user and the model. Key changes include the addition of tool-result-formatter.ts to provide clear guidance to the model on permission issues, and run-failure-message.ts to query the database and generate user-friendly error messages when a run fails due to insufficient permissions. Feedback on these changes suggests renaming and refactoring the misleadingly named isToolResult type guard to isPermissionDeniedToolResult for better clarity, and expanding the MODEL_ERROR_PATTERNS list to include Anthropic and Claude error patterns to ensure consistent behavior across all supported model providers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const PERMISSION_DENIED_STATUSES = new Set<string>(["permission_denied"]); | ||
|
|
||
| function isToolResult(value: unknown): value is ToolResult { | ||
| if (value === null || typeof value !== "object") { | ||
| return false; | ||
| } | ||
|
|
||
| const candidate = value as Partial<ToolResult> & Record<string, unknown>; | ||
|
|
||
| if (typeof candidate.status !== "string") { | ||
| return false; | ||
| } | ||
|
|
||
| if (!PERMISSION_DENIED_STATUSES.has(candidate.status)) { | ||
| return false; | ||
| } | ||
|
|
||
| // If present, permission_denied error payloads must be real objects. | ||
| const error = candidate.error; | ||
| if (error !== undefined && (error === null || typeof error !== "object")) { | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| export function formatToolResultForModel(result: unknown, toolName?: string): string { | ||
| if (isToolResult(result) && result.status === "permission_denied") { | ||
| return formatPermissionDeniedForModel(result, toolName); | ||
| } |
There was a problem hiding this comment.
The type guard isToolResult is misleadingly named and implemented because it only returns true for tool results with a status of "permission_denied". If a valid ToolResult with a "success" status is passed, it returns false, which breaks standard TypeScript type narrowing expectations.
Additionally, using a Set for a single status check adds unnecessary overhead, and the subsequent check result.status === "permission_denied" in formatToolResultForModel is redundant.
We can rename this to isPermissionDeniedToolResult and narrow the type to ToolResult & { status: "permission_denied" } directly, simplifying the logic and improving readability.
function isPermissionDeniedToolResult(value: unknown): value is ToolResult & { status: "permission_denied" } {
if (value === null || typeof value !== "object") {
return false;
}
const candidate = value as Record<string, unknown>;
if (candidate.status !== "permission_denied") {
return false;
}
const error = candidate.error;
if (error !== undefined && (error === null || typeof error !== "object")) {
return false;
}
return true;
}
export function formatToolResultForModel(result: unknown, toolName?: string): string {
if (isPermissionDeniedToolResult(result)) {
return formatPermissionDeniedForModel(result, toolName);
}| const MODEL_ERROR_PATTERNS = [ | ||
| "openai_error", | ||
| "gemini_error", | ||
| "model_error", | ||
| "api_error", | ||
| "rate limit", | ||
| "quota exceeded", | ||
| "context length", | ||
| "token limit exceeded", | ||
| "timeout", | ||
| "429" | ||
| ]; |
There was a problem hiding this comment.
The MODEL_ERROR_PATTERNS list is missing common patterns for other major LLM providers like Anthropic/Claude (e.g., anthropic, claude). If a run fails with an Anthropic-specific error after a permission denial, it might not be recognized as a model error, causing the raw error to leak instead of showing the user-friendly permission error.
Adding these patterns ensures consistent behavior across all configured model providers.
const MODEL_ERROR_PATTERNS = [
"openai_error",
"gemini_error",
"anthropic",
"claude",
"model_error",
"api_error",
"rate limit",
"quota exceeded",
"context length",
"token limit exceeded",
"timeout",
"429"
];
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并留下了一些总体反馈：
run-failure-message.ts中的MODEL_ERROR_PATTERNS启发式规则是基于字符串并且是硬编码的；建议将其集中管理或做成可配置，这样更容易与实际的模型错误类型保持同步，并且在不同服务商演进时减少误报/漏报。hasPermissionDeniedToolCall逻辑依赖特定的数据库结构和字符串格式（例如summary LIKE '%: skill_denied'）；相比依赖自由格式的 summary 文本，基于明确的状态/错误字段或结构化 JSON 可能会更健壮，从而避免在 summary 格式变更时出现破坏性影响。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `MODEL_ERROR_PATTERNS` heuristic in `run-failure-message.ts` is string-based and hardcoded; consider centralizing or making it configurable so it’s easier to keep in sync with actual model error types and reduce false positives/negatives as providers evolve.
- The `hasPermissionDeniedToolCall` logic relies on specific DB shapes and string formats (e.g. `summary LIKE '%: skill_denied'`); it might be more robust to key off explicit status/error fields or structured JSON instead of free-form summary text to avoid breaking if summary formatting changes.
## Individual Comments
### Comment 1
<location path="src/core/model/tool-result-formatter.ts" line_range="5-14" />
<code_context>
+
+const PERMISSION_DENIED_STATUSES = new Set<string>(["permission_denied"]);
+
+function isToolResult(value: unknown): value is ToolResult {
+ if (value === null || typeof value !== "object") {
+ return false;
+ }
+
+ const candidate = value as Partial<ToolResult> & Record<string, unknown>;
+
+ if (typeof candidate.status !== "string") {
+ return false;
+ }
+
+ if (!PERMISSION_DENIED_STATUSES.has(candidate.status)) {
+ return false;
+ }
+
+ // If present, permission_denied error payloads must be real objects.
+ const error = candidate.error;
+ if (error !== undefined && (error === null || typeof error !== "object")) {
+ return false;
+ }
+
+ return true;
+}
+
</code_context>
<issue_to_address>
**suggestion:** `isToolResult` 类型保护目前只识别 `permission_denied` 结果，这比函数名所暗示的范围更窄。
由于它只匹配 `permission_denied`，这个类型保护比名字所表达的更具体，可能会被误用为通用的 `ToolResult` 检查。建议重命名（例如 `isPermissionDeniedToolResult`），或者扩展该类型保护以校验完整的 `ToolResult` 结构，并单独检查 `status`，从而更好地反映其意图并避免误用。
建议实现：
```typescript
const PERMISSION_DENIED_STATUSES = new Set<string>(["permission_denied"]);
```
```typescript
function isPermissionDeniedToolResult(value: unknown): value is ToolResult {
```
你需要：
1. 将代码库中所有依赖该特定 `permission_denied` 检查的 `isToolResult` 用法替换为 `isPermissionDeniedToolResult`。
2. 如果有任何地方 `isToolResult` 原本是作为通用 `ToolResult` 结构类型保护来使用的，考虑实现一个新的、更通用的 `isToolResult`，用于校验完整的 `ToolResult` 结构，并在那里使用它。
</issue_to_address>帮我变得更有用！请对每条评论点 👍 或 👎，我会根据这些反馈改进后续评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The
MODEL_ERROR_PATTERNSheuristic inrun-failure-message.tsis string-based and hardcoded; consider centralizing or making it configurable so it’s easier to keep in sync with actual model error types and reduce false positives/negatives as providers evolve. - The
hasPermissionDeniedToolCalllogic relies on specific DB shapes and string formats (e.g.summary LIKE '%: skill_denied'); it might be more robust to key off explicit status/error fields or structured JSON instead of free-form summary text to avoid breaking if summary formatting changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `MODEL_ERROR_PATTERNS` heuristic in `run-failure-message.ts` is string-based and hardcoded; consider centralizing or making it configurable so it’s easier to keep in sync with actual model error types and reduce false positives/negatives as providers evolve.
- The `hasPermissionDeniedToolCall` logic relies on specific DB shapes and string formats (e.g. `summary LIKE '%: skill_denied'`); it might be more robust to key off explicit status/error fields or structured JSON instead of free-form summary text to avoid breaking if summary formatting changes.
## Individual Comments
### Comment 1
<location path="src/core/model/tool-result-formatter.ts" line_range="5-14" />
<code_context>
+
+const PERMISSION_DENIED_STATUSES = new Set<string>(["permission_denied"]);
+
+function isToolResult(value: unknown): value is ToolResult {
+ if (value === null || typeof value !== "object") {
+ return false;
+ }
+
+ const candidate = value as Partial<ToolResult> & Record<string, unknown>;
+
+ if (typeof candidate.status !== "string") {
+ return false;
+ }
+
+ if (!PERMISSION_DENIED_STATUSES.has(candidate.status)) {
+ return false;
+ }
+
+ // If present, permission_denied error payloads must be real objects.
+ const error = candidate.error;
+ if (error !== undefined && (error === null || typeof error !== "object")) {
+ return false;
+ }
+
+ return true;
+}
+
</code_context>
<issue_to_address>
**suggestion:** The `isToolResult` guard only recognizes `permission_denied` results, which is narrower than the function name suggests.
Because it only matches `permission_denied`, this guard is more specific than the name implies and could be misused as a general `ToolResult` check. Consider renaming it (e.g. `isPermissionDeniedToolResult`) or broadening the guard to validate the full `ToolResult` shape and checking `status` separately to better reflect its intent and avoid misuse.
Suggested implementation:
```typescript
const PERMISSION_DENIED_STATUSES = new Set<string>(["permission_denied"]);
```
```typescript
function isPermissionDeniedToolResult(value: unknown): value is ToolResult {
```
You will need to:
1. Replace all usages of `isToolResult` in the codebase that rely on this specific `permission_denied` check with `isPermissionDeniedToolResult`.
2. If there are any places where `isToolResult` was intended as a general `ToolResult` shape guard, consider implementing a new, broader `isToolResult` that validates the full `ToolResult` structure and using it there instead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@gemini-code-assist review |
|
@sourcery-ai review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust mechanism for handling and formatting tool execution failures, particularly permission-denied errors, and classifying model provider errors to present clearer user-facing failure messages. Key changes include the addition of a model error classifier, a tool result formatter that provides structured guidance to the model on permission denials, and a helper to generate user-facing failure messages. The review feedback suggests improving robustness by handling string-based error payloads in the formatter, expanding the model provider error patterns to include common gateway status codes and phrases, and adding corresponding unit tests.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // If present, permission_denied error payloads must be real objects. | ||
| const error = candidate.error; | ||
| if (error !== undefined && (error === null || typeof error !== "object")) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The current check strictly requires error to be an object if present. However, some tools (especially external or MCP tools) might return a string error or a non-standard error format. If error is a string, this check returns false, causing the formatter to fall back to raw JSON.stringify and completely skipping the helpful model guidance. We should relax this check to allow string errors or any other format, ensuring the model always receives the structured permission guidance.
| // If present, permission_denied error payloads must be real objects. | |
| const error = candidate.error; | |
| if (error !== undefined && (error === null || typeof error !== "object")) { | |
| return false; | |
| } | |
| // Accept any error payload type (object, string, etc.) to ensure robustness. | |
| function formatPermissionDeniedForModel(result: ToolResult, toolName?: string): string { | ||
| const error = result.error; | ||
| const reason = error?.message ?? "Permission denied"; | ||
| const code = error?.code ? ` (code: ${error.code})` : ""; |
There was a problem hiding this comment.
To support string-based error payloads robustly, we should update formatPermissionDeniedForModel to handle cases where error is a string rather than an object. This prevents TypeScript type issues and extracts the string directly as the reason.
| function formatPermissionDeniedForModel(result: ToolResult, toolName?: string): string { | |
| const error = result.error; | |
| const reason = error?.message ?? "Permission denied"; | |
| const code = error?.code ? ` (code: ${error.code})` : ""; | |
| function formatPermissionDeniedForModel(result: ToolResult, toolName?: string): string { | |
| const error = (result as any).error; | |
| const reason = typeof error === "string" ? error : (error?.message ?? "Permission denied"); | |
| const code = (error && typeof error === "object" && "code" in error && error.code) ? " (code: " + error.code + ")" : ""; |
| const MODEL_PROVIDER_ERROR_PATTERNS = [ | ||
| "openai_error", | ||
| "openai-compatible api error", | ||
| "openai api error", | ||
| "gemini_error", | ||
| "gemini api error", | ||
| "anthropic", | ||
| "claude", | ||
| "model_error", | ||
| "api_error", | ||
| "rate limit", | ||
| "quota exceeded", | ||
| "context length", | ||
| "token limit exceeded", | ||
| "timeout", | ||
| "429" | ||
| ] as const; |
There was a problem hiding this comment.
Consider adding common HTTP error status codes and gateway error phrases (such as "502", "503", "504", "bad gateway", "service unavailable") to MODEL_PROVIDER_ERROR_PATTERNS. If a model provider is temporarily down or unreachable, it often returns these status codes/phrases. Including them ensures that permission errors are still prioritized over these transient network/gateway errors.
const MODEL_PROVIDER_ERROR_PATTERNS = [
"openai_error",
"openai-compatible api error",
"openai api error",
"gemini_error",
"gemini api error",
"anthropic",
"claude",
"model_error",
"api_error",
"rate limit",
"quota exceeded",
"context length",
"token limit exceeded",
"timeout",
"429",
"502",
"503",
"504",
"bad gateway",
"service unavailable"
] as const;| expect(output).toContain("Permission denied"); | ||
| expect(output).toContain("Reason: Permission denied"); | ||
| expect(output).toContain("If this tool or permission is necessary"); | ||
| }); |
There was a problem hiding this comment.
Add a unit test to verify that formatToolResultForModel gracefully and robustly handles permission_denied results where the error field is a string instead of an object.
});
it("handles permission_denied with a string error gracefully", () => {
const result = {
status: "permission_denied" as const,
error: "User rejected the request" as any
};
const output = formatToolResultForModel(result);
expect(output).toContain("Permission denied");
expect(output).toContain("Reason: User rejected the request");
});
There was a problem hiding this comment.
Hey - 我在这里给了一些高层次的反馈：
- 面向用户的最终错误字符串的拼接逻辑目前分散在
runAgentForMessageInternal和getUserFacingFailureMessage中（两者都会在前面加上Run failed:）；建议把这部分职责集中到一个地方，以避免前缀重复以及随着时间推移出现消息不一致的问题。 hasPermissionDeniedToolCall对表/列名和值以及状态字符串进行了硬编码（例如'permission_denied'、'skill_tool_not_allowed'、kind = 'tool_completed'、summary LIKE '%: skill_denied'）；为了在这些值变更时不出现偏差，最好复用共享的常量/枚举或统一的辅助函数，让实现更健壮。- 针对
run_steps使用summary LIKE '%: skill_denied'/'%: permission_denied'这种模式来匹配，会把行为绑定到特定的 summary 格式约定上；如果可能，建议改用结构化字段（例如状态码或类型）来检测权限失败，而不是解析 summary 字符串。
AI Agents 的提示词
Please address the comments from this code review:
## Overall Comments
- The logic for composing the final user-facing error string is split between `runAgentForMessageInternal` and `getUserFacingFailureMessage` (both prepending `Run failed:`); consider centralizing that responsibility in one place to avoid prefix duplication and inconsistent messages over time.
- `hasPermissionDeniedToolCall` hardcodes table/column values and status strings (e.g. `'permission_denied'`, `'skill_tool_not_allowed'`, `kind = 'tool_completed'`, `summary LIKE '%: skill_denied'`); it would be more robust to reuse shared constants/enums or a single helper to avoid drift if those values change.
- The `summary LIKE '%: skill_denied'` / `'%: permission_denied'` pattern matching on `run_steps` ties behavior to a specific summary formatting convention; if possible, consider using a structured field (e.g. status code or type) to detect permission failures instead of parsing the summary string.帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进评审质量。
Original comment in English
Hey - I've left some high level feedback:
- The logic for composing the final user-facing error string is split between
runAgentForMessageInternalandgetUserFacingFailureMessage(both prependingRun failed:); consider centralizing that responsibility in one place to avoid prefix duplication and inconsistent messages over time. hasPermissionDeniedToolCallhardcodes table/column values and status strings (e.g.'permission_denied','skill_tool_not_allowed',kind = 'tool_completed',summary LIKE '%: skill_denied'); it would be more robust to reuse shared constants/enums or a single helper to avoid drift if those values change.- The
summary LIKE '%: skill_denied'/'%: permission_denied'pattern matching onrun_stepsties behavior to a specific summary formatting convention; if possible, consider using a structured field (e.g. status code or type) to detect permission failures instead of parsing the summary string.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The logic for composing the final user-facing error string is split between `runAgentForMessageInternal` and `getUserFacingFailureMessage` (both prepending `Run failed:`); consider centralizing that responsibility in one place to avoid prefix duplication and inconsistent messages over time.
- `hasPermissionDeniedToolCall` hardcodes table/column values and status strings (e.g. `'permission_denied'`, `'skill_tool_not_allowed'`, `kind = 'tool_completed'`, `summary LIKE '%: skill_denied'`); it would be more robust to reuse shared constants/enums or a single helper to avoid drift if those values change.
- The `summary LIKE '%: skill_denied'` / `'%: permission_denied'` pattern matching on `run_steps` ties behavior to a specific summary formatting convention; if possible, consider using a structured field (e.g. status code or type) to detect permission failures instead of parsing the summary string.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1 participant
Summary
Improve how
permission_deniedtool results are presented to the model and how run failures are reported to users.Changes
New module
src/core/model/tool-result-formatter.tsstatus: "permission_denied", the model now receives a clear, structured message with actionable guidance:New module
src/core/run-failure-message.tsopenai_error,gemini, etc.) after apermission_deniedoccurred, the final user-facing message now prioritizes a clear English permission error instead of leaking the raw model error:Run failed: openai_errorRun failed: insufficient permissions for required toolsUpdated
agent-tool-executor.tsandagent-runner.tsto use the new modules.Added dedicated unit test for the formatter.
Why
Previously,
permission_deniedresults were just rawJSON.stringify, which the model sometimes ignored. Worse, if a later model call failed, the outer catch would report the model error (e.g.openai_error), completely hiding the real permission problem from the user.This change ensures:
Testing
This is a targeted, low-risk improvement that directly addresses recent production incidents where permission problems were masked.
由 Sourcery 提供的摘要
改进对“权限被拒绝”的工具调用在模型和终端用户侧的呈现方式，使权限问题能够被清晰识别，而不是被泛化为通用的模型错误。
错误修复：
permission_denied工具调用的运行能够报告清晰的、与权限相关的失败信息，而不是通用的模型或服务提供方错误。功能增强：
permission_denied工具结果转换为面向模型的结构化、具指导性的消息，同时对其他结果保持类 JSON 的格式。permission_denied很可能是根本原因时，该方法会优先突出权限问题。测试：
permission_denied格式化行为。Original summary in English
Summary by Sourcery
改进与权限相关的工具失败的处理与呈现方式，使模型和终端用户能够获得更清晰、更可操作的错误信息。
新特性：
permission_denied工具输出转换为结构化、包含丰富指导信息的消息供模型使用，同时对其他结果保持类似 JSON 的格式。缺陷修复：
permission_denied工具调用时，报告明确的与权限相关的失败消息，而不是泛泛的模型或提供方错误。增强改进：
测试：
permission_denied格式化行为。Original summary in English
Summary by Sourcery
Improve handling and surfacing of permission-related tool failures so models and end users receive clearer, more actionable error information.
New Features:
Bug Fixes:
Enhancements:
Tests:
Bug 修复：
permission_denied工具调用时，向用户暴露明确的权限相关失败信息，而不是笼统的模型/服务错误。增强功能：
permission_denied工具结果转换为结构化的、对模型具有指导性的消息，同时对其他结果保持类似 JSON 的输出格式。测试：
permission_denied格式化行为。Original summary in English
由 Sourcery 提供的摘要
改进对“权限被拒绝”的工具调用在模型和终端用户侧的呈现方式，使权限问题能够被清晰识别，而不是被泛化为通用的模型错误。
错误修复：
permission_denied工具调用的运行能够报告清晰的、与权限相关的失败信息，而不是通用的模型或服务提供方错误。功能增强：
permission_denied工具结果转换为面向模型的结构化、具指导性的消息，同时对其他结果保持类 JSON 的格式。permission_denied很可能是根本原因时，该方法会优先突出权限问题。测试：
permission_denied格式化行为。Original summary in English
Summary by Sourcery
改进与权限相关的工具失败的处理与呈现方式，使模型和终端用户能够获得更清晰、更可操作的错误信息。
新特性：
permission_denied工具输出转换为结构化、包含丰富指导信息的消息供模型使用，同时对其他结果保持类似 JSON 的格式。缺陷修复：
permission_denied工具调用时，报告明确的与权限相关的失败消息，而不是泛泛的模型或提供方错误。增强改进：
测试：
permission_denied格式化行为。Original summary in English
Summary by Sourcery
Improve handling and surfacing of permission-related tool failures so models and end users receive clearer, more actionable error information.
New Features:
Bug Fixes:
Enhancements:
Tests:
Bug 修复：
permission_denied工具调用时，向用户报告清晰的权限相关失败信息，而不是笼统的模型/服务提供方错误。增强功能：
permission_denied工具结果转换为结构化且包含指导信息的消息供模型使用，同时对其他结果仍保持类 JSON 的输出格式。测试：
permission_denied格式化行为。Original summary in English
由 Sourcery 提供的摘要
改进对“权限被拒绝”的工具调用在模型和终端用户侧的呈现方式，使权限问题能够被清晰识别，而不是被泛化为通用的模型错误。
错误修复：
permission_denied工具调用的运行能够报告清晰的、与权限相关的失败信息，而不是通用的模型或服务提供方错误。功能增强：
permission_denied工具结果转换为面向模型的结构化、具指导性的消息，同时对其他结果保持类 JSON 的格式。permission_denied很可能是根本原因时，该方法会优先突出权限问题。测试：
permission_denied格式化行为。Original summary in English
Summary by Sourcery
改进与权限相关的工具失败的处理与呈现方式，使模型和终端用户能够获得更清晰、更可操作的错误信息。
新特性：
permission_denied工具输出转换为结构化、包含丰富指导信息的消息供模型使用，同时对其他结果保持类似 JSON 的格式。缺陷修复：
permission_denied工具调用时，报告明确的与权限相关的失败消息，而不是泛泛的模型或提供方错误。增强改进：
测试：
permission_denied格式化行为。Original summary in English
Summary by Sourcery
Improve handling and surfacing of permission-related tool failures so models and end users receive clearer, more actionable error information.
New Features:
Bug Fixes:
Enhancements:
Tests:
Bug 修复：
permission_denied工具调用时，向用户暴露明确的权限相关失败信息，而不是笼统的模型/服务错误。增强功能：
permission_denied工具结果转换为结构化的、对模型具有指导性的消息，同时对其他结果保持类似 JSON 的输出格式。测试：
permission_denied格式化行为。Original summary in English
由 Sourcery 提供的摘要
改进对“权限被拒绝”的工具调用在模型和终端用户侧的呈现方式，使权限问题能够被清晰识别，而不是被泛化为通用的模型错误。
错误修复：
permission_denied工具调用的运行能够报告清晰的、与权限相关的失败信息，而不是通用的模型或服务提供方错误。功能增强：
permission_denied工具结果转换为面向模型的结构化、具指导性的消息，同时对其他结果保持类 JSON 的格式。permission_denied很可能是根本原因时，该方法会优先突出权限问题。测试：
permission_denied格式化行为。Original summary in English
Summary by Sourcery
改进与权限相关的工具失败的处理与呈现方式，使模型和终端用户能够获得更清晰、更可操作的错误信息。
新特性：
permission_denied工具输出转换为结构化、包含丰富指导信息的消息供模型使用，同时对其他结果保持类似 JSON 的格式。缺陷修复：
permission_denied工具调用时，报告明确的与权限相关的失败消息，而不是泛泛的模型或提供方错误。增强改进：
测试：
permission_denied格式化行为。Original summary in English
Summary by Sourcery
Improve handling and surfacing of permission-related tool failures so models and end users receive clearer, more actionable error information.
New Features:
Bug Fixes:
Enhancements:
Tests: