From 9bf11e738eb94875692f97dcc324c77e37c19f0a Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 25 Oct 2026 22:59:08 +0000 Subject: [PATCH] fix(context-bill): follow symlinked SKILL.md files and skill dirs so a real install is billed (GSTA-97) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- CHANGELOG.md | 13 +++++ lib/context-bill.ts | 115 +++++++++++++++++++++++++++++--------- test/context-bill.test.ts | 76 +++++++++++++++++++++++++ 3 files changed, 179 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ba97bc0e7..905130e6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,18 @@ # Changelog +## [Unreleased] + +**`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. Each real directory is walked once (cycle guard), 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. `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. Both cases 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. + ## [1.91.221.0] - 2026-10-25 **The session-start updater no longer skips because of its own session's workflow lease, and the autostash tests stop failing their first pass in Free Tests.** Since #3385, `gstack-session-update` skips the fast-forward while another session holds a workflow lease, and it means to ignore the session that started it (`--exclude-ancestry-of`). The check runs in the forked background updater, though, and passed the hook's own `$$`, which has exited by then, so the ancestry walk excluded nothing. A Claude Code process that had already run a skill (and so held a lease) and then started a session again in the same process (`/clear`, resume or compaction) logged `SKIP leased pid=` and never updated. `test/session-update-autostash.test.ts` hit the same gap plus a fixture leak: its first two fixtures spawned the updater with the whole parent environment, so it read the test preload's `GSTACK_LEASE_DIR`, which every test file in the same Bun process shares. A lease another file left there gave "Received: SKIP leased pid=3196/1m" where UPDATING or PULL_FAILED was expected, in 13 Free Tests runs since #3385 merged (each passed on the serial retry). diff --git a/lib/context-bill.ts b/lib/context-bill.ts index b264e72c9..7be12a313 100644 --- a/lib/context-bill.ts +++ b/lib/context-bill.ts @@ -195,26 +195,84 @@ function parseFrontmatter(text: string): { bytes: number; keys: string[]; block: return { bytes: Buffer.byteLength(block, "utf8"), keys, block }; } +/** + * What a directory entry is once symlinks are followed. Installed skills are + * symlinks into the gstack checkout (`review/SKILL.md -> /review/SKILL.md`, + * `office-hours/sections -> /office-hours/sections`), so a plain + * Dirent type check sees none of them. Dangling links and anything that is + * neither a file nor a directory are null. + */ +function entryKind(p: string, e: fs.Dirent): "file" | "dir" | null { + if (e.isFile()) return "file"; + if (e.isDirectory()) return "dir"; + if (!e.isSymbolicLink()) return null; + try { + const st = fs.statSync(p); + return st.isFile() ? "file" : st.isDirectory() ? "dir" : null; + } catch { + return null; + } +} + +/** + * Whether directory `outer` contains (or is) `inner`, both realpaths. A + * followed directory symlink that contains the tree being walked (`..`, `/`, + * `$HOME`) would climb out of that tree, so walkers refuse it. + */ +function encloses(outer: string, inner: string): boolean { + const rel = path.relative(outer, inner); + return rel === "" || (!rel.startsWith("..") && !path.isAbsolute(rel)); +} + /** * Every .md file under a tree, for the on-disk total and for exact * measurement. Skips node_modules and dot-directories: a skills tree that is * also a repo checkout (dev symlink installs) would otherwise bill its - * dependency tree and CI state as skill content. + * dependency tree and CI state as skill content. A child repo checkout (the + * `gstack/` link inside ~/.claude/skills) is skipped for the same reason. + * + * Symlinked files and directories are followed, and every returned path stays + * the lexical path under `dir`, so buildBill and --exact key the same files. + * Each real directory is walked once (cycle guard), and a link to a directory + * that encloses `dir` is never followed. */ export function walkMd(dir: string): string[] { const out: string[] = []; - let entries: fs.Dirent[]; + let rootReal: string; try { - entries = fs.readdirSync(dir, { withFileTypes: true }); + rootReal = fs.realpathSync(dir); } catch { return out; } - for (const e of entries) { - if (e.name.startsWith(".") || e.name === "node_modules") continue; - const p = path.join(dir, e.name); - if (e.isDirectory()) out.push(...walkMd(p)); - else if (e.isFile() && e.name.endsWith(".md")) out.push(p); - } + const visited = new Set([rootReal]); + const walk = (current: string) => { + let entries: fs.Dirent[]; + try { + entries = fs.readdirSync(current, { withFileTypes: true }); + } catch { + return; + } + for (const e of entries) { + 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")) out.push(p); + continue; + } + if (kind !== "dir" || fs.existsSync(path.join(p, ".git"))) continue; + let real: string; + try { + real = fs.realpathSync(p); + } catch { + continue; + } + if (visited.has(real) || encloses(real, rootReal)) continue; + visited.add(real); + walk(p); + } + }; + walk(dir); return out; } @@ -275,8 +333,8 @@ export function parseSkill(skillDir: string, name: string, tokensOf: TokensOf = // Foreign-host skill files sitting next to SKILL.md. const foreignFiles: { path: string; bytes: number; tokens: number }[] = []; for (const entry of fs.readdirSync(skillDir, { withFileTypes: true })) { - if (entry.isFile() && FOREIGN_SKILL_FILE.test(entry.name)) { - const abs = path.join(skillDir, entry.name); + const abs = path.join(skillDir, entry.name); + if (FOREIGN_SKILL_FILE.test(entry.name) && entryKind(abs, entry) === "file") { const bytes = bytesOf(abs) ?? 0; foreignFiles.push({ path: entry.name, bytes, tokens: tokensOf(abs, bytes) }); } @@ -333,13 +391,23 @@ export function parseSkill(skillDir: string, name: string, tokensOf: TokensOf = * repo checkout (`gstack/`, with .git). Its nested SKILL.md files are the * repo's sources, not installed skills of the tree being billed. * - * Directory symlinks are followed (setup's shell glob follows them, so a - * symlinked skill like connect-chrome/ is real scanner load); a realpath - * seen-set breaks cycles. + * Symlinks are followed. Directory links: setup's shell glob follows them, so + * a symlinked skill like connect-chrome/ is real scanner load. File links: + * setup installs every skill as a real directory whose SKILL.md is a symlink + * into the checkout, so skipping them billed a real install as two skills. + * A realpath seen-set breaks cycles, a link to a directory that encloses the + * root is never followed, and every returned path stays under the root. */ export function findSkillDirs(root: string): string[] { const out: string[] = []; const visited = new Set(); + const rootDir = path.resolve(root); + let rootReal: string; + try { + rootReal = fs.realpathSync(rootDir); + } catch { + return out; + } const walk = (dir: string, isRoot: boolean) => { let entries: fs.Dirent[]; try { @@ -347,7 +415,7 @@ export function findSkillDirs(root: string): string[] { } catch { return; } - if (entries.some((e) => e.isFile() && e.name === "SKILL.md")) { + if (entries.some((e) => e.name === "SKILL.md" && entryKind(path.join(dir, e.name), e) === "file")) { out.push(dir); // Two symlinked paths to the same skill dir are BOTH billed (each is // real scanner load); only container recursion below is cycle-guarded. @@ -365,20 +433,17 @@ export function findSkillDirs(root: string): string[] { for (const e of entries) { if (e.name.startsWith(".") || e.name === "node_modules") continue; const child = path.join(dir, e.name); - let isDir = e.isDirectory(); - if (!isDir && e.isSymbolicLink()) { - try { - isDir = fs.statSync(child).isDirectory(); - } catch { - continue; // dangling symlink - } - } - if (!isDir) continue; + if (entryKind(child, e) !== "dir") continue; if (fs.existsSync(path.join(child, ".git"))) continue; // repo checkout, not a skill + try { + if (encloses(fs.realpathSync(child), rootReal)) continue; // would climb out of the root + } catch { + continue; + } walk(child, false); } }; - walk(path.resolve(root), true); + walk(rootDir, true); return out.sort(); } diff --git a/test/context-bill.test.ts b/test/context-bill.test.ts index 82afda6d9..81c663149 100644 --- a/test/context-bill.test.ts +++ b/test/context-bill.test.ts @@ -232,6 +232,82 @@ describe("upstream fix b: installed-tree layout (repo-checkout subdir skip)", () fs.rmSync(tmp, { recursive: true, force: true }); }); + it("bills an installed tree whose SKILL.md files and section dirs are symlinks into the checkout", () => { + if (process.platform === "win32") return; + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "context-bill-installed-")); + // The gstack checkout lives outside the skills root, as on a real install. + const checkout = path.join(tmp, "checkout"); + fs.mkdirSync(path.join(checkout, ".git"), { recursive: true }); + for (const name of ["review", "office-hours"]) { + fs.mkdirSync(path.join(checkout, name, "sections"), { recursive: true }); + fs.writeFileSync( + path.join(checkout, name, "SKILL.md"), + `---\nname: ${name}\ndescription: ${name} skill\n---\n# ${name}\n${"body ".repeat(200)}\n`, + ); + fs.writeFileSync(path.join(checkout, name, "sections", "flow.md"), "s".repeat(3_000)); + } + // setup's layout: a real dir per skill, SKILL.md and sections/ as links, + // plus a gstack/ link to the checkout itself. + const skills = path.join(tmp, "skills"); + for (const name of ["review", "office-hours"]) { + fs.mkdirSync(path.join(skills, name), { recursive: true }); + fs.symlinkSync(path.join(checkout, name, "SKILL.md"), path.join(skills, name, "SKILL.md")); + fs.symlinkSync(path.join(checkout, name, "sections"), path.join(skills, name, "sections")); + } + fs.symlinkSync(checkout, path.join(skills, "gstack")); + // Negative controls: a dangling SKILL.md link and a SKILL.md link to a + // directory are not skills. + fs.mkdirSync(path.join(skills, "dangling")); + fs.symlinkSync(path.join(checkout, "missing", "SKILL.md"), path.join(skills, "dangling", "SKILL.md")); + fs.mkdirSync(path.join(skills, "dirlink")); + fs.symlinkSync(path.join(checkout, "review", "sections"), path.join(skills, "dirlink", "SKILL.md")); + + const bill = buildBill(skills); + expect(bill.skills.map((s) => s.name).sort()).toEqual(["office-hours", "review"]); + for (const s of bill.skills) { + const target = path.join(checkout, s.name); + expect(s.frontmatterBytes).toBe(frontmatterBytes(path.join(target, "SKILL.md"))); + expect(s.skillMdBytes).toBe(fileBytes(target, "SKILL.md")); + // The linked sections/ dir is part of the skill's on-disk total. + expect(s.totalMdBytes).toBe(fileBytes(target, "SKILL.md") + fileBytes(target, "sections", "flow.md")); + } + // 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); + expect(walkMd(skills).map((f) => path.relative(skills, f)).sort()).toEqual([ + 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 }); + }); + + it("never follows a link that loops or climbs out of the root", () => { + if (process.platform === "win32") return; + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "context-bill-escape-")); + // A sibling skill outside the root, reachable only through a parent link. + fs.mkdirSync(path.join(tmp, "outside")); + fs.writeFileSync(path.join(tmp, "outside", "SKILL.md"), "---\nname: outside\ndescription: d\n---\n# o\n"); + fs.writeFileSync(path.join(tmp, "outside", "SECRET.md"), "x".repeat(10_000)); + const root = path.join(tmp, "skills"); + fs.mkdirSync(path.join(root, "qa", "references"), { recursive: true }); + fs.writeFileSync(path.join(root, "qa", "SKILL.md"), "---\nname: qa\ndescription: d\n---\n# qa\n"); + fs.writeFileSync(path.join(root, "qa", "references", "guide.md"), "g".repeat(500)); + fs.symlinkSync(root, path.join(root, "self")); + fs.symlinkSync(tmp, path.join(root, "parent")); + fs.symlinkSync(path.join(root, "qa"), path.join(root, "qa", "references", "loop")); + fs.symlinkSync(tmp, path.join(root, "qa", "references", "up")); + + expect(findSkillDirs(root).map((d) => path.relative(root, d))).toEqual(["qa"]); + expect(walkMd(path.join(root, "qa")).map((f) => path.relative(root, f)).sort()).toEqual([ + path.join("qa", "SKILL.md"), + path.join("qa", "references", "guide.md"), + ]); + const bill = buildBill(root); + expect(bill.totals.totalMdBytes).toBe(fileBytes(root, "qa", "SKILL.md") + fileBytes(root, "qa", "references", "guide.md")); + fs.rmSync(tmp, { recursive: true, force: true }); + }); + it("prefers ./skills, then .agents/skills, then .claude/skills, project before user", async () => { const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "context-bill-detect-")); const home = path.join(tmp, "home");