ref: Centralize dry-run logic with Proxy-based abstraction

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)

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',
+        },
+      ],
     },
   }
 );

src/artifact_providers/gcs.ts

@@ -53,10 +53,16 @@ export class GCSArtifactProvider extends BaseArtifactProvider {
     artifact: RemoteArtifact,
     downloadDirectory: string
   ): Promise<string> {
-    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;
   }
 
   /**

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 {

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<any> {
       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<any> {
       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 = [

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);

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();
     });
   });
 });