Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 4 additions & 2 deletions actions/setup/js/mcp_enhanced_errors.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@

// SEC-004: No sanitize needed - "body" is only used as example text

const { ERR_VALIDATION } = require("./error_codes.cjs");

/**
* Generate an enhanced error message with actionable guidance for missing parameters
* @param {string[]} missingFields - Array of missing field names
Expand All @@ -21,12 +23,12 @@
*/
function generateEnhancedErrorMessage(missingFields, toolName, inputSchema) {
if (!missingFields || missingFields.length === 0) {
return "Invalid arguments";
return `${ERR_VALIDATION}: Invalid arguments`;
}

// Base error message
const fieldsList = missingFields.map(m => `'${m}'`).join(", ");
let message = `Invalid arguments: missing or empty ${fieldsList}\n`;
let message = `${ERR_VALIDATION}: Invalid arguments: missing or empty ${fieldsList}\n`;

// Add guidance for each missing field
if (inputSchema && inputSchema.properties) {
Expand Down
12 changes: 6 additions & 6 deletions actions/setup/js/mcp_enhanced_errors.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ describe("mcp_enhanced_errors.cjs", () => {
};
const result = generateEnhancedErrorMessage(["item_number"], "add_comment", schema);

expect(result).toContain("Invalid arguments: missing or empty 'item_number'");
expect(result).toContain("ERR_VALIDATION: Invalid arguments: missing or empty 'item_number'");
expect(result).toContain("Required parameter 'item_number': The issue, pull request, or discussion number to comment on.");
expect(result).toContain("Example:");
expect(result).toContain('"item_number": 123');
Expand All @@ -136,7 +136,7 @@ describe("mcp_enhanced_errors.cjs", () => {
};
const result = generateEnhancedErrorMessage(["title", "body"], "create_issue", schema);

expect(result).toContain("Invalid arguments: missing or empty 'title', 'body'");
expect(result).toContain("ERR_VALIDATION: Invalid arguments: missing or empty 'title', 'body'");
expect(result).toContain("Required parameter 'title': Issue title.");
expect(result).toContain("Required parameter 'body': Issue body.");
expect(result).toContain("Example:");
Expand All @@ -152,25 +152,25 @@ describe("mcp_enhanced_errors.cjs", () => {
};
const result = generateEnhancedErrorMessage(["field1"], "test_tool", schema);

expect(result).toContain("Invalid arguments: missing or empty 'field1'");
expect(result).toContain("ERR_VALIDATION: Invalid arguments: missing or empty 'field1'");
expect(result).toContain("Example:");
});

it("should handle empty missing fields array", () => {
const result = generateEnhancedErrorMessage([], "test_tool", {});
expect(result).toBe("Invalid arguments");
expect(result).toBe("ERR_VALIDATION: Invalid arguments");
});

it("should handle null missing fields", () => {
const result = generateEnhancedErrorMessage(null, "test_tool", {});
expect(result).toBe("Invalid arguments");
expect(result).toBe("ERR_VALIDATION: Invalid arguments");
});

it("should handle schema without properties", () => {
const schema = { type: "object", required: ["field1"] };
const result = generateEnhancedErrorMessage(["field1"], "test_tool", schema);

expect(result).toContain("Invalid arguments: missing or empty 'field1'");
expect(result).toContain("ERR_VALIDATION: Invalid arguments: missing or empty 'field1'");
expect(result).toContain("Example:");
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ describe("require-error-code-in-thrown-error", () => {
valid: [
`const { ERR_CONFIG } = require("./error_codes.cjs"); function f(errorMessage) { throw new Error(errorMessage); }`,
`const { ERR_CONFIG } = require("./error_codes.cjs"); function f(e) { let msg = "boom"; msg = String(e); throw new Error(msg); }`,
`const { ERR_CONFIG } = require("./error_codes.cjs"); function f(e) { const errorMessage = getErrorMessage(e); if (errorMessage.startsWith(\`\${ERR_CONFIG}:\`)) { throw new Error(errorMessage); } }`,
],
invalid: [],
});
Expand Down Expand Up @@ -128,4 +127,36 @@ describe("require-error-code-in-thrown-error", () => {
],
});
});

it("handles helper call message arguments explicitly", () => {
cjsRuleTester.run("require-error-code-in-thrown-error", requireErrorCodeInThrownErrorRule, {
valid: [
`const { ERR_API } = require("./error_codes.cjs"); function helper() { return "ERR_API: failed to fetch"; } function f() { throw new Error(helper()); }`,
`const { ERR_CONFIG } = require("./error_codes.cjs"); const helper = () => ERR_CONFIG + ": invalid config"; function f() { throw new Error(helper()); }`,
`const { ERR_API } = require("./error_codes.cjs"); function helper() { const message = ERR_API + ": failed to fetch"; return message; } function f() { throw new Error(helper()); }`,
],
invalid: [
{
code: `const { ERR_API } = require("./error_codes.cjs"); function helper() { return "failed to fetch"; } function f() { throw new Error(helper()); }`,
errors: [{ messageId: "missingErrorCode" }],
},
{
code: `const { ERR_API } = require("./error_codes.cjs"); function helper(reason) { return reason; } function f(reason) { throw new Error(helper(reason)); }`,
errors: [{ messageId: "callExpressionNeedsReview" }],
},
{
code: `const { ERR_API } = require("./error_codes.cjs"); const { helper } = require("./helper.cjs"); function f() { throw new Error(helper()); }`,
errors: [{ messageId: "callExpressionNeedsReview" }],
},
{
code: `const { ERR_API } = require("./error_codes.cjs"); function f() { const message = helper(); throw new Error(message); }`,
errors: [{ messageId: "callExpressionNeedsReview" }],
},
{
code: `const { ERR_API } = require("./error_codes.cjs"); function helper(useFallback) { if (useFallback) return "fallback"; return ERR_API + ": failed"; } function f(useFallback) { throw new Error(helper(useFallback)); }`,
errors: [{ messageId: "callExpressionNeedsReview" }],
},
],
});
});
});
79 changes: 62 additions & 17 deletions eslint-factory/src/rules/require-error-code-in-thrown-error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ export const requireErrorCodeInThrownErrorRule = createRule({
messages: {
missingErrorCode:
"This file imports error_codes.cjs but this thrown Error message does not reference a standardized error code (e.g. ERR_API, ERR_NOT_FOUND). Prefix the message with an imported ERR_* constant for consistency with other errors in this file.",
callExpressionNeedsReview:
"This file imports error_codes.cjs but this thrown Error message comes from a helper call that cannot be statically verified to reference an error code. Add a visible ERR_* prefix at the throw site or review the helper and suppress this warning intentionally.",
},
},
defaultOptions: [],
Expand All @@ -67,6 +69,62 @@ export const requireErrorCodeInThrownErrorRule = createRule({
return null;
}

type MessageAuditResult = "hasCode" | "missingCode" | "needsReview";

function auditMessageExpression(node: TSESTree.Node, unresolvedIdentifierResult: MessageAuditResult = "hasCode"): MessageAuditResult {
if (messageReferencesErrorCode(node)) return "hasCode";
if (node.type === AST_NODE_TYPES.CallExpression) {
const returns = resolveSimpleLocalCallReturns(node);
if (!returns) return "needsReview";
for (const returnExpr of returns) {
const returnResult = auditMessageExpression(returnExpr, "needsReview");
if (returnResult !== "hasCode") return returnResult;
}
return "hasCode";
}
if (node.type === AST_NODE_TYPES.Identifier) {
const resolved = resolveWriteOnceInitializerChain(node, sourceCode);
if (resolved === node) return unresolvedIdentifierResult;
if (!isAuditableMessageExpression(resolved)) return resolved.type === AST_NODE_TYPES.CallExpression ? "needsReview" : "hasCode";
return messageReferencesErrorCode(resolved) ? "hasCode" : "missingCode";
}
return "missingCode";
Comment on lines +88 to +91
}

function isAuditableMessageExpression(node: TSESTree.Node): boolean {
return node.type === AST_NODE_TYPES.TemplateLiteral || node.type === AST_NODE_TYPES.Literal || node.type === AST_NODE_TYPES.Identifier || (node.type === AST_NODE_TYPES.BinaryExpression && node.operator === "+");
}

function resolveSimpleLocalCallReturns(node: TSESTree.CallExpression): TSESTree.Expression[] | null {
if (node.callee.type !== AST_NODE_TYPES.Identifier) return null;
const variable = findVariableInScopeChain(sourceCode.getScope(node.callee), node.callee.name);
if (!variable || variable.defs.length !== 1) return null;
const def = variable.defs[0];

let body: TSESTree.BlockStatement | TSESTree.Expression | null = null;
if (def.type === "FunctionName") {
body = (def.node as TSESTree.FunctionDeclaration).body;
} else if (def.type === "Variable" && def.node.init && (def.node.init.type === AST_NODE_TYPES.FunctionExpression || def.node.init.type === AST_NODE_TYPES.ArrowFunctionExpression)) {
body = def.node.init.body;
} else {
return null;
}
Comment on lines +104 to +111

if (!body) return null;
if (body.type !== AST_NODE_TYPES.BlockStatement) return [body];
let returnIndex = -1;
for (let index = 0; index < body.body.length; index++) {
if (body.body[index].type !== AST_NODE_TYPES.ReturnStatement) continue;
if (returnIndex !== -1) return null;
returnIndex = index;
}
if (returnIndex === -1) return null;
if (body.body.slice(0, returnIndex).some(statement => statement.type !== AST_NODE_TYPES.VariableDeclaration)) return null;
if (returnIndex + 1 < body.body.length) return null;
const statement = body.body[returnIndex];
return statement.type === AST_NODE_TYPES.ReturnStatement && statement.argument ? [statement.argument] : null;
}

function isErrorConstructorViaScope(callee: TSESTree.Identifier): boolean {
const visited = new Set<object>();

Expand Down Expand Up @@ -102,29 +160,16 @@ export const requireErrorCodeInThrownErrorRule = createRule({
if (callee.type !== AST_NODE_TYPES.Identifier || !isErrorConstructorViaScope(callee)) return;
const messageArg = arg.arguments[0];
if (!messageArg) return;
if (messageArg.type !== AST_NODE_TYPES.TemplateLiteral && messageArg.type !== AST_NODE_TYPES.Literal && messageArg.type !== AST_NODE_TYPES.Identifier && messageArg.type !== AST_NODE_TYPES.BinaryExpression) {
if (!isAuditableMessageExpression(messageArg) && messageArg.type !== AST_NODE_TYPES.CallExpression) {
return;
}

if (messageReferencesErrorCode(messageArg)) return;

if (messageArg.type === AST_NODE_TYPES.Identifier) {
// Resolve write-once local initializers so a message built from an
// ERR_* constant is recognized even when it is held in a plain-named
// variable (e.g. `const errorMsg = `${ERR_SYSTEM}: ...`;`).
const resolved = resolveWriteOnceInitializerChain(messageArg, sourceCode);
// Unresolvable values (parameters, reassigned bindings, call results,
// values from a guarded branch) stay silent: false positives are worse
// than silence for this consistency rule.
if (resolved.type !== AST_NODE_TYPES.TemplateLiteral && resolved.type !== AST_NODE_TYPES.Literal && (resolved.type !== AST_NODE_TYPES.BinaryExpression || resolved.operator !== "+")) {
return;
}
if (messageReferencesErrorCode(resolved)) return;
}
const auditResult = auditMessageExpression(messageArg);
if (auditResult === "hasCode") return;

context.report({
node: arg,
messageId: "missingErrorCode",
messageId: auditResult === "needsReview" ? "callExpressionNeedsReview" : "missingErrorCode",
});
},
};
Expand Down