fix: remove unused exports from public API surface#2509
Conversation
Remove export keywords from symbols that are never imported externally, reducing the public API surface and preventing unintended coupling. Symbols un-exported across 11 files: - LogsCommandOptions, StreamOptions (logs modules) - ParsedDomainList, DomainProtocol, ParsedDomain - Rule, ImageDigestKey, ParsedImageTag - AgentImageResult, ApiProxyValidationResult, FlagValidationResult, LocalhostProcessingResult - StatsFormat, SummaryFormat, LoggingOptions, loadLogsWithErrorHandling, LogFormatterOptions Skipped symbols that ARE imported externally: - SslConfig, DockerService, DockerNetwork - pid-tracker helpers (used in tests) - discoverAndSelectSource, findPolicyManifestForSource Closes #2488, closes #2492, closes #2499, closes #2500, closes #2502, closes #2504, closes #2505 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 reduces the TypeScript public API surface by un-exporting internal-only types/interfaces across the logs, domain parsing, image tag parsing, and rules subsystems, and by trimming a couple of barrel/CLI re-exports.
Changes:
- Removed
exportfrom several internal option/result/types (Rule,StreamOptions,LogFormatterOptions,DomainProtocol/ParsedDomain/ParsedDomainList,ImageDigestKey/ParsedImageTag,LogsCommandOptions,StatsFormat/SummaryFormat,LoggingOptions,loadLogsWithErrorHandling). - Updated
src/logs/index.tsto stop re-exportingLogFormatterOptionsandStreamOptions. - Updated
src/cli.tsto stop re-exporting several result types from other modules.
Show a summary per file
| File | Description |
|---|---|
| src/rules.ts | Makes Rule interface module-private to reduce exported surface. |
| src/logs/log-streamer.ts | Makes StreamOptions module-private. |
| src/logs/log-formatter.ts | Makes LogFormatterOptions module-private. |
| src/logs/index.ts | Removes barrel re-exports of LogFormatterOptions and StreamOptions. |
| src/image-tag.ts | Makes ImageDigestKey/ParsedImageTag module-private. |
| src/domain-patterns.ts | Makes DomainProtocol/ParsedDomain/ParsedDomainList module-private. |
| src/commands/logs.ts | Makes LogsCommandOptions module-private. |
| src/commands/logs-summary.ts | Makes SummaryFormat module-private. |
| src/commands/logs-stats.ts | Makes StatsFormat module-private. |
| src/commands/logs-command-helpers.ts | Makes LoggingOptions and loadLogsWithErrorHandling module-private. |
| src/cli.ts | Removes several type re-exports to reduce CLI module’s exported surface. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
src/cli.ts:64
- Similar to
AgentImageResult: this change removes theApiProxyValidationResulttype re-export fromsrc/cli.ts, butApiProxyValidationResultremains exported fromsrc/api-proxy-config.ts. Without anexportsmap, it remains part of the package’s public API via subpath imports; consider also making the interface non-exported (or restricting subpaths) if the goal is to shrink the overall API surface.
extractGhecDomainsFromServerUrl,
extractGhesDomainsFromEngineApiTarget,
resolveApiTargetsToAllowedDomains,
} from './api-proxy-config';
src/cli.ts:90
- This removes
FlagValidationResult/LocalhostProcessingResultfromsrc/cli.tsre-exports, but both interfaces are still exported fromsrc/option-parsers.ts. Because the package allows subpath imports, these types remain publicly importable unless the source declarations are also made non-exported (or subpath exports are restricted).
escapeShellArg,
joinShellArgs,
parseEnvironmentVariables,
parseVolumeMounts,
formatItem,
} from './option-parsers';
- Files reviewed: 11/11 changed files
- Comments generated: 1
| DEFAULT_GEMINI_API_TARGET, | ||
| DEFAULT_COPILOT_API_TARGET, | ||
| } from './domain-utils'; | ||
| export type { AgentImageResult } from './domain-utils'; | ||
|
|
||
|
|
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 |
…internal types Remove export keyword from AgentImageResult, ApiProxyValidationResult, FlagValidationResult, and LocalhostProcessingResult at their declaration sites (not just the re-exports in cli.ts), so they are no longer part of the public API surface even via subpath imports. Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/3faeefa6-3921-40ef-97e4-26da8b105338 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Done in 5ad83f7. Removed the
These types are used only within their own files, so no other imports were affected. TypeScript type-check and lint pass cleanly. |
🔥 Smoke Test: Copilot BYOK — PASS
Running in BYOK offline mode ( PR by @lpcox · reviewer
|
|
Smoke Tests:
Overall: PASS
|
Smoke TestTitles: "refactor: split docker-manager.ts into focused modules"; "refactor: split cli.ts into focused modules" 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.
|
🔬 Smoke Test Results
Overall: FAIL — pre-step template variables ( PR: fix: remove unused exports from public API surface — author @lpcox, reviewer
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
Chroot Version Comparison Results
Overall: ❌ Not all versions match. Python and Node.js versions differ between host and chroot environments.
|
Smoke Test: GitHub Actions Services Connectivity
Overall: FAIL —
|
Summary
Remove
exportkeywords from symbols that are never imported externally, reducing the public API surface and preventing unintended coupling across 11 files.Symbols un-exported (18 total)
LogsCommandOptionssrc/commands/logs.tsStreamOptionssrc/logs/log-streamer.ts+ re-export insrc/logs/index.tsParsedDomainListsrc/domain-patterns.tsDomainProtocol,ParsedDomainsrc/domain-patterns.tsRulesrc/rules.tsImageDigestKey,ParsedImageTagsrc/image-tag.tsAgentImageResult,ApiProxyValidationResult,FlagValidationResult,LocalhostProcessingResultsrc/cli.tsStatsFormatsrc/commands/logs-stats.tsSummaryFormatsrc/commands/logs-summary.tsLoggingOptions,loadLogsWithErrorHandlingsrc/commands/logs-command-helpers.tsLogFormatterOptionssrc/logs/log-formatter.ts+ re-export insrc/logs/index.tsSkipped (verified as imported externally)
SslConfig([Export Audit]SslConfigexported fromsrc/docker-manager.tsbut never consumed externally #2490) — imported bycontainer-lifecycle.ts,compose-generator.tsDockerService/DockerNetwork([Export Audit]DockerServiceandDockerNetworkinsrc/types.tsexported but never imported #2489) — re-exported fromtypes/index.tssrc/pid-tracker.tsleak implementation details #2491) — imported by test filesdiscoverAndSelectSource/findPolicyManifestForSource([Export Audit] Multiple log-commands modules export symbols that are only used internally #2488 partial) — imported bylogs-audit.tsVerification
npm run build✅npm test✅ (6 pre-existing failures, no new failures)Closes #2488, closes #2492, closes #2499, closes #2500, closes #2502, closes #2504, closes #2505