From c0caf45803fda5c83ddd4e417a1571cd56b847fc Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 26 Oct 2026 01:20:39 +0000 Subject: [PATCH] fix(skill-start): no CLAUDE.md routing offer under proactive off The one-time offer to append ## Skill routing to the project's CLAUDE.md fired whatever the proactive value, and its rules are the auto-invocation an off user turned off. The offer is skipped under off and the legacy false; a section already in CLAUDE.md is left alone. Under core the offered section still routes every skill, which docs/proactive.md records as the follow-up. Found by /ship's red team. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- CHANGELOG.md | 4 ++-- bin/gstack-skill-start | 6 ++++-- docs/proactive.md | 14 ++++++++------ test/gen-skill-docs.test.ts | 5 +++-- test/proactive-skill-start.test.ts | 19 +++++++++++++++++++ 5 files changed, 36 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 645a5ede2..9650dde9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ - **`SKILL.md.tmpl`** (the router): three branches, `off`, `core` and `all`; under `core` only the four core routing rules auto-invoke and a skill the user names is always invoked. The opt-out line writes `off`, and `all` and `core` have their own lines. - **Every skill's preamble** (`scripts/resolvers/preamble/generate-upgrade-check.ts`): the steady-state rule names the four core skills under `core`, scopes the setting to unasked suggestions ("This skill's steps still run", so a handoff or a dispatched `/document-release` inside an invoked skill is not a suggestion), and is exactly as long as before, so the 89 size pins in `test/fixtures/skill-md-size-allowlist.json` are unchanged. - **`bin/gstack-skill-start`**: the one-time prompt offers "Keep the core four (recommended)", "Suggest every matching skill" and "Turn it off", writing `core`, `all` or `off`. `scripts/question-registry.ts` lists the same three options. -- **`bin/gstack-skill-start`**: the `PROACTIVE:` STATUS fallback when `gstack-config` is missing is `core`, not `true` (found by /ship's coverage audit, red at the first head); a stored value gstack does not know reads as `core`; the first-run and first-loop tips name only core skills and stay quiet under `off`. +- **`bin/gstack-skill-start`**: the `PROACTIVE:` STATUS fallback when `gstack-config` is missing is `core`, not `true` (found by /ship's coverage audit, red at the first head); a stored value gstack does not know reads as `core`; the first-run and first-loop tips name only core skills and stay quiet under `off`; the one-time offer to add `## Skill routing` to the project's CLAUDE.md is not made under `off` (found by /ship's red team: the routing rules are the auto-invocation the user turned off). - **`office-hours/sections/design-and-handoff.md.tmpl`**: the next-review handoff takes the recommend-only path under `off` as well as the legacy `false` (found by /ship's review); under `core` it still offers the review, because a step inside an invoked skill is part of that skill, which the router now says in so many words. ### Added @@ -19,7 +19,7 @@ - `test/proactive-config.test.ts`: the default in get, list and defaults; the five accepted values stored verbatim; the warning and `core` fallback on an unknown value (negative control: `on`); the header names the four skills. - `test/proactive-core-set.test.ts`: the router template and its render name `core` as the default and exactly the four core skills (negative control: no non-core skill in the core block, `true` is no longer "the default", no `set proactive false` left); the preamble rule carries the same four and did not grow past the old line; the prompt and registry agree on `core`, `all`, `off`. - `test/proactive-skill-start.test.ts`: the `PROACTIVE:` STATUS line carries the stored value, `core` when unset or unknown, and the missing-`gstack-config` fallback equals the config default (regression test: failed at the first head). `test/proactive-core-set.test.ts` also checks every surface (config header and default, router, preamble, prompt, docs) for the same four skills and three values, the office-hours gate, the router's in-skill carve-out, and the tips. -- Known gap, follow-up: the `## Skill routing` section gstack injects into a project's CLAUDE.md, and the "Proactively suggest" sentences in non-core skill descriptions (full catalog mode), still route every skill; docs/proactive.md says so. +- Known gap, follow-up: under `core`, the `## Skill routing` section gstack offers for a project's CLAUDE.md, and the "Proactively suggest" sentences in non-core skill descriptions (full catalog mode), still route every skill; docs/proactive.md says so. ### Gate integrity - No threshold, budget, timeout, trial count or case kind changed. Size pins unchanged (the preamble is byte-for-byte the same length). diff --git a/bin/gstack-skill-start b/bin/gstack-skill-start index 1b2c0daac..0ffca701f 100755 --- a/bin/gstack-skill-start +++ b/bin/gstack-skill-start @@ -591,8 +591,10 @@ fi # Routing injection (interactive: AUQ + CLAUDE.md append + commit). The body # is a quoted heredoc (markdown backticks must stay literal), so the resolved -# bin path is substituted via sed on the __BIN__ placeholder. -if [ "$_HAS_ROUTING" = "no" ] && [ "$_ROUTING_DECLINED" = "false" ] && [ "$_PROACTIVE_PROMPTED" = "yes" ]; then +# bin path is substituted via sed on the __BIN__ placeholder. Under off the +# routing rules are the auto-invocation the user turned off, so no offer. +case "$_PROACTIVE" in off|false) _ROUTING_OFFER="no" ;; *) _ROUTING_OFFER="yes" ;; esac +if [ "$_HAS_ROUTING" = "no" ] && [ "$_ROUTING_DECLINED" = "false" ] && [ "$_PROACTIVE_PROMPTED" = "yes" ] && [ "$_ROUTING_OFFER" = "yes" ]; then sed "s|__BIN__|$_BIN|g" <<'EOI' | _emit_block routing-injection gstack works best when the project CLAUDE.md includes skill routing rules. If no CLAUDE.md exists in the project root, create it. Use AskUserQuestion: diff --git a/docs/proactive.md b/docs/proactive.md index 51af9ea90..a6ffa93d1 100644 --- a/docs/proactive.md +++ b/docs/proactive.md @@ -30,12 +30,14 @@ needed, because every skill reads `PROACTIVE` from `gstack-skill-start` at the start of each run and the router (`/gstack`) applies it to its routing rules. A value gstack does not know (a hand-edited `proactive: maybe`) reads as `core`. -One surface does not follow the setting yet: the `## Skill routing` section -gstack offers to append to your project's CLAUDE.md lists every routing rule and -tells the host to invoke any matching skill. If you accepted it before the -`core` default, trim that section to the four core rules (or delete it and let -gstack offer it again); making the injected section follow `proactive` is -tracked as a follow-up. +One surface follows the setting only in part: the `## Skill routing` section +gstack offers to append to your project's CLAUDE.md. Under `off` the offer is +not made, because those rules are the auto-invocation you turned off (a section +already in CLAUDE.md stays; delete it yourself). Under `core` the offered +section still lists every routing rule and tells the host to invoke any +matching skill. If you accepted it before the `core` default, trim that section +to the four core rules (or delete it and let gstack offer it again); making the +offered section follow `core` is tracked as a follow-up. gstack asks once, after the telemetry question, which of the three you want. "Say stop suggesting" in a session runs `gstack-config set proactive off` for you. diff --git a/test/gen-skill-docs.test.ts b/test/gen-skill-docs.test.ts index a39e72399..e6a37502f 100644 --- a/test/gen-skill-docs.test.ts +++ b/test/gen-skill-docs.test.ts @@ -2276,10 +2276,11 @@ describe('preamble routing injection (bin/gstack-skill-start emission layer)', ( }); test('routing injection only fires when all conditions met', () => { - // Must be: HAS_ROUTING=no AND ROUTING_DECLINED=false AND PROACTIVE_PROMPTED=yes + // Must be: HAS_ROUTING=no AND ROUTING_DECLINED=false AND PROACTIVE_PROMPTED=yes AND the offer is not switched off by proactive off/false expect(SKILL_START_SCRIPT).toContain( - 'if [ "$_HAS_ROUTING" = "no" ] && [ "$_ROUTING_DECLINED" = "false" ] && [ "$_PROACTIVE_PROMPTED" = "yes" ]; then', + 'if [ "$_HAS_ROUTING" = "no" ] && [ "$_ROUTING_DECLINED" = "false" ] && [ "$_PROACTIVE_PROMPTED" = "yes" ] && [ "$_ROUTING_OFFER" = "yes" ]; then', ); + expect(SKILL_START_SCRIPT).toContain('case "$_PROACTIVE" in off|false) _ROUTING_OFFER="no" ;; *) _ROUTING_OFFER="yes" ;; esac'); }); test('routing section content includes key routing rules', () => { diff --git a/test/proactive-skill-start.test.ts b/test/proactive-skill-start.test.ts index 2f7c77b48..21ea9ea28 100644 --- a/test/proactive-skill-start.test.ts +++ b/test/proactive-skill-start.test.ts @@ -85,6 +85,25 @@ describe('gstack-skill-start PROACTIVE status line', () => { } }); + test('the CLAUDE.md routing offer fires under core and not under off (the routing rules are the auto-invocation off turns off)', () => { + const cases: Array<[string, boolean]> = [ + ['update_check: false\n', true], + ['update_check: false\nproactive: off\n', false], + ['update_check: false\nproactive: false\n', false], + ]; + for (const [config, offered] of cases) { + const state = freshState(config); + fs.writeFileSync(path.join(state, '.proactive-prompted'), ''); + try { + const out = runStart(path.join(BIN, 'gstack-skill-start'), state); + expect(out, config).toContain('HAS_ROUTING: no'); + expect(out.includes('GSTACK_INSTRUCTION_BEGIN: routing-injection '), config).toBe(offered); + } finally { + fs.rmSync(state, { recursive: true, force: true }); + } + } + }); + test('regression: with no gstack-config beside it, the fallback reports the config default, not a stale literal', () => { const fakeBin = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-proactive-ss-bin-')); const state = freshState('update_check: false\n');