diff --git a/bin/gstack-settings-hook b/bin/gstack-settings-hook index 645616e61..d97efe6e8 100755 --- a/bin/gstack-settings-hook +++ b/bin/gstack-settings-hook @@ -14,7 +14,8 @@ # gstack-settings-hook add-event --event \ # --command --source [--matcher ] [--timeout ] # gstack-settings-hook ensure-event --event ... --command ... --source ... [--matcher ...] [--timeout ] -# gstack-settings-hook remove-source --source # removes items the table identifies as 's, tagged or not +# gstack-settings-hook remove-source --source [--event ] +# # removes items the table identifies as '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 # legacy SessionStart remove gstack-settings-hook add-event --event --command --source [--matcher ] [--timeout ] gstack-settings-hook ensure-event --event --command --source [--matcher ] [--timeout ] - gstack-settings-hook remove-source --source # tagged OR table-identified items of + gstack-settings-hook remove-source --source [--event ] # tagged OR table-identified items of gstack-settings-hook diff-event --event --command --source [--matcher ] [--timeout ] gstack-settings-hook prune-stale [--repoint ] [--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 = []; diff --git a/setup b/setup index 20ac8627e..1fd430bb6 100755 --- a/setup +++ b/setup @@ -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." diff --git a/test/setup-conductor-auq-hook.test.ts b/test/setup-conductor-auq-hook.test.ts new file mode 100644 index 000000000..81f5c6897 --- /dev/null +++ b/test/setup-conductor-auq-hook.test.ts @@ -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 = {}) { + 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); +}); diff --git a/test/setup-plan-tune-hooks-noninteractive.test.ts b/test/setup-plan-tune-hooks-noninteractive.test.ts index a9da5d26a..e5ebd07ff 100644 --- a/test/setup-plan-tune-hooks-noninteractive.test.ts +++ b/test/setup-plan-tune-hooks-noninteractive.test.ts @@ -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', () => {