Fix page-kit fresh repo build friction#22
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}
| function cleanOutputPath(outputPath) { | ||
| const resolved = path.resolve(outputPath); | ||
| const root = path.resolve(process.cwd()); | ||
| const volumeRoot = path.parse(resolved).root; |
There was a problem hiding this comment.
CRITICAL: cleanOutputPath safety guard is bypassable.
The check only refuses paths that resolve to cwd or the volume root. It does not prevent:
- Relative escapes:
path.resolve(cwd, '..')returns the parent directory, which is neithercwdnor/, sofs.rmSync(..., { recursive: true, force: true })will recursively delete it. - Arbitrary absolute paths: a programmatic caller passing
outputPath: '/home/user/important'(or/tmp/somewhere) passes both checks and is recursively deleted.
Today the CLI never passes an outputPath, but build({ clean: true, outputPath }) is part of the public engine API (see export at line 229), so any plugin or future caller can trigger this. Recommend requiring outputPath to be a child of cwd, e.g. if (!resolved.startsWith(root + path.sep)) throw ....
| return { data: {}, content: text }; | ||
| } | ||
|
|
||
| const closingIndex = lines.findIndex((line, index) => index > 0 && line.trim() === '---'); |
There was a problem hiding this comment.
WARNING: Silently swallows unclosed frontmatter.
When a page starts with --- but has no closing ---, this returns { data: {}, content: text } with no warning. The previous gray-matter parser threw an error here, which surfaced as a per-page FRONTMATTER_ERROR in the build summary (and stderr). Pages with a stray opening fence now render with empty frontmatter (missing title/page_type/etc.) and only get caught downstream as MISSING_FRONTMATTER warnings, which are non-fatal — broken pages ship to _site/. Consider throwing here, or at minimum logging a FRONTMATTER_ERROR via logger.warn.
|
|
||
| return { | ||
| data, | ||
| content: lines.slice(closingIndex + 1).join('\n'), |
There was a problem hiding this comment.
WARNING: Subtle behavior change vs. gray-matter — leading newline after the closing --- is dropped.
gray-matter returns content as the substring after --- with its leading \n preserved. This implementation uses lines.slice(closingIndex + 1) which throws away the closing-fence line and the newline that followed it, so a file like ---\ntitle: x\n---\n<p>body</p> now produces content: '<p>body</p>' instead of '\n<p>body</p>'. Most Liquid templates won't care, but templates that rely on a leading blank line (e.g. ones wrapped in {% raw %}\n...) will render differently. Easy fix: lines.slice(closingIndex).join('\n').replace(/^---\n/, '---\n') or similar to preserve the boundary.
| const info = (msg) => console.log(`${TAG} ${BRAND}INFO\x1b[0m ${msg}`); | ||
| const warn = (msg) => console.warn(`${TAG} \x1b[33mWARN\x1b[0m ${msg}`); | ||
| const error = (msg) => console.error(`${TAG} \x1b[31mERROR\x1b[0m ${msg}`); | ||
| let debugEnabled = true; |
There was a problem hiding this comment.
WARNING: Default debugEnabled = true contradicts the PR's "quiet by default" intent.
This module exports debug for use by other commands (dev.js, future CLIs) and tests. They never call setDebugEnabled(false), so they keep the noisy DEBUG-on-stderr behavior the PR is trying to remove from campaign-build. The behavior is only correct here because lib/actions/build.js:37 happens to flip it before any debug call. Recommend defaulting to false and explicitly opting in to debug at any call site that needs it.
| const clean = !args.includes('--no-clean'); | ||
| const verbose = args.includes('--verbose'); | ||
|
|
||
| logger.setDebugEnabled(verbose); |
There was a problem hiding this comment.
WARNING: setDebugEnabled mutates module-scoped state shared by every consumer of lib/logger.js.
If anything in the same process — a plugin, an embedded build() call, a test, or a future dev/hot-reload command — loads the logger after campaign-build runs, it inherits whatever verbosity the last command picked. Worse, since the module default is true, the leak is one-directional (cannot accidentally silence a future caller that wanted debug). Consider scoping the flag to the build call (pass verbose into build(), gate logger.debug on a per-call flag) or instantiating a fresh logger inside lib/actions/build.js.
| const args = process.argv.slice(2); | ||
|
|
||
| if (args.includes('--help') || args.includes('-h')) { | ||
| process.stdout.write(HELP_TEXT); |
There was a problem hiding this comment.
SUGGESTION: HELP_TEXT has no trailing newline.
Most CLI tools (e.g. node --help, man) emit a final \n so the next shell prompt doesn't glue onto the last line of help. Append \n before the closing backtick.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Incremental Review (8d336ff..7c0fb01)The PR's actual diff against New findings in this PR's diff:
Verified resolved in this PR's diff (no action needed):
Not in this PR's diff (carried forward — moot for merge)The previous review summary listed carried-forward issues on
Files Reviewed (5 files in PR diff)
Fix these issues in Kilo Cloud Previous Review Summaries (5 snapshots, latest commit 8d336ff)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 8d336ff)Status: No New Issues Found | Recommendation: Merge Overview
Incremental Review (68ca066..HEAD)The two follow-up commits (
New behavior matches the renamed tests: by default Carried Forward (on unchanged files — not in this PR's diff)These remain open from earlier reviews but live outside the current incremental diff; flagging them again here for visibility only:
Files Reviewed (4 files in incremental diff)
Previous review (commit 68ca066)Status: 3 Issues Found | Recommendation: Address before merge Overview
Incremental Review (4bcd623..68ca066)The latest commit adds a CRITICAL
WARNING
Carried Forward (from previous reviews on unchanged files)These remain on files outside the incremental diff (
Files Reviewed (2 files in incremental diff)
Fix these issues in Kilo Cloud Previous review (commit 4bcd623)Status: No Issues Found | Recommendation: Merge Overview
Both previously flagged incremental issues are now resolved in commit
The two test additions also exercise the per-call opt-out ( Files Reviewed (3 files in incremental diff)
Fix these issues in Kilo Cloud Previous review (commit 8c1838b)Status: 1 Issue Found (Incremental) | Recommendation: Address before merge Overview
Previously Reported Issues — StatusAll 6 issues from the prior review at
Incremental Findings (new commits since
|
| File | Line | Issue |
|---|---|---|
lib/frontmatter.js |
29 | null slips past the object type check. yaml.load('null') returns null (typeof object, not an array), so ---\nnull\n---\n<body> returns data: null and crashes the renderer on frontmatter.title / frontmatter.page_layout access in lib/engine/build.js. |
Additional Issues Observed in Incremental Diff (summary-only — affected lines not in this PR's diff)
| File | Line | Issue |
|---|---|---|
lib/actions/compress.js |
219 | logger.debug(\${skippedCount} image… already fully compressed, skipped`)is now permanently silenced. The newlogger.debug(msg, { enabled })API requires the caller to pass{ enabled: true }, but compresshas no--verboseplumbing. The change inlib/logger.js:9-11` silently broke this existing call site — users no longer see skip diagnostics. |
lib/actions/clone.js |
41 | Same regression: logger.debug(\updated permalinks in ${entry.name}`)no longer prints.clonealso has no--verbose` flag, so this message is dead. |
lib/logger.js |
9-11 | API design concern: the new debug signature silently breaks every existing caller. Either default opts.enabled to a module-level flag (e.g. from CPK_DEBUG env or a new setDebugEnabled) for backwards compatibility, or update every existing call site in the same PR. |
Files Reviewed (7 files)
README.md- 0 issues (cleaned up; no new findings)lib/actions/build.js- 0 issues (previous issues resolved)lib/engine/build.js- 0 issues in diff (previous CRITICAL resolved; newverboseplumbing correct)lib/frontmatter.js- 1 issue (new WARNING:nullnot rejected by object-type check)lib/logger.js- 0 issues in diff (previous WARNING resolved), but API change regresses 2 other call sites (see summary)lib/actions/compress.js- 1 issue (unchanged line; regression from logger API change)lib/actions/clone.js- 1 issue (unchanged line; regression from logger API change)test/build.test.js- 0 issues (good coverage for new frontmatter and cleanOutputPath behavior)
Fix these issues in Kilo Cloud
Previous review (commit 73e9da8)
Status: 6 Issues Found | Recommendation: Address before merge
Overview
| Severity | Count |
|---|---|
| CRITICAL | 1 |
| WARNING | 4 |
| SUGGESTION | 1 |
Issue Details (click to expand)
CRITICAL
| File | Line | Issue |
|---|---|---|
lib/engine/build.js |
165 | cleanOutputPath safety guard only blocks cwd and volume root; bypassable via .. traversal or arbitrary absolute path passed through the public build({ outputPath, clean }) API. |
WARNING
| File | Line | Issue |
|---|---|---|
lib/frontmatter.js |
11 | Unclosed frontmatter (--- with no closing fence) silently returns empty data instead of erroring like gray-matter did — broken pages ship to _site/ as warnings only. |
lib/frontmatter.js |
25 | Drops the leading \n after the closing ---, a subtle behavior change vs. gray-matter that may shift whitespace in Liquid templates. |
lib/logger.js |
7 | debugEnabled = true default contradicts the PR's "quiet by default" goal; only correct because lib/actions/build.js flips it early. Library consumers and tests still get DEBUG output. |
lib/actions/build.js |
37 | setDebugEnabled mutates module-scoped logger state shared across all consumers — leaks verbosity into plugins, dev server, and tests loaded after campaign-build. |
SUGGESTION
| File | Line | Issue |
|---|---|---|
lib/actions/build.js |
26 | HELP_TEXT has no trailing \n; shell prompt glues onto the last line. |
Files Reviewed (7 files)
README.md- 0 issueslib/actions/build.js- 2 issueslib/actions/dev.js- 0 issues (caller reviewed for behavioral impact)lib/engine/build.js- 1 issuelib/frontmatter.js- 2 issueslib/logger.js- 1 issuetest/build.test.js- 0 issuespackage.json/package-lock.json- 0 issues (lock file churn fromgray-matter→js-yamlswap)
Reviewed by minimax-m3 · Input: 75.7K · Output: 17.1K · Cached: 1.8M
| const source = text.slice(firstLine.next, closing.start).replace(/\r?\n$/, ''); | ||
| const data = source.trim() ? yaml.load(source) : {}; | ||
|
|
||
| if (typeof data !== 'object' || Array.isArray(data)) { |
There was a problem hiding this comment.
WARNING: The null-object type check is incomplete — yaml.load('null') returns null, which passes both typeof data !== 'object' (false) and Array.isArray(data) (false), so a frontmatter block of ---\nnull\n---\n<body> returns data: null. The renderer then crashes on null.title / null.page_layout access in lib/engine/build.js:125,129,188,192,194,206.
Reject null explicitly (e.g. data === null || typeof data !== 'object' || Array.isArray(data)), or check after the parse with if (!data) throw ....
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| logger.debug(`Cleaned \x1b[90m${relative}\x1b[0m`, { enabled: opts.verbose }); | ||
| } | ||
|
|
||
| function pathsOverlap(a, b) { |
There was a problem hiding this comment.
CRITICAL: pathsOverlap is bypassable via symlinks.
path.relative and path.resolve operate on the lexical path and do not resolve symlinks. If outputPath is a symlink whose target lives inside srcPath (e.g. src/_site → src/), the lexical comparison sees two distinct paths, the overlap check returns false, the guard passes, and fs.rmSync(resolved, { recursive: true, force: true }) follows the symlink and recursively deletes the source tree.
The whole point of this guard is to prevent exactly that — misconfigured outputPath deleting source — and the previous review flagged that scenario. The fix should compare real paths, e.g. fs.realpathSync for both sides, before the overlap test (and bail out if either path does not exist). At minimum, document this limitation.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| logger.debug(`Cleaned \x1b[90m${relative}\x1b[0m`, { enabled: opts.verbose }); | ||
| } | ||
|
|
||
| function pathsOverlap(a, b) { |
There was a problem hiding this comment.
CRITICAL: pathsOverlap is bypassable on case-insensitive filesystems.
path.relative is case-sensitive on every platform, but macOS (HFS+/APFS default) and Windows (NTFS) are case-insensitive. A repo with srcPath = /project/src and an outputPath like /project/SRC/_site (different casing) compares as a non-overlapping sibling under the guard, while fs.rmSync resolves both names to the same on-disk directory and deletes the source tree.
This is the same class of bug as the symlink bypass and the original CRITICAL — the safety guard passes, the destructive op still runs. Consider normalizing case (or using fs.realpathSync, which on case-insensitive volumes surfaces the canonical case) before comparing.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const start = Date.now(); | ||
|
|
||
| if (opts.clean === true && opts.files === undefined) { | ||
| cleanOutputPath(outputPath, { verbose, protectedPaths: [srcPath] }); |
There was a problem hiding this comment.
WARNING: Cleaning _site before file discovery assumes default outputPath/srcPath.
This call now runs unconditionally for full builds (opts.clean === true && opts.files === undefined) and protects srcPath from overlap. But the existing engine API still lets callers pass arbitrary outputPath values that are siblings of arbitrary directories outside srcPath (e.g. outputPath: '/tmp/somewhere-else'). The previous review's CRITICAL about arbitrary absolute paths is partially mitigated for srcPath but not addressed as a general API contract — programmatic callers can still recursively delete arbitrary trees. If the goal is clean to be safe to expose in the public API, this should be tightened (e.g. require outputPath to be inside a configurable safe root, not just disjoint from srcPath).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
@next-devin can you describe the problems this is solving? I don't quite understand the purpose of this or what problems we are solving. |
@alexphelps This PR is a small DX/reliability pass for next-campaign-page-kit itself, scoped to issues that affect any repo using the package. |
alexphelps
left a comment
alexphelps
left a comment
There was a problem hiding this comment.
We need to clarify the issues we are solving and how this PR addresses those issues directly.
Once issues are clearly identified and understood, the PR should be focused.
|
|
||
| By default, `campaign-build` preserves existing files in `_site/` so repos can | ||
| serve or manage additional campaign output there. Use `--clean` only when | ||
| `_site/` is fully owned by this build and stale output should be removed first. |
There was a problem hiding this comment.
@next-devin this will create massive confusion. Assets are always copied over here, users should not put their own content in the _site dir, what problem is this solving?
There was a problem hiding this comment.
Why are we trying to replace grey-matter? grey-matter is the defacto standard yaml parser used by all node based ssgs.
|
@alexphelps I tightened this based on your feedback. I pulled out the parser/audit change and the The PR now only targets package/CLI friction that should apply to any page-kit user:
No dependency changes, no output-directory semantics, and no Campaigns OS/internal workflow assumptions. |
| // primary output — required for `campaign-build --json | jq` to work. | ||
| const debug = (msg) => console.error(`${TAG} \x1b[90mDEBUG\x1b[0m ${msg}`); | ||
| const debug = (msg, opts = {}) => { | ||
| const enabled = Object.prototype.hasOwnProperty.call(opts, 'enabled') ? opts.enabled : true; |
There was a problem hiding this comment.
WARNING: The defensive Object.prototype.hasOwnProperty.call(opts, 'enabled') ? opts.enabled : true defaulting to true is at odds with the README's new claim ("Debug lines are quiet by default…"). In practice this means:
- The build pipeline's only debug call (
lib/engine/build.js:135) does honorverbosecorrectly because it explicitly passes{ enabled: verbose }. - But every other
logger.debugcall site (lib/actions/clone.js:41,lib/actions/compress.js:219) still prints to stderr unconditionally because they don't pass{ enabled: false }and the default istrue. None of those binaries expose a--verboseflag, so the "quiet by default" promise is silently violated.
This was previously flagged as carried-forward (lib/logger.js default debugEnabled = true contradicts the PR's "quiet by default" intent). The current PR narrows the disagreement to build's debug line only by threading verbose per call — it does not fix the underlying module-level default. Worth either (a) flipping the module-level default to false and requiring an explicit setDebugEnabled(true) or --verbose flag at every call site, or (b) scoping the README's "quiet by default" claim strictly to campaign-build --json so it doesn't overpromise.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| `campaign-build --json` reports, for every page in the build, which source file was rendered, which URL it resolved to, and which output file it was written to — in a form that CI jobs and scripts can consume directly. | ||
|
|
||
| In this mode, stdout carries exactly one JSON document and nothing else. Warnings, errors, and debug lines all go to stderr, so the summary can be piped or redirected without any filtering: | ||
| In this mode, stdout carries exactly one JSON document and nothing else. Warnings and errors go to stderr. Debug lines are quiet by default and are printed to stderr only when `--verbose` is passed, so the summary can be piped or redirected without any filtering: |
There was a problem hiding this comment.
WARNING: The new sentence claims "Debug lines are quiet by default and are printed to stderr only when --verbose is passed". This is true for campaign-build's single per-page debug message (which is gated by the new verbose parameter), but the claim is broader than what the implementation actually delivers:
lib/logger.js'sdebug()defaultsenabledtotrue.lib/actions/clone.js:41andlib/actions/compress.js:219calllogger.debug(msg)with no opts, so they always print regardless of any flag.- Neither
campaign-clonenorcampaign-compresshas a--verboseflag, so users cannot opt in or out at those entry points.
A reader skimming this paragraph could reasonably conclude that all DEBUG lines across the kit are gated by --verbose, which is not the case. Either narrow the wording to "campaign-build's per-page debug output is quiet by default and prints only with --verbose", or actually wire a --verbose flag through the other binaries and flip the logger default. (See also the comment on lib/logger.js:10.)
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
Too much churn, will resubmit this in a cleaner way |
2 participants
Purpose
This PR focuses on small, global friction points in the public page-kit build CLI: commands should be discoverable in a fresh repo, and machine-readable build output should be safe to pipe without filtering log noise.
Summary
next-campaign-page-kitcampaign-build --helpexit before loading project data, so help works in an empty or partially initialized repocampaign-build --jsonstdout as a single JSON document by making debug diagnostics opt-in with--verboseDeliberately out of scope
gray-matterstays in place_site/Validation
npm test— 249/249 passinggit diff --check— clean--help,--json, and--json --verboseNote:
npm audit --audit-level=moderatestill reports the existinggray-matter -> js-yamlmoderate advisory. I left that unchanged because replacing the standard frontmatter parser is not part of this focused PR.