feat(core): extract sync handler

[galligan]

Summary

• Extract syncHandler unifying sync orchestration from CLI and MCP
• Accept scopes option for selective sync
• Progress reporting via SyncProgressCallback in handler input (FIRE-8)

Test plan

• bun run check passes
• bun test passes (288 tests)

🤘🏻 In-collaboration-with: Claude Code

Greptile Summary

Extracts syncHandler to unify sync orchestration between CLI and MCP, moving auth, plugin detection, and cache clearing logic into the core handler.

Critical Issue Found:
The PR claims to "Accept scopes option for selective sync" but the implementation is incomplete:

• SyncInput interface missing scopes parameter
• syncHandler never passes scope to syncRepo(), defaulting to "open" only
• Both CLI and MCP iterate through scopes but don't pass them to the handler
• This breaks the ability to sync closed/merged PRs when using --closed flag

Other Changes:

• Removed direct GitHubClient, detectAuth, clearRepo, and Graphite plugin logic from CLI/MCP
• Added progress callback support via SyncProgressCallback
• Handler returns per-repo results with SyncRepoResult[]
• Comprehensive test coverage for validation, auth, clear flag, and progress callbacks (but missing scope tests)

Confidence Score: 1/5

• This PR has a critical logic bug that breaks core functionality
• The scopes feature is advertised but completely non-functional - syncHandler always defaults to "open" scope, breaking closed/merged PR syncing. While tests pass, they don't cover scope functionality
• Pay close attention to packages/core/src/handlers/sync.ts - requires scopes parameter support and proper iteration logic

Important Files Changed

Filename	Overview
packages/core/src/handlers/sync.ts	Extracted sync handler with critical bug: missing scopes support despite PR claiming to accept scopes option
apps/cli/src/commands/sync.ts	Refactored to use syncHandler but fails to pass scope parameter, breaking multi-scope sync functionality
apps/mcp/src/index.ts	Updated to use syncHandler with incorrect iteration structure - iterates scopes but doesn't pass to handler
packages/core/tests/handlers/sync.test.ts	Tests for syncHandler covering validation, auth, clear flag, and progress callbacks - missing scope functionality tests

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[CLI/MCP calls syncHandler] --> B[Resolve repos from input/config/detection]
    B --> C[Validate repo formats]
    C --> D{Clear flag set?}
    D -->|Yes| E[Clear cache for each repo]
    D -->|No| F[Authenticate with GitHub]
    E --> F
    F --> G[Create GitHubClient]
    G --> H[Detect Graphite plugin availability]
    H --> I[Iterate through repos]
    I --> J[Fire onProgress start event]
    J --> K[Call syncRepo with plugins]
    K --> L{Sync successful?}
    L -->|Yes| M[Fire onProgress done event]
    L -->|No| N[Fire onProgress error event]
    M --> O{More repos?}
    N --> O
    O -->|Yes| I
    O -->|No| P{All failed?}
    P -->|Yes| Q[Return error]
    P -->|No| R[Return aggregate results]

Loading

_{Last reviewed commit: 1558176}

[linear]

FIRE-8 Extract sync handler to core

[galligan]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• docs: update architecture docs for Outfitter stack adoption #172
• feat(mcp): adopt @outfitter/mcp with createMcpServer() and defineTool() #171
• refactor(cli): wire exitWithError() across key commands #170
• feat(cli): adopt @outfitter/cli with createCLI() entry point #169
• feat(core): extract sync handler #168 👈 (View in Graphite)
• feat(core): extract PR mutation handlers (approve, reject, edit) #167
• feat(core): extract feedback handlers (reply, comment, ack, close) #166
• feat(core): extract read handlers (doctor, query) #165
• feat(core): define handler contract and extract statusHandler #164
• chore(core): upgrade @outfitter/contracts to ^0.4.1 #163
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[greptile-apps]

_{6 files reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

missing scopes parameter - PR summary claims "Accept scopes option for selective sync" but SyncInput has no scopes field

Suggested change

	export interface SyncInput {
	/**
	* Explicit repos to sync (owner/repo format).
	* If not provided, uses config.repos + auto-detection.
	*/
	repos?: string[] | undefined;
	/** Clear cached entries for each repo before syncing. */
	clear?: boolean | undefined;
	/** Force full sync (ignore incremental cursors). */
	full?: boolean | undefined;
	/** Maximum number of PRs to fetch per repo (defaults to config value). */
	maxPrs?: number | undefined;
	/** Progress callback for reporting sync status per repo. */
	onProgress?: SyncProgressCallback | undefined;
	}
	export interface SyncInput {
	/**
	* Explicit repos to sync (owner/repo format).
	* If not provided, uses config.repos + auto-detection.
	*/
	repos?: string[] | undefined;
	/** Clear cached entries for each repo before syncing. */
	clear?: boolean | undefined;
	/** Force full sync (ignore incremental cursors). */
	full?: boolean | undefined;
	/** Which PR scopes to sync (default: ["open"]). */
	scopes?: SyncScope[] | undefined;
	/** Maximum number of PRs to fetch per repo (defaults to config value). */
	maxPrs?: number | undefined;
	/** Progress callback for reporting sync status per repo. */
	onProgress?: SyncProgressCallback | undefined;
	}

Prompt To Fix With AI

This is a comment left during a code review.
Path: packages/core/src/handlers/sync.ts
Line: 33-47

Comment:
missing `scopes` parameter - PR summary claims "Accept `scopes` option for selective sync" but `SyncInput` has no scopes field

```suggestion
export interface SyncInput {
  /**
   * Explicit repos to sync (owner/repo format).
   * If not provided, uses config.repos + auto-detection.
   */
  repos?: string[] | undefined;
  /** Clear cached entries for each repo before syncing. */
  clear?: boolean | undefined;
  /** Force full sync (ignore incremental cursors). */
  full?: boolean | undefined;
  /** Which PR scopes to sync (default: ["open"]). */
  scopes?: SyncScope[] | undefined;
  /** Maximum number of PRs to fetch per repo (defaults to config value). */
  maxPrs?: number | undefined;
  /** Progress callback for reporting sync status per repo. */
  onProgress?: SyncProgressCallback | undefined;
}
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

scope parameter not passed to syncRepo() - will always default to "open" scope, breaking closed/merged PR sync

needs to iterate scopes from input.scopes and pass each to syncRepo():

const scopes = input.scopes ?? (['open'] satisfies SyncScope[]);

for (const scope of scopes) {
  for (const repo of repos) {
    // ... existing repo iteration logic
    const syncResult = await syncRepo(client, repo, {
      ...(input.full && { full: true }),
      scope,  // <-- add this
      plugins,
    });
  }
}

Prompt To Fix With AI

This is a comment left during a code review.
Path: packages/core/src/handlers/sync.ts
Line: 184-195

Comment:
`scope` parameter not passed to `syncRepo()` - will always default to "open" scope, breaking closed/merged PR sync

needs to iterate scopes from `input.scopes` and pass each to `syncRepo()`:

```typescript
const scopes = input.scopes ?? (['open'] satisfies SyncScope[]);

for (const scope of scopes) {
  for (const repo of repos) {
    // ... existing repo iteration logic
    const syncResult = await syncRepo(client, repo, {
      ...(input.full && { full: true }),
      scope,  // <-- add this
      plugins,
    });
  }
}
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

CLI iterates scopes but never passes them to syncHandler - all syncs will use default "open" scope

the handler needs to accept scopes:

Suggested change

	for (const scope of scopes) {
	const ctx: SyncContext = {
	const ctx: SyncDisplayContext = {
	config,
	repo,
	db,
	outputJson,
	isFullSync,
	scope,
	};
	const startTime = Date.now();
	const spinner = createSpinner(ctx, options);
	try {
	const result = await syncRepo(client, repo, {
	const result = await syncHandler(
	{
	repos: [repo],
	clear: options.clear,
	full: isFullSync,
	scope,
	plugins,
	});
	},
	// Delegate auth + plugin resolution + sync to handler
	for (const scope of scopes) {
	const ctx: SyncDisplayContext = {
	config,
	repo,
	outputJson,
	isFullSync,
	scope,
	};
	const spinner = createSpinner(ctx, options);
	const result = await syncHandler(
	{
	repos: [repo],
	clear: options.clear,
	full: isFullSync,
	scopes: [scope],
	},

Prompt To Fix With AI

This is a comment left during a code review.
Path: apps/cli/src/commands/sync.ts
Line: 211-226

Comment:
CLI iterates scopes but never passes them to `syncHandler` - all syncs will use default "open" scope

the handler needs to accept scopes:

```suggestion
  // Delegate auth + plugin resolution + sync to handler
  for (const scope of scopes) {
    const ctx: SyncDisplayContext = {
      config,
      repo,
      outputJson,
      isFullSync,
      scope,
    };
    const spinner = createSpinner(ctx, options);

    const result = await syncHandler(
      {
        repos: [repo],
        clear: options.clear,
        full: isFullSync,
        scopes: [scope],
      },
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

scopes iteration but never passes scope to syncHandler - handler will ignore the loop and always sync "open" scope

fix by passing scopes to handler:

Suggested change

	for (const scope of scopes) {
	const syncResult = await syncHandler(
	{
	repos,
	...(options.full && { full: true }),
	...(options.since && {
	since: (() => {
	const sinceResult = parseSince(options.since!);
	if (sinceResult.isErr()) {
	throw new Error(sinceResult.error.message);
	}
	return sinceResult.value;
	})(),
	}),
	scope,
	plugins: useGraphite ? [graphitePlugin] : [],
	});
	},
	for (const scope of scopes) {
	const syncResult = await syncHandler(
	{
	repos,
	scopes: [scope],
	...(options.full && { full: true }),
	},
	{ config, db, logger: silentLogger }
	);

Prompt To Fix With AI

This is a comment left during a code review.
Path: apps/mcp/src/index.ts
Line: 586-591

Comment:
`scopes` iteration but never passes scope to `syncHandler` - handler will ignore the loop and always sync "open" scope

fix by passing scopes to handler:

```suggestion
  for (const scope of scopes) {
    const syncResult = await syncHandler(
      {
        repos,
        scopes: [scope],
        ...(options.full && { full: true }),
      },
      { config, db, logger: silentLogger }
    );
```

How can I resolve this? If you propose a fix, please make it concise.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 155817621e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Pass requested sync scope into syncRepo

syncHandler invokes syncRepo without a scope, so syncRepo always falls back to open; callers in CLI/MCP are looping over requested scopes externally and then reporting that loop scope, which makes --closed (or open+closed runs) fetch the wrong dataset while still labeling results as closed. This breaks selective sync semantics and stores sync metadata under the wrong scope.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Clear cache only once across multi-scope sync

This clear flag is passed on every iteration of the scope loop, and syncHandler clears before syncing; when users run fw sync --open --closed --clear, the second iteration clears out data written by the first iteration, so the final cache no longer contains both scopes. Clear should happen once before the loop (or only on the first scope) to preserve combined results.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Gate Graphite enrichment to the detected repo

Graphite enrichment is enabled for every repo whenever Graphite is available locally, but multi-repo syncs include repos unrelated to the local checkout; because the Graphite plugin matches stack membership by PR number, non-detected repos can be incorrectly tagged with local stack metadata when PR numbers overlap. This should stay scoped to the detected local repo as before.

Useful? React with 👍 / 👎.