Skip to content

Add managed local dev server helper#303

Open
lewtun wants to merge 3 commits into
mainfrom
codex/dev-server-helper
Open

Add managed local dev server helper#303
lewtun wants to merge 3 commits into
mainfrom
codex/dev-server-helper

Conversation

@lewtun
Copy link
Copy Markdown
Member

@lewtun lewtun commented Jun 7, 2026

Summary

  • Add a managed local dev server helper that starts backend and frontend together with conflict-free ports.
  • Configure Vite/backend local dev settings through environment variables so the helper can wire the proxy and CORS dynamically.
  • Add stale server cleanup before startup plus an explicit cleanup command, and document teardown guidance for retired PR worktrees.

Validation

  • uv run --frozen --extra dev ruff check .
  • uv run --frozen --extra dev ruff format --check .
  • uv run --frozen --extra dev pytest tests/unit/test_dev_server.py
  • npm run lint (passes with existing warnings)
  • npm run build
  • Smoke-tested dev_server.py cleanup, up, active cleanup, and down.

lewtun and others added 3 commits June 7, 2026 22:30
Co-authored-by: OpenAI Codex <codex@openai.com>
Co-authored-by: OpenAI Codex <codex@openai.com>
Co-authored-by: OpenAI Codex <codex@openai.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 7, 2026

Claude finished @lewtun's task in 2m 1s —— View job


PR Review: Add managed local dev server helper

No blocking issues — 2 P1, 1 P2. This is local dev tooling; the only production-path file is backend/main.py, and its CORS change is operator-controlled (env-driven, not request-reflected), so no security regression there.


P1 — Stale-server cleanup silently no-ops when lsof is absent

The headline "cleans stale dev servers from the same worktree" feature depends entirely on resolving each candidate's working directory, and that resolution only uses lsof:

  • process_cwd (scripts/dev_server.py:142-158) shells out to lsof; it returns None on FileNotFoundError or non-zero exit.
  • classify_dev_server_process (scripts/dev_server.py:271-280) returns None whenever process.cwd is None.
  • So discover_stale_dev_servers (scripts/dev_server.py:314-331) finds nothing, and cleanup/up quietly skip the cleanup they advertise.

lsof is commonly missing on Linux dev containers. On Linux a /proc/<pid>/cwd symlink (os.readlink) is a zero-dependency fallback that would make this robust. Worth at least a warning when lsof is unavailable so the no-op isn't silent.

Fix this →

P1 — New CORS-merging behavior has no test coverage

The custom review brief calls out test coverage for new behavior. The new security-adjacent logic is untested:

  • _split_env_list / _cors_origins (backend/main.py:29-44) — parsing + default-merge.
  • build_backend_env dedup + 127.0.0.1localhost mirroring (scripts/dev_server.py:418-429).

tests/unit/test_dev_server.py covers host_url, find_free_port, classify_*, and managed_stack_active, but not the origin-merging path that decides who may send credentialed requests. A couple of cases (empty env, comma list with blanks, dedup ordering) would lock this down.

Fix this →


P2 — Vite proxy target is now a bracketed IPv6 literal

build_frontend_env sets VITE_BACKEND_PROXY_TARGET to host_url(args.backend_host, port)http://[::1]:7860 by default (scripts/dev_server.py:432-436, 57-60), where the prior hardcoded target was http://localhost:7860. The up smoke test exercises the proxy health check, so this works in practice — just noting it as a deliberate behavior change (IPv6-literal target) in case a future contributor expects the old localhost target.


Nice touches: per-host find_free_port with SO_REUSEADDR, pgid-based group termination with SIGTERM→SIGKILL escalation, and the match_markers + cwd guard so cleanup won't kill unrelated uvicorn/vite processes. The managed_stack_active "all-or-nothing" check is a good way to avoid acting on a half-dead stack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant