add local dir and git private template sources#21
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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 { | ||
| execFileSync('tar', ['-xz'], { input: tarball, cwd: tmpDir, stdio: ['pipe', 'ignore', 'pipe'] }); |
There was a problem hiding this comment.
WARNING: tar -xz runs without --no-absolute-names and without first stripping/validating entries. A tarball from codeload.github.com (or a future authenticated provider) that contains ../ or absolute paths could write files outside tmpDir. fs.cpSync downstream then re-emits those into the project's src/. Recommend piping through tar -tz first to reject any entry with .. or starting with /, or use Node's tar library with strict mode.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| function defaultGitExec(args) { | ||
| return new Promise((resolve, reject) => { | ||
| execFile('git', args, { |
There was a problem hiding this comment.
WARNING: defaultGitExec does not set a timeout on execFile. A hung SSH connection (e.g., firewall that drops but never RSTs, or a slow remote) will block campaign-init indefinitely — the spinner will spin forever and the user has no way to cancel. Add a generous timeout (e.g., 60s for shallow clone) and surface a UPSTREAM_FETCH_FAILED with a git clone timed out message on rejection.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return `https://raw.githubusercontent.com/${this.source.repo}/${this.source.ref}/${rel}`; | ||
| } | ||
| _tarballUrl() { | ||
| return `https://codeload.github.com/${this.source.repo}/tar.gz/refs/heads/${this.source.ref}`; |
There was a problem hiding this comment.
WARNING: _tarballUrl() hard-codes refs/heads/${ref}. The companion raw URL (_rawUrl) just uses ref directly, which works for branches, tags, and SHAs. If a user adds a github source via _data/template-sources.json with ref: "v1.2.0" (a tag), readJson will work (raw.githubusercontent supports tags) but materialize will 404 because codeload's tarball endpoint for tags is /tar.gz/refs/tags/<ref>. Detect tag refs (no slash) and switch the path segment, or fetch the archive via the API's tarball_url from a release.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // relative paths resolve against ctx.brandRoot (the project root). | ||
| function resolveLocalRoot(source, ctx) { | ||
| let p = source.path || ''; | ||
| if (p === '~' || p.startsWith('~/')) p = path.join(os.homedir(), p.slice(1)); |
There was a problem hiding this comment.
SUGGESTION: ~user/... style paths (another user's home) are silently treated as a relative path against brandRoot. Either reject them with a clear error ('~user' expansion is not supported) or use child_process.execFile('sh', ['-c', 'echo ~user']) to expand. At minimum, fail loudly when the expanded form would be a path that doesn't exist (which the current code does, but the user-facing message will say "local source path not found: ~bob/templates" which is confusing).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const logger = require('../logger'); | ||
| const { | ||
| AI_CONTEXT_DOC_PATH, | ||
| fetchBuffer, |
There was a problem hiding this comment.
SUGGESTION: fetchBuffer is imported from ../template-source but no longer used in this file (callers go through provider.readJson/provider.readText). Remove it to keep the import surface lean and the git clone / https.get ownership explicit in template-source.js.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| AI_CONTEXT_DOC_PATH, | ||
| fetchBuffer, | ||
| countFiles, | ||
| copySubtree, |
There was a problem hiding this comment.
SUGGESTION: copySubtree is imported from ../template-source but no longer referenced in this file. Remove it from the import.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| selectableTemplates, | ||
| createProvider, | ||
| loadSources, | ||
| resolveSource, |
There was a problem hiding this comment.
SUGGESTION: resolveSource is imported from ../template-source but never called in this file. Remove it from the import.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 7 Issues Found (4 resolved in this commit) | Recommendation: Address remaining issues before merge Overview
Incremental commit
|
| Previous issue | Fix location | Status |
|---|---|---|
~user paths silently treated as relative |
lib/template-source.js:149-153 — new else if throws INVALID_INPUT before path.resolve |
✅ RESOLVED |
defaultGitExec no timeout |
lib/template-source.js:195 — timeout: GIT_CLONE_TIMEOUT_MS added; new killed branch at :180-182 maps it to a clear timeout error |
✅ RESOLVED |
_tarballUrl() hard-codes refs/heads/${ref} |
lib/template-source.js:222 — now uses bare ${ref}, so tags/SHAs resolve; matches _rawUrl |
✅ RESOLVED |
Placeholder references adsbranded-templates |
lib/actions/init.js:489 — now campaign-cart-starter-templates |
✅ RESOLVED |
Tests added (test/template-source.test.js) cover the ~user reject, killed/timeout mapping, and the new bare-ref tarball URL.
Remaining issues (carried forward — not fixed by this commit)
Issue Details (click to expand)
WARNING (2)
| File | Line | Issue |
|---|---|---|
lib/template-source.js |
113 | tar -xz runs without --no-absolute-names and without first stripping entries — a malicious upstream tarball can write outside tmpDir and the result is then copied into the user's project by fs.cpSync. Unchanged. |
lib/actions/init.js |
937 | The new else if (!nonInteractive && existingAiContext) branch fires and logs "AI context already present — skipping" when the user explicitly opted out with --ai-context none, silently overwriting the opt-out. Unchanged. |
SUGGESTION (5)
| File | Line | Issue |
|---|---|---|
lib/actions/init.js |
931 | existingAiContext does a first-match over AI_CONTEXT_TARGETS. A pre-existing AGENTS.md (codex) silently skips the prompt even when the user wants to install a different tool like CLAUDE.md. Unchanged. |
lib/template-source.js |
60 | Added a third positional get parameter to fetchBuffer purely as a test seam. An options bag would avoid coupling the recursive call to the positional contract. Unchanged. |
README.md |
145 | Text says the catalog is "an array of entries" but the example shows { "templates": [...] }. The Field table also omits the required top-level templates key. Unchanged. |
lib/actions/init.js |
121-130 | (unchanged code) In replaceDirectoryWithRollback, the commit callback writes _data/campaigns.json after the staged dir is renamed into place. If the write throws mid-stream the rollback restores the dir from backup but campaigns.json may be left truncated. Write the JSON to a tmp file and rename-into-place inside commit for atomicity. |
lib/actions/init.js |
624-626 | (unchanged code) mergeScripts + writeFileSync of package.json happens before the --source resolution. If the user cancels at the source picker, package.json has already been mutated. Move source resolution earlier (before any writes) or back up the original for rollback. |
Other Observations (not in diff)
Issues found in unchanged code that cannot receive inline comments:
| File | Line | Issue |
|---|---|---|
lib/actions/init.js |
742 | (unchanged code) Picker message was changed from "Pick a starter template" → "Pick a template"; inconsistent with the help text (init.js:302, :313), the file header in template-source.js:4, and the README (README.md:181, :540) which still say "starter template" |
Files Reviewed (5 files, incremental diff only)
lib/actions/init.js— 1 changed line (placeholder repo); fix verifiedlib/template-source.js— 4 changed regions (~user reject, timeout, killed mapping, tarball bare ref); all fixes verifiedtest/template-source.test.js— 3 new tests added; assertions match the new behaviorREADME.md— no changes in this incremental commit (the schema wording issue is carried forward)test/init.test.js— no changes in this incremental commit
No new issues were introduced by bf6b815.
Fix these issues in Kilo Cloud
Previous Review Summaries (3 snapshots, latest commit d9c30af)
Current summary above is authoritative. Previous snapshots are kept for context only.
Previous review (commit d9c30af)
Status: 11 Issues Found | Recommendation: Address before merge
Overview
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| WARNING | 4 |
| SUGGESTION | 7 |
Issue Details (click to expand)
WARNING
| File | Line | Issue |
|---|---|---|
lib/template-source.js |
112 | tar -xz runs without --no-absolute-names and without first stripping entries — a malicious upstream tarball can write outside tmpDir and the result is then copied into the user's project by fs.cpSync |
lib/template-source.js |
181 | defaultGitExec does not set a timeout on execFile. A hung SSH connection will block campaign-init indefinitely (no abort, no spinner cancellation) |
lib/template-source.js |
207 | _tarballUrl() hard-codes refs/heads/${ref}. The companion raw URL works for any ref, so a user-specified tag works for readJson/readText but materialize will 404 |
lib/actions/init.js |
937 | NEW: the new else if (!nonInteractive && existingAiContext) branch fires and logs "AI context already present — skipping" when the user explicitly opted out with --ai-context none, silently overwriting the opt-out |
SUGGESTION
| File | Line | Issue |
|---|---|---|
lib/template-source.js |
146 | ~user/... style paths (another user's home) are silently treated as relative — no shell expansion; the resulting "local source path not found" error is confusing for users |
lib/actions/init.js |
489 | Placeholder references a foreign repo adsbranded-templates that has no relationship to this project. The actual public repo is NextCommerceCo/campaign-cart-starter-templates |
lib/actions/init.js |
931 | NEW: existingAiContext does a first-match over AI_CONTEXT_TARGETS. A pre-existing AGENTS.md (codex) silently skips the prompt even when the user wants to install a different tool like CLAUDE.md |
lib/template-source.js |
59 | NEW: added a third positional get parameter to fetchBuffer purely as a test seam. An options bag would avoid coupling the recursive call to the positional contract |
README.md |
145 | NEW: text says the catalog is "an array of entries" but the example shows { "templates": [...] }. The Field table also omits the required top-level templates key |
lib/actions/init.js |
121-130 | (unchanged code) In replaceDirectoryWithRollback, the commit callback writes _data/campaigns.json after the staged dir is renamed into place. If the write throws mid-stream the rollback restores the dir from backup but campaigns.json may be left truncated. Write the JSON to a tmp file and rename-into-place inside commit for atomicity |
lib/actions/init.js |
624-626 | (unchanged code) mergeScripts + writeFileSync of package.json happens before the --source resolution. If the user cancels at the source picker, package.json has already been mutated. Move source resolution earlier (before any writes) or back up the original for rollback |
Other Observations (not in diff)
Issues found in unchanged code that cannot receive inline comments:
| File | Line | Issue |
|---|---|---|
lib/actions/init.js |
742 | (unchanged code) Picker message was changed from "Pick a starter template" → "Pick a template"; inconsistent with the help text (init.js:302, :313), the file header in template-source.js:4, and the README (README.md:181, :540) which still say "starter template" |
Files Reviewed (5 files)
lib/actions/init.js- 4 issues (1 new)lib/template-source.js- 4 issues (1 new)README.md- 1 new issuetest/init.test.js- 0 issuestest/template-source.test.js- 0 issues (new defensive-branch tests added are fine)
(Incremental commit d9c30af "improvement" lands three things: (1) it removes the dead fetchBuffer / copySubtree / resolveSource imports from init.js — closing 3 prior SUGGESTIONs, (2) it adds the get injection seam and renames PublicProvider → GithubProvider in template-source.js — same body, no behavior change beyond the seam, and (3) it adds the new existingAiContext early-skip branch in init.js, which unfortunately introduces a real --ai-context none regression (WARNING above) and a first-match-collision issue (SUGGESTION above). The README schema docs added in this commit are a useful addition but the array-vs-object wording is wrong (SUGGESTION above).)
Fix these issues in Kilo Cloud
Previous review (commit 87ac262)
Status: 8 Issues Found | Recommendation: Address before merge
Overview
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| WARNING | 3 |
| SUGGESTION | 5 |
Issue Details (click to expand)
WARNING
| File | Line | Issue |
|---|---|---|
lib/template-source.js |
115 | tar -xz invoked without --no-absolute-names or entry validation; a malicious upstream tarball can write outside tmpDir and get re-emitted into the project by fs.cpSync |
lib/template-source.js |
184 | defaultGitExec has no timeout — a hung SSH connection will block campaign-init indefinitely |
lib/template-source.js |
209 | _tarballUrl() hard-codes refs/heads/; tag refs work for readJson but 404 on materialize |
SUGGESTION
| File | Line | Issue |
|---|---|---|
lib/template-source.js |
149 | ~user/... paths silently treated as relative (no shell expansion); user-facing error is confusing |
lib/actions/init.js |
8 | Dead import: fetchBuffer |
lib/actions/init.js |
10 | Dead import: copySubtree |
lib/actions/init.js |
15 | Dead import: resolveSource |
lib/actions/init.js |
492 | NEW: placeholder path adsbranded-templates references a foreign/unrelated org; the actual public repo is NextCommerceCo/campaign-cart-starter-templates |
Other Observations (not in diff)
Issues found in unchanged code that cannot receive inline comments:
| File | Line | Issue |
|---|---|---|
lib/actions/init.js |
121-130 | In replaceDirectoryWithRollback, the commit callback runs after the staged dir is renamed into place. If commit throws mid-write to _data/campaigns.json, the rollback will rmSync(destDir) and restore from backup — but the campaigns.json may be left partially written/truncated. Consider writing campaigns.json to a tmp file and rename-into-place inside commit, so it's atomic with the dir swap. |
lib/actions/init.js |
624-626 | mergeScripts then writeFileSync of package.json happens before the --source resolution. If the user cancels at the source picker, package.json has already been mutated. Either move source resolution earlier (before any writes) or persist the original for rollback. |
lib/actions/init.js |
742 | Picker message was changed from "Pick a starter template" → "Pick a template"; inconsistent with the help text (init.js:302, :313), the file header in template-source.js:4, and the README (README.md:181, :540) which still say "starter template". |
Files Reviewed (5 files)
lib/actions/init.js- 4 issueslib/template-source.js- 4 issuesREADME.md- 0 issuestest/init.test.js- 0 issuestest/template-source.test.js- 0 issues
(Incremental commit 87ac262 "improve tui messaging and readme" only touches copy: README reformat, the local path prompt/placeholder, the picker message, and the built-in source label. Of these, the placeholder reference to a foreign org is the only new issue; the "Pick a template" rename creates a minor terminology drift with the rest of the docs/help text.)
Fix these issues in Kilo Cloud
Previous review (commit cd4152a)
Status: 7 Issues Found | Recommendation: Address before merge
Overview
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| WARNING | 3 |
| SUGGESTION | 4 |
Issue Details (click to expand)
WARNING
| File | Line | Issue |
|---|---|---|
lib/template-source.js |
115 | tar -xz invoked without --no-absolute-names or entry validation; a malicious upstream tarball can write outside tmpDir and get re-emitted into the project by fs.cpSync |
lib/template-source.js |
184 | defaultGitExec has no timeout — a hung SSH connection will block campaign-init indefinitely |
lib/template-source.js |
209 | _tarballUrl() hard-codes refs/heads/; tag refs work for readJson but 404 on materialize |
SUGGESTION
| File | Line | Issue |
|---|---|---|
lib/template-source.js |
149 | ~user/... paths silently treated as relative (no shell expansion); user-facing error is confusing |
lib/actions/init.js |
8 | Dead import: fetchBuffer |
lib/actions/init.js |
10 | Dead import: copySubtree |
lib/actions/init.js |
15 | Dead import: resolveSource |
Other Observations (not in diff)
Issues found in unchanged code that cannot receive inline comments:
| File | Line | Issue |
|---|---|---|
lib/actions/init.js |
121-130 | In replaceDirectoryWithRollback, the commit callback runs after the staged dir is renamed into place. If commit throws mid-write to _data/campaigns.json, the rollback will rmSync(destDir) and restore from backup — but the campaigns.json may be left partially written/truncated. Consider writing campaigns.json to a tmp file and rename-into-place inside commit, so it's atomic with the dir swap. |
lib/actions/init.js |
624-626 | mergeScripts then writeFileSync of package.json happens before the --source resolution. If the user cancels at the source picker, package.json has already been mutated. Either move source resolution earlier (before any writes) or persist the original for rollback. |
Files Reviewed (5 files)
lib/actions/init.js- 3 issueslib/template-source.js- 4 issuesREADME.md- 0 issuestest/init.test.js- 0 issuestest/template-source.test.js- 0 issues
(Deletions of the bundled opentui skill docs and skills-lock.json entry were inspected; they are housekeeping, not code, so no review issues.)
Reviewed by minimax-m3 · 206,143 tokens
| if (type === 'local') { | ||
| const p = await ui.text({ | ||
| message: `Path to the templates directory ${D}(absolute, ~, or relative to this project; must contain templates.json + src/)${R}`, | ||
| placeholder: '/home/you/git/adsbranded-templates', |
There was a problem hiding this comment.
SUGGESTION: Placeholder references a foreign repo adsbranded-templates that has no relation to this project (the default source is NextCommerceCo/campaign-cart-starter-templates).
| placeholder: '/home/you/git/adsbranded-templates', | |
| placeholder: '/home/you/git/campaign-cart-starter-templates', |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| .find(p => fs.existsSync(path.join(brandRoot, p))); | ||
| if (flags.aiContext && flags.aiContext !== 'none') { | ||
| aiContextTools.push(flags.aiContext); | ||
| } else if (!nonInteractive && existingAiContext) { |
There was a problem hiding this comment.
WARNING: The new else if (!nonInteractive && existingAiContext) branch fires whenever any known AI-context target exists on disk — including when the user has explicitly opted out with --ai-context none. In that case the first if (flags.aiContext && flags.aiContext !== 'none') is skipped, the new branch then logs a misleading "AI context already present — skipping" and silently overwrites the user's explicit opt-out. Add && !flags.aiContext (or && flags.aiContext !== 'none') to the condition so an explicit none is respected.
| } else if (!nonInteractive && existingAiContext) { | |
| } else if (!nonInteractive && !flags.aiContext && existingAiContext) { |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // AI context is repo-wide setup (CLAUDE.md etc. live at the brand root), not | ||
| // per-campaign — so if any known target already exists, don't re-prompt on | ||
| // later campaign-init runs. An explicit --ai-context still writes/refreshes. | ||
| const existingAiContext = Object.values(AI_CONTEXT_TARGETS) |
There was a problem hiding this comment.
SUGGESTION: existingAiContext uses Object.values(AI_CONTEXT_TARGETS).filter(Boolean).find(...) — a first match wins. The order in AI_CONTEXT_TARGETS is claude, codex, cursor, copilot, so a project with a pre-existing .cursor/rules/campaign-page-kit.mdc (or a hand-written AGENTS.md) will silently skip the prompt even when the user is running interactive init to install a different tool (e.g. CLAUDE.md). Either: (a) still ask but pre-select the detected one, (b) only auto-skip when existingAiContext was created by campaign-init (use the AI_CONTEXT_SENTINEL marker), or (c) at minimum, list all matched paths in the log line so the user knows which tool's file is being detected.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| // `get` is injectable (defaults to https.get) so the redirect/status/error | ||
| // branches can be unit-tested without real network. | ||
| function fetchBuffer(url, redirects = 5, get = https.get) { |
There was a problem hiding this comment.
SUGGESTION: Adding a third positional get parameter to fetchBuffer purely as a test seam couples the recursive redirect call (fetchBuffer(loc, redirects - 1, get)) to that positional contract. An options bag — fetchBuffer(url, { redirects = 5, get = https.get } = {}) — would (a) be forward-compatible if more injectable seams are added later, (b) make call sites self-documenting, and (c) keep the recursive call simple. Minor API smell but worth fixing now before external callers depend on the positional form.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| └── … | ||
| ``` | ||
|
|
||
| `templates.json` is the catalog the picker reads — an array of entries, one per template: |
There was a problem hiding this comment.
SUGGESTION: This line says the catalog is "an array of entries", but the immediately-following example shows an object with a templates key wrapping the array ({ "templates": [ ... ] }), and that is what the code in template-source.js (manifest.templates in selectableTemplates) actually expects. The Field table below also silently omits the required top-level templates key. Reword to "an object whose templates array contains one entry per template" and add a templates row to the table.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
1 participant
No description provided.