Add proactive Intent hook skill catalogs#180
Conversation
…loading and session catalog behavior
…ses for session catalog context
📝 WalkthroughWalkthrough
ChangesSessionStart Skill Catalog Hook Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 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 |
|
View your CI Pipeline Execution ↗ for commit b380827
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit b380827
☁️ Nx Cloud last updated this comment at |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/intent/src/hooks/install.ts (2)
550-553: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUnnecessary escape character in regex.
The forward slash doesn't need escaping inside a character class.
🔧 Suggested fix
function isIntentGateScriptReference(value: string): boolean { - return /(?:^|[\s"'\/])(?:old-)?intent-(claude|codex|copilot)-gate\.mjs(?:$|[?#\s"'])/i.test( + return /(?:^|[\s"'/])(?:old-)?intent-(claude|codex|copilot)-gate\.mjs(?:$|[?#\s"'])/i.test( value, ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/src/hooks/install.ts` around lines 550 - 553, The regex pattern in the isIntentGateScriptReference function contains an unnecessarily escaped forward slash inside the character class. Remove the backslash escape from the forward slash in the character class portion of the regex pattern where it matches starting delimiters, leaving just the forward slash without the escape character. This will clean up the regex without changing its functionality.Source: Linters/SAST tools
62-68: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueDefault
catalogCommandis computed at build time, not runtime.The default value
detectPackageManager()(nocwdargument) usesprocess.cwd()at the momentbuildHookRunnerScriptis called. If this function is invoked in a context whereprocess.cwd()differs from the target repository root (e.g., during tests or global installation), the wrong package manager may be detected and baked into the generated script.This is mitigated because
installAgentHookexplicitly passesdetectPackageManager(root)at line 352-353. However, direct callers relying on the default (like the test athooks-install.test.ts:314) may get unexpected results if their working directory differs from the intended root.Consider documenting this or making
catalogCommanda required parameter to avoid accidental misuse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/src/hooks/install.ts` around lines 62 - 68, The default value for the `catalogCommand` parameter in the `buildHookRunnerScript` function is computed at call time using `detectPackageManager()` without specifying a working directory, which means it uses the current `process.cwd()`. This can cause incorrect package manager detection if the function is invoked from a different working directory than the target repository root. Either make the `catalogCommand` parameter required (removing the default value) to force callers to explicitly pass the correct command with the proper context, or add clear documentation explaining that the default value depends on the current working directory and may not be suitable for all calling contexts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/intent/src/hooks/install.ts`:
- Around line 550-553: The regex pattern in the isIntentGateScriptReference
function contains an unnecessarily escaped forward slash inside the character
class. Remove the backslash escape from the forward slash in the character class
portion of the regex pattern where it matches starting delimiters, leaving just
the forward slash without the escape character. This will clean up the regex
without changing its functionality.
- Around line 62-68: The default value for the `catalogCommand` parameter in the
`buildHookRunnerScript` function is computed at call time using
`detectPackageManager()` without specifying a working directory, which means it
uses the current `process.cwd()`. This can cause incorrect package manager
detection if the function is invoked from a different working directory than the
target repository root. Either make the `catalogCommand` parameter required
(removing the default value) to force callers to explicitly pass the correct
command with the proper context, or add clear documentation explaining that the
default value depends on the current working directory and may not be suitable
for all calling contexts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d06ef29-ba71-4603-b9c4-25f91838a742
📒 Files selected for processing (6)
.changeset/mighty-friends-doubt.mddocs/cli/intent-hooks.mddocs/getting-started/quick-start-consumers.mdpackages/intent/src/cli.tspackages/intent/src/hooks/install.tspackages/intent/tests/hooks-install.test.ts
1 participant
Summary
intent loadbefore editingSummary by CodeRabbit
New Features
intent.skillsandintent.excludeconfigurationDocumentation