feat(cli): inspect JSON build manifests for issue #133 - #888
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support in the sh1pt build command to inspect and summarize JSON build manifests provided via --from, including targeted parser test coverage.
Changes:
- Introduce
summarizeManifestBuild()to validate and extract name/version/targets from a JSON manifest. - Update
buildcommand flow to print a “Build summary” when--frompoints at a JSON manifest. - Add unit tests for JSON manifest parsing and rejection cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/cli/src/commands/build.ts | Adds JSON-manifest parsing/summarization and prints a manifest-derived build summary in the CLI. |
| packages/cli/src/commands/build.test.ts | Adds focused tests for manifest summarization and rejection behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (input.kind === 'doc' && extname(input.value).toLowerCase() === '.json') { | ||
| const summary = summarizeManifestBuild(input); | ||
| console.log(kleur.green('[ok] JSON manifest resolved')); | ||
| console.log(); | ||
| console.log(kleur.bold('Build summary')); | ||
| console.log(` project: ${summary.projectName}`); | ||
| console.log(` version: ${summary.version ?? 'unknown'}`); | ||
| console.log(` targets: ${summary.targetIds.length ? summary.targetIds.join(', ') : 'none declared'}`); | ||
| console.log(` channel: ${opts.channel}`); | ||
| console.log(` target: ${where}`); | ||
| console.log(` manifest: ${summary.sourceFile}`); | ||
| return; | ||
| } |
| let parsed: unknown; | ||
| try { | ||
| parsed = JSON.parse(readFileSync(input.value, 'utf8')); | ||
| } catch (error) { | ||
| const detail = error instanceof Error ? error.message : 'invalid JSON'; | ||
| throw new Error(`build manifest is not valid JSON: ${detail}`); | ||
| } |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
Resolved the merge conflicts against public |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
packages/cli/src/commands/build.ts:264
- The CLI only calls summarizeManifestBuild() when the document path ends in .json. For other document inputs (e.g. manifest.yaml, .toml, .md) resolveInput() will classify them as kind='doc', but this branch falls through to the stub path and does not reject them with the actionable errors that summarizeManifestBuild already provides. This contradicts the PR goal of rejecting non-JSON manifest inputs.
if (input.kind === 'doc' && extname(input.value).toLowerCase() === '.json') {
const summary = summarizeManifestBuild(input);
console.log(kleur.green('[ok] JSON manifest resolved'));
console.log();
console.log(kleur.bold('Build summary'));
packages/cli/src/commands/build.ts:134
- summarizeManifestBuild() validates existence and extension, but it does not verify that the resolved path is a regular file. If this function is called directly (outside resolveInput()) with a directory path, the resulting EISDIR read error will be reported as “not valid JSON”, which is misleading. Mirror summarizeLocalBuild() by asserting statSync(...).isFile() to keep errors actionable.
if (!existsSync(input.value)) {
throw new Error(`build manifest does not exist: ${input.value}`);
}
if (extname(input.value).toLowerCase() !== '.json') {
throw new Error(`build manifest must be a JSON file: ${input.value}`);
packages/cli/src/commands/build.test.ts:279
- The new summarizeManifestBuild() logic has branches for missing files (existsSync), non-object roots (isRecord check), and array-based targets, but the added tests only cover the happy path, malformed JSON, and non-.json extension. Adding cases for “missing manifest path” and “root is not an object” (and optionally array targets) would better match the stated goal of rejecting missing/non-object inputs with actionable errors.
it('summarizes a JSON manifest with object targets', () => {
tempDir = mkdtempSync(join(tmpdir(), 'sh1pt-json-build-'));
const file = join(tempDir, 'manifest.json');
writeFileSync(file, JSON.stringify({
name: 'demo-app',
|
All required GitHub checks are now green and the PR is |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
2 similar comments
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
Summary
sh1pt build --from <manifest.json>Closes #133
Verification
node node_modules/vitest/vitest.mjs run packages/cli/src/commands/build.test.ts(18 tests passed)git diff --check(passed)