Feat: run the demo in the background on uncommon ports, with a preflight - #719
Conversation
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe demo now uses fixed loopback ports, validates those bindings, checks port availability before installation, starts the proxy in the background with readiness logging, and updates the README quick start for watching agent traffic. ChangesDemo flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant install-demo.sh
participant authbridge-proxy
participant demo.log
install-demo.sh->>install-demo.sh: Check required loopback ports
install-demo.sh->>authbridge-proxy: Start --demo with nohup
authbridge-proxy->>demo.log: Write binding status
install-demo.sh->>demo.log: Poll for forward-proxy readiness
install-demo.sh-->>install-demo.sh: Print viewer, proxy, and stop commands
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@authbridge/install-demo.sh`:
- Around line 75-82: Update the preflight port-check loop in install-demo.sh to
include the health listener port 9091 alongside DEMO_FORWARD_PORT,
DEMO_SESSION_PORT, and DEMO_STATS_PORT, while preserving the existing
AUTHBRIDGE_INSTALL_ONLY guard and failure handling.
- Around line 170-190: Update the startup readiness loop around demo_pid and the
forward-proxy log check so it only succeeds after the proxy has actually bound
its socket, using a real endpoint probe or a post-bind readiness signal rather
than the pre-bind name=forward-proxy entry. On timeout or bind failure, retain
the diagnostic warning/log-tail behavior and do not print the “Cortex demo is
running” message; emit that success message only when readiness is confirmed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2bcfff0b-5bbd-4fec-9418-4cda01559fd7
📒 Files selected for processing (4)
README.mdauthbridge/cmd/authbridge-proxy/demo.goauthbridge/cmd/authbridge-proxy/demo_test.goauthbridge/install-demo.sh
| # --- preflight: fail early (before downloading) if a demo port is taken --- | ||
| if [ "${AUTHBRIDGE_INSTALL_ONLY:-}" != "1" ]; then | ||
| for p in "$DEMO_FORWARD_PORT" "$DEMO_SESSION_PORT" "$DEMO_STATS_PORT"; do | ||
| if port_in_use "$p"; then | ||
| die "port ${p} is already in use. Is the demo already running (see ./cortex-ca/demo.pid)? Otherwise free the port, or change the ports in ./cortex-ca/demo.yaml, then re-run." | ||
| fi | ||
| done | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Preflight the health listener port too.
The demo still binds health on 9091, but this loop only checks the three new ports. A collision on 9091 downloads and launches the demo only for it to fail during startup. Add 9091 to the preflight set.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@authbridge/install-demo.sh` around lines 75 - 82, Update the preflight
port-check loop in install-demo.sh to include the health listener port 9091
alongside DEMO_FORWARD_PORT, DEMO_SESSION_PORT, and DEMO_STATS_PORT, while
preserving the existing AUTHBRIDGE_INSTALL_ONLY guard and failure handling.
Improve the local demo UX from three angles. install-demo.sh now starts the proxy in the background instead of foreground: nohup + redirected output + a pidfile, then it waits for readiness — the forward proxy's port actually accepting (a post-bind probe, not the pre-bind "listening" log line), or the process exiting (log tail + hard fail) — before returning to the prompt. It prints the viewer / agent commands and a stop command, and — because it now regains control — the agent command carries the absolute CA path, so the old "run from the same directory" caveat is gone. It also runs a best-effort preflight (lsof, then nc) on the demo ports before downloading, aborting with a clear message if one is taken (e.g. the demo is already running). Move the demo listeners to uncommon loopback ports to cut collision odds: the old 8081 / 9094 / 9093 overlap with common dev and observability tools. Forward proxy -> 127.0.0.1:47600, session API -> 127.0.0.1:47601, stats -> 127.0.0.1:47602. Pinning stats also de-wildcards it (it defaulted to :9093 on all interfaces). Health stays on :9091 (hardcoded, non-fatal on conflict). The port constants live in both demo.go and install-demo.sh; a comment in each notes the coupling. Rework the root README quickstart into three tight steps on the new ports: install + start (one backgrounded command), open the viewer (abctl), and send an agent's traffic through it (Claude Code). No CA prose — the env var just appears in the command — and no build-from-source fallback line. Test: shellcheck clean; go build/test/vet green (the demo test asserts the new loopback ports). Ran the branch binary --demo — binds 47600/47601/47602 on loopback, no transparent listener. Ran install-demo.sh end to end on darwin/arm64: preflight aborts on an occupied port; otherwise it backgrounds the proxy, writes the pidfile, detects readiness, prints the instructions, and returns to the shell with the proxy still running (killable via the pidfile). Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
esnible
left a comment
esnible
left a comment
There was a problem hiding this comment.
Clean, well-scoped UX improvement to the local --demo quickstart: backgrounds the proxy with a pidfile + readiness probe, moves listeners to uncommon loopback ports (47600–47602), adds a best-effort port preflight, and de-wildcards the stats endpoint (previously defaulted to :9093 on all interfaces — a genuine security tightening).
Verified the new stats:/address YAML maps correctly to cfg.Stats.StatsAddress (authbridge/authlib/config/config.go) and matches the new test assertion; the demo's explicit value also overrides the old :9093 wildcard default. Port constants are kept in sync between demo.go and install-demo.sh with cross-reference comments in both. Shell logic is sound: port_in_use semantics (lsof -sTCP:LISTEN, then nc -z, else assume-free) are consistent across preflight and readiness, the preflight is correctly skipped in AUTHBRIDGE_INSTALL_ONLY=1 mode, and startup detects early exit (bind failure) before probing the port.
Only a trivial nit (see inline). All CI green.
Author: huang195 (MEMBER — maintainer)
Areas reviewed: Shell, Go, Docs
Agent/IDE config (.claude/.vscode): none
Commits: 1 commit, signed-off: yes
CI status: passing (all checks green; Spellcheck skipped)
Assisted-By: Claude Code
|
|
||
| 3. **Run your agent through it** — e.g. Claude Code (from the same directory, so `./cortex-ca` resolves — or use the absolute path the proxy logged): | ||
| 3. **Send an agent's traffic through it** — e.g. Claude Code, from the directory where you started the demo: | ||
|
|
There was a problem hiding this comment.
nit: Step 3 still frames it as "from the directory where you started the demo" and uses $PWD/cortex-ca/ca.crt, while the installer now prints an absolute CA path (${ca_dir}/ca.crt) so it can be pasted from anywhere. The PR body notes the "run from the same directory" caveat is gone — worth aligning the README wording/path to match, though it's fine as-is for a simplified quickstart.
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
3 participants
Summary
Three UX improvements to the local
--demoquickstart, following on from #718:Plus a leaner root-README quickstart.
Changes
install-demo.shstarts the demo in the background. Instead ofexec authbridge-proxy --demo(foreground, Ctrl-C to stop), it nownohups the proxy with redirected output and a pidfile, waits until the forward proxy is actually listening — or surfaces the log tail if it died — then returns to the prompt. It prints the viewer/agent commands and akillstop command. Because it regains control, the agent command now carries the absolute CA path, so the old "run from the same directory" caveat is gone.Preflight. Before downloading, a best-effort check (
lsof, thennc; skipped if neither exists) tests the three demo ports and aborts with a clear message if one is taken — e.g. "port 47600 is already in use. Is the demo already running…?"Uncommon loopback ports.
8081/9094/9093overlap with common dev + observability tools (alt-http, Alertmanager, pushgateway). Moved to:127.0.0.1:8081127.0.0.1:47600127.0.0.1:9094127.0.0.1:47601:9093(wildcard)127.0.0.1:47602:9091Pinning stats also de-wildcards it (it had defaulted to all interfaces). Port constants live in both
demo.goandinstall-demo.sh, with a sync comment in each.Leaner README quickstart. One command that installs, starts the demo in the background, and prints the next steps. The CA,
NODE_EXTRA_CA_CERTS, port numbers, and the multi-env-var agent command are all shown by the tool at runtime, so the README no longer repeats them.Testing
shellcheckclean (viakoalaman/shellcheckcontainer);go build/test/vetgreen — the demo test asserts the new loopback ports (incl. stats).--demo: binds47600/47601/47602on loopback, no transparent listener.install-demo.shend-to-end on darwin/arm64: preflight aborts cleanly on an occupied port; otherwise it backgrounds the proxy, writescortex-ca/demo.pid, detects readiness (Cortex demo is running (pid N)), prints the instructions with the absolute CA path, and returns to the shell with the proxy still running (killable via the pidfile). (The releasedv0.7.0-alpha.1binary predates the port change, so the ports become fully consistent once this PR ships in a release; mechanics verified against it, ports verified against the branch binary.)Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit
New Features
Documentation