diff --git a/scripts/resolvers/learnings.ts b/scripts/resolvers/learnings.ts index 685188fb2..8182251d0 100644 --- a/scripts/resolvers/learnings.ts +++ b/scripts/resolvers/learnings.ts @@ -13,7 +13,27 @@ */ import type { TemplateContext } from './types'; -export function generateLearningsSearch(ctx: TemplateContext): string { +// Whitelist for query= macro values. Allows alphanumeric, space, hyphen, underscore. +// Anything else (e.g. $, backticks, quotes, ;) is a shell-injection vector when the +// emitted bash interpolates the value into `--query "${queryArg}"`. Static template +// queries hand-written in gstack are safe, but the resolver API must defend against +// future contributors writing dangerous values. +const QUERY_SAFE_RE = /^[A-Za-z0-9 _-]+$/; + +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). + const queryArg = (args || []) + .filter(a => a.startsWith('query=')) + .map(a => a.slice(6)) + .filter(Boolean)[0]; + if (queryArg && !QUERY_SAFE_RE.test(queryArg)) { + throw new Error( + `{{LEARNINGS_SEARCH:query=...}} value must match ${QUERY_SAFE_RE} (alphanumeric, space, hyphen, underscore). Got: ${JSON.stringify(queryArg)}` + ); + } + const queryFlag = queryArg ? ` --query "${queryArg}"` : ''; + if (ctx.host === 'codex') { // Codex: simpler version, no cross-project, uses $GSTACK_BIN return `## Prior Learnings @@ -21,7 +41,7 @@ export function generateLearningsSearch(ctx: TemplateContext): string { Search for relevant learnings from previous sessions on this project: \`\`\`bash -$GSTACK_BIN/gstack-learnings-search --limit 10 2>/dev/null || true +$GSTACK_BIN/gstack-learnings-search --limit 10${queryFlag} 2>/dev/null || true \`\`\` If learnings are found, incorporate them into your analysis. When a review finding @@ -36,9 +56,9 @@ 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 --cross-project 2>/dev/null || true + ${ctx.paths.binDir}/gstack-learnings-search --limit 10${queryFlag} --cross-project 2>/dev/null || true else - ${ctx.paths.binDir}/gstack-learnings-search --limit 10 2>/dev/null || true + ${ctx.paths.binDir}/gstack-learnings-search --limit 10${queryFlag} 2>/dev/null || true fi \`\`\` diff --git a/test/gen-skill-docs.test.ts b/test/gen-skill-docs.test.ts index b30a32464..4bf8abeee 100644 --- a/test/gen-skill-docs.test.ts +++ b/test/gen-skill-docs.test.ts @@ -3047,3 +3047,51 @@ describe('GSTACK REVIEW REPORT delete-then-append flow', () => { expect(src).not.toContain('If it was found mid-file, move it'); }); }); + +describe('LEARNINGS_SEARCH resolver: query parameter', () => { + // Lazy-load resolver and types after describe block to keep test file self-contained. + const { generateLearningsSearch } = require('../scripts/resolvers/learnings'); + const { HOST_PATHS } = require('../scripts/resolvers/types'); + + const claudeCtx = { + skillName: 'test', + tmplPath: 'test/SKILL.md.tmpl', + host: 'claude', + paths: HOST_PATHS.claude, + }; + const codexCtx = { ...claudeCtx, host: 'codex', paths: HOST_PATHS.codex }; + + test('no args → bash does not contain --query (backwards-compat)', () => { + const out = generateLearningsSearch(claudeCtx); + expect(out).not.toContain('--query'); + }); + + test('claude host + query=foo bar → both cross-project and project-scoped branches contain --query', () => { + const out = generateLearningsSearch(claudeCtx, ['query=foo bar']); + // Both branches of the if/else must carry the flag. + const lines = out.split('\n').filter(l => l.includes('gstack-learnings-search')); + expect(lines.length).toBeGreaterThanOrEqual(2); + for (const line of lines) { + expect(line).toContain('--query "foo bar"'); + } + }); + + test('codex host + query=foo bar → codex bash variant contains --query', () => { + const out = generateLearningsSearch(codexCtx, ['query=foo bar']); + expect(out).toContain('--query "foo bar"'); + expect(out).toContain('$GSTACK_BIN/gstack-learnings-search'); + }); + + test('empty value query= → bash does not contain --query (locked semantics: falls through)', () => { + const claudeOut = generateLearningsSearch(claudeCtx, ['query=']); + expect(claudeOut).not.toContain('--query'); + const codexOut = generateLearningsSearch(codexCtx, ['query=']); + expect(codexOut).not.toContain('--query'); + }); + + test('shell-injection chars in query= → throws at gen-time (defense in depth)', () => { + for (const bad of ['$(whoami)', '`cmd`', 'a;b', 'a&b', 'a"b', 'a\\b', 'foo$x']) { + expect(() => generateLearningsSearch(claudeCtx, [`query=${bad}`])).toThrow(/alphanumeric/); + } + }); +});