Skip to content

Comments

refactor!: make maxRetries explicit, add CLI_DEFAULT_MAX_RETRIES and --max-retries flag#165

Merged
jlevy merged 2 commits intomainfrom
claude/explicit-retry-parameter-qCnRc
Feb 23, 2026
Merged

refactor!: make maxRetries explicit, add CLI_DEFAULT_MAX_RETRIES and --max-retries flag#165
jlevy merged 2 commits intomainfrom
claude/explicit-retry-parameter-qCnRc

Conversation

@jlevy
Copy link
Owner

@jlevy jlevy commented Feb 23, 2026

Summary

Makes maxRetries a required field on the TypeScript API (FillOptions, LiveAgentConfig) so callers must choose their retry policy explicitly. Introduces a single CLI_DEFAULT_MAX_RETRIES = 3 constant as the sole source of truth for CLI commands, and adds a --max-retries flag to fill and research commands.

Changes

Breaking: TypeScript API

  • FillOptions.maxRetries changed from optional to required number
  • LiveAgentConfig.maxRetries changed from optional to required number
  • Removed DEFAULT_MAX_RETRIES constant (was the hidden default for the optional field)
  • Removed fallback config.maxRetries ?? DEFAULT_MAX_RETRIES in LiveAgent constructor
  • Simplified programmaticFill.ts — no more conditional spread, just direct pass-through

New: CLI default constant

  • Added CLI_DEFAULT_MAX_RETRIES = 3 in settings.ts — the ONE place the value lives
  • All CLI commands (fill, run, research, test-live-agent.ts) reference this constant
  • Zero hardcoded maxRetries: 3 anywhere in the codebase

New: --max-retries CLI flag

  • Added to fill and research commands with Commander default from the constant
  • run command uses the constant directly (it's an interactive launcher, no flag needed)

JSDoc improvements

  • Both FillOptions.maxRetries and LiveAgentConfig.maxRetries now document:
    • "Required — must be set explicitly"
    • "The Vercel AI SDK's own default is 2" (so users know the baseline)
    • "Common values: 0 (no retries / tests), 2–3 (production)"

Test updates

  • All ~60 test call sites updated to pass maxRetries: 0 explicitly (tests shouldn't retry)
  • Added test verifying CLI_DEFAULT_MAX_RETRIES === 3
  • Updated test documenting intentional removal of DEFAULT_MAX_RETRIES

Test Plan

  • All 73 test files pass (2267 tests, 1 skipped) — verified via pnpm test
  • TypeScript type checking passes — verified via pnpm typecheck
  • ESLint passes — verified via pre-commit hook
  • Formatter passes — verified via pre-commit hook
  • No hardcoded maxRetries: 3 in any .ts file (verified via grep)
  • CLI_DEFAULT_MAX_RETRIES is the sole occurrence of the value 3 in retry context
  • Manual: markform fill --help shows --max-retries with default
  • Manual: markform research --help shows --max-retries with default
  • Manual: markform fill --max-retries 0 overrides the default
  • Edge case: TypeScript compile error if maxRetries omitted from fillForm() call

Related Beads

None — this was a direct user request for API hygiene.

https://claude.ai/code/session_01Df5orNck9FRQ8sZMkqPRyU


Note

Medium Risk
This is an API-breaking type change that touches core harness/agent construction and all call sites; runtime behavior is mostly unchanged but missing/incorrect maxRetries plumbing could cause compile failures or unexpected retry behavior in consumers.

Overview
Breaking change: maxRetries is now required on the TypeScript API (FillOptions, LiveAgentConfig), removing the implicit DEFAULT_MAX_RETRIES fallback and forcing all callers (including internal harness paths) to pass an explicit retry policy.

Adds CLI_DEFAULT_MAX_RETRIES = 3 as the single CLI source of truth and wires it through CLI entrypoints (fill, research, run, scripts/test-live-agent.ts), including new --max-retries flags on fill and research; tests are updated to pass maxRetries: 0 explicitly to keep retries disabled in test runs.

Written by Cursor Bugbot for commit 5fa04cc. This will update automatically on new commits. Configure here.

BREAKING CHANGE: `maxRetries` is now required on both `FillOptions`
and `LiveAgentConfig`. Callers must explicitly specify the retry count
(0 for no retries, 2–3 for production). This prevents downstream
libraries from unknowingly inheriting a default retry policy, which
has been a source of bugs.

Changes:
- Remove `DEFAULT_MAX_RETRIES` constant from settings.ts
- Make `maxRetries` required (not optional) on `FillOptions` and
  `LiveAgentConfig` interfaces
- Remove fallback `?? DEFAULT_MAX_RETRIES` in LiveAgent constructor
- Pass `maxRetries` directly in programmaticFill.ts (not conditionally)
- CLI fill/run/research commands explicitly pass `maxRetries: 3`
- Fix test-live-agent.ts script to pass explicit `maxRetries: 3`
  to `generateText()` (was using AI SDK's own hidden default of 2)
- All test calls updated to pass `maxRetries: 0`

https://claude.ai/code/session_01Df5orNck9FRQ8sZMkqPRyU

Co-Authored-By: Claude <noreply@anthropic.com>
…flag

- Add CLI_DEFAULT_MAX_RETRIES = 3 in settings.ts as the single source
  of truth for the CLI retry default value
- Add --max-retries <n> CLI flag to fill and research commands
- Replace all 5 hardcoded `maxRetries: 3` values with the constant
- Update JSDoc on FillOptions/LiveAgentConfig to mention the AI SDK
  default (2) while keeping the field required
- TypeScript API remains explicit (no default) — CLI has one constant

Co-Authored-By: Claude <noreply@anthropic.com>

https://claude.ai/code/session_01Df5orNck9FRQ8sZMkqPRyU
@github-actions
Copy link
Contributor

Coverage Report for packages/markform

Status Category Percentage Covered / Total
🔵 Lines 66.19% (🎯 64%) 6116 / 9239
🔵 Statements 65.98% (🎯 64%) 6335 / 9600
🔵 Functions 65.58% (🎯 64%) 747 / 1139
🔵 Branches 62.62% (🎯 60%) 4413 / 7047
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
packages/markform/src/settings.ts 97.72% 94.11% 100% 98.75% 70-74, 330
packages/markform/src/cli/commands/fill.ts 0% 0% 0% 0% 89-1063
packages/markform/src/harness/liveAgent.ts 71.48% 48.97% 87.09% 71.53% 92-97, 109, 143, 148, 153, 185-189, 212-234, 240-249, 273-278, 288-295, 418-421, 470-473, 653, 707-708, 719-725, 733-736, 776-856
packages/markform/src/harness/programmaticFill.ts 71.35% 69.25% 65.71% 71.87% 102-125, 203, 211-307, 316, 334, 354, 601-602, 657-661, 731-736, 746-747, 914-915, 931-947, 981-1003, 1060-1062, 1071, 1117-1129, 1134-1135, 1188-1193, 1210, 1241-1246, 1261, 1287
Generated in workflow #1008 for commit 5fa04cc by the Vitest Coverage Report Action

@jlevy jlevy merged commit fabb7bd into main Feb 23, 2026
2 checks passed
@jlevy jlevy deleted the claude/explicit-retry-parameter-qCnRc branch February 23, 2026 20:36
Copy link
Owner Author

@jlevy jlevy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Senior Engineering Code Review

Summary

Clean, well-scoped refactoring that eliminates hidden defaults and makes the retry policy explicit everywhere. The breaking change is the right call — TypeScript should enforce that callers think about retries. The single-constant-for-CLI pattern is exactly the right level of DRY.

Strengths

  • Excellent single-source-of-truth discipline: CLI_DEFAULT_MAX_RETRIES = 3 in settings.ts is the ONE place the value lives. Zero hardcoded 3 anywhere else. This is the gold standard for magic numbers.
  • Clean API boundary: CLI_DEFAULT_MAX_RETRIES stays internal (not exported from index.ts), while the public types (FillOptions, LiveAgentConfig) simply require the number. Good separation.
  • JSDoc mentions AI SDK default (2): Telling callers "the SDK default is 2, we use 3" gives them the context to make an informed choice without having to read SDK source.
  • Tests use maxRetries: 0: The right choice — tests shouldn't retry, and this makes that explicit rather than relying on a mock path that happens to skip retries.
  • Commander flags show the default in help text: --max-retries <n> with (default: 3) is exactly what CLI users expect.

Issues

None blocking.

Nits (non-blocking)

  1. Style inconsistency between fill.ts and research.ts:

    • fill.ts:398 uses options.maxRetries! (non-null assertion)
    • research.ts:167 uses options.maxRetries as string (type assertion)

    Both are safe (Commander default guarantees the value), but the style is inconsistent. The ! approach is more idiomatic for Commander options with defaults. Minor — not worth a follow-up commit.

Documentation

  • No public docs (development.md) reference the removed constant — no updates needed.
  • Active specs reference maxRetries in design context — correct as historical artifacts.
  • CLI_DEFAULT_MAX_RETRIES is correctly NOT exported from public API (index.ts).

CI Status

  • test: pass (3m36s)
  • Cursor Bugbot: pass (6m2s)

All checks green.

Verdict

Approve. This is a well-executed breaking change with thorough test coverage (all ~60 call sites updated) and clear documentation of the breaking nature via conventional commit prefix (refactor!:).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants