fix(installer): instalação condicional de hooks por tier (#544)#597
Open
nikolasdehor wants to merge 1 commit intoSynkraAI:mainfrom
Open
fix(installer): instalação condicional de hooks por tier (#544)#597nikolasdehor wants to merge 1 commit intoSynkraAI:mainfrom
nikolasdehor wants to merge 1 commit intoSynkraAI:mainfrom
Conversation
…kraAI#544) Separa hooks em HOOKS_FREE e HOOKS_PRO_ONLY. O precompact-session-digest.cjs só é copiado quando pro/ está disponível (detectado via pro-detector). Evita processo desnecessário em todo compact para usuários free.
nikolasdehor
requested review from
Pedrovaleriolopez and
oalanicolas
as code owners
|
@nikolasdehor is attempting to deploy a commit to the Pedro Valério Lopez's projects Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
area: agents
WalkthroughThe pull request implements tier-aware hook installation during the installer wizard. The system now detects pro availability at runtime and conditionally copies either free-only hooks or both free and pro hooks, alongside adding comprehensive test coverage for this conditional logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Installer as IDE Config Generator
participant ProDetector as Pro Detector
participant Hooks as Hook Selection Logic
participant FileSystem as File System
Installer->>ProDetector: Check pro availability
ProDetector-->>Installer: Return pro status
alt Pro Available
Installer->>Hooks: Select HOOKS_FREE + HOOKS_PRO_ONLY
else Pro Not Available
Installer->>Hooks: Select HOOKS_FREE only
end
Hooks-->>Installer: Return hook list
Installer->>FileSystem: Copy selected hooks to project
FileSystem-->>Installer: Return copied file paths
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/installer/conditional-hooks.test.js (2)
17-23: Switch test imports to absolute paths per repo standard.The new test file introduces relative imports for both the SUT and mocked module.
As per coding guidelines
**/*.{js,jsx,ts,tsx}: Use absolute imports instead of relative imports in all code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/installer/conditional-hooks.test.js` around lines 17 - 23, Tests use relative requires for the system-under-test and mock; replace the two relative import paths with the repo-standard absolute paths so the SUT symbol copyClaudeHooksFolder is required from the absolute package path (instead of '../../packages/installer/src/wizard/ide-config-generator') and the mocked pro-detector module is required via its absolute module path (instead of '../../bin/utils/pro-detector'), updating the jest.mock and require calls accordingly.
40-60: Strengthen these tests to assert the exact hook set per tier.Current assertions check a subset, so regressions in the allowlist can pass unnoticed. Prefer exact-set checks for free/pro outputs.
Also applies to: 71-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/installer/conditional-hooks.test.js` around lines 40 - 60, Update the two tests that call copyClaudeHooksFolder so they assert the exact set of returned hook filenames for each tier instead of partial contains/notContains checks: when proDetector.isProAvailable is mocked false, assert that fileNames exactly equals the expected free-only array (e.g., ['synapse-engine.cjs','README.md'] in the correct order or as a set), and when mocked true assert fileNames exactly equals the expected pro array (e.g., ['synapse-engine.cjs','precompact-session-digest.cjs','README.md']); use deep equality (or compare as sets) to ensure no extra or missing hooks are present and update the equivalent pair of tests around the other mentioned block similarly.packages/installer/src/wizard/ide-config-generator.js (1)
698-698: Use the project’s absolute import convention forpro-detector.This newly added import is relative; align it with the absolute-import standard used by the repository.
As per coding guidelines
**/*.{js,jsx,ts,tsx}: Use absolute imports instead of relative imports in all code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/installer/src/wizard/ide-config-generator.js` at line 698, The new require for pro-detector uses a relative path; change it to the project's absolute-import convention while keeping the exported symbol name isProAvailable (replace const { isProAvailable } = require('../../../../bin/utils/pro-detector') with an absolute-import-style require/import that references the same module via the repo's configured root alias so other files follow the repository's absolute import rule).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/installer/src/wizard/ide-config-generator.js`:
- Around line 659-664: HOOK_EVENT_MAP still references "code-intel-pretool.cjs"
but the new HOOKS_FREE/tier allowlists removed it, so the file never gets copied
and its hook can't be installed; update the arrays used to determine which hook
files are copied (the HOOKS_FREE constant and the tier allowlist arrays
referenced around the same area) to include "code-intel-pretool.cjs" (or
alternatively ensure the tier allowlist logic includes it) so that the file is
copied and the mapping in HOOK_EVENT_MAP can be installed correctly.
- Around line 697-702: Wrap the pro-detection require and call in a try/catch so
any thrown error from require('../../../../bin/utils/pro-detector') or
isProAvailable() is caught and the code falls back to free hooks; specifically,
protect the isProAvailable import and invocation (used to compute isPro and
HOOKS_TO_COPY) by defaulting isPro to false on error, optionally logging a
warning, and then compute HOOKS_TO_COPY from HOOKS_FREE and HOOKS_PRO_ONLY as
before.
---
Nitpick comments:
In `@packages/installer/src/wizard/ide-config-generator.js`:
- Line 698: The new require for pro-detector uses a relative path; change it to
the project's absolute-import convention while keeping the exported symbol name
isProAvailable (replace const { isProAvailable } =
require('../../../../bin/utils/pro-detector') with an absolute-import-style
require/import that references the same module via the repo's configured root
alias so other files follow the repository's absolute import rule).
In `@tests/installer/conditional-hooks.test.js`:
- Around line 17-23: Tests use relative requires for the system-under-test and
mock; replace the two relative import paths with the repo-standard absolute
paths so the SUT symbol copyClaudeHooksFolder is required from the absolute
package path (instead of
'../../packages/installer/src/wizard/ide-config-generator') and the mocked
pro-detector module is required via its absolute module path (instead of
'../../bin/utils/pro-detector'), updating the jest.mock and require calls
accordingly.
- Around line 40-60: Update the two tests that call copyClaudeHooksFolder so
they assert the exact set of returned hook filenames for each tier instead of
partial contains/notContains checks: when proDetector.isProAvailable is mocked
false, assert that fileNames exactly equals the expected free-only array (e.g.,
['synapse-engine.cjs','README.md'] in the correct order or as a set), and when
mocked true assert fileNames exactly equals the expected pro array (e.g.,
['synapse-engine.cjs','precompact-session-digest.cjs','README.md']); use deep
equality (or compare as sets) to ensure no extra or missing hooks are present
and update the equivalent pair of tests around the other mentioned block
similarly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 00df0bad-e84d-4772-8c6b-6f1eb84734e4
📒 Files selected for processing (3)
.aiox-core/install-manifest.yamlpackages/installer/src/wizard/ide-config-generator.jstests/installer/conditional-hooks.test.js
Comment on lines
+659
to
+664
| const HOOKS_FREE = [ | ||
| 'synapse-engine.cjs', | ||
| 'synapse-wrapper.cjs', | ||
| 'precompact-wrapper.cjs', | ||
| 'README.md', | ||
| ]; |
There was a problem hiding this comment.
code-intel-pretool.cjs is never copied, so its mapped hook can’t be installed.
HOOK_EVENT_MAP still defines code-intel-pretool.cjs, but the new tier allowlists omit it entirely. That silently drops PreToolUse coverage in installed projects.
💡 Proposed fix
const HOOKS_FREE = [
'synapse-engine.cjs',
+ 'code-intel-pretool.cjs',
'synapse-wrapper.cjs',
'precompact-wrapper.cjs',
'README.md',
];Also applies to: 670-672
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/installer/src/wizard/ide-config-generator.js` around lines 659 -
664, HOOK_EVENT_MAP still references "code-intel-pretool.cjs" but the new
HOOKS_FREE/tier allowlists removed it, so the file never gets copied and its
hook can't be installed; update the arrays used to determine which hook files
are copied (the HOOKS_FREE constant and the tier allowlist arrays referenced
around the same area) to include "code-intel-pretool.cjs" (or alternatively
ensure the tier allowlist logic includes it) so that the file is copied and the
mapping in HOOK_EVENT_MAP can be installed correctly.
Comment on lines
+697
to
+702
| // Detecta se pro/ está disponível para decidir quais hooks copiar (#544) | ||
| const { isProAvailable } = require('../../../../bin/utils/pro-detector'); | ||
| const isPro = isProAvailable(); | ||
| const HOOKS_TO_COPY = isPro | ||
| ? [...HOOKS_FREE, ...HOOKS_PRO_ONLY] | ||
| : HOOKS_FREE; |
There was a problem hiding this comment.
Guard pro detection failures and default to free hooks.
If pro-detector throws, this path currently fails hard during installer execution. Safer behavior is fail-open to free-tier hook copy.
💡 Proposed fix
- const { isProAvailable } = require('../../../../bin/utils/pro-detector');
- const isPro = isProAvailable();
+ let isPro = false;
+ try {
+ const { isProAvailable } = require('../../../../bin/utils/pro-detector');
+ isPro = Boolean(isProAvailable());
+ } catch (_) {
+ isPro = false;
+ }
const HOOKS_TO_COPY = isPro
? [...HOOKS_FREE, ...HOOKS_PRO_ONLY]
: HOOKS_FREE;As per coding guidelines **/*.js: Verify error handling is comprehensive.
📝 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.
Suggested change
| // Detecta se pro/ está disponível para decidir quais hooks copiar (#544) | |
| const { isProAvailable } = require('../../../../bin/utils/pro-detector'); | |
| const isPro = isProAvailable(); | |
| const HOOKS_TO_COPY = isPro | |
| ? [...HOOKS_FREE, ...HOOKS_PRO_ONLY] | |
| : HOOKS_FREE; | |
| // Detecta se pro/ está disponível para decidir quais hooks copiar (`#544`) | |
| let isPro = false; | |
| try { | |
| const { isProAvailable } = require('../../../../bin/utils/pro-detector'); | |
| isPro = Boolean(isProAvailable()); | |
| } catch (_) { | |
| isPro = false; | |
| } | |
| const HOOKS_TO_COPY = isPro | |
| ? [...HOOKS_FREE, ...HOOKS_PRO_ONLY] | |
| : HOOKS_FREE; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/installer/src/wizard/ide-config-generator.js` around lines 697 -
702, Wrap the pro-detection require and call in a try/catch so any thrown error
from require('../../../../bin/utils/pro-detector') or isProAvailable() is caught
and the code falls back to free hooks; specifically, protect the isProAvailable
import and invocation (used to compute isPro and HOOKS_TO_COPY) by defaulting
isPro to false on error, optionally logging a warning, and then compute
HOOKS_TO_COPY from HOOKS_FREE and HOOKS_PRO_ONLY as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resumo
Implementa instalação condicional de hooks baseada na disponibilidade do
aios-pro(issue #544).Problema
precompact-session-digest.cjsera copiado para todos os projetos, mesmo free/communitysettings.local.jsonSolução
HOOKS_FREE(synapse, wrappers, README) eHOOKS_PRO_ONLY(precompact-session-digest)pro-detector.isProAvailable()em runtime para decidir quais hooks copiarArquivos modificados
packages/installer/src/wizard/ide-config-generator.jsHOOKS_TO_COPYvia pro-detectortests/installer/conditional-hooks.test.jsCloses #544
Plano de teste
npx jest tests/installer/conditional-hooks.test.js— 4/4 passandopro-detectorpara simular ambos os tiersSummary by CodeRabbit
New Features
Tests