feat(review): Add thinking effort to code reviews#732
Open
alex-alecu wants to merge 14 commits intomainfrom
Open
feat(review): Add thinking effort to code reviews#732alex-alecu wants to merge 14 commits intomainfrom
alex-alecu wants to merge 14 commits intomainfrom
Conversation
Harden all variant/thinkingEffort Zod schemas with regex(/^[a-zA-Z]+$/) to prevent shell injection via the CLI --variant flag in cloud-agent v1. Also rename 'xhigh' display label from 'Xhigh' to 'Extra High'.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (21 files)
|
Contributor
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Other Observations (not in diff)No issues found outside the diff. Files Reviewed (22 files)
|
…Schema Defense-in-depth: add .max(50).regex(/^[a-zA-Z]+$/) to match all other variant schemas in this PR, preventing potential injection if bad data reaches the DB.
Ensures getAvailableThinkingEfforts (client-safe) stays in sync with getModelVariants (server-side) for all provider branches, catching drift at CI time.
| isFirstExecution: currentIsFirst, | ||
| kiloSessionId, | ||
| images: options?.images, | ||
| variant: options?.variant, |
Contributor
There was a problem hiding this comment.
[SUGGESTION]: Redundant explicit property — variant is already included via ...options spread
images: options?.images (line above) has the same redundancy and predates this PR, so this is a pre-existing pattern. Both are no-ops since ...options already spreads images and variant. Not a bug, but worth noting for a future cleanup pass.
Collapse two redundant if-branches into one: if thinkingEffort is set and not in availableVariants (including empty array), clear it.
Defense-in-depth: reject non-alphabetic variant values at the shell interpolation site, independent of upstream Zod schema validation.
| customInstructions: null, | ||
| maxReviewTimeMinutes: 10, | ||
| modelSlug: PRIMARY_DEFAULT_MODEL, | ||
| thinkingEffort: null as string | null, |
Contributor
There was a problem hiding this comment.
SUGGESTION: Avoid as type assertion — use a typed annotation instead
Per project coding style, as should be strongly avoided. Here null as string | null widens the literal null type so the return object matches both branches. A cleaner alternative is to annotate the default object with a type or extract a typed variable:
Suggested change
| thinkingEffort: null as string | null, | |
| thinkingEffort: null as string | null, |
Alternative (no as):
const thinkingEffort: string | null = null;
// then use `thinkingEffort` in the objectLow priority — the same as const pattern is already used on adjacent lines, so this is consistent with existing code.
…emoving duplicated variant logic
| customInstructions: null, | ||
| maxReviewTimeMinutes: 10, | ||
| modelSlug: PRIMARY_DEFAULT_MODEL, | ||
| thinkingEffort: null as string | null, |
Contributor
There was a problem hiding this comment.
[SUGGESTION]: Avoid as type assertion — use a typed annotation instead
Same issue as in code-reviews-router.ts line 161: null as string | null uses TypeScript's as operator which the project style guide strongly discourages. Consider using satisfies or a typed variable instead.
Suggested change
| thinkingEffort: null as string | null, | |
| thinkingEffort: null satisfies string | null, |
…hat when POST /session fails, the response body is captured and logged at error level. This prevents the opaque "HTTP error from server" message and instead shows the actual error (like the ConfigInvalidError we just discovered).
The kilo CLI v7.0.34 uses strict config validation and rejects the autoApproval key. Replaced the entire autoApproval block with equivalent settings in the permission format:
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.