feat(generator): add AI-powered skill generator with multi-provider support#177
Open
charantejguniganti wants to merge 8 commits into
Open
feat(generator): add AI-powered skill generator with multi-provider support#177charantejguniganti wants to merge 8 commits into
charantejguniganti wants to merge 8 commits into
Conversation
- Add src/retry.ts: withRetry() wrapper with exponential backoff + jitter, configurable maxRetries, per-error isRetryable predicate, full state tracking (Pending/Running/Retrying/Success/Failed), structured stderr logging - Add src/retry.test.ts: 13 unit tests — happy path, partial success, exhausted retries, non-retryable 404 fast-fail, timeout handling, invalid input guard - Update src/index.ts: wrap fetchGitHubReadme and fetchRepoStructure with withRetry + isNetworkError predicate; bump version 1.0.0 → 1.1.0 - Add RETRY_DESIGN.md: architecture diagram, state table, config reference, edge case matrix, usage examples No breaking changes to existing MCP tool APIs.
…-docs-mcp - package.json: ESM module, correct scripts (build/start/dev/test), deps - tsconfig.json: NodeNext module resolution, strict mode, types=[node] - .gitignore: exclude node_modules and dist from version control - tsc --noEmit passes with zero errors
…and quality scorer
Author
|
Hey! This PR introduces an AI powered skill generator for AMRIT that can scaffold complete skills from a natural language description. It supports both Anthropic and local Ollama models, includes validation, quality scoring, and generates all the required skill artifacts automatically. Would love to hear your thoughts and any suggestions for improvement. Thanks! |
|
Need an answer fast? Review this PR in Change Stack to ask focused questions about the PR or a changed range. 📝 WalkthroughWalkthroughThis PR introduces two major framework components for AMRIT: a retry-powered MCP server for documentation access, and an AI-driven CLI for generating and managing skills. The docs server wraps GitHub helpers in configurable retry logic, while the skill generator orchestrates a 5-step AI pipeline, renders templates, validates artifacts, and provides quality scoring. ChangesAMRIT Docs MCP Server with Retry Engine
AI-Powered Skill Generator CLI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (1)
skill-generator/tests/security.test.ts (1)
24-27: ⚡ Quick winStrengthen the safe-path assertion to exact match.
Using a substring match can hide incorrect resolutions. Assert against the exact expected absolute path.
Suggested test update
it('should resolve a safe relative path inside the base directory', () => { const resolved = safeResolvePath(base, './skills/test-skill'); - expect(resolved).toContain(path.join('skills', 'test-skill')); + expect(resolved).toBe(path.resolve(base, './skills/test-skill')); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skill-generator/tests/security.test.ts` around lines 24 - 27, The test currently uses a substring check; replace that with an exact path equality assertion by computing the expected absolute path using path.resolve or path.join with the test's base and the relative segment and asserting resolved === expected; specifically update the 'should resolve a safe relative path inside the base directory' test to call safeResolvePath(base, './skills/test-skill'), build expected via path.resolve(base, 'skills', 'test-skill') (or path.join+path.resolve) and assert strict equality between resolved and expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/ai-framework/mcp-servers/amrit-docs-mcp/RETRY_DESIGN.md`:
- Line 24: The markdown contains unlabeled fenced code blocks that trigger
MD040; locate the three fences that wrap the snippets "MCP Tool Handler", "delay
= min(baseDelayMs × 2^attempt, maxDelayMs) + (jitter ? random(0, baseDelayMs) :
0)" and "running retry engine tests…" and add a language identifier (e.g.,
```text) to each opening fence so they become labeled code blocks.
- Around line 87-88: The docs claim "13 unit tests" but the actual test suite in
retry.test.ts contains 11 tests; update the RETRY_DESIGN.md entry that lists
`src/retry.test.ts` so the test count reflects the current suite (change "13
unit tests" to "11 unit tests" or adjust to the true current number), and ensure
the related changelog note for `src/index.ts` remains accurate; verify the
number by running the test file (retry.test.ts) before committing.
In `@docs/ai-framework/mcp-servers/amrit-docs-mcp/src/index.ts`:
- Around line 178-181: The catch-all in the README branch probing loop swallows
all errors; change the catch to accept the error (e.g., catch (err)) and only
continue when the error indicates a branch-not-found (HTTP 404) — otherwise
rethrow the error so transient/network/API failures bubble up; check err.status
/ err.statusCode or err.response?.status === 404 in the catch before continuing
in the loop.
In `@docs/ai-framework/mcp-servers/amrit-docs-mcp/src/retry.ts`:
- Around line 229-231: The backoff computation is using the zero-based loop
index `attempt` which causes the first retry to use baseDelayMs rather than
baseDelayMs * 2^1; update the call site in retry logic (where
`computeBackoff(attempt, cfg)` is invoked) to pass the 1-based attempt number
(e.g., `attempt + 1`) or else adjust `computeBackoff` to internally add 1 to the
exponent; ensure the log call that references `delayMs` remains consistent with
the corrected computation.
In `@skill-generator/PR_DESCRIPTION.md`:
- Line 13: Update the feature bullet text that currently reads
"Anthropic-Powered Generation" to reflect multi-provider support and provider
selection: mention both Anthropic and Ollama and that the CLI can choose the
provider (e.g., "Multi-provider Generation (Anthropic & Ollama) with provider
selection") so the PR_DESCRIPTION.md entry aligns with the implemented CLI
behavior.
In `@skill-generator/skill-generation-architecture.md`:
- Line 8: Update the architecture diagram node labeled "AI Client - Anthropic
Claude" and the surrounding responsibility text to reflect the multi-provider
implementation: replace the Anthropic-only node (e.g., the line "B --> C[AI
Client - Anthropic Claude]") with a neutral provider-selection node (e.g., "AI
Client - Provider Selector") and add branches/nodes for both "Anthropic
(Claude)" and "Ollama" backends; also adjust the descriptive text that
references the AI Client to explain dynamic provider selection and that both
Anthropic and Ollama are supported for model invocation.
In `@skill-generator/src/ai/client.ts`:
- Around line 124-127: The code currently logs raw model output with logger.info
using the raw variable and raw.slice(...) which can leak user-provided content;
change those logger.info calls to a lower/diagnostic level (e.g., logger.debug
or logger.trace) or redact/sanitize the preview (avoid printing full raw or
sensitive snippets) and keep only non-sensitive metadata (length) if needed;
apply the same change to the other occurrences referenced (the logger.info calls
around raw.slice at the later block). Ensure you update both places that call
logger.info with raw/raw.slice to use logger.debug/trace or a redacted preview
instead.
- Around line 102-115: The fetch call that posts to
'http://localhost:11434/api/generate' (creating the `response`) needs a request
timeout to avoid hanging; wrap the request in an AbortController, pass
controller.signal to fetch, start a setTimeout that calls controller.abort()
after a reasonable timeout (e.g., 30_000 ms), and clear the timeout when the
fetch resolves; also catch the abort/DOMException and rethrow or return a clear
timeout error so callers of this client function (the code that uses `model`,
`fullPrompt`, and the `response` variable) can handle it.
- Around line 468-472: The fallback test set currently returns only two items
which violates the downstream validator (validateTestsYaml); update the fallback
return in skill-generator/src/ai/client.ts so it returns at least five test
objects (e.g., keep 'smoke-test' and 'empty-input' and add three more distinct
fallback tests covering common cases such as 'missing-required-field',
'invalid-type', and 'large-input' or similar) so the array meets the minimum
count expected by validateTestsYaml; ensure each test object uses the same shape
({ name, input, expected }) as the existing entries.
- Around line 308-329: The generateMetadata method currently returns the parsed
JSON from robustJsonParse without runtime shape checks which can break
downstream uses like metadata.tools.join; after calling robustJsonParse in
generateMetadata, perform an explicit runtime validation of the SkillMetadata
shape (existence and types of name, friendlyName, description, version,
category, tags (array length 3-5), owner, tools (array of strings), mcp) and
enforce constraints (e.g., name regex, description minimum words, version equals
"1.0.0"); if validation fails either throw a clear error or attempt a
retry/regeneration, otherwise return the validated SkillMetadata object so later
callers (e.g., wherever metadata.tools.join is used) can rely on the shape.
In `@skill-generator/src/cli/command.ts`:
- Around line 33-34: The --provider option currently accepts any string, which
can misroute to Anthropic and cause runtime errors; update the CLI option
declaration (the .option('--provider <provider>') in the command setup) to
validate/limit values at parse time to only 'ollama' or 'anthropic' (use
Commander choice/validation or a custom parser) and produce a clear error
message when an invalid provider is given; ensure this change aligns with
AIClient.create() expectations and prevents passing arbitrary provider strings
to AnthropicProvider/AIClient.create().
In `@skill-generator/src/index.ts`:
- Around line 75-77: The code currently uses
yaml.load(fs.readFileSync(skillYamlPath, 'utf8')) as any to build skillMetadata
and then calls aiClient.generateSkill(...) which allows malformed manifests to
flow into prompt generation; instead, read and parse the YAML, then run it
through the shared manifest/schema validator (e.g., validateSkillManifest or the
common schema utility) and only use skillMetadata.description or proceed with
AIClient.create and generateSkill if validation passes—otherwise throw or log a
clear error and abort regeneration. Apply the same validation change to the
other two regeneration handlers that build skillMetadata (the blocks around the
existing yaml.load uses at the other locations referenced in the review).
In `@skill-generator/src/utils/scorer.ts`:
- Around line 25-31: The checks access data.tags and data.tests as if they were
arrays, allowing non-array values to pass; update the validation in scorer.ts to
first confirm Array.isArray(data.tags) before checking length (e.g., if
(!Array.isArray(data.tags) || data.tags.length < 2) push 'invalid or less than 2
tags') and likewise for data.tests (e.g., if (!Array.isArray(data.tests) ||
data.tests.length === 0) push 'missing tests' or similar); apply the same
Array.isArray guards to the related checks around the 69-71 area to match the
stricter contract in validation/schema.ts.
In `@skill-generator/src/utils/security.ts`:
- Around line 20-21: In safeResolvePath, the current check using
relative.startsWith('..') falsely flags names like '..cache'; change the test to
only reject actual parent-traversal segments by checking if relative equals '..'
or starts with '..' followed by the platform separator (e.g., replace the
condition with: relative === '..' || relative.startsWith('..' + path.sep) ||
path.isAbsolute(relative)), using the variables relative, resolvedPath and
baseDir to locate the code; this tightens the check to block only true
parent-traversal while allowing directory names that merely begin with "..".
In `@skill-generator/src/validation/schema.ts`:
- Around line 74-76: The current check uses content.includes(section) which
matches substrings anywhere; change it to detect markdown headings only by
testing for a heading line for each requiredSections entry. For each section in
requiredSections, test content with a heading-aware regex (e.g., matching
start-of-line, 1–6 '#' characters, optional space, the exact section text, and a
word-boundary/end-of-line) instead of content.includes; ensure you escape the
section text when building the regex (add an escapeRegExp helper) and throw the
same Error when the heading match is not found.
In `@skill-generator/tests/scorer.test.ts`:
- Around line 8-24: The test "should return a low score for a directory missing
all files" is flaky because setup uses beforeAll/afterAll and does not reset
testDir between tests; change setup to reset the fixture in beforeEach (and
teardown in afterEach) so testDir is cleared/created before each test (use
fs.rmSync(testDir, { recursive: true, force: true }) if it exists, then
fs.mkdirSync(testDir, { recursive: true })) and keep the assertions that call
assessSkillQuality(testDir) unchanged; locate the beforeAll/afterAll blocks and
replace them with beforeEach/afterEach or add a clearing step in beforeEach to
ensure deterministic state.
---
Nitpick comments:
In `@skill-generator/tests/security.test.ts`:
- Around line 24-27: The test currently uses a substring check; replace that
with an exact path equality assertion by computing the expected absolute path
using path.resolve or path.join with the test's base and the relative segment
and asserting resolved === expected; specifically update the 'should resolve a
safe relative path inside the base directory' test to call safeResolvePath(base,
'./skills/test-skill'), build expected via path.resolve(base, 'skills',
'test-skill') (or path.join+path.resolve) and assert strict equality between
resolved and expected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c7cf7db-42fd-440b-9614-7e5f67f68ea0
⛔ Files ignored due to path filters (2)
docs/ai-framework/mcp-servers/amrit-docs-mcp/package-lock.jsonis excluded by!**/package-lock.jsonskill-generator/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (42)
docs/ai-framework/mcp-servers/amrit-docs-mcp/.gitignoredocs/ai-framework/mcp-servers/amrit-docs-mcp/RETRY_DESIGN.mddocs/ai-framework/mcp-servers/amrit-docs-mcp/package.jsondocs/ai-framework/mcp-servers/amrit-docs-mcp/src/index.tsdocs/ai-framework/mcp-servers/amrit-docs-mcp/src/retry.test.tsdocs/ai-framework/mcp-servers/amrit-docs-mcp/src/retry.tsdocs/ai-framework/mcp-servers/amrit-docs-mcp/tsconfig.jsonskill-generator/.gitignoreskill-generator/PR_DESCRIPTION.mdskill-generator/jest.config.jsskill-generator/package.jsonskill-generator/skill-generation-architecture.mdskill-generator/src/ai/client.tsskill-generator/src/cli/command.tsskill-generator/src/generators/skill.tsskill-generator/src/generators/writer.tsskill-generator/src/index.tsskill-generator/src/templates/manager.tsskill-generator/src/utils/logger.tsskill-generator/src/utils/scorer.tsskill-generator/src/utils/security.tsskill-generator/src/validation/schema.tsskill-generator/templates/README.md.templateskill-generator/templates/prompts.md.templateskill-generator/templates/skill.yaml.templateskill-generator/templates/tests.yaml.templateskill-generator/tests/schema.test.tsskill-generator/tests/scorer.test.tsskill-generator/tests/security.test.tsskill-generator/tsconfig.jsonskills/docker-security-auditor/README.mdskills/docker-security-auditor/prompts.mdskills/docker-security-auditor/skill.yamlskills/docker-security-auditor/tests.yamlskills/github-pr-reviewer/README.mdskills/github-pr-reviewer/prompts.mdskills/github-pr-reviewer/skill.yamlskills/github-pr-reviewer/tests.yamlskills/nodejs-api-reviewer/README.mdskills/nodejs-api-reviewer/prompts.mdskills/nodejs-api-reviewer/skill.yamlskills/nodejs-api-reviewer/tests.yaml
|
|
||
| ## Architecture | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
These fences are unlabeled and currently trigger MD040 lint warnings.
💡 Proposed fix
-```
+```text
MCP Tool Handler
@@
-```
+```text
delay = min(baseDelayMs × 2^attempt, maxDelayMs) + (jitter ? random(0, baseDelayMs) : 0)
@@
-```
+```text
running retry engine tests…Also applies to: 69-69, 151-151
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 24-24: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/ai-framework/mcp-servers/amrit-docs-mcp/RETRY_DESIGN.md` at line 24, The
markdown contains unlabeled fenced code blocks that trigger MD040; locate the
three fences that wrap the snippets "MCP Tool Handler", "delay = min(baseDelayMs
× 2^attempt, maxDelayMs) + (jitter ? random(0, baseDelayMs) : 0)" and "running
retry engine tests…" and add a language identifier (e.g., ```text) to each
opening fence so they become labeled code blocks.
Comment on lines
+87
to
+88
| | `src/retry.test.ts` | **New** — 13 unit tests covering all states and edge cases | | ||
| | `src/index.ts` | **Modified** — imports retry module, wraps `fetchGitHubReadme` and `fetchRepoStructure`, version bumped `1.0.0` → `1.1.0` | |
There was a problem hiding this comment.
Test count in docs is stale versus the current suite.
The doc says 13 tests, but docs/ai-framework/mcp-servers/amrit-docs-mcp/src/retry.test.ts currently defines 11 tests.
Also applies to: 168-168
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/ai-framework/mcp-servers/amrit-docs-mcp/RETRY_DESIGN.md` around lines 87
- 88, The docs claim "13 unit tests" but the actual test suite in retry.test.ts
contains 11 tests; update the RETRY_DESIGN.md entry that lists
`src/retry.test.ts` so the test count reflects the current suite (change "13
unit tests" to "11 unit tests" or adjust to the true current number), and ensure
the related changelog note for `src/index.ts` remains accurate; verify the
number by running the test file (retry.test.ts) before committing.
Comment on lines
+229
to
+231
| // Compute and wait for backoff delay | ||
| const delayMs = computeBackoff(attempt, cfg); | ||
| log(record, `Attempt failed. Reason: ${reason}. Backing off ${delayMs}ms before retry.`); |
There was a problem hiding this comment.
Backoff exponent is off by one from the declared policy.
Line 230 uses the zero-based loop index, so the first retry waits baseDelayMs instead of baseDelayMs * 2^1 as documented.
💡 Proposed fix
- const delayMs = computeBackoff(attempt, cfg);
+ const delayMs = computeBackoff(attempt + 1, cfg);📝 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
| // Compute and wait for backoff delay | |
| const delayMs = computeBackoff(attempt, cfg); | |
| log(record, `Attempt failed. Reason: ${reason}. Backing off ${delayMs}ms before retry.`); | |
| // Compute and wait for backoff delay | |
| const delayMs = computeBackoff(attempt + 1, cfg); | |
| log(record, `Attempt failed. Reason: ${reason}. Backing off ${delayMs}ms before retry.`); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/ai-framework/mcp-servers/amrit-docs-mcp/src/retry.ts` around lines 229 -
231, The backoff computation is using the zero-based loop index `attempt` which
causes the first retry to use baseDelayMs rather than baseDelayMs * 2^1; update
the call site in retry logic (where `computeBackoff(attempt, cfg)` is invoked)
to pass the 1-based attempt number (e.g., `attempt + 1`) or else adjust
`computeBackoff` to internally add 1 to the exponent; ensure the log call that
references `delayMs` remains consistent with the corrected computation.
|
|
||
| ## Features | ||
|
|
||
| - **Anthropic-Powered Generation**: Infers skill purpose, tools, categories, and MCP requirements. |
There was a problem hiding this comment.
Feature bullet should reflect multi-provider support, not Anthropic-only.
Update this line to mention both Anthropic and Ollama (and provider selection) so the description matches the implemented CLI behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skill-generator/PR_DESCRIPTION.md` at line 13, Update the feature bullet text
that currently reads "Anthropic-Powered Generation" to reflect multi-provider
support and provider selection: mention both Anthropic and Ollama and that the
CLI can choose the provider (e.g., "Multi-provider Generation (Anthropic &
Ollama) with provider selection") so the PR_DESCRIPTION.md entry aligns with the
implemented CLI behavior.
Comment on lines
+75
to
+77
| const skillMetadata = yaml.load(fs.readFileSync(skillYamlPath, 'utf8')) as any; | ||
| const aiClient = await AIClient.create(options.provider as 'anthropic' | 'ollama' | undefined); | ||
| const payload = await aiClient.generateSkill(`Regenerate detailed prompts for: ${skillMetadata.description || skillName}`); |
There was a problem hiding this comment.
Validate skill.yaml before using metadata in regeneration flows.
These paths bypass the shared schema contract by using yaml.load(... ) as any, so malformed manifests can drive prompt generation and file rewrites.
Suggested fix
- const skillMetadata = yaml.load(fs.readFileSync(skillYamlPath, 'utf8')) as any;
+ const skillYamlContent = fs.readFileSync(skillYamlPath, 'utf8');
+ const skillMetadata = validateSkillYaml(skillYamlContent);Apply the same change in all three regeneration handlers.
Also applies to: 119-121, 158-160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skill-generator/src/index.ts` around lines 75 - 77, The code currently uses
yaml.load(fs.readFileSync(skillYamlPath, 'utf8')) as any to build skillMetadata
and then calls aiClient.generateSkill(...) which allows malformed manifests to
flow into prompt generation; instead, read and parse the YAML, then run it
through the shared manifest/schema validator (e.g., validateSkillManifest or the
common schema utility) and only use skillMetadata.description or proceed with
AIClient.create and generateSkill if validation passes—otherwise throw or log a
clear error and abort regeneration. Apply the same validation change to the
other two regeneration handlers that build skillMetadata (the blocks around the
existing yaml.load uses at the other locations referenced in the review).
Comment on lines
+25
to
+31
| const data = yaml.load(fs.readFileSync(skillYamlPath, 'utf8')) as any; | ||
| const issues: string[] = []; | ||
| if (!data.name) issues.push('missing name'); | ||
| if (!data.description || data.description.length < 10) issues.push('insufficient description'); | ||
| if (!data.version || !/^\d+\.\d+\.\d+$/.test(data.version)) issues.push('invalid semver version'); | ||
| if (!data.tags || data.tags.length < 2) issues.push('less than 2 tags'); | ||
|
|
There was a problem hiding this comment.
Scoring can incorrectly pass schema-invalid YAML shapes.
data.tags.length and data?.tests?.length accept non-array values, so malformed files can still receive high scores. This diverges from the stricter schema contract in skill-generator/src/validation/schema.ts and can report misleading quality results.
💡 Suggested fix
- const data = yaml.load(fs.readFileSync(skillYamlPath, 'utf8')) as any;
+ const data = yaml.load(fs.readFileSync(skillYamlPath, 'utf8')) as any;
const issues: string[] = [];
- if (!data.name) issues.push('missing name');
- if (!data.description || data.description.length < 10) issues.push('insufficient description');
- if (!data.version || !/^\d+\.\d+\.\d+$/.test(data.version)) issues.push('invalid semver version');
- if (!data.tags || data.tags.length < 2) issues.push('less than 2 tags');
+ if (typeof data?.name !== 'string' || data.name.trim().length === 0) issues.push('missing name');
+ if (typeof data?.description !== 'string' || data.description.length < 10) issues.push('insufficient description');
+ if (typeof data?.version !== 'string' || !/^\d+\.\d+\.\d+$/.test(data.version)) issues.push('invalid semver version');
+ if (!Array.isArray(data?.tags) || data.tags.length < 2 || !data.tags.every((t: unknown) => typeof t === 'string')) {
+ issues.push('invalid tags');
+ }
@@
- const data = yaml.load(fs.readFileSync(testsYamlPath, 'utf8')) as any;
- const count = data?.tests?.length || 0;
+ const data = yaml.load(fs.readFileSync(testsYamlPath, 'utf8')) as any;
+ const tests = Array.isArray(data?.tests) ? data.tests : [];
+ const count = tests.filter(
+ (t: any) => t && typeof t.name === 'string' && 'input' in t && 'expected' in t
+ ).length;Also applies to: 69-71
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skill-generator/src/utils/scorer.ts` around lines 25 - 31, The checks access
data.tags and data.tests as if they were arrays, allowing non-array values to
pass; update the validation in scorer.ts to first confirm
Array.isArray(data.tags) before checking length (e.g., if
(!Array.isArray(data.tags) || data.tags.length < 2) push 'invalid or less than 2
tags') and likewise for data.tests (e.g., if (!Array.isArray(data.tests) ||
data.tests.length === 0) push 'missing tests' or similar); apply the same
Array.isArray guards to the related checks around the 69-71 area to match the
stricter contract in validation/schema.ts.
Comment on lines
+20
to
+21
| if (relative.startsWith('..') || path.isAbsolute(relative)) { | ||
| throw new Error(`Security Violation: Path traversal detected outside base directory: ${relativePath}`); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the referenced file and surrounding lines
sed -n '1,120p' skill-generator/src/utils/security.ts | cat -n
# Extract the exact lines around 20-30 for precise context
nl -ba skill-generator/src/utils/security.ts | sed -n '10,60p'
# Search for the traversal check and related helpers/usages
rg -n "Path traversal detected|startsWith\\('\\.\\.'\\)|path\\.isAbsolute\\(" skill-generator/src/utils/security.ts
rg -n "relativePath|baseDir|relative\\b|normalize\\(|resolve\\(" skill-generator/src/utils/security.ts
# Find where this function is called (if exported) and how inputs are formed
rg -n "security\\.ts|from '\\.*/security'|utils/security" -S skill-generator/src || true
rg -n "traversal detected outside base directory" -S skill-generator/src || trueRepository: PSMRI/AMRIT
Length of output: 1146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List where safeResolvePath is referenced
rg -n "safeResolvePath\\(" -S skill-generator/src
# If exported and used elsewhere, broaden search
rg -n "safeResolvePath\\(" -S . || true
# Inspect any likely callers with small surrounding context
rg -n "safeResolvePath\\(" -S . | head -n 50
# Also check if there are any normalizations / derived relativePath constraints
rg -n "validateSkillName\\(|relativePath" -S skill-generator/srcRepository: PSMRI/AMRIT
Length of output: 3358
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show safeResolvePath definition (full function small)
sed -n '1,120p' skill-generator/src/utils/security.ts
# Inspect callers around the safeResolvePath usage
sed -n '1,220p' skill-generator/src/index.ts
sed -n '1,120p' skill-generator/src/generators/writer.ts
sed -n '1,200p' skill-generator/src/generators/skill.ts
# Inspect security tests
sed -n '1,120p' skill-generator/tests/security.test.tsRepository: PSMRI/AMRIT
Length of output: 18542
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate CLI command implementation and inspect regenerate handlers
ls -la skill-generator/src/cli || true
fd -a -t f "command.*" skill-generator/src/cli || true
fd -a -t f "cli" skill-generator/src/cli || true
# Show command file(s)
for f in $(fd -t f "command.*" skill-generator/src/cli); do
echo "---- $f ----"
sed -n '1,240p' "$f"
done
# Search for regen + validateSkillName usage
rg -n "regenerate|handleRegeneratePrompt|handleRegenerateTests|handleRegenerateReadme" skill-generator/src/cli skill-generator/src | head -n 200
rg -n "validateSkillName\\(" skill-generator/src/cli skill-generator/src | head -n 200Repository: PSMRI/AMRIT
Length of output: 5625
Tighten the parent-traversal check to avoid false positives for ..* directory names
safeResolvePath computes relative = path.relative(baseDir, resolvedPath) and then blocks any value whose string starts with ... That will also reject legitimate in-base directory names like ..cache when it becomes the first segment relative to baseDir (e.g., if a caller passes an output dir of .).
Suggested fix
- if (relative.startsWith('..') || path.isAbsolute(relative)) {
+ if (relative === '..' || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative)) {
throw new Error(`Security Violation: Path traversal detected outside base directory: ${relativePath}`);
}📝 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
| if (relative.startsWith('..') || path.isAbsolute(relative)) { | |
| throw new Error(`Security Violation: Path traversal detected outside base directory: ${relativePath}`); | |
| if (relative === '..' || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative)) { | |
| throw new Error(`Security Violation: Path traversal detected outside base directory: ${relativePath}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skill-generator/src/utils/security.ts` around lines 20 - 21, In
safeResolvePath, the current check using relative.startsWith('..') falsely flags
names like '..cache'; change the test to only reject actual parent-traversal
segments by checking if relative equals '..' or starts with '..' followed by the
platform separator (e.g., replace the condition with: relative === '..' ||
relative.startsWith('..' + path.sep) || path.isAbsolute(relative)), using the
variables relative, resolvedPath and baseDir to locate the code; this tightens
the check to block only true parent-traversal while allowing directory names
that merely begin with "..".
Comment on lines
+74
to
+76
| for (const section of requiredSections) { | ||
| if (!content.includes(section)) { | ||
| throw new Error(`Prompts file is missing the required section: "${section}"`); |
There was a problem hiding this comment.
Use heading-aware checks for required prompt sections.
Line 75 uses substring matching, so non-header text can satisfy required sections and let invalid prompts.md pass validation.
Suggested fix
export function validatePromptsMd(content: string): void {
@@
for (const section of requiredSections) {
- if (!content.includes(section)) {
+ const sectionRegex = new RegExp(
+ `^${section.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$`,
+ 'm'
+ );
+ if (!sectionRegex.test(content)) {
throw new Error(`Prompts file is missing the required section: "${section}"`);
}
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skill-generator/src/validation/schema.ts` around lines 74 - 76, The current
check uses content.includes(section) which matches substrings anywhere; change
it to detect markdown headings only by testing for a heading line for each
requiredSections entry. For each section in requiredSections, test content with
a heading-aware regex (e.g., matching start-of-line, 1–6 '#' characters,
optional space, the exact section text, and a word-boundary/end-of-line) instead
of content.includes; ensure you escape the section text when building the regex
(add an escapeRegExp helper) and throw the same Error when the heading match is
not found.
Comment on lines
+8
to
+24
| beforeAll(() => { | ||
| if (!fs.existsSync(testDir)) { | ||
| fs.mkdirSync(testDir, { recursive: true }); | ||
| } | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| if (fs.existsSync(testDir)) { | ||
| fs.rmSync(testDir, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
|
|
||
| it('should return a low score for a directory missing all files', () => { | ||
| const report = assessSkillQuality(testDir); | ||
| expect(report.score).toBe(0); | ||
| expect(report.checks.some((c) => !c.passed)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
The first test depends on filesystem state that isn’t reset per test.
The “missing all files” case can become flaky if tests/mock-skill already contains artifacts. Reset the fixture directory in beforeEach to keep tests deterministic.
💡 Suggested fix
- beforeAll(() => {
- if (!fs.existsSync(testDir)) {
- fs.mkdirSync(testDir, { recursive: true });
- }
- });
+ beforeEach(() => {
+ fs.rmSync(testDir, { recursive: true, force: true });
+ fs.mkdirSync(testDir, { recursive: true });
+ });📝 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
| beforeAll(() => { | |
| if (!fs.existsSync(testDir)) { | |
| fs.mkdirSync(testDir, { recursive: true }); | |
| } | |
| }); | |
| afterAll(() => { | |
| if (fs.existsSync(testDir)) { | |
| fs.rmSync(testDir, { recursive: true, force: true }); | |
| } | |
| }); | |
| it('should return a low score for a directory missing all files', () => { | |
| const report = assessSkillQuality(testDir); | |
| expect(report.score).toBe(0); | |
| expect(report.checks.some((c) => !c.passed)).toBe(true); | |
| }); | |
| beforeEach(() => { | |
| fs.rmSync(testDir, { recursive: true, force: true }); | |
| fs.mkdirSync(testDir, { recursive: true }); | |
| }); | |
| afterAll(() => { | |
| if (fs.existsSync(testDir)) { | |
| fs.rmSync(testDir, { recursive: true, force: true }); | |
| } | |
| }); | |
| it('should return a low score for a directory missing all files', () => { | |
| const report = assessSkillQuality(testDir); | |
| expect(report.score).toBe(0); | |
| expect(report.checks.some((c) => !c.passed)).toBe(true); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skill-generator/tests/scorer.test.ts` around lines 8 - 24, The test "should
return a low score for a directory missing all files" is flaky because setup
uses beforeAll/afterAll and does not reset testDir between tests; change setup
to reset the fixture in beforeEach (and teardown in afterEach) so testDir is
cleared/created before each test (use fs.rmSync(testDir, { recursive: true,
force: true }) if it exists, then fs.mkdirSync(testDir, { recursive: true }))
and keep the assertions that call assessSkillQuality(testDir) unchanged; locate
the beforeAll/afterAll blocks and replace them with beforeEach/afterEach or add
a clearing step in beforeEach to ensure deterministic state.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/ai-framework/mcp-servers/amrit-docs-mcp/src/index.ts (2)
335-343: 💤 Low valueUnreachable fallback due to exhaustive enum handling.
After the
layer === "all"check, the remaining enum values (spring-boot,angular,kotlin-android) all exist inLAYER_STANDARDS. Theif (!standards)branch is dead code.This is harmless but could be removed for clarity, or the type could be narrowed to reflect the compile-time guarantee.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ai-framework/mcp-servers/amrit-docs-mcp/src/index.ts` around lines 335 - 343, Remove the dead null-check by either narrowing the type of `layer` where `LAYER_STANDARDS` is looked up or deleting the unreachable `if (!standards)` branch: locate the lookup of `const standards = LAYER_STANDARDS[layer]` in this file and either (a) refine the type of `layer` (or the surrounding function) to exclude values not present in `LAYER_STANDARDS`, or (b) delete the `if (!standards) { return { content: [...] } }` block so the code continues directly using `standards`; keep references to `LAYER_STANDARDS` and `layer` intact and ensure TypeScript still compiles after the change.
197-225: ⚡ Quick winInconsistent branch handling compared to
fetchGitHubReadme.
fetchGitHubReadmetriesmain,master, thendevelop, butfetchRepoStructureonly requests themainbranch. Repos whose default branch ismasterordevelopwill succeed README fetch but return the fallback message for structure fetch.Consider aligning branch probing logic between both functions or extracting a shared helper.
♻️ Proposed fix
async function fetchRepoStructure(repoName: string): Promise<string> { + const branches = ["main", "master", "develop"]; + + for (const branch of branches) { try { const response = await withRetry( - `fetch-structure:${repoName}`, + `fetch-structure:${repoName}@${branch}`, () => axios.get<{ tree: Array<{ path: string; type: string }> }>( - `${GITHUB_API_BASE}/${repoName}/git/trees/main?recursive=0`, + `${GITHUB_API_BASE}/${repoName}/git/trees/${branch}?recursive=0`, { timeout: 5000 } ), { maxRetries: 3, baseDelayMs: 300, maxDelayMs: 8_000, jitter: true, isRetryable: isNetworkError, } ); const topLevel = response.data.tree .filter((item) => !item.path.startsWith(".") && item.path !== "node_modules") .slice(0, 30) .map((item) => `${item.type === "tree" ? "📁" : "📄"} ${item.path}`) .join("\n"); return topLevel; - } catch { - return `Could not fetch structure for ${repoName}. Try cloning locally.`; + } catch (error: any) { + if (error?.status === 404 || error?.response?.status === 404) { + continue; + } + throw error; } + } + + return `Could not fetch structure for ${repoName}. Try cloning locally.`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ai-framework/mcp-servers/amrit-docs-mcp/src/index.ts` around lines 197 - 225, fetchRepoStructure only requests the "main" branch while fetchGitHubReadme probes "main", "master", then "develop", causing inconsistent behavior; update fetchRepoStructure to mirror fetchGitHubReadme by iterating the same branch candidates (e.g., ["main","master","develop"]) and attempting the axios GET with withRetry for each branch until a successful response is received, returning the first successful topLevel result and falling back to the existing message only after all branches fail (or, alternatively, extract a shared helper used by both fetchGitHubReadme and fetchRepoStructure to DRY the branch-probing + withRetry logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/ai-framework/mcp-servers/amrit-docs-mcp/src/index.ts`:
- Around line 335-343: Remove the dead null-check by either narrowing the type
of `layer` where `LAYER_STANDARDS` is looked up or deleting the unreachable `if
(!standards)` branch: locate the lookup of `const standards =
LAYER_STANDARDS[layer]` in this file and either (a) refine the type of `layer`
(or the surrounding function) to exclude values not present in
`LAYER_STANDARDS`, or (b) delete the `if (!standards) { return { content: [...]
} }` block so the code continues directly using `standards`; keep references to
`LAYER_STANDARDS` and `layer` intact and ensure TypeScript still compiles after
the change.
- Around line 197-225: fetchRepoStructure only requests the "main" branch while
fetchGitHubReadme probes "main", "master", then "develop", causing inconsistent
behavior; update fetchRepoStructure to mirror fetchGitHubReadme by iterating the
same branch candidates (e.g., ["main","master","develop"]) and attempting the
axios GET with withRetry for each branch until a successful response is
received, returning the first successful topLevel result and falling back to the
existing message only after all branches fail (or, alternatively, extract a
shared helper used by both fetchGitHubReadme and fetchRepoStructure to DRY the
branch-probing + withRetry logic).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5cde1743-dac9-4a68-a75a-75b7f67966af
📒 Files selected for processing (1)
docs/ai-framework/mcp-servers/amrit-docs-mcp/src/index.ts
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
This PR introduces an AI-powered skill generator for the AMRIT framework.
Contributors can now generate complete, production-ready skill scaffolds from a natural language description using either Anthropic Claude or locally hosted Ollama models.
The generator automatically creates:
skill.yamlprompts.mdtests.yamlREADME.mdThis significantly reduces the effort required to create new AMRIT skills while ensuring consistent structure, documentation, validation, and testing.
The implementation includes:
Example Usage
amrit generate skill "review Node.js APIs" --provider ollamaScreenshots & Validation
Automated Test Suite (12/12 Passing)
All unit and integration tests pass successfully, validating the complete skill generation workflow and core framework integrations.
Coverage includes:
Result
Ollama End-to-End Generation
Successfully generated a complete AMRIT skill using a locally hosted Ollama model (
qwen2.5-coder:7b) without requiring external API credentials.The generated skill passed validation, quality scoring, and artifact generation checks, demonstrating compatibility with both local and cloud-based AI providers.
Skill Quality Score - 100/100
Generated skill passed all AMRIT validation criteria on the first pass.
Multi-provider AI architecture (Anthropic + Ollama)
AI-powered skill scaffolding from natural language
Multi-stage generation pipeline for improved reliability
Automatic generation of:
skill.yamlprompts.mdtests.yamlREADME.mdZod-based metadata validation
YAML validation and parsing
Skill quality scoring system
Interactive CLI support
Regeneration commands for prompts, tests, and documentation
Summary by CodeRabbit
New Features
Documentation
Tests
Chores