diff --git a/.changeset/glob-rule-subjects-cross-slashes.md b/.changeset/glob-rule-subjects-cross-slashes.md new file mode 100644 index 0000000000..fa4c9d196d --- /dev/null +++ b/.changeset/glob-rule-subjects-cross-slashes.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/kimi-code": patch +--- + +Fix permission rule argument patterns such as Bash(rm -rf*) not matching commands, URLs, or search text that contain slashes or dot segments. Patterns negated with `!` now exclude those subjects as well. diff --git a/packages/agent-core-v2/src/tool/rule-match.ts b/packages/agent-core-v2/src/tool/rule-match.ts index 94cf3720f7..d120287a88 100644 --- a/packages/agent-core-v2/src/tool/rule-match.ts +++ b/packages/agent-core-v2/src/tool/rule-match.ts @@ -5,9 +5,12 @@ * and the rule-subject helpers (`literalRulePattern`, * `escapeRuleSubjectLiteral`, `matchesGlobRuleSubject`, * `matchesPathRuleSubject`) that tool implementations use to build their - * `matchesRule` closures and canonical rule strings. Path matching compares - * normalized path variants, so `./a`, `dir/../a`, and Windows separator or - * case variants can match the same rule. Pure functions; no scoped service. + * `matchesRule` closures and canonical rule strings. Glob matching accepts a + * subject under path-glob semantics or as opaque text where `*` also crosses + * `/`; NUL-bearing subjects match under path-glob semantics only. Path + * matching compares normalized path variants, so `./a`, `dir/../a`, and + * Windows separator or case variants can match the same rule. Pure + * functions; no scoped service. */ import { isAbsolute, join, parse } from 'pathe'; @@ -27,19 +30,46 @@ interface PathMatchSemantics { readonly pathClass: PathClass; } +const SLASH_PLACEHOLDER = '\0'; + export function globMatch(value: string, pattern: string, options?: { nocase?: boolean }): boolean { - if (picomatch.isMatch(value, pattern, options)) return true; + if (pathSegmentGlobMatch(value, pattern, options)) return true; + if (value.includes(SLASH_PLACEHOLDER) || pattern.includes(SLASH_PLACEHOLDER)) return false; + + const opaqueOptions = { ...options, dot: true }; + if (picomatch.isMatch(asOpaqueText(value), asOpaqueText(pattern), opaqueOptions)) return true; const normalizedValue = stripLeadingDotSlash(value); const normalizedPattern = stripLeadingDotSlash(pattern); if (normalizedValue === value && normalizedPattern === pattern) return false; - return picomatch.isMatch(normalizedValue, normalizedPattern, options); + return picomatch.isMatch( + asOpaqueText(normalizedValue), + asOpaqueText(normalizedPattern), + opaqueOptions, + ); +} + +function asOpaqueText(value: string): string { + return value.replaceAll('/', SLASH_PLACEHOLDER); } function stripLeadingDotSlash(value: string): string { return value.startsWith('./') ? value.slice(2) : value; } +function pathSegmentGlobMatch( + value: string, + pattern: string, + options?: { nocase?: boolean }, +): boolean { + if (picomatch.isMatch(value, pattern, options)) return true; + + const normalizedValue = stripLeadingDotSlash(value); + const normalizedPattern = stripLeadingDotSlash(pattern); + if (normalizedValue === value && normalizedPattern === pattern) return false; + return picomatch.isMatch(normalizedValue, normalizedPattern, options); +} + export function pathGlobMatch( value: string, pattern: string, @@ -48,11 +78,11 @@ export function pathGlobMatch( const semantics = pathMatchSemantics(value, pattern, pathOptions); const nocase = pathOptions?.caseInsensitivePaths ?? true; - if (globMatch(value, pattern, { nocase })) return true; + if (pathSegmentGlobMatch(value, pattern, { nocase })) return true; for (const valueVariant of pathVariants(value, semantics, pathOptions)) { for (const patternVariant of pathVariants(pattern, semantics, pathOptions)) { - if (globMatch(valueVariant, patternVariant, { nocase })) return true; + if (pathSegmentGlobMatch(valueVariant, patternVariant, { nocase })) return true; } } return false; diff --git a/packages/agent-core-v2/test/agent/permissionRules/matchesRule.test.ts b/packages/agent-core-v2/test/agent/permissionRules/matchesRule.test.ts index 173f3d7575..1bd1070dfd 100644 --- a/packages/agent-core-v2/test/agent/permissionRules/matchesRule.test.ts +++ b/packages/agent-core-v2/test/agent/permissionRules/matchesRule.test.ts @@ -132,6 +132,44 @@ describe('permissionRules/matchPermissionRule', () => { expect(matches(rule('Bad(unclosed'), 'Bad', noArgs)).toBe(false); }); + it('matches glob rule subjects as opaque text rather than as paths', () => { + expect(matchesGlobRuleSubject('rm -rf*', 'rm -rf x')).toBe(true); + expect(matchesGlobRuleSubject('rm -rf*', 'rm -rf /tmp/x')).toBe(true); + expect(matchesGlobRuleSubject('git *', 'git commit -m "fix src/a.ts"')).toBe(true); + expect(matchesGlobRuleSubject('rm -rf*', 'rm -rf ./build')).toBe(true); + expect(matchesGlobRuleSubject('rm -rf*', 'rm -rf ~/.ssh')).toBe(true); + expect(matchesGlobRuleSubject('rm -rf*', 'rm -rf /home/u/.ssh')).toBe(true); + expect(matchesGlobRuleSubject('https://example.com/*', 'https://example.com/a/b')).toBe(true); + expect(matchesGlobRuleSubject('*acme corp*', 'news about acme corp / rivals')).toBe(true); + expect(matchesGlobRuleSubject('**rm**', 'rm -rf /tmp/x')).toBe(true); + expect(matchesGlobRuleSubject('git *', 'git status')).toBe(true); + expect(matchesGlobRuleSubject('git *', 'git2 status')).toBe(false); + expect(matchesGlobRuleSubject('rm -rf*', 'git status')).toBe(false); + expect(matchesGlobRuleSubject('git log -- src/*.ts', 'git log -- srcXx.ts')).toBe(false); + expect(matchesGlobRuleSubject('https://example.com/a', 'https://example.com/b')).toBe(false); + expect(matchesGlobRuleSubject('!git *', 'git commit -m "fix src/a.ts"')).toBe(false); + expect(matchesGlobRuleSubject('!git *', 'npm test')).toBe(true); + }); + + it('keeps historical glob matches that opaque-text semantics alone would drop', () => { + expect(matchesGlobRuleSubject('**/*.ts', 'a.ts')).toBe(true); + expect(matchesGlobRuleSubject('a/**/b', 'a/b')).toBe(true); + expect(matchesGlobRuleSubject('a/**/b', 'a/x/y/b')).toBe(true); + }); + + it('keeps NUL-bearing subjects distinct instead of conflating them', () => { + expect(matchesGlobRuleSubject('ab', 'a\u0000b')).toBe(false); + expect(matchesGlobRuleSubject('a/b', 'a\u0000b')).toBe(false); + expect(matchesGlobRuleSubject('a\u0000b', 'a\u0000b')).toBe(true); + expect(matchesGlobRuleSubject('a*', 'a\u0000b')).toBe(true); + }); + + it('keeps path rule subjects on path semantics where * does not cross /', () => { + expect(matchesPathRuleSubject('src/*', 'src/a.ts')).toBe(true); + expect(matchesPathRuleSubject('src/**', 'src/sub/a.ts')).toBe(true); + expect(matchesPathRuleSubject('src/*', 'src/sub/a.ts')).toBe(false); + }); + it('does not match rule arguments without an execution matcher', () => { expect(matches(rule('Custom("query":"a.b")'), 'Custom', noArgs)).toBe(false); expect(matches(rule('Bash("command":"git status")'), 'Bash', noArgs)).toBe(false); diff --git a/packages/agent-core-v2/test/lint/header-comments.test.ts b/packages/agent-core-v2/test/lint/header-comments.test.ts new file mode 100644 index 0000000000..7db771ad0c --- /dev/null +++ b/packages/agent-core-v2/test/lint/header-comments.test.ts @@ -0,0 +1,37 @@ +/** + * Header-only-comment gate — the package AGENTS.md "Comment conventions" + * section requires comments to live solely in the top-of-file block, never + * beside functions, methods, or statements. This probe guards files that + * previously attracted review findings for inline implementation narration; + * extend the list when a file newly trades in subtle encoding or matching + * rules that tempt an explanatory comment. + */ + +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { describe, expect, it } from 'vitest'; + +const SRC_ROOT = join(import.meta.dirname, '..', '..', 'src'); + +const HEADER_ONLY_FILES = ['tool/rule-match.ts']; + +function commentLinesOutsideHeader(source: string): string[] { + const headerEnd = source.indexOf('*/'); + const body = headerEnd === -1 ? source : source.slice(headerEnd + 2); + return body + .split('\n') + .filter( + (line) => + line.includes('//') || line.includes('/*') || line.trimStart().startsWith('*'), + ); +} + +describe('header-only comments', () => { + for (const file of HEADER_ONLY_FILES) { + it(`${file} keeps comments inside the top-of-file header`, () => { + const source = readFileSync(join(SRC_ROOT, file), 'utf8'); + expect(source.startsWith('/**')).toBe(true); + expect(commentLinesOutsideHeader(source)).toEqual([]); + }); + } +}); diff --git a/packages/agent-core/src/tools/support/path-glob-match.ts b/packages/agent-core/src/tools/support/path-glob-match.ts index d3531fe163..00f177e26c 100644 --- a/packages/agent-core/src/tools/support/path-glob-match.ts +++ b/packages/agent-core/src/tools/support/path-glob-match.ts @@ -15,23 +15,58 @@ interface PathMatchSemantics { readonly pathClass: PathClass; } +const SLASH_PLACEHOLDER = '\0'; + /** * Match ordinary string fields, like command text or search patterns. * `*` and `**` work as wildcards, but the value is not treated as a file path. */ export function globMatch(value: string, pattern: string, options?: { nocase?: boolean }): boolean { - if (picomatch.isMatch(value, pattern, options)) return true; + // Try the historical path-semantics match first so rules that matched + // before keep matching (e.g. `a/**/b` still matches `a/b`). + if (pathSegmentGlobMatch(value, pattern, options)) return true; + + // Then match the subject as opaque text: picomatch gives wildcards path + // semantics (`*` stops at `/` and refuses dot segments), so rewrite `/` to + // a placeholder and allow dots instead. NUL-bearing subjects skip this: + // the historical match above already compares them literally, and the + // rewrite must stay injective so distinct subjects cannot collide. + if (value.includes(SLASH_PLACEHOLDER) || pattern.includes(SLASH_PLACEHOLDER)) return false; + + const opaqueOptions = { ...options, dot: true }; + if (picomatch.isMatch(asOpaqueText(value), asOpaqueText(pattern), opaqueOptions)) return true; const normalizedValue = stripLeadingDotSlash(value); const normalizedPattern = stripLeadingDotSlash(pattern); if (normalizedValue === value && normalizedPattern === pattern) return false; - return picomatch.isMatch(normalizedValue, normalizedPattern, options); + return picomatch.isMatch( + asOpaqueText(normalizedValue), + asOpaqueText(normalizedPattern), + opaqueOptions, + ); +} + +function asOpaqueText(value: string): string { + return value.replaceAll('/', SLASH_PLACEHOLDER); } function stripLeadingDotSlash(value: string): string { return value.startsWith('./') ? value.slice(2) : value; } +function pathSegmentGlobMatch( + value: string, + pattern: string, + options?: { nocase?: boolean }, +): boolean { + if (picomatch.isMatch(value, pattern, options)) return true; + + const normalizedValue = stripLeadingDotSlash(value); + const normalizedPattern = stripLeadingDotSlash(pattern); + if (normalizedValue === value && normalizedPattern === pattern) return false; + return picomatch.isMatch(normalizedValue, normalizedPattern, options); +} + /** * Match file path fields, like Read/Write/Edit `path`. * Also compares normalized forms, so `./a`, `dir/../a`, and Windows @@ -45,11 +80,11 @@ export function pathGlobMatch( const semantics = pathMatchSemantics(value, pattern, pathOptions); const nocase = pathOptions?.caseInsensitivePaths ?? true; - if (globMatch(value, pattern, { nocase })) return true; + if (pathSegmentGlobMatch(value, pattern, { nocase })) return true; for (const valueVariant of pathVariants(value, semantics, pathOptions)) { for (const patternVariant of pathVariants(pattern, semantics, pathOptions)) { - if (globMatch(valueVariant, patternVariant, { nocase })) return true; + if (pathSegmentGlobMatch(valueVariant, patternVariant, { nocase })) return true; } } return false; diff --git a/packages/agent-core/test/agent/permission.test.ts b/packages/agent-core/test/agent/permission.test.ts index 6ebf967680..9e8dc4ebbe 100644 --- a/packages/agent-core/test/agent/permission.test.ts +++ b/packages/agent-core/test/agent/permission.test.ts @@ -3799,6 +3799,44 @@ describe('Permission rule helpers', () => { expect(ruleMatches(permissionRule('Bad(unclosed'), 'Bad', {})).toBe(false); }); + it('matches glob rule subjects as opaque text rather than as paths', () => { + expect(matchesGlobRuleSubject('rm -rf*', 'rm -rf x')).toBe(true); + expect(matchesGlobRuleSubject('rm -rf*', 'rm -rf /tmp/x')).toBe(true); + expect(matchesGlobRuleSubject('git *', 'git commit -m "fix src/a.ts"')).toBe(true); + expect(matchesGlobRuleSubject('rm -rf*', 'rm -rf ./build')).toBe(true); + expect(matchesGlobRuleSubject('rm -rf*', 'rm -rf ~/.ssh')).toBe(true); + expect(matchesGlobRuleSubject('rm -rf*', 'rm -rf /home/u/.ssh')).toBe(true); + expect(matchesGlobRuleSubject('https://example.com/*', 'https://example.com/a/b')).toBe(true); + expect(matchesGlobRuleSubject('*acme corp*', 'news about acme corp / rivals')).toBe(true); + expect(matchesGlobRuleSubject('**rm**', 'rm -rf /tmp/x')).toBe(true); + expect(matchesGlobRuleSubject('git *', 'git status')).toBe(true); + expect(matchesGlobRuleSubject('git *', 'git2 status')).toBe(false); + expect(matchesGlobRuleSubject('rm -rf*', 'git status')).toBe(false); + expect(matchesGlobRuleSubject('git log -- src/*.ts', 'git log -- srcXx.ts')).toBe(false); + expect(matchesGlobRuleSubject('https://example.com/a', 'https://example.com/b')).toBe(false); + expect(matchesGlobRuleSubject('!git *', 'git commit -m "fix src/a.ts"')).toBe(false); + expect(matchesGlobRuleSubject('!git *', 'npm test')).toBe(true); + }); + + it('keeps historical glob matches that opaque-text semantics alone would drop', () => { + expect(matchesGlobRuleSubject('**/*.ts', 'a.ts')).toBe(true); + expect(matchesGlobRuleSubject('a/**/b', 'a/b')).toBe(true); + expect(matchesGlobRuleSubject('a/**/b', 'a/x/y/b')).toBe(true); + }); + + it('keeps NUL-bearing subjects distinct instead of conflating them', () => { + expect(matchesGlobRuleSubject('ab', 'a\u0000b')).toBe(false); + expect(matchesGlobRuleSubject('a/b', 'a\u0000b')).toBe(false); + expect(matchesGlobRuleSubject('a\u0000b', 'a\u0000b')).toBe(true); + expect(matchesGlobRuleSubject('a*', 'a\u0000b')).toBe(true); + }); + + it('keeps path rule subjects on path semantics where * does not cross /', () => { + expect(matchesPathRuleSubject('src/*', 'src/a.ts')).toBe(true); + expect(matchesPathRuleSubject('src/**', 'src/sub/a.ts')).toBe(true); + expect(matchesPathRuleSubject('src/*', 'src/sub/a.ts')).toBe(false); + }); + it('does not match rule arguments without an execution matcher', () => { expect( ruleMatches(permissionRule('Custom("query":"a.b")'), 'Custom', {