Conversation
…nteractive prompt steps
…t to --agentConfig
|
Cursor Agent can help with this pull request. Just |
|
@cursoragent please /review using ai/commands/review |
|
Now I have all the context needed for a complete review. Let me produce it: 🔬 Code Review — PR #143: Aidd Prompt Epic1. Churn AnalysisTop hotspots in the diff cross-referenced against the churn leaderboard:
New files ( 2. Code Structure and Organization🎯 Is the new code well-organized? ✅ Good:
3. Coding Standards and Best Practices🎯 Does the code follow the JavaScript guide? ✅ Good:
❌ Issue — return preset;
getAgentConfig('claude') === getAgentConfig('claude') // true
getAgentConfig('claude').args === getAgentConfig('claude').args // true — same arrayAny caller that mutates ❌ Issue — handleAgentErrors({
AgentConfigParseError: ({ message }) => {
console.error(chalk.red(`\n❌ Agent config error: ${message}`));
},
AgentConfigReadError: ({ message }) => {
console.error(chalk.red(`\n❌ Agent config error: ${message}`));
},
AgentConfigValidationError: ({ message }) => {
console.error(chalk.red(`\n❌ Agent config error: ${message}`));
},
})(/** @type {Error} */ (err));All three handlers have identical bodies. The JS guide says DRY. A single shared handler or a lookup map would suffice. Minor, but real redundancy. 4. Test Coverage and Quality🎯 Are tests thorough, minimal, and following TDD patterns? ✅ Good:
❌ Issue — The test file covers ❌ Issue — The existing 5. Performance❌ Issue — const agentCfg = await resolveAgentConfig({
cwd: folder,
value: agentConfig,
});
await runAgent({
agentConfig: agentCfg,
cwd: folder,
prompt: step.prompt,
});For a manifest with N prompt steps, 6. Security (OWASP Top 10 Scan)
❌ Security-relevant comments deleted from The original - // Use JSON_SCHEMA to restrict parsing to plain JSON types (strings, numbers,
- // booleans, null, arrays, objects). This prevents YAML-specific extensions
- // like !!binary or !!timestamp from being accepted in untrusted manifests.- // No steps key — treat as an empty manifest (backward-compatible default).- // Validate that steps is an array. A string or plain object means the YAML
- // was written incorrectly and would silently iterate unexpected values.Per the JS guide: "Comments should stand-alone months or years later. Assume that the reader is not familiar with the task plan or epic." The 7. UI/UXN/A — no UI changes in this diff. CLI UX is clean: 8. Architecture✅ Good:
❌ Bug — This is the most critical issue in the PR. return new Promise((resolve, reject) => {
const child = spawn(command, spawnArgs, {
cwd,
shell: false,
stdio: "inherit",
});
child.on("close", (code) => {
if (code !== 0) {
reject(
createError({
...ScaffoldStepError,
message: `Agent exited with code ${code}: ${[command, ...spawnArgs].join(" ")}`,
}),
);
} else {
resolve();
}
});
});When The correct pattern is already in 9. Documentation and CommitsCommits: All 8 commits follow conventional commit format ( Docs: Docblocks: Public function signatures in 10. Requirements Checklist vs. EpicCross-referencing
Summary of Issues (ranked by severity)
All 7 issues have delegation prompts ready above. Issues 1–4, 6–7 have already been queued; issue 5 (DRY handlers in   |
…t shared preset mutation
…ss crash on ENOENT
…s crash on ENOENT
…once per prompt step
… mutation aliasing
|
@cursoragent please /review with ai/commands/review - validate all the issues mentioned in these comments and check to see if they are fixed, and look deeper for any other lingering issues. |
|
All checks done. Now writing the full review: 🔬 Code Review — PR #143 Follow-Up: Validation of All Previous Issues + Deep ScanIssue Validation — All 7 Previous Findings1. 🔴 The child.on("error", (spawnErr) => {
reject(
createError({
...ScaffoldStepError,
cause: spawnErr,
message: `Failed to spawn agent: ${[command, ...spawnArgs].join(" ")}`,
}),
);
});The companion ENOENT test is in 2. 🟠 The lazy pre-check pattern correctly resolves once before the loop, and never calls when no prompt steps exist: const hasPromptStep = steps.some((s) => s.prompt !== undefined);
const agentCfg = hasPromptStep
? await resolveAgentConfig({ cwd: folder, value: agentConfig })
: undefined;The new test in 3. 🟠 Returns a defensive copy: return { args: [...preset.args], command: preset.command };Two new tests confirm: distinct object identity per call, and mutation isolation. ✅ 4. 🟡 Security comments deleted from All three comments restored to exact original positions: // Use JSON_SCHEMA to restrict parsing to plain JSON types (strings, numbers,
// booleans, null, arrays, objects). This prevents YAML-specific extensions
// like !!binary or !!timestamp from being accepted in untrusted manifests.
const data = yaml.load(content, { schema: yaml.JSON_SCHEMA });
const steps = /** @type {any} */ (data)?.steps;
// No steps key — treat as an empty manifest (backward-compatible default).
if (steps === undefined || steps === null) return [];
// Validate that steps is an array. A string or plain object means the YAML
// was written incorrectly and would silently iterate unexpected values.
if (!Array.isArray(steps)) {The two stale 5. 🟡 DRY: identical Shared handler extracted: /** @type {(error: { message: string }) => void} */
const logAgentConfigError = ({ message }) =>
console.error(chalk.red(`\n❌ Agent config error: ${message}`));All three branches now reference 6. 🟡 7. 🟡 Four new tests added following the exact Deeper Scan — New Findings🔴 New Bug: The epic explicitly requires:
And | `agent-config` | `claude` | Agent used for prompt manifest steps and `npx aidd agent`. Accepted values: `claude` \| `opencode` \| `cursor` \| a path to a `.yml` agent config file \| an inline `{ command, args }` object. |However the implementation in const agentConfigValue = /** @type {string} */ (parsed["agent-config"]);
if (isYamlPath(agentConfigValue)) return loadYamlConfig(agentConfigValue);
return getAgentConfig(agentConfigValue);When a user writes this valid YAML in their agent-config:
command: myagent
args:
- --run
The Two agents raced to fix
The code is correct in its final state; lint, typecheck, and all 442 tests pass. The history is noisier than necessary but can't be cleaned up without a force-push. No code change needed. ℹ️ Minor: agentConfig: /** @type {NonNullable<typeof agentCfg>} */ (agentCfg),The logic is correct: Summary
One new actionable bug remains. Delegation prompt: |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR adds “agent delegation” capabilities to the aidd scaffolding flow by introducing an agent CLI subsystem (config resolution + spawn runner + CLI subcommand) and integrating prompt: steps into manifest execution with new validation rules.
Changes:
- Added
lib/agent-cli/*modules for agent config resolution, error types, a genericrunAgentspawn primitive, and a newaidd agent --promptCLI command. - Updated scaffold manifest parsing/execution to validate
prompt:ordering and to execute prompt steps viaresolveAgentConfig+runAgent. - Updated
aidd createto use--agentConfig(renamed from--agent) and added--promptfor post-scaffold agent execution; updated docs/tests accordingly.
Reviewed changes
Copilot reviewed 21 out of 23 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tasks/aidd-create-prompt-epic.md | Updates epic requirements for agent config/runner behavior and manifest execution expectations. |
| package.json | Adds new public subpath exports for aidd/agent and aidd/agent-config. |
| lib/scaffold-runner.js | Enforces prompt: ordering relative to an “aidd run” step; runs prompt steps via runAgent with resolved agent config. |
| lib/scaffold-runner.test.js | Updates parsing/execution tests for new ordering rule and runner/config integration. |
| lib/scaffold-verifier.test.js | Updates manifest validity tests to reflect the new ordering constraint. |
| lib/scaffold-create.js | Adds prompt flow after scaffolding and wires in agent config/runner dependencies. |
| lib/scaffold-create.test.js | Adds tests covering --prompt behavior and renamed agentConfig plumbing. |
| lib/scaffold-commands.js | Renames --agent to --agentConfig and adds --prompt to create. |
| lib/scaffold-commands.test.js | Updates CLI option assertions for --agentConfig. |
| lib/cli-core.js | Updates aidd-custom/config.yml template comments with the new agent-config option. |
| lib/cli-core.test.js | Tests presence of the commented agent-config example line. |
| lib/agent-cli/config.js | Implements agent preset lookup + YAML loading + env/config-file based resolution. |
| lib/agent-cli/config.test.js | Comprehensive tests for presets, YAML loading, env/config-file resolution. |
| lib/agent-cli/runner.js | Implements runAgent spawn wrapper with ScaffoldStepError mapping. |
| lib/agent-cli/runner.test.js | Tests spawn shape, exit-code failures, and ENOENT error handling. |
| lib/agent-cli/errors.js | Adds agent-config error causes and an exhaustive dispatcher helper. |
| lib/agent-cli/errors.test.js | Tests error-cause shapes and exhaustive dispatch behavior. |
| lib/agent-cli/command.js | Adds aidd agent --prompt command wiring to config resolution and runner. |
| lib/agent-cli/command.test.js | Tests CLI behavior, including config error formatting and exit handling. |
| lib/exports.test.js | Adds tests asserting the new aidd/agent and aidd/agent-config exports. |
| docs/aidd-custom.md | Documents the new agent-config setting in aidd-custom/config.yml. |
| bin/create-e2e.test.js | Updates create e2e coverage for --agentConfig and new manifest ordering rule. |
| bin/aidd.js | Registers the new agent subcommand. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…Windows compatibility
…e merge-base approach
… back to common names
…from parent package.json
… outside type:module ancestors
…nc handlers in valueKindHandlers
Updated the release date for version 3.0.0 and added new CLI features.
There was a problem hiding this comment.
Pull request overview
This PR adds first-class “agent delegation” to the aidd CLI and runtime: a new aidd agent command, --prompt support on aidd create, agent-config resolution/export APIs, and manifest validation/runtime updates so prompt: steps only run after an aidd-installing run: step.
Changes:
- Introduces
lib/agent-cli/*modules (config resolution, runner, CLI command) and exports them viapackage.jsonasaidd/agentandaidd/agent-config. - Updates scaffold execution to (1) enforce prompt ordering at parse time and (2) execute prompt steps via
runAgentwith resolved agent config. - Expands tests/docs and updates the bundled
next-shadcnscaffold to satisfy the new ordering rule.
Reviewed changes
Copilot reviewed 40 out of 44 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tasks/archive/2026-03-19-scaffold-review-followups-epic.md | Marks archived epic as completed |
| tasks/aidd-create-prompt-epic.md | Updates epic requirements/spec notes |
| package.json | Adds ./agent and ./agent-config subpath exports |
| lib/scaffold-verifier.test.js | Updates fixtures to satisfy prompt ordering rule |
| lib/scaffold-runner.test.js | Adds ordering-guard tests; runs prompt steps via agent config |
| lib/scaffold-runner.js | Adds prompt ordering guard + integrates agent config + runner |
| lib/scaffold-resolver.test.js | Moves tests to real fs state vs injected exists fn |
| lib/scaffold-resolver.js | Removes injectable existsFn; clarifies named scaffold resolution |
| lib/scaffold-create.test.js | Refactors create tests to integration style + prompt support + cleanup |
| lib/scaffold-create.js | Adds --prompt flow + agent config resolution + always-cleanup behavior |
| lib/scaffold-create-args.test.js | New isolated tests for resolveCreateArgs |
| lib/scaffold-commands.test.js | Removes outdated CLI surface tests for old --agent option |
| lib/scaffold-commands.js | Renames --agent → --agent-config; adds --prompt |
| lib/exports.test.js | Verifies new package exports and type entries |
| lib/churn-collector.test.js | Adds e2e git-based churn tests (PR-aware + default-branch aware) |
| lib/churn-collector.js | Adds default-branch detection and PR diff “+1” counting |
| lib/aidd-custom/setup.test.js | Ensures config template includes commented agent-config example |
| lib/aidd-custom/setup.js | Adds agent-config template comments to config.yml scaffold |
| lib/agent-cli/runner.test.js | Tests runAgent spawn behavior and error mapping |
| lib/agent-cli/runner.js | Implements no-shell agent process spawning + error handling |
| lib/agent-cli/runner.d.ts | Adds TS types for aidd/agent |
| lib/agent-cli/get-agent-config.test.js | Tests preset mapping + immutability behavior |
| lib/agent-cli/errors.test.js | Tests agent-config error cause types + handler dispatch |
| lib/agent-cli/errors.js | Defines typed agent-config error causes + handler |
| lib/agent-cli/config.test.js | Tests agent config resolution chain + warnings/fallbacks |
| lib/agent-cli/config.js | Implements preset map + resolution chain + YAML loading |
| lib/agent-cli/config.d.ts | Adds TS types for aidd/agent-config |
| lib/agent-cli/command.test.js | Tests agent subcommand registration |
| lib/agent-cli/command.js | Implements npx aidd agent --prompt ... command |
| docs/scaffold-authoring.md | Documents ordering rule + “prefer run over prompt” guidance |
| docs/aidd-custom.md | Documents agent-config option |
| docs/agent-cli.md | New docs for agent CLI usage + resolution chain |
| docs/agent-api.md | New docs for programmatic agent APIs |
| bin/create-e2e.test.js | Adds e2e coverage for --agent-config and aidd agent |
| bin/aidd.js | Registers the new agent subcommand |
| ai/skills/aidd-timing-safe-compare/SKILL.md | Refines review rules around hashed comparisons |
| ai/skills/aidd-tdd/SKILL.md | Adds “mocking is a code smell” constraint |
| ai/skills/aidd-churn/SKILL.md | Updates churn analysis constraints |
| ai/scaffolds/next-shadcn/stash.mjs | Adds stash helper to temporarily empty scaffold dir |
| ai/scaffolds/next-shadcn/restore.mjs | Adds restore helper for stashed scaffold files |
| ai/scaffolds/next-shadcn/SCAFFOLD-MANIFEST.yml | Replaces placeholder prompt with real run/prompt flow + ordering compliance |
| ai/scaffolds/next-shadcn/README.md | Documents named scaffolds + ordering rule |
| README.md | Adds quickstart examples + mentions new agent exports |
| CHANGELOG.md | Records new features + breaking changes for 3.0.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Match only genuine aidd CLI invocations: | ||
| // - via a package runner: `npx aidd`, `bunx aidd`, `yarn dlx aidd`, `pnpm dlx aidd` | ||
| // - flags (e.g. `-y`, `--yes`) between the runner name and `aidd` are allowed | ||
| // - or as the first token of the command string: `aidd .` | ||
| // Rejects false positives like `echo aidd` (argument) or `npm install aidd-something` | ||
| // (hyphenated package name). | ||
| const AIDD_INVOCATION_RE = | ||
| /(?:(?:^|\s)(?:npx|bunx|yarn(?:\s+dlx)?|pnpm(?:\s+dlx)?)\s+(?:-\S+\s+)*aidd|^aidd)(?![-\w])/; |
There was a problem hiding this comment.
AIDD_INVOCATION_RE is intended to match only genuine aidd CLI invocations, but the current pattern will also match substrings inside other commands because it allows (?:^|\s) before the runner token (e.g. run: echo npx aidd . would be treated as an invocation). It also permits yarn aidd / pnpm aidd because (?:\s+dlx)? is optional, which conflicts with the comment (“yarn dlx” / “pnpm dlx”). Consider anchoring to start-of-command (allowing leading whitespace) and requiring the correct runner forms (e.g. yarn\s+dlx, pnpm\s+dlx) to avoid false positives that would bypass the prompt ordering guard.
| if ( | ||
| !parsed || | ||
| typeof parsed !== "object" || | ||
| !("command" in parsed) || | ||
| !parsed.command | ||
| ) { | ||
| throw createError({ | ||
| ...AgentConfigValidationError, | ||
| message: "Agent config must contain a 'command' field", | ||
| }); | ||
| } | ||
|
|
||
| return /** @type {{ command: string, args?: string[] }} */ (parsed); |
There was a problem hiding this comment.
Agent configs loaded from YAML (and inline objects) are only validated for a truthy command field, but not for type correctness. With js-yaml JSON schema, command could be a number/boolean and args could be a string/object; runAgent would then call spawn(command, [...args, prompt]) and can throw a synchronous TypeError or spread a string into characters. Please validate that command is a non-empty string and that args (if present) is an array of strings, otherwise throw AgentConfigValidationError so callers get a consistent, user-friendly error instead of runtime crashes.
| const resolveAgentConfig = async ({ value, cwd = process.cwd() } = {}) => { | ||
| if (value !== undefined) return resolveExplicitValue(value); | ||
| const envValue = process.env.AIDD_AGENT_CONFIG; | ||
| if (envValue) return resolveExplicitValue(envValue); | ||
| const configFile = path.join(cwd, "aidd-custom", "config.yml"); | ||
| return ( | ||
| (await resolveFromConfigFile(configFile, cwd)) ?? getAgentConfig("claude") | ||
| ); |
There was a problem hiding this comment.
resolveAgentConfig ignores the provided cwd whenever an explicit value is passed (if (value !== undefined) return resolveExplicitValue(value)). This means relative YAML paths supplied via --agent-config ./agent.yml (or programmatic value: './agent.yml') are resolved relative to process.cwd() instead of the caller-provided cwd (notably, runManifest/runCreate pass cwd: folder). If cwd is meant to control relative path resolution, route explicit string YAML paths through the same resolver used for config-file entries (or resolve the path against cwd before calling loadYamlConfig).
| const display = [command, ...spawnArgs].join(" "); | ||
| const child = spawn(command, spawnArgs, { | ||
| cwd, | ||
| shell: false, | ||
| stdio: "inherit", | ||
| }); | ||
| child.on("close", (code, signal) => { | ||
| if (signal != null) { | ||
| reject( | ||
| createError({ | ||
| ...ScaffoldStepError, | ||
| message: `Agent killed by signal ${signal}: ${display}`, | ||
| }), | ||
| ); | ||
| } else if (code !== 0) { | ||
| reject( | ||
| createError({ | ||
| ...ScaffoldStepError, | ||
| message: `Agent exited with code ${code}: ${display}`, | ||
| }), |
There was a problem hiding this comment.
runAgent includes the full prompt text in display and then echoes it in error messages (non-zero exit, spawn failure, signal). Prompts may contain secrets (tokens, credentials, private code excerpts), so this can leak sensitive data into CI logs/terminal history. Consider omitting the prompt from the displayed command, or truncating/redacting it (e.g. show first N chars and length) while still logging the agent command/args for debugging.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| import { spawn } from "child_process"; | ||
| import { createError } from "error-causes"; | ||
|
|
||
| import { ScaffoldStepError } from "../scaffold-errors.js"; |
There was a problem hiding this comment.
Public API runAgent uses scaffold-specific error type
Low Severity
runAgent is exported as the public aidd/agent API for third-party tools, but all its errors use ScaffoldStepError (code SCAFFOLD_STEP_ERROR). Consumers using runAgent outside of scaffolding (e.g., in their own CLI tools) will receive errors with a misleading scaffold-specific error code, making error handling and diagnostics confusing.
This PR implements the "aidd-create-prompt-epic," introducing comprehensive agent delegation capabilities to the
aiddframework.Key Changes:
New CLI surface
npx aidd agent --prompt "<text>"— new subcommand that delegates a prompt to an AI agent (claude, opencode, or cursor) directly from the terminal--prompt <text>onnpx aidd create— after scaffolding completes, runs an AI agent in the new project directory to kick off autonomous development--agent-config <name|path>(renamed from--agent) — configures which AI agent to use; accepts a preset name, YAML config path, or falls through to env var / config file / claude defaultAIDD_AGENT_CONFIGenv var →agent-configinaidd-custom/config.yml→ claudeAgent runtime modules (
lib/agent-cli/)errors.js—AgentConfigReadError,AgentConfigParseError,AgentConfigValidationErrorconfig.js—getAgentConfig(preset map) +resolveAgentConfig(full resolution chain)runner.js—runAgentno-shell spawn with E2BIG, ENOBUFS, signal, and ENOENT handlingcommand.js—registerAgentCommandwired intobin/aidd.jsProgrammatic exports
aidd/agent→runAgentwith full TypeScript typesaidd/agent-config→getAgentConfig/resolveAgentConfigwith full TypeScript typesManifest integration
parseManifestprompt-ordering guard — aprompt:step must follow arun:step that invokes the aidd CLIrunManifestuses the agent resolution chain for prompt steps (non-interactive, no shell injection)Quality improvements (from review)
ScaffoldDestinationError— dedicated error type with clear UX message when destination folder exists~/.aidd/scaffold/in afinallyblock afternpx aidd create <url>(success or failure)agent-configdocumented indocs/aidd-custom.mdand pre-populated as a commented example in the config template