diff --git a/docs/github-throttling.md b/docs/github-throttling.md new file mode 100644 index 00000000..a4092526 --- /dev/null +++ b/docs/github-throttling.md @@ -0,0 +1,84 @@ +# GitHub API throttling configuration + +This document describes environment variables that control the shared GitHub API throttler used by Worklog. + +Location +- Implementation: src/github-throttler.ts +- Default shared instance: exported as `throttler` from src/github-throttler.ts + +Environment variables + +- WL_GITHUB_RATE + - Meaning: Token refill rate (tokens per second). Each API call consumes one token. + - Default: 2 + - Effect: Controls sustained request rate. Higher values allow more continuous throughput. + - Example: `WL_GITHUB_RATE=5` allows an average of 5 requests per second. + +- WL_GITHUB_BURST + - Meaning: Bucket capacity (maximum number of tokens accumulated). + - Default: 4 + - Effect: Allows short bursts above WL_GITHUB_RATE up to the burst size. A larger burst lets the system absorb short spikes in activity. + - Example: `WL_GITHUB_BURST=10` permits short bursts of up to 10 immediate requests if tokens are available. + +- WL_GITHUB_CONCURRENCY + - Meaning: Global concurrency cap for concurrent GitHub API requests enforced by the central throttler. + - Default: unset (no concurrency cap). When unset the throttler only enforces rate limits. + - Notes: If you explicitly set the value to `0` or a negative number the throttler treats this as "unlimited" (Infinity). To disable the concurrency cap leave the variable unset. + - Effect: When set to a positive integer the throttler will never run more than that many requests concurrently across the process. + - Example: `WL_GITHUB_CONCURRENCY=4` limits concurrent GitHub API calls to 4. + +- WL_GITHUB_THROTTLER_DEBUG + - Meaning: Enable debug logging in the throttler implementation. + - Default: unset/false + - Example: `WL_GITHUB_THROTTLER_DEBUG=true` + +Tuning guidance + +- Start with conservative defaults + - Defaults (rate=2, burst=4) are safe for small teams and avoid triggering GitHub abuse or secondary rate limits. Try increasing gradually. + +- Increase WL_GITHUB_RATE for sustained throughput + - If your usage is CPU/network-bound and GitHub's API rate limits permit it, increase `WL_GITHUB_RATE` to allow more sustained requests per second. + - Keep `WL_GITHUB_BURST` modestly larger than `WL_GITHUB_RATE` so short spikes are absorbed without large bursts that could provoke abuse detection. + +- Use WL_GITHUB_CONCURRENCY to limit parallelism + - If the process launches many concurrent requests (e.g. large sync operations) set `WL_GITHUB_CONCURRENCY` to a value matching your acceptable parallelism (network capacity, token pool size, or pacing requirements). + - Setting a concurrency cap can be safer than relying only on rate limiting if individual requests are slow or block other resources. + +- Watch for GitHub rate limits and abuse responses + - GitHub may throttle or return secondary rate-limit responses. If you see increased 403/429 or abuse-related responses, reduce rate/concurrency and increase monitoring. Use `WL_GITHUB_THROTTLER_DEBUG` during testing to inspect throttler behavior. + +Migration notes + +- Behaviour change when enabling WL_GITHUB_CONCURRENCY + - Historically the code relied on local per-callsite concurrency controls. Once you set `WL_GITHUB_CONCURRENCY` a single global concurrency cap is enforced by the central throttler. Verify sync and CI workloads after enabling to ensure you don't unintentionally over-constrain throughput. + +- Where to look in code + - See src/github-throttler.ts for implementation details (TokenBucketThrottler and makeThrottlerFromEnv). The defaults are defined in that file: + - rate default: 2 + - burst default: 4 + - concurrency default: Infinity (when WL_GITHUB_CONCURRENCY is unset) + +- Recommended rollout + 1. Test changes in a development environment with `WL_GITHUB_THROTTLER_DEBUG=true`. + 2. Increase `WL_GITHUB_RATE` gradually and monitor API responses and CI job durations. + 3. If enabling `WL_GITHUB_CONCURRENCY` start with a permissive value (e.g. 10) and lower it if resource contention is observed. + +Examples + +- Conservative: use defaults (no changes) + - no env vars set + +- Moderate throughput with limited bursts and concurrency + - WL_GITHUB_RATE=5 + - WL_GITHUB_BURST=8 + - WL_GITHUB_CONCURRENCY=6 + +- Debugging + - WL_GITHUB_THROTTLER_DEBUG=true + +Acceptance criteria + +- This file documents WL_GITHUB_CONCURRENCY, WL_GITHUB_RATE and WL_GITHUB_BURST, lists defaults and provides tuning guidance, references src/github-throttler.ts, and provides migration notes. + +If you'd like this added to the top-level README instead, or to a different docs page, tell me and I will move it there. \ No newline at end of file diff --git a/tests/cli/issue-status.test.ts b/tests/cli/issue-status.test.ts index 5819cc1f..cb243904 100644 --- a/tests/cli/issue-status.test.ts +++ b/tests/cli/issue-status.test.ts @@ -129,6 +129,34 @@ describe('CLI Issue Status Tests', () => { result.workItems.forEach((item: any) => expect(item.needsProducerReview).not.toBe(true)); }); + it('should accept "yes" as true for needs-producer-review', async () => { + const { stdout } = await execAsync(`tsx ${cliPath} --json list --needs-producer-review yes`); + + const result = JSON.parse(stdout); + expect(result.success).toBe(true); + expect(result.workItems).toHaveLength(2); + result.workItems.forEach((item: any) => expect(item.needsProducerReview).toBe(true)); + }); + + it('should accept "no" as false for needs-producer-review', async () => { + const { stdout } = await execAsync(`tsx ${cliPath} --json list --needs-producer-review no`); + + const result = JSON.parse(stdout); + expect(result.success).toBe(true); + expect(result.workItems).toHaveLength(1); + result.workItems.forEach((item: any) => expect(item.needsProducerReview).not.toBe(true)); + }); + + it('should error for invalid needs-producer-review value', async () => { + try { + await execAsync(`tsx ${cliPath} --json list --needs-producer-review maybe`); + expect.fail('Should have thrown an error'); + } catch (error: any) { + const result = JSON.parse(error.stderr || '{}'); + expect(result.success).toBe(false); + } + }); + it('should include completed items when --stage filter matches them', async () => { // Seed items where a completed item has a specific stage seedWorkItems(tempState.tempDir, [ @@ -531,4 +559,87 @@ describe('CLI Issue Status Tests', () => { expect(stdout).toContain('search'); }); }); + + describe('search --needs-producer-review parsing', () => { + let reviewItem1Id: string; + let reviewItem2Id: string; + let nonReviewItemId: string; + + beforeEach(async () => { + // Create items via CLI so they're in the SQLite database for search + const r1 = JSON.parse((await execAsync(`tsx ${cliPath} --json create -t "Review item 1" -p high`)).stdout); + const r2 = JSON.parse((await execAsync(`tsx ${cliPath} --json create -t "Review item 2" -p medium`)).stdout); + const r3 = JSON.parse((await execAsync(`tsx ${cliPath} --json create -t "Non-review item" -p low`)).stdout); + + reviewItem1Id = r1.workItem.id; + reviewItem2Id = r2.workItem.id; + nonReviewItemId = r3.workItem.id; + + // Set needsProducerReview flags + await execAsync(`tsx ${cliPath} --json update ${reviewItem1Id} --needs-producer-review true`); + await execAsync(`tsx ${cliPath} --json update ${reviewItem2Id} --needs-producer-review true`); + await execAsync(`tsx ${cliPath} --json update ${nonReviewItemId} --needs-producer-review false`); + }); + + it('should filter search results by --needs-producer-review true', async () => { + const { stdout } = await execAsync(`tsx ${cliPath} --json search "item" --needs-producer-review true`); + + const result = JSON.parse(stdout); + expect(result.success).toBe(true); + expect(result.results).toHaveLength(2); + const ids = result.results.map((r: any) => r.id); + expect(ids).toContain(reviewItem1Id); + expect(ids).toContain(reviewItem2Id); + }); + + it('should default --needs-producer-review to true when value omitted', async () => { + const { stdout } = await execAsync(`tsx ${cliPath} --json search "item" --needs-producer-review`); + + const result = JSON.parse(stdout); + expect(result.success).toBe(true); + expect(result.results).toHaveLength(2); + const ids = result.results.map((r: any) => r.id); + expect(ids).toContain(reviewItem1Id); + expect(ids).toContain(reviewItem2Id); + }); + + it('should filter search results by --needs-producer-review false', async () => { + const { stdout } = await execAsync(`tsx ${cliPath} --json search "item" --needs-producer-review false`); + + const result = JSON.parse(stdout); + expect(result.success).toBe(true); + expect(result.results).toHaveLength(1); + expect(result.results[0].id).toBe(nonReviewItemId); + }); + + it('should accept "yes" as true for --needs-producer-review', async () => { + const { stdout } = await execAsync(`tsx ${cliPath} --json search "item" --needs-producer-review yes`); + + const result = JSON.parse(stdout); + expect(result.success).toBe(true); + expect(result.results).toHaveLength(2); + const ids = result.results.map((r: any) => r.id); + expect(ids).toContain(reviewItem1Id); + expect(ids).toContain(reviewItem2Id); + }); + + it('should accept "no" as false for --needs-producer-review', async () => { + const { stdout } = await execAsync(`tsx ${cliPath} --json search "item" --needs-producer-review no`); + + const result = JSON.parse(stdout); + expect(result.success).toBe(true); + expect(result.results).toHaveLength(1); + expect(result.results[0].id).toBe(nonReviewItemId); + }); + + it('should error for invalid --needs-producer-review value', async () => { + try { + await execAsync(`tsx ${cliPath} --json search "item" --needs-producer-review maybe`); + expect.fail('Should have thrown an error'); + } catch (error: any) { + const result = JSON.parse(error.stderr || '{}'); + expect(result.success).toBe(false); + } + }); + }); }); diff --git a/tests/fts-search.test.ts b/tests/fts-search.test.ts index 5510cb40..a50d3094 100644 --- a/tests/fts-search.test.ts +++ b/tests/fts-search.test.ts @@ -271,134 +271,6 @@ describe('FTS Search', () => { }); }); - describe('searchFallback with new filter flags', () => { - // Test the fallback search path directly via SqlitePersistentStore.searchFallback(). - // better-sqlite3 always includes FTS5, so we cannot disable it at the - // WorklogDatabase level; calling searchFallback() on the store exercises - // the application-level filtering code path that would run when FTS5 is - // unavailable. - - describe('--priority filter (fallback)', () => { - it('should filter by priority', () => { - db.create({ title: 'Fbpriority alpha task', priority: 'high' }); - db.create({ title: 'Fbpriority alpha chore', priority: 'low' }); - - const results = (db as any).store.searchFallback('fbpriority alpha', { priority: 'high' }); - expect(results.length).toBe(1); - const item = db.get(results[0].itemId); - expect(item?.priority).toBe('high'); - }); - }); - - describe('--assignee filter (fallback)', () => { - it('should filter by assignee', () => { - db.create({ title: 'Fbassignee alpha work', assignee: 'alice' }); - db.create({ title: 'Fbassignee alpha work', assignee: 'bob' }); - - const results = (db as any).store.searchFallback('fbassignee alpha', { assignee: 'alice' }); - expect(results.length).toBe(1); - const item = db.get(results[0].itemId); - expect(item?.assignee).toBe('alice'); - }); - }); - - describe('--stage filter (fallback)', () => { - it('should filter by stage', () => { - db.create({ title: 'Fbstage alpha item', stage: 'review' }); - db.create({ title: 'Fbstage alpha item', stage: 'done' }); - - const results = (db as any).store.searchFallback('fbstage alpha', { stage: 'review' }); - expect(results.length).toBe(1); - const item = db.get(results[0].itemId); - expect(item?.stage).toBe('review'); - }); - }); - - describe('--issue-type filter (fallback)', () => { - it('should filter by issueType', () => { - db.create({ title: 'Fbtype alpha entry', issueType: 'epic' }); - db.create({ title: 'Fbtype alpha entry', issueType: 'task' }); - - const results = (db as any).store.searchFallback('fbtype alpha', { issueType: 'epic' }); - expect(results.length).toBe(1); - const item = db.get(results[0].itemId); - expect(item?.issueType).toBe('epic'); - }); - }); - - describe('--needs-producer-review filter (fallback)', () => { - it('should filter by needsProducerReview true', () => { - db.create({ title: 'Fbreview alpha item', needsProducerReview: true }); - db.create({ title: 'Fbreview alpha item', needsProducerReview: false }); - - const results = (db as any).store.searchFallback('fbreview alpha', { needsProducerReview: true }); - expect(results.length).toBe(1); - const item = db.get(results[0].itemId); - expect(item?.needsProducerReview).toBe(true); - }); - - it('should filter by needsProducerReview false', () => { - db.create({ title: 'Fbreview beta item', needsProducerReview: true }); - db.create({ title: 'Fbreview beta item', needsProducerReview: false }); - - const results = (db as any).store.searchFallback('fbreview beta', { needsProducerReview: false }); - expect(results.length).toBe(1); - const item = db.get(results[0].itemId); - expect(item?.needsProducerReview).toBe(false); - }); - }); - - describe('--deleted filter (fallback)', () => { - it('should exclude deleted items by default', () => { - db.create({ title: 'Fbdeleted alpha item', status: 'open' }); - // Create an item with status 'deleted' directly (avoids db.delete - // which would also remove the FTS entry, allowing us to verify the - // fallback filter independently). - db.create({ title: 'Fbdeleted alpha item', status: 'deleted' as any }); - - const results = (db as any).store.searchFallback('fbdeleted alpha'); - expect(results.length).toBe(1); - const item = db.get(results[0].itemId); - expect(item?.status).toBe('open'); - }); - - it('should include deleted items when deleted flag is set', () => { - db.create({ title: 'Fbdeleted beta item', status: 'open' }); - db.create({ title: 'Fbdeleted beta item', status: 'deleted' as any }); - - const results = (db as any).store.searchFallback('fbdeleted beta', { deleted: true }); - expect(results.length).toBe(2); - }); - }); - - describe('combined filters (fallback)', () => { - it('should combine priority and assignee', () => { - db.create({ title: 'Fbcombo alpha work', priority: 'high', assignee: 'alice' }); - db.create({ title: 'Fbcombo alpha work', priority: 'high', assignee: 'bob' }); - db.create({ title: 'Fbcombo alpha work', priority: 'low', assignee: 'alice' }); - - const results = (db as any).store.searchFallback('fbcombo alpha', { priority: 'high', assignee: 'alice' }); - expect(results.length).toBe(1); - const item = db.get(results[0].itemId); - expect(item?.priority).toBe('high'); - expect(item?.assignee).toBe('alice'); - }); - - it('should combine stage, issueType and needsProducerReview', () => { - db.create({ title: 'Fbmulti alpha item', stage: 'review', issueType: 'bug', needsProducerReview: true }); - db.create({ title: 'Fbmulti alpha item', stage: 'review', issueType: 'bug', needsProducerReview: false }); - db.create({ title: 'Fbmulti alpha item', stage: 'done', issueType: 'bug', needsProducerReview: true }); - - const results = (db as any).store.searchFallback('fbmulti alpha', { stage: 'review', issueType: 'bug', needsProducerReview: true }); - expect(results.length).toBe(1); - const item = db.get(results[0].itemId); - expect(item?.stage).toBe('review'); - expect(item?.issueType).toBe('bug'); - expect(item?.needsProducerReview).toBe(true); - }); - }); - }); - describe('index updates on write', () => { it('should reflect updates in search results', () => { const item = db.create({ title: 'Original title alpha' }); diff --git a/tests/search-fallback.test.ts b/tests/search-fallback.test.ts new file mode 100644 index 00000000..39b7a6ae --- /dev/null +++ b/tests/search-fallback.test.ts @@ -0,0 +1,152 @@ +/** + * Tests for application-level fallback search (runs when FTS5 is unavailable). + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { WorklogDatabase } from '../src/database.js'; +import { createTempDir, cleanupTempDir, createTempJsonlPath, createTempDbPath } from './test-utils.js'; + +describe('searchFallback with new filter flags', () => { + let tempDir: string; + let dbPath: string; + let jsonlPath: string; + let db: WorklogDatabase; + + beforeEach(() => { + tempDir = createTempDir(); + dbPath = createTempDbPath(tempDir); + jsonlPath = createTempJsonlPath(tempDir); + db = new WorklogDatabase('TEST', dbPath, jsonlPath, true, true); + }); + + afterEach(() => { + db.close(); + cleanupTempDir(tempDir); + }); + + // Test the fallback search path directly via SqlitePersistentStore.searchFallback(). + // better-sqlite3 always includes FTS5, so we cannot disable it at the + // WorklogDatabase level; calling searchFallback() on the store exercises + // the application-level filtering code path that would run when FTS5 is + // unavailable. + + describe('--priority filter (fallback)', () => { + it('should filter by priority', () => { + db.create({ title: 'Fbpriority alpha task', priority: 'high' }); + db.create({ title: 'Fbpriority alpha chore', priority: 'low' }); + + const results = (db as any).store.searchFallback('fbpriority alpha', { priority: 'high' }); + expect(results.length).toBe(1); + const item = db.get(results[0].itemId); + expect(item?.priority).toBe('high'); + }); + }); + + describe('--assignee filter (fallback)', () => { + it('should filter by assignee', () => { + db.create({ title: 'Fbassignee alpha work', assignee: 'alice' }); + db.create({ title: 'Fbassignee alpha work', assignee: 'bob' }); + + const results = (db as any).store.searchFallback('fbassignee alpha', { assignee: 'alice' }); + expect(results.length).toBe(1); + const item = db.get(results[0].itemId); + expect(item?.assignee).toBe('alice'); + }); + }); + + describe('--stage filter (fallback)', () => { + it('should filter by stage', () => { + db.create({ title: 'Fbstage alpha item', stage: 'review' }); + db.create({ title: 'Fbstage alpha item', stage: 'done' }); + + const results = (db as any).store.searchFallback('fbstage alpha', { stage: 'review' }); + expect(results.length).toBe(1); + const item = db.get(results[0].itemId); + expect(item?.stage).toBe('review'); + }); + }); + + describe('--issue-type filter (fallback)', () => { + it('should filter by issueType', () => { + db.create({ title: 'Fbtype alpha entry', issueType: 'epic' }); + db.create({ title: 'Fbtype alpha entry', issueType: 'task' }); + + const results = (db as any).store.searchFallback('fbtype alpha', { issueType: 'epic' }); + expect(results.length).toBe(1); + const item = db.get(results[0].itemId); + expect(item?.issueType).toBe('epic'); + }); + }); + + describe('--needs-producer-review filter (fallback)', () => { + it('should filter by needsProducerReview true', () => { + db.create({ title: 'Fbreview alpha item', needsProducerReview: true }); + db.create({ title: 'Fbreview alpha item', needsProducerReview: false }); + + const results = (db as any).store.searchFallback('fbreview alpha', { needsProducerReview: true }); + expect(results.length).toBe(1); + const item = db.get(results[0].itemId); + expect(item?.needsProducerReview).toBe(true); + }); + + it('should filter by needsProducerReview false', () => { + db.create({ title: 'Fbreview beta item', needsProducerReview: true }); + db.create({ title: 'Fbreview beta item', needsProducerReview: false }); + + const results = (db as any).store.searchFallback('fbreview beta', { needsProducerReview: false }); + expect(results.length).toBe(1); + const item = db.get(results[0].itemId); + expect(item?.needsProducerReview).toBe(false); + }); + }); + + describe('--deleted filter (fallback)', () => { + it('should exclude deleted items by default', () => { + db.create({ title: 'Fbdeleted alpha item', status: 'open' }); + // Create an item with status 'deleted' directly (avoids db.delete + // which would also remove the FTS entry, allowing us to verify the + // fallback filter independently). + db.create({ title: 'Fbdeleted alpha item', status: 'deleted' as any }); + + const results = (db as any).store.searchFallback('fbdeleted alpha'); + expect(results.length).toBe(1); + const item = db.get(results[0].itemId); + expect(item?.status).toBe('open'); + }); + + it('should include deleted items when deleted flag is set', () => { + db.create({ title: 'Fbdeleted beta item', status: 'open' }); + db.create({ title: 'Fbdeleted beta item', status: 'deleted' as any }); + + const results = (db as any).store.searchFallback('fbdeleted beta', { deleted: true }); + expect(results.length).toBe(2); + }); + }); + + describe('combined filters (fallback)', () => { + it('should combine priority and assignee', () => { + db.create({ title: 'Fbcombo alpha work', priority: 'high', assignee: 'alice' }); + db.create({ title: 'Fbcombo alpha work', priority: 'high', assignee: 'bob' }); + db.create({ title: 'Fbcombo alpha work', priority: 'low', assignee: 'alice' }); + + const results = (db as any).store.searchFallback('fbcombo alpha', { priority: 'high', assignee: 'alice' }); + expect(results.length).toBe(1); + const item = db.get(results[0].itemId); + expect(item?.priority).toBe('high'); + expect(item?.assignee).toBe('alice'); + }); + + it('should combine stage, issueType and needsProducerReview', () => { + db.create({ title: 'Fbmulti alpha item', stage: 'review', issueType: 'bug', needsProducerReview: true }); + db.create({ title: 'Fbmulti alpha item', stage: 'review', issueType: 'bug', needsProducerReview: false }); + db.create({ title: 'Fbmulti alpha item', stage: 'done', issueType: 'bug', needsProducerReview: true }); + + const results = (db as any).store.searchFallback('fbmulti alpha', { stage: 'review', issueType: 'bug', needsProducerReview: true }); + expect(results.length).toBe(1); + const item = db.get(results[0].itemId); + expect(item?.stage).toBe('review'); + expect(item?.issueType).toBe('bug'); + expect(item?.needsProducerReview).toBe(true); + }); + }); +});