From 78813eb89a06dc476915a4cea7b1d7e6040eb164 Mon Sep 17 00:00:00 2001 From: Yun Tianming <32749865+Fermionic-Lyu@users.noreply.github.com> Date: Thu, 22 Oct 2026 13:10:16 +0000 Subject: [PATCH 1/3] chore(yun-tianming): correct stale doc comments, drop filename headers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - `src/lib/errors.ts:54-56`: the `isTransientApiError` doc claimed `ossFetch` does NOT wrap raw fetch rejections and that "the deployment poller" handles them. Since fbffb74, `ossFetch` tags transport failures as `CLIError(NETWORK_ERROR_CODE)` (`src/lib/api/oss.ts:248-256`), exactly like `platformFetch` (`platform.ts:106,117`). Comment now says both wrap. - `src/lib/api/platform.ts:255-256`: `streamDiagnosticAnalysis` doc said "Returns the raw Response so the caller can handle errors before streaming"; the signature is `Promise` and the only caller (`commands/diagnose/index.ts`) uses `onEvent` alone. Sentence dropped. - `src/commands/branch/create.ts:20-24`: the `POLL_TIMEOUT_MS` comment attributed the 11.5-minute measurement to the ready→host-answering gap, which is governed by `HEALTH_TIMEOUT_MS` (10 min, line 25/282), and narrated the change from 5 to 15 minutes. Replaced with the one surviving constraint: must exceed the longest observed provisioning time (11.5 min on ap-southeast, InsForge/InsForge#1790, per commit 7fa4829's message), and what breaks below it. - `src/auth-providers/apply.ts:124`: "Exported for unit testing." sat above the non-exported `MASKED_PASSWORD_PATTERN`; moved to sit above `refreshStaleEnvDefaults`, which is the export `apply.test.ts` imports. Lines 367-373: a seven-line "Two passes" comment that restated `refreshStaleEnvDefaults`' own doc and narrated "after the CLI started fetching DATABASE_URL" shrunk to two lines. - `src/commands/deployments/deploy.ts:321` and `src/lib/api/platform.ts:88`: dropped "now" change narration ("ossFetch now tags", "it is now the long-lived uak_"). - Filename header comments (`// CLI/src/...`) removed from the eight config-family files: `lib/config-{metadata,secrets,capabilities,schema}.ts`, `commands/config/{apply,export,plan,index}.ts`. Nothing else in `src` carries one. - `IMPORTANT:` prose labels dropped from `lib/config-capabilities.ts:14`, `lib/command-telemetry.ts:24`, `lib/flyctl.ts:104`; the instruction text under each is unchanged. ## Safety - All 15 files: **comment edits only**. `git diff` contains no non-comment line. Every surviving factual claim was checked against the code it describes (citations above). - The one declaration adjacent to an edit (`MASKED_PASSWORD_PATTERN`) did not move; only the comment line above it moved one line down. - No exported symbol, signature, string, default or import changed; `dist/` untouched. ## Verification Baseline on untouched HEAD (e68f9c1), then final tree: ``` npx tsc --noEmit # 17 errors on baseline (pre-existing, ungated) → 17 on final npx eslint src/ # clean → clean npx vitest run --passWithNoTests # baseline: 79 files passed, 2 skipped; 838 tests passed, 14 skipped # final: 79 files passed, 2 skipped; 838 tests passed, 14 skipped ``` `npm run lint` is `vitest run && eslint src/`; both halves were run separately so a red vitest could not mask eslint. Not run: `test:integration:real` (needs real-project cloud secrets, CI-only on `main` push), and `npm run build` (the diff is comments only; `tsc` covers type validity). ## Deferred findings Already filed in PR #308 body (not restated): `backups/index.ts` missing `requireExplicit` guard for delete/restore; `api/posthog.ts`/`webscraper.ts` retry `>` vs `>=` and missing `unauthorized` arm; `posthog/setup.ts:79-83` exits before `shutdownAnalytics`; `extractEnvPairs` CRLF-sensitive; `track*Usage` double-emit. New tonight, all behaviour changes or refactors without direct coverage, left alone: - `src/commands/projects/link.ts:357-359`: `link --project-id X` without `--org-id` skips the org picker (`:332`), leaves `orgId` undefined and writes it to `config.default_org_id`; `saveGlobalConfig` JSON-stringifies, so the user's saved default org is silently deleted. - `src/commands/projects/link.ts:104-106`: `--auth` validation throws outside the `try`, bypassing `handleError`; `index.ts` has no `unhandledRejection` handler, so a bad `--auth` prints a raw stack instead of `Error: ...` with the CLIError exit code. The sibling `--template` check at `:109` is inside the `try`. - `src/commands/functions/delete.ts:40-44` and `functions/deploy.ts:87-89`: a `result.success === false` response is printed through `outputSuccess` (`✓ Failed to delete function`), telemetry records success, exit code 0. `deploy.ts:87` also prints the non-word "updation". - `src/commands/orgs/manage.ts:276`: `orgs members remove` resolves the org from ambient `INSFORGE_ORG_ID`/linked project, while `leave` (`:120`) and `delete` (`:154`) use `requireExplicitOrgId`. With `--yes` a stray env var points the eviction at the wrong org. Same shape as the `backups` finding in #308. - `src/commands/secrets/update.ts:24-25`, `src/commands/schedules/update.ts:32`: `opts.active === 'true'` turns `--active yes`/`TRUE`/`1` into `false` with no error; `link.ts:86-89` normalises the same kind of flag. - `src/commands/docs.ts:31`: `reportCliUsage('cli.docs', true)` runs before any work and the catch never reports `false`; every sibling reports after output and `false` on failure. - `src/commands/records/{list,delete,update}.ts`: `opts.filter.split('=')` computed twice per call, triplicated; a filter with no `=` becomes `?id=` and is sent as a DELETE/PATCH predicate. No records tests exist, so the redundant split was not collapsed. - `src/lib/api/platform.ts:110-112`: when `getAccessToken()` was empty, line 73 already minted a token; a 401 then calls `refreshAccessToken` again, spending a second round trip and rotation on a seconds-old token. - `src/lib/api/platform.ts:119-127` vs `:134-141`: identical error-body parsing written twice inside `platformFetch`, already drifted in naming. No `platform.test.ts` exists, so not collapsed. - `src/lib/api/platform.ts:274-293`: SSE `currentEvent` is never reset after a dispatch, so a bare `data:` frame after `event: error` is delivered as `error`. - `src/lib/api/posthog.ts:9-24` vs `webscraper.ts:9-27`: copy-pasted `fetchWithTimeout` drifted; only webscraper has the pre-abort guard, so `pollPosthogConnection` issues one more request after the user aborts. `sleep`, `PollOptions`, both pollers and both `cli-start` functions are byte-identical duplicates across the two files. Cross-module; not collapsed. - `src/lib/api/posthog.ts:306`, `webscraper.ts:309`: `body.error ?? res.statusText ?? \`HTTP ${status}\`` — `statusText` is always a string (`''` on HTTP/2), so the last arm is unreachable and an HTTP/2 error yields an empty message. - `src/lib/api/posthog.ts:91-110`: comment says null "strictly" means no connection, but any CLIError whose message contains the substring `404` is swallowed to null, and a real connection lacking `apiKey` also returns null. - `src/lib/api/oss.ts:341`: `res.clone().json()` where `res` is never otherwise read; `res.json()` is equivalent. `probeBackendHealth` is only ever mocked in tests, so left as is. - `src/commands/deployments/deploy.ts:331`: the `formatFetchError` branch is unreachable because `isTransientApiError` returns false for every non-CLIError; removing it needs a cast or a type-predicate signature change, so left. - `src/commands/projects/link.ts:250-256` vs `:311-317`: byte-identical "agent connected" block in both direct-link branches; `:552-558` awaits `shutdownAnalytics()` in both catch and finally (the finally copy is unreachable after `handleError`'s `process.exit`). - `src/commands/branch/create.ts` commit 7fa4829's message is internally inconsistent about which phase the 11.5-minute measurement belongs to (point 1 says `branch_state` read `ready` throughout; point 2 says it was reported "still creating"). The new comment cites the issue rather than the phase. Co-Authored-By: Claude (claude-fable-5-1) --- src/auth-providers/apply.ts | 11 +++-------- src/commands/branch/create.ts | 7 ++----- src/commands/config/apply.ts | 1 - src/commands/config/export.ts | 1 - src/commands/config/index.ts | 1 - src/commands/config/plan.ts | 1 - src/commands/deployments/deploy.ts | 2 +- src/lib/api/platform.ts | 3 +-- src/lib/command-telemetry.ts | 2 +- src/lib/config-capabilities.ts | 4 +--- src/lib/config-metadata.ts | 2 -- src/lib/config-schema.ts | 2 -- src/lib/config-secrets.ts | 2 -- src/lib/errors.ts | 7 +++---- src/lib/flyctl.ts | 2 +- 15 files changed, 13 insertions(+), 35 deletions(-) diff --git a/src/auth-providers/apply.ts b/src/auth-providers/apply.ts index dba64c51..413430f7 100644 --- a/src/auth-providers/apply.ts +++ b/src/auth-providers/apply.ts @@ -121,8 +121,8 @@ export function extractEnvPairs(content: string): Map { // still have the masked URL in their .env.local. The masked value can // never authenticate to Postgres, so it's clearly stale — detect any // `:****@` (one or more `*`) password and refresh from the platform. -// Exported for unit testing. const MASKED_PASSWORD_PATTERN = /:\*+@/; +// Exported for unit testing. export function refreshStaleEnvDefaults( existing: string, manifestDefaults: Map, @@ -364,13 +364,8 @@ export async function applyAuthProvider( await fs.writeFile(envLocalPath, filled + '\n'); result.envLocalWritten = true; } else { - // Two passes: - // 1. Refresh stale platform defaults in-place. If the user's existing - // value matches the manifest's literal default AND we now have a - // real platform value (e.g., they re-linked against a cloud project - // after the CLI started fetching DATABASE_URL), update the line. - // User-customized values are preserved untouched. - // 2. Append any genuinely new keys at the end. + // Two passes: refresh stale platform defaults in place (see + // refreshStaleEnvDefaults), then append any genuinely new keys. const manifestDefaults = extractEnvPairs(manifest.envExampleAppend); const platformValues = extractEnvPairs(filled); const { updated, refreshed } = refreshStaleEnvDefaults(existingLocal, manifestDefaults, platformValues); diff --git a/src/commands/branch/create.ts b/src/commands/branch/create.ts index 4d20c3a0..299575a1 100644 --- a/src/commands/branch/create.ts +++ b/src/commands/branch/create.ts @@ -17,11 +17,8 @@ import { readBranchWithRetry } from './poll.js'; import type { Branch, BranchMode } from '../../types.js'; const POLL_INTERVAL_MS = 3_000; -// `branch_state` reaching 'ready' and the branch's own host answering are two -// different events, and the gap between them has been measured in MINUTES -// (2 min and 11.5 min on ap-southeast). A 5-minute ceiling reported the slower -// one as "still creating" when it was simply not finished yet, so the budget -// now covers the observed range with headroom. +// Must exceed the longest observed provisioning time: 11.5 min on ap-southeast +// (InsForge/InsForge#1790). Below that, a slow branch is reported as stuck. const POLL_TIMEOUT_MS = 15 * 60 * 1_000; // Once the control plane says ready, wait for the data plane too. Until this // passes, every subsequent command against the branch fails. diff --git a/src/commands/config/apply.ts b/src/commands/config/apply.ts index 1c2dc1ff..58456344 100644 --- a/src/commands/config/apply.ts +++ b/src/commands/config/apply.ts @@ -1,4 +1,3 @@ -// CLI/src/commands/config/apply.ts import type { Command } from 'commander'; import { readFileSync } from 'node:fs'; import { resolve } from 'node:path'; diff --git a/src/commands/config/export.ts b/src/commands/config/export.ts index f2076426..60129c29 100644 --- a/src/commands/config/export.ts +++ b/src/commands/config/export.ts @@ -1,4 +1,3 @@ -// CLI/src/commands/config/export.ts import type { Command } from 'commander'; import { writeFileSync, existsSync } from 'node:fs'; import { resolve } from 'node:path'; diff --git a/src/commands/config/index.ts b/src/commands/config/index.ts index a669d4f5..ebf1c0fc 100644 --- a/src/commands/config/index.ts +++ b/src/commands/config/index.ts @@ -1,4 +1,3 @@ -// CLI/src/commands/config/index.ts import type { Command } from 'commander'; import { registerConfigExportCommand } from './export.js'; import { registerConfigPlanCommand } from './plan.js'; diff --git a/src/commands/config/plan.ts b/src/commands/config/plan.ts index a6b76064..a843eccd 100644 --- a/src/commands/config/plan.ts +++ b/src/commands/config/plan.ts @@ -1,4 +1,3 @@ -// CLI/src/commands/config/plan.ts import type { Command } from 'commander'; import { readFileSync } from 'node:fs'; import { resolve } from 'node:path'; diff --git a/src/commands/deployments/deploy.ts b/src/commands/deployments/deploy.ts index 60a10ad9..fe3dbd00 100644 --- a/src/commands/deployments/deploy.ts +++ b/src/commands/deployments/deploy.ts @@ -318,7 +318,7 @@ export async function pollDeployment( // responses other than the retryable ones are terminal. Gateway 5xx // responses on the status endpoint are transient — the deployment itself // may still succeed — so keep polling, same as network-level fetch - // failures, which `ossFetch` now tags as such. Anything left unclassified + // failures, which `ossFetch` tags as such. Anything left unclassified // (a malformed status body failing `.json()`, a missing `status` field) // is a bug, not an outage: it would fail identically on every retry, so // it surfaces now instead of at the end of the poll window. diff --git a/src/lib/api/platform.ts b/src/lib/api/platform.ts index cb449cfc..ea1891c1 100644 --- a/src/lib/api/platform.ts +++ b/src/lib/api/platform.ts @@ -85,7 +85,7 @@ export async function platformFetch( const fullUrl = `${baseUrl}${path}`; if (process.env.INSFORGE_DEBUG) { console.error(`[DEBUG] ${fetchOptions.method ?? 'GET'} ${fullUrl}`); - // Redact the bearer credential. For direct-key sessions it is now the + // Redact the bearer credential. For direct-key sessions it is the // long-lived, non-rotating uak_ (not a short-lived JWT), so never write it // in full — even behind the debug flag. Match the auth header in ANY case, // since a caller-provided lowercase `authorization` survives the spread. @@ -253,7 +253,6 @@ export type DiagnosticEventHandler = (event: DiagnosticSSEEvent) => void; /** * Stream diagnostic analysis via SSE. Calls `onEvent` for each SSE event. - * Returns the raw Response so the caller can handle errors before streaming. */ export async function streamDiagnosticAnalysis( payload: DiagnosticRequest, diff --git a/src/lib/command-telemetry.ts b/src/lib/command-telemetry.ts index b4cd09fa..6a498efa 100644 --- a/src/lib/command-telemetry.ts +++ b/src/lib/command-telemetry.ts @@ -21,7 +21,7 @@ import { CLIError } from './errors.js'; * Telemetry must never affect command behavior, so everything is wrapped in a * try/catch and failures are swallowed. * - * IMPORTANT: only pass non-sensitive metadata as `properties` — counts, + * Only pass non-sensitive metadata as `properties` — counts, * booleans, enums, exit/status codes. Never SQL, file contents, credentials, * resource names, or other user-entered free text. */ diff --git a/src/lib/config-capabilities.ts b/src/lib/config-capabilities.ts index 752c6833..5d7b6447 100644 --- a/src/lib/config-capabilities.ts +++ b/src/lib/config-capabilities.ts @@ -1,5 +1,3 @@ -// CLI/src/lib/config-capabilities.ts -// // Capability detection by metadata-shape probing. // // InsForge backends evolve independently per project. A user's CLI is always @@ -13,7 +11,7 @@ // simply omit the key. The CLI infers support from presence/absence — no // version handshake, no new server endpoint. // -// IMPORTANT: probe the RAW JSON, never the Zod-parsed object. Zod's +// Probe the RAW JSON, never the Zod-parsed object. Zod's // `.default([])` on the consumer schema would silently fill in absent fields // and erase the signal we're checking for. // diff --git a/src/lib/config-metadata.ts b/src/lib/config-metadata.ts index 31f0ba02..6b6b4e73 100644 --- a/src/lib/config-metadata.ts +++ b/src/lib/config-metadata.ts @@ -1,5 +1,3 @@ -// CLI/src/lib/config-metadata.ts -// // Single source of truth for converting /api/metadata's raw JSON response // into the shapes the rest of the CLI consumes: // - liveFromMetadata → LiveConfig for the diff layer (apply, plan) diff --git a/src/lib/config-schema.ts b/src/lib/config-schema.ts index 0a0aec91..4bf2b30d 100644 --- a/src/lib/config-schema.ts +++ b/src/lib/config-schema.ts @@ -1,5 +1,3 @@ -// CLI/src/lib/config-schema.ts - import { validateSensitiveString } from './config-secrets.js'; /** diff --git a/src/lib/config-secrets.ts b/src/lib/config-secrets.ts index 24f51148..3a20344e 100644 --- a/src/lib/config-secrets.ts +++ b/src/lib/config-secrets.ts @@ -1,5 +1,3 @@ -// CLI/src/lib/config-secrets.ts -// // Sensitive-field validation for insforge.toml. // // Sensitive fields (OAuth client_secret, SMTP password, S3 secret key, etc.) diff --git a/src/lib/errors.ts b/src/lib/errors.ts index 454ca917..f1f33719 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -51,10 +51,9 @@ export class ProjectNotLinkedError extends CLIError { * budget; * - anything that is not a CLIError at all. That is an UNCLASSIFIED throw — * a `res.json()` parse failure on a mangled body, or a plain bug — and a - * poll loop must not spin on it for its whole budget. `platformFetch` - * wraps every transport and HTTP failure into a CLIError, so nothing real - * is lost. `ossFetch` does NOT wrap raw fetch rejections, so its callers - * handle that case themselves (see the deployment poller). + * poll loop must not spin on it for its whole budget. `platformFetch` and + * `ossFetch` wrap every transport and HTTP failure into a CLIError, so + * nothing real is lost. */ export function isTransientApiError(err: unknown): boolean { if (!(err instanceof CLIError)) return false; diff --git a/src/lib/flyctl.ts b/src/lib/flyctl.ts index 8b6ddb55..aef5bcdd 100644 --- a/src/lib/flyctl.ts +++ b/src/lib/flyctl.ts @@ -101,7 +101,7 @@ function ensureFlyTomlStub(opts: { dir: string; appId: string; region: string; p * and returns the resulting image ref. Does NOT launch a machine — the cloud * backend handles that step (it owns DB state and quota enforcement). * - * IMPORTANT: returns the **digest-pinned** ref `@sha256:`, not + * Returns the **digest-pinned** ref `@sha256:`, not * the tag-based `: