Merged
Conversation
📝 WalkthroughWalkthroughThe PR adds a public library entry point ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Comment |
Fcmam5
force-pushed
the
fix/fix-linting-and-add-docstrings
branch
from
361c3bb to
cbb3ab5
Compare
![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 (7)
src/core/git-ref-resolver.ts (1)
14-24: Constructor parameter docs on class JSDoc.The
@paramtags forgitBinaryandrunare placed on the class-level JSDoc rather than on the constructor. While TypeDoc handles this, the conventional placement is on the constructor itself:/** * Resolves Git refs (branches, tags, SHAs) to full commit SHAs using `git ls-remote`. * Supports both full and short SHA prefixes. - * `@param` gitBinary The git binary to use (default: 'git') - * `@param` run The execa runner to use (default: defaultRunner) */ export class GitRefResolver { + /** + * `@param` _gitBinary The git binary to use (default: 'git') + * `@param` _run The execa runner to use (default: defaultRunner) + */ constructor(This is a minor style nit; current placement works fine with TypeDoc.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/git-ref-resolver.ts` around lines 14 - 24, Move the `@param` JSDoc tags from the class-level comment to the constructor JSDoc for GitRefResolver: add a brief JSDoc block immediately above the constructor describing parameters _gitBinary (string, default 'git') and _run (ExecaRunner, defaultRunner) so the constructor—not the class—documents its parameters; reference the constructor signature (private readonly _gitBinary: string = 'git', private readonly _run: ExecaRunner = defaultRunner) and keep the class-level JSDoc for the general description only.tests/git-ref-resolver.test.ts (1)
22-24: Redundant cast on conforming object literal.The object
{ stdout: '...' }already matches the{ stdout: string }type, making theas unknown as { stdout: string }cast unnecessary. This applies to lines 24, 40, and 48.That said, if this is intentional for consistency with other test files in this PR, it's a minor style choice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/git-ref-resolver.test.ts` around lines 22 - 24, Remove the unnecessary double-cast on the mock resolved values: the object literal passed to mockRunner.mockResolvedValue already matches the { stdout: string } shape, so delete the "as unknown as { stdout: string }" wrappers for the calls to mockRunner.mockResolvedValue (the instances creating { stdout: '...'} in tests/git-ref-resolver.test.ts) — leave the plain object literal (e.g., { stdout: 'abc123...' }) to keep the test types correct and cleaner.src/commands/types.ts (1)
16-16: Underscore prefix unusual for interface method signatures.The
_envand_argsnaming convention is typically used for unused parameters in implementations to suppress linter warnings. In interface definitions, all parameters are inherently "unused" (they're just type signatures), so this convention doesn't serve the same purpose and may confuse implementers.Consider using plain parameter names in the interface:
- run(_env: SkilletonEnvironment, _args: CommandArgs): Promise<void>; + run(env: SkilletonEnvironment, args: CommandArgs): Promise<void>;If your linter requires this, it may need configuration adjustment for interface definitions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/types.ts` at line 16, In the Command interface method signature, remove the leading underscores from the parameter names so the declaration reads run(env: SkilletonEnvironment, args: CommandArgs): Promise<void>; — update the signature for function run and types SkilletonEnvironment / CommandArgs in src/commands/types.ts (the interface declaration) to use plain parameter names (env, args) rather than _env and _args to avoid misleading unused-parameter convention.src/core/config.ts (1)
48-51: Schema URL ties manifests to the exact package version.The
schemaUrl()function embedspackageJson.versioninto the schema URL. Per the relevant snippet insrc/core/manifest.ts, this URL is written to user manifests when$schemais missing. During development or pre-release versions (e.g.,0.2.0-beta.1), this could produce URLs pointing to non-existent tags.Consider whether this behavior is intentional, or if you want to pin to a stable schema URL or validate that the version tag exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/config.ts` around lines 48 - 51, The schemaUrl() function currently embeds packageJson.version into the GitHub raw URL which can point to non-existent tags for pre-releases; update schemaUrl (and the manifest-writing path in manifest.ts) to avoid emitting non-existent-version URLs by either: 1) returning a stable URL (e.g., pin to "main" or a "latest" non-versioned path) for prerelease versions, or 2) validating that the tag v{packageJson.version} exists (via a lightweight HEAD request to the raw URL or GitHub tags API) and falling back to a stable URL when it does not; implement the check inside schemaUrl() (or call a helper like isVersionTagged()) so manifest.ts uses the safe URL.src/core/resolve.ts (1)
13-13: Underscore prefix on used parameter is misleading.The
_refResolverunderscore prefix typically indicates an intentionally unused parameter (common in TypeScript for interface compliance). However, this parameter is actively used on line 39. Consider removing the underscore prefix to follow conventional naming.Suggested fix
- constructor(private readonly _refResolver: GitRefResolver) {} + constructor(private readonly refResolver: GitRefResolver) {}And update line 39:
- const commit = await this._refResolver.resolve(descriptor.repo, descriptor.ref); + const commit = await this.refResolver.resolve(descriptor.repo, descriptor.ref);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/resolve.ts` at line 13, The constructor parameter _refResolver is misleadingly underscored despite being used; rename the parameter to refResolver and update all references inside the class (e.g., change this._refResolver to this.refResolver, including the usage in resolveBranch/resolveCommit or the method at the former line 39) so naming follows conventions and avoids implying the parameter is unused.src/index.ts (1)
23-29: ExportReconcileLockfileResultfrom the root API.Since
reconcileLockfileis public, exporting its result type from the same root entrypoint improves consumer ergonomics and keeps the API surface complete.♻️ Proposed export addition
export { serializeLockfile, getChangedSkills, pruneLockfile, reconcileLockfile, } from './core/lock'; +export type { ReconcileLockfileResult } from './core/lock';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 23 - 29, Add and re-export the public result type from the root API so consumers can import it alongside reconcileLockfile; specifically, export ReconcileLockfileResult from the same module that currently exports reconcileLockfile (the index exports for serializeLockfile, getChangedSkills, pruneLockfile, reconcileLockfile) — update the export list to include ReconcileLockfileResult (ensuring the type is imported/available from the ./core/lock module) so the root entrypoint re-exports it.eslint.config.cjs (1)
10-17: ChangesourceType: 'commonjs'tosourceType: 'module'for TypeScript files.The codebase uses ES module syntax (
import/export) throughout, but the ESLint configuration parses TypeScript files as CommonJS. This mismatch can cause parser behavior issues and incorrect lint results.Configuration fix
parserOptions: { ecmaVersion: 'latest', - sourceType: 'commonjs', + sourceType: 'module', },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.cjs` around lines 10 - 17, The ESLint config parses TypeScript files as CommonJS; change languageOptions.parserOptions.sourceType from 'commonjs' to 'module' for the TypeScript file pattern (files: ['**/*.ts']) so the parser (tseslint.parser) correctly handles import/export syntax; update the sourceType value in the languageOptions block accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 8-9: Remove the redundant .skilleton entry from the .gitignore so
it only appears once; locate the duplicate ".skilleton" line in the file and
delete that second occurrence, leaving the original ignore entry intact (ensure
other entries like "docs/api" remain unchanged).
---
Nitpick comments:
In `@eslint.config.cjs`:
- Around line 10-17: The ESLint config parses TypeScript files as CommonJS;
change languageOptions.parserOptions.sourceType from 'commonjs' to 'module' for
the TypeScript file pattern (files: ['**/*.ts']) so the parser (tseslint.parser)
correctly handles import/export syntax; update the sourceType value in the
languageOptions block accordingly.
In `@src/commands/types.ts`:
- Line 16: In the Command interface method signature, remove the leading
underscores from the parameter names so the declaration reads run(env:
SkilletonEnvironment, args: CommandArgs): Promise<void>; — update the signature
for function run and types SkilletonEnvironment / CommandArgs in
src/commands/types.ts (the interface declaration) to use plain parameter names
(env, args) rather than _env and _args to avoid misleading unused-parameter
convention.
In `@src/core/config.ts`:
- Around line 48-51: The schemaUrl() function currently embeds
packageJson.version into the GitHub raw URL which can point to non-existent tags
for pre-releases; update schemaUrl (and the manifest-writing path in
manifest.ts) to avoid emitting non-existent-version URLs by either: 1) returning
a stable URL (e.g., pin to "main" or a "latest" non-versioned path) for
prerelease versions, or 2) validating that the tag v{packageJson.version} exists
(via a lightweight HEAD request to the raw URL or GitHub tags API) and falling
back to a stable URL when it does not; implement the check inside schemaUrl()
(or call a helper like isVersionTagged()) so manifest.ts uses the safe URL.
In `@src/core/git-ref-resolver.ts`:
- Around line 14-24: Move the `@param` JSDoc tags from the class-level comment to
the constructor JSDoc for GitRefResolver: add a brief JSDoc block immediately
above the constructor describing parameters _gitBinary (string, default 'git')
and _run (ExecaRunner, defaultRunner) so the constructor—not the class—documents
its parameters; reference the constructor signature (private readonly
_gitBinary: string = 'git', private readonly _run: ExecaRunner = defaultRunner)
and keep the class-level JSDoc for the general description only.
In `@src/core/resolve.ts`:
- Line 13: The constructor parameter _refResolver is misleadingly underscored
despite being used; rename the parameter to refResolver and update all
references inside the class (e.g., change this._refResolver to this.refResolver,
including the usage in resolveBranch/resolveCommit or the method at the former
line 39) so naming follows conventions and avoids implying the parameter is
unused.
In `@src/index.ts`:
- Around line 23-29: Add and re-export the public result type from the root API
so consumers can import it alongside reconcileLockfile; specifically, export
ReconcileLockfileResult from the same module that currently exports
reconcileLockfile (the index exports for serializeLockfile, getChangedSkills,
pruneLockfile, reconcileLockfile) — update the export list to include
ReconcileLockfileResult (ensuring the type is imported/available from the
./core/lock module) so the root entrypoint re-exports it.
In `@tests/git-ref-resolver.test.ts`:
- Around line 22-24: Remove the unnecessary double-cast on the mock resolved
values: the object literal passed to mockRunner.mockResolvedValue already
matches the { stdout: string } shape, so delete the "as unknown as { stdout:
string }" wrappers for the calls to mockRunner.mockResolvedValue (the instances
creating { stdout: '...'} in tests/git-ref-resolver.test.ts) — leave the plain
object literal (e.g., { stdout: 'abc123...' }) to keep the test types correct
and cleaner.
🪄 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: 62e9b5a7-36c9-4913-82ba-be443a3da942
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (43)
.github/FUNDING.yml.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/feature_request.md.github/PULL_REQUEST_TEMPLATE.md.github/workflows/codeql.yaml.github/workflows/release.yml.gitignore.vscode/settings.jsonCODE_OF_CONDUCT.mdREADME.mdSECURITY.mdeslint.config.cjspackage.jsonsrc/adapters/git.tssrc/commands/add.tssrc/commands/audit.tssrc/commands/install.tssrc/commands/list.tssrc/commands/types.tssrc/commands/update.tssrc/core/config.tssrc/core/errors.tssrc/core/filesystem.tssrc/core/git-ref-resolver.tssrc/core/install.tssrc/core/lock.tssrc/core/logger.tssrc/core/manifest.tssrc/core/parse.tssrc/core/repos.tssrc/core/resolve.tssrc/core/types.tssrc/core/validate.tssrc/env.tssrc/index.tstests/commands.add.test.tstests/commands.install.test.tstests/commands.update.test.tstests/describe.test.tstests/flow.test.tstests/git-ref-resolver.test.tstsconfig.jsontypedoc.json
💤 Files with no reviewable changes (1)
- .github/workflows/release.yml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
eslint.config.cjs (1)
10-22: Consider widening the TypeScript file glob for future-proof rule coverage.Line 10 scopes this override to
**/*.tsonly. While the repository currently has only TypeScript files with this extension, if.tsx,.mts, or.ctsfiles are added in the future, theno-unused-varsoverride won't apply to them.♻️ Optional refactor
- files: ['**/*.ts'], + files: ['**/*.{ts,tsx,mts,cts}'],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.cjs` around lines 10 - 22, The override's files glob currently only matches '**/*.ts' so the TypeScript-specific rules ('no-unused-vars' and '@typescript-eslint/no-unused-vars') won't apply to .tsx/.mts/.cts files; update the files pattern (the files: entry in this config) to a broader glob that includes tsx, mts, and cts (e.g., a brace or multiple patterns) so the rules declared under rules: apply to those file types as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@eslint.config.cjs`:
- Around line 10-22: The override's files glob currently only matches '**/*.ts'
so the TypeScript-specific rules ('no-unused-vars' and
'@typescript-eslint/no-unused-vars') won't apply to .tsx/.mts/.cts files; update
the files pattern (the files: entry in this config) to a broader glob that
includes tsx, mts, and cts (e.g., a brace or multiple patterns) so the rules
declared under rules: apply to those file types as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: adba7a02-1baa-4c53-b6b9-26d7ae3db657
📒 Files selected for processing (9)
.gitignoreeslint.config.cjssrc/commands/types.tssrc/core/config.tssrc/core/git-ref-resolver.tssrc/core/resolve.tssrc/index.tstests/git-ref-resolver.test.tstypedoc.json
✅ Files skipped from review due to trivial changes (6)
- src/commands/types.ts
- typedoc.json
- .gitignore
- tests/git-ref-resolver.test.ts
- src/core/git-ref-resolver.ts
- src/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/resolve.ts
- src/core/config.ts
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.
Summary by CodeRabbit
New Features
Documentation
Chores