Skip to content

feat: clean add-steps API + serve readiness signal #54

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
naorsabag merged 1 commit into from
Apr 27, 2026
Merged

feat: clean add-steps API + serve readiness signal #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion packages/cli/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,9 @@ program
.description('Start the OpenHop API server (:8787) and web UI (:8788)')
.option('-p, --port <port>', 'API port', '8787')
.option('--no-web', 'Start API only, skip the web UI dev server')
.action((opts) => {
.option('--no-wait-ready', "Don't probe /health and don't print the ready line on stdout")
.option('--ready-timeout <seconds>', 'How long to wait for readiness before giving up', '60')
.action(async (opts) => {
Comment on lines +74 to +75
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Check the file structure and exact lines
head -150 packages/cli/src/index.ts | tail -80

Repository: naorsabag/OpenHop

Length of output: 3322

🏁 Script executed:

# Look at the actual option definition and handler
sed -n '60,150p' packages/cli/src/index.ts

Repository: naorsabag/OpenHop

Length of output: 3890

🏁 Script executed:

# Check if Commander's InvalidArgumentError is available
rg "InvalidArgumentError" packages/cli/src/

Repository: naorsabag/OpenHop

Length of output: 43

🏁 Script executed:

# Check Commander imports and version
rg "from 'commander'" packages/cli/src/index.ts -A 2

Repository: naorsabag/OpenHop

Length of output: 176

🏁 Script executed:

# Check the package.json to see commander version
cat packages/cli/package.json | grep -A 2 -B 2 "commander"

Repository: naorsabag/OpenHop

Length of output: 543

🏁 Script executed:

# Verify Number.parseInt behavior with edge cases
node <<'EOF'
console.log("Testing Number.parseInt edge cases:")
console.log("parseInt('10foo', 10):", Number.parseInt('10foo', 10))
console.log("parseInt('0', 10):", Number.parseInt('0', 10))
console.log("parseInt('-5', 10):", Number.parseInt('-5', 10))
console.log("parseInt('abc', 10):", Number.parseInt('abc', 10))
console.log("")
console.log("Current code behavior (|| 60):")
console.log("'10foo': Number.parseInt('10foo', 10) || 60 =", Number.parseInt('10foo', 10) || 60)
console.log("'0': Number.parseInt('0', 10) || 60 =", Number.parseInt('0', 10) || 60)
console.log("'-5': Number.parseInt('-5', 10) || 60 =", Number.parseInt('-5', 10) || 60)
console.log("'abc': Number.parseInt('abc', 10) || 60 =", Number.parseInt('abc', 10) || 60)
EOF

Repository: naorsabag/OpenHop

Length of output: 402

🏁 Script executed:

# Check if Commander's InvalidArgumentError is available in the docs or source
rg "class InvalidArgumentError|export.*InvalidArgumentError" --type ts --type js

Repository: naorsabag/OpenHop

Length of output: 43

🏁 Script executed:

# Check Commander v12 documentation for option validation approach
# Look for how other options in the file handle validation
rg "\.option\(" packages/cli/src/index.ts -A 1 | head -20

Repository: naorsabag/OpenHop

Length of output: 1033

🏁 Script executed:

# Check if there are any existing custom validation functions in the CLI
rg "throw|Error|parse|validate" packages/cli/src/index.ts | head -20

Repository: naorsabag/OpenHop

Length of output: 637

🏁 Script executed:

# Verify Commander v12 API - check what validation methods are available
web_search Commander.js v12 option validation InvalidArgumentError

Repository: naorsabag/OpenHop

Length of output: 112

🌐 Web query:

Commander.js v12 option validation InvalidArgumentError API

💡 Result:

In Commander.js v12, option validation primarily occurs through built-in mechanisms and custom processing. For custom validation in option processors (argParser), throw a commander.InvalidArgumentError(message) from the processor function to trigger Commander's error handling with a formatted message like "error: option '--port ' argument 'abc' is invalid: Not a number." Example from official npm docs and README: function myParseInt(value, dummyPrevious) { const parsedValue = parseInt(value, 10); if (isNaN(parsedValue)) { throw new commander.InvalidArgumentError('Not a number.'); } return parsedValue; } program.option('-p, --port ', 'port number', myParseInt); Commander catches this during parsing and displays: "error: option '-p, --port ' argument 'abc' is invalid: Not a number." Other validation: - Use .choices(['a', 'b']) on Option for predefined values (throws InvalidArgumentError automatically). - Required options: .requiredOption or .makeOptionMandatory. - Unknown options: Error by default; suppress with .allowUnknownOption. - No major API changes for this in v12 changelog; Node 18+ required. InvalidArgumentError extends CommanderError and is used consistently for argument/option validation failures across versions (renamed from older specific errors pre-v8). Official sources: npm package page, GitHub README, jsDocs.io.

Citations:

Validate --ready-timeout instead of silently coercing it.

Number.parseInt(opts.readyTimeout, 10) || 60 accepts partial parses like 10foo as 10, converts 0 to 60, and permits negative values. Add a custom option parser using Commander's InvalidArgumentError to validate input upfront.

Suggested fix 
-import { Command } from 'commander'
+import { Command, InvalidArgumentError } from 'commander'
+
+function parseReadyTimeout(value: string): number {
+  const parsed = Number(value)
+  if (!Number.isFinite(parsed) || parsed <= 0) {
+    throw new InvalidArgumentError('ready-timeout must be a positive number of seconds')
+  }
+  return parsed
+}
...
-  .option('--ready-timeout <seconds>', 'How long to wait for readiness before giving up', '60')
+  .option('--ready-timeout <seconds>', 'How long to wait for readiness before giving up', parseReadyTimeout, 60)
...
-      const timeoutSec = Number.parseInt(opts.readyTimeout, 10) || 60
+      const timeoutSec = opts.readyTimeout
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
.option('--ready-timeout <seconds>', 'How long to wait for readiness before giving up', '60')
.action(async (opts) => {
import { Command, InvalidArgumentError } from 'commander'
function parseReadyTimeout(value: string): number {
const parsed = Number(value)
if (!Number.isFinite(parsed) || parsed <= 0) {
throw new InvalidArgumentError('ready-timeout must be a positive number of seconds')
}
return parsed
}
// ... other code ...
.option('--ready-timeout <seconds>', 'How long to wait for readiness before giving up', parseReadyTimeout, 60)
.action(async (opts) => {
// ... other code ...
const timeoutSec = opts.readyTimeout
// ... rest of action handler ...
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/cli/src/index.ts` around lines 74 - 75, Replace the current loose
ready-timeout handling with a commander custom option parser: import
InvalidArgumentError from 'commander', change the .option('--ready-timeout
<seconds>', ...) call to provide a parser function that 1) rejects non-integer
strings (use a regex like /^-?\d+$/ or Number.isInteger after coercion), 2)
parses with Number.parseInt(value, 10), and 3) throws new InvalidArgumentError
for values <= 0 (or negative) so only positive integers are accepted, and keep
the default 60; update any code that expects opts.readyTimeout to account for it
being a validated number.

const cliDir = resolve(import.meta.dirname, '..', '..')
const serverEntry = resolve(cliDir, 'server', 'src', 'index.ts')
const webDir = resolve(cliDir, 'web')
Expand Down Expand Up @@ -119,6 +121,37 @@ program
web?.kill('SIGTERM')
process.exit(code ?? ExitCode.SUCCESS)
})

// Readiness probe. Default-on: poll /health until it returns 200, then
// emit a stable, machine-parseable line on stdout. This is the only
// thing `serve` puts on stdout, so callers can do:
// openhop serve & wait_for=$(grep -m1 '^openhop: ready ' fd 1)
// Without --no-wait-ready, downstream scripts have to poll /health
// themselves to know when they can push.
if (opts.waitReady !== false) {
const timeoutSec = Number.parseInt(opts.readyTimeout, 10) || 60
const apiUrl = `http://localhost:${opts.port}`
const webPart = opts.web !== false ? ` web=http://localhost:8788` : ''
const t0 = Date.now()
const deadline = t0 + timeoutSec * 1000
while (Date.now() < deadline) {
try {
const r = await fetch(`${apiUrl}/health`)
if (r.ok) {
const elapsed = Math.round((Date.now() - t0) / 100) / 10
process.stdout.write(`openhop: ready api=${apiUrl}${webPart} elapsed=${elapsed}s\n`)
break
}
} catch {
// Not ready yet — back off and retry.
}
await new Promise((r) => setTimeout(r, 500))
}
Comment on lines +132 to +149
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Bound each /health probe; current implementation can exceed --ready-timeout.

At Line 139, fetch has no abort signal. A stalled request can block past the deadline, so timeout behavior becomes non-deterministic.

🔧 Suggested fix 
       const t0 = Date.now()
       const deadline = t0 + timeoutSec * 1000
       while (Date.now() < deadline) {
+        const remainingMs = deadline - Date.now()
+        if (remainingMs <= 0) break
+        const controller = new AbortController()
+        const probeTimeoutMs = Math.min(1500, remainingMs)
+        const timer = setTimeout(() => controller.abort(), probeTimeoutMs)
         try {
-          const r = await fetch(`${apiUrl}/health`)
+          const r = await fetch(`${apiUrl}/health`, { signal: controller.signal })
           if (r.ok) {
             const elapsed = Math.round((Date.now() - t0) / 100) / 10
             process.stdout.write(`openhop: ready api=${apiUrl}${webPart} elapsed=${elapsed}s\n`)
             break
           }
         } catch {
           // Not ready yet — back off and retry.
+        } finally {
+          clearTimeout(timer)
         }
         await new Promise((r) => setTimeout(r, 500))
       }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/cli/src/index.ts` around lines 132 - 149, The /health probe fetch
can hang and let the loop exceed opts.readyTimeout; fix it by using an
AbortController per iteration to bound each fetch to the remaining time before
deadline: inside the while loop (which uses t0 and deadline) compute remainingMs
= deadline - Date.now(), create an AbortController, schedule a timeout to call
controller.abort() after remainingMs (or a sensible per-probe cap), pass
controller.signal into fetch(`${apiUrl}/health`, { signal }), and clear the
timeout after fetch completes; handle the abort/error in the existing catch so
the loop continues deterministically and never blocks past the deadline.

if (Date.now() >= deadline) {
errStderr(red(`✗ API did not become ready within ${timeoutSec}s. Check logs above.`))
// Don't exit — the children may still come up. Just warn.
}
}
})

// --- push ---
Expand Down
64 changes: 54 additions & 10 deletions packages/shared/__tests__/patch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,13 +216,13 @@ describe('applyPatch', () => {
})

describe('add-steps', () => {
it('inserts a step at the beginning (after=-1)', () => {
it('inserts at the beginning (index: 0)', () => {
const root = makeRoot()
const result = applyPatch(root, {
operations: [
{
op: 'add-steps',
after: -1,
index: 0,
steps: [{ from: 'b', to: 'a', data: 'response' }],
},
],
Expand All @@ -236,13 +236,15 @@ describe('applyPatch', () => {
})
})

it('inserts a step after a given index', () => {
it('inserts at a middle index (existing step gets pushed right)', () => {
// makeRoot has 1 step (a→b). Insert at index 1 → becomes the 2nd step,
// existing step stays at index 0.
Comment on lines +239 to +241
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Test name/comment claims “middle insertion,” but this case is append-at-end.

At Line 240, makeRoot() starts with one step, so index: 1 is the end boundary, not a middle index. This currently doesn’t validate the “push-right” behavior described.

✅ Suggested test adjustment 
-    it('inserts at a middle index (existing step gets pushed right)', () => {
-      // makeRoot has 1 step (a→b). Insert at index 1 → becomes the 2nd step,
-      // existing step stays at index 0.
-      const root = makeRoot()
+    it('inserts at a middle index (existing step gets pushed right)', () => {
+      const root = makeRoot({
+        flow: {
+          nodes: [
+            { id: 'a', label: 'Node A' },
+            { id: 'b', label: 'Node B' },
+          ],
+          steps: [
+            { from: 'a', to: 'b', data: 's1' },
+            { from: 'b', to: 'a', data: 's2' },
+          ],
+        },
+      })
       const result = applyPatch(root, {
         operations: [
           {
             op: 'add-steps',
             index: 1,
             steps: [{ from: 'b', to: 'a', data: 'response' }],
           },
         ],
       })
       expect(result.success).toBe(true)
-      expect(result.data!.flow.steps).toHaveLength(2)
+      expect(result.data!.flow.steps).toHaveLength(3)
       expect(result.data!.flow.steps![1]).toMatchObject({
         from: 'b',
         to: 'a',
       })
     })

Also applies to: 247-257

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/shared/__tests__/patch.test.ts` around lines 239 - 241, The test
labeled 'inserts at a middle index (existing step gets pushed right)' is
actually appending at the end because makeRoot() returns a single-step root so
index: 1 is the tail; either change the initial fixture to produce at least two
steps (so index: 1 is a true middle insertion) or change the test
name/assertions to reflect an append-at-end scenario; update the same pattern in
the other case around the block covering lines 247-257. Ensure you modify the
test that calls makeRoot() (and any related assertions) or rename the it()
description to match the intended behavior.

const root = makeRoot()
const result = applyPatch(root, {
operations: [
{
op: 'add-steps',
after: 0,
index: 1,
steps: [{ from: 'b', to: 'a', data: 'response' }],
},
],
Expand Down Expand Up @@ -307,7 +309,7 @@ describe('applyPatch', () => {
{ op: 'remove-steps', indices: [0] },
{
op: 'add-steps',
after: -1,
index: 0,
steps: [
{ from: 'a', to: 'c', data: 'new step' },
{ from: 'c', to: 'b', data: 'another' },
Expand All @@ -330,7 +332,7 @@ describe('applyPatch', () => {
operations: [
{
op: 'add-steps',
after: 0,
index: 1,
steps: [{ from: 'a', to: 'ghost', data: 'bad' }],
},
],
Expand All @@ -351,18 +353,60 @@ describe('applyPatch', () => {

describe('edge cases', () => {
it('add-steps reports out-of-range insertion position', () => {
const root = makeRoot()
const root = makeRoot() // 1 step → valid range is 0..1
const result = applyPatch(root, {
operations: [
{
op: 'add-steps',
after: 99,
index: 99,
steps: [{ from: 'a', to: 'b', data: 'x' }],
},
],
})
expect(result.success).toBe(false)
expect(result.errors[0].message).toContain('out of range')
// Error should teach the recovery: the valid range and the
// "omit to append" shortcut.
expect(result.errors[0].message).toContain('omit to append')
})

it('add-steps appends when index is omitted (no need to know steps.length)', () => {
const root = makeRoot()
const initialCount = root.flow.steps?.length ?? 0
const result = applyPatch(root, {
operations: [
{
op: 'add-steps',
steps: [
{ from: 'a', to: 'b', data: 'appended-1' },
{ from: 'b', to: 'a', data: 'appended-2' },
],
},
],
})
expect(result.success).toBe(true)
const steps = result.data!.flow.steps!
expect(steps).toHaveLength(initialCount + 2)
// The two new steps must be at the very end, in order.
expect(steps[steps.length - 2]).toMatchObject({ data: 'appended-1' })
expect(steps[steps.length - 1]).toMatchObject({ data: 'appended-2' })
})

it('add-steps with explicit index === steps.length also appends', () => {
const root = makeRoot()
const initialCount = root.flow.steps?.length ?? 0
const result = applyPatch(root, {
operations: [
{
op: 'add-steps',
index: initialCount,
steps: [{ from: 'a', to: 'b', data: 'appended' }],
},
],
})
expect(result.success).toBe(true)
const steps = result.data!.flow.steps!
expect(steps[steps.length - 1]).toMatchObject({ data: 'appended' })
})
Comment on lines +395 to 410
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Append-at-length test should also assert step count increases.

This test checks last element content but not that insertion actually grows the array. Add a length assertion to lock behavior.

✅ Suggested assertion 
     it('add-steps with explicit index === steps.length also appends', () => {
       const root = makeRoot()
       const initialCount = root.flow.steps?.length ?? 0
       const result = applyPatch(root, {
@@
       })
       expect(result.success).toBe(true)
       const steps = result.data!.flow.steps!
+      expect(steps).toHaveLength(initialCount + 1)
       expect(steps[steps.length - 1]).toMatchObject({ data: 'appended' })
     })
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it('add-steps with explicit index === steps.length also appends', () => {
const root = makeRoot()
const initialCount = root.flow.steps?.length ?? 0
const result = applyPatch(root, {
operations: [
{
op: 'add-steps',
index: initialCount,
steps: [{ from: 'a', to: 'b', data: 'appended' }],
},
],
})
expect(result.success).toBe(true)
const steps = result.data!.flow.steps!
expect(steps[steps.length - 1]).toMatchObject({ data: 'appended' })
})
it('add-steps with explicit index === steps.length also appends', () => {
const root = makeRoot()
const initialCount = root.flow.steps?.length ?? 0
const result = applyPatch(root, {
operations: [
{
op: 'add-steps',
index: initialCount,
steps: [{ from: 'a', to: 'b', data: 'appended' }],
},
],
})
expect(result.success).toBe(true)
const steps = result.data!.flow.steps!
expect(steps).toHaveLength(initialCount + 1)
expect(steps[steps.length - 1]).toMatchObject({ data: 'appended' })
})
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/shared/__tests__/patch.test.ts` around lines 395 - 410, The test
"add-steps with explicit index === steps.length also appends" should also assert
the steps array grew: after calling applyPatch (using makeRoot and initialCount)
check that result.success is true, then assert that steps.length ===
initialCount + 1 to ensure the add-steps actually increased the count, in
addition to the existing content check of the last element.


it('add-steps initializes the steps array when absent', () => {
Expand All @@ -379,7 +423,7 @@ describe('applyPatch', () => {
operations: [
{
op: 'add-steps',
after: -1,
// Omit index → append to a freshly-created empty array.
steps: [{ from: 'a', to: 'b', data: 'first' }],
},
],
Expand Down Expand Up @@ -467,7 +511,7 @@ describe('applyPatch', () => {
// Now adds a step referencing the removed 'b'
{
op: 'add-steps',
after: -1,
index: 0,
steps: [{ from: 'a', to: 'b', data: 'orphan' }],
},
],
Expand Down
11 changes: 8 additions & 3 deletions packages/shared/src/patch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,12 @@ const clearFlowsOp = z.object({

const addStepsOp = z.object({
op: z.literal('add-steps'),
after: z.number().int().min(-1), // insert position
// 0-based position the new steps will occupy after insertion.
// Valid range [0, steps.length] inclusive. Same semantics as
// Array.prototype.splice: 0 prepends, steps.length appends.
// Omit to append — the most common case, and it doesn't require
// knowing the current steps.length.
index: z.number().int().min(0).optional(),
steps: z.array(z.any()).min(1),
})

Expand Down Expand Up @@ -211,11 +216,11 @@ export function applyPatch(root: Root, patch: PatchOperations): PatchResult {

case 'add-steps': {
if (!result.flow.steps) result.flow.steps = []
const insertIndex = op.after + 1
const insertIndex = op.index ?? result.flow.steps.length
if (insertIndex > result.flow.steps.length) {
errors.push({
path: 'flow.steps',
message: `Insert position ${op.after} is out of range`,
message: `index ${op.index} is out of range (valid: 0..${result.flow.steps.length}; omit to append)`,
})
} else {
result.flow.steps.splice(insertIndex, 0, ...op.steps)
Expand Down
22 changes: 11 additions & 11 deletions skills/openhop/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,17 +320,17 @@ data:

All operations support multiple items. Apply with `openhop patch <id> <file.yaml>`.

| Operation | Fields | Description |
| ------------ | ------------------------------------------ | ----------------------------- |
| add-nodes | nodes: [{id, label, type?, icon?, color?}] | Add nodes |
| remove-nodes | nodes: ["id1", "id2"] | Remove nodes + their steps |
| rename-nodes | nodes: [{id, label}] | Change labels |
| update-nodes | nodes: [{id, type?, icon?, color?}] | Update properties |
| set-flows | nodes: [{id, flow: {nodes, steps}}] | Add sub-flows |
| clear-flows | nodes: ["id1"] | Remove sub-flows |
| add-steps | after: N, steps: [...] | Insert steps (-1 = beginning) |
| remove-steps | indices: [0, 3] | Remove steps by index |
| update-step | index: N, step: {...} | Replace a step |
| Operation | Fields | Description |
| ------------ | ------------------------------------------ | ------------------------------------------------------------------------------------------- |
| add-nodes | nodes: [{id, label, type?, icon?, color?}] | Add nodes |
| remove-nodes | nodes: ["id1", "id2"] | Remove nodes + their steps |
| rename-nodes | nodes: [{id, label}] | Change labels |
| update-nodes | nodes: [{id, type?, icon?, color?}] | Update properties |
| set-flows | nodes: [{id, flow: {nodes, steps}}] | Add sub-flows |
| clear-flows | nodes: ["id1"] | Remove sub-flows |
| add-steps | index?: N, steps: [...] | Insert steps at 0-based `index` (same semantics as `Array.splice`). Omit `index` to append. |
| remove-steps | indices: [0, 3] | Remove steps by index |
| update-step | index: N, step: {...} | Replace a step |

## Icons

Expand Down
Loading