feat(error-rules): record matched rule details in decision chain (#416)#418
feat(error-rules): record matched rule details in decision chain (#416)#418
Conversation
- Extend ErrorDetectionResult with ruleId and description fields - Add matchedRule field to ProviderChainItem.errorDetails - Record matched error rule info in client_error_non_retryable branch - Add client_error_non_retryable formatting in provider-chain-formatter - Add getProviderStatus and isActualRequest handling for new reason type - Add i18n translations for all 5 locales Closes #416 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @ding113, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the visibility and debuggability of error handling, particularly for client-side non-retryable errors. By capturing and exposing detailed information about the specific error rule that was matched, developers can more quickly understand the cause and context of such errors. This enhancement is integrated into the existing provider chain logging and display mechanisms, supported by new internationalization strings for a consistent user experience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
bot
added
enhancement
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![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 successfully adds detailed error rule matching information to the decision chain for non-retryable client errors. This is a great enhancement for debugging and observability. The changes are well-structured, touching the error detection, forwarding logic, data types, and UI formatting. The inclusion of i18n translations is also thorough. My review includes a couple of suggestions to improve code clarity and maintainability.
| const matchedRule = | ||
| detectionResult.matched && | ||
| detectionResult.ruleId !== undefined && | ||
| detectionResult.pattern && | ||
| detectionResult.matchType && | ||
| detectionResult.category | ||
| ? { | ||
| ruleId: detectionResult.ruleId, | ||
| pattern: detectionResult.pattern, | ||
| matchType: detectionResult.matchType, | ||
| category: detectionResult.category, | ||
| description: detectionResult.description, | ||
| hasOverrideResponse: detectionResult.overrideResponse !== undefined, | ||
| hasOverrideStatusCode: detectionResult.overrideStatusCode !== undefined, | ||
| } | ||
| : undefined; |
There was a problem hiding this comment.
The condition to check for a valid detectionResult is a bit verbose and can be fragile if a valid pattern is an empty string (which is falsy). The implementation of detect in error-rule-detector.ts ensures that if matched is true, ruleId, pattern, matchType, and category are always defined (not undefined or null).
A more robust check would explicitly check for undefined instead of relying on truthiness, which also improves readability.
| const matchedRule = | |
| detectionResult.matched && | |
| detectionResult.ruleId !== undefined && | |
| detectionResult.pattern && | |
| detectionResult.matchType && | |
| detectionResult.category | |
| ? { | |
| ruleId: detectionResult.ruleId, | |
| pattern: detectionResult.pattern, | |
| matchType: detectionResult.matchType, | |
| category: detectionResult.category, | |
| description: detectionResult.description, | |
| hasOverrideResponse: detectionResult.overrideResponse !== undefined, | |
| hasOverrideStatusCode: detectionResult.overrideStatusCode !== undefined, | |
| } | |
| : undefined; | |
| const matchedRule = | |
| detectionResult.matched && | |
| detectionResult.ruleId !== undefined && | |
| detectionResult.pattern !== undefined && | |
| detectionResult.matchType !== undefined && | |
| detectionResult.category !== undefined | |
| ? { | |
| ruleId: detectionResult.ruleId, | |
| pattern: detectionResult.pattern, | |
| matchType: detectionResult.matchType, | |
| category: detectionResult.category, | |
| description: detectionResult.description, | |
| hasOverrideResponse: detectionResult.overrideResponse !== undefined, | |
| hasOverrideStatusCode: detectionResult.overrideStatusCode !== undefined, | |
| } | |
| : undefined; |
| if (item.reason === "client_error_non_retryable") { | ||
| const attempt = item.attemptNumber ?? actualAttemptNumber ?? 0; | ||
| timeline += `${t("timeline.clientErrorNonRetryable", { attempt })}\n\n`; | ||
|
|
||
| if (item.errorDetails?.provider) { | ||
| const p = item.errorDetails.provider; | ||
| timeline += `${t("timeline.provider", { provider: p.name })}\n`; | ||
| timeline += `${t("timeline.statusCode", { code: p.statusCode })}\n`; | ||
| timeline += `${t("timeline.error", { error: p.statusText })}\n`; | ||
| } else { | ||
| timeline += `${t("timeline.provider", { provider: item.name })}\n`; | ||
| if (item.statusCode) { | ||
| timeline += `${t("timeline.statusCode", { code: item.statusCode })}\n`; | ||
| } | ||
| timeline += `${t("timeline.error", { error: item.errorMessage || t("timeline.unknown") })}\n`; | ||
| } | ||
|
|
||
| if (item.errorDetails?.matchedRule) { | ||
| const rule = item.errorDetails.matchedRule; | ||
| timeline += `\n${t("timeline.matchedRule")}:\n`; | ||
| timeline += `${t("timeline.ruleId", { id: rule.ruleId })}\n`; | ||
| timeline += `${t("timeline.ruleCategory", { category: rule.category })}\n`; | ||
| timeline += `${t("timeline.rulePattern", { pattern: rule.pattern })}\n`; | ||
| timeline += `${t("timeline.ruleMatchType", { matchType: rule.matchType })}\n`; | ||
| if (rule.description) { | ||
| timeline += `${t("timeline.ruleDescription", { description: rule.description })}\n`; | ||
| } | ||
| timeline += `${t("timeline.ruleHasOverride", { | ||
| response: rule.hasOverrideResponse ? "true" : "false", | ||
| statusCode: rule.hasOverrideStatusCode ? "true" : "false", | ||
| })}\n`; | ||
| } | ||
|
|
||
| if (item.errorDetails?.request) { | ||
| timeline += formatRequestDetails(item.errorDetails.request, t); | ||
| } | ||
|
|
||
| timeline += `\n${t("timeline.clientErrorNote")}`; | ||
| continue; |
There was a problem hiding this comment.
To improve the readability and maintainability of the formatProviderTimeline function, consider extracting the logic for formatting client_error_non_retryable into a separate helper function. This will make the main function cleaner and easier to follow.
For example, you could create a new function:
function formatClientErrorDetails(
item: ProviderChainItem,
t: (key: string, values?: Record<string, string | number>) => string,
actualAttemptNumber: number | undefined
): string {
let details = "";
const attempt = item.attemptNumber ?? actualAttemptNumber ?? 0;
details += `${t("timeline.clientErrorNonRetryable", { attempt })}\n\n`;
if (item.errorDetails?.provider) {
const p = item.errorDetails.provider;
details += `${t("timeline.provider", { provider: p.name })}\n`;
details += `${t("timeline.statusCode", { code: p.statusCode })}\n`;
details += `${t("timeline.error", { error: p.statusText })}\n`;
} else {
details += `${t("timeline.provider", { provider: item.name })}\n`;
if (item.statusCode) {
details += `${t("timeline.statusCode", { code: item.statusCode })}\n`;
}
details += `${t("timeline.error", { error: item.errorMessage || t("timeline.unknown") })}\n`;
}
if (item.errorDetails?.matchedRule) {
const rule = item.errorDetails.matchedRule;
details += `\n${t("timeline.matchedRule")}:\n`;
details += `${t("timeline.ruleId", { id: rule.ruleId })}\n`;
details += `${t("timeline.ruleCategory", { category: rule.category })}\n`;
details += `${t("timeline.rulePattern", { pattern: rule.pattern })}\n`;
details += `${t("timeline.ruleMatchType", { matchType: rule.matchType })}\n`;
if (rule.description) {
details += `${t("timeline.ruleDescription", { description: rule.description })}\n`;
}
details += `${t("timeline.ruleHasOverride", {
response: rule.hasOverrideResponse ? "true" : "false",
statusCode: rule.hasOverrideStatusCode ? "true" : "false",
})}\n`;
}
if (item.errorDetails?.request) {
details += formatRequestDetails(item.errorDetails.request, t);
}
details += `\n${t("timeline.clientErrorNote")}`;
return details;
}Then, in formatProviderTimeline, you can simply call this function:
// === 不可重试的客户端错误 ===
if (item.reason === "client_error_non_retryable") {
timeline += formatClientErrorDetails(item, t, actualAttemptNumber);
continue;
}![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR.
PR Size: M
- Lines changed: 204
- Files changed: 10
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate
- Code clarity - Good
Automated review by Codex AI
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR. The implementation correctly extends the error detection and decision chain infrastructure to capture and display matched rule metadata for debugging purposes.
PR Size: M (Medium)
- Lines changed: 204 (188 additions + 16 deletions)
- Files changed: 10
Review Coverage
- Logic and correctness - Clean
- Properly extends
ErrorDetectionResultwithruleIdanddescriptionfields - Correctly constructs
matchedRuleobject only when all required fields are present - Properly reuses the cached detection result via
getErrorDetectionResultAsync
- Properly extends
- Security (OWASP Top 10) - Clean
- No sensitive data exposure in logged rule information
- Rule patterns are already stored in the database and visible to admins
- Error handling - Clean
- New
getErrorDetectionResultAsyncproperly delegates to existingdetectErrorRuleOnceAsync - Existing cache and error handling patterns are preserved
- New
- Type safety - Clean
- New
matchedRuletype properly defined inProviderChainItem.errorDetails - Interface extensions in
ErrorDetectionResultand pattern interfaces are consistent
- New
- Documentation accuracy - Clean
- Chinese comments appropriately explain the purpose of new fields and functions
- Test coverage - Adequate
- Changes are observability/logging focused; existing detection logic unchanged
- Code clarity - Good
- Follows existing patterns in the codebase
- Consistent with how other decision chain details are recorded
Automated review by Claude AI
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR. The implementation correctly extends the error detection and decision chain infrastructure to capture and display matched rule metadata for debugging purposes.
PR Size: M (Medium)
- Lines changed: 204 (188 additions + 16 deletions)
- Files changed: 10
Review Coverage
- Logic and correctness - Clean
- Properly extends
ErrorDetectionResultwithruleIdanddescriptionfields - Correctly constructs
matchedRuleobject only when all required fields are present - Properly reuses the cached detection result via
getErrorDetectionResultAsync
- Properly extends
- Security (OWASP Top 10) - Clean
- No sensitive data exposure in logged rule information
- Rule patterns are already stored in the database and visible to admins
- Error handling - Clean
- New
getErrorDetectionResultAsyncproperly delegates to existingdetectErrorRuleOnceAsync - Existing cache and error handling patterns are preserved
- New
- Type safety - Clean
- New
matchedRuletype properly defined inProviderChainItem.errorDetails - Interface extensions in
ErrorDetectionResultand pattern interfaces are consistent
- New
- Documentation accuracy - Clean
- Chinese comments appropriately explain the purpose of new fields and functions
- Test coverage - Adequate
- Changes are observability/logging focused; existing detection logic unchanged
- Code clarity - Good
- Follows existing patterns in the codebase
- Consistent with how other decision chain details are recorded
Automated review by Claude AI
Address PR review feedback from Gemini Code Assist: replace truthiness checks with explicit !== undefined comparisons for pattern, matchType, and category fields. This prevents potential false negatives when these fields contain empty strings. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🧪 测试结果
总体结果: ✅ 所有测试通过 |
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
Records matched error rule details (ID, pattern, category, description) in the decision chain when a non-retryable client error is triggered, enabling easier debugging and troubleshooting of error rule matches.
Problem
When an error rule matches and triggers a
client_error_non_retryableresponse, the decision chain only recorded that a match occurred, but not which specific rule was matched. This made it difficult for administrators to:Related Issues:
Solution
Extended the error detection and decision chain infrastructure to capture and display matched rule metadata:
ErrorDetectionResultwithruleIdanddescriptionfieldsmatchedRulefield toProviderChainItem.errorDetailscontaining:Changes
Core Changes
src/lib/error-rule-detector.tsruleIdandrawPattern; return these fields in detection resultssrc/app/v1/_lib/proxy/forwarder.tssrc/app/v1/_lib/proxy/errors.tsgetErrorDetectionResultAsync()for reusing cached detection resultssrc/types/message.tsmatchedRuletype definition toProviderChainItem.errorDetailsUI/Formatting Changes
src/lib/utils/provider-chain-formatter.tsclient_error_non_retryablehandling ingetProviderStatus,isActualRequest, and timeline formattingi18n Changes
clientError,clientErrorNonRetryable,matchedRule,ruleId,ruleCategory,rulePattern,ruleMatchType,ruleDescription,ruleHasOverride,clientErrorNotein all 5 localesTesting
Manual Testing
Checklist
Description enhanced by Claude AI