From b23b784bcfa20bfda771629c8570a94fd60af860 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 4 Sep 2026 17:01:44 +0000 Subject: [PATCH] fix(relink): canonicalize link targets before the ownership check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-landing review finding: the ownership gate compared readlink output textually against INSTALL_DIR and RENDER_DIR, so two shapes of gstack's OWN entries read as foreign and were left behind on a mode flip — a legacy relative link (`gstack/qa/SKILL.md`, resolved against $PWD instead of the link's directory) and an entry linked against the real path of a symlinked install dir (~/.claude/skills/gstack -> checkout). Both now resolve: relative targets anchor at the link's directory, the directory part is canonicalized with pwd -P (the basename stays verbatim so a dangling managed target is not misread), and both spellings of each root are accepted. Two regression tests. Co-Authored-By: Claude Fable 5.1 --- bin/gstack-relink | 41 +++++++++++++++++++++++++++++++++++------ test/relink.test.ts | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 6 deletions(-) diff --git a/bin/gstack-relink b/bin/gstack-relink index 6ee0d367b..743f71893 100755 --- a/bin/gstack-relink +++ b/bin/gstack-relink @@ -59,27 +59,56 @@ RENDER_DIR="${GSTACK_USER_RENDER_DIR:-${GSTACK_HOME:-$HOME/.gstack}/render/claud # Anything else — a foreign symlink, a real dir with a real SKILL.md and no # marker, or an entry whose readlink fails — is FOREIGN: never deleted, never # linked over, reported on stderr. +# The install/render roots as written AND as resolved: a standalone relink +# detects INSTALL_DIR as ~/.claude/skills/gstack, which may itself be a symlink +# to a checkout, while setup linked entries against the checkout's real path. +# Both spellings are ours. (Roots stay quoted inside the case patterns, so a +# glob character or space in a path is matched literally.) +_INSTALL_REAL="$(cd "$INSTALL_DIR" 2>/dev/null && pwd -P || printf '%s' "$INSTALL_DIR")" +_RENDER_REAL="$(cd "$RENDER_DIR" 2>/dev/null && pwd -P || printf '%s' "$RENDER_DIR")" _target_is_ours() { - # $1 = a path that readlink resolved; ours when it lives under our roots. + # $1 = an ABSOLUTE path a symlink resolves to; ours when it lives under one + # of our roots. "$ROOT"/* requires the separator, so /home/u/gstack2/x never + # matches a /home/u/gstack root. case "$1" in - "$INSTALL_DIR"/*|"$RENDER_DIR"/*) return 0 ;; + "$INSTALL_DIR"/*|"$RENDER_DIR"/*|"$_INSTALL_REAL"/*|"$_RENDER_REAL"/*) return 0 ;; *) return 1 ;; esac } +# readlink of a RELATIVE symlink (older installs wrote `gstack/qa/SKILL.md`) +# is relative to the link's own directory, not to $PWD. Anchor it there, then +# canonicalize the DIRECTORY part (pwd -P) so `..` segments and symlinked +# path components (a `gstack` alias dir, a symlinked install) compare against +# the real roots. The basename is kept verbatim: canonicalizing it would follow +# the final link and turn every dangling target into "not ours". +_link_target_abs() { + local link="$1" dest d b d_real + dest="$(readlink "$link" 2>/dev/null || true)" + [ -n "$dest" ] || return 1 + case "$dest" in + /*) ;; + *) dest="$(dirname "$link")/$dest" ;; + esac + d="$(dirname "$dest")"; b="$(basename "$dest")" + if d_real="$(cd "$d" 2>/dev/null && pwd -P)"; then + printf '%s\n' "$d_real/$b" + else + printf '%s\n' "$dest" + fi +} + _entry_is_ours() { local entry="$1" dest if [ -L "$entry" ]; then - dest="$(readlink "$entry" 2>/dev/null || true)" - [ -n "$dest" ] || return 1 + dest="$(_link_target_abs "$entry")" || return 1 _target_is_ours "$dest" return $? fi if [ -d "$entry" ]; then [ -f "$entry/.gstack-owned" ] && return 0 if [ -L "$entry/SKILL.md" ]; then - dest="$(readlink "$entry/SKILL.md" 2>/dev/null || true)" - [ -n "$dest" ] || return 1 + dest="$(_link_target_abs "$entry/SKILL.md")" || return 1 _target_is_ours "$dest" return $? fi diff --git a/test/relink.test.ts b/test/relink.test.ts index f1174bfc6..fe495c960 100644 --- a/test/relink.test.ts +++ b/test/relink.test.ts @@ -764,6 +764,38 @@ describe('gstack-relink ownership gate (#2119)', () => { expect(out).not.toContain('skipped'); }); + test('a legacy RELATIVE symlink into the install (gstack/qa/SKILL.md) is ours and is cleaned', () => { + setupMockInstall(['qa']); + // Older setups wrote relative links; the skills dir sits beside the install + // dir named `gstack`, so `gstack/qa/SKILL.md` resolves into INSTALL_DIR. + const installAlias = path.join(skillsDir, 'gstack'); + fs.symlinkSync(installDir, installAlias); + fs.mkdirSync(path.join(skillsDir, 'gstack-qa')); + fs.symlinkSync('../gstack/qa/SKILL.md', path.join(skillsDir, 'gstack-qa', 'SKILL.md')); + setPrefix('false'); + const out = relink(); + expect(fs.existsSync(path.join(skillsDir, 'gstack-qa'))).toBe(false); + expect(out).not.toContain('skipped'); + }); + + test('an entry linked against the REAL path of a symlinked install dir is ours', () => { + setupMockInstall(['qa']); + const realInstall = fs.realpathSync(installDir); + const linkInstall = path.join(tmpDir, 'install-link'); + fs.symlinkSync(realInstall, linkInstall); + fs.mkdirSync(path.join(skillsDir, 'gstack-qa')); + fs.symlinkSync(path.join(realInstall, 'qa', 'SKILL.md'), path.join(skillsDir, 'gstack-qa', 'SKILL.md')); + // relink detects the install through the symlinked spelling. + run(`${path.join(installDir, 'bin', 'gstack-config')} set skill_prefix false`, { + GSTACK_INSTALL_DIR: linkInstall, GSTACK_SKILLS_DIR: skillsDir, + }); + const out = run(`${path.join(installDir, 'bin', 'gstack-relink')} 2>&1`, { + GSTACK_INSTALL_DIR: linkInstall, GSTACK_SKILLS_DIR: skillsDir, + }); + expect(fs.existsSync(path.join(skillsDir, 'gstack-qa'))).toBe(false); + expect(out).not.toContain('skipped'); + }); + test('a real-file copy WITHOUT the marker is foreign and survives', () => { setupMockInstall(['qa']); setPrefix('false');