feat: clean add-steps API + serve readiness signal#54
Merged
Conversation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/index.ts`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a5aecd19-e583-47f0-98ce-fbd0d386693c
📒 Files selected for processing (4)
packages/cli/src/index.tspackages/shared/__tests__/patch.test.tspackages/shared/src/patch.tsskills/openhop/SKILL.md
Comment on lines
+132
to
+149
| 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)) | ||
| } |
There was a problem hiding this comment.
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.
## 1. add-steps: pure 0-based `index`, no sentinels
Original `after: -1` / `after: "end"` design needed two sentinels and
a paragraph of explanation. Refactored to match Array.prototype.splice
exactly: `index` is the 0-based position the new steps will occupy
after insertion. Range [0, steps.length] inclusive. Omit `index` to
append (the most common case, no need to know steps.length first).
add-steps | index?: N, steps: [...] | Insert at 0-based index;
omit to append.
Out-of-range error is short and self-explanatory:
index 99 is out of range (valid: 0..4; omit to append)
This is a breaking change vs the previous patch shape, but we haven't
shipped v0.1 yet and there are no users. Cleaner mental model now,
no doc burden, fewer special cases to test.
## 2. openhop serve: machine-readable readiness signal
Three sub-issues from the cold-start report:
(a) Long silence between "Starting OpenHop API..." and the child's
"Server listening at..." line (10-15s on slow disks)
(b) The server's stdout banner is a freeform string, not parseable
(c) No --wait-ready flag for `serve & openhop push ...` patterns
Fix: parent process polls `:<port>/health` after spawning children.
On 200, prints exactly one line to stdout:
openhop: ready api=http://localhost:8787 web=http://localhost:8788 elapsed=12.4s
Stable prefix `^openhop: ready ` so callers can `grep -m1`. Default-on;
opt out with --no-wait-ready. Configurable via --ready-timeout
<seconds> (default 60).
Stdout from `serve` is now reserved for that one line; child logs go
to parent stderr per #53. Stays consistent with stdout=data discipline.
## Tests
- 32/32 in @openhop/shared (was 30) — new add-steps cases:
- inserts at index 0 (prepend)
- inserts at middle index, existing step pushed right
- omitted index appends
- explicit `index: steps.length` also appends
- out-of-range error mentions valid range and the "omit to append" hint
- 83/83 in @openhop/cli unchanged
- prettier --check + eslint clean
CLI bundle 47.2 → 48.6 kb.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
naorsabag
force-pushed
the
feat/patch-end-and-serve-ready
branch
from
79a56d6 to
eeea2ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/cli/src/index.ts (1)
137-151:⚠️ Potential issue | 🟠 MajorBound each readiness probe and gate the timeout warning on actual failure.
A single
/healthfetchcan still block past--ready-timeout, and if it returns200after the deadline this block can print theopenhop: ready ...line and then immediately print the timeout warning. Abort each probe against the remaining budget and track areadyflag before emitting the warning.Suggested fix
+ let ready = false while (Date.now() < deadline) { + const remainingMs = deadline - Date.now() + if (remainingMs <= 0) break + const controller = new AbortController() + const timer = setTimeout(() => controller.abort(), Math.min(1500, remainingMs)) 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`) + ready = true break } } catch { // Not ready yet — back off and retry. + } finally { + clearTimeout(timer) } await new Promise((r) => setTimeout(r, 500)) } - if (Date.now() >= deadline) { + if (!ready) { errStderr(red(`✗ API did not become ready within ${timeoutSec}s. Check logs above.`)) // Don't exit — the children may still come up. Just warn. }🤖 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 137 - 151, The readiness loop can race with the overall deadline because individual fetch calls may exceed the remaining budget; update the loop in index.ts to abort each /health fetch using an AbortController whose timeout is the remaining time (deadline - Date.now()), set a local ready boolean (e.g., ready = true) when r.ok is seen and break, and after the loop only emit the timeout warning guarded by if (!ready) using timeoutSec and the existing errStderr(red(...)) call; adjust references to deadline, apiUrl, webPart, and timeoutSec and ensure the AbortController signal is passed into fetch so probes don't block past the global timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/index.ts`:
- Around line 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.
In `@packages/shared/__tests__/patch.test.ts`:
- Around line 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.
- Around line 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.
---
Duplicate comments:
In `@packages/cli/src/index.ts`:
- Around line 137-151: The readiness loop can race with the overall deadline
because individual fetch calls may exceed the remaining budget; update the loop
in index.ts to abort each /health fetch using an AbortController whose timeout
is the remaining time (deadline - Date.now()), set a local ready boolean (e.g.,
ready = true) when r.ok is seen and break, and after the loop only emit the
timeout warning guarded by if (!ready) using timeoutSec and the existing
errStderr(red(...)) call; adjust references to deadline, apiUrl, webPart, and
timeoutSec and ensure the AbortController signal is passed into fetch so probes
don't block past the global timeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f292b58-b4c2-48c2-9dcb-c56c76a89d0f
📒 Files selected for processing (4)
packages/cli/src/index.tspackages/shared/__tests__/patch.test.tspackages/shared/src/patch.tsskills/openhop/SKILL.md
✅ Files skipped from review due to trivial changes (1)
- skills/openhop/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shared/src/patch.ts
Comment on lines
+74
to
+75
| .option('--ready-timeout <seconds>', 'How long to wait for readiness before giving up', '60') | ||
| .action(async (opts) => { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the file structure and exact lines
head -150 packages/cli/src/index.ts | tail -80Repository: naorsabag/OpenHop
Length of output: 3322
🏁 Script executed:
# Look at the actual option definition and handler
sed -n '60,150p' packages/cli/src/index.tsRepository: 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 2Repository: 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)
EOFRepository: 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 jsRepository: 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 -20Repository: 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 -20Repository: 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 InvalidArgumentErrorRepository: 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:
- 1: https://www.npmjs.com/package/commander
- 2: https://www.jsdocs.io/package/commander
- 3: Support custom error messages for option argument coercion failures tj/commander.js#1392
- 4: https://github.com/tj/commander.js/blob/master/Readme.md
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.
Comment on lines
+239
to
+241
| 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. |
There was a problem hiding this comment.
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.
Comment on lines
+395
to
410
| 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' }) | ||
| }) |
There was a problem hiding this comment.
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.
Owner
Author
|
@coderabbitai - Title fixed (was a stale draft from before mid-PR redesign). On docstring coverage 0%: this PR adds inline comments at the field/operation level (zod schema docstring, readiness-probe block comment) rather than function-level docstrings, since the new code is mostly inline schema fields and a 20-line action-callback block. Prefer keeping inline comments where they read naturally. Acknowledged as a non-blocking warning. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two real-code follow-ups from the cold-start agent's findings (deferred from #53).
1. `add-steps` accepts `after: "end"`
Agent previously had to `openhop get` first to learn `steps.length` before appending — `after: N` had no append sentinel. Now `after` is a union of `number | "end"`. Out-of-range error message also hints at the new sentinel:
```
Insert position 99 is out of range (have 4 steps; valid: -1..3 or "end")
```
2. `openhop serve` readiness signal
Addresses three sub-issues from the cold-start report:
Fix: parent polls `:/health` after spawning, prints one stable line on stdout when ready:
```
openhop: ready api=http://localhost:8787 web=http://localhost:8788 elapsed=12.4s
```
Default-on (since this is what callers want); `--no-wait-ready` to opt out, `--ready-timeout ` configurable (default 60).
Stays consistent with the CLI's stdout=data rule — readiness is the only thing on stdout; child logs go to stderr per #53.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--no-wait-readyto disable health checks and--ready-timeoutto set maximum polling duration (default 60 seconds).Improvements
add-stepsoperation to use optionalindexparameter with append behavior when omitted.Documentation