assign-ports: warn on malformed explicit port env vars (follow-up to #242)

[justin808]

Follow-up to shakacode/shakaperf-old#242 review

The shakacode/shakaperf-old#242 review noted a pre-existing inconsistency in assign-ports.ts:

• readBasePortOverride (SHAKAPERF_BASE_PORT / CONDUCTOR_PORT) warns on a non-integer, out-of-range, or privileged value.
• readEnvOverride (SHAKAPERF_CONTROL_PORT / SHAKAPERF_EXPERIMENT_PORT) silently returned null for non-integer/empty values — so a typo like SHAKAPERF_CONTROL_PORT=40x0 was swallowed with no feedback, and the run quietly fell through to the base-port/sticky path.

Change

Extract parseExplicitPort(env, varName):

• present + valid positive integer → returns the port
• absent or blank → null, silently (the common case)
• present but invalid → console.warn(...) + null, mirroring readBasePortOverride

Both vars must still be valid to pin the explicit pair.

Tightening the check to /^\d+$/ also drops the old Number() coercions for hex/scientific notation (e.g. "0x1F" → 31), which were never intended as port specs.

Tests

• a present-but-invalid explicit port var warns and falls through;
• blank/absent vars stay silent.

Local: shaka-shared 50/50 pass, tsc --noEmit clean.

---

Migrated from shakacode/shakaperf-old#245 (original branch jg/env-port-override-warnings). Cross-references above point to the original repo.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 05b2cb96-d600-48d6-af27-9be15558abc1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jg/env-port-override-warnings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

This PR tightens explicit port environment parsing for automatic port assignment. The main changes are:

• Added parseExplicitPort for explicit control and experiment port vars.
• Warns on malformed, out-of-range, and privileged explicit port values.
• Keeps blank or absent explicit vars silent.
• Adds tests for invalid, privileged, single-var, and blank env-var cases.

Confidence Score: 5/5

The changed flow looks mergeable after a small cleanup to its diagnostics.

• Explicit valid pairs still short-circuit automatic assignment.
• Invalid and out-of-range values fall through as intended.
• Malformed values can produce a confusing second warning.

packages/shaka-shared/src/assign-ports.ts

Important Files Changed

Filename	Overview
packages/shaka-shared/src/assign-ports.ts	Adds explicit env-var parsing and warnings; malformed present values can produce an extra generic warning.
packages/shaka-shared/src/tests/assign-ports.test.ts	Adds coverage for invalid, out-of-range, privileged, single-var, and blank explicit port vars.

_{Reviews (1): Last reviewed commit: "assign-ports: warn on partial explicit o..." | Re-trigger Greptile}

[claude]

Review

Small, well-scoped fix — extracting parseExplicitPort gives SHAKAPERF_CONTROL_PORT/SHAKAPERF_EXPERIMENT_PORT the same warn-on-malformed treatment readBasePortOverride already had, closing the silent-typo gap called out in the earlier review. Went through assign-ports.ts and its test file in detail.

Correctness

• parseExplicitPort logic checks out: blank/absent → silent null (common case), present-but-invalid → warn + null, valid-but-privileged → warn + still returns the port. Matches the new tests.
• The "both vars required" fallback warning in readEnvOverride correctly fires only when at least one var was actually set (via the isPresent check), so the common "no override configured" path stays silent.
• Tightening to /^\d+$/ cleanly drops the old implicit Number() coercion of hex/scientific notation — good catch, since those were never valid port specs.
• Traced through the double-warning case (one var invalid, e.g. SHAKAPERF_CONTROL_PORT=40x0 + a valid experiment port): this now emits two warnings — the specific "not a valid port" from parseExplicitPort plus the generic "must be set to valid ports" fallback warning. That looks intentional given the comment on line 160-163, and the tests only assert toHaveBeenCalledWith(...) (not call count), so it's consistent — just flagging in case the double message wasn't deliberate.

Tests: good coverage — invalid integer, out-of-range, privileged-but-valid, only-one-var-set, and blank/absent-is-silent are all exercised, mirroring the existing readBasePortOverride test suite's shape.

Nit: left one inline comment — the new parseExplicitPort doc comment claims its range matches readBasePortOverride's 1..MAX_PORT, but the latter actually bounds itself to 1..MAX_PORT-1 to leave room for the derived +1 experiment offset. Not a functional bug (the two functions have legitimately different constraints), just a wording tightening.

Security/perf: no concerns — this only affects local env-var parsing and console warnings, no new I/O or execution surface.

Overall: looks correct and safe to merge; only the doc-comment nit above is worth a look.

[claude]

Review summary

Focused, well-scoped change: extracting parseExplicitPort to make malformed SHAKAPERF_CONTROL_PORT/SHAKAPERF_EXPERIMENT_PORT values warn instead of silently falling through, mirroring readBasePortOverride. The logic checks out for the main paths (blank/absent stays silent, malformed/out-of-range/privileged all warn appropriately), and the new tests cover those cases well.

One real gap (left as an inline comment): the de-duplication added in the latest commit ("avoid duplicate explicit-port warnings") only suppresses the generic "must be set to valid ports" message when both vars are non-blank. When exactly one var is set and that value is itself malformed (e.g. SHAKAPERF_EXPERIMENT_PORT=40x0 with SHAKAPERF_CONTROL_PORT unset), both the specific parseExplicitPort warning and the generic warning fire — the exact double-warning problem this PR is trying to eliminate, just for a combination the new tests don't exercise.

No security or performance concerns — this is pure env-var parsing with no external input trust boundary crossed, and the added regex/branching is negligible cost, only run once at process startup.

[claude]

Review

Traced through parseExplicitPort / readEnvOverride by hand against every case in the new test file (both valid, one absent, one blank, one malformed, out-of-range, privileged, both malformed, lone var set) — the logic and warning de-duplication match the stated intent in every case, and the extracted validation now correctly mirrors readBasePortOverride. No functional bugs found. Number(stripped) is computed once as promised by the third commit's message, and the /^\d+$/ tightening cleanly rejects hex/scientific/+-prefixed input as intended.

One very minor, non-blocking observation: when one explicit var is valid-but-privileged (e.g. SHAKAPERF_CONTROL_PORT=80) and the other is malformed/invalid, parseExplicitPort still emits the "privileged range" advisory for the valid side even though the whole pair is discarded and the run falls through to automatic assignment (the privileged advisory fires independently per-var, before the pair-level validity is known). It's a slightly misleading diagnostic in that narrow case ("binding will fail without root" for a port that's never actually used), but it's cosmetic only — not worth blocking on.

Tests are thorough (covers the malformed/out-of-range/privileged/partial-pair/blank matrix) and the new jest.restoreAllMocks() in afterEach is a nice bit of hygiene given the growing number of console.warn spies in this file.

Nice, focused follow-up — LGTM.

[justin808]

Ready to merge at HEAD 0fef7665b261d636edfe5468d02a70c382dd246c.

Type: shaka-shared runtime/env-var parsing behavior plus tests.

Checks:

• GitHub current-head checks: claude-review passed; CodeRabbit status is success/skipped because automatic reviews are disabled.
• yarn workspace shaka-shared test passed: 47 tests.
• yarn workspace shaka-shared typecheck passed.
• yarn workspace shaka-shared build passed.
• yarn typecheck passed.
• yarn build passed.
• yarn test was run and still fails in unrelated packages/shaka-perf visreg specs. A representative preparePage_spec failure reproduces from a clean origin/main archive after building shaka-shared, so I am not treating that as introduced by PR 19.

Review threads:

• Greptile duplicate-warning thread: fixed and resolved.
• Claude doc-comment range thread: fixed and resolved.
• Claude lone-malformed-var duplicate-warning thread: fixed with regression coverage and resolved.

Manual QA:

• Ran compiled packages/shaka-shared/dist/assign-ports.js through yarn node with temp settings files and stubbed free ports.
• SHAKAPERF_CONTROL_PORT=40x0 + SHAKAPERF_EXPERIMENT_PORT=4001: falls back to 3040/3050, emits exactly one specific invalid-port warning.
• SHAKAPERF_EXPERIMENT_PORT=40x0 alone: falls back to 3040/3050, emits exactly one specific invalid-port warning.
• SHAKAPERF_CONTROL_PORT=4000 alone: falls back to 3040/3050, emits the generic “both vars required” warning.
• Valid pair 4000/4001: returns 4000/4001, emits no warnings, and does not persist a settings file.

Adversarial review:

• No blocking correctness, security, compatibility, performance, changelog, or review-gate findings remain.
• Current changed-file risk is limited to packages/shaka-shared/src/assign-ports.ts and its Jest coverage.
• Known non-blocking note from latest Claude review: a privileged valid side paired with an invalid counterpart can still log the privileged advisory even though the pair is discarded. Cosmetic diagnostic only; no merge blocker.

Docs-only skip: N/A.
Waivers: N/A.
Post-merge/release notes: none.