Skip to content

fix(skill): preserve custom installed instructions#20

Open
NianJiuZst wants to merge 4 commits into
Tencent:mainfrom
NianJiuZst:codex/protect-custom-installed-skills
Open

fix(skill): preserve custom installed instructions#20
NianJiuZst wants to merge 4 commits into
Tencent:mainfrom
NianJiuZst:codex/protect-custom-installed-skills

Conversation

@NianJiuZst

@NianJiuZst NianJiuZst commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

What problem this solves

bsk install-skill --source <path> is a documented way to install a customized BrowserSkill SKILL.md. However, the automatic synchronization path did not remember where an installed file came from. Daemon startup, session start, and doctor treated every existing BrowserSkill installation as a managed copy and rewrote any content that differed from the binary's bundled Skill.

That meant a successful custom installation was temporary. A later health check or normal automation session could silently replace the user's instructions with the bundled file. The overwrite happened without --force, without a confirmation prompt, and without a recoverable backup.

The same ambiguity applies to historical installations created before BrowserSkill recorded ownership. Automatically overwriting an unmarked file is unsafe because it may be either an old bundled copy or a user-maintained customization.

How this fixes it

Skill installation now writes a small .bsk-source provenance marker next to SKILL.md:

  • bundled means the file was installed from the binary's embedded Skill and may be automatically updated;
  • custom means --source or another non-bundled payload was used and automatic sync must preserve it.

The marker is written only when BrowserSkill actually installs or force-updates the file. A skipped existing installation is not retroactively claimed.

Automatic synchronization now follows a conservative ownership rule:

  • a differing file marked bundled is atomically updated;
  • a differing file marked custom is preserved;
  • a differing historical file with no marker is also preserved because ownership cannot be proven;
  • a file already byte-for-byte equal to the current bundled copy is reported up to date without being rewritten.

The sync report has a separate protected outcome so custom/unmanaged files are distinguishable from missing files and I/O failures. bsk doctor reports this as not applicable rather than failing or claiming that the custom file was synchronized.

User impact

Custom Skill instructions survive daemon startup, session creation, and doctor runs. Users can continue to replace a custom file deliberately with bsk install-skill --force.

Bundled installations created after this change continue receiving automatic updates. Historical unmarked installations are deliberately left untouched; users who want BrowserSkill to manage one can re-run the bundled installer with --force, which writes the managed marker.

This migration policy favors preserving user content over guessing provenance.

Validation

  • cargo fmt --all -- --check
  • cargo clippy --workspace --all-targets --locked -- -D warnings
  • cargo test --workspace --locked
    • 177 bsk library tests passed, plus all integration and protocol suites
  • Focused cargo test -p bsk skill_install --locked
    • 20 skill-install/sync tests passed
  • node --test scripts/*.test.mjs
  • git diff --check

Regression tests cover bundled provenance, custom provenance, force updates, custom-install followed by automatic sync, preservation of legacy unmarked files, managed bundled updates, unchanged-file mtimes, and partial I/O failures.

CI Baseline Compatibility

This branch also carries the one-line Biome 2.4 formatter output for apps/extension/vitest.config.ts. The same formatting mismatch currently fails the Frontend job on upstream/main; this minimal compatibility commit is intentionally separate from the functional fix and lets this PR run the repository CI suite to completion.

CI Reliability Follow-up

GitHub Actions exposed a pre-existing test-only port allocation race: integration helpers probed an ephemeral port, released it, and then asked the daemon to bind the same number, allowing another process to claim it in between. The resulting Address already in use failure was unrelated to the functional changes. The PR now lets each daemon bind port 0 directly and reads the assigned address from its handle, removing the TOCTOU window across all affected integration helpers.

@NianJiuZst NianJiuZst marked this pull request as ready for review July 10, 2026 14:59
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.

1 participant