From f69f83165d7242d74228d3f3822afb960d851413 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 3 Oct 2026 21:30:33 +0000 Subject: [PATCH] B5: learnings callers report a failed search instead of reading as empty Every generated gstack-learnings-search caller ended in 2>/dev/null || true, so a missing bun or a failed search looked like 'nothing recorded'. Callers now keep stdout, capture stderr and the exit status, and print 'LEARNINGS: unavailable ()' on failure (resolver callers in learnings.ts and review-army.ts, plus investigate, qa and ship's adversarial section). --- investigate/SKILL.md.tmpl | 3 +- qa/SKILL.md.tmpl | 3 +- scripts/resolvers/learnings.ts | 16 ++++- scripts/resolvers/review-army.ts | 4 +- ship/sections/adversarial.md.tmpl | 3 +- test/learnings-callers-unavailable.test.ts | 73 ++++++++++++++++++++++ 6 files changed, 95 insertions(+), 7 deletions(-) create mode 100644 test/learnings-callers-unavailable.test.ts diff --git a/investigate/SKILL.md.tmpl b/investigate/SKILL.md.tmpl index ab57e5fcd..ca3533607 100644 --- a/investigate/SKILL.md.tmpl +++ b/investigate/SKILL.md.tmpl @@ -106,7 +106,8 @@ Pick ONE keyword from the hypothesis. The keyword should be a noun: the failing Worked examples (investigate-specific): good keywords are `auth-cookie`, `session-expiry`, `redirect-loop`. Bad: `auth.ts:47`, `fix the auth bug`, ``. ```bash -~/.claude/skills/gstack/bin/gstack-learnings-search --query "" --limit 5 2>/dev/null || true +{ _LERR=$(~/.claude/skills/gstack/bin/gstack-learnings-search --query "" --limit 5 2>&1 >&3 3>&-); _LRC=$?; } 3>&1 +[ "$_LRC" = 0 ] || { _LERR=${_LERR%%$'\n'*}; echo "LEARNINGS: unavailable (${_LERR:-exit $_LRC})"; } ``` If any learnings come back, name which one applies to your investigation in one sentence. If none come back, continue without reference — the absence of a matching prior learning is itself useful information. diff --git a/qa/SKILL.md.tmpl b/qa/SKILL.md.tmpl index 643260452..11708c434 100644 --- a/qa/SKILL.md.tmpl +++ b/qa/SKILL.md.tmpl @@ -158,7 +158,8 @@ only letters, digits or hyphens (e.g., `checkout-button`, `payment`), never a pa quotes, whitespace or other punctuation; simplify to an alphanumeric stem if needed. ```bash -~/.claude/skills/gstack/bin/gstack-learnings-search --query "" --limit 5 2>/dev/null || true +{ _LERR=$(~/.claude/skills/gstack/bin/gstack-learnings-search --query "" --limit 5 2>&1 >&3 3>&-); _LRC=$?; } 3>&1 +[ "$_LRC" = 0 ] || { _LERR=${_LERR%%$'\n'*}; echo "LEARNINGS: unavailable (${_LERR:-exit $_LRC})"; } ``` Name an applicable learning in one sentence, or continue if none applies. diff --git a/scripts/resolvers/learnings.ts b/scripts/resolvers/learnings.ts index 2318c9b53..a00528c31 100644 --- a/scripts/resolvers/learnings.ts +++ b/scripts/resolvers/learnings.ts @@ -21,6 +21,14 @@ import { getHostConfig } from '../../hosts/index'; // future contributors writing dangerous values. const QUERY_SAFE_RE = /^[A-Za-z0-9 _-]+$/; +/** + * B5 (#2790): run a learnings search with its stdout intact and its stderr and + * exit status kept, so a missing bun or a failed script prints why instead of + * reading as "nothing recorded". Two lines: the capture, then the verdict. + */ +export const learningsCapture = (command: string) => `{ _LERR=$(${command} 2>&1 >&3 3>&-); _LRC=$?; } 3>&1`; +export const LEARNINGS_VERDICT = `[ "$_LRC" = 0 ] || { _LERR=\${_LERR%%$'\\n'*}; echo "LEARNINGS: unavailable (\${_LERR:-exit $_LRC})"; }`; + export function generateLearningsSearch(ctx: TemplateContext, args?: string[]): string { // Parse query= arg. Empty value falls through to no-query (principle of least surprise: // a stray {{LEARNINGS_SEARCH:query=}} placeholder gets today's behavior, not a build error). @@ -58,7 +66,8 @@ the current behavior. Reading old notes never requires writing new ones.`; Search for relevant learnings from previous sessions on this project: \`\`\`bash -$GSTACK_BIN/gstack-learnings-search --limit 10${queryFlag} 2>/dev/null || true +${learningsCapture(`$GSTACK_BIN/gstack-learnings-search --limit 10${queryFlag}`)} +${LEARNINGS_VERDICT} \`\`\` If learnings are found, incorporate them into your analysis. When a ${findingKind} finding @@ -73,10 +82,11 @@ Search for relevant learnings from previous sessions: _CROSS_PROJ=$(${ctx.paths.binDir}/gstack-config get cross_project_learnings 2>/dev/null || echo "unset") echo "CROSS_PROJECT: $_CROSS_PROJ" if [ "$_CROSS_PROJ" = "true" ]; then - ${ctx.paths.binDir}/gstack-learnings-search --limit 10${queryFlag} --cross-project 2>/dev/null || true + ${learningsCapture(`${ctx.paths.binDir}/gstack-learnings-search --limit 10${queryFlag} --cross-project`)} else - ${ctx.paths.binDir}/gstack-learnings-search --limit 10${queryFlag} 2>/dev/null || true + ${learningsCapture(`${ctx.paths.binDir}/gstack-learnings-search --limit 10${queryFlag}`)} fi +${LEARNINGS_VERDICT} \`\`\` If \`CROSS_PROJECT\` is \`unset\` (first time): ${ctx.skillName === 'plan-eng-review' ? 'Build a full decision brief from these facts and options using the preamble format, then ask and wait:' : 'Use AskUserQuestion:'} diff --git a/scripts/resolvers/review-army.ts b/scripts/resolvers/review-army.ts index 3af34dcb8..17dc78ff1 100644 --- a/scripts/resolvers/review-army.ts +++ b/scripts/resolvers/review-army.ts @@ -11,6 +11,7 @@ */ import type { TemplateContext } from './types'; import { CC_BACKGROUND_DEFAULT_SINCE } from './constants'; +import { learningsCapture, LEARNINGS_VERDICT } from './learnings'; function generateSpecialistSelection(ctx: TemplateContext): string { const isShip = ctx.skillName === 'ship'; @@ -100,7 +101,8 @@ Construct the prompt for each specialist. The prompt includes: 3. Past learnings for this domain (if any exist): \`\`\`bash -${ctx.paths.binDir}/gstack-learnings-search --type pitfall --query "{specialist domain}" --limit 5 2>/dev/null || true +${learningsCapture(`${ctx.paths.binDir}/gstack-learnings-search --type pitfall --query "{specialist domain}" --limit 5`)} +${LEARNINGS_VERDICT} \`\`\` If learnings are found, include them: "Past learnings for this domain: {learnings}" diff --git a/ship/sections/adversarial.md.tmpl b/ship/sections/adversarial.md.tmpl index d91ff1ab2..16665f0f8 100644 --- a/ship/sections/adversarial.md.tmpl +++ b/ship/sections/adversarial.md.tmpl @@ -14,7 +14,8 @@ be alphanumeric or hyphen only; simplify other characters. For example, use `token-or-search`, not `feat: token-or search`. ```bash -~/.claude/skills/gstack/bin/gstack-learnings-search --query "" --limit 5 2>/dev/null || true +{ _LERR=$(~/.claude/skills/gstack/bin/gstack-learnings-search --query "" --limit 5 2>&1 >&3 3>&-); _LRC=$?; } 3>&1 +[ "$_LRC" = 0 ] || { _LERR=${_LERR%%$'\n'*}; echo "LEARNINGS: unavailable (${_LERR:-exit $_LRC})"; } ``` Name an applicable learning and its effect on the version bump or CHANGELOG in diff --git a/test/learnings-callers-unavailable.test.ts b/test/learnings-callers-unavailable.test.ts new file mode 100644 index 000000000..8b37ad305 --- /dev/null +++ b/test/learnings-callers-unavailable.test.ts @@ -0,0 +1,73 @@ +/** + * B5 (#2790): generated learnings-search callers used `2>/dev/null || true`, so + * a missing bun or a failed search read as "no learnings recorded". Every + * caller fence now keeps stdout, and on failure prints + * `LEARNINGS: unavailable ()`. Executed with a stub search + * binary on Claude (full mode) and Codex (basic mode, fresh-shell prelude). + */ +import { afterAll, beforeAll, describe, expect, test } from 'bun:test'; +import { spawnSync } from 'child_process'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { runGeneration } from '../scripts/gen-skill-docs'; + +const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-learnings-callers-')); +const out = path.join(tmp, 'render'); +const BUN_MISSING = 'gstack-learnings-search: bun not found on PATH, so learnings could not be read. Fix: install Bun (https://bun.sh), then re-run ./setup.'; + +const CALLERS: Array<{ name: string; file: (host: string) => string; hosts: string[] }> = [ + { name: 'Prior Learnings (review)', file: h => (h === 'claude' ? 'review/SKILL.md' : '.agents/skills/gstack-review/SKILL.md'), hosts: ['claude', 'codex'] }, + { name: 'investigate hypothesis refresh', file: h => (h === 'claude' ? 'investigate/SKILL.md' : '.agents/skills/gstack-investigate/SKILL.md'), hosts: ['claude', 'codex'] }, + { name: 'qa fix-loop search', file: h => (h === 'claude' ? 'qa/SKILL.md' : '.agents/skills/gstack-qa/SKILL.md'), hosts: ['claude', 'codex'] }, + { name: 'ship adversarial search', file: h => (h === 'claude' ? 'ship/sections/adversarial.md' : '.agents/skills/gstack-ship/sections/adversarial.md'), hosts: ['claude', 'codex'] }, + { name: 'review army specialist search', file: () => 'review/sections/review-army.md', hosts: ['claude'] }, +]; + +function searchFences(file: string): string[] { + return [...fs.readFileSync(path.join(out, file), 'utf8').matchAll(/^```bash\n([\s\S]*?)\n```$/gm)] + .map(m => m[1]).filter(body => body.includes('gstack-learnings-search')); +} + +function run(fence: string, host: string, stub: string) { + const home = fs.mkdtempSync(path.join(tmp, 'home-')); + const root = path.join(home, host === 'claude' ? '.claude/skills/gstack' : '.codex/skills/gstack'); + for (const dir of ['bin', 'lib']) fs.mkdirSync(path.join(root, dir), { recursive: true }); + fs.writeFileSync(path.join(root, 'bin', 'gstack-learnings-search'), `#!/bin/sh\n${stub}\n`, { mode: 0o755 }); + fs.writeFileSync(path.join(root, 'bin', 'gstack-config'), '#!/bin/sh\necho false\n', { mode: 0o755 }); + const cwd = fs.mkdtempSync(path.join(tmp, 'cwd-')); + return spawnSync('env', ['-i', `HOME=${home}`, 'PATH=/usr/bin:/bin', 'bash', '-c', fence], { cwd, encoding: 'utf8', timeout: 10_000 }); +} + +beforeAll(async () => { + const r = await runGeneration({ host: 'all', outputRoot: out }); + if (r.exitCode !== 0) throw new Error(r.diagnostics.filter(d => d.kind === 'error').map(d => d.message).join('\n')); +}); +afterAll(() => fs.rmSync(tmp, { recursive: true, force: true })); + +describe('B5: learnings callers report a failed search', () => { + for (const caller of CALLERS) { + for (const host of caller.hosts) { + test(`${host}: ${caller.name}`, () => { + const fences = searchFences(caller.file(host)); + expect(fences.length).toBeGreaterThan(0); + for (const fence of fences) { + const missing = run(fence, host, `echo "${BUN_MISSING}" >&2\necho "second line" >&2\nexit 127`); + expect(missing.stdout, missing.stderr).toContain(`LEARNINGS: unavailable (${BUN_MISSING})`); + expect(missing.stdout).not.toContain('second line'); + + const silentFail = run(fence, host, 'exit 3'); + expect(silentFail.stdout).toContain('LEARNINGS: unavailable (exit 3)'); + + const found = run(fence, host, 'echo "LEARNING: retry-backoff (confidence 8)"'); + expect(found.stdout).toContain('LEARNING: retry-backoff (confidence 8)'); + expect(found.stdout).not.toContain('LEARNINGS: unavailable'); + + const none = run(fence, host, 'exit 0'); + expect(none.status).toBe(0); + expect(none.stdout).not.toContain('LEARNINGS: unavailable'); + } + }); + } + } +});