test pr #61
Open
test pr #61
Conversation
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
|
Skipped: This PR changes more files than the configured file change limit: ( |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 256 files
Confidence score: 3/5
- Validation schemas in
atmn/source/core/config.tsandatmn/source/compose/models/featureModels.tsreject valid feature/plan objects (missingtype/consumableandauto_enable), which can cause config loads or exports to fail for legitimate definitions. - Change detection in
atmn/src/commands/push/headless.tsskips feature/plan updates, risking “Already in sync” even when non-versioning updates are pending. - Environment switching in
atmn/source/commands/pull.tsis ineffective and the new try/catch won’t suppress failures because downstream calls exit the process, so cross-env pulls can still fail unexpectedly. - Pay close attention to
atmn/source/core/config.ts,atmn/source/compose/models/featureModels.ts,atmn/src/commands/push/headless.ts,atmn/source/commands/pull.ts- schema validation, change detection, and pull behavior issues.
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="atmn/source/core/config.ts">
<violation number="1" location="atmn/source/core/config.ts:86">
P1: `PlanSchema.strict()` rejects valid plans because `auto_enable` is added by the plan builder but not present in the schema. This makes `isPlan` return false and causes valid plan exports to fail validation. Use non-strict parsing (or a passthrough) so legitimate plan fields aren’t rejected.</violation>
<violation number="2" location="atmn/source/core/config.ts:95">
P1: `FeatureSchema.strict()` will reject valid feature objects because the schema doesn’t include `type`/`consumable` even though those fields are required by the Feature type. This makes `isFeature` return false and valid feature exports fail validation. Use non-strict parsing (or passthrough) to allow the expected fields.</violation>
</file>
<file name="atmn/src/commands/push/headless.ts">
<violation number="1" location="atmn/src/commands/push/headless.ts:461">
P2: `hasChanges` ignores feature/plan updates, so non-versioning updates will be skipped and reported as "Already in sync." Include `featuresToUpdate` and `plansToUpdate` in the change detection.</violation>
</file>
<file name="atmn/source/commands/pull.ts">
<violation number="1" location="atmn/source/commands/pull.ts:104">
P2: Mutating process.argv won’t switch environments because isProdFlag/readFromEnv rely on cliContext, so the second fetch still hits the current environment and never pulls the other one.</violation>
<violation number="2" location="atmn/source/commands/pull.ts:112">
P2: The new try/catch won’t actually “silently fail” because `getFeatures`/`getAllPlans` exit the process on error (they call `externalRequest` with `throwOnError = false`). If the other environment is unavailable or the key is invalid, the CLI still terminates. Consider using a request path that throws so the catch can handle it.</violation>
</file>
<file name="atmn/source/compose/models/featureModels.ts">
<violation number="1" location="atmn/source/compose/models/featureModels.ts:11">
P1: FeatureSchema omits the required `type` (and commonly used `consumable`) fields, but config loading validates features with `FeatureSchema.strict()`. As a result, valid feature definitions that include `type`/`consumable` are rejected. Add these fields (ideally via a discriminated union) so strict parsing accepts real feature objects.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| function isFeature(value: any): value is Feature { | ||
| try { | ||
| FeatureSchema.parse(value); | ||
| FeatureSchema.strict().parse(value); |
There was a problem hiding this comment.
P1: FeatureSchema.strict() will reject valid feature objects because the schema doesn’t include type/consumable even though those fields are required by the Feature type. This makes isFeature return false and valid feature exports fail validation. Use non-strict parsing (or passthrough) to allow the expected fields.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At atmn/source/core/config.ts, line 95:
<comment>`FeatureSchema.strict()` will reject valid feature objects because the schema doesn’t include `type`/`consumable` even though those fields are required by the Feature type. This makes `isFeature` return false and valid feature exports fail validation. Use non-strict parsing (or passthrough) to allow the expected fields.</comment>
<file context>
@@ -89,17 +92,17 @@ function isProduct(value: any): value is Product {
function isFeature(value: any): value is Feature {
try {
- FeatureSchema.parse(value);
+ FeatureSchema.strict().parse(value);
return true;
} catch {
</file context>
Suggested change
| FeatureSchema.strict().parse(value); | |
| FeatureSchema.parse(value); |
| function isPlan(value: any): value is Plan { | ||
| try { | ||
| ProductSchema.parse(value); | ||
| PlanSchema.strict().parse(value); |
There was a problem hiding this comment.
P1: PlanSchema.strict() rejects valid plans because auto_enable is added by the plan builder but not present in the schema. This makes isPlan return false and causes valid plan exports to fail validation. Use non-strict parsing (or a passthrough) so legitimate plan fields aren’t rejected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At atmn/source/core/config.ts, line 86:
<comment>`PlanSchema.strict()` rejects valid plans because `auto_enable` is added by the plan builder but not present in the schema. This makes `isPlan` return false and causes valid plan exports to fail validation. Use non-strict parsing (or a passthrough) so legitimate plan fields aren’t rejected.</comment>
<file context>
@@ -78,9 +81,9 @@ async function installAtmn(): Promise<boolean> {
+function isPlan(value: any): value is Plan {
try {
- ProductSchema.parse(value);
+ PlanSchema.strict().parse(value);
return true;
} catch (error) {
</file context>
Suggested change
| PlanSchema.strict().parse(value); | |
| PlanSchema.parse(value); |
| export const FeatureSchema = z.object({ | ||
| id: z.string(), | ||
| name: z.string(), | ||
| event_names: z.array(z.string()).optional(), |
There was a problem hiding this comment.
P1: FeatureSchema omits the required type (and commonly used consumable) fields, but config loading validates features with FeatureSchema.strict(). As a result, valid feature definitions that include type/consumable are rejected. Add these fields (ideally via a discriminated union) so strict parsing accepts real feature objects.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At atmn/source/compose/models/featureModels.ts, line 11:
<comment>FeatureSchema omits the required `type` (and commonly used `consumable`) fields, but config loading validates features with `FeatureSchema.strict()`. As a result, valid feature definitions that include `type`/`consumable` are rejected. Add these fields (ideally via a discriminated union) so strict parsing accepts real feature objects.</comment>
<file context>
@@ -0,0 +1,71 @@
+export const FeatureSchema = z.object({
+ id: z.string(),
+ name: z.string(),
+ event_names: z.array(z.string()).optional(),
+ credit_schema: z
+ .array(
</file context>
|
|
||
| // Check if there are any changes | ||
| const hasVersioningPlans = analysis.plansToUpdate.some((p) => p.willVersion); | ||
| const hasChanges = |
There was a problem hiding this comment.
P2: hasChanges ignores feature/plan updates, so non-versioning updates will be skipped and reported as "Already in sync." Include featuresToUpdate and plansToUpdate in the change detection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At atmn/src/commands/push/headless.ts, line 461:
<comment>`hasChanges` ignores feature/plan updates, so non-versioning updates will be skipped and reported as "Already in sync." Include `featuresToUpdate` and `plansToUpdate` in the change detection.</comment>
<file context>
@@ -0,0 +1,567 @@
+
+ // Check if there are any changes
+ const hasVersioningPlans = analysis.plansToUpdate.some((p) => p.willVersion);
+ const hasChanges =
+ analysis.featuresToCreate.length > 0 ||
+ analysis.featuresToDelete.length > 0 ||
</file context>
| } | ||
|
|
||
| try { | ||
| const otherFeatures = await getFeatures({ includeArchived: true }); |
There was a problem hiding this comment.
P2: The new try/catch won’t actually “silently fail” because getFeatures/getAllPlans exit the process on error (they call externalRequest with throwOnError = false). If the other environment is unavailable or the key is invalid, the CLI still terminates. Consider using a request path that throws so the catch can handle it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At atmn/source/commands/pull.ts, line 112:
<comment>The new try/catch won’t actually “silently fail” because `getFeatures`/`getAllPlans` exit the process on error (they call `externalRequest` with `throwOnError = false`). If the other environment is unavailable or the key is invalid, the CLI still terminates. Consider using a request path that throws so the catch can handle it.</comment>
<file context>
@@ -24,18 +28,184 @@ export default async function Pull(options?: {archived?: boolean}) {
+ }
+
+ try {
+ const otherFeatures = await getFeatures({ includeArchived: true });
+ otherFeatures.forEach((f: Feature) => {
+ if (!seenIds.has(f.id)) {
</file context>
| // Temporarily switch environment | ||
| const originalArgs = [...process.argv]; | ||
| if (currentEnvIsProd) { | ||
| process.argv = process.argv.filter( |
There was a problem hiding this comment.
P2: Mutating process.argv won’t switch environments because isProdFlag/readFromEnv rely on cliContext, so the second fetch still hits the current environment and never pulls the other one.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At atmn/source/commands/pull.ts, line 104:
<comment>Mutating process.argv won’t switch environments because isProdFlag/readFromEnv rely on cliContext, so the second fetch still hits the current environment and never pulls the other one.</comment>
<file context>
@@ -24,18 +28,184 @@ export default async function Pull(options?: {archived?: boolean}) {
+ // Temporarily switch environment
+ const originalArgs = [...process.argv];
+ if (currentEnvIsProd) {
+ process.argv = process.argv.filter(
+ (a) => a !== "--prod" && a !== "-p",
+ );
</file context>
There was a problem hiding this comment.
5 issues found across 324 files
Confidence score: 3/5
- Several 6–7/10 issues suggest real user-facing impact: unescaped single quotes in
plan.name,plan.group, andfeature.namecan generate invalid JS syntax when names contain apostrophes. atmn/src/commands/push/headless.tshas logic that may report 'Already in sync' and skip pushing some plan/feature updates, which could lead to missed changes.- Default URLs in
atmn/src/constants.tspoint to dev hosts despite production messaging, so users may unintentionally hit the wrong environment. - Pay close attention to
atmn/source/core/builders/planBuilder.ts,atmn/source/core/builders/featureBuilder.ts,atmn/src/commands/push/headless.ts,atmn/src/constants.ts- escaping and sync/defaults correctness.
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="atmn/source/core/builders/planBuilder.ts">
<violation number="1" location="atmn/source/core/builders/planBuilder.ts:53">
P2: Missing single quote escaping for `plan.group`. Unlike `plan.description` which is escaped, `plan.group` is not, which will cause syntax errors if the group name contains apostrophes.</violation>
<violation number="2" location="atmn/source/core/builders/planBuilder.ts:68">
P1: Missing single quote escaping for `plan.name`. If a plan name contains an apostrophe (e.g., "Pro's Plan"), the generated code will have a syntax error. Note that `plan.description` on line 48 IS escaped - this should be consistent.</violation>
</file>
<file name="atmn/source/core/builders/featureBuilder.ts">
<violation number="1" location="atmn/source/core/builders/featureBuilder.ts:59">
P1: Single quotes in `feature.name` are not escaped, which will generate invalid JavaScript syntax. For example, a name like "John's Feature" would produce `name: 'John's Feature',` breaking the generated code.
Escape single quotes before interpolation to prevent syntax errors in generated files.</violation>
</file>
<file name="atmn/src/commands/push/headless.ts">
<violation number="1" location="atmn/src/commands/push/headless.ts:460">
P1: Bug: `hasChanges` logic may incorrectly return 'Already in sync' when there are plan updates without versioning or feature updates. The condition only checks `hasVersioningPlans` but `executeCleanPush` pushes all `plansToUpdate`. This could cause legitimate changes to be silently skipped.</violation>
</file>
<file name="atmn/src/constants.ts">
<violation number="1" location="atmn/src/constants.ts:2">
P2: Default URLs point to dev hosts while the CLI advertises production defaults, so normal runs will hit the dev environment unless the user overrides them. Update the default constants to the production domains and keep the dev hosts for explicit local/dev flags.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const snippet = ` | ||
| export const ${idToVar({ id: plan.id, prefix: "plan" })} = plan({ | ||
| id: '${plan.id}', | ||
| name: '${plan.name}',${descriptionStr}${groupStr}${addOnStr}${autoEnableStr}${priceStr} |
There was a problem hiding this comment.
P1: Missing single quote escaping for plan.name. If a plan name contains an apostrophe (e.g., "Pro's Plan"), the generated code will have a syntax error. Note that plan.description on line 48 IS escaped - this should be consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At atmn/source/core/builders/planBuilder.ts, line 68:
<comment>Missing single quote escaping for `plan.name`. If a plan name contains an apostrophe (e.g., "Pro's Plan"), the generated code will have a syntax error. Note that `plan.description` on line 48 IS escaped - this should be consistent.</comment>
<file context>
@@ -0,0 +1,255 @@
+ const snippet = `
+export const ${idToVar({ id: plan.id, prefix: "plan" })} = plan({
+ id: '${plan.id}',
+ name: '${plan.name}',${descriptionStr}${groupStr}${addOnStr}${autoEnableStr}${priceStr}
+ features: [
+ ${planFeaturesStr}
</file context>
| } | ||
|
|
||
| export function featureBuilder(feature: ApiFeature) { | ||
| const nameStr = notNullish(feature.name) ? `\n name: '${feature.name}',` : ''; |
There was a problem hiding this comment.
P1: Single quotes in feature.name are not escaped, which will generate invalid JavaScript syntax. For example, a name like "John's Feature" would produce name: 'John's Feature', breaking the generated code.
Escape single quotes before interpolation to prevent syntax errors in generated files.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At atmn/source/core/builders/featureBuilder.ts, line 59:
<comment>Single quotes in `feature.name` are not escaped, which will generate invalid JavaScript syntax. For example, a name like "John's Feature" would produce `name: 'John's Feature',` breaking the generated code.
Escape single quotes before interpolation to prevent syntax errors in generated files.</comment>
<file context>
@@ -14,17 +26,47 @@ const creditSchemaBuilder = (feature: Feature) => {
+}
+
+export function featureBuilder(feature: ApiFeature) {
+ const nameStr = notNullish(feature.name) ? `\n name: '${feature.name}',` : '';
+ const creditSchemaStr = creditSchemaBuilder(feature);
+ const {type, consumable} = getTypeAndConsumable(feature.type);
</file context>
| const analysis = await analyzePush(config.features, config.plans); | ||
|
|
||
| // Check if there are any changes | ||
| const hasVersioningPlans = analysis.plansToUpdate.some((p) => p.willVersion); |
There was a problem hiding this comment.
P1: Bug: hasChanges logic may incorrectly return 'Already in sync' when there are plan updates without versioning or feature updates. The condition only checks hasVersioningPlans but executeCleanPush pushes all plansToUpdate. This could cause legitimate changes to be silently skipped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At atmn/src/commands/push/headless.ts, line 460:
<comment>Bug: `hasChanges` logic may incorrectly return 'Already in sync' when there are plan updates without versioning or feature updates. The condition only checks `hasVersioningPlans` but `executeCleanPush` pushes all `plansToUpdate`. This could cause legitimate changes to be silently skipped.</comment>
<file context>
@@ -0,0 +1,567 @@
+ const analysis = await analyzePush(config.features, config.plans);
+
+ // Check if there are any changes
+ const hasVersioningPlans = analysis.plansToUpdate.some((p) => p.willVersion);
+ const hasChanges =
+ analysis.featuresToCreate.length > 0 ||
</file context>
|
|
||
| const groupStr = | ||
| plan.group && plan.group !== "" && plan.group !== null | ||
| ? `\n group: '${plan.group}',` |
There was a problem hiding this comment.
P2: Missing single quote escaping for plan.group. Unlike plan.description which is escaped, plan.group is not, which will cause syntax errors if the group name contains apostrophes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At atmn/source/core/builders/planBuilder.ts, line 53:
<comment>Missing single quote escaping for `plan.group`. Unlike `plan.description` which is escaped, `plan.group` is not, which will cause syntax errors if the group name contains apostrophes.</comment>
<file context>
@@ -0,0 +1,255 @@
+
+ const groupStr =
+ plan.group && plan.group !== "" && plan.group !== null
+ ? `\n group: '${plan.group}',`
+ : "";
+
</file context>
Comment on lines
+2
to
+3
| export const FRONTEND_URL = "https://dev.useautumn.com"; | ||
| export const BACKEND_URL = "https://api-dev.useautumn.com"; |
There was a problem hiding this comment.
P2: Default URLs point to dev hosts while the CLI advertises production defaults, so normal runs will hit the dev environment unless the user overrides them. Update the default constants to the production domains and keep the dev hosts for explicit local/dev flags.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At atmn/src/constants.ts, line 2:
<comment>Default URLs point to dev hosts while the CLI advertises production defaults, so normal runs will hit the dev environment unless the user overrides them. Update the default constants to the production domains and keep the dev hosts for explicit local/dev flags.</comment>
<file context>
@@ -0,0 +1,60 @@
+// Production URLs (default)
+export const FRONTEND_URL = "https://dev.useautumn.com";
+export const BACKEND_URL = "https://api-dev.useautumn.com";
+
</file context>
Suggested change
| export const FRONTEND_URL = "https://dev.useautumn.com"; | |
| export const BACKEND_URL = "https://api-dev.useautumn.com"; | |
| export const FRONTEND_URL = "https://app.useautumn.com"; | |
| export const BACKEND_URL = "https://api.useautumn.com"; |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 by cubic
ATMN v2 CLI: switch from products to plans, add OAuth login, new pull/push flows (UI + headless), SDK typegen, and a new src-based architecture for future features.
New Features
Migration
Written for commit 9d1e6da. Summary will update on new commits.