mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-06 00:44:07 +01:00
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>
This commit is contained in:
1 parent
c23d7d48fe
commit
9bf11e738e
3 files changed
+179
-25
No files matched your search
@@ -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=<its own 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).
|
||||
|
||||
+90
-25
@@ -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 -> <checkout>/review/SKILL.md`,
|
||||
* `office-hours/sections -> <checkout>/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<string>();
|
||||
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();
|
||||
}
|
||||
|
||||
|
||||
@@ -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");
|
||||
|
||||
Reference in new issue
Block a user