Research/spok run cli - #16
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11d3f6f304
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| process.kill(-pid, signal); // Negative pid: the child's whole process group. | ||
| } catch { |
There was a problem hiding this comment.
On Windows, negative PIDs do not address process groups, so process.kill(-pid, signal) throws and this catch silently ignores it. The signal handler then immediately exits the parent while the detached Claude/Codex process remains alive, allowing an apparently cancelled spok run to continue editing or committing files in the background. Use platform-specific process-tree termination or avoid detaching there before reporting exit 130/143.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
9 issues found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/commands/workflow/harness-runners.ts">
<violation number="1" location="src/commands/workflow/harness-runners.ts:80">
P1: On Windows, `process.kill(-pid, signal)` cannot signal a POSIX process group, and the catch hides that failure. Use a Windows process-tree termination path such as `taskkill /T` so Ctrl-C cannot leave the harness editing after `spok` exits.</violation>
<violation number="2" location="src/commands/workflow/harness-runners.ts:101">
P2: `spawnHarness` has no timeout and buffers claude's stdout into an unbounded string. A hung harness never settles the run promise, so `spok run` blocks forever with only manual Ctrl+C as an escape, and a chatty harness grows memory without bound. Add a run timeout (and, for the captured-stdout path, an output cap) that resolves the promise with a failure reason.</violation>
<violation number="3" location="src/commands/workflow/harness-runners.ts:165">
P3: When a Codex run is interrupted, `run.ts` calls `process.exit` before this `finally` block can run, leaving a temporary directory behind. Clean up through an awaited shutdown path before exiting.</violation>
</file>
<file name="src/cli/index.ts">
<violation number="1" location="src/cli/index.ts:602">
P2: When `flow.self_learn` is invalid, `spok run` silently disables the setting because `CONFIG_WARNING_COMMANDS` omits the new command. Add `run` to that set so the human-facing driver reports the same configuration diagnostics as the other flow commands.</violation>
<violation number="2" location="src/cli/index.ts:609">
P2: When callers pass an explicitly empty `--profile`, this guard skips validation and the run silently uses another profile. Check for `undefined` instead of truthiness so every supplied value is validated.</violation>
</file>
<file name="src/commands/workflow/flow.ts">
<violation number="1" location="src/commands/workflow/flow.ts:908">
P2: When two flow commands operate on the same task concurrently, the shared `.tmp` path can make one state write fail or overwrite the other. Use a unique temporary path per write and clean it up after rename.</violation>
</file>
<file name="src/commands/workflow/run.ts">
<violation number="1" location="src/commands/workflow/run.ts:68">
P2: The `Work root:`/`Commit:` line regexes only allow leading `*`/`_`, but `stripTokenDecoration` already anticipates backtick wrapping. A harness that writes the whole line in backticks (`` `Commit: abc123` ``) fails the commit step with a hard exit 3, or silently drops the work root for summary steps. Allow optional backtick/quote wrapping around the label in the line regexes so the decoration helper is actually reachable.</violation>
<violation number="2" location="src/commands/workflow/run.ts:294">
P1: When implementation records a work root different from the task repository, this request still launches every later harness from the task root. Pass the recorded work root to the runner for subsequent editing and commit steps, while preserving access to the task artifacts as needed.</violation>
</file>
<file name="test/features/step-definitions/spok-run.steps.ts">
<violation number="1" location="test/features/step-definitions/spok-run.steps.ts:122">
P2: The Cucumber step timeout is 90s, but invokeRun kills the CLI at 60s via runCLI's timeoutMs. If a full flow run takes between 60 and 90 seconds, runCLI SIGKILLs the child before the step times out, producing a confusing signal/null exit code instead of reaching the Cucumber timeout. Make the runCLI timeout at least the step timeout, or drop the fixed 60s and let the harness step timeout govern.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| const pid = activeChild?.pid; | ||
| if (!pid) return; | ||
| try { | ||
| process.kill(-pid, signal); // Negative pid: the child's whole process group. |
There was a problem hiding this comment.
P1: On Windows, process.kill(-pid, signal) cannot signal a POSIX process group, and the catch hides that failure. Use a Windows process-tree termination path such as taskkill /T so Ctrl-C cannot leave the harness editing after spok exits.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/workflow/harness-runners.ts, line 80:
<comment>On Windows, `process.kill(-pid, signal)` cannot signal a POSIX process group, and the catch hides that failure. Use a Windows process-tree termination path such as `taskkill /T` so Ctrl-C cannot leave the harness editing after `spok` exits.</comment>
<file context>
@@ -0,0 +1,193 @@
+ const pid = activeChild?.pid;
+ if (!pid) return;
+ try {
+ process.kill(-pid, signal); // Negative pid: the child's whole process group.
+ } catch {
+ // Child already exited between the check and the kill.
</file context>
| model: step.model, | ||
| effort: step.effort, | ||
| prompt: step.prompt, | ||
| projectRoot, |
There was a problem hiding this comment.
P1: When implementation records a work root different from the task repository, this request still launches every later harness from the task root. Pass the recorded work root to the runner for subsequent editing and commit steps, while preserving access to the task artifacts as needed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/workflow/run.ts, line 294:
<comment>When implementation records a work root different from the task repository, this request still launches every later harness from the task root. Pass the recorded work root to the runner for subsequent editing and commit steps, while preserving access to the task artifacts as needed.</comment>
<file context>
@@ -0,0 +1,317 @@
+ model: step.model,
+ effort: step.effort,
+ prompt: step.prompt,
+ projectRoot,
+ });
+ if (!result.ok) return reportStepFailed(json, step.id, result.reason);
</file context>
| }); | ||
|
|
||
| program | ||
| .command('run <task-dir>') |
There was a problem hiding this comment.
P2: When flow.self_learn is invalid, spok run silently disables the setting because CONFIG_WARNING_COMMANDS omits the new command. Add run to that set so the human-facing driver reports the same configuration diagnostics as the other flow commands.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/index.ts, line 602:
<comment>When `flow.self_learn` is invalid, `spok run` silently disables the setting because `CONFIG_WARNING_COMMANDS` omits the new command. Add `run` to that set so the human-facing driver reports the same configuration diagnostics as the other flow commands.</comment>
<file context>
@@ -592,6 +598,31 @@ flowCmd
});
+program
+ .command('run <task-dir>')
+ .description('Drive the deterministic flow to completion by dispatching each step to its harness')
+ .option('--profile <profile>', 'Flow profile: claude, codex, or hybrid')
</file context>
| .action(async (taskDir: string, options: RunCommandOptions) => { | ||
| // An unrecognized profile is a usage error, caught before the state machine | ||
| // could report it as a blocker and exit 2. | ||
| if (options.profile && !isFlowProfile(options.profile)) { |
There was a problem hiding this comment.
P2: When callers pass an explicitly empty --profile, this guard skips validation and the run silently uses another profile. Check for undefined instead of truthiness so every supplied value is validated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/index.ts, line 609:
<comment>When callers pass an explicitly empty `--profile`, this guard skips validation and the run silently uses another profile. Check for `undefined` instead of truthiness so every supplied value is validated.</comment>
<file context>
@@ -592,6 +598,31 @@ flowCmd
+ .action(async (taskDir: string, options: RunCommandOptions) => {
+ // An unrecognized profile is a usage error, caught before the state machine
+ // could report it as a blocker and exit 2.
+ if (options.profile && !isFlowProfile(options.profile)) {
+ if (!options.json) console.log();
+ ora().fail(
</file context>
| if (options.profile && !isFlowProfile(options.profile)) { | |
| if (options.profile !== undefined && !isFlowProfile(options.profile)) { |
| state.updatedAt = nowIso(); | ||
| await fs.writeFile(getStatePath(state.taskDir), `${JSON.stringify(state, null, 2)}\n`, 'utf-8'); | ||
| const statePath = getStatePath(state.taskDir); | ||
| const tempPath = `${statePath}.tmp`; |
There was a problem hiding this comment.
P2: When two flow commands operate on the same task concurrently, the shared .tmp path can make one state write fail or overwrite the other. Use a unique temporary path per write and clean it up after rename.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/workflow/flow.ts, line 908:
<comment>When two flow commands operate on the same task concurrently, the shared `.tmp` path can make one state write fail or overwrite the other. Use a unique temporary path per write and clean it up after rename.</comment>
<file context>
@@ -886,9 +901,13 @@ async function recordFlowResponse(
state.updatedAt = nowIso();
- await fs.writeFile(getStatePath(state.taskDir), `${JSON.stringify(state, null, 2)}\n`, 'utf-8');
+ const statePath = getStatePath(state.taskDir);
+ const tempPath = `${statePath}.tmp`;
+ await fs.writeFile(tempPath, `${JSON.stringify(state, null, 2)}\n`, 'utf-8');
+ await fs.rename(tempPath, statePath);
</file context>
| const tempPath = `${statePath}.tmp`; | |
| const tempPath = `${statePath}.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2)}.tmp`; |
| world.cliResult = await runCLI(['run', world.flowTaskDir, ...args], { | ||
| cwd: world.projectDir, | ||
| env: stubEnv(world), | ||
| timeoutMs: 60_000, |
There was a problem hiding this comment.
P2: The Cucumber step timeout is 90s, but invokeRun kills the CLI at 60s via runCLI's timeoutMs. If a full flow run takes between 60 and 90 seconds, runCLI SIGKILLs the child before the step times out, producing a confusing signal/null exit code instead of reaching the Cucumber timeout. Make the runCLI timeout at least the step timeout, or drop the fixed 60s and let the harness step timeout govern.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/features/step-definitions/spok-run.steps.ts, line 122:
<comment>The Cucumber step timeout is 90s, but invokeRun kills the CLI at 60s via runCLI's timeoutMs. If a full flow run takes between 60 and 90 seconds, runCLI SIGKILLs the child before the step times out, producing a confusing signal/null exit code instead of reaching the Cucumber timeout. Make the runCLI timeout at least the step timeout, or drop the fixed 60s and let the harness step timeout govern.</comment>
<file context>
@@ -0,0 +1,282 @@
+ world.cliResult = await runCLI(['run', world.flowTaskDir, ...args], {
+ cwd: world.projectDir,
+ env: stubEnv(world),
+ timeoutMs: 60_000,
+ });
+}
</file context>
| timeoutMs: 60_000, | |
| timeoutMs: 90_000, |
| if (!trimmed) return undefined; | ||
|
|
||
| const { body, lastLine } = splitTrailingLine(trimmed); | ||
| const workRootMatch = lastLine.match(/^[*_]*Work root[*_]*:[*_]*\s*(.+)$/i); |
There was a problem hiding this comment.
P2: The Work root:/Commit: line regexes only allow leading */_, but stripTokenDecoration already anticipates backtick wrapping. A harness that writes the whole line in backticks (`Commit: abc123`) fails the commit step with a hard exit 3, or silently drops the work root for summary steps. Allow optional backtick/quote wrapping around the label in the line regexes so the decoration helper is actually reachable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/workflow/run.ts, line 68:
<comment>The `Work root:`/`Commit:` line regexes only allow leading `*`/`_`, but `stripTokenDecoration` already anticipates backtick wrapping. A harness that writes the whole line in backticks (`` `Commit: abc123` ``) fails the commit step with a hard exit 3, or silently drops the work root for summary steps. Allow optional backtick/quote wrapping around the label in the line regexes so the decoration helper is actually reachable.</comment>
<file context>
@@ -0,0 +1,317 @@
+ if (!trimmed) return undefined;
+
+ const { body, lastLine } = splitTrailingLine(trimmed);
+ const workRootMatch = lastLine.match(/^[*_]*Work root[*_]*:[*_]*\s*(.+)$/i);
+ if (!workRootMatch) return { summary: capSummary(trimmed) };
+
</file context>
| cwd: string, | ||
| forwardStdoutToStderr: boolean | ||
| ): Promise<SpawnOutcome> { | ||
| return new Promise((resolve) => { |
There was a problem hiding this comment.
P2: spawnHarness has no timeout and buffers claude's stdout into an unbounded string. A hung harness never settles the run promise, so spok run blocks forever with only manual Ctrl+C as an escape, and a chatty harness grows memory without bound. Add a run timeout (and, for the captured-stdout path, an output cap) that resolves the promise with a failure reason.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/workflow/harness-runners.ts, line 101:
<comment>`spawnHarness` has no timeout and buffers claude's stdout into an unbounded string. A hung harness never settles the run promise, so `spok run` blocks forever with only manual Ctrl+C as an escape, and a chatty harness grows memory without bound. Add a run timeout (and, for the captured-stdout path, an output cap) that resolves the promise with a failure reason.</comment>
<file context>
@@ -0,0 +1,193 @@
+ cwd: string,
+ forwardStdoutToStderr: boolean
+): Promise<SpawnOutcome> {
+ return new Promise((resolve) => {
+ // The run's profile override must not leak into the harness: a nested spok
+ // invocation from a subagent would inherit a profile it was never given.
</file context>
|
|
||
| export const codexRunner: HarnessRunner = { | ||
| async run(request) { | ||
| const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'spok-run-codex-')); |
There was a problem hiding this comment.
P3: When a Codex run is interrupted, run.ts calls process.exit before this finally block can run, leaving a temporary directory behind. Clean up through an awaited shutdown path before exiting.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/workflow/harness-runners.ts, line 165:
<comment>When a Codex run is interrupted, `run.ts` calls `process.exit` before this `finally` block can run, leaving a temporary directory behind. Clean up through an awaited shutdown path before exiting.</comment>
<file context>
@@ -0,0 +1,193 @@
+
+export const codexRunner: HarnessRunner = {
+ async run(request) {
+ const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'spok-run-codex-'));
+ const outputFile = path.join(tempDir, 'last-message.txt');
+ try {
</file context>
1 participant
spok run makes Spok directly executable by an external orchestrator: one foreground process drives the existing deterministic flow state machine from the next ready step to a terminal outcome, dispatching each step to its routed harness as a fresh claude -p or codex exec subprocess and recording completion from artifacts and a machine-readable output contract rather than from interpretation. The state machine stays authoritative over step order, routing, prompts, and gates; the new
run.ts
loop and
harness-runners.ts
adapters only execute what it decides. Callers select policy with --profile claude|codex|hybrid and supervise with --json, which emits one JSONL event per line (run_started, step_started, step_completed, warning, blocked with a stable code and any verbatim ## Human Decisions Required content, step_failed, complete) on stdout while harness output stays on stderr. Exit codes are stable: 0 complete, 2 blocked on a human gate, 3 resumable execution or contract failure, 130/143 on signals, 1 for usage errors. Resume is re-running the same command — everything re-derives from
workflow-state.json
and on-disk artifacts, with no agent conversation state to reconstruct. The vendored spok-flow skill drops from a 171-line dispatch loop to a 46-line wrapper that shells out to spok run and relays the outcome, so execution semantics now exist in exactly one place and the CLI and the skill cannot drift.
Summary by cubic
Adds
spok runto drive a staged flow end-to-end by dispatching each routed step to its harness (claudeorcodex), standardizing JSONL supervision events and exit codes. Thespok-flowskill now delegates the whole loop to this command; agents no longer callflow next/status/completedirectly.spok run <task-dir> [--profile claude|codex|hybrid] [--json]. Emits JSONL events (schemaVersion: 1) and exits 0 (complete), 2 (blocked), 3 (step failure), 1/130/143 for usage/interruption.SPOK_FLOW_PROFILEinto child processes;codexprogress streams to stderr, final message read from file.Work root: <abs path>); commit steps require trailingCommit: <sha>; file steps must write the expected artifact. Contract breaks exit 3 and resume the same step on re-run.humanDecisionsfromdesign-review.mdwhen design review blocks. Warnings (memory/work-root) surface once and do not stop the run.workflow-state.json.spok run(optionally--json) and handle its exit codes; use--profile hybridinstead ofSPOK_FLOW_PROFILE=hybrid; remove direct calls tospok flow next/complete; require subagents to end replies withWork root:/Commit:lines where applicable.Written for commit 11d3f6f. Summary will update on new commits.