From 23115eb86d376b65c1552b075b613b38fe98c40b Mon Sep 17 00:00:00 2001 From: "Urvish L." Date: Mon, 26 Oct 2026 02:55:38 +0100 Subject: [PATCH] feat(attachments): make TODO reminders configurable (#2214) * commit1 * fix(attachments): resolve reminder thresholds per field and validate env strictly - Resolve turnsSinceWrite/turnsBetweenReminders independently with per-field precedence (settings -> env -> default) so partial todoReminder settings are honored instead of discarded. - Parse OPENCLAUDE_TODO_REMINDER_* via whole-string positive-safe-integer validation so suffixed, fractional, scientific, and oversized values fall back to the default instead of being truncated. - Degrade invalid todoReminder thresholds or a malformed object to undefined in the settings schema so one bad value cannot invalidate the entire settings source. - Replace the copy-pasted JSDoc above getEnvNumber with an accurate one. - Extend focused tests: settings precedence, partial/mixed settings, malformed env values, and schema fallback behavior. * test(attachments): isolate reminder settings and cover consumer boundaries --------- Co-authored-by: jatmn --- .env.example | 4 + src/utils/attachments.todoReminder.test.ts | 319 +++++++++++++++++++++ src/utils/attachments.ts | 36 ++- src/utils/envUtils.ts | 25 ++ src/utils/settings/types.ts | 26 ++ 5 files changed, 402 insertions(+), 8 deletions(-) create mode 100644 src/utils/attachments.todoReminder.test.ts diff --git a/.env.example b/.env.example index e369f556d..69ddd0dcb 100644 --- a/.env.example +++ b/.env.example @@ -495,6 +495,10 @@ ANTHROPIC_API_KEY=sk-ant-your-key-here # Useful for users who want full transparency over what the model sees # OPENCLAUDE_DISABLE_TOOL_REMINDERS=1 +# Configure todo/task reminder thresholds (default: 10 turns each) +# OPENCLAUDE_TODO_REMINDER_TURNS_SINCE_WRITE=15 +# OPENCLAUDE_TODO_REMINDER_TURNS_BETWEEN_REMINDERS=20 + # Log structured per-request token usage (including cache metrics) to stderr. # Useful for auditing cache hit rate / debugging cost spikes outside the REPL. # Any truthy value enables it ("verbose", "1", "true"). diff --git a/src/utils/attachments.todoReminder.test.ts b/src/utils/attachments.todoReminder.test.ts new file mode 100644 index 000000000..acf2c8b5b --- /dev/null +++ b/src/utils/attachments.todoReminder.test.ts @@ -0,0 +1,319 @@ +import { afterEach, beforeEach, describe, expect, mock, test } from 'bun:test' +import { mkdtempSync, rmSync } from 'fs' +import { tmpdir } from 'os' +import { join } from 'path' +import { getIsInteractive, setIsInteractive } from '../bootstrap/state.js' +import { getDefaultAppState } from '../state/AppStateStore.js' +import { snapshotEnv } from '../test/envSnapshot.js' +import { acquireSharedMutationLock, releaseSharedMutationLock } from '../test/sharedMutationLock.js' +import type { ToolUseContext, Tools } from '../Tool.js' +import { asAgentId } from '../types/ids.js' +import { createFileStateCacheWithSizeLimit } from './fileStateCache.js' +import { createAssistantMessage } from './messages.js' +import { SettingsSchema } from './settings/types.js' + +const ENV_KEYS = [ + 'OPENCLAUDE_TODO_REMINDER_TURNS_SINCE_WRITE', + 'OPENCLAUDE_TODO_REMINDER_TURNS_BETWEEN_REMINDERS', +] as const + +const SAVED_ENV = Object.fromEntries( + ENV_KEYS.map(key => [key, process.env[key]]), +) as Record<(typeof ENV_KEYS)[number], string | undefined> + +function restoreEnv(): void { + for (const key of ENV_KEYS) { + const value = SAVED_ENV[key] + if (value === undefined) { + delete process.env[key] + } else { + process.env[key] = value + } + } +} + +function clearTestEnv(): void { + for (const key of ENV_KEYS) { + delete process.env[key] + } +} + +// Snapshot the real settings module with a cache-busted import before any +// mock.module runs, so the cleanup below can restore the genuine +// implementation even when a previous serialized Bun test mocked the same +// module (pattern from attribution.test.ts / modelOptions.crossProfile.test.ts). +const realSettings = { ...await import(`./settings/settings.js?real=${Date.now()}`) } + +// Settings stubs passed to mockMergedSettings must already be schema-valid: +// SettingsSchema degrades invalid todoReminder values to undefined upstream +// (see the schema tests below), so the consumer never observes them. +function mockMergedSettings(settings: Record): void { + mock.module('./settings/settings.js', () => ({ + ...realSettings, + getInitialSettings: () => settings, + getSettings_DEPRECATED: () => settings, + getSettingsForSource: () => settings, + })) +} + +let settingsAwareAttachments: Promise | undefined +let getTodoReminderConfig: typeof import('./attachments.js')['getTodoReminderConfig'] +async function getSettingsAwareAttachments(): Promise { + // Fresh nonced import so the consumer observes this file's settings mock + // even when earlier serialized tests mocked the same modules. + settingsAwareAttachments ??= import( + `./attachments.ts?todoreminderSettings=${Date.now()}-${Math.random()}` + ) + return settingsAwareAttachments +} + +beforeEach(async () => { + await acquireSharedMutationLock('utils/attachments.todoReminder.test.ts') + clearTestEnv() + mockMergedSettings({}) + ;({ getTodoReminderConfig } = await getSettingsAwareAttachments()) +}) + +afterEach(() => { + try { + mock.restore() + mock.module('./settings/settings.js', () => realSettings) + restoreEnv() + } finally { + releaseSharedMutationLock() + } +}) + +describe('getTodoReminderConfig', () => { + test('returns defaults when nothing is configured', () => { + const config = getTodoReminderConfig() + expect(config.turnsSinceWrite).toBe(10) + expect(config.turnsBetweenReminders).toBe(10) + }) + + test('reads from environment variables', () => { + process.env.OPENCLAUDE_TODO_REMINDER_TURNS_SINCE_WRITE = '15' + process.env.OPENCLAUDE_TODO_REMINDER_TURNS_BETWEEN_REMINDERS = '20' + + const config = getTodoReminderConfig() + expect(config.turnsSinceWrite).toBe(15) + expect(config.turnsBetweenReminders).toBe(20) + }) + + test('handles invalid environment variable values', () => { + process.env.OPENCLAUDE_TODO_REMINDER_TURNS_SINCE_WRITE = 'invalid' + process.env.OPENCLAUDE_TODO_REMINDER_TURNS_BETWEEN_REMINDERS = '0' + + const config = getTodoReminderConfig() + expect(config.turnsSinceWrite).toBe(10) + expect(config.turnsBetweenReminders).toBe(10) + }) + + test('handles negative environment variable values', () => { + process.env.OPENCLAUDE_TODO_REMINDER_TURNS_SINCE_WRITE = '-5' + process.env.OPENCLAUDE_TODO_REMINDER_TURNS_BETWEEN_REMINDERS = '-10' + + const config = getTodoReminderConfig() + expect(config.turnsSinceWrite).toBe(10) + expect(config.turnsBetweenReminders).toBe(10) + }) + + test('handles partial environment variable configuration', () => { + process.env.OPENCLAUDE_TODO_REMINDER_TURNS_SINCE_WRITE = '15' + + const config = getTodoReminderConfig() + expect(config.turnsSinceWrite).toBe(15) + expect(config.turnsBetweenReminders).toBe(10) + }) + + test('rejects suffixed, fractional, and scientific-notation values', () => { + process.env.OPENCLAUDE_TODO_REMINDER_TURNS_SINCE_WRITE = '15turns' + process.env.OPENCLAUDE_TODO_REMINDER_TURNS_BETWEEN_REMINDERS = '2.5' + + const config = getTodoReminderConfig() + expect(config.turnsSinceWrite).toBe(10) + expect(config.turnsBetweenReminders).toBe(10) + + process.env.OPENCLAUDE_TODO_REMINDER_TURNS_SINCE_WRITE = '1e2' + expect(getTodoReminderConfig().turnsSinceWrite).toBe(10) + }) + + test('rejects oversized values instead of truncating them', () => { + process.env.OPENCLAUDE_TODO_REMINDER_TURNS_SINCE_WRITE = '9'.repeat(400) + + const config = getTodoReminderConfig() + expect(config.turnsSinceWrite).toBe(10) + }) + + test('accepts whitespace-padded valid values', () => { + process.env.OPENCLAUDE_TODO_REMINDER_TURNS_SINCE_WRITE = ' 12 ' + + const config = getTodoReminderConfig() + expect(config.turnsSinceWrite).toBe(12) + }) +}) + +describe('getTodoReminderConfig settings precedence', () => { + test('both settings fields override environment variables', async () => { + process.env.OPENCLAUDE_TODO_REMINDER_TURNS_SINCE_WRITE = '99' + process.env.OPENCLAUDE_TODO_REMINDER_TURNS_BETWEEN_REMINDERS = '99' + mockMergedSettings({ todoReminder: { turnsSinceWrite: 15, turnsBetweenReminders: 20 } }) + + const { getTodoReminderConfig: resolve } = await getSettingsAwareAttachments() + const config = resolve() + expect(config.turnsSinceWrite).toBe(15) + expect(config.turnsBetweenReminders).toBe(20) + }) + + test('partial settings keep the configured field and fall back per field', async () => { + mockMergedSettings({ todoReminder: { turnsSinceWrite: 15 } }) + + const { getTodoReminderConfig: resolve } = await getSettingsAwareAttachments() + const config = resolve() + expect(config.turnsSinceWrite).toBe(15) + expect(config.turnsBetweenReminders).toBe(10) + }) + + test('partial settings combine with environment fallbacks per field', async () => { + process.env.OPENCLAUDE_TODO_REMINDER_TURNS_SINCE_WRITE = '7' + mockMergedSettings({ todoReminder: { turnsBetweenReminders: 20 } }) + + const { getTodoReminderConfig: resolve } = await getSettingsAwareAttachments() + const config = resolve() + expect(config.turnsSinceWrite).toBe(7) + expect(config.turnsBetweenReminders).toBe(20) + }) + + test('settings without todoReminder fall back to environment variables', async () => { + process.env.OPENCLAUDE_TODO_REMINDER_TURNS_SINCE_WRITE = '15' + process.env.OPENCLAUDE_TODO_REMINDER_TURNS_BETWEEN_REMINDERS = '20' + mockMergedSettings({}) + + const { getTodoReminderConfig: resolve } = await getSettingsAwareAttachments() + const config = resolve() + expect(config.turnsSinceWrite).toBe(15) + expect(config.turnsBetweenReminders).toBe(20) + }) +}) + +describe('reminder attachment thresholds', () => { + let configDir: string + let wasInteractive: boolean + let restoreConsumerEnv: () => void + + beforeEach(() => { + wasInteractive = getIsInteractive() + restoreConsumerEnv = snapshotEnv([ + 'OPENCLAUDE_CONFIG_DIR', + 'CLAUDE_CODE_TASK_LIST_ID', + 'CLAUDE_CODE_ENABLE_TASKS', + 'CLAUDE_CODE_DISABLE_ATTACHMENTS', + 'CLAUDE_CODE_SIMPLE', + 'USER_TYPE', + ], { clear: true }) + configDir = mkdtempSync(join(tmpdir(), 'todo-reminder-test-')) + process.env.OPENCLAUDE_CONFIG_DIR = configDir + process.env.CLAUDE_CODE_TASK_LIST_ID = 'todo-reminder-test' + }) + + afterEach(() => { + setIsInteractive(wasInteractive) + restoreConsumerEnv() + rmSync(configDir, { recursive: true, force: true }) + }) + + for (const [type, toolName] of [ + ['todo_reminder', 'TodoWrite'], + ['task_reminder', 'TaskUpdate'], + ] as const) { + test.each([ + { field: 'turnsSinceWrite', turnsSinceWrite: 3, turnsBetweenReminders: 1, threshold: 3 }, + { field: 'turnsBetweenReminders', turnsSinceWrite: 1, turnsBetweenReminders: 4, threshold: 4 }, + ])(`${type} applies $field at its configured boundary`, async ({ + turnsSinceWrite, turnsBetweenReminders, threshold, + }) => { + setIsInteractive(type === 'task_reminder') + mockMergedSettings({ todoReminder: { turnsSinceWrite, turnsBetweenReminders } }) + const { getAttachments } = await getSettingsAwareAttachments() + let appState = getDefaultAppState() + const messages = Array.from({ length: threshold - 1 }, () => + createAssistantMessage({ content: 'Working on the task' }), + ) + const context: ToolUseContext = { + agentId: asAgentId('todo-reminder-test'), + abortController: new AbortController(), + readFileState: createFileStateCacheWithSizeLimit(1), + getAppState: () => appState, + setAppState: updater => { appState = updater(appState) }, + setInProgressToolUseIDs: () => {}, + setResponseLength: () => {}, + updateFileHistoryState: () => {}, + updateAttributionState: () => {}, + messages, + options: { + commands: [], + debug: false, + thinkingConfig: { type: 'disabled' }, + tools: [{ name: toolName } as Tools[number]], + verbose: false, + mcpClients: [], + mcpResources: {}, + isNonInteractiveSession: type === 'todo_reminder', + agentDefinitions: { activeAgents: [], allAgents: [], allowedAgentTypes: undefined }, + mainLoopModel: 'gpt-4o', + }, + nestedMemoryAttachmentTriggers: new Set(), + dynamicSkillDirTriggers: new Set(), + } + const collect = async () => (await getAttachments( + null, context, null, [], messages, 'compact', { skipSkillDiscovery: true }, + )).filter(attachment => attachment.type === type) + + expect(await collect()).toEqual([]) + messages.push(createAssistantMessage({ content: 'Continuing the task' })) + expect(await collect()).toEqual([{ type, content: [], itemCount: 0 }]) + }) + } +}) + +describe('todoReminder settings schema', () => { + const parseSettings = (settings: unknown) => SettingsSchema().safeParse(settings) + + test('degrades an invalid threshold to undefined while keeping the source valid', () => { + const result = parseSettings({ + includeCoAuthoredBy: true, + todoReminder: { turnsSinceWrite: 0, turnsBetweenReminders: 5 }, + }) + expect(result.success).toBe(true) + if (!result.success) return + expect(result.data.includeCoAuthoredBy).toBe(true) + expect(result.data.todoReminder?.turnsSinceWrite).toBeUndefined() + expect(result.data.todoReminder?.turnsBetweenReminders).toBe(5) + }) + + test('degrades negative and non-integer thresholds to undefined', () => { + for (const turnsSinceWrite of [-3, 2.5, '15']) { + const result = parseSettings({ todoReminder: { turnsSinceWrite } }) + expect(result.success).toBe(true) + if (!result.success) return + expect(result.data.todoReminder?.turnsSinceWrite).toBeUndefined() + } + }) + + test('degrades a malformed todoReminder object to undefined instead of invalidating the source', () => { + const result = parseSettings({ includeCoAuthoredBy: true, todoReminder: 'yes' }) + expect(result.success).toBe(true) + if (!result.success) return + expect(result.data.includeCoAuthoredBy).toBe(true) + expect(result.data.todoReminder).toBeUndefined() + }) + + test('preserves valid thresholds', () => { + const result = parseSettings({ + todoReminder: { turnsSinceWrite: 15, turnsBetweenReminders: 20 }, + }) + expect(result.success).toBe(true) + if (!result.success) return + expect(result.data.todoReminder).toEqual({ turnsSinceWrite: 15, turnsBetweenReminders: 20 }) + }) +}) diff --git a/src/utils/attachments.ts b/src/utils/attachments.ts index 3cd6cd451..93ae931fd 100644 --- a/src/utils/attachments.ts +++ b/src/utils/attachments.ts @@ -66,6 +66,7 @@ import { import { randomUUID, type UUID } from 'crypto' import { getSettings_DEPRECATED } from './settings/settings.js' import { getAPIProvider, isFirstPartyAnthropicBaseUrl } from './model/providers.js' +import { getEnvNumber } from './envUtils.js' import { getEffortEnvOverride, modelSupportsXHighEffort } from './effort.js' import { getSnippetForTwoFileDiff } from 'src/tools/FileEditTool/utils.js' import type { @@ -266,10 +267,25 @@ import { unassignTeammateTasks } from './tasks.js' import { getCompanionIntroAttachment } from '../buddy/prompt.js' import { isBuddyEnabled } from '../buddy/feature.js' -export const TODO_REMINDER_CONFIG = { - TURNS_SINCE_WRITE: 10, - TURNS_BETWEEN_REMINDERS: 10, -} as const +export function getTodoReminderConfig(): { + turnsSinceWrite: number + turnsBetweenReminders: number +} { + // Per-field precedence: merged-settings value when present, otherwise the + // matching environment variable, otherwise the default. Each threshold is + // resolved independently so a partial settings object still honors the + // field it configures. + const config = getSettings_DEPRECATED()?.todoReminder + + return { + turnsSinceWrite: + config?.turnsSinceWrite ?? + getEnvNumber('OPENCLAUDE_TODO_REMINDER_TURNS_SINCE_WRITE', 10), + turnsBetweenReminders: + config?.turnsBetweenReminders ?? + getEnvNumber('OPENCLAUDE_TODO_REMINDER_TURNS_BETWEEN_REMINDERS', 10), + } +} export const PLAN_MODE_ATTACHMENT_CONFIG = { TURNS_BETWEEN_ATTACHMENTS: 5, @@ -3711,10 +3727,12 @@ async function getTodoReminderAttachments( const { turnsSinceLastTodoWrite, turnsSinceLastReminder } = getTodoReminderTurnCounts(messages) + const reminderConfig = getTodoReminderConfig() + // Check if we should show a reminder if ( - turnsSinceLastTodoWrite >= TODO_REMINDER_CONFIG.TURNS_SINCE_WRITE && - turnsSinceLastReminder >= TODO_REMINDER_CONFIG.TURNS_BETWEEN_REMINDERS + turnsSinceLastTodoWrite >= reminderConfig.turnsSinceWrite && + turnsSinceLastReminder >= reminderConfig.turnsBetweenReminders ) { const todoKey = toolUseContext.agentId ?? getSessionId() const appState = toolUseContext.getAppState() @@ -3828,10 +3846,12 @@ async function getTaskReminderAttachments( const { turnsSinceLastTaskManagement, turnsSinceLastReminder } = getTaskReminderTurnCounts(messages) + const reminderConfig = getTodoReminderConfig() + // Check if we should show a reminder if ( - turnsSinceLastTaskManagement >= TODO_REMINDER_CONFIG.TURNS_SINCE_WRITE && - turnsSinceLastReminder >= TODO_REMINDER_CONFIG.TURNS_BETWEEN_REMINDERS + turnsSinceLastTaskManagement >= reminderConfig.turnsSinceWrite && + turnsSinceLastReminder >= reminderConfig.turnsBetweenReminders ) { const tasks = await listTasks(getTaskListId()) return [ diff --git a/src/utils/envUtils.ts b/src/utils/envUtils.ts index 7f24d008b..8fdba7fe9 100644 --- a/src/utils/envUtils.ts +++ b/src/utils/envUtils.ts @@ -577,6 +577,31 @@ export function isRunningOnHomespace(): boolean { ) } +/** + * Parse an environment variable as a positive integer. + * + * Returns defaultValue when the variable is unset or its value is not a + * complete decimal representation of a positive safe integer. Partially + * parsed, fractional, scientific-notation, and oversized values (for + * example "15turns", "1.5", "1e2", or a value above Number.MAX_SAFE_INTEGER) + * fall back to defaultValue rather than being truncated. + */ +export function getEnvNumber(key: string, defaultValue: number): number { + const value = process.env[key] + if (value === undefined) { + return defaultValue + } + const trimmed = value.trim() + if (!/^(0|[1-9]\d*)$/.test(trimmed)) { + return defaultValue + } + const parsed = Number.parseInt(trimmed, 10) + if (!Number.isSafeInteger(parsed) || parsed <= 0) { + return defaultValue + } + return parsed +} + /** * Conservative check for whether Claude Code is running inside a protected * (privileged or ASL3+) COO namespace or cluster. diff --git a/src/utils/settings/types.ts b/src/utils/settings/types.ts index 17a0dd7e0..b190ce88f 100644 --- a/src/utils/settings/types.ts +++ b/src/utils/settings/types.ts @@ -582,6 +582,32 @@ export const SettingsSchema = lazySchema(() => 'Auto-fix configuration: automatically run lint/test after AI file edits ' + 'and feed errors back for self-repair.', ), + todoReminder: z + .object({ + turnsSinceWrite: z + .number() + .int() + .positive() + .optional() + .catch(undefined) + .describe( + 'Assistant turns since last TodoWrite/TaskUpdate before showing reminder (default: 10)', + ), + turnsBetweenReminders: z + .number() + .int() + .positive() + .optional() + .catch(undefined) + .describe( + 'Assistant turns between reminders (default: 10)', + ), + }) + .optional() + .catch(undefined) + .describe( + 'Configuration for todo/task tool usage reminders', + ), worktree: z .preprocess((val: unknown) => { if (val && typeof val === 'object') {