diff --git a/CHANGELOG.md b/CHANGELOG.md index 31962d5b..769d26d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -148,11 +148,22 @@ 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.** 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 + 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. 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_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..2f08a788 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,36 +209,42 @@ 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 + // 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 "", err + return repinOutcome{}, err } s.attributeConnectionPin(ctx, root, origin, trigger) + // 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, // 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 @@ -282,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 @@ -414,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 new file mode 100644 index 00000000..d008caf9 --- /dev/null +++ b/internal/cli/conn_repin_report.go @@ -0,0 +1,60 @@ +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), 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 + effective 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 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 + for _, id := range out.followed { + if id != caller { + followers++ + } + } + return tools.RepinReport{ + Root: out.root, + Scope: out.scope, + From: out.from, + Followers: followers, + 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..6cf0d5e3 --- /dev/null +++ b/internal/cli/conn_repin_report_race_test.go @@ -0,0 +1,149 @@ +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" + "strings" + "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() +} + +// 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 5e7730c6..79a3e5b2 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, @@ -65,7 +65,50 @@ 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. +// +// 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 { + 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 sh.root } // refuseAnonymousForcedMove refuses a forced connection re-pin that carries no 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..686c5a13 --- /dev/null +++ b/internal/cli/session_start_repin_report_test.go @@ -0,0 +1,416 @@ +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" + "github.com/plumbkit/plumb/internal/sessionstate" +) + +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") + }) + } +} + +// 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) + } + }) + } +} + +// 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) + } +} 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..2fe903cf 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, Effective: ws}, 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,51 @@ 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", Effective: "/b"}, + "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", + }, + { + // 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) { + 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..38a8431b 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, Effective: workspace}, 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..c0be7e77 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, Effective: 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, Effective: ws}, 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, Effective: ws}, 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, 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 befb7e8b..e1b5aeb4 100644 --- a/internal/tools/session_start_workspace.go +++ b/internal/tools/session_start_workspace.go @@ -13,10 +13,58 @@ 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 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 +} + +// 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 + } + 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"` @@ -46,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 != "" { @@ -70,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 } - 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 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 root, nil + return rep.headerRoot(), repinAnnouncement(rep), nil } // resolveAttached handles session_start on an already-attached connection: an @@ -143,18 +196,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.headerRoot(), repinAnnouncement(rep), nil } // forceLanguage re-pins the connection's CURRENT workspace to a forced primary @@ -171,13 +217,51 @@ 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)) + } + 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 + } +} + +// 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) + } }