diff --git a/CLAUDE.md b/CLAUDE.md index e501d6ee4f..bb6f65fd5b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -259,6 +259,10 @@ bun run cli workflow abandon # list, status, runs, get, approve, reject, abandon, resume. # For approve/reject/resume, --json records/validates the decision and returns a # clean JSON line WITHOUT the inline auto-resume (drive continuation separately). +# Adding --detach INVERTS that: the child re-invokes without --json, so it takes +# the inline path and DOES continue the run (just outside your shell). The ack +# carries `continues: true` so an automation knows it no longer owns continuation. +bun run cli workflow approve --detach --json # Delete old workflow run records (default: 7 days) bun run cli workflow cleanup diff --git a/packages/cli/src/cli.ts b/packages/cli/src/cli.ts index c1b91c9fd5..80aef4de0e 100755 --- a/packages/cli/src/cli.ts +++ b/packages/cli/src/cli.ts @@ -161,7 +161,7 @@ Options: --quiet, -q Reduce log verbosity to warnings and errors only --verbose, -v Show debug-level output --json Output machine-readable JSON (list/status/get/runs/approve/reject/abandon/resume) - --detach Run 'workflow run' in a detached background child (returns immediately) + --detach Run 'workflow run'/'approve'/'reject'/'resume' in a detached background child (returns immediately) --all For 'workflow runs': list across all projects (ignore cwd scope) --status For 'workflow runs': filter to one status (running, completed, failed, ...) --limit For 'workflow runs': max rows (default 20) @@ -618,7 +618,7 @@ async function main(): Promise { console.error('Usage: archon workflow resume '); return 1; } - await workflowResumeCommand(resumeRunId, jsonFlag, effectiveCwd); + await workflowResumeCommand(resumeRunId, jsonFlag, effectiveCwd, detachFlag); break; } @@ -645,7 +645,13 @@ async function main(): Promise { const rawApproveComment = (values.comment as string | undefined) || positionals.slice(3).join(' '); const approveComment = rawApproveComment.length > 0 ? rawApproveComment : undefined; - await workflowApproveCommand(approveRunId, approveComment, jsonFlag, effectiveCwd); + await workflowApproveCommand( + approveRunId, + approveComment, + jsonFlag, + effectiveCwd, + detachFlag + ); break; } @@ -658,7 +664,13 @@ async function main(): Promise { const rawRejectReason = (values.reason as string | undefined) || positionals.slice(3).join(' '); const rejectReason = rawRejectReason.length > 0 ? rawRejectReason : undefined; - await workflowRejectCommand(rejectRunId, rejectReason, jsonFlag, effectiveCwd); + await workflowRejectCommand( + rejectRunId, + rejectReason, + jsonFlag, + effectiveCwd, + detachFlag + ); break; } diff --git a/packages/cli/src/commands/workflow.test.ts b/packages/cli/src/commands/workflow.test.ts index f03533d324..7933f26107 100644 --- a/packages/cli/src/commands/workflow.test.ts +++ b/packages/cli/src/commands/workflow.test.ts @@ -3196,6 +3196,380 @@ describe('workflowRunCommand — detach', () => { }); }); +describe('workflowApproveCommand / workflowRejectCommand / workflowResumeCommand — detach', () => { + let consoleSpy: ReturnType; + + beforeEach(() => { + consoleSpy = spyOn(console, 'log').mockImplementation(() => {}); + }); + + afterEach(() => { + consoleSpy.mockRestore(); + }); + + // working_path is deliberately a distro-style path: the child must spawn with + // the PARENT's cwd (it re-resolves by run-id), because a container run's + // working_path is unreachable on the host and would ENOENT the spawn. + const pausedRun = { + id: 'run-123', + status: 'paused', + workflow_name: 'assist', + working_path: '/distro/only/path', + conversation_id: 'conv-123', + user_message: 'hello', + metadata: { approval: { nodeId: 'gate', message: 'Approve?' } }, + }; + + // Mirrors the run-detach tests: pid is required (spawnDetachedWorkflowRun + // throws when child.pid === undefined), and node:child_process.spawn routes + // through Bun.spawn so spying on Bun.spawn intercepts it. + const mockSpawn = () => + spyOn(Bun, 'spawn').mockReturnValue({ + pid: 12345, + unref: mock(() => undefined), + } as unknown as ReturnType); + + it('approve --detach spawns a detached child (minus --detach) and performs ZERO writes in the parent', async () => { + const workflowDb = await import('@archon/core/db/workflows'); + const { executeWorkflow } = await import('@archon/workflows/executor'); + const paths = await import('@archon/paths'); + (workflowDb.getWorkflowRun as ReturnType).mockResolvedValueOnce({ ...pausedRun }); + // Force the log-file path to fall back to 'ignore' so the test writes no files + (paths.getArchonHome as ReturnType).mockImplementationOnce(() => { + throw new Error('no home in test'); + }); + + const updateBefore = (workflowDb.updateWorkflowRun as ReturnType).mock.calls + .length; + const execBefore = (executeWorkflow as ReturnType).mock.calls.length; + const spawnSpy = mockSpawn(); + const savedArgv = process.argv; + process.argv = ['bun', '/abs/cli.ts', 'workflow', 'approve', 'run-123', 'ship it', '--detach']; + + let spawnCallCount = 0; + let spawnCmd: string[] = []; + let spawnOptions: { cwd: string; cmd: string[]; detached?: boolean } | undefined; + try { + // Signature: (runId, comment, json, cwd, detach) — detach is the 5th arg, + // layered after upstream's cwd (the orthogonal-collision the plan warns of). + await workflowApproveCommand('run-123', 'ship it', undefined, undefined, true); + spawnCallCount = spawnSpy.mock.calls.length; + spawnOptions = spawnSpy.mock.calls[0]?.[0] as + | { cwd: string; cmd: string[]; detached?: boolean } + | undefined; + spawnCmd = (spawnOptions?.cmd ?? []).slice(); + } finally { + process.argv = savedArgv; + spawnSpy.mockRestore(); + } + + expect(spawnCallCount).toBe(1); + expect(spawnOptions?.detached).toBe(true); + expect(spawnCmd).not.toContain('--detach'); + expect(spawnCmd).toContain('approve'); + expect(spawnCmd).toContain('run-123'); + // Parent cwd, never the run's working_path (see pausedRun comment above). + expect(spawnOptions?.cwd).toBe(process.cwd()); + expect(spawnCmd).not.toContain('/distro/only/path'); + // ZERO state mutation in the detaching parent — the child owns the approve + // (a parent-side approve would record the decision twice). + expect((workflowDb.updateWorkflowRun as ReturnType).mock.calls.length).toBe( + updateBefore + ); + expect((executeWorkflow as ReturnType).mock.calls.length).toBe(execBefore); + expect(consoleSpy).toHaveBeenCalledWith("Started 'approve' for run run-123 in the background."); + }); + + it('approve --detach --json emits a structured ack without approving', async () => { + const workflowDb = await import('@archon/core/db/workflows'); + const paths = await import('@archon/paths'); + (workflowDb.getWorkflowRun as ReturnType).mockResolvedValueOnce({ ...pausedRun }); + (paths.getArchonHome as ReturnType).mockImplementationOnce(() => { + throw new Error('no home in test'); + }); + const spawnSpy = mockSpawn(); + const savedArgv = process.argv; + process.argv = ['bun', '/abs/cli.ts', 'workflow', 'approve', 'run-123', '--detach', '--json']; + + try { + await workflowApproveCommand('run-123', undefined, true, undefined, true); + } finally { + process.argv = savedArgv; + spawnSpy.mockRestore(); + } + + const parsed = JSON.parse(consoleSpy.mock.calls[0][0] as string) as Record; + expect(parsed).toMatchObject({ + ok: true, + runId: 'run-123', + action: 'approve', + detached: true, + workflowName: 'assist', + }); + }); + + it('--detach --json spawns a child WITHOUT --json (so it continues) and says so in the ack', async () => { + const workflowDb = await import('@archon/core/db/workflows'); + const paths = await import('@archon/paths'); + (workflowDb.getWorkflowRun as ReturnType).mockResolvedValueOnce({ ...pausedRun }); + (paths.getArchonHome as ReturnType).mockImplementationOnce(() => { + throw new Error('no home in test'); + }); + const spawnSpy = mockSpawn(); + const savedArgv = process.argv; + process.argv = ['bun', '/abs/cli.ts', 'workflow', 'approve', 'run-123', '--detach', '--json']; + + let spawnCmd: string[] = []; + try { + await workflowApproveCommand('run-123', undefined, true, undefined, true); + spawnCmd = ( + (spawnSpy.mock.calls[0]?.[0] as { cmd: string[] } | undefined)?.cmd ?? [] + ).slice(); + } finally { + process.argv = savedArgv; + spawnSpy.mockRestore(); + } + + // Both flags are stripped: the child runs the ordinary inline path, which + // auto-resumes. That is deliberate — --detach exists to host that execution. + expect(spawnCmd).not.toContain('--detach'); + expect(spawnCmd).not.toContain('--json'); + // ...so the ack must tell the caller it does NOT own continuation. + const parsed = JSON.parse(consoleSpy.mock.calls[0][0] as string) as Record; + expect(parsed).toMatchObject({ ok: true, detached: true, continues: true }); + }); + + it('threads a caller-supplied --cwd to the child instead of the parent process.cwd()', async () => { + const workflowDb = await import('@archon/core/db/workflows'); + const paths = await import('@archon/paths'); + (workflowDb.getWorkflowRun as ReturnType).mockResolvedValueOnce({ ...pausedRun }); + (paths.getArchonHome as ReturnType).mockImplementationOnce(() => { + throw new Error('no home in test'); + }); + const spawnSpy = mockSpawn(); + const savedArgv = process.argv; + process.argv = [ + 'bun', + '/abs/cli.ts', + 'workflow', + 'approve', + 'run-123', + '--cwd', + '/caller/repo', + '--detach', + ]; + + let spawnCmd: string[] = []; + let spawnOptions: { cwd: string; cmd: string[] } | undefined; + try { + await workflowApproveCommand('run-123', undefined, undefined, '/caller/repo', true); + spawnOptions = spawnSpy.mock.calls[0]?.[0] as { cwd: string; cmd: string[] } | undefined; + spawnCmd = (spawnOptions?.cmd ?? []).slice(); + } finally { + process.argv = savedArgv; + spawnSpy.mockRestore(); + } + + expect(spawnOptions?.cwd).toBe('/caller/repo'); + // buildDetachedRunCmd appends --cwd LAST (parser is last-wins), so the + // appended value is what the child actually resolves. + const lastCwdIdx = spawnCmd.lastIndexOf('--cwd'); + expect(spawnCmd[lastCwdIdx + 1]).toBe('/caller/repo'); + }); + + it('approve --detach refuses a non-paused run synchronously and spawns nothing', async () => { + const workflowDb = await import('@archon/core/db/workflows'); + (workflowDb.getWorkflowRun as ReturnType).mockResolvedValueOnce({ + ...pausedRun, + status: 'running', + }); + const spawnSpy = mockSpawn(); + + let spawnCallCount = -1; + try { + await expect( + workflowApproveCommand('run-123', undefined, undefined, undefined, true) + ).rejects.toThrow("Cannot approve run with status 'running'"); + spawnCallCount = spawnSpy.mock.calls.length; + } finally { + spawnSpy.mockRestore(); + } + expect(spawnCallCount).toBe(0); + }); + + it('approve --detach refuses a child_workflow-blocked parent and spawns nothing', async () => { + const workflowDb = await import('@archon/core/db/workflows'); + (workflowDb.getWorkflowRun as ReturnType).mockResolvedValueOnce({ + ...pausedRun, + metadata: { + approval: { nodeId: 'sub', message: 'blocked', type: 'child_workflow', childRunId: 'c-9' }, + }, + }); + const spawnSpy = mockSpawn(); + try { + await expect( + workflowApproveCommand('run-123', undefined, undefined, undefined, true) + ).rejects.toThrow('Approve or reject the child run instead: /workflow approve c-9'); + expect(spawnSpy.mock.calls.length).toBe(0); + } finally { + spawnSpy.mockRestore(); + } + }); + + it('approve --detach refuses an already-resolved gate and spawns nothing', async () => { + const workflowDb = await import('@archon/core/db/workflows'); + (workflowDb.getWorkflowRun as ReturnType).mockResolvedValueOnce({ + ...pausedRun, + metadata: { approval: { nodeId: 'gate', message: 'Approve?', resolved: 'approved' } }, + }); + const spawnSpy = mockSpawn(); + try { + await expect( + workflowApproveCommand('run-123', undefined, undefined, undefined, true) + ).rejects.toThrow('was already approved and is awaiting resume'); + expect(spawnSpy.mock.calls.length).toBe(0); + } finally { + spawnSpy.mockRestore(); + } + }); + + it('approve --detach refuses a missing approval context and spawns nothing', async () => { + const workflowDb = await import('@archon/core/db/workflows'); + (workflowDb.getWorkflowRun as ReturnType).mockResolvedValueOnce({ + ...pausedRun, + metadata: {}, + }); + const spawnSpy = mockSpawn(); + try { + await expect( + workflowApproveCommand('run-123', undefined, undefined, undefined, true) + ).rejects.toThrow('Workflow run is paused but missing approval context.'); + expect(spawnSpy.mock.calls.length).toBe(0); + } finally { + spawnSpy.mockRestore(); + } + }); + + it('reject --detach refuses a child_workflow-blocked parent but TOLERATES missing context', async () => { + const workflowDb = await import('@archon/core/db/workflows'); + const paths = await import('@archon/paths'); + (workflowDb.getWorkflowRun as ReturnType).mockResolvedValueOnce({ + ...pausedRun, + metadata: { + approval: { nodeId: 'sub', message: 'blocked', type: 'child_workflow', childRunId: 'c-9' }, + }, + }); + let spawnSpy = mockSpawn(); + try { + await expect( + workflowRejectCommand('run-123', undefined, undefined, undefined, true) + ).rejects.toThrow('Reject the child run instead: /workflow reject c-9'); + expect(spawnSpy.mock.calls.length).toBe(0); + } finally { + spawnSpy.mockRestore(); + } + + // reject has no nodeId requirement — a malformed context must still spawn. + (workflowDb.getWorkflowRun as ReturnType).mockResolvedValueOnce({ + ...pausedRun, + metadata: {}, + }); + (paths.getArchonHome as ReturnType).mockImplementationOnce(() => { + throw new Error('no home in test'); + }); + spawnSpy = mockSpawn(); + const savedArgv = process.argv; + process.argv = ['bun', '/abs/cli.ts', 'workflow', 'reject', 'run-123', '--detach']; + try { + await workflowRejectCommand('run-123', undefined, undefined, undefined, true); + expect(spawnSpy.mock.calls.length).toBe(1); + } finally { + process.argv = savedArgv; + spawnSpy.mockRestore(); + } + }); + + it('reject --detach spawns a detached child (minus --detach) and performs ZERO writes in the parent', async () => { + const workflowDb = await import('@archon/core/db/workflows'); + const { executeWorkflow } = await import('@archon/workflows/executor'); + const paths = await import('@archon/paths'); + (workflowDb.getWorkflowRun as ReturnType).mockResolvedValueOnce({ ...pausedRun }); + (paths.getArchonHome as ReturnType).mockImplementationOnce(() => { + throw new Error('no home in test'); + }); + + const updateBefore = (workflowDb.updateWorkflowRun as ReturnType).mock.calls + .length; + const execBefore = (executeWorkflow as ReturnType).mock.calls.length; + const spawnSpy = mockSpawn(); + const savedArgv = process.argv; + process.argv = ['bun', '/abs/cli.ts', 'workflow', 'reject', 'run-123', 'not good', '--detach']; + + let spawnCallCount = 0; + let spawnCmd: string[] = []; + try { + await workflowRejectCommand('run-123', 'not good', undefined, undefined, true); + spawnCallCount = spawnSpy.mock.calls.length; + spawnCmd = ( + (spawnSpy.mock.calls[0]?.[0] as { cmd: string[] } | undefined)?.cmd ?? [] + ).slice(); + } finally { + process.argv = savedArgv; + spawnSpy.mockRestore(); + } + + expect(spawnCallCount).toBe(1); + expect(spawnCmd).not.toContain('--detach'); + expect(spawnCmd).toContain('reject'); + expect(spawnCmd).toContain('run-123'); + expect((workflowDb.updateWorkflowRun as ReturnType).mock.calls.length).toBe( + updateBefore + ); + expect((executeWorkflow as ReturnType).mock.calls.length).toBe(execBefore); + expect(consoleSpy).toHaveBeenCalledWith("Started 'reject' for run run-123 in the background."); + }); + + it('resume --detach spawns a detached child (minus --detach) and does NOT execute in the parent', async () => { + const workflowDb = await import('@archon/core/db/workflows'); + const { executeWorkflow } = await import('@archon/workflows/executor'); + const paths = await import('@archon/paths'); + // resume validates read-only via resumeWorkflowOp, which reads getWorkflowRun; + // a failed run is resumable. + (workflowDb.getWorkflowRun as ReturnType).mockResolvedValueOnce({ + ...pausedRun, + status: 'failed', + }); + (paths.getArchonHome as ReturnType).mockImplementationOnce(() => { + throw new Error('no home in test'); + }); + + const execBefore = (executeWorkflow as ReturnType).mock.calls.length; + const spawnSpy = mockSpawn(); + const savedArgv = process.argv; + process.argv = ['bun', '/abs/cli.ts', 'workflow', 'resume', 'run-123', '--detach']; + + let spawnCallCount = 0; + let spawnCmd: string[] = []; + try { + // Signature: (runId, json, cwd, detach) — detach is the 4th arg. + await workflowResumeCommand('run-123', undefined, undefined, true); + spawnCallCount = spawnSpy.mock.calls.length; + spawnCmd = ( + (spawnSpy.mock.calls[0]?.[0] as { cmd: string[] } | undefined)?.cmd ?? [] + ).slice(); + } finally { + process.argv = savedArgv; + spawnSpy.mockRestore(); + } + + expect(spawnCallCount).toBe(1); + expect(spawnCmd).not.toContain('--detach'); + expect(spawnCmd).toContain('resume'); + expect(spawnCmd).toContain('run-123'); + expect((executeWorkflow as ReturnType).mock.calls.length).toBe(execBefore); + }); +}); + describe('buildDetachedRunCmd', () => { // BUNDLED_IS_BINARY is a module-level const (mocked false), so the binary // branch is unreachable through spawnDetachedWorkflowRun — exercise both diff --git a/packages/cli/src/commands/workflow.ts b/packages/cli/src/commands/workflow.ts index 13064d41c8..a219ba85c1 100644 --- a/packages/cli/src/commands/workflow.ts +++ b/packages/cli/src/commands/workflow.ts @@ -62,6 +62,8 @@ import { abandonWorkflow, getWorkflowStatus, resetWorkflowNodeSessions, + assertApprovable, + assertRejectable, } from '@archon/core/operations/workflow-operations'; import * as conversationDb from '@archon/core/db/conversations'; import * as codebaseDb from '@archon/core/db/codebases'; @@ -2245,6 +2247,74 @@ async function resolveRunIdArg(runId: string, cwd?: string): Promise { return matches[0]?.id ?? runId; } +/** + * Shared `--detach` front half for `approve`/`reject`/`resume`. Validates the run + * READ-ONLY via `precheck`, then hands the whole command to a detached child that + * re-invokes the same argv (minus `--detach`/`--json`) and owns ALL state mutation + * in its own process group — so killing the shell that hosted the parent cannot + * zombie the run mid-resume. The parent must never call + * approveWorkflow/rejectWorkflow/resumeWorkflow itself, or the decision would be + * recorded twice (mirrors workflowRunCommand's detach shape: the parent does nothing). + * + * Spawns with the command's `cwd` (falling back to `process.cwd()`), never the run's + * working_path: the child re-resolves everything by run-id, and a container run's + * working_path is a distro path the host cannot spawn into (ENOENT → the detach would + * silently no-op). Reuses upstream's spawnDetachedWorkflowRun, which rebuilds the child + * command from process.argv. + */ +async function runDetachedControlCommand( + runId: string, + action: 'approve' | 'reject' | 'resume', + json: boolean | undefined, + cwd: string | undefined, + precheck: () => Promise +): Promise { + try { + const run = await precheck(); + // The caller's --cwd, already resolved by cli.ts — NOT process.cwd(). The + // appended --cwd is last-wins on the child's argv, so discarding it here + // strands the child in the parent's directory (possibly outside any git + // repo) after the parent has already acked success. The run's working_path + // is still never a candidate: a container run's working_path is a distro + // path the host cannot spawn into, so the child re-resolves by run id. + const logPath = spawnDetachedWorkflowRun(cwd ?? process.cwd(), runId, []); + if (json) { + console.log( + JSON.stringify( + { + ok: true, + runId, + action, + detached: true, + // The child is spawned WITHOUT --json (buildDetachedRunCmd strips it), + // so it takes the inline path and DRIVES THE RUN ONWARD — approve's + // auto-resume, reject's on_reject rework, resume's re-run. This is the + // opposite of bare `--json`, which withholds continuation on purpose. + // Surfaced so an automation knows it does not own continuation here. + continues: true, + workflowName: run.workflow_name, + logPath, + }, + null, + 2 + ) + ); + return; + } + console.log(`Started '${action}' for run ${runId} in the background.`); + console.log(`Track it with: archon workflow get ${runId}`); + if (logPath) console.log(`Child output: ${logPath}`); + } catch (error) { + // Precheck failures follow each mode's error contract: --json emits the + // standard { ok: false } line; human mode throws like the inline path. + if (json) { + printJsonWriteError(runId, action, error); + return; + } + throw error; + } +} + async function resolveDiscoveryCwdForCodebase( runId: string, codebaseId: string, @@ -2289,8 +2359,21 @@ async function resolveDiscoveryCwdForCodebase( export async function workflowResumeCommand( runId: string, json?: boolean, - cwd?: string + cwd?: string, + detach?: boolean ): Promise { + // --detach: validate read-only (resumeWorkflowOp checks the run is resumable), + // then let a detached child re-invoke the blocking resume and own all mutation + // + execution, so a reaped launching shell can't wedge the run mid-resume. + // Composes with --json (structured ack; nothing executes here). + if (detach) { + const resolvedId = await resolveRunIdArg(runId, cwd); + await runDetachedControlCommand(resolvedId, 'resume', json, cwd, () => + resumeWorkflowOp(resolvedId) + ); + return; + } + // JSON mode is a non-blocking control-plane ack: validate the run is resumable // and report its state, but do NOT re-execute the workflow inline (execution // streams workflow output to stdout, which would corrupt the JSON contract). @@ -2436,8 +2519,28 @@ export async function workflowApproveCommand( runId: string, comment?: string, json?: boolean, - cwd?: string + cwd?: string, + detach?: boolean ): Promise { + // --detach: hand the approve AND its inline auto-resume to a detached child + // (same argv minus --detach/--json). Handled BEFORE any state change — the + // parent only validates read-only, so the approval is recorded exactly once, + // in the child. Composes with --json (structured ack; nothing executes here). + if (detach) { + const resolvedId = await resolveRunIdArg(runId, cwd); + await runDetachedControlCommand(resolvedId, 'approve', json, cwd, async () => { + const run = await workflowDb.getWorkflowRun(resolvedId); + if (!run) { + throw new Error(`Workflow run not found: ${resolvedId}`); + } + // The SAME gate approveWorkflow enforces — not a copy of one branch of it. + // A partial copy acks { ok: true } and lets the child die unseen. + assertApprovable(run); + return run; + }); + return; + } + // JSON mode records the approval and returns a structured ack WITHOUT the // inline auto-resume (resuming executes the workflow and streams output to // stdout, which would corrupt the JSON contract). The run becomes resumable @@ -2538,8 +2641,28 @@ export async function workflowRejectCommand( runId: string, reason?: string, json?: boolean, - cwd?: string + cwd?: string, + detach?: boolean ): Promise { + // --detach: hand the reject AND its inline on_reject rework to a detached child, + // exactly as approve does. Without it, reject hosts the executor in the calling + // shell — a reaped shell (harness task, closed terminal) leaves the run wedged + // mid-rework. The parent only validates read-only. Composes with --json. + if (detach) { + const resolvedId = await resolveRunIdArg(runId, cwd); + await runDetachedControlCommand(resolvedId, 'reject', json, cwd, async () => { + const run = await workflowDb.getWorkflowRun(resolvedId); + if (!run) { + throw new Error(`Workflow run not found: ${resolvedId}`); + } + // The SAME gate rejectWorkflow enforces — not a copy of one branch of it. + // A partial copy acks { ok: true } and lets the child die unseen. + assertRejectable(run); + return run; + }); + return; + } + // JSON mode records the rejection and returns a structured ack WITHOUT the // inline auto-resume (an on_reject rework executes the workflow and streams // to stdout, corrupting the JSON contract). When `cancelled` is false the run diff --git a/packages/core/src/operations/workflow-operations.test.ts b/packages/core/src/operations/workflow-operations.test.ts index e8b3b97661..066e0f2e1c 100644 --- a/packages/core/src/operations/workflow-operations.test.ts +++ b/packages/core/src/operations/workflow-operations.test.ts @@ -1,4 +1,5 @@ import { describe, test, expect, mock, beforeEach } from 'bun:test'; +import type { WorkflowRun } from '@archon/workflows/schemas/workflow-run'; // --------------------------------------------------------------------------- // Mock DB modules before importing the module under test @@ -67,6 +68,8 @@ const { resumeWorkflow, abandonWorkflow, resetWorkflowNodeSessions, + assertApprovable, + assertRejectable, } = await import('./workflow-operations'); // --------------------------------------------------------------------------- @@ -655,6 +658,83 @@ describe('rejectWorkflow', () => { }); }); +describe('assertApprovable / assertRejectable — shared precondition gate', () => { + const baseRun = { + id: 'run-1', + status: 'paused', + workflow_name: 'assist', + working_path: '/tmp/x', + conversation_id: 'conv-1', + user_message: 'hi', + metadata: { approval: { nodeId: 'gate', message: 'Approve?' } }, + } as unknown as WorkflowRun; + + const withMeta = (metadata: Record, status = 'paused') => + ({ ...baseRun, status, metadata }) as unknown as WorkflowRun; + + test('assertApprovable returns the approval context on a well-formed paused run', () => { + expect(assertApprovable(baseRun).nodeId).toBe('gate'); + }); + + test('assertApprovable rejects a non-paused run', () => { + expect(() => assertApprovable(withMeta(baseRun.metadata, 'running'))).toThrow( + "Cannot approve run with status 'running'" + ); + }); + + test('assertApprovable rejects a missing approval context', () => { + expect(() => assertApprovable(withMeta({}))).toThrow( + 'Workflow run is paused but missing approval context.' + ); + }); + + test('assertApprovable redirects a child_workflow-blocked parent to the child run', () => { + expect(() => + assertApprovable( + withMeta({ + approval: { + nodeId: 'sub', + message: 'blocked', + type: 'child_workflow', + childRunId: 'child-9', + }, + }) + ) + ).toThrow('Approve or reject the child run instead: /workflow approve child-9'); + }); + + test('assertApprovable rejects an already-resolved gate', () => { + expect(() => + assertApprovable(withMeta({ approval: { nodeId: 'g', message: 'm', resolved: 'approved' } })) + ).toThrow('was already approved and is awaiting resume'); + }); + + test('assertRejectable TOLERATES a missing approval context (unlike approve)', () => { + expect(assertRejectable(withMeta({}))).toBeUndefined(); + }); + + test('assertRejectable redirects a child_workflow-blocked parent', () => { + expect(() => + assertRejectable( + withMeta({ + approval: { + nodeId: 'sub', + message: 'blocked', + type: 'child_workflow', + childRunId: 'child-9', + }, + }) + ) + ).toThrow('Reject the child run instead: /workflow reject child-9'); + }); + + test('assertRejectable rejects an already-resolved gate', () => { + expect(() => + assertRejectable(withMeta({ approval: { nodeId: 'g', message: 'm', resolved: 'rejected' } })) + ).toThrow('was already rejected and is awaiting resume'); + }); +}); + describe('getWorkflowStatus', () => { beforeEach(() => { mockListWorkflowRuns.mockClear(); diff --git a/packages/core/src/operations/workflow-operations.ts b/packages/core/src/operations/workflow-operations.ts index 3a8970d638..11159662e5 100644 --- a/packages/core/src/operations/workflow-operations.ts +++ b/packages/core/src/operations/workflow-operations.ts @@ -167,6 +167,96 @@ async function getRunOrThrow(runId: string, logEvent: string): Promise'} ` + + `('workflow:' node '${approval.nodeId}'). Approve or reject the child run instead` + + (approval.childRunId ? `: /workflow approve ${approval.childRunId}` : '.') + ); + } + if (isGateResolved(approval)) { + // Fast-path friendly error for the common (sequential) case. The run stays + // 'paused' after a resolution, so the status check alone no longer blocks a + // second approve. This in-memory read can still race a concurrent approve — + // the resolveApprovalGate CAS is the real arbiter; a second approve that + // slips past this read loses the atomic UPDATE and throws the same way. + throw new Error( + `Workflow run ${run.id} was already ${String(approval.resolved)} and is awaiting resume.` + ); + } + return approval; +} + +/** + * The THREE preconditions `rejectWorkflow` enforces. Deliberately NOT the same + * gate as `assertApprovable`: reject has no `nodeId` requirement — it falls back + * to `approval?.nodeId ?? 'unknown'` when writing its audit event, so a run whose + * approval metadata is malformed is still legitimately rejectable. Merging the two + * would either break reject or over-permit approve. + */ +export function assertRejectable(run: WorkflowRun): ApprovalContext | undefined { + if (run.status !== 'paused') { + throw new Error( + `Cannot reject run with status '${run.status}'. Only paused runs can be rejected.` + ); + } + const rawApproval = run.metadata.approval; + const approval: ApprovalContext | undefined = isApprovalContext(rawApproval) + ? rawApproval + : undefined; + if (approval?.type === 'child_workflow') { + // Same redirect as assertApprovable: the parent's pause is not a rejectable + // gate — cancelling the parent here would silently orphan the still-paused + // child run. Reject the child (its own gate) or abandon the parent (which + // cascade-cancels the subtree) instead. + throw new Error( + `Run ${run.id} is paused waiting on sub-run ${approval.childRunId ?? ''} ` + + `('workflow:' node '${approval.nodeId}'). Reject the child run instead` + + (approval.childRunId ? `: /workflow reject ${approval.childRunId}` : '.') + + ' To discard the whole tree, abandon this run.' + ); + } + if (approval && isGateResolved(approval)) { + throw new Error( + `Workflow run ${run.id} was already ${String(approval.resolved)} and is awaiting resume.` + ); + } + return approval; +} + // --------------------------------------------------------------------------- // Operations // --------------------------------------------------------------------------- @@ -291,41 +381,7 @@ export async function approveWorkflow( comment?: string ): Promise { const run = await getRunOrThrow(runId, 'operations.workflow_approve_lookup_failed'); - if (run.status !== 'paused') { - throw new Error( - `Cannot approve run with status '${run.status}'. Only paused runs can be approved.` - ); - } - const rawApproval = run.metadata.approval; - const approval: ApprovalContext | undefined = isApprovalContext(rawApproval) - ? rawApproval - : undefined; - if (!approval?.nodeId) { - throw new Error('Workflow run is paused but missing approval context.'); - } - if (approval.type === 'child_workflow') { - // A parent blocked on a `workflow:` sub-run has no approvable gate of its - // own — the pause resolves automatically when the child run completes. - // Falling through to the generic branch would stamp a node_completed for the - // parent's workflow node with empty output (the child's real output is then - // discarded on resume) and orphan the still-paused child. Redirect the - // operator to the child run, where the actual gate lives. - throw new Error( - `Run ${runId} is paused waiting on sub-run ${approval.childRunId ?? ''} ` + - `('workflow:' node '${approval.nodeId}'). Approve or reject the child run instead` + - (approval.childRunId ? `: /workflow approve ${approval.childRunId}` : '.') - ); - } - if (isGateResolved(approval)) { - // Fast-path friendly error for the common (sequential) case. The run stays - // 'paused' after a resolution, so the status check alone no longer blocks a - // second approve. This in-memory read can still race a concurrent approve — - // the resolveApprovalGate CAS below is the real arbiter; a second approve - // that slips past this read loses the atomic UPDATE and throws the same way. - throw new Error( - `Workflow run ${runId} was already ${String(approval.resolved)} and is awaiting resume.` - ); - } + const approval = assertApprovable(run); // Whitespace-only comments count as absent (mirrors feedbackProvided below): // HTTP/CLI/chat pass the raw comment through since #2074, so ' ' would @@ -447,35 +503,7 @@ export async function rejectWorkflow( reason?: string ): Promise { const run = await getRunOrThrow(runId, 'operations.workflow_reject_lookup_failed'); - if (run.status !== 'paused') { - throw new Error( - `Cannot reject run with status '${run.status}'. Only paused runs can be rejected.` - ); - } - const rawApproval = run.metadata.approval; - const approval: ApprovalContext | undefined = isApprovalContext(rawApproval) - ? rawApproval - : undefined; - if (approval?.type === 'child_workflow') { - // Same redirect as approveWorkflow: the parent's pause is not a rejectable - // gate — cancelling the parent here would silently orphan the still-paused - // child run. Reject the child (its own gate) or abandon the parent (which - // cascade-cancels the subtree) instead. - throw new Error( - `Run ${runId} is paused waiting on sub-run ${approval.childRunId ?? ''} ` + - `('workflow:' node '${approval.nodeId}'). Reject the child run instead` + - (approval.childRunId ? `: /workflow reject ${approval.childRunId}` : '.') + - ' To discard the whole tree, abandon this run.' - ); - } - if (approval && isGateResolved(approval)) { - // Fast-path friendly error, same as approveWorkflow — the run stays 'paused' - // after a resolution, so status alone no longer blocks a second reject. The - // CAS below is the real arbiter for the concurrent case. - throw new Error( - `Workflow run ${runId} was already ${String(approval.resolved)} and is awaiting resume.` - ); - } + const approval = assertRejectable(run); const isWriteBack = approval?.type === 'writeback'; // Engine-level container write-back gate (Phase C): reject means DISCARD the diff --git a/packages/docs-web/src/content/docs/reference/cli.md b/packages/docs-web/src/content/docs/reference/cli.md index 36ff85ba93..d7a7a6b1b8 100644 --- a/packages/docs-web/src/content/docs/reference/cli.md +++ b/packages/docs-web/src/content/docs/reference/cli.md @@ -213,7 +213,7 @@ Progress events (node start/complete/fail/skip, approval gates) are written to s | `--resume` | Resume from last failed run at the working path (skips completed nodes) | | `--quiet`, `-q` | Suppress all progress output to stderr | | `--verbose`, `-v` | Also show tool-level events (tool name and duration) | -| `--detach` | Run in a detached background child and return immediately. The child does all the work; find it later with `workflow runs`/`workflow get`. Child stdout/stderr is captured to `~/.archon/logs/detached-run-.log`. Combine with `--json` for a machine-readable ack. | +| `--detach` | Run in a detached background child and return immediately. The child does all the work; find it later with `workflow runs`/`workflow get`. Child stdout/stderr is captured to `~/.archon/logs/detached-run-.log`. Combine with `--json` for a machine-readable ack. Also available on `approve`/`reject`/`resume` — see [Detached control verbs](#detached-control-verbs). | **Default (no flags):** - Creates worktree with auto-generated branch (`archon/task--`) @@ -289,6 +289,8 @@ archon workflow resume --json # validate + ack only; does NOT re-exec In `--json` mode the command is a non-blocking control-plane ack: it validates the run is resumable and reports its state but does **not** re-execute inline (execution streams output to stdout, which would corrupt the JSON). To actually drive a resumable run to completion, use the blocking form or `workflow run --resume --detach`. +Adding `--detach` **inverts** that: the child is re-invoked without `--json`, so it takes the inline path and does re-execute the run — just outside your shell. The ack carries `continues: true` to say so. See [Detached control verbs](#detached-control-verbs). + ### `workflow abandon` Discard a workflow run (marks it as `cancelled`). Use this to unblock a worktree when you don't want to resume — the path lock is released immediately so a new workflow can start. @@ -317,6 +319,42 @@ archon workflow approve --json # record approval + ack; does NOT auto In human mode `approve`/`reject` auto-resume the run inline. In `--json` mode they record the decision and return an ack **without** resuming (the run is left resumable for a backgrounded `resume`/`run --resume`). +#### Detached control verbs + +`approve`, `reject`, and `resume` accept `--detach`. The parent validates the run +**read-only** — the same four/three preconditions the operation itself enforces, so a +wrong-status, missing-context, `child_workflow`-blocked, or already-resolved run is +refused synchronously and nothing is spawned — then hands the whole command to a +detached child that owns all state mutation in its own process group. A shell that +dies mid-flight can no longer wedge the run. + +```bash +archon workflow approve --detach +archon workflow approve --detach --json +``` + +**`--detach --json` deliberately differs from bare `--json`.** Bare `--json` records the +decision and withholds the inline auto-resume (you drive continuation separately). +`--detach --json` spawns a child that takes the ordinary inline path, so the run **is** +driven onward — approve's auto-resume, reject's `on_reject` rework, resume's re-run — +just outside your shell. The ack carries `continues: true` to say so: + +```json +{ + "ok": true, + "runId": "…", + "action": "approve", + "detached": true, + "continues": true, + "workflowName": "assist", + "logPath": "~/.archon/logs/detached-run-.log" +} +``` + +Read `continues` to decide whether your automation still owns continuation. Precheck +failures follow each verb's existing error contract: `{ ok: false }` under `--json`, +a thrown error otherwise. + ### `workflow reject` Reject a paused workflow run at an approval gate. Optionally provide a reason that is available to the workflow via `$REJECTION_REASON`.