Conversation
- Detect Copilot CLI hooks from ~/.copilot/hooks/ and .github/hooks/ - Add Copilot CLI version detection with timeout handling - Update README with global hook documentation - Add comprehensive tests for Copilot CLI detection
Add documentation for both hook files and inline hooks config styles, including version requirements and configuration precedence (user -> repo -> local).
Adds tests for: - Global config.json inline hooks at minimum supported version - Repository hooks path being a file instead of a directory
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds GitHub Copilot CLI detection to the doctor command: probes Copilot CLI version with timeouts, discovers Copilot hook sources (repo/user hook files and inline settings), applies semver gating and disable precedence, runs self-test when configured, surfaces Copilot in formatted output, updates types, tests, and README docs. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Doctor CLI
participant Doctor as detectAllHooks()
participant Sys as getSystemInfo()
participant FS as File System
participant Parser as Config Parser
participant SemVer as SemVer Checker
participant SelfTest as Self-Test Runner
CLI->>Sys: getSystemInfo()
Sys->>CLI: return SystemInfo (includes copilotCliVersion)
CLI->>Doctor: detectAllHooks(cwd, {copilotCliVersion})
Note over Doctor: discover sources: repo hooks, user hooks, inline settings
Doctor->>FS: read `.github/hooks/*.json`
FS-->>Doctor: file list / contents
Doctor->>Parser: parse each hook file
Parser-->>Doctor: configs / parse errors
Doctor->>FS: read `<COPILOT_HOME>/hooks/*.json`
FS-->>Doctor: file list / contents
Doctor->>Parser: parse each hook file
Parser-->>Doctor: configs / parse errors
Doctor->>FS: read inline config files (`settings.local.json`, `settings.json`, `config.json`)
FS-->>Doctor: contents
Doctor->>Parser: parse inline configs
Parser-->>Doctor: configs / parse errors
Doctor->>SemVer: check copilotCliVersion against required thresholds
SemVer-->>Doctor: supported / unsupported
alt supported & relevant config found
Doctor->>Parser: evaluate disableAllHooks precedence
alt disabled
Doctor-->>CLI: status 'disabled' (include configPaths/errors)
else
Doctor->>SelfTest: runSelfTest() for Safety Net
SelfTest-->>Doctor: pass/fail
Doctor-->>CLI: status 'configured' (include configPaths/errors)
end
else unsupported or none found
Doctor-->>CLI: status 'n/a' (include errors if any)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
Greptile SummaryThis PR extends the Key changes:
Issue to address: The Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[detectCopilot called] --> B[_getCopilotConfigHome]
B --> C[_supportsCopilotInlineHooks\nversion]
C --> D{inlineSupport?}
D -- "true (≥1.0.8)" --> E[collectCopilotInlineConfig\nfor all 3 sources\nerrors forwarded]
D -- "false / null" --> F[collectCopilotInlineConfig\nfor all 3 sources\nerrors suppressed]
E --> G{_resolveCopilotInlineDisableSource}
G -- "disableAllHooks: true found" --> H[return disabledBy\nstatus: disabled]
G -- "not disabled" --> I[collect repoHookPaths]
F --> I
I --> J[_supportsCopilotUserHookFiles\nversion]
J --> K{userHookSupport?}
K -- "true (≥0.0.422)" --> L[collect userHookPaths\nerrors forwarded]
K -- "false / null" --> M[collect userHookPaths\nerrors suppressed]
L --> N{Safety Net hooks\nin user files?}
M --> N
N -- "yes & support!=true" --> O[warn unsupported\nclear userHookPaths]
N -- "yes & support==true" --> P[keep userHookPaths]
N -- "no" --> P
O --> Q[iterate inlineSourcesByPrecedence]
P --> Q
Q --> R{Safety Net hook\nin source?}
R -- "no" --> Q
R -- "yes & inlineSupport==true" --> S[add to inlinePaths]
R -- "yes & inlineSupport!=true" --> T[warn unsupported\nbreak loop]
S --> Q
T --> U[build activeConfigPaths\nlocal→repo→hookFiles→user]
Q -- done --> U
U --> V{activeConfigPaths\nnon-empty?}
V -- "yes" --> W[return configured\n+ selfTest]
V -- "no" --> X[return n/a]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tests/helpers.ts (1)
66-70: MarkmockVersionFetcheras internal.This helper is exported only for tests. Adding the standard JSDoc here keeps knip from treating it as public API.
Suggested tweak
/** * Mock version fetcher for testing. * Returns predefined versions instantly without spawning processes. + * `@internal` Exported for testing */ export const mockVersionFetcher: VersionFetcher = async (args: string[]) => {As per coding guidelines: For test-only exports, add
/**@internalExported for testing */JSDoc comment to satisfy knip.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers.ts` around lines 66 - 70, Mark the exported test helper mockVersionFetcher as internal by adding the JSDoc tag /** `@internal` Exported for testing */ immediately above the export declaration for mockVersionFetcher so knip treats it as non-public; locate the export const mockVersionFetcher: VersionFetcher = ... and insert that comment directly above it.src/bin/doctor/types.ts (1)
42-42: MakeconfigPathsreadonly.
HookStatusis report data, so callers should not need to mutate this array. Usingreadonly string[]preserves assignability for producers and makes the new field safer for consumers.Suggested tweak
- configPaths?: string[]; + configPaths?: readonly string[];As per coding guidelines: Use
readonlyarrays where mutation isn't needed in TypeScript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bin/doctor/types.ts` at line 42, HookStatus currently exposes configPaths as a mutable array (configPaths?: string[]) but this is report data and should be immutable for consumers; update the HookStatus type declaration to use a readonly array (configPaths?: readonly string[]) so producers can still assign plain string[] while preventing downstream mutation, and ensure any code constructing HookStatus continues to accept/assign string[] without changes to runtime behavior.tests/bin/doctor/hooks.test.ts (1)
12-60: Prefix the local test helpers with_.
writeCopilotHookandwriteCopilotInlineConfigare file-local helpers, so_writeCopilotHook/_writeCopilotInlineConfigwould match the repo’s TS naming rule.As per coding guidelines, "Use leading underscore for private/internal module functions in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bin/doctor/hooks.test.ts` around lines 12 - 60, Rename the two file-local helper functions to use a leading underscore to follow the TS naming rule: change writeCopilotHook -> _writeCopilotHook and writeCopilotInlineConfig -> _writeCopilotInlineConfig, and update all internal references/calls within the same file (tests/bin/doctor/hooks.test.ts) to the new names; keep their signatures and behavior identical.src/bin/doctor/hooks.ts (1)
426-658: Prefix the new module-private helpers with_.These helpers are internal to this module, so public-style names drift from the repo’s TypeScript naming convention.
As per coding guidelines, "Use leading underscore for private/internal module functions in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bin/doctor/hooks.ts` around lines 426 - 658, The helpers in this module should be renamed to use a leading underscore to mark them as module-private; rename the functions isSafetyNetCopilotCommand, parseSemver, compareSemver, supportsCopilotUserHookFiles, supportsCopilotInlineHooks, getCopilotConfigHome, hasSafetyNetCopilotHook, readCopilotConfigFile, listJsonFiles, collectSafetyNetCopilotHookFiles, collectCopilotInlineConfig, warnOnUnsupportedCopilotSource, resolveCopilotInlineDisableSource, and checkCopilotEnabled to _isSafetyNetCopilotCommand, _parseSemver, _compareSemver, _supportsCopilotUserHookFiles, _supportsCopilotInlineHooks, _getCopilotConfigHome, _hasSafetyNetCopilotHook, _readCopilotConfigFile, _listJsonFiles, _collectSafetyNetCopilotHookFiles, _collectCopilotInlineConfig, _warnOnUnsupportedCopilotSource, _resolveCopilotInlineDisableSource, and _checkCopilotEnabled respectively, and update every call site in this file to use the new names so imports/exports aren’t affected. Ensure there are no lingering references to the old names and run type-checks to catch any missed usages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 212-223: Update the README text that currently reads "Copilot
settings cascade from user -> repository -> local. `disableAllHooks: true`
disables both repo-level and user-level hooks." to explicitly state the
precedence direction so it's unambiguous which setting wins; for example, change
it to something like "Copilot settings cascade from user -> repository -> local
(later files override earlier ones — i.e., local overrides repository overrides
user). `disableAllHooks: true` disables both repo-level and user-level hooks."
Make this edit around the sentence containing "Copilot settings cascade from
user -> repository -> local" and keep the `disableAllHooks` reference intact.
In `@src/bin/doctor/hooks.ts`:
- Around line 614-619: The warning currently hardcodes '~/.copilot/hooks';
update the call to warnOnUnsupportedCopilotSource so it uses the resolved
Copilot home path (e.g. pass userHookDir or configHome) instead of the literal
string. Locate the call to warnOnUnsupportedCopilotSource in this file (the
invocation with errors and copilotCliVersion) and replace the hardcoded 'user
hook files in ~/.copilot/hooks' message with a message that includes the
resolved userHookDir/configHome variable so the doctor points to the actual hook
directory.
- Around line 602-620: The warning for unsupported Copilot sources should only
be emitted when at least one config actually affects Safety Net; instead of
warning whenever there are JSON files, scan the userHookFiles with
readCopilotConfigFile and hasSafetyNetCopilotHook and only call
warnOnUnsupportedCopilotSource if any file returns true (update the logic around
userHookFiles/userHookSupport/userHookPaths and similarly the other block at
630-646 that handles inline Copilot configs), keeping the existing
paths/variables (userHookDir, userHookFiles, userHookPaths,
readCopilotConfigFile, hasSafetyNetCopilotHook, warnOnUnsupportedCopilotSource,
copilotCliVersion) to locate and gate the warning.
---
Nitpick comments:
In `@src/bin/doctor/hooks.ts`:
- Around line 426-658: The helpers in this module should be renamed to use a
leading underscore to mark them as module-private; rename the functions
isSafetyNetCopilotCommand, parseSemver, compareSemver,
supportsCopilotUserHookFiles, supportsCopilotInlineHooks, getCopilotConfigHome,
hasSafetyNetCopilotHook, readCopilotConfigFile, listJsonFiles,
collectSafetyNetCopilotHookFiles, collectCopilotInlineConfig,
warnOnUnsupportedCopilotSource, resolveCopilotInlineDisableSource, and
checkCopilotEnabled to _isSafetyNetCopilotCommand, _parseSemver, _compareSemver,
_supportsCopilotUserHookFiles, _supportsCopilotInlineHooks,
_getCopilotConfigHome, _hasSafetyNetCopilotHook, _readCopilotConfigFile,
_listJsonFiles, _collectSafetyNetCopilotHookFiles, _collectCopilotInlineConfig,
_warnOnUnsupportedCopilotSource, _resolveCopilotInlineDisableSource, and
_checkCopilotEnabled respectively, and update every call site in this file to
use the new names so imports/exports aren’t affected. Ensure there are no
lingering references to the old names and run type-checks to catch any missed
usages.
In `@src/bin/doctor/types.ts`:
- Line 42: HookStatus currently exposes configPaths as a mutable array
(configPaths?: string[]) but this is report data and should be immutable for
consumers; update the HookStatus type declaration to use a readonly array
(configPaths?: readonly string[]) so producers can still assign plain string[]
while preventing downstream mutation, and ensure any code constructing
HookStatus continues to accept/assign string[] without changes to runtime
behavior.
In `@tests/bin/doctor/hooks.test.ts`:
- Around line 12-60: Rename the two file-local helper functions to use a leading
underscore to follow the TS naming rule: change writeCopilotHook ->
_writeCopilotHook and writeCopilotInlineConfig -> _writeCopilotInlineConfig, and
update all internal references/calls within the same file
(tests/bin/doctor/hooks.test.ts) to the new names; keep their signatures and
behavior identical.
In `@tests/helpers.ts`:
- Around line 66-70: Mark the exported test helper mockVersionFetcher as
internal by adding the JSDoc tag /** `@internal` Exported for testing */
immediately above the export declaration for mockVersionFetcher so knip treats
it as non-public; locate the export const mockVersionFetcher: VersionFetcher =
... and insert that comment directly above it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca34aa8c-8685-4059-8271-b38436fb8369
⛔ Files ignored due to path filters (3)
dist/bin/cc-safety-net.jsis excluded by!**/dist/**dist/bin/doctor/hooks.d.tsis excluded by!**/dist/**dist/bin/doctor/types.d.tsis excluded by!**/dist/**
📒 Files selected for processing (10)
README.mdsrc/bin/doctor/format.tssrc/bin/doctor/hooks.tssrc/bin/doctor/index.tssrc/bin/doctor/system-info.tssrc/bin/doctor/types.tstests/bin/doctor/format.test.tstests/bin/doctor/hooks.test.tstests/bin/doctor/system-info.test.tstests/helpers.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #35 +/- ##
==========================================
+ Coverage 99.50% 99.53% +0.02%
==========================================
Files 50 50
Lines 5085 5330 +245
==========================================
+ Hits 5060 5305 +245
Misses 25 25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/bin/doctor/system-info.test.ts (1)
106-136: Consider increasing the test timeout margin.The test uses a 100ms timeout (
}, 100)) to verify thatgetSystemInfodoesn't wait for the fallback--versionprobe when--binary-versionsucceeds. While this works, the tight timeout could cause flaky failures on slower CI runners.💡 Suggestion
- }, 100); + }, 500);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bin/doctor/system-info.test.ts` around lines 106 - 136, The test "does not wait for copilot --version when --binary-version succeeds" uses a 100ms Jest timeout which is too tight and can cause flakes on slow CI; update the test timeout (the final argument to the test declaration) to a larger value (e.g., 1000ms or 2000ms) so getSystemInfo(fetcher) with deferred promises (binaryVersion and fallbackVersion) has enough time to resolve and assertions on copilotCliVersion succeed reliably.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/bin/doctor/system-info.test.ts`:
- Around line 106-136: The test "does not wait for copilot --version when
--binary-version succeeds" uses a 100ms Jest timeout which is too tight and can
cause flakes on slow CI; update the test timeout (the final argument to the test
declaration) to a larger value (e.g., 1000ms or 2000ms) so
getSystemInfo(fetcher) with deferred promises (binaryVersion and
fallbackVersion) has enough time to resolve and assertions on copilotCliVersion
succeed reliably.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d65e498c-7ab2-434d-806b-765ab876af83
⛔ Files ignored due to path filters (2)
dist/bin/cc-safety-net.jsis excluded by!**/dist/**dist/bin/doctor/types.d.tsis excluded by!**/dist/**
📒 Files selected for processing (7)
README.mdsrc/bin/doctor/hooks.tssrc/bin/doctor/system-info.tssrc/bin/doctor/types.tstests/bin/doctor/hooks.test.tstests/bin/doctor/system-info.test.tstests/helpers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/helpers.ts
|
@greptileai review the PR |
src/bin/doctor/hooks.ts
Outdated
| if (inlineSupport === true) { | ||
| const disableSource = _resolveCopilotInlineDisableSource(inlineSources); | ||
| if (disableSource) { | ||
| return { activeConfigPaths: [], disabledBy: disableSource }; | ||
| } | ||
| } |
There was a problem hiding this comment.
disableAllHooks ignored when copilotCliVersion is null
The disableAllHooks check (lines 596–601) is guarded by inlineSupport === true, meaning it is entirely skipped when copilotCliVersion is null (binary absent, spawn failed, or version unparseable). In that scenario the code continues past this guard, finds any matching traditional hook files in .github/hooks/, and returns status: 'configured' — even when a disableAllHooks: true entry is present in an inline settings file that the actual Copilot CLI runtime would honor (if the binary is >= 1.0.8).
Concrete failing scenario:
- User on Copilot CLI 1.2.0, but
copilot --binary-versionandcopilot --versionboth fail to produce a parseable string →copilotCliVersion = null .github/hooks/safety-net.jsonexists ✓.github/copilot/settings.jsoncontains{ "disableAllHooks": true }✓
Doctor reports status: 'configured' (false positive) while the runtime actually disables all hooks.
A safe path is to check disableAllHooks whenever inlineSupport !== false (i.e., unknown or supported), and surface a warning when the disable was found but the version is unknown so the user can act:
// Check disableAllHooks whenever version doesn't rule out inline support
if (inlineSupport !== false) {
const disableSource = _resolveCopilotInlineDisableSource(inlineSources);
if (disableSource) {
if (inlineSupport === null) {
errors.push(
`disableAllHooks: true found in ${disableSource}, but Copilot CLI version is unknown; reporting as disabled to be safe`,
);
}
return { activeConfigPaths: [], disabledBy: disableSource };
}
}Alternatively, add a test covering this path (copilotCliVersion = null, traditional hook file present, inline disableAllHooks: true present) to at least document the known limitation explicitly.
| return { | ||
| activeConfigPaths: [ | ||
| ...inlinePaths.filter((path) => path.endsWith('settings.local.json')), | ||
| ...inlinePaths.filter((path) => path.endsWith('settings.json')), | ||
| ...repoHookPaths, | ||
| ...inlinePaths.filter((path) => path.endsWith('config.json')), | ||
| ...userHookPaths, | ||
| ], | ||
| }; |
There was a problem hiding this comment.
activeConfigPaths ordering relies on endsWith filename suffixes
The three inlinePaths.filter calls categorize paths by suffix ('settings.local.json', 'settings.json', 'config.json'). This works correctly for the three hardcoded source paths today, but it is fragile: a non-standard COPILOT_HOME whose path itself ends in one of these suffixes, or any future path addition, could silently mis-sort entries.
Consider tracking the category alongside each path (e.g. a tuple [path, 'local' | 'repo' | 'user']) as it is assembled in the inlineSourcesByPrecedence loop, then sorting on the category label rather than the file name suffix. That makes the intent explicit and removes the implicit coupling to the specific file names.
1 participant
Summary
doctor, supporting both hook-file and inline-hook configuration styles--binary-version(with--versionfallback) and display it in system infodisableAllHooksChanges
src/bin/doctor/hooks.ts— AddeddetectCopilot()detection function with support for:.github/hooks/*.json)~/.copilot/hooks/*.json, requires Copilot CLI0.0.422+)~/.copilot/config.json,.github/copilot/settings.json,.github/copilot/settings.local.json, requires Copilot CLI1.0.8+)disableAllHooksdetection with correct precedence (local → repo → user)COPILOT_HOMEenvironment variable supportparseSemver,compareSemver) for feature-gating by versionCopilotHookConfig,CopilotInlineConfigSource,CopilotDetectionStatesrc/bin/doctor/types.ts— Added'copilot-cli'toHookPlatformunion; addedconfigPathstoHookStatus; addedcopilotCliVersiontoSystemInfosrc/bin/doctor/system-info.ts— Addedcopilot --binary-version/--versionfetching with fallback logic; fixed potential race indefaultVersionFetcher(double-resolve guard viaisSettled)src/bin/doctor/format.ts— Added'copilot-cli': 'Copilot CLI'display name; added Copilot CLI row to system info tablesrc/bin/doctor/index.ts— PasscopilotCliVersionfrom system info intodetectAllHooksoptions; reordered calls so system info is fetched before hook detectionREADME.md— Rewrote Copilot CLI section with Option A (hook files) and Option B (inline hooks), version requirements,disableAllHooks, andCOPILOT_HOMEnotestests/bin/doctor/hooks.test.ts— Added ~536 lines of tests covering all Copilot CLI detection paths (hook files, inline hooks, version gating, disable flags,COPILOT_HOME, error handling, edge cases)tests/bin/doctor/system-info.test.ts— Added tests for Copilot CLI version fetching (--binary-versionpreference,--versionfallback, null handling)tests/bin/doctor/format.test.ts— Added test for Copilot CLI display name renderingtests/helpers.ts— Addedcopilotentry tomockVersionFetcherTesting
Additional manual verification:
Related Issue
PR Checklist
bun run checkpasses (lint, types, dead code, rules, tests)package.jsonSummary by CodeRabbit
New Features
Documentation
Tests