Skip to content

fix(scripts): reject invalid mock API ports#3072

Merged
senamakel merged 3 commits into
tinyhumansai:mainfrom
alexzhu0:codex/OH-2611-mock-api-port-arg
Jun 2, 2026
Merged

fix(scripts): reject invalid mock API ports#3072
senamakel merged 3 commits into
tinyhumansai:mainfrom
alexzhu0:codex/OH-2611-mock-api-port-arg

Conversation

@alexzhu0
Copy link
Copy Markdown
Contributor

@alexzhu0 alexzhu0 commented May 31, 2026

Summary

  • Makes scripts/mock-api-server.mjs reject malformed explicit --port / -p values.
  • Adds a help path that exits before opening a listener.
  • Adds focused Node tests that exercise argument handling without starting the mock server.

Problem

  • node scripts/mock-api-server.mjs --port nope currently falls back to the default port.
  • That can make an E2E runner start the mock backend on a different port than the caller intended.
  • In restricted environments, the resulting startup failure points at listener setup instead of the malformed argument.

Solution

  • Parse explicit port values before falling back to env/default ports.
  • Exit 2 for missing, flag-shaped, or non-integer explicit port values.
  • Keep env/default fallback behavior unchanged when no explicit port is provided.

Submission Checklist

If a section does not apply to this change, mark the item as N/A with a one-line reason. Do not delete items.

  • Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy
  • N/A: Diff coverage gate was not run because this is a root helper script change and local Rust tooling is unavailable.
  • N/A: Coverage matrix not affected; this does not add, remove, or rename a product feature.
  • N/A: No affected feature IDs; mock helper CLI only.
  • No new external network dependencies introduced (mock backend used per Testing Strategy)
  • N/A: Manual smoke checklist not affected; this does not touch release-cut surfaces.
  • N/A: Related to [Automated] Weekly code-review report — 2026-05-25 #2611 but does not close it by itself.

Impact

  • Runtime/platform impact: no product runtime change.
  • Developer/test impact: malformed mock backend port arguments now fail fast instead of silently using the default port.

Related


AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

  • Key: N/A
  • URL: N/A

Commit & Branch

  • Branch: codex/OH-2611-mock-api-port-arg
  • Commit SHA: e85596a

Validation Run

  • pnpm --filter openhuman-app format:check (blocked after app Prettier passed; see below)
  • pnpm typecheck
  • Focused tests: node --test scripts/__tests__/mock-api-server-args.test.mjs
  • Rust fmt/check (if changed): N/A, no Rust files changed
  • Tauri fmt/check (if changed): N/A, no Tauri files changed
  • node scripts/mock-api-server.mjs --help
  • node scripts/mock-api-server.mjs --port nope
  • node scripts/mock-api-server.mjs --port 65536
  • node scripts/codex-pr-preflight.mjs --lightweight
  • git diff --check

Validation Blocked

  • command: env COREPACK_HOME=/Users/alex/PR/.corepack PNPM_HOME=/Users/alex/PR/.pnpm-home pnpm --filter openhuman-app format:check
  • error: sh: cargo: command not found
  • impact: The app Prettier phase passed, but Rust formatting and the pre-push hook cannot complete in this local environment until Cargo is installed.

Behavior Changes

  • Intended behavior change: invalid explicit mock API port arguments now exit 2 before startup.
  • User-visible effect: E2E/mock-backend invocation errors are clearer for contributors.

Parity Contract

  • Legacy behavior preserved: env/default port fallback remains unchanged when no explicit port is provided.
  • Guard/fallback/dispatch parity checks: focused tests cover help, missing port, flag-shaped port, non-integer port, and out-of-range port paths.

Duplicate / Superseded PR Handling

  • Duplicate PR(s): none found during triage
  • Canonical PR: this PR
  • Resolution (closed/superseded/updated): N/A

Constraint: E2E scripts pass an explicit mock API port, so malformed values should fail before server startup.

Rejected: Fall back to the default port for invalid --port values | it hides the caller mistake and can start the server on the wrong port.

Confidence: high

Scope-risk: narrow

Directive: Treat explicit CLI configuration as authoritative; reject malformed values rather than silently falling back.

Tested: node --test scripts/__tests__/mock-api-server-args.test.mjs; node scripts/mock-api-server.mjs --help; node scripts/mock-api-server.mjs --port nope; node scripts/codex-pr-preflight.mjs --lightweight; pnpm typecheck; git diff --check

Not-tested: Full format:check/pre-push hook; cargo is not installed in this environment, though the app Prettier phase passed. Did not start the mock server because local sandbox denies listen on 127.0.0.1.
@alexzhu0 alexzhu0 requested a review from a team May 31, 2026 05:43
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 31, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

Adds structured CLI parsing to scripts/mock-api-server.mjs (usage/help and validated --port/-p parsing with specific exit codes) and a new test scripts/__tests__/mock-api-server-args.test.mjs that spawns the script to verify help output and rejection of invalid port arguments.

Changes

Mock API Server CLI Argument Parsing

Layer / File(s) Summary
CLI parsing helpers and argument validation tests
scripts/mock-api-server.mjs, scripts/__tests__/mock-api-server-args.test.mjs
readPortArg() is refactored to recognize --help/-h (printing usage and exiting 0), validate --port/-p via parsePortValue() as an integer in 1..65535, and exit with code 2 on validation errors. New test suite uses spawnSync to assert help output and that invalid --port inputs are rejected before server start.

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped through args with curious cheer,
Found --help to show and --port to clear,
Numbers checked tight, errors neatly said,
Tests spawn the script, no server ahead,
A bunny-approved CLI, crisp and near.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix(scripts): reject invalid mock API ports' directly and concisely describes the main change: adding validation to reject invalid port arguments in the mock API server script.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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 `@scripts/mock-api-server.mjs`:
- Around line 12-15: parsePortValue currently accepts any positive integer so
values >65535 (e.g., 70000) slip through; update the validation in
parsePortValue to ensure the port is within the TCP range (1–65535) by checking
Number.isInteger(port) && port > 0 && port <= 65535, and update the thrown Error
(using the existing label variable) to state the valid range so CLI fails fast
on invalid ports.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fcdc966f-f02c-42ee-9f6c-ff72345a8a36

📥 Commits

Reviewing files that changed from the base of the PR and between 4e221e9 and 3ee3bc5.

📒 Files selected for processing (2)
  • scripts/__tests__/mock-api-server-args.test.mjs
  • scripts/mock-api-server.mjs

Comment thread scripts/mock-api-server.mjs Outdated
Constraint: CodeRabbit identified that explicit mock API ports above the TCP range still reached listener startup.

Rejected: Delegate invalid high ports to Node listener errors | contributors need deterministic CLI validation before startup.

Confidence: high

Scope-risk: narrow

Directive: Keep script argument validators aligned with TCP listener bounds.

Tested: node --test scripts/__tests__/mock-api-server-args.test.mjs; node scripts/mock-api-server.mjs --port 65536; git diff --check

Not-tested: Full repository format/check; cargo is unavailable locally.
@alexzhu0
Copy link
Copy Markdown
Contributor Author

Thanks for the checks. I rechecked this PR on current state:

  • The only true blocker is Rust Core Coverage (cargo-llvm-cov) failing.
  • Other core checks in this PR pass; remaining failures are likely rerun / shared-suite related.
  • This is likely a shared upstream/core coverage issue across the fix(codex): compare strict preflight paths by realpath #3060-3073 batch, not a regression from this PR's diff.
  • PR Submission Checklist has historical failed/cancelled runs due to workflow re-triggers.

@alexzhu0
Copy link
Copy Markdown
Contributor Author

I reviewed all open PRs from alexzhu0 in this batch (#3060-3073).

Clarification on PR Submission Checklist

Most PR Submission Checklist failure/cancellation entries are historical/retry artifacts rather than new functional regressions:

  • Multiple checklist runs are expected for these PRs because re-runs happen while soft checks are triggered.
  • In the same PRs, CodeRabbit, lint/format, and most pipeline checks are passing.
  • The remaining hard blockers are concentrated in Rust Core Coverage (cargo-llvm-cov) and, for a subset, Frontend Coverage (Vitest) / Playwright web lane 1.

Suggested action

Please treat PR Submission Checklist failures for this batch as noise unless accompanied by a fresh diff-scope failure in core quality gates.

This should help avoid blocking PRs that are not functionally regressed by their own changes.

@sanil-23 sanil-23 assigned sanil-23 and unassigned sanil-23 May 31, 2026
Copy link
Copy Markdown
Contributor

@sanil-23 sanil-23 left a comment

Choose a reason for hiding this comment

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

@alexzhu0 the code looks good — the argument parsing is clean, the port range validation (CodeRabbit's finding) was already addressed in e85596a, and the focused tests cover the right rejection paths.

One CI check is failing: Rust Core Coverage (cargo-llvm-cov). This PR only touches scripts/ with no Rust changes, so the failure looks pre-existing and unrelated to these changes. Once CI is green i'll come back and approve. Let me know if you need help sorting it out.

Copy link
Copy Markdown
Contributor

@graycyrus graycyrus left a comment

Choose a reason for hiding this comment

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

Review Summary

This PR fixes a real bug where malformed --port arguments would silently fall back to the default port instead of failing fast. Good catch.

What changed: argument validation for scripts/mock-api-server.mjs — now rejects non-integer, out-of-range, or missing explicit port values with clear error messages before server startup. Adds --help flag and comprehensive Node tests.

Quality: Code is clean, focused, and correct. Tests cover the key paths: help flag, invalid ports (non-integer, out-of-range), missing ports, and flag-shaped arguments. No AI slop, proper error handling, good exit codes.

CodeRabbit dedup: CodeRabbit flagged missing upper-bound check on port validation. Already fixed in this commit — validation properly checks port <= 65535 and test includes ['--port', '65536'] to verify rejection.

No code issues — structure, logic, and edge case handling all look solid.


CI blocker: Rust Core Coverage is failing. Once that's resolved, this is ready to merge. @alexzhu0 let me know if you need help with the coverage issue.

Otherwise this looks great — clean fix, good tests, backward-compatible. Approving once CI goes green.

Copy link
Copy Markdown
Contributor

@sanil-23 sanil-23 left a comment

Choose a reason for hiding this comment

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

@alexzhu0 quick update — i dug into the Rust Core Coverage failure and it's a CI runner infrastructure issue: the action log shows "No space left on device" on the runner host. Completely unrelated to this PR (no Rust files changed). Recommend re-triggering the check run; the code here is clean and ready to go once that passes.

Copy link
Copy Markdown
Contributor

@graycyrus graycyrus left a comment

Choose a reason for hiding this comment

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

@alexzhu0 still waiting on CI to complete. The code looks good to me — once the remaining E2E and coverage jobs finish and turn green, I'll come back and approve.

Copy link
Copy Markdown
Contributor

@sanil-23 sanil-23 left a comment

Choose a reason for hiding this comment

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

@alexzhu0 picked up the merge-from-upstream commit — thanks for keeping the branch current.

CI is still not fully green on the new run:

  • Rust Core Coverage — same persistent failure as before (infrastructure issue, pre-existing on the repo, not caused by this JS-only PR).
  • E2E Playwright web lane 1/4 — this one is new, but lanes 2/4, 3/4, and 4/4 all passed in the same run. That asymmetry is a flaky-test pattern, not a real regression from this PR's argument-parsing changes.

The code itself is still clean. Once both of these checks go green (re-trigger if the flake recurs), I'll approve.

@senamakel senamakel merged commit e4cb9ab into tinyhumansai:main Jun 2, 2026
17 of 19 checks passed
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.

4 participants