Skip to content

fix(provisioning): mount Shelbi skills non-destructively, never touch user .claude content - #577

Merged
jlong merged 1 commit into
mainfrom
jlong/provisioning-never-destroy-user-claude-skills-files-mount-shelbi-s-without-committing
Aug 17, 2026
Merged

fix(provisioning): mount Shelbi skills non-destructively, never touch user .claude content#577
jlong merged 1 commit into
mainfrom
jlong/provisioning-never-destroy-user-claude-skills-files-mount-shelbi-s-without-committing

Conversation

@jlong

@jlong jlong commented Aug 17, 2026

Copy link
Copy Markdown
Owner

Task

Shelbi's worktree provisioning destroys pre-existing .claude/ content that the user (or plain Claude Code) put there. On every task dispatch, Shelbi blows away the entire .claude/skills/ directory and re-populates it with only its own agent skills. Any user-authored skills, and anything else living under that dir, are gone. This breaks projects that already use Claude Code skills/hooks and makes Shelbi hostile to existing Claude setups.

The desired behavior: Shelbi mounts its own skills and hooks into a project without ever touching entries it doesn't own, and its mounted entries are never committed and never dirty the working tree (symlink + gitignore is the clean way to get "never committed"). It is fine for Shelbi's own named skill to win over a user's same-named skill — but Shelbi must never delete or clobber skills/files it did not place.

wire_settings_local (crates/shelbi-orchestrator/src/workspace.rs:3009) is the model to follow: it already merges Shelbi's hook block into a user's .claude/settings.local.json additively and never clobbers user keys. The skills/hooks path needs the same "own only what you own" discipline.

Current Behavior

  • refresh_agent_skills (crates/shelbi-orchestrator/src/workspace.rs:3766) runs rm -rf <worktree>/.claude/skills (line ~3774), recreates it empty, then copies Shelbi's agent skills in. No guard for pre-existing user content — the doc comment at ~3535 even states it "clears .claude/skills/ before mounting." Called on every dispatch via deploy_agent_context → deploy_and_spawn (workspace.rs:2196-2198) and on orchestrator launch/reload (lib.rs:572, lib.rs:2106).
  • deploy_orchestrator_system_skill (workspace.rs:3599) detects a colliding pre-existing skill file (~3657-3664) but only prints warning: suppressing colliding skill ... and then overwrites it anyway (~3665).
  • ensure_gitignored (crates/shelbi-cli/src/commands/spawn.rs:284) appends .claude/skills/ (and .claude/settings.json, .claude/agent-instructions.md, .claude/shelbi-ready) to the user's repo .gitignore — Shelbi both claims the whole .claude/skills/ dir and wipes it.
  • Net effect for a user with their own .claude/skills/my-skill/: the dir is gitignored (hidden from their git) AND rm -rf'd on the next dispatch (destroyed).

Concrete failure

# User's project before Shelbi dispatches a task:
.claude/skills/
  my-skill/        <- user authored
  q-scan-project/  <- user authored (installed skill)

# After Shelbi dispatches ONE task:
.claude/skills/
  update-shelbi-configuration/   <- Shelbi's, all user skills GONE

Expected Behavior

  • Shelbi provisions only its own skills/hooks into .claude/, mounted so they are never committed and never show as working-tree changes (prefer symlinks into a Shelbi-managed source dir; ensure the mounted entries are gitignored). The original rm -rf of the whole .claude/skills/ directory must be removed.
  • Shelbi must track which entries it owns (e.g. a small manifest, or a reserved set of names) so refresh/teardown only ever removes or replaces Shelbi's own entries. Entries Shelbi did not place are never deleted, moved, or emptied.
  • On a name collision (user has a skill with the same name as one Shelbi mounts), Shelbi's mounted version wins for that name — but this is the only case Shelbi overwrites, and it is done via the never-committed mount (symlink), so the user's tracked copy on disk/in git is not destroyed. Log a clear warning when this happens.
  • Same principle for any hook/other file Shelbi drops into .claude/: mount Shelbi's own, never destroy the user's.
  • Reconsider ensure_gitignored in spawn.rs: it should gitignore exactly the entries Shelbi mounts (so they never get committed), not blanket-claim .claude/skills/. If Shelbi uses per-name symlinks, gitignore those specific names/paths rather than the whole dir.
  • deploy_orchestrator_system_skill's warn-then-overwrite is acceptable ONLY for Shelbi's own reserved skill name; it must not stomp an unrelated user file, and its mount must also be never-committed.
# Desired: after Shelbi dispatches a task
.claude/skills/
  my-skill/                       <- user's, untouched, still tracked
  q-scan-project/                 <- user's, untouched
  update-shelbi-configuration/    <- symlink -> Shelbi source, gitignored, not committed
# On next dispatch: only Shelbi's own entries are refreshed; user's are never touched.

Acceptance Criteria

  • A pre-existing .claude/skills/<user-skill>/ present before dispatch is still present, unmodified, after one or more dispatches.
  • Other pre-existing files/dirs under .claude/ (that Shelbi does not own) survive dispatch untouched.
  • Shelbi's own skills are present in .claude/skills/ after dispatch and function.
  • Shelbi's mounted skills/hooks are never committed and do not appear as changes in git status in the user's repo (verify: dispatch a task, then git status in the worktree shows no new tracked .claude/skills changes).
  • The blanket rm -rf .claude/skills in refresh_agent_skills is gone; teardown/refresh only removes Shelbi-owned entries.
  • Name collision between a user skill and a Shelbi skill: Shelbi's mounted version takes effect for that name, a warning is logged, and the user's on-disk/tracked file is not deleted.
  • ensure_gitignored no longer blanket-claims .claude/skills/; it ignores exactly Shelbi's mounted entries.
  • Existing behavior for .claude/settings.local.json (additive merge via wire_settings_local) is unchanged.
  • cargo build --workspace, cargo test --workspace, and cargo clippy --workspace --all-targets -- -D warnings pass.
  • Functional check: create a scratch project with a user skill + a user hook in .claude/, run a dispatch through it, confirm user content survives and Shelbi's skills are mounted and gitignored.

Context

Reported by the user 2026-08-17: "make Shelbi play nicer with existing Claude work — worktrees are overwriting the skills dir and other files. We can symlink in new skills/hooks but should never overwrite what's already there. Shelbi skills may overwrite the user's same-named skills but must never be committed." This is core provisioning behavior in shelbi-orchestrator/src/workspace.rs; mirror the non-destructive discipline wire_settings_local already uses.

Auto-opened by Shelbi — review at: /Users/jlong/.shelbi/projects/shelbi/tasks/provisioning-never-destroy-user-claude-skills-files-mount-shelbi-s-without-committing.md

@jlong @claude
fix(provisioning): mount Shelbi skills non-destructively, never touch…
22c2178 
… user .claude content

Worktree provisioning used to `rm -rf <worktree>/.claude/skills` on every
dispatch and re-populate it with only Shelbi's agent skills, destroying any
user-authored skills (and dirtying/hiding the dir via a blanket `.gitignore`
claim). Rework it to the same own-only-what-you-own discipline
`wire_settings_local` already uses: Shelbi mounts only its own entries and
never deletes, moves, or empties anything it didn't place.

refresh_agent_skills:
- No more `rm -rf .claude/skills`. Ownership is tracked in a per-worktree
  manifest (`.shelbi/mounted-claude-skills`); each dispatch removes exactly the
  entries it mounted last time, then mounts the current agent's skills. A user's
  `.claude/skills/my-skill/` and any other `.claude/` content survive untouched
  across any number of dispatches.
- Mount mechanism: symlink into the Shelbi-managed source on unix (never
  committed, single source of truth); copy on remote/non-unix, where the source
  lives on the hub and a symlink would dangle.
- Collision: Shelbi wins over an untracked entry of the same name, but NEVER
  overwrites a git-tracked user skill (that would destroy/dirty tracked
  content) — it logs a warning and leaves the user's version in place.

deploy_orchestrator_system_skill: same tracked-collision guard for the reserved
`update-shelbi-configuration` name (its precedence still comes from the
`--plugin-dir` bundle when we skip the on-disk copy), and it now removes any
pre-existing entry before writing so it can't relay a write *through* a refresh
symlink into the agent's source tree.

git cleanliness: Shelbi's mounts are hidden from `git status` via the worktree's
`info/exclude` (git's never-committed, per-checkout ignore), not the repo
`.gitignore` — an uncommitted `.gitignore` append doesn't even propagate into a
linked worktree (verified empirically), so the old approach never actually kept
a user repo's worktree clean. Also excludes `.shelbi/` (manifest + hook
scripts). ensure_gitignored no longer blanket-claims `.claude/skills/`.

Adds real-git functional tests: user content preserved + mounts hidden from
`git status`, tracked-collision left intact, untracked-collision won by Shelbi.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@vercel

vercel Bot commented Aug 17, 2026 •
edited
Loading

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
shelbi Ready Ready Preview Aug 17, 2026 7:03pm

Request Review

@jlong
jlong merged commit 7be7327 into main Aug 17, 2026
3 checks passed
@jlong
jlong deleted the jlong/provisioning-never-destroy-user-claude-skills-files-mount-shelbi-s-without-committing branch August 17, 2026 19:16
@jlong jlong mentioned this pull request Aug 18, 2026
Merged
6 tasks
jlong added a commit that referenced this pull request Aug 18, 2026
@jlong @claude
fix(review): reap markerless orphaned review slot + unwedge the dialo…
41f18e4 
…g-blocked poller (#579)

Two coupled defects left a review slot orphaned with a stale status.yaml
after a review-routed task was accepted and merged (observed live 2026-08-17,
PR #577).

1. Dialog-block wedged the status poller. When a Claude pane parks on a
   blocking modal (`dialog:question`, trust, permission), neither the busy
   footer nor the ready input box is drawn and Claude commonly clobbers the
   `shelbi:` pane-title marker with the modal's chrome. `poll_one` then found
   no live state AND no title marker and `return`ed early *before*
   `save_workspace_status`, so status.yaml froze at the pre-dialog state
   (`working`) and `last_seen` stopped advancing for the life of the modal —
   hours, in the incident. Fix: `maybe_emit_dialog_event` now returns the
   detected dialog kind, and `poll_one` folds it into the persisted state —
   a detected modal records `Blocked` (keeping `last_seen` fresh and honest),
   which clears on the next live busy/ready sample once the human answers.

2. Orphaned review pane was never reaped without a serving marker.
   `maybe_reap_orphaned_review_slot` gated the reap on the
   `.claude/shelbi-review-loaded` marker. A slot that was dispatched a review
   task but blocked on a dialog before its branch server came up never got
   that marker written, so when the task was accepted (review → done) nothing
   tore the pane down — and the dev-orphan reaper deliberately skips `review`
   slots, so the pane lingered as an `orphaned session`, keeping the heartbeat
   `idle_workspaces` count dishonest. Fix: drop the marker gate; the
   liveness/user-shell probe is the real guard, so a live non-user-shell agent
   pane with no assigned review task is reaped regardless of marker. A user
   shell / dead / unreachable slot is still left alone.

Also: on any reap (review + dev orphan) and on the TUI accept teardown
(`close_review_window`), clear the freed slot's status.yaml via the new
`shelbi_state::clear_workspace_status`. A killed pane emits no further markers,
so the poller can never refresh the file; left in place it would report the
agent's last state frozen forever. Cleared, the slot reads as idle in both the
board-derived `workspace list` and `workspace status`.

Decisions made without asking:
- Persist `Blocked` (not a new Idle variant) for a detected modal — it's the
  honest state and rides the existing decide()/dedupe machinery.
- Clear (delete) status.yaml on reap rather than write a synthetic idle state:
  there is no agent to describe, and the whole system already derives idle-ness
  from the board + live probe, not a persisted "idle" marker.
- Reap is left to the poller (covers CLI `task move`, hand board moves, and
  eviction uniformly); the CLI accept path is not given its own teardown call.

cargo build/test/clippy --workspace all pass.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@jlong