-
Notifications
You must be signed in to change notification settings - Fork 134
fix(skills): stop walking the whole tree to answer "does any file match" #1213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c7e9371
6ca4853
fc4b78c
1f1e23a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -245,21 +245,52 @@ export namespace SystemPrompt { | |
| return v.filter((s) => typeof s === "string" && s.length > 0) | ||
| } | ||
|
|
||
| /** | ||
| * Directory an `applyPaths` glob is matched against. | ||
| * | ||
| * `Project.fromDirectory` reports `/` as the worktree for a directory belonging to no git | ||
| * project — a sentinel meaning "no project", not a tree to search. Matching against it | ||
| * auto-loads a skill because an unrelated file exists elsewhere on the machine: an empty | ||
| * directory picked up the dbt skills from any `dbt_project.yml` anywhere on disk. | ||
| * | ||
| * `/` is only that sentinel when there is no VCS. A git repository genuinely rooted at `/` | ||
| * reports the same worktree but with `vcs: "git"`, and must keep scanning from its root — | ||
| * the same distinction `fromDirectory` itself draws when it chooses the value. | ||
| * | ||
| * The fallback deliberately narrows to at-or-below the session directory. Outside a repo | ||
| * there is no project boundary to walk up to, so anything wider is a guess about which of | ||
| * the machine's files are "this project"; the previous behaviour made that guess and got it | ||
| * wrong. A marker file above the cwd no longer auto-loads its skill in that case, which is | ||
| * the intended trade against loading skills from unrelated directories. | ||
| */ | ||
| export function autoLoadScanRoot(worktree: string, directory: string, vcs: string | undefined): string { | ||
| return worktree === "/" && !vcs ? directory : worktree | ||
|
Comment on lines
+266
to
+267
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline packages/core/src/util/glob.ts --items all --type function
rg -n -C 8 'Glob\.exists|function exists|cwd|absolute|symlink|\.\.' \
packages/core/src/util/glob.ts \
packages/core/test/util/glob.test.tsRepository: AltimateAI/altimate-code Length of output: 15248 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '1,115p' packages/core/src/util/glob.ts
sed -n '205,240p' packages/opencode/src/session/system.ts
sed -n '250,275p' packages/opencode/src/session/system.ts
rg -n -C 5 'glob@|from "glob"|from .glob.|"glob"' package.json packages/*/package.json bun.lockb bun.lock 2>/dev/null || trueRepository: AltimateAI/altimate-code Length of output: 33211 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '275,305p' packages/opencode/src/session/system.ts
rg -n -C 6 'normalizeApplyPaths|applyPaths|anyMatchInWorktree' packages/opencode/src/session packages/opencode/src/skillRepository: AltimateAI/altimate-code Length of output: 18055 🌐 Web query:
💡 Result: In glob 13.0.5, several key options and underlying architectural components influence how paths are resolved and traversed. CWD (Current Working Directory) The cwd option defines the current working directory in which the glob search occurs [1][2]. It defaults to process.cwd [1][3]. It accepts either a string path or a file:// URL object or string [1][4]. Absolute Patterns and Root The root option allows you to define a string path that serves as the starting point for absolute patterns (patterns starting with /) [1][3]. This root is resolved against the cwd option [2][5]. On Windows, patterns like x:/* or //host/share/* will always start in the corresponding drive or share directory, ignoring the root setting [1][6]. Notably, the root option does not limit the file system walk to that directory; a pattern containing.. can still traverse outside of the root [1][3]. Absolute Option The absolute option, when set to true, ensures that matched files are returned as absolute paths [1][3]. When false, they are returned as relative paths (relative to the cwd) [1][2]. If this option is not explicitly set, absolute paths are returned for patterns that are absolute, while all others are returned relative to the cwd [1][5]. This process involves string path resolution rather than extra system calls to get a realpath [1][4]. Path-Scurry and Containment Path-Scurry is the underlying utility glob uses for efficient file system traversal, designed to minimize readdir and stat calls by tracking path information and preventing redundant operations [7][8][9]. Glob allows you to provide a custom PathScurry object via the scurry option, which is used to traverse the file system [2][3][10]. Because Path-Scurry manages the tree traversal, it handles path normalization and resolution (including.. segments) internally [8][9]. However, as noted in the glob documentation, these mechanisms do not impose strict filesystem containment; if a pattern contains.. or is otherwise constructed to point outside the intended search area, the traversal is not restricted by the cwd or root settings [1][3]. Citations:
🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '1,170p' packages/opencode/src/skill/skill.ts
sed -n '130,180p' packages/opencode/src/skill/index.ts
rg -n -C 5 'Skill\.list|scanExternal|EXTERNAL_SKILL|GLOBAL|project' packages/opencode/src/skill/skill.ts packages/opencode/src/skill/index.ts packages/opencode/srcRepository: AltimateAI/altimate-code Length of output: 50381 Path Traversal (CWE-22): Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') Reachability: External · Exploitability: Moderate Constrain
🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| async function anyMatchInWorktree(globs: string[]): Promise<boolean> { | ||
| // Search from worktree root so a skill that wants `dbt_project.yml` | ||
| // catches the file no matter how deep the user's cwd is. | ||
| // Search from the worktree root, so a skill that wants `dbt_project.yml` catches the file | ||
| // no matter how deep the user's cwd is — within a project. Outside one there is no root to | ||
| // search and `autoLoadScanRoot` falls back to the session directory; see its docstring for | ||
| // why that narrowing is deliberate. | ||
| // Errors propagate to the caller's try/catch (collectAutoLoadedSkills) | ||
| // so the warning log there actually fires. | ||
| const root = Instance.worktree | ||
| // `Glob.exists` rather than `scan(...).length > 0`: this only needs to know whether any | ||
| // file matches, and `scan` walks the whole tree before the caller can look. That cost is | ||
| // paid once per `applyPaths` skill — two ship builtin — and the root is the worktree, which | ||
| // is `/` for a directory outside any git repo. Measured from such a directory, the two | ||
| // scans were ~45s of a ~51s startup, all of it before the first token. | ||
| const root = autoLoadScanRoot(Instance.worktree, Instance.directory, Instance.project.vcs) | ||
| for (const g of globs) { | ||
| const matches = await Glob.scan(g, { | ||
| cwd: root, | ||
| absolute: true, | ||
| include: "file", | ||
| dot: false, | ||
| symlink: false, | ||
| }) | ||
| if (matches.length > 0) return true | ||
| if ( | ||
| await Glob.exists(g, { | ||
| cwd: root, | ||
| absolute: true, | ||
| include: "file", | ||
| dot: false, | ||
| symlink: false, | ||
| }) | ||
| ) | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| // altimate_change start — a skill must not auto-load because of a file on some other project. | ||
| import { describe, expect, test } from "bun:test" | ||
| import { SystemPrompt } from "../../src/session/system" | ||
|
|
||
| describe("autoLoadScanRoot", () => { | ||
| test("uses the worktree when there is a real project", () => { | ||
| expect(SystemPrompt.autoLoadScanRoot("/Users/me/code/proj", "/Users/me/code/proj/sub", "git")).toBe( | ||
| "/Users/me/code/proj", | ||
| ) | ||
| }) | ||
|
|
||
| test("falls back to the session directory when the worktree is the no-project sentinel", () => { | ||
| // `Project.fromDirectory` returns `/` with no vcs for a directory belonging to no git | ||
| // project. Searching it matched any `dbt_project.yml` anywhere on the machine, so an empty | ||
| // scratch directory silently loaded the dbt skills into its system prompt. | ||
| expect(SystemPrompt.autoLoadScanRoot("/", "/tmp/scratch", undefined)).toBe("/tmp/scratch") | ||
| }) | ||
|
|
||
| test("keeps the root for a git repository genuinely rooted at /", () => { | ||
| // Same worktree value, different meaning: `fromDirectory` reports `/` with `vcs: "git"` for | ||
| // a real repo at the filesystem root, and narrowing that to the cwd would stop a marker at | ||
| // `/` matching a session started in `/workspace/sub`. | ||
| expect(SystemPrompt.autoLoadScanRoot("/", "/workspace/sub", "git")).toBe("/") | ||
| }) | ||
|
|
||
| test("does not treat a path merely starting with / as the sentinel", () => { | ||
| // Guard against matching by prefix rather than equality — every absolute path starts with "/". | ||
| expect(SystemPrompt.autoLoadScanRoot("/srv", "/tmp/scratch", undefined)).toBe("/srv") | ||
| }) | ||
| }) | ||
| // altimate_change end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: When an
applyPathsglob contains.., this fallback still letsGlob.existsmatch files aboveInstance.directory, so parent markers can auto-load skills despite the intended narrowing. Enforce that matches remain under the fallback directory or reject escaping patterns.Prompt for AI agents