From bb935ac1cd1f585ca6abdbd9fef238b07918908c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Herna=CC=81n=20Pau=CC=81l=20Ossando=CC=81n?= Date: Sun, 24 May 2026 11:29:29 -0400 Subject: [PATCH 01/12] feat(orchestrator): product context in spec-writer prompt + publish spec to knowledge-repo PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part A — context injection: - New fetchProductContext() fetches README.md + AGENT.md/CLAUDE.md fallback from raw.githubusercontent.com using GITHUB_TOKEN; truncates to 2000 chars; 404 → skip - buildSpecWriterPrompt() injects a ## Product Context section (product slug, workflow stages, README, agent instructions) when context is provided - dispatchStageHandler() fetches context when githubToken present; errors non-fatal Part B — publish spec to knowledge-repo PR: - New publishSpecToPR() clones/updates knowledge repo, creates branch helm/spec/{externalId}, copies spec, commits, push --force, opens PR via gh - Idempotent: reuses existing open PR for the branch; never creates duplicates - Token-embedded HTTPS URL for auth (no interactive git prompts) - RunGit / RunGh injectable for testing; descriptive error messages per step - handleSpecWriterResult() accepts optional SpecPublishOptions; publish step runs before stage transition (stage change reflects PR state) - DispatchResult gains optional prUrl field Storage: - DataPaths.knowledgeRepos → data/knowledge-repos/ pre-created by ensureDataDir Tests: 67 orchestrator + 132 API (all passing); tsc --noEmit clean Co-Authored-By: Claude Sonnet 4.6 --- apps/api/src/routes/dispatch.ts | 11 +- packages/orchestrator/src/dispatcher.ts | 47 +++- packages/orchestrator/src/index.ts | 17 +- .../specialists/fetch-product-context.test.ts | 162 +++++++++++ .../src/specialists/fetch-product-context.ts | 99 +++++++ .../src/specialists/spec-publisher.test.ts | 242 ++++++++++++++++ .../src/specialists/spec-publisher.ts | 265 ++++++++++++++++++ .../src/specialists/spec-writer.test.ts | 178 +++++++++++- .../src/specialists/spec-writer.ts | 106 ++++++- packages/storage/src/data-dir.test.ts | 1 + packages/storage/src/data-dir.ts | 4 + 11 files changed, 1107 insertions(+), 25 deletions(-) create mode 100644 packages/orchestrator/src/specialists/fetch-product-context.test.ts create mode 100644 packages/orchestrator/src/specialists/fetch-product-context.ts create mode 100644 packages/orchestrator/src/specialists/spec-publisher.test.ts create mode 100644 packages/orchestrator/src/specialists/spec-publisher.ts diff --git a/apps/api/src/routes/dispatch.ts b/apps/api/src/routes/dispatch.ts index a2283ba..39c73d0 100644 --- a/apps/api/src/routes/dispatch.ts +++ b/apps/api/src/routes/dispatch.ts @@ -29,7 +29,9 @@ async function runDispatchJob( item: ItemState; store: ItemStore; workdir: string; + dataRoot: string; specialistId: string | undefined; + githubToken: string | undefined; }, ): Promise { const jobStore = await getJobStore(); @@ -44,7 +46,12 @@ async function runDispatchJob( ctx.product, runtime, (input) => ctx.store.transition(input), - { workdir: ctx.workdir, specialistId: ctx.specialistId }, + { + workdir: ctx.workdir, + dataRoot: ctx.dataRoot, + specialistId: ctx.specialistId, + githubToken: ctx.githubToken, + }, ); const now = new Date().toISOString(); @@ -163,7 +170,9 @@ dispatchRouter.post('/products/:slug/items/:externalId/dispatch', async (c) => { item, store, workdir, + dataRoot, specialistId: bodyResult.data.specialistId, + githubToken: process.env.GITHUB_TOKEN?.trim(), }); return c.json({ jobId: job.jobId, status: 'running' }, 202); diff --git a/packages/orchestrator/src/dispatcher.ts b/packages/orchestrator/src/dispatcher.ts index 45726c6..ff81fd1 100644 --- a/packages/orchestrator/src/dispatcher.ts +++ b/packages/orchestrator/src/dispatcher.ts @@ -5,6 +5,8 @@ import type { Product } from '@helm/shared'; import type { IAgentRuntime } from './runtime.js'; import type { ItemTransitionFn } from './specialists/spec-writer.js'; import { buildSpecWriterParams, handleSpecWriterResult } from './specialists/spec-writer.js'; +import { fetchProductContext } from './specialists/fetch-product-context.js'; +import type { FetchFn } from './specialists/fetch-product-context.js'; // ── Stage → specialist mapping ──────────────────────────────────────────────── // Expanded in later sessions (plan-writer, implementer, etc.) @@ -27,6 +29,8 @@ export type DispatchResult = { newStage?: WorkflowStage; costUsd: number; durationMs: number; + /** URL of the knowledge-repo PR opened by the publish step, if applicable. */ + prUrl?: string; error?: string; }; @@ -35,6 +39,23 @@ export type DispatchOptions = { workdir?: string; /** Override the specialist determined by stage mapping. */ specialistId?: string; + /** + * Absolute path to the Helm data root (e.g. HELM_DATA_DIR or cwd/data). + * Used to locate `knowledge-repos/{productSlug}/` for the publish step. + * Required for the publish step to run. + */ + dataRoot?: string; + /** + * GitHub personal access token (repo scope). + * Required for product context fetching (Part A) and spec publishing (Part B). + * When absent, both features are silently skipped. + */ + githubToken?: string; + /** + * Injectable HTTP fetch function — for testing the context-fetch path. + * Defaults to the global fetch. + */ + fetchFn?: FetchFn; }; // ── Dispatcher ──────────────────────────────────────────────────────────────── @@ -89,15 +110,38 @@ export async function dispatchStageHandler( // Route to specialist if (specialistId === 'spec-writer') { - const params = buildSpecWriterParams(item.externalId, product, workdir); + // ── Part A: Fetch product context (README + agent instructions) ────────── + // Skipped gracefully when no token is provided. + const context = options?.githubToken + ? await fetchProductContext(product, options.githubToken, options.fetchFn).catch((err) => { + console.error( + '[dispatcher] Failed to fetch product context (continuing without it):', + err, + ); + return undefined; + }) + : undefined; + + const params = buildSpecWriterParams(item.externalId, product, workdir, context); const session = await runtime.spawn(params); const agentResult = await session.wait(); + // ── Part B: Publish spec to knowledge repo (optional) ──────────────────── + const publishOpts = + options?.githubToken && options?.dataRoot + ? { + product, + knowledgeRepoLocalPath: join(options.dataRoot, 'knowledge-repos', item.productSlug), + githubToken: options.githubToken, + } + : undefined; + const specResult = await handleSpecWriterResult( item.externalId, agentResult, workdir, transition, + publishOpts, ); return { @@ -106,6 +150,7 @@ export async function dispatchStageHandler( newStage: specResult.newStage, costUsd: agentResult.totalCostUsd, durationMs: agentResult.durationMs, + prUrl: specResult.prUrl, error: specResult.error, }; } diff --git a/packages/orchestrator/src/index.ts b/packages/orchestrator/src/index.ts index b676f50..c44f131 100644 --- a/packages/orchestrator/src/index.ts +++ b/packages/orchestrator/src/index.ts @@ -16,9 +16,24 @@ export type { SubprocessLike, SpawnFn } from './runtimes/claude-code.js'; export { dispatchStageHandler } from './dispatcher.js'; export type { DispatchInput, DispatchResult, DispatchOptions } from './dispatcher.js'; -export type { ItemTransitionFn, SpecWriterResult } from './specialists/spec-writer.js'; +export type { + ItemTransitionFn, + SpecWriterResult, + SpecPublishOptions, +} from './specialists/spec-writer.js'; export { buildSpecWriterPrompt, buildSpecWriterParams, handleSpecWriterResult, } from './specialists/spec-writer.js'; + +export { fetchProductContext, parseGitHubRepoUrl } from './specialists/fetch-product-context.js'; +export type { ProductContext, FetchFn } from './specialists/fetch-product-context.js'; + +export { publishSpecToPR } from './specialists/spec-publisher.js'; +export type { + PublishSpecOpts, + PublishSpecResult, + RunGit, + RunGh, +} from './specialists/spec-publisher.js'; diff --git a/packages/orchestrator/src/specialists/fetch-product-context.test.ts b/packages/orchestrator/src/specialists/fetch-product-context.test.ts new file mode 100644 index 0000000..75687b6 --- /dev/null +++ b/packages/orchestrator/src/specialists/fetch-product-context.test.ts @@ -0,0 +1,162 @@ +import { describe, expect, it, vi } from 'vitest'; +import { fetchProductContext, parseGitHubRepoUrl } from './fetch-product-context.js'; +import type { FetchFn } from './fetch-product-context.js'; +import type { Product } from '@helm/shared'; + +// ── Fixtures ────────────────────────────────────────────────────────────────── + +const makeProduct = (): Product => ({ + helm_version: '0', + product: { slug: 'my-product', name: 'My Product' }, + issue_tracker: { + provider: 'github_projects', + org: 'test-org', + project_number: 1, + custom_field_name: 'Helm Stage', + }, + code_repos: [ + { url: 'https://github.com/test-org/test-repo', default_branch: 'main', role: 'app' }, + ], + knowledge_repo: { url: 'https://github.com/test-org/knowledge', default_branch: 'main' }, + workflow: { + stages_enabled: ['discovery', 'spec-draft', 'released'], + designer_gate: 'skip', + qa_gate: 'skip', + }, + specialists: { + spec_writer: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + plan_writer: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + implementer: { runtime: 'claude_code', model: 'claude-opus-4-7' }, + code_reviewer: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + security_reviewer: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + test_reviewer: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + remediation: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + }, +}); + +/** Returns a minimal mock Response-like object. */ +const okResponse = (body: string) => ({ ok: true, text: () => Promise.resolve(body) }) as Response; +const notFound = () => + ({ ok: false, status: 404, text: () => Promise.resolve('Not Found') }) as Response; + +// ── parseGitHubRepoUrl ──────────────────────────────────────────────────────── + +describe('parseGitHubRepoUrl', () => { + it('parses standard HTTPS URL', () => { + expect(parseGitHubRepoUrl('https://github.com/owner/repo')).toEqual({ + owner: 'owner', + repo: 'repo', + }); + }); + + it('strips .git suffix', () => { + expect(parseGitHubRepoUrl('https://github.com/owner/repo.git')).toEqual({ + owner: 'owner', + repo: 'repo', + }); + }); + + it('handles trailing slash', () => { + expect(parseGitHubRepoUrl('https://github.com/owner/repo/')).toEqual({ + owner: 'owner', + repo: 'repo', + }); + }); + + it('returns null for non-GitHub URLs', () => { + expect(parseGitHubRepoUrl('https://gitlab.com/owner/repo')).toBeNull(); + expect(parseGitHubRepoUrl('not-a-url')).toBeNull(); + }); +}); + +// ── fetchProductContext ─────────────────────────────────────────────────────── + +describe('fetchProductContext', () => { + it('returns readme and agentMd when both files exist', async () => { + const mockFetch: FetchFn = vi.fn().mockImplementation((url: string) => { + if ((url as string).includes('README.md')) return Promise.resolve(okResponse('# My README')); + if ((url as string).includes('AGENT.md')) + return Promise.resolve(okResponse('# Agent instructions')); + return Promise.resolve(notFound()); + }); + + const ctx = await fetchProductContext(makeProduct(), 'tok', mockFetch); + + expect(ctx.readme).toBe('# My README'); + expect(ctx.agentMd).toBe('# Agent instructions'); + }); + + it('falls back to CLAUDE.md when AGENT.md is absent', async () => { + const mockFetch: FetchFn = vi.fn().mockImplementation((url: string) => { + if ((url as string).includes('README.md')) return Promise.resolve(okResponse('readme')); + if ((url as string).includes('AGENT.md')) return Promise.resolve(notFound()); + if ((url as string).includes('CLAUDE.md')) return Promise.resolve(okResponse('claude md')); + return Promise.resolve(notFound()); + }); + + const ctx = await fetchProductContext(makeProduct(), 'tok', mockFetch); + + expect(ctx.readme).toBe('readme'); + expect(ctx.agentMd).toBe('claude md'); + }); + + it('returns undefined fields when files are absent (404)', async () => { + const mockFetch: FetchFn = vi.fn().mockResolvedValue(notFound()); + + const ctx = await fetchProductContext(makeProduct(), 'tok', mockFetch); + + expect(ctx.readme).toBeUndefined(); + expect(ctx.agentMd).toBeUndefined(); + }); + + it('truncates README longer than 2000 chars', async () => { + const longReadme = 'x'.repeat(3000); + const mockFetch: FetchFn = vi.fn().mockImplementation((url: string) => { + if ((url as string).includes('README.md')) return Promise.resolve(okResponse(longReadme)); + return Promise.resolve(notFound()); + }); + + const ctx = await fetchProductContext(makeProduct(), 'tok', mockFetch); + + expect(ctx.readme).toBeDefined(); + expect(ctx.readme!.length).toBeLessThan(longReadme.length); + expect(ctx.readme!).toContain('[...truncated]'); + }); + + it('passes Authorization header with token', async () => { + const mockFetch: FetchFn = vi.fn().mockResolvedValue(notFound()); + + await fetchProductContext(makeProduct(), 'my-token', mockFetch); + + const calls = (mockFetch as ReturnType).mock.calls as [ + string, + { headers: Record }, + ][]; + expect(calls.length).toBeGreaterThan(0); + expect(calls[0][1].headers.Authorization).toBe('Bearer my-token'); + }); + + it('returns empty context when product has no code repos', async () => { + const mockFetch: FetchFn = vi.fn(); + const product = { ...makeProduct(), code_repos: [] } as unknown as Product; + + const ctx = await fetchProductContext(product, 'tok', mockFetch); + + expect(ctx.readme).toBeUndefined(); + expect(ctx.agentMd).toBeUndefined(); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it('fetches from the correct raw.githubusercontent.com URL', async () => { + const mockFetch: FetchFn = vi.fn().mockResolvedValue(notFound()); + + await fetchProductContext(makeProduct(), 'tok', mockFetch); + + const urls = (mockFetch as ReturnType).mock.calls.map( + (c: unknown[]) => c[0] as string, + ); + expect( + urls.some((u) => u.startsWith('https://raw.githubusercontent.com/test-org/test-repo/main/')), + ).toBe(true); + }); +}); diff --git a/packages/orchestrator/src/specialists/fetch-product-context.ts b/packages/orchestrator/src/specialists/fetch-product-context.ts new file mode 100644 index 0000000..fba72be --- /dev/null +++ b/packages/orchestrator/src/specialists/fetch-product-context.ts @@ -0,0 +1,99 @@ +import type { Product } from '@helm/shared'; + +// ── Types ───────────────────────────────────────────────────────────────────── + +export type ProductContext = { + /** Truncated README.md from the primary code repo, or undefined if unavailable. */ + readme?: string; + /** Truncated AGENT.md or CLAUDE.md from the primary code repo, or undefined. */ + agentMd?: string; +}; + +/** Injectable HTTP fetcher for testing — defaults to the global fetch. */ +export type FetchFn = typeof fetch; + +// ── Constants ───────────────────────────────────────────────────────────────── + +const MAX_CHARS = 2000; +const TRUNCATION_SUFFIX = '\n\n[...truncated]'; + +// ── Helpers ─────────────────────────────────────────────────────────────────── + +/** + * Parses a GitHub HTTPS URL into owner/repo. + * Handles trailing .git, trailing slashes, and optional paths after the repo. + */ +export function parseGitHubRepoUrl(url: string): { owner: string; repo: string } | null { + const match = url.match(/github\.com\/([^/]+)\/([^/.]+?)(?:\.git)?(?:[/?#]|$)/); + return match ? { owner: match[1]!, repo: match[2]! } : null; +} + +/** + * Fetches raw file content from GitHub via raw.githubusercontent.com. + * Returns null on 404 or any non-2xx response (treated as "file does not exist"). + * Throws on network errors. + */ +async function fetchRawFile( + owner: string, + repo: string, + branch: string, + path: string, + token: string, + fetchFn: FetchFn, +): Promise { + const url = `https://raw.githubusercontent.com/${owner}/${repo}/${branch}/${path}`; + const res = await fetchFn(url, { + headers: { Authorization: `Bearer ${token}` }, + }); + if (!res.ok) return null; + return res.text(); +} + +function truncate(content: string): string { + if (content.length <= MAX_CHARS) return content; + return content.slice(0, MAX_CHARS) + TRUNCATION_SUFFIX; +} + +// ── Main export ─────────────────────────────────────────────────────────────── + +/** + * Fetches README and agent instructions from the product's primary code repo. + * All fields are optional — a missing file is not an error. + * + * Design rationale: the spec-writer only needs to understand the domain, not + * modify code. Fetching README + AGENT.md via the GitHub contents API is + * sufficient and avoids the overhead of a full git clone (which is reserved + * for the implementer specialist that needs to modify files). + * + * @param product The parsed product config. + * @param token GitHub personal access token (repo scope). + * @param fetchFn HTTP fetch function — injectable for testing. + */ +export async function fetchProductContext( + product: Product, + token: string, + fetchFn: FetchFn = fetch, +): Promise { + const primaryRepo = product.code_repos[0]; + if (!primaryRepo) return {}; + + const parsed = parseGitHubRepoUrl(primaryRepo.url); + if (!parsed) return {}; + + const { owner, repo } = parsed; + const branch = primaryRepo.default_branch; + + // Fetch README and agent instructions concurrently. + const [readmeRaw, agentMdRaw] = await Promise.all([ + fetchRawFile(owner, repo, branch, 'README.md', token, fetchFn), + // Try AGENT.md first; fall back to CLAUDE.md. + fetchRawFile(owner, repo, branch, 'AGENT.md', token, fetchFn).then((content) => + content !== null ? content : fetchRawFile(owner, repo, branch, 'CLAUDE.md', token, fetchFn), + ), + ]); + + return { + readme: readmeRaw !== null ? truncate(readmeRaw) : undefined, + agentMd: agentMdRaw !== null ? truncate(agentMdRaw) : undefined, + }; +} diff --git a/packages/orchestrator/src/specialists/spec-publisher.test.ts b/packages/orchestrator/src/specialists/spec-publisher.test.ts new file mode 100644 index 0000000..39752e6 --- /dev/null +++ b/packages/orchestrator/src/specialists/spec-publisher.test.ts @@ -0,0 +1,242 @@ +import { mkdir, rm, writeFile, stat } from 'node:fs/promises'; +import { randomUUID } from 'node:crypto'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { publishSpecToPR } from './spec-publisher.js'; +import type { PublishSpecOpts, RunGit, RunGh } from './spec-publisher.js'; +import type { Product } from '@helm/shared'; + +// ── Fixtures ────────────────────────────────────────────────────────────────── + +const makeProduct = (): Product => ({ + helm_version: '0', + product: { slug: 'my-product', name: 'My Product' }, + issue_tracker: { + provider: 'github_projects', + org: 'test-org', + project_number: 1, + custom_field_name: 'Helm Stage', + }, + code_repos: [ + { url: 'https://github.com/test-org/test-repo', default_branch: 'main', role: 'app' }, + ], + knowledge_repo: { url: 'https://github.com/test-org/knowledge', default_branch: 'main' }, + workflow: { + stages_enabled: ['discovery', 'spec-draft', 'released'], + designer_gate: 'skip', + qa_gate: 'skip', + }, + specialists: { + spec_writer: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + plan_writer: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + implementer: { runtime: 'claude_code', model: 'claude-opus-4-7' }, + code_reviewer: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + security_reviewer: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + test_reviewer: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + remediation: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + }, +}); + +// ── Mock runners ────────────────────────────────────────────────────────────── + +/** + * Builds a mock runGit that simulates git operations on the real local + * filesystem. clone creates a .git dir; other commands are no-ops. + */ +function makeRunGit(cloneTargetPath: string): RunGit { + return vi.fn().mockImplementation(async (args: string[]) => { + if (args[0] === 'clone') { + // Simulate clone by creating .git marker dir + await mkdir(join(cloneTargetPath, '.git'), { recursive: true }); + } + return { stdout: '' }; + }); +} + +/** Builds a mock runGh that returns an empty PR list (no existing PR). */ +function makeRunGh(prListResult: { url: string }[] = []): RunGh { + return vi.fn().mockImplementation(async (args: string[]) => { + if (args[0] === 'pr' && args[1] === 'list') { + return { stdout: JSON.stringify(prListResult) }; + } + if (args[0] === 'pr' && args[1] === 'create') { + return { stdout: 'https://github.com/test-org/knowledge/pull/42\n' }; + } + return { stdout: '' }; + }); +} + +// ── Setup ───────────────────────────────────────────────────────────────────── + +let tmpDir: string; +let specPath: string; +let knowledgeRepoLocalPath: string; + +beforeEach(async () => { + tmpDir = join(tmpdir(), `spec-publisher-${randomUUID()}`); + await mkdir(tmpDir, { recursive: true }); + + // Create a spec file to publish + const specsDir = join(tmpDir, 'workdir', 'specs'); + await mkdir(specsDir, { recursive: true }); + specPath = join(specsDir, 'issue_1.md'); + await writeFile(specPath, '# issue_1 — Specification\n'); + + knowledgeRepoLocalPath = join(tmpDir, 'knowledge-repos', 'my-product'); +}); + +afterEach(async () => { + await rm(tmpDir, { recursive: true, force: true }); +}); + +// ── Helpers ─────────────────────────────────────────────────────────────────── + +const makeOpts = (overrides?: Partial): PublishSpecOpts => ({ + externalId: 'issue_1', + product: makeProduct(), + specPath, + knowledgeRepoLocalPath, + githubToken: 'test-token', + ...overrides, +}); + +// ── Tests ───────────────────────────────────────────────────────────────────── + +describe('publishSpecToPR', () => { + it('clones repo, copies spec, commits, pushes, and creates PR', async () => { + const runGit = makeRunGit(knowledgeRepoLocalPath); + const runGh = makeRunGh(); + + const result = await publishSpecToPR(makeOpts(), runGit, runGh); + + expect(result.prUrl).toBe('https://github.com/test-org/knowledge/pull/42'); + + // Spec file should exist in the knowledge repo clone + const destSpec = join(knowledgeRepoLocalPath, 'specs', 'issue_1.md'); + await expect(stat(destSpec)).resolves.toBeDefined(); + + // git was called with clone, checkout -B, add, commit, push + // mock.calls[i] = [args, opts] since runGit takes two parameters + const gitCalls = (runGit as ReturnType).mock.calls as [string[], unknown][]; + expect(gitCalls.some(([args]) => args[0] === 'clone')).toBe(true); + expect(gitCalls.some(([args]) => args[0] === 'checkout' && args[1] === '-B')).toBe(true); + expect(gitCalls.some(([args]) => args[0] === 'add')).toBe(true); + expect(gitCalls.some(([args]) => args[0] === 'commit')).toBe(true); + expect(gitCalls.some(([args]) => args[0] === 'push')).toBe(true); + }); + + it('reuses existing open PR without creating a duplicate', async () => { + const runGit = makeRunGit(knowledgeRepoLocalPath); + const existingPrUrl = 'https://github.com/test-org/knowledge/pull/7'; + const runGh = makeRunGh([{ url: existingPrUrl }]); + + const result = await publishSpecToPR(makeOpts(), runGit, runGh); + + expect(result.prUrl).toBe(existingPrUrl); + + // mock.calls[i] = [args, opts] + const ghCalls = (runGh as ReturnType).mock.calls as [string[], unknown][]; + // pr create must NOT have been called + expect(ghCalls.some(([args]) => args[0] === 'pr' && args[1] === 'create')).toBe(false); + }); + + it('skips clone and pulls when knowledge repo already exists', async () => { + // Pre-create .git directory to simulate an already-cloned repo + await mkdir(join(knowledgeRepoLocalPath, '.git'), { recursive: true }); + const runGit: RunGit = vi.fn().mockResolvedValue({ stdout: '' }); + const runGh = makeRunGh(); + + await publishSpecToPR(makeOpts(), runGit, runGh); + + // mock.calls[i] = [args, opts] + const gitCalls = (runGit as ReturnType).mock.calls as [string[], unknown][]; + // Should fetch + checkout + reset — NOT clone + expect(gitCalls.some(([args]) => args[0] === 'clone')).toBe(false); + expect(gitCalls.some(([args]) => args[0] === 'fetch')).toBe(true); + expect(gitCalls.some(([args]) => args[0] === 'reset')).toBe(true); + }); + + it('throws with a descriptive message when clone fails', async () => { + const runGit: RunGit = vi.fn().mockImplementation(async (args: string[]) => { + if (args[0] === 'clone') throw new Error('Authentication failed'); + return { stdout: '' }; + }); + const runGh = makeRunGh(); + + await expect(publishSpecToPR(makeOpts(), runGit, runGh)).rejects.toThrow( + /Failed to clone\/update knowledge repo.*Authentication failed/, + ); + }); + + it('throws with a descriptive message when push fails', async () => { + const runGit: RunGit = vi.fn().mockImplementation(async (args: string[]) => { + if (args[0] === 'clone') { + await mkdir(join(knowledgeRepoLocalPath, '.git'), { recursive: true }); + return { stdout: '' }; + } + if (args[0] === 'push') throw new Error('remote: Permission to repo denied'); + return { stdout: '' }; + }); + const runGh = makeRunGh(); + + await expect(publishSpecToPR(makeOpts(), runGit, runGh)).rejects.toThrow( + /Failed to push branch.*Permission to repo denied/, + ); + }); + + it('throws with a descriptive message when PR creation fails', async () => { + const runGit = makeRunGit(knowledgeRepoLocalPath); + const runGh: RunGh = vi.fn().mockImplementation(async (args: string[]) => { + if (args[0] === 'pr' && args[1] === 'list') return { stdout: '[]' }; + if (args[0] === 'pr' && args[1] === 'create') throw new Error('rate limit exceeded'); + return { stdout: '' }; + }); + + await expect(publishSpecToPR(makeOpts(), runGit, runGh)).rejects.toThrow( + /Failed to create PR.*rate limit exceeded/, + ); + }); + + it('throws when knowledge repo URL cannot be parsed', async () => { + const runGit: RunGit = vi.fn().mockResolvedValue({ stdout: '' }); + const runGh = makeRunGh(); + + const opts = makeOpts(); + const badProduct: Product = { + ...makeProduct(), + knowledge_repo: { url: 'https://gitlab.com/owner/repo', default_branch: 'main' }, + }; + + await expect(publishSpecToPR({ ...opts, product: badProduct }, runGit, runGh)).rejects.toThrow( + /Cannot parse knowledge repo URL/, + ); + }); + + it('uses a branch name derived from the externalId', async () => { + const runGit = makeRunGit(knowledgeRepoLocalPath); + const runGh = makeRunGh(); + + await publishSpecToPR(makeOpts({ externalId: 'HLM-99' }), runGit, runGh); + + // mock.calls[i] = [args, opts] + const gitCalls = (runGit as ReturnType).mock.calls as [string[], unknown][]; + const checkoutCall = gitCalls.find(([args]) => args[0] === 'checkout' && args[1] === '-B'); + expect(checkoutCall).toBeDefined(); + expect(checkoutCall![0][2]).toBe('helm/spec/HLM-99'); + }); + + it('uses token-embedded URL for clone and push', async () => { + const runGit = makeRunGit(knowledgeRepoLocalPath); + const runGh = makeRunGh(); + + await publishSpecToPR(makeOpts({ githubToken: 'secret-token' }), runGit, runGh); + + // mock.calls[i] = [args, opts] + const gitCalls = (runGit as ReturnType).mock.calls as [string[], unknown][]; + const cloneCall = gitCalls.find(([args]) => args[0] === 'clone'); + const pushCall = gitCalls.find(([args]) => args[0] === 'push'); + expect(cloneCall![0][1]).toContain('x-access-token:secret-token@'); + expect(pushCall![0][1]).toContain('x-access-token:secret-token@'); + }); +}); diff --git a/packages/orchestrator/src/specialists/spec-publisher.ts b/packages/orchestrator/src/specialists/spec-publisher.ts new file mode 100644 index 0000000..ee3f4ce --- /dev/null +++ b/packages/orchestrator/src/specialists/spec-publisher.ts @@ -0,0 +1,265 @@ +import { copyFile, mkdir, stat } from 'node:fs/promises'; +import { execFile } from 'node:child_process'; +import { join, dirname } from 'node:path'; +import { promisify } from 'node:util'; +import type { Product } from '@helm/shared'; +import { parseGitHubRepoUrl } from './fetch-product-context.js'; + +const execFileAsync = promisify(execFile); + +// ── Types ───────────────────────────────────────────────────────────────────── + +export type PublishSpecOpts = { + externalId: string; + product: Product; + /** Absolute path to specs/{externalId}.md in the workdir. */ + specPath: string; + /** Absolute path where the knowledge repo should be cloned / already lives. */ + knowledgeRepoLocalPath: string; + /** GitHub personal access token (repo scope). */ + githubToken: string; +}; + +export type PublishSpecResult = { + prUrl: string; +}; + +/** + * Injectable runner for git commands — receives an arg list and cwd. + * Resolves with stdout; throws on non-zero exit. + */ +export type RunGit = ( + args: string[], + opts: { cwd: string; env?: NodeJS.ProcessEnv }, +) => Promise<{ stdout: string }>; + +/** + * Injectable runner for gh commands — receives an arg list. + * Resolves with stdout; throws on non-zero exit. + */ +export type RunGh = ( + args: string[], + opts: { env?: NodeJS.ProcessEnv }, +) => Promise<{ stdout: string }>; + +// ── Default runners ─────────────────────────────────────────────────────────── + +export const defaultRunGit: RunGit = async (args, opts) => { + const { stdout } = await execFileAsync('/usr/bin/git', args, { + cwd: opts.cwd, + env: { ...process.env, ...opts.env, GIT_TERMINAL_PROMPT: '0' }, + }); + return { stdout }; +}; + +export const defaultRunGh: RunGh = async (args, opts) => { + const ghPath = '/opt/homebrew/bin/gh'; + const { stdout } = await execFileAsync(ghPath, args, { + env: { ...process.env, ...opts.env }, + }); + return { stdout }; +}; + +// ── Helpers ─────────────────────────────────────────────────────────────────── + +async function pathExists(p: string): Promise { + try { + await stat(p); + return true; + } catch { + return false; + } +} + +/** + * Ensures the knowledge repo is cloned and up to date. + * If not yet cloned → git clone (with token-embedded HTTPS URL). + * If already cloned → fetch + reset to latest default branch. + */ +async function ensureKnowledgeRepo( + localPath: string, + repoUrl: string, + defaultBranch: string, + githubToken: string, + runGit: RunGit, +): Promise { + // Token-embedded URL so git doesn't prompt for credentials. + const authenticatedUrl = repoUrl.replace('https://', `https://x-access-token:${githubToken}@`); + + const gitDir = join(localPath, '.git'); + if (!(await pathExists(gitDir))) { + await mkdir(dirname(localPath), { recursive: true }); + await runGit(['clone', authenticatedUrl, localPath], { cwd: dirname(localPath) }); + } else { + // Pull latest without interactive prompts. + await runGit(['fetch', 'origin'], { cwd: localPath }); + await runGit(['checkout', defaultBranch], { cwd: localPath }); + await runGit(['reset', '--hard', `origin/${defaultBranch}`], { cwd: localPath }); + } +} + +// ── Main export ─────────────────────────────────────────────────────────────── + +/** + * Publishes a spec file to the product's knowledge repo as a PR. + * + * Idempotency: if the branch `helm/spec/{externalId}` already has an open PR, + * the branch is updated (force-pushed) and the existing PR URL is returned — + * no duplicate PRs are created. + * + * @param opts Publish options including product config, spec path, and token. + * @param runGit Injectable git runner (defaults to /usr/bin/git via execFile). + * @param runGh Injectable gh runner (defaults to /opt/homebrew/bin/gh). + */ +export async function publishSpecToPR( + opts: PublishSpecOpts, + runGit: RunGit = defaultRunGit, + runGh: RunGh = defaultRunGh, +): Promise { + const { externalId, product, specPath, knowledgeRepoLocalPath, githubToken } = opts; + + const knowledgeRepo = product.knowledge_repo; + const defaultBranch = knowledgeRepo.default_branch; + const branchName = `helm/spec/${externalId}`; + + const parsed = parseGitHubRepoUrl(knowledgeRepo.url); + if (!parsed) { + throw new Error(`[spec-publisher] Cannot parse knowledge repo URL: ${knowledgeRepo.url}`); + } + const { owner, repo } = parsed; + + // ── Step 1: Clone or update knowledge repo ─────────────────────────────── + try { + await ensureKnowledgeRepo( + knowledgeRepoLocalPath, + knowledgeRepo.url, + defaultBranch, + githubToken, + runGit, + ); + } catch (err) { + throw new Error( + `[spec-publisher] Failed to clone/update knowledge repo: ${err instanceof Error ? err.message : String(err)}`, + ); + } + + // ── Step 2: Create or reset branch from default branch ────────────────── + try { + // -B creates the branch if absent, or resets it to HEAD if it already exists. + await runGit(['checkout', '-B', branchName, `origin/${defaultBranch}`], { + cwd: knowledgeRepoLocalPath, + }); + } catch (err) { + throw new Error( + `[spec-publisher] Failed to create branch '${branchName}': ${err instanceof Error ? err.message : String(err)}`, + ); + } + + // ── Step 3: Copy spec into knowledge repo ──────────────────────────────── + const destDir = join(knowledgeRepoLocalPath, 'specs'); + const destPath = join(destDir, `${externalId}.md`); + try { + await mkdir(destDir, { recursive: true }); + await copyFile(specPath, destPath); + } catch (err) { + throw new Error( + `[spec-publisher] Failed to copy spec file: ${err instanceof Error ? err.message : String(err)}`, + ); + } + + // ── Step 4: Commit ─────────────────────────────────────────────────────── + const gitEnv: NodeJS.ProcessEnv = { + GIT_AUTHOR_NAME: 'helm-bot', + GIT_AUTHOR_EMAIL: 'helm-bot@users.noreply.github.com', + GIT_COMMITTER_NAME: 'helm-bot', + GIT_COMMITTER_EMAIL: 'helm-bot@users.noreply.github.com', + }; + try { + await runGit(['add', join('specs', `${externalId}.md`)], { cwd: knowledgeRepoLocalPath }); + await runGit(['commit', '--allow-empty', '-m', `docs(spec): add spec for ${externalId}`], { + cwd: knowledgeRepoLocalPath, + env: gitEnv, + }); + } catch (err) { + throw new Error( + `[spec-publisher] Failed to commit spec: ${err instanceof Error ? err.message : String(err)}`, + ); + } + + // ── Step 5: Push branch ────────────────────────────────────────────────── + const authenticatedUrl = knowledgeRepo.url.replace( + 'https://', + `https://x-access-token:${githubToken}@`, + ); + try { + await runGit(['push', authenticatedUrl, `${branchName}:${branchName}`, '--force'], { + cwd: knowledgeRepoLocalPath, + }); + } catch (err) { + throw new Error( + `[spec-publisher] Failed to push branch '${branchName}': ${err instanceof Error ? err.message : String(err)}`, + ); + } + + // ── Step 6: Open PR (idempotent) ───────────────────────────────────────── + const ghEnv: NodeJS.ProcessEnv = { GITHUB_TOKEN: githubToken }; + + // Check for an existing open PR before creating a new one. + let existingPrUrl: string | null = null; + try { + const listOut = await runGh( + [ + 'pr', + 'list', + '--repo', + `${owner}/${repo}`, + '--head', + branchName, + '--state', + 'open', + '--json', + 'url', + ], + { env: ghEnv }, + ); + const prs = JSON.parse(listOut.stdout.trim()) as { url: string }[]; + if (prs.length > 0) existingPrUrl = prs[0]!.url; + } catch (err) { + throw new Error( + `[spec-publisher] Failed to check for existing PRs: ${err instanceof Error ? err.message : String(err)}`, + ); + } + + if (existingPrUrl) { + return { prUrl: existingPrUrl }; + } + + // No open PR exists — create one. + let prUrl: string; + try { + const createOut = await runGh( + [ + 'pr', + 'create', + '--repo', + `${owner}/${repo}`, + '--head', + branchName, + '--base', + defaultBranch, + '--title', + `docs(spec): add spec for ${externalId}`, + '--body', + `Spec generated by Helm for item \`${externalId}\` in product \`${product.product.slug}\`.\n\nReview and merge to advance the item to **spec-ready**.`, + ], + { env: ghEnv }, + ); + prUrl = createOut.stdout.trim(); + } catch (err) { + throw new Error( + `[spec-publisher] Failed to create PR: ${err instanceof Error ? err.message : String(err)}`, + ); + } + + return { prUrl }; +} diff --git a/packages/orchestrator/src/specialists/spec-writer.test.ts b/packages/orchestrator/src/specialists/spec-writer.test.ts index cc4fe3c..3462606 100644 --- a/packages/orchestrator/src/specialists/spec-writer.test.ts +++ b/packages/orchestrator/src/specialists/spec-writer.test.ts @@ -3,11 +3,41 @@ import { randomUUID } from 'node:crypto'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { handleSpecWriterResult } from './spec-writer.js'; +import { buildSpecWriterPrompt, handleSpecWriterResult } from './spec-writer.js'; import type { AgentResult } from '../runtime.js'; -import type { ItemTransitionFn } from './spec-writer.js'; +import type { ItemTransitionFn, SpecPublishOptions } from './spec-writer.js'; +import type { Product } from '@helm/shared'; -// ── Helpers ─────────────────────────────────────────────────────────────────── +// ── Fixtures ────────────────────────────────────────────────────────────────── + +const makeProduct = (): Product => ({ + helm_version: '0', + product: { slug: 'test-product', name: 'Test Product' }, + issue_tracker: { + provider: 'github_projects', + org: 'test-org', + project_number: 1, + custom_field_name: 'Helm Stage', + }, + code_repos: [ + { url: 'https://github.com/test-org/test-repo', default_branch: 'main', role: 'app' }, + ], + knowledge_repo: { url: 'https://github.com/test-org/knowledge', default_branch: 'main' }, + workflow: { + stages_enabled: ['discovery', 'spec-draft', 'released'], + designer_gate: 'skip', + qa_gate: 'skip', + }, + specialists: { + spec_writer: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + plan_writer: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + implementer: { runtime: 'claude_code', model: 'claude-opus-4-7' }, + code_reviewer: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + security_reviewer: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + test_reviewer: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + remediation: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + }, +}); const doneResult = (overrides?: Partial): AgentResult => ({ status: 'done', @@ -17,7 +47,44 @@ const doneResult = (overrides?: Partial): AgentResult => ({ ...overrides, }); -// ── Tests ───────────────────────────────────────────────────────────────────── +// ── buildSpecWriterPrompt ───────────────────────────────────────────────────── + +describe('buildSpecWriterPrompt', () => { + it('includes product name and externalId without context', () => { + const prompt = buildSpecWriterPrompt('issue_1', makeProduct()); + expect(prompt).toContain('Test Product'); + expect(prompt).toContain('issue_1'); + expect(prompt).not.toContain('Product Context'); + }); + + it('injects Product Context section when context is provided', () => { + const prompt = buildSpecWriterPrompt('issue_1', makeProduct(), { + readme: '# My Product README', + agentMd: 'Use TypeScript.', + }); + expect(prompt).toContain('## Product Context'); + expect(prompt).toContain('# My Product README'); + expect(prompt).toContain('Use TypeScript.'); + expect(prompt).toContain('test-product'); + expect(prompt).toContain('discovery → spec-draft → released'); + }); + + it('omits README subsection when context has no readme', () => { + const prompt = buildSpecWriterPrompt('issue_1', makeProduct(), { agentMd: 'instructions' }); + expect(prompt).toContain('## Product Context'); + expect(prompt).not.toContain('### README'); + expect(prompt).toContain('instructions'); + }); + + it('omits Agent Instructions subsection when context has no agentMd', () => { + const prompt = buildSpecWriterPrompt('issue_1', makeProduct(), { readme: '# Readme' }); + expect(prompt).toContain('## Product Context'); + expect(prompt).not.toContain('### Agent Instructions'); + expect(prompt).toContain('# Readme'); + }); +}); + +// ── handleSpecWriterResult ──────────────────────────────────────────────────── describe('handleSpecWriterResult', () => { let workdir: string; @@ -81,7 +148,6 @@ describe('handleSpecWriterResult', () => { }); it('returns error without transitioning when spec file was not created', async () => { - // workdir/specs exists but issue_1.md does NOT const result = await handleSpecWriterResult( 'issue_1', doneResult(), @@ -95,10 +161,6 @@ describe('handleSpecWriterResult', () => { }); it('transitions successfully when finalOutput contains a denial note but artifact exists', async () => { - // Regression guard for the Session 10 smoke-test finding: permission_denials - // is NOT a verdict — the agent can deny one tool and succeed via another. - // The handler must not inspect finalOutput for denial notes; artifact - // existence on disk is the only success criterion. await writeFile(join(workdir, 'specs', 'issue_1.md'), '# Spec'); const result = await handleSpecWriterResult( @@ -113,9 +175,6 @@ describe('handleSpecWriterResult', () => { }); it('logs finalOutput server-side and returns a generic error message to caller', async () => { - // finalOutput (stderr, timeout details) must NOT be exposed to callers — - // it could contain internal paths or implementation details. It is logged - // server-side so operators can investigate without leaking it upstream. const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); try { const result = await handleSpecWriterResult( @@ -127,9 +186,7 @@ describe('handleSpecWriterResult', () => { expect(result.transitioned).toBe(false); expect(result.error).toContain("status 'error'"); - // Raw finalOutput must NOT appear in the returned error string expect(result.error).not.toContain('command not found'); - // But it IS logged server-side for operator visibility expect(consoleSpy).toHaveBeenCalledWith( expect.stringContaining('[spec-writer]'), expect.objectContaining({ finalOutput: expect.stringContaining('command not found') }), @@ -167,4 +224,97 @@ describe('handleSpecWriterResult', () => { expect(result.transitioned).toBe(true); expect(transition).toHaveBeenCalledWith(expect.objectContaining({ externalId: 'HLM-42' })); }); + + // ── Publish step ─────────────────────────────────────────────────────────── + + it('calls publisher and includes prUrl in result when publishOpts provided', async () => { + await writeFile(join(workdir, 'specs', 'issue_1.md'), '# Spec'); + + const mockRunGit = vi.fn().mockResolvedValue({ stdout: '' }); + const mockRunGh = vi.fn().mockImplementation(async (args: string[]) => { + if (args[1] === 'list') return { stdout: '[]' }; + if (args[1] === 'create') return { stdout: 'https://github.com/test-org/knowledge/pull/1\n' }; + return { stdout: '' }; + }); + + // Pre-create .git so ensureKnowledgeRepo skips the real clone + const knowledgeRepoLocalPath = join(workdir, 'knowledge-repos', 'test-product'); + await mkdir(join(knowledgeRepoLocalPath, '.git'), { recursive: true }); + + const publishOpts: SpecPublishOptions = { + product: makeProduct(), + knowledgeRepoLocalPath, + githubToken: 'test-token', + runGit: mockRunGit, + runGh: mockRunGh, + }; + + const result = await handleSpecWriterResult( + 'issue_1', + doneResult(), + workdir, + transition as ItemTransitionFn, + publishOpts, + ); + + expect(result.transitioned).toBe(true); + expect(result.prUrl).toBe('https://github.com/test-org/knowledge/pull/1'); + expect(transition).toHaveBeenCalledWith( + expect.objectContaining({ + note: expect.stringContaining('https://github.com/test-org/knowledge/pull/1'), + }), + ); + }); + + it('returns error without transitioning when publish step fails', async () => { + await writeFile(join(workdir, 'specs', 'issue_1.md'), '# Spec'); + + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + const mockRunGit = vi.fn().mockRejectedValue(new Error('network timeout')); + const mockRunGh = vi.fn().mockResolvedValue({ stdout: '[]' }); + + const knowledgeRepoLocalPath = join(workdir, 'knowledge-repos', 'test-product'); + const publishOpts: SpecPublishOptions = { + product: makeProduct(), + knowledgeRepoLocalPath, + githubToken: 'test-token', + runGit: mockRunGit, + runGh: mockRunGh, + }; + + const result = await handleSpecWriterResult( + 'issue_1', + doneResult(), + workdir, + transition as ItemTransitionFn, + publishOpts, + ); + + expect(result.transitioned).toBe(false); + expect(result.error).toBeDefined(); + expect(transition).not.toHaveBeenCalled(); + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringContaining('[spec-writer]'), + expect.objectContaining({ error: expect.any(String) }), + ); + } finally { + consoleSpy.mockRestore(); + } + }); + + it('skips publish step and still transitions when publishOpts not provided', async () => { + await writeFile(join(workdir, 'specs', 'issue_1.md'), '# Spec'); + + const result = await handleSpecWriterResult( + 'issue_1', + doneResult(), + workdir, + transition as ItemTransitionFn, + // No publishOpts — backward-compatible path + ); + + expect(result.transitioned).toBe(true); + expect(result.prUrl).toBeUndefined(); + }); }); diff --git a/packages/orchestrator/src/specialists/spec-writer.ts b/packages/orchestrator/src/specialists/spec-writer.ts index 79f6d42..63b7683 100644 --- a/packages/orchestrator/src/specialists/spec-writer.ts +++ b/packages/orchestrator/src/specialists/spec-writer.ts @@ -3,6 +3,9 @@ import { join } from 'node:path'; import type { WorkflowStage } from '@helm/workflow'; import type { Product } from '@helm/shared'; import type { AgentResult, SpawnParams } from '../runtime.js'; +import type { ProductContext } from './fetch-product-context.js'; +import type { PublishSpecOpts, RunGit, RunGh } from './spec-publisher.js'; +import { publishSpecToPR } from './spec-publisher.js'; // ── Minimal transition interface ────────────────────────────────────────────── // Avoids a dependency on @helm/api internals. ItemStore.transition satisfies this. @@ -19,19 +22,59 @@ export type ItemTransitionFn = (input: { export type SpecWriterResult = { transitioned: boolean; newStage?: WorkflowStage; + /** URL of the opened knowledge-repo PR. Present on successful publish. */ + prUrl?: string; error?: string; }; // ── Prompt template ─────────────────────────────────────────────────────────── +/** + * Builds the "## Product Context" section injected into the spec-writer prompt. + * Returns an empty string when no context is available. + */ +function buildContextSection(product: Product, context: ProductContext): string { + const lines: string[] = [ + '## Product Context', + '', + `**Product:** ${product.product.name} (slug: \`${product.product.slug}\`)`, + `**Code repo:** ${product.code_repos[0]?.url ?? 'N/A'} (branch: \`${product.code_repos[0]?.default_branch ?? 'main'}\`)`, + `**Workflow stages:** ${product.workflow.stages_enabled.join(' → ')}`, + '', + ]; + + if (context.readme) { + lines.push('### README', '', context.readme, ''); + } + + if (context.agentMd) { + lines.push('### Agent Instructions', '', context.agentMd, ''); + } + + lines.push('---', ''); + return lines.join('\n'); +} + /** * Builds the initial prompt for the spec-writer specialist. - * The template is intentionally minimal for v0 — Sesión 10 will refine it. + * + * @param externalId The item identifier. + * @param product Parsed product config. + * @param context Optional product context (README, agent instructions). + * When provided, a "## Product Context" section is injected + * at the top of the prompt so the agent understands the + * product domain rather than guessing from the name alone. */ -export function buildSpecWriterPrompt(externalId: string, product: Product): string { +export function buildSpecWriterPrompt( + externalId: string, + product: Product, + context?: ProductContext, +): string { + const contextSection = context ? buildContextSection(product, context) : ''; + return `You are the spec writer for the product "${product.product.name}". -Your task: write a specification for item ${externalId}. +${contextSection}Your task: write a specification for item ${externalId}. Steps: 1. Review any existing context in the working directory. @@ -57,15 +100,18 @@ Product: ${product.product.slug}`; /** * Builds SpawnParams for the spec-writer specialist. + * + * @param context Optional product context — passed to buildSpecWriterPrompt. */ export function buildSpecWriterParams( externalId: string, product: Product, workdir: string, + context?: ProductContext, ): SpawnParams { return { specialistId: 'spec-writer', - prompt: buildSpecWriterPrompt(externalId, product), + prompt: buildSpecWriterPrompt(externalId, product, context), workdir, productSlug: product.product.slug, externalId, @@ -73,6 +119,22 @@ export function buildSpecWriterParams( }; } +// ── Publish options ─────────────────────────────────────────────────────────── + +/** + * Options that enable the publish-to-knowledge-repo step. + * When absent, the spec is written locally but not published. + */ +export type SpecPublishOptions = { + product: Product; + knowledgeRepoLocalPath: string; + githubToken: string; + /** Injectable git runner — defaults to /usr/bin/git. For testing. */ + runGit?: RunGit; + /** Injectable gh runner — defaults to /opt/homebrew/bin/gh. For testing. */ + runGh?: RunGh; +}; + // ── Post-completion handler ─────────────────────────────────────────────────── /** @@ -80,9 +142,12 @@ export function buildSpecWriterParams( * * On success: * 1. Verifies specs/{externalId}.md was created in workdir. - * 2. Transitions the item discovery → spec-draft. + * 2. If publishOpts provided: publishes spec to knowledge repo as a PR. + * 3. Transitions the item discovery → spec-draft. + * (Transition happens after PR is opened so the stage accurately reflects + * that the spec is under review.) * - * On agent error or missing file: + * On agent error, missing spec file, or publish failure: * Returns an error result without transitioning. */ export async function handleSpecWriterResult( @@ -90,6 +155,7 @@ export async function handleSpecWriterResult( agentResult: AgentResult, workdir: string, transition: ItemTransitionFn, + publishOpts?: SpecPublishOptions, ): Promise { if (agentResult.status !== 'done') { // Log diagnostics server-side (stderr content, timeout details, etc.) so @@ -117,14 +183,38 @@ export async function handleSpecWriterResult( }; } + // ── Publish step (optional) ─────────────────────────────────────────────── + let prUrl: string | undefined; + if (publishOpts) { + const publishInput: PublishSpecOpts = { + externalId, + product: publishOpts.product, + specPath, + knowledgeRepoLocalPath: publishOpts.knowledgeRepoLocalPath, + githubToken: publishOpts.githubToken, + }; + try { + const result = await publishSpecToPR(publishInput, publishOpts.runGit, publishOpts.runGh); + prUrl = result.prUrl; + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + console.error('[spec-writer] Publish step failed', { externalId, error: message }); + return { + transitioned: false, + error: message, + }; + } + } + + // ── Transition (after successful publish or when publish is skipped) ────── try { const updated = await transition({ externalId, toStage: 'spec-draft', triggeredBy: 'agent:spec-writer', - note: `Spec written to specs/${externalId}.md`, + note: `Spec written to specs/${externalId}.md${prUrl ? ` — PR: ${prUrl}` : ''}`, }); - return { transitioned: true, newStage: updated.currentStage }; + return { transitioned: true, newStage: updated.currentStage, prUrl }; } catch (err) { return { transitioned: false, diff --git a/packages/storage/src/data-dir.test.ts b/packages/storage/src/data-dir.test.ts index 7905710..15290df 100644 --- a/packages/storage/src/data-dir.test.ts +++ b/packages/storage/src/data-dir.test.ts @@ -37,6 +37,7 @@ describe('ensureDataDir', () => { expect(await isDir(paths.reviewers)).toBe(true); expect(await isDir(paths.items)).toBe(true); expect(await isDir(paths.jobs)).toBe(true); + expect(await isDir(paths.knowledgeRepos)).toBe(true); // linear-cache/items/ is pre-created even though it's not a top-level DataPaths field expect(await isDir(join(testDir, 'linear-cache', 'items'))).toBe(true); }); diff --git a/packages/storage/src/data-dir.ts b/packages/storage/src/data-dir.ts index 4e37aa0..9f5ddce 100644 --- a/packages/storage/src/data-dir.ts +++ b/packages/storage/src/data-dir.ts @@ -22,6 +22,8 @@ export type DataPaths = { items: string; /** data/jobs/ — one JSON file per async dispatch job */ jobs: string; + /** data/knowledge-repos/ — one subdir per product slug; cloned knowledge repos */ + knowledgeRepos: string; }; /** @@ -39,6 +41,7 @@ export async function ensureDataDir(rootPath: string): Promise { reviewers: join(rootPath, 'reviewers'), items: join(rootPath, 'items'), jobs: join(rootPath, 'jobs'), + knowledgeRepos: join(rootPath, 'knowledge-repos'), }; await Promise.all([ @@ -49,6 +52,7 @@ export async function ensureDataDir(rootPath: string): Promise { mkdir(paths.reviewers, { recursive: true }), mkdir(paths.items, { recursive: true }), mkdir(paths.jobs, { recursive: true }), + mkdir(paths.knowledgeRepos, { recursive: true }), ]); return paths; From 22b141f06299f9f7e6b52483cc1b8b2455d25776 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Herna=CC=81n=20Pau=CC=81l=20Ossando=CC=81n?= Date: Sun, 24 May 2026 11:50:46 -0400 Subject: [PATCH 02/12] fix(orchestrator): address CR/Haystack findings on PR #19 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CR-1 (major): make isSafePathPart guard unconditional — was gated on !options?.workdir, missing the publish path that also uses productSlug CR-2 (major): parseGitHubRepoUrl now uses new URL() + hostname === 'github.com' check; the regex previously matched any string containing 'github.com/...' CR-3/CR-4 (critical): stop embedding githubToken in HTTPS remote URLs — token appeared in .git/config and git error messages. Switch to GIT_HTTP_EXTRAHEADER via GIT_CONFIG_COUNT/KEY/VALUE env; also fix defaultRunGh to resolve 'gh' from PATH instead of hardcoded /opt/homebrew/bin/gh CR-5/H-1 (critical): validate externalId at top of publishSpecToPR with /^[A-Za-z0-9._-]+$/ before any branch/path construction H-3: add 4 dispatcher tests — fetchFn called when token present, skipped when absent, dispatch continues on context-fetch failure (fallback), prUrl absent when githubToken missing 72/72 orchestrator tests, 132/132 API tests, tsc + bun build clean Co-Authored-By: Claude Sonnet 4.6 --- packages/orchestrator/src/dispatcher.test.ts | 70 +++++++++++++++++++ packages/orchestrator/src/dispatcher.ts | 26 +++---- .../src/specialists/fetch-product-context.ts | 14 +++- .../src/specialists/spec-publisher.test.ts | 27 +++++-- .../src/specialists/spec-publisher.ts | 37 ++++++---- 5 files changed, 137 insertions(+), 37 deletions(-) diff --git a/packages/orchestrator/src/dispatcher.test.ts b/packages/orchestrator/src/dispatcher.test.ts index ee8a341..a8aacfe 100644 --- a/packages/orchestrator/src/dispatcher.test.ts +++ b/packages/orchestrator/src/dispatcher.test.ts @@ -7,6 +7,7 @@ import { dispatchStageHandler } from './dispatcher.js'; import { MockAgentRuntime } from './runtimes/mock.js'; import type { Product } from '@helm/shared'; import type { ItemTransitionFn } from './specialists/spec-writer.js'; +import type { FetchFn } from './specialists/fetch-product-context.js'; // ── Fixtures ────────────────────────────────────────────────────────────────── @@ -185,4 +186,73 @@ describe('dispatchStageHandler', () => { expect(result.error).toContain('not implemented'); expect(result.specialistId).toBe('plan-writer'); }); + + it('calls fetchFn when githubToken is provided', async () => { + const mockFetch: FetchFn = vi.fn().mockResolvedValue({ + ok: false, + status: 404, + text: () => Promise.resolve('Not Found'), + } as Response); + + await dispatchStageHandler( + { externalId: 'issue_1', productSlug: 'test-product', currentStage: 'discovery' }, + makeProduct(), + makeSpecWriterRuntime('issue_1'), + transition as ItemTransitionFn, + { workdir, githubToken: 'test-token', fetchFn: mockFetch }, + ); + + expect(mockFetch).toHaveBeenCalled(); + }); + + it('does not call fetchFn when githubToken is absent', async () => { + const mockFetch: FetchFn = vi.fn(); + + await dispatchStageHandler( + { externalId: 'issue_1', productSlug: 'test-product', currentStage: 'discovery' }, + makeProduct(), + makeSpecWriterRuntime('issue_1'), + transition as ItemTransitionFn, + { workdir, fetchFn: mockFetch }, + ); + + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it('continues dispatch when context fetch throws (fallback to no context)', async () => { + const mockFetch: FetchFn = vi.fn().mockRejectedValue(new Error('network error')); + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + try { + const result = await dispatchStageHandler( + { externalId: 'issue_1', productSlug: 'test-product', currentStage: 'discovery' }, + makeProduct(), + makeSpecWriterRuntime('issue_1'), + transition as ItemTransitionFn, + { workdir, githubToken: 'test-token', fetchFn: mockFetch }, + ); + + expect(result.status).toBe('done'); + expect(result.newStage).toBe('spec-draft'); + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringContaining('[dispatcher]'), + expect.any(Error), + ); + } finally { + consoleSpy.mockRestore(); + } + }); + + it('omits prUrl when githubToken is absent (publish step skipped)', async () => { + const result = await dispatchStageHandler( + { externalId: 'issue_1', productSlug: 'test-product', currentStage: 'discovery' }, + makeProduct(), + makeSpecWriterRuntime('issue_1'), + transition as ItemTransitionFn, + { workdir, dataRoot: '/some/data' }, // dataRoot present but no token + ); + + expect(result.status).toBe('done'); + expect(result.prUrl).toBeUndefined(); + }); }); diff --git a/packages/orchestrator/src/dispatcher.ts b/packages/orchestrator/src/dispatcher.ts index ff81fd1..c1e5b95 100644 --- a/packages/orchestrator/src/dispatcher.ts +++ b/packages/orchestrator/src/dispatcher.ts @@ -74,21 +74,17 @@ export async function dispatchStageHandler( transition: ItemTransitionFn, options?: DispatchOptions, ): Promise { - // Guard against path traversal when the default workdir is constructed from - // productSlug / externalId. The API layer validates these values before calling - // dispatchStageHandler, but the dispatcher is a library function — validate - // defensively here too. Only enforced when options.workdir is not provided. - if (!options?.workdir) { - const isSafePathPart = (v: string): boolean => /^[A-Za-z0-9._-]+$/.test(v); - if (!isSafePathPart(item.productSlug) || !isSafePathPart(item.externalId)) { - return { - specialistId: options?.specialistId ?? 'none', - status: 'error', - costUsd: 0, - durationMs: 0, - error: 'Invalid productSlug or externalId for filesystem path', - }; - } + // Guard against path traversal — both productSlug (used in the publish path) and + // externalId (used in workdir + branch names) must be safe filesystem components. + const isSafePathPart = (v: string): boolean => /^[A-Za-z0-9._-]+$/.test(v); + if (!isSafePathPart(item.productSlug) || !isSafePathPart(item.externalId)) { + return { + specialistId: options?.specialistId ?? 'none', + status: 'error', + costUsd: 0, + durationMs: 0, + error: 'Invalid productSlug or externalId for filesystem path', + }; } const specialistId = options?.specialistId ?? STAGE_TO_SPECIALIST[item.currentStage]; diff --git a/packages/orchestrator/src/specialists/fetch-product-context.ts b/packages/orchestrator/src/specialists/fetch-product-context.ts index fba72be..4966f30 100644 --- a/packages/orchestrator/src/specialists/fetch-product-context.ts +++ b/packages/orchestrator/src/specialists/fetch-product-context.ts @@ -24,8 +24,18 @@ const TRUNCATION_SUFFIX = '\n\n[...truncated]'; * Handles trailing .git, trailing slashes, and optional paths after the repo. */ export function parseGitHubRepoUrl(url: string): { owner: string; repo: string } | null { - const match = url.match(/github\.com\/([^/]+)\/([^/.]+?)(?:\.git)?(?:[/?#]|$)/); - return match ? { owner: match[1]!, repo: match[2]! } : null; + try { + const u = new URL(url); + if (u.hostname !== 'github.com') return null; + const parts = u.pathname.replace(/^\/+/, '').split('/'); + if (parts.length < 2) return null; + const owner = parts[0]!; + const repo = parts[1]!.replace(/\.git$/, ''); + if (!owner || !repo) return null; + return { owner, repo }; + } catch { + return null; + } } /** diff --git a/packages/orchestrator/src/specialists/spec-publisher.test.ts b/packages/orchestrator/src/specialists/spec-publisher.test.ts index 39752e6..9e449b3 100644 --- a/packages/orchestrator/src/specialists/spec-publisher.test.ts +++ b/packages/orchestrator/src/specialists/spec-publisher.test.ts @@ -226,17 +226,34 @@ describe('publishSpecToPR', () => { expect(checkoutCall![0][2]).toBe('helm/spec/HLM-99'); }); - it('uses token-embedded URL for clone and push', async () => { + it('passes auth token via GIT_HTTP_EXTRAHEADER, not embedded in URL', async () => { const runGit = makeRunGit(knowledgeRepoLocalPath); const runGh = makeRunGh(); await publishSpecToPR(makeOpts({ githubToken: 'secret-token' }), runGit, runGh); - // mock.calls[i] = [args, opts] - const gitCalls = (runGit as ReturnType).mock.calls as [string[], unknown][]; + // mock.calls[i] = [args, opts] where opts has .env + const gitCalls = (runGit as ReturnType).mock.calls as [ + string[], + { cwd: string; env?: NodeJS.ProcessEnv }, + ][]; const cloneCall = gitCalls.find(([args]) => args[0] === 'clone'); const pushCall = gitCalls.find(([args]) => args[0] === 'push'); - expect(cloneCall![0][1]).toContain('x-access-token:secret-token@'); - expect(pushCall![0][1]).toContain('x-access-token:secret-token@'); + + // Token must NOT appear in any URL argument + expect(cloneCall![0][1]).not.toContain('secret-token'); + expect(pushCall![0][1]).not.toContain('secret-token'); + + // Token must appear as an Authorization header in the git env + expect(cloneCall![1].env?.GIT_CONFIG_VALUE_0).toContain('secret-token'); + }); + + it('throws on invalid externalId containing path traversal characters', async () => { + const runGit: RunGit = vi.fn().mockResolvedValue({ stdout: '' }); + const runGh = makeRunGh(); + + await expect( + publishSpecToPR(makeOpts({ externalId: '../evil' }), runGit, runGh), + ).rejects.toThrow(/Invalid externalId/); }); }); diff --git a/packages/orchestrator/src/specialists/spec-publisher.ts b/packages/orchestrator/src/specialists/spec-publisher.ts index ee3f4ce..6d830cd 100644 --- a/packages/orchestrator/src/specialists/spec-publisher.ts +++ b/packages/orchestrator/src/specialists/spec-publisher.ts @@ -53,8 +53,7 @@ export const defaultRunGit: RunGit = async (args, opts) => { }; export const defaultRunGh: RunGh = async (args, opts) => { - const ghPath = '/opt/homebrew/bin/gh'; - const { stdout } = await execFileAsync(ghPath, args, { + const { stdout } = await execFileAsync('gh', args, { env: { ...process.env, ...opts.env }, }); return { stdout }; @@ -73,26 +72,23 @@ async function pathExists(p: string): Promise { /** * Ensures the knowledge repo is cloned and up to date. - * If not yet cloned → git clone (with token-embedded HTTPS URL). + * If not yet cloned → git clone using plain URL + GIT_HTTP_EXTRAHEADER for auth. * If already cloned → fetch + reset to latest default branch. */ async function ensureKnowledgeRepo( localPath: string, repoUrl: string, defaultBranch: string, - githubToken: string, + tokenEnv: NodeJS.ProcessEnv, runGit: RunGit, ): Promise { - // Token-embedded URL so git doesn't prompt for credentials. - const authenticatedUrl = repoUrl.replace('https://', `https://x-access-token:${githubToken}@`); - const gitDir = join(localPath, '.git'); if (!(await pathExists(gitDir))) { await mkdir(dirname(localPath), { recursive: true }); - await runGit(['clone', authenticatedUrl, localPath], { cwd: dirname(localPath) }); + await runGit(['clone', repoUrl, localPath], { cwd: dirname(localPath), env: tokenEnv }); } else { // Pull latest without interactive prompts. - await runGit(['fetch', 'origin'], { cwd: localPath }); + await runGit(['fetch', 'origin'], { cwd: localPath, env: tokenEnv }); await runGit(['checkout', defaultBranch], { cwd: localPath }); await runGit(['reset', '--hard', `origin/${defaultBranch}`], { cwd: localPath }); } @@ -118,6 +114,11 @@ export async function publishSpecToPR( ): Promise { const { externalId, product, specPath, knowledgeRepoLocalPath, githubToken } = opts; + const EXTERNAL_ID_SAFE = /^[A-Za-z0-9._-]+$/; + if (!EXTERNAL_ID_SAFE.test(externalId)) { + throw new Error(`[spec-publisher] Invalid externalId: "${externalId}"`); + } + const knowledgeRepo = product.knowledge_repo; const defaultBranch = knowledgeRepo.default_branch; const branchName = `helm/spec/${externalId}`; @@ -128,13 +129,22 @@ export async function publishSpecToPR( } const { owner, repo } = parsed; + // Build token env once — used for clone, fetch, and push (network operations). + // Token is passed as an HTTP Authorization header via git config, so it never + // appears in remote URLs or git error messages. + const tokenEnv: NodeJS.ProcessEnv = { + GIT_CONFIG_COUNT: '1', + GIT_CONFIG_KEY_0: 'http.https://github.com/.extraHeader', + GIT_CONFIG_VALUE_0: `Authorization: token ${githubToken}`, + }; + // ── Step 1: Clone or update knowledge repo ─────────────────────────────── try { await ensureKnowledgeRepo( knowledgeRepoLocalPath, knowledgeRepo.url, defaultBranch, - githubToken, + tokenEnv, runGit, ); } catch (err) { @@ -187,13 +197,10 @@ export async function publishSpecToPR( } // ── Step 5: Push branch ────────────────────────────────────────────────── - const authenticatedUrl = knowledgeRepo.url.replace( - 'https://', - `https://x-access-token:${githubToken}@`, - ); try { - await runGit(['push', authenticatedUrl, `${branchName}:${branchName}`, '--force'], { + await runGit(['push', 'origin', `${branchName}:${branchName}`, '--force'], { cwd: knowledgeRepoLocalPath, + env: tokenEnv, }); } catch (err) { throw new Error( From 70838c97e6c5494050638f79f7e2fecda3f3c855 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Herna=CC=81n=20Pau=CC=81l=20Ossando=CC=81n?= Date: Sun, 24 May 2026 13:29:41 -0400 Subject: [PATCH 03/12] fix(orchestrator): address second round of Haystack findings on PR #19 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit H-1/H-2 (block-dot-segment-path-inputs): both isSafePathPart in dispatcher and EXTERNAL_ID_SAFE in publishSpecToPR used /^[A-Za-z0-9._-]+$/ which allowed '.' and '..'; add (?!\.) lookahead to block dot-segment values H-5: defaultRunGit now calls 'git' from PATH instead of /usr/bin/git for portability (NixOS, containers, non-standard installations) H-4 (test coverage): new dispatch-options.test.ts verifies GITHUB_TOKEN trimming and dataRoot forwarding to dispatchStageHandler via a focused vi.mock('@helm/orchestrator') that captures options without running the full spec-writer flow Skipped H-3 (error-detail sanitization): internal tool — operators need full diagnostic detail from git/gh failures 72/72 orchestrator tests, 136/136 API tests, tsc + bun build clean Co-Authored-By: Claude Sonnet 4.6 --- apps/api/src/routes/dispatch-options.test.ts | 187 ++++++++++++++++++ packages/orchestrator/src/dispatcher.ts | 4 +- .../src/specialists/spec-publisher.ts | 8 +- 3 files changed, 196 insertions(+), 3 deletions(-) create mode 100644 apps/api/src/routes/dispatch-options.test.ts diff --git a/apps/api/src/routes/dispatch-options.test.ts b/apps/api/src/routes/dispatch-options.test.ts new file mode 100644 index 0000000..fc3615c --- /dev/null +++ b/apps/api/src/routes/dispatch-options.test.ts @@ -0,0 +1,187 @@ +/** + * Focused tests for the option-forwarding logic in the dispatch route: + * GITHUB_TOKEN trimming, dataRoot, and githubToken wiring. + * + * Uses a module-level mock of dispatchStageHandler to capture what options + * the route passes to the orchestrator without running the full spec-writer flow. + */ +import { mkdir, rm } from 'node:fs/promises'; +import { randomUUID } from 'node:crypto'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { app } from '../app.js'; +import { _resetForTests } from '../services/index.js'; +import type { Product } from '@helm/shared'; +import type { DispatchOptions } from '@helm/orchestrator'; + +// ── Module-level mock of dispatchStageHandler ───────────────────────────────── +// Captures the options argument without running real dispatch logic. +// vi.hoisted is required because vi.mock factories are hoisted to module top. + +const { mockDispatch } = vi.hoisted(() => ({ + mockDispatch: vi.fn().mockResolvedValue({ + specialistId: 'spec-writer', + status: 'done' as const, + newStage: 'spec-draft' as const, + costUsd: 0, + durationMs: 0, + }), +})); + +vi.mock('@helm/orchestrator', async (importOriginal) => { + const real = await importOriginal(); + return { ...real, dispatchStageHandler: mockDispatch }; +}); + +// ── Fixtures ────────────────────────────────────────────────────────────────── + +const makeProduct = (): Product => ({ + helm_version: '0', + product: { slug: 'test-product', name: 'Test Product' }, + issue_tracker: { + provider: 'github_projects', + org: 'test-org', + project_number: 1, + custom_field_name: 'Helm Stage', + }, + code_repos: [{ url: 'https://github.com/test-org/test', default_branch: 'main', role: 'app' }], + knowledge_repo: { url: 'https://github.com/test-org/knowledge', default_branch: 'main' }, + workflow: { + stages_enabled: ['discovery', 'spec-draft', 'released'], + designer_gate: 'skip', + qa_gate: 'skip', + }, + specialists: { + spec_writer: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + plan_writer: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + implementer: { runtime: 'claude_code', model: 'claude-opus-4-7' }, + code_reviewer: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + security_reviewer: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + test_reviewer: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + remediation: { runtime: 'claude_code', model: 'claude-sonnet-4-6' }, + }, +}); + +const makeItem = () => ({ + externalId: 'issue_1', + productSlug: 'test-product', + currentStage: 'discovery', + history: [], + createdAt: '2026-01-01T00:00:00Z', + updatedAt: '2026-01-01T00:00:00Z', +}); + +// ── Mocks ───────────────────────────────────────────────────────────────────── + +const { mockGetProductRegistry, mockGet, mockTransition } = vi.hoisted(() => ({ + mockGetProductRegistry: vi.fn(), + mockGet: vi.fn(), + mockTransition: vi.fn(), +})); + +vi.mock('../services/index.js', async (importOriginal) => { + const real = await importOriginal(); + return { + ...real, + getProductRegistry: mockGetProductRegistry, + getItemStore: vi.fn().mockResolvedValue({ get: mockGet, transition: mockTransition }), + }; +}); + +vi.mock('../services/runtime-factory.js', () => ({ + createRuntimeForProduct: vi.fn().mockReturnValue({ + spawn: vi.fn().mockResolvedValue({ + wait: vi + .fn() + .mockResolvedValue({ status: 'done', totalCostUsd: 0, durationMs: 0, finalOutput: '' }), + }), + }), +})); + +// ── Setup ───────────────────────────────────────────────────────────────────── + +let dataDir: string; +let savedDataDir: string | undefined; +let savedGithubToken: string | undefined; + +beforeEach(async () => { + _resetForTests(); + vi.clearAllMocks(); + + dataDir = join(tmpdir(), `dispatch-opts-${randomUUID()}`); + await mkdir(dataDir, { recursive: true }); + + savedDataDir = process.env.HELM_DATA_DIR; + savedGithubToken = process.env.GITHUB_TOKEN; + process.env.HELM_DATA_DIR = dataDir; + delete process.env.GITHUB_TOKEN; + + mockGetProductRegistry.mockResolvedValue([makeProduct()]); + mockGet.mockResolvedValue(makeItem()); + mockTransition.mockResolvedValue({ currentStage: 'spec-draft' }); +}); + +afterEach(async () => { + _resetForTests(); + await rm(dataDir, { recursive: true, force: true }); + if (savedDataDir === undefined) delete process.env.HELM_DATA_DIR; + else process.env.HELM_DATA_DIR = savedDataDir; + if (savedGithubToken === undefined) delete process.env.GITHUB_TOKEN; + else process.env.GITHUB_TOKEN = savedGithubToken; +}); + +const dispatch = (slug: string, externalId: string) => + app.request(`/api/products/${slug}/items/${externalId}/dispatch`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + }); + +// ── Helper: wait for the background job to invoke dispatchStageHandler ──────── +const waitForDispatch = () => + vi.waitFor(() => expect(mockDispatch).toHaveBeenCalled(), { timeout: 5000 }); + +const capturedOptions = (): DispatchOptions => { + const call = mockDispatch.mock.calls[0] as unknown[]; + return call[4] as DispatchOptions; +}; + +// ── Tests ───────────────────────────────────────────────────────────────────── + +describe('dispatch route option forwarding', () => { + it('forwards dataRoot derived from HELM_DATA_DIR to dispatchStageHandler', async () => { + const res = await dispatch('test-product', 'issue_1'); + expect(res.status).toBe(202); + await waitForDispatch(); + + expect(capturedOptions().dataRoot).toBe(dataDir); + }); + + it('forwards undefined githubToken when GITHUB_TOKEN is not set', async () => { + const res = await dispatch('test-product', 'issue_1'); + expect(res.status).toBe(202); + await waitForDispatch(); + + expect(capturedOptions().githubToken).toBeUndefined(); + }); + + it('trims GITHUB_TOKEN whitespace before forwarding to dispatchStageHandler', async () => { + process.env.GITHUB_TOKEN = ' padded-token '; + + const res = await dispatch('test-product', 'issue_1'); + expect(res.status).toBe(202); + await waitForDispatch(); + + expect(capturedOptions().githubToken).toBe('padded-token'); + }); + + it('forwards non-padded GITHUB_TOKEN unchanged', async () => { + process.env.GITHUB_TOKEN = 'clean-token'; + + const res = await dispatch('test-product', 'issue_1'); + expect(res.status).toBe(202); + await waitForDispatch(); + + expect(capturedOptions().githubToken).toBe('clean-token'); + }); +}); diff --git a/packages/orchestrator/src/dispatcher.ts b/packages/orchestrator/src/dispatcher.ts index c1e5b95..789428c 100644 --- a/packages/orchestrator/src/dispatcher.ts +++ b/packages/orchestrator/src/dispatcher.ts @@ -76,7 +76,9 @@ export async function dispatchStageHandler( ): Promise { // Guard against path traversal — both productSlug (used in the publish path) and // externalId (used in workdir + branch names) must be safe filesystem components. - const isSafePathPart = (v: string): boolean => /^[A-Za-z0-9._-]+$/.test(v); + // The (?!\.) lookahead blocks dot-segment values (`.`, `..`, `.hidden`, …) in + // addition to the character-class restriction. + const isSafePathPart = (v: string): boolean => /^(?!\.)[A-Za-z0-9._-]+$/.test(v); if (!isSafePathPart(item.productSlug) || !isSafePathPart(item.externalId)) { return { specialistId: options?.specialistId ?? 'none', diff --git a/packages/orchestrator/src/specialists/spec-publisher.ts b/packages/orchestrator/src/specialists/spec-publisher.ts index 6d830cd..bb8bc25 100644 --- a/packages/orchestrator/src/specialists/spec-publisher.ts +++ b/packages/orchestrator/src/specialists/spec-publisher.ts @@ -45,7 +45,9 @@ export type RunGh = ( // ── Default runners ─────────────────────────────────────────────────────────── export const defaultRunGit: RunGit = async (args, opts) => { - const { stdout } = await execFileAsync('/usr/bin/git', args, { + // Resolve 'git' from PATH for portability (avoids hardcoding /usr/bin/git + // which may differ on Linux containers, NixOS, Windows, or Homebrew setups). + const { stdout } = await execFileAsync('git', args, { cwd: opts.cwd, env: { ...process.env, ...opts.env, GIT_TERMINAL_PROMPT: '0' }, }); @@ -114,7 +116,9 @@ export async function publishSpecToPR( ): Promise { const { externalId, product, specPath, knowledgeRepoLocalPath, githubToken } = opts; - const EXTERNAL_ID_SAFE = /^[A-Za-z0-9._-]+$/; + // The (?!\.) lookahead rejects dot-segment values (`.`, `..`, `.hidden`, …) + // in addition to the character-class restriction, preventing path traversal. + const EXTERNAL_ID_SAFE = /^(?!\.)[A-Za-z0-9._-]+$/; if (!EXTERNAL_ID_SAFE.test(externalId)) { throw new Error(`[spec-publisher] Invalid externalId: "${externalId}"`); } From 6f9d6bc6a63fc6334fe2b4d29d412f015c98aa96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Herna=CC=81n=20Pau=CC=81l=20Ossando=CC=81n?= Date: Sun, 24 May 2026 13:53:19 -0400 Subject: [PATCH 04/12] fix(orchestrator): address third round of Haystack findings on PR #19 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit H-new-1 (SSH URLs): parseGitHubRepoUrl now handles git@github.com:owner/repo.git before calling new URL() — previously threw and silently dropped context; 3 new tests cover SSH with/without .git, non-GitHub SSH host H-new-2 (concurrent publish isolation): knowledgeRepoLocalPath in dispatcher now includes item.externalId as a subdirectory so concurrent dispatches for different items of the same product each get their own git clone, preventing working-tree clobber; on re-dispatch the same path is reused (fetch+reset, not re-clone) ENOTEMPTY race fix in dispatch-options.test.ts: waitForJobDone() now polls job status instead of only checking mockDispatch.toHaveBeenCalled() — ensures the jobStore.updateJob write completes before afterEach rm() runs Skipped H-new-3 (sanitize-and-map-http-errors): 3rd occurrence, same reasoning — internal tool where operators need full diagnostic detail from git/gh failures 75/75 orchestrator tests, 136/136 API tests, build clean Co-Authored-By: Claude Sonnet 4.6 --- apps/api/src/routes/dispatch-options.test.ts | 31 ++++++++++++++----- packages/orchestrator/src/dispatcher.ts | 11 ++++++- .../specialists/fetch-product-context.test.ts | 18 +++++++++++ .../src/specialists/fetch-product-context.ts | 12 +++++-- 4 files changed, 62 insertions(+), 10 deletions(-) diff --git a/apps/api/src/routes/dispatch-options.test.ts b/apps/api/src/routes/dispatch-options.test.ts index fc3615c..f1e11ff 100644 --- a/apps/api/src/routes/dispatch-options.test.ts +++ b/apps/api/src/routes/dispatch-options.test.ts @@ -137,9 +137,22 @@ const dispatch = (slug: string, externalId: string) => headers: { 'Content-Type': 'application/json' }, }); -// ── Helper: wait for the background job to invoke dispatchStageHandler ──────── -const waitForDispatch = () => - vi.waitFor(() => expect(mockDispatch).toHaveBeenCalled(), { timeout: 5000 }); +// ── Helper: fully drain the background job ──────────────────────────────────── +// Waiting for mockDispatch.toHaveBeenCalled() only guarantees the mock was +// *invoked* — jobStore.updateJob (which writes to disk) runs AFTER mockDispatch +// resolves. We must also wait for the job to leave 'running' state, otherwise +// afterEach's rm() races with the in-flight write and throws ENOTEMPTY. +const waitForJobDone = async (jobId: string) => { + const { getJobStore } = await import('../services/index.js'); + const jobStore = await getJobStore(); + await vi.waitFor( + async () => { + const job = await jobStore.getJob(jobId); + expect(job?.status).not.toBe('running'); + }, + { timeout: 5000 }, + ); +}; const capturedOptions = (): DispatchOptions => { const call = mockDispatch.mock.calls[0] as unknown[]; @@ -152,7 +165,8 @@ describe('dispatch route option forwarding', () => { it('forwards dataRoot derived from HELM_DATA_DIR to dispatchStageHandler', async () => { const res = await dispatch('test-product', 'issue_1'); expect(res.status).toBe(202); - await waitForDispatch(); + const { jobId } = (await res.json()) as { jobId: string }; + await waitForJobDone(jobId); expect(capturedOptions().dataRoot).toBe(dataDir); }); @@ -160,7 +174,8 @@ describe('dispatch route option forwarding', () => { it('forwards undefined githubToken when GITHUB_TOKEN is not set', async () => { const res = await dispatch('test-product', 'issue_1'); expect(res.status).toBe(202); - await waitForDispatch(); + const { jobId } = (await res.json()) as { jobId: string }; + await waitForJobDone(jobId); expect(capturedOptions().githubToken).toBeUndefined(); }); @@ -170,7 +185,8 @@ describe('dispatch route option forwarding', () => { const res = await dispatch('test-product', 'issue_1'); expect(res.status).toBe(202); - await waitForDispatch(); + const { jobId } = (await res.json()) as { jobId: string }; + await waitForJobDone(jobId); expect(capturedOptions().githubToken).toBe('padded-token'); }); @@ -180,7 +196,8 @@ describe('dispatch route option forwarding', () => { const res = await dispatch('test-product', 'issue_1'); expect(res.status).toBe(202); - await waitForDispatch(); + const { jobId } = (await res.json()) as { jobId: string }; + await waitForJobDone(jobId); expect(capturedOptions().githubToken).toBe('clean-token'); }); diff --git a/packages/orchestrator/src/dispatcher.ts b/packages/orchestrator/src/dispatcher.ts index 789428c..7d977bc 100644 --- a/packages/orchestrator/src/dispatcher.ts +++ b/packages/orchestrator/src/dispatcher.ts @@ -129,7 +129,16 @@ export async function dispatchStageHandler( options?.githubToken && options?.dataRoot ? { product, - knowledgeRepoLocalPath: join(options.dataRoot, 'knowledge-repos', item.productSlug), + // Each item gets its own clone subdirectory to prevent concurrent + // publishes for different items of the same product from clobbering + // each other's working tree. On re-dispatch the same path is reused + // (fetch + reset) rather than re-cloned. + knowledgeRepoLocalPath: join( + options.dataRoot, + 'knowledge-repos', + item.productSlug, + item.externalId, + ), githubToken: options.githubToken, } : undefined; diff --git a/packages/orchestrator/src/specialists/fetch-product-context.test.ts b/packages/orchestrator/src/specialists/fetch-product-context.test.ts index 75687b6..0d4ac64 100644 --- a/packages/orchestrator/src/specialists/fetch-product-context.test.ts +++ b/packages/orchestrator/src/specialists/fetch-product-context.test.ts @@ -67,6 +67,24 @@ describe('parseGitHubRepoUrl', () => { expect(parseGitHubRepoUrl('https://gitlab.com/owner/repo')).toBeNull(); expect(parseGitHubRepoUrl('not-a-url')).toBeNull(); }); + + it('parses SSH-style URL (git@github.com:owner/repo.git)', () => { + expect(parseGitHubRepoUrl('git@github.com:owner/repo.git')).toEqual({ + owner: 'owner', + repo: 'repo', + }); + }); + + it('parses SSH-style URL without .git suffix', () => { + expect(parseGitHubRepoUrl('git@github.com:owner/repo')).toEqual({ + owner: 'owner', + repo: 'repo', + }); + }); + + it('returns null for SSH URLs pointing to non-GitHub hosts', () => { + expect(parseGitHubRepoUrl('git@gitlab.com:owner/repo.git')).toBeNull(); + }); }); // ── fetchProductContext ─────────────────────────────────────────────────────── diff --git a/packages/orchestrator/src/specialists/fetch-product-context.ts b/packages/orchestrator/src/specialists/fetch-product-context.ts index 4966f30..ad7af16 100644 --- a/packages/orchestrator/src/specialists/fetch-product-context.ts +++ b/packages/orchestrator/src/specialists/fetch-product-context.ts @@ -20,10 +20,18 @@ const TRUNCATION_SUFFIX = '\n\n[...truncated]'; // ── Helpers ─────────────────────────────────────────────────────────────────── /** - * Parses a GitHub HTTPS URL into owner/repo. - * Handles trailing .git, trailing slashes, and optional paths after the repo. + * Parses a GitHub URL into owner/repo. + * Supports both HTTPS (`https://github.com/owner/repo[.git]`) and + * SSH (`git@github.com:owner/repo[.git]`) formats. */ export function parseGitHubRepoUrl(url: string): { owner: string; repo: string } | null { + // SSH format: git@github.com:owner/repo[.git] + const sshMatch = url.match(/^git@github\.com:([^/]+)\/([^/.]+?)(?:\.git)?$/); + if (sshMatch) { + return { owner: sshMatch[1]!, repo: sshMatch[2]! }; + } + + // HTTPS format: https://github.com/owner/repo[.git][/] try { const u = new URL(url); if (u.hostname !== 'github.com') return null; From ae583960a700e2799f0de46c67225ad11ae0724e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Herna=CC=81n=20Pau=CC=81l=20Ossando=CC=81n?= Date: Sun, 24 May 2026 14:26:58 -0400 Subject: [PATCH 05/12] fix(orchestrator): allow dots in SSH repo names; add dot-prefix traversal tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes a bug where parseGitHubRepoUrl rejected SSH URLs for repos whose names contain dots (e.g. git@github.com:org/my.repo.git) — the [^/.]+? character class was incorrectly excluding dots from the repo-name segment. Changed to [^/]+? so dotted names parse correctly while still stripping the optional .git suffix via the trailing (?:\.git)? group. Added tests: SSH repo with dots (.git), SSH repo with dots (no suffix), HTTPS repo with dots, dispatcher dot-prefix productSlug → error, dispatcher '..' externalId → error. Co-Authored-By: Claude Sonnet 4.6 --- packages/orchestrator/src/dispatcher.test.ts | 26 +++++++++++++++++++ .../specialists/fetch-product-context.test.ts | 21 +++++++++++++++ .../src/specialists/fetch-product-context.ts | 2 +- 3 files changed, 48 insertions(+), 1 deletion(-) diff --git a/packages/orchestrator/src/dispatcher.test.ts b/packages/orchestrator/src/dispatcher.test.ts index a8aacfe..b1c049f 100644 --- a/packages/orchestrator/src/dispatcher.test.ts +++ b/packages/orchestrator/src/dispatcher.test.ts @@ -255,4 +255,30 @@ describe('dispatchStageHandler', () => { expect(result.status).toBe('done'); expect(result.prUrl).toBeUndefined(); }); + + it('returns error when productSlug starts with a dot (dot-segment traversal)', async () => { + const result = await dispatchStageHandler( + { externalId: 'issue_1', productSlug: '.hidden-product', currentStage: 'discovery' }, + makeProduct(), + makeSpecWriterRuntime('issue_1'), + transition as ItemTransitionFn, + { workdir }, + ); + + expect(result.status).toBe('error'); + expect(result.error).toMatch(/Invalid productSlug or externalId/); + }); + + it('returns error when externalId is ".." (parent-directory traversal)', async () => { + const result = await dispatchStageHandler( + { externalId: '..', productSlug: 'test-product', currentStage: 'discovery' }, + makeProduct(), + makeSpecWriterRuntime('issue_1'), + transition as ItemTransitionFn, + { workdir }, + ); + + expect(result.status).toBe('error'); + expect(result.error).toMatch(/Invalid productSlug or externalId/); + }); }); diff --git a/packages/orchestrator/src/specialists/fetch-product-context.test.ts b/packages/orchestrator/src/specialists/fetch-product-context.test.ts index 0d4ac64..db2fb6c 100644 --- a/packages/orchestrator/src/specialists/fetch-product-context.test.ts +++ b/packages/orchestrator/src/specialists/fetch-product-context.test.ts @@ -85,6 +85,27 @@ describe('parseGitHubRepoUrl', () => { it('returns null for SSH URLs pointing to non-GitHub hosts', () => { expect(parseGitHubRepoUrl('git@gitlab.com:owner/repo.git')).toBeNull(); }); + + it('parses SSH-style URL with dots in repo name (e.g. my.repo.git)', () => { + expect(parseGitHubRepoUrl('git@github.com:owner/my.repo.git')).toEqual({ + owner: 'owner', + repo: 'my.repo', + }); + }); + + it('parses HTTPS URL with dots in repo name', () => { + expect(parseGitHubRepoUrl('https://github.com/owner/my.repo')).toEqual({ + owner: 'owner', + repo: 'my.repo', + }); + }); + + it('parses SSH-style URL with dots in repo name and no .git suffix', () => { + expect(parseGitHubRepoUrl('git@github.com:owner/my.repo')).toEqual({ + owner: 'owner', + repo: 'my.repo', + }); + }); }); // ── fetchProductContext ─────────────────────────────────────────────────────── diff --git a/packages/orchestrator/src/specialists/fetch-product-context.ts b/packages/orchestrator/src/specialists/fetch-product-context.ts index ad7af16..e2f9567 100644 --- a/packages/orchestrator/src/specialists/fetch-product-context.ts +++ b/packages/orchestrator/src/specialists/fetch-product-context.ts @@ -26,7 +26,7 @@ const TRUNCATION_SUFFIX = '\n\n[...truncated]'; */ export function parseGitHubRepoUrl(url: string): { owner: string; repo: string } | null { // SSH format: git@github.com:owner/repo[.git] - const sshMatch = url.match(/^git@github\.com:([^/]+)\/([^/.]+?)(?:\.git)?$/); + const sshMatch = url.match(/^git@github\.com:([^/]+)\/([^/]+?)(?:\.git)?$/); if (sshMatch) { return { owner: sshMatch[1]!, repo: sshMatch[2]! }; } From 2ed679814869a21ecddfcab3bc5cd10243517df2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Herna=CC=81n=20Pau=CC=81l=20Ossando=CC=81n?= Date: Sun, 24 May 2026 14:36:45 -0400 Subject: [PATCH 06/12] fix(orchestrator): reject SSH knowledge repo URLs; add prUrl wiring test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SSH knowledge repo URLs are unsupported for token-based auth because GIT_HTTP_EXTRAHEADER only applies to HTTPS transport. publishSpecToPR now fails early with a clear message directing operators to use an HTTPS URL instead of silently attempting the operation and producing a confusing auth failure. Also threads runGit/runGh from DispatchOptions through to the publish step so integration tests can verify the full token+dataRoot→prUrl happy path without spawning real git/gh processes. Added test cases for both. Co-Authored-By: Claude Sonnet 4.6 --- packages/orchestrator/src/dispatcher.test.ts | 30 +++++++++++++++++++ packages/orchestrator/src/dispatcher.ts | 13 ++++++++ .../src/specialists/spec-publisher.test.ts | 13 ++++++++ .../src/specialists/spec-publisher.ts | 11 +++++++ 4 files changed, 67 insertions(+) diff --git a/packages/orchestrator/src/dispatcher.test.ts b/packages/orchestrator/src/dispatcher.test.ts index b1c049f..35fd768 100644 --- a/packages/orchestrator/src/dispatcher.test.ts +++ b/packages/orchestrator/src/dispatcher.test.ts @@ -8,6 +8,7 @@ import { MockAgentRuntime } from './runtimes/mock.js'; import type { Product } from '@helm/shared'; import type { ItemTransitionFn } from './specialists/spec-writer.js'; import type { FetchFn } from './specialists/fetch-product-context.js'; +import type { RunGit, RunGh } from './specialists/spec-publisher.js'; // ── Fixtures ────────────────────────────────────────────────────────────────── @@ -281,4 +282,33 @@ describe('dispatchStageHandler', () => { expect(result.status).toBe('error'); expect(result.error).toMatch(/Invalid productSlug or externalId/); }); + + it('propagates prUrl from publish step when token and dataRoot are provided', async () => { + const expectedPrUrl = 'https://github.com/test-org/test-knowledge/pull/5'; + + // Stub runners: clone creates a .git dir; gh create returns a PR URL. + const runGit: RunGit = vi.fn().mockImplementation(async (args: string[]) => { + if (args[0] === 'clone') { + const cloneDest = args[2]!; + await mkdir(join(cloneDest, '.git'), { recursive: true }); + } + return { stdout: '' }; + }); + const runGh: RunGh = vi.fn().mockImplementation(async (args: string[]) => { + if (args[0] === 'pr' && args[1] === 'list') return { stdout: '[]' }; + if (args[0] === 'pr' && args[1] === 'create') return { stdout: `${expectedPrUrl}\n` }; + return { stdout: '' }; + }); + + const result = await dispatchStageHandler( + { externalId: 'issue_1', productSlug: 'test-product', currentStage: 'discovery' }, + makeProduct(), + makeSpecWriterRuntime('issue_1'), + transition as ItemTransitionFn, + { workdir, dataRoot: workdir, githubToken: 'test-token', runGit, runGh }, + ); + + expect(result.status).toBe('done'); + expect(result.prUrl).toBe(expectedPrUrl); + }); }); diff --git a/packages/orchestrator/src/dispatcher.ts b/packages/orchestrator/src/dispatcher.ts index 7d977bc..ec5fa38 100644 --- a/packages/orchestrator/src/dispatcher.ts +++ b/packages/orchestrator/src/dispatcher.ts @@ -7,6 +7,7 @@ import type { ItemTransitionFn } from './specialists/spec-writer.js'; import { buildSpecWriterParams, handleSpecWriterResult } from './specialists/spec-writer.js'; import { fetchProductContext } from './specialists/fetch-product-context.js'; import type { FetchFn } from './specialists/fetch-product-context.js'; +import type { RunGit, RunGh } from './specialists/spec-publisher.js'; // ── Stage → specialist mapping ──────────────────────────────────────────────── // Expanded in later sessions (plan-writer, implementer, etc.) @@ -56,6 +57,16 @@ export type DispatchOptions = { * Defaults to the global fetch. */ fetchFn?: FetchFn; + /** + * Injectable git runner — for testing the publish path. + * Defaults to the real git binary via execFile. + */ + runGit?: RunGit; + /** + * Injectable gh runner — for testing the publish path. + * Defaults to the real gh binary via execFile. + */ + runGh?: RunGh; }; // ── Dispatcher ──────────────────────────────────────────────────────────────── @@ -140,6 +151,8 @@ export async function dispatchStageHandler( item.externalId, ), githubToken: options.githubToken, + runGit: options.runGit, + runGh: options.runGh, } : undefined; diff --git a/packages/orchestrator/src/specialists/spec-publisher.test.ts b/packages/orchestrator/src/specialists/spec-publisher.test.ts index 9e449b3..38def80 100644 --- a/packages/orchestrator/src/specialists/spec-publisher.test.ts +++ b/packages/orchestrator/src/specialists/spec-publisher.test.ts @@ -256,4 +256,17 @@ describe('publishSpecToPR', () => { publishSpecToPR(makeOpts({ externalId: '../evil' }), runGit, runGh), ).rejects.toThrow(/Invalid externalId/); }); + + it('throws with a helpful message when knowledge repo URL is SSH-format', async () => { + const runGit: RunGit = vi.fn().mockResolvedValue({ stdout: '' }); + const runGh = makeRunGh(); + const sshProduct: Product = { + ...makeProduct(), + knowledge_repo: { url: 'git@github.com:test-org/knowledge.git', default_branch: 'main' }, + }; + + await expect(publishSpecToPR(makeOpts({ product: sshProduct }), runGit, runGh)).rejects.toThrow( + /SSH knowledge repo URLs are not supported.*Use an HTTPS URL/, + ); + }); }); diff --git a/packages/orchestrator/src/specialists/spec-publisher.ts b/packages/orchestrator/src/specialists/spec-publisher.ts index bb8bc25..89f65c5 100644 --- a/packages/orchestrator/src/specialists/spec-publisher.ts +++ b/packages/orchestrator/src/specialists/spec-publisher.ts @@ -133,6 +133,17 @@ export async function publishSpecToPR( } const { owner, repo } = parsed; + // SSH-format knowledge repo URLs (git@github.com:org/repo) are not supported + // for token-based authentication: GIT_HTTP_EXTRAHEADER only applies to HTTPS + // operations and has no effect on SSH transport. Fail early with a clear + // message rather than silently falling through to a confusing auth error. + if (knowledgeRepo.url.startsWith('git@')) { + throw new Error( + `[spec-publisher] SSH knowledge repo URLs are not supported for token-based auth. ` + + `Use an HTTPS URL (https://github.com/${owner}/${repo}) instead.`, + ); + } + // Build token env once — used for clone, fetch, and push (network operations). // Token is passed as an HTTP Authorization header via git config, so it never // appears in remote URLs or git error messages. From 674c2328c23be2c562ee1898ca567a1cdf46afa7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Herna=CC=81n=20Pau=CC=81l=20Ossando=CC=81n?= Date: Sun, 24 May 2026 14:48:30 -0400 Subject: [PATCH 07/12] fix(orchestrator): extend SSH URL guard to cover ssh:// scheme The existing git@ check missed URLs using the ssh:// URI scheme (e.g. ssh://git@github.com/org/repo.git), which would bypass validation and fail later with a confusing auth error. Extended the guard to reject both git@ and ssh:// prefixes, and added a matching test case. Co-Authored-By: Claude Sonnet 4.6 --- .../src/specialists/spec-publisher.test.ts | 18 +++++++++++++++++- .../src/specialists/spec-publisher.ts | 11 ++++++----- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/packages/orchestrator/src/specialists/spec-publisher.test.ts b/packages/orchestrator/src/specialists/spec-publisher.test.ts index 38def80..af84fd8 100644 --- a/packages/orchestrator/src/specialists/spec-publisher.test.ts +++ b/packages/orchestrator/src/specialists/spec-publisher.test.ts @@ -257,7 +257,7 @@ describe('publishSpecToPR', () => { ).rejects.toThrow(/Invalid externalId/); }); - it('throws with a helpful message when knowledge repo URL is SSH-format', async () => { + it('throws with a helpful message when knowledge repo URL is SSH-format (git@)', async () => { const runGit: RunGit = vi.fn().mockResolvedValue({ stdout: '' }); const runGh = makeRunGh(); const sshProduct: Product = { @@ -269,4 +269,20 @@ describe('publishSpecToPR', () => { /SSH knowledge repo URLs are not supported.*Use an HTTPS URL/, ); }); + + it('throws with a helpful message when knowledge repo URL uses ssh:// scheme', async () => { + const runGit: RunGit = vi.fn().mockResolvedValue({ stdout: '' }); + const runGh = makeRunGh(); + const sshProduct: Product = { + ...makeProduct(), + knowledge_repo: { + url: 'ssh://git@github.com/test-org/knowledge.git', + default_branch: 'main', + }, + }; + + await expect(publishSpecToPR(makeOpts({ product: sshProduct }), runGit, runGh)).rejects.toThrow( + /SSH knowledge repo URLs are not supported.*Use an HTTPS URL/, + ); + }); }); diff --git a/packages/orchestrator/src/specialists/spec-publisher.ts b/packages/orchestrator/src/specialists/spec-publisher.ts index 89f65c5..16e3248 100644 --- a/packages/orchestrator/src/specialists/spec-publisher.ts +++ b/packages/orchestrator/src/specialists/spec-publisher.ts @@ -133,11 +133,12 @@ export async function publishSpecToPR( } const { owner, repo } = parsed; - // SSH-format knowledge repo URLs (git@github.com:org/repo) are not supported - // for token-based authentication: GIT_HTTP_EXTRAHEADER only applies to HTTPS - // operations and has no effect on SSH transport. Fail early with a clear - // message rather than silently falling through to a confusing auth error. - if (knowledgeRepo.url.startsWith('git@')) { + // SSH-format knowledge repo URLs (git@github.com:org/repo or ssh://...) are + // not supported for token-based authentication: GIT_HTTP_EXTRAHEADER only + // applies to HTTPS operations and has no effect on SSH transport. Fail early + // with a clear message rather than silently falling through to a confusing + // auth error. + if (knowledgeRepo.url.startsWith('git@') || knowledgeRepo.url.startsWith('ssh://')) { throw new Error( `[spec-publisher] SSH knowledge repo URLs are not supported for token-based auth. ` + `Use an HTTPS URL (https://github.com/${owner}/${repo}) instead.`, From 791c69d539990b6b34c6c829b0dcf648fa11e30a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Herna=CC=81n=20Pau=CC=81l=20Ossando=CC=81n?= Date: Sun, 24 May 2026 15:40:53 -0400 Subject: [PATCH 08/12] fix(orchestrator): isolate each publish in its own temp clone to prevent cross-item races MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit publishSpecToPR now clones the knowledge repo to a fresh temporary directory (tmpdir/helm-publish-{externalId}-{uuid}/) on every call instead of mutating a shared checkout. The temp dir is always removed in a finally block, even on failure. Two concurrent publishes for any combination of items can never interleave their git operations or corrupt each other's branches. Breaking: knowledgeRepoLocalPath removed from PublishSpecOpts and SpecPublishOptions — the publish function manages its own working directory. DispatchOptions.dataRoot is retained for future specialists but is no longer needed by the spec-writer publish step (githubToken alone enables it). Also removes the ensureKnowledgeRepo abstraction (always-fresh clone replaces the clone-or-update logic) and updates all affected tests, including a new concurrent-publish test and a cleanup-on-failure test. Co-Authored-By: Claude Sonnet 4.6 --- packages/orchestrator/src/dispatcher.test.ts | 36 ++- packages/orchestrator/src/dispatcher.ts | 33 +- .../src/specialists/spec-publisher.test.ts | 120 ++++--- .../src/specialists/spec-publisher.ts | 305 ++++++++---------- .../src/specialists/spec-writer.test.ts | 16 +- .../src/specialists/spec-writer.ts | 6 +- 6 files changed, 277 insertions(+), 239 deletions(-) diff --git a/packages/orchestrator/src/dispatcher.test.ts b/packages/orchestrator/src/dispatcher.test.ts index 35fd768..38c80df 100644 --- a/packages/orchestrator/src/dispatcher.test.ts +++ b/packages/orchestrator/src/dispatcher.test.ts @@ -195,12 +195,27 @@ describe('dispatchStageHandler', () => { text: () => Promise.resolve('Not Found'), } as Response); + // Provide runner mocks so the publish step (also triggered by githubToken) + // doesn't attempt a real git clone during this context-fetch test. + const runGit: RunGit = vi.fn().mockImplementation(async (args: string[]) => { + if (args[0] === 'clone') { + await mkdir(join(args[2]!, '.git'), { recursive: true }); + } + return { stdout: '' }; + }); + const runGh: RunGh = vi.fn().mockImplementation(async (args: string[]) => { + if (args[0] === 'pr' && args[1] === 'list') return { stdout: '[]' }; + if (args[0] === 'pr' && args[1] === 'create') + return { stdout: 'https://github.com/test-org/knowledge/pull/1\n' }; + return { stdout: '' }; + }); + await dispatchStageHandler( { externalId: 'issue_1', productSlug: 'test-product', currentStage: 'discovery' }, makeProduct(), makeSpecWriterRuntime('issue_1'), transition as ItemTransitionFn, - { workdir, githubToken: 'test-token', fetchFn: mockFetch }, + { workdir, githubToken: 'test-token', fetchFn: mockFetch, runGit, runGh }, ); expect(mockFetch).toHaveBeenCalled(); @@ -224,13 +239,28 @@ describe('dispatchStageHandler', () => { const mockFetch: FetchFn = vi.fn().mockRejectedValue(new Error('network error')); const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + // Provide runner mocks so the publish step (also triggered by githubToken) + // succeeds and doesn't mask the context-fetch fallback being tested. + const runGit: RunGit = vi.fn().mockImplementation(async (args: string[]) => { + if (args[0] === 'clone') { + await mkdir(join(args[2]!, '.git'), { recursive: true }); + } + return { stdout: '' }; + }); + const runGh: RunGh = vi.fn().mockImplementation(async (args: string[]) => { + if (args[0] === 'pr' && args[1] === 'list') return { stdout: '[]' }; + if (args[0] === 'pr' && args[1] === 'create') + return { stdout: 'https://github.com/test-org/knowledge/pull/1\n' }; + return { stdout: '' }; + }); + try { const result = await dispatchStageHandler( { externalId: 'issue_1', productSlug: 'test-product', currentStage: 'discovery' }, makeProduct(), makeSpecWriterRuntime('issue_1'), transition as ItemTransitionFn, - { workdir, githubToken: 'test-token', fetchFn: mockFetch }, + { workdir, githubToken: 'test-token', fetchFn: mockFetch, runGit, runGh }, ); expect(result.status).toBe('done'); @@ -305,7 +335,7 @@ describe('dispatchStageHandler', () => { makeProduct(), makeSpecWriterRuntime('issue_1'), transition as ItemTransitionFn, - { workdir, dataRoot: workdir, githubToken: 'test-token', runGit, runGh }, + { workdir, githubToken: 'test-token', runGit, runGh }, ); expect(result.status).toBe('done'); diff --git a/packages/orchestrator/src/dispatcher.ts b/packages/orchestrator/src/dispatcher.ts index ec5fa38..e393f22 100644 --- a/packages/orchestrator/src/dispatcher.ts +++ b/packages/orchestrator/src/dispatcher.ts @@ -42,8 +42,7 @@ export type DispatchOptions = { specialistId?: string; /** * Absolute path to the Helm data root (e.g. HELM_DATA_DIR or cwd/data). - * Used to locate `knowledge-repos/{productSlug}/` for the publish step. - * Required for the publish step to run. + * Reserved for future specialists; currently unused by the spec-writer path. */ dataRoot?: string; /** @@ -136,25 +135,17 @@ export async function dispatchStageHandler( const agentResult = await session.wait(); // ── Part B: Publish spec to knowledge repo (optional) ──────────────────── - const publishOpts = - options?.githubToken && options?.dataRoot - ? { - product, - // Each item gets its own clone subdirectory to prevent concurrent - // publishes for different items of the same product from clobbering - // each other's working tree. On re-dispatch the same path is reused - // (fetch + reset) rather than re-cloned. - knowledgeRepoLocalPath: join( - options.dataRoot, - 'knowledge-repos', - item.productSlug, - item.externalId, - ), - githubToken: options.githubToken, - runGit: options.runGit, - runGh: options.runGh, - } - : undefined; + // publishSpecToPR internally clones to an isolated temp directory per call, + // so no knowledgeRepoLocalPath is needed here — concurrency safety is + // handled inside the function itself. + const publishOpts = options?.githubToken + ? { + product, + githubToken: options.githubToken, + runGit: options.runGit, + runGh: options.runGh, + } + : undefined; const specResult = await handleSpecWriterResult( item.externalId, diff --git a/packages/orchestrator/src/specialists/spec-publisher.test.ts b/packages/orchestrator/src/specialists/spec-publisher.test.ts index af84fd8..83940f3 100644 --- a/packages/orchestrator/src/specialists/spec-publisher.test.ts +++ b/packages/orchestrator/src/specialists/spec-publisher.test.ts @@ -42,13 +42,18 @@ const makeProduct = (): Product => ({ /** * Builds a mock runGit that simulates git operations on the real local - * filesystem. clone creates a .git dir; other commands are no-ops. + * filesystem. `clone ` creates a `.git` marker inside `dest` + * (which publishSpecToPR pre-creates); all other commands are no-ops. + * + * Since publishSpecToPR always clones to a fresh temp directory (known only + * at call time), the destination is read directly from the args array rather + * than being injected as a parameter. */ -function makeRunGit(cloneTargetPath: string): RunGit { +function makeRunGit(): RunGit { return vi.fn().mockImplementation(async (args: string[]) => { if (args[0] === 'clone') { - // Simulate clone by creating .git marker dir - await mkdir(join(cloneTargetPath, '.git'), { recursive: true }); + const dest = args[2]!; + await mkdir(join(dest, '.git'), { recursive: true }); } return { stdout: '' }; }); @@ -71,7 +76,6 @@ function makeRunGh(prListResult: { url: string }[] = []): RunGh { let tmpDir: string; let specPath: string; -let knowledgeRepoLocalPath: string; beforeEach(async () => { tmpDir = join(tmpdir(), `spec-publisher-${randomUUID()}`); @@ -82,8 +86,6 @@ beforeEach(async () => { await mkdir(specsDir, { recursive: true }); specPath = join(specsDir, 'issue_1.md'); await writeFile(specPath, '# issue_1 — Specification\n'); - - knowledgeRepoLocalPath = join(tmpDir, 'knowledge-repos', 'my-product'); }); afterEach(async () => { @@ -96,7 +98,6 @@ const makeOpts = (overrides?: Partial): PublishSpecOpts => ({ externalId: 'issue_1', product: makeProduct(), specPath, - knowledgeRepoLocalPath, githubToken: 'test-token', ...overrides, }); @@ -105,17 +106,13 @@ const makeOpts = (overrides?: Partial): PublishSpecOpts => ({ describe('publishSpecToPR', () => { it('clones repo, copies spec, commits, pushes, and creates PR', async () => { - const runGit = makeRunGit(knowledgeRepoLocalPath); + const runGit = makeRunGit(); const runGh = makeRunGh(); const result = await publishSpecToPR(makeOpts(), runGit, runGh); expect(result.prUrl).toBe('https://github.com/test-org/knowledge/pull/42'); - // Spec file should exist in the knowledge repo clone - const destSpec = join(knowledgeRepoLocalPath, 'specs', 'issue_1.md'); - await expect(stat(destSpec)).resolves.toBeDefined(); - // git was called with clone, checkout -B, add, commit, push // mock.calls[i] = [args, opts] since runGit takes two parameters const gitCalls = (runGit as ReturnType).mock.calls as [string[], unknown][]; @@ -127,7 +124,7 @@ describe('publishSpecToPR', () => { }); it('reuses existing open PR without creating a duplicate', async () => { - const runGit = makeRunGit(knowledgeRepoLocalPath); + const runGit = makeRunGit(); const existingPrUrl = 'https://github.com/test-org/knowledge/pull/7'; const runGh = makeRunGh([{ url: existingPrUrl }]); @@ -141,22 +138,6 @@ describe('publishSpecToPR', () => { expect(ghCalls.some(([args]) => args[0] === 'pr' && args[1] === 'create')).toBe(false); }); - it('skips clone and pulls when knowledge repo already exists', async () => { - // Pre-create .git directory to simulate an already-cloned repo - await mkdir(join(knowledgeRepoLocalPath, '.git'), { recursive: true }); - const runGit: RunGit = vi.fn().mockResolvedValue({ stdout: '' }); - const runGh = makeRunGh(); - - await publishSpecToPR(makeOpts(), runGit, runGh); - - // mock.calls[i] = [args, opts] - const gitCalls = (runGit as ReturnType).mock.calls as [string[], unknown][]; - // Should fetch + checkout + reset — NOT clone - expect(gitCalls.some(([args]) => args[0] === 'clone')).toBe(false); - expect(gitCalls.some(([args]) => args[0] === 'fetch')).toBe(true); - expect(gitCalls.some(([args]) => args[0] === 'reset')).toBe(true); - }); - it('throws with a descriptive message when clone fails', async () => { const runGit: RunGit = vi.fn().mockImplementation(async (args: string[]) => { if (args[0] === 'clone') throw new Error('Authentication failed'); @@ -165,14 +146,15 @@ describe('publishSpecToPR', () => { const runGh = makeRunGh(); await expect(publishSpecToPR(makeOpts(), runGit, runGh)).rejects.toThrow( - /Failed to clone\/update knowledge repo.*Authentication failed/, + /Failed to clone knowledge repo.*Authentication failed/, ); }); it('throws with a descriptive message when push fails', async () => { const runGit: RunGit = vi.fn().mockImplementation(async (args: string[]) => { if (args[0] === 'clone') { - await mkdir(join(knowledgeRepoLocalPath, '.git'), { recursive: true }); + const dest = args[2]!; + await mkdir(join(dest, '.git'), { recursive: true }); return { stdout: '' }; } if (args[0] === 'push') throw new Error('remote: Permission to repo denied'); @@ -186,7 +168,7 @@ describe('publishSpecToPR', () => { }); it('throws with a descriptive message when PR creation fails', async () => { - const runGit = makeRunGit(knowledgeRepoLocalPath); + const runGit = makeRunGit(); const runGh: RunGh = vi.fn().mockImplementation(async (args: string[]) => { if (args[0] === 'pr' && args[1] === 'list') return { stdout: '[]' }; if (args[0] === 'pr' && args[1] === 'create') throw new Error('rate limit exceeded'); @@ -202,19 +184,18 @@ describe('publishSpecToPR', () => { const runGit: RunGit = vi.fn().mockResolvedValue({ stdout: '' }); const runGh = makeRunGh(); - const opts = makeOpts(); const badProduct: Product = { ...makeProduct(), knowledge_repo: { url: 'https://gitlab.com/owner/repo', default_branch: 'main' }, }; - await expect(publishSpecToPR({ ...opts, product: badProduct }, runGit, runGh)).rejects.toThrow( - /Cannot parse knowledge repo URL/, - ); + await expect( + publishSpecToPR({ ...makeOpts(), product: badProduct }, runGit, runGh), + ).rejects.toThrow(/Cannot parse knowledge repo URL/); }); it('uses a branch name derived from the externalId', async () => { - const runGit = makeRunGit(knowledgeRepoLocalPath); + const runGit = makeRunGit(); const runGh = makeRunGh(); await publishSpecToPR(makeOpts({ externalId: 'HLM-99' }), runGit, runGh); @@ -227,7 +208,7 @@ describe('publishSpecToPR', () => { }); it('passes auth token via GIT_HTTP_EXTRAHEADER, not embedded in URL', async () => { - const runGit = makeRunGit(knowledgeRepoLocalPath); + const runGit = makeRunGit(); const runGh = makeRunGh(); await publishSpecToPR(makeOpts({ githubToken: 'secret-token' }), runGit, runGh); @@ -285,4 +266,65 @@ describe('publishSpecToPR', () => { /SSH knowledge repo URLs are not supported.*Use an HTTPS URL/, ); }); + + it('cleans up the temp workdir even when push fails', async () => { + // Capture the clone destination so we can verify it was removed afterwards. + const capturedDirs: string[] = []; + + const runGit: RunGit = vi.fn().mockImplementation(async (args: string[]) => { + if (args[0] === 'clone') { + const dest = args[2]!; + capturedDirs.push(dest); + await mkdir(join(dest, '.git'), { recursive: true }); + return { stdout: '' }; + } + if (args[0] === 'push') throw new Error('remote: Permission to repo denied'); + return { stdout: '' }; + }); + const runGh = makeRunGh(); + + await expect(publishSpecToPR(makeOpts(), runGit, runGh)).rejects.toThrow(/Failed to push/); + + // The temp workdir must be cleaned up even though publish failed. + expect(capturedDirs).toHaveLength(1); + await expect(stat(capturedDirs[0]!)).rejects.toMatchObject({ code: 'ENOENT' }); + }); + + it('two concurrent publishes for the same product use isolated workdirs and do not interfere', async () => { + // Create a second spec file for the second concurrent publish. + const specPath2 = join(tmpDir, 'workdir2', 'specs', 'issue_2.md'); + await mkdir(join(tmpDir, 'workdir2', 'specs'), { recursive: true }); + await writeFile(specPath2, '# issue_2 — Specification\n'); + + const runGit: RunGit = vi.fn().mockImplementation(async (args: string[]) => { + if (args[0] === 'clone') { + const dest = args[2]!; + await mkdir(join(dest, '.git'), { recursive: true }); + } + return { stdout: '' }; + }); + + // Return a PR URL that embeds the branch name so we can verify each publish + // got a distinct, correct URL. + const runGh: RunGh = vi.fn().mockImplementation(async (args: string[]) => { + if (args[0] === 'pr' && args[1] === 'list') return { stdout: '[]' }; + if (args[0] === 'pr' && args[1] === 'create') { + const headIdx = args.indexOf('--head'); + const branch = headIdx >= 0 && args[headIdx + 1] ? args[headIdx + 1] : 'unknown'; + return { stdout: `https://github.com/test-org/knowledge/pull/${branch}\n` }; + } + return { stdout: '' }; + }); + + const [result1, result2] = await Promise.all([ + publishSpecToPR(makeOpts({ externalId: 'issue_1' }), runGit, runGh), + publishSpecToPR(makeOpts({ externalId: 'issue_2', specPath: specPath2 }), runGit, runGh), + ]); + + // Each publish must have produced a PR URL containing its own branch. + expect(result1.prUrl).toContain('helm/spec/issue_1'); + expect(result2.prUrl).toContain('helm/spec/issue_2'); + // The two PRs must be distinct (no cross-contamination). + expect(result1.prUrl).not.toBe(result2.prUrl); + }); }); diff --git a/packages/orchestrator/src/specialists/spec-publisher.ts b/packages/orchestrator/src/specialists/spec-publisher.ts index 16e3248..ed17db7 100644 --- a/packages/orchestrator/src/specialists/spec-publisher.ts +++ b/packages/orchestrator/src/specialists/spec-publisher.ts @@ -1,6 +1,8 @@ -import { copyFile, mkdir, stat } from 'node:fs/promises'; +import { copyFile, mkdir, rm } from 'node:fs/promises'; import { execFile } from 'node:child_process'; -import { join, dirname } from 'node:path'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { randomUUID } from 'node:crypto'; import { promisify } from 'node:util'; import type { Product } from '@helm/shared'; import { parseGitHubRepoUrl } from './fetch-product-context.js'; @@ -14,8 +16,6 @@ export type PublishSpecOpts = { product: Product; /** Absolute path to specs/{externalId}.md in the workdir. */ specPath: string; - /** Absolute path where the knowledge repo should be cloned / already lives. */ - knowledgeRepoLocalPath: string; /** GitHub personal access token (repo scope). */ githubToken: string; }; @@ -61,60 +61,31 @@ export const defaultRunGh: RunGh = async (args, opts) => { return { stdout }; }; -// ── Helpers ─────────────────────────────────────────────────────────────────── - -async function pathExists(p: string): Promise { - try { - await stat(p); - return true; - } catch { - return false; - } -} - -/** - * Ensures the knowledge repo is cloned and up to date. - * If not yet cloned → git clone using plain URL + GIT_HTTP_EXTRAHEADER for auth. - * If already cloned → fetch + reset to latest default branch. - */ -async function ensureKnowledgeRepo( - localPath: string, - repoUrl: string, - defaultBranch: string, - tokenEnv: NodeJS.ProcessEnv, - runGit: RunGit, -): Promise { - const gitDir = join(localPath, '.git'); - if (!(await pathExists(gitDir))) { - await mkdir(dirname(localPath), { recursive: true }); - await runGit(['clone', repoUrl, localPath], { cwd: dirname(localPath), env: tokenEnv }); - } else { - // Pull latest without interactive prompts. - await runGit(['fetch', 'origin'], { cwd: localPath, env: tokenEnv }); - await runGit(['checkout', defaultBranch], { cwd: localPath }); - await runGit(['reset', '--hard', `origin/${defaultBranch}`], { cwd: localPath }); - } -} - // ── Main export ─────────────────────────────────────────────────────────────── /** * Publishes a spec file to the product's knowledge repo as a PR. * + * **Isolation**: each call clones the knowledge repo into a fresh temporary + * directory (under `os.tmpdir()`), performs all git operations there, and + * removes the directory in a `finally` block. This guarantees that two + * concurrent publishes — even for the same product — never share a checkout + * and cannot interleave git operations or corrupt each other's branches. + * * Idempotency: if the branch `helm/spec/{externalId}` already has an open PR, * the branch is updated (force-pushed) and the existing PR URL is returned — * no duplicate PRs are created. * * @param opts Publish options including product config, spec path, and token. - * @param runGit Injectable git runner (defaults to /usr/bin/git via execFile). - * @param runGh Injectable gh runner (defaults to /opt/homebrew/bin/gh). + * @param runGit Injectable git runner (defaults to git via execFile). + * @param runGh Injectable gh runner (defaults to gh via execFile). */ export async function publishSpecToPR( opts: PublishSpecOpts, runGit: RunGit = defaultRunGit, runGh: RunGh = defaultRunGh, ): Promise { - const { externalId, product, specPath, knowledgeRepoLocalPath, githubToken } = opts; + const { externalId, product, specPath, githubToken } = opts; // The (?!\.) lookahead rejects dot-segment values (`.`, `..`, `.hidden`, …) // in addition to the character-class restriction, preventing path traversal. @@ -145,7 +116,7 @@ export async function publishSpecToPR( ); } - // Build token env once — used for clone, fetch, and push (network operations). + // Build token env once — used for clone and push (network operations). // Token is passed as an HTTP Authorization header via git config, so it never // appears in remote URLs or git error messages. const tokenEnv: NodeJS.ProcessEnv = { @@ -154,135 +125,141 @@ export async function publishSpecToPR( GIT_CONFIG_VALUE_0: `Authorization: token ${githubToken}`, }; - // ── Step 1: Clone or update knowledge repo ─────────────────────────────── - try { - await ensureKnowledgeRepo( - knowledgeRepoLocalPath, - knowledgeRepo.url, - defaultBranch, - tokenEnv, - runGit, - ); - } catch (err) { - throw new Error( - `[spec-publisher] Failed to clone/update knowledge repo: ${err instanceof Error ? err.message : String(err)}`, - ); - } + // Each publish gets its own isolated working directory so that concurrent + // publishes for the same product do not share a checkout and cannot interleave + // git operations (checkout, add, commit, push). The directory is always + // removed in the finally block — whether the publish succeeds or fails. + const workDir = join(tmpdir(), `helm-publish-${externalId}-${randomUUID()}`); + await mkdir(workDir, { recursive: true }); - // ── Step 2: Create or reset branch from default branch ────────────────── try { - // -B creates the branch if absent, or resets it to HEAD if it already exists. - await runGit(['checkout', '-B', branchName, `origin/${defaultBranch}`], { - cwd: knowledgeRepoLocalPath, - }); - } catch (err) { - throw new Error( - `[spec-publisher] Failed to create branch '${branchName}': ${err instanceof Error ? err.message : String(err)}`, - ); - } + // ── Step 1: Clone knowledge repo to isolated workdir ──────────────────── + try { + await runGit(['clone', knowledgeRepo.url, workDir], { cwd: tmpdir(), env: tokenEnv }); + } catch (err) { + throw new Error( + `[spec-publisher] Failed to clone knowledge repo: ${err instanceof Error ? err.message : String(err)}`, + ); + } - // ── Step 3: Copy spec into knowledge repo ──────────────────────────────── - const destDir = join(knowledgeRepoLocalPath, 'specs'); - const destPath = join(destDir, `${externalId}.md`); - try { - await mkdir(destDir, { recursive: true }); - await copyFile(specPath, destPath); - } catch (err) { - throw new Error( - `[spec-publisher] Failed to copy spec file: ${err instanceof Error ? err.message : String(err)}`, - ); - } + // ── Step 2: Create or reset branch from default branch ────────────────── + try { + // -B creates the branch if absent, or resets it to HEAD if it already exists. + await runGit(['checkout', '-B', branchName, `origin/${defaultBranch}`], { + cwd: workDir, + }); + } catch (err) { + throw new Error( + `[spec-publisher] Failed to create branch '${branchName}': ${err instanceof Error ? err.message : String(err)}`, + ); + } - // ── Step 4: Commit ─────────────────────────────────────────────────────── - const gitEnv: NodeJS.ProcessEnv = { - GIT_AUTHOR_NAME: 'helm-bot', - GIT_AUTHOR_EMAIL: 'helm-bot@users.noreply.github.com', - GIT_COMMITTER_NAME: 'helm-bot', - GIT_COMMITTER_EMAIL: 'helm-bot@users.noreply.github.com', - }; - try { - await runGit(['add', join('specs', `${externalId}.md`)], { cwd: knowledgeRepoLocalPath }); - await runGit(['commit', '--allow-empty', '-m', `docs(spec): add spec for ${externalId}`], { - cwd: knowledgeRepoLocalPath, - env: gitEnv, - }); - } catch (err) { - throw new Error( - `[spec-publisher] Failed to commit spec: ${err instanceof Error ? err.message : String(err)}`, - ); - } + // ── Step 3: Copy spec into knowledge repo ──────────────────────────────── + const destDir = join(workDir, 'specs'); + const destPath = join(destDir, `${externalId}.md`); + try { + await mkdir(destDir, { recursive: true }); + await copyFile(specPath, destPath); + } catch (err) { + throw new Error( + `[spec-publisher] Failed to copy spec file: ${err instanceof Error ? err.message : String(err)}`, + ); + } - // ── Step 5: Push branch ────────────────────────────────────────────────── - try { - await runGit(['push', 'origin', `${branchName}:${branchName}`, '--force'], { - cwd: knowledgeRepoLocalPath, - env: tokenEnv, - }); - } catch (err) { - throw new Error( - `[spec-publisher] Failed to push branch '${branchName}': ${err instanceof Error ? err.message : String(err)}`, - ); - } + // ── Step 4: Commit ─────────────────────────────────────────────────────── + const gitEnv: NodeJS.ProcessEnv = { + GIT_AUTHOR_NAME: 'helm-bot', + GIT_AUTHOR_EMAIL: 'helm-bot@users.noreply.github.com', + GIT_COMMITTER_NAME: 'helm-bot', + GIT_COMMITTER_EMAIL: 'helm-bot@users.noreply.github.com', + }; + try { + await runGit(['add', join('specs', `${externalId}.md`)], { cwd: workDir }); + await runGit(['commit', '--allow-empty', '-m', `docs(spec): add spec for ${externalId}`], { + cwd: workDir, + env: gitEnv, + }); + } catch (err) { + throw new Error( + `[spec-publisher] Failed to commit spec: ${err instanceof Error ? err.message : String(err)}`, + ); + } - // ── Step 6: Open PR (idempotent) ───────────────────────────────────────── - const ghEnv: NodeJS.ProcessEnv = { GITHUB_TOKEN: githubToken }; + // ── Step 5: Push branch ────────────────────────────────────────────────── + try { + await runGit(['push', 'origin', `${branchName}:${branchName}`, '--force'], { + cwd: workDir, + env: tokenEnv, + }); + } catch (err) { + throw new Error( + `[spec-publisher] Failed to push branch '${branchName}': ${err instanceof Error ? err.message : String(err)}`, + ); + } - // Check for an existing open PR before creating a new one. - let existingPrUrl: string | null = null; - try { - const listOut = await runGh( - [ - 'pr', - 'list', - '--repo', - `${owner}/${repo}`, - '--head', - branchName, - '--state', - 'open', - '--json', - 'url', - ], - { env: ghEnv }, - ); - const prs = JSON.parse(listOut.stdout.trim()) as { url: string }[]; - if (prs.length > 0) existingPrUrl = prs[0]!.url; - } catch (err) { - throw new Error( - `[spec-publisher] Failed to check for existing PRs: ${err instanceof Error ? err.message : String(err)}`, - ); - } + // ── Step 6: Open PR (idempotent) ───────────────────────────────────────── + const ghEnv: NodeJS.ProcessEnv = { GITHUB_TOKEN: githubToken }; - if (existingPrUrl) { - return { prUrl: existingPrUrl }; - } + // Check for an existing open PR before creating a new one. + let existingPrUrl: string | null = null; + try { + const listOut = await runGh( + [ + 'pr', + 'list', + '--repo', + `${owner}/${repo}`, + '--head', + branchName, + '--state', + 'open', + '--json', + 'url', + ], + { env: ghEnv }, + ); + const prs = JSON.parse(listOut.stdout.trim()) as { url: string }[]; + if (prs.length > 0) existingPrUrl = prs[0]!.url; + } catch (err) { + throw new Error( + `[spec-publisher] Failed to check for existing PRs: ${err instanceof Error ? err.message : String(err)}`, + ); + } - // No open PR exists — create one. - let prUrl: string; - try { - const createOut = await runGh( - [ - 'pr', - 'create', - '--repo', - `${owner}/${repo}`, - '--head', - branchName, - '--base', - defaultBranch, - '--title', - `docs(spec): add spec for ${externalId}`, - '--body', - `Spec generated by Helm for item \`${externalId}\` in product \`${product.product.slug}\`.\n\nReview and merge to advance the item to **spec-ready**.`, - ], - { env: ghEnv }, - ); - prUrl = createOut.stdout.trim(); - } catch (err) { - throw new Error( - `[spec-publisher] Failed to create PR: ${err instanceof Error ? err.message : String(err)}`, - ); - } + if (existingPrUrl) { + return { prUrl: existingPrUrl }; + } - return { prUrl }; + // No open PR exists — create one. + let prUrl: string; + try { + const createOut = await runGh( + [ + 'pr', + 'create', + '--repo', + `${owner}/${repo}`, + '--head', + branchName, + '--base', + defaultBranch, + '--title', + `docs(spec): add spec for ${externalId}`, + '--body', + `Spec generated by Helm for item \`${externalId}\` in product \`${product.product.slug}\`.\n\nReview and merge to advance the item to **spec-ready**.`, + ], + { env: ghEnv }, + ); + prUrl = createOut.stdout.trim(); + } catch (err) { + throw new Error( + `[spec-publisher] Failed to create PR: ${err instanceof Error ? err.message : String(err)}`, + ); + } + + return { prUrl }; + } finally { + // Always clean up the isolated workdir, even on error. + await rm(workDir, { recursive: true, force: true }); + } } diff --git a/packages/orchestrator/src/specialists/spec-writer.test.ts b/packages/orchestrator/src/specialists/spec-writer.test.ts index 3462606..4207bf0 100644 --- a/packages/orchestrator/src/specialists/spec-writer.test.ts +++ b/packages/orchestrator/src/specialists/spec-writer.test.ts @@ -230,20 +230,22 @@ describe('handleSpecWriterResult', () => { it('calls publisher and includes prUrl in result when publishOpts provided', async () => { await writeFile(join(workdir, 'specs', 'issue_1.md'), '# Spec'); - const mockRunGit = vi.fn().mockResolvedValue({ stdout: '' }); + const mockRunGit = vi.fn().mockImplementation(async (args: string[]) => { + if (args[0] === 'clone') { + // Simulate clone by creating .git in the temp destination dir. + const dest = args[2]!; + await mkdir(join(dest, '.git'), { recursive: true }); + } + return { stdout: '' }; + }); const mockRunGh = vi.fn().mockImplementation(async (args: string[]) => { if (args[1] === 'list') return { stdout: '[]' }; if (args[1] === 'create') return { stdout: 'https://github.com/test-org/knowledge/pull/1\n' }; return { stdout: '' }; }); - // Pre-create .git so ensureKnowledgeRepo skips the real clone - const knowledgeRepoLocalPath = join(workdir, 'knowledge-repos', 'test-product'); - await mkdir(join(knowledgeRepoLocalPath, '.git'), { recursive: true }); - const publishOpts: SpecPublishOptions = { product: makeProduct(), - knowledgeRepoLocalPath, githubToken: 'test-token', runGit: mockRunGit, runGh: mockRunGh, @@ -274,10 +276,8 @@ describe('handleSpecWriterResult', () => { const mockRunGit = vi.fn().mockRejectedValue(new Error('network timeout')); const mockRunGh = vi.fn().mockResolvedValue({ stdout: '[]' }); - const knowledgeRepoLocalPath = join(workdir, 'knowledge-repos', 'test-product'); const publishOpts: SpecPublishOptions = { product: makeProduct(), - knowledgeRepoLocalPath, githubToken: 'test-token', runGit: mockRunGit, runGh: mockRunGh, diff --git a/packages/orchestrator/src/specialists/spec-writer.ts b/packages/orchestrator/src/specialists/spec-writer.ts index 63b7683..2896a8c 100644 --- a/packages/orchestrator/src/specialists/spec-writer.ts +++ b/packages/orchestrator/src/specialists/spec-writer.ts @@ -127,11 +127,10 @@ export function buildSpecWriterParams( */ export type SpecPublishOptions = { product: Product; - knowledgeRepoLocalPath: string; githubToken: string; - /** Injectable git runner — defaults to /usr/bin/git. For testing. */ + /** Injectable git runner — defaults to git via execFile. For testing. */ runGit?: RunGit; - /** Injectable gh runner — defaults to /opt/homebrew/bin/gh. For testing. */ + /** Injectable gh runner — defaults to gh via execFile. For testing. */ runGh?: RunGh; }; @@ -190,7 +189,6 @@ export async function handleSpecWriterResult( externalId, product: publishOpts.product, specPath, - knowledgeRepoLocalPath: publishOpts.knowledgeRepoLocalPath, githubToken: publishOpts.githubToken, }; try { From 16562ad7955baa7986ba8fe97629121931194175 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Herna=CC=81n=20Pau=CC=81l=20Ossando=CC=81n?= Date: Sun, 24 May 2026 16:03:26 -0400 Subject: [PATCH 09/12] fix(orchestrator): swallow cleanup errors in finally to preserve original error rm() in the finally block could throw (e.g. if the OS temp dir was already purged), which would replace the actual publish error with an ENOENT and make debugging harder. Wrapping with .catch(() => {}) ensures the original error always propagates. Co-Authored-By: Claude Sonnet 4.6 --- packages/orchestrator/src/specialists/spec-publisher.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/orchestrator/src/specialists/spec-publisher.ts b/packages/orchestrator/src/specialists/spec-publisher.ts index ed17db7..9fff29d 100644 --- a/packages/orchestrator/src/specialists/spec-publisher.ts +++ b/packages/orchestrator/src/specialists/spec-publisher.ts @@ -260,6 +260,7 @@ export async function publishSpecToPR( return { prUrl }; } finally { // Always clean up the isolated workdir, even on error. - await rm(workDir, { recursive: true, force: true }); + // Swallow cleanup errors so they never mask the original failure. + await rm(workDir, { recursive: true, force: true }).catch(() => {}); } } From 678d6a254cc87d457ccb1091f0d37b6e5c3f4b65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Herna=CC=81n=20Pau=CC=81l=20Ossando=CC=81n?= Date: Sun, 24 May 2026 17:57:30 -0400 Subject: [PATCH 10/12] fix(orchestrator): authenticate git clone via token-in-URL; sanitize token from errors Root cause: GIT_CONFIG_COUNT/KEY/VALUE env vars for http.extraheader require Git 2.31+ and were silently ignored on older versions, causing 'invalid credentials' on private knowledge repos. Fix: embed the token as x-access-token credentials in the clone URL (https://x-access-token:{token}@github.com/{owner}/{repo}). The 'origin' remote inside the temp clone inherits this URL, so git push origin is authenticated without additional configuration. Security: the authenticated URL is never logged. All error paths now run through sanitizeToken() which replaces both the x-access-token:{token}@ pattern and any bare occurrence of the token with *** before propagating or logging. The token lives on-disk only in the temp clone's .git/config for the duration of the publish operation and is removed with the workdir in the finally block. Also folds two manual edits to spec-writer.ts: - Prompt: replaced the literal '{workdir}' placeholder with a clearer instruction to write relative to cwd (avoids confusing the agent with an unresolved template variable) - Artifact-missing error: includes agentResult.finalOutput in both the returned error string and a new console.error so operators can see WHY the agent didn't write the spec New tests: authenticated clone URL format, token sanitization in errors. Co-Authored-By: Claude Sonnet 4.6 --- .../src/specialists/spec-publisher.test.ts | 44 +++++++--- .../src/specialists/spec-publisher.ts | 86 +++++++++++++------ .../src/specialists/spec-writer.ts | 15 +++- 3 files changed, 107 insertions(+), 38 deletions(-) diff --git a/packages/orchestrator/src/specialists/spec-publisher.test.ts b/packages/orchestrator/src/specialists/spec-publisher.test.ts index 83940f3..9ffe40d 100644 --- a/packages/orchestrator/src/specialists/spec-publisher.test.ts +++ b/packages/orchestrator/src/specialists/spec-publisher.test.ts @@ -207,26 +207,48 @@ describe('publishSpecToPR', () => { expect(checkoutCall![0][2]).toBe('helm/spec/HLM-99'); }); - it('passes auth token via GIT_HTTP_EXTRAHEADER, not embedded in URL', async () => { + it('embeds token in clone URL as x-access-token credentials', async () => { const runGit = makeRunGit(); const runGh = makeRunGh(); await publishSpecToPR(makeOpts({ githubToken: 'secret-token' }), runGit, runGh); - // mock.calls[i] = [args, opts] where opts has .env - const gitCalls = (runGit as ReturnType).mock.calls as [ - string[], - { cwd: string; env?: NodeJS.ProcessEnv }, - ][]; + // mock.calls[i] = [args, opts] + const gitCalls = (runGit as ReturnType).mock.calls as [string[], unknown][]; const cloneCall = gitCalls.find(([args]) => args[0] === 'clone'); const pushCall = gitCalls.find(([args]) => args[0] === 'push'); - // Token must NOT appear in any URL argument - expect(cloneCall![0][1]).not.toContain('secret-token'); - expect(pushCall![0][1]).not.toContain('secret-token'); + // Clone URL must embed the token in the expected format. + expect(cloneCall![0][1]).toBe( + 'https://x-access-token:secret-token@github.com/test-org/knowledge', + ); + + // Push uses 'origin' (inherits authenticated URL from clone) — no explicit + // token in the push args. + expect(pushCall![0][1]).toBe('origin'); + }); + + it('sanitizes token from error messages when clone fails', async () => { + const runGit: RunGit = vi.fn().mockImplementation(async (args: string[]) => { + if (args[0] === 'clone') { + // Simulate git echoing the authenticated URL back in its error message. + throw new Error( + `fatal: repository 'https://x-access-token:secret-token@github.com/test-org/knowledge/' not found`, + ); + } + return { stdout: '' }; + }); + const runGh = makeRunGh(); + + const err = await publishSpecToPR(makeOpts({ githubToken: 'secret-token' }), runGit, runGh) + .then(() => null) + .catch((e: unknown) => e as Error); - // Token must appear as an Authorization header in the git env - expect(cloneCall![1].env?.GIT_CONFIG_VALUE_0).toContain('secret-token'); + expect(err).not.toBeNull(); + // The propagated error must not contain the bare token. + expect(err!.message).not.toContain('secret-token'); + // It should contain the redacted form instead. + expect(err!.message).toContain('x-access-token:***@'); }); it('throws on invalid externalId containing path traversal characters', async () => { diff --git a/packages/orchestrator/src/specialists/spec-publisher.ts b/packages/orchestrator/src/specialists/spec-publisher.ts index 9fff29d..65675c4 100644 --- a/packages/orchestrator/src/specialists/spec-publisher.ts +++ b/packages/orchestrator/src/specialists/spec-publisher.ts @@ -61,6 +61,37 @@ export const defaultRunGh: RunGh = async (args, opts) => { return { stdout }; }; +// ── Auth helpers ───────────────────────────────────────────────────────────── + +/** + * Builds an HTTPS URL with the token embedded as basic-auth credentials. + * Format: `https://x-access-token:{token}@github.com/{owner}/{repo}` + * + * This URL is used for `git clone` only. The `origin` remote inside the + * resulting clone stores this URL (including the token) in `.git/config`. + * Since every publish uses a fresh isolated temp directory that is deleted in + * the `finally` block, the on-disk lifetime of the token is bounded to the + * duration of a single publish operation. + * + * ⚠ NEVER pass this URL to logging calls. Use the plain `knowledgeRepo.url` + * in user-visible messages; pass raw error text through `sanitizeToken`. + */ +function buildAuthenticatedUrl(owner: string, repo: string, token: string): string { + return `https://x-access-token:${token}@github.com/${owner}/${repo}`; +} + +/** + * Redacts any occurrence of the token from a string so that git error messages + * (which may echo the remote URL) are safe to surface to operators. + * + * Replaces both: + * - `x-access-token:@` → `x-access-token:***@` (URL pattern) + * - the bare token string → `***` (safety net) + */ +function sanitizeToken(text: string, token: string): string { + return text.replace(`x-access-token:${token}@`, 'x-access-token:***@').replace(token, '***'); +} + // ── Main export ─────────────────────────────────────────────────────────────── /** @@ -105,10 +136,8 @@ export async function publishSpecToPR( const { owner, repo } = parsed; // SSH-format knowledge repo URLs (git@github.com:org/repo or ssh://...) are - // not supported for token-based authentication: GIT_HTTP_EXTRAHEADER only - // applies to HTTPS operations and has no effect on SSH transport. Fail early - // with a clear message rather than silently falling through to a confusing - // auth error. + // not supported for token-based authentication. Fail early with a clear + // message rather than silently falling through to a confusing auth error. if (knowledgeRepo.url.startsWith('git@') || knowledgeRepo.url.startsWith('ssh://')) { throw new Error( `[spec-publisher] SSH knowledge repo URLs are not supported for token-based auth. ` + @@ -116,14 +145,15 @@ export async function publishSpecToPR( ); } - // Build token env once — used for clone and push (network operations). - // Token is passed as an HTTP Authorization header via git config, so it never - // appears in remote URLs or git error messages. - const tokenEnv: NodeJS.ProcessEnv = { - GIT_CONFIG_COUNT: '1', - GIT_CONFIG_KEY_0: 'http.https://github.com/.extraHeader', - GIT_CONFIG_VALUE_0: `Authorization: token ${githubToken}`, - }; + // Build the authenticated clone URL once. The token is embedded as basic-auth + // credentials so that `git clone` works with all git versions without requiring + // GIT_CONFIG env var support (added in Git 2.31). The `origin` remote inside + // the resulting clone inherits this URL, so `git push origin` is authenticated + // without any additional configuration. + // + // ⚠ Never log `authenticatedUrl`. Always use `knowledgeRepo.url` (the plain + // URL) in error text, and pass raw git error strings through `sanitizeToken`. + const authenticatedUrl = buildAuthenticatedUrl(owner, repo, githubToken); // Each publish gets its own isolated working directory so that concurrent // publishes for the same product do not share a checkout and cannot interleave @@ -134,11 +164,14 @@ export async function publishSpecToPR( try { // ── Step 1: Clone knowledge repo to isolated workdir ──────────────────── + // Use the authenticated URL so git can access private repos. The plain URL + // (without credentials) is used in the error message. try { - await runGit(['clone', knowledgeRepo.url, workDir], { cwd: tmpdir(), env: tokenEnv }); + await runGit(['clone', authenticatedUrl, workDir], { cwd: tmpdir() }); } catch (err) { + const raw = err instanceof Error ? err.message : String(err); throw new Error( - `[spec-publisher] Failed to clone knowledge repo: ${err instanceof Error ? err.message : String(err)}`, + `[spec-publisher] Failed to clone knowledge repo (${knowledgeRepo.url}): ${sanitizeToken(raw, githubToken)}`, ); } @@ -149,8 +182,9 @@ export async function publishSpecToPR( cwd: workDir, }); } catch (err) { + const raw = err instanceof Error ? err.message : String(err); throw new Error( - `[spec-publisher] Failed to create branch '${branchName}': ${err instanceof Error ? err.message : String(err)}`, + `[spec-publisher] Failed to create branch '${branchName}': ${sanitizeToken(raw, githubToken)}`, ); } @@ -161,8 +195,9 @@ export async function publishSpecToPR( await mkdir(destDir, { recursive: true }); await copyFile(specPath, destPath); } catch (err) { + const raw = err instanceof Error ? err.message : String(err); throw new Error( - `[spec-publisher] Failed to copy spec file: ${err instanceof Error ? err.message : String(err)}`, + `[spec-publisher] Failed to copy spec file: ${sanitizeToken(raw, githubToken)}`, ); } @@ -180,20 +215,21 @@ export async function publishSpecToPR( env: gitEnv, }); } catch (err) { - throw new Error( - `[spec-publisher] Failed to commit spec: ${err instanceof Error ? err.message : String(err)}`, - ); + const raw = err instanceof Error ? err.message : String(err); + throw new Error(`[spec-publisher] Failed to commit spec: ${sanitizeToken(raw, githubToken)}`); } // ── Step 5: Push branch ────────────────────────────────────────────────── + // The `origin` remote already holds the authenticated URL from the clone step, + // so no extra credentials are needed here. try { await runGit(['push', 'origin', `${branchName}:${branchName}`, '--force'], { cwd: workDir, - env: tokenEnv, }); } catch (err) { + const raw = err instanceof Error ? err.message : String(err); throw new Error( - `[spec-publisher] Failed to push branch '${branchName}': ${err instanceof Error ? err.message : String(err)}`, + `[spec-publisher] Failed to push branch '${branchName}': ${sanitizeToken(raw, githubToken)}`, ); } @@ -221,8 +257,9 @@ export async function publishSpecToPR( const prs = JSON.parse(listOut.stdout.trim()) as { url: string }[]; if (prs.length > 0) existingPrUrl = prs[0]!.url; } catch (err) { + const raw = err instanceof Error ? err.message : String(err); throw new Error( - `[spec-publisher] Failed to check for existing PRs: ${err instanceof Error ? err.message : String(err)}`, + `[spec-publisher] Failed to check for existing PRs: ${sanitizeToken(raw, githubToken)}`, ); } @@ -252,9 +289,8 @@ export async function publishSpecToPR( ); prUrl = createOut.stdout.trim(); } catch (err) { - throw new Error( - `[spec-publisher] Failed to create PR: ${err instanceof Error ? err.message : String(err)}`, - ); + const raw = err instanceof Error ? err.message : String(err); + throw new Error(`[spec-publisher] Failed to create PR: ${sanitizeToken(raw, githubToken)}`); } return { prUrl }; diff --git a/packages/orchestrator/src/specialists/spec-writer.ts b/packages/orchestrator/src/specialists/spec-writer.ts index 2896a8c..9907ef3 100644 --- a/packages/orchestrator/src/specialists/spec-writer.ts +++ b/packages/orchestrator/src/specialists/spec-writer.ts @@ -93,7 +93,10 @@ Steps: 3. Confirm once the file is written. -Working directory: {workdir} +IMPORTANT: Write the file to the path \`specs/${externalId}.md\` relative to your +current working directory. Create the \`specs/\` directory if it does not exist. +Do not ask for confirmation before writing — create the file directly. + Item: ${externalId} Product: ${product.product.slug}`; } @@ -176,9 +179,17 @@ export async function handleSpecWriterResult( try { await access(specPath); } catch { + // Surface what the agent reported so we can see WHY it didn't write the + // spec (wrong path, confusion, asked a question, etc.) instead of a blind + // "not found". + console.error('[spec-writer] Spec file missing after agent run', { + externalId, + specPath, + agentFinalOutput: agentResult.finalOutput, + }); return { transitioned: false, - error: `spec file not found at ${specPath} — agent did not create it`, + error: `spec file not found at ${specPath} — agent did not create it. Agent said: ${agentResult.finalOutput || '(no output)'}`, }; } From 3c7f1a82fd81ad082fe7ec1b92b91d7f8009f086 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Herna=CC=81n=20Pau=CC=81l=20Ossando=CC=81n?= Date: Sun, 24 May 2026 18:52:59 -0400 Subject: [PATCH 11/12] fix(orchestrator): replaceAll for full token redaction; drop finalOutput from error payload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sanitizeToken used .replace() (replaces first occurrence only) — a verbose git error echoing the authenticated URL twice or including both the URL pattern and the bare token would leak the token in remaining occurrences. Changed both substitutions to .replaceAll() so every occurrence is redacted. handleSpecWriterResult was including agentResult.finalOutput in the error string returned to callers. With product context injected into the prompt, finalOutput can contain README/AGENT.md fragments. Removed finalOutput from the returned error string; the console.error server-side diagnostic retains the full output for operator debugging. Also fixes a pre-existing noUncheckedIndexedAccess TS error in fetch-product-context.test.ts (calls[0] → calls[0]!). Co-Authored-By: Claude Sonnet 4.6 --- .../specialists/fetch-product-context.test.ts | 2 +- .../src/specialists/spec-publisher.test.ts | 31 +++++++++++++++++-- .../src/specialists/spec-publisher.ts | 14 ++++++--- .../src/specialists/spec-writer.ts | 6 +++- 4 files changed, 44 insertions(+), 9 deletions(-) diff --git a/packages/orchestrator/src/specialists/fetch-product-context.test.ts b/packages/orchestrator/src/specialists/fetch-product-context.test.ts index db2fb6c..28a8c35 100644 --- a/packages/orchestrator/src/specialists/fetch-product-context.test.ts +++ b/packages/orchestrator/src/specialists/fetch-product-context.test.ts @@ -172,7 +172,7 @@ describe('fetchProductContext', () => { { headers: Record }, ][]; expect(calls.length).toBeGreaterThan(0); - expect(calls[0][1].headers.Authorization).toBe('Bearer my-token'); + expect(calls[0]![1].headers.Authorization).toBe('Bearer my-token'); }); it('returns empty context when product has no code repos', async () => { diff --git a/packages/orchestrator/src/specialists/spec-publisher.test.ts b/packages/orchestrator/src/specialists/spec-publisher.test.ts index 9ffe40d..e91e268 100644 --- a/packages/orchestrator/src/specialists/spec-publisher.test.ts +++ b/packages/orchestrator/src/specialists/spec-publisher.test.ts @@ -228,7 +228,7 @@ describe('publishSpecToPR', () => { expect(pushCall![0][1]).toBe('origin'); }); - it('sanitizes token from error messages when clone fails', async () => { + it('sanitizes token from error messages when clone fails (single occurrence)', async () => { const runGit: RunGit = vi.fn().mockImplementation(async (args: string[]) => { if (args[0] === 'clone') { // Simulate git echoing the authenticated URL back in its error message. @@ -245,12 +245,37 @@ describe('publishSpecToPR', () => { .catch((e: unknown) => e as Error); expect(err).not.toBeNull(); - // The propagated error must not contain the bare token. expect(err!.message).not.toContain('secret-token'); - // It should contain the redacted form instead. expect(err!.message).toContain('x-access-token:***@'); }); + it('sanitizes ALL occurrences of the token when it appears multiple times in an error', async () => { + const runGit: RunGit = vi.fn().mockImplementation(async (args: string[]) => { + if (args[0] === 'clone') { + // Simulate a verbose git error that echoes the URL twice plus the bare token. + throw new Error( + `error: could not read Username for 'https://x-access-token:secret-token@github.com': ` + + `terminal prompts disabled\n` + + `fatal: repository 'https://x-access-token:secret-token@github.com/test-org/knowledge/' not found\n` + + `hint: token=secret-token`, + ); + } + return { stdout: '' }; + }); + const runGh = makeRunGh(); + + const err = await publishSpecToPR(makeOpts({ githubToken: 'secret-token' }), runGit, runGh) + .then(() => null) + .catch((e: unknown) => e as Error); + + expect(err).not.toBeNull(); + // No occurrence of the bare token must survive — not in the URL pattern, + // not in the bare `token=secret-token` hint line. + expect(err!.message).not.toContain('secret-token'); + // All URL occurrences should be replaced with the redacted form. + expect(err!.message).not.toContain('x-access-token:secret-token@'); + }); + it('throws on invalid externalId containing path traversal characters', async () => { const runGit: RunGit = vi.fn().mockResolvedValue({ stdout: '' }); const runGh = makeRunGh(); diff --git a/packages/orchestrator/src/specialists/spec-publisher.ts b/packages/orchestrator/src/specialists/spec-publisher.ts index 65675c4..85254e0 100644 --- a/packages/orchestrator/src/specialists/spec-publisher.ts +++ b/packages/orchestrator/src/specialists/spec-publisher.ts @@ -81,15 +81,21 @@ function buildAuthenticatedUrl(owner: string, repo: string, token: string): stri } /** - * Redacts any occurrence of the token from a string so that git error messages + * Redacts ALL occurrences of the token from a string so that git error messages * (which may echo the remote URL) are safe to surface to operators. * * Replaces both: - * - `x-access-token:@` → `x-access-token:***@` (URL pattern) - * - the bare token string → `***` (safety net) + * - every `x-access-token:@` → `x-access-token:***@` (URL pattern) + * - every bare token string → `***` (safety net) + * + * Uses replaceAll so that multiple occurrences in a single message are all + * redacted (e.g. a git error that echoes the URL twice, or a stack trace that + * includes both the URL pattern and the raw token). */ function sanitizeToken(text: string, token: string): string { - return text.replace(`x-access-token:${token}@`, 'x-access-token:***@').replace(token, '***'); + return text + .replaceAll(`x-access-token:${token}@`, 'x-access-token:***@') + .replaceAll(token, '***'); } // ── Main export ─────────────────────────────────────────────────────────────── diff --git a/packages/orchestrator/src/specialists/spec-writer.ts b/packages/orchestrator/src/specialists/spec-writer.ts index 9907ef3..a76d36d 100644 --- a/packages/orchestrator/src/specialists/spec-writer.ts +++ b/packages/orchestrator/src/specialists/spec-writer.ts @@ -182,6 +182,10 @@ export async function handleSpecWriterResult( // Surface what the agent reported so we can see WHY it didn't write the // spec (wrong path, confusion, asked a question, etc.) instead of a blind // "not found". + // Log the agent's final output server-side so operators can diagnose WHY + // the spec was not created (wrong path, confusion, pending question, etc.). + // Do NOT include finalOutput in the returned error: with product context + // injected, it may contain fragments of README/AGENT.md content. console.error('[spec-writer] Spec file missing after agent run', { externalId, specPath, @@ -189,7 +193,7 @@ export async function handleSpecWriterResult( }); return { transitioned: false, - error: `spec file not found at ${specPath} — agent did not create it. Agent said: ${agentResult.finalOutput || '(no output)'}`, + error: `spec file not found at ${specPath} — agent did not create it`, }; } From 0f1e331a4c165dc6927b4bd7840bb44ad61eff65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Herna=CC=81n=20Pau=CC=81l=20Ossando=CC=81n?= Date: Sun, 24 May 2026 19:11:06 -0400 Subject: [PATCH 12/12] fix(spec-writer): address three Haystack findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove contradictory "Confirm once the file is written" step from the spec-writer prompt; the IMPORTANT block already says to write directly without asking for confirmation — having both caused agent ambiguity. - Guard buildContextSection call: only render the "## Product Context" section when context actually contains readme or agentMd; an empty object no longer emits a scaffolded section that wastes context tokens. - Add path-validation tests without a custom workdir to confirm the isSafePathPart guard fires before the default workdir (which embeds productSlug/externalId in a filesystem path) is computed. Co-Authored-By: Claude Sonnet 4.6 --- packages/orchestrator/src/dispatcher.test.ts | 31 +++++++++++++++++++ .../src/specialists/spec-writer.test.ts | 5 +++ .../src/specialists/spec-writer.ts | 5 ++- 3 files changed, 38 insertions(+), 3 deletions(-) diff --git a/packages/orchestrator/src/dispatcher.test.ts b/packages/orchestrator/src/dispatcher.test.ts index 38c80df..153c07f 100644 --- a/packages/orchestrator/src/dispatcher.test.ts +++ b/packages/orchestrator/src/dispatcher.test.ts @@ -313,6 +313,37 @@ describe('dispatchStageHandler', () => { expect(result.error).toMatch(/Invalid productSlug or externalId/); }); + it('path-validation fires before default workdir is computed (dot productSlug, no custom workdir)', async () => { + // Guard must reject before dispatcher tries to mkdir the default + // data/worktrees/{productSlug}/{externalId} path, which would embed the + // untrusted value directly into a filesystem path. + const result = await dispatchStageHandler( + { externalId: 'issue_1', productSlug: '.hidden', currentStage: 'discovery' }, + makeProduct(), + makeSpecWriterRuntime('issue_1'), + transition as ItemTransitionFn, + // intentionally no `workdir` option + ); + + expect(result.status).toBe('error'); + expect(result.error).toMatch(/Invalid productSlug or externalId/); + expect(transition).not.toHaveBeenCalled(); + }); + + it('path-validation fires before default workdir is computed (dotdot externalId, no custom workdir)', async () => { + const result = await dispatchStageHandler( + { externalId: '..', productSlug: 'test-product', currentStage: 'discovery' }, + makeProduct(), + makeSpecWriterRuntime('issue_1'), + transition as ItemTransitionFn, + // intentionally no `workdir` option + ); + + expect(result.status).toBe('error'); + expect(result.error).toMatch(/Invalid productSlug or externalId/); + expect(transition).not.toHaveBeenCalled(); + }); + it('propagates prUrl from publish step when token and dataRoot are provided', async () => { const expectedPrUrl = 'https://github.com/test-org/test-knowledge/pull/5'; diff --git a/packages/orchestrator/src/specialists/spec-writer.test.ts b/packages/orchestrator/src/specialists/spec-writer.test.ts index 4207bf0..a618240 100644 --- a/packages/orchestrator/src/specialists/spec-writer.test.ts +++ b/packages/orchestrator/src/specialists/spec-writer.test.ts @@ -82,6 +82,11 @@ describe('buildSpecWriterPrompt', () => { expect(prompt).not.toContain('### Agent Instructions'); expect(prompt).toContain('# Readme'); }); + + it('omits Product Context section when context has neither readme nor agentMd', () => { + const prompt = buildSpecWriterPrompt('issue_1', makeProduct(), {}); + expect(prompt).not.toContain('## Product Context'); + }); }); // ── handleSpecWriterResult ──────────────────────────────────────────────────── diff --git a/packages/orchestrator/src/specialists/spec-writer.ts b/packages/orchestrator/src/specialists/spec-writer.ts index a76d36d..29cca9a 100644 --- a/packages/orchestrator/src/specialists/spec-writer.ts +++ b/packages/orchestrator/src/specialists/spec-writer.ts @@ -70,7 +70,8 @@ export function buildSpecWriterPrompt( product: Product, context?: ProductContext, ): string { - const contextSection = context ? buildContextSection(product, context) : ''; + const contextSection = + context && (context.readme || context.agentMd) ? buildContextSection(product, context) : ''; return `You are the spec writer for the product "${product.product.name}". @@ -91,8 +92,6 @@ Steps: ## Technical Notes -3. Confirm once the file is written. - IMPORTANT: Write the file to the path \`specs/${externalId}.md\` relative to your current working directory. Create the \`specs/\` directory if it does not exist. Do not ask for confirmation before writing — create the file directly.