feat: add recoup content command suite#13
Merged
sweetmantech merged 12 commits intomainfrom Mar 13, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new top-level CLI command Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant CLI as contentCommand
participant Client as HTTP Client
participant API as Content API
participant Tasks as Tasks API
User->>CLI: content create (artist, template, batch, options)
CLI->>Client: POST /api/content/create
Client->>API: forward request
API-->>Client: returns runId(s)
Client-->>CLI: runId(s)
CLI-->>User: prints started run(s) and polling guidance
alt User requests status
User->>CLI: tasks status --run <id>
CLI->>Client: GET /api/tasks/runs?runId=...
Client->>Tasks: fetch run status
Tasks-->>Client: run status + outputs
Client-->>CLI: status payload
CLI-->>User: prints status and video URL (if present)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
__tests__/commands/content.test.ts (1)
75-94: Tests verify default values for new flags; consider adding explicit flag tests.The create test correctly asserts that
caption_length: "short"andupscale: falseare sent by default. For completeness, consider adding a test that explicitly passes--caption-length medium --upscaleto verify non-default values are propagated.💡 Optional: Test explicit flag values
it("creates content run with custom caption-length and upscale", async () => { vi.mocked(post).mockResolvedValue({ runId: "run_xyz789", status: "triggered", }); await contentCommand.parseAsync( ["create", "--artist", "test-artist", "--caption-length", "long", "--upscale"], { from: "user" }, ); expect(post).toHaveBeenCalledWith("/api/content/create", { artist_slug: "test-artist", template: "artist-caption-bedroom", lipsync: false, caption_length: "long", upscale: true, }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/commands/content.test.ts` around lines 75 - 94, Add a new test that verifies non-default flag propagation: mock post (vi.mocked(post).mockResolvedValue(...)) then call contentCommand.parseAsync with explicit flags like "--caption-length" (e.g., "long" or "medium") and "--upscale", and assert post was called with the corresponding payload keys (artist_slug, template, lipsync, caption_length set to the explicit value, and upscale: true); reference the existing test structure that uses contentCommand.parseAsync and expect(post).toHaveBeenCalledWith to mirror naming and assertions.src/commands/content.ts (1)
91-92: Usenew Option()with.addOption()to validate--caption-lengthwith.choices().The option currently accepts arbitrary strings, but only
short,medium, andlongare valid. Commander v13's.choices()method (available onOptionobjects) provides CLI-level validation and auto-generated help output.The
.option()method doesn't support chaining.choices()directly. Instead, usenew Option()with.addOption():♻️ Proposed fix
import { Command, Option } from "commander"; // ... const createCommand = new Command("create") // ... .addOption( new Option("--caption-length <length>", "Caption length") .choices(["short", "medium", "long"]) .default("short") )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/content.ts` around lines 91 - 92, Replace the free-form .option("--caption-length <length>", ...) with a Commander Option to enforce choices: import Option if not present, then on the Command instance used to build the create command (e.g., createCommand) call .addOption(new Option("--caption-length <length>", "Caption length").choices(["short","medium","long"]).default("short")); this ensures only "short", "medium", or "long" are accepted and updates help output accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@__tests__/commands/content.test.ts`:
- Around line 75-94: Add a new test that verifies non-default flag propagation:
mock post (vi.mocked(post).mockResolvedValue(...)) then call
contentCommand.parseAsync with explicit flags like "--caption-length" (e.g.,
"long" or "medium") and "--upscale", and assert post was called with the
corresponding payload keys (artist_slug, template, lipsync, caption_length set
to the explicit value, and upscale: true); reference the existing test structure
that uses contentCommand.parseAsync and expect(post).toHaveBeenCalledWith to
mirror naming and assertions.
In `@src/commands/content.ts`:
- Around line 91-92: Replace the free-form .option("--caption-length <length>",
...) with a Commander Option to enforce choices: import Option if not present,
then on the Command instance used to build the create command (e.g.,
createCommand) call .addOption(new Option("--caption-length <length>", "Caption
length").choices(["short","medium","long"]).default("short")); this ensures only
"short", "medium", or "long" are accepted and updates help output accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4099a92-93b9-4045-a7d2-9dae460426de
📒 Files selected for processing (3)
__tests__/commands/content.test.tssrc/bin.tssrc/commands/content.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/commands/content.ts (1)
5-7: Export a single command-builder function to align with repo SRP rule.This file currently exports a constant command object. The guideline asks for one exported function per TS file; exporting
buildContentCommand()would align with that rule while keeping this command group cohesive.Suggested refactor
-export const contentCommand = new Command("content") - .description("Content-creation pipeline commands"); +export function buildContentCommand(): Command { + const contentCommand = new Command("content") + .description("Content-creation pipeline commands"); + + contentCommand.addCommand(templatesCommand); + contentCommand.addCommand(validateCommand); + contentCommand.addCommand(estimateCommand); + contentCommand.addCommand(createCommand); + contentCommand.addCommand(statusCommand); + + return contentCommand; +} @@ -contentCommand.addCommand(templatesCommand); -contentCommand.addCommand(validateCommand); -contentCommand.addCommand(estimateCommand); -contentCommand.addCommand(createCommand); -contentCommand.addCommand(statusCommand);As per coding guidelines,
src/**/*.tsshould “Follow Single Responsibility Principle with one exported function per file.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/content.ts` around lines 5 - 7, Replace the exported constant contentCommand with a single exported function buildContentCommand(): Command that constructs and returns new Command("content").description("Content-creation pipeline commands"); change the file to export only that function (no other top-level exports) and update any call sites that referenced contentCommand to call buildContentCommand() to obtain the Command instance; keep the same Command setup (name and description) and explicit return type to satisfy the repo SRP rule.
🤖 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/commands/content.ts`:
- Around line 29-30: The code calls printError((err as Error).message) in
several catch blocks which assumes thrown values are Error instances; add a
small utility (e.g., formatError(err: unknown): string) that normalizes the
error: if err instanceof Error return err.message, else if typeof err ===
'string' return err, else try JSON.stringify(err) and fallback to String(err) or
'Unknown error'. Replace all occurrences of printError((err as Error).message)
in this file (references: printError usage in src/commands/content.ts) with
printError(formatError(err)) so non-Error throws are printed sensibly.
- Around line 64-72: The CLI currently passes unvalidated opts.batch and
opts.captionLength into the request payload (inside the .action handler that
calls get("/api/content/estimate")), relying on parseInt which can yield NaN;
update the .action code to validate opts.batch and opts.captionLength before
building the payload: call parseInt on opts.batch and opts.captionLength, ensure
the results are finite integers (e.g., Number.isInteger and >0 or other allowed
range), and if invalid print a clear CLI error and exit (or throw) instead of
sending NaN downstream; reference the option flags defined with
.option("--batch") and .option("--caption-length") and the parseInt usages to
locate where to add these checks.
---
Nitpick comments:
In `@src/commands/content.ts`:
- Around line 5-7: Replace the exported constant contentCommand with a single
exported function buildContentCommand(): Command that constructs and returns new
Command("content").description("Content-creation pipeline commands"); change the
file to export only that function (no other top-level exports) and update any
call sites that referenced contentCommand to call buildContentCommand() to
obtain the Command instance; keep the same Command setup (name and description)
and explicit return type to satisfy the repo SRP rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1568db81-f62d-4e4c-9716-05dd8a058111
📒 Files selected for processing (2)
__tests__/commands/content.test.tssrc/commands/content.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/commands/content.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/commands/content.ts (2)
63-71:⚠️ Potential issue | 🟠 MajorReject invalid
--batchand--caption-lengthbefore building the request.These values are still forwarded unchecked.
parseIntcan yieldNaN, and arbitrary caption lengths can slip through, so malformed requests reach the API instead of failing fast in the CLI.Suggested fix
+const parsePositiveInt = (value: string, flag: string): number => { + const parsed = Number.parseInt(value, 10); + if (!Number.isInteger(parsed) || parsed < 1) { + throw new Error(`${flag} must be a positive integer`); + } + return parsed; +}; + +const ALLOWED_CAPTION_LENGTHS = new Set(["short", "medium", "long"]); + const estimateCommand = new Command("estimate") @@ .action(async opts => { try { + const batch = parsePositiveInt(String(opts.batch ?? "1"), "--batch"); const data = await get("/api/content/estimate", { lipsync: opts.lipsync ? "true" : "false", - batch: String(opts.batch || "1"), + batch: String(batch), compare: opts.compare ? "true" : "false", }); @@ const createCommand = new Command("create") @@ .action(async opts => { try { + const batch = parsePositiveInt(String(opts.batch ?? "1"), "--batch"); + if (!ALLOWED_CAPTION_LENGTHS.has(opts.captionLength)) { + throw new Error("--caption-length must be one of: short, medium, long"); + } const data = await post("/api/content/create", { artist_account_id: opts.artist, template: opts.template, lipsync: !!opts.lipsync, caption_length: opts.captionLength, upscale: !!opts.upscale, - batch: parseInt(opts.batch, 10), + batch, });Also applies to: 91-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/content.ts` around lines 63 - 71, Validate and reject invalid numeric CLI inputs before calling get("/api/content/estimate"): parse opts.batch and the caption-length option (e.g., opts.batch and opts.captionLength or opts["caption-length"]) with Number/parseInt, check for NaN and enforce sane bounds (e.g., batch ≥1 and within your max, caption length within allowed min/max), and short-circuit the action by printing an error and exiting (or throwing) if validation fails; update both the block that builds the estimate request and the similar block at the later occurrence (lines around the second get("/api/content/estimate") usage) so only validated, sanitized values are sent to the API.
28-30:⚠️ Potential issue | 🟡 MinorNormalize caught errors before printing.
(err as Error).messagestill assumes every thrown value is anError. Thrown strings/objects will degrade toundefined, which makes CLI failures much harder to diagnose.Also applies to: 55-57, 81-83, 121-123, 153-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/content.ts` around lines 28 - 30, Several catch blocks call printError((err as Error).message) which assumes thrown values are Error instances; replace that with a small normalization helper (e.g., getErrorMessage(err)) and use printError(getErrorMessage(err)) wherever present (all catch blocks currently using printError((err as Error).message)). Implement getErrorMessage to accept any unknown value, return err.message if err is an Error, return String(err) for primitives, JSON.stringify for plain objects (falling back to a safe placeholder like "Unknown error" on circular/undefined), and use that helper in the catch blocks referencing the err variable and printError function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/commands/content.test.ts`:
- Around line 97-116: The test mocks post to return { runId } but the command
expects data.runIds; update the mockResolvedValue to return { runIds:
["run_xyz789"] } (matching how contentCommand reads the response), then also
assert the expected success log output (the logger used in the test or console
output) after calling contentCommand.parseAsync so the happy path is actually
verified rather than only the POST payload; ensure the mock for process.exit
remains a no-op so the command can complete and the success log assertion runs.
In `@src/commands/content.ts`:
- Around line 111-120: Guard access to data.runIds before dereferencing:
validate that data.runIds is an array (use Array.isArray(data.runIds)) and
handle the non-array or empty cases by printing a clear CLI error (e.g.,
console.error with guidance) and exiting/returning early; then use the validated
runIds array for the existing branching that logs single vs batch runs. Ensure
you reference the variable runIds (and original data) so the change replaces the
current unconditional const runIds = data.runIds as string[]; with a safe type
check and early error path.
---
Duplicate comments:
In `@src/commands/content.ts`:
- Around line 63-71: Validate and reject invalid numeric CLI inputs before
calling get("/api/content/estimate"): parse opts.batch and the caption-length
option (e.g., opts.batch and opts.captionLength or opts["caption-length"]) with
Number/parseInt, check for NaN and enforce sane bounds (e.g., batch ≥1 and
within your max, caption length within allowed min/max), and short-circuit the
action by printing an error and exiting (or throwing) if validation fails;
update both the block that builds the estimate request and the similar block at
the later occurrence (lines around the second get("/api/content/estimate")
usage) so only validated, sanitized values are sent to the API.
- Around line 28-30: Several catch blocks call printError((err as
Error).message) which assumes thrown values are Error instances; replace that
with a small normalization helper (e.g., getErrorMessage(err)) and use
printError(getErrorMessage(err)) wherever present (all catch blocks currently
using printError((err as Error).message)). Implement getErrorMessage to accept
any unknown value, return err.message if err is an Error, return String(err) for
primitives, JSON.stringify for plain objects (falling back to a safe placeholder
like "Unknown error" on circular/undefined), and use that helper in the catch
blocks referencing the err variable and printError function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d5bc99c-b633-4d4a-a16e-c3f8a21cd727
📒 Files selected for processing (2)
__tests__/commands/content.test.tssrc/commands/content.ts
- supports short, medium, long (defaults to short) - update test for new param
- test caption-length, upscale, and batch with explicit values
sidneyswift
force-pushed
the
feat/content-creation-pipeline
branch
from
240375a to
a1f2283
Compare
- Create tasks command with status subcommand - Remove status subcommand from content command - Update create hint to reference `recoup tasks status` - Add tasks tests, update content tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nIds guard - Extract getErrorMessage() to output.ts for safe non-Error handling - Validate --batch as positive integer, --caption-length as short|medium|long - Guard data.runIds before dereferencing in content create - Fix custom flags test mock to use runIds array - Add tests for all three issues Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
__tests__/commands/tasks.test.ts (1)
3-6: Minor: Unusedpostmock.The
postfunction is mocked but not used in any of the tests sincetasks.tsonly usesget. Consider removing the unused mock for clarity.♻️ Suggested simplification
vi.mock("../../src/client.js", () => ({ get: vi.fn(), - post: vi.fn(), }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/commands/tasks.test.ts` around lines 3 - 6, The test mock declares both get and post via vi.mock but only get is used by the tested module (tasks.ts); remove the unused post mock to keep the test setup minimal and clear by updating the vi.mock call to only provide get (leave vi.fn for get intact) so tests continue to use the mocked get function and the extraneous post mock is eliminated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@__tests__/commands/tasks.test.ts`:
- Around line 3-6: The test mock declares both get and post via vi.mock but only
get is used by the tested module (tasks.ts); remove the unused post mock to keep
the test setup minimal and clear by updating the vi.mock call to only provide
get (leave vi.fn for get intact) so tests continue to use the mocked get
function and the extraneous post mock is eliminated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8146f24-4ecf-4d45-b652-074b443ca218
📒 Files selected for processing (6)
__tests__/commands/content.test.ts__tests__/commands/tasks.test.tssrc/bin.tssrc/commands/content.tssrc/commands/tasks.tssrc/getErrorMessage.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/commands/content.test.ts
- src/commands/content.ts
- parsePositiveInt.ts - templatesCommand.ts - validateCommand.ts - estimateCommand.ts - createCommand.ts - content.ts now only composes subcommands Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- statusCommand.ts extracted to tasks/ subdirectory - tasks.ts now only composes subcommands Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
2 participants
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.
What
Verification
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests