diff --git a/.claude/skills/implementer/SKILL.md b/.claude/skills/implementer/SKILL.md index db44295..5f8eb74 100644 --- a/.claude/skills/implementer/SKILL.md +++ b/.claude/skills/implementer/SKILL.md @@ -9,11 +9,13 @@ The turn contract (one `bridge_reply`, `bridge_ask` for the lead's decisions, ho never merge, never commit `.mcp.json` or `wiki/`) is in **`CLAUDE.md` → Bridge communication → Worker** and already applies. This skill is only the *implement-and-hand-off procedure*. -You run in an **isolated git worktree on your own branch** — a full peer of the primary (same -repo, `CLAUDE.md`, skills, MCP), differing in the model behind you and the branch you sit on. +You run in an **isolated git worktree on your own branch** — a full peer of the primary (same repo, +`CLAUDE.md`, skills), differing in the model behind you and the branch you sit on. Your MCP surface +is **only what your launcher mounted** (the bridge): the primary's IDE and forge servers are not +yours, and the worktree's `.mcp.json` is deliberately emptied so you cannot inherit them. The worktree model is documented in [`docs/Worker-Git-Workflow.md`](../../../docs/Worker-Git-Workflow.md). -## 1. Confirm where you are +## 1. Confirm where you are — then never leave Before touching anything: @@ -26,13 +28,36 @@ git status # should be clean at the start Do **all** work here, on this branch. Never `git checkout main`, never rebase onto or push to `main`. The branch is your isolation — respect it. +**Every path you read, edit, or build is relative to that root.** Work from `$PWD`; if a tool, a +brief, or your own memory hands you an absolute path, check it starts with your worktree root +before you touch it, and stop if it doesn't. An absolute path pointing anywhere else is the +primary's checkout — editing there while building here means **every build you run is of code that +does not contain your changes**, and it passes while your work goes nowhere. This has happened: +a worker made all 59 of its edits in the primary's tree and never noticed. + +```bash +test "$(git rev-parse --show-toplevel)" = "$PWD" || cd "$(git rev-parse --show-toplevel)" +``` + ## 2. Implement - Implement exactly the scope the lead named. Keep the diff focused; note anything out of scope in your reply instead of widening it. - Match the surrounding code's style, naming, and idioms. -- Run whatever build/test you can — `mvn clean install` from the module root. Read its **full** - output; a piped `mvn ... | tail` hides failures. + +**Acceptance criterion — a green build, quoted.** Your work is not done until this passes *inside +your worktree*: + +```bash +cd "$(git rev-parse --show-toplevel)/bridged" && mvn clean install +echo "exit=$?" +``` + +Read its **full** output — never pipe it through `tail`/`head`/`grep`, which hide a failure behind +a zero exit. Then quote the real `Tests run: … Failures: … Errors: …` line and the +`BUILD SUCCESS`/`FAILURE` verbatim in your reply. If it does not go green, say so with the actual +error; a failing build honestly reported is a usable result, a claimed-green one is not. You have +no IDE MCP tools, so `mvn` is your only verification — never claim a check you had no way to run. ## 3. Commit @@ -41,7 +66,8 @@ git add # explicitly — never `git add -A` / `git a git commit -m ": " ``` -`.mcp.json` will show as modified. Leave it — it is `--skip-worktree` and not yours to commit. +`.mcp.json` is neutralized and `--skip-worktree` in your worktree — never `git add` it, and never +"restore" it from the primary's copy. Same for `wiki/` (a submodule with its own remote). ## 4. Push @@ -84,8 +110,9 @@ The reply is the entire handoff; the lead cannot see your terminal. ``` PR: " + branch name> branch: -files: -tests: "> +root: +files: +build: "> summary: <2-3 lines: what you implemented and any caveat the reviewer needs> ``` @@ -97,7 +124,8 @@ sequenceDiagram participant G as git / gitea L->>I: delegated task (you are in a worktree on your branch) - I->>I: implement + build/test here + I->>I: "implement here — every path under $PWD" + I->>I: "mvn clean install in this worktree, unpiped, until green" I->>G: git commit (never .mcp.json / wiki) I->>G: git push -u origin HEAD I->>G: POST /pulls (GITEA_TOKEN) — open PR to main diff --git a/bridged/bridged.example.yaml b/bridged/bridged.example.yaml index 6fc69bb..112e5e9 100644 --- a/bridged/bridged.example.yaml +++ b/bridged/bridged.example.yaml @@ -64,7 +64,16 @@ herdrSocket: ~/.config/herdr/herdr.sock # skills/MCP/hooks. Omit to leave the worker on the host default. # parityOverlay → repo-relative paths copied primary→worktree so a worker in a provisioned # worktree sees the same local config (CB-301-ext). Omit for the default set: -# [.mcp.json, .claude/settings.local.json, .env, .envrc]. +# [.claude/settings.local.json, .env, .envrc]. +# +# Do NOT add .mcp.json (CB-525). A worker's tools are whatever its launcher +# mounts — the bridge, and nothing else. Replicating the primary's MCP config +# handed a worker the primary's IDE servers, which are bound to the primary's +# checkout, so its navigation returned paths OUTSIDE its own worktree: one +# worker made all 59 of its edits in the primary tree while compiling its +# worktree, and every build it ran was of code that did not contain them. +# bridged neutralizes a provisioned worktree's .mcp.json for this reason; +# listing it here would copy the primary's back over that. # gitTokenEnv → host env var holding the git-forge API token. When set, its value is injected # as GITEA_TOKEN so the worker can open its OWN PR at checkpoint (CB-302). # Opt-in by design — omit and the worker gets no PR-create grant (push over @@ -106,7 +115,7 @@ workers: # gitHostEnv: GITEA_HOST # defaults to GITEA_HOST; injected only with gitTokenEnv # configDir: /Users/me/.ccs/instances/gx10 # CLAUDE_CONFIG_DIR — inherit that profile's skills/MCP # cwd: /Users/me/src/myrepo # pin the working dir; omit to inherit the primary's - # parityOverlay: [".mcp.json", ".claude/settings.local.json", ".env", ".envrc"] + # parityOverlay: [".claude/settings.local.json", ".env", ".envrc"] # never add .mcp.json — see above ollama: baseUrl: http://ollama.ltms.dev # local/self-hosted; usually no token placement: tab diff --git a/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java b/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java index 27f31c4..c1816df 100644 --- a/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java +++ b/bridged/src/main/java/dev/ltms/bridged/config/BridgedConfig.java @@ -140,8 +140,12 @@ public record BridgedConfig( placement = (placement == null || placement.isBlank()) ? "tab" : placement.toLowerCase(); workspace = (workspace == null || workspace.isBlank()) ? "bridged-workers" : workspace; tabLabel = (tabLabel == null || tabLabel.isBlank()) ? "worker: {profile} #{n}" : tabLabel; + // CB-525: .mcp.json is deliberately NOT here. Replicating the primary's MCP config gave a + // worker the primary's IDE servers, which are bound to the primary's checkout — so its + // navigation returned paths outside its own worktree. GitWorktrees now neutralizes that + // file instead; a worker's tools are whatever its launcher mounts. parityOverlay = (parityOverlay == null || parityOverlay.isEmpty()) - ? List.of(".mcp.json", ".claude/settings.local.json", ".env", ".envrc") + ? List.of(".claude/settings.local.json", ".env", ".envrc") : List.copyOf(parityOverlay); // gitTokenEnv stays null when unset (opt-in). gitHostEnv defaults so operators enabling // checkpoints need only set gitTokenEnv; it is injected only alongside a resolved token. diff --git a/bridged/src/main/java/dev/ltms/bridged/session/GitWorktrees.java b/bridged/src/main/java/dev/ltms/bridged/session/GitWorktrees.java index 5ab087c..9bf58f4 100644 --- a/bridged/src/main/java/dev/ltms/bridged/session/GitWorktrees.java +++ b/bridged/src/main/java/dev/ltms/bridged/session/GitWorktrees.java @@ -27,6 +27,12 @@ public final class GitWorktrees implements Worktrees { private static final Logger log = LoggerFactory.getLogger(GitWorktrees.class); + /** Project-level MCP config. Present in the repo, so every worktree checks the primary's out. */ + private static final String MCP_CONFIG = ".mcp.json"; + + /** What {@link #isolateToolSurface} writes: a valid, explicitly empty server map. */ + private static final String NEUTRAL_MCP_CONFIG = "{\n \"mcpServers\": {}\n}\n"; + private final String configuredRoot; private final SecureRandom random = new SecureRandom(); private final AtomicLong seq = new AtomicLong(); @@ -55,9 +61,41 @@ public final class GitWorktrees implements Worktrees { String wt = path.toAbsolutePath().toString(); log.info("adding worktree branch={} path={} base={}", branch, wt, base); exec("git", "-C", repoRoot, "worktree", "add", wt, "-b", branch, base); + isolateToolSurface(wt); return wt; } + /** + * Neutralize the worktree's project MCP config so a worker inherits only the tools its launcher + * mounts (the bridge, via {@code --mcp-config}) — never the primary's. + * + *

This is unconditional, and it is not the same job as the parity overlay. The repo's own + * committed {@code .mcp.json} declares the primary's IDE servers, so a fresh checkout mounts them + * whether or not the overlay copies anything; a worker that inherits them navigates and edits + * through tools bound to the primary's IntelliJ project, which silently hands it absolute + * paths outside its own worktree. That is not hypothetical: a CB-523 worker made all 59 of its + * edits in the primary checkout while compiling its worktree, so every build it ran was of code + * that did not contain its changes. + * + *

Writing an empty server map (rather than deleting the file) keeps a project-level + * {@code .mcp.json} present and explicit, and the {@code --skip-worktree} bit keeps the + * neutralized copy from ever showing up as a local modification the worker might commit. + */ + private void isolateToolSurface(String worktreePath) { + Path root = Path.of(worktreePath).toAbsolutePath().normalize(); + Path mcp = root.resolve(MCP_CONFIG); + try { + Files.writeString(mcp, NEUTRAL_MCP_CONFIG); + } catch (IOException e) { + throw new WorktreeException("cannot neutralize " + MCP_CONFIG + " in the worktree: " + + e.getMessage(), e); + } + if (isTracked(root, MCP_CONFIG)) { + exec("git", "-C", worktreePath, "update-index", "--skip-worktree", MCP_CONFIG); + } + log.debug("neutralized {} — worker tool surface is launcher-mounted only", MCP_CONFIG); + } + @Override public void remove(String repoRoot, String worktreePath) { Path p = Path.of(worktreePath); diff --git a/bridged/src/test/java/dev/ltms/bridged/session/GitWorktreesTest.java b/bridged/src/test/java/dev/ltms/bridged/session/GitWorktreesTest.java new file mode 100644 index 0000000..126a25b --- /dev/null +++ b/bridged/src/test/java/dev/ltms/bridged/session/GitWorktreesTest.java @@ -0,0 +1,119 @@ +package dev.ltms.bridged.session; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.concurrent.TimeUnit; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * CB-525 acceptance test for tool-surface isolation. This is one of the few tests that drives real + * {@code git} — the behaviour under test is precisely what {@link GitWorktrees} does to a checkout, + * so a fake would assert nothing. Everything happens inside a {@link TempDir} throwaway repo. + */ +class GitWorktreesTest { + + /** A project MCP config with servers in it — what this repo actually commits. */ + private static final String WITH_SERVERS = """ + { + "mcpServers": { + "jetbrains": { "type": "sse", "url": "http://localhost:64342/sse" } + } + } + """; + + private static Path initRepo(Path dir) throws Exception { + Files.createDirectories(dir); + git(dir, "init", "-q", "-b", "main"); + git(dir, "config", "user.email", "test@example.invalid"); + git(dir, "config", "user.name", "Test"); + Files.writeString(dir.resolve(".mcp.json"), WITH_SERVERS); + Files.writeString(dir.resolve("README.md"), "seed\n"); + git(dir, "add", ".mcp.json", "README.md"); + git(dir, "commit", "-q", "-m", "seed"); + return dir; + } + + private static void git(Path cwd, String... args) throws Exception { + List cmd = new java.util.ArrayList<>(List.of("git")); + cmd.addAll(List.of(args)); + Process p = new ProcessBuilder(cmd).directory(cwd.toFile()).redirectErrorStream(true).start(); + String out = new String(p.getInputStream().readAllBytes()); + assertTrue(p.waitFor(30, TimeUnit.SECONDS), "git timed out: " + String.join(" ", cmd)); + assertEquals(0, p.exitValue(), "git " + String.join(" ", args) + " failed:\n" + out); + } + + /** Pending changes to {@code .mcp.json} in {@code cwd}, empty when git considers it unmodified. */ + private static String mcpStatus(Path cwd) throws Exception { + Process p = new ProcessBuilder("git", "status", "--porcelain", "--", ".mcp.json") + .directory(cwd.toFile()).redirectErrorStream(true).start(); + String out = new String(p.getInputStream().readAllBytes()); + assertTrue(p.waitFor(30, TimeUnit.SECONDS), "git status timed out"); + return out; + } + + /** + * The heart of CB-525: a provisioned worktree must not inherit the primary's MCP servers. Without + * the isolation step the checked-out {@code .mcp.json} carries them in, and a worker navigating + * through the primary's IDE servers edits the primary's tree while building its own. + */ + @Test + void aProvisionedWorktreeInheritsNoMcpServers(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + String wt = new GitWorktrees(tmp.resolve("wts").toString()) + .add(repo.toString(), "cb-525-a", "HEAD"); + + Path mcp = Path.of(wt).resolve(".mcp.json"); + assertTrue(Files.exists(mcp), ".mcp.json must still exist — present and explicitly empty"); + String body = Files.readString(mcp); + assertFalse(body.contains("jetbrains"), "worktree inherited the primary's MCP servers:\n" + body); + assertTrue(body.replaceAll("\\s+", "").contains("\"mcpServers\":{}"), + "expected an explicitly empty server map, got:\n" + body); + } + + /** Neutralizing must not look like work in progress, or a worker would commit it into its PR. */ + @Test + void theNeutralizedConfigIsNotAPendingLocalModification(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + String wt = new GitWorktrees(tmp.resolve("wts").toString()) + .add(repo.toString(), "cb-525-b", "HEAD"); + + assertEquals("", mcpStatus(Path.of(wt)), + "the neutralized .mcp.json shows as modified — --skip-worktree did not take"); + } + + /** Isolation is the worktree's business only; the primary's own checkout must be untouched. */ + @Test + void thePrimaryCheckoutIsLeftAlone(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + new GitWorktrees(tmp.resolve("wts").toString()).add(repo.toString(), "cb-525-c", "HEAD"); + + assertEquals(WITH_SERVERS, Files.readString(repo.resolve(".mcp.json")), + "the primary's .mcp.json was rewritten — isolation reached out of the worktree"); + } + + /** A repo that commits no {@code .mcp.json} still gets one, so nothing can be inherited later. */ + @Test + void aRepoWithoutAnMcpConfigStillGetsANeutralOne(@TempDir Path tmp) throws Exception { + Path repo = tmp.resolve("repo"); + Files.createDirectories(repo); + git(repo, "init", "-q", "-b", "main"); + git(repo, "config", "user.email", "test@example.invalid"); + git(repo, "config", "user.name", "Test"); + Files.writeString(repo.resolve("README.md"), "seed\n"); + git(repo, "add", "README.md"); + git(repo, "commit", "-q", "-m", "seed"); + + String wt = new GitWorktrees(tmp.resolve("wts").toString()) + .add(repo.toString(), "cb-525-d", "HEAD"); + + // Untracked is the normal case here, so the --skip-worktree branch must be skipped rather + // than run and fail: `update-index --skip-worktree` on an unknown path exits non-zero. + String body = Files.readString(Path.of(wt).resolve(".mcp.json")); + assertTrue(body.replaceAll("\\s+", "").contains("\"mcpServers\":{}"), body); + } +} diff --git a/bridged/src/test/java/dev/ltms/bridged/session/WorktreeSessionManagerTest.java b/bridged/src/test/java/dev/ltms/bridged/session/WorktreeSessionManagerTest.java index f36cba0..7b2bf9f 100644 --- a/bridged/src/test/java/dev/ltms/bridged/session/WorktreeSessionManagerTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/session/WorktreeSessionManagerTest.java @@ -82,7 +82,7 @@ class WorktreeSessionManagerTest { void worktreeAcquireRunsParityOverlayWithProfileDefaults() { FakeHerdr herdr = new FakeHerdr(); FakeWorktrees worktrees = new FakeWorktrees().withRepoRoot("/repo").withPrefix("/wt") - .track(".mcp.json") + .track(".envrc") .exists(".claude/settings.local.json"); SessionManager sessions = new SessionManager(workerService(herdr), worktrees); @@ -93,11 +93,14 @@ class WorktreeSessionManagerTest { FakeWorktrees.OverlayCall overlay = worktrees.lastOverlay(); assertNotNull(overlay); assertEquals("/repo", overlay.repoRoot()); - assertEquals(List.of(".mcp.json", ".claude/settings.local.json", ".env", ".envrc"), + assertEquals(List.of(".claude/settings.local.json", ".env", ".envrc"), overlay.requested(), "default parity overlay is used when unset"); - assertEquals(List.of(".mcp.json", ".claude/settings.local.json"), overlay.copied(), + assertFalse(overlay.requested().contains(".mcp.json"), + "CB-525: replicating the primary's MCP config gives a worker the primary's IDE " + + "servers, which navigate its edits out of its own worktree"); + assertEquals(List.of(".claude/settings.local.json", ".envrc"), overlay.copied(), "existing paths are copied; missing paths are skipped"); - assertEquals(List.of(".mcp.json"), overlay.skipWorktree(), + assertEquals(List.of(".envrc"), overlay.skipWorktree(), "tracked copied paths are --skip-worktree'd"); }