Feature/epic 13 stack aware implementer - #19
Merged
Conversation
…cumentation - Updated `PLAN.md` to reflect the transition to Epic 13, detailing the new stack-aware implementer agent variants and their detection logic. - Removed the legacy `reactTsSenior` agent and introduced 13 stack-specific variants, ensuring backward compatibility with existing configurations. - Enhanced `README.md` and `AGENTS.md` to clarify the new agent structure and provide detailed guidance on the stack matrix. - Implemented new detection logic for JVM and .NET frameworks, improving support for diverse technology stacks. - Added new utility functions and prompts to facilitate user selection of implementer variants based on detected stacks. This commit aims to improve the flexibility and clarity of agent configurations while ensuring comprehensive documentation for users.
… security - Updated `PRD.md` to include a new epic on "Caveman Communication Style for All Agents," expanding the implementation epics section. - Revised `.claude/settings.json` to add new permissions for shell utilities, including `Read`, `Glob`, and `Grep`, while streamlining existing Bash command permissions. - Refactored `permission-constants.ts` to simplify the allowance of sandbox-wrapped commands using a single wildcard per wrapper, enhancing security by restricting dangerous sub-commands. - Improved tests in `permissions.test.ts` and `epic-1-safety.test.ts` to validate the new permissions structure and ensure comprehensive coverage. This commit aims to enhance the clarity of the PRD and improve the security of command permissions across the project.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the single react-ts-senior agent with 13 stack-aware implementer variants, adds JVM and .NET detectors, routes implementer templates by detected stack, tightens sandbox permissions, adds caveman compression tooling/skills, and introduces migration + safe-delete logic for legacy artifacts. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/Init
participant Detector as Stack Detector
participant Router as Implementer Router
participant Generator as Generator
participant Templates as Template System
participant Renderer as EJS Renderer
CLI->>Detector: detectStack(projectRoot)
Detector->>Detector: read manifests, search files (package.json, pyproject, pom.xml, *.csproj, etc.)
Detector-->>CLI: DetectedStack
CLI->>Router: getApplicableImplementerVariant(DetectedStack)
Router->>Router: match framework/language rules
Router-->>CLI: ImplementerVariant
CLI->>Generator: generateAll(config with implementerVariant)
Generator->>Generator: buildContext(config)
Generator->>Templates: resolve agents/implementer-variants/{variant}.md.ejs
Templates->>Renderer: renderTemplate(template, context)
Renderer->>Renderer: include implementer-core partial + specificsBlock
Renderer-->>Generator: rendered markdown
Generator->>Generator: applyPostProcessors (cavemanCompress if enabled)
Generator-->>CLI: GeneratedFile[] (final)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/prompt/prompt-flow.ts (1)
69-76:⚠️ Potential issue | 🟠 MajorDrive the implementer variant prompt from the selected stack, not raw detection.
After
askStack, the user can change the language/framework/runtime, but Line 75 still derives the default from the originaldetectedvalues. A repo detected as React that the user switches to Python will still preselect the React variant, which can persist the wrong implementer routing in the final config. If implementer is optional, this prompt should also happen only after confirming that agent stays enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompt/prompt-flow.ts` around lines 69 - 76, The implementer variant is being derived from the original detected values instead of the user-selected stack; change the flow so askImplementerVariant uses the resolved stack (the return of askStack) and only prompt for implementer after confirming agents are enabled: call askAgentSelection({ isFrontend }) before askImplementerVariant, check selectedAgents includes the agent that uses implementer variants, and then call askImplementerVariant(stack) (or pass stack.framework) so the default/choices reflect the user-selected stack rather than detected.
🧹 Nitpick comments (9)
tests/installer/safe-delete-stale-files.test.ts (1)
6-6: Use UPPER_SNAKE_CASE for the module-level mock constant.Please rename
mockConfirmtoMOCK_CONFIRMto match the repo naming rule for module-level constants.♻️ Proposed fix
-const mockConfirm = jest.fn<() => Promise<boolean>>(); +const MOCK_CONFIRM = jest.fn<() => Promise<boolean>>(); jest.unstable_mockModule('@inquirer/prompts', () => ({ - confirm: mockConfirm, + confirm: MOCK_CONFIRM, select: jest.fn<() => Promise<string>>(), input: jest.fn<() => Promise<string>>(), checkbox: jest.fn<() => Promise<string[]>>(), })); const { safeDeleteStaleFiles } = await import('../../src/installer/safe-delete-stale-files.js'); describe('safeDeleteStaleFiles', () => { let projectRoot: string; beforeEach(async () => { projectRoot = await mkdtemp(join(tmpdir(), 'agents-safe-del-')); - mockConfirm.mockReset(); + MOCK_CONFIRM.mockReset(); }); @@ - expect(mockConfirm).not.toHaveBeenCalled(); + expect(MOCK_CONFIRM).not.toHaveBeenCalled(); @@ - mockConfirm.mockResolvedValueOnce(false); + MOCK_CONFIRM.mockResolvedValueOnce(false); @@ - expect(mockConfirm).toHaveBeenCalledTimes(1); + expect(MOCK_CONFIRM).toHaveBeenCalledTimes(1); @@ - mockConfirm.mockResolvedValueOnce(true); + MOCK_CONFIRM.mockResolvedValueOnce(true); @@ - expect(mockConfirm).toHaveBeenCalledTimes(1); + expect(MOCK_CONFIRM).toHaveBeenCalledTimes(1);As per coding guidelines
Name module-level constants in UPPER_SNAKE_CASE.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/installer/safe-delete-stale-files.test.ts` at line 6, Rename the module-level mock constant `mockConfirm` to `MOCK_CONFIRM` to follow UPPER_SNAKE_CASE convention: change the declaration `const mockConfirm = jest.fn<() => Promise<boolean>>();` to `const MOCK_CONFIRM = jest.fn<() => Promise<boolean>>();` and update every usage/reference in this test file (e.g., calls, resets, expect assertions) to the new `MOCK_CONFIRM` identifier so tests continue to work.src/templates/agents/implementer-variants/generic.md.ejs (1)
1-7: Consider extracting shared frontmatter into a partial.These fields are duplicated across all implementer variants. Centralizing them would reduce drift risk when model/tools metadata changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/templates/agents/implementer-variants/generic.md.ejs` around lines 1 - 7, The frontmatter block at the top of generic.md.ejs is duplicated across implementer variant templates; extract this shared YAML (name, description, tools, model, color) into a reusable partial (e.g., _implementer_frontmatter.ejs) and replace the hard-coded block in generic.md.ejs and the other implementer variant templates with an include/partial call (using the project’s EJS include mechanism) so metadata updates are centralized; ensure the partial exports identical keys and update any template rendering paths that reference the original frontmatter to use the new partial.README.md (1)
125-125: Consider splitting the migration paragraph into bullets.Line 125 packs migration behavior, legacy cleanup, and safe-delete semantics into one sentence. Breaking it into bullets would make operational behavior easier to scan.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 125, Split the long migration sentence into a short bulleted list in README.md that separately states: the emitted filenames (.claude/agents/implementer.md and .codex/skills/implementer/SKILL.md), where the active variant is recorded (.agents-workflows.json under agents.implementerVariant), how legacy manifests migrate (reactTsSenior: true → implementerVariant: 'react-ts' on first update), and the cleanup/safe-delete semantics (removal of .claude/agents/react-ts-senior.md and .codex/skills/react-ts-senior/SKILL.md via the Epic 7 safe-delete confirmation flow, mention that --yes skips the prompt and a backup is always written first) so each operational behavior is its own bullet for easier scanning.src/installer/safe-delete-stale-files.ts (1)
30-43: Avoid backup creation when deletion is declined.Right now backup runs before confirmation, so “keep file” still generates a backup artifact. Consider backing up only once deletion is confirmed (or suppression is enabled).
♻️ Proposed flow adjustment
- const fileEntry: GeneratedFile = { path: candidate, content: '' }; - await backupExistingFiles(projectRoot, [fileEntry]); - - if (!suppressed) { + if (!suppressed) { const confirmed = await confirm({ message: `Removing stale file replaced by implementer variant: ${candidate}. Delete?`, default: false, }); if (!confirmed) { logger.warn(`Skipped stale file: ${candidate} (kept on disk).`); continue; } } + const fileEntry: GeneratedFile = { path: candidate, content: '' }; + await backupExistingFiles(projectRoot, [fileEntry]); await rm(absolutePath); logger.info(`Removed stale: ${candidate}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/installer/safe-delete-stale-files.ts` around lines 30 - 43, The current flow calls backupExistingFiles(...) before asking the user to confirm, creating a backup even when deletion is declined; change the sequence so you only call backupExistingFiles(fileEntry) after you know deletion will proceed (i.e., when suppressed is true OR confirm(...) returned true). Concretely, move the creation/usage of fileEntry and the backupExistingFiles(...) call to just after the confirmation block (or trigger it immediately when suppressed is true), using the existing variables candidate, fileEntry, suppressed, confirmed and logger to keep behavior and logging consistent.tests/generator/epic-4-standards.test.ts (1)
117-118: Extend backend matrix to include newly supported backend frameworks.To keep this suite aligned with backend expansion in this PR, add
spring-bootandaspnetcoreto the backend case list.✅ Suggested test matrix update
- const BACKEND = ['express', 'fastify', 'hono', 'nestjs', 'fastapi', 'django', 'flask'] as const; + const BACKEND = ['express', 'fastify', 'hono', 'nestjs', 'fastapi', 'django', 'flask', 'spring-boot', 'aspnetcore'] as const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/generator/epic-4-standards.test.ts` around lines 117 - 118, The BACKEND test matrix (the constant named BACKEND in the tests/generator/epic-4-standards.test.ts) is missing the newly supported frameworks; update the BACKEND const to include 'spring-boot' and 'aspnetcore' (e.g., const BACKEND = ['express', 'fastify', 'hono', 'nestjs', 'fastapi', 'django', 'flask', 'spring-boot', 'aspnetcore'] as const) so the test suite covers those backends; ensure the array remains a readonly const and run the tests to confirm no other matrices (e.g., NON_BACKEND) need matching changes.tests/generator/implementer-routing.test.ts (1)
7-80: Refactor theit.eachcallback to a typed single-object parameter.Line 78 currently uses three inferred positional parameters, which conflicts with your TS conventions.
Suggested patch
+type RoutingCase = { + label: string; + overrides: Parameters<typeof makeDetectedStack>[0]; + expected: ImplementerVariant; +}; + describe('getApplicableImplementerVariant — 13-row decision table', () => { - it.each<[string, Parameters<typeof makeDetectedStack>[0], ImplementerVariant]>([ + const ROUTING_CASES: readonly RoutingCase[] = [ [ 'spring-boot framework → java-spring', { framework: { value: 'spring-boot', confidence: 0.9 }, language: { value: 'java', confidence: 0.9 } }, 'java-spring', ], @@ [ 'unknown language + no framework → generic', { framework: { value: null, confidence: 0 }, language: { value: 'kotlin', confidence: 0.9 } }, 'generic', ], - ])('%s', (_label, overrides, expected) => { + ]; + + it.each(ROUTING_CASES)('$label', ({ overrides, expected }: RoutingCase) => { const detected = makeDetectedStack(overrides); expect(getApplicableImplementerVariant(detected)).toBe(expected); }); });As per coding guidelines: “Always add explicit type annotations to function parameters — never rely on implicit inference” and “Functions with more than 2 parameters must use a single object parameter”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/generator/implementer-routing.test.ts` around lines 7 - 80, The test callback passed to it.each currently uses three positional parameters; change it to accept a single typed object parameter (e.g., { label, overrides, expected }: { label: string; overrides: Parameters<typeof makeDetectedStack>[0]; expected: ImplementerVariant }) and update the body to use those properties when calling makeDetectedStack and getApplicableImplementerVariant; ensure the callback signature has an explicit type annotation and replace usages of the positional _label, overrides, expected with the new destructured object properties to satisfy the "single object parameter" and explicit-typing rules.scripts/capture-implementer-baseline.ts (1)
7-13: Pin the captured variant in the fixture config.This script renders the generic template directly, but the config never explicitly says the agent variant is
generic. That makes the baseline depend on whatevermakeStackConfigcurrently derives for a Next.js stack, which is brittle if that helper changes later. Please setagents.implementerVariantexplicitly here as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/capture-implementer-baseline.ts` around lines 7 - 13, The generated fixture doesn't pin the implementer variant, so update the config passed to makeStackConfig (the object literal used to create config) to include agents: { implementerVariant: 'generic' } so buildContext(config) and subsequent renderTemplate('agents/implementer-variants/generic.md.ejs', ctx) always use the generic implementer; ensure the unique symbol agents.implementerVariant is set on the config before calling buildContext or renderTemplate.tests/generator/stack-aware-helpers.ts (1)
12-58: Consider splitting this helper surface behind anindex.tsbarrel.This file exposes several public helpers and path constants at once. It will be easier to maintain in this repo if each public helper lives in its own small file and this module becomes a thin barrel.
As per coding guidelines, "One public component/helper per file"; based on learnings, "Use folder-based module organization with colocated tests and
index.tsbarrel exports".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/generator/stack-aware-helpers.ts` around lines 12 - 58, Split the large tests/generator/stack-aware-helpers.ts module into one public helper per file and add a thin index.ts barrel that re-exports them: move FIXTURES_DIR and path constants to a constants.ts, move detectFixture to detectFixture.ts, configForFixture to configForFixture.ts, generateForFixture to generateForFixture.ts, and getImplementerContent to getImplementerContent.ts (each file exporting its corresponding symbol), then create an index.ts that imports and re-exports detectFixture, configForFixture, generateForFixture, getImplementerContent and the constants so existing imports keep working; ensure imports in each new file reference createDefaultConfig, detectStack, generateAll, getContent, and StackConfig/GeneratedFile types as before.src/prompt/questions.ts (1)
67-68: Move these re-exports to a dedicated barrel (src/prompt/index.ts).
questions.tsnow mixes concrete helper implementations with many public re-exports, which blurs module ownership. Keep this file implementation-focused and centralize exports in a barrel file.♻️ Suggested refactor
- export type { ProjectDocumentationFiles } from './ask-project-docs.js'; - export { askProjectDocumentationFiles, askMainBranch } from './ask-project-docs.js'; ... - export { askConventions } from './ask-conventions.js'; - export { askAgentSelection } from './ask-agent-selection.js'; - export { askCommandSelection } from './ask-command-selection.js'; ... - export { askImplementerVariant } from './ask-implementer-variant.js';// src/prompt/index.ts export type { ProjectDocumentationFiles } from './ask-project-docs.js'; export { askProjectDocumentationFiles, askMainBranch } from './ask-project-docs.js'; export { askConventions } from './ask-conventions.js'; export { askAgentSelection } from './ask-agent-selection.js'; export { askCommandSelection } from './ask-command-selection.js'; export { askTargets } from './ask-targets.js'; export { askGovernance } from './ask-governance.js'; export { askIsolation } from './ask-isolation.js'; export { askNonInteractiveMode, HOST_OS_ACCEPT_PHRASE } from './ask-non-interactive.js'; export { enableNonInteractiveWithIsolation } from './enable-non-interactive-with-isolation.js'; export { askWorkspaceSelection } from './ask-workspace-selection.js'; export { askImplementerVariant } from './ask-implementer-variant.js';As per coding guidelines, "One public component/helper per file" and "Use folder-based module organization with colocated tests and
index.tsbarrel exports".Also applies to: 154-156, 163-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompt/questions.ts` around lines 67 - 68, Create a new barrel module src/prompt/index.ts that re-exports the public symbols now mixed into questions.ts (export type { ProjectDocumentationFiles } and export { askProjectDocumentationFiles, askMainBranch } plus the other helpers: askConventions, askAgentSelection, askCommandSelection, askTargets, askGovernance, askIsolation, askNonInteractiveMode, HOST_OS_ACCEPT_PHRASE, enableNonInteractiveWithIsolation, askWorkspaceSelection, askImplementerVariant), then remove those re-export lines from src/prompt/questions.ts so that questions.ts only contains implementation; update any imports elsewhere to import these symbols from src/prompt (the new barrel) instead of from questions.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/settings.json:
- Around line 26-30: The current wildcard wrapper executor entries like
"Bash(wsl *)", "Bash(docker exec *)", "Bash(docker compose exec *)",
"Bash(podman exec *)", and "Bash(devcontainer exec *)" let any command run
inside containers/WSL and bypass the allow-list; remove the wildcard forms and
replace them with explicit, scoped wrapper entries that mirror the vetted host
allow-list (e.g., only the exact commands you permit such as "Bash(wsl git)",
"Bash(wsl npm)", etc.), or drop wrapper executors entirely for those
environments so wrapper calls must match the same approved command tokens used
on the host; ensure each replacement references the exact command names used
elsewhere in your allow-list so behavior is consistent.
In `@src/cli/update-command.ts`:
- Around line 126-130: The safeDeleteStaleFiles call is executed too early and
can remove stale files before writeGeneratedFiles and the manifest are
successfully written; move the safeDeleteStaleFiles invocation to after
writeGeneratedFiles (and after the manifest write/commit step) so
rollback/backups (e.g., the changedFiles/backup logic) cover all file changes;
update the flow around safeDeleteStaleFiles, writeGeneratedFiles, and the
manifest write function to ensure deletion runs only on successful completion of
writes.
In `@src/detector/detect-dotnet-framework.ts`:
- Around line 13-57: The detector currently only inspects the first .csproj
found via findFirstCsproj which can miss ASP.NET evidence in other projects;
update the logic so detectDotnetFramework scans all discovered .csproj files (or
rename findFirstCsproj to return all matches) and for each path returned call
tryReadFile and containsAny(content, ASPNETCORE_NEEDLES) until a match is found,
then return { value: 'aspnetcore', confidence:
BACKEND_FRAMEWORK_CONFIDENCE['aspnetcore'] }; if none match return { value:
null, confidence: 0 }. Ensure you reference and update the functions
findFirstCsproj (or create findAllCsproj), detectDotnetFramework, tryReadFile,
and use ASPNETCORE_NEEDLES in the loop.
In `@src/generator/permission-constants.ts`:
- Around line 237-241: The current SANDBOX_WRAPPER_ALLOWS uses a blanket
wildcard (Bash(${wrapper} *)) which permits unsafe inner shapes that aren’t
covered by SANDBOX_INNER_DENIES; update SANDBOX_WRAPPER_ALLOWS to derive allowed
wrapper patterns from the safe inner allow-list (e.g., map each entry of the
inner allow-list into `Bash(${wrapper} <inner-pattern>)`) so only explicitly
allowed inner commands are wrapped, or alternatively expand SANDBOX_INNER_DENIES
to cover the missing inner forms (reference SANDBOX_WRAPPER_PREFIXES,
SANDBOX_WRAPPER_ALLOWS, and SANDBOX_INNER_DENIES when making the change).
In `@src/templates/agents/implementer-variants/angular.md.ejs`:
- Around line 11-13: The template include for implementer core is emitting an
internal placeholder ("Detailed body (citation-backed, top-5 anti-patterns)
lands in Epic 17.") — locate the include call that passes specificsBlock in
implementer-variants/angular.md.ejs (the
include('../../partials/implementer-core.md.ejs', { specificsBlock: `...` })
invocation) and replace that internal Epic placeholder with real, actionable
Angular stack guidance (concise guidance points, actionable anti-patterns to
avoid, and citation hints) so the generated agent instructions contain useful
stack-specific content instead of the Epic placeholder.
In `@src/templates/agents/implementer-variants/python.md.ejs`:
- Around line 19-21: The guidance currently assumes Pydantic v2 by recommending
model_validator/field_validator; update the template to be version-aware: detect
or document which Pydantic major version the project uses and conditionally
advise using model_validator/field_validator for v2 and
`@validator/`@root_validator for v1, and/or show both patterns with a clear note.
Refer to the validator symbols model_validator, field_validator, `@validator`, and
`@root_validator` in the change so maintainers can find and update the text in
implementer-variants/python.md.ejs.
In `@src/templates/agents/implementer-variants/typescript.md.ejs`:
- Around line 11-13: Replace the placeholder specificsBlock passed to the
implementer-core include with concrete TypeScript-specific guidance: update the
specificsBlock argument in
src/templates/agents/implementer-variants/typescript.md.ejs to provide
practical, actionable TypeScript advice (e.g., recommended tsconfig settings,
typing patterns for interfaces/types, preferred async patterns and generics, top
5 TypeScript anti-patterns to avoid, and a short note on common tooling like
eslint/ts-node/tsup) so the TypeScript variant produces stack-specific
directions rather than the current generic placeholder; locate the include call
and replace the backtick string for specificsBlock accordingly.
In `@src/templates/agents/implementer-variants/vue.md.ejs`:
- Around line 11-13: The template currently passes a placeholder string via the
include call's specificsBlock in
src/templates/agents/implementer-variants/vue.md.ejs; replace that placeholder
with concrete Vue/Nuxt guidance by updating the specificsBlock payload (the
argument to include('../../partials/implementer-core.md.ejs', { specificsBlock:
... })) to contain actionable items: a concise "Stack Specifics" section
covering recommended Nuxt/Vue versions and modules, SSR/SSG considerations,
routing and data-fetching patterns, composition API and state-management
guidance (Pinia vs Vuex), top-5 anti-patterns with short mitigations,
deployment/config tips (build/config flags), and recommended linters/formatters;
ensure the new content is markdown text string passed as specificsBlock so
implementer-core.md.ejs renders the detailed Vue/Nuxt specifics instead of the
Epic-17 placeholder.
In `@tests/fixtures/frontend-svelte/package.json`:
- Around line 4-13: Move the package "@sveltejs/kit" from the dependencies
object to devDependencies in the package.json fixture: remove "@sveltejs/kit"
entry under "dependencies" and add the same version string under
"devDependencies" alongside "svelte", "vite", etc., ensuring the manifest now
lists both "@sveltejs/kit" and "svelte" in devDependencies to match SvelteKit
requirements and preserve the exact semver string.
In `@tests/generator/__fixtures__/implementer-generic-baseline.md`:
- Around line 13-21: Update the baseline list to use canonical casing for the
two tech names: replace the string "Typescript" with "TypeScript" and replace
"Eslint" with "ESLint" in the baseline content (look for the exact items
"Typescript" and "Eslint" in the fixture file to modify).
In `@tests/generator/stack-aware-agents.test.ts`:
- Around line 169-173: The test currently allows missing AGENTS.md because it
uses agentsMd?.content ?? '' and thus should fail loudly if the file is absent;
update the test named "backend %s AGENTS.md: no ui-designer sub-agent row" to
assert the file exists before checking contents by ensuring agentsMd is defined
(e.g., expect(agentsMd).toBeDefined() or throw if undefined) using the agentsMd
variable returned from generateForFixture(fixture), then assert agentsMd.content
does not contain 'UI/UX design & review'.
- Around line 108-114: The test "go: context.Context, go test" in the suite is
intentionally failing and must be fixed: either add the missing "go test"
guidance to the Go implementer template that getImplementerContent('backend-go')
returns so the assertions for 'context.Context' and 'go test' pass, or mark this
test as skipped (e.g., replace the failing it(...) with it.skip(...) or
test.skip(...)) until the template change lands; locate the test by its
description string and the helper getImplementerContent to apply the change.
---
Outside diff comments:
In `@src/prompt/prompt-flow.ts`:
- Around line 69-76: The implementer variant is being derived from the original
detected values instead of the user-selected stack; change the flow so
askImplementerVariant uses the resolved stack (the return of askStack) and only
prompt for implementer after confirming agents are enabled: call
askAgentSelection({ isFrontend }) before askImplementerVariant, check
selectedAgents includes the agent that uses implementer variants, and then call
askImplementerVariant(stack) (or pass stack.framework) so the default/choices
reflect the user-selected stack rather than detected.
---
Nitpick comments:
In `@README.md`:
- Line 125: Split the long migration sentence into a short bulleted list in
README.md that separately states: the emitted filenames
(.claude/agents/implementer.md and .codex/skills/implementer/SKILL.md), where
the active variant is recorded (.agents-workflows.json under
agents.implementerVariant), how legacy manifests migrate (reactTsSenior: true →
implementerVariant: 'react-ts' on first update), and the cleanup/safe-delete
semantics (removal of .claude/agents/react-ts-senior.md and
.codex/skills/react-ts-senior/SKILL.md via the Epic 7 safe-delete confirmation
flow, mention that --yes skips the prompt and a backup is always written first)
so each operational behavior is its own bullet for easier scanning.
In `@scripts/capture-implementer-baseline.ts`:
- Around line 7-13: The generated fixture doesn't pin the implementer variant,
so update the config passed to makeStackConfig (the object literal used to
create config) to include agents: { implementerVariant: 'generic' } so
buildContext(config) and subsequent
renderTemplate('agents/implementer-variants/generic.md.ejs', ctx) always use the
generic implementer; ensure the unique symbol agents.implementerVariant is set
on the config before calling buildContext or renderTemplate.
In `@src/installer/safe-delete-stale-files.ts`:
- Around line 30-43: The current flow calls backupExistingFiles(...) before
asking the user to confirm, creating a backup even when deletion is declined;
change the sequence so you only call backupExistingFiles(fileEntry) after you
know deletion will proceed (i.e., when suppressed is true OR confirm(...)
returned true). Concretely, move the creation/usage of fileEntry and the
backupExistingFiles(...) call to just after the confirmation block (or trigger
it immediately when suppressed is true), using the existing variables candidate,
fileEntry, suppressed, confirmed and logger to keep behavior and logging
consistent.
In `@src/prompt/questions.ts`:
- Around line 67-68: Create a new barrel module src/prompt/index.ts that
re-exports the public symbols now mixed into questions.ts (export type {
ProjectDocumentationFiles } and export { askProjectDocumentationFiles,
askMainBranch } plus the other helpers: askConventions, askAgentSelection,
askCommandSelection, askTargets, askGovernance, askIsolation,
askNonInteractiveMode, HOST_OS_ACCEPT_PHRASE, enableNonInteractiveWithIsolation,
askWorkspaceSelection, askImplementerVariant), then remove those re-export lines
from src/prompt/questions.ts so that questions.ts only contains implementation;
update any imports elsewhere to import these symbols from src/prompt (the new
barrel) instead of from questions.ts.
In `@src/templates/agents/implementer-variants/generic.md.ejs`:
- Around line 1-7: The frontmatter block at the top of generic.md.ejs is
duplicated across implementer variant templates; extract this shared YAML (name,
description, tools, model, color) into a reusable partial (e.g.,
_implementer_frontmatter.ejs) and replace the hard-coded block in generic.md.ejs
and the other implementer variant templates with an include/partial call (using
the project’s EJS include mechanism) so metadata updates are centralized; ensure
the partial exports identical keys and update any template rendering paths that
reference the original frontmatter to use the new partial.
In `@tests/generator/epic-4-standards.test.ts`:
- Around line 117-118: The BACKEND test matrix (the constant named BACKEND in
the tests/generator/epic-4-standards.test.ts) is missing the newly supported
frameworks; update the BACKEND const to include 'spring-boot' and 'aspnetcore'
(e.g., const BACKEND = ['express', 'fastify', 'hono', 'nestjs', 'fastapi',
'django', 'flask', 'spring-boot', 'aspnetcore'] as const) so the test suite
covers those backends; ensure the array remains a readonly const and run the
tests to confirm no other matrices (e.g., NON_BACKEND) need matching changes.
In `@tests/generator/implementer-routing.test.ts`:
- Around line 7-80: The test callback passed to it.each currently uses three
positional parameters; change it to accept a single typed object parameter
(e.g., { label, overrides, expected }: { label: string; overrides:
Parameters<typeof makeDetectedStack>[0]; expected: ImplementerVariant }) and
update the body to use those properties when calling makeDetectedStack and
getApplicableImplementerVariant; ensure the callback signature has an explicit
type annotation and replace usages of the positional _label, overrides, expected
with the new destructured object properties to satisfy the "single object
parameter" and explicit-typing rules.
In `@tests/generator/stack-aware-helpers.ts`:
- Around line 12-58: Split the large tests/generator/stack-aware-helpers.ts
module into one public helper per file and add a thin index.ts barrel that
re-exports them: move FIXTURES_DIR and path constants to a constants.ts, move
detectFixture to detectFixture.ts, configForFixture to configForFixture.ts,
generateForFixture to generateForFixture.ts, and getImplementerContent to
getImplementerContent.ts (each file exporting its corresponding symbol), then
create an index.ts that imports and re-exports detectFixture, configForFixture,
generateForFixture, getImplementerContent and the constants so existing imports
keep working; ensure imports in each new file reference createDefaultConfig,
detectStack, generateAll, getContent, and StackConfig/GeneratedFile types as
before.
In `@tests/installer/safe-delete-stale-files.test.ts`:
- Line 6: Rename the module-level mock constant `mockConfirm` to `MOCK_CONFIRM`
to follow UPPER_SNAKE_CASE convention: change the declaration `const mockConfirm
= jest.fn<() => Promise<boolean>>();` to `const MOCK_CONFIRM = jest.fn<() =>
Promise<boolean>>();` and update every usage/reference in this test file (e.g.,
calls, resets, expect assertions) to the new `MOCK_CONFIRM` identifier so tests
continue to work.
🪄 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: f8ee8972-2482-47f1-bb9e-ea96ea5eef6e
📒 Files selected for processing (81)
.claude/settings.jsonPLAN.mdPRD.mdQA.mdREADME.mdscripts/capture-implementer-baseline.tssrc/cli/list-command.tssrc/cli/update-command.tssrc/constants/frameworks.tssrc/detector/detect-dotnet-framework.tssrc/detector/detect-framework.tssrc/detector/detect-jvm-framework.tssrc/detector/text-match.tssrc/generator/build-context.tssrc/generator/generate-agents.tssrc/generator/implementer-routing.tssrc/generator/permission-constants.tssrc/generator/permissions.tssrc/generator/types.tssrc/installer/index.tssrc/installer/safe-delete-stale-files.tssrc/prompt/ask-agent-selection.tssrc/prompt/ask-command-selection.tssrc/prompt/ask-conventions.tssrc/prompt/ask-implementer-variant.tssrc/prompt/ask-project-docs.tssrc/prompt/default-config.tssrc/prompt/prompt-flow.tssrc/prompt/questions.tssrc/schema/implementer-variants.tssrc/schema/stack-config.tssrc/templates/agents/implementer-variants/angular.md.ejssrc/templates/agents/implementer-variants/dotnet-csharp.md.ejssrc/templates/agents/implementer-variants/generic.md.ejssrc/templates/agents/implementer-variants/go.md.ejssrc/templates/agents/implementer-variants/java-spring.md.ejssrc/templates/agents/implementer-variants/javascript.md.ejssrc/templates/agents/implementer-variants/node-ts-backend.md.ejssrc/templates/agents/implementer-variants/python.md.ejssrc/templates/agents/implementer-variants/react-ts.md.ejssrc/templates/agents/implementer-variants/rust.md.ejssrc/templates/agents/implementer-variants/svelte.md.ejssrc/templates/agents/implementer-variants/typescript.md.ejssrc/templates/agents/implementer-variants/vue.md.ejssrc/templates/agents/implementer.md.ejssrc/templates/agents/react-ts-senior.md.ejssrc/templates/config/AGENTS.md.ejssrc/templates/config/CLAUDE.md.ejssrc/templates/partials/implementer-core.md.ejstests/detector/detect-dotnet-framework.test.tstests/detector/detect-jvm-framework.test.tstests/detector/detect-monorepo.test.tstests/detector/detect-stack.test.tstests/fixtures/backend-dotnet/Project.csprojtests/fixtures/backend-go/go.modtests/fixtures/backend-go/main.gotests/fixtures/backend-java-spring/pom.xmltests/fixtures/backend-node-nestjs/package.jsontests/fixtures/backend-python-fastapi/pyproject.tomltests/fixtures/backend-rust/Cargo.tomltests/fixtures/frontend-angular/package.jsontests/fixtures/frontend-svelte/package.jsontests/fixtures/frontend-vue/package.jsontests/generator/__fixtures__/implementer-generic-baseline.mdtests/generator/build-context.test.tstests/generator/epic-1-safety.test.tstests/generator/epic-16-cross-model-routing.test.tstests/generator/epic-2-quality.test.tstests/generator/epic-3-review-depth.test.tstests/generator/epic-4-standards.test.tstests/generator/fixtures.tstests/generator/generate-all.test.tstests/generator/generic-byte-identical.test.tstests/generator/implementer-routing.test.tstests/generator/permissions.test.tstests/generator/react-ts-senior.test.tstests/generator/stack-aware-agents.test.tstests/generator/stack-aware-helpers.tstests/installer/safe-delete-stale-files.test.tstests/prompt/prompt-flow.test.tstests/schema/implementer-variants-migration.test.ts
💤 Files with no reviewable changes (5)
- tests/generator/epic-2-quality.test.ts
- src/templates/agents/react-ts-senior.md.ejs
- src/templates/agents/implementer.md.ejs
- tests/generator/epic-3-review-depth.test.ts
- tests/generator/react-ts-senior.test.ts
…ency - Updated `PRD.md` to include attribution for the original caveman skills work and enhance the rationale for using caveman communication styles in agent templates. - Introduced `skills-lock.json` to manage caveman skills, including `caveman`, `caveman-commit`, and `caveman-compress`, ensuring proper tracking and versioning. - Created new skills for `caveman`, `caveman-commit`, and `caveman-compress`, detailing their functionality and usage for improved token efficiency in communication and commit messages. - Enhanced existing documentation to clarify the implementation of caveman styles across generated agent templates, ensuring all agents utilize the new communication directives. This commit aims to streamline communication within agents and reduce token usage while maintaining technical accuracy.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (12)
.agents/skills/caveman-commit/SKILL.md-41-42 (1)
41-42:⚠️ Potential issue | 🟡 MinorAdd fenced code block languages to satisfy markdownlint.
Line 41 and Line 52 use fenced code blocks without a language, triggering MD040.
💡 Suggested fix
- ``` + ```text feat(api): add GET /users/:id/profile Mobile client needs profile data without the full user payload to reduce LTE bandwidth on cold-launch screens. Closes `#128`
feat(api)!: rename /v1/orders to /v1/checkout BREAKING CHANGE: clients on /v1/orders must migrate to /v1/checkout before 2026-06-01. Old route returns 410 after that date.</details> Also applies to: 52-53 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.agents/skills/caveman-commit/SKILL.md around lines 41 - 42, The markdown
fenced code blocks containing the commit messages (the block with "feat(api):
add GET /users/:id/profile" and the block with "feat(api)!: rename /v1/orders to
/v1/checkout") are missing a language tag and trigger MD040; update each opening
fence to include a language (e.g., changetotext) for both occurrences
so the blocks becometext ...and satisfy markdownlint.</details> </blockquote></details> <details> <summary>src/generator/index.ts-31-33 (1)</summary><blockquote> `31-33`: _⚠️ Potential issue_ | _🟡 Minor_ **Add an explicit type annotation to the `.map()` callback parameter.** The callback parameter `file` on line 31 relies on implicit type inference, which violates the TypeScript requirement for explicit parameter annotations. Change it to `(generatedFile: GeneratedFile)` to comply with guidelines and use a more descriptive variable name. <details> <summary>✅ Suggested fix</summary> ```diff - return files.map((file) => - file.path.endsWith('.md') ? { ...file, content: cavemanCompress(file.content) } : file, + return files.map((generatedFile: GeneratedFile) => + generatedFile.path.endsWith('.md') + ? { ...generatedFile, content: cavemanCompress(generatedFile.content) } + : generatedFile, ); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/generator/index.ts` around lines 31 - 33, The map callback currently uses an implicitly typed parameter `file`, which violates the explicit parameter annotation requirement; update the callback in the files.map call to use an explicit parameter type and clearer name like `(generatedFile: GeneratedFile)` and replace usages of `file` inside the callback with `generatedFile` (preserving the existing conditional logic that calls cavemanCompress on generatedFile.content when generatedFile.path.endsWith('.md')). ``` </details> </blockquote></details> <details> <summary>.agents/skills/caveman-compress/README.md-1-3 (1)</summary><blockquote> `1-3`: _⚠️ Potential issue_ | _🟡 Minor_ **Add alt text to the hero image.** The opening `<img>` tag has no `alt`, which trips markdownlint and makes the README less accessible in rendered views. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/caveman-compress/README.md around lines 1 - 3, The hero <img> tag in README.md lacks an alt attribute which fails markdownlint and reduces accessibility; update the <img src="https://em-content.zobj.net/source/apple/391/rock_1faa8.png" width="80" /> tag to include a concise, descriptive alt attribute (e.g. alt="rock emoji" or alt="caveman rock icon") so the image is accessible and linting passes. ``` </details> </blockquote></details> <details> <summary>.agents/skills/caveman-compress/README.md-19-25 (1)</summary><blockquote> `19-25`: _⚠️ Potential issue_ | _🟡 Minor_ **Specify languages for the fenced blocks.** These blocks are all unlabeled, so markdownlint flags them and renderers lose syntax hints. `bash` or `text` would be enough here depending on the block. Also applies to: 86-95, 108-125, 149-156 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.agents/skills/caveman-compress/README.md around lines 19 - 25, The fenced
code blocks in .agents/skills/caveman-compress/README.md (for example the block
showing the CLI command "/caveman:compress CLAUDE.md" and the file list block
"CLAUDE.md ← compressed ...") are unlabeled; add language identifiers (e.g.,
bash for shell/CLI snippets andtext for plain output/file lists) to each
fenced block to satisfy markdownlint and enable proper renderer syntax
highlighting—update all occurrences mentioned (around lines referenced: the
command block and the blocks at 86-95, 108-125, 149-156).</details> </blockquote></details> <details> <summary>src/prompt/ask-caveman-style.ts-4-6 (1)</summary><blockquote> `4-6`: _⚠️ Potential issue_ | _🟡 Minor_ **Avoid overpromising the compression savings here.** This toggle only runs the new markdown post-processor, but the prompt advertises `~65-75%` savings. The new `caveman-compress` docs in this PR benchmark markdown compression much closer to ~46% on average, so this copy is likely to set the wrong expectation for users enabling the feature. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/prompt/ask-caveman-style.ts` around lines 4 - 6, Update the confirm prompt message in ask-caveman-style.ts (the confirm(...) call) to stop overpromising compression; change the percent range "~65-75%" to the more accurate benchmark (e.g., "about ~46% on average") or remove the numeric claim entirely so the message reads something like "Apply caveman style? (runs markdown post-processor, see https://github.com/juliusbrussee/caveman)"; ensure the edited string is the one passed as the message argument to confirm to correct user expectations. ``` </details> </blockquote></details> <details> <summary>tests/generator/epic-20-caveman-style.test.ts-36-45 (1)</summary><blockquote> `36-45`: _⚠️ Potential issue_ | _🟡 Minor_ **These assertions don't actually verify identical file sets.** The `.md` case only compares counts, and both non-`.md` cases only compare content for files that happen to exist in both runs. This suite will still pass if `cavemanStyle` adds or drops files, which is exactly the regression surface this toggle introduces. Assert sorted path sets first, then compare contents. Also applies to: 49-69 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tests/generator/epic-20-caveman-style.test.ts` around lines 36 - 45, The tests for cavemanStyle must assert identical file sets before comparing contents: in the tests using makeStackConfig and generateAll (the 'does not modify non-.md files' and the similar '.md' and non-'.md' cases), first compute and compare sorted arrays of result paths (e.g., files.map(f=>f.path) vs baseline.map(b=>b.path)) to ensure no files were added or removed, then for each path assert contents match; update the blocks that define nonMd and the .md case to perform this sorted path equality check prior to per-file content comparisons so the test fails if cavemanStyle changes the set of files. ``` </details> </blockquote></details> <details> <summary>.agents/skills/compress/scripts/detect.py-58-59 (1)</summary><blockquote> `58-59`: _⚠️ Potential issue_ | _🟡 Minor_ **Rename the comprehension variable to satisfy Ruff E741.** The single-letter name `l` trips Ruff's ambiguous-name rule in all three comprehensions here. <details> <summary>Suggested fix</summary> ```diff - non_empty = sum(1 for l in lines[:30] if l.strip()) + non_empty = sum(1 for line in lines[:30] if line.strip()) @@ - code_lines = sum(1 for l in lines if l.strip() and _is_code_line(l)) - non_empty = sum(1 for l in lines if l.strip()) + code_lines = sum(1 for line in lines if line.strip() and _is_code_line(line)) + non_empty = sum(1 for line in lines if line.strip()) ``` </details> Also applies to: 90-91 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/compress/scripts/detect.py around lines 58 - 59, Rename the single-letter generator variable `l` in the comprehensions inside detect.py to a descriptive name (e.g., `line`) to satisfy Ruff E741; specifically update the comprehensions used to compute `non_empty = sum(1 for l in lines[:30] if l.strip())` and the other two occurrences around lines 90-91 so they read `for line in ...` and adjust the inner usage (`line.strip()` etc.) accordingly, leaving the surrounding logic and variable names unchanged. ``` </details> </blockquote></details> <details> <summary>.agents/skills/caveman-compress/scripts/detect.py-58-59 (1)</summary><blockquote> `58-59`: _⚠️ Potential issue_ | _🟡 Minor_ **Rename the comprehension variable to satisfy Ruff E741.** The single-letter name `l` trips Ruff's ambiguous-name rule in all three comprehensions here. <details> <summary>Suggested fix</summary> ```diff - non_empty = sum(1 for l in lines[:30] if l.strip()) + non_empty = sum(1 for line in lines[:30] if line.strip()) @@ - code_lines = sum(1 for l in lines if l.strip() and _is_code_line(l)) - non_empty = sum(1 for l in lines if l.strip()) + code_lines = sum(1 for line in lines if line.strip() and _is_code_line(line)) + non_empty = sum(1 for line in lines if line.strip()) ``` </details> Also applies to: 90-91 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/caveman-compress/scripts/detect.py around lines 58 - 59, The comprehensions in detect.py use the single-letter variable name `l`, which violates Ruff E741; update each comprehension (e.g., the one computing non_empty and the other two around yaml_indicators) to use a clearer name such as `line` (or `ln`) instead of `l` so Ruff stops flagging ambiguous names; keep all logic identical and only rename the loop variable inside the generator expressions used to compute non_empty and yaml_indicators. ``` </details> </blockquote></details> <details> <summary>.agents/skills/caveman-compress/scripts/cli.py-43-46 (1)</summary><blockquote> `43-46`: _⚠️ Potential issue_ | _🟡 Minor_ **Skip reason is wrong for backup files.** `should_compress()` also returns `False` for `*.original.md`, but this branch always reports "not natural language". That makes a valid backup look like a detector bug. <details> <summary>Suggested fix</summary> ```diff # Check if compressible if not should_compress(filepath): - print("Skipping: file is not natural language (code/config)") + if filepath.name.endswith(".original.md"): + print("Skipping: backup files are never recompressed") + else: + print(f"Skipping: detected {file_type}") sys.exit(0) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/caveman-compress/scripts/cli.py around lines 43 - 46, The skip message is misleading for backup files because should_compress(filepath) can be False for backups (e.g., "*.original.md"); update the branch that handles a False return from should_compress to distinguish backup filenames by checking filepath patterns (for example endswith(".original.md") or matching your backup naming convention) and print "Skipping: backup file" for those, otherwise keep "Skipping: file is not natural language (code/config)"; leave the sys.exit(0) behavior unchanged and refer to should_compress and filepath when making the change. ``` </details> </blockquote></details> <details> <summary>.agents/skills/compress/scripts/benchmark.py-44-56 (1)</summary><blockquote> `44-56`: _⚠️ Potential issue_ | _🟡 Minor_ **Reject unsupported CLI arities.** `main()` only handles `0` user args and `2` user args, but every other arity currently falls through into discovery mode. A typo like `benchmark.py original.md` will silently benchmark unrelated test pairs instead of failing fast. <details> <summary>Suggested fix</summary> ```diff def main(): # Direct file pair: python3 benchmark.py original.md compressed.md + if len(sys.argv) not in {1, 3}: + print("Usage: python3 benchmark.py [original.md compressed.md]") + sys.exit(1) + if len(sys.argv) == 3: orig = Path(sys.argv[1]).resolve() comp = Path(sys.argv[2]).resolve() ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/compress/scripts/benchmark.py around lines 44 - 56, main() currently only branches for len(sys.argv)==1 (no user args) and len(sys.argv)==3 (two user args) but lets all other arities fall into discovery; add an explicit guard in main() that rejects any unsupported CLI arity (i.e., if len(sys.argv) not in (1,3)) by printing a short usage message (showing usage like "python3 benchmark.py [original.md compressed.md]") and exiting with non-zero status; update the main() function around the existing sys.argv handling (the block that calls benchmark_pair and print_table) to perform this check early so typos like a single filename fail fast. ``` </details> </blockquote></details> <details> <summary>.agents/skills/caveman-compress/scripts/benchmark.py-44-56 (1)</summary><blockquote> `44-56`: _⚠️ Potential issue_ | _🟡 Minor_ **Reject unsupported CLI arities.** `main()` only handles `0` user args and `2` user args, but every other arity currently falls through into discovery mode. A typo like `benchmark.py original.md` will silently benchmark unrelated test pairs instead of failing fast. <details> <summary>Suggested fix</summary> ```diff def main(): # Direct file pair: python3 benchmark.py original.md compressed.md + if len(sys.argv) not in {1, 3}: + print("Usage: python3 benchmark.py [original.md compressed.md]") + sys.exit(1) + if len(sys.argv) == 3: orig = Path(sys.argv[1]).resolve() comp = Path(sys.argv[2]).resolve() ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/caveman-compress/scripts/benchmark.py around lines 44 - 56, main() currently only expects zero user args (discovery mode) or two user args (len(sys.argv)==3) but silently proceeds to discovery for any other arity; update main() to validate sys.argv length and reject unsupported arities by printing a clear usage message and exiting non-zero. Add an explicit guard such that if len(sys.argv) is not 1 or 3, you call sys.exit(1) after printing usage instructions (mentioning expected forms like "benchmark.py" or "benchmark.py original.md compressed.md"), ensuring this check sits before calling benchmark_pair() or the discovery flow so typos like "benchmark.py original.md" fail fast. ``` </details> </blockquote></details> <details> <summary>.agents/skills/caveman-compress/scripts/compress.py-91-101 (1)</summary><blockquote> `91-101`: _⚠️ Potential issue_ | _🟡 Minor_ **Add exception chaining and consider a timeout for the subprocess call.** 1. The re-raised `RuntimeError` loses the original exception context. Use `raise ... from e` for proper chaining. 2. Without a timeout, the subprocess could hang indefinitely if the CLI stalls. <details> <summary>Proposed fix</summary> ```diff try: result = subprocess.run( ["claude", "--print"], input=prompt, text=True, capture_output=True, check=True, + timeout=120, # 2 minute timeout ) return strip_llm_wrapper(result.stdout.strip()) except subprocess.CalledProcessError as e: - raise RuntimeError(f"Claude call failed:\n{e.stderr}") + raise RuntimeError(f"Claude call failed:\n{e.stderr}") from e + except subprocess.TimeoutExpired as e: + raise RuntimeError("Claude CLI timed out after 120 seconds") from e ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/caveman-compress/scripts/compress.py around lines 91 - 101, The subprocess call in compress.py (the subprocess.run call that returns strip_llm_wrapper(result.stdout.strip())) should include a reasonable timeout and preserve exception chaining: add a timeout argument to subprocess.run (e.g., timeout=...) and update the exception handling to use "raise RuntimeError(... ) from e" for subprocess.CalledProcessError; also add a separate except block for subprocess.TimeoutExpired to raise a clear RuntimeError (or similar) that includes the timeout error and chains from the TimeoutExpired instance so the original context is retained. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (6)</summary><blockquote> <details> <summary>.agents/skills/caveman-review/SKILL.md (1)</summary><blockquote> `25-25`: **Clarify conflicting guidance on praise in terse mode.** Line 25 conflicts with Line 10 (“No throat-clearing”) and can produce non-actionable output in strict mode. Keep praise out of `caveman-review` entirely, or gate it explicitly to non-terse mode only. <details> <summary>Proposed doc tweak</summary> ```diff -- "Great work!", "Looks good overall but..." — say it once at the top, not per comment +- "Great work!", "Looks good overall but..." — omit in `caveman-review` mode; keep only in normal verbose mode ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/caveman-review/SKILL.md at line 25, The caveman-review skill currently emits praise ("Great work!", "Looks good overall but...") which conflicts with the "No throat-clearing" terse-mode rule; update the SKILL.md behavior for the caveman-review skill to either remove praise entirely or conditionally emit praise only when the mode is non‑terse (check the mode flag used by caveman-review and gate the praise text behind it), and update the documentation to state that praise is omitted in terse mode to avoid non-actionable output (referencing the caveman-review skill and the terse/non‑terse mode flag). ``` </details> </blockquote></details> <details> <summary>tests/generator/epic-20-caveman-style.test.ts (1)</summary><blockquote> `33-33`: **Use a descriptive `.map()` callback name.** `f` is pretty opaque in a test that already deals with generated-file collections. Something like `generatedFile` reads much cleaner here. As per coding guidelines "Use descriptive variable names in `.map()` callbacks". <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tests/generator/epic-20-caveman-style.test.ts` at line 33, The map callback in the assertion using withFiller is using an opaque name `f`; rename the callback parameter to a descriptive name (e.g., `generatedFile` or `generated`) so the assertion reads like expect(withFiller.map((generatedFile) => generatedFile.path)).toEqual([]); update any similar `.map()` callbacks in this test that use `f` to use the new descriptive name for clarity. ``` </details> </blockquote></details> <details> <summary>tests/utils/caveman-compress.test.ts (1)</summary><blockquote> `44-61`: **Add regression cases for the non-code markdown that must stay exact.** This suite only guards fenced and inline backticks, but the new skill docs promise exact preservation for headings, links/URLs, file paths, commands, and tables too. Adding those cases will make the documented contract enforceable once the compressor logic is tightened. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tests/utils/caveman-compress.test.ts` around lines 44 - 61, Add regression tests to tests/utils/caveman-compress.test.ts alongside the existing cavemanCompress tests to assert exact preservation for non-code markdown types promised by the docs: create it() cases that pass inputs containing a heading (e.g. "# Title\n"), a markdown link and raw URL (e.g. "[text](http://example.com)" and "https://example.com/path"), a file path or filesystem fragment (e.g. "src/app/index.ts"), a shell command invocation (e.g. "npm install --save package"), and a markdown table (pipe-delimited rows); for each case call cavemanCompress(input) and assert the output contains the exact original substring and does not trim or alter it (use toContain and not.toMatch where appropriate). ``` </details> </blockquote></details> <details> <summary>.agents/skills/caveman-compress/scripts/compress.py (3)</summary><blockquote> `158-158`: **Consider moving `MAX_FILE_SIZE` to module level.** Other constants like `MAX_RETRIES`, `SENSITIVE_BASENAME_REGEX`, etc. are defined at module level. Moving this constant alongside them improves discoverability and consistency. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/caveman-compress/scripts/compress.py at line 158, MAX_FILE_SIZE is currently defined inside a local scope; move it up to the module-level alongside other constants (e.g., MAX_RETRIES, SENSITIVE_BASENAME_REGEX) so it's discoverable and consistent. Remove the local definition at line where MAX_FILE_SIZE = 500_000 appears and add the same constant near the other top-level constants in .agents/skills/caveman-compress/scripts/compress.py; ensure any references (e.g., in functions like compress_file or related validation/helpers) continue to use MAX_FILE_SIZE without changes. ``` </details> --- `66-67`: **Consider moving imports to the top of the file.** Placing imports after function definitions is unconventional and can make dependencies harder to discover. If these are positioned here to avoid circular imports, consider adding a brief comment explaining the rationale. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/caveman-compress/scripts/compress.py around lines 66 - 67, Move the two relative imports (should_compress from .detect and validate from .validate) to the top-level import section of the module so dependencies are discoverable; if they were intentionally placed after function definitions to avoid a circular import, either refactor to break the cycle or leave them where they are but add a brief explanatory comment above those imports stating why (e.g., "placed here to avoid circular import with X") or convert them to explicit local imports inside the functions that need them (e.g., import .detect.should_compress inside the caller) and run tests to verify behavior. ``` </details> --- `81-87`: **Set an explicit timeout for the Anthropic API call.** While the Anthropic SDK has a default 10-minute timeout, explicitly setting a timeout is a best practice for reliability. Consider adding a client-level timeout to handle network issues more predictably: <details> <summary>Proposed fix</summary> ```diff - client = anthropic.Anthropic(api_key=api_key) + client = anthropic.Anthropic(api_key=api_key, timeout=120.0) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/caveman-compress/scripts/compress.py around lines 81 - 87, The Anthropic API call currently instantiates anthropic.Anthropic and calls client.messages.create without an explicit timeout; update the code that creates the client (the anthropic.Anthropic(...) instantiation) or the messages.create invocation to pass an explicit timeout value (e.g., timeout=60 or another appropriate seconds value) so network hangs are bounded; modify the client creation in the compress logic that calls client = anthropic.Anthropic(api_key=api_key) and/or the messages.create(...) call to include the timeout parameter and ensure any exceptions from timeouts are handled or allowed to propagate consistently. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/caveman-compress/scripts/benchmark.py:
- Around line 58-62: The tests_dir path is computed relative to the script
directory (using Path(file).parent.parent.parent) which resolves to
.agents/skills/tests instead of the repository-level tests; change the repo root
computation to use an absolute resolved parent (e.g. repo_root =
Path(file).resolve().parents[4]) and then set tests_dir = repo_root /
"tests" / "caveman-compress", keeping the existing exists() check and exit
logic; update references to tests_dir and remove the old
Path(file).parent.parent.parent usage.In @.agents/skills/caveman-compress/scripts/validate.py:
- Around line 110-160: The validator currently only checks fenced code blocks
via validate_code_blocks/extract_code_blocks; add explicit checks for inline
code and indented code so they are preserved exactly per SKILL.md. Implement (or
extend) extract_inline_code(orig) and extract_indented_code(orig) (and their
compressed counterparts) and add two new validators validate_inline_code(orig,
comp, result) and validate_indented_code(orig, comp, result) that compare
sets/lists and call result.add_error(...) on mismatches; finally call these two
validators from validate(...) alongside validate_code_blocks to ensure inline
and indented code cannot be rewritten silently.In @.agents/skills/caveman-compress/SKILL.md:
- Around line 22-26: The example command assumes the caller is already in
.agents/skills; instead update the instruction to use the resolved skill
directory (the directory you locate by searching for
caveman-compress/scripts/main.py) and run the module from that
absolute/resolved path (e.g. use "<resolved_skill_dir>/scripts" or "python3 -m
<resolved_skill_dir>.scripts <absolute_filepath>" semantics) so it works
regardless of the current working directory; update the SKILL.md text and the
example command to reference the resolved directory variable rather than "cd
caveman-compress".In @.agents/skills/compress/scripts/benchmark.py:
- Around line 58-62: The tests_dir calculation uses
Path(file).parent.parent.parent which points into the agents tree instead of
the repository root; update the discovery logic so tests_dir points to the
repo-level tests directory. Replace the current
Path(file).parent.parent.parent / "tests" computation (the tests_dir
variable) with a repo-root-aware resolution (for example compute repo_root =
Path(file).resolve().parents[<appropriate_index>] and then set tests_dir =
repo_root / "tests" / "caveman-compress"), or alternatively detect the git root
(via subprocess/git) and join "tests/caveman-compress"; keep the variable name
tests_dir and the same existence-check logic.In @.agents/skills/compress/scripts/cli.py:
- Around line 50-61: compress_file currently returns False for multiple failure
modes, making the CLI branch that prints "failed after retries" ambiguous;
change compress_file to return a structured status (e.g., enum/str like
"success", "backup_exists", "retries_exhausted") or raise specific exceptions
(e.g., BackupExistsError, RetriesExhaustedError) and then update the CLI logic
that calls compress_file to handle these distinct cases: on
"backup_exists"/BackupExistsError print a clear message about the existing
.original.md and exit with a non-retry-specific code, on
"retries_exhausted"/RetriesExhaustedError print the retries-exhausted message
and exit with the current retry exit code, and on success preserve the existing
success output and exit(0); reference compress_file and any new
BackupExistsError/RetriesExhaustedError symbols in your changes.In @.agents/skills/compress/scripts/detect.py:
- Around line 68-77: The extensionless-file heuristic misclassifies files like
Dockerfile, Makefile, and .env as natural_language; before the "if not ext:"
block in detect.py, add a filename-based check using filepath.name.lower()
(e.g., check names "dockerfile", "makefile" and ".env" / ".env.example" or names
that start with ".env") and return the appropriate type ("code" for
Dockerfile/Makefile, "config" for .env variants) so that should_compress() won't
treat those critical files as compressible; update or add a small SKIP_FILENAMES
mapping or conditional branch alongside COMPRESSIBLE_EXTENSIONS and
SKIP_EXTENSIONS to implement this behavior.In
@src/utils/caveman-compress.ts:
- Around line 5-21: extractCodeBlocks currently only replaces fenced and inline
backtick code so headings, link labels, tables, file paths and shell/command
lines still get rewritten by stripFiller; update extractCodeBlocks (and its
CodeBlocks map usage with PLACEHOLDER_PREFIX) to also detect and
placeholder-preserve other markdown constructs before calling stripFiller: e.g.
block-level headings (lines starting with #), reference link definitions
(patterns like [label]: ...), table rows (lines containing pipes or
header/separator rows), code/command lines starting with $/, ./, /, or >, and
any other block-level fenced constructs; ensure the same placeholder/key
generation logic used by extractCodeBlocks is applied so stripFiller only
touches plain paragraph text and not these preserved constructs (also apply the
same changes where similar extraction is used around the 43-51 area).
Minor comments:
In @.agents/skills/caveman-commit/SKILL.md:
- Around line 41-42: The markdown fenced code blocks containing the commit
messages (the block with "feat(api): add GET /users/:id/profile" and the block
with "feat(api)!: rename /v1/orders to /v1/checkout") are missing a language tag
and trigger MD040; update each opening fence to include a language (e.g., change
totext) for both occurrences so the blocks becometext ...and
satisfy markdownlint.In @.agents/skills/caveman-compress/README.md:
- Around line 1-3: The hero
tag in README.md lacks an alt attribute which
fails markdownlint and reduces accessibility; update the
tag to include a concise, descriptive alt attribute (e.g. alt="rock emoji" or
alt="caveman rock icon") so the image is accessible and linting passes.- Around line 19-25: The fenced code blocks in
.agents/skills/caveman-compress/README.md (for example the block showing the CLI
command "/caveman:compress CLAUDE.md" and the file list block "CLAUDE.md ←
compressed ...") are unlabeled; add language identifiers (e.g.,bash for shell/CLI snippets andtext for plain output/file lists) to each fenced block
to satisfy markdownlint and enable proper renderer syntax highlighting—update
all occurrences mentioned (around lines referenced: the command block and the
blocks at 86-95, 108-125, 149-156).In @.agents/skills/caveman-compress/scripts/benchmark.py:
- Around line 44-56: main() currently only expects zero user args (discovery
mode) or two user args (len(sys.argv)==3) but silently proceeds to discovery for
any other arity; update main() to validate sys.argv length and reject
unsupported arities by printing a clear usage message and exiting non-zero. Add
an explicit guard such that if len(sys.argv) is not 1 or 3, you call sys.exit(1)
after printing usage instructions (mentioning expected forms like "benchmark.py"
or "benchmark.py original.md compressed.md"), ensuring this check sits before
calling benchmark_pair() or the discovery flow so typos like "benchmark.py
original.md" fail fast.In @.agents/skills/caveman-compress/scripts/cli.py:
- Around line 43-46: The skip message is misleading for backup files because
should_compress(filepath) can be False for backups (e.g., "*.original.md");
update the branch that handles a False return from should_compress to
distinguish backup filenames by checking filepath patterns (for example
endswith(".original.md") or matching your backup naming convention) and print
"Skipping: backup file" for those, otherwise keep "Skipping: file is not natural
language (code/config)"; leave the sys.exit(0) behavior unchanged and refer to
should_compress and filepath when making the change.In @.agents/skills/caveman-compress/scripts/compress.py:
- Around line 91-101: The subprocess call in compress.py (the subprocess.run
call that returns strip_llm_wrapper(result.stdout.strip())) should include a
reasonable timeout and preserve exception chaining: add a timeout argument to
subprocess.run (e.g., timeout=...) and update the exception handling to use
"raise RuntimeError(... ) from e" for subprocess.CalledProcessError; also add a
separate except block for subprocess.TimeoutExpired to raise a clear
RuntimeError (or similar) that includes the timeout error and chains from the
TimeoutExpired instance so the original context is retained.In @.agents/skills/caveman-compress/scripts/detect.py:
- Around line 58-59: The comprehensions in detect.py use the single-letter
variable namel, which violates Ruff E741; update each comprehension (e.g.,
the one computing non_empty and the other two around yaml_indicators) to use a
clearer name such asline(orln) instead oflso Ruff stops flagging
ambiguous names; keep all logic identical and only rename the loop variable
inside the generator expressions used to compute non_empty and yaml_indicators.In @.agents/skills/compress/scripts/benchmark.py:
- Around line 44-56: main() currently only branches for len(sys.argv)==1 (no
user args) and len(sys.argv)==3 (two user args) but lets all other arities fall
into discovery; add an explicit guard in main() that rejects any unsupported CLI
arity (i.e., if len(sys.argv) not in (1,3)) by printing a short usage message
(showing usage like "python3 benchmark.py [original.md compressed.md]") and
exiting with non-zero status; update the main() function around the existing
sys.argv handling (the block that calls benchmark_pair and print_table) to
perform this check early so typos like a single filename fail fast.In @.agents/skills/compress/scripts/detect.py:
- Around line 58-59: Rename the single-letter generator variable
lin the
comprehensions inside detect.py to a descriptive name (e.g.,line) to satisfy
Ruff E741; specifically update the comprehensions used to computenon_empty = sum(1 for l in lines[:30] if l.strip())and the other two occurrences around
lines 90-91 so they readfor line in ...and adjust the inner usage
(line.strip()etc.) accordingly, leaving the surrounding logic and variable
names unchanged.In
@src/generator/index.ts:
- Around line 31-33: The map callback currently uses an implicitly typed
parameterfile, which violates the explicit parameter annotation requirement;
update the callback in the files.map call to use an explicit parameter type and
clearer name like(generatedFile: GeneratedFile)and replace usages offile
inside the callback withgeneratedFile(preserving the existing conditional
logic that calls cavemanCompress on generatedFile.content when
generatedFile.path.endsWith('.md')).In
@src/prompt/ask-caveman-style.ts:
- Around line 4-6: Update the confirm prompt message in ask-caveman-style.ts
(the confirm(...) call) to stop overpromising compression; change the percent
range "~65-75%" to the more accurate benchmark (e.g., "about ~46% on average")
or remove the numeric claim entirely so the message reads something like "Apply
caveman style? (runs markdown post-processor, see
https://github.com/juliusbrussee/caveman)"; ensure the edited string is the one
passed as the message argument to confirm to correct user expectations.In
@tests/generator/epic-20-caveman-style.test.ts:
- Around line 36-45: The tests for cavemanStyle must assert identical file sets
before comparing contents: in the tests using makeStackConfig and generateAll
(the 'does not modify non-.md files' and the similar '.md' and non-'.md' cases),
first compute and compare sorted arrays of result paths (e.g.,
files.map(f=>f.path) vs baseline.map(b=>b.path)) to ensure no files were added
or removed, then for each path assert contents match; update the blocks that
define nonMd and the .md case to perform this sorted path equality check prior
to per-file content comparisons so the test fails if cavemanStyle changes the
set of files.
Nitpick comments:
In @.agents/skills/caveman-compress/scripts/compress.py:
- Line 158: MAX_FILE_SIZE is currently defined inside a local scope; move it up
to the module-level alongside other constants (e.g., MAX_RETRIES,
SENSITIVE_BASENAME_REGEX) so it's discoverable and consistent. Remove the local
definition at line where MAX_FILE_SIZE = 500_000 appears and add the same
constant near the other top-level constants in
.agents/skills/caveman-compress/scripts/compress.py; ensure any references
(e.g., in functions like compress_file or related validation/helpers) continue
to use MAX_FILE_SIZE without changes.- Around line 66-67: Move the two relative imports (should_compress from .detect
and validate from .validate) to the top-level import section of the module so
dependencies are discoverable; if they were intentionally placed after function
definitions to avoid a circular import, either refactor to break the cycle or
leave them where they are but add a brief explanatory comment above those
imports stating why (e.g., "placed here to avoid circular import with X") or
convert them to explicit local imports inside the functions that need them
(e.g., import .detect.should_compress inside the caller) and run tests to verify
behavior.- Around line 81-87: The Anthropic API call currently instantiates
anthropic.Anthropic and calls client.messages.create without an explicit
timeout; update the code that creates the client (the anthropic.Anthropic(...)
instantiation) or the messages.create invocation to pass an explicit timeout
value (e.g., timeout=60 or another appropriate seconds value) so network hangs
are bounded; modify the client creation in the compress logic that calls client
= anthropic.Anthropic(api_key=api_key) and/or the messages.create(...) call to
include the timeout parameter and ensure any exceptions from timeouts are
handled or allowed to propagate consistently.In @.agents/skills/caveman-review/SKILL.md:
- Line 25: The caveman-review skill currently emits praise ("Great work!",
"Looks good overall but...") which conflicts with the "No throat-clearing"
terse-mode rule; update the SKILL.md behavior for the caveman-review skill to
either remove praise entirely or conditionally emit praise only when the mode is
non‑terse (check the mode flag used by caveman-review and gate the praise text
behind it), and update the documentation to state that praise is omitted in
terse mode to avoid non-actionable output (referencing the caveman-review skill
and the terse/non‑terse mode flag).In
@tests/generator/epic-20-caveman-style.test.ts:
- Line 33: The map callback in the assertion using withFiller is using an opaque
namef; rename the callback parameter to a descriptive name (e.g.,
generatedFileorgenerated) so the assertion reads like
expect(withFiller.map((generatedFile) => generatedFile.path)).toEqual([]);
update any similar.map()callbacks in this test that usefto use the new
descriptive name for clarity.In
@tests/utils/caveman-compress.test.ts:
- Around line 44-61: Add regression tests to
tests/utils/caveman-compress.test.ts alongside the existing cavemanCompress
tests to assert exact preservation for non-code markdown types promised by the
docs: create it() cases that pass inputs containing a heading (e.g. "#
Title\n"), a markdown link and raw URL (e.g. "text" and
"https://example.com/path"), a file path or filesystem fragment (e.g.
"src/app/index.ts"), a shell command invocation (e.g. "npm install --save
package"), and a markdown table (pipe-delimited rows); for each case call
cavemanCompress(input) and assert the output contains the exact original
substring and does not trim or alter it (use toContain and not.toMatch where
appropriate).</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `63c46881-1ff9-4ea0-b183-038e69cbbd70` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between a07570fedd2d2ec0f6410830db01a9378ae4c679 and ba614815220b8916041ccba3691c05da34dd2043. </details> <details> <summary>📒 Files selected for processing (35)</summary> * `.agents/skills/caveman-commit/SKILL.md` * `.agents/skills/caveman-compress/README.md` * `.agents/skills/caveman-compress/SECURITY.md` * `.agents/skills/caveman-compress/SKILL.md` * `.agents/skills/caveman-compress/scripts/__init__.py` * `.agents/skills/caveman-compress/scripts/__main__.py` * `.agents/skills/caveman-compress/scripts/benchmark.py` * `.agents/skills/caveman-compress/scripts/cli.py` * `.agents/skills/caveman-compress/scripts/compress.py` * `.agents/skills/caveman-compress/scripts/detect.py` * `.agents/skills/caveman-compress/scripts/validate.py` * `.agents/skills/caveman-help/SKILL.md` * `.agents/skills/caveman-review/SKILL.md` * `.agents/skills/caveman/SKILL.md` * `.agents/skills/compress/SKILL.md` * `.agents/skills/compress/scripts/__init__.py` * `.agents/skills/compress/scripts/__main__.py` * `.agents/skills/compress/scripts/benchmark.py` * `.agents/skills/compress/scripts/cli.py` * `.agents/skills/compress/scripts/compress.py` * `.agents/skills/compress/scripts/detect.py` * `.agents/skills/compress/scripts/validate.py` * `PRD.md` * `skills-lock.json` * `src/cli/init-command.ts` * `src/generator/index.ts` * `src/prompt/ask-caveman-style.ts` * `src/prompt/default-config.ts` * `src/prompt/prompt-flow.ts` * `src/schema/stack-config.ts` * `src/utils/caveman-compress.ts` * `src/utils/index.ts` * `tests/generator/epic-20-caveman-style.test.ts` * `tests/generator/fixtures.ts` * `tests/utils/caveman-compress.test.ts` </details> <details> <summary>✅ Files skipped from review due to trivial changes (5)</summary> * skills-lock.json * src/utils/index.ts * .agents/skills/compress/scripts/__init__.py * .agents/skills/caveman-compress/scripts/__init__.py * .agents/skills/caveman-help/SKILL.md </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (2)</summary> * tests/generator/fixtures.ts * src/prompt/prompt-flow.ts </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
- Updated `PRD.md` to reflect changes in the detection logic for Spring Boot and ASP.NET Core frameworks, improving accuracy in identifying backend technologies. - Refactored `src/detector/detect-dotnet-framework.ts` to return all discovered `.csproj` files instead of just the first, enhancing the detection process. - Introduced new helper functions in `src/constants/frameworks.ts` to classify frameworks as backend or fullstack, ensuring better categorization of detected stacks. - Modified `src/prompt/prompt-flow.ts` to streamline the implementer variant selection process based on user-confirmed stack information. - Enhanced `README.md` and `.claude/settings.json` to clarify the implications of the new implementer variant logic and framework detection improvements. This commit aims to improve the flexibility and accuracy of agent configurations while ensuring comprehensive documentation for users.
- Updated `AGENTS.md` to specify that clarifying questions should be asked until ≥95% confidence is achieved before proceeding with tasks. - Revised `CLAUDE.md` to emphasize the importance of confidence thresholds in planning and implementation steps. - Enhanced `README.md` and various agent templates to incorporate new communication styles, promoting concise and direct messaging. - Improved inline and indented code validation in `validate.py` to ensure accurate preservation of code formatting during compression processes. This commit aims to strengthen agent communication protocols and improve the clarity of documentation for better user guidance.
- Introduced new best-practice partials for agent templates, including `vibe-loop`, `prompt-dna`, `quality-gates`, `pre-release-checklist`, `small-diffs`, `debug-ladder`, and `handoff-format`. - Established a doc-pointer convention by replacing inline documentation in `CLAUDE.md` and `AGENTS.md` with a canonical-docs link table for better scalability and clarity. - Added new commands for `vibe-check` and `ralph-loop`, enhancing agent functionality and documentation practices. - Updated various agent templates to incorporate the new partials and ensure compliance with the vibe-coding-guide schema. This commit aims to enhance coding practices and documentation standards across generated agents, promoting better project consistency and quality assurance.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (1)
.agents/skills/compress/scripts/cli.py (1)
62-64:⚠️ Potential issue | 🟠 MajorFailure message still assumes a single
Falsecause fromcompress_file().This remains ambiguous and can misreport the actual failure reason; please split outcomes via structured status or dedicated exceptions.
#!/bin/bash # Verify all False-return causes from compress skill compress_file() rg -nP --type=py -C4 'def compress_file\(|return False|raise ' .agents/skills/compress/scripts/compress.py rg -nP --type=py -C3 'compress_file\(' .agents/skills/compress/scripts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/compress/scripts/cli.py around lines 62 - 64, The exit message in cli.py currently treats any False from compress_file() as a single generic failure; modify compress_file (in .agents/skills/compress/scripts/compress.py) to return a structured result (e.g., enum/status dict with codes like SKIPPED, RETRYABLE_ERROR, FATAL_ERROR) or raise dedicated exceptions (e.g., CompressRetryableError, CompressFatalError), then update the CLI flow that calls compress_file() to branch on those distinct outcomes (or catch the specific exceptions) and print precise messages and appropriate sys.exit codes instead of the single "Compression failed after retries" line; ensure the CLI uses the specific symbols compress_file, CompressRetryableError/CompressFatalError (or the chosen enum keys) so callers map each outcome to a clear user-facing message and exit code.
🧹 Nitpick comments (8)
tests/generator/epic-16-cross-model-routing.test.ts (1)
132-137: Add the missing WSL assertion forclaude -pto keep handoff coverage symmetric.This block now validates only the WSL-wrapped
codex execpath. GivenCROSS_MODEL_HANDOFF_ALLOWSincludes both commands andbuildSandboxWrapperAllowswraps host Bash permissions intoBash(wsl ...), this test should also assert theclaude -pwrapped form.Suggested test update
it('.claude/settings.json allows scoped wsl-wrapped cross-model handoff commands', () => { const settings = getRootFileContent(files, '.claude/settings.json'); expect(settings).not.toContain('Bash(wsl *)'); expect(settings).toContain('Bash(wsl codex exec:*)'); + expect(settings).toContain('Bash(wsl claude -p:*)'); expect(settings).not.toContain('Bash(wsl * codex exec:*)'); + expect(settings).not.toContain('Bash(wsl * claude -p:*)'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/generator/epic-16-cross-model-routing.test.ts` around lines 132 - 137, The test validates the WSL-wrapped "codex exec" permission but misses the symmetric assertion for the "claude -p" handoff; update the same test (inside the it block that reads settings via getRootFileContent and stores it in settings) to assert that settings contains the WSL-wrapped claude form using the same wrapping pattern as the codex check (e.g. add expect(settings).toContain('Bash(wsl claude -p:*)') so both cross-model handoff commands are covered).src/prompt/questions.ts (1)
7-10: Finish splittingquestions.tsinto single-helper modules.Pulling docs/branch prompts out is a good start, but
src/prompt/questions.tsstill exports four public helpers. Continuing this split would align the module with the repo rule and keep future prompt changes more localized.As per coding guidelines, "
src/**/*.{ts,tsx}: One public component/helper per file".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompt/questions.ts` around lines 7 - 10, The file still exports multiple public helpers — split each public prompt helper into its own module: move askProjectDocumentationFiles and askMainBranch (and the two other exported helpers from src/prompt/questions.ts) into separate files (e.g., ask-project-documentation-files.ts, ask-main-branch.ts, etc.), export a single helper per file, update the original imports/exports to re-export or replace usage sites to import the new single-helper modules, and ensure all references to the original function names (askProjectDocumentationFiles, askMainBranch, plus the other two helper symbols) are updated throughout the codebase.src/installer/safe-delete-stale-files.ts (1)
42-43: Reduce cross-module coupling toGeneratedFilefor backup-only behaviorThis path creates a synthetic
GeneratedFile(content: '') even though backup needs only file paths. Consider a dedicated backup API for paths (or overload) to avoid installer ↔ generator type coupling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/installer/safe-delete-stale-files.ts` around lines 42 - 43, The code creates a synthetic GeneratedFile ({ path: candidate, content: '' }) just to call backupExistingFiles, coupling the installer to generator types; change backupExistingFiles to accept plain paths (either add a new overload/variant like backupExistingFilesFromPaths(projectRoot: string, paths: string[]) or extend backupExistingFiles to accept string[] alongside GeneratedFile[]), then update safe-delete-stale-files.ts to call the path-based API with [candidate] (remove the synthetic GeneratedFile/fileEntry creation) and adjust any call-sites to preserve existing behavior when passing GeneratedFile objects.tests/installer/safe-delete-stale-files.test.ts (1)
41-44: Extract repeated stale-file setup into one helperThe same setup block is repeated across three tests. Extract it once to keep tests easier to maintain and reduce drift.
As per coding guidelines, “Same function + different appearance = extend via props/params, not copy. Any code appearing in 2+ places must be extracted.”
Also applies to: 58-61, 79-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/installer/safe-delete-stale-files.test.ts` around lines 41 - 44, Extract the repeated stale-file setup (mkdir + writeFile for '.claude/agents/...') into a single helper, e.g. createStaleFile(projectRoot, relPath = '.claude/agents/react-ts-senior.md'), then replace the three duplicate blocks (the ones creating join(projectRoot, '.claude/agents') and writing the stale file) with calls to that helper; ensure the helper uses join(projectRoot, relPath), mkdir(..., { recursive: true }) and writeFile(..., 'utf-8') so behavior is identical and update tests that currently inline the setup to call createStaleFile instead.tests/generator/epic-20-caveman-style.test.ts (1)
5-13: Consider centralizing filler phrase definitions.Keeping this phrase list in sync with
src/utils/caveman-compress.tsis manual; exporting a canonical list (or test helper) would reduce drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/generator/epic-20-caveman-style.test.ts` around lines 5 - 13, The test defines a local FILLER_PHRASES array that duplicates the canonical list in src/utils/caveman-compress.ts; to avoid drift, export the canonical list (e.g., export const FILLER_PHRASES) or provide a test helper from that module and import it into tests/generator/epic-20-caveman-style.test.ts, then remove the duplicated array in the test and reference the exported symbol instead (ensure the export name matches the test import).src/cli/init-command.ts (1)
209-227: Avoid hardcodednpxin user-facing commands.These hints should be derived from the configured package-manager runner (or at least prefer
pnpmconsistently), instead of embeddingnpxliterals in multiple places.♻️ Suggested refactor
+ const packageExec = + config.tooling.packageManagerPrefix === 'pnpm' ? 'pnpm dlx' : 'npx'; + - logger.info(' config change → npx agents-workflows update'); + logger.info(` config change → ${packageExec} agents-workflows update`); - logger.info(' codex: npx skills add JuliusBrussee/caveman'); + logger.info(` codex: ${packageExec} skills add JuliusBrussee/caveman`); - logger.info(' cursor: npx skills add JuliusBrussee/caveman -a cursor'); + logger.info(` cursor: ${packageExec} skills add JuliusBrussee/caveman -a cursor`); - logger.info(' windsurf: npx skills add JuliusBrussee/caveman -a windsurf'); + logger.info(` windsurf: ${packageExec} skills add JuliusBrussee/caveman -a windsurf`); - logger.info(' copilot: npx skills add JuliusBrussee/caveman -a copilot'); + logger.info(` copilot: ${packageExec} skills add JuliusBrussee/caveman -a copilot`);Based on learnings: Always use
pnpmfor running scripts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/init-command.ts` around lines 209 - 227, The hardcoded "npx" strings in the user-facing hints (inside the init flow that checks config.cavemanStyle and the config.targets.* branches) should be replaced with the configured package-runner or a consistent default (prefer "pnpm"); update the logger.info messages that reference commands for claude, codexCli, cursor, windsurf, copilot and the earlier "config change → npx agents-workflows update" so they use a packageRunner variable/helper (or fallback to "pnpm") instead of literal "npx"—locate the code paths guarded by config.cavemanStyle and the logger.info calls and emit commands built from that runner.tests/generator/stack-aware-agents.test.ts (2)
28-30: Annotate the Jest callback parameters explicitly.These callbacks still rely on inference for
fixtureandexpected. The repo rule requires explicit parameter types in*.tsfiles, including tests.Example cleanup
- ])('fixture %s produces variant %s', async (fixture, expected) => { + ])('fixture %s produces variant %s', async (fixture: string, expected: ImplementerVariant) => { @@ - it.each(BACKENDS)('backend %s excludes ui-designer.md', async (fixture) => { + it.each(BACKENDS)('backend %s excludes ui-designer.md', async (fixture: string) => { @@ - it.each(FRONTENDS)('frontend %s includes ui-designer.md', async (fixture) => { + it.each(FRONTENDS)('frontend %s includes ui-designer.md', async (fixture: string) => {As per coding guidelines,
**/*.{ts,tsx}: Always add explicit type annotations to function parameters — never rely on implicit inference.Also applies to: 64-69, 82-88, 134-136, 162-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/generator/stack-aware-agents.test.ts` around lines 28 - 30, The test callback currently relies on inferred types for the parameters "fixture" and "expected"; update the Jest test declaration (the callback passed to the parameterized test that calls detectFixture and getApplicableImplementerVariant) to include explicit TypeScript types for both parameters (e.g., annotate "fixture" and "expected" with appropriate types used by detectFixture and getApplicableImplementerVariant). Make the same change for the other test callbacks flagged (lines around the other ranges) so all test callbacks in this file have explicit parameter type annotations.
84-84: Use descriptive names in.map()callbacks.
files.map((f) => f.path)is harder to scan than a descriptive callback name likegeneratedFile.As per coding guidelines,
**/*.{ts,tsx}: Use descriptive variable names in.map()callbacks.Also applies to: 164-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/generator/stack-aware-agents.test.ts` at line 84, Replace terse single-letter callback names in your .map() calls with descriptive identifiers: for example change the callback parameter used when building paths (currently files.map((f) => f.path) which assigns to paths) to a descriptive name like generatedFile or generatedFilesItem; do the same for the other .map() usages referenced around lines 164-176 so each callback parameter clearly conveys its role (e.g., generatedFile, generatedFilePath), improving readability without changing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/caveman-compress/scripts/cli.py:
- Around line 62-64: compress_file() currently returns False for three different
outcomes but the CLI always prints "Compression failed after retries" and exits
2; change compress_file() to return a discriminated result (e.g., an Enum
CompressResult with SKIPPED, SKIPPED_BACKUP_EXISTS, FAILED_RETRIES) or raise
distinct exceptions (e.g., SkippedNonNaturalLanguage, SkippedBackupExists,
FailedRetries), then update the CLI branch that now prints "\n❌ Compression
failed after retries" to inspect the returned enum or catch the new exceptions
and print a specific message and exit code for each case (informational skip ->
no error exit or exit 0, backup exists -> exit with a distinct non-2 code and
descriptive message, failed retries -> keep exit 2 and the failure message);
update all call sites of compress_file() to handle the new return type or
exceptions accordingly.
In @.agents/skills/caveman-compress/scripts/compress.py:
- Line 69: MAX_RETRIES is set to 2 which causes only one actual retry pass
because the first iteration is the initial validation; change MAX_RETRIES to 3
(or otherwise make the loop inclusive) so build_fix_prompt() can be invoked for
two retries; update the constant MAX_RETRIES in compress.py and the same
constant usages referenced around the other loop (the block that checks attempt
== 0 and calls build_fix_prompt()) so the retry flow actually performs the
documented two retries.
- Around line 90-100: The subprocess.run call that invokes ["claude", "--print"]
should resolve "claude" to an absolute executable before spawning to avoid
picking up a shadowed binary; use shutil.which("claude") to find the full path,
validate it is found (raise/log and fail early if not), then call
subprocess.run([claude_path, "--print"], ...) instead of ["claude", "--print"]
(keep existing args: input=prompt, text=True, capture_output=True, check=True,
timeout=120) and return strip_llm_wrapper(result.stdout.strip()) as before.
In @.agents/skills/caveman-compress/scripts/validate.py:
- Around line 118-126: The heading/text/order, path changes, and list-structure
drift checks currently call result.add_warning so they don't mark the output
invalid and thus compress_file() won't retry; change those specific checks
(e.g., validate_headings using extract_headings, the path-validation and
list-structure validation functions referenced around lines 155-173 and 179-191)
to call result.add_error (or otherwise set result.is_valid = False) when they
detect violations so they become errors that trigger retry logic in
compress_file(); keep non-exact-preservation issues as warnings only.
- Line 7: INLINE_CODE_REGEX currently only matches single-backtick spans;
replace it with a regex that captures a run of one-or-more backticks as a
delimiter and requires the same run to close (for example use a pattern like
re.compile(r"(`+)([^`\n]+?)\1")). Update the INLINE_CODE_REGEX definition and
any uses in validate_inline_code() (and the related logic around the previous
INLINE_CODE_REGEX at the other occurrence) so multi-backtick delimiters (``
`...` ``, ```...```) are treated as a single inline-code span.
In `@scripts/capture-implementer-baseline.ts`:
- Around line 13-14: The script currently writes fixtures using cwd-relative
paths; change it to resolve the fixture path from the script file location using
import.meta.url: compute const scriptDir =
dirname(fileURLToPath(import.meta.url)) (import fileURLToPath from 'url' and
dirname/join from 'path'), build the fixture path with join(scriptDir, '..',
'tests', 'generator', '__fixtures__', 'implementer-generic-baseline.md'), ensure
mkdirSync(dirname(fixturePath), { recursive: true }) and then
writeFileSync(fixturePath, rendered, 'utf8') instead of the current cwd-relative
mkdirSync/writeFileSync calls.
In `@src/installer/safe-delete-stale-files.ts`:
- Around line 22-24: Replace unsafe join() usage when building absolutePath from
candidates by normalizing with resolve(); compute const absolutePath =
resolve(projectRoot, candidate) and then get the relative path via
relative(projectRoot, absolutePath); if the relative path startsWith('..') or
startsWith(path.sep) (i.e., escapes projectRoot) skip that candidate and do not
call rm() or perform the backup operation for it; keep references to the same
variables (candidates, projectRoot, absolutePath) and existing rm/backup logic
but gated by this validation.
In `@src/templates/config/CLAUDE.md.ejs`:
- Line 29: Update the table row that currently reads "Code review (after every
file edit) | `code-reviewer` (sonnet)" to match the workflow language: change
the timing to "after every implementation session" (or otherwise mirror the
exact phrase used in the workflow section) so the table and the mandatory
workflow are consistent; ensure the cell still references `code-reviewer`
(sonnet) and, if helpful, add a short note pointing readers to the workflow
paragraph that defines the full review loop (code-reviewer, security-reviewer,
type-check, tests, lint).
- Line 62: The sentence "**Never advance a plan step or begin implementation**
until ≥95% confident about scope and approach" is missing "intent" and should
match the architect prompt and fail-safe wording; update that string in
CLAUDE.md.ejs to read that the agent must be ≥95% confident about scope, intent,
and approach before proceeding, and make the identical change to the other
occurrence(s) referenced (the architect prompt / fail-safe partial gate entries
labeled 99-99) so all gates consistently include "intent".
In `@tests/generator/stack-aware-helpers/detectFixture.ts`:
- Around line 6-8: detectFixture should validate that the constructed fixture
path exists before delegating to detectStack to avoid false-positive detections;
update detectFixture (and the path created with join(FIXTURES_DIR, fixtureName))
to check that the directory exists (e.g., fs.existsSync or fs.stat) and throw a
clear, descriptive Error mentioning the missing fixtureName/path if it does not
exist, then call detectStack as before to return Promise<DetectedStack>.
In `@tests/installer/safe-delete-stale-files.test.ts`:
- Around line 40-55: The test currently verifies deletion but not the backup;
after calling safeDeleteStaleFiles (the function under test) add an assertion
that a backup artifact exists and contains the original contents: search within
projectRoot for any file whose name includes the basename from staleRelPath
(e.g., "react-ts-senior") or whose path differs from staleAbsPath, read that
file and expect its contents to equal 'stale content' (and still assert
MOCK_CONFIRM was not called). This ensures safeDeleteStaleFiles actually wrote a
backup copy of the deleted file.
---
Duplicate comments:
In @.agents/skills/compress/scripts/cli.py:
- Around line 62-64: The exit message in cli.py currently treats any False from
compress_file() as a single generic failure; modify compress_file (in
.agents/skills/compress/scripts/compress.py) to return a structured result
(e.g., enum/status dict with codes like SKIPPED, RETRYABLE_ERROR, FATAL_ERROR)
or raise dedicated exceptions (e.g., CompressRetryableError,
CompressFatalError), then update the CLI flow that calls compress_file() to
branch on those distinct outcomes (or catch the specific exceptions) and print
precise messages and appropriate sys.exit codes instead of the single
"Compression failed after retries" line; ensure the CLI uses the specific
symbols compress_file, CompressRetryableError/CompressFatalError (or the chosen
enum keys) so callers map each outcome to a clear user-facing message and exit
code.
---
Nitpick comments:
In `@src/cli/init-command.ts`:
- Around line 209-227: The hardcoded "npx" strings in the user-facing hints
(inside the init flow that checks config.cavemanStyle and the config.targets.*
branches) should be replaced with the configured package-runner or a consistent
default (prefer "pnpm"); update the logger.info messages that reference commands
for claude, codexCli, cursor, windsurf, copilot and the earlier "config change →
npx agents-workflows update" so they use a packageRunner variable/helper (or
fallback to "pnpm") instead of literal "npx"—locate the code paths guarded by
config.cavemanStyle and the logger.info calls and emit commands built from that
runner.
In `@src/installer/safe-delete-stale-files.ts`:
- Around line 42-43: The code creates a synthetic GeneratedFile ({ path:
candidate, content: '' }) just to call backupExistingFiles, coupling the
installer to generator types; change backupExistingFiles to accept plain paths
(either add a new overload/variant like
backupExistingFilesFromPaths(projectRoot: string, paths: string[]) or extend
backupExistingFiles to accept string[] alongside GeneratedFile[]), then update
safe-delete-stale-files.ts to call the path-based API with [candidate] (remove
the synthetic GeneratedFile/fileEntry creation) and adjust any call-sites to
preserve existing behavior when passing GeneratedFile objects.
In `@src/prompt/questions.ts`:
- Around line 7-10: The file still exports multiple public helpers — split each
public prompt helper into its own module: move askProjectDocumentationFiles and
askMainBranch (and the two other exported helpers from src/prompt/questions.ts)
into separate files (e.g., ask-project-documentation-files.ts,
ask-main-branch.ts, etc.), export a single helper per file, update the original
imports/exports to re-export or replace usage sites to import the new
single-helper modules, and ensure all references to the original function names
(askProjectDocumentationFiles, askMainBranch, plus the other two helper symbols)
are updated throughout the codebase.
In `@tests/generator/epic-16-cross-model-routing.test.ts`:
- Around line 132-137: The test validates the WSL-wrapped "codex exec"
permission but misses the symmetric assertion for the "claude -p" handoff;
update the same test (inside the it block that reads settings via
getRootFileContent and stores it in settings) to assert that settings contains
the WSL-wrapped claude form using the same wrapping pattern as the codex check
(e.g. add expect(settings).toContain('Bash(wsl claude -p:*)') so both
cross-model handoff commands are covered).
In `@tests/generator/epic-20-caveman-style.test.ts`:
- Around line 5-13: The test defines a local FILLER_PHRASES array that
duplicates the canonical list in src/utils/caveman-compress.ts; to avoid drift,
export the canonical list (e.g., export const FILLER_PHRASES) or provide a test
helper from that module and import it into
tests/generator/epic-20-caveman-style.test.ts, then remove the duplicated array
in the test and reference the exported symbol instead (ensure the export name
matches the test import).
In `@tests/generator/stack-aware-agents.test.ts`:
- Around line 28-30: The test callback currently relies on inferred types for
the parameters "fixture" and "expected"; update the Jest test declaration (the
callback passed to the parameterized test that calls detectFixture and
getApplicableImplementerVariant) to include explicit TypeScript types for both
parameters (e.g., annotate "fixture" and "expected" with appropriate types used
by detectFixture and getApplicableImplementerVariant). Make the same change for
the other test callbacks flagged (lines around the other ranges) so all test
callbacks in this file have explicit parameter type annotations.
- Line 84: Replace terse single-letter callback names in your .map() calls with
descriptive identifiers: for example change the callback parameter used when
building paths (currently files.map((f) => f.path) which assigns to paths) to a
descriptive name like generatedFile or generatedFilesItem; do the same for the
other .map() usages referenced around lines 164-176 so each callback parameter
clearly conveys its role (e.g., generatedFile, generatedFilePath), improving
readability without changing behavior.
In `@tests/installer/safe-delete-stale-files.test.ts`:
- Around line 41-44: Extract the repeated stale-file setup (mkdir + writeFile
for '.claude/agents/...') into a single helper, e.g.
createStaleFile(projectRoot, relPath = '.claude/agents/react-ts-senior.md'),
then replace the three duplicate blocks (the ones creating join(projectRoot,
'.claude/agents') and writing the stale file) with calls to that helper; ensure
the helper uses join(projectRoot, relPath), mkdir(..., { recursive: true }) and
writeFile(..., 'utf-8') so behavior is identical and update tests that currently
inline the setup to call createStaleFile instead.
🪄 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: 1b46ccb8-e502-4a11-b636-50888a014d25
📒 Files selected for processing (63)
.agents/skills/caveman-commit/SKILL.md.agents/skills/caveman-compress/README.md.agents/skills/caveman-compress/SKILL.md.agents/skills/caveman-compress/scripts/benchmark.py.agents/skills/caveman-compress/scripts/cli.py.agents/skills/caveman-compress/scripts/compress.py.agents/skills/caveman-compress/scripts/detect.py.agents/skills/caveman-compress/scripts/validate.py.agents/skills/compress/scripts/benchmark.py.agents/skills/compress/scripts/cli.py.agents/skills/compress/scripts/compress.py.agents/skills/compress/scripts/detect.py.agents/skills/compress/scripts/validate.py.claude/agents/architect.md.claude/settings.jsonAGENTS.mdCLAUDE.mdPRD.mdQA.mdREADME.mdscripts/capture-implementer-baseline.tssrc/cli/init-command.tssrc/cli/resolve-update-project-config.tssrc/cli/update-command.tssrc/detector/detect-dotnet-framework.tssrc/generator/build-context.tssrc/generator/implementer-routing.tssrc/generator/index.tssrc/generator/permission-constants.tssrc/generator/permissions.tssrc/installer/safe-delete-stale-files.tssrc/prompt/ask-caveman-style.tssrc/prompt/ask-implementer-variant.tssrc/prompt/index.tssrc/prompt/prompt-flow.tssrc/prompt/questions.tssrc/templates/agents/architect.md.ejssrc/templates/agents/e2e-tester.md.ejssrc/templates/agents/reviewer.md.ejssrc/templates/agents/test-writer.md.ejssrc/templates/agents/ui-designer.md.ejssrc/templates/config/AGENTS.md.ejssrc/templates/config/CLAUDE.md.ejssrc/templates/partials/architect-fail-safe.md.ejstests/fixtures/frontend-svelte/package.jsontests/generator/__fixtures__/implementer-generic-baseline.mdtests/generator/build-context.test.tstests/generator/epic-16-cross-model-routing.test.tstests/generator/epic-20-caveman-style.test.tstests/generator/epic-4-standards.test.tstests/generator/generic-byte-identical.test.tstests/generator/implementer-routing.test.tstests/generator/permissions.test.tstests/generator/security-reviewer.test.tstests/generator/stack-aware-agents.test.tstests/generator/stack-aware-helpers/configForFixture.tstests/generator/stack-aware-helpers/constants.tstests/generator/stack-aware-helpers/detectFixture.tstests/generator/stack-aware-helpers/generateForFixture.tstests/generator/stack-aware-helpers/getImplementerContent.tstests/generator/stack-aware-helpers/index.tstests/installer/safe-delete-stale-files.test.tstests/prompt/project-documentation.test.ts
✅ Files skipped from review due to trivial changes (17)
- src/templates/agents/e2e-tester.md.ejs
- src/templates/agents/test-writer.md.ejs
- src/cli/resolve-update-project-config.ts
- src/templates/partials/architect-fail-safe.md.ejs
- src/templates/agents/ui-designer.md.ejs
- tests/generator/security-reviewer.test.ts
- tests/prompt/project-documentation.test.ts
- QA.md
- AGENTS.md
- tests/fixtures/frontend-svelte/package.json
- CLAUDE.md
- src/templates/agents/reviewer.md.ejs
- tests/generator/fixtures/implementer-generic-baseline.md
- .agents/skills/caveman-commit/SKILL.md
- .agents/skills/compress/scripts/benchmark.py
- README.md
- tests/generator/stack-aware-helpers/index.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- tests/generator/epic-4-standards.test.ts
- src/prompt/ask-implementer-variant.ts
- tests/generator/generic-byte-identical.test.ts
- tests/generator/implementer-routing.test.ts
- src/detector/detect-dotnet-framework.ts
- src/cli/update-command.ts
- src/generator/permission-constants.ts
- src/templates/config/AGENTS.md.ejs
- .agents/skills/compress/scripts/validate.py
- tests/generator/permissions.test.ts
- src/prompt/prompt-flow.ts
- .claude/settings.json
- tests/generator/build-context.test.ts
- src/prompt/ask-caveman-style.ts
- src/generator/permissions.ts
…nter-baseline script - Updated the `capture-implementer-baseline.ts` script to use `dirname` and `join` for constructing file paths, improving path management. - Enhanced error handling in the script to provide clearer error messages when exceptions occur. - Modified the `safe-delete-stale-files.ts` to include checks for absolute paths and symbolic links, ensuring safer file deletion operations. - Improved tests for `safeDeleteStaleFiles` to validate new behavior and ensure robustness against invalid paths. - Updated various test files to include type annotations for better clarity and maintainability. This commit aims to enhance the reliability and clarity of file operations within the project, ensuring better error handling and path management.
- Introduced a new `fetch-plugins` script to automate the retrieval of plugin skill files from specified sources in `plugin-sources.json`. - Updated the build process to ensure the creation of the `dist/plugins` directory and handle missing source directories gracefully. - Added a `generatePlugins` function to manage the generation of plugin skill files based on the configuration. - Implemented a plugin selection prompt to allow users to choose which plugins to install, enhancing user experience. - Updated the stack configuration schema to include a new `plugins` section for better management of plugin states. This commit aims to enhance the plugin system, providing better integration and usability for users managing skills and plugins within the project.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
src/installer/safe-delete-stale-files.ts (1)
30-31: Use canonical relative path for backup entriesYou validate
absolutePath, but still pass rawcandidatetobackupExistingFiles. A traversal-shapedcandidatecan normalize inside root for deletion yet still reshape backup destination viajoin(projectRoot, BACKUP_DIR, file.path). Derive a sanitized relative path fromabsolutePathand use that forGeneratedFile.path.Suggested hardening refactor
-import { resolve, sep, isAbsolute } from 'node:path'; +import { resolve, sep, isAbsolute, relative } from 'node:path'; @@ - const normalizedRoot = resolve(projectRoot) + sep; + const normalizedProjectRoot = resolve(projectRoot); + const normalizedRoot = normalizedProjectRoot + sep; @@ - const absolutePath = resolve(projectRoot, candidate); + const absolutePath = resolve(normalizedProjectRoot, candidate); @@ - const fileEntry: GeneratedFile = { path: candidate, content: '' }; + const safeRelativePath = relative(normalizedProjectRoot, absolutePath); + const fileEntry: GeneratedFile = { path: safeRelativePath, content: '' };Also applies to: 56-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/installer/safe-delete-stale-files.ts` around lines 30 - 31, Validate absolutePath using resolve/normalizedRoot, then compute a sanitized relative path (e.g., const rel = normalize(relative(projectRoot, absolutePath)).replace(/^(\.\.(\/|\\|$))+/, '') or strip leading path separators) and use that rel when constructing GeneratedFile.path passed into backupExistingFiles instead of the original candidate; update both places where candidate is passed (the check using normalizedRoot and the backupExistingFiles calls) to ensure backup destination is derived from the canonical absolutePath, not the raw candidate, and keep references to projectRoot and BACKUP_DIR when joining the sanitized relative path.scripts/fetch-plugin-skills.ts (1)
20-23: Use object parameters for helpers with more than 2 arguments.Both
buildRawUrlandwriteSkillFileexceed the max positional-parameter rule.As per coding guidelines: "Functions with more than 2 parameters must use a single object parameter".Suggested fix
-function buildRawUrl(source: string, basePath: string | undefined, skillPath: string): string { - const base = basePath ? `${basePath}/` : ''; - return `https://raw.githubusercontent.com/${source}/HEAD/${base}${skillPath}`; -} +interface BuildRawUrlInput { + source: string; + basePath?: string; + skillPath: string; +} +function buildRawUrl({ source, basePath, skillPath }: BuildRawUrlInput): string { + const base = basePath ? `${basePath}/` : ''; + return `https://raw.githubusercontent.com/${source}/HEAD/${base}${skillPath}`; +} -async function writeSkillFile(pluginId: string, skillId: string, content: string): Promise<string> { - const skillDir = join(PLUGINS_DIR, pluginId, skillId); +interface WriteSkillFileInput { + pluginId: string; + skillId: string; + content: string; +} +async function writeSkillFile({ pluginId, skillId, content }: WriteSkillFileInput): Promise<string> { + const skillDir = join(PLUGINS_DIR, pluginId, skillId); await mkdir(skillDir, { recursive: true }); const filePath = join(skillDir, 'SKILL.md'); await writeFile(filePath, content, 'utf-8'); return createHash('sha256').update(content).digest('hex'); }Also applies to: 33-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/fetch-plugin-skills.ts` around lines 20 - 23, buildRawUrl and writeSkillFile currently use more than two positional parameters; change each to accept a single object parameter (e.g., function buildRawUrl({ source, basePath, skillPath }: { source: string; basePath?: string; skillPath: string }) and similarly for writeSkillFile) and update all call sites to pass a single object with those named properties; ensure TypeScript types/interfaces are added/adjusted for the parameter object and any existing usages (including the caller(s) referenced around lines 33-39) are updated to use destructured named args so the functions comply with the "single object parameter" rule.tests/generator/generate-plugins.test.ts (1)
61-61: Use a descriptive.map()callback variable name.Rename
fto a descriptive name (for examplegeneratedFile) for readability and consistency.As per coding guidelines:
**/*.{ts,tsx}: Use descriptive variable names in.map()callbacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/generator/generate-plugins.test.ts` at line 61, Rename the anonymous callback variable `f` in the files.map call that assigns to `paths` to a descriptive name like `generatedFile` (or `fileItem`) to improve readability; update the expression `const paths = files.map((f) => f.path);` to use the new identifier (e.g., `const paths = files.map((generatedFile) => generatedFile.path);`) ensuring all references within that callback use the new name.src/prompt/prompt-flow.ts (1)
20-21: ExposeaskCavemanStylevia the prompt barrel for consistent module boundaries.
askCavemanStyleis imported directly from its file while other prompt entry points are treated as shared prompt APIs. Please export it fromsrc/prompt/index.tsand consume it consistently through the module surface.As per coding guidelines:
src/**/*.{ts,tsx}: Use folder-based module organization with colocated tests andindex.tsbarrel exports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompt/prompt-flow.ts` around lines 20 - 21, The file imports askCavemanStyle directly instead of through the prompt barrel; add an export for askCavemanStyle in src/prompt/index.ts (e.g., export { askCavemanStyle } from './ask-caveman-style';), then update prompt-flow.ts to import askCavemanStyle from the prompt barrel (import { askCavemanStyle, askPluginSelection } from './'; or './index') so all prompt entry points are consumed via the shared module surface; ensure any tests or other callers use the barrel export too.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/fetch-plugin-skills.ts`:
- Around line 20-23: The buildRawUrl function currently uses the moving ref
"HEAD" which makes fetched plugin artifacts non-reproducible; change buildRawUrl
(and its callers) to accept an explicit git ref parameter (e.g., refOrSha)
instead of hardcoding "HEAD", and construct the URL as
`https://raw.githubusercontent.com/${source}/${refOrSha}/${base}${skillPath}`;
ensure callers supply a stable tag or commit SHA (or fail if none provided) so
fetched content is pinned and reproducible (refer to the buildRawUrl function
and its parameters source, basePath, skillPath).
In `@src/generator/generate-plugins.ts`:
- Around line 25-30: The current check-then-read creates a TOCTOU race; replace
the fileExists(...) then readFile(...) pattern by calling readFile(skillFile,
'utf-8') inside a try/catch, and on error inspect err.code === 'ENOENT' to emit
the same console.warn using plugin.id and skill.id and continue; for any other
error rethrow or propagate so generation still fails on unexpected I/O
issues—remove the prior fileExists branch and keep the files.push({ path:
`.claude/skills/${skill.id}/SKILL.md`, content }) only after successful
readFile.
In `@src/installer/safe-delete-stale-files.ts`:
- Around line 36-37: Replace the blanket catch on lstat so only ENOENT is
ignored: call lstat(absolutePath) and if it throws check err.code === 'ENOENT'
then continue, otherwise rethrow or propagate the error so it hits the existing
warning logic; update the references around lstat, absolutePath and stat in
safe-delete-stale-files.ts to ensure non-ENOENT errors are not swallowed and
will go to the existing warning path (lines that produce the warning for
problematic files).
In `@src/prompt/ask-plugin-selection.ts`:
- Around line 5-9: The .map callback used to build PLUGIN_CHOICES currently uses
an implicitly typed parameter `plugin`; update the callback to include an
explicit parameter type annotation (e.g., `(plugin: PluginDef) => { ... }`) so
it conforms to the project's TS rule. Locate the PLUGIN_CHOICES declaration that
maps over PLUGIN_REGISTRY and add the explicit type for `plugin` (reference
symbols: PLUGIN_CHOICES, PLUGIN_REGISTRY, plugin, PluginDef) without changing
the mapping logic or returned shape.
In `@tests/generator/fixtures.ts`:
- Line 81: Update the test fixture so its default matches the schema: change the
implementerVariant value in the baseAgents object (typed as
StackConfig['agents']) from 'react-ts' to 'generic' so tests using
makeStackConfig() without overrides reflect the schema default for
implementerVariant.
In `@tests/generator/generate-plugins.test.ts`:
- Line 25: Replace the incorrect parameter-based cast by giving
mockResolvedValue an explicit return type that matches readFile: change the
mockReadFile call to use an explicit generic type of Awaited<ReturnType<typeof
readFile>> on mockResolvedValue so the resolved value '# SKILL content' is typed
as the actual readFile return type (reference mockReadFile, readFile, and
mockResolvedValue to locate the call).
---
Nitpick comments:
In `@scripts/fetch-plugin-skills.ts`:
- Around line 20-23: buildRawUrl and writeSkillFile currently use more than two
positional parameters; change each to accept a single object parameter (e.g.,
function buildRawUrl({ source, basePath, skillPath }: { source: string;
basePath?: string; skillPath: string }) and similarly for writeSkillFile) and
update all call sites to pass a single object with those named properties;
ensure TypeScript types/interfaces are added/adjusted for the parameter object
and any existing usages (including the caller(s) referenced around lines 33-39)
are updated to use destructured named args so the functions comply with the
"single object parameter" rule.
In `@src/installer/safe-delete-stale-files.ts`:
- Around line 30-31: Validate absolutePath using resolve/normalizedRoot, then
compute a sanitized relative path (e.g., const rel =
normalize(relative(projectRoot, absolutePath)).replace(/^(\.\.(\/|\\|$))+/, '')
or strip leading path separators) and use that rel when constructing
GeneratedFile.path passed into backupExistingFiles instead of the original
candidate; update both places where candidate is passed (the check using
normalizedRoot and the backupExistingFiles calls) to ensure backup destination
is derived from the canonical absolutePath, not the raw candidate, and keep
references to projectRoot and BACKUP_DIR when joining the sanitized relative
path.
In `@src/prompt/prompt-flow.ts`:
- Around line 20-21: The file imports askCavemanStyle directly instead of
through the prompt barrel; add an export for askCavemanStyle in
src/prompt/index.ts (e.g., export { askCavemanStyle } from
'./ask-caveman-style';), then update prompt-flow.ts to import askCavemanStyle
from the prompt barrel (import { askCavemanStyle, askPluginSelection } from
'./'; or './index') so all prompt entry points are consumed via the shared
module surface; ensure any tests or other callers use the barrel export too.
In `@tests/generator/generate-plugins.test.ts`:
- Line 61: Rename the anonymous callback variable `f` in the files.map call that
assigns to `paths` to a descriptive name like `generatedFile` (or `fileItem`) to
improve readability; update the expression `const paths = files.map((f) =>
f.path);` to use the new identifier (e.g., `const paths =
files.map((generatedFile) => generatedFile.path);`) ensuring all references
within that callback use the new name.
🪄 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: 5c34c601-9923-4e10-819a-64dd4925fee7
📒 Files selected for processing (22)
package.jsonplugin-sources.jsonscripts/build.mjsscripts/capture-implementer-baseline.tsscripts/fetch-plugin-skills.tssrc/cli/init-command.tssrc/generator/generate-plugins.tssrc/generator/index.tssrc/generator/plugin-registry.tssrc/installer/safe-delete-stale-files.tssrc/prompt/ask-plugin-selection.tssrc/prompt/default-config.tssrc/prompt/index.tssrc/prompt/prompt-flow.tssrc/schema/stack-config.tssrc/templates/config/CLAUDE.md.ejstests/generator/epic-16-cross-model-routing.test.tstests/generator/fixtures.tstests/generator/generate-plugins.test.tstests/generator/stack-aware-agents.test.tstests/generator/stack-aware-helpers/detectFixture.tstests/installer/safe-delete-stale-files.test.ts
✅ Files skipped from review due to trivial changes (3)
- package.json
- plugin-sources.json
- scripts/capture-implementer-baseline.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/generator/index.ts
- tests/generator/epic-16-cross-model-routing.test.ts
- tests/generator/stack-aware-helpers/detectFixture.ts
- src/cli/init-command.ts
- src/prompt/default-config.ts
- tests/installer/safe-delete-stale-files.test.ts
- src/prompt/index.ts
- Added `refOrSha` fields to `plugin-sources.json` for better version control of plugins. - Refactored the `fetch-plugin-skills.ts` script to include validation for `refOrSha` and improve error handling. - Enhanced the `writeSkillFile` function to ensure safe file operations and prevent overwriting symlinks. - Updated tests to cover new functionality and ensure robustness in plugin management. This commit aims to improve the reliability and clarity of the plugin fetching process, ensuring that the correct versions of plugins are utilized and managed effectively.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/installer/safe-delete-stale-files.test.ts (1)
18-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert against the hashed backup artifact, not the source-relative path.
buildBackupFileName()duplicates the production filename contract, and thejoin(..., '.claude/agents/react-ts-senior.md')check can never observe the file this implementation writes. That means this negative test can still pass if a backup is incorrectly created elsewhere under.agents-workflows-backup. Prefer scanning the backup directory and asserting it stays empty here, or sharing the filename helper instead of copying it.As per coding guidelines, same function + different appearance = extend via props/params, not copy; any code appearing in 2+ places must be extracted.
Also applies to: 66-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/installer/safe-delete-stale-files.test.ts` around lines 18 - 22, The test currently asserts against the source-relative path instead of the produced backup artifact; update the test to either (A) call or import the existing buildBackupFileName function (rather than duplicating it) and assert that join(backupDir, buildBackupFileName(relativePath)) does not exist, or (B) scan the backup directory (e.g., list files under the .agents-workflows-backup directory) and assert the directory is empty after deletion; remove the duplicated buildBackupFileName implementation in the test and reuse the shared helper so the assertion targets the actual hashed backup filename created by the production code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/installer/safe-delete-stale-files.test.ts`:
- Around line 18-22: The test currently asserts against the source-relative path
instead of the produced backup artifact; update the test to either (A) call or
import the existing buildBackupFileName function (rather than duplicating it)
and assert that join(backupDir, buildBackupFileName(relativePath)) does not
exist, or (B) scan the backup directory (e.g., list files under the
.agents-workflows-backup directory) and assert the directory is empty after
deletion; remove the duplicated buildBackupFileName implementation in the test
and reuse the shared helper so the assertion targets the actual hashed backup
filename created by the production code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5914686a-3990-4da9-a5b7-45b3c8613655
📒 Files selected for processing (10)
plugin-sources.jsonscripts/fetch-plugin-skills.tssrc/generator/generate-plugins.tssrc/installer/safe-delete-stale-files.tssrc/prompt/ask-plugin-selection.tssrc/prompt/index.tstests/generator/fixtures.tstests/generator/generate-plugins.test.tstests/installer/safe-delete-stale-files.test.tstests/schema/implementer-variants-migration.test.ts
✅ Files skipped from review due to trivial changes (2)
- plugin-sources.json
- tests/schema/implementer-variants-migration.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- scripts/fetch-plugin-skills.ts
- src/prompt/ask-plugin-selection.ts
- src/generator/generate-plugins.ts
- src/prompt/index.ts
- tests/generator/generate-plugins.test.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 by CodeRabbit
New Features
Improvements