Fix/use harness process for rule generation#11
Conversation
![mesa-dev-development[bot]](https://avatars.githubusercontent.com/in/629974?s=60&v=4){kind=small}
There was a problem hiding this comment.
Performed full review of 7effb24...8c33e33
Analysis
• Unsafe optional schema fields break existing code: Making examples and tags optional in RuleProposalSchema without updating all consumers (rules.ts, synthesis.ts, hook-runner.ts) will cause runtime errors when these fields are missing. Direct property access like rule.examples.violations lacks null checks.
• DEFAULT_MAX_TURNS tripled globally without justification: Increasing from 10 to 30 turns affects all agent operations system-wide, not just rule generation. No justification provided for why rule generation needs 3x turns—this suggests either a prompting issue or needs to be scoped per-operation rather than a global constant change.
• Silent backend fallback without validation: The backend resolution falls back from CLI to SDK without verifying an API key is configured, deferring failures deep into execution instead of failing fast with actionable errors at startup.
• No test coverage for critical abstraction layer: The new llm-backend.ts contains complex logic for JSON extraction, backend resolution, and error handling but lacks unit tests despite being a core abstraction.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
10 files reviewed | 3 comments | Edit Agent Settings • Read Docs
| const FIVE_MINUTES_MS = 5 * 60 * 1000; | ||
| const TEN_MB = 10 * 1024 * 1024; | ||
| const DEFAULT_MAX_TURNS = 10; | ||
| const DEFAULT_MAX_TURNS = 30; |
There was a problem hiding this comment.
Increasing DEFAULT_MAX_TURNS from 10 to 30 (3x) has significant implications. This affects ALL agent operations system-wide, not just rule generation. Each turn involves an LLM API call, so this change could triple execution time and API costs for operations that hit the limit. Without justification in the PR description or code comments, this appears to be a global change to solve a specific problem. Consider: (1) making maxTurns configurable per operation type, (2) documenting why 30 turns is necessary, or (3) investigating if the need for many turns indicates a prompting/architecture issue that should be fixed differently.
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: mesa-dot-dev/saguaro#11
File: src/ai/agent-runner.ts#L6
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Increasing DEFAULT_MAX_TURNS from 10 to 30 (3x) has significant implications. This affects ALL agent operations system-wide, not just rule generation. Each turn involves an LLM API call, so this change could triple execution time and API costs for operations that hit the limit. Without justification in the PR description or code comments, this appears to be a global change to solve a specific problem. Consider: (1) making maxTurns configurable per operation type, (2) documenting why 30 turns is necessary, or (3) investigating if the need for many turns indicates a prompting/architecture issue that should be fixed differently.
| } | ||
|
|
||
| // Fallback: SDK (requires API key) | ||
| logger.debug(`[generator] CLI ${cliCommand} not available, falling back to SDK`); |
There was a problem hiding this comment.
Silent fallback from CLI to SDK could fail unexpectedly if no API key is configured. The code assumes resolveApiKey(config) will succeed on line 174, but if the API key is missing, the tool will fail deep in execution rather than at startup with a clear error message. Recommendation: Add validation before fallback: const apiKey = resolveApiKey(config); if (!apiKey) { throw new Error('${cliCommand} CLI not found and no API key configured. Install CLI or set API key.'); } This provides users with clear, actionable feedback.
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: mesa-dot-dev/saguaro#11
File: src/generator/llm-backend.ts#L173
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Silent fallback from CLI to SDK could fail unexpectedly if no API key is configured. The code assumes `resolveApiKey(config)` will succeed on line 174, but if the API key is missing, the tool will fail deep in execution rather than at startup with a clear error message. Recommendation: Add validation before fallback: `const apiKey = resolveApiKey(config); if (!apiKey) { throw new Error('${cliCommand} CLI not found and no API key configured. Install CLI or set API key.'); }` This provides users with clear, actionable feedback.
| violations: z.array(z.string()).describe('Code snippets (10-120 chars) showing violations'), | ||
| compliant: z.array(z.string()).describe('Code snippets (10-120 chars) showing correct code'), | ||
| }) | ||
| .optional() |
There was a problem hiding this comment.
Making examples optional creates runtime errors in consuming code. Found multiple locations that directly access rule.examples.violations and rule.examples.compliant without null checks: src/cli/lib/rules.ts lines 83, 87, 250; src/generator/synthesis.ts lines 123, 126; src/adapter/hook-runner.ts lines 190, 199. These will throw 'Cannot read property of undefined' errors when examples is missing. While some code has proper checks (src/ai/prompt.ts, src/rules/saguaro-rules.ts), inconsistent handling makes this a high-severity issue. Either: (1) revert to required and ensure LLM always generates examples, or (2) audit all consuming code to add null checks, or (3) use .default({ violations: [], compliant: [] }) in the schema.
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: mesa-dot-dev/saguaro#11
File: src/generator/schemas.ts#L14
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Making `examples` optional creates runtime errors in consuming code. Found multiple locations that directly access `rule.examples.violations` and `rule.examples.compliant` without null checks: src/cli/lib/rules.ts lines 83, 87, 250; src/generator/synthesis.ts lines 123, 126; src/adapter/hook-runner.ts lines 190, 199. These will throw 'Cannot read property of undefined' errors when examples is missing. While some code has proper checks (src/ai/prompt.ts, src/rules/saguaro-rules.ts), inconsistent handling makes this a high-severity issue. Either: (1) revert to required and ensure LLM always generates examples, or (2) audit all consuming code to add null checks, or (3) use `.default({ violations: [], compliant: [] })` in the schema.
1 participant
No description provided.