From 11cc71b4f0f3211d95ec848dfdfaef07d11db288 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 3 Jun 2026 14:55:48 +0000 Subject: [PATCH] chore: drop MIGRATION-PLAN.md, gitignore scratch plans, harden patch check Three follow-ups from the PR #1057 post-merge review: - Remove MIGRATION-PLAN.md (a throwaway Bun->Node migration scratch doc that drifted stale, e.g. still referencing the old @stricli/core@1.2.5 patch) and gitignore the whole class of scratch planning/migration notes so they stay local instead of rotting in the repo. - Drop the stale `.lore.md` entry that documented the old `bun patch` SDK patch-regeneration workflow (the project uses `pnpm patch` now). - Add content assertions to script/check-patches.ts that verify each patch's *effect* is present in the installed package, not just that the version matches. The @stricli/core `-H` patch edits exact line-context in both the ESM and CJS bundles and will break on any Stricli version bump that shifts those lines; pnpm only warns (not fails) when a patch no longer applies, so a version-only check would pass while silently shipping the unpatched package. The assertion fails loudly if the `-H` removal is missing. The header also documents this fragility as a permanent maintenance cost (upstreaming a config option for it is unlikely to be accepted). --- .gitignore | 9 +++++ .lore.md | 3 -- MIGRATION-PLAN.md | 77 ------------------------------------- script/check-patches.ts | 84 +++++++++++++++++++++++++++++++++++++++-- 4 files changed, 89 insertions(+), 84 deletions(-) delete mode 100644 MIGRATION-PLAN.md diff --git a/.gitignore b/.gitignore index 0e7b4e5b6..e6499e8b4 100644 --- a/.gitignore +++ b/.gitignore @@ -77,3 +77,12 @@ opencode.json* DOCUMENTATION_GAPS.md docs/DOCUMENTATION_GAPS.md DOCUMENTATION_GAP_REPORT.md + +# Throwaway planning / migration notes (agent- or human-authored scratch docs, +# not part of the project's real documentation — these should stay local) +*-PLAN.md +*_PLAN.md +PLAN.md +*-MIGRATION.md +MIGRATION-*.md +MIGRATION_*.md diff --git a/.lore.md b/.lore.md index 1baa876fa..d4f2071f4 100644 --- a/.lore.md +++ b/.lore.md @@ -67,9 +67,6 @@ * **Preserve ApiError type so classifySilenced can silence 4xx errors**: Preserve ApiError type for classifySilenced: \`classifySilenced\` only silences \`ApiError\` with status 401-499 — wrapping in generic \`CliError\` loses \`status\` and causes 403s to be captured. Re-throw via \`new ApiError(msg, error.status, error.detail, error.endpoint)\`. \`ValidationError\` without \`field\` collapses unfielded errors into one fingerprint; always pass \`field\`. \`ApiError.enriched403=true\` set by \`throwApiError\`/\`throwRawApiError\` — command-layer code checks this to avoid double-enriching. For graceful-fallback operations, use \`withTracingSpan\` (\`onlyIfParent: true\`) and \`captureException\` with \`level: 'warning'\` for non-fatal errors. Several commands bypass telemetry by importing \`buildCommand\` from \`@stricli/core\` directly instead of \`../../lib/command.js\`. 401 errors: fix is always re-authenticate or regenerate token — scope hints do NOT apply to 401s. - -* **Sentry SDK tree-shaking patches must be regenerated via bun patch workflow**: Sentry SDK tree-shaking via bun patch: \`patchedDependencies\` in \`package.json\` strips unused exports from \`@sentry/core\` and \`@sentry/node-core\`. Non-light root of \`@sentry/node-core\` pulls uninstalled \`@opentelemetry/instrumentation\` — \*\*always import from \`@sentry/node-core/light\`\*\* (subpaths: \`.\`, \`./light\`, \`./light/otlp\`, \`./init\`, \`./loader\`, \`./import\`). No supported import for \`HttpsProxyAgent\`. Bumping SDK: remove old patches, \`rm -rf ~/.bun/install/cache/@sentry\`, \`bun install\`, \`bun patch @sentry/core\`, edit, \`bun patch --commit\`; repeat for node-core. Preserved: \`\_INTERNAL\_safeUnref\`, \`\_INTERNAL\_safeDateNow\`, \`nodeRuntimeMetricsIntegration\`. Before stripping any core export, grep \`node-core/build/{cjs,esm}/light/sdk.js\` for runtime usage (e.g. \`spanStreamingIntegration\` when \`traceLifecycle === 'stream'\`). Remove \`.bun-tag-\*\` hunks from generated patches. Manual \`git diff\` patches fail. - * **Testing Stricli command func() bodies via spyOn mocking**: Testing Stricli command func() bodies: \`const func = await cmd.loader(); func.call(mockContext, flags, ...args)\` with mock \`stdout\`, \`stderr\`, \`cwd\`, \`setContext\`. \`.call()\` LSP false-positives pass \`tsc --noEmit\`. When API functions are renamed, update both spy target AND mock return shape. \`normalizeSlug\` replaces \`\_\`→\`-\` but does NOT lowercase. Vitest: use \`vi.spyOn\` / mock fetch via \`globalThis.fetch\`. \`mock.module()\` pollutes module registry — put in \`test/isolated/\` and run via \`test:isolated\`. ALL test files MUST import from \`'vitest'\` — NEVER \`'bun:test'\`. Variadic flag pattern: \`kind: "parsed", parse: String, variadic: true, optional: true\` (see \`src/commands/explore.ts\`). Aliases: \`aliases: { s: "scope" }\` style — place INSIDE \`parameters\` (sibling to \`flags\`), NOT at top-level of \`buildCommand\` options (causes TS2353). \`-s\` is free in \`auth/login.ts\` (no conflict). diff --git a/MIGRATION-PLAN.md b/MIGRATION-PLAN.md deleted file mode 100644 index 17f471691..000000000 --- a/MIGRATION-PLAN.md +++ /dev/null @@ -1,77 +0,0 @@ -# Bun → Node.js Migration Plan - -Status: In progress. See below for completed and remaining steps. - -## Completed - -### Phase 4 (early): Package Manager Switch -- [x] Changed `packageManager` from `bun@1.3.13` to `pnpm@10.11.0` -- [x] Moved `patchedDependencies` into `pnpm` config section -- [x] Added `onlyBuiltDependencies: ["esbuild"]` -- [x] Added phantom deps as explicit devDependencies: `@sentry/core`, `@clack/prompts` -- [x] Generated `pnpm-lock.yaml` -- [x] Verified all patches apply (including cross-version: `@stricli/core@1.2.5` patch on `1.2.7`) - -### Phase 2, Group D: SQLite Adapter -- [x] Created `src/lib/db/sqlite.ts` — runtime-detecting adapter (bun:sqlite under Bun, node:sqlite under Node.js) -- [x] Updated 4 source files: `db/index.ts`, `schema.ts`, `migration.ts`, `utils.ts` -- [x] Updated 3 test files: `fix.test.ts`, `telemetry.test.ts`, `schema.test.ts` -- [x] Zero `bun:sqlite` imports remain in `src/` or `test/` - -## Remaining - -### Phase 2: Source Code Migration (replace Bun.* APIs in `src/`) - -**Group A: File I/O** — Replace `Bun.file()` / `Bun.write()` with `node:fs/promises` -- `Bun.file(path).text()` → `readFile(path, "utf-8")` -- `Bun.file(path).json()` → `readFile(path, "utf-8")` then `JSON.parse()` -- `Bun.file(path).exists()` → `access(path).then(() => true, () => false)` -- `Bun.write(path, content)` → `writeFile(path, content)` -- Scan all of `src/` for occurrences - -**Group B: Process/System APIs** — Replace Bun.which / Bun.spawn / Bun.sleep -- `Bun.which("cmd")` → `which` from a Node.js-compatible package or custom implementation -- `Bun.spawn()` / `Bun.spawnSync()` → `child_process.spawn()` / `spawnSync()` -- `Bun.sleep(ms)` → `setTimeout` promise wrapper - -**Group C: Miscellaneous Bun APIs** -- `Bun.Glob` → `tinyglobby` or `picomatch` (already in devDependencies) -- `Bun.randomUUIDv7()` → `uuidv7` package (already in devDependencies) -- `Bun.semver.order()` → `semver.compare()` (already in devDependencies) -- `Bun.zstdCompressSync()` / `Bun.zstdDecompressSync()` → Node.js zlib or `zstd-napi` package - -**Group E: Unpolyfilled APIs** -- `bspatch.ts` and `upgrade.ts` — Replace any Bun-specific APIs not covered by node-polyfills.ts - -### Phase 3: Test Migration (`bun:test` → Vitest) - -- Add `vitest` as devDependency -- Replace `import { ... } from "bun:test"` with Vitest equivalents -- Replace `bun test` scripts with `vitest` -- Key differences: - - `bun:test`'s `mock.module()` → Vitest's `vi.mock()` - - `bun:test`'s `spyOn` → Vitest's `vi.spyOn()` - - Test file discovery patterns may differ - - `--isolate --parallel` behavior needs Vitest equivalent - -### Phase 4: CI & Dev Scripts (remaining) - -- Update `package.json` scripts: `bun run` → `pnpm run` where appropriate -- Replace `bun run src/bin.ts` with `tsx src/bin.ts` (add `tsx` devDependency) -- Replace `bun run script/*.ts` with `tsx script/*.ts` -- Replace `bunx` with `pnpm exec` -- Update GitHub Actions workflows to use pnpm + Node.js instead of Bun -- Update `Dockerfile` / build scripts if applicable - -### Phase 5: Cleanup - -- Remove `@types/bun` from devDependencies -- Remove `bun.lock` (replaced by `pnpm-lock.yaml`) -- Remove or update `script/node-polyfills.ts` (may become unnecessary) -- Update `AGENTS.md` Bun API reference table -- Remove Bun-specific `.cursor/rules/bun-cli.mdc` or update for Node.js -- Clean up any remaining `Bun.*` references in comments/docs - -## Known Issues - -- `test/lib/index.test.ts` — `sdk.run throws when auth is required but missing` fails under pnpm's strict `node_modules`. The mock fetch returns empty 200s which prevents the expected auth error from being thrown. Pre-existing test fragility, not caused by migration changes. diff --git a/script/check-patches.ts b/script/check-patches.ts index 1d775892e..6529c41b7 100644 --- a/script/check-patches.ts +++ b/script/check-patches.ts @@ -12,12 +12,34 @@ * often apply cleanly across minor/patch bumps. The warning ensures engineers * notice and can regenerate the patch if needed. * + * Beyond version matching, this script runs CONTENT assertions that verify a + * patch's *effect* is actually present in the installed package. This guards + * against a class of silent failure: when a dependency is bumped and the patch + * fails to apply (line-number/context drift), pnpm only WARNS and installs the + * unpatched package — re-introducing whatever the patch fixed. A version-only + * check would still pass in that case, so content assertions are the real + * safety net. + * + * Known-fragile patch — @stricli/core (`-H` alias): + * The @stricli/core patch frees the `-H` short alias (Stricli hardcodes it + * for `--help-all`) so the `api` command can use `-H` for `--header`, + * gh-style. The patch edits exact line-context in BOTH `dist/index.js` (ESM) + * and `dist/index.cjs` (CJS). It WILL break on any @stricli/core version bump + * that shifts those lines. Upstreaming a config option for this is unlikely + * to be accepted, so the patch is a permanent maintenance cost: on every + * Stricli bump, expect to regenerate it (`pnpm patch @stricli/core`, reapply + * the four edits per file, `pnpm patch-commit`) and rename the patch file to + * the new version. This content check fails loudly if the bump silently + * dropped the patch. + * * Usage: * tsx script/check-patches.ts * * Exit codes: - * 0 - All patch versions match, or mismatches are non-critical (warnings) - * 1 - A patched package is missing entirely + * 0 - All patch versions match (or only non-critical warnings) and all + * content assertions pass + * 1 - A patched package is missing entirely, OR a content assertion failed + * (patch did not apply to the installed package) */ import { readFile } from "node:fs/promises"; @@ -58,6 +80,54 @@ for (const [name, patchPath] of Object.entries(patches)) { } } +/** + * Content assertions: verify a patch's *effect* is present in the installed + * package, not just that the version matches. Each entry checks that a stale + * (pre-patch) marker is absent from a given installed file. If the marker is + * still present, the patch did not apply and we fail hard. + * + * @stricli/core: the unpatched source registers `-H` as the reserved alias for + * `--help-all` via `checkForReservedAliases(aliases, ["h", "H"])`. After our + * patch that becomes `["h"]`. The presence of `"H"` in that call is a reliable + * signal that the patch did NOT apply (in either the ESM or CJS bundle). + */ +const CONTENT_ASSERTIONS: ReadonlyArray<{ + /** Installed file to inspect, relative to repo root. */ + file: string; + /** Stale marker that MUST be absent once the patch is applied. */ + staleMarker: string; + /** Human-readable explanation shown on failure. */ + description: string; +}> = [ + { + file: "node_modules/@stricli/core/dist/index.js", + staleMarker: 'checkForReservedAliases(aliases, ["h", "H"])', + description: + "@stricli/core ESM: -H alias not freed (api -H/--header will crash)", + }, + { + file: "node_modules/@stricli/core/dist/index.cjs", + staleMarker: 'checkForReservedAliases(aliases, ["h", "H"])', + description: + "@stricli/core CJS: -H alias not freed (api -H/--header will crash)", + }, +]; + +for (const assertion of CONTENT_ASSERTIONS) { + try { + const contents = await readFile(assertion.file, "utf-8"); + if (contents.includes(assertion.staleMarker)) { + errors.push( + ` ${assertion.description} — patch not applied to ${assertion.file} (regenerate the patch for the current dependency version)` + ); + } + } catch { + errors.push( + ` ${assertion.description} — could not read ${assertion.file} (run pnpm install)` + ); + } +} + // Emit GitHub Actions annotations for CI visibility const isCI = !!process.env.CI; for (const w of warnings) { @@ -69,13 +139,19 @@ for (const w of warnings) { } if (errors.length > 0) { - console.error("✗ Missing patched dependencies:"); + console.error("✗ Patch problems detected:"); console.error(""); for (const e of errors) { console.error(e); } console.error(""); - console.error("Run pnpm install to install missing dependencies."); + console.error( + "A missing package is fixed by `pnpm install`. A content-assertion failure" + ); + console.error( + "means the patch no longer applies (likely a dependency bump) — regenerate" + ); + console.error("it with `pnpm patch ` and re-commit."); process.exit(1); }