Refactor option parser utilities into domain-focused modules with compatibility wrappers#3160
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR refactors the previously monolithic src/option-parsers.ts into domain-focused parser modules under src/parsers/, while keeping src/option-parsers.ts as a compatibility entrypoint that delegates to the new implementations.
Changes:
- Added new domain-scoped parser/util modules under
src/parsers/(rate-limit, host ports, DNS, shell escaping, volumes). - Updated
src/option-parsers.tsto delegate parsing/validation logic to the new modules while retaining existing exports. - Kept non-split option parsing logic in
src/option-parsers.ts(e.g., memory/timeout/docker host checks, formatting, etc.).
Show a summary per file
| File | Description |
|---|---|
| src/parsers/volume-parsers.ts | New parsing helpers for volume mounts (and currently env var parsing). |
| src/parsers/shell-utils.ts | New shell argument escaping/joining utilities. |
| src/parsers/rate-limit-parsers.ts | New rate limit config builder and flag validators. |
| src/parsers/host-port-parsers.ts | New host port flag validators and config application helper. |
| src/parsers/dns-parsers.ts | New DNS parsing and localhost keyword processing. |
| src/option-parsers.ts | Compatibility wrapper delegating to the new parser modules. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
src/option-parsers.ts:289
- Similarly,
processLocalhostKeywordnow exposesReturnType<typeof processLocalhostKeywordImpl>in its exported signature. With declaration emit enabled, this ties the public type surface ofsrc/option-parsers.tsto internal parser modules and reduces the intended encapsulation of the compatibility entrypoint. Consider spelling out the return shape explicitly here to keep the compatibility boundary stable.
export function processLocalhostKeyword(
allowedDomains: string[],
enableHostAccess: boolean,
allowHostPorts: string | undefined
): ReturnType<typeof processLocalhostKeywordImpl> {
return processLocalhostKeywordImpl(allowedDomains, enableHostAccess, allowHostPorts);
}
- Files reviewed: 6/6 changed files
- Comments generated: 2
| rateLimitRpm?: string; | ||
| rateLimitRph?: string; | ||
| rateLimitBytesPm?: string; | ||
| }): ReturnType<typeof buildRateLimitConfigImpl> { |
| /** | ||
| * Parses environment variables from an array of KEY=VALUE strings | ||
| */ | ||
| export function parseEnvironmentVariables( | ||
| envVars: string[] | ||
| ): { success: true; env: Record<string, string> } | { success: false; invalidVar: string } { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@copilot address review feedback |
Addressed in commit
|
🔬 Smoke Test Results
Overall: FAIL — GitHub MCP auth unavailable in this run.
|
Smoke Test Results❌ GitHub API: HTTP 401 authentication error FAIL (2/3 tests passed)
|
|
Smoke Test: Copilot BYOK (Offline) Mode — PASS ✅
Running in BYOK offline mode ( Overall: PASS (MCP read unavailable due to sandbox credentials, all functional tests passed)
|
|
✅ GitHub PR review: Refactor API proxy request path into focused guard and transport modules; fix: remove dead re-exports of WrapperConfig component types Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
Chroot Smoke Test Results
Overall: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
Smoke Test: Gemini Engine Validation
Overall Status: FAIL Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
Smoke Test Results
Overall: FAIL — service containers are not reachable from this runner environment.
|
✨ Enhancement
src/option-parsers.tshad grown into a 600+ line mixed-responsibility utility file spanning rate limits, host ports, DNS, shell escaping, and volume parsing. This change separates those concerns into focused parser modules while keeping current imports stable through the existingoption-parsersentrypoint.What does this improve?
src/parsers/:rate-limit-parsers.tshost-port-parsers.tsdns-parsers.tsshell-utils.tsvolume-parsers.tssrc/option-parsers.tsby removing “grab-bag” utility density.Why is this valuable?
main-action,network-setup,preflight,cli-options) without requiring import churn.Implementation approach:
src/parsers/*.src/option-parsers.tsinto a compatibility surface that delegates to the new modules for split domains, while retaining non-split functions (parseMemoryLimit,parseAgentTimeout,checkDockerHost,resolveDockerHostPathPrefix,parseModelMultipliersCli,formatItem,collectRulesetFile,validateSkipPullWithBuildLocal).