mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-05 16:34:00 +01:00
fix(context-bill): walk each real directory once so link fan-out cannot explode
walkMd guarded only the current path, and walked every link to the same directory, so links that fan out into each other grew the walk exponentially; under --exact every duplicate path was another count_tokens POST on the user's key. Expand each real directory and return each real file once per walk, under the first lexical path reached with entries sorted. Refuse a walk past 200,000 entries with an error instead of truncating. --exact sends each real file once and prices every lexical key that resolves to it from that measurement. Found by the /ship Step 11 adversarial reviews (Claude and GPT). Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
0b734d3494
commit
4574c06e1a
3 files changed
+105
-27
No files matched your search
+2
-2
@@ -5,10 +5,10 @@
|
||||
**`gstack-context-bill` now bills a real install.** On `~/.claude/skills` it reported "2 skills, ~134 tok" always-on, because setup installs each skill as a real directory whose `SKILL.md` and `sections/` are symlinks into the gstack checkout, and the walker only counted regular files. The tool built to answer the token-cost complaint gave a falsely tiny answer on the layout users actually have. On a fresh install it now reports 58 skills, ~6.8K tokens always-on, and 3.6 MB on disk (it was 37 KB across 2 skills). Billing the checkout itself is unchanged (66 skills, ~7.2K tokens).
|
||||
|
||||
### Fixed
|
||||
- **`lib/context-bill.ts`**: `findSkillDirs`, `walkMd` and the foreign-file check follow symlinked files and directories. Every path the bill reports, and every file `--exact` keys on, stays the lexical path under the root you passed. A directory already on the current walk (a loop) is never re-entered, and a link to a directory that contains the tree being walked (`..`, `/`, `$HOME`) is never followed, so a link cannot pull a sibling skill or your home directory into the bill. A link named `*.md` counts only when its target is a `.md` file, so `--exact` never sends a non-markdown file through a markdown-named link. Two links to one directory are both walked, as two links to one skill already were, so the result never depends on directory listing order. `walkMd` skips a child repo checkout (the `gstack/` link beside the installed skills), as `findSkillDirs` already did, so `--exact` does not send the whole checkout.
|
||||
- **`lib/context-bill.ts`**: `findSkillDirs`, `walkMd` and the foreign-file check follow symlinked files and directories. Every path the bill reports, and every file `--exact` keys on, stays the lexical path under the root you passed. A link to a directory that contains the tree being walked (`..`, `/`, `$HOME`) is never followed, so a link cannot climb out of the root into its parent or your home directory. The markdown walk expands each real directory once and counts each real file once per skill, under the first path reached in sorted order, which is how a skill loads it: loops end, links that fan out into each other cannot multiply the walk, and a tree with more than 200,000 entries is refused with an error instead of being cut short. `--exact` sends each real file once and prices every link to it with that one measurement. A link named `*.md` counts only when its target is a `.md` file, so `--exact` never sends a non-markdown file through a markdown-named link. `walkMd` skips a child repo checkout (the `gstack/` link beside the installed skills), as `findSkillDirs` already did, so `--exact` does not send the whole checkout.
|
||||
|
||||
### Tests
|
||||
- `test/context-bill.test.ts`: an installed-layout fixture (a checkout outside the skills root, real skill directories with symlinked `SKILL.md` and `sections/`, and a `gstack/` link to the checkout) bills both skills with the target's exact frontmatter, `SKILL.md` and section bytes, and keeps every walked path under the root. Negative controls: a dangling `SKILL.md` link and a `SKILL.md` link to a directory are not skills. A second fixture holds a self link, a parent link that reaches a skill outside the root, a loop back into the skill and a link up to the temp root; only the one real skill and its own files are billed. Further cases cover a loop to a directory below the walk root, a root whose name starts with `..`, a symlinked `agents.md` (with a dangling link and a directory of a foreign-file name as controls), and `--exact` on the installed layout (every billed file measured; nothing behind `gstack/` or a `.md`-named link to a non-markdown file is sent). The installed-layout and escape fixtures fail on main (the first finds no skills, the second bills the outside skill).
|
||||
- `test/context-bill.test.ts`: an installed-layout fixture (a checkout outside the skills root, real skill directories with symlinked `SKILL.md` and `sections/`, and a `gstack/` link to the checkout) bills both skills with the target's exact frontmatter, `SKILL.md` and section bytes, and keeps every walked path under the root. Negative controls: a dangling `SKILL.md` link and a `SKILL.md` link to a directory are not skills. A second fixture holds a self link, a parent link that reaches a skill outside the root, a loop back into the skill and a link up to the temp root; only the one real skill and its own files are billed. Further cases cover a loop to a directory below the walk root, a root whose name starts with `..`, a symlinked `agents.md` (with a dangling link and a directory of a foreign-file name as controls), `--exact` on the installed layout (every billed file measured; nothing behind `gstack/` or a `.md`-named link to a non-markdown file is sent), a 20-level fan-out of links that reaches one file by 2^20 paths (walked once; hangs on the previous walker), and two skills linking one checkout file and directory (each real file sent once, both skills priced from it). The installed-layout and escape fixtures fail on main (the first finds no skills, the second bills the outside skill).
|
||||
|
||||
### Gate integrity
|
||||
- No threshold, budget, timeout, trial count, case kind or assertion changed. The bill of this repo's own tree, which the context-budget ratchet reads, is byte-for-byte unchanged.
|
||||
|
||||
+50
-23
@@ -43,6 +43,8 @@ const PROSE_REF = /`(references\/[^`<>]+\.md)`/g;
|
||||
const ROUTER_KEYS = new Set(["name", "description", "version", "allowed-tools", "triggers", "preamble-tier"]);
|
||||
// Skill-shaped files other hosts drop into scanner scope.
|
||||
const FOREIGN_SKILL_FILE = /^(skill\.(ya?ml|json)|agents?\.md|\.cursorrules|\.windsurfrules)$/i;
|
||||
/** Directory entries one walkMd call may examine before it refuses the tree. */
|
||||
const WALK_ENTRY_CAP = 200_000;
|
||||
|
||||
/**
|
||||
* Bytes per token, per content class, fitted to real count_tokens measurements.
|
||||
@@ -233,11 +235,13 @@ function encloses(outer: string, inner: string): boolean {
|
||||
*
|
||||
* Symlinked files and directories are followed, and every returned path stays
|
||||
* the lexical path under `dir`, so buildBill and --exact key the same files.
|
||||
* Two links to one directory are both walked (each is real scanner load), so
|
||||
* the result never depends on readdir order. A directory that is already on
|
||||
* the current walk (a loop) or that encloses `dir` is never entered. A link
|
||||
* named *.md counts only when its target is a .md file too, so --exact never
|
||||
* sends a non-markdown file through a markdown-named link.
|
||||
* Each real directory is expanded once and each real file returned once per
|
||||
* walk, under the first lexical path reached with entries in sorted order: a
|
||||
* skill loads a file once however many links reach it, and links that fan out
|
||||
* into each other cannot multiply the walk. A directory that encloses `dir` is
|
||||
* never entered. A link named *.md counts only when its target is a .md file
|
||||
* too, so --exact never sends a non-markdown file through a markdown-named
|
||||
* link. More than WALK_ENTRY_CAP entries is an error, never a silent cut.
|
||||
*/
|
||||
export function walkMd(dir: string): string[] {
|
||||
const out: string[] = [];
|
||||
@@ -247,41 +251,46 @@ export function walkMd(dir: string): string[] {
|
||||
} catch {
|
||||
return out;
|
||||
}
|
||||
const walk = (current: string, ancestors: string[]) => {
|
||||
const seen = new Set<string>([rootReal]);
|
||||
let entryCount = 0;
|
||||
const walk = (current: string) => {
|
||||
let entries: fs.Dirent[];
|
||||
try {
|
||||
entries = fs.readdirSync(current, { withFileTypes: true });
|
||||
} catch {
|
||||
return;
|
||||
}
|
||||
entries.sort((a, b) => (a.name < b.name ? -1 : a.name > b.name ? 1 : 0));
|
||||
for (const e of entries) {
|
||||
if (++entryCount > WALK_ENTRY_CAP) {
|
||||
throw new Error(
|
||||
`${dir} has more than ${WALK_ENTRY_CAP} entries to walk; a symlink in it probably points at a large tree. ` +
|
||||
`Nothing was billed or sent. Remove or narrow that link and run again.`,
|
||||
);
|
||||
}
|
||||
if (e.name.startsWith(".") || e.name === "node_modules") continue;
|
||||
const p = path.join(current, e.name);
|
||||
const kind = entryKind(p, e);
|
||||
if (kind === "file") {
|
||||
if (!e.name.endsWith(".md")) continue;
|
||||
if (e.isSymbolicLink()) {
|
||||
try {
|
||||
if (!fs.realpathSync(p).endsWith(".md")) continue;
|
||||
} catch {
|
||||
continue;
|
||||
}
|
||||
}
|
||||
out.push(p);
|
||||
continue;
|
||||
}
|
||||
if (kind !== "dir" || fs.existsSync(path.join(p, ".git"))) continue;
|
||||
if (kind === "file" ? !e.name.endsWith(".md") : kind !== "dir" || fs.existsSync(path.join(p, ".git"))) continue;
|
||||
let real: string;
|
||||
try {
|
||||
real = fs.realpathSync(p);
|
||||
} catch {
|
||||
continue;
|
||||
}
|
||||
if (ancestors.includes(real) || encloses(real, rootReal)) continue;
|
||||
walk(p, [...ancestors, real]);
|
||||
if (seen.has(real)) continue;
|
||||
if (kind === "file") {
|
||||
if (!real.endsWith(".md")) continue;
|
||||
seen.add(real);
|
||||
out.push(p);
|
||||
continue;
|
||||
}
|
||||
if (encloses(real, rootReal)) continue;
|
||||
seen.add(real);
|
||||
walk(p);
|
||||
}
|
||||
};
|
||||
walk(dir, [rootReal]);
|
||||
walk(dir);
|
||||
return out;
|
||||
}
|
||||
|
||||
@@ -799,12 +808,17 @@ export async function measureExactTokens(
|
||||
const opts: CountTokensOptions = { model, apiKey, fetchImpl };
|
||||
|
||||
const texts = new Map<string, string>();
|
||||
// The walk returns each real file once, so each is sent once. The bill keys
|
||||
// the same file by every lexical path it is billed under; lexicalOf maps a
|
||||
// real file back to the one key that was measured.
|
||||
const lexicalOf = new Map<string, string>();
|
||||
// Resolve before keying. buildBill resolves its root, so a relative root here
|
||||
// would produce keys that never match and every lookup would fall back to the
|
||||
// estimate -- exact mode silently degrading to the thing it replaces.
|
||||
for (const file of walkMd(path.resolve(root))) {
|
||||
const text = fs.readFileSync(file, "utf8");
|
||||
texts.set(file, text);
|
||||
lexicalOf.set(fs.realpathSync(file), file);
|
||||
if (path.basename(file) === "SKILL.md") {
|
||||
const fm = parseFrontmatter(text);
|
||||
if (fm.block) texts.set(frontmatterKey(file), fm.block);
|
||||
@@ -852,8 +866,21 @@ export async function measureExactTokens(
|
||||
// estimate rather than billing zero. Misses are counted, not swallowed: a bill
|
||||
// that is part-measured and part-estimated must not present itself as measured.
|
||||
const missed = new Set<string>();
|
||||
const fmSuffix = frontmatterKey("");
|
||||
const measuredKey = (key: string): string | undefined => {
|
||||
const isFm = key.endsWith(fmSuffix);
|
||||
let real: string;
|
||||
try {
|
||||
real = fs.realpathSync(isFm ? key.slice(0, -fmSuffix.length) : key);
|
||||
} catch {
|
||||
return undefined;
|
||||
}
|
||||
const lexical = lexicalOf.get(real);
|
||||
return lexical === undefined ? undefined : isFm ? frontmatterKey(lexical) : lexical;
|
||||
};
|
||||
const tokensOf: TokensOf = (key, bytes) => {
|
||||
const exact = counts.get(key);
|
||||
const alias = counts.has(key) ? key : measuredKey(key);
|
||||
const exact = alias === undefined ? undefined : counts.get(alias);
|
||||
if (exact !== undefined) return exact;
|
||||
missed.add(key);
|
||||
return estimateTokensOf(key, bytes);
|
||||
|
||||
@@ -273,13 +273,13 @@ describe("upstream fix b: installed-tree layout (repo-checkout subdir skip)", ()
|
||||
}
|
||||
// Every path the bill (and --exact) keys on stays under the skills root.
|
||||
for (const file of walkMd(skills)) expect(file.startsWith(skills + path.sep)).toBe(true);
|
||||
// dirlink/SKILL.md links to review/sections, so its file is walked under both names.
|
||||
// dirlink/SKILL.md links to review/sections: the one real file is walked
|
||||
// once, under the first name reached in sorted order.
|
||||
expect(walkMd(skills).map((f) => path.relative(skills, f)).sort()).toEqual([
|
||||
path.join("dirlink", "SKILL.md", "flow.md"),
|
||||
path.join("office-hours", "SKILL.md"),
|
||||
path.join("office-hours", "sections", "flow.md"),
|
||||
path.join("review", "SKILL.md"),
|
||||
path.join("review", "sections", "flow.md"),
|
||||
]);
|
||||
fs.rmSync(tmp, { recursive: true, force: true });
|
||||
});
|
||||
@@ -331,6 +331,29 @@ describe("upstream fix b: installed-tree layout (repo-checkout subdir skip)", ()
|
||||
fs.rmSync(tmp, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it("walks a fan-out of links once per real directory and file", () => {
|
||||
if (process.platform === "win32") return;
|
||||
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "context-bill-fanout-"));
|
||||
// L0..L19 sit outside the root; each level holds two links to the next, so
|
||||
// a walk that expands every link reaches leaf.md by 2^20 paths.
|
||||
const depth = 20;
|
||||
for (let d = 0; d <= depth; d++) fs.mkdirSync(path.join(tmp, `L${d}`));
|
||||
fs.writeFileSync(path.join(tmp, `L${depth}`, "leaf.md"), "l".repeat(300));
|
||||
for (let d = 0; d < depth; d++) {
|
||||
for (const name of ["a", "b"]) fs.symlinkSync(path.join(tmp, `L${d + 1}`), path.join(tmp, `L${d}`, name));
|
||||
}
|
||||
const root = path.join(tmp, "skills");
|
||||
fs.mkdirSync(path.join(root, "qa"), { recursive: true });
|
||||
fs.writeFileSync(path.join(root, "qa", "SKILL.md"), "---\nname: qa\ndescription: d\n---\n# qa\n");
|
||||
fs.symlinkSync(path.join(tmp, "L0"), path.join(root, "qa", "refs"));
|
||||
|
||||
const leaf = path.join("qa", "refs", ...Array(depth).fill("a"), "leaf.md");
|
||||
expect(walkMd(root).map((f) => path.relative(root, f))).toEqual([path.join("qa", "SKILL.md"), leaf]);
|
||||
const bill = buildBill(root);
|
||||
expect(bill.totals.totalMdBytes).toBe(fileBytes(root, "qa", "SKILL.md") + 300);
|
||||
fs.rmSync(tmp, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it("refuses a parent link when the root's own name starts with '..'", () => {
|
||||
if (process.platform === "win32") return;
|
||||
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "context-bill-dotdot-"));
|
||||
@@ -694,6 +717,34 @@ describe("--exact (opt-in measurement; offline here via an injected fetch)", ()
|
||||
fs.rmSync(tmp, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it("sends each real file once and prices every link to it with that measurement", async () => {
|
||||
if (process.platform === "win32") return;
|
||||
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "context-bill-exact-once-"));
|
||||
const checkout = path.join(tmp, "checkout");
|
||||
fs.mkdirSync(path.join(checkout, "shared"), { recursive: true });
|
||||
fs.writeFileSync(path.join(checkout, "SKILL.md"), "---\nname: twin\ndescription: d\n---\n# twin\n");
|
||||
fs.writeFileSync(path.join(checkout, "shared", "guide.md"), "SHARED-GUIDE ".repeat(20));
|
||||
const skills = path.join(tmp, "skills");
|
||||
for (const name of ["a", "b"]) {
|
||||
fs.mkdirSync(path.join(skills, name), { recursive: true });
|
||||
fs.symlinkSync(path.join(checkout, "SKILL.md"), path.join(skills, name, "SKILL.md"));
|
||||
fs.symlinkSync(path.join(checkout, "shared"), path.join(skills, name, "refs"));
|
||||
}
|
||||
|
||||
const calls: { body: string }[] = [];
|
||||
const measured = await measureExactTokens(skills, { model: "m", apiKey: "sk-test", fetchImpl: fakeFetch(calls), egressHome });
|
||||
const sent = calls.map((c) => JSON.parse(c.body).messages[0].content as string);
|
||||
expect(sent.filter((t) => t.startsWith("SHARED-GUIDE")).length).toBe(1);
|
||||
expect(sent.filter((t) => t.includes("# twin")).length).toBe(1);
|
||||
const bill = buildBill(skills, { tokensOf: measured.tokensOf, tokenSource: measured.tokenSource });
|
||||
expect(bill.skills.map((s) => s.name)).toEqual(["a", "b"]);
|
||||
expect([...measured.missedKeys]).toEqual([]);
|
||||
const [a, b] = bill.skills;
|
||||
expect(a.totalMdTokens).toBe(b.totalMdTokens);
|
||||
expect(a.frontmatterTokens).toBe(b.frontmatterTokens);
|
||||
fs.rmSync(tmp, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it("counts anything it could not measure instead of passing it off as measured", async () => {
|
||||
const measured = await measureExactTokens(TREE_A, {
|
||||
model: "m", apiKey: "sk-test", fetchImpl: fakeFetch(), egressHome,
|
||||
|
||||
Reference in new issue
Block a user