fix(cli): cold-start agent findings — serve+web, get nodeCount, error URLs#53
Merged
Conversation
… URLs
Three bugs surfaced by a fresh-context agent that ran the full OpenHop
flow end-to-end (validate → push → get → patch → get → remove). All
commands worked first try, but it caught three rough edges:
1. `openhop serve` only started the API on :8787, not the web UI on
:8788. The URL printed by `push` points at :8788, so users clicked
into a 404. Now serves both: spawns the Vite dev server alongside
Fastify. `--no-web` opts out for headless / CI use. Both children
are torn down on SIGINT/SIGTERM and on API exit.
2. `get --json` returned the full storedFlow shape but no flat
`nodeCount`, while `push --json` did. Inconsistent. Add `nodeCount`
to `get --json` so agents can read it without drilling into
`.flow.nodes.length`.
3. Network-error JSON used to be {ok:false, error:"network",
message:"fetch failed"}. Useless for diagnosis — no host or port.
Now includes the URL it tried for every command (push, list, get,
patch, remove). Human-mode error message also says
`Connection failed (http://...)` instead of just `Connection failed`.
Contract suite extended: every server-touching command's network-error
JSON must include `url` matching the failing endpoint. New test
iterates list/get/remove/push and locks the contract.
Tests: 83/83 in @openhop/cli (was 82). Bundle 47.1 kb (was 45.8 — the
extended serve action adds ~1.3 kb).
Caveat noted in serve's comment: this command only works in a
from-source checkout; the published `npm i -g openhop` package doesn't
ship server/ or web/. Tracked for v0.2 with a separate @openhop/server
package or a docker-based prod path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughExtends the network-error JSON contract to include a Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer / CLI
participant CLI as CLI (parent)
participant API as Fastify API (child)
participant Web as Vite Dev Server (child)
Dev->>CLI: run `cli serve` (optionally `--no-web`)
CLI->>API: spawn API process (npx tsx ...)
CLI->>Web: spawn Web process (npm run dev) / skipped if --no-web
API-->>CLI: error / exit
CLI->>Web: kill (if running)
CLI->>API: ensure shutdown
Dev->>CLI: SIGINT / SIGTERM
CLI->>API: forward shutdown
CLI->>Web: kill (if running)
CLI-->>Dev: exit with propagated code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🧹 Nitpick comments (1)
packages/cli/__tests__/contract.test.ts (1)
255-265: Includepatchin the cross-command URL contract matrix.The new matrix claims to lock all server-touching commands, but
patchis still missing fromcases, so itsurlfield can regress without failing this test.♻️ Suggested diff
it('every network-error JSON includes the URL that failed', () => { @@ - const cases = [ + const validPatch = + 'operations:\n - op: rename-nodes\n nodes:\n - id: a\n label: A\n' + const cases: Array<{ args: string[]; input?: string }> = [ { args: ['list', '--json', '--server', 'http://127.0.0.1:1'] }, { args: ['get', 'x', '--json', '--server', 'http://127.0.0.1:1'] }, { args: ['remove', 'x', '--json', '--server', 'http://127.0.0.1:1'] }, { args: ['push', EXAMPLE_GOOD, '--json', '--server', 'http://127.0.0.1:1'], }, + { + args: ['patch', 'x', '-', '--json', '--server', 'http://127.0.0.1:1'], + input: validPatch, + }, ] for (const c of cases) { - const r = run(c.args) + const r = run(c.args, c.input)Also applies to: 266-274
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/__tests__/contract.test.ts` around lines 255 - 265, The test matrix variable cases in contract.test.ts is missing the 'patch' command so its URL contract can regress; add a new case entry in the cases array for the 'patch' command (similar to the existing 'push' entry) using the same fixture symbol EXAMPLE_GOOD and flags (e.g. '--json', '--server', 'http://127.0.0.1:1') so the patch command's URL is validated alongside list/get/remove/push.
🤖 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/get.ts`:
- Around line 24-27: Remove the unused variable bindings named fullUrl that
cause the ESLint failure: locate the occurrences of "const fullUrl =
`${opts.server}/api/flows/${flowId}`" (and the similar binding around lines
62-66) in packages/cli/src/get.ts and either delete those const declarations or
replace usages so the value is actually used (preferably delete the unused
consts since fetch already builds the URL); ensure no other references to
fullUrl remain.
In `@packages/cli/src/index.ts`:
- Around line 79-91: The child processes (api/web) are currently started with
stdio: 'inherit', which sends child stdout into the CLI stdout and can mix logs
with data; change both spawn calls that create api (spawn('npx', ...,
serverEntry)) and web (spawn('npm', ['run','dev'], { cwd: webDir })) to use a
stdio tuple that prevents inheriting stdout (e.g., stdio:
['ignore','pipe','inherit'] or ['inherit','pipe','inherit']) so child stdout is
piped (not inherited) while stderr remains inherited; also ensure any created
child.stdout streams (from the api/web child) are either consumed, forwarded to
a logger, or explicitly ignored to avoid unhandled stream warnings.
---
Nitpick comments:
In `@packages/cli/__tests__/contract.test.ts`:
- Around line 255-265: The test matrix variable cases in contract.test.ts is
missing the 'patch' command so its URL contract can regress; add a new case
entry in the cases array for the 'patch' command (similar to the existing 'push'
entry) using the same fixture symbol EXAMPLE_GOOD and flags (e.g. '--json',
'--server', 'http://127.0.0.1:1') so the patch command's URL is validated
alongside list/get/remove/push.
🪄 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: f7e04ff2-60ab-4ecc-8029-c8ad891e8160
📒 Files selected for processing (3)
packages/cli/__tests__/contract.test.tspackages/cli/src/get.tspackages/cli/src/index.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/src/get.ts (1)
47-47: Optional: simplifynodeCountexpression and drop non-null assertionsThis is equivalent but easier to read and avoids
!assertions.♻️ Suggested refactor
- const nodeCount = Array.isArray(flow.flow?.nodes) ? flow.flow!.nodes!.length : 0 + const nodes = flow.flow?.nodes + const nodeCount = Array.isArray(nodes) ? nodes.length : 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/get.ts` at line 47, The nodeCount expression uses Array.isArray and non-null assertions; replace it with a simpler optional-chaining/nullish-coalescing expression to avoid `!` assertions and improve readability: update the declaration of nodeCount (the variable currently computed from flow.flow?.nodes) to use something like optional chaining on flow and nodes and `?? 0` to default when missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/cli/src/get.ts`:
- Line 47: The nodeCount expression uses Array.isArray and non-null assertions;
replace it with a simpler optional-chaining/nullish-coalescing expression to
avoid `!` assertions and improve readability: update the declaration of
nodeCount (the variable currently computed from flow.flow?.nodes) to use
something like optional chaining on flow and nodes and `?? 0` to default when
missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fcd654a5-9f62-4783-b712-2c9f04b41446
📒 Files selected for processing (1)
packages/cli/src/get.ts
CodeRabbit on PR #53 flagged that `stdio: 'inherit'` on the API + web children leaks Fastify's pino logs and Vite's dev banner onto the CLI's stdout. CLI contract is stdout=data, stderr=logs. Pipe both children's stdout and stderr through the parent's stderr so jq-style post-processing of `serve` output stays clean. Tests still 83/83.
Owner
Author
|
@coderabbitai addressed in 0df5e87 — both children now use |
|
Tip For best results, initiate chat on the files or code changes.
|
5 tasks
naorsabag
added a commit
that referenced
this pull request
Apr 27, 2026
## 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
added a commit
that referenced
this pull request
Apr 27, 2026
## 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: Naor Sabag <naorsabag@users.noreply.github.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 2, 2026
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.
Summary
A fresh-context agent ran the full OpenHop flow end-to-end (validate → push → get → patch → get → remove) — every command worked first try. It caught three rough edges, all fixed here.
Three fixes
1. `openhop serve` now starts both API + web UI
2. `get --json` includes `nodeCount`
3. Network-error JSON includes failing URL
Test plan
Caveat
`serve` only works in a from-source checkout — the published `npm i -g openhop` doesn't ship server/ or web/. Tracked for v0.2 with a separate @openhop/server package or a docker-based prod path.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
servenow launches API and web servers together (use--no-webfor API-only), coordinates shutdown across them, and preserves stdout for data-only CLI output.pushsuccess output distinguishes the API base URL from the returned web UI flow URL.Bug Fixes
list,get,remove,pushconsistently report network errors with the same exit status and include the attempted URL.getJSON now includes a node count; human output prefers flow metadata title when present.