feat(permissions): optimize auto-mode classifier with in-CWD fast path and two-stage classification - #1098
feat(permissions): optimize auto-mode classifier with in-CWD fast path and two-stage classification#1098Zilv1nas wants to merge 1 commit into
Conversation
…h and two-stage classification - Add in-CWD file edit fast path: write/edit calls targeting files within ctx.cwd are auto-approved without invoking the LLM classifier, saving GPU resources. Protected paths (.git/, .env, .kimchi/, .claude/, shell configs) still go through the classifier. - Verify expanded allowlist: isReadOnlyTool() and isReadOnlyBashCommand() checks already run before the auto-mode classifier branch, so read-only MCP tools and read-only bash commands skip the classifier in auto mode. - Implement two-stage classification: Stage 1 uses a minimal prompt suffix with maxTokens=64 for a fast verdict. If Stage 1 returns safe, Stage 2 is skipped entirely. Stage 2 is the existing full reasoning call with retries and fallback. ClassifierResult gains an optional stage field. - Add comprehensive tests for all three changes (384 tests pass, build clean). Co-Authored-By: Kimchi <noreply@kimchi.dev>
Kimchi Code Review
Summary📊 Review Score: 70/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — Extensive test coverage was added. 🔒 Security concerns found: 📝 Found 5 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 70/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 3/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Extensive test coverage was added. classifier.test.ts now covers Stage 1 safe short-circuit, Stage 1 fallthrough on requires-confirmation/parse failure/error/aborted, Stage 2 retry semantics, fallback model interactions, and the maxTokens: 64 request option. index.test.ts covers in-CWD auto-approval for write/edit, protected path exclusions, relative paths, path traversal, mode gating, and notification emission. Existing tests were correctly updated for the extra Stage 1 call in retry/fallback scenarios.
🔒 Security concerns found: isInCwdFileEdit normalizes cwd to ${cwd}/ only when it lacks a trailing slash. When ctx.cwd is the filesystem root /, the normalized prefix remains /, so every absolute path satisfies resolved.startsWith('/') and can be auto-approved without classifier review. Protected-path regexes are also case-sensitive, which may be bypassed on case-insensitive filesystems (e.g. .ENV).
📝 Found 5 issue(s). See inline comments for details.
| const filePath = | ||
| typeof input.file_path === "string" ? input.file_path : typeof input.path === "string" ? input.path : "" | ||
| if (!filePath) return false | ||
|
|
There was a problem hiding this comment.
🚨🔒 Security
isInCwdFileEdit builds normalizedCwd as ${cwd}/ only when cwd does not end with /. If ctx.cwd is exactly /, normalizedCwd is also /, and resolved.startsWith('/') is true for any absolute path. A malicious or mistaken write/edit targeting /etc/passwd or any other sensitive file outside the workspace would be auto-approved without reaching the classifier.
💡 Suggestion: Normalize cwd so the prefix check always requires a separator after the directory. Use const normalizedCwd = cwd === '/' ? cwd : cwd.replace(/\/+$/, '') and check resolved === normalizedCwd || resolved.startsWith(${normalizedCwd}/), or alternatively reject when resolved is the directory itself.
| * fast path, even when they resolve within ctx.cwd. These paths can contain | ||
| * secrets, modify shell behaviour, or corrupt agent/harness state. | ||
| */ | ||
| const PROTECTED_PATH_PATTERNS: readonly RegExp[] = [ |
There was a problem hiding this comment.
PROTECTED_PATH_PATTERNS uses case-sensitive regexes such as /(^|\/)\.env$/. On case-insensitive filesystems an attacker or model could bypass the check with paths like .ENV or .Git/config, causing a protected file inside ctx.cwd to be auto-approved.
💡 Suggestion: Build or normalize the resolved path with a consistent casing and separator before matching, e.g. isProtectedPath(resolvedPath.toLowerCase()), and ensure the regex set is tested against common case variants.
| const filePath = | ||
| typeof input.file_path === "string" ? input.file_path : typeof input.path === "string" ? input.path : "" | ||
| if (!filePath) return false | ||
|
|
There was a problem hiding this comment.
The prefix check assumes POSIX / separators, but resolve from path uses the platform separator. On Windows, resolve returns backslash-separated paths while normalizedCwd may contain a trailing /, so resolved.startsWith(normalizedCwd) will always be false and the optimization is silently disabled.
💡 Suggestion: Normalize both resolved and cwd to POSIX-style separators with a helper before comparing, e.g. path.normalize(...).replace(/\\/g, '/').
| return { ...stage1Result, stage: 1 } | ||
| } | ||
|
|
||
| // Stage 2 (full reasoning): the existing full classifier call with retries |
There was a problem hiding this comment.
ℹ️🔧 Maintainability
The if (signal?.aborted) return unavailable("classifier aborted") check is duplicated immediately before the Stage 1 call. The first check at the top of classifyToolCall already returns in the same way, so the second check is dead code that can confuse readers.
💡 Suggestion: Remove the redundant signal?.aborted check on line 58, leaving only the initial guard.
| // Stage 1 errors are non-fatal — fall through to Stage 2. | ||
| return { verdict: "requires-confirmation", reason: "stage 1 error, falling through", ok: false, retryable: false } | ||
| } finally { | ||
| clearTimeout(timeoutHandle) |
There was a problem hiding this comment.
ℹ️
runClassifierFast swallows Stage 1 exceptions and error/aborted stop reasons without logging the underlying cause. If Stage 1 starts consistently falling through in production, there is no diagnostic signal to explain why the optimization is not short-circuiting.
💡 Suggestion: Emit a debug or trace log in the catch block and in the error/aborted branches that includes the error message or stop reason, while still returning the non-fatal fallthrough result.
1 participant
Summary
Optimizes the auto-mode permission classifier to reduce unnecessary GPU calls on shared B2B inference infrastructure.
Changes
1. In-CWD File Edit Fast Path (
src/extensions/permissions/index.ts)When mode is auto and the tool is
writeoredit, checks if the target file path resolves withinctx.cwd. If yes, auto-approves without calling the LLM classifier. Protected paths are excluded:.git/,.env,.env.*,.kimchi/,.claude/, shell config files (.bashrc,.zshrc,.profile)These protected paths still go through the classifier even when inside cwd.
2. Expanded Allowlist Verification (
src/extensions/permissions/index.ts)Verified that
isReadOnlyTool()andisReadOnlyBashCommand()checks already run before themode === "auto"branch in the tool_call handler (lines ~953-956). This means read-only MCP tools (list_*,get_*,describe_*) and read-only bash commands already skip the classifier in auto mode. No code change needed — confirmed correct.3. Two-Stage Classification (
src/extensions/permissions/classifier.ts,src/extensions/permissions/types.ts)Restructured
classifyToolCallto use a two-stage approach:maxTokens=64. If verdict issafe, returns immediately — no second call needed.safe. This is the existing full classifier call with retries and fallback model.ClassifierResultgains an optionalstagefield (1or2) to track which stage produced the result.Test Coverage
Checklist