Skip to content

fix(smithy): sf task commands honor merge.requireApproval config#107

Open
fnnzzz wants to merge 1 commit into
stoneforge-ai:masterfrom
fnnzzz:fix/sf-task-merge-honor-require-approval
Open

fix(smithy): sf task commands honor merge.requireApproval config#107
fnnzzz wants to merge 1 commit into
stoneforge-ai:masterfrom
fnnzzz:fix/sf-task-merge-honor-require-approval

Conversation

@fnnzzz
Copy link
Copy Markdown
Contributor

@fnnzzz fnnzzz commented May 6, 2026

Summary

sf task subcommands (complete, merge, handoff, reject, sync) silently bypass merge.requireApproval because the shared helper that wires their merge provider is hardcoded to LocalMergeProvider. Workspaces using the approve workflow preset never see GitHub PRs created from the CLI even though the long-running orchestrator (server/services.ts) does the right thing — the CLI just no-ops.

Problem

In packages/smithy/src/cli/commands/task.ts, createTaskAssignmentService(options) (the helper every sf task ... subcommand routes through) builds the merge provider with:

const mergeProvider = createLocalMergeProvider();

That wiring ignores config.merge.requireApproval entirely. Server-side, packages/smithy/src/server/services.ts already does the right thing:

const requireApproval = config.merge?.requireApproval ?? false;
mergeRequestProvider: requireApproval ? createGitHubMergeProvider() : undefined,

So setting the workspace to the approve preset turns on PR creation only when the orchestrator runs work — invoking sf task complete <id> from the CLI silently falls back to a no-op local merge instead of opening a GitHub PR. From the user's POV, the workflow preset is broken.

Root cause

packages/smithy/src/cli/commands/task.ts:46 (pre-fix) — const mergeProvider = createLocalMergeProvider(); runs unconditionally inside createTaskAssignmentService, with no read of loadConfig() or config.merge.requireApproval.

Fix

createTaskAssignmentService now mirrors the canonical pattern from server/services.ts:

  1. loadConfig() is destructured from @stoneforge/quarry (the same module the helper already imports dynamically).
  2. The merge provider selection is delegated to a small new helper selectMergeProvider(config, factories) which returns GitHubMergeProvider when config.merge?.requireApproval === true and LocalMergeProvider otherwise. The helper is exported (@internal) so unit tests can pass stub factories.
  3. When requireApproval is true, a new assertGhCliAvailable() runs gh --version and throws a user-friendly error if gh is missing on PATH — instead of silently degrading to LocalMergeProvider (the original bug). The error tells the user how to install gh or how to flip requireApproval back to false.

Default behaviour is unchanged: when requireApproval is unset, false, or merge is absent entirely, LocalMergeProvider is used exactly as before.

Changes

  • packages/smithy/src/cli/commands/task.ts — load config and select provider conditionally; export selectMergeProvider, assertGhCliAvailable, and the supporting types for tests.
  • packages/smithy/src/cli/commands/task.bun.test.ts — new test file (no task.bun.test.ts existed previously) covering provider selection, the gh-missing error path, and command-structure parity with sibling test files.
  • .changeset/fix-sf-task-merge-honor-require-approval.md — patch bump for @stoneforge/smithy.

Test coverage

New cases in task.bun.test.ts:

selectMergeProvider (the regression):

  • Default behaviour preserved — requireApproval=false → LocalMergeProvider, github factory not invoked.
  • GitHub flow engaged — requireApproval=true → GitHubMergeProvider, local factory not invoked.
  • merge.requireApproval is undefined → falls back to LocalMergeProvider (?? false).
  • merge config absent entirely → falls back to LocalMergeProvider.
  • merge.requireApproval is explicitly undefined → falls back to LocalMergeProvider.

assertGhCliAvailable (the new error path):

  • Resolves silently when probe reports available: true.
  • Throws with Cannot create GitHub pull requests when probe reports available: false.
  • Error message references cli.github.com and merge.requireApproval so users know how to fix it.
  • Probe failure reason is included in the detail.
  • No empty () when the probe gives no reason.

Command-structure parity with task-merge-status.bun.test.ts / agent.bun.test.ts:

  • Smoke checks on name/description/handler for taskHandoffCommand, taskCompleteCommand, taskMergeCommand, taskRejectCommand, taskSyncCommand, taskSetOwnerCommand plus a few key options (--no-mr, --reason).

All 21 new tests pass; the package's full Bun suite (bun test src) is green at 1589 pass / 0 fail / 19 skip.

Test plan

  • pnpm install at the monorepo root.
  • cd packages/smithy && pnpm run typecheck — clean.
  • cd packages/smithy && pnpm run build — clean (compiled dist/cli/commands/task.js reflects the fix).
  • cd packages/smithy && bun test src/cli/commands/task.bun.test.ts — 21 pass, 0 fail.
  • cd packages/smithy && pnpm run test — 1589 pass, 0 fail, 19 skip across 53 files.
  • Manual smoke on a temp workspace with three configs:
    1. merge.requireApproval=true, gh stripped from PATH → command exits with the new clear error: Cannot create GitHub pull requests: the \gh` CLI is not available on PATH (Executable not found in $PATH: "gh"). Install it from https://cli.github.com/ and run `gh auth login`, or set `merge.requireApproval` to `false` in your workspace config to use the local merge provider.`
    2. merge.requireApproval=false → command proceeds past provider initialization (errors later on the unrelated "Task not found" lookup, exactly as on master).
    3. merge.requireApproval=true, gh available → command proceeds past provider initialization (same "Task not found" downstream error, confirming gh check passes).

Checklist

  • Tests pass
  • Typecheck passes
  • Changeset added (if applicable)

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 6, 2026

CLA assistant check
All committers have signed the CLA.

The shared createTaskAssignmentService helper used by sf task complete,
sf task merge, sf task handoff, etc. was hardcoded to wire a
LocalMergeProvider regardless of workspace config. The workflow preset
'approve' (which sets merge.requireApproval=true) was therefore silently
downgraded to a no-op local merge instead of the GitHub PR flow the
long-running orchestrator already uses in server/services.ts.

This change mirrors that wiring in the CLI: when merge.requireApproval
is true the CLI uses GitHubMergeProvider via the existing
createGitHubMergeProvider() factory, otherwise it stays on the existing
LocalMergeProvider. Default behaviour for users without merge config is
unchanged.

When requireApproval=true and the gh CLI is not on PATH, the command
fails fast with an actionable error pointing the user at install/auth
or at the requireApproval=false fallback — instead of silently degrading
to LocalMergeProvider, which is the original bug.

Tests: adds task.bun.test.ts covering provider selection (default,
explicit false, true, missing config), the gh-missing error path, and
command-structure parity with sibling test files.
@fnnzzz fnnzzz force-pushed the fix/sf-task-merge-honor-require-approval branch from ddc29ea to ac24935 Compare May 7, 2026 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants