From 7e8cb6791ca422102897fb75fb748f6cb1e5493e Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 1 Nov 2026 07:05:25 +0000 Subject: [PATCH] fix(harness): tolerant JSON read-back, short read-back timeout, retry an interrupted explicit activation, symlink checks on every hook opt-out and folder cleanup Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-harness-ledger.sh | 5 +++- lib/harness-register.ts | 10 ++++---- lib/harness-start-hook.ts | 13 ++++++---- test/harness-start-hook.test.ts | 42 +++++++++++++++++++++++++++++++++ 4 files changed, 61 insertions(+), 9 deletions(-) diff --git a/bin/gstack-harness-ledger.sh b/bin/gstack-harness-ledger.sh index 4fec7582b..6d24acb4b 100644 --- a/bin/gstack-harness-ledger.sh +++ b/bin/gstack-harness-ledger.sh @@ -125,7 +125,10 @@ gstack_harness_unregister_ledger() { done < "$ledger" while IFS=$'\t' read -r kind a b; do [ "$kind" = dir ] && [ -d "$b" ] && [ ! -L "$b" ] || continue - case "$b" in "$dest"/*) _gstack_harness_owned "$b" "$dest" || continue ;; esac + case "$b" in + "$dest"/*) _gstack_harness_owned "$b" "$dest" || continue ;; + *) if [ -n "$hprof" ]; then case "$b" in "$hprof"/*) _gstack_harness_below "$b" "$hprof" || continue ;; esac; fi ;; + esac rmdir "$b" 2>/dev/null done < "$ledger" case "$target" in diff --git a/lib/harness-register.ts b/lib/harness-register.ts index 8768a3b35..9b00b665f 100644 --- a/lib/harness-register.ts +++ b/lib/harness-register.ts @@ -541,19 +541,20 @@ function hookPlan(opts: RegisterOptions, dest: string, prev: Ledger | null): Hoo const root = hookRoot(t.name, profile); const plan = { on: choice.on, prevOn, explicit: choice.source === 'flag', retry: prevOn && prev!.meta.hook_active === 'no', profile, root, files: new Map() }; const saved = prev?.meta.hook_root && prev.meta.hook_profile ? { profile: prev.meta.hook_profile, root: prev.meta.hook_root } : null; - if (saved && !choice.on) assertHookRoot(t.name, saved.profile, saved.root, saved.root, (code, msg) => { throw new HarnessError(code, msg); }); - if (!choice.on) return plan; + const name = t.name; + const off = (p: HookPlan) => { if (saved) assertHookRoot(name, saved.profile, saved.root, saved.root, (code, msg) => { throw new HarnessError(code, msg); }); return p; }; + if (!choice.on) return off(plan); const why = dest !== profileSkills(t.name, profile) ? 'this registration uses another skills folder (' + dest + ')' : t.name === 'openclaw' && opts.env.OPENCLAW_CONFIG_PATH ? 'OPENCLAW_CONFIG_PATH points OpenClaw at a config outside that profile' : ''; if (why && (plan.explicit || !prevOn)) { if (plan.explicit) throw new HarnessError('HARNESS_HOOK_UNSUPPORTED', 'the start hook goes into the ' + t.displayName + ' profile ' + profile + ', and ' + why); - return { ...plan, on: false, note: 'off: ' + why + ' (' + choice.source + ' asked for the hook)' }; + return off({ ...plan, on: false, note: 'off: ' + why + ' (' + choice.source + ' asked for the hook)' }); } try { assertHookRoot(t.name, profile, root, prevOn ? prev!.meta.hook_root : undefined, (code, msg) => { throw new HarnessError(code, msg); }); } catch (e) { if (plan.explicit || prevOn || !(e instanceof HarnessError)) throw e; - return { ...plan, on: false, note: 'off: ' + e.message + ' (' + choice.source + ' asked for the hook; ' + HARNESS_CODES[e.code].fix + ')' }; + return off({ ...plan, on: false, note: 'off: ' + e.message + ' (' + choice.source + ' asked for the hook; ' + HARNESS_CODES[e.code].fix + ')' }); } return { ...plan, files: hookFiles(t.name, root) }; } @@ -675,6 +676,7 @@ export function register(opts: RegisterOptions): RegisterResult { fs.writeFileSync(file, text); } if (opts.registry && t.render === 'host') shellRegistry(opts, 'gstack_install_registry_upsert', [t.name, 'global', '-', dest, opts.checkout, opts.checkout, opts.version, '-', '-']); + if (same && hook?.on && hook.explicit && prev.meta.hook_active !== 'no') writeLedger(lfile, { ...prev, meta: { ...prev.meta, hook_active: 'no' } }); const hookResult = finishHook(opts, hook, released); const active = hookResult?.required || hookResult?.retried ? ({ active: 'yes', disabled: 'disabled' } as Record)[hookResult.state] ?? 'no' : ''; const saved = active ? readLedger(lfile) : null; diff --git a/lib/harness-start-hook.ts b/lib/harness-start-hook.ts index a81989b5f..7594d6865 100644 --- a/lib/harness-start-hook.ts +++ b/lib/harness-start-hook.ts @@ -131,26 +131,31 @@ export function activateHook(t: HookTarget, profile: string, env: Env, home: str return ''; } -const parseJson = (s: string): any => { try { return JSON.parse(s.slice(s.search(/[[{]/))); } catch { return null; } }; +function parseJson(s: string): any { + for (const m of s.matchAll(/^[ \t]*[[{]/gm)) { try { return JSON.parse(s.slice(m.index)); } catch {} } + return null; +} /** Reads the hook back from the harness: active (configured for new sessions), disabled by the user, or written (with the fix). */ +const READ_TIMEOUT = 15_000; + export function inspectHook(t: HookTarget, profile: string, env: Env, home: string): HookResult { const fix = activateCommands(t, profile); if (!onPath(t, env)) return { state: 'written', detail: t + ' is not on PATH, so gstack could not turn the plugin on or check it', fix }; if (t === 'hermes') { - const r = runHarness(t, profile, ['plugins', 'list', '--json'], env, home); + const r = runHarness(t, profile, ['plugins', 'list', '--json'], env, home, READ_TIMEOUT); const row = (parseJson(r.stdout) ?? []).find?.((p: any) => p?.name === HOOK_ID); if (row?.status === 'enabled') return { state: 'active' }; if (row?.status === 'disabled') return { state: 'disabled', detail: 'turned off in Hermes (plugins.disabled); gstack leaves it off', fix: activateCommands(t, profile) }; return { state: 'written', detail: row ? 'Hermes lists it as ' + row.status : 'hermes plugins list does not show ' + HOOK_ID + (r.why ? ' (' + r.why + ')' : ''), fix }; } - const cfg = parseJson(runHarness(t, profile, ['config', 'get', 'plugins', '--json'], env, home).stdout) ?? {}; + const cfg = parseJson(runHarness(t, profile, ['config', 'get', 'plugins', '--json'], env, home, READ_TIMEOUT).stdout) ?? {}; const entry = cfg.entries?.[HOOK_ID] ?? {}; if (entry.enabled === false) return { state: 'disabled', detail: 'turned off in openclaw.json (plugins.entries.' + HOOK_ID + '.enabled false); gstack leaves it off', fix }; if (Array.isArray(cfg.deny) && cfg.deny.includes(HOOK_ID)) return { state: 'written', detail: 'plugins.deny lists ' + HOOK_ID, fix: 'remove ' + HOOK_ID + ' from plugins.deny in openclaw.json' }; if (Array.isArray(cfg.allow) && cfg.allow.length && !cfg.allow.includes(HOOK_ID)) return { state: 'written', detail: 'plugins.allow is set and does not list ' + HOOK_ID, fix: 'OPENCLAW_STATE_DIR=' + JSON.stringify(profile) + " openclaw config set plugins.allow '" + JSON.stringify([...cfg.allow, HOOK_ID]) + "'" }; if (entry.hooks?.allowPromptInjection === false) return { state: 'written', detail: 'plugins.entries.' + HOOK_ID + '.hooks.allowPromptInjection is false', fix }; - const r = runHarness(t, profile, ['plugins', 'inspect', HOOK_ID, '--runtime', '--json'], env, home); + const r = runHarness(t, profile, ['plugins', 'inspect', HOOK_ID, '--runtime', '--json'], env, home, READ_TIMEOUT); const d = parseJson(r.stdout); if (d?.plugin?.status === 'loaded' && d.plugin.enabled && (d.typedHooks ?? []).some((h: any) => h?.name === 'before_prompt_build')) return { state: 'active' }; const why = d?.plugin ? 'status ' + d.plugin.status + (d.plugin.enabled ? '' : ', not enabled') + ((d.typedHooks ?? []).length ? '' : ', before_prompt_build not registered') : 'openclaw plugins inspect failed' + (r.why ? ' (' + r.why + ')' : ''); diff --git a/test/harness-start-hook.test.ts b/test/harness-start-hook.test.ts index 33cb17ff0..a226c2c6c 100644 --- a/test/harness-start-hook.test.ts +++ b/test/harness-start-hook.test.ts @@ -24,6 +24,8 @@ d="$OPENCLAW_STATE_DIR"; c="$d/fake-config" printf '%s|secret=%s|home=%s|cfg=%s\n' "$*" "$ANTHROPIC_API_KEY" "$HOME" "$OPENCLAW_CONFIG_PATH" >> "$d/fake-calls" [ -f "$d/fake-fail" ] && { echo "config write refused" >&2; exit 3; } [ -f "$d/fake-kill" ] && { rm -f "$d/fake-kill"; kill -9 "$PPID"; sleep 5; } +[ -f "$d/fake-noisy" ] && echo "[plugins] gstack-start: cannot verify where this plugin came from" +[ -f "$d/fake-hang" ] && [ "$1" = plugins ] && exec sleep 40 touch "$c" acc=false; grep -q '^plugins.entries.gstack-start.hooks.allowConversationAccess=true$' "$c" && acc=true en=$(grep '^plugins.entries.gstack-start.enabled=' "$c" | tail -1 | cut -d= -f2); enj=null; [ -n "$en" ] && enj=$en @@ -281,6 +283,34 @@ describe('start hook: OpenClaw lifecycle (fake CLI)', () => { expect(fs.readFileSync(off.kept[0], 'utf8')).toBe('my note\n'); }, 300_000); + test('an explicit --start-hook on an unchanged registration killed during activation is retried by the next run', () => { + const r = rig('openclaw'); + expect(register(r.o({ startHook: true })).hook?.state).toBe('active'); + fs.writeFileSync(path.join(r.profile, 'fake-kill'), ''); + const cli = spawnSync(process.execPath, ['--no-env-file', '--config=/dev/null', path.join(ROOT, 'bin', 'gstack-harness'), 'register', 'openclaw', '--start-hook'], + { env: { ...r.env, PATH: r.env.PATH + path.delimiter + path.dirname(process.execPath), HOME: path.join(r.d, 'home'), GSTACK_STATE_ROOT: path.join(r.d, 'state') }, encoding: 'utf8', timeout: 240_000 }); + expect(cli.signal, cli.stderr).toBe('SIGKILL'); + const dir = path.join(r.d, 'state', 'harness'); + expect(readLedger(path.join(dir, fs.readdirSync(dir).find(f => f.endsWith('.ledger'))!))!.meta.hook_active).toBe('no'); + expect(register(r.o()).hook).toMatchObject({ state: 'active', retried: true }); + }, 300_000); + + test('a log line in brackets before the harness JSON still reads back as active', () => { + const r = rig('openclaw'); + fs.writeFileSync(path.join(r.profile, 'fake-noisy'), ''); + expect(register(r.o({ startHook: true })).hook?.state).toBe('active'); + expect(inspectHook('openclaw', r.profile, r.env, path.join(r.d, 'home')).state).toBe('active'); + }, 120_000); + + test('a hung harness read-back gives up after a short timeout', () => { + const r = rig('openclaw'); + expect(register(r.o({ startHook: true })).hook?.state).toBe('active'); + fs.writeFileSync(path.join(r.profile, 'fake-hang'), ''); + const t0 = Date.now(); + expect(inspectHook('openclaw', r.profile, r.env, path.join(r.d, 'home')).state).toBe('written'); + expect(Date.now() - t0).toBeLessThan(35_000); + }, 120_000); + test('without the harness on PATH the hook is written, not active, and names the commands to run', () => { const r = rig('openclaw', { noCli: true }); const res = register(r.o({ startHook: true })); @@ -424,6 +454,7 @@ describe('start hook: teardown keeps what gstack did not write (fake CLI)', () = fs.renameSync(ext, moved); fs.symlinkSync(moved, ext); expect(() => register(r.o({ startHook: false }))).toThrow(expect.objectContaining({ code: 'HARNESS_UNSAFE_DESTINATION' })); + expect(() => register({ ...r.o(), env: { ...r.env, GSTACK_HARNESS_START_HOOK: '1' } })).toThrow(expect.objectContaining({ code: 'HARNESS_UNSAFE_DESTINATION' })); expect(fs.existsSync(path.join(moved, HOOK_ID, 'index.js'))).toBe(true); fs.unlinkSync(ext); fs.renameSync(moved, ext); @@ -433,6 +464,17 @@ describe('start hook: teardown keeps what gstack did not write (fake CLI)', () = expect(readLedger(off.ledger)!.meta.hook_root).toBeUndefined(); }, 300_000); + test('unregister leaves an empty plugin folder alone when the folder above it was swapped for a symlink', () => { + const r = rig('openclaw'); + expect(register(r.o({ startHook: true })).hook?.state).toBe('active'); + const ext = path.join(r.profile, 'extensions'), elsewhere = path.join(r.d, 'elsewhere'); + fs.renameSync(ext, path.join(r.d, 'old')); + fs.mkdirSync(path.join(elsewhere, HOOK_ID), { recursive: true }); + fs.symlinkSync(elsewhere, ext); + unregister(r.o().stateRoot, ROOT, 'openclaw', r.env); + expect(fs.existsSync(path.join(elsewhere, HOOK_ID))).toBe(true); + }, 300_000); + test('a saved opt-in survives one run from a shell that sets OPENCLAW_CONFIG_PATH', () => { const r = rig('openclaw'); expect(register(r.o({ startHook: true })).hook?.state).toBe('active');