Rework one-click deploy to read persona specs#145
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new --one-click deployment feature to the CLI, allowing users to quickly deploy a persona (JSON or TS) to the cloud by parsing its requirements, resolving inputs, displaying a deployment plan, and executing the deployment. Feedback focuses on fixing a routing bug where the inline --one-click=path format is not correctly recognized, and resolving a logic issue in the plan rendering where required inputs are always shown as "none" because they are filtered out once resolved.
| if (args.includes('--one-click')) { | ||
| await runOneClickDeploy(args); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The router check args.includes('--one-click') only matches the exact string '--one-click'. If a user passes the inline format --one-click=persona.json (which is explicitly supported and parsed in parseOneClickDeployArgs), the router will fail to delegate to runOneClickDeploy and will instead fall through to the standard deploy command, resulting in an 'unknown flag' error.
Using args.some(...) to check for both formats ensures correct routing.
| if (args.includes('--one-click')) { | |
| await runOneClickDeploy(args); | |
| return; | |
| } | |
| if (args.some((arg) => arg === '--one-click' || arg.startsWith('--one-click='))) { | |
| await runOneClickDeploy(args); | |
| return; | |
| } |
| function formatOneClickPlan( | ||
| persona: PersonaSpec, | ||
| requirements: DeployRequirements, | ||
| resolvedInputs: Record<string, string> | ||
| ): string { | ||
| const unresolvedInputs = requirements.inputs.filter((input) => !hasResolvedInput(resolvedInputs[input.name])); | ||
| return [ | ||
| 'One-click deploy plan', | ||
| `Persona: ${persona.id}`, | ||
| formatPlanList('Fires on', requirements.firesOn), | ||
| formatIntegrationPlan(requirements), | ||
| formatInputPlan(unresolvedInputs), | ||
| 'Platform secrets: none required (shared platform)', | ||
| '' | ||
| ].join('\n'); | ||
| } |
There was a problem hiding this comment.
Since resolveOneClickInputs is called before formatOneClickPlan and strictly ensures all required inputs are resolved (or throws an error), unresolvedInputs will always be an empty array. As a result, formatInputPlan(unresolvedInputs) will always return 'Required inputs: none', even if the persona has required inputs that were successfully resolved.
To make the deploy plan accurate and informative, we should pass all required inputs and their resolved values to formatInputPlan so they can be listed.
function formatOneClickPlan(
persona: PersonaSpec,
requirements: DeployRequirements,
resolvedInputs: Record<string, string>
): string {
return [
'One-click deploy plan',
`Persona: ${persona.id}`,
formatPlanList('Fires on', requirements.firesOn),
formatIntegrationPlan(requirements),
formatInputPlan(requirements.inputs, resolvedInputs),
'Platform secrets: none required (shared platform)',
''
].join('\n');
}| function formatInputPlan(inputs: DeployRequirements['inputs']): string { | ||
| if (inputs.length === 0) return 'Required inputs: none'; | ||
| return [ | ||
| 'Required inputs:', | ||
| ...inputs.map((input) => { | ||
| const env = input.env ? ` env=${input.env}` : ''; | ||
| const description = input.description ? ` - ${input.description}` : ''; | ||
| return `- ${input.name}${env}${description}`; | ||
| }) | ||
| ].join('\n'); | ||
| } |
There was a problem hiding this comment.
Update formatInputPlan to accept the resolved inputs map and display the resolved values alongside the required inputs.
| function formatInputPlan(inputs: DeployRequirements['inputs']): string { | |
| if (inputs.length === 0) return 'Required inputs: none'; | |
| return [ | |
| 'Required inputs:', | |
| ...inputs.map((input) => { | |
| const env = input.env ? ` env=${input.env}` : ''; | |
| const description = input.description ? ` - ${input.description}` : ''; | |
| return `- ${input.name}${env}${description}`; | |
| }) | |
| ].join('\n'); | |
| } | |
| function formatInputPlan( | |
| inputs: DeployRequirements['inputs'], | |
| resolvedInputs: Record<string, string> | |
| ): string { | |
| if (inputs.length === 0) return 'Required inputs: none'; | |
| return [ | |
| 'Required inputs:', | |
| ...inputs.map((input) => { | |
| const val = resolvedInputs[input.name] ? `=${resolvedInputs[input.name]}` : ''; | |
| const env = input.env ? ` env=${input.env}` : ''; | |
| const description = input.description ? ` - ${input.description}` : ''; | |
| return `- ${input.name}${val}${env}${description}`; | |
| }) | |
| ].join('\n'); | |
| } |
| assert.match(trap.stdout, /One-click deploy plan/); | ||
| assert.match(trap.stdout, /Fires on:\n- github:issues\.opened\n- slack:message\.channels\n- schedule:daily/); | ||
| assert.match(trap.stdout, /Integrations:\n- github \(required\): issues\.opened\n- slack \(optional\): message\.channels/); | ||
| assert.match(trap.stdout, /Required inputs: none/); |
There was a problem hiding this comment.
Update the test assertion to match the improved plan output that lists the resolved required inputs.
| assert.match(trap.stdout, /Required inputs: none/); | |
| assert.match(trap.stdout, /Required inputs:\n- LEAD=alice env=TRIAGE_LEAD - Owner to mention/); |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
You’re at about 91% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/cli/src/deploy-command.ts">
<violation number="1" location="packages/cli/src/deploy-command.ts:86">
P1: `runDeploy` misses the `--one-click=<path>` form, so valid one-click invocations are routed to the wrong parser.</violation>
<violation number="2" location="packages/cli/src/deploy-command.ts:355">
P2: The plan is built from `unresolvedInputs`, which is empty after successful input resolution, so it misleadingly reports `Required inputs: none` for personas that do require inputs.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| process.exit(args.length === 0 ? 1 : 0); | ||
| } | ||
|
|
||
| if (args.includes('--one-click')) { |
There was a problem hiding this comment.
P1: runDeploy misses the --one-click=<path> form, so valid one-click invocations are routed to the wrong parser.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/deploy-command.ts, line 86:
<comment>`runDeploy` misses the `--one-click=<path>` form, so valid one-click invocations are routed to the wrong parser.</comment>
<file context>
@@ -70,6 +83,11 @@ export async function runDeploy(args: readonly string[]): Promise<void> {
process.exit(args.length === 0 ? 1 : 0);
}
+ if (args.includes('--one-click')) {
+ await runOneClickDeploy(args);
+ return;
</file context>
| if (args.includes('--one-click')) { | |
| if (args.some((arg) => arg === '--one-click' || arg.startsWith('--one-click='))) { |
| `Persona: ${persona.id}`, | ||
| formatPlanList('Fires on', requirements.firesOn), | ||
| formatIntegrationPlan(requirements), | ||
| formatInputPlan(unresolvedInputs), |
There was a problem hiding this comment.
P2: The plan is built from unresolvedInputs, which is empty after successful input resolution, so it misleadingly reports Required inputs: none for personas that do require inputs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/deploy-command.ts, line 355:
<comment>The plan is built from `unresolvedInputs`, which is empty after successful input resolution, so it misleadingly reports `Required inputs: none` for personas that do require inputs.</comment>
<file context>
@@ -115,6 +133,263 @@ export async function runDeploy(args: readonly string[]): Promise<void> {
+ `Persona: ${persona.id}`,
+ formatPlanList('Fires on', requirements.firesOn),
+ formatIntegrationPlan(requirements),
+ formatInputPlan(unresolvedInputs),
+ 'Platform secrets: none required (shared platform)',
+ ''
</file context>
1 participant
Summary
Validation