feat(skills): add --force flag to install#233
Merged
Merged
Conversation
Allow `oo skills install` to overwrite an existing same-name skill directory that is not managed by oo (no readable `.oo-metadata.json`). Previously the install was blocked by a name conflict, forcing users to manually clean up the directory before retrying. `--force` (or `-f`) replaces the previous contents and writes the new skill, emitting a `warn` log entry to record the overwrite. It still honors path containment, package validation, auth, and download validation, and does not affect startup auto-sync, `update`, `sync`, `uninstall`, or `publish`. In multi-skill packages it does not implicitly select all skills; combine with `--skill` or `--all -y`. Telemetry records a low-cardinality `has_force` boolean so adoption of the new flag can be observed without leaking user input. Signed-off-by: Kevin Cui <bh@bugs.cc>
Summary by CodeRabbit
WalkthroughThis PR adds a Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
![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 (2)
docs/commands.md (1)
853-861: ⚡ Quick winRemove internal log-level detail from command contract docs.
Consider dropping “a
warnlog records the overwrite” here. The option behavior is user-facing, but log severity is an internal implementation detail and can drift without changing command contract.As per coding guidelines,
docs/commands*.md: "Documentation underdocs/commands*.mdshould describe only user-facing CLI contract ... Do not document internal implementation details."🤖 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 `@docs/commands.md` around lines 853 - 861, Remove the internal logging detail from the -f/--force option description: delete the phrase "a `warn` log records the overwrite" so the docs only state the user-facing behavior of --force (overrides install when the target directory exists and is not managed by oo, removes previous contents before writing, and does not bypass path containment, package validation, auth, download validation, or affect startup auto-sync / `oo skills update` / `oo skills sync` / `oo skills uninstall` / `oo skills publish`, nor implicitly select multi-skill packages — use `--skill` or `--all -y` with `--force`). Ensure the resulting text contains only the CLI contract and no implementation/log-level details.docs/commands.zh-CN.md (1)
728-734: ⚡ Quick win文档中建议去掉内部日志级别细节。
这里的
warn日志级别属于实现细节，建议只保留--force的用户可观察行为说明，避免把内部日志实现写入命令契约文档。As per coding guidelines,
docs/commands*.md: "Documentation underdocs/commands*.mdshould describe only user-facing CLI contract ... Do not document internal implementation details."🤖 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 `@docs/commands.zh-CN.md` around lines 728 - 734, Remove the implementation-detail about the internal log level from the `-f, --force` option description: delete the phrase that says the override "并以 `warn` 日志记录此事件" and any similar wording; keep only the user-facing behavior (that `--force` allows overwriting a directory with an existing skill that lacks readable `.oo-metadata.json`, and that it does not bypass path/package/auth/download checks nor affect auto-sync or other `oo skills` commands and does not implicitly select all skills). Ensure the option text references the flag `-f, --force` and `.oo-metadata.json` but does not mention internal logging levels or other implementation details.
🤖 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.
Inline comments:
In `@src/application/commands/skills/index.test.ts`:
- Around line 1286-1384: Add assertions to verify the new telemetry property for
the `--force` path: after the call to sandbox.run([... "skills", "install",
"oo", "--force" ]) (or the `-f` case), retrieve the emitted telemetry event from
the test sandbox (using the same sandbox instance used in the test) and assert
that the telemetry properties include has_force: true and that no raw/private
fields are present (for example, assert the event properties do not contain keys
like api_key, openai_key, raw_input or other sensitive/raw fields). Apply this
check in at least one of the `--force` tests (e.g., the "`--force` overwrites an
unmanaged same-name bundled host directory" test) and mirror for the `-f`
short-flag test if desired; use the existing sandbox.run/createCliSandbox
variables to locate where to read the recorded telemetry event and add the
assertions right after the existing exitCode/stderr/metadataContent checks.
---
Nitpick comments:
In `@docs/commands.md`:
- Around line 853-861: Remove the internal logging detail from the -f/--force
option description: delete the phrase "a `warn` log records the overwrite" so
the docs only state the user-facing behavior of --force (overrides install when
the target directory exists and is not managed by oo, removes previous contents
before writing, and does not bypass path containment, package validation, auth,
download validation, or affect startup auto-sync / `oo skills update` / `oo
skills sync` / `oo skills uninstall` / `oo skills publish`, nor implicitly
select multi-skill packages — use `--skill` or `--all -y` with `--force`).
Ensure the resulting text contains only the CLI contract and no
implementation/log-level details.
In `@docs/commands.zh-CN.md`:
- Around line 728-734: Remove the implementation-detail about the internal log
level from the `-f, --force` option description: delete the phrase that says the
override "并以 `warn` 日志记录此事件" and any similar wording; keep only the user-facing
behavior (that `--force` allows overwriting a directory with an existing skill
that lacks readable `.oo-metadata.json`, and that it does not bypass
path/package/auth/download checks nor affect auto-sync or other `oo skills`
commands and does not implicitly select all skills). Ensure the option text
references the flag `-f, --force` and `.oo-metadata.json` but does not mention
internal logging levels or other implementation details.
🪄 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: c7b7362e-f1cf-423c-ba05-653402de1f00
📒 Files selected for processing (8)
docs/commands.mddocs/commands.zh-CN.mdsrc/application/commands/skills/index.test.tssrc/application/commands/skills/install.tssrc/application/commands/skills/registry-skill-install.tssrc/application/commands/skills/registry-skill-publication.tssrc/application/commands/skills/shared.tssrc/i18n/catalog.ts
Signed-off-by: Kevin Cui <bh@bugs.cc>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/application/commands/skills/index.test.ts (1)
2967-3040:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd telemetry assertions for the registry
--forcepath.The past review comment requested telemetry assertions for
--forcetests and specifically mentioned "Also applies to: 2948-3021" (the original location of this test). While the bundled-skill test now has telemetry assertions, this registry-install test still lacks them. Please add assertions to verifyhas_force: true,package_kind: "registry",package_name: "openai", and absence of forbidden properties.💡 Suggested assertion block
expect( await readFile(join(codexSkillDirectoryPath, "SKILL.md"), "utf8"), ).toContain("# ChatGPT"); + const storePaths = resolveStorePaths({ + appName: APP_NAME, + env: sandbox.env, + platform: process.platform, + }); + const telemetryPayload = parseTelemetryRowPayload( + readTelemetryRowsForTest(storePaths.telemetryDirectory)[0]!, + ); + expect(telemetryPayload).toMatchObject({ + properties: { + command_full: "skills.install", + has_force: true, + package_kind: "registry", + package_name: "openai", + }, + }); + expect(telemetryPayload.properties).not.toHaveProperty("cwd"); + expect(telemetryPayload.properties).not.toHaveProperty("path"); + expect(telemetryPayload.properties).not.toHaveProperty("file_name"); } finally { await sandbox.cleanup();As per coding guidelines,
src/application/commands/**/*.{ts,test.ts}: "When adding command-specific telemetry properties, add or update tests that assert the expected safe property shape and absence of raw/private values."🤖 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 `@src/application/commands/skills/index.test.ts` around lines 2967 - 3040, The test "`--force` installs a published registry skill over an unmanaged host target" currently lacks telemetry assertions; update the test (inside the same test block after the sandbox.run call and after checking exitCode/stderr) to assert telemetry properties on the captured telemetry payload (the same place you inspect result) verifying has_force: true, package_kind: "registry", package_name: "openai" and ensure forbidden/raw properties (e.g. any private tokens/URLs) are not present; reference the test's result from sandbox.run and the same test flow that creates the registry archive (createRegistrySkillArchiveBytes) so the assertions run for the registry install path and maintain existing checks that read SKILL.md and resolveManagedSkillMetadataFilePath/renderSkillMetadataJson.
🤖 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.
Duplicate comments:
In `@src/application/commands/skills/index.test.ts`:
- Around line 2967-3040: The test "`--force` installs a published registry skill
over an unmanaged host target" currently lacks telemetry assertions; update the
test (inside the same test block after the sandbox.run call and after checking
exitCode/stderr) to assert telemetry properties on the captured telemetry
payload (the same place you inspect result) verifying has_force: true,
package_kind: "registry", package_name: "openai" and ensure forbidden/raw
properties (e.g. any private tokens/URLs) are not present; reference the test's
result from sandbox.run and the same test flow that creates the registry archive
(createRegistrySkillArchiveBytes) so the assertions run for the registry install
path and maintain existing checks that read SKILL.md and
resolveManagedSkillMetadataFilePath/renderSkillMetadataJson.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a0cdb1b-5fcc-41ee-97e7-f8eb1df57421
📒 Files selected for processing (2)
src/application/commands/skills/index.test.tssrc/application/commands/telemetry-decisions.test.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.
Allow
oo skills installto overwrite an existing same-name skill directory that is not managed by oo (no readable.oo-metadata.json). Previously the install was blocked by a name conflict, forcing users to manually clean up the directory before retrying.--force(or-f) replaces the previous contents and writes the new skill, emitting awarnlog entry to record the overwrite. It still honors path containment, package validation, auth, and download validation, and does not affect startup auto-sync,update,sync,uninstall, orpublish. In multi-skill packages it does not implicitly select all skills; combine with--skillor--all -y.Telemetry records a low-cardinality
has_forceboolean so adoption of the new flag can be observed without leaking user input.