From 14862f3b33117fc1bc7a22680fea191ef44a20f3 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 26 Oct 2026 16:52:46 -0400 Subject: [PATCH 1/3] Look up a Step skill in ~/.thirdshift/skills before the Factory shorthand A Custom process skill is resolved in the repository first, then ~/.thirdshift/skills//SKILL.md, then the thirdshift- Factory skill. Home skills are linked into the worktree like the Factory skills, and a repository skill of the same name wins. --- docs/guide.md | 43 ++++++--- site/docs/index.html | 43 ++++++--- src/skills.rs | 204 +++++++++++++++++++++++++++++++++++++------ tests/process.rs | 201 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 439 insertions(+), 52 deletions(-) diff --git a/docs/guide.md b/docs/guide.md index 2d44d996..cda47cae 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -425,7 +425,7 @@ Every factory session on Claude Code, Repairs, reviews and Resumes included, als ### What sessions leave behind -Cleanup removes only thirdshift's own worktree, local Issue branch and the temporary directory it wrote the Factory skills out to. The `.git/info/exclude` entry that keeps the linked skills out of git, `/.claude/skills/thirdshift-*`, or `/.agents/skills/thirdshift-*` on the other Harnesses, is added once and left in place, as every worktree of the repository shares the file. Anything else an agent does as your user stays. For example, one Ticket run downloaded a JDK to `~/.local/jdk/` because the server had no Java, installed Playwright in `/tmp/pw`, and left a pull request body draft in `/tmp/`. This is by design, but worth knowing: +Cleanup removes only thirdshift's own worktree, local Issue branch and the temporary directory it wrote the Factory skills out to. The `.git/info/exclude` entry that keeps the linked skills out of git, `/.claude/skills/thirdshift-*`, or `/.agents/skills/thirdshift-*` on the other Harnesses, is added once and left in place, as every worktree of the repository shares the file. Each home skill linked from `~/.thirdshift/skills` adds its own entry for that name, and that entry is left in place too. Anything else an agent does as your user stays. For example, one Ticket run downloaded a JDK to `~/.local/jdk/` because the server had no Java, installed Playwright in `/tmp/pw`, and left a pull request body draft in `/tmp/`. This is by design, but worth knowing: - **Install the target repository's toolchains up front** (language runtimes, package managers, test tools), so the agents don't download their own on every Ticket run. - **For UI work, install the headless browser dependencies**, e.g. `libgbm1` on Ubuntu server, which headless Chromium needs. Without it, agents can't take screenshots to check a UI change and fall back to reading the code. @@ -809,18 +809,27 @@ thirdshift skill thirdshift-code-review thirdshift skill my-repository-skill ``` -The command uses the User config's default Harness, Model and Effort. It first -looks for the exact name among the Factory skills and the repository's committed -`.agents/skills//SKILL.md` and `.claude/skills//SKILL.md` files. If none -matches, it tries the Factory skill `thirdshift-`. A skill file that -resolves outside the worktree is not a match. An unknown name lists the -available skills and starts no session. The optional issue is an Issue URL or a -positive number resolved from `origin`. An Issue URL from another repository -is an origin mismatch and stops the command before any work. +The command uses the User config's default Harness, Model and Effort. It looks +up `` in order. First a Factory skill whose directory is exactly +``. Then the repository's committed `.agents/skills//SKILL.md`, or +`.claude/skills//SKILL.md` when only that copy exists; `.agents/skills` +wins when both exist. Then `~/.thirdshift/skills//SKILL.md`. Then the +Factory skill `thirdshift-`. A skill file that resolves outside the +directory being searched is not a match, so a symlink cannot select +instructions from somewhere else. A repository skill wins over the home skill +of the same name. An unknown name lists the available skills and starts no +session. The optional issue is an Issue URL or a positive number resolved from +`origin`. An Issue URL from another repository is an origin mismatch and stops +the command before any work. The session runs in a temporary worktree at the head of the checked-out branch, -with the Factory skills available. Uncommitted changes in your checkout stay out -of it. Your checkout stays untouched. The final message goes to stdout; progress, +with the Factory skills available, and with each home skill linked under its +own name in that same project skills directory. A repository skill of the same +name is left in place. Each linked home skill is kept out of git by its own +`.git/info/exclude` entry, added once and left there, as the Factory +`thirdshift-*` pattern is ([ADR-0012](adr/0012-factory-skills-linked-into-the-worktree-codex-unsandboxed.md)). +Uncommitted changes in your checkout stay out of it. Your checkout stays +untouched. The final message goes to stdout; progress, failure causes and the path of any retained worktree go to stderr. A completed session exits `0`; a failed or incomplete session exits non-zero. @@ -853,7 +862,17 @@ optional = true ``` `version` must be `1`. Each Step has a `name` and exactly one kind. `skill` is -looked up the same way as `thirdshift skill`. `command` is a command or script, +looked up the same way as `thirdshift skill`: a Factory skill whose directory +is exactly that name, then the repository's committed +`.agents/skills//SKILL.md` and `.claude/skills//SKILL.md`, then +`~/.thirdshift/skills//SKILL.md`, then the Factory shorthand +`thirdshift-`. `.agents/skills` wins when both repository copies exist. +A repository skill wins over the home skill of the same name. A skill file +that resolves outside the directory being searched is not a match. The home +skill is linked into the Step's worktree the way the Factory skills are +([ADR-0012](adr/0012-factory-skills-linked-into-the-worktree-codex-unsandboxed.md)), +under its own name, and kept out of git. A repository skill of the same name is not +replaced. `command` is a command or script, run with `sh -c` in the Step's worktree. It starts no session. `optional = true` records that Step's failure and the Process carries on. Without `optional`, or with `optional = false`, a failure stops the Process. diff --git a/site/docs/index.html b/site/docs/index.html index 916fd48d..59cd54e8 100644 --- a/site/docs/index.html +++ b/site/docs/index.html @@ -401,7 +401,7 @@

What sessions leave behind
-

Cleanup removes only thirdshift's own worktree, local Issue branch and the temporary directory it wrote the Factory skills out to. The .git/info/exclude entry that keeps the linked skills out of git, /.claude/skills/thirdshift-*, or /.agents/skills/thirdshift-* on the other Harnesses, is added once and left in place, as every worktree of the repository shares the file. Anything else an agent does as your user stays. For example, one Ticket run downloaded a JDK to ~/.local/jdk/ because the server had no Java, installed Playwright in /tmp/pw, and left a pull request body draft in /tmp/. This is by design, but worth knowing:

+

Cleanup removes only thirdshift's own worktree, local Issue branch and the temporary directory it wrote the Factory skills out to. The .git/info/exclude entry that keeps the linked skills out of git, /.claude/skills/thirdshift-*, or /.agents/skills/thirdshift-* on the other Harnesses, is added once and left in place, as every worktree of the repository shares the file. Each home skill linked from ~/.thirdshift/skills adds its own entry for that name, and that entry is left in place too. Anything else an agent does as your user stays. For example, one Ticket run downloaded a JDK to ~/.local/jdk/ because the server had no Java, installed Playwright in /tmp/pw, and left a pull request body draft in /tmp/. This is by design, but worth knowing:

  • Install the target repository's toolchains up front (language runtimes, package managers, test tools), so the agents don't download their own on every Ticket run.
  • For UI work, install the headless browser dependencies, e.g. libgbm1 on Ubuntu server, which headless Chromium needs. Without it, agents can't take screenshots to check a UI change and fall back to reading the code.
    • @@ -735,17 +735,26 @@ thirdshift skill thirdshift-code-review thirdshift skill my-repository-skill -

    The command uses the User config's default Harness, Model and Effort. It first -looks for the exact name among the Factory skills and the repository's committed -.agents/skills/<name>/SKILL.md and .claude/skills/<name>/SKILL.md files. If none -matches, it tries the Factory skill thirdshift-<name>. A skill file that -resolves outside the worktree is not a match. An unknown name lists the -available skills and starts no session. The optional issue is an Issue URL or a -positive number resolved from origin. An Issue URL from another repository -is an origin mismatch and stops the command before any work.

    +

    The command uses the User config's default Harness, Model and Effort. It looks +up <name> in order. First a Factory skill whose directory is exactly +<name>. Then the repository's committed .agents/skills/<name>/SKILL.md, or +.claude/skills/<name>/SKILL.md when only that copy exists; .agents/skills +wins when both exist. Then ~/.thirdshift/skills/<name>/SKILL.md. Then the +Factory skill thirdshift-<name>. A skill file that resolves outside the +directory being searched is not a match, so a symlink cannot select +instructions from somewhere else. A repository skill wins over the home skill +of the same name. An unknown name lists the available skills and starts no +session. The optional issue is an Issue URL or a positive number resolved from +origin. An Issue URL from another repository is an origin mismatch and stops +the command before any work.

    The session runs in a temporary worktree at the head of the checked-out branch, -with the Factory skills available. Uncommitted changes in your checkout stay out -of it. Your checkout stays untouched. The final message goes to stdout; progress, +with the Factory skills available, and with each home skill linked under its +own name in that same project skills directory. A repository skill of the same +name is left in place. Each linked home skill is kept out of git by its own +.git/info/exclude entry, added once and left there, as the Factory +thirdshift-* pattern is (ADR-0012). +Uncommitted changes in your checkout stay out of it. Your checkout stays +untouched. The final message goes to stdout; progress, failure causes and the path of any retained worktree go to stderr. A completed session exits 0; a failed or incomplete session exits non-zero.

    The worktree is kept when it has uncommitted changes, commits absent from all @@ -774,7 +783,17 @@ optional = true

    version must be 1. Each Step has a name and exactly one kind. skill is -looked up the same way as thirdshift skill. command is a command or script, +looked up the same way as thirdshift skill: a Factory skill whose directory +is exactly that name, then the repository's committed +.agents/skills/<name>/SKILL.md and .claude/skills/<name>/SKILL.md, then +~/.thirdshift/skills/<name>/SKILL.md, then the Factory shorthand +thirdshift-<name>. .agents/skills wins when both repository copies exist. +A repository skill wins over the home skill of the same name. A skill file +that resolves outside the directory being searched is not a match. The home +skill is linked into the Step's worktree the way the Factory skills are +(ADR-0012), +under its own name, and kept out of git. A repository skill of the same name is not +replaced. command is a command or script, run with sh -c in the Step's worktree. It starts no session. optional = true records that Step's failure and the Process carries on. Without optional, or with optional = false, a failure stops the Process.

    diff --git a/src/skills.rs b/src/skills.rs index 20e097c3..37b52371 100644 --- a/src/skills.rs +++ b/src/skills.rs @@ -1,6 +1,7 @@ //! The Factory skills, embedded at compile time (ADR-0001), written out once //! per Command and linked into each worktree its sessions run in, where the -//! Harness finds project skills (ADR-0012). +//! Harness finds project skills (ADR-0012). Home skills from +//! `~/.thirdshift/skills` are linked the same way. use std::collections::BTreeMap; use std::fs::{self, OpenOptions}; @@ -34,18 +35,61 @@ pub struct Skill { enum Source { Factory, Repository(PathBuf), + Home(PathBuf), } -/// Resolve exact names across Factory and repository skills before trying -/// a Factory shorthand. Only skill files that resolve inside the committed -/// worktree are searched, so a symlink cannot select instructions outside it. +/// Resolve a skill name. An exact Factory directory name wins, then a +/// repository skill in `.agents/skills//SKILL.md` or +/// `.claude/skills//SKILL.md` that resolves inside the worktree, then +/// `~/.thirdshift/skills//SKILL.md` when that file resolves inside the +/// home skills directory. The Factory shorthand `thirdshift-` is last. +/// A symlink cannot select instructions outside the directory being searched. pub fn find(worktree: &Path, name: &str) -> Result { let mut available = BTreeMap::new(); for dir in SKILLS.dirs() { available.insert(dir.path().to_string_lossy().into_owned(), Source::Factory); } + for (skill_name, path) in repository_skills(worktree)? { + available + .entry(skill_name) + .or_insert(Source::Repository(path)); + } + for (skill_name, path) in home_skills()? { + available.entry(skill_name).or_insert(Source::Home(path)); + } + let selected = available.get_key_value(name).or_else(|| { + available + .get_key_value(&format!("thirdshift-{name}")) + .filter(|(_, source)| matches!(source, Source::Factory)) + }); + if let Some((name, source)) = selected { + return Ok(Skill { + path: match source { + Source::Factory => written_out()?.join(name).join("SKILL.md"), + Source::Repository(path) | Source::Home(path) => path.clone(), + }, + name: name.clone(), + }); + } + bail!( + "unknown skill {name:?}; available skills: {}", + available.keys().cloned().collect::>().join(", ") + ) +} + +/// `candidate` resolves to a file or directory inside `root`. Symlinks are +/// followed, so a skill that points outside the directory being searched is +/// not a match. +fn resolved_inside(root: &Path, candidate: &Path) -> bool { + fs::canonicalize(candidate).is_ok_and(|target| target.starts_with(root)) +} + +/// Repository skills whose `SKILL.md` resolves inside `worktree`. `.agents` +/// is listed before `.claude`, and the caller keeps the first name. +fn repository_skills(worktree: &Path) -> Result> { let root = fs::canonicalize(worktree) .with_context(|| format!("could not resolve {}", worktree.display()))?; + let mut skills = Vec::new(); for folder in [".agents/skills", ".claude/skills"] { let folder_path = worktree.join(folder); match fs::symlink_metadata(&folder_path) { @@ -70,33 +114,75 @@ pub fn find(worktree: &Path, name: &str) -> Result { entry.path().display() ) })?; - available.entry(name).or_insert(Source::Repository(path)); + skills.push((name, path)); } } - let selected = available.get_key_value(name).or_else(|| { - available - .get_key_value(&format!("thirdshift-{name}")) - .filter(|(_, source)| matches!(source, Source::Factory)) - }); - if let Some((name, source)) = selected { - return Ok(Skill { - path: match source { - Source::Factory => written_out()?.join(name).join("SKILL.md"), - Source::Repository(path) => path.clone(), - }, - name: name.clone(), - }); + Ok(skills) +} + +/// Home skills, each `~/.thirdshift/skills//SKILL.md` that resolves +/// inside that directory. No home directory, or `HOME` unset, is none. +fn home_skills() -> Result> { + let Ok(home) = crate::config::home() else { + return Ok(Vec::new()); + }; + let skills_dir = home.join(".thirdshift/skills"); + let entries = match fs::read_dir(&skills_dir) { + Ok(entries) => entries, + Err(error) if error.kind() == ErrorKind::NotFound => return Ok(Vec::new()), + Err(error) => { + return Err(error).with_context(|| format!("could not list {}", skills_dir.display())); + } + }; + let root = match fs::canonicalize(&skills_dir) { + Ok(root) => root, + Err(error) if error.kind() == ErrorKind::NotFound => return Ok(Vec::new()), + Err(error) => { + return Err(error) + .with_context(|| format!("could not resolve {}", skills_dir.display())); + } + }; + let mut skills = Vec::new(); + for entry in entries { + let entry = entry.with_context(|| format!("could not list {}", skills_dir.display()))?; + // One odd directory name must not make every skill lookup fail. + let Ok(name) = entry.file_name().into_string() else { + continue; + }; + if !usable_home_skill_name(&name) { + continue; + } + let path = entry.path().join("SKILL.md"); + if !resolved_inside(&root, &path) || !path.is_file() { + continue; + } + skills.push((name, path)); } - bail!( - "unknown skill {name:?}; available skills: {}", - available.keys().cloned().collect::>().join(", ") - ) + Ok(skills) } -/// `candidate` resolves to a file or directory inside `root`. Symlinks are -/// followed, so a skill that points outside the worktree is not a match. -fn resolved_inside(root: &Path, candidate: &Path) -> bool { - fs::canonicalize(candidate).is_ok_and(|target| target.starts_with(root)) +/// A single path component that can be one line of `.git/info/exclude`. +fn usable_home_skill_name(name: &str) -> bool { + !name.is_empty() + && name != "." + && name != ".." + && !name + .chars() + .any(|ch| ch.is_control() || matches!(ch, '/' | '\\')) +} + +/// A gitignore line that matches only `path`. `path` starts with `/`. +fn exclude_literal(path: &str) -> String { + let chars: Vec = path.chars().collect(); + let last_non_space = chars.iter().rposition(|ch| *ch != ' ').unwrap_or(0); + let mut escaped = String::new(); + for (index, ch) in chars.into_iter().enumerate() { + if matches!(ch, '*' | '?' | '[' | ']' | '\\') || (ch == ' ' && index > last_non_space) { + escaped.push('\\'); + } + escaped.push(ch); + } + escaped } /// The temp directory the Command wrote the Factory skills out to, once it @@ -106,8 +192,9 @@ static WRITTEN: Mutex> = Mutex::new(None); /// Link every Factory skill into `worktree`'s project skills for `adapter`, /// `.claude/skills/` or `.agents/skills/`, writing them out first if the /// Command has not yet, and make sure the repository's `.git/info/exclude` -/// keeps them out of git. The links are left in place: they go with the -/// worktree. +/// keeps them out of git. Home skills are linked under their own names the +/// same way, except where the repository already has that skill. The links +/// are left in place: they go with the worktree. pub fn link_into(worktree: &Path, adapter: &dyn Adapter) -> Result<()> { let dir = adapter.project_skills(); // Refuse an unsafe path before writing skills or updating excludes. @@ -119,9 +206,70 @@ pub fn link_into(worktree: &Path, adapter: &dyn Adapter) -> Result<()> { let name = skill.path(); project_skills.link(&written.join(name), name)?; } + link_home_skills(worktree, &project_skills, dir)?; Ok(()) } +/// Link each home skill under its own name, unless the repository already has +/// that skill or the destination is some other entry. Each linked name is +/// excluded on its own, because home skills do not share the `thirdshift-` +/// prefix. The entry stays, as the Factory pattern does. +fn link_home_skills(worktree: &Path, project_skills: &CheckedDirectory, dir: &str) -> Result<()> { + let skills = home_skills()?; + if skills.is_empty() { + return Ok(()); + } + // Probe each name. Listing the project skills directory would fail when + // it is searchable but not readable, which Factory linking allows. + let root = fs::canonicalize(worktree) + .with_context(|| format!("could not resolve {}", worktree.display()))?; + for (name, skill_md) in skills { + if repository_has_skill(&root, worktree, &name) { + continue; + } + let Some(directory) = skill_md.parent() else { + continue; + }; + let destination = worktree.join(dir).join(&name); + if !home_link_allowed(&destination, directory)? { + continue; + } + exclude( + &Git::new(worktree), + &exclude_literal(&format!("/{dir}/{name}")), + )?; + project_skills.link(directory, Path::new(&name))?; + } + Ok(()) +} + +/// `worktree` has a committed skill named `name` in either project skills +/// directory. Missing paths and directories that cannot be searched are not. +fn repository_has_skill(root: &Path, worktree: &Path, name: &str) -> bool { + [".agents/skills", ".claude/skills"] + .into_iter() + .any(|folder| { + let path = worktree.join(folder).join(name).join("SKILL.md"); + resolved_inside(root, &path) && path.is_file() + }) +} + +/// The destination is absent, or it is already this home skill's link and can +/// be refreshed. A repository file or directory, or a link to somewhere else, +/// stays. +fn home_link_allowed(destination: &Path, directory: &Path) -> Result { + match fs::symlink_metadata(destination) { + Err(error) if error.kind() == ErrorKind::NotFound => Ok(true), + Err(error) => { + Err(error).with_context(|| format!("could not inspect {}", destination.display())) + } + Ok(metadata) if metadata.file_type().is_symlink() => { + Ok(fs::read_link(destination).ok().as_deref() == Some(directory)) + } + Ok(_) => Ok(false), + } +} + /// Remove the Factory skills the Command wrote out, if it did, once its /// sessions are over. pub fn remove_written() { diff --git a/tests/process.rs b/tests/process.rs index e5fa8e89..85737f74 100644 --- a/tests/process.rs +++ b/tests/process.rs @@ -3,6 +3,7 @@ mod support; use std::fs; +use std::os::unix::fs::symlink; use std::path::Path; use support::{RunResult, Scenario}; @@ -895,6 +896,206 @@ fn stop_interrupts_a_running_process() { assert_eq!(record["outcome"], "interrupted"); } +#[test] +fn a_home_skill_runs_and_the_session_finds_it_in_the_worktree() { + let scenario = Scenario::new(); + let house = scenario.path("home/.thirdshift/skills/house-style"); + let other = scenario.path("home/.thirdshift/skills/other-notes"); + fs::create_dir_all(&house).unwrap(); + fs::create_dir_all(&other).unwrap(); + fs::write(house.join("SKILL.md"), "# House style\n").unwrap(); + fs::write(other.join("SKILL.md"), "# Other notes\n").unwrap(); + write_process( + &scenario, + "style", + "\ +version = 1 +description = \"Apply the house style\" + +[[steps]] +name = \"style\" +skill = \"house-style\" +", + ); + scenario.agent_does( + "test \"$(cat .claude/skills/house-style/SKILL.md)\" = '# House style'\n\ + test \"$(cat .claude/skills/other-notes/SKILL.md)\" = '# Other notes'\n\ + test -z \"$(git status --porcelain --untracked-files=all)\"\n\ + printf 'styled' > \"$FAKE_CLAUDE_FINAL_MESSAGE\"", + ); + + let result = scenario.run(&["process", "style"]); + + assert_eq!(result.code, Some(0), "{}", result.stderr); + assert_eq!(result.stdout, "styled\n"); + let calls = scenario.claude_calls(); + assert_eq!(calls.len(), 1, "{}", result.stderr); + let prompt = calls[0]["prompt"].as_str().unwrap(); + let found = scenario.path("home/.thirdshift/skills/house-style/SKILL.md"); + assert!( + prompt.contains(&format!( + "house-style skill. Read and follow `{}`", + found.display() + )), + "{prompt}" + ); + assert!(!prompt.contains("thirdshift-house-style"), "{prompt}"); + let cwd = Path::new(calls[0]["cwd"].as_str().unwrap()); + assert!(!cwd.exists(), "worktree was kept: {}", result.stderr); +} + +#[test] +fn a_repository_skill_wins_over_the_home_skill() { + for folder in [".agents/skills", ".claude/skills"] { + let scenario = Scenario::new(); + let relative = format!("{folder}/house-style/SKILL.md"); + fs::create_dir_all(scenario.launch_dir().join(format!("{folder}/house-style"))).unwrap(); + scenario.commit_locally(&relative, "# Repository style\n", "Add repository skill"); + scenario.launch_git(&["push", "origin", "main"]); + let home = scenario.path("home/.thirdshift/skills/house-style"); + let other = scenario.path("home/.thirdshift/skills/other-notes"); + fs::create_dir_all(&home).unwrap(); + fs::create_dir_all(&other).unwrap(); + fs::write(home.join("SKILL.md"), "# House style\n").unwrap(); + fs::write(other.join("SKILL.md"), "# Other notes\n").unwrap(); + write_process( + &scenario, + "style", + "\ +version = 1 +description = \"Apply the house style\" + +[[steps]] +name = \"style\" +skill = \"house-style\" +", + ); + let preserved = if folder == ".claude/skills" { + "test ! -L .claude/skills/house-style" + } else { + "test ! -e .claude/skills/house-style" + }; + scenario.agent_does(&format!( + "test \"$(cat {relative})\" = '# Repository style'\n\ + {preserved}\n\ + test \"$(cat .claude/skills/other-notes/SKILL.md)\" = '# Other notes'\n\ + printf 'repo' > \"$FAKE_CLAUDE_FINAL_MESSAGE\"" + )); + + let result = scenario.run(&["process", "style"]); + + assert_eq!(result.code, Some(0), "{folder}: {}", result.stderr); + assert_eq!(result.stdout, "repo\n"); + let calls = scenario.claude_calls(); + let prompt = calls[0]["prompt"].as_str().unwrap(); + assert!(prompt.contains(&relative), "{folder}: {prompt}"); + assert!( + !prompt.contains(".thirdshift/skills/house-style"), + "{folder}: {prompt}" + ); + } +} + +#[test] +fn check_prints_where_each_steps_skill_was_found() { + let scenario = Scenario::new(); + let relative = ".claude/skills/house-style/SKILL.md"; + fs::create_dir_all(scenario.launch_dir().join(".claude/skills/house-style")).unwrap(); + scenario.commit_locally(relative, "# Repository style\n", "Add repository skill"); + scenario.launch_git(&["push", "origin", "main"]); + let home = scenario.path("home/.thirdshift/skills/code-review"); + let hidden = scenario.path("home/.thirdshift/skills/house-style"); + fs::create_dir_all(&home).unwrap(); + fs::create_dir_all(&hidden).unwrap(); + fs::write(home.join("SKILL.md"), "# Home review\n").unwrap(); + fs::write(hidden.join("SKILL.md"), "# House style\n").unwrap(); + write_process( + &scenario, + "style", + "\ +version = 1 +description = \"Style, review, then plan\" + +[[steps]] +name = \"house\" +skill = \"house-style\" + +[[steps]] +name = \"review\" +skill = \"code-review\" + +[[steps]] +name = \"plan\" +skill = \"to-spec\" +", + ); + + let result = scenario.run(&["process", "check", "style"]); + + assert_eq!(result.code, Some(0), "{}", result.stderr); + assert!(scenario.claude_calls().is_empty(), "a session started"); + let house = result + .stdout + .lines() + .find(|line| line.starts_with("house:")) + .unwrap_or_else(|| panic!("no house step in {}", result.stdout)); + assert!(house.contains("house-style"), "{house}"); + assert!(house.contains(relative), "{house}"); + assert!(!house.contains(".thirdshift/skills"), "{house}"); + let review = result + .stdout + .lines() + .find(|line| line.starts_with("review:")) + .unwrap_or_else(|| panic!("no review step in {}", result.stdout)); + let found = scenario.path("home/.thirdshift/skills/code-review/SKILL.md"); + assert!( + review.contains(&format!("code-review {}", found.display())), + "{review}" + ); + assert!(!review.contains("thirdshift-code-review"), "{review}"); + let plan = result + .stdout + .lines() + .find(|line| line.starts_with("plan:")) + .unwrap_or_else(|| panic!("no plan step in {}", result.stdout)); + assert!(plan.contains("thirdshift-to-spec"), "{plan}"); + assert!(plan.contains("SKILL.md"), "{plan}"); + assert!(!plan.contains(".thirdshift/skills"), "{plan}"); +} + +#[test] +fn a_home_skill_outside_its_directory_is_not_used() { + let scenario = Scenario::new(); + let outside = scenario.path("outside-skill"); + fs::create_dir_all(&outside).unwrap(); + fs::write(outside.join("SKILL.md"), "# Outside\n").unwrap(); + let skills = scenario.path("home/.thirdshift/skills"); + fs::create_dir_all(&skills).unwrap(); + symlink(&outside, skills.join("escaped")).unwrap(); + write_process( + &scenario, + "style", + "\ +version = 1 +description = \"Use an escaped skill\" + +[[steps]] +name = \"style\" +skill = \"escaped\" +", + ); + + let result = scenario.run(&["process", "check", "style"]); + + assert_no_session(&scenario, &result); + assert!( + result.stderr.contains("skill not found"), + "{}", + result.stderr + ); + assert!(result.stderr.contains("unknown skill"), "{}", result.stderr); +} + fn step_line<'a>(log: &'a str, name: &str) -> &'a str { log.lines() .find(|line| line.contains(&format!("step {name}:"))) From a3d87a08e57e099a46a9546f37aa482931b0afa9 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 26 Oct 2026 17:27:19 -0400 Subject: [PATCH 2/3] Publish home skills into the Factory skills folder A Trial binds that folder and not ~/.thirdshift/skills, so a link at the home directory was unreadable. The worktree link now points at the copy. check and the session prompt still name the home file. --- docs/guide.md | 28 +++++---- site/docs/index.html | 28 +++++---- src/skills.rs | 140 +++++++++++++++++++++++++++++++++++-------- tests/process.rs | 65 ++++++++++++++++++++ 4 files changed, 213 insertions(+), 48 deletions(-) diff --git a/docs/guide.md b/docs/guide.md index cda47cae..2f310545 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -824,8 +824,11 @@ the command before any work. The session runs in a temporary worktree at the head of the checked-out branch, with the Factory skills available, and with each home skill linked under its -own name in that same project skills directory. A repository skill of the same -name is left in place. Each linked home skill is kept out of git by its own +own name in that same project skills directory. The link points at a copy +written out beside the Factory skills. A Trial binds that folder and not +`~/.thirdshift/skills`, so the session reads the copy; the prompt still names +the home file, which is where the skill was found. A repository skill of the +same name is left in place. Each linked home skill is kept out of git by its own `.git/info/exclude` entry, added once and left there, as the Factory `thirdshift-*` pattern is ([ADR-0012](adr/0012-factory-skills-linked-into-the-worktree-codex-unsandboxed.md)). Uncommitted changes in your checkout stay out of it. Your checkout stays @@ -862,17 +865,18 @@ optional = true ``` `version` must be `1`. Each Step has a `name` and exactly one kind. `skill` is -looked up the same way as `thirdshift skill`: a Factory skill whose directory -is exactly that name, then the repository's committed -`.agents/skills//SKILL.md` and `.claude/skills//SKILL.md`, then -`~/.thirdshift/skills//SKILL.md`, then the Factory shorthand -`thirdshift-`. `.agents/skills` wins when both repository copies exist. -A repository skill wins over the home skill of the same name. A skill file -that resolves outside the directory being searched is not a match. The home -skill is linked into the Step's worktree the way the Factory skills are +looked up the same way as `thirdshift skill`. First a Factory skill whose +directory is exactly that name. Then the repository's committed +`.agents/skills//SKILL.md`, or `.claude/skills//SKILL.md` when +only that copy exists; `.agents/skills` wins when both exist. Then +`~/.thirdshift/skills//SKILL.md`. Then the Factory skill +`thirdshift-`. A repository skill wins over the home skill of the same +name. A skill file that resolves outside the directory being searched is not +a match. The home skill is linked into the Step's worktree the way the Factory +skills are ([ADR-0012](adr/0012-factory-skills-linked-into-the-worktree-codex-unsandboxed.md)), -under its own name, and kept out of git. A repository skill of the same name is not -replaced. `command` is a command or script, +under its own name, as a copy written out beside those skills, and kept out +of git. A repository skill of the same name is not replaced. `command` is a command or script, run with `sh -c` in the Step's worktree. It starts no session. `optional = true` records that Step's failure and the Process carries on. Without `optional`, or with `optional = false`, a failure stops the Process. diff --git a/site/docs/index.html b/site/docs/index.html index 59cd54e8..5867a3cf 100644 --- a/site/docs/index.html +++ b/site/docs/index.html @@ -749,8 +749,11 @@ the command before any work.

    The session runs in a temporary worktree at the head of the checked-out branch, with the Factory skills available, and with each home skill linked under its -own name in that same project skills directory. A repository skill of the same -name is left in place. Each linked home skill is kept out of git by its own +own name in that same project skills directory. The link points at a copy +written out beside the Factory skills. A Trial binds that folder and not +~/.thirdshift/skills, so the session reads the copy; the prompt still names +the home file, which is where the skill was found. A repository skill of the +same name is left in place. Each linked home skill is kept out of git by its own .git/info/exclude entry, added once and left there, as the Factory thirdshift-* pattern is (ADR-0012). Uncommitted changes in your checkout stay out of it. Your checkout stays @@ -783,17 +786,18 @@ optional = true

    version must be 1. Each Step has a name and exactly one kind. skill is -looked up the same way as thirdshift skill: a Factory skill whose directory -is exactly that name, then the repository's committed -.agents/skills/<name>/SKILL.md and .claude/skills/<name>/SKILL.md, then -~/.thirdshift/skills/<name>/SKILL.md, then the Factory shorthand -thirdshift-<name>. .agents/skills wins when both repository copies exist. -A repository skill wins over the home skill of the same name. A skill file -that resolves outside the directory being searched is not a match. The home -skill is linked into the Step's worktree the way the Factory skills are +looked up the same way as thirdshift skill. First a Factory skill whose +directory is exactly that name. Then the repository's committed +.agents/skills/<name>/SKILL.md, or .claude/skills/<name>/SKILL.md when +only that copy exists; .agents/skills wins when both exist. Then +~/.thirdshift/skills/<name>/SKILL.md. Then the Factory skill +thirdshift-<name>. A repository skill wins over the home skill of the same +name. A skill file that resolves outside the directory being searched is not +a match. The home skill is linked into the Step's worktree the way the Factory +skills are (ADR-0012), -under its own name, and kept out of git. A repository skill of the same name is not -replaced. command is a command or script, +under its own name, as a copy written out beside those skills, and kept out +of git. A repository skill of the same name is not replaced. command is a command or script, run with sh -c in the Step's worktree. It starts no session. optional = true records that Step's failure and the Process carries on. Without optional, or with optional = false, a failure stops the Process.

    diff --git a/src/skills.rs b/src/skills.rs index 37b52371..4d18a7d6 100644 --- a/src/skills.rs +++ b/src/skills.rs @@ -120,23 +120,46 @@ fn repository_skills(worktree: &Path) -> Result> { Ok(skills) } +struct HomeSkill { + name: String, + skill_md: PathBuf, +} + +struct HomeCatalog { + root: PathBuf, + skills: Vec, +} + /// Home skills, each `~/.thirdshift/skills//SKILL.md` that resolves /// inside that directory. No home directory, or `HOME` unset, is none. fn home_skills() -> Result> { + Ok(read_home_skills()? + .map(|catalog| { + catalog + .skills + .into_iter() + .map(|skill| (skill.name, skill.skill_md)) + .collect() + }) + .unwrap_or_default()) +} + +/// The canonical home skills directory and the skills inside it. +fn read_home_skills() -> Result> { let Ok(home) = crate::config::home() else { - return Ok(Vec::new()); + return Ok(None); }; let skills_dir = home.join(".thirdshift/skills"); let entries = match fs::read_dir(&skills_dir) { Ok(entries) => entries, - Err(error) if error.kind() == ErrorKind::NotFound => return Ok(Vec::new()), + Err(error) if error.kind() == ErrorKind::NotFound => return Ok(None), Err(error) => { return Err(error).with_context(|| format!("could not list {}", skills_dir.display())); } }; let root = match fs::canonicalize(&skills_dir) { Ok(root) => root, - Err(error) if error.kind() == ErrorKind::NotFound => return Ok(Vec::new()), + Err(error) if error.kind() == ErrorKind::NotFound => return Ok(None), Err(error) => { return Err(error) .with_context(|| format!("could not resolve {}", skills_dir.display())); @@ -156,9 +179,12 @@ fn home_skills() -> Result> { if !resolved_inside(&root, &path) || !path.is_file() { continue; } - skills.push((name, path)); + skills.push(HomeSkill { + name, + skill_md: path, + }); } - Ok(skills) + Ok(Some(HomeCatalog { root, skills })) } /// A single path component that can be one line of `.git/info/exclude`. @@ -192,9 +218,11 @@ static WRITTEN: Mutex> = Mutex::new(None); /// Link every Factory skill into `worktree`'s project skills for `adapter`, /// `.claude/skills/` or `.agents/skills/`, writing them out first if the /// Command has not yet, and make sure the repository's `.git/info/exclude` -/// keeps them out of git. Home skills are linked under their own names the -/// same way, except where the repository already has that skill. The links -/// are left in place: they go with the worktree. +/// keeps them out of git. Home skills are copied into that same folder and +/// linked under their own names, except where the repository already has +/// that skill or the name is a Factory skill. The copy is what a Trial +/// reads: its sandbox binds this folder and not `~/.thirdshift/skills`. The +/// links are left in place: they go with the worktree. pub fn link_into(worktree: &Path, adapter: &dyn Adapter) -> Result<()> { let dir = adapter.project_skills(); // Refuse an unsafe path before writing skills or updating excludes. @@ -211,34 +239,96 @@ pub fn link_into(worktree: &Path, adapter: &dyn Adapter) -> Result<()> { } /// Link each home skill under its own name, unless the repository already has -/// that skill or the destination is some other entry. Each linked name is -/// excluded on its own, because home skills do not share the `thirdshift-` -/// prefix. The entry stays, as the Factory pattern does. +/// that skill, the name is a Factory skill, or the destination is some other +/// entry. The worktree link points at a copy in the Factory skills folder. +/// Each linked name is excluded on its own, because home skills do not share +/// the `thirdshift-` prefix. The entry stays, as the Factory pattern does. fn link_home_skills(worktree: &Path, project_skills: &CheckedDirectory, dir: &str) -> Result<()> { - let skills = home_skills()?; - if skills.is_empty() { + let Some(catalog) = read_home_skills()? else { + return Ok(()); + }; + if catalog.skills.is_empty() { return Ok(()); } // Probe each name. Listing the project skills directory would fail when // it is searchable but not readable, which Factory linking allows. let root = fs::canonicalize(worktree) .with_context(|| format!("could not resolve {}", worktree.display()))?; - for (name, skill_md) in skills { - if repository_has_skill(&root, worktree, &name) { + for skill in catalog.skills { + if factory_skill(&skill.name) || repository_has_skill(&root, worktree, &skill.name) { continue; } - let Some(directory) = skill_md.parent() else { + let Some(home_directory) = skill.skill_md.parent() else { continue; }; - let destination = worktree.join(dir).join(&name); - if !home_link_allowed(&destination, directory)? { + let destination = worktree.join(dir).join(&skill.name); + let published = written_out()?.join(&skill.name); + if !home_link_allowed(&destination, &published, home_directory)? { continue; } + publish_home_skill(&catalog.root, &skill.name, home_directory, &published)?; exclude( &Git::new(worktree), - &exclude_literal(&format!("/{dir}/{name}")), + &exclude_literal(&format!("/{dir}/{}", skill.name)), )?; - project_skills.link(directory, Path::new(&name))?; + project_skills.link(&published, Path::new(&skill.name))?; + } + Ok(()) +} + +/// A Factory skill already uses this directory name in the written-out folder. +fn factory_skill(name: &str) -> bool { + SKILLS.dirs().any(|dir| dir.path().to_str() == Some(name)) +} + +/// Copy `source` into `destination`, which lives in the Factory skills folder. +/// A symlink is kept only by copying the file it names when that file stays +/// inside `root`, so the published tree does not point at the operator's home. +fn publish_home_skill(root: &Path, name: &str, source: &Path, destination: &Path) -> Result<()> { + if destination.exists() { + fs::remove_dir_all(destination) + .with_context(|| format!("could not replace {}", destination.display()))?; + } + copy_inside(source, destination, root, 0)?; + if !destination.join("SKILL.md").is_file() { + bail!("could not publish home skill {name}"); + } + Ok(()) +} + +fn copy_inside(source: &Path, destination: &Path, root: &Path, depth: u32) -> Result<()> { + if depth > 32 { + bail!("home skill {} is nested too deeply", source.display()); + } + fs::create_dir_all(destination) + .with_context(|| format!("could not create {}", destination.display()))?; + let entries = + fs::read_dir(source).with_context(|| format!("could not list {}", source.display()))?; + for entry in entries { + let entry = entry.with_context(|| format!("could not list {}", source.display()))?; + let path = entry.path(); + if !resolved_inside(root, &path) { + continue; + } + let file_type = entry + .file_type() + .with_context(|| format!("could not inspect {}", path.display()))?; + let target = destination.join(entry.file_name()); + if file_type.is_symlink() { + let metadata = fs::metadata(&path) + .with_context(|| format!("could not inspect {}", path.display()))?; + if metadata.is_dir() { + copy_inside(&path, &target, root, depth + 1)?; + } else if metadata.is_file() { + fs::copy(&path, &target) + .with_context(|| format!("could not copy {}", path.display()))?; + } + } else if file_type.is_dir() { + copy_inside(&path, &target, root, depth + 1)?; + } else if file_type.is_file() { + fs::copy(&path, &target) + .with_context(|| format!("could not copy {}", path.display()))?; + } } Ok(()) } @@ -255,16 +345,18 @@ fn repository_has_skill(root: &Path, worktree: &Path, name: &str) -> bool { } /// The destination is absent, or it is already this home skill's link and can -/// be refreshed. A repository file or directory, or a link to somewhere else, -/// stays. -fn home_link_allowed(destination: &Path, directory: &Path) -> Result { +/// be refreshed. A link still aimed at the home directory is refreshed onto +/// the published copy. A repository file or directory, or a link to somewhere +/// else, stays. +fn home_link_allowed(destination: &Path, published: &Path, home_directory: &Path) -> Result { match fs::symlink_metadata(destination) { Err(error) if error.kind() == ErrorKind::NotFound => Ok(true), Err(error) => { Err(error).with_context(|| format!("could not inspect {}", destination.display())) } Ok(metadata) if metadata.file_type().is_symlink() => { - Ok(fs::read_link(destination).ok().as_deref() == Some(directory)) + let target = fs::read_link(destination).ok(); + Ok(target.as_deref() == Some(published) || target.as_deref() == Some(home_directory)) } Ok(_) => Ok(false), } diff --git a/tests/process.rs b/tests/process.rs index 85737f74..a19628d0 100644 --- a/tests/process.rs +++ b/tests/process.rs @@ -944,6 +944,71 @@ skill = \"house-style\" assert!(!cwd.exists(), "worktree was kept: {}", result.stderr); } +#[test] +fn a_home_skill_link_is_readable_in_the_trial_sandbox() { + let scenario = Scenario::new(); + let house = scenario.path("home/.thirdshift/skills/house-style"); + let outside = scenario.path("outside-notes"); + fs::create_dir_all(&house).unwrap(); + fs::create_dir_all(&outside).unwrap(); + fs::write(house.join("SKILL.md"), "# House style\n").unwrap(); + fs::write(house.join("notes.md"), "# Notes\n").unwrap(); + fs::write(outside.join("secret.md"), "secret\n").unwrap(); + symlink(outside.join("secret.md"), house.join("secret.md")).unwrap(); + write_process( + &scenario, + "style", + "\ +version = 1 +description = \"Apply the house style\" + +[[steps]] +name = \"style\" +skill = \"house-style\" +", + ); + // The Trial sandbox binds the repository, the Factory skills folder and a + // fresh home. /usr, /lib, /lib64 and /bin are here only so /usr/bin/test + // can run; the skill binds are those three. The link used to point at + // ~/.thirdshift/skills, which that sandbox does not mount. + scenario.agent_does( + "test \"$(cat .claude/skills/house-style/SKILL.md)\" = '# House style'\n\ + test \"$(cat .claude/skills/house-style/notes.md)\" = '# Notes'\n\ + test ! -e .claude/skills/house-style/secret.md\n\ + factory=$(dirname \"$(readlink .claude/skills/thirdshift-implement)\")\n\ + case \"$(readlink .claude/skills/house-style)\" in\n\ + \"$factory\"/*) ;;\n\ + *) printf 'home skill link is outside the Factory skills folder\n' >&2; exit 1 ;;\n\ + esac\n\ + case \"$(readlink .claude/skills/house-style)\" in\n\ + *.thirdshift/skills/*) printf 'home skill link points at the home directory\n' >&2; exit 1 ;;\n\ + esac\n\ + fresh=$(mktemp -d)\n\ + bwrap --bind \"$PWD\" \"$PWD\" \\\n\ + --ro-bind \"$factory\" \"$factory\" \\\n\ + --bind \"$fresh\" \"$fresh\" \\\n\ + --ro-bind /usr /usr --ro-bind /lib /lib --ro-bind /lib64 /lib64 --ro-bind /bin /bin \\\n\ + -- /usr/bin/test -f \"$PWD/.claude/skills/house-style/SKILL.md\"\n\ + test -z \"$(git status --porcelain --untracked-files=all)\"\n\ + printf 'readable' > \"$FAKE_CLAUDE_FINAL_MESSAGE\"", + ); + + let result = scenario.run(&["process", "style"]); + + assert_eq!(result.code, Some(0), "{}", result.stderr); + assert_eq!(result.stdout, "readable\n"); + let calls = scenario.claude_calls(); + let prompt = calls[0]["prompt"].as_str().unwrap(); + let found = scenario.path("home/.thirdshift/skills/house-style/SKILL.md"); + assert!( + prompt.contains(&format!( + "house-style skill. Read and follow `{}`", + found.display() + )), + "{prompt}" + ); +} + #[test] fn a_repository_skill_wins_over_the_home_skill() { for folder in [".agents/skills", ".claude/skills"] { From 808f48e5257829d6b4d4df20cbc008ec4d1d40f5 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 26 Oct 2026 19:41:20 -0400 Subject: [PATCH 3/3] Check the published home skill without running bwrap The Trial sandbox binds the Factory skills folder, not ~/.thirdshift/skills. The readability test invoked bwrap to prove the copy was visible under those binds. CI does not install bwrap, so the session exited 127. The published tree is plain files in that folder. --- tests/process.rs | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/tests/process.rs b/tests/process.rs index e3166fab..c0267137 100644 --- a/tests/process.rs +++ b/tests/process.rs @@ -968,27 +968,29 @@ skill = \"house-style\" ", ); // The Trial sandbox binds the repository, the Factory skills folder and a - // fresh home. /usr, /lib, /lib64 and /bin are here only so /usr/bin/test - // can run; the skill binds are those three. The link used to point at - // ~/.thirdshift/skills, which that sandbox does not mount. + // fresh home. It does not mount ~/.thirdshift/skills. The worktree link + // has to name a plain copy in that folder: a symlink back to the home + // directory is readable in this session and absent in the sandbox. scenario.agent_does( "test \"$(cat .claude/skills/house-style/SKILL.md)\" = '# House style'\n\ test \"$(cat .claude/skills/house-style/notes.md)\" = '# Notes'\n\ test ! -e .claude/skills/house-style/secret.md\n\ factory=$(dirname \"$(readlink .claude/skills/thirdshift-implement)\")\n\ - case \"$(readlink .claude/skills/house-style)\" in\n\ + published=$(readlink .claude/skills/house-style)\n\ + case \"$published\" in\n\ \"$factory\"/*) ;;\n\ *) printf 'home skill link is outside the Factory skills folder\n' >&2; exit 1 ;;\n\ esac\n\ - case \"$(readlink .claude/skills/house-style)\" in\n\ + case \"$published\" in\n\ *.thirdshift/skills/*) printf 'home skill link points at the home directory\n' >&2; exit 1 ;;\n\ esac\n\ - fresh=$(mktemp -d)\n\ - bwrap --bind \"$PWD\" \"$PWD\" \\\n\ - --ro-bind \"$factory\" \"$factory\" \\\n\ - --bind \"$fresh\" \"$fresh\" \\\n\ - --ro-bind /usr /usr --ro-bind /lib /lib --ro-bind /lib64 /lib64 --ro-bind /bin /bin \\\n\ - -- /usr/bin/test -f \"$PWD/.claude/skills/house-style/SKILL.md\"\n\ + test -f \"$published/SKILL.md\"\n\ + test ! -L \"$published/SKILL.md\"\n\ + test \"$(cat \"$published/SKILL.md\")\" = '# House style'\n\ + test -f \"$published/notes.md\"\n\ + test ! -L \"$published/notes.md\"\n\ + test \"$(cat \"$published/notes.md\")\" = '# Notes'\n\ + test ! -e \"$published/secret.md\"\n\ test -z \"$(git status --porcelain --untracked-files=all)\"\n\ printf 'readable' > \"$FAKE_CLAUDE_FINAL_MESSAGE\"", );