From f80cce6b5246d308ab779ac7e86f89c0f663689c Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Mon, 29 Dec 2025 20:37:42 +0300 Subject: [PATCH 1/8] refactor: Centralize dry-run logic with Proxy-based abstraction Replaces scattered isDryRun() checks with a centralized abstraction layer that automatically intercepts destructive operations: - Add src/utils/dryRun.ts with Proxy wrappers for SimpleGit, Octokit, and fs - Update getGitClient()/createGitClient() to return dry-run-aware git clients - Update getGitHubClient() to return dry-run-aware Octokit instance - Add dryRunFs for file write operations (writeFile, unlink, mkdir, rename) - Add dryRunExec() helper for custom destructive operations - Migrate commands and targets to use wrapped APIs - Add ESLint rules to enforce using dry-run wrapped APIs - Add comprehensive tests for proxy behavior - Document the pattern in AGENTS.md --- AGENTS.md | 44 +++ eslint.config.mjs | 19 ++ src/commands/prepare.ts | 42 +-- src/commands/publish.ts | 61 ++-- src/config.ts | 4 +- src/targets/__tests__/github.test.ts | 28 +- src/targets/awsLambdaLayer.ts | 15 +- src/targets/brew.ts | 7 +- src/targets/commitOnGitRepository.ts | 26 +- src/targets/crates.ts | 4 +- src/targets/ghPages.ts | 16 +- src/targets/github.ts | 34 +-- src/targets/hex.ts | 4 +- src/targets/pubDev.ts | 7 +- src/targets/registry.ts | 31 +- src/targets/upm.ts | 61 ++-- src/utils/__tests__/dryRun.test.ts | 279 +++++++++++++++++ src/utils/__tests__/githubApi.test.ts | 1 + src/utils/dryRun.ts | 418 ++++++++++++++++++++++++++ src/utils/gcsApi.ts | 22 +- src/utils/git.ts | 20 +- src/utils/githubApi.ts | 6 +- 22 files changed, 944 insertions(+), 205 deletions(-) create mode 100644 src/utils/__tests__/dryRun.test.ts create mode 100644 src/utils/dryRun.ts diff --git a/AGENTS.md b/AGENTS.md index f555a090b..c357e3fc4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -67,3 +67,47 @@ dist/ - Project configuration lives in `.craft.yml` at the repository root. - The configuration schema is defined in `src/schemas/`. + +## Dry-Run Mode + +Craft supports a `--dry-run` flag that prevents destructive operations. This is implemented via a centralized abstraction layer. + +### How It Works + +Instead of checking `isDryRun()` manually in every function, destructive operations are wrapped with dry-run-aware proxies: + +- **Git operations**: Use `getGitClient()` from `src/utils/git.ts` or `createGitClient(directory)` for working with specific directories +- **GitHub API**: Use `getGitHubClient()` from `src/utils/githubApi.ts` +- **File writes**: Use `dryRunFs` from `src/utils/dryRun.ts` +- **Other actions**: Use `dryRunExec()` or `dryRunExecSync()` from `src/utils/dryRun.ts` + +### ESLint Enforcement + +ESLint rules prevent direct usage of raw APIs: + +- `no-restricted-imports`: Blocks direct `simple-git` imports +- `no-restricted-syntax`: Blocks `new Octokit()` instantiation + +If you're writing a wrapper module that needs raw access, use: + +```typescript +// eslint-disable-next-line no-restricted-imports -- This is the wrapper module +import simpleGit from 'simple-git'; +``` + +### Adding New Destructive Operations + +When adding new code that performs destructive operations: + +1. **Git**: Get the git client via `getGitClient()` or `createGitClient()` - mutating methods are automatically blocked +2. **GitHub API**: Get the client via `getGitHubClient()` - `create*`, `update*`, `delete*`, `upload*` methods are automatically blocked +3. **File writes**: Use `dryRunFs.writeFile()`, `dryRunFs.unlink()`, etc. instead of raw `fs` methods +4. **Other**: Wrap with `dryRunExec(action, description)` for custom operations + +### Special Cases + +Some operations need explicit `isDryRun()` checks: + +- Commands with their own `--dry-run` flag (e.g., `dart pub publish --dry-run` in pubDev target) +- Operations that need to return mock data in dry-run mode +- User experience optimizations (e.g., skipping sleep timers) diff --git a/eslint.config.mjs b/eslint.config.mjs index bfade070f..cb16d0131 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -27,6 +27,25 @@ export default tseslint.config( ], '@typescript-eslint/no-require-imports': 'off', '@typescript-eslint/no-empty-object-type': 'off', + + // Dry-run safety: enforce using wrapped APIs that respect --dry-run flag + // Block direct calls to simpleGit() - use getGitClient() or createGitClient() instead + // Block direct instantiation of new Octokit() - use getGitHubClient() instead + 'no-restricted-syntax': [ + 'error', + { + selector: 'CallExpression[callee.name="simpleGit"]', + message: + 'Use getGitClient() or createGitClient() from src/utils/git.ts for dry-run support. ' + + 'If this is the wrapper module, disable with: // eslint-disable-next-line no-restricted-syntax', + }, + { + selector: 'NewExpression[callee.name="Octokit"]', + message: + 'Use getGitHubClient() from src/utils/githubApi.ts for dry-run support. ' + + 'If this is the wrapper module, disable with: // eslint-disable-next-line no-restricted-syntax', + }, + ], }, } ); diff --git a/src/commands/prepare.ts b/src/commands/prepare.ts index ca9a893bb..ba7e622d0 100644 --- a/src/commands/prepare.ts +++ b/src/commands/prepare.ts @@ -1,4 +1,6 @@ import { existsSync, promises as fsPromises } from 'fs'; + +import { dryRunFs } from '../utils/dryRun'; import { join, relative } from 'path'; import * as shellQuote from 'shell-quote'; import { SimpleGit, StatusResult } from 'simple-git'; @@ -213,13 +215,10 @@ async function createReleaseBranch( reportError(errorMsg, logger); } - if (!isDryRun()) { - await git.checkoutBranch(branchName, rev); - logger.info(`Created a new release branch: "${branchName}"`); - logger.info(`Switched to branch "${branchName}"`); - } else { - logger.info('[dry-run] Not creating a new release branch'); - } + // Git operations are automatically handled by the dry-run proxy + await git.checkoutBranch(branchName, rev); + logger.info(`Created a new release branch: "${branchName}"`); + logger.info(`Switched to branch "${branchName}"`); return branchName; } @@ -239,11 +238,8 @@ async function pushReleaseBranch( if (pushFlag) { logger.info(`Pushing the release branch "${branchName}"...`); // TODO check remote somehow - if (!isDryRun()) { - await git.push(remoteName, branchName, ['--set-upstream']); - } else { - logger.info('[dry-run] Not pushing the release branch.'); - } + // Git operations are automatically handled by the dry-run proxy + await git.push(remoteName, branchName, ['--set-upstream']); } else { logger.info('Not pushing the release branch.'); logger.info( @@ -271,11 +267,8 @@ async function commitNewVersion( logger.debug('Committing the release changes...'); logger.trace(`Commit message: "${message}"`); - if (!isDryRun()) { - await git.commit(message, ['--all']); - } else { - logger.info('[dry-run] Not committing the changes.'); - } + // Git operations are automatically handled by the dry-run proxy + await git.commit(message, ['--all']); } /** @@ -470,12 +463,8 @@ async function prepareChangelog( changelogString = prependChangeset(changelogString, changeset); } - if (!isDryRun()) { - await fsPromises.writeFile(relativePath, changelogString); - } else { - logger.info('[dry-run] Not updating changelog file.'); - logger.trace(`New changelog:\n${changelogString}`); - } + // File writes are automatically handled by the dry-run proxy + await dryRunFs.writeFile(relativePath, changelogString); break; default: @@ -506,11 +495,8 @@ async function switchToDefaultBranch( return; } logger.info(`Switching back to the default branch (${defaultBranch})...`); - if (!isDryRun()) { - await git.checkout(defaultBranch); - } else { - logger.info('[dry-run] Not switching branches.'); - } + // Git operations are automatically handled by the dry-run proxy + await git.checkout(defaultBranch); } interface ResolveVersionOptions { diff --git a/src/commands/publish.ts b/src/commands/publish.ts index 07ab1355f..6078366eb 100644 --- a/src/commands/publish.ts +++ b/src/commands/publish.ts @@ -1,11 +1,8 @@ import { Arguments, Argv, CommandBuilder } from 'yargs'; import chalk from 'chalk'; -import { - existsSync, - readFileSync, - writeFileSync, - promises as fsPromises, -} from 'fs'; +import { existsSync, readFileSync } from 'fs'; + +import { dryRunFs } from '../utils/dryRun'; import { join } from 'path'; import shellQuote from 'shell-quote'; import stringLength from 'string-length'; @@ -25,7 +22,7 @@ import { BaseTarget } from '../targets/base'; import { handleGlobalError, reportError } from '../utils/errors'; import { withTempDir } from '../utils/files'; import { stringToRegexp } from '../utils/filters'; -import { isDryRun, promptConfirmation } from '../utils/helpers'; +import { promptConfirmation } from '../utils/helpers'; import { formatSize } from '../utils/strings'; import { catchKeyboardInterrupt, @@ -376,25 +373,19 @@ async function handleReleaseBranch( await git.checkout(mergeTarget); logger.debug(`Merging ${branch} into: ${mergeTarget}`); - if (!isDryRun()) { - await git - .pull(remoteName, mergeTarget, ['--rebase']) - .merge(['--no-ff', '--no-edit', branch]) - .push(remoteName, mergeTarget); - } else { - logger.info('[dry-run] Not merging the release branch'); - } + // Git operations are automatically handled by the dry-run proxy + await git + .pull(remoteName, mergeTarget, ['--rebase']) + .merge(['--no-ff', '--no-edit', branch]) + .push(remoteName, mergeTarget); if (keepBranch) { logger.info('Not deleting the release branch.'); } else { logger.debug(`Deleting the release branch: ${branch}`); - if (!isDryRun()) { - await git.branch(['-D', branch]).push([remoteName, '--delete', branch]); - logger.info(`Removed the remote branch: "${branch}"`); - } else { - logger.info('[dry-run] Not deleting the remote branch'); - } + // Git operations are automatically handled by the dry-run proxy + await git.branch(['-D', branch]).push([remoteName, '--delete', branch]); + logger.info(`Removed the remote branch: "${branch}"`); } } @@ -575,9 +566,8 @@ export async function publishMain(argv: PublishOptions): Promise { for (const target of targetList) { await publishToTarget(target, newVersion, revision); publishState.published[BaseTarget.getId(target.config)] = true; - if (!isDryRun()) { - writeFileSync(publishStateFile, JSON.stringify(publishState)); - } + // File writes are automatically handled by the dry-run proxy + dryRunFs.writeFileSync(publishStateFile, JSON.stringify(publishState)); } if (argv.keepDownloads) { @@ -610,19 +600,16 @@ export async function publishMain(argv: PublishOptions): Promise { argv.mergeTarget, argv.keepBranch ); - if (!isDryRun()) { - // XXX(BYK): intentionally DO NOT await unlinking as we do not want - // to block (both in terms of waiting for IO and the success of the - // operation) finishing the publish flow on the removal of a temporary - // file. If unlinking fails, we honestly don't care, at least to fail - // the final steps. And it doesn't make sense to wait until this op - // finishes then as nothing relies on the removal of this file. - fsPromises - .unlink(publishStateFile) - .catch(err => - logger.trace("Couldn't remove publish state file: ", err) - ); - } + // XXX(BYK): intentionally DO NOT await unlinking as we do not want + // to block (both in terms of waiting for IO and the success of the + // operation) finishing the publish flow on the removal of a temporary + // file. If unlinking fails, we honestly don't care, at least to fail + // the final steps. And it doesn't make sense to wait until this op + // finishes then as nothing relies on the removal of this file. + // File operations are automatically handled by the dry-run proxy + dryRunFs + .unlink(publishStateFile) + .catch(err => logger.trace("Couldn't remove publish state file: ", err)); logger.success(`Version ${newVersion} has been published!`); } else { const msg = [ diff --git a/src/config.ts b/src/config.ts index 23124f87c..765b7db3e 100644 --- a/src/config.ts +++ b/src/config.ts @@ -3,7 +3,7 @@ import path from 'path'; import { load } from 'js-yaml'; import GitUrlParse from 'git-url-parse'; -import simpleGit from 'simple-git'; +import { createGitClient } from './utils/git'; import { ZodError } from 'zod'; import { logger } from './logger'; @@ -297,7 +297,7 @@ export async function getGlobalGitHubConfig( if (!repoGitHubConfig) { const configDir = getConfigFileDir() || '.'; - const git = simpleGit(configDir); + const git = createGitClient(configDir); let remoteUrl; try { const remotes = await git.getRemotes(true); diff --git a/src/targets/__tests__/github.test.ts b/src/targets/__tests__/github.test.ts index 0cfb16d6a..a5096020b 100644 --- a/src/targets/__tests__/github.test.ts +++ b/src/targets/__tests__/github.test.ts @@ -150,14 +150,13 @@ describe('GitHubTarget', () => { draft: true, }; - githubTarget.github.repos.deleteRelease = vi - .fn() - .mockResolvedValue({ status: 204 }); + const deleteReleaseSpy = vi.fn().mockResolvedValue({ status: 204 }); + githubTarget.github.repos.deleteRelease = deleteReleaseSpy; const result = await githubTarget.deleteRelease(draftRelease); expect(result).toBe(true); - expect(githubTarget.github.repos.deleteRelease).toHaveBeenCalledWith({ + expect(deleteReleaseSpy).toHaveBeenCalledWith({ release_id: 123, owner: 'testOwner', repo: 'testRepo', @@ -177,14 +176,13 @@ describe('GitHubTarget', () => { draft: false, }; - githubTarget.github.repos.deleteRelease = vi - .fn() - .mockResolvedValue({ status: 204 }); + const deleteReleaseSpy = vi.fn().mockResolvedValue({ status: 204 }); + githubTarget.github.repos.deleteRelease = deleteReleaseSpy; const result = await githubTarget.deleteRelease(publishedRelease); expect(result).toBe(false); - expect(githubTarget.github.repos.deleteRelease).not.toHaveBeenCalled(); + expect(deleteReleaseSpy).not.toHaveBeenCalled(); }); it('allows deletion when draft status is undefined (backwards compatibility)', async () => { @@ -194,14 +192,13 @@ describe('GitHubTarget', () => { upload_url: 'https://example.com/upload', }; - githubTarget.github.repos.deleteRelease = vi - .fn() - .mockResolvedValue({ status: 204 }); + const deleteReleaseSpy = vi.fn().mockResolvedValue({ status: 204 }); + githubTarget.github.repos.deleteRelease = deleteReleaseSpy; const result = await githubTarget.deleteRelease(releaseWithoutDraftFlag); expect(result).toBe(true); - expect(githubTarget.github.repos.deleteRelease).toHaveBeenCalled(); + expect(deleteReleaseSpy).toHaveBeenCalled(); }); it('does not delete in dry-run mode', async () => { @@ -214,14 +211,13 @@ describe('GitHubTarget', () => { draft: true, }; - githubTarget.github.repos.deleteRelease = vi - .fn() - .mockResolvedValue({ status: 204 }); + const deleteReleaseSpy = vi.fn().mockResolvedValue({ status: 204 }); + githubTarget.github.repos.deleteRelease = deleteReleaseSpy; const result = await githubTarget.deleteRelease(draftRelease); expect(result).toBe(false); - expect(githubTarget.github.repos.deleteRelease).not.toHaveBeenCalled(); + expect(deleteReleaseSpy).not.toHaveBeenCalled(); }); }); }); diff --git a/src/targets/awsLambdaLayer.ts b/src/targets/awsLambdaLayer.ts index ea0826de0..0ec2e2226 100644 --- a/src/targets/awsLambdaLayer.ts +++ b/src/targets/awsLambdaLayer.ts @@ -2,6 +2,7 @@ import * as fs from 'fs'; import * as path from 'path'; import { Octokit } from '@octokit/rest'; +// eslint-disable-next-line no-restricted-imports -- Need raw simpleGit for initial clone import simpleGit from 'simple-git'; import { getGitHubApiToken, @@ -22,6 +23,8 @@ import { import { createSymlinks } from '../utils/symlink'; import { withTempDir } from '../utils/files'; import { isDryRun } from '../utils/helpers'; +import { createGitClient } from '../utils/git'; +import { logDryRun } from '../utils/dryRun'; import { renderTemplateSafe } from '../utils/strings'; import { isPreviewRelease, parseVersion } from '../utils/version'; import { DEFAULT_REGISTRY_REMOTE } from '../utils/registry'; @@ -171,11 +174,12 @@ export class AwsLambdaLayerTarget extends BaseTarget { await withTempDir( async directory => { - const git = simpleGit(directory); this.logger.info( `Cloning ${remote.getRemoteString()} to ${directory}...` ); - await git.clone(remote.getRemoteStringWithAuth(), directory); + // eslint-disable-next-line no-restricted-syntax -- Clone needs raw simpleGit, wrapped client used after + await simpleGit().clone(remote.getRemoteStringWithAuth(), directory); + const git = createGitClient(directory); if (!isDryRun()) { await this.publishRuntimes( @@ -186,9 +190,10 @@ export class AwsLambdaLayerTarget extends BaseTarget { ); this.logger.debug('Finished publishing runtimes.'); } else { - this.logger.info('[dry-run] Not publishing new layers.'); + logDryRun('publishRuntimes(...)'); } + // Git operations are automatically handled by the dry-run proxy await git.add(['.']); await git.checkout('master'); const runtimeNames = this.config.compatibleRuntimes.map( @@ -222,7 +227,9 @@ export class AwsLambdaLayerTarget extends BaseTarget { */ private isPushableToRegistry(version: string): boolean { if (isDryRun()) { - this.logger.info('[dry-run] Not pushing the branch.'); + // Skip early - git.push() will also be blocked by the proxy but we want + // to skip the whole logic chain + logDryRun('git.push()'); return false; } if (isPreviewRelease(version) && !this.awsLambdaConfig.linkPrereleases) { diff --git a/src/targets/brew.ts b/src/targets/brew.ts index 60d689731..93f2d6f46 100644 --- a/src/targets/brew.ts +++ b/src/targets/brew.ts @@ -5,6 +5,7 @@ import { GitHubGlobalConfig, TargetConfig } from '../schemas/project_config'; import { ConfigurationError } from '../utils/errors'; import { getGitHubClient } from '../utils/githubApi'; import { isDryRun } from '../utils/helpers'; +import { logDryRun } from '../utils/dryRun'; import { renderTemplateSafe } from '../utils/strings'; import { HashAlgorithm, HashOutputFormat } from '../utils/system'; import { BaseTarget } from './base'; @@ -206,10 +207,10 @@ export class BrewTarget extends BaseTarget { `${action} file ${params.owner}/${params.repo}:${params.path} (${params.sha})` ); - if (!isDryRun()) { - await this.github.repos.createOrUpdateFileContents(params); + if (isDryRun()) { + logDryRun(`github.repos.createOrUpdateFileContents(${params.path})`); } else { - this.logger.info(`[dry-run] Skipping file action: ${action}`); + await this.github.repos.createOrUpdateFileContents(params); } this.logger.info('Homebrew release complete'); } diff --git a/src/targets/commitOnGitRepository.ts b/src/targets/commitOnGitRepository.ts index 764146744..4b94db4eb 100644 --- a/src/targets/commitOnGitRepository.ts +++ b/src/targets/commitOnGitRepository.ts @@ -1,12 +1,13 @@ +// eslint-disable-next-line no-restricted-imports -- Need raw simpleGit for initial clone import simpleGit from 'simple-git'; import { BaseArtifactProvider } from '../artifact_providers/base'; import { TargetConfig } from '../schemas/project_config'; import { ConfigurationError, reportError } from '../utils/errors'; import { withTempDir } from '../utils/files'; +import { createGitClient } from '../utils/git'; import { BaseTarget } from './base'; import childProcess from 'child_process'; import type { Consola } from 'consola'; -import { isDryRun } from '../utils/helpers'; import { URL } from 'url'; interface GitRepositoryTargetConfig { @@ -132,8 +133,6 @@ export async function pushArchiveToGitRepository({ }) { await withTempDir( async directory => { - const git = simpleGit(directory); - logger?.info(`Cloning ${repositoryUrl} into ${directory}...`); let parsedUrl; @@ -153,7 +152,9 @@ export async function pushArchiveToGitRepository({ const authenticatedUrl = parsedUrl.toString(); - await git.clone(authenticatedUrl, directory); + // eslint-disable-next-line no-restricted-syntax -- Clone needs raw simpleGit, wrapped client used after + await simpleGit().clone(authenticatedUrl, directory); + const git = createGitClient(directory); logger?.info(`Checking out branch "${branch}"...`); await git.checkout(branch); @@ -176,30 +177,23 @@ export async function pushArchiveToGitRepository({ logger?.info(`Staging files...`); await git.raw('add', '--all'); + // Git operations are automatically handled by the dry-run proxy logger?.info(`Creating commit...`); - if (!isDryRun()) { - await git.commit(`release: ${version}`); - } + await git.commit(`release: ${version}`); if (createTag) { logger?.info(`Adding a tag "${version}"...`); - if (!isDryRun()) { - await git.addTag(version); - } + await git.addTag(version); } else { logger?.info(`Not adding a tag because it was disabled.`); } logger?.info(`Pushing changes to repository...`); - if (!isDryRun()) { - await git.raw('push', authenticatedUrl, '--force'); - } + await git.raw('push', authenticatedUrl, '--force'); if (createTag) { logger?.info(`Pushing tag...`); - if (!isDryRun()) { - await git.raw('push', authenticatedUrl, '--tags'); - } + await git.raw('push', authenticatedUrl, '--tags'); } }, true, diff --git a/src/targets/crates.ts b/src/targets/crates.ts index d926a29b6..33ca85989 100644 --- a/src/targets/crates.ts +++ b/src/targets/crates.ts @@ -1,7 +1,7 @@ import * as fs from 'fs'; import * as path from 'path'; -import simpleGit from 'simple-git'; +import { createGitClient } from '../utils/git'; import { GitHubGlobalConfig, TargetConfig } from '../schemas/project_config'; import { forEachChained, sleep, withRetry } from '../utils/async'; @@ -316,7 +316,7 @@ export class CratesTarget extends BaseTarget { directory: string ): Promise { const { owner, repo } = config; - const git = simpleGit(directory); + const git = createGitClient(directory); const url = `https://github.com/${owner}/${repo}.git`; this.logger.info(`Cloning ${owner}/${repo} into ${directory}`); diff --git a/src/targets/ghPages.ts b/src/targets/ghPages.ts index c83dd7f63..86b2d17fe 100644 --- a/src/targets/ghPages.ts +++ b/src/targets/ghPages.ts @@ -2,6 +2,7 @@ import * as fs from 'fs'; import * as path from 'path'; import { Octokit } from '@octokit/rest'; +// eslint-disable-next-line no-restricted-imports -- Need raw simpleGit for initial clone import simpleGit from 'simple-git'; import { GitHubGlobalConfig, TargetConfig } from '../schemas/project_config'; @@ -12,7 +13,7 @@ import { getGitHubClient, GitHubRemote, } from '../utils/githubApi'; -import { isDryRun } from '../utils/helpers'; +import { createGitClient } from '../utils/git'; import { extractZipArchive } from '../utils/system'; import { BaseTarget } from './base'; import { BaseArtifactProvider } from '../artifact_providers/base'; @@ -150,8 +151,9 @@ export class GhPagesTarget extends BaseTarget { this.logger.info( `Cloning "${remote.getRemoteString()}" to "${directory}"...` ); + // eslint-disable-next-line no-restricted-syntax -- Clone needs raw simpleGit, wrapped client used after await simpleGit().clone(remote.getRemoteStringWithAuth(), directory); - const git = simpleGit(directory); + const git = createGitClient(directory); this.logger.debug(`Checking out branch: "${branch}"`); try { await git.checkout([branch]); @@ -180,17 +182,13 @@ export class GhPagesTarget extends BaseTarget { // Extract the archive await this.extractAssets(archivePath, directory); - // Commit + // Commit - git operations are automatically handled by the dry-run proxy await git.add(['.']); await git.commit(`craft(gh-pages): update, version "${version}"`); - // Push! + // Push! - git operations are automatically handled by the dry-run proxy this.logger.info(`Pushing branch "${branch}"...`); - if (!isDryRun()) { - await git.push('origin', branch, ['--set-upstream']); - } else { - this.logger.info('[dry-run] Not pushing the branch.'); - } + await git.push('origin', branch, ['--set-upstream']); } /** diff --git a/src/targets/github.ts b/src/targets/github.ts index 089b6e7b0..cfab78ebb 100644 --- a/src/targets/github.ts +++ b/src/targets/github.ts @@ -15,6 +15,7 @@ import { } from '../utils/changelog'; import { getGitHubClient } from '../utils/githubApi'; import { isDryRun } from '../utils/helpers'; +import { logDryRun } from '../utils/dryRun'; import { isPreviewRelease, parseVersion, @@ -133,7 +134,7 @@ export class GitHubTarget extends BaseTarget { this.githubConfig.previewReleases && isPreviewRelease(version); if (isDryRun()) { - this.logger.info(`[dry-run] Not creating the draft release`); + logDryRun(`github.repos.createRelease(${tag})`); return { id: 0, tag_name: tag, @@ -186,7 +187,7 @@ export class GitHubTarget extends BaseTarget { ): Promise { this.logger.debug(`Deleting asset: "${asset.name}"...`); if (isDryRun()) { - this.logger.info(`[dry-run] Not deleting "${asset.name}"`); + logDryRun(`github.repos.deleteReleaseAsset(${asset.name})`); return false; } @@ -220,7 +221,7 @@ export class GitHubTarget extends BaseTarget { } if (isDryRun()) { - this.logger.info(`[dry-run] Not deleting release "${release.tag_name}"`); + logDryRun(`github.repos.deleteRelease(${release.tag_name})`); return false; } @@ -291,7 +292,7 @@ export class GitHubTarget extends BaseTarget { const name = basename(path); if (isDryRun()) { - this.logger.info(`[dry-run] Not uploading asset "${name}"`); + logDryRun(`github.repos.uploadReleaseAsset(${name})`); return; } @@ -380,7 +381,7 @@ export class GitHubTarget extends BaseTarget { options: { makeLatest: boolean } = { makeLatest: true } ) { if (isDryRun()) { - this.logger.info(`[dry-run] Not publishing the draft release`); + logDryRun(`github.repos.updateRelease(${release.tag_name})`); return; } @@ -409,16 +410,17 @@ export class GitHubTarget extends BaseTarget { const tag = versionToTag(version, this.githubConfig.tagPrefix); const tagRef = `refs/tags/${tag}`; if (isDryRun()) { - this.logger.info(`[dry-run] Not pushing the tag reference: "${tagRef}"`); - } else { - this.logger.info(`Pushing the tag reference: "${tagRef}"...`); - await this.github.rest.git.createRef({ - owner: this.githubConfig.owner, - repo: this.githubConfig.repo, - ref: tagRef, - sha: revision, - }); + logDryRun(`github.git.createRef(${tagRef})`); + return; } + + this.logger.info(`Pushing the tag reference: "${tagRef}"...`); + await this.github.rest.git.createRef({ + owner: this.githubConfig.owner, + repo: this.githubConfig.repo, + ref: tagRef, + sha: revision, + }); } /** @@ -466,9 +468,7 @@ export class GitHubTarget extends BaseTarget { const tagRef = `refs/tags/${tag}`; if (isDryRun()) { - this.logger.info( - `[dry-run] Not updating floating tag: "${tag}" (from pattern "${pattern}")` - ); + logDryRun(`github.git.updateRef(tags/${tag})`); continue; } diff --git a/src/targets/hex.ts b/src/targets/hex.ts index acc194321..ee415bcef 100644 --- a/src/targets/hex.ts +++ b/src/targets/hex.ts @@ -1,4 +1,4 @@ -import simpleGit from 'simple-git'; +import { createGitClient } from '../utils/git'; import { BaseTarget } from './base'; import { withTempDir } from '../utils/files'; @@ -66,7 +66,7 @@ export class HexTarget extends BaseTarget { directory: string ): Promise { const { owner, repo } = config; - const git = simpleGit(directory); + const git = createGitClient(directory); const url = `https://github.com/${owner}/${repo}.git`; this.logger.info(`Cloning ${owner}/${repo} into ${directory}`); diff --git a/src/targets/pubDev.ts b/src/targets/pubDev.ts index 202b25849..ec882faf8 100644 --- a/src/targets/pubDev.ts +++ b/src/targets/pubDev.ts @@ -2,7 +2,7 @@ import { constants, promises as fsPromises } from 'fs'; import { homedir, platform } from 'os'; import { join, dirname } from 'path'; import { load, dump } from 'js-yaml'; -import simpleGit from 'simple-git'; +import { createGitClient } from '../utils/git'; import { BaseTarget } from './base'; import { BaseArtifactProvider } from '../artifact_providers/base'; import { GitHubGlobalConfig, TargetConfig } from '../schemas/project_config'; @@ -11,6 +11,7 @@ import { checkEnvForPrerequisite } from '../utils/env'; import { withTempDir } from '../utils/files'; import { checkExecutableIsPresent, spawnProcess } from '../utils/system'; import { isDryRun } from '../utils/helpers'; +import { logDryRun } from '../utils/dryRun'; export const targetSecrets = [ 'PUBDEV_ACCESS_TOKEN', @@ -124,7 +125,7 @@ export class PubDevTarget extends BaseTarget { public async publish(_version: string, revision: string): Promise { // `dart pub publish --dry-run` can be run without any credentials if (isDryRun()) { - this.logger.info('[dry-run] Skipping credentials file creation.'); + logDryRun('createCredentialsFile()'); } else { await this.createCredentialsFile(); } @@ -188,7 +189,7 @@ export class PubDevTarget extends BaseTarget { directory: string ): Promise { const { owner, repo } = config; - const git = simpleGit(directory); + const git = createGitClient(directory); const url = `https://github.com/${owner}/${repo}.git`; this.logger.info(`Cloning ${owner}/${repo} into ${directory}`); diff --git a/src/targets/registry.ts b/src/targets/registry.ts index c7994c99f..3592e4cc2 100644 --- a/src/targets/registry.ts +++ b/src/targets/registry.ts @@ -1,5 +1,6 @@ import { mapLimit } from 'async'; import { Octokit } from '@octokit/rest'; +// eslint-disable-next-line no-restricted-imports -- Need raw simpleGit for initial clone import simpleGit, { SimpleGit } from 'simple-git'; import { GitHubGlobalConfig, TargetConfig } from '../schemas/project_config'; @@ -30,7 +31,7 @@ import { updateManifestSymlinks, RegistryPackageType, } from '../utils/registry'; -import { isDryRun } from '../utils/helpers'; +import { createGitClient } from '../utils/git'; import { filterAsync, withRetry } from '../utils/async'; /** "registry" target options */ @@ -428,15 +429,15 @@ export class RegistryTarget extends BaseTarget { const remote = this.remote; remote.setAuth(getGitHubApiToken()); - const git = simpleGit(directory); this.logger.info( `Cloning "${remote.getRemoteString()}" to "${directory}"...` ); - await git.clone(remote.getRemoteStringWithAuth(), directory, [ + // eslint-disable-next-line no-restricted-syntax -- Clone needs raw simpleGit, wrapped client used after + await simpleGit().clone(remote.getRemoteStringWithAuth(), directory, [ '--filter=tree:0', '--single-branch', ]); - return git; + return createGitClient(directory); } public async getValidItems( @@ -499,24 +500,20 @@ export class RegistryTarget extends BaseTarget { ) ); - // Commit + // Commit - git operations are automatically handled by the dry-run proxy await localRepo.git .add(['.']) .commit( `craft: release "${this.githubRepo.repo}", version "${version}"` ); - // Push! - if (!isDryRun()) { - this.logger.info(`Pushing the changes...`); - // Ensure we are still up to date with upstream - await withRetry(() => - localRepo.git - .pull('origin', 'master', ['--rebase']) - .push('origin', 'master') - ); - } else { - this.logger.info('[dry-run] Not pushing the changes.'); - } + // Push! - git operations are automatically handled by the dry-run proxy + this.logger.info(`Pushing the changes...`); + // Ensure we are still up to date with upstream + await withRetry(() => + localRepo.git + .pull('origin', 'master', ['--rebase']) + .push('origin', 'master') + ); }, true, 'craft-release-registry-' diff --git a/src/targets/upm.ts b/src/targets/upm.ts index 1529aaa22..d4c9569a8 100644 --- a/src/targets/upm.ts +++ b/src/targets/upm.ts @@ -1,4 +1,5 @@ import { Octokit } from '@octokit/rest'; +// eslint-disable-next-line no-restricted-imports -- Need raw simpleGit for initial clone import simpleGit from 'simple-git'; import { getGitHubApiToken, @@ -16,7 +17,7 @@ import { import { reportError } from '../utils/errors'; import { extractZipArchive } from '../utils/system'; import { withTempDir } from '../utils/files'; -import { isDryRun } from '../utils/helpers'; +import { createGitClient } from '../utils/git'; import { isPreviewRelease } from '../utils/version'; import { NoneArtifactProvider } from '../artifact_providers/none'; @@ -116,9 +117,9 @@ export class UpmTarget extends BaseTarget { await withTempDir( async directory => { - const git = simpleGit(directory); - this.logger.info(`Cloning ${remoteAddr} to ${directory}...`); - await git.clone(remote.getRemoteStringWithAuth(), directory); + // eslint-disable-next-line no-restricted-syntax -- Clone needs raw simpleGit, wrapped client used after + await simpleGit().clone(remote.getRemoteStringWithAuth(), directory); + const git = createGitClient(directory); this.logger.info('Clearing the repository.'); await git.rm(['-r', '-f', '.']); @@ -128,6 +129,7 @@ export class UpmTarget extends BaseTarget { this.logger.info('Adding files to repository.'); await git.add(['.']); + // Git operations are automatically handled by the dry-run proxy const commitResult = await git.commit(`release ${version}`); if (!commitResult.commit) { throw new Error( @@ -136,35 +138,32 @@ export class UpmTarget extends BaseTarget { } const targetRevision = await git.revparse([commitResult.commit]); - if (isDryRun()) { - this.logger.info('[dry-run]: git push origin main'); - } else { - await git.push(['origin', 'main']); - const changes = await this.githubTarget.getChangelog(version); - const isPrerelease = isPreviewRelease(version); - const draftRelease = await this.githubTarget.createDraftRelease( - version, - targetRevision, - changes - ); + // Git operations are automatically handled by the dry-run proxy + await git.push(['origin', 'main']); + const changes = await this.githubTarget.getChangelog(version); + const isPrerelease = isPreviewRelease(version); + const draftRelease = await this.githubTarget.createDraftRelease( + version, + targetRevision, + changes + ); + try { + await this.githubTarget.publishRelease(draftRelease, { + makeLatest: !isPrerelease, + }); + } catch (error) { + // Clean up the orphaned draft release try { - await this.githubTarget.publishRelease(draftRelease, { - makeLatest: !isPrerelease, - }); - } catch (error) { - // Clean up the orphaned draft release - try { - await this.githubTarget.deleteRelease(draftRelease); - this.logger.info( - `Deleted orphaned draft release: ${draftRelease.tag_name}` - ); - } catch (deleteError) { - this.logger.warn( - `Failed to delete orphaned draft release: ${deleteError}` - ); - } - throw error; + await this.githubTarget.deleteRelease(draftRelease); + this.logger.info( + `Deleted orphaned draft release: ${draftRelease.tag_name}` + ); + } catch (deleteError) { + this.logger.warn( + `Failed to delete orphaned draft release: ${deleteError}` + ); } + throw error; } }, true, diff --git a/src/utils/__tests__/dryRun.test.ts b/src/utils/__tests__/dryRun.test.ts new file mode 100644 index 000000000..b3f7c7bfd --- /dev/null +++ b/src/utils/__tests__/dryRun.test.ts @@ -0,0 +1,279 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import * as helpers from '../helpers'; + +// Mock the helpers module to control isDryRun +vi.mock('../helpers', async () => { + const actual = await vi.importActual('../helpers'); + return { + ...actual, + isDryRun: vi.fn(() => false), + }; +}); + +// Mock the logger +vi.mock('../../logger', () => ({ + logger: { + info: vi.fn(), + debug: vi.fn(), + trace: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + }, +})); + +import { + createDryRunGit, + createDryRunOctokit, + dryRunFs, + dryRunExec, + dryRunExecSync, + logDryRun, +} from '../dryRun'; +import { logger } from '../../logger'; + +describe('dryRun utilities', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe('logDryRun', () => { + it('logs with consistent format', () => { + logDryRun('test operation'); + expect(logger.info).toHaveBeenCalledWith( + '[dry-run] Would execute: test operation' + ); + }); + }); + + describe('createDryRunGit', () => { + const mockGit = { + push: vi.fn().mockResolvedValue(undefined), + commit: vi.fn().mockResolvedValue({ commit: 'abc123' }), + checkout: vi.fn().mockResolvedValue(undefined), + status: vi.fn().mockResolvedValue({ current: 'main' }), + log: vi.fn().mockResolvedValue({ all: [] }), + raw: vi.fn().mockResolvedValue(''), + revparse: vi.fn().mockResolvedValue('abc123'), + }; + + it('passes through non-mutating methods in normal mode', async () => { + vi.mocked(helpers.isDryRun).mockReturnValue(false); + const git = createDryRunGit(mockGit as any); + + await git.status(); + expect(mockGit.status).toHaveBeenCalled(); + + await git.log(); + expect(mockGit.log).toHaveBeenCalled(); + }); + + it('executes mutating methods in normal mode', async () => { + vi.mocked(helpers.isDryRun).mockReturnValue(false); + const git = createDryRunGit(mockGit as any); + + await git.push(); + expect(mockGit.push).toHaveBeenCalled(); + + await git.commit('test'); + expect(mockGit.commit).toHaveBeenCalledWith('test'); + }); + + it('blocks mutating methods in dry-run mode', async () => { + vi.mocked(helpers.isDryRun).mockReturnValue(true); + const git = createDryRunGit(mockGit as any); + + mockGit.push.mockClear(); + await git.push(); + expect(mockGit.push).not.toHaveBeenCalled(); + expect(logger.info).toHaveBeenCalledWith( + expect.stringContaining('[dry-run]') + ); + }); + + it('blocks git.raw() for mutating commands in dry-run mode', async () => { + vi.mocked(helpers.isDryRun).mockReturnValue(true); + const git = createDryRunGit(mockGit as any); + + mockGit.raw.mockClear(); + await git.raw('push', 'origin', 'main'); + expect(mockGit.raw).not.toHaveBeenCalled(); + expect(logger.info).toHaveBeenCalledWith( + expect.stringContaining('git push origin main') + ); + }); + + it('allows git.raw() for non-mutating commands in dry-run mode', async () => { + vi.mocked(helpers.isDryRun).mockReturnValue(true); + const git = createDryRunGit(mockGit as any); + + mockGit.raw.mockClear(); + await git.raw('status'); + expect(mockGit.raw).toHaveBeenCalledWith('status'); + }); + + it('passes through read-only methods in dry-run mode', async () => { + vi.mocked(helpers.isDryRun).mockReturnValue(true); + const git = createDryRunGit(mockGit as any); + + await git.status(); + expect(mockGit.status).toHaveBeenCalled(); + + await git.revparse('HEAD'); + expect(mockGit.revparse).toHaveBeenCalledWith('HEAD'); + }); + }); + + describe('createDryRunOctokit', () => { + const mockOctokit = { + repos: { + createRelease: vi.fn().mockResolvedValue({ data: { id: 1 } }), + getContent: vi.fn().mockResolvedValue({ data: {} }), + updateRelease: vi.fn().mockResolvedValue({ data: {} }), + deleteReleaseAsset: vi.fn().mockResolvedValue({ status: 204 }), + }, + rest: { + git: { + createRef: vi.fn().mockResolvedValue({ data: {} }), + }, + }, + }; + + it('passes through read methods in normal mode', async () => { + vi.mocked(helpers.isDryRun).mockReturnValue(false); + const octokit = createDryRunOctokit(mockOctokit as any); + + await octokit.repos.getContent({ owner: 'test', repo: 'test', path: '/' }); + expect(mockOctokit.repos.getContent).toHaveBeenCalled(); + }); + + it('executes mutating methods in normal mode', async () => { + vi.mocked(helpers.isDryRun).mockReturnValue(false); + const octokit = createDryRunOctokit(mockOctokit as any); + + await octokit.repos.createRelease({ + owner: 'test', + repo: 'test', + tag_name: 'v1.0.0', + }); + expect(mockOctokit.repos.createRelease).toHaveBeenCalled(); + }); + + it('blocks mutating methods in dry-run mode', async () => { + vi.mocked(helpers.isDryRun).mockReturnValue(true); + const octokit = createDryRunOctokit(mockOctokit as any); + + mockOctokit.repos.createRelease.mockClear(); + await octokit.repos.createRelease({ + owner: 'test', + repo: 'test', + tag_name: 'v1.0.0', + }); + expect(mockOctokit.repos.createRelease).not.toHaveBeenCalled(); + expect(logger.info).toHaveBeenCalledWith( + expect.stringContaining('[dry-run]') + ); + }); + + it('blocks nested mutating methods in dry-run mode', async () => { + vi.mocked(helpers.isDryRun).mockReturnValue(true); + const octokit = createDryRunOctokit(mockOctokit as any); + + mockOctokit.rest.git.createRef.mockClear(); + await octokit.rest.git.createRef({ + owner: 'test', + repo: 'test', + ref: 'refs/tags/v1.0.0', + sha: 'abc123', + }); + expect(mockOctokit.rest.git.createRef).not.toHaveBeenCalled(); + }); + + it('passes through read methods in dry-run mode', async () => { + vi.mocked(helpers.isDryRun).mockReturnValue(true); + const octokit = createDryRunOctokit(mockOctokit as any); + + await octokit.repos.getContent({ owner: 'test', repo: 'test', path: '/' }); + expect(mockOctokit.repos.getContent).toHaveBeenCalled(); + }); + }); + + describe('dryRunFs', () => { + // We can't easily test actual fs operations, so we test the dry-run behavior + it('logs instead of writing in dry-run mode', async () => { + vi.mocked(helpers.isDryRun).mockReturnValue(true); + + await dryRunFs.writeFile('/tmp/test.txt', 'content'); + expect(logger.info).toHaveBeenCalledWith( + '[dry-run] Would execute: fs.writeFile(/tmp/test.txt)' + ); + }); + + it('logs instead of unlinking in dry-run mode', async () => { + vi.mocked(helpers.isDryRun).mockReturnValue(true); + + await dryRunFs.unlink('/tmp/test.txt'); + expect(logger.info).toHaveBeenCalledWith( + '[dry-run] Would execute: fs.unlink(/tmp/test.txt)' + ); + }); + + it('logs instead of renaming in dry-run mode', async () => { + vi.mocked(helpers.isDryRun).mockReturnValue(true); + + await dryRunFs.rename('/tmp/old.txt', '/tmp/new.txt'); + expect(logger.info).toHaveBeenCalledWith( + '[dry-run] Would execute: fs.rename(/tmp/old.txt, /tmp/new.txt)' + ); + }); + }); + + describe('dryRunExec', () => { + it('executes action in normal mode', async () => { + vi.mocked(helpers.isDryRun).mockReturnValue(false); + const action = vi.fn().mockResolvedValue('result'); + + const result = await dryRunExec(action, 'test action'); + + expect(action).toHaveBeenCalled(); + expect(result).toBe('result'); + }); + + it('skips action and logs in dry-run mode', async () => { + vi.mocked(helpers.isDryRun).mockReturnValue(true); + const action = vi.fn().mockResolvedValue('result'); + + const result = await dryRunExec(action, 'test action'); + + expect(action).not.toHaveBeenCalled(); + expect(result).toBeUndefined(); + expect(logger.info).toHaveBeenCalledWith( + '[dry-run] Would execute: test action' + ); + }); + }); + + describe('dryRunExecSync', () => { + it('executes action in normal mode', () => { + vi.mocked(helpers.isDryRun).mockReturnValue(false); + const action = vi.fn().mockReturnValue('result'); + + const result = dryRunExecSync(action, 'test action'); + + expect(action).toHaveBeenCalled(); + expect(result).toBe('result'); + }); + + it('skips action and logs in dry-run mode', () => { + vi.mocked(helpers.isDryRun).mockReturnValue(true); + const action = vi.fn().mockReturnValue('result'); + + const result = dryRunExecSync(action, 'test action'); + + expect(action).not.toHaveBeenCalled(); + expect(result).toBeUndefined(); + expect(logger.info).toHaveBeenCalledWith( + '[dry-run] Would execute: test action' + ); + }); + }); +}); diff --git a/src/utils/__tests__/githubApi.test.ts b/src/utils/__tests__/githubApi.test.ts index a9bf46073..a9c2e88d9 100644 --- a/src/utils/__tests__/githubApi.test.ts +++ b/src/utils/__tests__/githubApi.test.ts @@ -12,6 +12,7 @@ vi.mock('@octokit/rest', () => ({ })); describe('getFile', () => { + // eslint-disable-next-line no-restricted-syntax -- Testing with mock Octokit const github = new Octokit(); const owner = 'owner'; const repo = 'repo'; diff --git a/src/utils/dryRun.ts b/src/utils/dryRun.ts new file mode 100644 index 000000000..f29a2d1f8 --- /dev/null +++ b/src/utils/dryRun.ts @@ -0,0 +1,418 @@ +/** + * Dry-run abstraction layer for destructive operations. + * + * This module provides Proxy-wrapped versions of external libraries/APIs that + * automatically respect the --dry-run flag. Instead of checking isDryRun() in + * every function, use these wrapped versions which intercept mutating operations. + */ +import * as fs from 'fs'; +import * as fsPromises from 'fs/promises'; +import type { SimpleGit } from 'simple-git'; +import type { Octokit } from '@octokit/rest'; + +import { logger } from '../logger'; +import { isDryRun } from './helpers'; + +/** + * Log a dry-run message with consistent formatting. + */ +export function logDryRun(operation: string): void { + logger.info(`[dry-run] Would execute: ${operation}`); +} + +// ============================================================================ +// Git Proxy +// ============================================================================ + +/** + * Git methods that modify state and should be blocked in dry-run mode. + */ +const GIT_MUTATING_METHODS = new Set([ + 'push', + 'commit', + 'checkout', + 'checkoutBranch', + 'merge', + 'branch', + 'addTag', + 'rm', + 'add', + 'clone', + 'pull', + 'reset', + 'revert', + 'stash', + 'tag', +]); + +/** + * Git raw commands that modify state and should be blocked in dry-run mode. + */ +const GIT_RAW_MUTATING_COMMANDS = new Set([ + 'push', + 'commit', + 'checkout', + 'merge', + 'tag', + 'rm', + 'add', + 'reset', + 'revert', + 'stash', + 'branch', + 'clone', + 'pull', +]); + +/** + * Creates a dry-run-aware wrapper around a SimpleGit instance. + * + * Mutating operations (push, commit, checkout, etc.) are automatically + * blocked and logged when isDryRun() returns true. + * + * @param git The base SimpleGit instance to wrap + * @returns A proxied SimpleGit that respects dry-run mode + */ +export function createDryRunGit(git: SimpleGit): SimpleGit { + return new Proxy(git, { + get(target, prop: string) { + const value = target[prop as keyof SimpleGit]; + + // If it's not a function, return as-is + if (typeof value !== 'function') { + return value; + } + + // Handle the special 'raw' method + if (prop === 'raw') { + return function (...args: string[]) { + const command = args[0]; + if (isDryRun() && GIT_RAW_MUTATING_COMMANDS.has(command)) { + logDryRun(`git ${args.join(' ')}`); + // Return a resolved promise for async compatibility + return Promise.resolve(''); + } + return value.apply(target, args); + }; + } + + // Check if this is a mutating method + if (GIT_MUTATING_METHODS.has(prop)) { + return function (...args: unknown[]) { + if (isDryRun()) { + const argsStr = args + .map(a => (typeof a === 'string' ? a : JSON.stringify(a))) + .join(' '); + logDryRun(`git.${prop}(${argsStr})`); + // Return a resolved promise for async compatibility + // Some git methods return the git instance for chaining + return Promise.resolve(createDryRunGit(target)); + } + return value.apply(target, args); + }; + } + + // For non-mutating methods, bind and return + return value.bind(target); + }, + }); +} + +// ============================================================================ +// Octokit (GitHub API) Proxy +// ============================================================================ + +/** + * GitHub API method prefixes that indicate mutating operations. + */ +const GITHUB_MUTATING_PREFIXES = [ + 'create', + 'update', + 'delete', + 'upload', + 'remove', + 'add', + 'set', + 'merge', +]; + +/** + * Check if a GitHub API method name indicates a mutating operation. + */ +function isGitHubMutatingMethod(methodName: string): boolean { + return GITHUB_MUTATING_PREFIXES.some(prefix => + methodName.toLowerCase().startsWith(prefix.toLowerCase()) + ); +} + +/** + * Creates a recursive proxy that intercepts GitHub API calls. + * Handles nested namespaces like github.repos.createRelease(). + */ +function createGitHubNamespaceProxy( + target: Record, + path: string[] = [] +): Record { + return new Proxy(target, { + get(obj, prop: string) { + const value = obj[prop]; + + // Skip non-existent properties and symbols + if (value === undefined || typeof prop === 'symbol') { + return value; + } + + const currentPath = [...path, prop]; + + // If it's a function, potentially intercept it + if (typeof value === 'function') { + return function (...args: unknown[]) { + if (isDryRun() && isGitHubMutatingMethod(prop)) { + const pathStr = currentPath.join('.'); + logDryRun(`github.${pathStr}(...)`); + // Return a mock response for compatibility + return Promise.resolve({ data: {} }); + } + return (value as (...a: unknown[]) => unknown).apply(obj, args); + }; + } + + // If it's an object (namespace), recursively proxy it + if (typeof value === 'object' && value !== null) { + return createGitHubNamespaceProxy( + value as Record, + currentPath + ); + } + + return value; + }, + }); +} + +/** + * Creates a dry-run-aware wrapper around an Octokit instance. + * + * Mutating API calls (create*, update*, delete*, upload*) are automatically + * blocked and logged when isDryRun() returns true. + * + * @param octokit The base Octokit instance to wrap + * @returns A proxied Octokit that respects dry-run mode + */ +export function createDryRunOctokit(octokit: Octokit): Octokit { + return createGitHubNamespaceProxy( + octokit as unknown as Record + ) as unknown as Octokit; +} + +// ============================================================================ +// File System Operations +// ============================================================================ + +/** + * Dry-run-aware file system operations. + * + * Write operations are blocked and logged in dry-run mode. + * Read operations always execute normally. + */ +export const dryRunFs = { + /** + * Write data to a file asynchronously. + */ + writeFile: async ( + filePath: string, + data: string | Buffer, + options?: fs.WriteFileOptions + ): Promise => { + if (isDryRun()) { + logDryRun(`fs.writeFile(${filePath})`); + return; + } + return fsPromises.writeFile(filePath, data, options); + }, + + /** + * Write data to a file synchronously. + */ + writeFileSync: ( + filePath: string, + data: string | Buffer, + options?: fs.WriteFileOptions + ): void => { + if (isDryRun()) { + logDryRun(`fs.writeFileSync(${filePath})`); + return; + } + return fs.writeFileSync(filePath, data, options); + }, + + /** + * Delete a file asynchronously. + */ + unlink: async (filePath: string): Promise => { + if (isDryRun()) { + logDryRun(`fs.unlink(${filePath})`); + return; + } + return fsPromises.unlink(filePath); + }, + + /** + * Delete a file synchronously. + */ + unlinkSync: (filePath: string): void => { + if (isDryRun()) { + logDryRun(`fs.unlinkSync(${filePath})`); + return; + } + return fs.unlinkSync(filePath); + }, + + /** + * Rename a file asynchronously. + */ + rename: async (oldPath: string, newPath: string): Promise => { + if (isDryRun()) { + logDryRun(`fs.rename(${oldPath}, ${newPath})`); + return; + } + return fsPromises.rename(oldPath, newPath); + }, + + /** + * Rename a file synchronously. + */ + renameSync: (oldPath: string, newPath: string): void => { + if (isDryRun()) { + logDryRun(`fs.renameSync(${oldPath}, ${newPath})`); + return; + } + return fs.renameSync(oldPath, newPath); + }, + + /** + * Remove a directory recursively asynchronously. + */ + rm: async ( + filePath: string, + options?: fs.RmOptions + ): Promise => { + if (isDryRun()) { + logDryRun(`fs.rm(${filePath})`); + return; + } + return fsPromises.rm(filePath, options); + }, + + /** + * Remove a directory recursively synchronously. + */ + rmSync: (filePath: string, options?: fs.RmOptions): void => { + if (isDryRun()) { + logDryRun(`fs.rmSync(${filePath})`); + return; + } + return fs.rmSync(filePath, options); + }, + + /** + * Create a directory asynchronously. + */ + mkdir: async ( + dirPath: string, + options?: fs.MakeDirectoryOptions + ): Promise => { + if (isDryRun()) { + logDryRun(`fs.mkdir(${dirPath})`); + return undefined; + } + return fsPromises.mkdir(dirPath, options); + }, + + /** + * Create a directory synchronously. + */ + mkdirSync: ( + dirPath: string, + options?: fs.MakeDirectoryOptions + ): string | undefined => { + if (isDryRun()) { + logDryRun(`fs.mkdirSync(${dirPath})`); + return undefined; + } + return fs.mkdirSync(dirPath, options); + }, + + /** + * Append data to a file asynchronously. + */ + appendFile: async ( + filePath: string, + data: string | Buffer, + options?: fs.WriteFileOptions + ): Promise => { + if (isDryRun()) { + logDryRun(`fs.appendFile(${filePath})`); + return; + } + return fsPromises.appendFile(filePath, data, options); + }, + + /** + * Append data to a file synchronously. + */ + appendFileSync: ( + filePath: string, + data: string | Buffer, + options?: fs.WriteFileOptions + ): void => { + if (isDryRun()) { + logDryRun(`fs.appendFileSync(${filePath})`); + return; + } + return fs.appendFileSync(filePath, data, options); + }, +}; + +// ============================================================================ +// Generic Action Wrapper +// ============================================================================ + +/** + * Execute an action only if not in dry-run mode. + * + * This is useful for wrapping arbitrary async operations that don't fit + * into the git/github/fs categories. + * + * @param action The action to execute + * @param description Human-readable description for dry-run logging + * @returns The result of the action, or undefined in dry-run mode + */ +export async function dryRunExec( + action: () => Promise, + description: string +): Promise { + if (isDryRun()) { + logDryRun(description); + return undefined; + } + return action(); +} + +/** + * Execute a synchronous action only if not in dry-run mode. + * + * @param action The action to execute + * @param description Human-readable description for dry-run logging + * @returns The result of the action, or undefined in dry-run mode + */ +export function dryRunExecSync( + action: () => T, + description: string +): T | undefined { + if (isDryRun()) { + logDryRun(description); + return undefined; + } + return action(); +} diff --git a/src/utils/gcsApi.ts b/src/utils/gcsApi.ts index 686f1a186..36b803f91 100644 --- a/src/utils/gcsApi.ts +++ b/src/utils/gcsApi.ts @@ -7,10 +7,10 @@ import { Storage as GCSStorage, UploadOptions as GCSUploadOptions, } from '@google-cloud/storage'; -import { isDryRun } from './helpers'; import { logger } from '../logger'; import { reportError } from './errors'; +import { dryRunExec } from './dryRun'; import { RequiredConfigVar } from './env'; import { detectContentType } from './files'; import { RemoteArtifact } from '../artifact_providers/base'; @@ -207,13 +207,9 @@ export class CraftGCSClient { `File \`${filename}\`, upload options: ${formatJson(uploadConfig)}` ); - if (!isDryRun()) { - logger.debug( - `Attempting to upload \`${filename}\` to \`${path.posix.join( - this.bucketName, - pathInBucket - )}\`.` - ); + const destination = path.posix.join(this.bucketName, pathInBucket); + await dryRunExec(async () => { + logger.debug(`Attempting to upload \`${filename}\` to \`${destination}\`.`); try { await this.bucket.upload(artifactLocalPath, uploadConfig); @@ -232,9 +228,7 @@ export class CraftGCSClient { filename )} \`.` ); - } else { - logger.info(`[dry-run] Skipping upload for \`${filename}\``); - } + }, `upload ${filename} to ${destination}`); } /** @@ -260,7 +254,7 @@ export class CraftGCSClient { ); } - if (!isDryRun()) { + await dryRunExec(async () => { logger.debug( `Attempting to download \`${destinationFilename}\` to \`${destinationDirectory}\`.` ); @@ -275,9 +269,7 @@ export class CraftGCSClient { } logger.debug(`Successfully downloaded \`${destinationFilename}\`.`); - } else { - logger.info(`[dry-run] Skipping download for \`${destinationFilename}\``); - } + }, `download ${destinationFilename} to ${destinationDirectory}`); return path.join(destinationDirectory, destinationFilename); } diff --git a/src/utils/git.ts b/src/utils/git.ts index 0237182ad..c1ecc3f07 100644 --- a/src/utils/git.ts +++ b/src/utils/git.ts @@ -1,7 +1,9 @@ +// eslint-disable-next-line no-restricted-imports -- This is the wrapper module import simpleGit, { type SimpleGit, type LogOptions, type Options, type StatusResult } from 'simple-git'; import { getConfigFileDir } from '../config'; import { ConfigurationError } from './errors'; +import { createDryRunGit } from './dryRun'; import { logger } from '../logger'; export interface GitChange { @@ -104,12 +106,28 @@ export async function getGitClient(): Promise { process.chdir(configFileDir); logger.debug("Working directory:", process.cwd()); + // eslint-disable-next-line no-restricted-syntax -- This is the git wrapper module const git = simpleGit(configFileDir); const isRepo = await git.checkIsRepo(); if (!isRepo) { throw new ConfigurationError('Not in a git repository!'); } - return git; + // Wrap with dry-run-aware proxy + return createDryRunGit(git); +} + +/** + * Creates a dry-run-aware git client for a specific directory. + * + * Use this when you need a git client for a directory other than the + * config file directory (e.g., for cloned repos in temp directories). + * + * @param directory The directory to use as the git working directory + * @returns A SimpleGit instance wrapped with dry-run support + */ +export function createGitClient(directory: string): SimpleGit { + // eslint-disable-next-line no-restricted-syntax -- This is the git wrapper module + return createDryRunGit(simpleGit(directory)); } /** diff --git a/src/utils/githubApi.ts b/src/utils/githubApi.ts index 4dbfab81d..9fec9d295 100644 --- a/src/utils/githubApi.ts +++ b/src/utils/githubApi.ts @@ -3,6 +3,7 @@ import { Octokit } from '@octokit/rest'; import { LogLevel, logger } from '../logger'; import { ConfigurationError } from './errors'; +import { createDryRunOctokit } from './dryRun'; /** * Abstraction for GitHub remotes @@ -109,10 +110,11 @@ export function getGitHubClient(token = ''): Octokit { }; } - const { retry } = require('@octokit/plugin-retry'); const octokitWithRetries = Octokit.plugin(retry); - _GitHubClientCache[githubApiToken] = new octokitWithRetries(attrs); + const client = new octokitWithRetries(attrs); + // Wrap with dry-run-aware proxy + _GitHubClientCache[githubApiToken] = createDryRunOctokit(client); } return _GitHubClientCache[githubApiToken]; From fa8c219a2646ad5ef220be8e36f18d66e11bdf8f Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Mon, 29 Dec 2025 21:20:54 +0300 Subject: [PATCH 2/8] Address PR review feedback - Remove repetitive dry-run proxy comments - Remove isDryRun() checks from brew.ts, awsLambdaLayer.ts, github.ts (use proxied APIs instead) - Allow clone() through dry-run proxy (safe local operation) - Remove eslint-disable comments for clone operations - Update Octokit proxy to return mock status for status-based checks --- src/commands/prepare.ts | 5 ---- src/commands/publish.ts | 4 --- src/targets/awsLambdaLayer.ts | 38 ++++++++++---------------- src/targets/brew.ts | 8 +----- src/targets/commitOnGitRepository.ts | 6 +---- src/targets/ghPages.ts | 5 +--- src/targets/github.ts | 40 +++++++--------------------- src/targets/registry.ts | 6 ++--- src/targets/upm.ts | 7 +---- src/utils/dryRun.ts | 9 ++++--- 10 files changed, 36 insertions(+), 92 deletions(-) diff --git a/src/commands/prepare.ts b/src/commands/prepare.ts index ba7e622d0..b2f691105 100644 --- a/src/commands/prepare.ts +++ b/src/commands/prepare.ts @@ -215,7 +215,6 @@ async function createReleaseBranch( reportError(errorMsg, logger); } - // Git operations are automatically handled by the dry-run proxy await git.checkoutBranch(branchName, rev); logger.info(`Created a new release branch: "${branchName}"`); logger.info(`Switched to branch "${branchName}"`); @@ -238,7 +237,6 @@ async function pushReleaseBranch( if (pushFlag) { logger.info(`Pushing the release branch "${branchName}"...`); // TODO check remote somehow - // Git operations are automatically handled by the dry-run proxy await git.push(remoteName, branchName, ['--set-upstream']); } else { logger.info('Not pushing the release branch.'); @@ -267,7 +265,6 @@ async function commitNewVersion( logger.debug('Committing the release changes...'); logger.trace(`Commit message: "${message}"`); - // Git operations are automatically handled by the dry-run proxy await git.commit(message, ['--all']); } @@ -463,7 +460,6 @@ async function prepareChangelog( changelogString = prependChangeset(changelogString, changeset); } - // File writes are automatically handled by the dry-run proxy await dryRunFs.writeFile(relativePath, changelogString); break; @@ -495,7 +491,6 @@ async function switchToDefaultBranch( return; } logger.info(`Switching back to the default branch (${defaultBranch})...`); - // Git operations are automatically handled by the dry-run proxy await git.checkout(defaultBranch); } diff --git a/src/commands/publish.ts b/src/commands/publish.ts index 6078366eb..60a1000ac 100644 --- a/src/commands/publish.ts +++ b/src/commands/publish.ts @@ -373,7 +373,6 @@ async function handleReleaseBranch( await git.checkout(mergeTarget); logger.debug(`Merging ${branch} into: ${mergeTarget}`); - // Git operations are automatically handled by the dry-run proxy await git .pull(remoteName, mergeTarget, ['--rebase']) .merge(['--no-ff', '--no-edit', branch]) @@ -383,7 +382,6 @@ async function handleReleaseBranch( logger.info('Not deleting the release branch.'); } else { logger.debug(`Deleting the release branch: ${branch}`); - // Git operations are automatically handled by the dry-run proxy await git.branch(['-D', branch]).push([remoteName, '--delete', branch]); logger.info(`Removed the remote branch: "${branch}"`); } @@ -566,7 +564,6 @@ export async function publishMain(argv: PublishOptions): Promise { for (const target of targetList) { await publishToTarget(target, newVersion, revision); publishState.published[BaseTarget.getId(target.config)] = true; - // File writes are automatically handled by the dry-run proxy dryRunFs.writeFileSync(publishStateFile, JSON.stringify(publishState)); } @@ -606,7 +603,6 @@ export async function publishMain(argv: PublishOptions): Promise { // file. If unlinking fails, we honestly don't care, at least to fail // the final steps. And it doesn't make sense to wait until this op // finishes then as nothing relies on the removal of this file. - // File operations are automatically handled by the dry-run proxy dryRunFs .unlink(publishStateFile) .catch(err => logger.trace("Couldn't remove publish state file: ", err)); diff --git a/src/targets/awsLambdaLayer.ts b/src/targets/awsLambdaLayer.ts index 0ec2e2226..8f49d6f78 100644 --- a/src/targets/awsLambdaLayer.ts +++ b/src/targets/awsLambdaLayer.ts @@ -2,8 +2,6 @@ import * as fs from 'fs'; import * as path from 'path'; import { Octokit } from '@octokit/rest'; -// eslint-disable-next-line no-restricted-imports -- Need raw simpleGit for initial clone -import simpleGit from 'simple-git'; import { getGitHubApiToken, getGitHubClient, @@ -22,9 +20,8 @@ import { } from '../utils/awsLambdaLayerManager'; import { createSymlinks } from '../utils/symlink'; import { withTempDir } from '../utils/files'; -import { isDryRun } from '../utils/helpers'; import { createGitClient } from '../utils/git'; -import { logDryRun } from '../utils/dryRun'; +import { dryRunExec } from '../utils/dryRun'; import { renderTemplateSafe } from '../utils/strings'; import { isPreviewRelease, parseVersion } from '../utils/version'; import { DEFAULT_REGISTRY_REMOTE } from '../utils/registry'; @@ -177,23 +174,22 @@ export class AwsLambdaLayerTarget extends BaseTarget { this.logger.info( `Cloning ${remote.getRemoteString()} to ${directory}...` ); - // eslint-disable-next-line no-restricted-syntax -- Clone needs raw simpleGit, wrapped client used after - await simpleGit().clone(remote.getRemoteStringWithAuth(), directory); + await createGitClient('.').clone(remote.getRemoteStringWithAuth(), directory); const git = createGitClient(directory); - if (!isDryRun()) { - await this.publishRuntimes( - version, - directory, - awsRegions, - artifactBuffer - ); - this.logger.debug('Finished publishing runtimes.'); - } else { - logDryRun('publishRuntimes(...)'); - } + await dryRunExec( + async () => { + await this.publishRuntimes( + version, + directory, + awsRegions, + artifactBuffer + ); + this.logger.debug('Finished publishing runtimes.'); + }, + 'publishRuntimes(...)' + ); - // Git operations are automatically handled by the dry-run proxy await git.add(['.']); await git.checkout('master'); const runtimeNames = this.config.compatibleRuntimes.map( @@ -226,12 +222,6 @@ export class AwsLambdaLayerTarget extends BaseTarget { * @param linkPrereleases Whether the current release is a prerelease. */ private isPushableToRegistry(version: string): boolean { - if (isDryRun()) { - // Skip early - git.push() will also be blocked by the proxy but we want - // to skip the whole logic chain - logDryRun('git.push()'); - return false; - } if (isPreviewRelease(version) && !this.awsLambdaConfig.linkPrereleases) { // preview release this.logger.info( diff --git a/src/targets/brew.ts b/src/targets/brew.ts index 93f2d6f46..04e881a5e 100644 --- a/src/targets/brew.ts +++ b/src/targets/brew.ts @@ -4,8 +4,6 @@ import { Octokit } from '@octokit/rest'; import { GitHubGlobalConfig, TargetConfig } from '../schemas/project_config'; import { ConfigurationError } from '../utils/errors'; import { getGitHubClient } from '../utils/githubApi'; -import { isDryRun } from '../utils/helpers'; -import { logDryRun } from '../utils/dryRun'; import { renderTemplateSafe } from '../utils/strings'; import { HashAlgorithm, HashOutputFormat } from '../utils/system'; import { BaseTarget } from './base'; @@ -207,11 +205,7 @@ export class BrewTarget extends BaseTarget { `${action} file ${params.owner}/${params.repo}:${params.path} (${params.sha})` ); - if (isDryRun()) { - logDryRun(`github.repos.createOrUpdateFileContents(${params.path})`); - } else { - await this.github.repos.createOrUpdateFileContents(params); - } + await this.github.repos.createOrUpdateFileContents(params); this.logger.info('Homebrew release complete'); } } diff --git a/src/targets/commitOnGitRepository.ts b/src/targets/commitOnGitRepository.ts index 4b94db4eb..46cc89405 100644 --- a/src/targets/commitOnGitRepository.ts +++ b/src/targets/commitOnGitRepository.ts @@ -1,5 +1,3 @@ -// eslint-disable-next-line no-restricted-imports -- Need raw simpleGit for initial clone -import simpleGit from 'simple-git'; import { BaseArtifactProvider } from '../artifact_providers/base'; import { TargetConfig } from '../schemas/project_config'; import { ConfigurationError, reportError } from '../utils/errors'; @@ -152,8 +150,7 @@ export async function pushArchiveToGitRepository({ const authenticatedUrl = parsedUrl.toString(); - // eslint-disable-next-line no-restricted-syntax -- Clone needs raw simpleGit, wrapped client used after - await simpleGit().clone(authenticatedUrl, directory); + await createGitClient('.').clone(authenticatedUrl, directory); const git = createGitClient(directory); logger?.info(`Checking out branch "${branch}"...`); @@ -177,7 +174,6 @@ export async function pushArchiveToGitRepository({ logger?.info(`Staging files...`); await git.raw('add', '--all'); - // Git operations are automatically handled by the dry-run proxy logger?.info(`Creating commit...`); await git.commit(`release: ${version}`); diff --git a/src/targets/ghPages.ts b/src/targets/ghPages.ts index 86b2d17fe..9109470cb 100644 --- a/src/targets/ghPages.ts +++ b/src/targets/ghPages.ts @@ -2,8 +2,6 @@ import * as fs from 'fs'; import * as path from 'path'; import { Octokit } from '@octokit/rest'; -// eslint-disable-next-line no-restricted-imports -- Need raw simpleGit for initial clone -import simpleGit from 'simple-git'; import { GitHubGlobalConfig, TargetConfig } from '../schemas/project_config'; import { ConfigurationError, reportError } from '../utils/errors'; @@ -151,8 +149,7 @@ export class GhPagesTarget extends BaseTarget { this.logger.info( `Cloning "${remote.getRemoteString()}" to "${directory}"...` ); - // eslint-disable-next-line no-restricted-syntax -- Clone needs raw simpleGit, wrapped client used after - await simpleGit().clone(remote.getRemoteStringWithAuth(), directory); + await createGitClient('.').clone(remote.getRemoteStringWithAuth(), directory); const git = createGitClient(directory); this.logger.debug(`Checking out branch: "${branch}"`); try { diff --git a/src/targets/github.ts b/src/targets/github.ts index cfab78ebb..8014db672 100644 --- a/src/targets/github.ts +++ b/src/targets/github.ts @@ -133,16 +133,6 @@ export class GitHubTarget extends BaseTarget { const isPreview = this.githubConfig.previewReleases && isPreviewRelease(version); - if (isDryRun()) { - logDryRun(`github.repos.createRelease(${tag})`); - return { - id: 0, - tag_name: tag, - upload_url: '', - draft: true, - }; - } - const { data } = await this.github.repos.createRelease({ draft: true, name: tag, @@ -153,6 +143,16 @@ export class GitHubTarget extends BaseTarget { target_commitish: revision, ...changes, }); + + // In dry-run mode, the proxy returns an empty object - provide mock data + if (!data.id) { + return { + id: 0, + tag_name: tag, + upload_url: '', + draft: true, + }; + } return data; } @@ -186,11 +186,6 @@ export class GitHubTarget extends BaseTarget { asset: ReposListAssetsForReleaseResponseItem ): Promise { this.logger.debug(`Deleting asset: "${asset.name}"...`); - if (isDryRun()) { - logDryRun(`github.repos.deleteReleaseAsset(${asset.name})`); - return false; - } - return ( ( await this.github.repos.deleteReleaseAsset({ @@ -220,11 +215,6 @@ export class GitHubTarget extends BaseTarget { return false; } - if (isDryRun()) { - logDryRun(`github.repos.deleteRelease(${release.tag_name})`); - return false; - } - return ( ( await this.github.repos.deleteRelease({ @@ -380,11 +370,6 @@ export class GitHubTarget extends BaseTarget { release: GitHubRelease, options: { makeLatest: boolean } = { makeLatest: true } ) { - if (isDryRun()) { - logDryRun(`github.repos.updateRelease(${release.tag_name})`); - return; - } - await this.github.repos.updateRelease({ ...this.githubConfig, release_id: release.id, @@ -409,11 +394,6 @@ export class GitHubTarget extends BaseTarget { ): Promise { const tag = versionToTag(version, this.githubConfig.tagPrefix); const tagRef = `refs/tags/${tag}`; - if (isDryRun()) { - logDryRun(`github.git.createRef(${tagRef})`); - return; - } - this.logger.info(`Pushing the tag reference: "${tagRef}"...`); await this.github.rest.git.createRef({ owner: this.githubConfig.owner, diff --git a/src/targets/registry.ts b/src/targets/registry.ts index 3592e4cc2..c6732420f 100644 --- a/src/targets/registry.ts +++ b/src/targets/registry.ts @@ -1,7 +1,6 @@ import { mapLimit } from 'async'; import { Octokit } from '@octokit/rest'; -// eslint-disable-next-line no-restricted-imports -- Need raw simpleGit for initial clone -import simpleGit, { SimpleGit } from 'simple-git'; +import type { SimpleGit } from 'simple-git'; import { GitHubGlobalConfig, TargetConfig } from '../schemas/project_config'; import { ConfigurationError, reportError } from '../utils/errors'; @@ -432,8 +431,7 @@ export class RegistryTarget extends BaseTarget { this.logger.info( `Cloning "${remote.getRemoteString()}" to "${directory}"...` ); - // eslint-disable-next-line no-restricted-syntax -- Clone needs raw simpleGit, wrapped client used after - await simpleGit().clone(remote.getRemoteStringWithAuth(), directory, [ + await createGitClient('.').clone(remote.getRemoteStringWithAuth(), directory, [ '--filter=tree:0', '--single-branch', ]); diff --git a/src/targets/upm.ts b/src/targets/upm.ts index d4c9569a8..4d28f86ab 100644 --- a/src/targets/upm.ts +++ b/src/targets/upm.ts @@ -1,6 +1,4 @@ import { Octokit } from '@octokit/rest'; -// eslint-disable-next-line no-restricted-imports -- Need raw simpleGit for initial clone -import simpleGit from 'simple-git'; import { getGitHubApiToken, getGitHubClient, @@ -117,8 +115,7 @@ export class UpmTarget extends BaseTarget { await withTempDir( async directory => { - // eslint-disable-next-line no-restricted-syntax -- Clone needs raw simpleGit, wrapped client used after - await simpleGit().clone(remote.getRemoteStringWithAuth(), directory); + await createGitClient('.').clone(remote.getRemoteStringWithAuth(), directory); const git = createGitClient(directory); this.logger.info('Clearing the repository.'); @@ -129,7 +126,6 @@ export class UpmTarget extends BaseTarget { this.logger.info('Adding files to repository.'); await git.add(['.']); - // Git operations are automatically handled by the dry-run proxy const commitResult = await git.commit(`release ${version}`); if (!commitResult.commit) { throw new Error( @@ -138,7 +134,6 @@ export class UpmTarget extends BaseTarget { } const targetRevision = await git.revparse([commitResult.commit]); - // Git operations are automatically handled by the dry-run proxy await git.push(['origin', 'main']); const changes = await this.githubTarget.getChangelog(version); const isPrerelease = isPreviewRelease(version); diff --git a/src/utils/dryRun.ts b/src/utils/dryRun.ts index f29a2d1f8..114d86758 100644 --- a/src/utils/dryRun.ts +++ b/src/utils/dryRun.ts @@ -37,12 +37,13 @@ const GIT_MUTATING_METHODS = new Set([ 'addTag', 'rm', 'add', - 'clone', 'pull', 'reset', 'revert', 'stash', 'tag', + // Note: 'clone' is intentionally NOT included - it creates a local copy + // which is safe to do in dry-run mode and needed for subsequent operations ]); /** @@ -60,8 +61,9 @@ const GIT_RAW_MUTATING_COMMANDS = new Set([ 'revert', 'stash', 'branch', - 'clone', 'pull', + // Note: 'clone' is intentionally NOT included - it creates a local copy + // which is safe to do in dry-run mode and needed for subsequent operations ]); /** @@ -171,7 +173,8 @@ function createGitHubNamespaceProxy( const pathStr = currentPath.join('.'); logDryRun(`github.${pathStr}(...)`); // Return a mock response for compatibility - return Promise.resolve({ data: {} }); + // status: 0 ensures status-based checks (e.g., === 204) fail gracefully + return Promise.resolve({ data: {}, status: 0 }); } return (value as (...a: unknown[]) => unknown).apply(obj, args); }; From adf08a493e323f4496437ae1c0f18d2331cc806e Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Mon, 29 Dec 2025 22:11:38 +0300 Subject: [PATCH 3/8] Address second round of PR review feedback - Add cloneRepo() helper for cleaner clone+createGitClient pattern - Rename dryRunFs to safeFs, dryRunExec to safeExec - Remove remaining isDryRun() checks from github.ts using safeExec - Remove repetitive dry-run proxy comments from registry.ts and ghPages.ts - Update AGENTS.md with new naming --- AGENTS.md | 8 +-- src/commands/prepare.ts | 4 +- src/commands/publish.ts | 6 +-- src/targets/awsLambdaLayer.ts | 9 ++-- src/targets/commitOnGitRepository.ts | 5 +- src/targets/ghPages.ts | 7 +-- src/targets/github.ts | 79 +++++++++++++--------------- src/targets/registry.ts | 7 +-- src/targets/upm.ts | 5 +- src/utils/__tests__/dryRun.test.ts | 26 ++++----- src/utils/dryRun.ts | 6 +-- src/utils/gcsApi.ts | 6 +-- src/utils/git.ts | 26 +++++++++ 13 files changed, 102 insertions(+), 92 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c357e3fc4..90048ee0d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -78,8 +78,8 @@ Instead of checking `isDryRun()` manually in every function, destructive operati - **Git operations**: Use `getGitClient()` from `src/utils/git.ts` or `createGitClient(directory)` for working with specific directories - **GitHub API**: Use `getGitHubClient()` from `src/utils/githubApi.ts` -- **File writes**: Use `dryRunFs` from `src/utils/dryRun.ts` -- **Other actions**: Use `dryRunExec()` or `dryRunExecSync()` from `src/utils/dryRun.ts` +- **File writes**: Use `safeFs` from `src/utils/dryRun.ts` +- **Other actions**: Use `safeExec()` or `safeExecSync()` from `src/utils/dryRun.ts` ### ESLint Enforcement @@ -101,8 +101,8 @@ When adding new code that performs destructive operations: 1. **Git**: Get the git client via `getGitClient()` or `createGitClient()` - mutating methods are automatically blocked 2. **GitHub API**: Get the client via `getGitHubClient()` - `create*`, `update*`, `delete*`, `upload*` methods are automatically blocked -3. **File writes**: Use `dryRunFs.writeFile()`, `dryRunFs.unlink()`, etc. instead of raw `fs` methods -4. **Other**: Wrap with `dryRunExec(action, description)` for custom operations +3. **File writes**: Use `safeFs.writeFile()`, `safeFs.unlink()`, etc. instead of raw `fs` methods +4. **Other**: Wrap with `safeExec(action, description)` for custom operations ### Special Cases diff --git a/src/commands/prepare.ts b/src/commands/prepare.ts index b2f691105..5d283fd0b 100644 --- a/src/commands/prepare.ts +++ b/src/commands/prepare.ts @@ -1,6 +1,6 @@ import { existsSync, promises as fsPromises } from 'fs'; -import { dryRunFs } from '../utils/dryRun'; +import { safeFs } from '../utils/dryRun'; import { join, relative } from 'path'; import * as shellQuote from 'shell-quote'; import { SimpleGit, StatusResult } from 'simple-git'; @@ -460,7 +460,7 @@ async function prepareChangelog( changelogString = prependChangeset(changelogString, changeset); } - await dryRunFs.writeFile(relativePath, changelogString); + await safeFs.writeFile(relativePath, changelogString); break; default: diff --git a/src/commands/publish.ts b/src/commands/publish.ts index 60a1000ac..f15c05daa 100644 --- a/src/commands/publish.ts +++ b/src/commands/publish.ts @@ -2,7 +2,7 @@ import { Arguments, Argv, CommandBuilder } from 'yargs'; import chalk from 'chalk'; import { existsSync, readFileSync } from 'fs'; -import { dryRunFs } from '../utils/dryRun'; +import { safeFs } from '../utils/dryRun'; import { join } from 'path'; import shellQuote from 'shell-quote'; import stringLength from 'string-length'; @@ -564,7 +564,7 @@ export async function publishMain(argv: PublishOptions): Promise { for (const target of targetList) { await publishToTarget(target, newVersion, revision); publishState.published[BaseTarget.getId(target.config)] = true; - dryRunFs.writeFileSync(publishStateFile, JSON.stringify(publishState)); + safeFs.writeFileSync(publishStateFile, JSON.stringify(publishState)); } if (argv.keepDownloads) { @@ -603,7 +603,7 @@ export async function publishMain(argv: PublishOptions): Promise { // file. If unlinking fails, we honestly don't care, at least to fail // the final steps. And it doesn't make sense to wait until this op // finishes then as nothing relies on the removal of this file. - dryRunFs + safeFs .unlink(publishStateFile) .catch(err => logger.trace("Couldn't remove publish state file: ", err)); logger.success(`Version ${newVersion} has been published!`); diff --git a/src/targets/awsLambdaLayer.ts b/src/targets/awsLambdaLayer.ts index 8f49d6f78..b372ce442 100644 --- a/src/targets/awsLambdaLayer.ts +++ b/src/targets/awsLambdaLayer.ts @@ -20,8 +20,8 @@ import { } from '../utils/awsLambdaLayerManager'; import { createSymlinks } from '../utils/symlink'; import { withTempDir } from '../utils/files'; -import { createGitClient } from '../utils/git'; -import { dryRunExec } from '../utils/dryRun'; +import { cloneRepo, createGitClient } from '../utils/git'; +import { safeExec } from '../utils/dryRun'; import { renderTemplateSafe } from '../utils/strings'; import { isPreviewRelease, parseVersion } from '../utils/version'; import { DEFAULT_REGISTRY_REMOTE } from '../utils/registry'; @@ -174,10 +174,9 @@ export class AwsLambdaLayerTarget extends BaseTarget { this.logger.info( `Cloning ${remote.getRemoteString()} to ${directory}...` ); - await createGitClient('.').clone(remote.getRemoteStringWithAuth(), directory); - const git = createGitClient(directory); + const git = await cloneRepo(remote.getRemoteStringWithAuth(), directory); - await dryRunExec( + await safeExec( async () => { await this.publishRuntimes( version, diff --git a/src/targets/commitOnGitRepository.ts b/src/targets/commitOnGitRepository.ts index 46cc89405..c30e750dc 100644 --- a/src/targets/commitOnGitRepository.ts +++ b/src/targets/commitOnGitRepository.ts @@ -2,7 +2,7 @@ import { BaseArtifactProvider } from '../artifact_providers/base'; import { TargetConfig } from '../schemas/project_config'; import { ConfigurationError, reportError } from '../utils/errors'; import { withTempDir } from '../utils/files'; -import { createGitClient } from '../utils/git'; +import { cloneRepo, createGitClient } from '../utils/git'; import { BaseTarget } from './base'; import childProcess from 'child_process'; import type { Consola } from 'consola'; @@ -150,8 +150,7 @@ export async function pushArchiveToGitRepository({ const authenticatedUrl = parsedUrl.toString(); - await createGitClient('.').clone(authenticatedUrl, directory); - const git = createGitClient(directory); + const git = await cloneRepo(authenticatedUrl, directory); logger?.info(`Checking out branch "${branch}"...`); await git.checkout(branch); diff --git a/src/targets/ghPages.ts b/src/targets/ghPages.ts index 9109470cb..96a67ca5a 100644 --- a/src/targets/ghPages.ts +++ b/src/targets/ghPages.ts @@ -11,7 +11,7 @@ import { getGitHubClient, GitHubRemote, } from '../utils/githubApi'; -import { createGitClient } from '../utils/git'; +import { cloneRepo, createGitClient } from '../utils/git'; import { extractZipArchive } from '../utils/system'; import { BaseTarget } from './base'; import { BaseArtifactProvider } from '../artifact_providers/base'; @@ -149,8 +149,7 @@ export class GhPagesTarget extends BaseTarget { this.logger.info( `Cloning "${remote.getRemoteString()}" to "${directory}"...` ); - await createGitClient('.').clone(remote.getRemoteStringWithAuth(), directory); - const git = createGitClient(directory); + const git = await cloneRepo(remote.getRemoteStringWithAuth(), directory); this.logger.debug(`Checking out branch: "${branch}"`); try { await git.checkout([branch]); @@ -179,11 +178,9 @@ export class GhPagesTarget extends BaseTarget { // Extract the archive await this.extractAssets(archivePath, directory); - // Commit - git operations are automatically handled by the dry-run proxy await git.add(['.']); await git.commit(`craft(gh-pages): update, version "${version}"`); - // Push! - git operations are automatically handled by the dry-run proxy this.logger.info(`Pushing branch "${branch}"...`); await git.push('origin', branch, ['--set-upstream']); } diff --git a/src/targets/github.ts b/src/targets/github.ts index 8014db672..bbf258a13 100644 --- a/src/targets/github.ts +++ b/src/targets/github.ts @@ -14,8 +14,7 @@ import { findChangeset, } from '../utils/changelog'; import { getGitHubClient } from '../utils/githubApi'; -import { isDryRun } from '../utils/helpers'; -import { logDryRun } from '../utils/dryRun'; +import { safeExec } from '../utils/dryRun'; import { isPreviewRelease, parseVersion, @@ -281,23 +280,20 @@ export class GitHubTarget extends BaseTarget { ): Promise { const name = basename(path); - if (isDryRun()) { - logDryRun(`github.repos.uploadReleaseAsset(${name})`); - return; - } - - process.stderr.write( - `Uploading asset "${name}" to ${this.githubConfig.owner}/${this.githubConfig.repo}:${release.tag_name}\n` - ); + return safeExec(async () => { + process.stderr.write( + `Uploading asset "${name}" to ${this.githubConfig.owner}/${this.githubConfig.repo}:${release.tag_name}\n` + ); - try { - const { url } = await this.handleGitHubUpload(release, path, contentType); - process.stderr.write(`✔ Uploaded asset "${name}".\n`); - return url; - } catch (e) { - process.stderr.write(`✖ Cannot upload asset "${name}".\n`); - throw e; - } + try { + const { url } = await this.handleGitHubUpload(release, path, contentType); + process.stderr.write(`✔ Uploaded asset "${name}".\n`); + return url; + } catch (e) { + process.stderr.write(`✖ Cannot upload asset "${name}".\n`); + throw e; + } + }, `github.repos.uploadReleaseAsset(${name})`); } private async handleGitHubUpload( @@ -447,37 +443,34 @@ export class GitHubTarget extends BaseTarget { const tag = this.resolveFloatingTag(pattern, parsedVersion); const tagRef = `refs/tags/${tag}`; - if (isDryRun()) { - logDryRun(`github.git.updateRef(tags/${tag})`); - continue; - } - - this.logger.info(`Updating floating tag: "${tag}"...`); + await safeExec(async () => { + this.logger.info(`Updating floating tag: "${tag}"...`); - try { - // Try to update existing tag - await this.github.rest.git.updateRef({ - owner: this.githubConfig.owner, - repo: this.githubConfig.repo, - ref: `tags/${tag}`, - sha: revision, - force: true, - }); - this.logger.debug(`Updated existing floating tag: "${tag}"`); - } catch (error) { - // Tag doesn't exist, create it - if (error.status === 422) { - await this.github.rest.git.createRef({ + try { + // Try to update existing tag + await this.github.rest.git.updateRef({ owner: this.githubConfig.owner, repo: this.githubConfig.repo, - ref: tagRef, + ref: `tags/${tag}`, sha: revision, + force: true, }); - this.logger.debug(`Created new floating tag: "${tag}"`); - } else { - throw error; + this.logger.debug(`Updated existing floating tag: "${tag}"`); + } catch (error) { + // Tag doesn't exist, create it + if (error.status === 422) { + await this.github.rest.git.createRef({ + owner: this.githubConfig.owner, + repo: this.githubConfig.repo, + ref: tagRef, + sha: revision, + }); + this.logger.debug(`Created new floating tag: "${tag}"`); + } else { + throw error; + } } - } + }, `github.git.updateRef(tags/${tag})`); } } diff --git a/src/targets/registry.ts b/src/targets/registry.ts index c6732420f..0b32c691a 100644 --- a/src/targets/registry.ts +++ b/src/targets/registry.ts @@ -30,7 +30,7 @@ import { updateManifestSymlinks, RegistryPackageType, } from '../utils/registry'; -import { createGitClient } from '../utils/git'; +import { cloneRepo } from '../utils/git'; import { filterAsync, withRetry } from '../utils/async'; /** "registry" target options */ @@ -431,11 +431,10 @@ export class RegistryTarget extends BaseTarget { this.logger.info( `Cloning "${remote.getRemoteString()}" to "${directory}"...` ); - await createGitClient('.').clone(remote.getRemoteStringWithAuth(), directory, [ + return cloneRepo(remote.getRemoteStringWithAuth(), directory, [ '--filter=tree:0', '--single-branch', ]); - return createGitClient(directory); } public async getValidItems( @@ -498,13 +497,11 @@ export class RegistryTarget extends BaseTarget { ) ); - // Commit - git operations are automatically handled by the dry-run proxy await localRepo.git .add(['.']) .commit( `craft: release "${this.githubRepo.repo}", version "${version}"` ); - // Push! - git operations are automatically handled by the dry-run proxy this.logger.info(`Pushing the changes...`); // Ensure we are still up to date with upstream await withRetry(() => diff --git a/src/targets/upm.ts b/src/targets/upm.ts index 4d28f86ab..8609a5a2e 100644 --- a/src/targets/upm.ts +++ b/src/targets/upm.ts @@ -15,7 +15,7 @@ import { import { reportError } from '../utils/errors'; import { extractZipArchive } from '../utils/system'; import { withTempDir } from '../utils/files'; -import { createGitClient } from '../utils/git'; +import { cloneRepo, createGitClient } from '../utils/git'; import { isPreviewRelease } from '../utils/version'; import { NoneArtifactProvider } from '../artifact_providers/none'; @@ -115,8 +115,7 @@ export class UpmTarget extends BaseTarget { await withTempDir( async directory => { - await createGitClient('.').clone(remote.getRemoteStringWithAuth(), directory); - const git = createGitClient(directory); + const git = await cloneRepo(remote.getRemoteStringWithAuth(), directory); this.logger.info('Clearing the repository.'); await git.rm(['-r', '-f', '.']); diff --git a/src/utils/__tests__/dryRun.test.ts b/src/utils/__tests__/dryRun.test.ts index b3f7c7bfd..1dfd66140 100644 --- a/src/utils/__tests__/dryRun.test.ts +++ b/src/utils/__tests__/dryRun.test.ts @@ -24,9 +24,9 @@ vi.mock('../../logger', () => ({ import { createDryRunGit, createDryRunOctokit, - dryRunFs, - dryRunExec, - dryRunExecSync, + safeFs, + safeExec, + safeExecSync, logDryRun, } from '../dryRun'; import { logger } from '../../logger'; @@ -197,12 +197,12 @@ describe('dryRun utilities', () => { }); }); - describe('dryRunFs', () => { + describe('safeFs', () => { // We can't easily test actual fs operations, so we test the dry-run behavior it('logs instead of writing in dry-run mode', async () => { vi.mocked(helpers.isDryRun).mockReturnValue(true); - await dryRunFs.writeFile('/tmp/test.txt', 'content'); + await safeFs.writeFile('/tmp/test.txt', 'content'); expect(logger.info).toHaveBeenCalledWith( '[dry-run] Would execute: fs.writeFile(/tmp/test.txt)' ); @@ -211,7 +211,7 @@ describe('dryRun utilities', () => { it('logs instead of unlinking in dry-run mode', async () => { vi.mocked(helpers.isDryRun).mockReturnValue(true); - await dryRunFs.unlink('/tmp/test.txt'); + await safeFs.unlink('/tmp/test.txt'); expect(logger.info).toHaveBeenCalledWith( '[dry-run] Would execute: fs.unlink(/tmp/test.txt)' ); @@ -220,19 +220,19 @@ describe('dryRun utilities', () => { it('logs instead of renaming in dry-run mode', async () => { vi.mocked(helpers.isDryRun).mockReturnValue(true); - await dryRunFs.rename('/tmp/old.txt', '/tmp/new.txt'); + await safeFs.rename('/tmp/old.txt', '/tmp/new.txt'); expect(logger.info).toHaveBeenCalledWith( '[dry-run] Would execute: fs.rename(/tmp/old.txt, /tmp/new.txt)' ); }); }); - describe('dryRunExec', () => { + describe('safeExec', () => { it('executes action in normal mode', async () => { vi.mocked(helpers.isDryRun).mockReturnValue(false); const action = vi.fn().mockResolvedValue('result'); - const result = await dryRunExec(action, 'test action'); + const result = await safeExec(action, 'test action'); expect(action).toHaveBeenCalled(); expect(result).toBe('result'); @@ -242,7 +242,7 @@ describe('dryRun utilities', () => { vi.mocked(helpers.isDryRun).mockReturnValue(true); const action = vi.fn().mockResolvedValue('result'); - const result = await dryRunExec(action, 'test action'); + const result = await safeExec(action, 'test action'); expect(action).not.toHaveBeenCalled(); expect(result).toBeUndefined(); @@ -252,12 +252,12 @@ describe('dryRun utilities', () => { }); }); - describe('dryRunExecSync', () => { + describe('safeExecSync', () => { it('executes action in normal mode', () => { vi.mocked(helpers.isDryRun).mockReturnValue(false); const action = vi.fn().mockReturnValue('result'); - const result = dryRunExecSync(action, 'test action'); + const result = safeExecSync(action, 'test action'); expect(action).toHaveBeenCalled(); expect(result).toBe('result'); @@ -267,7 +267,7 @@ describe('dryRun utilities', () => { vi.mocked(helpers.isDryRun).mockReturnValue(true); const action = vi.fn().mockReturnValue('result'); - const result = dryRunExecSync(action, 'test action'); + const result = safeExecSync(action, 'test action'); expect(action).not.toHaveBeenCalled(); expect(result).toBeUndefined(); diff --git a/src/utils/dryRun.ts b/src/utils/dryRun.ts index 114d86758..9165861c5 100644 --- a/src/utils/dryRun.ts +++ b/src/utils/dryRun.ts @@ -218,7 +218,7 @@ export function createDryRunOctokit(octokit: Octokit): Octokit { * Write operations are blocked and logged in dry-run mode. * Read operations always execute normally. */ -export const dryRunFs = { +export const safeFs = { /** * Write data to a file asynchronously. */ @@ -391,7 +391,7 @@ export const dryRunFs = { * @param description Human-readable description for dry-run logging * @returns The result of the action, or undefined in dry-run mode */ -export async function dryRunExec( +export async function safeExec( action: () => Promise, description: string ): Promise { @@ -409,7 +409,7 @@ export async function dryRunExec( * @param description Human-readable description for dry-run logging * @returns The result of the action, or undefined in dry-run mode */ -export function dryRunExecSync( +export function safeExecSync( action: () => T, description: string ): T | undefined { diff --git a/src/utils/gcsApi.ts b/src/utils/gcsApi.ts index 36b803f91..4c174915c 100644 --- a/src/utils/gcsApi.ts +++ b/src/utils/gcsApi.ts @@ -10,7 +10,7 @@ import { import { logger } from '../logger'; import { reportError } from './errors'; -import { dryRunExec } from './dryRun'; +import { safeExec } from './dryRun'; import { RequiredConfigVar } from './env'; import { detectContentType } from './files'; import { RemoteArtifact } from '../artifact_providers/base'; @@ -208,7 +208,7 @@ export class CraftGCSClient { ); const destination = path.posix.join(this.bucketName, pathInBucket); - await dryRunExec(async () => { + await safeExec(async () => { logger.debug(`Attempting to upload \`${filename}\` to \`${destination}\`.`); try { @@ -254,7 +254,7 @@ export class CraftGCSClient { ); } - await dryRunExec(async () => { + await safeExec(async () => { logger.debug( `Attempting to download \`${destinationFilename}\` to \`${destinationDirectory}\`.` ); diff --git a/src/utils/git.ts b/src/utils/git.ts index c1ecc3f07..b3def6010 100644 --- a/src/utils/git.ts +++ b/src/utils/git.ts @@ -130,6 +130,32 @@ export function createGitClient(directory: string): SimpleGit { return createDryRunGit(simpleGit(directory)); } +/** + * Clones a git repository to a target directory. + * + * This is a convenience wrapper that handles the common pattern of cloning + * a repo and then creating a git client for the cloned directory. + * + * @param url The repository URL to clone from + * @param targetDirectory The directory to clone into + * @param options Optional clone options (e.g., ['--filter=tree:0']) + * @returns A SimpleGit instance for the cloned repository + */ +export async function cloneRepo( + url: string, + targetDirectory: string, + options?: string[] +): Promise { + // eslint-disable-next-line no-restricted-syntax -- This is the git wrapper module + const git = simpleGit(); + if (options) { + await git.clone(url, targetDirectory, options); + } else { + await git.clone(url, targetDirectory); + } + return createGitClient(targetDirectory); +} + /** * Checks if the git repository has uncommitted changes * From 0d2c40e3e6cc36c4344a074bbbf0e4210818af60 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 31 Dec 2025 15:37:32 +0300 Subject: [PATCH 4/8] Address code review feedback for dry-run abstraction - Fix GCS download to return null in dry-run mode (instead of invalid path) - Use explicit isDryRun() check in createDraftRelease for mock data - Cache wrapped git instances in proxy to avoid recreation on chaining - Add copyFile/copyFileSync to safeFs - Fix import ordering in prepare.ts - Add tests for chained git operations and proxy caching --- src/artifact_providers/gcs.ts | 8 +++++- src/commands/prepare.ts | 2 +- src/targets/github.ts | 21 ++++++++------ src/utils/__tests__/dryRun.test.ts | 45 ++++++++++++++++++++++++++++++ src/utils/dryRun.ts | 45 ++++++++++++++++++++++++++++-- src/utils/gcsApi.ts | 14 ++++++---- 6 files changed, 116 insertions(+), 19 deletions(-) diff --git a/src/artifact_providers/gcs.ts b/src/artifact_providers/gcs.ts index d96ce8487..01caa49b6 100644 --- a/src/artifact_providers/gcs.ts +++ b/src/artifact_providers/gcs.ts @@ -53,10 +53,16 @@ export class GCSArtifactProvider extends BaseArtifactProvider { artifact: RemoteArtifact, downloadDirectory: string ): Promise { - return this.gcsClient.downloadArtifact( + const result = await this.gcsClient.downloadArtifact( artifact.storedFile.downloadFilepath, downloadDirectory ); + // In dry-run mode, downloadArtifact returns null. Return a placeholder path + // that indicates the file would have been downloaded here. + if (result === null) { + return `${downloadDirectory}/${artifact.filename} [dry-run: not downloaded]`; + } + return result; } /** diff --git a/src/commands/prepare.ts b/src/commands/prepare.ts index 5d283fd0b..745f86526 100644 --- a/src/commands/prepare.ts +++ b/src/commands/prepare.ts @@ -1,7 +1,7 @@ import { existsSync, promises as fsPromises } from 'fs'; +import { join, relative } from 'path'; import { safeFs } from '../utils/dryRun'; -import { join, relative } from 'path'; import * as shellQuote from 'shell-quote'; import { SimpleGit, StatusResult } from 'simple-git'; import { Arguments, Argv, CommandBuilder } from 'yargs'; diff --git a/src/targets/github.ts b/src/targets/github.ts index bbf258a13..4829e939b 100644 --- a/src/targets/github.ts +++ b/src/targets/github.ts @@ -14,6 +14,7 @@ import { findChangeset, } from '../utils/changelog'; import { getGitHubClient } from '../utils/githubApi'; +import { isDryRun } from '../utils/helpers'; import { safeExec } from '../utils/dryRun'; import { isPreviewRelease, @@ -132,6 +133,17 @@ export class GitHubTarget extends BaseTarget { const isPreview = this.githubConfig.previewReleases && isPreviewRelease(version); + // In dry-run mode, return mock release data since the API call is blocked + if (isDryRun()) { + this.logger.info('[dry-run] Would create draft release'); + return { + id: 0, + tag_name: tag, + upload_url: '', + draft: true, + }; + } + const { data } = await this.github.repos.createRelease({ draft: true, name: tag, @@ -143,15 +155,6 @@ export class GitHubTarget extends BaseTarget { ...changes, }); - // In dry-run mode, the proxy returns an empty object - provide mock data - if (!data.id) { - return { - id: 0, - tag_name: tag, - upload_url: '', - draft: true, - }; - } return data; } diff --git a/src/utils/__tests__/dryRun.test.ts b/src/utils/__tests__/dryRun.test.ts index 1dfd66140..8e93bd6f9 100644 --- a/src/utils/__tests__/dryRun.test.ts +++ b/src/utils/__tests__/dryRun.test.ts @@ -121,6 +121,51 @@ describe('dryRun utilities', () => { await git.revparse('HEAD'); expect(mockGit.revparse).toHaveBeenCalledWith('HEAD'); }); + + it('supports method chaining in dry-run mode', async () => { + vi.mocked(helpers.isDryRun).mockReturnValue(true); + + // Create a mock that supports chaining by returning itself + const chainableMockGit = { + pull: vi.fn().mockReturnThis(), + merge: vi.fn().mockReturnThis(), + push: vi.fn().mockReturnThis(), + add: vi.fn().mockReturnThis(), + commit: vi.fn().mockReturnThis(), + status: vi.fn().mockResolvedValue({ current: 'main' }), + }; + + const git = createDryRunGit(chainableMockGit as any); + + // Test chaining: pull().merge().push() + const result = await git.pull('origin', 'main'); + // In dry-run mode, mutating methods should return the proxy for chaining + expect(result).toBeDefined(); + // The actual methods should not be called + expect(chainableMockGit.pull).not.toHaveBeenCalled(); + + // Ensure we can continue chaining + const result2 = await (result as any).merge(['--no-ff', 'branch']); + expect(result2).toBeDefined(); + expect(chainableMockGit.merge).not.toHaveBeenCalled(); + + const result3 = await (result2 as any).push('origin', 'main'); + expect(result3).toBeDefined(); + expect(chainableMockGit.push).not.toHaveBeenCalled(); + + // Verify dry-run messages were logged + expect(logger.info).toHaveBeenCalledWith( + expect.stringContaining('[dry-run]') + ); + }); + + it('caches proxy instances for the same git object', () => { + const git1 = createDryRunGit(mockGit as any); + const git2 = createDryRunGit(mockGit as any); + + // Same underlying object should return the same proxy + expect(git1).toBe(git2); + }); }); describe('createDryRunOctokit', () => { diff --git a/src/utils/dryRun.ts b/src/utils/dryRun.ts index 9165861c5..404b4f7e5 100644 --- a/src/utils/dryRun.ts +++ b/src/utils/dryRun.ts @@ -66,6 +66,9 @@ const GIT_RAW_MUTATING_COMMANDS = new Set([ // which is safe to do in dry-run mode and needed for subsequent operations ]); +// WeakMap to cache wrapped git instances, avoiding recreation on chaining +const gitProxyCache = new WeakMap(); + /** * Creates a dry-run-aware wrapper around a SimpleGit instance. * @@ -76,7 +79,13 @@ const GIT_RAW_MUTATING_COMMANDS = new Set([ * @returns A proxied SimpleGit that respects dry-run mode */ export function createDryRunGit(git: SimpleGit): SimpleGit { - return new Proxy(git, { + // Return cached proxy if we've already wrapped this instance + const cached = gitProxyCache.get(git); + if (cached) { + return cached; + } + + const proxy = new Proxy(git, { get(target, prop: string) { const value = target[prop as keyof SimpleGit]; @@ -107,8 +116,8 @@ export function createDryRunGit(git: SimpleGit): SimpleGit { .join(' '); logDryRun(`git.${prop}(${argsStr})`); // Return a resolved promise for async compatibility - // Some git methods return the git instance for chaining - return Promise.resolve(createDryRunGit(target)); + // Return the same proxy for chaining (already cached) + return Promise.resolve(proxy); } return value.apply(target, args); }; @@ -118,6 +127,10 @@ export function createDryRunGit(git: SimpleGit): SimpleGit { return value.bind(target); }, }); + + // Cache the proxy for this git instance + gitProxyCache.set(git, proxy); + return proxy; } // ============================================================================ @@ -375,6 +388,32 @@ export const safeFs = { } return fs.appendFileSync(filePath, data, options); }, + + /** + * Copy a file asynchronously. + */ + copyFile: async ( + src: string, + dest: string, + mode?: number + ): Promise => { + if (isDryRun()) { + logDryRun(`fs.copyFile(${src}, ${dest})`); + return; + } + return fsPromises.copyFile(src, dest, mode); + }, + + /** + * Copy a file synchronously. + */ + copyFileSync: (src: string, dest: string, mode?: number): void => { + if (isDryRun()) { + logDryRun(`fs.copyFileSync(${src}, ${dest})`); + return; + } + return fs.copyFileSync(src, dest, mode); + }, }; // ============================================================================ diff --git a/src/utils/gcsApi.ts b/src/utils/gcsApi.ts index 4c174915c..aa684ab13 100644 --- a/src/utils/gcsApi.ts +++ b/src/utils/gcsApi.ts @@ -240,13 +240,13 @@ export class CraftGCSClient { * file * @param destinationFilename Name to give the downloaded file, if different from its * name on the artifact provider - * @returns Path to the downloaded file + * @returns Path to the downloaded file, or `null` in dry-run mode (file won't exist) */ public async downloadArtifact( downloadFilepath: string, destinationDirectory: string, destinationFilename: string = path.basename(downloadFilepath) - ): Promise { + ): Promise { if (!fs.existsSync(destinationDirectory)) { reportError( `Unable to download \`${destinationFilename}\` to ` + @@ -254,14 +254,16 @@ export class CraftGCSClient { ); } - await safeExec(async () => { + const localPath = path.join(destinationDirectory, destinationFilename); + + const result = await safeExec(async () => { logger.debug( `Attempting to download \`${destinationFilename}\` to \`${destinationDirectory}\`.` ); try { await this.bucket.file(downloadFilepath).download({ - destination: path.join(destinationDirectory, destinationFilename), + destination: localPath, }); } catch (err) { reportError(`Encountered an error while downloading \`${destinationFilename}\`: @@ -269,9 +271,11 @@ export class CraftGCSClient { } logger.debug(`Successfully downloaded \`${destinationFilename}\`.`); + return localPath; }, `download ${destinationFilename} to ${destinationDirectory}`); - return path.join(destinationDirectory, destinationFilename); + // In dry-run mode, safeExec returns undefined - return null to indicate file doesn't exist + return result ?? null; } /** From 7133f3c90eb82f71c237bf8a138bf4857b481343 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 31 Dec 2025 16:36:46 +0300 Subject: [PATCH 5/8] fix: return mock results for git methods that return data in dry-run mode In dry-run mode, git methods like commit() now return proper mock result objects instead of the proxy itself. This fixes issues where code expects to access properties like commitResult.commit to get the commit hash. --- src/utils/__tests__/dryRun.test.ts | 55 +++++++++++++++--------------- src/utils/dryRun.ts | 23 +++++++++++-- 2 files changed, 47 insertions(+), 31 deletions(-) diff --git a/src/utils/__tests__/dryRun.test.ts b/src/utils/__tests__/dryRun.test.ts index 8e93bd6f9..aa55c20c4 100644 --- a/src/utils/__tests__/dryRun.test.ts +++ b/src/utils/__tests__/dryRun.test.ts @@ -50,6 +50,8 @@ describe('dryRun utilities', () => { push: vi.fn().mockResolvedValue(undefined), commit: vi.fn().mockResolvedValue({ commit: 'abc123' }), checkout: vi.fn().mockResolvedValue(undefined), + pull: vi.fn().mockResolvedValue({ files: [] }), + add: vi.fn().mockResolvedValue(undefined), status: vi.fn().mockResolvedValue({ current: 'main' }), log: vi.fn().mockResolvedValue({ all: [] }), raw: vi.fn().mockResolvedValue(''), @@ -122,36 +124,33 @@ describe('dryRun utilities', () => { expect(mockGit.revparse).toHaveBeenCalledWith('HEAD'); }); - it('supports method chaining in dry-run mode', async () => { + it('returns mock results for methods that return data in dry-run mode', async () => { vi.mocked(helpers.isDryRun).mockReturnValue(true); - // Create a mock that supports chaining by returning itself - const chainableMockGit = { - pull: vi.fn().mockReturnThis(), - merge: vi.fn().mockReturnThis(), - push: vi.fn().mockReturnThis(), - add: vi.fn().mockReturnThis(), - commit: vi.fn().mockReturnThis(), - status: vi.fn().mockResolvedValue({ current: 'main' }), - }; - - const git = createDryRunGit(chainableMockGit as any); - - // Test chaining: pull().merge().push() - const result = await git.pull('origin', 'main'); - // In dry-run mode, mutating methods should return the proxy for chaining - expect(result).toBeDefined(); - // The actual methods should not be called - expect(chainableMockGit.pull).not.toHaveBeenCalled(); - - // Ensure we can continue chaining - const result2 = await (result as any).merge(['--no-ff', 'branch']); - expect(result2).toBeDefined(); - expect(chainableMockGit.merge).not.toHaveBeenCalled(); - - const result3 = await (result2 as any).push('origin', 'main'); - expect(result3).toBeDefined(); - expect(chainableMockGit.push).not.toHaveBeenCalled(); + const git = createDryRunGit(mockGit as any); + + // commit() should return a mock CommitResult with a commit hash + const commitResult = await git.commit('test commit'); + expect(commitResult).toBeDefined(); + expect((commitResult as any).commit).toBe('dry-run-commit-hash'); + expect(mockGit.commit).not.toHaveBeenCalled(); + + // pull() should return a mock PullResult + const pullResult = await git.pull('origin', 'main'); + expect(pullResult).toBeDefined(); + expect((pullResult as any).files).toBeDefined(); + expect(mockGit.pull).not.toHaveBeenCalled(); + + // push() should return a mock PushResult + const pushResult = await git.push('origin', 'main'); + expect(pushResult).toBeDefined(); + expect((pushResult as any).pushed).toBeDefined(); + expect(mockGit.push).not.toHaveBeenCalled(); + + // Methods without mock results should return the proxy for chaining + const addResult = await git.add(['.']); + expect(addResult).toBe(git); + expect(mockGit.add).not.toHaveBeenCalled(); // Verify dry-run messages were logged expect(logger.info).toHaveBeenCalledWith( diff --git a/src/utils/dryRun.ts b/src/utils/dryRun.ts index 404b4f7e5..de30c9b3d 100644 --- a/src/utils/dryRun.ts +++ b/src/utils/dryRun.ts @@ -69,6 +69,22 @@ const GIT_RAW_MUTATING_COMMANDS = new Set([ // WeakMap to cache wrapped git instances, avoiding recreation on chaining const gitProxyCache = new WeakMap(); +/** + * Mock results for git methods that return data structures (not just for chaining). + * Methods not listed here will return the proxy for chaining compatibility. + */ +const GIT_MOCK_RESULTS: Record = { + commit: { + commit: 'dry-run-commit-hash', + author: null, + branch: '', + root: false, + summary: { changes: 0, insertions: 0, deletions: 0 }, + }, + push: { pushed: [], remoteMessages: { all: [] } }, + pull: { files: [], insertions: {}, deletions: {}, summary: { changes: 0, insertions: 0, deletions: 0 } }, +}; + /** * Creates a dry-run-aware wrapper around a SimpleGit instance. * @@ -115,9 +131,10 @@ export function createDryRunGit(git: SimpleGit): SimpleGit { .map(a => (typeof a === 'string' ? a : JSON.stringify(a))) .join(' '); logDryRun(`git.${prop}(${argsStr})`); - // Return a resolved promise for async compatibility - // Return the same proxy for chaining (already cached) - return Promise.resolve(proxy); + // Return a mock result if available (for methods that return data), + // otherwise return the proxy for chaining compatibility + const mockResult = GIT_MOCK_RESULTS[prop]; + return Promise.resolve(mockResult ?? proxy); } return value.apply(target, args); }; From 77447c95648efb642e35b938d807e042aa8dfc92 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 31 Dec 2025 16:53:38 +0300 Subject: [PATCH 6/8] fix: only add mock results for git methods where result is accessed Remove pull and push from GIT_MOCK_RESULTS because they are used in method chains like git.pull().merge().push() in publish.ts. Returning a mock object instead of the proxy breaks these chains. Only commit needs a mock result because upm.ts accesses commitResult.commit. --- src/utils/__tests__/dryRun.test.ts | 16 +++++++--------- src/utils/dryRun.ts | 11 ++++++++--- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/utils/__tests__/dryRun.test.ts b/src/utils/__tests__/dryRun.test.ts index aa55c20c4..e2b0815c1 100644 --- a/src/utils/__tests__/dryRun.test.ts +++ b/src/utils/__tests__/dryRun.test.ts @@ -124,32 +124,30 @@ describe('dryRun utilities', () => { expect(mockGit.revparse).toHaveBeenCalledWith('HEAD'); }); - it('returns mock results for methods that return data in dry-run mode', async () => { + it('returns mock results for methods that need them in dry-run mode', async () => { vi.mocked(helpers.isDryRun).mockReturnValue(true); const git = createDryRunGit(mockGit as any); // commit() should return a mock CommitResult with a commit hash + // because upm.ts accesses commitResult.commit const commitResult = await git.commit('test commit'); expect(commitResult).toBeDefined(); expect((commitResult as any).commit).toBe('dry-run-commit-hash'); expect(mockGit.commit).not.toHaveBeenCalled(); - // pull() should return a mock PullResult + // Methods without mock results should return the proxy for chaining + // This is important for chains like git.pull().merge().push() const pullResult = await git.pull('origin', 'main'); - expect(pullResult).toBeDefined(); - expect((pullResult as any).files).toBeDefined(); + expect(pullResult).toBe(git); // Returns proxy for chaining expect(mockGit.pull).not.toHaveBeenCalled(); - // push() should return a mock PushResult const pushResult = await git.push('origin', 'main'); - expect(pushResult).toBeDefined(); - expect((pushResult as any).pushed).toBeDefined(); + expect(pushResult).toBe(git); // Returns proxy for chaining expect(mockGit.push).not.toHaveBeenCalled(); - // Methods without mock results should return the proxy for chaining const addResult = await git.add(['.']); - expect(addResult).toBe(git); + expect(addResult).toBe(git); // Returns proxy for chaining expect(mockGit.add).not.toHaveBeenCalled(); // Verify dry-run messages were logged diff --git a/src/utils/dryRun.ts b/src/utils/dryRun.ts index de30c9b3d..a9014883e 100644 --- a/src/utils/dryRun.ts +++ b/src/utils/dryRun.ts @@ -70,10 +70,15 @@ const GIT_RAW_MUTATING_COMMANDS = new Set([ const gitProxyCache = new WeakMap(); /** - * Mock results for git methods that return data structures (not just for chaining). + * Mock results for git methods that return data structures consumers access. * Methods not listed here will return the proxy for chaining compatibility. + * + * IMPORTANT: Only add methods here if their return value properties are actually + * accessed in the codebase. Methods used in chains (like pull, push, branch) + * should NOT be listed here, as returning a mock object breaks chaining. */ const GIT_MOCK_RESULTS: Record = { + // commit: Used in upm.ts where commitResult.commit is accessed commit: { commit: 'dry-run-commit-hash', author: null, @@ -81,8 +86,8 @@ const GIT_MOCK_RESULTS: Record = { root: false, summary: { changes: 0, insertions: 0, deletions: 0 }, }, - push: { pushed: [], remoteMessages: { all: [] } }, - pull: { files: [], insertions: {}, deletions: {}, summary: { changes: 0, insertions: 0, deletions: 0 } }, + // NOTE: pull and push are intentionally NOT included here because they are + // used in method chains like git.pull().merge().push() in publish.ts }; /** From 793fdcd5c88322346dc31ed648b3409e175a6647 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 31 Dec 2025 17:02:23 +0300 Subject: [PATCH 7/8] refactor: use Proxy for file system operations in safeFs Refactor safeFs to use the same Proxy pattern as Git and Octokit for consistency. This also exports safeFsPromises and safeFsSync for direct access to the full proxied fs modules. --- src/utils/dryRun.ts | 286 ++++++++++++++++---------------------------- 1 file changed, 106 insertions(+), 180 deletions(-) diff --git a/src/utils/dryRun.ts b/src/utils/dryRun.ts index a9014883e..dbb094d70 100644 --- a/src/utils/dryRun.ts +++ b/src/utils/dryRun.ts @@ -248,194 +248,120 @@ export function createDryRunOctokit(octokit: Octokit): Octokit { // ============================================================================ /** - * Dry-run-aware file system operations. - * - * Write operations are blocked and logged in dry-run mode. - * Read operations always execute normally. + * File system methods that modify state and should be blocked in dry-run mode. + * Maps method names to the number of path arguments to include in the log. */ -export const safeFs = { - /** - * Write data to a file asynchronously. - */ - writeFile: async ( - filePath: string, - data: string | Buffer, - options?: fs.WriteFileOptions - ): Promise => { - if (isDryRun()) { - logDryRun(`fs.writeFile(${filePath})`); - return; - } - return fsPromises.writeFile(filePath, data, options); - }, - - /** - * Write data to a file synchronously. - */ - writeFileSync: ( - filePath: string, - data: string | Buffer, - options?: fs.WriteFileOptions - ): void => { - if (isDryRun()) { - logDryRun(`fs.writeFileSync(${filePath})`); - return; - } - return fs.writeFileSync(filePath, data, options); - }, - - /** - * Delete a file asynchronously. - */ - unlink: async (filePath: string): Promise => { - if (isDryRun()) { - logDryRun(`fs.unlink(${filePath})`); - return; - } - return fsPromises.unlink(filePath); - }, - - /** - * Delete a file synchronously. - */ - unlinkSync: (filePath: string): void => { - if (isDryRun()) { - logDryRun(`fs.unlinkSync(${filePath})`); - return; - } - return fs.unlinkSync(filePath); - }, - - /** - * Rename a file asynchronously. - */ - rename: async (oldPath: string, newPath: string): Promise => { - if (isDryRun()) { - logDryRun(`fs.rename(${oldPath}, ${newPath})`); - return; - } - return fsPromises.rename(oldPath, newPath); - }, - - /** - * Rename a file synchronously. - */ - renameSync: (oldPath: string, newPath: string): void => { - if (isDryRun()) { - logDryRun(`fs.renameSync(${oldPath}, ${newPath})`); - return; - } - return fs.renameSync(oldPath, newPath); - }, - - /** - * Remove a directory recursively asynchronously. - */ - rm: async ( - filePath: string, - options?: fs.RmOptions - ): Promise => { - if (isDryRun()) { - logDryRun(`fs.rm(${filePath})`); - return; - } - return fsPromises.rm(filePath, options); - }, +const FS_MUTATING_METHODS: Record = { + // Single path methods + writeFile: 1, + writeFileSync: 1, + unlink: 1, + unlinkSync: 1, + rm: 1, + rmSync: 1, + rmdir: 1, + rmdirSync: 1, + mkdir: 1, + mkdirSync: 1, + appendFile: 1, + appendFileSync: 1, + chmod: 1, + chmodSync: 1, + chown: 1, + chownSync: 1, + truncate: 1, + truncateSync: 1, + // Two path methods (source, dest) + rename: 2, + renameSync: 2, + copyFile: 2, + copyFileSync: 2, + symlink: 2, + symlinkSync: 2, + link: 2, + linkSync: 2, +}; - /** - * Remove a directory recursively synchronously. - */ - rmSync: (filePath: string, options?: fs.RmOptions): void => { - if (isDryRun()) { - logDryRun(`fs.rmSync(${filePath})`); - return; - } - return fs.rmSync(filePath, options); - }, +/** + * Creates a proxy handler for file system modules. + * Intercepts mutating operations and blocks them in dry-run mode. + */ +function createFsProxyHandler( + isAsync: boolean +): ProxyHandler { + return { + get(target, prop: string) { + const value = target[prop as keyof typeof target]; - /** - * Create a directory asynchronously. - */ - mkdir: async ( - dirPath: string, - options?: fs.MakeDirectoryOptions - ): Promise => { - if (isDryRun()) { - logDryRun(`fs.mkdir(${dirPath})`); - return undefined; - } - return fsPromises.mkdir(dirPath, options); - }, + // If it's not a function, return as-is + if (typeof value !== 'function') { + return value; + } - /** - * Create a directory synchronously. - */ - mkdirSync: ( - dirPath: string, - options?: fs.MakeDirectoryOptions - ): string | undefined => { - if (isDryRun()) { - logDryRun(`fs.mkdirSync(${dirPath})`); - return undefined; - } - return fs.mkdirSync(dirPath, options); - }, + // Check if this is a mutating method + const pathArgCount = FS_MUTATING_METHODS[prop]; + if (pathArgCount !== undefined) { + return function (...args: unknown[]) { + if (isDryRun()) { + const paths = args.slice(0, pathArgCount).join(', '); + logDryRun(`fs.${prop}(${paths})`); + // Return appropriate value for async vs sync + return isAsync ? Promise.resolve(undefined) : undefined; + } + return (value as (...a: unknown[]) => unknown).apply(target, args); + }; + } - /** - * Append data to a file asynchronously. - */ - appendFile: async ( - filePath: string, - data: string | Buffer, - options?: fs.WriteFileOptions - ): Promise => { - if (isDryRun()) { - logDryRun(`fs.appendFile(${filePath})`); - return; - } - return fsPromises.appendFile(filePath, data, options); - }, + // For non-mutating methods, bind and return + return (value as (...a: unknown[]) => unknown).bind(target); + }, + }; +} - /** - * Append data to a file synchronously. - */ - appendFileSync: ( - filePath: string, - data: string | Buffer, - options?: fs.WriteFileOptions - ): void => { - if (isDryRun()) { - logDryRun(`fs.appendFileSync(${filePath})`); - return; - } - return fs.appendFileSync(filePath, data, options); - }, +/** + * Dry-run-aware file system operations (async). + * + * Write operations are blocked and logged in dry-run mode. + * Read operations always execute normally. + */ +export const safeFsPromises = new Proxy( + fsPromises, + createFsProxyHandler(true) +) as typeof fsPromises; - /** - * Copy a file asynchronously. - */ - copyFile: async ( - src: string, - dest: string, - mode?: number - ): Promise => { - if (isDryRun()) { - logDryRun(`fs.copyFile(${src}, ${dest})`); - return; - } - return fsPromises.copyFile(src, dest, mode); - }, +/** + * Dry-run-aware file system operations (sync). + * + * Write operations are blocked and logged in dry-run mode. + * Read operations always execute normally. + */ +export const safeFsSync = new Proxy( + fs, + createFsProxyHandler(false) +) as typeof fs; - /** - * Copy a file synchronously. - */ - copyFileSync: (src: string, dest: string, mode?: number): void => { - if (isDryRun()) { - logDryRun(`fs.copyFileSync(${src}, ${dest})`); - return; - } - return fs.copyFileSync(src, dest, mode); - }, +/** + * Convenience object that provides the most commonly used fs operations. + * Combines async and sync methods in one object for backwards compatibility. + */ +export const safeFs = { + // Async methods (from fs/promises) + writeFile: safeFsPromises.writeFile.bind(safeFsPromises), + unlink: safeFsPromises.unlink.bind(safeFsPromises), + rename: safeFsPromises.rename.bind(safeFsPromises), + rm: safeFsPromises.rm.bind(safeFsPromises), + mkdir: safeFsPromises.mkdir.bind(safeFsPromises), + appendFile: safeFsPromises.appendFile.bind(safeFsPromises), + copyFile: safeFsPromises.copyFile.bind(safeFsPromises), + + // Sync methods (from fs) + writeFileSync: safeFsSync.writeFileSync.bind(safeFsSync), + unlinkSync: safeFsSync.unlinkSync.bind(safeFsSync), + renameSync: safeFsSync.renameSync.bind(safeFsSync), + rmSync: safeFsSync.rmSync.bind(safeFsSync), + mkdirSync: safeFsSync.mkdirSync.bind(safeFsSync), + appendFileSync: safeFsSync.appendFileSync.bind(safeFsSync), + copyFileSync: safeFsSync.copyFileSync.bind(safeFsSync), }; // ============================================================================ From 2b381e834589aba5831282c3ba17a8dc4140f0ef Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 31 Dec 2025 17:09:16 +0300 Subject: [PATCH 8/8] fix: return proxy directly for git chaining compatibility Return proxy directly instead of Promise.resolve(proxy) to support method chaining like git.pull().merge().push(). The proxy wraps SimpleGit which is thenable, so await still works correctly. Only methods in GIT_MOCK_RESULTS (like commit) return a Promise with mock data since their return values are actually accessed. --- src/utils/dryRun.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/utils/dryRun.ts b/src/utils/dryRun.ts index dbb094d70..36d7dfe63 100644 --- a/src/utils/dryRun.ts +++ b/src/utils/dryRun.ts @@ -136,10 +136,16 @@ export function createDryRunGit(git: SimpleGit): SimpleGit { .map(a => (typeof a === 'string' ? a : JSON.stringify(a))) .join(' '); logDryRun(`git.${prop}(${argsStr})`); - // Return a mock result if available (for methods that return data), - // otherwise return the proxy for chaining compatibility + // Return a mock result if the method's return value is accessed, + // otherwise return the proxy directly for chaining compatibility. + // SimpleGit is thenable, so `await proxy` works correctly. const mockResult = GIT_MOCK_RESULTS[prop]; - return Promise.resolve(mockResult ?? proxy); + if (mockResult) { + return Promise.resolve(mockResult); + } + // Return proxy directly (not wrapped in Promise) to support + // chaining like git.pull().merge().push() + return proxy; } return value.apply(target, args); };