fix(kaos): treat a leading ] in a glob character class as a literal - #2263
fix(kaos): treat a leading ] in a glob character class as a literal#2263LHMQ878 wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: f237cb8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c08a8806b3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // POSIX (and Python fnmatch): a `]` in the first position of the class | ||
| // body — after an optional negating `!` — is a literal member rather | ||
| // than the terminator, so the scan for the real terminator has to start | ||
| // past it. Treating it as the terminator yields an empty JS class | ||
| // (`[]`), which matches nothing at all, so the pattern silently stops |
There was a problem hiding this comment.
Move this newly added implementation narration into the existing top-of-file module header, or remove it if the code is sufficiently clear. The scoped agent-core-v2 guide explicitly prohibits comments beside functions or statements, but this block is embedded within the character-class parsing logic.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L13-L13
Useful? React with 👍 / 👎.
| // Python: fnmatch.fnmatch('].txt', '[]].txt') is True. Reading the `]` as | ||
| // the terminator instead would leave an empty class, which matches nothing, | ||
| // so the pattern would stop matching altogether. |
There was a problem hiding this comment.
Move this explanation into a top-of-file scenario header rather than leaving it beside the test statements. Besides making the new test file start with the scenario context, this is required by the scoped agent-core-v2 convention that comments live solely in the file header.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L13-L13
Useful? React with 👍 / 👎.
| let scanFrom = i + 1; | ||
| if (pattern[scanFrom] === '!') scanFrom++; | ||
| if (pattern[scanFrom] === ']') scanFrom++; | ||
| const end = pattern.indexOf(']', scanFrom); |
There was a problem hiding this comment.
When a caller supplies a class such as []--], the new scan treats the final ] as the terminator and constructs the JavaScript class [\]--]; new RegExp then throws Range out of order in character class. Before this change the pattern compiled to an empty class and simply matched nothing, which is also what Python fnmatch produces, so both LocalKaos.glob and the SSH implementation now throw for an input that previously returned an empty result. Normalize invalid ranges to a never-match expression or otherwise preserve the non-throwing behavior.
Useful? React with 👍 / 👎.
…class Review on MoonshotAI#2263 caught that `[]--]` regressed: before the leading-`]` fix it compiled to an empty class and matched nothing, and afterwards it produces `[\]--]`, which makes `new RegExp` throw `Range out of order in character class`. `_globWalk` calls `globPatternToRegex` outside its try block, so the throw aborts the whole directory walk. Reversed ranges that do not involve a leading `]` — `[a--]`, `[z-a]` — threw on `main` too, so guard the compile itself rather than just that one input: fall back to a never-matching pattern, which is what Python `fnmatch` yields for all three. Also move the agent-core-v2 comments into the file header per that package's AGENTS.md, which requires comments to live solely in the top-of-file block.
|
Thanks — all three were valid, and all three are addressed in 2fe604b (pushed 2026-07-27, after the reviewed P2 — reversed range now throws where it previously matched nothingThis was the important one, and it is a regression my own change introduced. Confirmed before fixing, by building the exact class the new scan produces for Your reading of the mechanism is exact: scanning past the leading Two things made this worse than a cosmetic difference, and both are why I took the fail-soft route you suggested rather than rejecting the pattern:
>>> import fnmatch
>>> [fnmatch.fnmatch(s, '[]--]') for s in (']', 'a', '-')]
[False, False, False]
>>> [fnmatch.fnmatch(s, '[z-a]') for s in (']', 'a', '-')]
[False, False, False]So the fix normalizes to a never-match expression, in both copies of the helper: const flags = caseSensitive ? '' : 'i';
try {
return new RegExp(regex, flags);
} catch {
// A character class can still be invalid as a JS regex even though it is a
// legal glob — `[a--]` and `[z-a]` are reversed ranges and make `RegExp`
// throw `Range out of order in character class`. Python `fnmatch` matches
// nothing for those, and callers such as `_globWalk` invoke this outside a
// try block, so throwing here would abort the whole walk. Fall back to a
// pattern that matches nothing.
return new RegExp('(?!)', flags);
}
Regression test in both packages, over Restored: P1 ×2 — comment placement under the agent-core-v2 conventionBoth correct, and I had missed that Note that the convention is scoped to Current head is |
`globPatternToRegex` scanned for the class terminator with
`pattern.indexOf(']', i + 1)`, so `[]]` closed the class on its first
character. That emitted the empty JS class `[]`, which matches nothing,
and the whole pattern silently stopped matching — `kaos.glob(dir, '[]].txt')`
returned no results even with `].txt` on disk.
POSIX and Python `fnmatch` treat a `]` in the first position of the class
body — after an optional negating `!` — as a literal member, and these
tests declare Python parity as the contract. Start the terminator scan
past an optional `!` and a leading `]`, and escape `]` inside the class
body so a literal member cannot close the class early.
Applied to both copies of the function: the original in `packages/kaos`
and the vendored copy in `agent-core-v2/_base/execEnv`.
…class Review on MoonshotAI#2263 caught that `[]--]` regressed: before the leading-`]` fix it compiled to an empty class and matched nothing, and afterwards it produces `[\]--]`, which makes `new RegExp` throw `Range out of order in character class`. `_globWalk` calls `globPatternToRegex` outside its try block, so the throw aborts the whole directory walk. Reversed ranges that do not involve a leading `]` — `[a--]`, `[z-a]` — threw on `main` too, so guard the compile itself rather than just that one input: fall back to a never-matching pattern, which is what Python `fnmatch` yields for all three. Also move the agent-core-v2 comments into the file header per that package's AGENTS.md, which requires comments to live solely in the top-of-file block.
2fe604b to
f237cb8
Compare
|
Rebased onto The conflict was comment-only. #2451 (remove the L0-L7 domain layering and clean up comment conventions) rewrote the module header of Worth noting because it isn't obvious from the conflict: #2451 also removed the sentence saying this file is vendored from Changeset added ( Status of the earlier review, all pushed before this rebase and carried through it:
|
1 participant
Related Issue
No existing issue — the problem is described below.
Problem
globPatternToRegexscans for the character-class terminator withpattern.indexOf(']', i + 1), so a]in the first position of the class body is read as the terminator instead of as a member.[]]therefore compiles to the empty JS character class[], which matches nothing at all, and the whole pattern silently stops matching.POSIX and Python
fnmatchboth treat that]as a literal member:Python parity is the stated contract for this helper —
packages/kaos/test/internal.test.tshas adescribe('glob semantic compatibility (Python parity)')block, and the existing "treats an unclosed[as a literal bracket" test comments that it "Mirrors Python fnmatch/glob".Reproduced end-to-end through the public API, with
].txtanda.txtin a temp dir:*.txt].txt,a.txt[]].txt[]].txtshould have matched].txt. Failure is silent: the caller gets an empty result set, not an error.The reachable call path is
LocalKaos.glob()→_globWalk(packages/kaos/src/local.ts:411) and the SSH equivalent (packages/kaos/src/ssh.ts:683), where the compiled regex filtersreaddiroutput.Kaosis part of the SDK surface (KimiHarnessaccepts akaosoption), so an embedder passing a bracket-class pattern hits this.What changed
Start the terminator scan past an optional negating
!and a leading], and escape]inside the class body so a literal member cannot close the class early:Applied to both copies of the function — the original in
packages/kaos/src/internal.tsand the copy vendored intopackages/agent-core-v2/src/_base/execEnv/globPattern.ts(its header says "Vendored from@moonshot-ai/kaosinternal.ts"), so the two do not drift.Compiled output, before → after:
[]].txt^[]\.txt$(matches nothing)^[\]]\.txt$[!]].txt^[]\.txt$^[^\]]\.txt$[]a].txt^[]a\]\.txt$^[\]a]\.txt$[a]].txt^[a]\]\.txt$^[a]\]\.txt$(unchanged — a]closing a non-empty class is still the terminator)file[abc\].txt^file[abc\]\.txt$^file[abc\]\.txt$(byte-identical)file[,[,[!The last two rows are the no-regression checks: the existing test at
packages/kaos/test/internal.test.ts:213(escapes backslashes inside character classes…) and the unclosed-[test both compile to exactly the same regex as before.Tests
glob semantic compatibility (Python parity)block inpackages/kaos/test/internal.test.ts.packages/agent-core-v2/test/_base/execEnv/globPattern.test.tsis new — the vendored copy had no test file at all. 5 cases, covering the same four plus the unclosed-[fallback.Verification run locally:
vitest runon both files: 33 passed, 1 skipped.scanFromlines makes 3 of the new tests fail, confirming they actually pin the bug.oxlint --type-awareon all 4 files: 0 warnings, 0 errors.tsc --noEmitclean for both packages.packages/kaos/test/local.test.tshas 5 failing symlink cases (T-C1…T-C6) on my Windows box. Agit stashcontrol run reproduces the identical 5 failures without this change, so they are a pre-existing local symlink-permission issue, not a regression here.Not in scope
While reproducing this I found a second, separate deviation in the same function:
/is permitted inside a character class, soa[/]bcompiles to^a[/]b$and one glob segment can cross a path separator — inconsistent with*and?, which both compile to[^/]. I left it out to keep this fix reviewable; happy to open a follow-up if you'd like it addressed.Changeset
No changeset. Per
.changeset/README.mdonly@moonshot-ai/kimi-codeand@moonshot-ai/kimi-code-sdkare publishable, and the table's "only modifies internal packages, no user-visible change in SDK / CLI" row applies:@moonshot-ai/kaosand@moonshot-ai/agent-core-v2are bothprivate, and no in-repo product code callskaos.globwith a bracket-class pattern — the CLI'sGlobtool delegates to ripgrep's own glob engine, and the only in-treeglob()callers areAcpKaos's pass-through and thecurrent.tsre-export. Say the word and I'll add apatchchangeset for@moonshot-ai/kimi-codeinstead.Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update. (docs/does not document glob character-class syntax.)