Skip to content

fix(cli): validate play and present ports - #6402

Merged
miguel-heygen merged 2 commits into
heygen-com:mainfrom
Muzzy5150:fix/validate-play-present-ports
Oct 26, 2026
Merged

miguel-heygen merged 2 commits into
heygen-com:mainfrom
Muzzy5150:fix/validate-play-present-ports

Conversation

@Muzzy5150

Copy link
Copy Markdown
Contributor

What

Reject invalid --port values in hyperframes play and hyperframes present before either command resolves a project or starts a server.

Why

Both commands previously accepted malformed values such as 3003oops and silently used port 3003. They also accepted port 0, which cannot produce a reliable advertised player or presenter URL. preview already rejected the same inputs, so the local-server commands behaved inconsistently.

Root cause

play and present parsed their port strings with parseInt. JavaScript's prefix parsing accepts trailing non-digits and does not enforce the valid TCP port range.

Related work

No related issue or overlapping PR was found. This is a small self-contained correction, so no separate issue was created.

How

  • Add a shared strict parser for decimal ports from 1 through 65535.
  • Validate play and present before project resolution and server setup.
  • Reuse the parser in the existing preview and remote-debugging validation paths so the rules stay consistent.
  • Add parser boundary coverage and command-level regression tests for both affected commands.

Before and after

Before:

  • hyperframes play ... --port 3003oops --no-open started on port 3003.
  • hyperframes present ... --port 3004oops --no-open started on port 3004.
  • Port 0 passed parsing.

After:

  • Malformed, decimal, zero, negative, and out-of-range values exit with code 1.
  • The message is --port must be an integer between 1 and 65535.
  • Valid ports and existing defaults are unchanged.

Test plan

  • Unit tests added/updated
  • Manual testing performed
  • Documentation updated (not applicable: valid syntax and defaults are unchanged)
  • Comments follow CONTRIBUTING.md "Comments": they say why, not what, and a bug fix says what the code must do and how to reproduce the bug

Regression tests:

  • bun run --cwd packages/cli test src/commands/serverPort.test.ts src/utils/port.test.ts src/utils/openBrowser.test.ts src/commands/preview.test.ts: 4 files passed, 101 tests passed.
  • bun run --cwd packages/cli typecheck: passed.
  • bun run build: passed after rebasing onto current main.
  • bun run lint: passed with 0 warnings and 0 errors.
  • COMMENT_CHECK_BASE=upstream/main node scripts/check-comment-citations.mjs: passed, 7 files examined.
  • COMMENT_CHECK_BASE=upstream/main node scripts/comment-ratchet.mjs: passed, 5 changed source files.
  • Built CLI smoke test: invalid play and present values both returned exit code 1 with the expected message.

A full CLI test run was also attempted locally: 5,938 tests passed, 105 skipped, and 10 unrelated environment-sensitive tests failed. An untouched upstream worktree reproduced the stable failures involving macOS /var path canonicalization, an installed system whisper-cli, unavailable WebGL contexts, and Node deprecation output; filesystem-watcher and proxy teardown failures were intermittent under the concurrent full run.

Compatibility

The parser is platform-independent. This was executed on macOS only, so this PR makes no Windows test claim. Valid port values, default ports, browser launch behavior, and server fallback behavior are unchanged.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is a nice cleanup as well as a fix. The strict port check already existed twice, in previewPortError and parseRemoteDebuggingPort, and this PR pulls it into one parsePort helper that all three commands use, without adding another copy. Keeping preview's existing message exactly the same is a good touch.

What I checked:

  • On main, play.ts and present.ts use parseInt, so 3003oops silently became 3003 and 0/65536 passed through. args.port is only read in play, present and preview, and all three now go through parsePort.
  • The new early returns use the same clack.log.error + exit code 1 pattern these files already use for --remote-debugging-port, and they run before project resolution.
  • I broke the fix and reran the tests. Switching play back to parseInt fails all four play --port cases, and loosening parsePort (prefix match, allowing 0) fails four cases in port.test.ts. On the PR head all four touched test files pass, 101/101, three runs in a row.

One small, non-blocking nit:

  • preview.ts:313 still parses the port again with parseInt after previewPortError has validated it. It's correct today, but the same flag is parsed by two different rules. Having preview use the value from parsePort directly would leave just one parse. Fine as a follow-up.

CI has finished green at this head.

Verdict: APPROVE
Reasoning: Correct fix that consolidates the existing duplicate port validators, with tests that fail without it, and all checks pass at this head.

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 26, 2026
Merged via the queue into heygen-com:main with commit 0cc14dd Oct 26, 2026
69 checks passed
SanHsien added a commit to SanHsien/hyperframes that referenced this pull request Oct 26, 2026
Merge 20 more upstream commits (acd0bc3..ad4d3fa, v0.8.205: heygen-com#6403,
heygen-com#6363, heygen-com#6209, heygen-com#6395, heygen-com#6424, heygen-com#6426, heygen-com#6442, heygen-com#6430, heygen-com#6402, heygen-com#6435, heygen-com#6439,
heygen-com#6449, heygen-com#5436, heygen-com#6455 and docs). No conflicts. Record the 11 open PRs
heygen-com#6440-heygen-com#6458 and issue heygen-com#6443 in docs/DECISIONS.md (none adopted), re-check
the earlier open PRs, and advance the commit watermark to ad4d3fa, the
PR watermark to heygen-com#6458 and the issue watermark to heygen-com#6443.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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