feat: add install catalog and project config autodetection#878
Conversation
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
affaan-m has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
📝 WalkthroughWalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as catalog.js
participant Loader as install-manifests
participant FS as File System
User->>CLI: catalog show component-id --json
CLI->>Loader: getInstallComponent(componentId)
Loader->>FS: Load install manifests
FS-->>Loader: Manifest data
Loader->>Loader: Normalize & validate ID
Loader->>Loader: Build component object<br/>(modules, targets, metadata)
Loader-->>CLI: Component object
CLI->>CLI: Serialize to JSON
CLI-->>User: JSON output
sequenceDiagram
participant User
participant Script as install-plan.js
participant ConfigFinder as config.js
participant FS as File System
participant Loader as install-manifests
User->>Script: invoke with no --config
Script->>ConfigFinder: findDefaultInstallConfigPath({ cwd })
ConfigFinder->>FS: Check for ecc-install.json
alt Config exists
FS-->>ConfigFinder: Path found
ConfigFinder-->>Script: Return path
Script->>Loader: loadInstallConfig(path)
Loader-->>Script: Config loaded
else Config not found
FS-->>ConfigFinder: null
ConfigFinder-->>Script: null
Script->>Script: Display help/exit
end
Script-->>User: Planning output or help
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
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 |
Analysis CompleteGenerated ECC bundle from 500 commits | Confidence: 100% View Pull Request #879Repository Profile
Detected Workflows (8)
Generated Instincts (16)
After merging, import with: Files
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1166c6ab58
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| catalog: { | ||
| script: 'catalog.js', | ||
| description: 'Discover install profiles and component IDs', |
There was a problem hiding this comment.
Adding the catalog subcommand here makes ecc delegate to scripts/catalog.js, but package.json’s files allowlist is not updated to ship that new script. This works from a full repo checkout, but in npm-installed environments ecc catalog ... will fail at runtime because the delegated script is missing from the package tarball; please add scripts/catalog.js to the published file list.
Useful? React with 👍 / 👎.
![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
🧹 Nitpick comments (1)
scripts/install-plan.js (1)
237-240: Edge case: non-actionable flags without config produce a cryptic error.The condition
process.argv.length <= 2only triggers help when there are zero user arguments. However, if a user runsnode install-plan.js --jsonwithout anecc-install.jsonpresent,process.argv.lengthis 3, so help won't be shown. Instead,normalizeInstallRequestwill throw"No install profile, module IDs, or included component IDs were provided", which is less helpful than showing usage.Consider checking whether any actionable options are present (profile, modules, components, list flags) rather than relying on argv length:
💡 Suggested improvement
- if (process.argv.length <= 2 && !config) { + const hasActionableOptions = options.profileId + || options.moduleIds.length > 0 + || options.includeComponentIds.length > 0 + || options.listProfiles + || options.listModules + || options.listComponents; + + if (!hasActionableOptions && !config) { showHelp(); process.exit(0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install-plan.js` around lines 237 - 240, The current check uses process.argv.length to decide to show help, which misses runs with only non-actionable flags (e.g., --json) and leads to a cryptic error from normalizeInstallRequest; update the pre-flight check to detect whether any actionable options were provided (presence of config, a profile flag, module IDs, included component IDs, or list flags) before calling normalizeInstallRequest and if none are present call showHelp() and exit; reference the existing symbols config, normalizeInstallRequest, showHelp and the parsed options (or process.argv flags like --profile/--modules/--components/--list) to implement the more precise presence check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/catalog.js`:
- Around line 77-88: The parseArgs logic currently accepts --family and --target
for every command but main() only uses them for the "components" command, so you
must validate flag applicability and reject incompatible flags: update parseArgs
(the branch handling '--family' and '--target' that sets parsed.family and
parsed.target) to check parsed.command (or the command being parsed) and throw a
clear Error when these flags are passed to commands other than "components";
likewise add the same validation for the other flag parsing block around lines
143-177 so any unsupported flag is rejected rather than silently ignored by
main().
---
Nitpick comments:
In `@scripts/install-plan.js`:
- Around line 237-240: The current check uses process.argv.length to decide to
show help, which misses runs with only non-actionable flags (e.g., --json) and
leads to a cryptic error from normalizeInstallRequest; update the pre-flight
check to detect whether any actionable options were provided (presence of
config, a profile flag, module IDs, included component IDs, or list flags)
before calling normalizeInstallRequest and if none are present call showHelp()
and exit; reference the existing symbols config, normalizeInstallRequest,
showHelp and the parsed options (or process.argv flags like
--profile/--modules/--components/--list) to implement the more precise presence
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e67f1b69-4d7e-4690-a36e-84548277b757
📒 Files selected for processing (11)
scripts/catalog.jsscripts/ecc.jsscripts/install-apply.jsscripts/install-plan.jsscripts/lib/install-manifests.jsscripts/lib/install/config.jstests/lib/install-config.test.jstests/scripts/catalog.test.jstests/scripts/ecc.test.jstests/scripts/install-apply.test.jstests/scripts/install-plan.test.js
| } else if (arg === '--family') { | ||
| if (!args[index + 1]) { | ||
| throw new Error('Missing value for --family'); | ||
| } | ||
| parsed.family = normalizeFamily(args[index + 1]); | ||
| index += 1; | ||
| } else if (arg === '--target') { | ||
| if (!args[index + 1]) { | ||
| throw new Error('Missing value for --target'); | ||
| } | ||
| parsed.target = args[index + 1]; | ||
| index += 1; |
There was a problem hiding this comment.
Reject command-incompatible flags instead of accepting and ignoring them.
parseArgs currently allows --family / --target for all commands, but main() only applies them to components. That means invalid invocations (for example, show ... --target cursor) succeed while silently discarding user input.
Suggested fix
function parseArgs(argv) {
@@
for (let index = 1; index < args.length; index += 1) {
@@
}
+
+ if (parsed.command !== 'components' && parsed.family) {
+ throw new Error('--family is only supported with the components command');
+ }
+ if (parsed.command !== 'components' && parsed.target) {
+ throw new Error('--target is only supported with the components command');
+ }
return parsed;
}Also applies to: 143-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/catalog.js` around lines 77 - 88, The parseArgs logic currently
accepts --family and --target for every command but main() only uses them for
the "components" command, so you must validate flag applicability and reject
incompatible flags: update parseArgs (the branch handling '--family' and
'--target' that sets parsed.family and parsed.target) to check parsed.command
(or the command being parsed) and throw a clear Error when these flags are
passed to commands other than "components"; likewise add the same validation for
the other flag parsing block around lines 143-177 so any unsupported flag is
rejected rather than silently ignored by main().
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/lib/install-manifests.js">
<violation number="1" location="scripts/lib/install-manifests.js:226">
P2: Validate component module IDs before mapping; the current logic silently drops unknown IDs and can return inconsistent component data.</violation>
</file>
<file name="scripts/catalog.js">
<violation number="1" location="scripts/catalog.js:65">
P2: Reject `--family` and `--target` unless the `components` subcommand is used; right now other commands accept these flags and silently ignore them, which hides invalid CLI input.</violation>
<violation number="2" location="scripts/catalog.js:81">
P2: `--family` accepts another flag as its value, so malformed input is silently misparsed instead of raising a missing-value error.</violation>
<violation number="3" location="scripts/catalog.js:87">
P2: `--target` also accepts another flag as its value, leading to incorrect argument parsing on malformed input.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| throw new Error(`Unknown install component: ${normalizedComponentId}`); | ||
| } | ||
|
|
||
| const moduleIds = dedupeStrings(component.modules); |
There was a problem hiding this comment.
P2: Validate component module IDs before mapping; the current logic silently drops unknown IDs and can return inconsistent component data.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/lib/install-manifests.js, line 226:
<comment>Validate component module IDs before mapping; the current logic silently drops unknown IDs and can return inconsistent component data.</comment>
<file context>
@@ -210,6 +210,45 @@ function listInstallComponents(options = {}) {
+ throw new Error(`Unknown install component: ${normalizedComponentId}`);
+ }
+
+ const moduleIds = dedupeStrings(component.modules);
+ const modules = moduleIds
+ .map(moduleId => manifests.modulesById.get(moduleId))
</file context>
| const moduleIds = dedupeStrings(component.modules); | |
| const moduleIds = dedupeStrings(component.modules); | |
| assertKnownModuleIds(moduleIds, manifests); |
| if (!args[index + 1]) { | ||
| throw new Error('Missing value for --target'); | ||
| } | ||
| parsed.target = args[index + 1]; |
There was a problem hiding this comment.
P2: --target also accepts another flag as its value, leading to incorrect argument parsing on malformed input.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/catalog.js, line 87:
<comment>`--target` also accepts another flag as its value, leading to incorrect argument parsing on malformed input.</comment>
<file context>
@@ -0,0 +1,186 @@
+ if (!args[index + 1]) {
+ throw new Error('Missing value for --target');
+ }
+ parsed.target = args[index + 1];
+ index += 1;
+ } else if (parsed.command === 'show' && !parsed.componentId) {
</file context>
| if (!args[index + 1]) { | ||
| throw new Error('Missing value for --family'); | ||
| } | ||
| parsed.family = normalizeFamily(args[index + 1]); |
There was a problem hiding this comment.
P2: --family accepts another flag as its value, so malformed input is silently misparsed instead of raising a missing-value error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/catalog.js, line 81:
<comment>`--family` accepts another flag as its value, so malformed input is silently misparsed instead of raising a missing-value error.</comment>
<file context>
@@ -0,0 +1,186 @@
+ if (!args[index + 1]) {
+ throw new Error('Missing value for --family');
+ }
+ parsed.family = normalizeFamily(args[index + 1]);
+ index += 1;
+ } else if (arg === '--target') {
</file context>
|
|
||
| if (args.length === 0 || args[0] === '--help' || args[0] === '-h') { | ||
| parsed.help = true; | ||
| return parsed; |
There was a problem hiding this comment.
P2: Reject --family and --target unless the components subcommand is used; right now other commands accept these flags and silently ignore them, which hides invalid CLI input.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/catalog.js, line 65:
<comment>Reject `--family` and `--target` unless the `components` subcommand is used; right now other commands accept these flags and silently ignore them, which hides invalid CLI input.</comment>
<file context>
@@ -0,0 +1,186 @@
+
+ if (args.length === 0 || args[0] === '--help' || args[0] === '-h') {
+ parsed.help = true;
+ return parsed;
+ }
+
</file context>
affaan-m
force-pushed
the
feat/install-catalog-project-config
branch
from
1166c6a to
b4296c7
Compare
|
There was a problem hiding this comment.
affaan-m has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Analysis Failed
Troubleshooting
Retry: |
1 participant
Summary\n- add an \ command for install profiles and components\n- auto-detect \ from the project root for plan/apply flows\n- cover the new CLI and config behaviors with tests\n\n## Test Plan\n- node tests/lib/install-config.test.js\n- node tests/scripts/ecc.test.js\n- node tests/scripts/install-plan.test.js\n- node tests/scripts/install-apply.test.js\n- node tests/scripts/catalog.test.js
Summary by cubic
Adds a new install catalog CLI and auto-detects project install config to simplify planning and applying installs. You can now discover profiles/components easily and run plan/apply without passing a config path;
planalso runs with no args when a project config exists.ecc catalogwith:profiles,components [--family|--target](supports family aliases like language/lang), andshow <component-id>(prints resolved modules); all support--json.ecc-install.jsonfrom the project root forplanandinstallwhen--configisn’t provided;planproceeds with no args if a config is found.getInstallComponent()in install manifests; updatedecchelp/command routing and added tests for catalog and config autodetection.Written for commit b4296c7. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
catalogcommand to discover and list install profiles and components with filtering options (--family,--target)