From 77374d1f0eed05c2ec6c893d55bb3ab57dfb0495 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sun, 16 Aug 2026 13:54:11 -0700 Subject: [PATCH] fix(setup): Windows refresh bypass no longer deletes a user's own skill dirs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #2444 IS_WINDOWS refresh bypass (link_codex/factory/opencode/cursor _skill_dirs) rm -rf's the destination before re-copying — and the host skills dirs are SHARED namespaces, so the gstack* glob can land on a user's OWN real directory (e.g. ~/.cursor/skills/gstack-notes). Every ./setup re-run silently deleted it — the ownership guard the comments still claimed (#2142). The sidecar installers had the same shape against a hand-written skill squatting on the canonical .../skills/gstack root, and create_cursor_runtime_root wiped that root unconditionally on every platform. Same provenance model as bin/gstack-uninstall (#2563): - _owned_for_windows_refresh: a real dir is only replaced when its SKILL.md carries the AUTO-GENERATED banner; symlinks and missing targets always pass. Non-matching dirs are kept and listed to stderr. Wired into all four *_skill_dirs loops. - _sidecar_root_user_owned: a root whose SKILL.md exists WITHOUT the banner is the user's — create_agents_sidecar, create_cursor_sidecar, and create_cursor_runtime_root skip it entirely instead of writing into (or wiping) someone else's skill. A root with no SKILL.md stays presumed ours (the documented install location; old/partial installs look like that). Pinned by a static census (every bypass site must carry its gate) plus behavior fixtures: a bannerless user dir survives the Windows re-run while a bannered install still refreshes, and a squatted sidecar root is left untouched. Co-Authored-By: Claude Fable 5 --- setup | 95 ++++++++++++++++++++--- test/setup-windows-rerun-refresh.test.ts | 99 +++++++++++++++++++++++- 2 files changed, 180 insertions(+), 14 deletions(-) diff --git a/setup b/setup index b61b465ad..4301ce80a 100755 --- a/setup +++ b/setup @@ -104,6 +104,38 @@ _link_or_copy() { fi } +# ─── Ownership gates for the Windows refresh bypass (#2444 → #2142) ───────── +# On Windows a refresh means rm -rf + re-copy (_link_or_copy). The host +# skills dirs are SHARED namespaces (~/.codex/skills, ~/.factory/skills, +# ~/.cursor/skills, ...), so a gstack* glob name can collide with a user's +# OWN real directory (e.g. ~/.cursor/skills/gstack-notes) — deleting it on +# every ./setup re-run is silent data loss. Mirror of bin/gstack-uninstall's +# provenance gate (#2563): an existing REAL skill dir may only be replaced +# when its SKILL.md carries the generated banner. Missing targets and +# symlinks always pass (replacing a link never destroys content); non-dir +# targets pass (file targets live inside gstack-owned roots). +_owned_for_windows_refresh() { + local dst="$1" + if [ ! -e "$dst" ] && [ ! -L "$dst" ]; then return 0; fi + if [ -L "$dst" ]; then return 0; fi + if [ ! -d "$dst" ]; then return 0; fi + grep -q '\n'; + // Generated tree ships two skills. + for (const name of ['gstack-demo', 'gstack-notes']) { + const d = path.join(fake, '.agents', 'skills', name); + fs.mkdirSync(d, { recursive: true }); + fs.writeFileSync(path.join(d, 'SKILL.md'), `${banner}upstream-v2\n`); + } + fs.mkdirSync(skills, { recursive: true }); + // gstack-demo: a prior gstack install (bannered) — must refresh. + fs.mkdirSync(path.join(skills, 'gstack-demo'), { recursive: true }); + fs.writeFileSync(path.join(skills, 'gstack-demo', 'SKILL.md'), `${banner}installed-v1\n`); + // gstack-notes: the USER'S own hand-written skill — must survive. + fs.mkdirSync(path.join(skills, 'gstack-notes'), { recursive: true }); + fs.writeFileSync(path.join(skills, 'gstack-notes', 'SKILL.md'), '# my own notes\n'); + + const r = runInstaller( + '1', + ['_owned_for_windows_refresh', 'link_codex_skill_dirs'], + `link_codex_skill_dirs "${tmp}/gstack" "${skills}"`, + ); + expect(r.status).toBe(0); + expect(fs.readFileSync(path.join(skills, 'gstack-demo', 'SKILL.md'), 'utf-8')).toContain('upstream-v2'); + expect(fs.readFileSync(path.join(skills, 'gstack-notes', 'SKILL.md'), 'utf-8')).toBe('# my own notes\n'); + expect(r.stderr).toContain('left in place'); + expect(r.stderr).toContain('gstack-notes'); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('IS_WINDOWS=1: create_agents_sidecar refuses a user-owned root and writes nothing into it', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-owned-sidecar-')); + try { + const fake = path.join(tmp, 'gstack'); + fs.mkdirSync(path.join(fake, 'bin'), { recursive: true }); + fs.writeFileSync(path.join(fake, 'bin', 'tool.sh'), 'v1\n'); + // The user's own skill squats on .agents/skills/gstack. + const root = path.join(fake, '.agents', 'skills', 'gstack'); + fs.mkdirSync(root, { recursive: true }); + fs.writeFileSync(path.join(root, 'SKILL.md'), '# hand-written\n'); + + const vars = `SOURCE_GSTACK_DIR="${fake}"`; + const r = runInstaller( + '1', + ['_sidecar_root_user_owned', 'create_agents_sidecar'], + `create_agents_sidecar "${fake}"`, + vars, + ); + expect(r.status).toBe(0); + expect(r.stderr).toContain('left in place'); + expect(fs.existsSync(path.join(root, 'bin'))).toBe(false); + expect(fs.readFileSync(path.join(root, 'SKILL.md'), 'utf-8')).toBe('# hand-written\n'); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); }); interface RunResult { @@ -74,6 +159,9 @@ function runInstaller( `IS_WINDOWS=${isWindows}`, extraVars, extractFn('_link_or_copy'), + // Ownership gates (#2142) — dependencies of every installer under test. + extractFn('_owned_for_windows_refresh'), + extractFn('_sidecar_root_user_owned'), ...fns.map(extractFn), invocation, ].join('\n'); @@ -90,22 +178,25 @@ describe('setup: Windows re-run refresh — behavior fixture (#2444)', () => { const demo = path.join(fake, '.agents', 'skills', 'gstack-demo'); fs.mkdirSync(demo, { recursive: true }); fs.mkdirSync(skills, { recursive: true }); - fs.writeFileSync(path.join(demo, 'SKILL.md'), 'v1-original\n'); + // Generated SKILL.md files always carry the banner — the #2142 + // ownership gate keys the Windows refresh on it. + const banner = '\n'; + fs.writeFileSync(path.join(demo, 'SKILL.md'), `${banner}v1-original\n`); // First run: installs the copy. let r = runInstaller('1', ['link_codex_skill_dirs'], `link_codex_skill_dirs "${fake}" "${skills}"`); expect(r.status).toBe(0); const installed = path.join(skills, 'gstack-demo', 'SKILL.md'); - expect(fs.readFileSync(installed, 'utf-8')).toBe('v1-original\n'); + expect(fs.readFileSync(installed, 'utf-8')).toBe(`${banner}v1-original\n`); expect(fs.lstatSync(path.join(skills, 'gstack-demo')).isSymbolicLink()).toBe(false); // Upstream ships a change (the git pull). - fs.writeFileSync(path.join(demo, 'SKILL.md'), 'v2-UPDATED\n'); + fs.writeFileSync(path.join(demo, 'SKILL.md'), `${banner}v2-UPDATED\n`); // Second run: pre-#2444 this was a silent no-op on Windows. r = runInstaller('1', ['link_codex_skill_dirs'], `link_codex_skill_dirs "${fake}" "${skills}"`); expect(r.status).toBe(0); - expect(fs.readFileSync(installed, 'utf-8')).toBe('v2-UPDATED\n'); + expect(fs.readFileSync(installed, 'utf-8')).toBe(`${banner}v2-UPDATED\n`); } finally { fs.rmSync(tmp, { recursive: true, force: true }); }