From 1204833642fd01802cd798a0d0e8d6efabe5d81e Mon Sep 17 00:00:00 2001 From: Atlas from Plumb Date: Wed, 14 Oct 2026 19:25:26 +1100 Subject: [PATCH 1/2] fix(context_for_task): detect each directory's project once per call (PLAN-548) After #663's per-file memo, a pack still asked the daemon's sensitive-path decision about each of its 100-250 distinct files, and each ask resolved the file's project with workspacePool.Detect, globbing for markers on disk. On the live daemon the slowest arm R pack ran at 1.6-1.9 s of its 2 s graph deadline, and another was cut in 2 of 5 reps. Collect now attaches a tools.CallMemo to the call's context. historyRootFor reads it and detects each directory once for that call. The memo is fresh per call, so a marker created between two calls is seen by the second, and writes, which never carry one, detect on every ask as before. changeSensitive stays the one decision; workspaceFromArgs is split into detectStartDir and detectedRoot with identical behaviour. Tests: - one memo per call, and a fresh one for the next call; - a marker created mid-call is not re-detected, but the next call and an ask outside a call both see it; - changeSensitive gives the same verdict with and without the memo, including a source in another project that is sensitive only by that project's glob. Co-Authored-By: Claude Opus 5.5 Plumb-Session: scarlet-badger --- CHANGELOG.md | 9 ++ internal/cli/conn_history.go | 13 ++- internal/cli/conn_history_callmemo_test.go | 116 +++++++++++++++++++++ internal/cli/daemon_workspace.go | 20 +++- internal/tools/context_callmemo.go | 54 ++++++++++ internal/tools/context_callmemo_test.go | 70 +++++++++++++ internal/tools/context_collect.go | 3 + 7 files changed, 280 insertions(+), 5 deletions(-) create mode 100644 internal/cli/conn_history_callmemo_test.go create mode 100644 internal/tools/context_callmemo.go create mode 100644 internal/tools/context_callmemo_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 35e37050..cec1fb3e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -300,6 +300,15 @@ ### Fixed +- **`context_for_task` detects each directory's project once per call.** After + the per-file memo below, a pack still resolved the project of each of its 100 + to 250 files on disk, and the slowest arm R pack ran at 1.7–1.9 s of its 2 s + graph deadline. The files of one directory resolve alike, so within a call + the detection is now remembered per directory. The memo lives on the call's + context: it is fresh for every call, a marker created between two calls is + seen by the second, and writes (which never carry one) detect on every ask as + before. The sensitive-path decision itself is unchanged (PLAN-548). + - **`context_for_task` asks each file's sensitivity once per call, so live packs keep their affected tests.** The walk asks whether every node it reaches is in a sensitive file, 600 to 2,000 times per pack over a hundred or two files, and diff --git a/internal/cli/conn_history.go b/internal/cli/conn_history.go index a24ab52f..df0dd53b 100644 --- a/internal/cli/conn_history.go +++ b/internal/cli/conn_history.go @@ -9,6 +9,7 @@ import ( "github.com/plumbkit/plumb/internal/config" "github.com/plumbkit/plumb/internal/history" "github.com/plumbkit/plumb/internal/mcp" + "github.com/plumbkit/plumb/internal/tools" "github.com/plumbkit/plumb/internal/tools/txlog" ) @@ -59,6 +60,11 @@ func (s *connSession) historyOnFor(ctx context.Context, path string) bool { // agent's root, then the project the path itself lies in (onAfterTool's // resolution, so history and stats file a call under the same project), else // the path's directory. +// +// Inside a context_for_task call the project detection is remembered per +// directory for that call (tools.CallMemo): the pack asks about every file it +// reaches, and detection globs for markers on disk each time. Outside one, as +// for every write, it runs on each ask. func (s *connSession) historyRootFor(ctx context.Context, path string) string { root := s.view().acquiredRoot if w := s.recordedRootFor(mcp.LogicalAgentFromCtx(ctx)); w != "" { @@ -67,8 +73,11 @@ func (s *connSession) historyRootFor(ctx context.Context, path string) string { // No pool (a session not yet attached to the daemon's, as tests build them) // has no project to resolve the path against; the agent's root stands. if s.pool != nil { - if w := workspaceFromArgs(s.pool, pathArg(path), root); w != "" { - root = w + if dir := detectStartDir(pathArg(path), root); dir != "" { + w := tools.CallMemoFrom(ctx).Once(dir, func() string { return detectedRoot(s.pool, dir) }) + if w != "" { + root = w + } } } if root == "" { diff --git a/internal/cli/conn_history_callmemo_test.go b/internal/cli/conn_history_callmemo_test.go new file mode 100644 index 00000000..e0ab5ea6 --- /dev/null +++ b/internal/cli/conn_history_callmemo_test.go @@ -0,0 +1,116 @@ +package cli + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/plumbkit/plumb/internal/config" + "github.com/plumbkit/plumb/internal/tools" +) + +// conn_history_callmemo_test.go — inside one context_for_task call the project of +// each directory is detected once (tools.CallMemo, PLAN-548); outside one, as for +// every write, it is detected on each ask. The verdict is the same either way. + +func memoTestSession(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 writeTree(t *testing.T, root string, files map[string]string) { + t.Helper() + for name, body := range files { + p := filepath.Join(root, filepath.FromSlash(name)) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + } +} + +// assertSameDir compares by identity, not by string: a TempDir path and the root +// Detect returns can spell one directory differently (macOS /var → /private/var). +func assertSameDir(t *testing.T, got, want, why string) { + t.Helper() + gi, err1 := os.Stat(got) + wi, err2 := os.Stat(want) + if err1 != nil || err2 != nil || !os.SameFile(gi, wi) { + t.Errorf("%s: root = %q, want %q", why, got, want) + } +} + +func TestHistoryRootFor_DetectsEachDirectoryOncePerContextCall(t *testing.T) { + s := memoTestSession(t) + root := t.TempDir() + pkg := filepath.Join(root, "pkg") + writeTree(t, root, map[string]string{"go.mod": "module x\n", "pkg/a.go": "package pkg\n", "pkg/b.go": "package pkg\n"}) + a, b := filepath.Join(pkg, "a.go"), filepath.Join(pkg, "b.go") + + call := tools.WithCallMemo(context.Background()) + assertSameDir(t, s.historyRootFor(call, a), root, "first ask") + + // pkg becomes a project of its own part-way through the call. The call's + // second ask about that directory keeps the answer it already has: it did not + // detect again. + writeTree(t, root, map[string]string{"pkg/go.mod": "module x/pkg\n"}) + assertSameDir(t, s.historyRootFor(call, b), root, "a second ask in the same call (remembered)") + + // The next call, and any ask outside a call, sees the marker. + assertSameDir(t, s.historyRootFor(tools.WithCallMemo(context.Background()), b), pkg, "the next call") + assertSameDir(t, s.historyRootFor(context.Background(), a), pkg, "an ask outside a call (every write)") +} + +// TestChangeSensitive_CallMemoKeepsEveryVerdict: the memo changes how often the +// decision detects, never what it decides, including a change whose source lies +// in another project whose own globs make it sensitive. +func TestChangeSensitive_CallMemoKeepsEveryVerdict(t *testing.T) { + s := memoTestSession(t) + a, b := t.TempDir(), t.TempDir() + writeTree(t, a, map[string]string{"go.mod": "module a\n", "main.go": "package main\n", "sub/util.go": "package sub\n"}) + writeTree(t, b, map[string]string{ + "go.mod": "module b\n", + "main.go": "package main\n", + "key.vault": "secret\n", + ".plumb/config.toml": "[history]\nsensitive_globs = [\"*.vault\"]\n", + }) + cases := []struct{ name, path, from string }{ + {"an ordinary file", filepath.Join(a, "main.go"), ""}, + {"a default glob", filepath.Join(a, "server.pem"), ""}, + {"a glob of its own project", filepath.Join(b, "key.vault"), ""}, + {"the same name where no glob says so", filepath.Join(a, "key.vault"), ""}, + {"moved out of another project's sensitive file", filepath.Join(a, "sub/util.go"), filepath.Join(b, "key.vault")}, + {"moved in from an ordinary file", filepath.Join(b, "main.go"), filepath.Join(a, "main.go")}, + } + + call := tools.WithCallMemo(context.Background()) + var sensitive, plain int + for _, tc := range cases { + want := s.changeSensitive(context.Background(), tc.path, tc.from) + // Asked twice in the call, so the second answer comes from the memo. + for range 2 { + if got := s.changeSensitive(call, tc.path, tc.from); got != want { + t.Errorf("%s: verdict in a call = %v, outside one = %v", tc.name, got, want) + } + } + if want { + sensitive++ + } else { + plain++ + } + } + // Controls: the cases span both verdicts, and the cross-project source really + // is sensitive only through its own project's glob. + if sensitive == 0 || plain == 0 { + t.Fatalf("control: %d sensitive and %d plain verdicts; the cases must span both", sensitive, plain) + } + if !s.changeSensitive(context.Background(), cases[4].path, cases[4].from) || s.changeSensitive(context.Background(), cases[3].path, "") { + t.Fatal("control: the cross-project case must be sensitive through b's glob alone") + } +} diff --git a/internal/cli/daemon_workspace.go b/internal/cli/daemon_workspace.go index 2a239e0c..8e99a784 100644 --- a/internal/cli/daemon_workspace.go +++ b/internal/cli/daemon_workspace.go @@ -99,6 +99,16 @@ func rootsFromClient(ctx context.Context, request mcp.RequestFn, logger *slog.Lo // resolve it against the daemon's cwd, which belongs to no caller. With no base // a relative seed attributes nothing. func workspaceFromArgs(pool *workspacePool, args json.RawMessage, base string) string { + dir := detectStartDir(args, base) + if dir == "" { + return "" + } + return detectedRoot(pool, dir) +} + +// detectStartDir is the directory workspaceFromArgs detects from: the seed when +// it is a directory, else its parent. "" when args name no usable seed. +func detectStartDir(args json.RawMessage, base string) string { seed := seedPathFromArgs(args) if seed == "" { seed = repoFromArgs(args) @@ -114,11 +124,15 @@ func workspaceFromArgs(pool *workspacePool, args json.RawMessage, base string) s } // If seed is already a directory, use it directly — filepath.Dir would // strip the last component and miss the project root marker. - startDir := seed if info, err := os.Stat(seed); err != nil || !info.IsDir() { - startDir = filepath.Dir(seed) + return filepath.Dir(seed) } - root, _, err := pool.Detect(startDir) + return seed +} + +// detectedRoot is the project root Detect finds from dir, or "" when none. +func detectedRoot(pool *workspacePool, dir string) string { + root, _, err := pool.Detect(dir) if err != nil { return "" } diff --git a/internal/tools/context_callmemo.go b/internal/tools/context_callmemo.go new file mode 100644 index 00000000..f6178b7d --- /dev/null +++ b/internal/tools/context_callmemo.go @@ -0,0 +1,54 @@ +package tools + +import ( + "context" + "sync" +) + +// CallMemo remembers lookups for the length of one context_for_task collect. A +// pack asks the sensitive-path decision about a hundred or more files, and the +// daemon's decision resolves each file's project on disk; the files of one +// directory all resolve the same way, and nothing the call does creates a +// project marker, so within a call each directory need be resolved once. +// +// It lives on the call's context and nowhere else: Collect attaches a fresh one +// on every call, so an answer can never outlive the call that asked, and a +// marker created between two calls is seen by the second. A nil *CallMemo (a +// context no collect made) remembers nothing and runs every lookup. +// +// Concurrency: safe for concurrent use; a lookup runs under the memo's lock, so +// two askers of one key run it once. +type CallMemo struct { + mu sync.Mutex + seen map[string]string +} + +type callMemoKey struct{} + +// WithCallMemo returns ctx carrying a fresh, empty CallMemo. Collect makes one +// per call; a test of a decision that reads it makes its own. +func WithCallMemo(ctx context.Context) context.Context { + return context.WithValue(ctx, callMemoKey{}, &CallMemo{seen: map[string]string{}}) +} + +// CallMemoFrom returns the memo of the collect ctx belongs to, or nil outside one. +func CallMemoFrom(ctx context.Context) *CallMemo { + m, _ := ctx.Value(callMemoKey{}).(*CallMemo) + return m +} + +// Once returns the answer remembered for key, running lookup to get it the first +// time. On a nil memo it runs lookup every time. +func (m *CallMemo) Once(key string, lookup func() string) string { + if m == nil { + return lookup() + } + m.mu.Lock() + defer m.mu.Unlock() + if v, ok := m.seen[key]; ok { + return v + } + v := lookup() + m.seen[key] = v + return v +} diff --git a/internal/tools/context_callmemo_test.go b/internal/tools/context_callmemo_test.go new file mode 100644 index 00000000..d9887d1f --- /dev/null +++ b/internal/tools/context_callmemo_test.go @@ -0,0 +1,70 @@ +package tools + +import ( + "context" + "sync" + "testing" +) + +// TestCallMemo_RemembersPerKeyAndNilRemembersNothing: a memo runs a key's lookup +// once; a nil memo (a context no collect made) runs it on every ask. +func TestCallMemo_RemembersPerKeyAndNilRemembersNothing(t *testing.T) { + m := CallMemoFrom(WithCallMemo(context.Background())) + runs := map[string]int{} + look := func(key string) func() string { + return func() string { runs[key]++; return "root-" + key } + } + for range 3 { + if got := m.Once("a", look("a")); got != "root-a" { + t.Fatalf("Once(a) = %q", got) + } + } + m.Once("b", look("b")) + if runs["a"] != 1 || runs["b"] != 1 { + t.Errorf("lookups ran %v, want each key once", runs) + } + + var none *CallMemo + if CallMemoFrom(context.Background()) != nil { + t.Fatal("a context no collect made carries a memo") + } + none.Once("c", look("c")) + none.Once("c", look("c")) + if runs["c"] != 2 { + t.Errorf("a nil memo ran its lookup %d times over two asks, want twice", runs["c"]) + } +} + +// TestContextForTask_SensitiveAsksShareOneMemoPerCall: every ask one call makes +// of the sensitive-path decision carries the same memo, and the next call a fresh +// one, so a remembered answer never outlives the call that asked. +func TestContextForTask_SensitiveAsksShareOneMemoPerCall(t *testing.T) { + s := newShop(t) + var mu sync.Mutex + var seen []*CallMemo + s.collector.WithSensitive(func(ctx context.Context, _, _ string) bool { + mu.Lock() + seen = append(seen, CallMemoFrom(ctx)) + mu.Unlock() + return false + }) + args := map[string]any{"symbols": []string{"pricing/discount.go#Apply"}, "intent": contextIntentChange} + + s.collect(t, args) + first := len(seen) + if first == 0 { + t.Fatal("control: the call never asked about sensitivity, so the test proves nothing") + } + s.collect(t, args) + for i, m := range seen { + if m == nil { + t.Fatalf("ask %d carried no memo", i) + } + if want := seen[0]; i < first && m != want { + t.Errorf("ask %d of the first call carried a different memo", i) + } + } + if len(seen) == first || seen[first] == seen[0] { + t.Error("the second call reused the first call's memo: its answers would outlive the call") + } +} diff --git a/internal/tools/context_collect.go b/internal/tools/context_collect.go index b15eb5ad..94c84879 100644 --- a/internal/tools/context_collect.go +++ b/internal/tools/context_collect.go @@ -238,6 +238,9 @@ func (r *contextRequest) seedInputs() []seedInput { // pack. Everything it reads is keyed by the agent's canonical root, never the // connection's. func (c *ContextCollector) Collect(ctx context.Context, req contextRequest) (contextPack, error) { + // Every ask this call makes of the sensitive-path decision carries one memo, + // fresh per call (see CallMemo). + ctx = WithCallMemo(ctx) root, err := c.agentRoot(ctx) if err != nil { return contextPack{}, err From bdff688e0807c215fec6e4dbd2aedc0a940f64ee Mon Sep 17 00:00:00 2001 From: Atlas from Plumb Date: Wed, 14 Oct 2026 19:32:44 +1100 Subject: [PATCH 2/2] =?UTF-8?q?docs(tools):=20CallMemo=20holds=20directory?= =?UTF-8?q?=20=E2=86=92=20project-root=20answers=20only?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review NIT on #665: a second kind of lookup through the same memo would share its key space with directories. Co-Authored-By: Claude Opus 5.5 Plumb-Session: scarlet-badger --- internal/tools/context_callmemo.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/tools/context_callmemo.go b/internal/tools/context_callmemo.go index f6178b7d..ebd34218 100644 --- a/internal/tools/context_callmemo.go +++ b/internal/tools/context_callmemo.go @@ -16,6 +16,10 @@ import ( // marker created between two calls is seen by the second. A nil *CallMemo (a // context no collect made) remembers nothing and runs every lookup. // +// It holds one kind of answer only: a directory's project root, keyed by the +// directory (cli's historyRootFor). A second kind of lookup would share that key +// space, so it needs a memo of its own rather than this one. +// // Concurrency: safe for concurrent use; a lookup runs under the memo's lock, so // two askers of one key run it once. type CallMemo struct {