overhaul campaign-init setup flow with a starter template picker#12
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| fs.mkdirSync(parent, { recursive: true }); | ||
| const tmpDir = fs.mkdtempSync(path.join(parent, '.cpk-extract-')); | ||
| try { | ||
| execSync('tar -xz', { input: tarball, cwd: tmpDir, stdio: ['pipe', 'ignore', 'pipe'] }); |
There was a problem hiding this comment.
WARNING: tar command flags behave differently on GNU tar vs BSD tar (macOS). The -z flag works on Linux but some BSD tars expect -xzf with stdin. Consider using a more portable approach or at minimum testing on macOS.
Additionally, if the tarball is malformed or empty, execSync will throw with an unclear error message. Consider adding explicit error handling around this call.
| })), | ||
| }); | ||
| if (isCancel(slug)) { outro('Cancelled.'); return; } | ||
| const picked = templates.find(t => t.slug === slug); |
There was a problem hiding this comment.
CRITICAL: If templates.find() returns undefined (slug not found in manifest), picked will be undefined. Line 196-197 then accesses picked.name which will throw TypeError: Cannot read property 'name' of undefined.
This can happen if the templates.json manifest changes between fetch and user selection.
| // Apply an API key to a config.js source string. Returns the new string, | ||
| // or null if no `apiKey:` field was found. | ||
| function applyApiKey(configSource, apiKey) { | ||
| const updated = configSource.replace(/apiKey:\s*['"].*?['"]/, `apiKey: '${apiKey.trim()}'`); |
There was a problem hiding this comment.
WARNING: The regex /apiKey:\s*['"].*?['"]/ with non-greedy .*? only matches the shortest possible string between quotes. If apiKey values themselves could contain embedded quotes (unlikely for API keys but possible in testing), the replacement could be incorrect. The regex also assumes single-line values. Consider using a more robust approach if API keys might contain special characters.
|
|
||
| function fetchBuffer(url, redirects = 5) { | ||
| return new Promise((resolve, reject) => { | ||
| https.get(url, (res) => { |
There was a problem hiding this comment.
WARNING: No timeout is set on the HTTPS request. If the remote server is unreachable or hangs, fetchBuffer will hang indefinitely with no way to cancel. Consider adding an AbortSignal with a timeout:
https.get(url, { signal: AbortSignal.timeout(30000) }, (res) => { ... }| const repoRoot = fs.readdirSync(tmpDir).find(d => | ||
| fs.statSync(path.join(tmpDir, d)).isDirectory() | ||
| ); | ||
| if (!repoRoot) throw new Error('tarball had no top-level directory'); |
There was a problem hiding this comment.
WARNING: If the tarball's top-level contains multiple directories (e.g., 'fixture-repo-main' and some other folder), fs.readdirSync(...).find() takes the first one alphabetically, which may not be the intended repo root. This could cause extraction to fail silently or extract the wrong directory.
|
|
||
| // Produce the next state of `_data/campaigns.json` after merging an upstream | ||
| // entry under `localSlug` and overriding the name with what the user provided. | ||
| function mergeCampaignEntry(localCampaigns, localSlug, upstreamEntry, name) { |
There was a problem hiding this comment.
SUGGESTION: mergeCampaignEntry copies ALL properties from upstreamEntry. If the upstream campaigns.json contains internal-only fields (e.g., hidden: true, deprecated: true, or local paths), they would be merged into the local registry. Consider explicitly listing which fields should be copied instead of spreading everything.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review SummaryStatus: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING| File | Line | Issue | Changes Since Last Review3-line UX improvement: Added descriptive hints to prompt messages for campaign name, slug, and API key. No functional changes. No new issues introduced. All 6 previous issues remain outstanding. Files Reviewed (2 files)
Note: The changed lines (240, 253, 331) are UX improvements adding helper text to prompts. No new issues found. Fix these issues in Kilo Cloud Reviewed by minimax-m2.7 · 107,641 tokens |
1 participant
Overhaul the campaign-init function to pull a list of starter templates and streamline setting up a new template from a starter template.