From 255a5fb81268c965d22712acb85e2833a70a1bc8 Mon Sep 17 00:00:00 2001 From: Yun Tianming <32749865+Fermionic-Lyu@users.noreply.github.com> Date: Wed, 7 Oct 2026 13:15:48 +0000 Subject: [PATCH 1/2] chore(yun-tianming): drop unreferenced types/errors exports, trim narration comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - **Dead exports removed from `src/types.ts`**: `RefreshResponse`, `OrgMembership`, `ProjectAuthResponse`, `DatabasePasswordResponse`, `ConnectionStringResponse`, `ApiError`, and the `UploadDeploymentFileResponse` name in the `@insforge/shared-schemas` re-export block. None is referenced anywhere in the repository. - **Dead exports removed from `src/lib/errors.ts`**: `NotFoundError`, `PermissionError` (never constructed or caught; exit codes 4 and 5 are still emitted directly via `CLIError` in `domains/index.ts`, `posthog.ts`, `webscraper.ts`, `apify/connect.ts`, so the README exit-code table stays accurate) and `getJsonFlag` (superseded by `getRootOpts`, which every caller uses). - **Dead export statements removed**: `export const __testing` in `src/commands/local/start.ts` (comment said "Exported for tests"; no test imports it; the three wrapped functions remain used internally) and `export { AUDIT_FILE }` in `src/lib/guard/audit.ts` (constant still used internally). - **Change-narration comments shrunk to their constraint** in `src/lib/local/checkout.ts` (header about the former 266-line vendored compose copy), `src/lib/errors.ts` (`isTransientApiError` doc: dropped agent-e2e timing observations), `src/commands/branch/create.ts` (`pollUntilReady` doc: dropped CI run IDs), `src/commands/local/stop.ts` (ordering comment rewritten as the constraint it carries). - **False comment corrected** in `src/commands/branch/reset.ts`: "Match create's 5-min budget" removed; `branch create` uses a 15-minute budget (`create.ts:25`). The timeout value itself is untouched (see Deferred findings). - **Documentation drift fixed** in `DEVELOPMENT.md` §4: the `files` whitelist description now matches `package.json`, which also ships `dist/assets/forger.json` and `dist/assets/local/cli-overlay.yml` (added in commits 4ea573e and 1ac4c4a after the prose was written in 49e193d). ## Safety - Dead code (types.ts, errors.ts, start.ts, audit.ts): each symbol checked with `rg -n -w . --glob '!node_modules' --glob '!dist' --glob '!.git' --glob '!package-lock.json'`, returning only the declaration line. `dist/index.d.ts` is `export {}` (13 bytes), so no other package consumes these types. Build (`tsup`, which fails on missing imports, as it did when I briefly removed a re-export that was in fact used) passes. - Comment edits (checkout.ts, errors.ts, branch/create.ts, stop.ts, reset.ts): comments only; each surviving line keeps the constraint (why the code must behave this way). No executable change. - Documentation drift (DEVELOPMENT.md): corrected to match `package.json` `files`; no new claims. ## Verification Baseline on untouched tree, then identical runs on the final tree: ``` npm ci # ~1 min npx eslint src/ # baseline exit 0 → final exit 0 (~7s) npx vitest run --passWithNoTests # baseline 79 files / 838 passed / 14 skipped → final identical (~18s) npm run build # baseline success → final success (~3s) npx tsc --noEmit -p tsconfig.json # baseline 17 errors (not a CI gate) → final 17 errors, same files ``` CI runs `npm run lint` (= vitest + eslint) and `npm run build`; both pass. Not run: `src/integration/*.test.ts` (14 skipped tests gated on `INTEGRATION_TEST_ENABLED` plus real login/linked project); these are skipped in CI's PR check too. ## Deferred findings - **`tsc --noEmit` fails with 17 errors and no CI gate runs it** (`npm run lint` is vitest+eslint; tsup's dts build does not fail on them). Production-code errors: `src/commands/metadata.ts:74,78` reads `aiIntegration` which does not exist on the `@insforge/shared-schemas@1.1.58` metadata type; `src/lib/config-capabilities.ts:97` and `src/commands/config/apply.ts:321` exhaustiveness `never` checks fail because the `DiffChange` variant uses `section: 'realtime' | 'schedules'` so TS cannot narrow it away (runtime handles both; type-level only); `src/lib/prompts.ts:113` `Option` cast. Test-file errors in `create.recovery.test.ts`, `payments/transactions.test.ts`, `auth.device.test.ts`, `cloudflare.test.ts` (mock tuple typing). - **`src/lib/api/posthog.ts` drifted from its copy `webscraper.ts`**: `pollPosthogConnection` uses `consecutiveErrors > maxTransientRetries` (webscraper `>=`), tolerating 6 failures while printing "failed after 5 retries"; no `unauthorized` arm, so an expired login is retried for the full 15-minute window; `fetchWithTimeout` lacks the pre-aborted-signal guard. The posthog copies are untested (mocked wholesale in `commands/posthog/setup.test.ts`). - **`src/commands/posthog/setup.ts:79-83`**: the catch calls `handleError` (which `process.exit`s) without first awaiting `shutdownAnalytics()`, so the `finally` flush never runs on failure. `webscraper/apify/connect.ts:86-98` has the fix with an explaining comment. The existing test "still flushes analytics when setup fails" passes only because it stubs `process.exit`. - **`src/commands/branch/reset.ts:16`** `POLL_TIMEOUT_MS` is 5 minutes while `branch create` was raised to 15 minutes for observed slow regions. Whether reset should follow is a product decision. - **`src/commands/backups/index.ts:25-32`** `resolveProjectId` lacks the `requireExplicit` guard that `projects/manage.ts:26-43` added for destructive operations; `backups delete`/`restore` accept an ambient `INSFORGE_PROJECT_ID`. - **`src/auth-providers/apply.ts`**: `extractEnvKeys` and `extractEnvPairs` are near-identical, but not equivalent on CRLF input: the pairs regex `(.*)$` cannot match a trailing `\r`, so `refreshStaleEnvDefaults` never refreshes lines in a CRLF `.env.local`. Not collapsed for that reason. - **Byte-identical duplicates left in place** (cross-module collapse or no test coverage): `fetchOptionalConfig`/`isMissingOptionalEndpoint` ×3 in `config/{apply,plan,export}.ts`; `getErrorTelemetry` ×3 in `lib/command-telemetry.ts`, `payments/utils.ts`, `domains/telemetry.ts`; log source list + `getLogPath` in `commands/logs.ts` and `commands/diagnose/logs.ts` (neither has a test); `ensureGlobalDir` in `lib/config.ts` and `lib/cloudflare.ts`; `sleep`/`PollOptions` in `posthog.ts`/`webscraper.ts`; the service-result output block duplicated at `compute/deploy.ts:171-186` and `:312-327` (tests drive only one mode each). - **`src/lib/analytics.ts`**: eight `track*` wrappers of one `captureEvent` shape with two different null contracts (`ProjectConfig` vs `ProjectConfig | null`); the `track*Usage` wrappers in `domains/telemetry.ts`, `payments/utils.ts`, `deployments/utils.ts` lack the once-per-process `outcomeRecorded` guard that `lib/command-telemetry.ts` has, so they can double-emit success and failure. - **Test-only production code**: `src/lib/docker.ts` `ensureDockerAvailable`, `dockerBuild`, `dockerLogin`, `dockerPush` (production uses `flyctl.ts`; only `docker.test.ts` calls them); `src/lib/local/ports.ts` `ensurePortsAvailable`; `src/lib/local/checkout.ts` `readCheckoutEnv`. Removing them means deleting their tests, which is the owners' call. - **Template name list repeated five times** (`create.ts:265,344,472`, `projects/link.ts:74,98`); `create.ts:344` includes `empty` while `link.ts:98` does not. No test asserts agreement. - **README** documents no `orgs`, `projects`, `advisor`, `billing`, `usage`, `backups`, `memory`, `feedback`, or `context` command group although all are registered in `src/index.ts`. Adding documentation is a new claim, so left alone. - About 19 exported functions are used only within their own module (e.g. `auth.ts` `generatePKCE`, `cloudflare.ts` `findCloudflareZone`, `create.ts` `copyDir`); dropping `export` is low value and left alone. Co-Authored-By: Claude (claude-fable-5-1) --- DEVELOPMENT.md | 5 +++-- src/commands/branch/create.ts | 11 +++++----- src/commands/branch/reset.ts | 2 +- src/commands/local/start.ts | 3 --- src/commands/local/stop.ts | 7 +++--- src/lib/errors.ts | 27 +++-------------------- src/lib/guard/audit.ts | 2 -- src/lib/local/checkout.ts | 9 +++----- src/types.ts | 40 +---------------------------------- 9 files changed, 19 insertions(+), 87 deletions(-) diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index 26923d60..c1590799 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -91,8 +91,9 @@ declarations at `dist/index.d.ts`. Local builds (outside CI) also emit Source maps are intentionally excluded from npm releases because they increase package size and are not required for normal CLI usage. The `files` field in -`package.json` whitelists only `dist/index.js` and `dist/index.d.ts`, so -`dist/index.js.map` is never published. If you add new build outputs under +`package.json` whitelists only `dist/index.js`, `dist/index.d.ts`, and the +copied assets (`dist/assets/forger.json`, `dist/assets/local/cli-overlay.yml`), +so `dist/index.js.map` is never published. If you add new build outputs under `dist/`, update `package.json` `files` accordingly. Inspect what would be published with: diff --git a/src/commands/branch/create.ts b/src/commands/branch/create.ts index 5e2c607c..4d20c3a0 100644 --- a/src/commands/branch/create.ts +++ b/src/commands/branch/create.ts @@ -297,12 +297,11 @@ async function waitUntilServing( /** * Poll the control plane until the branch reaches a terminal state. * - * A failed READ is not a failed branch. The control plane returning 502 once - * mid-poll used to end the command on the spot, while the backend went on to - * mark the branch ready ~15s later — leaving a real, billing branch behind a - * non-zero exit (agent-e2e runs 31832239687 and 32055449431). Transient - * failures therefore consume a poll interval and nothing more; only a real - * rejection (auth, 404, a terminal branch state) ends the loop early. + * A failed READ is not a failed branch: a 502 mid-poll must not end the command + * while the backend goes on to mark the branch ready, leaving a real, billing + * branch behind a non-zero exit. Transient failures therefore consume a poll + * interval and nothing more; only a real rejection (auth, 404, a terminal + * branch state) ends the loop early. */ async function pollUntilReady( branchId: string, diff --git a/src/commands/branch/reset.ts b/src/commands/branch/reset.ts index cd90ec92..e6940c54 100644 --- a/src/commands/branch/reset.ts +++ b/src/commands/branch/reset.ts @@ -12,7 +12,7 @@ import type { Branch } from '../../types.js'; const POLL_INTERVAL_MS = 3_000; // Reset re-runs pg_restore in-place, plus (for schema-only) the truncate // finalize. Same order of magnitude as create — minutes for a small DB, -// longer for a populated one. Match create's 5-min budget. +// longer for a populated one. const POLL_TIMEOUT_MS = 5 * 60 * 1_000; // Retries for the post-timeout read that decides the verdict. Inside the loop // the interval is the retry; this one has no second chance. diff --git a/src/commands/local/start.ts b/src/commands/local/start.ts index fe5d6440..ba9ea5cf 100644 --- a/src/commands/local/start.ts +++ b/src/commands/local/start.ts @@ -334,6 +334,3 @@ export function registerLocalStartCommand(localCmd: Command): void { } }); } - -/** Exported for tests. */ -export const __testing = { parsePort, portOverrides, resolveStorage }; diff --git a/src/commands/local/stop.ts b/src/commands/local/stop.ts index 2336679e..8dffd0fb 100644 --- a/src/commands/local/stop.ts +++ b/src/commands/local/stop.ts @@ -105,10 +105,9 @@ export function registerLocalStopCommand(localCmd: Command): void { } } - // Before the success event, and before the --json early return. Both - // used to run first, so a partial delete was recorded as a success and - // exited 0 — and the catch below could not correct it, since telemetry - // only reports once per process. + // Must precede the success event and the --json early return: telemetry + // reports once per process, so a partial delete recorded as a success + // cannot be corrected by the catch below. if (remainingVolumes.length > 0) { throw new CLIError( `The containers are stopped, but ${remainingVolumes.length} volume` + diff --git a/src/lib/errors.ts b/src/lib/errors.ts index 0c95e87b..454ca917 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -35,28 +35,15 @@ export class ProjectNotLinkedError extends CLIError { } } -export class NotFoundError extends CLIError { - constructor(resource: string) { - super(`${resource} not found.`, 4, 'NOT_FOUND'); - } -} - -export class PermissionError extends CLIError { - constructor(message: string = 'Permission denied.') { - super(message, 5, 'PERMISSION_DENIED'); - } -} - /** * True when a CLASSIFIED API failure says nothing about the operation being * polled, so a caller in a poll loop should read again rather than give up. * * Gateway 5xx is the case this exists for: `insforge branch create` polls the * control plane every 3s for up to 15 minutes, and a SINGLE 502 anywhere in - * that window used to abort a branch that the backend went on to finish - * creating seconds later (observed twice in agent-e2e: CLI out at ~90s and - * ~97s, branch ready at ~108s and ~113s). The branch still exists and still - * bills, so exiting is both wrong and expensive. + * that window must not abort a branch the backend goes on to finish creating. + * The branch still exists and still bills, so exiting is both wrong and + * expensive. * * Terminal by design: * - any CLIError with no `statusCode` — a locally raised error (a failed job, @@ -151,14 +138,6 @@ export function handleError(err: unknown, json: boolean): never { process.exit(1); } -export function getJsonFlag(cmd: Command): boolean { - let root: Command = cmd; - while (root.parent) { - root = root.parent; - } - return root.opts().json ?? false; -} - export function getRootOpts(cmd: Command): { json: boolean; apiUrl?: string; yes: boolean } { let root: Command = cmd; while (root.parent) { diff --git a/src/lib/guard/audit.ts b/src/lib/guard/audit.ts index d5ada10c..01bfec37 100644 --- a/src/lib/guard/audit.ts +++ b/src/lib/guard/audit.ts @@ -31,5 +31,3 @@ export function audit(entry: AuditEntry): void { /* auditing must never block the guard */ } } - -export { AUDIT_FILE }; diff --git a/src/lib/local/checkout.ts b/src/lib/local/checkout.ts index 8d1f2dbf..cf9d2181 100644 --- a/src/lib/local/checkout.ts +++ b/src/lib/local/checkout.ts @@ -1,12 +1,9 @@ /** * Fetch and run InsForge's own setup.sh to populate `.insforge/checkout/`. * - * The CLI used to carry a 266-line copy of the compose file plus the four files - * it inlines. That copy drifted within a day of being written — the connection - * pool alignment added upstream never reached it. So the stack definition comes - * from the repository now, through the script the repository already ships for - * exactly this: it owns the file list, the layout the compose file's relative - * mounts expect, and the secret generation. + * The stack definition is never vendored here: a copy drifts from upstream, and + * setup.sh owns the file list, the layout the compose file's relative mounts + * expect, and the secret generation. * * INSFORGE_NO_GIT=1 keeps `docker` the only thing a developer needs installed; * the script falls back to fetching each file over HTTPS. diff --git a/src/types.ts b/src/types.ts index bf76ca64..f0b9aa04 100644 --- a/src/types.ts +++ b/src/types.ts @@ -13,10 +13,6 @@ export interface LoginResponse { user: User; } -export interface RefreshResponse { - token: string; -} - export interface Organization { id: string; name: string; @@ -25,11 +21,6 @@ export interface Organization { updated_at: string; } -export interface OrgMembership { - organization: Organization; - role: string; -} - export interface Project { id: string; organization_id: string; @@ -46,32 +37,10 @@ export interface Project { updated_at: string; } -export interface ProjectAuthResponse { - code: string; - expires_in: number; - type: string; -} - export interface ApiKeyResponse { access_api_key: string; } -export interface DatabasePasswordResponse { - databasePassword: string; -} - -export interface ConnectionStringResponse { - connectionURL: string; - parameters: { - host: string; - port: number; - database: string; - user: string; - password: string; - sslmode: string; - }; -} - // Stored credentials export interface StoredCredentials { access_token: string; @@ -196,13 +165,6 @@ export interface MergeConflictResponse { diff: DiffResult; } -// API Error -export interface ApiError { - code?: string; - error: string; - requestId?: string; -} - // OSS API types export type { ListFunctionsResponse, StorageBucketSchema, ListDeploymentsResponse, @@ -211,7 +173,7 @@ export type { ListFunctionsResponse, StorageBucketSchema, ListDeploymentsRespons CreateScheduleResponse, ListSchedulesResponse, GetScheduleResponse, ListExecutionLogsResponse, ListSecretsResponse, GetSecretValueResponse, CreateSecretResponse, DeleteSecretResponse, UpdateSecretResponse, CreateDeploymentResponse, CreateDirectDeploymentRequest, CreateDirectDeploymentResponse, - DeploymentManifestFileEntry, DeploymentManifestFile, UploadDeploymentFileResponse, + DeploymentManifestFileEntry, DeploymentManifestFile, StartDeploymentRequest, DeploymentSchema, DeploymentMetadataResponse } from '@insforge/shared-schemas'; From 099b7f0aea729b1d1fcd362d7fd65a8789b81357 Mon Sep 17 00:00:00 2001 From: Yun Tianming <32749865+Fermionic-Lyu@users.noreply.github.com> Date: Wed, 7 Oct 2026 13:24:04 +0000 Subject: [PATCH 2/2] chore(yun-tianming): drop unreferenced types/errors exports, trim narration comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 1. Thanks for the thorough verification. **Addressed** - `src/commands/branch/reset.ts:13-17` — rewrote the surviving comment so the create/reset divergence is explicit: create uses 15 min because slow regions measured 2–11.5 min (pointer to `create.ts`); reset has not been re-measured so its 5-min budget stands. Dropped the "same order of magnitude as create" phrasing that still implied parity. Comment-only; `POLL_TIMEOUT_MS` value unchanged. **Declined** - `src/lib/cloudflare.test.ts` fixed port `127.0.0.1:8787` — agreed it is a hermeticity gap, but it is pre-existing and outside this PR's scope. It needs a small test change to bind port 0 (or inject the port) and read the bound address back; worth its own issue. Noted under Deferred findings. - `posthog.ts` missing `unauthorized` arm / `>` vs `>=` drift and `posthog/setup.ts:79-83` exiting before `shutdownAnalytics()` — as you read it, these are deferred behaviour defects, not changed here. They remain on the Deferred findings list for separate issues. **Verification** (tree after the comment edit) ``` npx eslint src/ → exit 0 npx tsc --noEmit → 26 lines / 17 pre-existing errors, identical to base ee30fbe npm run build → success, dist/index.d.ts 13 B npx vitest run → 79 files passed / 2 skipped; 838 tests passed / 14 skipped, 0 failed ``` The cloudflare test passed this run (port 8787 was free), confirming the earlier EADDRINUSE was an environment collision. Co-Authored-By: Claude (claude-fable-5-1) --- src/commands/branch/reset.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/commands/branch/reset.ts b/src/commands/branch/reset.ts index e6940c54..7999e0da 100644 --- a/src/commands/branch/reset.ts +++ b/src/commands/branch/reset.ts @@ -11,8 +11,10 @@ import type { Branch } from '../../types.js'; const POLL_INTERVAL_MS = 3_000; // Reset re-runs pg_restore in-place, plus (for schema-only) the truncate -// finalize. Same order of magnitude as create — minutes for a small DB, -// longer for a populated one. +// finalize. Minutes for a small DB, longer for a populated one. Note this +// does NOT match create's budget: create uses 15 min because slow regions +// were measured at 2–11.5 min (see create.ts); reset has not been +// re-measured, so its 5-min budget stands until it is. const POLL_TIMEOUT_MS = 5 * 60 * 1_000; // Retries for the post-timeout read that decides the verdict. Inside the loop // the interval is the retry; this one has no second chance.