mirror of https://github.com/garrytan/gstack.git
fix(plan-tune): project-local question preferences never matched — hook used basename, writers use gstack-slug
`slugFromCwd()` in the AskUserQuestion PreToolUse hook returned
`path.basename(cwd)`. Every writer of project-local state uses the slug from
`bin/gstack-slug`, which derives it from the git remote (owner-repo) and only
falls back to basename when the repo has NO remote configured:
plan-tune/SKILL.md: _PROJ="${GSTACK_HOME:-$HOME/.gstack}/projects/${SLUG}"
hook: <stateRoot>/projects/<basename>/question-preferences.json
In any repo with a remote the two disagree, so the hook read a directory that
does not exist. Observed on a real machine:
gstack-slug → Shmulyitz-tristatelvs (what /plan-tune writes)
basename → tristatelvs (what the hook read)
~/.gstack/projects/ → Shmulyitz-tristatelvs, Shmulyitz-Tristate-pim,
Shmulyitz-tristate-vendure, millwoods-registry
Every bucket is remote-derived; no basename-named directory existed at all.
Impact: project-local preferences were silently inert, and the documented
project > global precedence (D8) collapsed to global-only. A user setting a
per-project never-ask preference got no enforcement and no error.
Fix: resolve the same slug gstack-slug does by reading its on-disk cache
(`<root>/slug-cache/<abs-path-with-slashes-as-underscores>`), falling back to
basename on a cache miss — which mirrors gstack-slug's own no-remote
fallback. This is a single file read, so the hot-path constraint that
motivated the basename shortcut still holds; we do not shell out to git. The
cached value is re-sanitized to the [a-zA-Z0-9._-] invariant on read because
it becomes a path segment.
Why it survived: every fixture in question-preference-hook.test.ts used a cwd
whose basename happened to equal the slug, so the mismatch could not appear.
The two added tests pin the real contract — the first FAILS on the old code
(expected "deny", received "defer"), the second covers the no-remote fallback.
Verified: 29 pass / 0 fail across question-preference-hook and
memory-cache-injection. Also confirmed end-to-end against a real slug cache.
This commit is contained in:
parent
a3259400a3
commit
21bcb1c296
|
|
@ -282,11 +282,43 @@ function extractRecommended(
|
|||
}
|
||||
|
||||
function slugFromCwd(cwd: string | undefined): string {
|
||||
// Mirror gstack-slug's basename fallback. The full slug resolver shells out
|
||||
// to git, which is too expensive on a hot hook path; the basename is close
|
||||
// enough for preference lookup (preferences are keyed by question_id, slug
|
||||
// is just the directory bucket).
|
||||
// Resolve the SAME slug that bin/gstack-slug produces, because that is the
|
||||
// bucket /plan-tune writes project-local preferences into
|
||||
// (~/.gstack/projects/<slug>/question-preferences.json).
|
||||
//
|
||||
// gstack-slug derives the slug from the git remote (owner-repo, e.g.
|
||||
// "garrytan-gstack") and only falls back to basename when the repo has NO
|
||||
// remote configured. Using basename unconditionally — the previous behaviour
|
||||
// — therefore looked up a directory that does not exist in any repo with a
|
||||
// remote, so project-local preferences could never be found and the
|
||||
// project > global precedence (D8) silently collapsed to global-only.
|
||||
//
|
||||
// We do NOT shell out to git: that is too expensive on this hot hook path,
|
||||
// which is why the basename shortcut existed. Instead we read gstack-slug's
|
||||
// own on-disk cache, keyed by the absolute path with '/' replaced by '_'.
|
||||
// That is a single file read, and gstack-slug populates it on first run.
|
||||
// On a cache miss we fall back to basename, matching gstack-slug's own
|
||||
// no-remote fallback.
|
||||
if (!cwd) return 'unknown';
|
||||
|
||||
const cacheKey = cwd.replace(/\//g, '_');
|
||||
// stateRoot() honours GSTACK_STATE_ROOT/GSTACK_HOME (used by tests);
|
||||
// gstack-slug itself always writes under $HOME/.gstack. Check both.
|
||||
const candidates = [
|
||||
path.join(stateRoot(), 'slug-cache', cacheKey),
|
||||
path.join(os.homedir(), '.gstack', 'slug-cache', cacheKey),
|
||||
];
|
||||
for (const cachePath of candidates) {
|
||||
try {
|
||||
// Re-sanitize on read: gstack-slug promises the [a-zA-Z0-9._-] invariant
|
||||
// but a hand-edited cache file could violate it, and this value becomes
|
||||
// a path segment.
|
||||
const cached = fs.readFileSync(cachePath, 'utf-8').trim().replace(/[^a-zA-Z0-9._-]/g, '');
|
||||
if (cached) return cached;
|
||||
} catch {
|
||||
// miss → try next candidate, then basename
|
||||
}
|
||||
}
|
||||
return path.basename(cwd);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -493,3 +493,74 @@ describe('auto-decided event tagging', () => {
|
|||
expect(fs.existsSync(markerPath)).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
// ----------------------------------------------------------------------
|
||||
// Slug resolution (project-local preference bucket)
|
||||
// ----------------------------------------------------------------------
|
||||
|
||||
describe('slug resolution matches gstack-slug', () => {
|
||||
// Regression: the hook used path.basename(cwd) while /plan-tune writes
|
||||
// project-local preferences under the gstack-slug value, which is derived
|
||||
// from the git remote (owner-repo). In any repo WITH a remote the two
|
||||
// disagree, so project-local preferences were never found and D8 precedence
|
||||
// silently collapsed to global-only. The earlier fixtures all used a cwd
|
||||
// whose basename happened to equal the slug, which is why this survived.
|
||||
|
||||
test('uses the cached gstack-slug value, not the cwd basename', () => {
|
||||
const remoteSlug = 'owner-reponame';
|
||||
const repoDir = path.join(stateRoot, 'reponame');
|
||||
fs.mkdirSync(repoDir, { recursive: true });
|
||||
|
||||
// gstack-slug's cache: key is the absolute path with '/' -> '_'
|
||||
const cacheDir = path.join(stateRoot, 'slug-cache');
|
||||
fs.mkdirSync(cacheDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(cacheDir, repoDir.replace(/\//g, '_')), remoteSlug);
|
||||
|
||||
// Preference lives ONLY under the remote-derived slug bucket.
|
||||
fs.mkdirSync(path.join(stateRoot, 'projects', remoteSlug), { recursive: true });
|
||||
fs.writeFileSync(
|
||||
path.join(stateRoot, 'projects', remoteSlug, 'question-preferences.json'),
|
||||
JSON.stringify({ 'test-q': 'never-ask' }),
|
||||
);
|
||||
|
||||
const r = runHook({
|
||||
session_id: 'slug-1',
|
||||
tool_name: 'AskUserQuestion',
|
||||
tool_use_id: 'tu-slug-1',
|
||||
tool_input: {
|
||||
questions: [
|
||||
{ question: '<gstack-qid:test-q> Need approval?', options: ['A) Yes (recommended)', 'B) No'] },
|
||||
],
|
||||
},
|
||||
}, repoDir);
|
||||
|
||||
// Enforcement fired => the hook looked in the remote-derived bucket.
|
||||
expect(r.status).toBe(0);
|
||||
expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('deny');
|
||||
});
|
||||
|
||||
test('falls back to basename when no slug-cache entry exists', () => {
|
||||
// Mirrors gstack-slug's own no-remote fallback.
|
||||
const repoDir = path.join(stateRoot, 'no-remote-repo');
|
||||
fs.mkdirSync(repoDir, { recursive: true });
|
||||
fs.mkdirSync(path.join(stateRoot, 'projects', 'no-remote-repo'), { recursive: true });
|
||||
fs.writeFileSync(
|
||||
path.join(stateRoot, 'projects', 'no-remote-repo', 'question-preferences.json'),
|
||||
JSON.stringify({ 'test-q': 'never-ask' }),
|
||||
);
|
||||
|
||||
const r = runHook({
|
||||
session_id: 'slug-2',
|
||||
tool_name: 'AskUserQuestion',
|
||||
tool_use_id: 'tu-slug-2',
|
||||
tool_input: {
|
||||
questions: [
|
||||
{ question: '<gstack-qid:test-q> Need approval?', options: ['A) Yes (recommended)', 'B) No'] },
|
||||
],
|
||||
},
|
||||
}, repoDir);
|
||||
|
||||
expect(r.status).toBe(0);
|
||||
expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('deny');
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in New Issue