Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 11 additions & 2 deletions internal/cli/conn_history.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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 != "" {
Expand All @@ -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 == "" {
Expand Down
116 changes: 116 additions & 0 deletions internal/cli/conn_history_callmemo_test.go
Original file line number Diff line number Diff line change
@@ -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")
}
}
20 changes: 17 additions & 3 deletions internal/cli/daemon_workspace.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 ""
}
Expand Down
58 changes: 58 additions & 0 deletions internal/tools/context_callmemo.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
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.
//
// 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 {
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
}
70 changes: 70 additions & 0 deletions internal/tools/context_callmemo_test.go
Original file line number Diff line number Diff line change
@@ -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")
}
}
3 changes: 3 additions & 0 deletions internal/tools/context_collect.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading