Merge pull request #1176 from TheWizardsCode/wl-WL-0MN834ICI00339E7-dedup-double-scheduling

Deduplicate double-scheduling in github-sync (WL-0MN834ICI00339E7)

Author: SorraTheOrc

src/github-sync.ts

@@ -251,7 +251,10 @@ export async function upsertIssuesFromWorkItems(
   };
 
   // Concurrency: upsert issues and comments with a bounded concurrency pool
-  const upsertConcurrency = Number(process.env.WL_GITHUB_CONCURRENCY || '6');
+  // The central throttler enforces concurrency/rate limits. Do not rely on
+  // a local worker pool here; schedule GitHub API calls through `throttler`.
+  // Keep the env var available to the throttler implementation.
+  // (local upsertConcurrency removed)
 
   const truncateTitle = (title: string, maxLen = 60): string =>
     title.length <= maxLen ? title : title.slice(0, maxLen - 1) + '\u2026';
@@ -292,14 +295,16 @@ export async function upsertIssuesFromWorkItems(
       const shouldUpdateIssue = !item.githubIssueNumber
         || !item.githubIssueUpdatedAt
         || new Date(item.updatedAt).getTime() > new Date(item.githubIssueUpdatedAt).getTime();
-      if (shouldUpdateIssue) {
+    if (shouldUpdateIssue) {
         const upsertStart = Date.now();
         if (onVerboseLog) {
           onVerboseLog(`[upsert] ${item.githubIssueNumber ? 'update' : 'create'} ${item.id}`);
         }
-        if (item.githubIssueNumber) {
-          increment('api.issue.update');
-          issue = await updateGithubIssueAsync(config, item.githubIssueNumber!, payload);
+            if (item.githubIssueNumber) {
+              increment('api.issue.update');
+              // updateGithubIssueAsync already schedules via the central throttler
+              // internally (see src/github.ts). Avoid double-scheduling here.
+              issue = await updateGithubIssueAsync(config, item.githubIssueNumber!, payload);
           if (item.status === 'deleted') {
             result.closed += 1;
             result.syncedItems.push({
@@ -317,13 +322,14 @@ export async function upsertIssuesFromWorkItems(
               issueNumber: item.githubIssueNumber,
             });
           }
-        } else {
-          increment('api.issue.create');
-          issue = await createGithubIssueAsync(config, {
-            title: payload.title,
-            body: payload.body,
-            labels: payload.labels,
-          });
+            } else {
+              increment('api.issue.create');
+              // createGithubIssueAsync schedules via the central throttler itself.
+              issue = await createGithubIssueAsync(config, {
+                title: payload.title,
+                body: payload.body,
+                labels: payload.labels,
+              });
           result.created += 1;
           result.syncedItems.push({
             action: 'created',
@@ -343,14 +349,16 @@ export async function upsertIssuesFromWorkItems(
       }
 
       const shouldSyncCommentsNow = itemComments.length > 0 && (shouldSyncComments || shouldUpdateIssue);
-      if (shouldSyncCommentsNow && issueNumber) {
-        const commentListStart = Date.now();
-        increment('api.comment.list');
-        const existingComments = await listGithubIssueCommentsAsync(config, issueNumber!);
-        timing.commentListMs += Date.now() - commentListStart;
-        const commentUpsertStart = Date.now();
-        const commentSummary = await upsertGithubIssueCommentsAsync(config, issueNumber, itemComments, existingComments);
-        timing.commentUpsertMs += Date.now() - commentUpsertStart;
+        if (shouldSyncCommentsNow && issueNumber) {
+          const commentListStart = Date.now();
+          increment('api.comment.list');
+          // listGithubIssueCommentsAsync now schedules internally via the throttler
+          // (see src/github.ts). Call it directly to avoid double-scheduling.
+          const existingComments = await listGithubIssueCommentsAsync(config, issueNumber!);
+          timing.commentListMs += Date.now() - commentListStart;
+          const commentUpsertStart = Date.now();
+          const commentSummary = await upsertGithubIssueCommentsAsync(config, issueNumber, itemComments, existingComments);
+          timing.commentUpsertMs += Date.now() - commentUpsertStart;
         increment('api.comment.create', commentSummary.created || 0);
         increment('api.comment.update', commentSummary.updated || 0);
         result.commentsCreated = (result.commentsCreated || 0) + commentSummary.created;
@@ -399,12 +407,14 @@ export async function upsertIssuesFromWorkItems(
     for (const comment of sorted) {
       const body = buildGithubCommentBody(comment);
       const existing = byWorklogId.get(comment.id);
-      if (existing) {
+        if (existing) {
         // If the GH comment exists, only update if body changed OR GH's updatedAt is newer than our recorded mapping
         const bodyMatch = (existing.body || '').trim() === body.trim();
-        if (!bodyMatch) {
-          increment('api.comment.update');
-           const updatedComment = await updateGithubIssueCommentAsync(issueConfig, existing.id!, body);
+           if (!bodyMatch) {
+           increment('api.comment.update');
+             // updateGithubIssueCommentAsync now schedules internally via the throttler
+             // (see src/github.ts). Call it directly to avoid double-scheduling.
+             const updatedComment = await updateGithubIssueCommentAsync(issueConfig, existing.id!, body);
           // Persist mapping back to local comment
           comment.githubCommentId = existing.id;
           comment.githubCommentUpdatedAt = updatedComment.updatedAt;
@@ -417,9 +427,11 @@ export async function upsertIssuesFromWorkItems(
         continue;
       }
 
-      // No GH comment mapping found — create a new comment
-       increment('api.comment.create');
-          const createdComment = await createGithubIssueCommentAsync(issueConfig, issueNumber, body);
+       // No GH comment mapping found — create a new comment
+        increment('api.comment.create');
+            // createGithubIssueCommentAsync now schedules internally via the throttler
+            // (see src/github.ts). Call it directly to avoid double-scheduling.
+            const createdComment = await createGithubIssueCommentAsync(issueConfig, issueNumber, body);
       // Persist mapping back to local comment so future runs can directly reference by ID
       comment.githubCommentId = createdComment.id;
       comment.githubCommentUpdatedAt = createdComment.updatedAt;
@@ -433,23 +445,10 @@ export async function upsertIssuesFromWorkItems(
     return { created, updated, latestUpdatedAt };
   }
 
-  // simple concurrent mapper for issue upserts
-  async function mapWithConcurrencyItems(arr: WorkItem[], limit: number, fn: (v: WorkItem, i: number) => Promise<void>) {
-    const results: Promise<void>[] = [];
-    let i = 0;
-    async function worker() {
-      while (true) {
-        const idx = i++;
-        if (idx >= arr.length) return;
-        await fn(arr[idx], idx);
-      }
-    }
-    const workers = Math.min(limit, arr.length);
-    for (let w = 0; w < workers; w += 1) results.push(worker());
-    await Promise.all(results);
-  }
-
-  await mapWithConcurrencyItems(issueItems, upsertConcurrency, upsertMapper);
+  // Launch upsert mappers without a local worker pool; schedule external
+  // GitHub API calls through the central throttler. The throttler enforces
+  // WL_GITHUB_CONCURRENCY and rate limits configured in src/github-throttler.ts.
+  await Promise.all(issueItems.map((it, idx) => upsertMapper(it, idx)));
 
   result.skipped = items.length - issueItems.length + skippedUpdates;
 
@@ -554,23 +553,10 @@ export async function upsertIssuesFromWorkItems(
       }
     }
 
-  // simple concurrent mapper
-  async function mapWithConcurrency(arr: string[], limit: number, fn: (v: string, i: number) => Promise<void>) {
-    const results: Promise<void>[] = [];
-    let i = 0;
-    async function worker() {
-      while (true) {
-        const idx = i++;
-        if (idx >= arr.length) return;
-        await fn(arr[idx], idx);
-      }
-    }
-    const workers = Math.min(limit, arr.length);
-    for (let w = 0; w < workers; w += 1) results.push(worker());
-    await Promise.all(results);
-  }
-
-  await mapWithConcurrency(pairs, concurrency, mapper);
+  // Process hierarchy pairs concurrently and let the throttler limit GitHub
+  // requests. Avoid a local worker pool — schedule linking/fetch calls via
+  // the central throttler inside `mapper`.
+  await Promise.all(pairs.map((p, idx) => mapper(p, idx)));
 
   result.updated += linkedCount;
   timing.totalMs = Date.now() - startTime;

src/github.ts

@@ -934,14 +934,18 @@ export function listGithubIssueComments(config: GithubConfig, issueNumber: numbe
 export async function listGithubIssueCommentsAsync(config: GithubConfig, issueNumber: number): Promise<GithubIssueComment[]> {
   const { owner, name } = parseRepoSlug(config.repo);
   const command = `gh api repos/${owner}/${name}/issues/${issueNumber}/comments --paginate`;
-  try {
-    const data = await runGhJsonAsync(command);
-    if (!data) return [];
-    const raw = Array.isArray(data) ? data : [];
-    return raw.map(comment => normalizeGithubIssueComment(comment));
-  } catch {
-    return [];
-  }
+  // Schedule network call through central throttler to enforce concurrency
+  // and rate limits. Callers should not need to schedule this themselves.
+  return await throttler.schedule(async () => {
+    try {
+      const data = await runGhJsonAsync(command);
+      if (!data) return [];
+      const raw = Array.isArray(data) ? data : [];
+      return raw.map(comment => normalizeGithubIssueComment(comment));
+    } catch {
+      return [];
+    }
+  });
 }
 
 export function createGithubIssueComment(config: GithubConfig, issueNumber: number, body: string): GithubIssueComment {
@@ -952,10 +956,13 @@ export function createGithubIssueComment(config: GithubConfig, issueNumber: numb
 }
 
 export async function createGithubIssueCommentAsync(config: GithubConfig, issueNumber: number, body: string): Promise<GithubIssueComment> {
-  const { owner, name } = parseRepoSlug(config.repo);
-  const command = `gh api -X POST repos/${owner}/${name}/issues/${issueNumber}/comments -F body=@-`;
-  const data = await runGhJsonAsync(command, body);
-  return normalizeGithubIssueComment(data);
+  // Ensure comment creation is scheduled through the central throttler.
+  return await throttler.schedule(async () => {
+    const { owner, name } = parseRepoSlug(config.repo);
+    const command = `gh api -X POST repos/${owner}/${name}/issues/${issueNumber}/comments -F body=@-`;
+    const data = await runGhJsonAsync(command, body);
+    return normalizeGithubIssueComment(data);
+  });
 }
 
 export function updateGithubIssueComment(config: GithubConfig, commentId: number, body: string): GithubIssueComment {
@@ -966,10 +973,13 @@ export function updateGithubIssueComment(config: GithubConfig, commentId: number
 }
 
 export async function updateGithubIssueCommentAsync(config: GithubConfig, commentId: number, body: string): Promise<GithubIssueComment> {
-  const { owner, name } = parseRepoSlug(config.repo);
-  const command = `gh api -X PATCH repos/${owner}/${name}/issues/comments/${commentId} -F body=@-`;
-  const data = await runGhJsonAsync(command, body);
-  return normalizeGithubIssueComment(data);
+  // Ensure comment updates are scheduled through the central throttler.
+  return await throttler.schedule(async () => {
+    const { owner, name } = parseRepoSlug(config.repo);
+    const command = `gh api -X PATCH repos/${owner}/${name}/issues/comments/${commentId} -F body=@-`;
+    const data = await runGhJsonAsync(command, body);
+    return normalizeGithubIssueComment(data);
+  });
 }
 
 export function getGithubIssueComment(config: GithubConfig, commentId: number): GithubIssueComment {

tests/cli/__snapshots__/human-show-list-audit-snapshots.test.ts.snap

@@ -0,0 +1,41 @@
+// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
+
+exports[`Human snapshots: show and list outputs with audit > renders concise/list and single-item human outputs with and without audit (snapshots) > human-list-with-audit 1`] = `
+"Found 2 work item(s):
+
+
+├── Audited task TEST-1
+│   Status: Open · Stage: Undefined | Priority: medium
+│   SortIndex: 0
+│   Risk: —
+│   Effort: —
+│   Audit: Ready to close: Yes
+└── No audit TEST-2
+    Status: Open · Stage: Undefined | Priority: medium
+    SortIndex: 0
+    Risk: —
+    Effort: —
+
+"
+`;
+
+exports[`Human snapshots: show and list outputs with audit > renders concise/list and single-item human outputs with and without audit (snapshots) > human-show-with-audit 1`] = `
+"
+└── Audited task TEST-1
+    Status: Open · Stage: Undefined | Priority: medium
+    SortIndex: 0
+    Risk: —
+    Effort: —
+    Audit: Ready to close: Yes
+"
+`;
+
+exports[`Human snapshots: show and list outputs with audit > renders concise/list and single-item human outputs with and without audit (snapshots) > human-show-without-audit 1`] = `
+"
+└── No audit TEST-2
+    Status: Open · Stage: Undefined | Priority: medium
+    SortIndex: 0
+    Risk: —
+    Effort: —
+"
+`;

tests/cli/human-show-list-audit-snapshots.test.ts

@@ -0,0 +1,43 @@
+import { describe, it, expect, beforeEach, afterEach } from 'vitest';
+import { execAsync, enterTempDir, leaveTempDir, writeConfig, writeInitSemaphore, seedWorkItems, cliPath } from './cli-helpers.js';
+
+describe('Human snapshots: show and list outputs with audit', () => {
+  let state: { tempDir: string; originalCwd: string };
+
+  beforeEach(() => {
+    state = enterTempDir();
+    writeConfig(state.tempDir, 'Test Project', 'TEST');
+    writeInitSemaphore(state.tempDir, '1.0.0');
+  });
+
+  afterEach(() => {
+    leaveTempDir(state);
+  });
+
+  it('renders concise/list and single-item human outputs with and without audit (snapshots)', async () => {
+    // Seed two work items: one with audit and one without
+    seedWorkItems(state.tempDir, [
+      {
+        id: 'TEST-1',
+        title: 'Audited task',
+        audit: { time: '2026-01-01T00:00:00Z', author: 'alice', text: 'Ready to close: Yes\nExtra details' }
+      },
+      {
+        id: 'TEST-2',
+        title: 'No audit'
+      }
+    ]);
+
+    // List (human) - compact output used by default
+    const { stdout: listOut } = await execAsync(`tsx ${cliPath} list`);
+    expect(listOut).toMatchSnapshot('human-list-with-audit');
+
+    // Single-item show (human)
+    const { stdout: showOut } = await execAsync(`tsx ${cliPath} show TEST-1`);
+    expect(showOut).toMatchSnapshot('human-show-with-audit');
+
+    // Single-item show for item without audit should not include an Audit block/placeholder
+    const { stdout: showOut2 } = await execAsync(`tsx ${cliPath} show TEST-2`);
+    expect(showOut2).toMatchSnapshot('human-show-without-audit');
+  });
+});

tests/cli/throttler-github-sync.test.ts

@@ -0,0 +1,59 @@
+import { describe, it, expect, vi, beforeEach } from 'vitest';
+import throttler from '../../src/github-throttler.js';
+import * as githubSync from '../../src/github-sync.js';
+import * as githubHelpers from '../../src/github.js';
+
+describe('github-sync throttler integration (unit)', () => {
+  beforeEach(() => {
+    vi.restoreAllMocks();
+  });
+
+  it('uses throttler.schedule for GitHub issue create/update and comment operations', async () => {
+    // Spy on throttler.schedule
+    const scheduleSpy = vi.spyOn(throttler, 'schedule');
+
+    // Prepare minimal items and comments to exercise upsert path
+    const items = [
+      {
+        id: 'WI-1',
+        title: 'T1',
+        description: 'desc',
+        status: 'open',
+        priority: 'medium',
+        sortIndex: 0,
+        parentId: null,
+        createdAt: new Date().toISOString(),
+        updatedAt: new Date().toISOString(),
+        tags: [],
+        assignee: '',
+      },
+    ];
+    const comments = [];
+
+    // Stub out github API helpers (they are exported from src/github.js).
+    // Each stub should still call the central throttler so we can assert
+    // `throttler.schedule` is used by the flow.
+    vi.spyOn(githubHelpers as any, 'createGithubIssueAsync').mockImplementation(() =>
+      throttler.schedule(async () => ({ number: 123, id: 99, updatedAt: new Date().toISOString() }))
+    );
+    vi.spyOn(githubHelpers as any, 'updateGithubIssueAsync').mockImplementation(() =>
+      throttler.schedule(async () => ({ number: 123, id: 99, updatedAt: new Date().toISOString() }))
+    );
+    vi.spyOn(githubHelpers as any, 'listGithubIssueCommentsAsync').mockImplementation(() =>
+      throttler.schedule(async () => [])
+    );
+    vi.spyOn(githubHelpers as any, 'createGithubIssueCommentAsync').mockImplementation(() =>
+      throttler.schedule(async () => ({ id: 1, updatedAt: new Date().toISOString() }))
+    );
+    vi.spyOn(githubHelpers as any, 'updateGithubIssueCommentAsync').mockImplementation(() =>
+      throttler.schedule(async () => ({ id: 1, updatedAt: new Date().toISOString() }))
+    );
+
+    const config = { repo: 'owner/repo', labelPrefix: 'wl:' } as any;
+
+    await githubSync.upsertIssuesFromWorkItems(items as any, comments as any, config);
+
+    // Assert that throttle.schedule was used at least once (multiple callsites exist)
+    expect(scheduleSpy).toHaveBeenCalled();
+  });
+});