Conversation
Add `egg-bin manifest` command with three actions: - `generate`: fork a child process to boot app in metadataOnly mode and write .egg/manifest.json with resolveCache, fileDiscovery, and tegg extension data - `validate`: in-process ManifestStore.load() check with metadata output - `clean`: remove .egg/manifest.json Extract buildRequiresExecArgv() from dev.ts into BaseCommand for reuse. Add E2E verification script (ecosystem-ci/scripts/verify-manifest.mjs) that tests the full manifest lifecycle including app boot with manifest. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds an egg-bin manifest workflow: new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as egg-bin manifest
participant Command as Manifest Command
participant Subproc as manifest-generate.mjs
participant Framework as Framework App
participant Store as ManifestStore
Note over CLI: Generate flow
CLI->>Command: run action=generate
Command->>Subproc: spawn Node with execArgv + JSON options
Subproc->>Framework: import & start app (metadataOnly)
Framework-->>Subproc: app.loader.generateManifest()
Subproc->>Store: ManifestStore.write(.egg/manifest.json)
Store-->>Subproc: written
Subproc-->>Command: exit 0
Command-->>CLI: success
sequenceDiagram
participant Verifier as verify-manifest.mjs
participant CLI as egg-bin
participant AppCtl as eggctl
participant HTTP as App HTTP
Note over Verifier: E2E verification
Verifier->>CLI: run manifest generate
CLI-->>Verifier: manifest created
Verifier->>CLI: run manifest validate
CLI-->>Verifier: manifest valid
Verifier->>AppCtl: eggctl start --daemon --port
AppCtl->>HTTP: app listens
Verifier->>HTTP: poll / until success or timeout
HTTP-->>Verifier: healthy response
Verifier->>AppCtl: eggctl stop
Verifier->>CLI: run manifest clean
CLI-->>Verifier: manifest removed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Startup Benchmark Report (cnpmcore, single process, compiled JS mode)Environment: macOS, Node.js v22.22.0, Run 1
Run 2
Summary (averaged)
Notes
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| function run(cmd) { | ||
| console.log(`\n$ ${cmd}`); | ||
| execSync(cmd, { stdio: 'inherit', cwd: projectDir }); |
Check warning
Code scanning / CodeQL
Indirect uncontrolled command line Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 9 hours ago
In general, the right fix is to avoid passing a single concatenated command string to execSync, and instead use an API that does not invoke a shell and takes the executable plus its arguments as an array, e.g. execFileSync. This inherently avoids shell parsing, so environment variables like MANIFEST_VERIFY_ENV and MANIFEST_VERIFY_PORT are passed as literal arguments and cannot alter the shell command structure.
For this file, the simplest change without altering functionality is:
- Replace the generic
run(cmd)andrunCapture(cmd)helpers with argument-based wrappers that:- Accept
commandandargsseparately. - Log a reconstructed “pretty” shell-style command for human readability (using proper quoting when desired).
- Call
execFileSync(orexecSyncwith{ shell: false }, butexecFileSyncis clearer) with the command and argument array.
- Accept
- Update the call site that currently passes a string built with template interpolation:
- Replace
run(\npx egg-bin manifest generate --env=${env}`);` - With something like
run('npx', ['egg-bin', 'manifest', 'generate',--env=${env}]);
- Replace
- Optionally, introduce a small helper to format the logged command (
formatCommandForLog) to mimic shell syntax in logs, but that helper should not actually invoke a shell—only build a string forconsole.log.
All changes are confined to ecosystem-ci/scripts/verify-manifest.mjs. We can reuse the existing execSync import by switching to execFileSync (also exported from node:child_process), which requires a small import change. No change in observable behavior occurs other than removing the possibility of shell interpretation.
| @@ -11,7 +11,7 @@ | ||
| * 4. clean — removes .egg/manifest.json | ||
| */ | ||
|
|
||
| import { execSync } from 'node:child_process'; | ||
| import { execFileSync } from 'node:child_process'; | ||
| import { existsSync, readFileSync, rmSync } from 'node:fs'; | ||
| import { join } from 'node:path'; | ||
| import { setTimeout as sleep } from 'node:timers/promises'; | ||
| @@ -22,16 +22,21 @@ | ||
| const healthPort = process.env.MANIFEST_VERIFY_PORT || '7002'; | ||
| const healthTimeout = parseInt(process.env.MANIFEST_VERIFY_TIMEOUT || '60', 10); | ||
|
|
||
| function run(cmd) { | ||
| console.log(`\n$ ${cmd}`); | ||
| execSync(cmd, { stdio: 'inherit', cwd: projectDir }); | ||
| function formatCommandForLog(command, args) { | ||
| const renderedArgs = (args || []).map(a => JSON.stringify(String(a))); | ||
| return [command].concat(renderedArgs).join(' '); | ||
| } | ||
|
|
||
| function runCapture(cmd) { | ||
| console.log(`\n$ ${cmd}`); | ||
| return execSync(cmd, { cwd: projectDir, encoding: 'utf-8' }); | ||
| function run(command, args = []) { | ||
| console.log(`\n$ ${formatCommandForLog(command, args)}`); | ||
| execFileSync(command, args, { stdio: 'inherit', cwd: projectDir }); | ||
| } | ||
|
|
||
| function runCapture(command, args = []) { | ||
| console.log(`\n$ ${formatCommandForLog(command, args)}`); | ||
| return execFileSync(command, args, { cwd: projectDir, encoding: 'utf-8' }); | ||
| } | ||
|
|
||
| function assert(condition, message) { | ||
| if (!condition) { | ||
| console.error(`FAIL: ${message}`); | ||
| @@ -52,7 +53,7 @@ | ||
|
|
||
| // Step 2: Generate manifest | ||
| console.log('\n--- Step 1: Generate manifest ---'); | ||
| run(`npx egg-bin manifest generate --env=${env}`); | ||
| run('npx', ['egg-bin', 'manifest', 'generate', `--env=${env}`]); | ||
|
|
||
| // Step 3: Verify manifest file exists and has valid structure | ||
| console.log('\n--- Step 2: Verify manifest structure ---'); |
|
|
||
| function runCapture(cmd) { | ||
| console.log(`\n$ ${cmd}`); | ||
| return execSync(cmd, { cwd: projectDir, encoding: 'utf-8' }); |
Check warning
Code scanning / CodeQL
Indirect uncontrolled command line Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 9 hours ago
General fix: avoid executing tainted data via a shell. Prefer execFileSync / spawnSync with an argument array, and validate any environment-derived values (like ports) before using them. Where shell commands must remain string-based, perform strict validation/whitelisting.
Best minimal fix here:
- Keep the existing
run/runCaptureAPIs for the rest of the file to avoid behavior changes. - For the health-check curl command, stop going through the generic
runCapturewrapper that usesexecSyncwith a shell string. - Instead, import
execFileSyncfromnode:child_processand call it directly with a fixed executable (curl) and an explicit argument array, passinghealthUrlas a plain argument so it is not interpreted by the shell. - Additionally, validate
healthPortwhen it is read from the environment to ensure it is a reasonable numeric TCP port, falling back to a safe default (7002) if invalid. This removes the ability to inject shell metacharacters via the port value and also improves robustness.
Concretely:
- Update the import on line 14 to also import
execFileSync. - After
healthTimeoutis defined, parse and validatehealthPortinto a numerichealthPortNumberwithin 1–65535, then derivehealthPortfrom that numeric value (ensuring it contains only digits). - Replace the
runCapturecall at line 91 with a directexecFileSync('curl', [...])call that returns the HTTP status code without invoking a shell.
All changes are confined to ecosystem-ci/scripts/verify-manifest.mjs.
| @@ -11,7 +11,7 @@ | ||
| * 4. clean — removes .egg/manifest.json | ||
| */ | ||
|
|
||
| import { execSync } from 'node:child_process'; | ||
| import { execSync, execFileSync } from 'node:child_process'; | ||
| import { existsSync, readFileSync, rmSync } from 'node:fs'; | ||
| import { join } from 'node:path'; | ||
| import { setTimeout as sleep } from 'node:timers/promises'; | ||
| @@ -19,7 +19,15 @@ | ||
| const projectDir = process.cwd(); | ||
| const manifestPath = join(projectDir, '.egg', 'manifest.json'); | ||
| const env = process.env.MANIFEST_VERIFY_ENV || 'unittest'; | ||
| const healthPort = process.env.MANIFEST_VERIFY_PORT || '7002'; | ||
| const rawHealthPort = process.env.MANIFEST_VERIFY_PORT || '7002'; | ||
| const healthPortNumber = (() => { | ||
| const n = Number(rawHealthPort); | ||
| if (!Number.isInteger(n) || n < 1 || n > 65535) { | ||
| return 7002; | ||
| } | ||
| return n; | ||
| })(); | ||
| const healthPort = String(healthPortNumber); | ||
| const healthTimeout = parseInt(process.env.MANIFEST_VERIFY_TIMEOUT || '60', 10); | ||
|
|
||
| function run(cmd) { | ||
| @@ -88,7 +96,7 @@ | ||
| console.log(`Waiting for app at ${healthUrl} (timeout: ${healthTimeout}s)...`); | ||
| while (true) { | ||
| try { | ||
| const output = runCapture(`curl -s -o /dev/null -w "%{http_code}" "${healthUrl}"`); | ||
| const output = execFileSync('curl', ['-s', '-o', '/dev/null', '-w', '%{http_code}', healthUrl], { cwd: projectDir, encoding: 'utf-8' }); | ||
| const status = output.trim(); | ||
| console.log(' Health check: status=%s', status); | ||
| // Any HTTP response (not connection refused) means the app is up. |
Deploying egg with
|
| Latest commit: |
98cc593
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://adbc9c15.egg-cci.pages.dev |
| Branch Preview URL: | https://feat-tegg-manifest-cli.egg-cci.pages.dev |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a manifest management system for egg-bin to optimize application cold starts. It adds a new manifest command for generating, validating, and cleaning startup manifests, along with a supporting generation script and an E2E verification suite. The review feedback highlights several areas for improvement in the verification script, including safer parsing of environment variables, more robust process management for daemonized instances, and enhanced error reporting in catch blocks. There is also a recommendation to synchronize default environment settings between the CLI and the test scripts to ensure consistent behavior.
| const manifestPath = join(projectDir, '.egg', 'manifest.json'); | ||
| const env = process.env.MANIFEST_VERIFY_ENV || 'unittest'; | ||
| const healthPort = process.env.MANIFEST_VERIFY_PORT || '7002'; | ||
| const healthTimeout = parseInt(process.env.MANIFEST_VERIFY_TIMEOUT || '30', 10); |
There was a problem hiding this comment.
The parseInt function can return NaN if process.env.MANIFEST_VERIFY_TIMEOUT is not a valid number. Using NaN in subsequent calculations or comparisons can lead to unexpected behavior. It's safer to check for isNaN or provide a default fallback if parsing fails. Note: ensure the variable is declared with let to allow reassignment.
let healthTimeout = parseInt(process.env.MANIFEST_VERIFY_TIMEOUT || '30', 10);
if (isNaN(healthTimeout)) {
console.warn('Invalid MANIFEST_VERIFY_TIMEOUT, defaulting to 30s');
healthTimeout = 30;
}| // Step 5: Boot the app with manifest and verify it starts correctly | ||
| console.log('\n--- Step 4: Boot app with manifest ---'); | ||
| try { | ||
| run(`npx eggctl start --port=${healthPort} --env=${env} --daemon`); |
There was a problem hiding this comment.
Using eggctl start --daemon followed by a generic eggctl stop might not reliably stop the specific process started by this script, especially if multiple eggctl instances are running. This could lead to orphaned processes or interfere with subsequent test runs. Consider capturing the PID of the daemonized process and using it for a targeted kill command or ensuring eggctl stop has a mechanism to stop the specific instance started by the script.
| } catch { | ||
| console.log(' Health check: connection refused, retrying...'); | ||
| } |
There was a problem hiding this comment.
The broad catch block (catch { ... }) for the curl command silently ignores any error that might occur during the health check, not just connection refused. This can hide other issues that prevent the app from starting correctly. It's better to log the error object (err) to provide more context for debugging.
| } catch { | |
| console.log(' Health check: connection refused, retrying...'); | |
| } | |
| } catch (err) { | |
| console.log(' Health check: connection refused, retrying... Error: %s', err.message); | |
| } |
| } catch { | ||
| /* ignore */ | ||
| } |
There was a problem hiding this comment.
The catch block for eggctl stop during error handling is too broad and silently ignores any error. If eggctl stop fails for a reason other than the app not being started, this error will be hidden, making debugging difficult. Consider logging the error object (err) to provide more context.
| } catch { | |
| /* ignore */ | |
| } | |
| } catch (err) { | |
| console.error('Failed to stop eggctl during cleanup:', err.message); | |
| } |
| description: 'server environment for manifest generation/validation', | ||
| default: 'prod', |
There was a problem hiding this comment.
The manifest command's default env flag is 'prod', while the E2E verification script (ecosystem-ci/scripts/verify-manifest.mjs) uses 'unittest' as its default. This discrepancy can lead to different behaviors between local testing and direct CLI usage, potentially causing confusion or requiring explicit --env flags in scenarios where consistency is expected. Consider aligning the default env value for consistency, or add a clear comment explaining the intentional difference.
| description: 'server environment for manifest generation/validation', | |
| default: 'prod', | |
| env: Flags.string({ | |
| description: 'server environment for manifest generation/validation', | |
| default: 'unittest', | |
| }), |
Deploying egg-v3 with
|
| Latest commit: |
98cc593
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8833ffc0.egg-v3.pages.dev |
| Branch Preview URL: | https://feat-tegg-manifest-cli.egg-v3.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5850 +/- ##
=======================================
Coverage 85.45% 85.45%
=======================================
Files 666 666
Lines 13171 13171
Branches 1522 1522
=======================================
Hits 11255 11255
Misses 1785 1785
Partials 131 131 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The working directory in CI is ecosystem-ci/examples/hello-tegg, so the path to ecosystem-ci/scripts/ is ../../scripts/, not ../../ecosystem-ci/scripts/. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a new egg-bin manifest CLI command to generate/validate/clean startup manifests, along with CI/E2E verification to exercise the full manifest lifecycle and improve cold-start performance.
Changes:
- Add
manifestcommand (generate,validate,clean) and generation helper script. - Refactor
buildRequiresExecArgv()intoBaseCommandand reuse it indev. - Add E2E verification script and wire it into the
examples/hello-teggCI workflow.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/egg-bin/src/index.ts | Exports the new Manifest command from the egg-bin entrypoint. |
| tools/egg-bin/src/commands/manifest.ts | Implements egg-bin manifest actions and CLI flags. |
| tools/egg-bin/src/commands/dev.ts | Uses the extracted buildRequiresExecArgv() helper. |
| tools/egg-bin/src/baseCommand.ts | Introduces reusable buildRequiresExecArgv() for forking with --require/--import. |
| tools/egg-bin/scripts/manifest-generate.mjs | Forked subprocess entry to boot in metadataOnly mode and write .egg/manifest.json. |
| tools/egg-bin/package.json | Exposes commands/manifest export and adds @eggjs/core dependency. |
| pnpm-lock.yaml | Locks the new workspace dependency (@eggjs/core) for egg-bin. |
| ecosystem-ci/scripts/verify-manifest.mjs | Adds an end-to-end script to generate/validate/boot/clean manifests. |
| .github/workflows/e2e-test.yml | Runs the new manifest E2E script in the examples/hello-tegg CI job. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| const { ManifestStore } = await import('@eggjs/core'); | ||
| const store = ManifestStore.load(flags.base, flags.env, flags.scope); | ||
|
|
There was a problem hiding this comment.
manifest validate imports @eggjs/core via normal Node resolution, which can be a different version than the framework’s @eggjs/core (especially when --framework points at a different framework install). This can lead to validating a manifest with different validation logic than the runtime loader uses. Consider resolving ManifestStore relative to the resolved framework path (similar to manifest-generate.mjs), and/or actually using the --framework flag here so generation/validation are consistent.
| const { flags } = this; | ||
| debug('clean manifest: baseDir=%s', flags.base); | ||
|
|
||
| const { ManifestStore } = await import('@eggjs/core'); |
There was a problem hiding this comment.
manifest clean also imports @eggjs/core from egg-bin’s dependency graph. If the goal is to operate on the manifest for a specific --framework, this can again pick a different ManifestStore implementation/version than the framework uses. Consider resolving ManifestStore from the framework path (or avoid importing core at all and delete the file directly).
| const { ManifestStore } = await import('@eggjs/core'); | |
| const framework = getFrameworkPath({ | |
| framework: flags.framework, | |
| baseDir: flags.base, | |
| }); | |
| const frameworkModule: any = await import(framework); | |
| const { ManifestStore } = frameworkModule; | |
| if (!ManifestStore || typeof ManifestStore.clean !== 'function') { | |
| throw new Error(`ManifestStore.clean is not available on framework module: ${framework}`); | |
| } |
| scope: Flags.string({ | ||
| description: 'server scope for manifest validation', | ||
| default: '', | ||
| }), |
There was a problem hiding this comment.
--scope is defined as a flag but generate doesn’t accept/propagate it, so users can validate a scoped manifest but can’t easily generate one for the same scope without setting EGG_SERVER_SCOPE manually. Consider supporting manifest generate --scope=... and propagating it into the generation subprocess.
| debug('generate manifest: baseDir=%s, framework=%s, env=%s', flags.base, framework, flags.env); | ||
|
|
||
| const options = { | ||
| baseDir: flags.base, | ||
| framework, | ||
| env: flags.env, |
There was a problem hiding this comment.
Manifest generation options passed to manifest-generate.mjs don’t include scope, and the script only sets EGG_SERVER_ENV. If the app uses EGG_SERVER_SCOPE, the generated manifest may be tied to the default scope and won’t validate for other scopes. Consider including scope in the options and setting EGG_SERVER_SCOPE (or passing serverScope to framework.start).
| debug('generate manifest: baseDir=%s, framework=%s, env=%s', flags.base, framework, flags.env); | |
| const options = { | |
| baseDir: flags.base, | |
| framework, | |
| env: flags.env, | |
| debug( | |
| 'generate manifest: baseDir=%s, framework=%s, env=%s, scope=%s', | |
| flags.base, | |
| framework, | |
| flags.env, | |
| flags.scope, | |
| ); | |
| const options = { | |
| baseDir: flags.base, | |
| framework, | |
| env: flags.env, | |
| scope: flags.scope, |
| const resolveCacheCount = Object.keys(data.resolveCache).length; | ||
| const fileDiscoveryCount = Object.keys(data.fileDiscovery).length; | ||
| const extensionCount = Object.keys(data.extensions).length; |
There was a problem hiding this comment.
ManifestStore.load() currently doesn’t validate that resolveCache / fileDiscovery / extensions are present, so a manifest could pass load() but still have these fields missing. Calling Object.keys(data.resolveCache) would then throw. Consider defaulting to {} (or guarding) when computing these counts so validate fails gracefully for malformed manifests.
| const resolveCacheCount = Object.keys(data.resolveCache).length; | |
| const fileDiscoveryCount = Object.keys(data.fileDiscovery).length; | |
| const extensionCount = Object.keys(data.extensions).length; | |
| const resolveCacheCount = Object.keys(data.resolveCache ?? {}).length; | |
| const fileDiscoveryCount = Object.keys(data.fileDiscovery ?? {}).length; | |
| const extensionCount = Object.keys(data.extensions ?? {}).length; |
| public async run(): Promise<void> { | ||
| const { action } = this.args; | ||
| switch (action) { | ||
| case 'generate': | ||
| await this.runGenerate(); | ||
| break; | ||
| case 'validate': | ||
| await this.runValidate(); | ||
| break; | ||
| case 'clean': | ||
| await this.runClean(); | ||
| break; | ||
| } |
There was a problem hiding this comment.
New manifest command adds non-trivial behavior (forking metadataOnly boot, validating manifests, cleaning) but there are existing command-level tests in tools/egg-bin/test/commands/*. Consider adding unit/integration tests for manifest generate/validate/clean to prevent regressions (e.g., correct framework resolution, scope/env handling, and exit codes).
| break; | ||
| } | ||
|
|
||
| execSync('sleep 2'); | ||
| } |
There was a problem hiding this comment.
The retry loop uses execSync('sleep 2'), which relies on an external shell command and will fail on Windows/non-POSIX environments. Consider replacing this with an in-process delay (e.g., await new Promise(r => setTimeout(r, 2000))) so the script is portable and doesn’t depend on sleep being available.
killagu
force-pushed
the
feat/tegg-manifest-cli
branch
from
6ea7434 to
0116e51
Compare
killagu
force-pushed
the
feat/tegg-manifest-cli
branch
from
0116e51 to
202b701
Compare
killagu
force-pushed
the
feat/tegg-manifest-cli
branch
from
202b701 to
e08ae36
Compare
hello-tegg has no `/` route (only `/hello`), so the health check returns 404. Change the check to accept any HTTP response (status != "000") as proof the app booted successfully. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
killagu
force-pushed
the
feat/tegg-manifest-cli
branch
from
e08ae36 to
ee4cc01
Compare
|
|
||
| const { ManifestStore } = await import('@eggjs/core'); | ||
| ManifestStore.clean(flags.base); |
There was a problem hiding this comment.
clean also imports @eggjs/core without considering --framework. For consistency with generate (and to avoid pnpm strict / version mismatch issues), resolve and load ManifestStore from the same framework/core that owns the manifest, rather than egg-bin's own dependency graph.
| const framework = await importModule(options.framework); | ||
| const app = await framework.start({ | ||
| baseDir: options.baseDir, | ||
| framework: options.framework, | ||
| env: options.env, | ||
| metadataOnly: true, | ||
| }); |
There was a problem hiding this comment.
Manifest generation boots the app in metadataOnly mode, but if a valid .egg/manifest.json already exists the loader will load that manifest and serve many lookups from cache, so the collector used by generateManifest() will only contain cache misses. That can cause the newly written manifest to be incomplete/delta-only. Consider removing/ignoring any existing manifest before framework.start(...) (e.g., ManifestStore.clean(baseDir) or unlinking the file) so generation always starts from a collector store.
| public async run(): Promise<void> { | ||
| const { action } = this.args; | ||
| switch (action) { | ||
| case 'generate': | ||
| await this.runGenerate(); | ||
| break; | ||
| case 'validate': | ||
| await this.runValidate(); | ||
| break; | ||
| case 'clean': | ||
| await this.runClean(); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| private async runGenerate(): Promise<void> { | ||
| const { flags } = this; | ||
| const framework = getFrameworkPath({ | ||
| framework: flags.framework, | ||
| baseDir: flags.base, | ||
| }); | ||
| debug('generate manifest: baseDir=%s, framework=%s, env=%s', flags.base, framework, flags.env); | ||
|
|
||
| const options = { | ||
| baseDir: flags.base, | ||
| framework, | ||
| env: flags.env, | ||
| }; | ||
|
|
||
| const serverBin = getSourceFilename('../scripts/manifest-generate.mjs'); | ||
| const args = [JSON.stringify(options)]; | ||
| const execArgv = await this.buildRequiresExecArgv(); | ||
| await this.forkNode(serverBin, args, { execArgv }); | ||
| } |
There was a problem hiding this comment.
New egg-bin manifest functionality isn't covered by the existing tools/egg-bin/test/commands/* suite. Adding command-level tests (generate/validate/clean, plus a failure case when manifest is missing/invalid) would help prevent regressions, especially around env/scope/framework resolution.
Review comments responseThanks for the thorough reviews! Here's my take on each: Will fix
Won't fix (by design)
|
- Propagate --scope flag to manifest generate subprocess
- Add ?? {} fallback for malformed manifest in validate
- Clean existing manifest before generate to avoid incomplete data
- Replace execSync('sleep 2') with portable await setTimeout
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ecosystem-ci/scripts/verify-manifest.mjs (1)
24-31:⚠️ Potential issue | 🟠 MajorAvoid shell-string
execSyncwrappers for env-influenced commandsLine 24-31 executes interpolated shell strings; env-sourced inputs (Line 20-22) flow into these commands. Prefer argv-based execution to remove shell interpretation risk and stop recurring CodeQL findings.
🔒 Proposed refactor pattern
-import { execSync } from 'node:child_process'; +import { execFileSync } from 'node:child_process'; ... -function run(cmd) { - console.log(`\n$ ${cmd}`); - execSync(cmd, { stdio: 'inherit', cwd: projectDir }); +function run(bin, args) { + console.log(`\n$ ${bin} ${args.join(' ')}`); + execFileSync(bin, args, { stdio: 'inherit', cwd: projectDir }); } -function runCapture(cmd) { - console.log(`\n$ ${cmd}`); - return execSync(cmd, { cwd: projectDir, encoding: 'utf-8' }); +function runCapture(bin, args) { + console.log(`\n$ ${bin} ${args.join(' ')}`); + return execFileSync(bin, args, { cwd: projectDir, encoding: 'utf-8' }); }#!/bin/bash set -euo pipefail rg -n "MANIFEST_VERIFY_|execSync\\(|run\\(`|runCapture\\(`" ecosystem-ci/scripts/verify-manifest.mjs -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ecosystem-ci/scripts/verify-manifest.mjs` around lines 24 - 31, The run and runCapture wrappers currently pass interpolated shell strings to execSync which allows shell interpretation of env-influenced inputs; change them to argv-based child_process calls to eliminate shell parsing: replace run(cmd) and runCapture(cmd) with functions that accept a command name and an args array (e.g., run(cmdName, args = []), runCapture(cmdName, args = [])) and call child_process.execFileSync (or spawnSync) with those arguments and options {stdio: 'inherit', cwd: projectDir} (or {encoding: 'utf-8', cwd: projectDir} for capture), leaving shell disabled so inputs aren’t interpreted by a shell; update all callsites that pass interpolated strings to instead pass the program and argument array.
🧹 Nitpick comments (1)
tools/egg-bin/src/commands/manifest.ts (1)
11-12: Add JSDoc for exportedManifestcommand classThe exported class currently lacks API documentation.
As per coding guidelines "Document all exported functions, classes, and configuration properties with JSDoc comments".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bin/src/commands/manifest.ts` around lines 11 - 12, Add a JSDoc block immediately above the exported class Manifest<T extends typeof Manifest> describing the command's purpose and public API: include a brief summary that it manages the startup manifest for faster cold starts, an `@template` T describing the generic type parameter, an `@extends` BaseCommand noting the inheritance, and an `@export` or `@public` tag; also document the static property description (static override description) in the JSDoc or as an inline `@description` tag so the exported API is fully documented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/egg-bin/src/commands/manifest.ts`:
- Around line 86-88: The validate and clean flows import ManifestStore using a
bare import('@eggjs/core') which can resolve the wrong package in pnpm strict
mode; change these to resolve and import `@eggjs/core` from the selected framework
path the same way runGenerate() does (i.e., use the framework resolution helper
used by runGenerate() to locate the framework, then dynamic-import the
framework's '@eggjs/core' and obtain ManifestStore), and then call
ManifestStore.load(flags.base, flags.env, flags.scope) so validate, clean, and
runGenerate all use the same resolved ManifestStore implementation.
---
Duplicate comments:
In `@ecosystem-ci/scripts/verify-manifest.mjs`:
- Around line 24-31: The run and runCapture wrappers currently pass interpolated
shell strings to execSync which allows shell interpretation of env-influenced
inputs; change them to argv-based child_process calls to eliminate shell
parsing: replace run(cmd) and runCapture(cmd) with functions that accept a
command name and an args array (e.g., run(cmdName, args = []),
runCapture(cmdName, args = [])) and call child_process.execFileSync (or
spawnSync) with those arguments and options {stdio: 'inherit', cwd: projectDir}
(or {encoding: 'utf-8', cwd: projectDir} for capture), leaving shell disabled so
inputs aren’t interpreted by a shell; update all callsites that pass
interpolated strings to instead pass the program and argument array.
---
Nitpick comments:
In `@tools/egg-bin/src/commands/manifest.ts`:
- Around line 11-12: Add a JSDoc block immediately above the exported class
Manifest<T extends typeof Manifest> describing the command's purpose and public
API: include a brief summary that it manages the startup manifest for faster
cold starts, an `@template` T describing the generic type parameter, an `@extends`
BaseCommand noting the inheritance, and an `@export` or `@public` tag; also document
the static property description (static override description) in the JSDoc or as
an inline `@description` tag so the exported API is fully documented.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9af182d6-b58f-4397-828d-d49ac67fc299
📒 Files selected for processing (2)
ecosystem-ci/scripts/verify-manifest.mjstools/egg-bin/src/commands/manifest.ts
| try { | ||
| const { ManifestStore } = await import('@eggjs/core'); | ||
| const store = ManifestStore.load(flags.base, flags.env, flags.scope); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check `@eggjs/core` imports in egg-bin:"
rg -n "import\\('@eggjs/core'\\)|importModule\\('@eggjs/core'" tools/egg-bin/src tools/egg-bin/scripts -C2
echo
echo "Check egg-bin dependency declaration for `@eggjs/core`:"
cat tools/egg-bin/package.json | sed -n '1,220p'Repository: eggjs/egg
Length of output: 4471
Resolve @eggjs/core via framework path in validate and clean to ensure consistent module resolution
Lines 87 and 121 use bare import('@eggjs/core'), while runGenerate() properly resolves core from the selected framework path. In pnpm strict mode, this inconsistency can cause the wrong ManifestStore version to be used when validating or cleaning manifests from different framework versions, leading to compatibility issues.
Proposed fix
-import { getFrameworkPath } from '@eggjs/utils';
+import { getFrameworkPath, importModule } from '@eggjs/utils';
...
private async runValidate(): Promise<void> {
const { flags } = this;
debug('validate manifest: baseDir=%s, env=%s, scope=%s', flags.base, flags.env, flags.scope);
+ const framework = getFrameworkPath({
+ framework: flags.framework,
+ baseDir: flags.base,
+ });
...
- const { ManifestStore } = await import('@eggjs/core');
+ const { ManifestStore } = await importModule('@eggjs/core', {
+ paths: [framework],
+ });
...
private async runClean(): Promise<void> {
const { flags } = this;
debug('clean manifest: baseDir=%s', flags.base);
+ const framework = getFrameworkPath({
+ framework: flags.framework,
+ baseDir: flags.base,
+ });
- const { ManifestStore } = await import('@eggjs/core');
+ const { ManifestStore } = await importModule('@eggjs/core', {
+ paths: [framework],
+ });
ManifestStore.clean(flags.base);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const { ManifestStore } = await import('@eggjs/core'); | |
| const store = ManifestStore.load(flags.base, flags.env, flags.scope); | |
| import { BaseCommand, command, description, option } from 'egg-console'; | |
| import { join } from 'node:path'; | |
| import Debug from 'debug'; | |
| import { getFrameworkPath, importModule } from '@eggjs/utils'; | |
| import type { PkgInfo } from '@eggjs/core'; | |
| const debug = Debug('egg-bin'); | |
| export class Manifest extends BaseCommand { | |
| static override get description() { | |
| return 'Generate, validate, or clean framework manifest files for optimized startup and module discovery'; | |
| } | |
| `@option`({ | |
| alias: 'm', | |
| description: 'Manifest mode (generate, validate, or clean)', | |
| default: 'generate', | |
| }) | |
| manifest!: 'generate' | 'validate' | 'clean'; | |
| `@option`({ | |
| alias: 'b', | |
| description: 'Base directory for manifest operations', | |
| }) | |
| base!: string; | |
| `@option`({ | |
| alias: 'e', | |
| description: 'Environment (development, production, etc.)', | |
| }) | |
| env!: string; | |
| `@option`({ | |
| alias: 's', | |
| description: 'Scope for manifest generation', | |
| }) | |
| scope!: string; | |
| `@option`({ | |
| alias: 'f', | |
| description: 'Framework path', | |
| }) | |
| framework!: string; | |
| override async run(): Promise<void> { | |
| const { manifest } = this; | |
| switch (manifest) { | |
| case 'generate': | |
| await this.runGenerate(); | |
| break; | |
| case 'validate': | |
| await this.runValidate(); | |
| break; | |
| case 'clean': | |
| await this.runClean(); | |
| break; | |
| default: | |
| throw new Error(`Unknown manifest mode: ${manifest}`); | |
| } | |
| } | |
| private async runGenerate(): Promise<void> { | |
| const { flags } = this; | |
| debug('generate manifest: baseDir=%s, env=%s, scope=%s', flags.base, flags.env, flags.scope); | |
| const framework = getFrameworkPath({ | |
| framework: flags.framework, | |
| baseDir: flags.base, | |
| }); | |
| try { | |
| const manifestGenerateScript = join(__dirname, '../scripts/manifest-generate.mjs'); | |
| const { fork } = await import('child_process'); | |
| return new Promise((resolve, reject) => { | |
| const child = fork(manifestGenerateScript, [], { | |
| stdio: 'inherit', | |
| env: { | |
| ...process.env, | |
| EGG_BIN_FRAMEWORK: framework, | |
| EGG_BIN_BASE_DIR: flags.base, | |
| EGG_BIN_ENV: flags.env, | |
| EGG_BIN_SCOPE: flags.scope, | |
| }, | |
| }); | |
| child.on('exit', (code) => { | |
| if (code === 0) { | |
| resolve(); | |
| } else { | |
| reject(new Error(`manifest generate script exited with code ${code}`)); | |
| } | |
| }); | |
| child.on('error', reject); | |
| }); | |
| } catch (error) { | |
| this.ctx.logger.error('generate manifest failed', error); | |
| throw error; | |
| } | |
| } | |
| private async runValidate(): Promise<void> { | |
| const { flags } = this; | |
| debug('validate manifest: baseDir=%s, env=%s, scope=%s', flags.base, flags.env, flags.scope); | |
| const framework = getFrameworkPath({ | |
| framework: flags.framework, | |
| baseDir: flags.base, | |
| }); | |
| try { | |
| const { ManifestStore } = await importModule('@eggjs/core', { | |
| paths: [framework], | |
| }); | |
| const store = ManifestStore.load(flags.base, flags.env, flags.scope); | |
| if (!store.isValid()) { | |
| throw new Error('Manifest validation failed'); | |
| } | |
| this.ctx.console.log('✓ Manifest is valid'); | |
| } catch (error) { | |
| this.ctx.logger.error('validate manifest failed', error); | |
| throw error; | |
| } | |
| } | |
| private async runClean(): Promise<void> { | |
| const { flags } = this; | |
| debug('clean manifest: baseDir=%s', flags.base); | |
| const framework = getFrameworkPath({ | |
| framework: flags.framework, | |
| baseDir: flags.base, | |
| }); | |
| const { ManifestStore } = await importModule('@eggjs/core', { | |
| paths: [framework], | |
| }); | |
| ManifestStore.clean(flags.base); | |
| this.ctx.console.log('✓ Manifest cleaned'); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/egg-bin/src/commands/manifest.ts` around lines 86 - 88, The validate
and clean flows import ManifestStore using a bare import('@eggjs/core') which
can resolve the wrong package in pnpm strict mode; change these to resolve and
import `@eggjs/core` from the selected framework path the same way runGenerate()
does (i.e., use the framework resolution helper used by runGenerate() to locate
the framework, then dynamic-import the framework's '@eggjs/core' and obtain
ManifestStore), and then call ManifestStore.load(flags.base, flags.env,
flags.scope) so validate, clean, and runGenerate all use the same resolved
ManifestStore implementation.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tools/egg-bin/src/commands/manifest.ts (1)
11-19: Consider adding JSDoc documentation for the exported command class.As per coding guidelines, exported classes should have JSDoc comments. A brief description would improve discoverability.
💡 Suggested improvement
+/** + * Oclif command to manage the startup manifest for faster cold starts. + * Supports generate, validate, and clean actions. + */ export default class Manifest<T extends typeof Manifest> extends BaseCommand<T> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bin/src/commands/manifest.ts` around lines 11 - 19, Add a JSDoc block above the exported Manifest class to document its purpose and usage: describe that Manifest<T extends typeof Manifest> extends BaseCommand and manages the startup manifest for faster cold starts, list supported subcommands (generate, validate, clean) and any key options like --env, and include `@export/`@class tags and type parameter note so IDEs and docs surface the class properly; place the comment immediately above the declaration of Manifest to follow project coding guidelines.tools/egg-bin/scripts/manifest-generate.mjs (1)
7-10: Consider wrapping JSON.parse in try-catch for better error messaging.If
process.argv[2]is missing or contains malformed JSON, the current code will throw an unhandled exception before reaching the main catch block, producing a cryptic error. A dedicated try-catch would improve debuggability.💡 Suggested improvement
async function main() { debug('argv: %o', process.argv); - const options = JSON.parse(process.argv[2]); + let options; + try { + options = JSON.parse(process.argv[2]); + } catch (e) { + throw new Error(`Failed to parse options from argv: ${e.message}`); + } debug('manifest generate options: %o', options);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bin/scripts/manifest-generate.mjs` around lines 7 - 10, Wrap the JSON.parse of process.argv[2] inside a try-catch in the async function main so malformed or missing argv data yields a clear error; catch the parse error around the JSON.parse call that assigns options, log a descriptive message including the offending process.argv[2] and the parse error (use the same debug/processLogger in scope), and rethrow or exit so the existing outer error handling can take over. Ensure you reference the JSON.parse(...) that creates the options variable in main when adding the try-catch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/egg-bin/scripts/manifest-generate.mjs`:
- Around line 7-10: Wrap the JSON.parse of process.argv[2] inside a try-catch in
the async function main so malformed or missing argv data yields a clear error;
catch the parse error around the JSON.parse call that assigns options, log a
descriptive message including the offending process.argv[2] and the parse error
(use the same debug/processLogger in scope), and rethrow or exit so the existing
outer error handling can take over. Ensure you reference the JSON.parse(...)
that creates the options variable in main when adding the try-catch.
In `@tools/egg-bin/src/commands/manifest.ts`:
- Around line 11-19: Add a JSDoc block above the exported Manifest class to
document its purpose and usage: describe that Manifest<T extends typeof
Manifest> extends BaseCommand and manages the startup manifest for faster cold
starts, list supported subcommands (generate, validate, clean) and any key
options like --env, and include `@export/`@class tags and type parameter note so
IDEs and docs surface the class properly; place the comment immediately above
the declaration of Manifest to follow project coding guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ed3579f-6ec0-44f9-8991-898e570d787b
📒 Files selected for processing (3)
ecosystem-ci/scripts/verify-manifest.mjstools/egg-bin/scripts/manifest-generate.mjstools/egg-bin/src/commands/manifest.ts
Summary
egg-bin manifestcommand with three actions:generate,validate,cleangenerate: forks child process to boot app inmetadataOnlymode, writes.egg/manifest.jsonvalidate: in-processManifestStore.load()check with metadata outputclean: removes.egg/manifest.jsonbuildRequiresExecArgv()fromdev.tsintoBaseCommandfor reuseecosystem-ci/scripts/verify-manifest.mjs) — tests full manifest lifecycle including app boot with manifest + health check.github/workflows/e2e-test.ymlto run manifest E2E in hello-teggLocal benchmark (cnpmcore, purge before each run)
Test plan
pnpm --filter=@eggjs/bin run typecheckpassespnpm --filter=@eggjs/bin run pretest(tsdown build) passessudo purgeconfirms ~20% cold start improvement🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Scripts
Refactor