Conversation
Extract validateAllowHostPorts() function from inline CLI validation and add 7 unit tests covering: error without --enable-host-access, pass with --enable-host-access, undefined ports, port ranges, and combined flag scenarios. Fixes #498 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (2 files)
Coverage comparison generated by |
There was a problem hiding this comment.
Pull request overview
This PR improves testability of the CLI’s host-port gating logic by extracting --allow-host-ports validation into a dedicated helper and adding unit coverage for the flag-combination behavior (fixing #498).
Changes:
- Extracts
validateAllowHostPorts()from inline CLI validation logic. - Adds unit tests for
validateAllowHostPorts()covering enabled/disabled/undefined scenarios and port list/range strings. - Updates CLI execution path to use the extracted validation function before continuing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/cli.ts |
Extracts and uses validateAllowHostPorts() during CLI option validation. |
src/cli.test.ts |
Adds 7 unit tests covering validateAllowHostPorts() behavior across flag combinations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (allowHostPorts && !enableHostAccess) { | ||
| return { | ||
| valid: false, | ||
| error: '--allow-host-ports requires --enable-host-access to be set', | ||
| }; | ||
| } |
There was a problem hiding this comment.
validateAllowHostPorts uses a truthy check (if (allowHostPorts && ...)). If the flag is provided as an empty string (e.g. --allow-host-ports ""), this bypasses validation even though the option was explicitly set. Consider checking allowHostPorts !== undefined (and optionally allowHostPorts.trim() !== '') instead of relying on truthiness so the constraint is enforced consistently.
| // Validate --allow-host-ports requires --enable-host-access | ||
| const hostPortsValidation = validateAllowHostPorts(config.allowHostPorts, config.enableHostAccess); | ||
| if (!hostPortsValidation.valid) { | ||
| logger.error(`❌ ${hostPortsValidation.error}`); |
There was a problem hiding this comment.
hostPortsValidation.error is optional in FlagValidationResult, but this log interpolates it without a non-null assertion or fallback. If a future refactor returns { valid: false } without an error message, this would log ❌ undefined. Consider asserting non-null (error!) or providing a default message when valid is false.
| logger.error(`❌ ${hostPortsValidation.error}`); | |
| logger.error(`❌ ${hostPortsValidation.error ?? 'Invalid --allow-host-ports configuration'}`); |
🏗️ Build Test Suite Results
Overall: 0/8 ecosystems passed — ❌ FAIL ❌ Error DetailsAll repository clones failed with the following error: Root cause: The Fix: Ensure
|
|
Smoke test results for run 22931851492 — ✅ GitHub MCP: Last 2 merged PRs: #1160 "fix(squid): block direct IP connections that bypass domain filtering", #1157 "feat: combine all build-test workflows into single build-test.md" Overall: PASS
|
Smoke Test Results
Overall: PASS
|
Chroot Version Comparison Results
Result: ❌ Not all tests passed. Python and Node.js versions differ between host and chroot environments.
|
|
PR Titles:
|
Summary
validateAllowHostPorts()function from inline CLI validation for testability--enable-host-access, pass with--enable-host-access, undefined ports, port ranges, and combined flag scenariosFixes #498
Test plan
npm run buildpassesnpm testpasses (846 tests, 7 new)npm run lintpasses (0 errors)🤖 Generated with Claude Code