From b3f98f13f25aea90a175c90e9d9780711490dc90 Mon Sep 17 00:00:00 2001 From: Atlas from Plumb Date: Wed, 30 Sep 2026 23:41:26 +1000 Subject: [PATCH 1/3] fix(session_start): report which pin a re-pin moved (#517) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The re-pin callback (repinWorkspace, wired through WithRepin) now returns a tools.RepinReport: the pin the daemon moved (the caller's own shard or the connection's), that pin's previous root, how many OTHER agents' shards followed a connection move, and the root the caller's next relative path resolves against. repinWorkspaceFrom, repinConnection and repinAgent carry the outcome up; followConnectionShards returns the ids it moved. session_start renders "Re-pinned your pin: A → B" or "Re-pinned this connection's pin: A → B (N other agents follow it)" plus a "Next relative-path call resolves against:" line in both the full and brief packets, and the "# Workspace:" header now names the caller's own effective root. Previously an anonymous default-scope re-pin on a shared connection dragged its peers silently, and scope "connection" from an agent holding its own pin showed its own root as "from" and the connection's new root as its workspace. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 14 + internal/cli/conn_agent_language_test.go | 4 +- internal/cli/conn_agent_shard.go | 28 +- internal/cli/conn_attribution_test.go | 6 +- internal/cli/conn_canonicalroot_test.go | 9 +- internal/cli/conn_commands_agent_cwd_test.go | 2 +- internal/cli/conn_pin_restart_test.go | 6 +- internal/cli/conn_repin.go | 54 ++-- internal/cli/conn_repin_report.go | 59 ++++ internal/cli/conn_repin_scope.go | 4 +- internal/cli/conn_restoredrift_test.go | 6 +- internal/cli/conn_shard_pin_ownership_test.go | 6 +- internal/cli/conn_stickypin_test.go | 6 +- internal/cli/repin_test.go | 9 +- internal/cli/routing_agent_boundary_test.go | 4 +- .../cli/session_start_repin_report_test.go | 271 ++++++++++++++++++ internal/tools/session_start.go | 77 ++--- internal/tools/session_start_brief.go | 11 +- internal/tools/session_start_brief_test.go | 51 +++- .../session_start_declared_agent_test.go | 8 +- internal/tools/session_start_sections.go | 11 +- internal/tools/session_start_test.go | 18 +- internal/tools/session_start_workspace.go | 113 ++++++-- 23 files changed, 634 insertions(+), 143 deletions(-) create mode 100644 internal/cli/conn_repin_report.go create mode 100644 internal/cli/session_start_repin_report_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 5664d1d8..a9a4c8fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -142,6 +142,20 @@ notices, `plumb doctor`'s shared-connection fix and the client instruction templates now describe the current refusal rule. +- **`session_start` says which pin a re-pin moved, and where your next + relative path goes.** The neutral `Re-pinned: → ` line above could + not say whether the caller's own pin moved or the connection's, which every + agent without a pin of its own follows. An anonymous re-pin on a shared + connection moved its peers without telling the caller. With + `scope: "connection"`, an agent holding its own pin was shown its own root as + "from", and the header named the connection's new root although the agent's + relative paths still resolved against its own. The daemon now reports which + pin it moved, that pin's previous root and how many other agents followed it. + Both the full and the brief packet print "Re-pinned your pin" or "Re-pinned + this connection's pin (N other agents follow it)", plus a line naming what the + caller's next relative-path call resolves against. The `# Workspace:` header + names the caller's own root. Closes #517. + - **Agent identity now reaches plumb through Claude desktop's connector, and a worktree edit no longer lands in another checkout.** Claude desktop runs one `plumb serve` (`claude_desktop_config.json`, client `local-agent-mode-plumb`) diff --git a/internal/cli/conn_agent_language_test.go b/internal/cli/conn_agent_language_test.go index 2983af8c..e8b08b52 100644 --- a/internal/cli/conn_agent_language_test.go +++ b/internal/cli/conn_agent_language_test.go @@ -132,7 +132,7 @@ func TestSameRootLanguageSwitchPreservesShardTrackers(t *testing.T) { // A same-root language change, as an ordinary re-pin would produce it. It // reports a change — the shard's language really did move — which is what // makes keeping the trackers a deliberate exception rather than a no-op. - changed, refused := s.repinAgent(ctxA, root, "go", sessionstate.PinSourceSessionStart, false) + _, changed, refused := s.repinAgent(ctxA, root, "go", sessionstate.PinSourceSessionStart, false) if refused != nil { t.Fatalf("same-root language change on a shard: %v", refused) } @@ -146,7 +146,7 @@ func TestSameRootLanguageSwitchPreservesShardTrackers(t *testing.T) { // The contrast: moving the agent to another project does start clean. other := freshTempDir(t) mustGitDir(t, other) - if moved, refused := s.repinAgent(ctxA, other, "go", sessionstate.PinSourceSessionStart, true); refused != nil || !moved { + if _, moved, refused := s.repinAgent(ctxA, other, "go", sessionstate.PinSourceSessionStart, true); refused != nil || !moved { t.Fatalf("agent move: changed=%v err=%v", moved, refused) } if s.writeTrackerFor(ctxA).Wrote(written) { diff --git a/internal/cli/conn_agent_shard.go b/internal/cli/conn_agent_shard.go index b3882ebb..a867735e 100644 --- a/internal/cli/conn_agent_shard.go +++ b/internal/cli/conn_agent_shard.go @@ -286,10 +286,14 @@ func (s *connSession) rateLimiterFor(ctx context.Context) *tools.RateLimiter { // connection being unusable, and flagging it would raise a dashboard alert // against the coordinator for a peer's call. The log line is greppable, carries // the agent id and both roots, and does not expire. -func (s *connSession) repinAgent(ctx context.Context, root, language string, origin sessionstate.PinSource, force bool) (changed bool, refused error) { +// +// prev is the shard's root BEFORE the call, read under the same sh.mu +// acquisition that moves it, so session_start can report the pin's previous +// root without a second, racy read (issue #517). +func (s *connSession) repinAgent(ctx context.Context, root, language string, origin sessionstate.PinSource, force bool) (prev string, changed bool, refused error) { sh := s.repinShard(ctx) if sh == nil { - return false, nil + return "", false, nil } // The roster sync registers or moves a session.Info, which takes a flock on // the session directory. Doing that while holding sh.mu wedges the whole @@ -306,7 +310,7 @@ func (s *connSession) repinAgent(ctx context.Context, root, language string, ori }() sh.mu.Lock() defer sh.mu.Unlock() - prev := sh.root + prev = sh.root // The guard keys on the pin ORIGIN, which a seeded shard inherits wholesale: // shardFor copies the CONNECTION's pin and its origin onto a new shard, and // attachOrRepinTo's same-root promotion branch upgrades a roots-held pin to @@ -360,7 +364,7 @@ func (s *connSession) repinAgent(ctx context.Context, root, language string, ori s.log().Warn("daemon: per-agent session_start re-pin refused — this agent's pin is sticky (issue #182)", "agent", sh.id, "pinned", prev, "requested", root, "remedy", "call session_start again with force: true to move THIS agent, or run one plumb serve per agent") - return false, refused + return prev, false, refused } if root == prev && language == sh.language { // Nothing moves — but naming the root the shard already holds is still @@ -380,7 +384,7 @@ func (s *connSession) repinAgent(ctx context.Context, root, language string, ori // issue #472 describes, by a path no live-move test exercises. // confirmShardPin cannot host this: it returns early for a shard that is // already selfPinned, which a restored-and-reconfirming one is. - return false, nil + return prev, false, nil } changed = true // The agent has CHOSEN this root (even back to the seeded one, via a @@ -405,7 +409,7 @@ func (s *connSession) repinAgent(ctx context.Context, root, language string, ori s.rehydrateReadsForAgent(sh, root) s.persistPinForAgent(sh, root, language, origin) syncRoot, syncLang = root, language - return changed, nil + return prev, changed, nil } // seedShardOnLink hydrates the linkage owner's shard from the connection's @@ -454,13 +458,17 @@ func (s *connSession) seedShardOnLink(linkage string) { // (shardsMu before sh.mu, s.mu innermost), so the per-tool-call hot path's lock // pattern is unchanged; the writes mirror repinAgent's success path, held under // one sh.mu acquisition each. -func (s *connSession) followConnectionShards(prevRoot string) { +// +// Returns the ids of the agents whose shards followed, so session_start can +// tell the caller how many other agents its connection move took with it +// (issue #517). +func (s *connSession) followConnectionShards(prevRoot string) (followed []string) { if prevRoot == "" { - return + return nil } v := s.view() if v.acquiredRoot == "" || v.acquiredRoot == prevRoot { - return + return nil } s.shardsMu.Lock() defer s.shardsMu.Unlock() @@ -490,9 +498,11 @@ func (s *connSession) followConnectionShards(prevRoot string) { // Same rule persistReadShard states: the shard's root is read under sh.mu. root, language := sh.root, sh.language sh.mu.Unlock() + followed = append(followed, sh.id) s.rehydrateReadsForAgent(sh, root) s.persistPinForAgent(sh, root, language, v.pinOrigin) } + return followed } // persistReadShard mirrors a per-agent recorded read to the durable store, keyed diff --git a/internal/cli/conn_attribution_test.go b/internal/cli/conn_attribution_test.go index 646ca13d..3c8e5410 100644 --- a/internal/cli/conn_attribution_test.go +++ b/internal/cli/conn_attribution_test.go @@ -170,7 +170,7 @@ func TestAfterToolFilesTheRowUnderTheAgentsOwnWorkspace(t *testing.T) { s.recordLogicalAgentAttach("agent-here") s.recordLogicalAgentCall("agent-elsewhere") ctx := mcp.WithLogicalAgent(context.Background(), "agent-elsewhere") - if moved, refused := s.repinAgent(ctx, agentRoot, "", sessionstate.PinSourceSessionStart, true); refused != nil || !moved { + if _, moved, refused := s.repinAgent(ctx, agentRoot, "", sessionstate.PinSourceSessionStart, true); refused != nil || !moved { t.Fatalf("agent pin: moved=%v refused=%v", moved, refused) } if got := s.workspaceFor(ctx); got != agentRoot { @@ -236,7 +236,7 @@ func TestAfterToolPrefersThePathArgumentOverTheAgentsRoot(t *testing.T) { s.recordLogicalAgentAttach("agent-here") s.recordLogicalAgentCall("agent-elsewhere") ctx := mcp.WithLogicalAgent(context.Background(), "agent-elsewhere") - if moved, refused := s.repinAgent(ctx, agentRoot, "", sessionstate.PinSourceSessionStart, true); refused != nil || !moved { + if _, moved, refused := s.repinAgent(ctx, agentRoot, "", sessionstate.PinSourceSessionStart, true); refused != nil || !moved { t.Fatalf("agent pin: moved=%v refused=%v", moved, refused) } if got := s.workspaceFor(ctx); got != agentRoot { @@ -305,7 +305,7 @@ func TestAfterToolFilesAGitCallUnderItsRepo(t *testing.T) { s.recordLogicalAgentAttach("agent-here") s.recordLogicalAgentCall("agent-elsewhere") ctx := mcp.WithLogicalAgent(context.Background(), "agent-elsewhere") - if moved, refused := s.repinAgent(ctx, agentRoot, "", sessionstate.PinSourceSessionStart, true); refused != nil || !moved { + if _, moved, refused := s.repinAgent(ctx, agentRoot, "", sessionstate.PinSourceSessionStart, true); refused != nil || !moved { t.Fatalf("agent pin: moved=%v refused=%v", moved, refused) } diff --git a/internal/cli/conn_canonicalroot_test.go b/internal/cli/conn_canonicalroot_test.go index 92e72a4a..e9c75929 100644 --- a/internal/cli/conn_canonicalroot_test.go +++ b/internal/cli/conn_canonicalroot_test.go @@ -114,7 +114,8 @@ func TestCanonicalRoot_AliasedRepinIsANoOpNotAStickyRefusal(t *testing.T) { t.Fatalf("first explicit pin: %v", err) } - root, err := s.repinWorkspace(context.Background(), alias, "", false, false) + rootRep, err := s.repinWorkspace(context.Background(), alias, "", false, false) + root := rootRep.Root if err != nil { t.Fatalf("re-pinning to the SAME project by its other spelling must be a no-op, "+ "not a sticky-pin refusal: %v", err) @@ -145,7 +146,8 @@ func TestCanonicalRoot_SyntheticRootIsCanonicalised(t *testing.T) { s := newPersistSession(t, store, ss, "proxySynth") defer s.close() - root, err := s.repinWorkspace(context.Background(), alias, "", false, false) + rootRep, err := s.repinWorkspace(context.Background(), alias, "", false, false) + root := rootRep.Root if err != nil { t.Fatalf("explicit pin on a markerless folder: %v", err) } @@ -251,7 +253,8 @@ func TestCanonicalRoot_NonexistentRootStillPins(t *testing.T) { s := newPersistSession(t, store, ss, "proxyMissing") defer s.close() - root, err := s.repinWorkspace(context.Background(), missing, "", false, false) + rootRep, err := s.repinWorkspace(context.Background(), missing, "", false, false) + root := rootRep.Root if err != nil { t.Fatalf("a nonexistent root must still pin: %v", err) } diff --git a/internal/cli/conn_commands_agent_cwd_test.go b/internal/cli/conn_commands_agent_cwd_test.go index e43e218a..24541757 100644 --- a/internal/cli/conn_commands_agent_cwd_test.go +++ b/internal/cli/conn_commands_agent_cwd_test.go @@ -35,7 +35,7 @@ func TestRunCommandResolvesTheCallingAgentsRoot(t *testing.T) { s.recordLogicalAgentCall("coordinator") s.recordLogicalAgentCall("subagent") ctx := mcp.WithLogicalAgent(context.Background(), "subagent") - if _, err := s.repinAgent(ctx, worktree, "", sessionstate.PinSourceSessionStart, false); err != nil { + if _, _, err := s.repinAgent(ctx, worktree, "", sessionstate.PinSourceSessionStart, false); err != nil { t.Fatalf("pinning the subagent to its worktree: %v", err) } if got := s.workspaceFor(ctx); filepath.Clean(got) != filepath.Clean(worktree) { diff --git a/internal/cli/conn_pin_restart_test.go b/internal/cli/conn_pin_restart_test.go index 8f1e7e59..6896d1a5 100644 --- a/internal/cli/conn_pin_restart_test.go +++ b/internal/cli/conn_pin_restart_test.go @@ -52,7 +52,8 @@ func TestPin_SurvivesDaemonRestartByteIdentical(t *testing.T) { mustGitDir(t, root) before := newPersistSession(t, store, ss, "proxyX") - pinned, err := before.repinWorkspace(context.Background(), root, "", false, false) + pinnedRep, err := before.repinWorkspace(context.Background(), root, "", false, false) + pinned := pinnedRep.Root if err != nil { t.Fatalf("repinWorkspace: %v", err) } @@ -118,7 +119,8 @@ func TestPin_RestoreDoesNotResolveAfresh(t *testing.T) { mustGitDir(t, child) before := newPersistSession(t, store, ss, "proxyX") - pinned, err := before.repinWorkspace(context.Background(), child, "", false, false) + pinnedRep, err := before.repinWorkspace(context.Background(), child, "", false, false) + pinned := pinnedRep.Root if err != nil { t.Fatalf("repinWorkspace: %v", err) } diff --git a/internal/cli/conn_repin.go b/internal/cli/conn_repin.go index 9a664e3e..d23eb13a 100644 --- a/internal/cli/conn_repin.go +++ b/internal/cli/conn_repin.go @@ -15,6 +15,7 @@ import ( "github.com/plumbkit/plumb/internal/paths" "github.com/plumbkit/plumb/internal/session" "github.com/plumbkit/plumb/internal/sessionstate" + "github.com/plumbkit/plumb/internal/tools" "github.com/plumbkit/plumb/internal/tools/txlog" ) @@ -65,7 +66,8 @@ func (s *connSession) repinRemedy() string { // folder may be any absolute path inside the target project. It is resolved to // a workspace root via pool.Detect; when no marker is found the folder itself // becomes the workspace (SynthesiseRoot), so an explicit pin always succeeds. -// Returns the resolved root. +// It is session_start's re-pin callback (WithRepin), so it reports the resolved +// root together with which pin moved — see repinReport (issue #517). // // langOverride, when a non-empty active language, forces the primary language // instead of the detected one — for an ambiguous project (e.g. an Xcode app with @@ -83,11 +85,20 @@ func (s *connSession) repinRemedy() string { // connection must not silently steal the pin the caller deliberately chose. // The refusal error names the remediation (retry with force: true), so a new // conversation deliberately switching projects still can, one round-trip later. -func (s *connSession) repinWorkspace(ctx context.Context, folder, langOverride string, force, connectionScope bool) (string, error) { +func (s *connSession) repinWorkspace(ctx context.Context, folder, langOverride string, force, connectionScope bool) (tools.RepinReport, error) { + var ( + out repinOutcome + err error + ) if connectionScope { - return s.repinConnection(ctx, folder, langOverride, force) + out, err = s.repinConnection(ctx, folder, langOverride, force) + } else { + out, err = s.repinWorkspaceFrom(ctx, folder, langOverride, sessionstate.PinSourceSessionStart, pinTriggerLive, force) } - return s.repinWorkspaceFrom(ctx, folder, langOverride, sessionstate.PinSourceSessionStart, pinTriggerLive, force) + if err != nil { + return tools.RepinReport{}, err + } + return s.repinReport(ctx, out), nil } // repinWorkspaceFrom is repinWorkspace with the pin origin made explicit. @@ -101,10 +112,15 @@ func (s *connSession) repinWorkspace(ctx context.Context, folder, langOverride s // where the request is silently not honoured — a live roots-driven re-pin kept // off an explicit pin — it therefore differs from the pin the connection still // holds; the only such caller (onRootsChanged) discards the value. -func (s *connSession) repinWorkspaceFrom(ctx context.Context, folder, langOverride string, origin sessionstate.PinSource, trigger pinTrigger, force bool) (string, error) { +// +// Besides the root, the outcome records which pin moved, that pin's previous +// root and which agents' shards followed it (issue #517). The choice between +// the caller's shard and the connection's pin is made here, so this is the +// only place that can report it. +func (s *connSession) repinWorkspaceFrom(ctx context.Context, folder, langOverride string, origin sessionstate.PinSource, trigger pinTrigger, force bool) (repinOutcome, error) { folder = paths.URIToPath(folder) if folder == "" || folder == "/" { - return "", fmt.Errorf("repin: empty workspace path %q", folder) + return repinOutcome{}, fmt.Errorf("repin: empty workspace path %q", folder) } // A RELATIVE workspace is refused rather than resolved. The only anchor // available here is the daemon's working directory, and the daemon is a @@ -123,7 +139,7 @@ func (s *connSession) repinWorkspaceFrom(ctx context.Context, folder, langOverri // refused when there is no workspace to anchor it to, and a workspace is // refused when there is nothing to anchor IT to. if !filepath.IsAbs(folder) { - return "", fmt.Errorf( + return repinOutcome{}, fmt.Errorf( "repin: workspace %q is relative, so it names no particular directory. "+ "Pass an absolute path: the daemon is shared between clients and its working "+ "directory belongs to whichever one started it, so resolving %q here would "+ @@ -141,7 +157,7 @@ func (s *connSession) repinWorkspaceFrom(ctx context.Context, folder, langOverri // home directory — SynthesiseRoot refuses it and the pin stays put. root = s.pool.SynthesiseRoot(folder, origin == sessionstate.PinSourceSessionStart) if root == "" { - return "", fmt.Errorf("refusing to pin %s as a workspace from a non-explicit source (%s): it is the home directory, so pinning it would put every credential file under it inside the boundary. Call session_start({workspace: %q}) if you really mean it", folder, pinSourceLabel(origin), folder) + return repinOutcome{}, fmt.Errorf("refusing to pin %s as a workspace from a non-explicit source (%s): it is the home directory, so pinning it would put every credential file under it inside the boundary. Call session_start({workspace: %q}) if you really mean it", folder, pinSourceLabel(origin), folder) } language = LanguageNone } @@ -156,7 +172,7 @@ func (s *connSession) repinWorkspaceFrom(ctx context.Context, folder, langOverri // carries. Refuse the drift here, before that exemption ever applies. A live // trigger is untouched: rung 1b's declared-wide-root restore still works. if err := restoreDriftErr(folder, root, trigger); err != nil { - return "", err + return repinOutcome{}, err } // Containment guard (issue #306): SynthesiseRoot above refuses a // home-IDENTITY root for a non-explicit origin, but a root CONTAINING a @@ -165,7 +181,7 @@ func (s *connSession) repinWorkspaceFrom(ctx context.Context, folder, langOverri // refused here, where the origin is in scope; an explicit session_start // still succeeds (issue #182). if err := undeclaredWideRootErr(root, origin); err != nil { - return "", err + return repinOutcome{}, err } // root is canonical here — both Detect and SynthesiseRoot resolve symlinks // (issue #263) — which is what lets the sticky-pin guard below recognise a @@ -177,12 +193,12 @@ func (s *connSession) repinWorkspaceFrom(ctx context.Context, folder, langOverri // ever apply — work done on the strength of an error, only to be refused on // the retry (PLAN-428). if err := s.refuseShardLanguageOverride(ctx, langOverride); err != nil { - return "", err + return repinOutcome{}, err } langForced := false if langOverride != "" { if err := s.languageOverrideErr(root, langOverride); err != nil { - return "", err + return repinOutcome{}, err } language = langOverride langForced = true @@ -193,15 +209,16 @@ func (s *connSession) repinWorkspaceFrom(ctx context.Context, folder, langOverri // actual issue #182 fix. The connection-level attachOrRepinTo below runs only // for an unattributed re-pin (roots, restore) or a non-shared connection. if s.repinShard(ctx) != nil { - if _, refused := s.repinAgent(ctx, root, language, origin, force); refused != nil { - return "", refused + prev, _, refused := s.repinAgent(ctx, root, language, origin, force) + if refused != nil { + return repinOutcome{}, refused } // The agent's workspace question is settled, so drop any pending-refusal // marker an earlier attempt left. Both this branch and the connection // branch below are settling paths for the CALLER, whichever scope the // pin landed on. s.clearDeclarationRefused(mcp.LogicalAgentFromCtx(ctx)) - return root, nil + return repinOutcome{root: root, scope: tools.PinScopeAgent, from: prev}, nil } // The sticky-pin guard (issue #182) lives inside attachOrRepinTo's mutation // lane: after the root resolution above, so a requested path that resolves @@ -211,18 +228,19 @@ func (s *connSession) repinWorkspaceFrom(ctx context.Context, folder, langOverri prevConnRoot := s.workspace() changed, err := s.attachOrRepinTo(ctx, root, language, origin, trigger, force, synthetic, langForced) if err != nil { - return "", err + return repinOutcome{}, err } s.attributeConnectionPin(ctx, root, origin, trigger) + out := repinOutcome{root: root, scope: tools.PinScopeConnection, from: prevConnRoot} if changed { s.applyProjectConfig(root) // PLAN-398: shards seeded from the old connection pin follow the move, // so a seeded agent is not left refusing its next legitimate call off a // stale sticky root. - s.followConnectionShards(prevConnRoot) + out.followed = s.followConnectionShards(prevConnRoot) } s.clearDeclarationRefused(mcp.LogicalAgentFromCtx(ctx)) - return root, nil + return out, nil } // attributeConnectionPin records a pin that landed on the CONNECTION under the diff --git a/internal/cli/conn_repin_report.go b/internal/cli/conn_repin_report.go new file mode 100644 index 00000000..e2a2473a --- /dev/null +++ b/internal/cli/conn_repin_report.go @@ -0,0 +1,59 @@ +package cli + +// conn_repin_report.go — telling session_start WHICH pin a re-pin moved +// (issue #517). +// +// The daemon decides whether a re-pin moves the calling agent's own shard +// (repinShard: an identified caller on a shared connection) or the +// connection's pin (scope "connection", an anonymous caller, or a +// single-agent connection), and shards that never chose a root follow the +// connection (followConnectionShards). The tool used to receive only the +// previous root and render a neutral line, so an anonymous caller that +// dragged its peers with a connection move was never told, and an agent that +// moved the connection's pin while holding its own was shown its own root as +// "from" and the connection's new root as its workspace. + +import ( + "context" + + "github.com/plumbkit/plumb/internal/mcp" + "github.com/plumbkit/plumb/internal/tools" +) + +// repinOutcome is what one re-pin did: the requested folder's resolved root, +// which pin it moved, that pin's root before the call (equal to root when +// nothing moved), and the ids of the shards that followed a connection move. +type repinOutcome struct { + root string + scope tools.PinScope + from string + followed []string +} + +// repinReport renders a completed re-pin's outcome in session_start's terms. +// +// Followers excludes the caller: a scope "connection" move by an agent whose +// shard still sits where the connection seeded it drags that shard too, but +// the caller is not one of the OTHER agents the report counts. +// +// Effective is read AFTER the move, through the same per-call resolver every +// later tool uses (workspaceFor), under the caller's own ctx — not the +// identity-stripped one repinConnection moves the connection with. It is +// therefore what the caller's next relative path resolves against: its own pin +// when it holds one, the connection's otherwise. +func (s *connSession) repinReport(ctx context.Context, out repinOutcome) tools.RepinReport { + caller := mcp.LogicalAgentFromCtx(ctx) + followers := 0 + for _, id := range out.followed { + if id != caller { + followers++ + } + } + return tools.RepinReport{ + Root: out.root, + Scope: out.scope, + From: out.from, + Followers: followers, + Effective: s.workspaceFor(ctx), + } +} diff --git a/internal/cli/conn_repin_scope.go b/internal/cli/conn_repin_scope.go index 5e7730c6..0cdadfef 100644 --- a/internal/cli/conn_repin_scope.go +++ b/internal/cli/conn_repin_scope.go @@ -49,10 +49,10 @@ func connScopeAuthorised(ctx context.Context) bool { // The caller must be identified. The move resets every peer shard that has not // pinned a root of its own (followConnectionShards), including its read, write // and undo state, so it has to be attributable to the agent that asked for it. -func (s *connSession) repinConnection(ctx context.Context, folder, langOverride string, force bool) (string, error) { +func (s *connSession) repinConnection(ctx context.Context, folder, langOverride string, force bool) (repinOutcome, error) { id := mcp.LogicalAgentFromCtx(ctx) if id == "" && s.logicalAgents.sharedWith("") { - return "", toolerror.Wrap( + return repinOutcome{}, toolerror.Wrap( fmt.Errorf(`refusing a connection-scoped re-pin to %s: this connection serves several logical agents and this call carries no identity, so a move that resets every peer's workspace, read tracking and undo state cannot be attributed to the agent that asked for it. Identify yourself — pass session_start.session_id, or a per-call _meta[%s] — and retry with scope: "connection"`, folder, mcp.MetaLogicalAgentKey), toolerror.KindPinRefused, toolerror.ClassFixArguments, diff --git a/internal/cli/conn_restoredrift_test.go b/internal/cli/conn_restoredrift_test.go index cad890d7..1663dd98 100644 --- a/internal/cli/conn_restoredrift_test.go +++ b/internal/cli/conn_restoredrift_test.go @@ -71,7 +71,8 @@ func TestRepinRestore_SameRootStillAttaches(t *testing.T) { s := newPersistSession(t, store, ss, "proxyX") defer s.close() - root, err := s.repinWorkspaceFrom(context.Background(), sub, "", sessionstate.PinSourceSessionStart, pinTriggerRestore, false) + rootRep, err := s.repinWorkspaceFrom(context.Background(), sub, "", sessionstate.PinSourceSessionStart, pinTriggerRestore, false) + root := rootRep.root if err != nil { t.Fatalf("repinWorkspaceFrom: %v", err) } @@ -95,7 +96,8 @@ func TestRepinLive_StillResolvesSubdirToRoot(t *testing.T) { s := newPersistSession(t, store, ss, "proxyX") defer s.close() - root, err := s.repinWorkspaceFrom(context.Background(), sub, "", sessionstate.PinSourceSessionStart, pinTriggerLive, false) + rootRep, err := s.repinWorkspaceFrom(context.Background(), sub, "", sessionstate.PinSourceSessionStart, pinTriggerLive, false) + root := rootRep.root if err != nil { t.Fatalf("a live re-pin of a markerless subdirectory must still resolve to its enclosing root: %v", err) } diff --git a/internal/cli/conn_shard_pin_ownership_test.go b/internal/cli/conn_shard_pin_ownership_test.go index 8c5444d1..484b9187 100644 --- a/internal/cli/conn_shard_pin_ownership_test.go +++ b/internal/cli/conn_shard_pin_ownership_test.go @@ -98,7 +98,8 @@ func TestSeededShardDoesNotInheritAPeersStickiness(t *testing.T) { // The reporting agent's FIRST session_start, naming its own worktree. It // has never pinned anything on this connection. ctxAgent := mcp.WithLogicalAgent(context.Background(), "agent-A") - root, err := s.repinWorkspace(ctxAgent, worktree, "", false, false) + rootRep, err := s.repinWorkspace(ctxAgent, worktree, "", false, false) + root := rootRep.Root if err != nil { t.Fatalf("an agent's first explicit pin was refused off a root it never chose: %v", err) } @@ -172,7 +173,8 @@ func TestAgentPinSurvivesItsShardMaterialising(t *testing.T) { // rebuild): same proxy session, a fresh connection whose observed-identity // set starts empty, so this agent is once again the only identity known. second := newPersistSession(t, store, ss, "proxy-drift") - root, err := second.repinWorkspace(ctxAgent, worktree, "", false, false) + rootRep, err := second.repinWorkspace(ctxAgent, worktree, "", false, false) + root := rootRep.Root if err != nil { t.Fatalf("the agent's explicit session_start: %v", err) } diff --git a/internal/cli/conn_stickypin_test.go b/internal/cli/conn_stickypin_test.go index 5162c93d..406cb307 100644 --- a/internal/cli/conn_stickypin_test.go +++ b/internal/cli/conn_stickypin_test.go @@ -63,7 +63,8 @@ func TestStickyPin_ForceOverrides(t *testing.T) { t.Fatalf("first explicit pin: %v", err) } - root, err := s.repinWorkspace(context.Background(), rootB, "", true, false) + rootRep, err := s.repinWorkspace(context.Background(), rootB, "", true, false) + root := rootRep.Root if err != nil { t.Fatalf("force: true must override the sticky-pin guard: %v", err) } @@ -178,7 +179,8 @@ func TestStickyPin_SameRootAndSubdirNotRefused(t *testing.T) { if _, err := s.repinWorkspace(context.Background(), root, "", false, false); err != nil { t.Fatalf("same-root re-pin refused: %v", err) } - resolved, err := s.repinWorkspace(context.Background(), sub, "", false, false) + resolvedRep, err := s.repinWorkspace(context.Background(), sub, "", false, false) + resolved := resolvedRep.Root if err != nil { t.Fatalf("subdir re-pin refused: %v", err) } diff --git a/internal/cli/repin_test.go b/internal/cli/repin_test.go index f0595469..49fcde5a 100644 --- a/internal/cli/repin_test.go +++ b/internal/cli/repin_test.go @@ -229,7 +229,8 @@ func TestRepinWorkspace_SwitchesPinnedRoot(t *testing.T) { t.Fatalf("attach: workspace = %s, want %s", got, rootA) } - newRoot, err := s.repinWorkspace(context.Background(), rootB, "", false, false) + newRootRep, err := s.repinWorkspace(context.Background(), rootB, "", false, false) + newRoot := newRootRep.Root if err != nil { t.Fatalf("repin: %v", err) } @@ -241,7 +242,8 @@ func TestRepinWorkspace_SwitchesPinnedRoot(t *testing.T) { } // Re-pinning to the already-pinned root is a no-op (no error, same root). - again, err := s.repinWorkspace(context.Background(), rootB, "", false, false) + againRep, err := s.repinWorkspace(context.Background(), rootB, "", false, false) + again := againRep.Root if err != nil || again != rootB { t.Fatalf("no-op repin: returned %s, err %v; want %s, nil", again, err, rootB) } @@ -265,7 +267,8 @@ func TestRepinWorkspace_MarkerlessFolderBecomesWorkspace(t *testing.T) { defer s.close() s.attachWorkspace(context.Background(), "file://"+rootA) - newRoot, err := s.repinWorkspace(context.Background(), bare, "", false, false) + newRootRep, err := s.repinWorkspace(context.Background(), bare, "", false, false) + newRoot := newRootRep.Root if err != nil { t.Fatalf("repin to marker-less dir: %v", err) } diff --git a/internal/cli/routing_agent_boundary_test.go b/internal/cli/routing_agent_boundary_test.go index 9c0fd2b1..18e41aa4 100644 --- a/internal/cli/routing_agent_boundary_test.go +++ b/internal/cli/routing_agent_boundary_test.go @@ -66,7 +66,7 @@ func TestBoundaryGuards_AgentScopedGuardUsesTheAgentsPinNotTheConnections(t *tes // Commit one identity so the connection is shared and the agent gets a shard. s.recordLogicalAgentAttach("coordinator") agentCtx := mcp.WithLogicalAgent(ctx, "agent-A") - if _, refused := s.repinAgent(agentCtx, agentRoot, "", sessionstate.PinSourceSessionStart, true); refused != nil { + if _, _, refused := s.repinAgent(agentCtx, agentRoot, "", sessionstate.PinSourceSessionStart, true); refused != nil { t.Fatalf("per-agent re-pin refused: %v", refused) } @@ -94,7 +94,7 @@ func pinSharedConnection(t *testing.T, s *connSession, connRoot, agentRoot strin // Commit one identity so the connection is shared and the agent gets a shard. s.recordLogicalAgentAttach("coordinator") agentCtx := mcp.WithLogicalAgent(ctx, "agent-A") - if _, refused := s.repinAgent(agentCtx, agentRoot, "", sessionstate.PinSourceSessionStart, true); refused != nil { + if _, _, refused := s.repinAgent(agentCtx, agentRoot, "", sessionstate.PinSourceSessionStart, true); refused != nil { t.Fatalf("per-agent re-pin refused: %v", refused) } return agentCtx diff --git a/internal/cli/session_start_repin_report_test.go b/internal/cli/session_start_repin_report_test.go new file mode 100644 index 00000000..6dc0ce7b --- /dev/null +++ b/internal/cli/session_start_repin_report_test.go @@ -0,0 +1,271 @@ +package cli + +// session_start_repin_report_test.go — issue #517: session_start's re-pin +// output says WHICH pin moved (the caller's own, or the connection's and how +// many other agents followed it), what the caller's next relative path +// resolves against, and names the caller's own root in the header. +// +// Every case drives the real tool surface (newSessionStartTool: the +// tools.SessionStart wired to this connSession's resolver and re-pin callback) +// because the defect was the tool guessing from its own view what the daemon +// had decided. Each case runs through both the full and the brief packet. + +import ( + "context" + "encoding/json" + "strings" + "testing" + + "github.com/plumbkit/plumb/internal/config" + "github.com/plumbkit/plumb/internal/mcp" +) + +func newRepinReportSession(t *testing.T) *connSession { + t.Helper() + t.Setenv("XDG_DATA_HOME", t.TempDir()) + s := newConnSession(context.Background(), detectTestPool(), nil, config.NewStore(config.Defaults()), nil, nil, newSharedBudgets()) + t.Cleanup(s.close) + return s +} + +func repinReportRoots(t *testing.T, n int) []string { + t.Helper() + roots := make([]string, n) + for i := range roots { + roots[i] = freshTempDir(t) + mustGitDir(t, roots[i]) + } + return roots +} + +// runRepinReport executes session_start through the real wiring with args +// plus the given detail, failing the test on an error. +func runRepinReport(t *testing.T, s *connSession, ctx context.Context, args map[string]any, detail string) string { + t.Helper() + full := map[string]any{"detail": detail} + for k, v := range args { + full[k] = v + } + raw, err := json.Marshal(full) + if err != nil { + t.Fatalf("marshal: %v", err) + } + out, err := newSessionStartTool(s).Execute(ctx, raw) + if err != nil { + t.Fatalf("session_start %s: %v", raw, err) + } + return out +} + +func wantLines(t *testing.T, out string, want ...string) { + t.Helper() + for _, w := range want { + if !strings.Contains(out, w) { + t.Errorf("output lacks %q\n%s", w, out) + } + } +} + +func refuseLines(t *testing.T, out string, refuse ...string) { + t.Helper() + for _, r := range refuse { + if strings.Contains(out, r) { + t.Errorf("output must not contain %q\n%s", r, out) + } + } +} + +// An identified agent on a shared connection moves ITS OWN pin: the report +// says so, and the connection's pin is left exactly where it was. +func TestSessionStartRepinReport_AgentScopeOnSharedConnection(t *testing.T) { + for _, detail := range []string{"full", "brief"} { + t.Run(detail, func(t *testing.T) { + s := newRepinReportSession(t) + r := repinReportRoots(t, 2) + rootX, rootY := r[0], r[1] + if _, err := s.repinWorkspace(context.Background(), rootX, "", false, false); err != nil { + t.Fatalf("connection pin to X: %v", err) + } + s.recordLogicalAgentAttach("coordinator") + s.recordLogicalAgentAttach("sub") + ctxSub := mcp.WithLogicalAgent(context.Background(), "sub") + + out := runRepinReport(t, s, ctxSub, map[string]any{"workspace": rootY, "force": true}, detail) + + wantLines(t, out, + "# Workspace: "+rootY+"\n", + "Re-pinned your pin: "+rootX+" → "+rootY+"\n", + "Next relative-path call resolves against: "+rootY+"\n", + ) + refuseLines(t, out, "this connection's pin", "your own pin, not the connection's") + if got := s.workspace(); got != rootX { + t.Errorf("connection pin = %q, want it unchanged at %q", got, rootX) + } + if got := s.workspaceFor(ctxSub); got != rootY { + t.Errorf("sub resolves to %q, want %q", got, rootY) + } + }) + } +} + +// An anonymous default-scope re-pin on a shared connection moves the +// CONNECTION's pin, and every seeded peer shard follows it. The caller must be +// told that other agents moved with it. +func TestSessionStartRepinReport_AnonymousDefaultScopeMovesFollowers(t *testing.T) { + for _, detail := range []string{"full", "brief"} { + t.Run(detail, func(t *testing.T) { + s := newRepinReportSession(t) + r := repinReportRoots(t, 2) + rootX, rootY := r[0], r[1] + // A roots-origin pin is not sticky, so an unforced anonymous + // session_start may move it — the path case 1 of the issue takes. + s.attachWorkspace(context.Background(), "file://"+rootX) + if got := s.workspace(); got != rootX { + t.Fatalf("precondition: roots attach = %q, want %q", got, rootX) + } + s.recordLogicalAgentAttach("a") + s.recordLogicalAgentAttach("b") + ctxA := mcp.WithLogicalAgent(context.Background(), "a") + ctxB := mcp.WithLogicalAgent(context.Background(), "b") + // Seed both peers' shards where the connection sits. + for _, ctx := range []context.Context{ctxA, ctxB} { + if got := s.workspaceFor(ctx); got != rootX { + t.Fatalf("precondition: seeded shard at %q, want %q", got, rootX) + } + } + + out := runRepinReport(t, s, context.Background(), map[string]any{"workspace": rootY}, detail) + + wantLines(t, out, + "# Workspace: "+rootY+"\n", + "Re-pinned this connection's pin: "+rootX+" → "+rootY+" (2 other agents follow it)\n", + "Next relative-path call resolves against: "+rootY+"\n", + ) + refuseLines(t, out, "Re-pinned your pin", "your own pin, not the connection's") + // The count is the truth, not a label: both peers really moved. + for id, ctx := range map[string]context.Context{"a": ctxA, "b": ctxB} { + if got := s.workspaceFor(ctx); got != rootY { + t.Errorf("peer %s resolves to %q, want it to have followed to %q", id, got, rootY) + } + } + }) + } +} + +// scope: "connection" from an agent holding a pin of its own moves the +// connection's pin but NOT the agent. The report must name the connection's +// previous root (not the agent's), and both the header and the next-call line +// must name the agent's own root, which is where its relative paths still go. +func TestSessionStartRepinReport_ConnectionScopeFromAgentWithOwnPin(t *testing.T) { + for _, detail := range []string{"full", "brief"} { + t.Run(detail, func(t *testing.T) { + s := newRepinReportSession(t) + r := repinReportRoots(t, 3) + rootX, rootW, rootZ := r[0], r[1], r[2] + if _, err := s.repinWorkspace(context.Background(), rootX, "", false, false); err != nil { + t.Fatalf("connection pin to X: %v", err) + } + s.recordLogicalAgentAttach("own") + s.recordLogicalAgentAttach("peer") + ctxOwn := mcp.WithLogicalAgent(context.Background(), "own") + ctxPeer := mcp.WithLogicalAgent(context.Background(), "peer") + if _, err := s.repinWorkspace(ctxOwn, rootW, "", true, false); err != nil { + t.Fatalf("own's pin to W: %v", err) + } + if got := s.workspaceFor(ctxPeer); got != rootX { + t.Fatalf("precondition: peer seeded at %q, want %q", got, rootX) + } + + out := runRepinReport(t, s, ctxOwn, map[string]any{"workspace": rootZ, "scope": "connection", "force": true}, detail) + + wantLines(t, out, + "# Workspace: "+rootW+"\n", + "Re-pinned this connection's pin: "+rootX+" → "+rootZ+" (1 other agent follows it)\n", + "Next relative-path call resolves against: "+rootW+" (your own pin, not the connection's)\n", + ) + refuseLines(t, out, "# Workspace: "+rootZ, "Re-pinned your pin", rootW+" → ") + if got := s.workspace(); got != rootZ { + t.Errorf("connection pin = %q, want %q", got, rootZ) + } + if got := s.workspaceFor(ctxOwn); got != rootW { + t.Errorf("own resolves to %q, want its own pin %q", got, rootW) + } + if got := s.workspaceFor(ctxPeer); got != rootZ { + t.Errorf("peer resolves to %q, want it to have followed to %q", got, rootZ) + } + }) + } +} + +// scope: "connection" from an agent that never chose a root of its own: its +// shard sits where the connection seeded it, so it follows the move along with +// its peer. It is the caller, not one of the OTHER agents the count reports, +// and it resolves against the connection's new root. +func TestSessionStartRepinReport_ConnectionScopeFromSeededAgent(t *testing.T) { + for _, detail := range []string{"full", "brief"} { + t.Run(detail, func(t *testing.T) { + s := newRepinReportSession(t) + r := repinReportRoots(t, 2) + rootX, rootZ := r[0], r[1] + if _, err := s.repinWorkspace(context.Background(), rootX, "", false, false); err != nil { + t.Fatalf("connection pin to X: %v", err) + } + s.recordLogicalAgentAttach("seeded") + s.recordLogicalAgentAttach("peer") + ctxSeeded := mcp.WithLogicalAgent(context.Background(), "seeded") + ctxPeer := mcp.WithLogicalAgent(context.Background(), "peer") + for _, ctx := range []context.Context{ctxSeeded, ctxPeer} { + if got := s.workspaceFor(ctx); got != rootX { + t.Fatalf("precondition: seeded shard at %q, want %q", got, rootX) + } + } + + out := runRepinReport(t, s, ctxSeeded, map[string]any{"workspace": rootZ, "scope": "connection", "force": true}, detail) + + wantLines(t, out, + "# Workspace: "+rootZ+"\n", + "Re-pinned this connection's pin: "+rootX+" → "+rootZ+" (1 other agent follows it)\n", + "Next relative-path call resolves against: "+rootZ+"\n", + ) + refuseLines(t, out, "your own pin, not the connection's", "Re-pinned your pin") + for id, ctx := range map[string]context.Context{"seeded": ctxSeeded, "peer": ctxPeer} { + if got := s.workspaceFor(ctx); got != rootZ { + t.Errorf("%s resolves to %q, want it to have followed to %q", id, got, rootZ) + } + } + }) + } +} + +// On a single-agent connection the connection IS the agent: even an +// identified caller moves the connection's pin, and nobody follows it. +func TestSessionStartRepinReport_SingleAgentConnection(t *testing.T) { + for _, detail := range []string{"full", "brief"} { + t.Run(detail, func(t *testing.T) { + s := newRepinReportSession(t) + r := repinReportRoots(t, 2) + rootX, rootY := r[0], r[1] + ctxSolo := mcp.WithLogicalAgent(context.Background(), "solo") + if _, err := s.repinWorkspace(ctxSolo, rootX, "", false, false); err != nil { + t.Fatalf("first pin: %v", err) + } + + out := runRepinReport(t, s, ctxSolo, map[string]any{"workspace": rootY, "force": true}, detail) + + wantLines(t, out, + "# Workspace: "+rootY+"\n", + "Re-pinned this connection's pin: "+rootX+" → "+rootY+" (no other agent follows it)\n", + "Next relative-path call resolves against: "+rootY+"\n", + ) + refuseLines(t, out, "Re-pinned your pin") + if got := s.workspace(); got != rootY { + t.Errorf("connection pin = %q, want %q", got, rootY) + } + + // Absence control for the banner: naming the root already held + // moves nothing, so no "Re-pinned" line at all. + again := runRepinReport(t, s, ctxSolo, map[string]any{"workspace": rootY}, detail) + refuseLines(t, again, "Re-pinned") + }) + } +} diff --git a/internal/tools/session_start.go b/internal/tools/session_start.go index 12a5b5aa..0e52b82e 100644 --- a/internal/tools/session_start.go +++ b/internal/tools/session_start.go @@ -91,38 +91,38 @@ type RootsResolver func(ctx context.Context) string // — always Detect-validated and never persisted as the sticky pin. type SessionStart struct { ws WorkspaceFn - diag diagnosticsSource // may be nil; diagnostics section skipped when nil - roots RootsResolver // may be nil; roots/list fallback skipped when nil - refuseFn func() bool // may be nil; treated as false (no refusal) - clientNameFn func() string // may be nil; returns current MCP client name - topo topologyStoreFn // may be nil; returns the live topology store, or nil when disabled - gitPolicyFn func() GitPolicy // may be nil; git policy section skipped when nil - projectGitFn func() ProjectGitStatus // may be nil; this session's captured view of the capability-granting keys its project config sets - lspLangFn func() string // may be nil; the LSP language attached to this session ("" when none) - lspSkipNoteFn func() string // may be nil; names why no LSP is attached when the skip is deliberate (e.g. a home-directory workspace root) - pinProvFn func() PinProvenance // may be nil; this connection's pin provenance, for the contested-connection note - lspLangsFn func() []string // may be nil; the distinct child languages of a monorepo root (>1 ⇒ multi-language identity line) - lspRoutedFn func() []string // may be nil; non-primary languages whose servers have actually served this session - externalIDFn func(id string) string // may be nil; links session to external ID, returns inherited name - linkageStateFn func() LinkageState // may be nil; the connection's PERSISTED linkage + recovery outcome, for state-true linkage notes - resumedNewIDFn func() bool // may be nil; this call resumed a predecessor's NAME under a new internal session ID - stampChannelFn func(ctx context.Context) StampChannelState // may be nil; whether THIS call carried a per-call logical-agent identity, and whether the gate is already armed - declaredAgent func(ctx context.Context, id string) context.Context // may be nil; derives the per-call ctx carrying the logical-agent identity declared by session_id - pinConflict func(requested string) // may be nil; records a same-connection workspace switch attempt - repin func(ctx context.Context, workspace, language string, force, connectionScope bool) (string, error) // may be nil; re-pins the connection to an explicit workspace, optionally forcing a primary language; force overrides the sticky-pin guard - episodicFn func(ws string) (string, bool) // may be nil; returns the last episodic summary for the workspace - toolProfile func() (profile string, hidden int, reason string) // may be nil; the resolved tool profile, count of tools hidden from tools/list, and the resolution reason - lspWarmingFn func() (bool, time.Duration) // may be nil; reports whether the primary LSP is still warming + elapsed - lspDiagModeFn func() string // may be nil; the resolved diagnostics mode of the primary LSP ("" when unresolved) - lspGoWorkFn func() string // may be nil; the go.work the primary LSP was started with GOWORK=off against ("" when none) - purposeFn func(purpose string) // may be nil; persists a validated session purpose tag - selfSessID func() string // this session's ID, excluded from the peer digest and shown as the caller's own - selfName func() string // may be nil; this session's own current name, shown as the caller's own - collabFn func() (peerAwareness bool, hintBudgetBytes int) // may be nil; the resolved [collab] snapshot for the peer digest - mailboxFn func() (on bool, inbox Inbox) // may be nil; the mailbox delivery snapshot - xcodeHintFn XcodeHintFn // may be nil; bare-Xcode BSP guidance - tasksFn func() TaskState // may be nil; the resolved run_task/run_command state for this workspace - surchargeFn func() (bytes int, tokens int, toolCount int) // may be nil; the per-request tool-schema surcharge (measured bytes + derived token estimate) for the tools THIS connection actually advertises + diag diagnosticsSource // may be nil; diagnostics section skipped when nil + roots RootsResolver // may be nil; roots/list fallback skipped when nil + refuseFn func() bool // may be nil; treated as false (no refusal) + clientNameFn func() string // may be nil; returns current MCP client name + topo topologyStoreFn // may be nil; returns the live topology store, or nil when disabled + gitPolicyFn func() GitPolicy // may be nil; git policy section skipped when nil + projectGitFn func() ProjectGitStatus // may be nil; this session's captured view of the capability-granting keys its project config sets + lspLangFn func() string // may be nil; the LSP language attached to this session ("" when none) + lspSkipNoteFn func() string // may be nil; names why no LSP is attached when the skip is deliberate (e.g. a home-directory workspace root) + pinProvFn func() PinProvenance // may be nil; this connection's pin provenance, for the contested-connection note + lspLangsFn func() []string // may be nil; the distinct child languages of a monorepo root (>1 ⇒ multi-language identity line) + lspRoutedFn func() []string // may be nil; non-primary languages whose servers have actually served this session + externalIDFn func(id string) string // may be nil; links session to external ID, returns inherited name + linkageStateFn func() LinkageState // may be nil; the connection's PERSISTED linkage + recovery outcome, for state-true linkage notes + resumedNewIDFn func() bool // may be nil; this call resumed a predecessor's NAME under a new internal session ID + stampChannelFn func(ctx context.Context) StampChannelState // may be nil; whether THIS call carried a per-call logical-agent identity, and whether the gate is already armed + declaredAgent func(ctx context.Context, id string) context.Context // may be nil; derives the per-call ctx carrying the logical-agent identity declared by session_id + pinConflict func(requested string) // may be nil; records a same-connection workspace switch attempt + repin func(ctx context.Context, workspace, language string, force, connectionScope bool) (RepinReport, error) // may be nil; re-pins to an explicit workspace, optionally forcing a primary language, and reports which pin moved; force overrides the sticky-pin guard + episodicFn func(ws string) (string, bool) // may be nil; returns the last episodic summary for the workspace + toolProfile func() (profile string, hidden int, reason string) // may be nil; the resolved tool profile, count of tools hidden from tools/list, and the resolution reason + lspWarmingFn func() (bool, time.Duration) // may be nil; reports whether the primary LSP is still warming + elapsed + lspDiagModeFn func() string // may be nil; the resolved diagnostics mode of the primary LSP ("" when unresolved) + lspGoWorkFn func() string // may be nil; the go.work the primary LSP was started with GOWORK=off against ("" when none) + purposeFn func(purpose string) // may be nil; persists a validated session purpose tag + selfSessID func() string // this session's ID, excluded from the peer digest and shown as the caller's own + selfName func() string // may be nil; this session's own current name, shown as the caller's own + collabFn func() (peerAwareness bool, hintBudgetBytes int) // may be nil; the resolved [collab] snapshot for the peer digest + mailboxFn func() (on bool, inbox Inbox) // may be nil; the mailbox delivery snapshot + xcodeHintFn XcodeHintFn // may be nil; bare-Xcode BSP guidance + tasksFn func() TaskState // may be nil; the resolved run_task/run_command state for this workspace + surchargeFn func() (bytes int, tokens int, toolCount int) // may be nil; the per-request tool-schema surcharge (measured bytes + derived token estimate) for the tools THIS connection actually advertises } // WithProjectPolicy wires the accessor for this session's capability-granting @@ -309,10 +309,12 @@ func (t *SessionStart) WithPinConflict(fn func(requested string)) *SessionStart // WithRepin wires the deliberate workspace-switch callback. When the connection // is already pinned and the caller passes an explicit `workspace` that differs, // session_start re-pins the connection to it (via fn) instead of refusing. fn -// returns the resolved root. Nil-safe: with no callback wired, session_start -// falls back to the historical "start a new connection" refusal. Returns the -// receiver for chaining. -func (t *SessionStart) WithRepin(fn func(ctx context.Context, workspace, language string, force, connectionScope bool) (string, error)) *SessionStart { +// reports the resolved root, which pin moved (the caller's own or the +// connection's), that pin's previous root, how many other agents followed it, +// and the root the caller now resolves against (issue #517). Nil-safe: with no +// callback wired, session_start falls back to the historical "start a new +// connection" refusal. Returns the receiver for chaining. +func (t *SessionStart) WithRepin(fn func(ctx context.Context, workspace, language string, force, connectionScope bool) (RepinReport, error)) *SessionStart { t.repin = fn return t } @@ -377,8 +379,7 @@ func (t *SessionStart) Execute(ctx context.Context, raw json.RawMessage) (string // exactly when it is being dropped (see session_start_stamp.go). perCallCtx := ctx ctx = t.withDeclaredAgent(ctx, raw) - ws, repinnedFrom, err := t.resolveSessionWorkspace(ctx, raw) - repinLine := repinAnnouncement(repinnedFrom, ws) + ws, repinLine, err := t.resolveSessionWorkspace(ctx, raw) if err != nil { return "", err } diff --git a/internal/tools/session_start_brief.go b/internal/tools/session_start_brief.go index 9dabdf00..60b6be30 100644 --- a/internal/tools/session_start_brief.go +++ b/internal/tools/session_start_brief.go @@ -66,7 +66,7 @@ func resolveDetail(raw json.RawMessage, autoBrief bool) (string, error) { // grows a second "sections" knob to pick among them. // // Three signals are NOT optional, even though the rest of the identity block -// (writeSessionIdentity) is full-only: inheritedName and repinnedFrom, plus +// (writeSessionIdentity) is full-only: inheritedName and repinLine, plus // the wired lspSkipNoteFn, are exactly the loud, one-shot announcements full // always carries — the PR #181 re-pin guarantee, the #316 why-no-server note, // and the resumed session's own peer-addressable name (how a woken agent @@ -77,12 +77,13 @@ func resolveDetail(raw json.RawMessage, autoBrief bool) (string, error) { // woken session must still see its pending mail, or the wake flow loses its // point; it is nil-safe and a no-op when mailbox delivery is unwired or empty, // so it costs nothing when there is nothing to deliver. -func (t *SessionStart) executeBrief(ws, lang, inheritedName, repinnedFrom string, linked bool, stampNote string, claimable bool) string { +func (t *SessionStart) executeBrief(ws, lang, inheritedName, repinLine string, linked bool, stampNote string, claimable bool) string { var sb strings.Builder fmt.Fprintf(&sb, "# Workspace: %s\n\n", ws) - if repinnedFrom != "" { - sb.WriteString(repinnedFrom) - } + // The same re-pin block the full packet renders: which pin moved, how many + // other agents followed it, and what this caller's next relative path + // resolves against (issue #517). + sb.WriteString(repinLine) if lang != "" { fmt.Fprintf(&sb, "Language: %s\n", lang) } diff --git a/internal/tools/session_start_brief_test.go b/internal/tools/session_start_brief_test.go index f1f2f826..8e960706 100644 --- a/internal/tools/session_start_brief_test.go +++ b/internal/tools/session_start_brief_test.go @@ -181,7 +181,9 @@ func TestSessionStartBrief_CarriesIdentitySignals(t *testing.T) { const skipNote = "LSP skipped: the workspace root is the home directory" tool := NewSessionStart(func(context.Context) string { return attached }, nil, nil, nil, func() string { return "" }, nil). - WithRepin(func(_ context.Context, ws, _ string, _, _ bool) (string, error) { return ws, nil }). + WithRepin(func(_ context.Context, ws, _ string, _, _ bool) (RepinReport, error) { + return RepinReport{Root: ws, Scope: PinScopeConnection, From: attached}, nil + }). WithExternalID(func(string) string { return "resumed-session" }). WithLSPSkipNote(func() string { return skipNote }) @@ -195,7 +197,7 @@ func TestSessionStartBrief_CarriesIdentitySignals(t *testing.T) { if !strings.Contains(out, briefOrientationFooter) { t.Fatalf("expected this call to auto-brief, got:\n%s", out) } - if want := "Re-pinned: " + attached + " → " + target; !strings.Contains(out, want) { + if want := "Re-pinned this connection's pin: " + attached + " → " + target; !strings.Contains(out, want) { t.Errorf("brief must carry the re-pin announcement %q, got:\n%s", want, out) } if !strings.Contains(out, "Session: resumed-session (resumed)") { @@ -273,13 +275,44 @@ func TestSessionStartBrief_JoinNamesCap(t *testing.T) { } } -// TestRepinAnnouncement: the line names no pin, because which one moved is the -// daemon's decision; it only reports the move itself. +// TestRepinAnnouncement pins the rendered block for every pin the daemon can +// report moving (issue #517): the caller's own, or the connection's with its +// follower count, plus the line naming what the caller's next relative path +// resolves against — its own pin when that is not the connection's new root. func TestRepinAnnouncement(t *testing.T) { - if got := repinAnnouncement("", "/b"); got != "" { - t.Errorf("nothing moved, got %q", got) - } - if got := repinAnnouncement("/a", "/b"); got != "Re-pinned: /a → /b\n\n" { - t.Errorf("got %q", got) + cases := []struct { + name string + rep RepinReport + want string + }{ + {"first pin, nothing moved", RepinReport{Root: "/b", Scope: PinScopeConnection}, ""}, + {"same root, nothing moved", RepinReport{Root: "/b", Scope: PinScopeAgent, From: "/b"}, ""}, + { + "agent pin", + RepinReport{Root: "/b", Scope: PinScopeAgent, From: "/a", Effective: "/b"}, + "Re-pinned your pin: /a → /b\nNext relative-path call resolves against: /b\n\n", + }, + { + "connection pin, single agent", + RepinReport{Root: "/b", Scope: PinScopeConnection, From: "/a"}, + "Re-pinned this connection's pin: /a → /b (no other agent follows it)\nNext relative-path call resolves against: /b\n\n", + }, + { + "connection pin, one follower", + RepinReport{Root: "/b", Scope: PinScopeConnection, From: "/a", Followers: 1, Effective: "/b"}, + "Re-pinned this connection's pin: /a → /b (1 other agent follows it)\nNext relative-path call resolves against: /b\n\n", + }, + { + "connection pin moved by an agent holding its own", + RepinReport{Root: "/b", Scope: PinScopeConnection, From: "/a", Followers: 2, Effective: "/own"}, + "Re-pinned this connection's pin: /a → /b (2 other agents follow it)\nNext relative-path call resolves against: /own (your own pin, not the connection's)\n\n", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := repinAnnouncement(tc.rep); got != tc.want { + t.Errorf("got %q\nwant %q", got, tc.want) + } + }) } } diff --git a/internal/tools/session_start_declared_agent_test.go b/internal/tools/session_start_declared_agent_test.go index 1b75f2f8..96358254 100644 --- a/internal/tools/session_start_declared_agent_test.go +++ b/internal/tools/session_start_declared_agent_test.go @@ -31,12 +31,12 @@ func TestSessionStart_AttributionPrecedesTheWorkspace(t *testing.T) { order = append(order, "declared-agent") return context.WithValue(ctx, declaredAgentKeyType{}, id) }). - WithRepin(func(ctx context.Context, workspace, _ string, _, _ bool) (string, error) { + WithRepin(func(ctx context.Context, workspace, _ string, _, _ bool) (RepinReport, error) { order = append(order, "repin") if got, _ := ctx.Value(declaredAgentKeyType{}).(string); got != "subagent-7" { t.Errorf("re-pin ctx logical agent = %q, want %q — the re-pin ran unattributed", got, "subagent-7") } - return workspace, nil + return RepinReport{Root: workspace, Scope: PinScopeAgent, From: ws}, nil }) if _, err := tool.Execute(context.Background(), json.RawMessage(`{"workspace":"`+ws+`","session_id":"subagent-7"}`)); err != nil { @@ -74,8 +74,8 @@ func TestSessionStart_LinkageNotCommittedOnARefusedCall(t *testing.T) { attributed = true return context.WithValue(ctx, declaredAgentKeyType{}, id) }). - WithRepin(func(context.Context, string, string, bool, bool) (string, error) { - return "", errors.New("refusing to re-pin: sticky (issue #182)") + WithRepin(func(context.Context, string, string, bool, bool) (RepinReport, error) { + return RepinReport{}, errors.New("refusing to re-pin: sticky (issue #182)") }) _, err := tool.Execute(context.Background(), json.RawMessage(`{"workspace":"`+t.TempDir()+`","session_id":"drifter"}`)) diff --git a/internal/tools/session_start_sections.go b/internal/tools/session_start_sections.go index 34462630..247fd81d 100644 --- a/internal/tools/session_start_sections.go +++ b/internal/tools/session_start_sections.go @@ -14,12 +14,13 @@ import ( "github.com/plumbkit/plumb/internal/memory" ) -// repinnedFrom is the rendered re-pin line (repinAnnouncement), or "". -func (t *SessionStart) writeSessionIdentity(sb *strings.Builder, ws, lang, inheritedName, repinnedFrom string, linked bool, stampNote string) { +// ws is the root the CALLER resolves against, which after a scope: +// "connection" re-pin by an agent holding its own pin is not the connection's +// new root (issue #517). repinLine is the rendered re-pin block +// (repinAnnouncement), or "". +func (t *SessionStart) writeSessionIdentity(sb *strings.Builder, ws, lang, inheritedName, repinLine string, linked bool, stampNote string) { fmt.Fprintf(sb, "# Workspace: %s\n\n", ws) - if repinnedFrom != "" { - sb.WriteString(repinnedFrom) - } + sb.WriteString(repinLine) if lang != "" { fmt.Fprintf(sb, "Language: %s\n", lang) } diff --git a/internal/tools/session_start_test.go b/internal/tools/session_start_test.go index c5347cb9..299a3cb0 100644 --- a/internal/tools/session_start_test.go +++ b/internal/tools/session_start_test.go @@ -398,9 +398,9 @@ func TestSessionStart_LanguageOverride(t *testing.T) { var gotWs, gotLang string tool := NewSessionStart(func(context.Context) string { return attached }, nil, nil, nil, func() string { return "" }, nil). WithLSPLanguage(func() string { return "swift" }). // server attached after the forced pin - WithRepin(func(_ context.Context, ws, lang string, _, _ bool) (string, error) { + WithRepin(func(_ context.Context, ws, lang string, _, _ bool) (RepinReport, error) { gotWs, gotLang = ws, lang - return ws, nil + return RepinReport{Root: ws, Scope: PinScopeConnection, From: ws}, nil }) out, err := tool.Execute(context.Background(), json.RawMessage(`{"language":"swift"}`)) if err != nil { @@ -447,9 +447,9 @@ func TestSessionStart_WorkspaceResolution(t *testing.T) { target := t.TempDir() var got string tool := NewSessionStart(func(context.Context) string { return attached }, nil, nil, nil, func() string { return "" }, nil). - WithRepin(func(_ context.Context, ws, _ string, _, _ bool) (string, error) { + WithRepin(func(_ context.Context, ws, _ string, _, _ bool) (RepinReport, error) { got = ws - return ws, nil + return RepinReport{Root: ws, Scope: PinScopeConnection, From: attached}, nil }) out, err := tool.Execute(context.Background(), json.RawMessage(`{"workspace":"`+target+`"}`)) if err != nil { @@ -461,7 +461,7 @@ func TestSessionStart_WorkspaceResolution(t *testing.T) { if !strings.Contains(out, "# Workspace: "+target) { t.Errorf("output should show the new workspace %q\n%s", target, out) } - if !strings.Contains(out, "Re-pinned: "+attached+" → "+target) { + if !strings.Contains(out, "Re-pinned this connection's pin: "+attached+" → "+target) { t.Errorf("output should announce the re-pin\n%s", out) } }) @@ -809,9 +809,9 @@ func TestSessionStart_ForceThreadedToRepin(t *testing.T) { target := t.TempDir() var gotForce []bool tool := NewSessionStart(func(context.Context) string { return attached }, nil, nil, nil, func() string { return "" }, nil). - WithRepin(func(_ context.Context, ws, _ string, force, _ bool) (string, error) { + WithRepin(func(_ context.Context, ws, _ string, force, _ bool) (RepinReport, error) { gotForce = append(gotForce, force) - return ws, nil + return RepinReport{Root: ws, Scope: PinScopeConnection, From: attached}, nil }) if _, err := tool.Execute(context.Background(), json.RawMessage(`{"workspace":"`+target+`"}`)); err != nil { t.Fatalf("Execute without force: %v", err) @@ -832,9 +832,9 @@ func TestSessionStart_ForceThreadedOnUnattachedLanguagePin(t *testing.T) { target := t.TempDir() var gotForce bool tool := NewSessionStart(func(context.Context) string { return "" }, nil, nil, nil, func() string { return "" }, nil). - WithRepin(func(_ context.Context, _, _ string, force, _ bool) (string, error) { + WithRepin(func(_ context.Context, _, _ string, force, _ bool) (RepinReport, error) { gotForce = force - return target, nil + return RepinReport{Root: target, Scope: PinScopeConnection}, nil }) raw := json.RawMessage(`{"workspace":"` + target + `","language":"go","force":true}`) if _, err := tool.Execute(context.Background(), raw); err != nil { diff --git a/internal/tools/session_start_workspace.go b/internal/tools/session_start_workspace.go index befb7e8b..e4410a90 100644 --- a/internal/tools/session_start_workspace.go +++ b/internal/tools/session_start_workspace.go @@ -13,10 +13,55 @@ import ( "fmt" ) -// resolveSessionWorkspace resolves the workspace for this call. repinnedFrom is -// the previous root when an explicit `workspace` argument switched an -// already-pinned connection to a different project; it is empty otherwise. -func (t *SessionStart) resolveSessionWorkspace(ctx context.Context, raw json.RawMessage) (ws string, repinnedFrom string, err error) { +// PinScope names which pin a session_start re-pin moved: the calling agent's own +// per-agent pin, or the connection's pin, which every agent on the connection +// that has not chosen a root of its own follows. +type PinScope string + +const ( + // PinScopeAgent is the caller's own per-agent pin on a shared connection. + PinScopeAgent PinScope = "agent" + // PinScopeConnection is the connection's pin: an anonymous caller's, a + // single-agent connection's, or the one scope: "connection" moves. + PinScopeConnection PinScope = "connection" +) + +// RepinReport is what the re-pin callback reports back. Which pin moves is +// decided in the daemon, from facts the tool cannot see (whether the +// connection is shared, whether the caller is identified, whether it holds a +// pin of its own), so the daemon reports it rather than the tool guessing from +// the call's arguments — a guess was wrong for an anonymous caller on a shared +// connection (issue #517). +type RepinReport struct { + // Root is the requested folder's resolved root. + Root string + // Scope is the pin the re-pin moved (or, on a no-op, would have moved). + Scope PinScope + // From is that pin's root before the call. Equal to Root when nothing moved. + From string + // Followers counts the OTHER agents whose pins followed the connection's + // when it moved. Always 0 for PinScopeAgent. + Followers int + // Effective is the root the caller's next unqualified (relative-path) call + // resolves against. It differs from Root when an agent with a pin of its own + // moves the connection's pin: the agent itself stays where it was. Empty + // means Root. + Effective string +} + +// effectiveRoot is the root the caller resolves against after the re-pin. +func (r RepinReport) effectiveRoot() string { + if r.Effective != "" { + return r.Effective + } + return r.Root +} + +// resolveSessionWorkspace resolves the workspace for this call: the root the +// CALLER resolves against, which is what the packet's header names. repinLine +// is the rendered re-pin announcement when an explicit `workspace` argument +// moved a pin to a different project, and "" otherwise. +func (t *SessionStart) resolveSessionWorkspace(ctx context.Context, raw json.RawMessage) (ws string, repinLine string, err error) { var a struct { Workspace string `json:"workspace"` Language string `json:"language"` @@ -74,14 +119,14 @@ func (t *SessionStart) resolveUnattachedWorkspace(ctx context.Context, workspace if t.repin == nil { return workspace, nil } - root, err := t.repin(ctx, workspace, language, force, connScope) + rep, err := t.repin(ctx, workspace, language, force, connScope) if err != nil { if language != "" { return "", fmt.Errorf("session_start: pinning %s as %s: %w", workspace, language, err) } return "", fmt.Errorf("session_start: pinning %s: %w", workspace, err) } - return root, nil + return rep.effectiveRoot(), nil } // resolveAttached handles session_start on an already-attached connection: an @@ -143,18 +188,11 @@ func (t *SessionStart) repinExplicit(ctx context.Context, current, requested, la current, requested, ) } - newRoot, err := t.repin(ctx, requested, language, force, connScope) + rep, err := t.repin(ctx, requested, language, force, connScope) if err != nil { return "", "", fmt.Errorf("session_start: re-pinning to %s: %w", requested, err) } - // Suppress the "re-pinned" banner when the requested path resolves to the - // same root (e.g. a subdir of the current project, or a language-only pin): - // no project switch actually happened. - from := current - if sameDir(newRoot, current) { - from = "" - } - return newRoot, from, nil + return rep.effectiveRoot(), repinAnnouncement(rep), nil } // forceLanguage re-pins the connection's CURRENT workspace to a forced primary @@ -171,13 +209,44 @@ func (t *SessionStart) forceLanguage(ctx context.Context, current, language stri return current, "", nil } -// repinAnnouncement renders the re-pin line, or "" when nothing moved. It -// deliberately names no pin: which one moved (the caller's shard or the -// connection's) is decided in the daemon, not visible here, and a guess from -// the arguments was wrong for an anonymous caller on a shared connection. -func repinAnnouncement(from, to string) string { - if from == "" { +// repinAnnouncement renders the re-pin block, or "" when no pin moved: the +// pin had no previous root, or the requested path resolved to the root it +// already held (a subdir of the current project, a language-only pin). +// +// It names the pin the DAEMON reports moving, never one guessed from the +// arguments (issue #517), and says how many other agents moved with a +// connection pin — a caller that moved its peers must be told so. The second +// line is what the caller's own next relative path resolves against, which is +// not the new connection root when an agent with a pin of its own moved the +// connection's. Every variant opens with "Re-pinned", so an absence check on +// that bare prefix still covers all of them. +func repinAnnouncement(rep RepinReport) string { + if rep.From == "" || sameDir(rep.From, rep.Root) { return "" } - return fmt.Sprintf("Re-pinned: %s → %s\n\n", from, to) + var line string + switch rep.Scope { + case PinScopeAgent: + line = fmt.Sprintf("Re-pinned your pin: %s → %s", rep.From, rep.Root) + default: + line = fmt.Sprintf("Re-pinned this connection's pin: %s → %s (%s)", rep.From, rep.Root, followersPhrase(rep.Followers)) + } + next := rep.effectiveRoot() + suffix := "" + if !sameDir(next, rep.Root) { + suffix = " (your own pin, not the connection's)" + } + return fmt.Sprintf("%s\nNext relative-path call resolves against: %s%s\n\n", line, next, suffix) +} + +// followersPhrase renders how many other agents follow a connection pin. +func followersPhrase(n int) string { + switch n { + case 0: + return "no other agent follows it" + case 1: + return "1 other agent follows it" + default: + return fmt.Sprintf("%d other agents follow it", n) + } } From 6381c76db8aa63cebd5ef4c1745611f541ff35b1 Mon Sep 17 00:00:00 2001 From: Atlas from Plumb Date: Thu, 1 Oct 2026 00:29:20 +1000 Subject: [PATCH 2/3] fix(session_start): read the re-pin report inside the move (#533 review) - attachOrRepinTo returns the connection's previous root read inside its mutation lane. It was read before the lane, so two callers racing to one target both reported moving the pin, and followConnectionShards could be handed a stale root, missing shards seeded at an intermediate one. - The caller's next-call root is derived from the move rather than re-read through workspaceFor afterwards, where a peer's concurrent move could mislabel it. An empty one is rendered as "nothing" instead of the connection's root. - The unattached path (a caller whose declaration is refused resolves to nothing while the connection is pinned) renders the report too. - repinConnection clears the caller's own refused-declaration marker; the identity-stripped move used to clear only the anonymous one. - The #516 CHANGELOG bullet is folded into this change's entry, since its interim wording never shipped. Tests: a concurrent same-target regression, an agent re-pinning its own pin (pins "from" on the agent path), and a connection-scoped move from a refused declaration. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 19 ++-- internal/cli/conn_repin.go | 25 +++-- internal/cli/conn_repin_report.go | 23 ++--- internal/cli/conn_repin_report_race_test.go | 92 +++++++++++++++++++ internal/cli/conn_repin_scope.go | 32 ++++++- .../cli/session_start_repin_report_test.go | 76 +++++++++++++++ internal/tools/session_start_brief_test.go | 11 ++- .../session_start_declared_agent_test.go | 2 +- internal/tools/session_start_test.go | 8 +- internal/tools/session_start_workspace.go | 47 ++++++---- 10 files changed, 282 insertions(+), 53 deletions(-) create mode 100644 internal/cli/conn_repin_report_race_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index a9a4c8fb..13baeab8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -136,17 +136,10 @@ declared, an unidentified call still resolves against the connection's pin, as before. -- **`session_start` no longer claims it re-pinned the connection.** It - printed "Re-pinned this connection" even when only the calling agent's own - pin moved; it now prints `Re-pinned: → `. The no-identity - notices, `plumb doctor`'s shared-connection fix and the client instruction - templates now describe the current refusal rule. - - **`session_start` says which pin a re-pin moved, and where your next - relative path goes.** The neutral `Re-pinned: → ` line above could - not say whether the caller's own pin moved or the connection's, which every - agent without a pin of its own follows. An anonymous re-pin on a shared - connection moved its peers without telling the caller. With + relative path goes.** It printed "Re-pinned this connection" even when only + the calling agent's own pin moved. It also never said when an anonymous + re-pin on a shared connection moved the other agents' workspaces with it. With `scope: "connection"`, an agent holding its own pin was shown its own root as "from", and the header named the connection's new root although the agent's relative paths still resolved against its own. The daemon now reports which @@ -154,7 +147,11 @@ Both the full and the brief packet print "Re-pinned your pin" or "Re-pinned this connection's pin (N other agents follow it)", plus a line naming what the caller's next relative-path call resolves against. The `# Workspace:` header - names the caller's own root. Closes #517. + names the caller's own root. Two callers racing to the same root no longer + both report moving the pin. A connection-scoped move now also clears the + caller's own refused-declaration marker. The no-identity notices, + `plumb doctor`'s shared-connection fix and the client instruction templates + now describe the current refusal rule. Closes #517. - **Agent identity now reaches plumb through Claude desktop's connector, and a worktree edit no longer lands in another checkout.** Claude desktop runs one diff --git a/internal/cli/conn_repin.go b/internal/cli/conn_repin.go index d23eb13a..2f08a788 100644 --- a/internal/cli/conn_repin.go +++ b/internal/cli/conn_repin.go @@ -218,20 +218,24 @@ func (s *connSession) repinWorkspaceFrom(ctx context.Context, folder, langOverri // branch below are settling paths for the CALLER, whichever scope the // pin landed on. s.clearDeclarationRefused(mcp.LogicalAgentFromCtx(ctx)) - return repinOutcome{root: root, scope: tools.PinScopeAgent, from: prev}, nil + // The caller now resolves against the root its shard was just set to: + // peers move only their own shards, and a connection move never drags a + // shard its agent chose. + return repinOutcome{root: root, scope: tools.PinScopeAgent, from: prev, effective: root}, nil } // The sticky-pin guard (issue #182) lives inside attachOrRepinTo's mutation // lane: after the root resolution above, so a requested path that resolves // to the current root is never falsely refused, and on the view under // mutation, so a concurrent re-pin can never land between the refusal - // decision and the pin move. - prevConnRoot := s.workspace() - changed, err := s.attachOrRepinTo(ctx, root, language, origin, trigger, force, synthetic, langForced) + // decision and the pin move. The previous root comes out of that same lane. + prevConnRoot, changed, err := s.attachOrRepinTo(ctx, root, language, origin, trigger, force, synthetic, langForced) if err != nil { return repinOutcome{}, err } s.attributeConnectionPin(ctx, root, origin, trigger) - out := repinOutcome{root: root, scope: tools.PinScopeConnection, from: prevConnRoot} + // A caller routed here resolves against the connection, which this move + // left at root. repinConnection corrects it for an agent keeping its own pin. + out := repinOutcome{root: root, scope: tools.PinScopeConnection, from: prevConnRoot, effective: root} if changed { s.applyProjectConfig(root) // PLAN-398: shards seeded from the old connection pin follow the move, @@ -300,9 +304,16 @@ func (s *connSession) attributeConnectionPin(ctx context.Context, root string, o // replays instead of hardcoding false. langForced marks language as an // explicit, active session_start override — the only signal allowed to // re-acquire on a same-root call (see the no-op branch). -func (s *connSession) attachOrRepinTo(ctx context.Context, root, language string, origin sessionstate.PinSource, trigger pinTrigger, force, synthetic, langForced bool) (changed bool, refused error) { +// +// prevRoot is the connection's root as the lane found it, read under the same +// mutation that moves it. The caller reports it as the pin's previous root and +// re-seeds followers from it: a root read before the lane can be stale by then, +// so two racing re-pins to one target both reported moving the pin, and shards +// seeded at an intermediate root were neither followed nor counted (#517). +func (s *connSession) attachOrRepinTo(ctx context.Context, root, language string, origin sessionstate.PinSource, trigger pinTrigger, force, synthetic, langForced bool) (prevRoot string, changed bool, refused error) { s.mutate(func(v *sessionView) { prev := v.acquiredRoot + prevRoot = prev if err := s.refuseAnonymousForcedMove(ctx, prev, root, trigger, force); err != nil { refused = err return @@ -432,7 +443,7 @@ func (s *connSession) attachOrRepinTo(ctx context.Context, root, language string // that flipped the signal. s.announceContestedPin(justContested) }) - return changed, refused + return prevRoot, changed, refused } // logLanguageOverrideBreadcrumb emits the distinguishing signal for a same-root diff --git a/internal/cli/conn_repin_report.go b/internal/cli/conn_repin_report.go index e2a2473a..d008caf9 100644 --- a/internal/cli/conn_repin_report.go +++ b/internal/cli/conn_repin_report.go @@ -22,12 +22,14 @@ import ( // repinOutcome is what one re-pin did: the requested folder's resolved root, // which pin it moved, that pin's root before the call (equal to root when -// nothing moved), and the ids of the shards that followed a connection move. +// nothing moved), the ids of the shards that followed a connection move, and +// the root the caller resolves against afterwards. type repinOutcome struct { - root string - scope tools.PinScope - from string - followed []string + root string + scope tools.PinScope + from string + followed []string + effective string } // repinReport renders a completed re-pin's outcome in session_start's terms. @@ -36,11 +38,10 @@ type repinOutcome struct { // shard still sits where the connection seeded it drags that shard too, but // the caller is not one of the OTHER agents the report counts. // -// Effective is read AFTER the move, through the same per-call resolver every -// later tool uses (workspaceFor), under the caller's own ctx — not the -// identity-stripped one repinConnection moves the connection with. It is -// therefore what the caller's next relative path resolves against: its own pin -// when it holds one, the connection's otherwise. +// Effective is derived from the move itself (repinWorkspaceFrom and +// repinConnection set it), not re-read through workspaceFor afterwards, where +// a peer's concurrent move could already have changed what the connection +// resolves to and so mislabel whose pin the caller is on. func (s *connSession) repinReport(ctx context.Context, out repinOutcome) tools.RepinReport { caller := mcp.LogicalAgentFromCtx(ctx) followers := 0 @@ -54,6 +55,6 @@ func (s *connSession) repinReport(ctx context.Context, out repinOutcome) tools.R Scope: out.scope, From: out.from, Followers: followers, - Effective: s.workspaceFor(ctx), + Effective: out.effective, } } diff --git a/internal/cli/conn_repin_report_race_test.go b/internal/cli/conn_repin_report_race_test.go new file mode 100644 index 00000000..6a98dfe4 --- /dev/null +++ b/internal/cli/conn_repin_report_race_test.go @@ -0,0 +1,92 @@ +package cli + +// conn_repin_report_race_test.go — concurrent re-pins must not produce false +// re-pin reports (#517 review). The connection's previous root used to be read +// before the mutation lane that moves it, so of several callers racing to the +// same target every one could report moving the pin, although only the first +// did; the rest were same-root no-ops. + +import ( + "context" + "encoding/json" + "sync" + "testing" + + "github.com/plumbkit/plumb/internal/mcp" +) + +// Four callers ask for the same root at once, off a non-sticky (roots) pin. +// Exactly one move happens, so exactly one report may claim it (From != Root). +func TestRepinReport_ConcurrentSameTargetClaimsOneMove(t *testing.T) { + const iters, callers = 50, 4 + for i := range iters { + s := newRepinReportSession(t) + r := repinReportRoots(t, 2) + rootA, rootB := r[0], r[1] + s.attachWorkspace(context.Background(), "file://"+rootA) + var wg sync.WaitGroup + claims := make([]bool, callers) + start := make(chan struct{}) + for g := range callers { + wg.Add(1) + go func() { + defer wg.Done() + <-start + rep, err := s.repinWorkspace(context.Background(), rootB, "", false, false) + if err != nil { + t.Errorf("repin: %v", err) + return + } + claims[g] = rep.From != "" && rep.From != rep.Root + }() + } + close(start) + wg.Wait() + n := 0 + for _, c := range claims { + if c { + n++ + } + } + // Positive control: the one real move must still be reported. + if n != 1 { + t.Fatalf("iteration %d: %d of %d concurrent re-pins to one target reported moving the pin, want exactly 1", i, n, callers) + } + } +} + +// Concurrent session_starts from several identified agents, mixing agent and +// connection scope, through the real tool surface. It asserts nothing beyond +// completing: its value is under -race, over the new report paths. +func TestRepinReport_ConcurrentSessionStartsRace(t *testing.T) { + s := newRepinReportSession(t) + r := repinReportRoots(t, 3) + if _, err := s.repinWorkspace(context.Background(), r[0], "", false, false); err != nil { + t.Fatal(err) + } + ids := []string{"a", "b", "c"} + for _, id := range ids { + s.recordLogicalAgentAttach(id) + } + var wg sync.WaitGroup + for i := range 30 { + for _, id := range ids { + wg.Add(1) + go func() { + defer wg.Done() + args := map[string]any{"detail": "brief", "workspace": r[(i+len(id))%3], "force": true} + if i%3 == 0 { + args["scope"] = "connection" + } + raw, err := json.Marshal(args) + if err != nil { + t.Errorf("marshal: %v", err) + return + } + // Refusals are legitimate under this contention; only races matter. + _, _ = newSessionStartTool(s).Execute(mcp.WithLogicalAgent(context.Background(), id), raw) + }() + } + } + wg.Wait() +} diff --git a/internal/cli/conn_repin_scope.go b/internal/cli/conn_repin_scope.go index 0cdadfef..10a6161e 100644 --- a/internal/cli/conn_repin_scope.go +++ b/internal/cli/conn_repin_scope.go @@ -65,7 +65,37 @@ func (s *connSession) repinConnection(ctx context.Context, folder, langOverride // Deliberately NOT under the caller's ctx identity: this moves the // CONNECTION, so it must reach attachOrRepinTo rather than being routed to // the caller's shard by repinShard. - return s.repinWorkspaceFrom(withConnScopeAuthorised(mcp.WithoutLogicalAgent(ctx)), folder, langOverride, sessionstate.PinSourceSessionStart, pinTriggerLive, force) + out, err := s.repinWorkspaceFrom(withConnScopeAuthorised(mcp.WithoutLogicalAgent(ctx)), folder, langOverride, sessionstate.PinSourceSessionStart, pinTriggerLive, force) + if err != nil || id == "" { + return out, err + } + // The stripped ctx made repinWorkspaceFrom settle the ANONYMOUS declaration + // marker, not this agent's. A deliberate move of the connection is a + // settling path for its caller too, so clear the caller's own marker — + // otherwise it kept resolving to nothing after a successful call. + s.clearDeclarationRefused(id) + out.effective = s.connScopeCallerRoot(id, out) + return out, nil +} + +// connScopeCallerRoot is the root an identified caller of a connection-scoped +// re-pin resolves against afterwards: the root its shard holds — the new root +// when the shard followed the move, its own pin when it did not — or, with no +// shard yet (one is seeded from the connection's new pin on its next call), the +// new root. +// +// The shard's root is read after the move rather than inside it: the move and +// the shard are guarded by different locks, and holding both would invert the +// documented order (shardsMu before sh.mu, s.mu innermost). The window left is +// narrow — only this agent's own concurrent session_start, or a later +// connection move off a root this shard shares without having chosen it, can +// change that root in between, and either leaves the answer true of the moment +// it was read. +func (s *connSession) connScopeCallerRoot(id string, out repinOutcome) string { + if own := s.recordedRootFor(id); own != "" { + return own + } + return out.root } // refuseAnonymousForcedMove refuses a forced connection re-pin that carries no diff --git a/internal/cli/session_start_repin_report_test.go b/internal/cli/session_start_repin_report_test.go index 6dc0ce7b..44dae987 100644 --- a/internal/cli/session_start_repin_report_test.go +++ b/internal/cli/session_start_repin_report_test.go @@ -269,3 +269,79 @@ func TestSessionStartRepinReport_SingleAgentConnection(t *testing.T) { }) } } + +// An agent that already holds a pin of its own moves it again. "From" must be +// the agent's own previous root, not the connection's, which it never sat on. +func TestSessionStartRepinReport_AgentScopeFromItsOwnPin(t *testing.T) { + for _, detail := range []string{"full", "brief"} { + t.Run(detail, func(t *testing.T) { + s := newRepinReportSession(t) + r := repinReportRoots(t, 3) + rootX, rootW, rootY := r[0], r[1], r[2] + if _, err := s.repinWorkspace(context.Background(), rootX, "", false, false); err != nil { + t.Fatalf("connection pin to X: %v", err) + } + s.recordLogicalAgentAttach("coordinator") + s.recordLogicalAgentAttach("own") + ctxOwn := mcp.WithLogicalAgent(context.Background(), "own") + if _, err := s.repinWorkspace(ctxOwn, rootW, "", true, false); err != nil { + t.Fatalf("own's pin to W: %v", err) + } + + out := runRepinReport(t, s, ctxOwn, map[string]any{"workspace": rootY, "force": true}, detail) + + // Positive control and absence share the prefix, so the absence + // check is live: the line exists, and names W rather than X. + wantLines(t, out, + "Re-pinned your pin: "+rootW+" → "+rootY+"\n", + "Next relative-path call resolves against: "+rootY+"\n", + ) + refuseLines(t, out, "Re-pinned your pin: "+rootX) + if got := s.workspace(); got != rootX { + t.Errorf("connection pin = %q, want it unchanged at %q", got, rootX) + } + }) + } +} + +// An agent whose workspace declaration was refused resolves to nothing, so +// session_start takes its unattached path — yet the connection is pinned, and +// a scope: "connection" move from there really moves it. The report must be +// rendered on that path too, and the move must settle the CALLER's refusal +// marker (the identity-stripped move used to clear only the anonymous one), +// so the next-call line is true. +func TestSessionStartRepinReport_ConnectionScopeFromRefusedDeclaration(t *testing.T) { + for _, detail := range []string{"full", "brief"} { + t.Run(detail, func(t *testing.T) { + s := newRepinReportSession(t) + r := repinReportRoots(t, 3) + rootX, rootY, rootZ := r[0], r[1], r[2] + if _, err := s.repinWorkspace(context.Background(), rootX, "", false, false); err != nil { + t.Fatalf("connection pin to X: %v", err) + } + s.recordLogicalAgentAttach("coord") + s.recordLogicalAgentAttach("sub") + ctxSub := mcp.WithLogicalAgent(context.Background(), "sub") + if _, err := s.repinWorkspace(ctxSub, rootY, "", false, false); err == nil { + t.Fatal("precondition: an unforced move of a seeded shard to an unrelated root is refused") + } + if _, pending := s.pendingDeclarationFor("sub"); !pending { + t.Fatal("precondition: sub's declaration is pending") + } + + out := runRepinReport(t, s, ctxSub, map[string]any{"workspace": rootZ, "scope": "connection", "force": true}, detail) + + wantLines(t, out, + "# Workspace: "+rootZ+"\n", + "Re-pinned this connection's pin: "+rootX+" → "+rootZ+" (no other agent follows it)\n", + "Next relative-path call resolves against: "+rootZ+"\n", + ) + if _, pending := s.pendingDeclarationFor("sub"); pending { + t.Error("a successful connection-scoped move left the caller's own refusal marker in place") + } + if got := s.workspaceFor(ctxSub); got != rootZ { + t.Errorf("sub resolves to %q, want %q — the report's next-call line must be true", got, rootZ) + } + }) + } +} diff --git a/internal/tools/session_start_brief_test.go b/internal/tools/session_start_brief_test.go index 8e960706..2fe903cf 100644 --- a/internal/tools/session_start_brief_test.go +++ b/internal/tools/session_start_brief_test.go @@ -182,7 +182,7 @@ func TestSessionStartBrief_CarriesIdentitySignals(t *testing.T) { tool := NewSessionStart(func(context.Context) string { return attached }, nil, nil, nil, func() string { return "" }, nil). WithRepin(func(_ context.Context, ws, _ string, _, _ bool) (RepinReport, error) { - return RepinReport{Root: ws, Scope: PinScopeConnection, From: attached}, nil + return RepinReport{Root: ws, Scope: PinScopeConnection, From: attached, Effective: ws}, nil }). WithExternalID(func(string) string { return "resumed-session" }). WithLSPSkipNote(func() string { return skipNote }) @@ -294,7 +294,7 @@ func TestRepinAnnouncement(t *testing.T) { }, { "connection pin, single agent", - RepinReport{Root: "/b", Scope: PinScopeConnection, From: "/a"}, + RepinReport{Root: "/b", Scope: PinScopeConnection, From: "/a", Effective: "/b"}, "Re-pinned this connection's pin: /a → /b (no other agent follows it)\nNext relative-path call resolves against: /b\n\n", }, { @@ -307,6 +307,13 @@ func TestRepinAnnouncement(t *testing.T) { RepinReport{Root: "/b", Scope: PinScopeConnection, From: "/a", Followers: 2, Effective: "/own"}, "Re-pinned this connection's pin: /a → /b (2 other agents follow it)\nNext relative-path call resolves against: /own (your own pin, not the connection's)\n\n", }, + { + // A caller that resolves to nothing must be told so, not shown the + // connection's root its relative paths will not reach. + "caller resolves to nothing", + RepinReport{Root: "/b", Scope: PinScopeConnection, From: "/a"}, + "Re-pinned this connection's pin: /a → /b (no other agent follows it)\nNext relative-path call resolves against: nothing — your workspace declaration is still unresolved, so pass absolute paths\n\n", + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/tools/session_start_declared_agent_test.go b/internal/tools/session_start_declared_agent_test.go index 96358254..38a8431b 100644 --- a/internal/tools/session_start_declared_agent_test.go +++ b/internal/tools/session_start_declared_agent_test.go @@ -36,7 +36,7 @@ func TestSessionStart_AttributionPrecedesTheWorkspace(t *testing.T) { if got, _ := ctx.Value(declaredAgentKeyType{}).(string); got != "subagent-7" { t.Errorf("re-pin ctx logical agent = %q, want %q — the re-pin ran unattributed", got, "subagent-7") } - return RepinReport{Root: workspace, Scope: PinScopeAgent, From: ws}, nil + return RepinReport{Root: workspace, Scope: PinScopeAgent, From: ws, Effective: workspace}, nil }) if _, err := tool.Execute(context.Background(), json.RawMessage(`{"workspace":"`+ws+`","session_id":"subagent-7"}`)); err != nil { diff --git a/internal/tools/session_start_test.go b/internal/tools/session_start_test.go index 299a3cb0..c0be7e77 100644 --- a/internal/tools/session_start_test.go +++ b/internal/tools/session_start_test.go @@ -400,7 +400,7 @@ func TestSessionStart_LanguageOverride(t *testing.T) { WithLSPLanguage(func() string { return "swift" }). // server attached after the forced pin WithRepin(func(_ context.Context, ws, lang string, _, _ bool) (RepinReport, error) { gotWs, gotLang = ws, lang - return RepinReport{Root: ws, Scope: PinScopeConnection, From: ws}, nil + return RepinReport{Root: ws, Scope: PinScopeConnection, From: ws, Effective: ws}, nil }) out, err := tool.Execute(context.Background(), json.RawMessage(`{"language":"swift"}`)) if err != nil { @@ -449,7 +449,7 @@ func TestSessionStart_WorkspaceResolution(t *testing.T) { tool := NewSessionStart(func(context.Context) string { return attached }, nil, nil, nil, func() string { return "" }, nil). WithRepin(func(_ context.Context, ws, _ string, _, _ bool) (RepinReport, error) { got = ws - return RepinReport{Root: ws, Scope: PinScopeConnection, From: attached}, nil + return RepinReport{Root: ws, Scope: PinScopeConnection, From: attached, Effective: ws}, nil }) out, err := tool.Execute(context.Background(), json.RawMessage(`{"workspace":"`+target+`"}`)) if err != nil { @@ -811,7 +811,7 @@ func TestSessionStart_ForceThreadedToRepin(t *testing.T) { tool := NewSessionStart(func(context.Context) string { return attached }, nil, nil, nil, func() string { return "" }, nil). WithRepin(func(_ context.Context, ws, _ string, force, _ bool) (RepinReport, error) { gotForce = append(gotForce, force) - return RepinReport{Root: ws, Scope: PinScopeConnection, From: attached}, nil + return RepinReport{Root: ws, Scope: PinScopeConnection, From: attached, Effective: ws}, nil }) if _, err := tool.Execute(context.Background(), json.RawMessage(`{"workspace":"`+target+`"}`)); err != nil { t.Fatalf("Execute without force: %v", err) @@ -834,7 +834,7 @@ func TestSessionStart_ForceThreadedOnUnattachedLanguagePin(t *testing.T) { tool := NewSessionStart(func(context.Context) string { return "" }, nil, nil, nil, func() string { return "" }, nil). WithRepin(func(_ context.Context, _, _ string, force, _ bool) (RepinReport, error) { gotForce = force - return RepinReport{Root: target, Scope: PinScopeConnection}, nil + return RepinReport{Root: target, Scope: PinScopeConnection, Effective: target}, nil }) raw := json.RawMessage(`{"workspace":"` + target + `","language":"go","force":true}`) if _, err := tool.Execute(context.Background(), raw); err != nil { diff --git a/internal/tools/session_start_workspace.go b/internal/tools/session_start_workspace.go index e4410a90..e1b5aeb4 100644 --- a/internal/tools/session_start_workspace.go +++ b/internal/tools/session_start_workspace.go @@ -45,12 +45,15 @@ type RepinReport struct { // Effective is the root the caller's next unqualified (relative-path) call // resolves against. It differs from Root when an agent with a pin of its own // moves the connection's pin: the agent itself stays where it was. Empty - // means Root. + // means the caller resolves against NOTHING (an agent whose workspace + // declaration is still refused), and the announcement says so rather than + // naming a root its relative paths will not reach. Effective string } -// effectiveRoot is the root the caller resolves against after the re-pin. -func (r RepinReport) effectiveRoot() string { +// headerRoot is the root the packet orients on: the caller's own, or the +// resolved root when the caller has none to name. +func (r RepinReport) headerRoot() string { if r.Effective != "" { return r.Effective } @@ -91,8 +94,7 @@ func (t *SessionStart) resolveSessionWorkspace(ctx context.Context, raw json.Raw // across all connections), and guessing it produced confidently-wrong // "workspaces". if a.Workspace != "" { - ws, uerr := t.resolveUnattachedWorkspace(ctx, a.Workspace, a.Language, a.Force, connScope) - return ws, "", uerr + return t.resolveUnattachedWorkspace(ctx, a.Workspace, a.Language, a.Force, connScope) } if t.roots != nil { if ws := t.roots(ctx); ws != "" { @@ -115,18 +117,24 @@ func (t *SessionStart) resolveSessionWorkspace(ctx context.Context, raw json.Raw // deferral removes. The resolved root (not the raw argument) coming back keeps // the displayed workspace consistent with the TUI, memory, and topology, as // the language branch already did. -func (t *SessionStart) resolveUnattachedWorkspace(ctx context.Context, workspace, language string, force, connScope bool) (string, error) { +// +// "Unattached" is the CALLER's view: an agent whose workspace declaration is +// still refused resolves to nothing while the connection itself is pinned, so +// a re-pin from here can move an existing pin — even the connection's, with +// its followers. The announcement is therefore rendered here too; on a truly +// unattached connection there is no previous root and it renders nothing. +func (t *SessionStart) resolveUnattachedWorkspace(ctx context.Context, workspace, language string, force, connScope bool) (string, string, error) { if t.repin == nil { - return workspace, nil + return workspace, "", nil } rep, err := t.repin(ctx, workspace, language, force, connScope) if err != nil { if language != "" { - return "", fmt.Errorf("session_start: pinning %s as %s: %w", workspace, language, err) + return "", "", fmt.Errorf("session_start: pinning %s as %s: %w", workspace, language, err) } - return "", fmt.Errorf("session_start: pinning %s: %w", workspace, err) + return "", "", fmt.Errorf("session_start: pinning %s: %w", workspace, err) } - return rep.effectiveRoot(), nil + return rep.headerRoot(), repinAnnouncement(rep), nil } // resolveAttached handles session_start on an already-attached connection: an @@ -192,7 +200,7 @@ func (t *SessionStart) repinExplicit(ctx context.Context, current, requested, la if err != nil { return "", "", fmt.Errorf("session_start: re-pinning to %s: %w", requested, err) } - return rep.effectiveRoot(), repinAnnouncement(rep), nil + return rep.headerRoot(), repinAnnouncement(rep), nil } // forceLanguage re-pins the connection's CURRENT workspace to a forced primary @@ -231,12 +239,19 @@ func repinAnnouncement(rep RepinReport) string { default: line = fmt.Sprintf("Re-pinned this connection's pin: %s → %s (%s)", rep.From, rep.Root, followersPhrase(rep.Followers)) } - next := rep.effectiveRoot() - suffix := "" - if !sameDir(next, rep.Root) { - suffix = " (your own pin, not the connection's)" + return fmt.Sprintf("%s\nNext relative-path call resolves against: %s\n\n", line, nextCallTarget(rep)) +} + +// nextCallTarget names what the caller's next relative path resolves against. +func nextCallTarget(rep RepinReport) string { + switch { + case rep.Effective == "": + return "nothing — your workspace declaration is still unresolved, so pass absolute paths" + case !sameDir(rep.Effective, rep.Root): + return rep.Effective + " (your own pin, not the connection's)" + default: + return rep.Effective } - return fmt.Sprintf("%s\nNext relative-path call resolves against: %s%s\n\n", line, next, suffix) } // followersPhrase renders how many other agents follow a connection pin. From 4159b4f075ea9ab1e9a6c155f35607d5bf292fa2 Mon Sep 17 00:00:00 2001 From: Atlas from Plumb Date: Thu, 1 Oct 2026 00:59:54 +1000 Subject: [PATCH 3/3] fix(session_start): label a connection follower by whether it follows (#533 review 2) connScopeCallerRoot read the caller's shard root after a connection-scoped move and the report labelled any difference "(your own pin, not the connection's)". A shard that never chose a root follows the connection, so a peer's concurrent connection move dragged it on and the caller was told it was on its own pin (107 of 320 reports in the reviewer's run). Decide by whether the shard follows instead: a shard that is neither self-pinned nor restored resolves against the root this move left the connection at; only a self-pinned or restored shard reports its own root. selfPinned never goes back to false, and a self-pinned shard's root moves only through its own agent, so the one window left is that agent's own concurrent session_start. Tests: a bounded stress test of concurrent connection-scoped moves by two following agents asserting zero mislabels, a restored-pin case, and a guard that a refused connection-scoped move keeps the caller's refused-declaration marker. Co-Authored-By: Claude Opus 5.5 --- internal/cli/conn_repin_report_race_test.go | 57 +++++++++++++++ internal/cli/conn_repin_scope.go | 41 +++++++---- .../cli/session_start_repin_report_test.go | 69 +++++++++++++++++++ 3 files changed, 153 insertions(+), 14 deletions(-) diff --git a/internal/cli/conn_repin_report_race_test.go b/internal/cli/conn_repin_report_race_test.go index 6a98dfe4..6cf0d5e3 100644 --- a/internal/cli/conn_repin_report_race_test.go +++ b/internal/cli/conn_repin_report_race_test.go @@ -9,6 +9,7 @@ package cli import ( "context" "encoding/json" + "strings" "sync" "testing" @@ -90,3 +91,59 @@ func TestRepinReport_ConcurrentSessionStartsRace(t *testing.T) { } wg.Wait() } + +// Two agents that never chose a root of their own both follow the connection, +// and move it concurrently with scope "connection". Each report must say the +// caller resolves against the root ITS move left the connection at; none may +// call it the caller's own pin. Deciding by comparing the shard's root after +// the move mislabelled a follower a peer had already dragged on. +func TestRepinReport_ConcurrentConnectionScopeFollowersNotMislabelled(t *testing.T) { + const iters, callers = 20, 8 + mislabels, total := 0, 0 + for range iters { + s := newRepinReportSession(t) + r := repinReportRoots(t, 3) + if _, err := s.repinWorkspace(context.Background(), r[0], "", false, false); err != nil { + t.Fatal(err) + } + ids := []string{"a", "b"} + for _, id := range ids { + s.recordLogicalAgentAttach(id) + } + for _, id := range ids { + _ = s.workspaceFor(mcp.WithLogicalAgent(context.Background(), id)) // seed both shards + } + var mu sync.Mutex + var wg sync.WaitGroup + for g := range callers { + wg.Add(1) + go func() { + defer wg.Done() + raw, err := json.Marshal(map[string]any{"detail": "brief", "workspace": r[1+g%2], "scope": "connection", "force": true}) + if err != nil { + t.Errorf("marshal: %v", err) + return + } + out, err := newSessionStartTool(s).Execute(mcp.WithLogicalAgent(context.Background(), ids[g%2]), raw) + if err != nil { + return + } + mu.Lock() + defer mu.Unlock() + total++ + if strings.Contains(out, "your own pin, not the connection's") { + mislabels++ + } + }() + } + wg.Wait() + s.close() + } + // Positive control: the calls must actually have produced reports to judge. + if total == 0 { + t.Fatal("no session_start succeeded, so nothing was checked") + } + if mislabels > 0 { + t.Errorf("%d of %d reports told an agent that follows the connection it was on its own pin", mislabels, total) + } +} diff --git a/internal/cli/conn_repin_scope.go b/internal/cli/conn_repin_scope.go index 10a6161e..79a3e5b2 100644 --- a/internal/cli/conn_repin_scope.go +++ b/internal/cli/conn_repin_scope.go @@ -79,23 +79,36 @@ func (s *connSession) repinConnection(ctx context.Context, folder, langOverride } // connScopeCallerRoot is the root an identified caller of a connection-scoped -// re-pin resolves against afterwards: the root its shard holds — the new root -// when the shard followed the move, its own pin when it did not — or, with no -// shard yet (one is seeded from the connection's new pin on its next call), the -// new root. +// re-pin resolves against afterwards. // -// The shard's root is read after the move rather than inside it: the move and -// the shard are guarded by different locks, and holding both would invert the -// documented order (shardsMu before sh.mu, s.mu innermost). The window left is -// narrow — only this agent's own concurrent session_start, or a later -// connection move off a root this shard shares without having chosen it, can -// change that root in between, and either leaves the answer true of the moment -// it was read. +// It is decided by WHETHER the caller's shard follows the connection, not by +// comparing roots after the fact. A shard that never chose a root follows the +// connection, so it resolves against the root this move left the connection +// at — even if a peer's concurrent connection move has already dragged it on, +// which a root comparison mislabelled as "your own pin". A caller with no shard +// yet is seeded from the connection on its next call, so the same holds. Only +// a shard that chose its root (selfPinned) or restored its own persisted pin +// keeps a root of its own, and that root is reported. +// +// The shard is read after the move: the move and the shard are guarded by +// different locks, and holding both would invert the documented order +// (shardsMu before sh.mu, s.mu innermost). selfPinned only ever goes from false +// to true, and a self-pinned shard's root changes only through its own agent's +// repinAgent, so the one window left is this agent's own concurrent +// session_start. func (s *connSession) connScopeCallerRoot(id string, out repinOutcome) string { - if own := s.recordedRootFor(id); own != "" { - return own + s.shardsMu.Lock() + sh := s.shards[id] + s.shardsMu.Unlock() + if sh == nil { + return out.root + } + sh.mu.RLock() + defer sh.mu.RUnlock() + if !sh.selfPinned && !sh.restored { + return out.root } - return out.root + return sh.root } // refuseAnonymousForcedMove refuses a forced connection re-pin that carries no diff --git a/internal/cli/session_start_repin_report_test.go b/internal/cli/session_start_repin_report_test.go index 44dae987..686c5a13 100644 --- a/internal/cli/session_start_repin_report_test.go +++ b/internal/cli/session_start_repin_report_test.go @@ -18,6 +18,7 @@ import ( "github.com/plumbkit/plumb/internal/config" "github.com/plumbkit/plumb/internal/mcp" + "github.com/plumbkit/plumb/internal/sessionstate" ) func newRepinReportSession(t *testing.T) *connSession { @@ -345,3 +346,71 @@ func TestSessionStartRepinReport_ConnectionScopeFromRefusedDeclaration(t *testin }) } } + +// The other half of the settling rule: a connection-scoped move that is +// REFUSED settles nothing, so the caller's refused-declaration marker must +// survive it. The successful twin above is the positive control that clearing +// happens at all. +func TestSessionStartRepinReport_RefusedConnectionScopeKeepsCallerMarker(t *testing.T) { + s := newRepinReportSession(t) + r := repinReportRoots(t, 3) + rootX, rootY, rootZ := r[0], r[1], r[2] + if _, err := s.repinWorkspace(context.Background(), rootX, "", false, false); err != nil { + t.Fatalf("connection pin to X: %v", err) + } + s.recordLogicalAgentAttach("coord") + s.recordLogicalAgentAttach("sub") + ctxSub := mcp.WithLogicalAgent(context.Background(), "sub") + if _, err := s.repinWorkspace(ctxSub, rootY, "", false, false); err == nil { + t.Fatal("precondition: an unforced move of a seeded shard to an unrelated root is refused") + } + // Unforced, against the connection's sticky explicit pin: refused. + raw, err := json.Marshal(map[string]any{"workspace": rootZ, "scope": "connection"}) + if err != nil { + t.Fatal(err) + } + if _, err := newSessionStartTool(s).Execute(ctxSub, raw); err == nil { + t.Fatal("precondition: an unforced connection-scoped move off a sticky pin is refused") + } + if _, pending := s.pendingDeclarationFor("sub"); !pending { + t.Error("a REFUSED connection-scoped move cleared the caller's refused-declaration marker") + } + if got := s.workspace(); got != rootX { + t.Errorf("connection pin = %q, want it unchanged at %q", got, rootX) + } +} + +// A shard restored from the agent's own persisted pin holds a root of its own +// without ever having re-pinned in this daemon (selfPinned stays false). It did +// not follow a connection move off another root, so the report must name its +// restored root as the caller's own pin, not the connection's new one. +func TestSessionStartRepinReport_ConnectionScopeFromRestoredPin(t *testing.T) { + store, ss := newOriginStore(t) + r := repinReportRoots(t, 3) + rootX, rootW, rootZ := r[0], r[1], r[2] + const proxyID = "proxy-restored" + if err := ss.UpsertPinForAgent(proxyID, "own", rootW, "", sessionstate.PinSourceSessionStart); err != nil { + t.Fatalf("seed the agent's persisted pin: %v", err) + } + s := newPersistSession(t, store, ss, proxyID) + if _, err := s.repinWorkspace(context.Background(), rootX, "", false, false); err != nil { + t.Fatalf("connection pin to X: %v", err) + } + s.recordLogicalAgentAttach("own") + s.recordLogicalAgentAttach("peer") + ctxOwn := mcp.WithLogicalAgent(context.Background(), "own") + if got := s.workspaceFor(ctxOwn); got != rootW { + t.Fatalf("precondition: own's shard restored at %q, want %q", got, rootW) + } + + out := runRepinReport(t, s, ctxOwn, map[string]any{"workspace": rootZ, "scope": "connection", "force": true}, "brief") + + wantLines(t, out, + "# Workspace: "+rootW+"\n", + "Re-pinned this connection's pin: "+rootX+" → "+rootZ, + "Next relative-path call resolves against: "+rootW+" (your own pin, not the connection's)\n", + ) + if got := s.workspaceFor(ctxOwn); got != rootW { + t.Errorf("own resolves to %q, want its restored pin %q", got, rootW) + } +}