json build output and usage docs#18
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}
| built++; | ||
| } catch (e) { | ||
| page.status = 'error'; | ||
| page.errors.push({ code: 'RENDER_ERROR', message: e.message }); |
There was a problem hiding this comment.
WARNING: RENDER_ERROR is used as the catch-all for every failure in the page loop, but the block also catches readFileSync (line 82), gray-matter parse errors (line 83), and mkdirSync/writeFileSync (lines 121-122). A file that fails to read or a malformed frontmatter that throws inside matter() will be reported as a RENDER_ERROR, which is misleading for callers scripting around the JSON summary. Consider using distinct codes (READ_ERROR, FRONTMATTER_ERROR, WRITE_ERROR) or at least capturing the actual exception name in the message.
| const summary = await build(); | ||
|
|
||
| if (json) { | ||
| process.stdout.write(JSON.stringify(summary, null, 2) + '\n'); |
There was a problem hiding this comment.
WARNING: process.stdout.write(...) is asynchronous, but the very next conditional can call process.exit(1) (line 27) if any page errored. On a non-POSIX stdout (or any stdout backed by a pipe that hasn't been drained yet) the buffered JSON can be discarded before it reaches the consumer. The new test campaign-build --json: exits 1 and still emits JSON when a page errors only passes because execFile drains the pipe after the child closes; a CI script that reads stdout incrementally could see an empty/partial document. Pass a callback to write and exit inside it, or write via fs.writeSync(1, ...).
|
|
||
| if (!campaign) { | ||
| page.status = 'skipped'; | ||
| page.warnings.push({ code: 'NO_CAMPAIGN', message: `no campaign found for slug "${campaignSlug}"` }); |
There was a problem hiding this comment.
SUGGESTION: The NO_CAMPAIGN branch pushes the warning entry directly and then calls logger.warn(...) with a different message text ("Skipping X — no campaign found for slug Y" vs "X: no campaign found for slug Y"). Every other warning in this function goes through the pageWarn helper defined just above (line 76), which keeps the JSON message and the stderr message in lockstep. Using pageWarn('NO_CAMPAIGN', ...) here too would prevent the two formats from drifting if the message ever changes.
|
|
||
| const { url, outputFile } = resolveOutput(relFile, frontmatter, outputPath); | ||
| page.url = url; | ||
| page.outputFile = path.relative(process.cwd(), outputFile); |
There was a problem hiding this comment.
WARNING: page.outputFile is documented (README, line ~210) as "relative to the project root", but it's computed via path.relative(process.cwd(), outputFile). If a caller invokes build() from any directory other than the project root (CI, programmatic embedding, a future watcher running from node_modules), the returned path will be wrong — possibly escaping the project root entirely with .. segments. Same problem on page.inputFile (line 66) and the debug log on line 124. Anchor to projectConfig.getProjectRoot() (or pass an explicit cwd option) for stable, documented paths.
| const debug = (msg) => console.log(`${TAG} \x1b[90mDEBUG\x1b[0m ${msg}`); | ||
| // Diagnostics go to stderr (like warn/error) so stdout stays reserved for | ||
| // primary output — required for `campaign-build --json | jq` to work. | ||
| const debug = (msg) => console.error(`${TAG} \x1b[90mDEBUG\x1b[0m ${msg}`); |
There was a problem hiding this comment.
SUGGESTION: Routing debug through console.error is the right call for --json, but this is a silent breaking change for any caller that previously captured debug output from stdout (e.g. node ... 2>/dev/null | grep ... or a wrapper that tees stdout). Worth a one-line note in the changelog / commit message, or — if you want to be conservative — gate it: keep console.log when stdout is a TTY or pipe, and only switch to console.error when --json is active. At minimum, the commit message should call out the behavior change explicitly.
Code Review SummaryStatus: 5 Issues Found | Recommendation: Address before merge Overview
Incremental Changes Since Last ReviewThe new commits (18e1d2c docs improvements, cbfaf37 json build output) introduce two meaningful changes:
WARNING (new)
SUGGESTION (new)
Issue Details (click to expand)WARNING
SUGGESTION
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Reviewed by minimax-m3 · 208,567 tokens |
| const summary = await build(); | ||
|
|
||
| if (json) { | ||
| process.stdout.write(JSON.stringify(summary, null, 2) + '\n'); |
There was a problem hiding this comment.
WARNING: process.stdout.write(...) is still fire-and-forget without a callback. The previous review flagged that the write races process.exit(1); the new process.exitCode = 1 (line 29) lets the event loop drain, but if the write throws (EPIPE on a closed pipe, ENOSPC on a full disk) the error is silently swallowed and the process still exits 0 via the if (summary.errors > 0) branch not running. Pass a callback and bubble the error to exitCode:
| process.stdout.write(JSON.stringify(summary, null, 2) + '\n'); | |
| process.stdout.write(JSON.stringify(summary, null, 2) + '\n', (err) => { if (err) process.exitCode = 1; }); |
| }); | ||
| }); | ||
|
|
||
| test('build: error code names the failed step — malformed frontmatter is FRONTMATTER_ERROR', async () => { |
There was a problem hiding this comment.
SUGGESTION: The test name promises "error code names the failed step", but only one step (FRONTMATTER) is exercised. The new stage taxonomy in lib/engine/build.js:81-137 introduces five codes — READ_ERROR, FRONTMATTER_ERROR, RESOLVE_ERROR, RENDER_ERROR, WRITE_ERROR — and there's no regression coverage for the other four. A future refactor that drops a stage = '...' assignment (e.g., removes the stage = 'WRITE' before the mkdirSync/writeFileSync) would silently flatten all errors back to whatever stage was last set, and this test wouldn't catch it. Either rename the test to be specific to frontmatter, or add parallel cases for the other four stages.
| built++; | ||
| } catch (e) { | ||
| page.status = 'error'; | ||
| page.errors.push({ code: `${stage}_ERROR`, message: e.message }); |
There was a problem hiding this comment.
SUGGESTION: The new ${stage}_ERROR codes change the public surface of the JSON summary (consumers may have been filtering on code === 'RENDER_ERROR'). This is a breaking change for any existing script consuming campaign-build --json. Worth a CHANGELOG entry or a note in the README's --json section that documents the full code list (READ_ERROR, FRONTMATTER_ERROR, RESOLVE_ERROR, RENDER_ERROR, WRITE_ERROR). The previous test at test/build.test.js:650 still asserts the literal RENDER_ERROR, which masks this for render-stage failures, but any code that special-cased the old catch-all across all stages will silently misclassify.
1 participant
No description provided.