refactor: extract parseScheduleCreateArgs pure function (#103)#113
refactor: extract parseScheduleCreateArgs pure function (#103)#113
Conversation
Extract parsing and validation from the 188-line scheduleCreate handler into a pure function returning Result<ParsedScheduleCreateArgs, string>, matching the parseLoopCreateArgs pattern in loop.ts. - Add ParsedScheduleCreateArgs interface (private) - Export parseScheduleCreateArgs (no side effects, no process.exit) - Simplify scheduleCreate handler to ~45 lines - Replace hand-rolled test helpers with real parser calls - Add 22 parser unit tests (happy paths + error cases) - Fix workingDirectory test to use process.cwd() (was bypassing validation) Closes #103
- Fix biome formatting (extra blank line in test file) - Simplifier changes: deduplicate baseOptions, replace nested ternary with if/else, remove redundant JSDoc, remove 4 duplicate tests
Include .claudeignore to protect against sensitive files and context pollution when using Claude-based tools. Co-Authored-By: Claude <noreply@anthropic.com>
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[beat schedule create args] --> B[parseScheduleCreateArgs\npure function]
B --> C{Result ok?}
C -->|err| D[ui.error + process.exit 1]
C -->|ok| E{isPipeline?}
E -->|true| F[pipelineSteps variant]
E -->|false| G[prompt variant]
F --> H[service.createScheduledPipeline\nbaseOptions + steps]
G --> I[service.createSchedule\nbaseOptions + prompt]
H --> J[ui.info success]
I --> J
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| process.exit(1); | ||
| return err('--missed-run-policy must be "skip", "catchup", or "fail"'); | ||
| } | ||
| missedRunPolicy = next as 'skip' | 'catchup' | 'fail'; |
There was a problem hiding this comment.
Type Safety: Type Assertions
The lines using as 'skip' | 'catchup' | 'fail' and as 'P0' | 'P1' | 'P2' after runtime includes() checks are safe but lose type information. TypeScript's includes() doesn't narrow the type to the literal union, necessitating the assertion.
Suggestion: Use type guard functions to achieve proper narrowing without as casts:
function isValidPolicy(v: string): v is 'skip' | 'catchup' | 'fail' {
return v === 'skip' || v === 'catchup' || v === 'fail';
}
function isValidPriority(v: string): v is 'P0' | 'P1' | 'P2' {
return v === 'P0' || v === 'P1' || v === 'P2';
}Then the checks become if (!isValidPolicy(next)) return err(...) followed by missedRunPolicy = next; — no cast needed, and full type narrowing.
Confidence: 80% | Category: Should Consider
src/cli/commands/schedule.ts
Outdated
| workingDirectory = pathResult.value; | ||
| i++; | ||
| } else if (arg === '--max-runs' && next) { | ||
| maxRuns = parseInt(next); |
There was a problem hiding this comment.
TypeScript: parseInt without explicit radix
Line uses parseInt(next) without specifying the radix parameter. While modern engines default to base-10 for decimal strings, best practice and ESLint's radix rule recommend always providing 10 explicitly to avoid ambiguity with edge cases (leading-zero strings in older contexts).
Fix: Change to parseInt(next, 10)
Confidence: 85% | Category: Best Practice
src/cli/commands/schedule.ts
Outdated
| /** | ||
| * Parsed arguments from CLI schedule create command | ||
| */ | ||
| interface ParsedScheduleCreateArgs { |
There was a problem hiding this comment.
Architecture: Discriminated Union for Pipeline/Prompt Invariant
The ParsedScheduleCreateArgs interface uses optional types (prompt?: string, pipelineSteps?: readonly string[]), but the parser guarantees: when isPipeline === true, pipelineSteps has items; when isPipeline === false, prompt is set. This forces lines 252 and 271 to use non-null assertions (args.pipelineSteps!, args.prompt!), losing the type-level proof.
Fix: Use a discriminated union to make TypeScript enforce the invariant:
type ParsedScheduleCreateArgs = {
readonly scheduleType: 'cron' | 'one_time';
readonly cronExpression?: string;
readonly scheduledAt?: string;
readonly timezone?: string;
readonly missedRunPolicy?: 'skip' | 'catchup' | 'fail';
readonly priority?: 'P0' | 'P1' | 'P2';
readonly workingDirectory?: string;
readonly maxRuns?: number;
readonly expiresAt?: string;
readonly afterScheduleId?: string;
readonly agent?: AgentProvider;
} & (
| { readonly isPipeline: true; readonly pipelineSteps: readonly string[]; readonly prompt?: string }
| { readonly isPipeline: false; readonly prompt: string; readonly pipelineSteps?: undefined }
);Then args.pipelineSteps and args.prompt narrow automatically after checking args.isPipeline. No ! assertions needed.
Confidence: 82% | Category: Type Safety
src/cli/commands/schedule.ts
Outdated
| // Non-pipeline mode: prompt is required | ||
| const prompt = promptWords.join(' '); | ||
| if (!isPipeline && !prompt) { | ||
| return err('Usage: beat schedule create <prompt> --cron "..." | --at "..." [options]'); |
There was a problem hiding this comment.
Consistency: Missing Pipeline Usage Hint
The original scheduleCreate function displayed a two-line error when no prompt was provided in non-pipeline mode: the usage string plus a pipeline example (Pipeline: beat schedule create --pipeline --step "lint" --step "test" --cron "0 9 * * *"). This refactored version only returns the single-line usage error via parseScheduleCreateArgs, and the pipeline hint is lost. Users who forget the prompt no longer see the helpful pipeline alternative.
Fix: Include the pipeline hint in the error message returned by the parser:
if (!isPipeline && !prompt) {
return err(
'Usage: beat schedule create <prompt> --cron "..." | --at "..." [options]\n' +
' Pipeline: beat schedule create --pipeline --step "lint" --step "test" --cron "0 9 * * *"'
);
}Confidence: 90% | Category: UX/Regression Prevention
Summary: Code Review FindingsThis PR is a well-executed extraction that follows the established pattern from loop.ts. 3 inline comments created for high-confidence findings (≥80%). Below are lower-confidence suggestions (60-79%) for consideration. Blocking IssuesNone. All findings are improvements to type safety, consistency, or code quality. Should-Consider Suggestions (60-79% Confidence)1. Inconsistent Prompt Field Handling vs Loop Parser (Line 154, 82% confidence) 2. Duplicated Option-Mapping Logic (tests/unit/cli.test.ts:2497-2582, 72% confidence) 3. Pure Function Location (src/cli/commands/schedule.ts:33, 65% confidence) 4. Missing ARCHITECTURE JSDoc (src/cli/commands/schedule.ts:30-32, 85% confidence) Overall AssessmentScore: 8/10 across all review dimensions. Recommendation: APPROVED. The refactoring is sound and well-tested. The 3 inline comments address type safety and UX regression. Lower-confidence suggestions are improvements to consistency and maintainability, not blockers. Test Coverage: 233 CLI tests pass (up from ~208 on main). No regressions detected. Review by Claude Code | Backbeat Code Review System |
- Suppress prompt in pipeline mode to match loop parser pattern - Remove dead handler warning (prompt now undefined in pipeline mode) - Add pipeline usage hint to missing-prompt error message - Add explicit radix 10 to parseInt for --max-runs Co-Authored-By: Claude <noreply@anthropic.com>
Replace optional prompt/pipelineSteps with discriminated union on isPipeline, eliminating non-null assertions in handlers. Both parsers now encode mutual exclusivity in the type system.
- Add ARCHITECTURE JSDoc to parseScheduleCreateArgs (matches loop parser) - Add missing agent option to simulateScheduleCreate test helper
| // Pipeline mode | ||
| if (isPipeline) { | ||
| if (promptWords.length > 0) { | ||
| ui.info(`Ignoring positional prompt text in --pipeline mode: "${promptWords.join(' ')}". Use --step flags only.`); | ||
| } | ||
| if (pipelineSteps.length < 2) { | ||
| ui.error('Pipeline requires at least 2 --step flags'); | ||
| process.exit(1); | ||
| return err('Pipeline requires at least 2 --step flags'); | ||
| } | ||
| } else if (pipelineSteps.length > 0) { | ||
| return err( | ||
| '--step requires --pipeline. Did you mean: beat schedule create --pipeline --step "..." --step "..." --cron "..."', | ||
| ); | ||
| } |
There was a problem hiding this comment.
--pipeline mode are now silently discarded
The original handler emitted a ui.info(...) diagnostic when positional prompt words were provided alongside --pipeline:
ui.info(`Ignoring positional prompt text in --pipeline mode: "${promptWords.join(' ')}". Use --step flags only.`);
This is now gone. The new pure function simply ignores promptWords in pipeline mode (they are collected but never referenced in the isPipeline branch). A user who accidentally writes beat schedule create my notes --pipeline --step lint --step test --cron "0 9 * * *" gets a successful result with the prompt text silently swallowed, rather than a hint that they're doing something unexpected.
If keeping this silent behavior is deliberate (to match the loop parser), consider at minimum surfacing a warning in the scheduleCreate handler after parsing, e.g.:
if (args.isPipeline && scheduleArgs.some(a => !a.startsWith('-') && /* not a value */ ...)) {
ui.warn('Positional text is ignored in --pipeline mode; use --step flags only.');
}Or alternatively, reject the input with an error (the strictest option, ensuring no ambiguity).
| } else if (pipelineSteps.length > 0) { | ||
| return err( | ||
| '--step requires --pipeline. Did you mean: beat schedule create --pipeline --step "..." --step "..." --cron "..."', | ||
| ); |
There was a problem hiding this comment.
--step error hint only mentions --cron, not --at
The error message for --step without --pipeline ends with --cron "...", which inadvertently implies cron is the only valid scheduling option. A user building a one-time pipeline (--at "2026-06-01T09:00:00Z") would find the suggestion misleading.
return err(
'--step requires --pipeline. Did you mean: beat schedule create --pipeline --step "..." --step "..." --cron "..."',
);Consider broadening the hint to cover both scheduling forms:
| } else if (pipelineSteps.length > 0) { | |
| return err( | |
| '--step requires --pipeline. Did you mean: beat schedule create --pipeline --step "..." --step "..." --cron "..."', | |
| ); | |
| return err( | |
| '--step requires --pipeline. Did you mean: beat schedule create --pipeline --step "..." --step "..." --cron "0 9 * * *" | --at "..."', | |
| ); |
1 participant
Summary
Extract parsing and validation logic from the scheduleCreate handler into a pure function returning
Result<ParsedScheduleCreateArgs, string>, following theparseLoopCreateArgspattern.Changes
Core refactor:
parseScheduleCreateArgs()pure function from 188-line handlerParsedScheduleCreateArgsinterface (private)Test improvements:
process.cwd()(was incorrectly bypassing validation)Polish (self-review):
Breaking Changes
None.
Testing