[JacobStephens2]

Summary

A Step's skill is resolved in this order, then linked into the worktree the way the Factory skills are:

find(name)
  Factory directory named exactly name
  repository .agents/skills/name, or .claude/skills/name when that is the only copy
  ~/.thirdshift/skills/name/SKILL.md
  Factory shorthand thirdshift-name

link home skill
  copy the directory into the Factory skills folder
  symlink that copy into the project skills directory under its own name
  add one .git/info/exclude line for that name

thirdshift process check prints the home file, which is where the skill was found. The session prompt names that same file. The worktree link points at the copy. A Trial already binds the Factory skills folder and does not bind ~/.thirdshift/skills, so the copy is what the sandbox can read.

A repository skill of the same name wins and is left in place. A skill file that resolves outside the directory being searched is not a match.

Seam: thirdshift process and thirdshift process check through tests/process.rs. skills::find is the same lookup thirdshift skill uses.

Evidence

• Before: a worktree link at ~/.thirdshift/skills/house-style is readable on the host and unreadable inside the Trial's binds. With /usr, /lib, /lib64, and /bin mounted only so /usr/bin/test can run, this exits 1:

  bwrap --bind "$repo" "$repo" \
    --ro-bind "$factory" "$factory" \
    --bind "$fresh" "$fresh" \
    --ro-bind /usr /usr --ro-bind /lib /lib --ro-bind /lib64 /lib64 --ro-bind /bin /bin \
    -- /usr/bin/test -f "$repo/.claude/skills/house-style/SKILL.md"

  The reviewer's command as written (bwrap … -- test -f … with only the repository, an empty factory directory, and a fresh home) exits 1 with bwrap: execvp test: No such file or directory, because test is not in that mount set. The run above is the same three skill binds with a test binary that can execute, and it fails because the skill file is not there. Mounting ~/.thirdshift/skills makes that same check exit 0.

  After: cargo test --test process -- a_home_skill_link_is_readable_in_the_trial_sandbox passes. The session reads SKILL.md and notes.md through .claude/skills/house-style, the link target is inside the Factory skills folder, a sibling symlink that leaves the home skills directory is not copied, and the same bwrap binds can read SKILL.md. The prompt still names the home file.

• cargo test --bin thirdshift skills::tests — 4 passed.

• cargo clippy --all-targets -- -D warnings — passed.

• npm run docs:check --prefix site — passed.

• /usr/bin/time -p scripts/affected-tests origin/issue-877 on a3d87a08 exited 0 in real 2925.56s, compilation included. npm test --prefix site passed 50 tests. cargo nextest run passed 3120 tests, skipped 70, in 2554.229s.

• After merging origin/issue-877 (93d6f8ed, pull request Custom process: named throwaway worktrees and parallel groups #1409), cargo test --test process passed 31 tests (1 ignored) and npm run docs:check --prefix site passed. That merge did not change src/skills.rs. link_into runs once per Step worktree, including a parallel group.

Merge Danger

Door: two-way

Reverting the commits removes the home-skill lookup and the published copy. Exclude lines already written stay, as the Factory thirdshift-* lines do.

Blast Radius: sessions

Every session that links Factory skills also links home skills, including Ticket runs, Resumes, the security-review fallback, standalone thirdshift skill, and Trials. A Trial can read those published copies because they sit in the Factory skills folder it already binds. check still prints the home path.

Unaddressed findings

Standards

1. Duplicated Code in src/skills.rs (repository_skills, read_home_skills, repository_has_skill). The two directory scans already disagree on purpose: a non-UTF-8 repository directory name fails the lookup (repository_skills, entry.file_name().into_string().map_err), and a non-UTF-8 home name is skipped (read_home_skills, let Ok(name) = entry.file_name().into_string() else { continue; }). The reviewer said that split is not a claim that either policy is wrong. repository_has_skill probes SKILL.md for one name and does not read_dir the project skills directory. Listing that directory fails skills::tests::links_in_ordinary_directories_without_directory_read_permission, which sets the directory mode to 0o300.

2. Duplicated Code in docs/guide.md. Standalone skills and Custom processes both state the lookup order. The acceptance criterion is that docs/guide.md documents that order, and the Custom process section is where a process reader looks. The process wording now matches the standalone wording: Factory exact name, then .agents/skills/<name>/SKILL.md, or .claude/skills/<name>/SKILL.md when only that copy exists, then ~/.thirdshift/skills/<name>/SKILL.md, then thirdshift-<name>. The earlier drift ("and .claude/skills" against "or .claude/skills … when only that copy exists") is gone.

Spec

1. Scope creep of shared skills::find and skills::link_into onto thirdshift skill, Ticket runs, Resumes, the security-review fallback, and Trials. The spec says a home skill is linked into the worktree the way the Factory skills are (ADR-0012). ADR-0012's mechanism is that shared link_into, called from src/session.rs (within_sandboxed, checked_within, and the resume path) and from src/skill.rs. The dangling Trial link was the part that was wrong. tests/process.rs a_home_skill_link_is_readable_in_the_trial_sandbox keeps the reviewer's binds (repository, Factory skills folder, fresh home) and passes: the link points at the published copy, and test -f inside those binds succeeds.

Security

No unaddressed findings introduced by this change. Guidance-mode review of src/skills.rs against the supply-chain plugin-trust and agent-instruction attack classes: home skills are same-user operator content (per the skill guidance, intended installation of same-user plugins is not a privilege boundary, and sessions run fully trusted per ADR-0013), so no cross-principal boundary is crossed. Confinement holds on both new paths: find resolves names by exact lookup only, home and repository entries that canonicalize outside the searched directory are skipped (resolved_inside), home skill directory names are restricted to a single path component, and the published copy drops symlinks that leave the home skills directory. Evidence: cargo test --bin thirdshift skills::tests (4 passed), cargo test --test process -- skill (5 passed, including the outside-directory symlink cases), plus a local git check-ignore probe confirming the per-name .git/info/exclude lines match. No fix was needed, so no commits were made in this review.

Security review outcome

No unaddressed introduced findings.

Review coverage

Neither axis left a changed file unread. site/docs/index.html was excluded as generated output of docs/guide.md (npm run docs:check --prefix site exited 0; the placeholder is escaped as ~/.thirdshift/skills/<name>/SKILL.md).

Closes #1384

Built with grok grok 1.0.70 (f50904eaee6c) [stable] · default model · default effort