mirror of
https://github.com/garrytan/gstack.git
synced 2026-10-31 14:06:50 +01:00
Q3: setup stops auto-installing the AskUserQuestion preference hook in Conductor and removes one it added
The PreToolUse question-preference hook breaks Conductor's native AskUserQuestion round-trip even when it only defers (#2207), and setup auto-installed it on every Conductor workspace setup. Now a Conductor setup without an explicit opt-in never installs it and removes an existing one (the question-log and AUQ-error hooks stay), printing how to keep it (gstack-config set plan_tune_hooks yes). An explicit opt-in still installs it. gstack-settings-hook remove-source gains --event to scope the removal. Test migration: setup-plan-tune-hooks-noninteractive pinned the retired Conductor auto-opt-in. Co-authored-by: Phil Hie <philhie@outlook.com>
This commit is contained in:
1 parent
4bc9b0686f
commit
e6f4d0ffb1
4 files changed
+98
-34
No files matched your search
@@ -14,7 +14,8 @@
|
||||
# gstack-settings-hook add-event --event <name — see the validator in add-event> \
|
||||
# --command <cmd> --source <tag> [--matcher <regex>] [--timeout <s>]
|
||||
# gstack-settings-hook ensure-event --event ... --command ... --source ... [--matcher ...] [--timeout <s>]
|
||||
# gstack-settings-hook remove-source --source <tag> # removes items the table identifies as <tag>'s, tagged or not
|
||||
# gstack-settings-hook remove-source --source <tag> [--event <name>]
|
||||
# # removes items the table identifies as <tag>'s, tagged or not
|
||||
# gstack-settings-hook diff-event --event ... --command ... --source ... [--matcher ...]
|
||||
# gstack-settings-hook rollback # restore latest backup (single-step undo)
|
||||
# gstack-settings-hook list-sources # show all gstack-tagged hook entries
|
||||
@@ -79,7 +80,7 @@ Usage:
|
||||
gstack-settings-hook remove <hook-command> # legacy SessionStart remove
|
||||
gstack-settings-hook add-event --event <name> --command <cmd> --source <tag> [--matcher <re>] [--timeout <s>]
|
||||
gstack-settings-hook ensure-event --event <name> --command <cmd> --source <tag> [--matcher <re>] [--timeout <s>]
|
||||
gstack-settings-hook remove-source --source <tag> # tagged OR table-identified items of <tag>
|
||||
gstack-settings-hook remove-source --source <tag> [--event <name>] # tagged OR table-identified items of <tag>
|
||||
gstack-settings-hook diff-event --event <name> --command <cmd> --source <tag> [--matcher <re>] [--timeout <s>]
|
||||
gstack-settings-hook prune-stale [--repoint <root>] [--all]
|
||||
gstack-settings-hook rollback
|
||||
@@ -641,10 +642,12 @@ case "$ACTION" in
|
||||
|
||||
remove-source)
|
||||
SOURCE=""
|
||||
RS_EVENT=""
|
||||
shift
|
||||
while [ $# -gt 0 ]; do
|
||||
case "$1" in
|
||||
--source) SOURCE="$2"; shift 2 ;;
|
||||
--event) RS_EVENT="$2"; shift 2 ;;
|
||||
*) echo "unknown flag: $1" >&2; exit 1 ;;
|
||||
esac
|
||||
done
|
||||
@@ -655,9 +658,10 @@ case "$ACTION" in
|
||||
[ -f "$SETTINGS_FILE" ] || exit 0
|
||||
_acquire_lock || exit 5
|
||||
_mutation_env
|
||||
GSTACK_SETTINGS_PATH="$SETTINGS_FILE" GSTACK_SOURCE="$SOURCE" bun -e "$_HOOK_JS_PRELUDE"'gsMain(function () {
|
||||
GSTACK_SETTINGS_PATH="$SETTINGS_FILE" GSTACK_SOURCE="$SOURCE" GSTACK_RS_EVENT="$RS_EVENT" bun -e "$_HOOK_JS_PRELUDE"'gsMain(function () {
|
||||
const settingsPath = process.env.GSTACK_SETTINGS_PATH;
|
||||
const source = process.env.GSTACK_SOURCE;
|
||||
const onlyEvent = process.env.GSTACK_RS_EVENT || "";
|
||||
const loaded = gsLoadSettings(settingsPath);
|
||||
const settings = loaded.settings;
|
||||
if (!settings.hooks) { console.log("OK: removed 0 hook entry/entries tagged source=" + source); process.exit(0); }
|
||||
@@ -677,6 +681,7 @@ case "$ACTION" in
|
||||
// the tag with its last owned item. Callers that need every gstack
|
||||
// item gone still pair this with prune-stale --all.
|
||||
for (const event of Object.keys(settings.hooks)) {
|
||||
if (onlyEvent && event !== onlyEvent) continue; // --event scopes the removal (Q3)
|
||||
const entries = settings.hooks[event];
|
||||
if (!Array.isArray(entries)) continue; // foreign shape: not ours to judge
|
||||
const kept = [];
|
||||
|
||||
@@ -3892,19 +3892,22 @@ if [ "$NO_TEAM_MODE" -ne 1 ] \
|
||||
*) PT_DECISION="prompt" ;;
|
||||
esac
|
||||
|
||||
# Conductor host reliability: the PreToolUse preference hook also carries the
|
||||
# Conductor-prose enforcement (deny the flaky mcp__conductor__AskUserQuestion,
|
||||
# redirect to a prose decision brief). A Conductor workspace setup otherwise
|
||||
# falls through to "prompt" → the non-interactive skip below, leaving Conductor
|
||||
# users without that backstop. Treat Conductor as an implicit opt-in — but
|
||||
# only on the silent fall-through, never overriding an explicit --no-plan-tune-hooks.
|
||||
# Only the true silent fall-through auto-opts-in. An explicit
|
||||
# --plan-tune-hooks=prompt (bin/dev-setup passes exactly this so ephemeral
|
||||
# workspace setups never install) stays "prompt" — this was the bug that
|
||||
# baked worktree hook paths into every Conductor user's settings.json.
|
||||
if [ "$PT_DECISION" = "prompt" ] && [ "$PT_EXPLICIT" -eq 0 ] && { [ -n "${CONDUCTOR_WORKSPACE_PATH:-}" ] || [ -n "${CONDUCTOR_PORT:-}" ]; }; then
|
||||
PT_DECISION="yes"
|
||||
_PT_CONDUCTOR_AUTO=1
|
||||
# Conductor (Q3, #2207): the PreToolUse preference hook breaks Conductor's
|
||||
# native AskUserQuestion round-trip even when it only defers, so setup never
|
||||
# installs it in a Conductor workspace unless the user explicitly opted in,
|
||||
# and removes one an earlier setup added. The other plan-tune hooks are
|
||||
# unaffected.
|
||||
_PT_SKIP_PREF=0
|
||||
if { [ -n "${CONDUCTOR_WORKSPACE_PATH:-}" ] || [ -n "${CONDUCTOR_PORT:-}" ]; } \
|
||||
&& ! { [ "$PT_EXPLICIT" -eq 1 ] && [ "$PT_DECISION" = "yes" ]; }; then
|
||||
_PT_SKIP_PREF=1
|
||||
if [ -n "$("$SETTINGS_HOOK" list-items --event PreToolUse --owned-by plan-tune-cathedral 2>/dev/null)" ]; then
|
||||
if "$SETTINGS_HOOK" remove-source --source plan-tune-cathedral --event PreToolUse >/dev/null 2>&1; then
|
||||
log " removed the AskUserQuestion preference hook: it breaks Conductor's native AskUserQuestion (#2207). Keep it anyway: gstack-config set plan_tune_hooks yes, then ./setup"
|
||||
else
|
||||
log " warning: could not remove the AskUserQuestion preference hook, which breaks Conductor's native AskUserQuestion. Fix: $SETTINGS_HOOK remove-source --source plan-tune-cathedral --event PreToolUse"
|
||||
fi
|
||||
fi
|
||||
fi
|
||||
|
||||
_install_plan_tune_hooks() {
|
||||
@@ -3921,12 +3924,14 @@ if [ "$NO_TEAM_MODE" -ne 1 ] \
|
||||
--command "$PLAN_TUNE_LOG_HOOK" \
|
||||
--source plan-tune-cathedral \
|
||||
--timeout 5 || _pt_install_rc=1
|
||||
"$SETTINGS_HOOK" ensure-event \
|
||||
--event PreToolUse \
|
||||
--matcher '(AskUserQuestion|mcp__.*__AskUserQuestion)' \
|
||||
--command "$PLAN_TUNE_PREF_HOOK" \
|
||||
--source plan-tune-cathedral \
|
||||
--timeout 5 || _pt_install_rc=1
|
||||
if [ "$_PT_SKIP_PREF" -eq 0 ]; then
|
||||
"$SETTINGS_HOOK" ensure-event \
|
||||
--event PreToolUse \
|
||||
--matcher '(AskUserQuestion|mcp__.*__AskUserQuestion)' \
|
||||
--command "$PLAN_TUNE_PREF_HOOK" \
|
||||
--source plan-tune-cathedral \
|
||||
--timeout 5 || _pt_install_rc=1
|
||||
fi
|
||||
# AskUserQuestion-failure prose-fallback reliability hook (OV3:B). Fires only when
|
||||
# an AskUserQuestion call returns an error/missing result; inert on success and
|
||||
# inert if the platform doesn't invoke PostToolUse on tool errors. MUST use its
|
||||
@@ -3958,15 +3963,10 @@ if [ "$NO_TEAM_MODE" -ne 1 ] \
|
||||
log ""
|
||||
log "Plan-tune hooks already installed. Run \`$SETTINGS_HOOK list-sources\` to inspect."
|
||||
elif [ "$PT_DECISION" = "yes" ]; then
|
||||
# Explicit opt-in (flag / env / config) or Conductor implicit opt-in. Non-interactive.
|
||||
# Explicit opt-in (flag / env / config). Non-interactive.
|
||||
if _install_plan_tune_hooks; then
|
||||
log ""
|
||||
if [ "${_PT_CONDUCTOR_AUTO:-0}" -eq 1 ]; then
|
||||
log "AskUserQuestion reliability hooks installed (Conductor detected): decisions"
|
||||
log "render as a prose brief instead of the flaky AskUserQuestion tool. Inspect with /plan-tune."
|
||||
else
|
||||
log "Plan-tune hooks installed. Run /plan-tune anytime to inspect."
|
||||
fi
|
||||
log "Plan-tune hooks installed. Run /plan-tune anytime to inspect."
|
||||
else
|
||||
log ""
|
||||
log " warning: some AskUserQuestion hooks were NOT registered (settings lock contention or a settings error above) — re-run ./setup to complete."
|
||||
|
||||
@@ -0,0 +1,58 @@
|
||||
/**
|
||||
* Q3 (#2207, #2208): in a Conductor workspace the PreToolUse question-
|
||||
* preference hook breaks Conductor's native AskUserQuestion round-trip even
|
||||
* when it only defers. setup no longer auto-installs it there, removes one an
|
||||
* earlier setup added (keeping the other plan-tune hooks), and still honors
|
||||
* an explicit opt-in. Real ./setup into a temp HOME (test/helpers/install-fixture).
|
||||
*/
|
||||
import { afterEach, describe, expect, test } from 'bun:test';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import { join } from 'node:path';
|
||||
import { cleanupFixtures, makeFixture, makeSource, type Fixture } from './helpers/install-fixture';
|
||||
|
||||
afterEach(cleanupFixtures);
|
||||
|
||||
function setup(f: Fixture, src: string, args: string[], env: Record<string, string> = {}) {
|
||||
const r = spawnSync('bash', [join(src, 'setup'), ...args], { cwd: f.home, env: { ...f.env, ...env }, encoding: 'utf8', timeout: 60_000, input: '' });
|
||||
expect(r.status, r.stdout + r.stderr).toBe(0);
|
||||
return r.stdout + r.stderr;
|
||||
}
|
||||
|
||||
function items(f: Fixture, src: string, event: string): string {
|
||||
const r = spawnSync('bash', [join(src, 'bin/gstack-settings-hook'), 'list-items', '--event', event, '--owned-by', 'plan-tune-cathedral'],
|
||||
{ env: f.env, encoding: 'utf8', timeout: 30_000 });
|
||||
return r.stdout.trim();
|
||||
}
|
||||
|
||||
const CONDUCTOR = { CONDUCTOR_WORKSPACE_PATH: '/tmp/conductor/workspaces/demo' };
|
||||
|
||||
describe.skipIf(process.platform === 'win32')('setup: Conductor and the AskUserQuestion preference hook (Q3)', () => {
|
||||
test('a Conductor setup with no explicit choice does not install it', () => {
|
||||
const f = makeFixture();
|
||||
const src = makeSource(f, join(f.home, '.claude/skills/gstack'));
|
||||
setup(f, src, [], CONDUCTOR);
|
||||
expect(items(f, src, 'PreToolUse')).toBe('');
|
||||
}, 90_000);
|
||||
|
||||
test('a Conductor setup removes one an earlier setup added and keeps the other plan-tune hooks', () => {
|
||||
const f = makeFixture();
|
||||
const src = makeSource(f, join(f.home, '.claude/skills/gstack'));
|
||||
setup(f, src, ['--plan-tune-hooks']);
|
||||
expect(items(f, src, 'PreToolUse')).toContain('question-preference-hook');
|
||||
const out = setup(f, src, [], CONDUCTOR);
|
||||
expect(out).toContain("removed the AskUserQuestion preference hook: it breaks Conductor's native AskUserQuestion");
|
||||
expect(out).toContain('Keep it anyway: gstack-config set plan_tune_hooks yes, then ./setup');
|
||||
expect(items(f, src, 'PreToolUse')).toBe('');
|
||||
expect(items(f, src, 'PostToolUse')).toContain('question-log-hook');
|
||||
// A later run neither re-adds it nor repeats the notice.
|
||||
expect(setup(f, src, [], CONDUCTOR)).not.toContain('removed the AskUserQuestion preference hook');
|
||||
expect(items(f, src, 'PreToolUse')).toBe('');
|
||||
}, 120_000);
|
||||
|
||||
test('an explicit opt-in in Conductor still installs it', () => {
|
||||
const f = makeFixture();
|
||||
const src = makeSource(f, join(f.home, '.claude/skills/gstack'));
|
||||
setup(f, src, [], { ...CONDUCTOR, GSTACK_PLAN_TUNE_HOOKS: 'yes' });
|
||||
expect(items(f, src, 'PreToolUse')).toContain('question-preference-hook');
|
||||
}, 90_000);
|
||||
});
|
||||
@@ -74,7 +74,7 @@ describe('dev-setup: never silently mutates global settings.json', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('setup: PT_EXPLICIT provenance (Conductor auto-opt-in respects explicit decisions)', () => {
|
||||
describe('setup: PT_EXPLICIT provenance (explicit decisions are never overridden)', () => {
|
||||
// The phantom-hooks root cause (Bug A): the Conductor auto-opt-in upgraded
|
||||
// PT_DECISION "prompt" → "yes" even when "prompt" came from dev-setup's
|
||||
// EXPLICIT --plan-tune-hooks=prompt flag, so every new Conductor workspace
|
||||
@@ -88,10 +88,11 @@ describe('setup: PT_EXPLICIT provenance (Conductor auto-opt-in respects explicit
|
||||
expect(explicitIdx).toBeGreaterThan(flagIdx);
|
||||
});
|
||||
|
||||
test('the Conductor auto-opt-in fires only on the true silent fall-through', () => {
|
||||
expect(setupSrc).toMatch(
|
||||
/\[ "\$PT_DECISION" = "prompt" \] && \[ "\$PT_EXPLICIT" -eq 0 \] && \{ \[ -n "\$\{CONDUCTOR_WORKSPACE_PATH:-\}" \] \|\| \[ -n "\$\{CONDUCTOR_PORT:-\}" \]; \}/,
|
||||
);
|
||||
// Q3 (#2207) retired the Conductor auto-opt-in: Conductor never upgrades a
|
||||
// silent "prompt" to "yes". Behavior is pinned by setup-conductor-auq-hook.
|
||||
test('Conductor never auto-opts into the plan-tune hooks', () => {
|
||||
expect(setupSrc).not.toMatch(/CONDUCTOR[A-Z_]*:-\}" \]; \}; then\n\s*PT_DECISION="yes"/);
|
||||
expect(setupSrc).not.toContain('_PT_CONDUCTOR_AUTO');
|
||||
});
|
||||
|
||||
test('config provenance uses gstack-config has (env-resolution-safe), never a hardcoded config grep', () => {
|
||||
|
||||
Reference in new issue
Block a user