Refactor option parser facade to direct re-exports and unify flag validation type#3428
Conversation
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (2 files)
Coverage comparison generated by |
🔬 Smoke Test Results
Overall: FAIL — pre-step outputs ( PR: "Refactor option parser facade to direct re-exports and unify flag validation type" by
|
Smoke Test: Copilot BYOK Offline Mode
Running in BYOK offline mode ( Overall: PARTIAL — BYOK inference ✅, pre-step smoke data not injected. PR author:
|
Smoke Test Results✅ GitHub API: 2 PR entries verified PASS — Claude engine validation complete
|
There was a problem hiding this comment.
Pull request overview
This PR reduces the apparent public surface area of src/parsers/* by simplifying the src/option-parsers.ts facade to use direct re-exports (instead of wrapper functions), and deduplicates the FlagValidationResult type into a single shared definition under src/types.
Changes:
- Replaced wrapper functions in
src/option-parsers.tswith directexport { ... } from ...re-exports. - Introduced a shared
FlagValidationResulttype insrc/types/validation.tsand re-exported it fromsrc/types/index.ts; updated parser modules to use it. - Added a Jest test to assert the facade exports are direct references to underlying parser exports.
Show a summary per file
| File | Description |
|---|---|
| src/types/validation.ts | Adds shared FlagValidationResult type definition. |
| src/types/index.ts | Re-exports FlagValidationResult from the types barrel. |
| src/parsers/rate-limit-parsers.ts | Switches to importing the shared FlagValidationResult type. |
| src/parsers/host-port-parsers.ts | Switches to importing the shared FlagValidationResult type. |
| src/option-parsers.ts | Simplifies the facade to direct re-exports and removes duplicate type/wrappers. |
| src/option-parsers-reexports.test.ts | Adds guard test ensuring facade exports are direct references (no wrappers). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 0
|
✅ GitHub merged PR review: fix: switch doc-maintainer from copilot to claude engine; Optimize Security Guard workflow token usage and reduce runaway turn cost 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 Version Comparison
Result: Some versions differ between host and chroot. The
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
Smoke Test: GitHub Actions Services Connectivity
Overall: FAIL — service containers are not reachable via
|
|
Smoke test FAIL (Connectivity ❌) 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.
|
src/parsers/*exposed a broad public API that was only consumed bysrc/option-parsers.ts, while the facade itself added pass-through wrappers and duplicated validation types. This made the parser surface look externally supported when it is effectively internal behind the facade.Facade simplification
*Implimport + wrapper-function pattern insrc/option-parsers.tswith directexport { ... } from ...re-exports for parser modules.FlagValidationResultinterface from the facade.Type deduplication
src/types/validation.tswith a single sharedFlagValidationResultdefinition.src/types/index.ts.src/parsers/rate-limit-parsers.tssrc/parsers/host-port-parsers.tsBehavioral guard for facade contract
src/option-parsers-reexports.test.tsto assert the facade exports are direct references to the underlying parser functions (not wrappers).