fix(scripts): validate mock webview bridge port#3073
Conversation
Constraint: The mock webview bridge is a local debugging CLI and should reject malformed ports before constructing the WebSocket server. Rejected: Let ws validate NaN/out-of-range ports | it emits a low-level stack trace instead of actionable usage feedback. Confidence: high Scope-risk: narrow Directive: Validate contributor-facing script arguments before starting listeners. Tested: node --test scripts/__tests__/mock-webview-bridge-args.test.mjs; node scripts/mock-webview-bridge.mjs --help; node scripts/mock-webview-bridge.mjs 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 webview bridge because local sandbox listener behavior is restricted.
📝 WalkthroughWalkthroughThis PR adds structured command-line argument handling to ChangesCLI Argument Handling and Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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-webview-bridge.mjs`:
- Around line 14-22: The current check uses truthiness of extra which fails for
an explicit empty-string positional arg; instead compute args =
process.argv.slice(2) (or use the existing destructured [rawPort, extra]) and
reject by count: if args.length > 1 then print usage and exit with code 2; keep
the help check as if (rawPort === '--help' || rawPort === '-h') using the first
arg. Update the logic around rawPort/extra to rely on args.length rather than
truthiness to ensure cases like "" are treated as extra arguments.
🪄 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: 67a5d300-4552-4477-9785-645afb784a67
📒 Files selected for processing (2)
scripts/__tests__/mock-webview-bridge-args.test.mjsscripts/mock-webview-bridge.mjs
Constraint: CodeRabbit identified that empty-string extra arguments bypassed truthiness-based validation. Rejected: Keep checking the destructured extra value | argument presence should be determined by count, not truthiness. Confidence: high Scope-risk: narrow Directive: Validate CLI arity with argument counts before starting local listeners. Tested: node --test scripts/__tests__/mock-webview-bridge-args.test.mjs; node scripts/mock-webview-bridge.mjs 9826 ""; git diff --check Not-tested: Full repository format/check; cargo is unavailable locally.
|
Thanks for the checks. I rechecked this PR on current state:
|
|
I reviewed all open PRs from Clarification on
|
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
@alexzhu0 the code looks good — CI has one failing check that needs resolving before I can approve.
The Rust Core Coverage (cargo-llvm-cov) job is failing on inference_provider_admin_round22_raw_coverage_e2e, which is a pre-existing Rust test unrelated to your changes (this PR only touches .mjs files). Once that check goes green (or a maintainer confirms it's a pre-existing flake), I'll come back and approve.
A few notes on the implementation itself:
The CodeRabbit flag about empty-string args ('' being falsy) is already fixed in your final commit — the current code correctly uses args.length > 1 instead of checking truthiness. Good catch and clean fix.
The validation logic is correct: Number.isInteger rejects floats and NaN, the 1–65535 range check is right, and the ?? default preserves existing behavior for no-arg invocations. The help-flag ordering (check before length guard) is also correct — --help as the sole arg exits cleanly.
One minor gap: the test suite covers all invalid paths but doesn't include a happy-path case for a valid non-default port (e.g. node script.mjs 9000 should exit 0). It's not blocking — the underlying WebSocket path is unchanged — but a positive test would complete the coverage story. Up to you whether to add it here or in a follow-up.
There was a problem hiding this comment.
Code review: 0 critical, 0 major, 0 minor. This is clean.
Summary
This PR adds proper CLI argument validation to mock-webview-bridge.mjs before starting the WebSocket server. Instead of passing NaN into WebSocketServer (which produces cryptic low-level errors), the script now:
- Validates the optional port argument (must be an integer 1-65535)
- Supports
--help/-hflag - Exits with code 2 for invalid input (non-integer, out-of-range, extra args)
- Preserves the default 9826 port when no arg is supplied
Tests comprehensively cover the negative cases (help, invalid ports, out-of-range, extra args including empty strings).
Code Review
Implementation quality: Solid. The validation logic is correct—Number.isInteger() handles NaN and floats properly, the 1-65535 range is right, and help-flag ordering (checked before length guard) is correct.
Test coverage: Good for negative paths. The test suite covers help output, non-integers, boundaries (0, 65536), and the edge case of empty-string args that CodeRabbit originally flagged. Already fixed in commit d63cbc8 using args.length > 1 instead of truthiness check.
Minor observation (not blocking): No happy-path test for a valid non-default port (e.g. node script.mjs 9000). The underlying WebSocket path is unchanged, so this doesn't affect correctness, but a positive case would round out coverage. Worth adding if you're touching this again, otherwise a follow-up is fine.
AI slop: None detected. Clean, minimal, well-named functions. No excessive comments or over-engineering.
Security: No injection, no secrets, no dynamic code execution. Port validation is correct.
AI Summary
- What it does: Validates mock bridge port argument before server startup; supports
--help; exits cleanly on invalid input with code 2. - Breaking risk: Zero (dev tool only).
- Security risk: Zero.
- Bottom line: Safe to merge once CI passes.
Blocker
CI is currently failing on "Rust Core Coverage (cargo-llvm-cov)" with a cache error. This is a pre-existing infrastructure issue unrelated to this PR (which only touches .mjs files). @sanil-23 noted the same—once that clears up, this is ready to approve.
—Cyrus
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Continuation review: New test commit looks solid. The test file properly validates the --help flag and error conditions (invalid ports, extra args). Validation logic is clean and handles edge cases correctly.
Code remains good—awaiting the pending E2E and coverage checks to complete. Once those green, this is ready to go.
4 participants
Summary
scripts/mock-webview-bridge.mjsvalidate the optional positional port before starting the WebSocket server.--help/-husage output that exits without opening a listener.Problem
node scripts/mock-webview-bridge.mjs nopecurrently passesNaNintoWebSocketServer.Solution
WebSocketServer.9826behavior when no port is supplied.Submission Checklist
Impact
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
codex/OH-2611-mock-webview-port-argd63cbc8Validation Run
pnpm --filter openhuman-app format:check(blocked after app Prettier passed; see below)pnpm typechecknode --test scripts/__tests__/mock-webview-bridge-args.test.mjsnode scripts/mock-webview-bridge.mjs --helpnode scripts/mock-webview-bridge.mjs nopenode scripts/mock-webview-bridge.mjs 9826 ""node scripts/codex-pr-preflight.mjs --lightweightgit diff --checkValidation Blocked
command:env COREPACK_HOME=/Users/alex/PR/.corepack PNPM_HOME=/Users/alex/PR/.pnpm-home pnpm --filter openhuman-app format:checkerror:sh: cargo: command not foundimpact: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
Parity Contract
9826port is still used when no explicit port is supplied.Duplicate / Superseded PR Handling