Skip to content

fix(cli): reject a malformed coordinator address/port in dora up - #3100

Open
phil-opp wants to merge 2 commits into
mainfrom
claude/dreamy-bardeen-tkzcup-up-coordinator-env
Open

fix(cli): reject a malformed coordinator address/port in dora up#3100
phil-opp wants to merge 2 commits into
mainfrom
claude/dreamy-bardeen-tkzcup-up-coordinator-env

Conversation

@phil-opp

@phil-opp phil-opp commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Issue

dora up resolved the coordinator location from DORA_COORDINATOR_ADDR / DORA_COORDINATOR_PORT like this (binaries/cli/src/command/up.rs):

let addr: std::net::IpAddr = std::env::var("DORA_COORDINATOR_ADDR")
    .ok().and_then(|s| s.parse().ok()).unwrap_or(LOCALHOST);
let port: u16 = std::env::var("DORA_COORDINATOR_PORT")
    .ok().and_then(|s| s.parse().ok()).unwrap_or(DORA_COORDINATOR_PORT_WS_DEFAULT);

.and_then(|s| s.parse().ok()) throws away a parse error, so a present-but-unparseable value silently falls back to the default. Every other lifecycle command reads the same two env vars through clap's typed env= on CoordinatorOptions (common.rs), which errors on an unparseable value.

The result is a silent divergence: DORA_COORDINATOR_PORT=abc dora up starts a coordinator on the default port 53290, while dora stop / dora list / dora down abort with invalid value for '--coordinator-port' — so they can no longer find the coordinator that up just started.

Fix

Add a small coordinator_env_value helper that returns the default only when the variable is unset, and returns an error when it is set but unparseable — mirroring the strictness of CoordinatorOptions. Route both reads through it. Behavior is unchanged for the unset case and for valid values.

Validation

  • Added unit tests (coordinator_env_tests): unset → default, valid → parsed, malformed → error (not defaulted).
  • cargo +1.97.1 fmt -p dora-cli -- --check — clean.
  • cargo +1.97.1 clippy -p dora-cli -- -D warnings — clean.
  • cargo +1.97.1 test -p dora-cli coordinator_env — 3 passed.

⚠️ This is a machine-generated pull request authored by Claude (Claude Code). A human should review before merging.

🤖 Generated with Claude Code


Generated by Claude Code

`dora up` read DORA_COORDINATOR_ADDR / DORA_COORDINATOR_PORT with
`.and_then(|s| s.parse().ok()).unwrap_or(default)`, so a present but
unparseable value (e.g. a typo'd port) was silently discarded and the
default was used. Every other lifecycle command reads the same env vars
through clap's typed `env=` on `CoordinatorOptions`, which errors on an
unparseable value. The result was a silent divergence: `DORA_COORDINATOR_PORT=abc dora up`
started a coordinator on the default port while `dora stop`/`list`/`down`
aborted with an invalid-value error and could not find it.

Add a small `coordinator_env_value` helper that returns the default only
when the var is unset and errors when it is set but unparseable, and route
both reads through it. Adds unit tests for the unset/valid/malformed cases.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K1Fmp8pELuTiPGTStomkZj
@trunk-io

trunk-io Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

Follow-up to the /simplify review: the two call sites each repeated the
env var name once as the error-label argument and once inside
`std::env::var("…")`, which could drift out of sync. Fold the
`std::env::var(...)` read into `coordinator_env_value` so callers name
each variable once, and keep the raw-value seam as a private
`parse_coordinator_env` that the unit tests target without touching the
process environment.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K1Fmp8pELuTiPGTStomkZj

Copy link
Copy Markdown
Collaborator Author

🤖 Automated review by Claude Code — fully automated review, not vetted by a human.

No issues found. The change replaces the silent .and_then(|s| s.parse().ok()).unwrap_or(default) fallback with a helper that returns the default only when the var is unset and errors on a present-but-unparseable value, matching the strictness of clap's typed env= on CoordinatorOptions. The parse_coordinator_env seam is a clean way to unit-test without touching the process environment, and the three tests (unset / valid / malformed) exercise meaningful behavior rather than being tautological.

One minor behavioral note, not a blocker: an env var set to an empty string (e.g. DORA_COORDINATOR_PORT=) now errors where it previously fell back to the default, since "".parse::<u16>()/IpAddr fails. That is consistent with the stated goal of agreeing with the other lifecycle commands (which route through clap and would also reject empty), so it looks intentional.


Generated by Claude Code

@phil-opp
phil-opp marked this pull request as ready for review August 11, 2026 16:07
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.

2 participants