Skip to content

feat: add oo skills publish#127

Merged
BlackHole1 merged 7 commits into
mainfrom
skills-publish
Apr 30, 2026
Merged

feat: add oo skills publish#127
BlackHole1 merged 7 commits into
mainfrom
skills-publish

Conversation

@crimx
Copy link
Copy Markdown
Contributor

@crimx crimx commented Apr 30, 2026

Support publishing skills created with oo via oo skills publish.

Plans:

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 30, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f6e2db3a-30fe-46ec-aaca-7d05142b053a

📥 Commits

Reviewing files that changed from the base of the PR and between 603bcaa and ce11759.

📒 Files selected for processing (1)
  • src/application/commands/skills/package-conversion.test.ts

Summary by CodeRabbit

  • New Features

    • Added oo skills publish command to publish skills to the registry with optional visibility settings (public/private).
    • Integrated metadata validation, interactive confirmation prompts for safety checks, and automatic version management.
  • Documentation

    • Added comprehensive command documentation in English and Chinese for the new publish workflow, including requirements and behavior.
  • Tests

    • Added extensive test coverage for publish functionality, package conversion, and registry integration.

Walkthrough

Adds a new "oo skills publish " CLI command and supporting modules to convert a local SKILL.md-based skill into a deterministic npm-like package, apply per-skill .gitignore filtering, reject symlinks, strip OO-managed artifacts, validate/normalize packageName and semver versioning, produce a gzipped tarball with sha1/sha512 integrity, and publish via an authenticated HTTP PUT to the registry. The flow may prompt interactively if the remote package is blocked. On success the resolved packageName/version are written back to SKILL.md; failures leave SKILL.md unchanged. Also adds templates, i18n entries, tests, and CLI registration.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant CLI as "oo skills publish"
    participant FS as "Local FS"
    participant Auth as "Auth Service"
    participant Registry as "Registry API"

    User->>CLI: run publish <skill-id> [--visibility]
    activate CLI

    CLI->>FS: locate canonical skill dir, read SKILL.md
    FS-->>CLI: frontmatter + files

    CLI->>Auth: resolve account & apiKey
    Auth-->>CLI: account + apiKey

    CLI->>Registry: GET latest package info (blocks, latestVersion)
    Registry-->>CLI: latestVersion + blocksFlag

    alt blocksFlag && stdin is TTY
        CLI->>User: prompt confirm publish when remote has blocks [y/N]
        User-->>CLI: response
    end

    CLI->>CLI: validate frontmatter, compute finalVersion (semver / patch bump)
    CLI->>FS: convert skill -> temp package root (apply .gitignore, reject symlinks, strip OO artifacts)
    FS-->>CLI: package files

    CLI->>FS: create deterministic tarball, compute sha1/sha512
    FS-->>CLI: tarball + integrity

    CLI->>Registry: PUT publish (tarball + metadata, auth)
    Registry-->>CLI: 2xx or error

    alt success
        CLI->>FS: write metadata.packageName/version back to SKILL.md
        FS-->>CLI: write acknowledged
        CLI->>User: print success (hub URL, version)
    else failure
        CLI->>User: surface publish error (leave SKILL.md unchanged)
    end

    CLI->>FS: cleanup temporary files
    deactivate CLI
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title clearly describes the main feature being added: a new skills publish command for the oo CLI.
Description check ✅ Passed The description is related to the changeset, explaining the purpose of adding a skills publish command and referencing planning documents.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch skills-publish

Review rate limit: 3/5 reviews remaining, refill in 15 minutes and 27 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (4)
src/application/auth/login-flow.test.ts (1)

203-203: ⚡ Quick win

Extract duplicated endpoint setup into shared test-local setup.

const endpoint = "example.test" is duplicated in both fast-login tests. Hoist it to the describe scope (or a tiny local factory) to avoid drift between related scenarios.

Proposed change
 describe("startAuthLoginSession", () => {
+    const endpoint = "example.test";
+
     test("requests a fast login profile with a session token", async () => {
         const logCapture = createLogCapture();
-        const endpoint = "example.test";
         const requests: Request[] = [];
@@
     test("redacts the session token from fast login failures", async () => {
         const logCapture = createLogCapture();
-        const endpoint = "example.test";
         const sessionToken = "token with/sensitive value";

Based on learnings: Applies to **/*.test.{ts,tsx} : Extract repeated setup (mocks, stubs, setup objects) into local factory functions in test files; avoid copy-pasting test setup.

Also applies to: 265-265

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/application/auth/login-flow.test.ts` at line 203, Hoist the duplicated
const endpoint = "example.test" used in the fast-login tests into a shared
test-local setup by declaring it once in the surrounding describe scope (or
create a tiny factory function such as makeTestEndpoint) and remove the per-test
duplication; update references in both fast-login test blocks to use the shared
endpoint variable so both tests use the single source of truth (look for the
endpoint variable and the two fast-login tests to change).
docs/commands.zh-CN.md (1)

344-347: ⚡ Quick win

删除文档中的实现细节表述(PUT)。

“在转换、PUT 和本地 metadata 回写前停止”包含实现细节。建议改成用户可观察语义(例如“不会继续发布,也不会回写本地 metadata”)。

As per coding guidelines "Documentation under docs/commands*.md should describe only user-facing CLI contract ... not internal implementation details."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/commands.zh-CN.md` around lines 344 - 347, The docs line under the
"Registry 安全检查" section exposes implementation detail ("PUT") — replace the
phrase "在转换、PUT 和本地 metadata 回写前停止" with a user-facing description such as
"不会继续发布,也不会回写本地 metadata" so the document describes only observable CLI
behavior; update the text in the Registry 安全检查 paragraph to remove the word
"PUT" and similar implementation terms and use the suggested user-visible
wording instead.
src/application/commands/skills/publish.test.ts (1)

193-196: ⚡ Quick win

Extract repeated publish fixtures/stubs into local test factories.

The blocked-package response and checkAuthoringEnvironment stub are copy-pasted across several tests, which makes updates brittle.

♻️ Example refactor
+function createAuthoringEnvironmentStub() {
+    return Promise.resolve({
+        canonicalRootDirectoryPath: "",
+        hostCount: 1,
+    });
+}
+
+function createRemoteBlockedPackageResponse(): Response {
+    return new Response(JSON.stringify({
+        blocks: [
+            {
+                blockName: "main",
+                inputHandleDefs: [],
+                outputHandleDefs: [],
+            },
+        ],
+        description: "Remote block package",
+        packageName: "@alice/blocked-skill",
+        packageVersion: "1.2.3",
+        title: "Blocked Skill",
+    }));
+}

As per coding guidelines "Extract repeated setup (mocks, stubs, setup objects) into local factory functions in test files; avoid copy-pasting test setup."

Also applies to: 219-231, 258-261, 291-303, 328-331, 357-369, 392-395, 438-441, 496-499, 528-531

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/application/commands/skills/publish.test.ts` around lines 193 - 196,
Multiple tests copy the same checkAuthoringEnvironment stub and blocked-package
response fixtures which makes updates brittle; extract these repeated setups
into local factory functions (e.g., createCheckAuthoringEnvironmentStub and
createBlockedPackageResponse or similar) inside the test file and replace inline
stubs with calls to those factories; ensure factories return the same shapes
currently used by tests (the Promise-resolving object with
canonicalRootDirectoryPath and hostCount for checkAuthoringEnvironment and the
blocked-package response object), update all occurrences (the repeated blocks at
the indicated ranges) to call the factories, and keep names like
checkAuthoringEnvironment and blockedPackageResponse in tests for readability.
docs/commands.md (1)

394-399: ⚡ Quick win

Remove internal transport wording from command docs.

The phrase “stops before conversion, PUT...” leaks implementation detail. Prefer user-observable behavior only (e.g., “stops before publishing and local metadata writeback”).

As per coding guidelines "Documentation under docs/commands*.md must describe only user-facing CLI contract ... without internal implementation details."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/commands.md` around lines 394 - 399, The Registry safety paragraph
currently leaks internal transport details by saying “stops before conversion,
PUT, or local metadata writeback”; update that sentence in the docs/commands.md
Registry safety section (the paragraph that describes interactive confirmation
of existing remote blocks) to use only user-facing behavior language such as
“stops before publishing and local metadata writeback” (or similar) and remove
terms like “conversion” and “PUT”, ensuring the CLI docs under commands*.md
describe only the observable publish/metadata behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/application/commands/skills/package-conversion.ts`:
- Around line 81-86: The current skillPackageFiles constant copies the entire
skill directory into package/skills/... which can include local secret files
like .env; update the packaging logic so that skillPackageFiles (and the same
pattern at the other occurrence around the 116-122 range) excludes local secret
files by either (a) replacing the broad directory entry with an explicit
whitelist of allowed files/directories, or (b) adding a filter during the copy
step that rejects patterns such as .env, .env.*, *.local, *.secret, and any
other local-only files; make the change at the skillPackageFiles definition and
ensure the same exclusion/filter is applied where the other similar list is used
so .env and similar files are never included in the published package.
- Around line 388-395: Reject symlinks when building tar entries: before calling
readFile(entry.absolutePath) (and in the other addition code paths around the
block that later pushes entries), call fs.promises.lstat(entry.absolutePath) and
if stat.isSymbolicLink() throw a validation error; alternatively resolve
fs.promises.realpath(entry.absolutePath) and ensure the resolved path is inside
the package root (e.g. path.resolve(packageRootDirectoryPath) is a prefix of
resolved path) and throw if not. Apply this same check for all places that
follow symlinks (the code that reads files and pushes
createTarHeader/content/createTarPadding) so symlinks are either hard-rejected
or validated by realpath containment before being added to the tar.

---

Nitpick comments:
In `@docs/commands.md`:
- Around line 394-399: The Registry safety paragraph currently leaks internal
transport details by saying “stops before conversion, PUT, or local metadata
writeback”; update that sentence in the docs/commands.md Registry safety section
(the paragraph that describes interactive confirmation of existing remote
blocks) to use only user-facing behavior language such as “stops before
publishing and local metadata writeback” (or similar) and remove terms like
“conversion” and “PUT”, ensuring the CLI docs under commands*.md describe only
the observable publish/metadata behavior.

In `@docs/commands.zh-CN.md`:
- Around line 344-347: The docs line under the "Registry 安全检查" section exposes
implementation detail ("PUT") — replace the phrase "在转换、PUT 和本地 metadata 回写前停止"
with a user-facing description such as "不会继续发布,也不会回写本地 metadata" so the document
describes only observable CLI behavior; update the text in the Registry 安全检查
paragraph to remove the word "PUT" and similar implementation terms and use the
suggested user-visible wording instead.

In `@src/application/auth/login-flow.test.ts`:
- Line 203: Hoist the duplicated const endpoint = "example.test" used in the
fast-login tests into a shared test-local setup by declaring it once in the
surrounding describe scope (or create a tiny factory function such as
makeTestEndpoint) and remove the per-test duplication; update references in both
fast-login test blocks to use the shared endpoint variable so both tests use the
single source of truth (look for the endpoint variable and the two fast-login
tests to change).

In `@src/application/commands/skills/publish.test.ts`:
- Around line 193-196: Multiple tests copy the same checkAuthoringEnvironment
stub and blocked-package response fixtures which makes updates brittle; extract
these repeated setups into local factory functions (e.g.,
createCheckAuthoringEnvironmentStub and createBlockedPackageResponse or similar)
inside the test file and replace inline stubs with calls to those factories;
ensure factories return the same shapes currently used by tests (the
Promise-resolving object with canonicalRootDirectoryPath and hostCount for
checkAuthoringEnvironment and the blocked-package response object), update all
occurrences (the repeated blocks at the indicated ranges) to call the factories,
and keep names like checkAuthoringEnvironment and blockedPackageResponse in
tests for readability.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 72befac6-2c90-4ec4-871e-562b5924d126

📥 Commits

Reviewing files that changed from the base of the PR and between 06768b1 and b9702c6.

📒 Files selected for processing (14)
  • docs/commands.md
  • docs/commands.zh-CN.md
  • src/application/auth/login-flow.test.ts
  • src/application/commands/skills/index.ts
  • src/application/commands/skills/init.ts
  • src/application/commands/skills/package-conversion.test.ts
  • src/application/commands/skills/package-conversion.ts
  • src/application/commands/skills/package-templates.ts
  • src/application/commands/skills/publish.test.ts
  • src/application/commands/skills/publish.ts
  • src/application/commands/skills/registry-skill-markdown.test.ts
  • src/application/commands/skills/registry-skill-markdown.ts
  • src/application/commands/skills/skill-title.ts
  • src/i18n/catalog.ts

Comment thread src/application/commands/skills/package-conversion.ts
Comment thread src/application/commands/skills/package-conversion.ts
crimx added 3 commits April 30, 2026 11:58
- Introduced a new gitignore template file to standardize ignored files for Python and Node.js projects.
- Added a gitattributes template file for handling line endings and merge strategies.
- Refactored package-templates.ts to import the new templates directly instead of using base64 encoding.
- Updated file-imports.d.ts to declare module types for .template files.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/application/commands/skills/package-conversion.ts`:
- Around line 381-385: The validation for parsed.files in package-conversion.ts
is too permissive—invalid entries (non-strings or blank strings) are currently
filtered later in readManifestFilePaths causing silent omission; update the
check in the block that inspects parsed.files to verify every entry is a
non-empty string and throw createInvalidPackageMetadataError with a clear
message if any element fails (mentioning parsed.files and offending
index/value), or alternatively normalize/convert only when safe before calling
readManifestFilePaths; ensure readManifestFilePaths no longer silently drops
invalid entries so callers get an explicit error.
- Around line 829-838: The blocking PUT to
options.context.fetcher(options.requestUrl, ...) needs a deadline: wrap the call
with an AbortController, pass controller.signal to the fetcher call, start a
timer (e.g., 30s or a configurable options.timeout) that calls
controller.abort() on expiry, and clear the timer after the request completes;
update the options.context.fetcher(...) invocation to include the signal and
ensure any AbortError is handled/propagated appropriately so the command doesn't
hang (refer to the response variable and the
options.context.fetcher/options.requestUrl call to locate the change).
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 82bbba00-b008-4e76-ad8b-e3d505d02834

📥 Commits

Reviewing files that changed from the base of the PR and between b9702c6 and 755d875.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (9)
  • docs/commands.md
  • docs/commands.zh-CN.md
  • package.json
  • src/application/commands/skills/package-conversion.test.ts
  • src/application/commands/skills/package-conversion.ts
  • src/application/commands/skills/package-template-files/gitattributes.template
  • src/application/commands/skills/package-template-files/gitignore.template
  • src/application/commands/skills/package-templates.ts
  • src/file-imports.d.ts
✅ Files skipped from review due to trivial changes (5)
  • package.json
  • src/file-imports.d.ts
  • src/application/commands/skills/package-template-files/gitattributes.template
  • src/application/commands/skills/package-template-files/gitignore.template
  • docs/commands.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • docs/commands.zh-CN.md
  • src/application/commands/skills/package-conversion.test.ts

Comment thread src/application/commands/skills/package-conversion.ts
Comment thread src/application/commands/skills/package-conversion.ts
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (3)
src/application/commands/skills/package-conversion.ts (3)

381-385: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fail fast on invalid package.json.files entries instead of silently dropping them.

Line 381 only checks that files is an array, while Line 479 filters invalid elements out. That causes non-string/blank entries to disappear without actionable feedback.

Suggested fix
     if (parsed.files !== undefined && !Array.isArray(parsed.files)) {
         throw createInvalidPackageMetadataError(
             "Package files field must be an array if provided.",
         );
     }
+    if (
+        Array.isArray(parsed.files)
+        && parsed.files.some(filePath => !isNonBlankString(filePath))
+    ) {
+        throw createInvalidPackageMetadataError(
+            "Package files field must contain only non-empty string paths.",
+        );
+    }
 function readManifestFilePaths(manifest: PublishManifest): string[] {
-    if (manifest.files === undefined) {
-        return [];
-    }
-
-    if (!Array.isArray(manifest.files)) {
-        return [];
-    }
-
-    return manifest.files.filter(isNonBlankString);
+    return Array.isArray(manifest.files)
+        ? (manifest.files as string[])
+        : [];
 }

Also applies to: 470-479

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/application/commands/skills/package-conversion.ts` around lines 381 -
385, The code currently only validates that parsed.files is an array but later
silently filters out non-string/blank entries; instead, perform element-level
validation immediately and fail-fast: when parsed.files is defined, iterate its
elements and if any element is not a non-empty string, throw
createInvalidPackageMetadataError with a clear message listing or describing the
invalid entries; update the logic around parsed.files (and remove/replace the
later filter at the block handling files at 470-479) so invalid items are
rejected rather than dropped, referencing parsed.files and
createInvalidPackageMetadataError to locate the change.

818-839: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add a request deadline for the publish PUT call.

Line 829 performs a blocking network call without timeout/abort signal; a stalled connection can hang the command indefinitely.

Suggested fix
 async function requestSkillPackagePublish(options: {
@@
 }): Promise<Response> {
     let response: Response;
+    const abortController = new AbortController();
+    const timeoutHandle = setTimeout(() => abortController.abort(), 30_000);

     try {
         response = await options.context.fetcher(options.requestUrl, {
             body: options.body,
             headers: {
@@
                 "User-Agent": npmCompatiblePublishUserAgent,
             },
             method: "PUT",
+            signal: abortController.signal,
         });
     }
     catch (error) {
         throw new CliUserError("errors.skills.publish.requestError", 1, {
             message: getUnexpectedRequestErrorMessage(
                 error,
                 options.context.translator,
             ),
         });
     }
+    finally {
+        clearTimeout(timeoutHandle);
+    }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/application/commands/skills/package-conversion.ts` around lines 818 -
839, The requestSkillPackagePublish function performs a PUT via
options.context.fetcher without a timeout; create an AbortController, start a
timer (e.g., setTimeout) that calls controller.abort() after a reasonable
deadline, pass controller.signal to the fetcher call in
requestSkillPackagePublish, and clear the timer once the fetch completes (or on
error) so the request cannot hang indefinitely; ensure the fetch error from
abort is surfaced or translated into a clear timeout error.

123-147: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Do not rely only on user .gitignore for secret-file exclusion.

Line 123 builds filtering from the skill’s .gitignore; if that file does not exclude .env/local secret patterns, those files are copied and can be published.

Suggested fix
+const mandatorySecretIgnorePatterns = [
+    ".env",
+    ".env.*",
+    "*.local",
+    "*.secret",
+    "*.pem",
+    "*.key",
+] as const;
+
 async function createSkillPackageIgnore(
     skillDirectoryPath: string,
 ): Promise<GitIgnore> {
     const gitIgnoreContent = await readSkillGitIgnoreContent(skillDirectoryPath);

-    return ignore().add(gitIgnoreContent);
+    return ignore()
+        .add(gitIgnoreContent)
+        .add([...mandatorySecretIgnorePatterns]);
 }

Also applies to: 607-647

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/application/commands/skills/package-conversion.ts` around lines 123 -
147, The current cp filter uses createSkillPackageIgnore and
isIgnoredByGitIgnore (inside the anonymous filter passed to cp) which lets files
like .env slip through if the repo .gitignore doesn't list them; update the
filter used in the cp call so it first checks a hardcoded secret-exclusion list
(e.g., ".env", ".env.*", "*.local", ".secrets", "secrets/*", "*.pem", "*.key",
"private.*", etc.) against sourceRelativePath and returns false for matches
before calling isIgnoredByGitIgnore; reference the anonymous filter function
(the async (sourcePath) => { ... }) and ensure both file and directory patterns
are considered and that metadata.isDirectory() is still passed to
isIgnoredByGitIgnore when falling back to gitignore logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/application/commands/skills/package-conversion.ts`:
- Around line 381-385: The code currently only validates that parsed.files is an
array but later silently filters out non-string/blank entries; instead, perform
element-level validation immediately and fail-fast: when parsed.files is
defined, iterate its elements and if any element is not a non-empty string,
throw createInvalidPackageMetadataError with a clear message listing or
describing the invalid entries; update the logic around parsed.files (and
remove/replace the later filter at the block handling files at 470-479) so
invalid items are rejected rather than dropped, referencing parsed.files and
createInvalidPackageMetadataError to locate the change.
- Around line 818-839: The requestSkillPackagePublish function performs a PUT
via options.context.fetcher without a timeout; create an AbortController, start
a timer (e.g., setTimeout) that calls controller.abort() after a reasonable
deadline, pass controller.signal to the fetcher call in
requestSkillPackagePublish, and clear the timer once the fetch completes (or on
error) so the request cannot hang indefinitely; ensure the fetch error from
abort is surfaced or translated into a clear timeout error.
- Around line 123-147: The current cp filter uses createSkillPackageIgnore and
isIgnoredByGitIgnore (inside the anonymous filter passed to cp) which lets files
like .env slip through if the repo .gitignore doesn't list them; update the
filter used in the cp call so it first checks a hardcoded secret-exclusion list
(e.g., ".env", ".env.*", "*.local", ".secrets", "secrets/*", "*.pem", "*.key",
"private.*", etc.) against sourceRelativePath and returns false for matches
before calling isIgnoredByGitIgnore; reference the anonymous filter function
(the async (sourcePath) => { ... }) and ensure both file and directory patterns
are considered and that metadata.isDirectory() is still passed to
isIgnoredByGitIgnore when falling back to gitignore logic.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8f725708-21f5-4559-be0f-c97ad1db01a4

📥 Commits

Reviewing files that changed from the base of the PR and between 755d875 and e143b1c.

📒 Files selected for processing (1)
  • src/application/commands/skills/package-conversion.ts

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/application/commands/skills/package-conversion.test.ts (1)

227-339: ⚡ Quick win

Extract repeated SKILL.md/context setup into local test factories.

There’s substantial repeated setup for frontmatter markdown and publish context objects. Pulling these into local factory helpers will reduce copy-paste drift and make each test intent clearer.

As per coding guidelines, "Extract repeated setup (mocks, stubs, setup objects) into local factory functions in test files; avoid copy-pasting test setup".

Also applies to: 483-549, 588-595, 812-824

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/application/commands/skills/package-conversion.test.ts` around lines 227
- 339, Tests in package-conversion.test.ts duplicate skill/frontmatter and
directory setup across cases; extract those into local test factory helpers
(e.g., createSkillSourceDir, createPackageRootDir, buildSkillMarkdown) and a
helper to call convertSkillDirectoryToPackage with common defaults; update tests
that currently call writeSkillFile, createTemporaryDirectory, Bun.write, mkdir,
and convertSkillDirectoryToPackage (and assertions that read package.json or
package/skills paths) to use these factories so setup is centralized and
reusable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/application/commands/skills/package-conversion.test.ts`:
- Around line 451-463: Replace hardcoded POSIX separators in the test objects'
expectedPath values with platform-safe path construction (e.g., use node's
path.join or path.resolve). Specifically, update the test entries that set
expectedPath: "package/skills/demo-skill/linked-secret.txt" and expectedPath:
"package/skills/demo-skill/linked-secret" (and the similar cases at the other
occurrence) to build the path via join("package", "skills", "demo-skill",
"linked-secret.txt") and join("package","skills","demo-skill","linked-secret")
respectively so assertions work on Windows and POSIX; keep the surrounding test
object shapes (createLinkedPath, linkName, name) unchanged.

---

Nitpick comments:
In `@src/application/commands/skills/package-conversion.test.ts`:
- Around line 227-339: Tests in package-conversion.test.ts duplicate
skill/frontmatter and directory setup across cases; extract those into local
test factory helpers (e.g., createSkillSourceDir, createPackageRootDir,
buildSkillMarkdown) and a helper to call convertSkillDirectoryToPackage with
common defaults; update tests that currently call writeSkillFile,
createTemporaryDirectory, Bun.write, mkdir, and convertSkillDirectoryToPackage
(and assertions that read package.json or package/skills paths) to use these
factories so setup is centralized and reusable.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 33f6b18a-6443-4e72-becf-065f1de9cbed

📥 Commits

Reviewing files that changed from the base of the PR and between e143b1c and 603bcaa.

📒 Files selected for processing (2)
  • src/application/commands/skills/package-conversion.test.ts
  • src/application/commands/skills/package-conversion.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/application/commands/skills/package-conversion.ts

Comment thread src/application/commands/skills/package-conversion.test.ts Outdated
@BlackHole1 BlackHole1 merged commit 71948dc into main Apr 30, 2026
5 checks passed
@BlackHole1 BlackHole1 deleted the skills-publish branch April 30, 2026 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants