feat: extract spawn operation to core/operations/spawn.ts#98
Open
2witstudios wants to merge 3 commits into
Open
feat: extract spawn operation to core/operations/spawn.ts#982witstudios wants to merge 3 commits into
2witstudios wants to merge 3 commits into
Conversation
Move worktree creation + agent spawning pipeline from commands/spawn.ts into core/operations/spawn.ts as performSpawn() with typed options and result. Commands layer now handles only arg parsing + output formatting. Closes #62
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR extracts the spawn command's core workflow into a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- P2: Replace inline import('...').Config with top-level type import
- P3: Restore context-specific success messages (new vs branch vs existing)
- P4: Only show attach hint for newly created worktrees
- P5: Remove redundant result-shape test (TypeScript already guarantees)
- P6: Add --template prompt resolution test
- P7: Add manifest updater ordering tests (skeleton before agents, tmux
window persisted before spawn, partial failure scenario)
- P8: Remove unused mock variable declarations
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/operations/spawn.ts`:
- Line 60: Validate and sanitize options.count at the start of the spawn routine
before any side effects: ensure the value read from options.count (used to set
const count) is a finite integer >= 1 (e.g. coerce via Math.floor or parseInt
then check Number.isFinite and >=1) and reject or default invalid values (NaN,
0, negative, decimals) so the subsequent spawn loop and any side-effecting code
(references around const count and the later spawn loop at the section around
lines 165-166) cannot create zero or too many agents; if invalid, set a safe
default (1) or throw a clear error.
- Line 59: resolveAgentConfig(config, options.agent) can throw a plain Error
which bypasses our typed CLI error handling; wrap the call in a try/catch around
the const agentConfig = resolveAgentConfig(...) expression and on failure throw
a PpgError with the proper code (e.g. ErrorCode.INVALID_ARGS) that includes the
original error message (or sets the original error as the cause) so callers
receive a typed error. Ensure you reference the symbols resolveAgentConfig and
PpgError (and ErrorCode.INVALID_ARGS) when making the change and preserve the
original error text in the new PpgError.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/commands/spawn.test.tssrc/commands/spawn.tssrc/core/operations/spawn.test.tssrc/core/operations/spawn.ts
| throw new NotInitializedError(projectRoot); | ||
| } | ||
|
|
||
| const agentConfig = resolveAgentConfig(config, options.agent); |
There was a problem hiding this comment.
Unknown --agent errors leak as untyped Error.
resolveAgentConfig can throw a generic Error, so this operation may bypass typed error codes and break consistent CLI error handling.
Proposed fix
- const agentConfig = resolveAgentConfig(config, options.agent);
+ let agentConfig: AgentConfig;
+ try {
+ agentConfig = resolveAgentConfig(config, options.agent);
+ } catch (error) {
+ throw new PpgError(
+ error instanceof Error ? error.message : 'Invalid --agent value',
+ 'INVALID_ARGS',
+ );
+ }As per coding guidelines: "Use PpgError hierarchy with typed error codes: ... INVALID_ARGS ...".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const agentConfig = resolveAgentConfig(config, options.agent); | |
| let agentConfig: AgentConfig; | |
| try { | |
| agentConfig = resolveAgentConfig(config, options.agent); | |
| } catch (error) { | |
| throw new PpgError( | |
| error instanceof Error ? error.message : 'Invalid --agent value', | |
| 'INVALID_ARGS', | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/operations/spawn.ts` at line 59, resolveAgentConfig(config,
options.agent) can throw a plain Error which bypasses our typed CLI error
handling; wrap the call in a try/catch around the const agentConfig =
resolveAgentConfig(...) expression and on failure throw a PpgError with the
proper code (e.g. ErrorCode.INVALID_ARGS) that includes the original error
message (or sets the original error as the cause) so callers receive a typed
error. Ensure you reference the symbols resolveAgentConfig and PpgError (and
ErrorCode.INVALID_ARGS) when making the change and preserve the original error
text in the new PpgError.
| } | ||
|
|
||
| const agentConfig = resolveAgentConfig(config, options.agent); | ||
| const count = options.count ?? 1; |
There was a problem hiding this comment.
Validate count before any spawn side effects.
count currently accepts non-integer/invalid values (0, negative, NaN, decimals), which can create worktrees/windows with zero agents or over-spawn due loop semantics.
Proposed fix
const count = options.count ?? 1;
+ if (!Number.isInteger(count) || count < 1) {
+ throw new PpgError('--count must be a positive integer', 'INVALID_ARGS');
+ }Also applies to: 165-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/operations/spawn.ts` at line 60, Validate and sanitize options.count
at the start of the spawn routine before any side effects: ensure the value read
from options.count (used to set const count) is a finite integer >= 1 (e.g.
coerce via Math.floor or parseInt then check Number.isFinite and >=1) and reject
or default invalid values (NaN, 0, negative, decimals) so the subsequent spawn
loop and any side-effecting code (references around const count and the later
spawn loop at the section around lines 165-166) cannot create zero or too many
agents; if invalid, set a safe default (1) or throw a clear error.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
commands/spawn.tsintocore/operations/spawn.tsperformSpawn()function with typedPerformSpawnOptionsinput andSpawnResultoutputcommands/spawn.tsto ~30 lines: arg parsing + output formatting--branch), existing worktree (--worktree)Test plan
core/operations/spawn.test.tscovering all spawn paths, validation, and result shapecommands/spawn.test.tstests still pass (2/2)cron-parserdep)schedule.tsandspawn.test.tsunrelated)Closes #62
Summary by CodeRabbit