From 9e28c3f06e5c745f20418319b8aa1126f289c9e1 Mon Sep 17 00:00:00 2001 From: Adrian Komorek Date: Thu, 7 May 2026 08:17:03 +0200 Subject: [PATCH] fix(smithy): sf task merge honors workspace merge.targetBranch config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `merge.targetBranch` value in `.stoneforge/config.yaml` was previously only consumed by prompt-builders (sessions, steward scheduler, dispatch daemon) to populate WorkflowPresetContext for agent system prompts. The actual git tooling in the `sf task` CLI commands never loaded it, so setting `merge.target_branch: dev` told agents about the target in their prompts but the merge operations themselves still defaulted to git origin/HEAD (typically `main`). Resolution chain across all merge-related paths is now: task.targetBranch → config.merge.targetBranch → git origin/HEAD → main Wired in: - packages/smithy/src/cli/commands/task.ts (merge handler, sync handler, update --status merged verify path) - packages/smithy/src/services/task-assignment-service.ts (MR-creation path used by sf task complete) A new exported `loadConfigTargetBranch()` helper centralises the config read with safe handling of missing config, missing merge block, null values, and empty strings. Six unit tests in task-target-branch.bun.test.ts cover those cases. Stewards (which cannot own a targetBranch field per the agent type contract) finally have a config-only path to set a project-wide target. Refs #63. --- ...ask-merge-target-branch-config-fallback.md | 9 ++ .../commands/task-target-branch.bun.test.ts | 89 +++++++++++++++++++ packages/smithy/src/cli/commands/task.ts | 44 +++++++-- .../src/services/task-assignment-service.ts | 17 +++- 4 files changed, 152 insertions(+), 7 deletions(-) create mode 100644 .changeset/sf-task-merge-target-branch-config-fallback.md create mode 100644 packages/smithy/src/cli/commands/task-target-branch.bun.test.ts diff --git a/.changeset/sf-task-merge-target-branch-config-fallback.md b/.changeset/sf-task-merge-target-branch-config-fallback.md new file mode 100644 index 00000000..ecea12f4 --- /dev/null +++ b/.changeset/sf-task-merge-target-branch-config-fallback.md @@ -0,0 +1,9 @@ +--- +"@stoneforge/smithy": patch +--- + +fix(smithy): `sf task merge` honors workspace `merge.targetBranch` config (#63) + +`sf task merge`, `sf task complete`, `sf task sync`, and `sf task update --status merged` previously resolved the merge target as `task.targetBranch → git origin/HEAD → main`, ignoring the workspace-level `merge.targetBranch` value in `.stoneforge/config.yaml`. The config value was only consumed by prompt-builders (sessions, steward scheduler, dispatch daemon) for system-prompt context, so setting `merge.target_branch: dev` told agents the right thing in their prompts but did nothing to the actual git tooling. + +Resolution is now `task.targetBranch → workspace config (`merge.targetBranch`) → git origin/HEAD → main` across all merge-related CLI paths and the MR-creation path in `task-assignment-service.ts`. Stewards (which cannot own a `targetBranch` field directly) finally have a config-only path to set a project-wide target. diff --git a/packages/smithy/src/cli/commands/task-target-branch.bun.test.ts b/packages/smithy/src/cli/commands/task-target-branch.bun.test.ts new file mode 100644 index 00000000..b6686d18 --- /dev/null +++ b/packages/smithy/src/cli/commands/task-target-branch.bun.test.ts @@ -0,0 +1,89 @@ +/** + * Tests for the workspace `merge.targetBranch` config fallback used by the + * `sf task` merge-related commands. Regression for #63: previously, the + * config workaround Adam suggested was only consumed by prompt-builders + * (sessions/scheduler/dispatch-daemon) for system-prompt context — the actual + * git tooling never loaded it, so setting `merge.target_branch` had no effect + * on `sf task merge` / `sf task complete` / `sf task update --status merged`. + */ + +import { describe, it, expect, afterEach } from 'bun:test'; +import { mkdtempSync, rmSync, mkdirSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { loadConfigTargetBranch } from './task.js'; + +// ============================================================================ +// Helpers +// ============================================================================ + +const cleanups: Array<() => void> = []; + +afterEach(() => { + while (cleanups.length > 0) { + const fn = cleanups.pop(); + try { fn?.(); } catch { /* ignore */ } + } +}); + +/** + * Creates a tmp `.stoneforge` workspace with an optional `config.yaml`, + * chdirs into it, and registers cleanup. + */ +function chdirIntoTmpWorkspace(yamlContent?: string): string { + const root = mkdtempSync(join(tmpdir(), 'sf-target-branch-test-')); + mkdirSync(join(root, '.stoneforge'), { recursive: true }); + if (yamlContent !== undefined) { + writeFileSync(join(root, '.stoneforge', 'config.yaml'), yamlContent, 'utf8'); + } + const cwdBefore = process.cwd(); + process.chdir(root); + cleanups.push(() => { + process.chdir(cwdBefore); + rmSync(root, { recursive: true, force: true }); + }); + return root; +} + +// ============================================================================ +// Tests +// ============================================================================ + +describe('loadConfigTargetBranch', () => { + it('returns the configured value when `merge.target_branch` is set', async () => { + chdirIntoTmpWorkspace('merge:\n target_branch: dev\n'); + const value = await loadConfigTargetBranch(); + expect(value).toBe('dev'); + }); + + it('returns undefined when no config file exists', async () => { + chdirIntoTmpWorkspace(); + const value = await loadConfigTargetBranch(); + expect(value).toBeUndefined(); + }); + + it('returns undefined when config exists but `merge` block is absent', async () => { + chdirIntoTmpWorkspace('logging:\n level: info\n'); + const value = await loadConfigTargetBranch(); + expect(value).toBeUndefined(); + }); + + it('returns undefined when `merge.target_branch` is null', async () => { + chdirIntoTmpWorkspace('merge:\n target_branch: null\n'); + const value = await loadConfigTargetBranch(); + expect(value).toBeUndefined(); + }); + + it('returns undefined when `merge.target_branch` is an empty string', async () => { + chdirIntoTmpWorkspace('merge:\n target_branch: ""\n'); + const value = await loadConfigTargetBranch(); + expect(value).toBeUndefined(); + }); + + it('preserves non-default branch names (e.g. "develop", "release/v2")', async () => { + chdirIntoTmpWorkspace('merge:\n target_branch: release/v2\n'); + const value = await loadConfigTargetBranch(); + expect(value).toBe('release/v2'); + }); +}); diff --git a/packages/smithy/src/cli/commands/task.ts b/packages/smithy/src/cli/commands/task.ts index 982c2aba..1395a32e 100644 --- a/packages/smithy/src/cli/commands/task.ts +++ b/packages/smithy/src/cli/commands/task.ts @@ -18,6 +18,28 @@ import { TaskStatus, createTimestamp } from '@stoneforge/core'; // Shared Helpers // ============================================================================ +/** + * Loads the workspace-level `merge.targetBranch` from `.stoneforge/config.yaml`, + * if set. Returns `undefined` if config is missing, `merge` is unset, or the + * value is null/empty. + * + * This is the second-priority fallback when resolving an effective target + * branch for merge operations: task metadata wins, then this config value, + * then git origin/HEAD detection, then the hardcoded `main` default. + * + * Exported so callers (and tests) can reuse the same precedence rule. + */ +export async function loadConfigTargetBranch(): Promise { + try { + const { loadConfig } = await import('@stoneforge/quarry'); + const config = loadConfig(); + const value = (config.merge as { targetBranch?: string | null } | undefined)?.targetBranch; + return typeof value === 'string' && value.length > 0 ? value : undefined; + } catch { + return undefined; + } +} + /** * Creates task assignment service */ @@ -414,7 +436,12 @@ async function taskMergeHandler( const { getOrchestratorTaskMeta, updateOrchestratorTaskMeta } = await import('../../types/task-meta.js'); const orchestratorMeta = getOrchestratorTaskMeta(task.metadata as Record); const sourceBranch = orchestratorMeta?.branch; - const targetBranch = orchestratorMeta?.targetBranch; + // Resolution: task metadata → workspace config → git detection (in mergeBranch). + // Workspace config (`merge.targetBranch` in `.stoneforge/config.yaml`) lets + // operators set a project-wide target without per-task or per-director + // metadata, which is the only path stewards have today since they cannot + // own a `targetBranch` field directly. + const targetBranch = orchestratorMeta?.targetBranch ?? await loadConfigTargetBranch(); if (!sourceBranch) { return failure(`Task ${taskId} has no branch in orchestrator metadata.`, ExitCode.GENERAL_ERROR); @@ -457,6 +484,8 @@ async function taskMergeHandler( const { exec: execCb } = await import('node:child_process'); const { promisify: promisifyUtil } = await import('node:util'); const execVerify = promisifyUtil(execCb); + // `targetBranch` already includes the workspace config fallback (see resolution comment above); + // detectTargetBranch is the final fallback to git origin/HEAD when neither is set. const effectiveTargetForVerify = targetBranch ?? await detectTargetBranch(workspaceRoot); try { @@ -864,9 +893,10 @@ async function taskSyncHandler( return failure(syncResult.message, ExitCode.GENERAL_ERROR); } - // Use the task's targetBranch if set, otherwise fall back to default branch - const targetBranch = orchestratorMeta?.targetBranch as string | undefined; - const syncBranch = targetBranch ?? await worktreeManager.getDefaultBranch(); + // Use the task's targetBranch if set, then workspace config `merge.targetBranch`, + // otherwise fall back to the worktree's default branch. + const taskTargetBranch = orchestratorMeta?.targetBranch as string | undefined; + const syncBranch = taskTargetBranch ?? await loadConfigTargetBranch() ?? await worktreeManager.getDefaultBranch(); const remoteBranch = `origin/${syncBranch}`; // Attempt to merge @@ -1162,9 +1192,11 @@ async function taskMergeStatusHandler( } catch { /* no remote */ } if (hasRemote) { - // Determine effective target branch + // Determine effective target branch: + // task metadata → workspace config → git origin/HEAD → "main" const { detectTargetBranch } = await import('../../git/merge.js'); - const effectiveTarget = targetBranch ?? await detectTargetBranch(workspaceRoot); + const configTarget = await loadConfigTargetBranch(); + const effectiveTarget = targetBranch ?? configTarget ?? await detectTargetBranch(workspaceRoot); const mergeCommitHash = orchestratorMeta?.mergeCommitHash; const result = await verifyMergeStatus({ diff --git a/packages/smithy/src/services/task-assignment-service.ts b/packages/smithy/src/services/task-assignment-service.ts index 18f1ebd8..36796451 100644 --- a/packages/smithy/src/services/task-assignment-service.ts +++ b/packages/smithy/src/services/task-assignment-service.ts @@ -564,7 +564,22 @@ export class TaskAssignmentServiceImpl implements TaskAssignmentService { let mergeRequestId: number | undefined; if (branch && this.mergeRequestProvider && options?.createMergeRequest !== false) { - const baseBranch = options?.baseBranch || currentMeta?.targetBranch || 'main'; + // Resolution order: explicit --baseBranch → task's targetBranch metadata → + // workspace config (`merge.targetBranch`) → hardcoded `main`. Without the + // config layer, stewards (which cannot own a `targetBranch` field) had no + // way to set a project-wide target. + let configTargetBranch: string | undefined; + try { + const { loadConfig } = await import('@stoneforge/quarry'); + const cfg = loadConfig(); + const value = (cfg.merge as { targetBranch?: string | null } | undefined)?.targetBranch; + if (typeof value === 'string' && value.length > 0) { + configTargetBranch = value; + } + } catch { + // Config not loadable — fall through to hardcoded default + } + const baseBranch = options?.baseBranch || currentMeta?.targetBranch || configTargetBranch || 'main'; let body = `## Task\n\n**ID:** ${task.id}\n**Title:** ${task.title}\n\n`; if (options?.summary) { body += `## Summary\n\n${options.summary}\n\n`;