Skip to content

Conversation

@khaliqgant
Copy link
Collaborator

@khaliqgant khaliqgant commented Dec 20, 2025

User description

Add support for Amp (ampcode.com) AI code editor format with:

  • Skills: .agents/skills/{name}/SKILL.md with YAML frontmatter
  • Commands: .agents/commands/{name}.md markdown files
  • AGENTS.md: Plain markdown with optional globs for conditional inclusion

Implementation includes:

  • from-amp.ts and to-amp.ts converters
  • JSON schemas for validation (amp, amp-skill, amp-command)
  • Format documentation (amp.md)
  • CLI format mappings (icon: ⚡, label: Amp)
  • Webapp filter dropdown option
  • Registry route schema updates
  • 12 comprehensive tests (all passing)

Closes: app-amp


CodeAnt-AI Description

Add Amp editor format support (skills, commands, AGENTS.md) across PRPM

What Changed

  • Users can import and parse Amp SKILL.md skills, .agents/commands/*.md commands, and AGENTS.md files into the canonical editor format and install/publish Amp packages
  • Conversion tools now export canonical packages to Amp-compatible SKILL.md, command markdown, and AGENTS.md (including preservation of Amp fields like name, description, argument-hint, disable-model-invocation, and globs) and support roundtrip conversions to other formats
  • CLI and web UI include Amp as a selectable format (icons/labels and search filter), registry API validation and analytics accept Amp format, and JSON schemas + docs for Amp are included
  • Tests added to verify Amp parsing and cross-format conversions (skill and rule roundtrips and conversions to Claude/Cursor/Continue/Copilot/Kiro/Windsurf)

Impact

✅ Can convert Amp skills and commands
✅ Amp appears in search and install flows
✅ Registry accepts and validates Amp packages

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

Add support for Amp (ampcode.com) AI code editor format with:

- Skills: .agents/skills/{name}/SKILL.md with YAML frontmatter
- Commands: .agents/commands/{name}.md markdown files
- AGENTS.md: Plain markdown with optional globs for conditional inclusion

Implementation includes:
- from-amp.ts and to-amp.ts converters
- JSON schemas for validation (amp, amp-skill, amp-command)
- Format documentation (amp.md)
- CLI format mappings (icon: ⚡, label: Amp)
- Webapp filter dropdown option
- Registry route schema updates
- 12 comprehensive tests (all passing)

Closes: app-amp
… tests

- Change amp:skill validation to use agent-skills.schema.json (shared with
  Codex/Copilot) instead of separate amp-skill.schema.json
- Delete redundant amp-skill.schema.json
- Add 14 cross-format integration tests for Amp format:
  - Amp Skill → Claude/Cursor/Continue/Copilot/Kiro/Windsurf (7 tests)
  - Amp Rule → Claude/Cursor/Continue/Copilot/Kiro/Windsurf (7 tests)

All 788 converter tests pass.
@codeant-ai
Copy link

codeant-ai bot commented Dec 20, 2025

CodeAnt AI is reviewing your PR.


Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@codeant-ai codeant-ai bot added the size:XXL This PR changes 1000+ lines, ignoring generated files label Dec 20, 2025
@my-senior-dev-pr-review
Copy link

my-senior-dev-pr-review bot commented Dec 20, 2025

🤖 My Senior Dev — Analysis Complete

👤 For @khaliqgant

📁 Expert in packages/ (188 edits) • ⚡ 38th PR this month

View your contributor analytics →


📊 25 files reviewed • 5 need attention

⚠️ Needs Attention:

  • packages/cli/src/commands/install.ts — New 'amp' format implementation requires validation for correctness and integration with existing systems.

🚀 Open Interactive Review →

The full interface unlocks features not available in GitHub:

  • 💬 AI Chat — Ask questions on any file, get context-aware answers
  • 🔍 Smart Hovers — See symbol definitions and usage without leaving the diff
  • 📚 Code Archeology — Understand how files evolved over time (/archeology)
  • 🎯 Learning Insights — See how this PR compares to similar changes

💬 Chat here: @my-senior-dev explain this change — or try @chaos-monkey @security-auditor @optimizer @skeptic @junior-dev

📖 View all 12 personas & slash commands

You can interact with me by mentioning @my-senior-dev in any comment:

In PR comments or on any line of code:

  • Ask questions about the code or PR
  • Request explanations of specific changes
  • Get suggestions for improvements

Slash commands:

  • /help — Show all available commands
  • /archeology — See the history and evolution of changed files
  • /profile — Performance analysis and suggestions
  • /expertise — Find who knows this code best
  • /personas — List all available AI personas

AI Personas (mention to get their perspective):

Persona Focus
@chaos-monkey 🐵 Edge cases & failure scenarios
@skeptic 🤨 Challenge assumptions
@optimizer Performance & efficiency
@security-auditor 🔒 Security vulnerabilities
@accessibility-advocate Inclusive design
@junior-dev 🌱 Simple explanations
@tech-debt-collector 💳 Code quality & shortcuts
@ux-champion 🎨 User experience
@devops-engineer 🚀 Deployment & scaling
@documentation-nazi 📚 Documentation gaps
@legacy-whisperer 🏛️ Working with existing code
@test-driven-purist Testing & TDD

For the best experience, view this PR on myseniordev.com — includes AI chat, file annotations, and interactive reviews.

Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6 issues found across 22 files

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="packages/converters/schemas/amp.schema.json">

<violation number="1" location="packages/converters/schemas/amp.schema.json:37">
P2: Missing `&quot;additionalProperties&quot;: false` at the root schema level. The similar `agents-md.schema.json` includes this constraint to ensure strict validation. Without it, objects with unexpected root-level properties will pass validation.</violation>
</file>

<file name="packages/converters/src/__tests__/amp.test.ts">

<violation number="1" location="packages/converters/src/__tests__/amp.test.ts:62">
P2: Test could silently pass without verifying anything. If `metadataSection` is undefined or has a different type, the `if` block is skipped and the test passes. Add an assertion before the `if` block (matching the pattern used in other tests in this file).</violation>
</file>

<file name="packages/converters/src/from-amp.ts">

<violation number="1" location="packages/converters/src/from-amp.ts:48">
P2: Missing error handling for `yaml.load()`. If the YAML frontmatter is malformed, this will throw an unhandled exception. Consider wrapping in a try-catch block, similar to `from-kiro.ts`.</violation>
</file>

<file name="packages/converters/schemas/amp-command.schema.json">

<violation number="1" location="packages/converters/schemas/amp-command.schema.json:36">
P2: Missing `additionalProperties: false` at the root level. Other command schemas in this project (e.g., `cursor-command.schema.json`) include this to ensure strict validation. Without it, unexpected properties at the root level will pass validation silently.</violation>
</file>

<file name="packages/converters/src/to-amp.ts">

<violation number="1" location="packages/converters/src/to-amp.ts:161">
P2: Custom and file-reference sections are silently ignored without warnings. Other converters (e.g., `to-cursor.ts`) either convert these sections or add warnings when skipping them. Consider adding warnings for these section types to alert users about potential data loss.</violation>
</file>

<file name="packages/converters/docs/amp.md">

<violation number="1" location="packages/converters/docs/amp.md:331">
P2: Broken relative link: `../../docs/formats.mdx` does not exist. The path resolves to `packages/docs/formats.mdx` which doesn&#39;t exist. Consider linking to an existing file like `../README.md` (the format docs README) or remove this link.</violation>
</file>

Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR

"description": "Plain markdown content with project-specific instructions. Supports @-mentions for including other files (e.g., @doc/*.md)"
}
},
"examples": [
Copy link

@cubic-dev-ai cubic-dev-ai bot Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Missing "additionalProperties": false at the root schema level. The similar agents-md.schema.json includes this constraint to ensure strict validation. Without it, objects with unexpected root-level properties will pass validation.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/converters/schemas/amp.schema.json, line 37:

<comment>Missing `&quot;additionalProperties&quot;: false` at the root schema level. The similar `agents-md.schema.json` includes this constraint to ensure strict validation. Without it, objects with unexpected root-level properties will pass validation.</comment>

<file context>
@@ -0,0 +1,48 @@
+      &quot;description&quot;: &quot;Plain markdown content with project-specific instructions. Supports @-mentions for including other files (e.g., @doc/*.md)&quot;
+    }
+  },
+  &quot;examples&quot;: [
+    {
+      &quot;content&quot;: &quot;# Backend Guidelines\n\n## Architecture\n\nWe use a clean architecture pattern with dependency injection.\n\n## Database\n\nPostgreSQL with Prisma ORM.\n\n## Coding Standards\n\n- Use TypeScript strict mode\n- Write unit tests for all services\n- Follow @docs/api-conventions.md&quot;
</file context>
Fix with Cubic

expect(result.subtype).toBe('skill');
const metadataSection = result.content.sections.find(s => s.type === 'metadata');
expect(metadataSection?.type).toBe('metadata');
if (metadataSection?.type === 'metadata') {
Copy link

@cubic-dev-ai cubic-dev-ai bot Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Test could silently pass without verifying anything. If metadataSection is undefined or has a different type, the if block is skipped and the test passes. Add an assertion before the if block (matching the pattern used in other tests in this file).

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/converters/src/__tests__/amp.test.ts, line 62:

<comment>Test could silently pass without verifying anything. If `metadataSection` is undefined or has a different type, the `if` block is skipped and the test passes. Add an assertion before the `if` block (matching the pattern used in other tests in this file).</comment>

<file context>
@@ -0,0 +1,364 @@
+      expect(result.subtype).toBe(&#39;skill&#39;);
+      const metadataSection = result.content.sections.find(s =&gt; s.type === &#39;metadata&#39;);
+      expect(metadataSection?.type).toBe(&#39;metadata&#39;);
+      if (metadataSection?.type === &#39;metadata&#39;) {
+        expect(metadataSection.data.amp).toEqual({
+          argumentHint: &#39;[test file]&#39;,
</file context>
Fix with Cubic

return { frontmatter: {}, body: content };
}

const frontmatter = yaml.load(match[1]) as Record<string, any>;
Copy link

@cubic-dev-ai cubic-dev-ai bot Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Missing error handling for yaml.load(). If the YAML frontmatter is malformed, this will throw an unhandled exception. Consider wrapping in a try-catch block, similar to from-kiro.ts.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/converters/src/from-amp.ts, line 48:

<comment>Missing error handling for `yaml.load()`. If the YAML frontmatter is malformed, this will throw an unhandled exception. Consider wrapping in a try-catch block, similar to `from-kiro.ts`.</comment>

<file context>
@@ -0,0 +1,161 @@
+    return { frontmatter: {}, body: content };
+  }
+
+  const frontmatter = yaml.load(match[1]) as Record&lt;string, any&gt;;
+  const body = match[2];
+
</file context>
Fix with Cubic

},
"content": "# Generate Tests\n\nCreate comprehensive tests for the specified code:\n\n- Unit tests for all functions\n- Integration tests for API endpoints\n- Edge case coverage"
}
]
Copy link

@cubic-dev-ai cubic-dev-ai bot Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Missing additionalProperties: false at the root level. Other command schemas in this project (e.g., cursor-command.schema.json) include this to ensure strict validation. Without it, unexpected properties at the root level will pass validation silently.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/converters/schemas/amp-command.schema.json, line 36:

<comment>Missing `additionalProperties: false` at the root level. Other command schemas in this project (e.g., `cursor-command.schema.json`) include this to ensure strict validation. Without it, unexpected properties at the root level will pass validation silently.</comment>

<file context>
@@ -0,0 +1,37 @@
+      },
+      &quot;content&quot;: &quot;# Generate Tests\n\nCreate comprehensive tests for the specified code:\n\n- Unit tests for all functions\n- Integration tests for API endpoints\n- Edge case coverage&quot;
+    }
+  ]
+}
</file context>
Fix with Cubic

lines.push('');
} else if (section.type === 'hook' || section.type === 'cursor-hook') {
warnings.push(`Hook sections are not supported in Amp format`);
} else if (section.type !== 'custom' && section.type !== 'file-reference') {
Copy link

@cubic-dev-ai cubic-dev-ai bot Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Custom and file-reference sections are silently ignored without warnings. Other converters (e.g., to-cursor.ts) either convert these sections or add warnings when skipping them. Consider adding warnings for these section types to alert users about potential data loss.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/converters/src/to-amp.ts, line 161:

<comment>Custom and file-reference sections are silently ignored without warnings. Other converters (e.g., `to-cursor.ts`) either convert these sections or add warnings when skipping them. Consider adding warnings for these section types to alert users about potential data loss.</comment>

<file context>
@@ -0,0 +1,169 @@
+      lines.push(&#39;&#39;);
+    } else if (section.type === &#39;hook&#39; || section.type === &#39;cursor-hook&#39;) {
+      warnings.push(`Hook sections are not supported in Amp format`);
+    } else if (section.type !== &#39;custom&#39; &amp;&amp; section.type !== &#39;file-reference&#39;) {
+      // Handle any other section types that may be added in the future
+      const sectionType = (section as { type: string }).type;
</file context>
Fix with Cubic

- [Amp Manual](https://ampcode.com/manual)
- [Amp Skills](https://ampcode.com/manual#skills)
- [Amp Custom Commands](https://ampcode.com/manual#custom-commands)
- [PRPM Format Guide](../../docs/formats.mdx)
Copy link

@cubic-dev-ai cubic-dev-ai bot Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Broken relative link: ../../docs/formats.mdx does not exist. The path resolves to packages/docs/formats.mdx which doesn't exist. Consider linking to an existing file like ../README.md (the format docs README) or remove this link.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/converters/docs/amp.md, line 331:

<comment>Broken relative link: `../../docs/formats.mdx` does not exist. The path resolves to `packages/docs/formats.mdx` which doesn&#39;t exist. Consider linking to an existing file like `../README.md` (the format docs README) or remove this link.</comment>

<file context>
@@ -0,0 +1,339 @@
+- [Amp Manual](https://ampcode.com/manual)
+- [Amp Skills](https://ampcode.com/manual#skills)
+- [Amp Custom Commands](https://ampcode.com/manual#custom-commands)
+- [PRPM Format Guide](../../docs/formats.mdx)
+
+## Changelog
</file context>
Fix with Cubic

@codeant-ai
Copy link

codeant-ai bot commented Dec 20, 2025

Nitpicks 🔍

🔒 No security issues identified
⚡ Recommended areas for review

  • Unsafe YAML load cast
    The result of yaml.load(...) is cast directly to Record<string, any> without checking for undefined or non-object values. If YAML parsing returns null, a scalar, or throws, subsequent code that assumes an object can fail at runtime.

  • Frontmatter parsing fragility
    The regex used to extract YAML frontmatter is strict (expects LF only and exact newline placement). Files with CRLF line endings or slightly different frontmatter spacing may not be recognized, causing the parser to treat frontmatter as body. Consider making frontmatter detection more tolerant or using a small parsing helper.

  • Possible runtime error
    Several section handlers iterate unguarded properties (e.g. section.items, section.examples) assuming arrays exist. If those fields are undefined or not arrays this will throw at runtime. Add guards and emit warnings when required data is missing.

  • Format type mismatch
    The PR adds 'amp' to the local ConversionOptions.targetFormat union but the canonical package's format field uses the imported Format type from '@pr-pm/types'. If the central Format type is not updated to include 'amp', this will create a type mismatch across the codebase and may prevent canonical packages from being marked as 'amp' where expected. Verify the shared Format type is updated, or align the canonical package's type to accept 'amp'.

  • Runtime handling gap
    editorType and targetFormat unions now include 'amp'. Ensure all consumers (converters, serializers, CLI mappings, registry schemas and any switch/dispatch logic) are updated to handle 'amp' at runtime; otherwise code paths may hit unhandled cases.

import { toCopilot } from '../to-copilot.js';
import { toKiro } from '../to-kiro.js';
import { toWindsurf } from '../to-windsurf.js';
import { toAmp } from '../to-amp.js';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The toAmp symbol is imported but never used anywhere in this test file, leaving dead code that can cause linting failures and hides the fact that conversions to the Amp format are not actually exercised by tests. [possible bug]

Severity Level: Critical 🚨

Suggested change
import { toAmp } from '../to-amp.js';
Why it matters? ⭐

The test file's final state includes the import "import { toAmp } from '../to-amp.js';" but there is no usage of toAmp anywhere in the file. This is a legitimate issue: unused imports cause linter failures and indicate either dead code or missing tests that should exercise Amp conversions. Removing the import (or adding tests that actually call toAmp and related fromAmp/from-amp behavior) fixes a real problem rather than a mere stylistic nit.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** packages/converters/src/__tests__/cross-format.test.ts
**Line:** 21:21
**Comment:**
	*Possible Bug: The `toAmp` symbol is imported but never used anywhere in this test file, leaving dead code that can cause linting failures and hides the fact that conversions to the Amp format are not actually exercised by tests.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

if (normalized.includes('replit')) return 'replit';
if (normalized.includes('zed')) return 'zed';
if (normalized.includes('mcp')) return 'mcp';
if (normalized.includes('amp')) return 'amp';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Using normalized.includes('amp') to detect the Amp format is overly broad and will classify any string containing "amp" (for example "example") as the Amp format, which can incorrectly normalize unrelated identifiers or paths; this should be tightened to match only actual Amp format identifiers such as exactly "amp" or known Amp-specific prefixes. [logic error]

Severity Level: Minor ⚠️

Suggested change
if (normalized.includes('amp')) return 'amp';
if (normalized === 'amp' || normalized.startsWith('amp:') || normalized.startsWith('amp-')) return 'amp';
Why it matters? ⭐

The current normalizeFormat implementation uses includes() for many short identifiers. For 'amp' that is risky because many unrelated strings (e.g., "example", "champ", file paths) contain the substring "amp" and would be mis-normalized to 'amp'. Tightening the match to exact equals or known prefixes (amp:, amp-) reduces false positives. This is a legitimate logic-improvement suggestion to avoid incorrect normalization.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** packages/converters/src/taxonomy-utils.ts
**Line:** 114:114
**Comment:**
	*Logic Error: Using `normalized.includes('amp')` to detect the Amp format is overly broad and will classify any string containing "amp" (for example "example") as the Amp format, which can incorrectly normalize unrelated identifiers or paths; this should be tightened to match only actual Amp format identifiers such as exactly "amp" or known Amp-specific prefixes.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

homepage: z.string().url().optional(),
documentation: z.string().url().optional(),
format: z.enum(['cursor', 'claude', 'claude-plugin', 'continue', 'windsurf', 'copilot', 'kiro', 'agents.md', 'generic', 'mcp']),
format: z.enum(['cursor', 'claude', 'claude-plugin', 'continue', 'windsurf', 'copilot', 'kiro', 'agents.md', 'gemini', 'ruler', 'droid', 'opencode', 'codex', 'amp', 'trae', 'aider', 'zencoder', 'replit', 'generic', 'mcp']),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The format enum used for manifest validation does not match the canonical Format type (packages/types/src/package.ts): it omits supported formats like gemini.md, claude.md, gemini-extension, and zed, and includes claude-plugin which is not part of Format. This mismatch means manifests using officially supported formats will be rejected by the registry while an unsupported value may be accepted, creating schema/contract inconsistencies across the system. [logic error]

Severity Level: Minor ⚠️

Suggested change
format: z.enum(['cursor', 'claude', 'claude-plugin', 'continue', 'windsurf', 'copilot', 'kiro', 'agents.md', 'gemini', 'ruler', 'droid', 'opencode', 'codex', 'amp', 'trae', 'aider', 'zencoder', 'replit', 'generic', 'mcp']),
format: z.enum(['cursor', 'claude', 'continue', 'windsurf', 'copilot', 'kiro', 'agents.md', 'gemini.md', 'claude.md', 'gemini', 'gemini-extension', 'opencode', 'ruler', 'droid', 'trae', 'aider', 'zencoder', 'replit', 'zed', 'codex', 'amp', 'generic', 'mcp']),
Why it matters? ⭐

The suggestion highlights a real contract mismatch risk: if the canonical Format type (in packages/types) contains values not present here (e.g., gemini.md, claude.md, gemini-extension, zed) then valid manifests using those values will be rejected by this schema; conversely, keeping 'claude-plugin' here when it's not in the canonical type allows an unsupported value through. That is a functional correctness/compatibility issue, not just style. The proper fix is to align this z.enum with the central Format definition (or import/derive it), ensuring the registry accepts exactly the supported formats and rejects unsupported ones.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** packages/registry/src/validation/package.ts
**Line:** 34:34
**Comment:**
	*Logic Error: The `format` enum used for manifest validation does not match the canonical `Format` type (`packages/types/src/package.ts`): it omits supported formats like `gemini.md`, `claude.md`, `gemini-extension`, and `zed`, and includes `claude-plugin` which is not part of `Format`. This mismatch means manifests using officially supported formats will be rejected by the registry while an unsupported value may be accepted, creating schema/contract inconsistencies across the system.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

const frontmatter = yaml.load(match[1]) as Record<string, any>;
const body = match[2];

return { frontmatter, body };
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: If the YAML frontmatter block is empty or parses to a non-object value (e.g. ---\n---), yaml.load returns undefined or a scalar, and the subsequent detectSubtype(frontmatter) call will try to access properties like frontmatter.name, causing a runtime "Cannot read properties of undefined/null" error during conversion instead of safely treating it as "no frontmatter". [null pointer]

Severity Level: Minor ⚠️

Suggested change
return { frontmatter, body };
const raw = yaml.load(match[1]);
const frontmatter = raw && typeof raw === 'object' ? (raw as Record<string, any>) : {};
Why it matters? ⭐

yaml.load can return undefined/null or a scalar for empty or non-object frontmatter (e.g. ---\n---).
The current code casts that result to a Record and returns it; later code calls detectSubtype(frontmatter)
which will attempt to read properties like .name and throw if frontmatter is not an object.
Normalizing non-object parse results to an empty object prevents a runtime TypeError and safely treats the file
as having no frontmatter.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** packages/converters/src/from-amp.ts
**Line:** 51:51
**Comment:**
	*Null Pointer: If the YAML frontmatter block is empty or parses to a non-object value (e.g. `---\n---`), `yaml.load` returns `undefined` or a scalar, and the subsequent `detectSubtype(frontmatter)` call will try to access properties like `frontmatter.name`, causing a runtime "Cannot read properties of undefined/null" error during conversion instead of safely treating it as "no frontmatter".

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

Comment on lines +29 to +32
// Check for lossy conversion
const lossyConversion = warnings.some(w =>
w.includes('not supported') || w.includes('skipped')
);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The lossyConversion flag only checks for warnings containing the substrings not supported or skipped, so cases like the fallback for a missing required description emit a warning but do not mark the conversion as lossy or lower the quality score, misleading callers about the fidelity of the conversion; treating any warning as an indicator of potential loss fixes this. [logic error]

Severity Level: Minor ⚠️

Suggested change
// Check for lossy conversion
const lossyConversion = warnings.some(w =>
w.includes('not supported') || w.includes('skipped')
);
// Check for lossy conversion - any warning indicates potential loss of fidelity
const lossyConversion = warnings.length > 0;
Why it matters? ⭐

The current heuristic only treats specific substrings as lossy. But the code emits other warnings (for example when falling back to package description for a missing required description) which indicate diminished fidelity but won't flip lossyConversion or reduce qualityScore. Treating any warning as potential loss is a safer, less surprising behavior for callers expecting fidelity info.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** packages/converters/src/to-amp.ts
**Line:** 29:32
**Comment:**
	*Logic Error: The `lossyConversion` flag only checks for warnings containing the substrings `not supported` or `skipped`, so cases like the fallback for a missing required description emit a warning but do not mark the conversion as lossy or lower the quality score, misleading callers about the fidelity of the conversion; treating any warning as an indicator of potential loss fixes this.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

Comment on lines +161 to +164
} else if (section.type !== 'custom' && section.type !== 'file-reference') {
// Handle any other section types that may be added in the future
const sectionType = (section as { type: string }).type;
warnings.push(`Section type '${sectionType}' may not be fully supported in Amp format`);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Custom and file-reference sections are dropped without any warning or indication, so content from these sections disappears in the Amp output while lossyConversion may remain false; emitting an explicit warning that they are not supported and were skipped makes the data loss visible and allows callers to detect a lossy conversion. [logic error]

Severity Level: Minor ⚠️

Suggested change
} else if (section.type !== 'custom' && section.type !== 'file-reference') {
// Handle any other section types that may be added in the future
const sectionType = (section as { type: string }).type;
warnings.push(`Section type '${sectionType}' may not be fully supported in Amp format`);
} else {
// Handle any other section types (including custom/file-reference) that are not rendered
const sectionType = (section as { type: string }).type;
warnings.push(`Section type '${sectionType}' is not supported in Amp format and was skipped`);
Why it matters? ⭐

Right now custom and file-reference sections fall through (no warning), so their content is dropped silently. The improved code surfaces an explicit warning for any unsupported section (including custom and file-reference) and clarifies that it was skipped. That's a real bugfix for visibility of data loss and aligns with the goal of reporting lossy conversions.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** packages/converters/src/to-amp.ts
**Line:** 161:164
**Comment:**
	*Logic Error: Custom and file-reference sections are dropped without any warning or indication, so content from these sections disappears in the Amp output while `lossyConversion` may remain false; emitting an explicit warning that they are not supported and were skipped makes the data loss visible and allows callers to detect a lossy conversion.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

@codeant-ai
Copy link

codeant-ai bot commented Dec 20, 2025

CodeAnt AI finished reviewing your PR.

@codeant-ai
Copy link

codeant-ai bot commented Jan 10, 2026

CodeAnt AI is running Incremental review


Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 issues found across 82 files (changes from recent commits).

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="packages/registry/migrations/068_add_snippet_subtype.sql">

<violation number="1">
P2: Drop+add of the check constraint is split across two statements, which can leave a brief window without validation (and can cause `ADD CONSTRAINT` to fail if concurrent writes insert invalid subtypes). Prefer doing this in a single `ALTER TABLE ... DROP CONSTRAINT ..., ADD CONSTRAINT ...` statement (or ensure the migration is explicitly transactional).</violation>
</file>

<file name=".claude/skills/creating-claude-commands/SKILL.md">

<violation number="1">
P2: The commit message format `$1: $ARGUMENTS` will duplicate the type argument since `$ARGUMENTS` includes all arguments (including `$1`). For invocation `/git.quick-commit feat "add user auth"`, this produces `feat: feat add user auth`. Consider using `$1: $2` instead for the expected conventional commit format.</violation>
</file>

<file name=".github/workflows/deploy.yml">

<violation number="1">
P2: CloudFront invalidation truncates to 15 paths even when up to 50 changed files are tracked. If 30 files change, only the first 15 would be invalidated, potentially serving stale content for the remaining files. Consider increasing this limit to match the 50-file threshold, or use wildcard patterns when >15 files change.</violation>
</file>

<file name=".claude/commands/create-slash-command.md">

<violation number="1">
P2: Malformed markdown for inline code containing backticks. This will render incorrectly, potentially confusing users about the proper bash execution syntax. To show backticks inside inline code, use double backticks with spaces: ``` `` !`command` `` ```</violation>
</file>

<file name="packages/cli/src/core/lockfile.ts">

<violation number="1">
P1: Snippet lockfile entries are now keyed by `#format:location`, but some consumers still look up `#format` only (e.g., integrity verification), so snippets can’t be found/verified consistently. Update all lockfile key lookups to pass the same location (or add a fallback scan when location isn’t provided).</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

@khaliqgant khaliqgant merged commit a5822de into main Jan 10, 2026
12 checks passed
@khaliqgant khaliqgant deleted the claude/review-beads-tasks-W6cYo branch January 10, 2026 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL This PR changes 1000+ lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants