Remove internal-only interfaces and inline return types in src/cli.ts#2435
Remove internal-only interfaces and inline return types in src/cli.ts#2435
src/cli.ts#2435Conversation
Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/bc9df357-7482-49d1-a6ba-892014a7c40e Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
export from internal-only interfaces in src/cli.ts
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (2 files)
Coverage comparison generated by |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR aims to narrow the exported TypeScript API of the CLI module by removing export from several result/error interfaces that are described as internal implementation details.
Changes:
- Removed
exportfromParseEnvResult,ParseEnvError,ParseVolumeMountsResult, andParseVolumeMountsErrorinsrc/cli.ts.
Show a summary per file
| File | Description |
|---|---|
| src/cli.ts | Stops exporting several parsing result/error interfaces intended to be internal-only. |
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/cli.ts:1119
- Same concern here:
ParseVolumeMountsResult/ParseVolumeMountsErrorare used in the return type of an exported function. Removingexportfrom the interfaces narrows named exports, but it also introduces a breaking change for external consumers who imported these types. If these are intended to be internal, consider also unexporting the function (or avoid referencing the named interfaces in the public signature).
interface ParseVolumeMountsResult {
success: true;
mounts: string[];
}
interface ParseVolumeMountsError {
success: false;
invalidMount: string;
reason: string;
- Files reviewed: 1/1 changed files
- Comments generated: 1
| interface ParseEnvResult { | ||
| success: true; | ||
| env: Record<string, string>; | ||
| } | ||
|
|
||
| export interface ParseEnvError { | ||
| interface ParseEnvError { | ||
| 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.
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 the review feedback |
Addressed in 4092ebb. Removed the four named interfaces entirely and replaced the return type annotations with inline discriminated-union types directly in the function signatures. No unexported names appear in the exported API surface, TypeScript narrowing still works correctly in the callers, and the functions remain exported for the test suite. |
export from internal-only interfaces in src/cli.tssrc/cli.ts
🔥 Smoke Test: Copilot BYOK (Offline) Mode
Running in BYOK offline mode ( PR author: Overall: PASS (file test inconclusive due to unresolved template variables)
|
|
Smoke Test Results ✅ PASS
|
🤖 Smoke Test Results
PR: "Remove internal-only interfaces and inline return types in Overall: PASS (MCP + connectivity verified; file test skipped due to missing pre-step data)
|
|
Smoke test summary 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 Results
Result: ❌ Not all versions match — Python and Node.js versions differ between host and chroot environments.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
Smoke Test: Services Connectivity
Overall: FAIL —
|
Four interfaces used solely as return types for internal helpers (
parseEnvironmentVariables,parseVolumeMounts) were unnecessarily exported, widening the public API surface with unstable implementation details. Additionally, having unexported interfaces referenced by name in exported function signatures is misleading to external consumers.Changes
src/cli.ts: RemovedParseEnvResult,ParseEnvError,ParseVolumeMountsResult, andParseVolumeMountsErrorentirely, replacing them with inline discriminated-union types directly in the exported function signatures.No named internal types appear in the exported API surface. TypeScript discriminant narrowing is preserved in all call sites, and the functions remain exported for test use.