diff --git a/AGENTS.md b/AGENTS.md index f555a090..90048ee0 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 `safeFs` from `src/utils/dryRun.ts` +- **Other actions**: Use `safeExec()` or `safeExecSync()` 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 `safeFs.writeFile()`, `safeFs.unlink()`, etc. instead of raw `fs` methods +4. **Other**: Wrap with `safeExec(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 bfade070..cb16d013 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/artifact_providers/gcs.ts b/src/artifact_providers/gcs.ts index d96ce848..01caa49b 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 b553cf4e..81336d2d 100644 --- a/src/commands/prepare.ts +++ b/src/commands/prepare.ts @@ -1,5 +1,7 @@ import { existsSync, promises as fsPromises } from 'fs'; import { join, relative } from 'path'; + +import { safeFs } from '../utils/dryRun'; import * as shellQuote from 'shell-quote'; import { SimpleGit, StatusResult } from 'simple-git'; import { Arguments, Argv, CommandBuilder } from 'yargs'; @@ -213,13 +215,9 @@ 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'); - } + await git.checkoutBranch(branchName, rev); + logger.info(`Created a new release branch: "${branchName}"`); + logger.info(`Switched to branch "${branchName}"`); return branchName; } @@ -239,11 +237,7 @@ 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.'); - } + await git.push(remoteName, branchName, ['--set-upstream']); } else { logger.info('Not pushing the release branch.'); logger.info( @@ -271,11 +265,7 @@ 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.'); - } + await git.commit(message, ['--all']); } /** @@ -477,12 +467,7 @@ 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}`); - } + await safeFs.writeFile(relativePath, changelogString); break; default: @@ -513,11 +498,7 @@ 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.'); - } + await git.checkout(defaultBranch); } interface ResolveVersionOptions { diff --git a/src/commands/publish.ts b/src/commands/publish.ts index 07ab1355..f15c05da 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 { safeFs } 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,17 @@ 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'); - } + 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'); - } + await git.branch(['-D', branch]).push([remoteName, '--delete', branch]); + logger.info(`Removed the remote branch: "${branch}"`); } } @@ -575,9 +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; - if (!isDryRun()) { - writeFileSync(publishStateFile, JSON.stringify(publishState)); - } + safeFs.writeFileSync(publishStateFile, JSON.stringify(publishState)); } if (argv.keepDownloads) { @@ -610,19 +597,15 @@ 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. + safeFs + .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 23124f87..765b7db3 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 0cfb16d6..a5096020 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 ea0826de..b372ce44 100644 --- a/src/targets/awsLambdaLayer.ts +++ b/src/targets/awsLambdaLayer.ts @@ -2,7 +2,6 @@ import * as fs from 'fs'; import * as path from 'path'; import { Octokit } from '@octokit/rest'; -import simpleGit from 'simple-git'; import { getGitHubApiToken, getGitHubClient, @@ -21,7 +20,8 @@ import { } from '../utils/awsLambdaLayerManager'; import { createSymlinks } from '../utils/symlink'; import { withTempDir } from '../utils/files'; -import { isDryRun } from '../utils/helpers'; +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'; @@ -171,23 +171,23 @@ 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); - - if (!isDryRun()) { - await this.publishRuntimes( - version, - directory, - awsRegions, - artifactBuffer - ); - this.logger.debug('Finished publishing runtimes.'); - } else { - this.logger.info('[dry-run] Not publishing new layers.'); - } + const git = await cloneRepo(remote.getRemoteStringWithAuth(), directory); + + await safeExec( + async () => { + await this.publishRuntimes( + version, + directory, + awsRegions, + artifactBuffer + ); + this.logger.debug('Finished publishing runtimes.'); + }, + 'publishRuntimes(...)' + ); await git.add(['.']); await git.checkout('master'); @@ -221,10 +221,6 @@ export class AwsLambdaLayerTarget extends BaseTarget { * @param linkPrereleases Whether the current release is a prerelease. */ private isPushableToRegistry(version: string): boolean { - if (isDryRun()) { - this.logger.info('[dry-run] Not pushing the branch.'); - 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 60d68973..04e881a5 100644 --- a/src/targets/brew.ts +++ b/src/targets/brew.ts @@ -4,7 +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 { renderTemplateSafe } from '../utils/strings'; import { HashAlgorithm, HashOutputFormat } from '../utils/system'; import { BaseTarget } from './base'; @@ -206,11 +205,7 @@ export class BrewTarget extends BaseTarget { `${action} file ${params.owner}/${params.repo}:${params.path} (${params.sha})` ); - if (!isDryRun()) { - await this.github.repos.createOrUpdateFileContents(params); - } 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 76414674..c30e750d 100644 --- a/src/targets/commitOnGitRepository.ts +++ b/src/targets/commitOnGitRepository.ts @@ -1,12 +1,11 @@ -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 { cloneRepo, 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 +131,6 @@ export async function pushArchiveToGitRepository({ }) { await withTempDir( async directory => { - const git = simpleGit(directory); - logger?.info(`Cloning ${repositoryUrl} into ${directory}...`); let parsedUrl; @@ -153,7 +150,7 @@ export async function pushArchiveToGitRepository({ const authenticatedUrl = parsedUrl.toString(); - await git.clone(authenticatedUrl, directory); + const git = await cloneRepo(authenticatedUrl, directory); logger?.info(`Checking out branch "${branch}"...`); await git.checkout(branch); @@ -177,29 +174,21 @@ export async function pushArchiveToGitRepository({ await git.raw('add', '--all'); 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 d926a29b..33ca8598 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 c83dd7f6..96a67ca5 100644 --- a/src/targets/ghPages.ts +++ b/src/targets/ghPages.ts @@ -2,7 +2,6 @@ import * as fs from 'fs'; import * as path from 'path'; import { Octokit } from '@octokit/rest'; -import simpleGit from 'simple-git'; import { GitHubGlobalConfig, TargetConfig } from '../schemas/project_config'; import { ConfigurationError, reportError } from '../utils/errors'; @@ -12,7 +11,7 @@ import { getGitHubClient, GitHubRemote, } from '../utils/githubApi'; -import { isDryRun } from '../utils/helpers'; +import { cloneRepo, createGitClient } from '../utils/git'; import { extractZipArchive } from '../utils/system'; import { BaseTarget } from './base'; import { BaseArtifactProvider } from '../artifact_providers/base'; @@ -150,8 +149,7 @@ export class GhPagesTarget extends BaseTarget { this.logger.info( `Cloning "${remote.getRemoteString()}" to "${directory}"...` ); - await simpleGit().clone(remote.getRemoteStringWithAuth(), directory); - const git = simpleGit(directory); + const git = await cloneRepo(remote.getRemoteStringWithAuth(), directory); this.logger.debug(`Checking out branch: "${branch}"`); try { await git.checkout([branch]); @@ -180,17 +178,11 @@ export class GhPagesTarget extends BaseTarget { // Extract the archive await this.extractAssets(archivePath, directory); - // Commit await git.add(['.']); await git.commit(`craft(gh-pages): update, version "${version}"`); - // Push! 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 089b6e7b..4829e939 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 { safeExec } from '../utils/dryRun'; import { isPreviewRelease, parseVersion, @@ -132,8 +133,9 @@ 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] Not creating the draft release`); + this.logger.info('[dry-run] Would create draft release'); return { id: 0, tag_name: tag, @@ -152,6 +154,7 @@ export class GitHubTarget extends BaseTarget { target_commitish: revision, ...changes, }); + return data; } @@ -185,11 +188,6 @@ export class GitHubTarget extends BaseTarget { asset: ReposListAssetsForReleaseResponseItem ): Promise { this.logger.debug(`Deleting asset: "${asset.name}"...`); - if (isDryRun()) { - this.logger.info(`[dry-run] Not deleting "${asset.name}"`); - return false; - } - return ( ( await this.github.repos.deleteReleaseAsset({ @@ -219,11 +217,6 @@ export class GitHubTarget extends BaseTarget { return false; } - if (isDryRun()) { - this.logger.info(`[dry-run] Not deleting release "${release.tag_name}"`); - return false; - } - return ( ( await this.github.repos.deleteRelease({ @@ -290,23 +283,20 @@ export class GitHubTarget extends BaseTarget { ): Promise { const name = basename(path); - if (isDryRun()) { - this.logger.info(`[dry-run] Not uploading asset "${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( @@ -379,11 +369,6 @@ export class GitHubTarget extends BaseTarget { release: GitHubRelease, options: { makeLatest: boolean } = { makeLatest: true } ) { - if (isDryRun()) { - this.logger.info(`[dry-run] Not publishing the draft release`); - return; - } - await this.github.repos.updateRelease({ ...this.githubConfig, release_id: release.id, @@ -408,17 +393,13 @@ export class GitHubTarget extends BaseTarget { ): Promise { 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, - }); - } + 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, + }); } /** @@ -465,39 +446,34 @@ export class GitHubTarget extends BaseTarget { const tag = this.resolveFloatingTag(pattern, parsedVersion); const tagRef = `refs/tags/${tag}`; - if (isDryRun()) { - this.logger.info( - `[dry-run] Not updating floating tag: "${tag}" (from pattern "${pattern}")` - ); - 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/hex.ts b/src/targets/hex.ts index acc19432..ee415bce 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 202b2584..ec882faf 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 81e49d91..fd2de23a 100644 --- a/src/targets/registry.ts +++ b/src/targets/registry.ts @@ -1,6 +1,6 @@ import { mapLimit } from 'async'; import { Octokit } from '@octokit/rest'; -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'; @@ -31,7 +31,7 @@ import { RegistryPackageType, InitialManifestData, } from '../utils/registry'; -import { isDryRun } from '../utils/helpers'; +import { cloneRepo } from '../utils/git'; import { filterAsync, withRetry } from '../utils/async'; /** "registry" target options */ @@ -476,15 +476,13 @@ 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, [ + return cloneRepo(remote.getRemoteStringWithAuth(), directory, [ '--filter=tree:0', '--single-branch', ]); - return git; } public async getValidItems( @@ -547,24 +545,18 @@ export class RegistryTarget extends BaseTarget { ) ); - // Commit 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.'); - } + 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 1529aaa2..8609a5a2 100644 --- a/src/targets/upm.ts +++ b/src/targets/upm.ts @@ -1,5 +1,4 @@ import { Octokit } from '@octokit/rest'; -import simpleGit from 'simple-git'; import { getGitHubApiToken, getGitHubClient, @@ -16,7 +15,7 @@ import { import { reportError } from '../utils/errors'; import { extractZipArchive } from '../utils/system'; import { withTempDir } from '../utils/files'; -import { isDryRun } from '../utils/helpers'; +import { cloneRepo, createGitClient } from '../utils/git'; import { isPreviewRelease } from '../utils/version'; import { NoneArtifactProvider } from '../artifact_providers/none'; @@ -116,9 +115,7 @@ 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); + const git = await cloneRepo(remote.getRemoteStringWithAuth(), directory); this.logger.info('Clearing the repository.'); await git.rm(['-r', '-f', '.']); @@ -136,35 +133,31 @@ 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 - ); + 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 00000000..e2b0815c --- /dev/null +++ b/src/utils/__tests__/dryRun.test.ts @@ -0,0 +1,321 @@ +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, + safeFs, + safeExec, + safeExecSync, + 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), + 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(''), + 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'); + }); + + 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(); + + // 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).toBe(git); // Returns proxy for chaining + expect(mockGit.pull).not.toHaveBeenCalled(); + + const pushResult = await git.push('origin', 'main'); + expect(pushResult).toBe(git); // Returns proxy for chaining + expect(mockGit.push).not.toHaveBeenCalled(); + + const addResult = await git.add(['.']); + expect(addResult).toBe(git); // Returns proxy for chaining + expect(mockGit.add).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', () => { + 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('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 safeFs.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 safeFs.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 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('safeExec', () => { + it('executes action in normal mode', async () => { + vi.mocked(helpers.isDryRun).mockReturnValue(false); + const action = vi.fn().mockResolvedValue('result'); + + const result = await safeExec(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 safeExec(action, 'test action'); + + expect(action).not.toHaveBeenCalled(); + expect(result).toBeUndefined(); + expect(logger.info).toHaveBeenCalledWith( + '[dry-run] Would execute: test action' + ); + }); + }); + + describe('safeExecSync', () => { + it('executes action in normal mode', () => { + vi.mocked(helpers.isDryRun).mockReturnValue(false); + const action = vi.fn().mockReturnValue('result'); + + const result = safeExecSync(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 = safeExecSync(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 a9bf4607..a9c2e88d 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 00000000..36d7dfe6 --- /dev/null +++ b/src/utils/dryRun.ts @@ -0,0 +1,414 @@ +/** + * 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', + '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 +]); + +/** + * 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', + '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 +]); + +// WeakMap to cache wrapped git instances, avoiding recreation on chaining +const gitProxyCache = new WeakMap(); + +/** + * 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, + branch: '', + root: false, + 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 +}; + +/** + * 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 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]; + + // 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 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]; + 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); + }; + } + + // For non-mutating methods, bind and return + return value.bind(target); + }, + }); + + // Cache the proxy for this git instance + gitProxyCache.set(git, proxy); + return proxy; +} + +// ============================================================================ +// 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 + // 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); + }; + } + + // 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 +// ============================================================================ + +/** + * 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. + */ +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, +}; + +/** + * 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]; + + // If it's not a function, return as-is + if (typeof value !== 'function') { + return value; + } + + // 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); + }; + } + + // For non-mutating methods, bind and return + return (value as (...a: unknown[]) => unknown).bind(target); + }, + }; +} + +/** + * 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; + +/** + * 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; + +/** + * 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), +}; + +// ============================================================================ +// 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 safeExec( + 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 safeExecSync( + 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 686f1a18..aa684ab1 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 { safeExec } 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 safeExec(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}`); } /** @@ -246,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 ` + @@ -260,14 +254,16 @@ export class CraftGCSClient { ); } - if (!isDryRun()) { + 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}\`: @@ -275,11 +271,11 @@ export class CraftGCSClient { } logger.debug(`Successfully downloaded \`${destinationFilename}\`.`); - } else { - logger.info(`[dry-run] Skipping download for \`${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; } /** diff --git a/src/utils/git.ts b/src/utils/git.ts index 0237182a..b3def601 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,54 @@ 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)); +} + +/** + * 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); } /** diff --git a/src/utils/githubApi.ts b/src/utils/githubApi.ts index 4dbfab81..9fec9d29 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];