mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-15 01:15:29 +02:00
fix: pre-landing review fixes for the Aside-first branch
Review army + adversarial passes (Claude and Codex) on the merged branch:
setup
- _prune_stale_generated scans the host dirs too (the generator already
removed the render before setup ran, so the host branch was dead), skips
symlinks in the render tree (rm -rf on a slash-terminated link empties its
target), removes a host symlink only when it resolves into gstack, cleans a
bannered real dir through _cleanup_weak_dir, recognizes frontmatter-renamed
skills, and logs through log. The always-run codex render passes every host
dir that may link to it.
- NEEDS_BUILD checks all three binaries (with $_EXE) and lib/ sources; the
browser hint and the bootstrap summary honor GSTACK_SKIP_ASIDE, treat a
requested skip as a request, and derive one skill list.
lib/aside-render.ts + bin/gstack-render.ts
- The loopback server carries a per-render secret path, checks containment on
the real path (symlink escapes are 403), and rejects malformed encoding.
- Inline eval results are one base64 line, so page text cannot forge
ASIDE_DIR= or the sentinel; the last ASIDE_DIR wins.
- runProc escalates SIGTERM to SIGKILL, bounds every wait, and clears every
timer (an uncleared one kept gstack-render alive after printing OK).
- renderTmpDir refuses a shared /tmp name owned by someone else; the work dir
and server are created inside try; goto's budget follows the render budget.
- probeAside classifies a present-but-failing CLI as ASIDE_NOT_RUNNING like
the skills' bash probe; render() retries on gstack's own browser when Aside
could not start or its private CDP bridge is gone (never on a page error
or a timeout of a running script); the CLI reports the engine that actually
rendered, exits 0 on --help, rejects non-numeric flags, documents
--wait-timeout, fences EVAL/PAGE_ERRORS as untrusted content, and names the
daemon's cookie-import JS lock remedy.
- The browse path passes --scale only when asked (a scale change rebuilds
the daemon context) and restores the viewport after a sized screenshot.
resolvers / templates
- The bash probe honors GSTACK_SKIP_ASIDE and has a perl deadline on stock
macOS; .local is no longer LOCAL (mDNS); same-origin filters compare parsed
origins; link status is HEAD-checked only on LOCAL targets; every
aside exec goes through the receipted _aside_exec prelude
({{ASIDE_EXEC_PRELUDE}}), including nine template blocks that called it
bare; the design sketch and diagram staging use private directories.
- The generator prunes only bannered renders and never a host whose
generation failed.
Docs, stale comments and dead code cleaned; goldens re-rendered; tests
updated and added for every behavior above.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5.1
parent
ea61bd65be
commit
444f8feff8
@@ -3,7 +3,12 @@
|
||||
*
|
||||
* The generator only ever wrote outputs, so a skill deleted from the source
|
||||
* tree stayed rendered under every host's skills/ dir (and setup kept linking
|
||||
* it). Now a render removes `gstack-*` dirs it did not write.
|
||||
* it). Now a render removes `gstack-*` dirs it did not write — but only those
|
||||
* carrying the generated banner (proof it was ours), never after a host's
|
||||
* generation failed (a partial rendered set must not delete the rest), never
|
||||
* under --dry-run (a freshness check writes nothing and deletes nothing), and
|
||||
* never in ANOTHER host's skills/ dir (a `--host codex` run has no rendered
|
||||
* set for factory, so it has no basis to judge factory's entries).
|
||||
*/
|
||||
import { describe, test, expect } from 'bun:test';
|
||||
import { spawnSync } from 'child_process';
|
||||
@@ -12,23 +17,83 @@ import * as os from 'os';
|
||||
import * as path from 'path';
|
||||
|
||||
const ROOT = path.resolve(import.meta.dir, '..');
|
||||
const BANNER = '<!-- AUTO-GENERATED from SKILL.md.tmpl — do not edit directly -->';
|
||||
|
||||
/** A stale RENDER: the generator's banner is the proof of ownership the prune requires. */
|
||||
function staleRender(name: string, body: string): string {
|
||||
return `---\nname: ${name}\n---\n${BANNER}\n${body}\n`;
|
||||
}
|
||||
|
||||
function gen(out: string, ...extra: string[]) {
|
||||
return spawnSync('bun', ['run', 'scripts/gen-skill-docs.ts', '--host', 'codex', '--out-dir', out, ...extra], { cwd: ROOT, encoding: 'utf-8', timeout: 180_000 });
|
||||
}
|
||||
|
||||
describe('gen-skill-docs stale-render prune', () => {
|
||||
test('a gstack-* dir for a skill that no longer exists is removed; the sidecar symlink and real skills stay', () => {
|
||||
test('a gstack-* render for a skill that no longer exists is removed; the sidecar symlink, real skills, un-bannered gstack-* dirs, and other hosts\' trees stay', () => {
|
||||
const out = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-prune-'));
|
||||
const skills = path.join(out, '.agents', 'skills');
|
||||
fs.mkdirSync(path.join(skills, 'gstack-retired-zzz'), { recursive: true });
|
||||
fs.writeFileSync(path.join(skills, 'gstack-retired-zzz', 'SKILL.md'), '---\nname: gstack-retired-zzz\n---\nstale\n');
|
||||
fs.writeFileSync(path.join(skills, 'gstack-retired-zzz', 'SKILL.md'), staleRender('gstack-retired-zzz', 'stale'));
|
||||
// Someone's own skill that happens to use the gstack- prefix: no banner, never touched (#2119).
|
||||
fs.mkdirSync(path.join(skills, 'gstack-mine'), { recursive: true });
|
||||
fs.writeFileSync(path.join(skills, 'gstack-mine', 'SKILL.md'), '---\nname: gstack-mine\n---\nuser skill\n');
|
||||
// Two more shapes the banner gate must keep: a dir with no SKILL.md at all,
|
||||
// and a look-alike comment that is NOT the generator's exact banner.
|
||||
fs.mkdirSync(path.join(skills, 'gstack-nomd'), { recursive: true });
|
||||
fs.writeFileSync(path.join(skills, 'gstack-nomd', 'README.md'), 'no SKILL.md here\n');
|
||||
fs.mkdirSync(path.join(skills, 'gstack-lookalike'), { recursive: true });
|
||||
fs.writeFileSync(path.join(skills, 'gstack-lookalike', 'SKILL.md'), '---\nname: gstack-lookalike\n---\n<!-- auto-generated by someone else — do not edit -->\nmine\n');
|
||||
fs.mkdirSync(path.join(skills, 'not-ours'), { recursive: true });
|
||||
fs.symlinkSync(ROOT, path.join(skills, 'gstack'));
|
||||
// Host isolation: a bannered stale-looking render under ANOTHER host's tree
|
||||
// in the same out-dir is not a codex run's to prune (or to touch at all).
|
||||
const factorySkills = path.join(out, '.factory', 'skills');
|
||||
fs.mkdirSync(path.join(factorySkills, 'gstack-zzz'), { recursive: true });
|
||||
fs.writeFileSync(path.join(factorySkills, 'gstack-zzz', 'SKILL.md'), staleRender('gstack-zzz', 'stale factory'));
|
||||
try {
|
||||
const r = spawnSync('bun', ['run', 'scripts/gen-skill-docs.ts', '--host', 'codex', '--out-dir', out], { cwd: ROOT, encoding: 'utf-8', timeout: 180_000 });
|
||||
const r = gen(out);
|
||||
expect(r.status).toBe(0);
|
||||
expect(r.stdout).toContain('pruned stale codex render: gstack-retired-zzz');
|
||||
expect(fs.existsSync(path.join(skills, 'gstack-retired-zzz'))).toBe(false);
|
||||
// Exactly one prune in this run: the bannered stale render and nothing else.
|
||||
expect(r.stdout.match(/pruned stale /g)).toHaveLength(1);
|
||||
// Banner gate: three un-bannered gstack-* dirs survive, each named in the log.
|
||||
for (const kept of ['gstack-mine', 'gstack-nomd', 'gstack-lookalike']) {
|
||||
expect(r.stdout).toContain(`kept codex skills/${kept}: not a gstack render (no generated banner)`);
|
||||
}
|
||||
expect(fs.readFileSync(path.join(skills, 'gstack-mine', 'SKILL.md'), 'utf-8')).toContain('user skill');
|
||||
expect(fs.readFileSync(path.join(skills, 'gstack-nomd', 'README.md'), 'utf-8')).toContain('no SKILL.md here');
|
||||
expect(fs.readFileSync(path.join(skills, 'gstack-lookalike', 'SKILL.md'), 'utf-8')).toContain('mine');
|
||||
expect(fs.existsSync(path.join(skills, 'not-ours'))).toBe(true);
|
||||
expect(fs.lstatSync(path.join(skills, 'gstack')).isSymbolicLink()).toBe(true);
|
||||
expect(fs.existsSync(path.join(skills, 'gstack-ship', 'SKILL.md'))).toBe(true);
|
||||
// Host isolation: the factory tree is byte-identical to how we left it, and the log never mentions it.
|
||||
expect(fs.readFileSync(path.join(factorySkills, 'gstack-zzz', 'SKILL.md'), 'utf-8')).toBe(staleRender('gstack-zzz', 'stale factory'));
|
||||
expect(fs.readdirSync(factorySkills)).toEqual(['gstack-zzz']);
|
||||
expect(r.stdout).not.toContain('pruned stale factory');
|
||||
expect(r.stdout).not.toContain('gstack-zzz');
|
||||
} finally {
|
||||
fs.rmSync(out, { recursive: true, force: true });
|
||||
}
|
||||
}, 200_000);
|
||||
|
||||
test('--dry-run never prunes: a bannered stale render stays byte-identical, no SKILL.md is written, and the run reports STALE', () => {
|
||||
const out = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-prune-dry-'));
|
||||
const skills = path.join(out, '.agents', 'skills');
|
||||
const stale = path.join(skills, 'gstack-retired-zzz', 'SKILL.md');
|
||||
fs.mkdirSync(path.dirname(stale), { recursive: true });
|
||||
fs.writeFileSync(stale, staleRender('gstack-retired-zzz', 'stale'));
|
||||
try {
|
||||
const r = gen(out, '--dry-run');
|
||||
// An empty out-dir is stale by definition: dry-run says so and exits 1 instead of writing.
|
||||
expect(r.status).toBe(1);
|
||||
expect(r.stdout).toContain('STALE: ');
|
||||
expect(r.stdout).not.toContain('GENERATED: ');
|
||||
expect(fs.existsSync(path.join(skills, 'gstack-ship', 'SKILL.md'))).toBe(false);
|
||||
// The prune step is skipped wholesale — no deletions, no "kept" verdicts either.
|
||||
expect(r.stdout).not.toContain('pruned stale');
|
||||
expect(r.stdout).not.toContain('kept codex skills/');
|
||||
expect(fs.readFileSync(stale, 'utf-8')).toBe(staleRender('gstack-retired-zzz', 'stale'));
|
||||
} finally {
|
||||
fs.rmSync(out, { recursive: true, force: true });
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user