mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
## What Identify agent workspace-context **sources** by their lexical (configured) path so `coder exp chat context list` no longer shows the same directory twice, and so a source is shown as the path the operator actually configured. ## Why (the bug) Source identity was the canonical path from `CanonicalizePath`, which resolves symlinks via `EvalSymlinks` **only when the target exists**. That makes canonicalization time-dependent: - At boot the agent seeds sources from `CODER_AGENT_EXP_*_DIRS`. If `~/.coder/skills -> ~/my-agent/agent-rules/skills` and the target does not exist yet (a startup script creates it later), `~/.coder/skills` canonicalizes to the lexical `/home/coder/.coder/skills`. - After the manifest lands (or the target is added directly), the same configured source canonicalizes to the resolved `/home/coder/my-agent/agent-rules/skills`. The same configured source produced two different strings, so dedupe keyed on the string registered both and the list showed one directory twice. Resolving symlinks for identity is also misleading on its own (per @mafredri's review): a source added by a symlink path appears in the list as its resolved target, as if that target had been added explicitly. ## How Source identity is now the **lexical** path: cleaned, `~`-expanded, absolute, with symlinks **not** resolved (new `lexicalPath`; `CanonicalizePath` is refactored to build on it). `AddSource`, `SeedSources`, `HasSource`, `RemoveSource`, and boot seeding all key on this stable identity. `AddSource` still **validates** the resolved (`CanonicalizePath`) path against the allowed roots, so a symlink cannot escape them. Only the identity/display path changed. This replaces the earlier `os.SameFile`/inode dedupe, which was unstable and failed on Windows runners. ## Testing - `go test ./agent/agentcontext/` (full package) and `go vet` pass; `gofmt` clean. - `TestManager_SourceIdentityIsLexicalAndStable`: adds the same symlinked source before and after its target exists and asserts one source whose path is the lexical link (skipped on Windows, matching the package's other symlink tests). - Existing `TestCanonicalizePath_FollowsSymlinks` and `TestValidateSourcePath_*` confirm symlink resolution and the security boundary are unchanged. <details> <summary>Related review findings</summary> Fixes the "duplicate symlinked paths in `context list`" issue from the chat-context system review and the dedupe-ordering question (lexical identity preserves first-come-first-served order). Showing the configured path for **resources** (not just sources) and restoring scope-based skill precedence are separate, larger changes tracked elsewhere. </details> --- *This PR was created by Coder Agents on behalf of @kylecarbs.*
498 lines
14 KiB
Go
498 lines
14 KiB
Go
package agentcontext_test
|
|
|
|
import (
|
|
"context"
|
|
"os"
|
|
"path/filepath"
|
|
"runtime"
|
|
"sync/atomic"
|
|
"testing"
|
|
"time"
|
|
|
|
"github.com/stretchr/testify/require"
|
|
|
|
"github.com/coder/coder/v2/agent/agentcontext"
|
|
"github.com/coder/coder/v2/testutil"
|
|
)
|
|
|
|
// TestMain points the test binary's HOME (and USERPROFILE on
|
|
// Windows) at a fresh empty directory before any test runs.
|
|
// The package's built-in scan roots (~/.coder,
|
|
// ~/.coder/skills, ~/.claude/plugins/cache) canonicalize
|
|
// against this directory, so they resolve to non-existent
|
|
// paths and the resolver silently skips them. Without this,
|
|
// running the tests on a developer host pulls real Coder and
|
|
// Claude config files into snapshots and breaks every
|
|
// Len(Resources, N) assertion.
|
|
func TestMain(m *testing.M) {
|
|
home, err := os.MkdirTemp("", "agentcontext-test-home-")
|
|
if err != nil {
|
|
panic(err)
|
|
}
|
|
if err := os.Setenv("HOME", home); err != nil {
|
|
panic(err)
|
|
}
|
|
if runtime.GOOS == "windows" {
|
|
if err := os.Setenv("USERPROFILE", home); err != nil {
|
|
panic(err)
|
|
}
|
|
}
|
|
code := m.Run()
|
|
_ = os.RemoveAll(home)
|
|
os.Exit(code)
|
|
}
|
|
|
|
func newTestManager(t *testing.T, opts agentcontext.ManagerOptions) *agentcontext.Manager {
|
|
t.Helper()
|
|
opts.Logger = testutil.Logger(t).Named("agentcontext-test")
|
|
m := agentcontext.NewManager(opts)
|
|
t.Cleanup(func() { _ = m.Close() })
|
|
return m
|
|
}
|
|
|
|
func TestManager_InitialSnapshotIsPopulated(t *testing.T) {
|
|
t.Parallel()
|
|
dir := t.TempDir()
|
|
mustWriteFile(t, filepath.Join(dir, "AGENTS.md"), "boot snapshot")
|
|
|
|
m := newTestManager(t, agentcontext.ManagerOptions{
|
|
WorkingDir: func() string { return dir },
|
|
})
|
|
|
|
snap := m.Snapshot()
|
|
require.Equal(t, uint64(1), snap.Version)
|
|
require.Len(t, snap.Resources, 1)
|
|
}
|
|
|
|
func TestManager_AddSourceTriggersResolve(t *testing.T) {
|
|
t.Parallel()
|
|
wd := testutil.TempDirResolved(t)
|
|
src := testutil.TempDirResolved(t)
|
|
mustWriteFile(t, filepath.Join(src, "AGENTS.md"), "from source")
|
|
|
|
m := newTestManager(t, agentcontext.ManagerOptions{
|
|
WorkingDir: func() string { return wd },
|
|
AllowedRoots: []string{wd, src},
|
|
})
|
|
|
|
ctx := testutil.Context(t, testutil.WaitLong)
|
|
go func() { _ = m.Run(ctx) }()
|
|
|
|
t.Cleanup(func() { _ = m.Close() })
|
|
|
|
// Subscribe before mutating so we observe the broadcast.
|
|
ch, unsub := m.SubscribeChanges()
|
|
defer unsub()
|
|
|
|
added, err := m.AddSource(agentcontext.Source{Path: src})
|
|
require.NoError(t, err)
|
|
require.Equal(t, src, added.Path)
|
|
|
|
select {
|
|
case <-ch:
|
|
case <-time.After(testutil.WaitShort):
|
|
t.Fatalf("expected a change broadcast after AddSource")
|
|
}
|
|
|
|
snap := m.Snapshot()
|
|
require.Greater(t, snap.Version, uint64(1))
|
|
|
|
found := false
|
|
for _, r := range snap.Resources {
|
|
if r.Kind == agentcontext.KindInstructionFile && r.SourcePath == src {
|
|
found = true
|
|
}
|
|
}
|
|
require.True(t, found, "expected AGENTS.md attributed to the user source")
|
|
}
|
|
|
|
func TestManager_AddSourceRejectsOutsideAllowedRoots(t *testing.T) {
|
|
t.Parallel()
|
|
wd := t.TempDir()
|
|
outside := t.TempDir()
|
|
|
|
m := newTestManager(t, agentcontext.ManagerOptions{
|
|
WorkingDir: func() string { return wd },
|
|
AllowedRoots: []string{wd},
|
|
})
|
|
|
|
_, err := m.AddSource(agentcontext.Source{Path: outside})
|
|
require.Error(t, err)
|
|
}
|
|
|
|
// TestManager_AddSourceAcceptsLateWorkingDir mirrors the agent's
|
|
// real boot order: AllowedRoots is configured before the
|
|
// manifest provides the workspace working directory. The Manager
|
|
// must consult WorkingDir on every check so paths under the
|
|
// resolved working dir validate once the manifest lands.
|
|
func TestManager_AddSourceAcceptsLateWorkingDir(t *testing.T) {
|
|
t.Parallel()
|
|
wd := t.TempDir()
|
|
var resolved atomic.Pointer[string]
|
|
m := newTestManager(t, agentcontext.ManagerOptions{
|
|
WorkingDir: func() string {
|
|
if p := resolved.Load(); p != nil {
|
|
return *p
|
|
}
|
|
return ""
|
|
},
|
|
AllowedRoots: []string{"/never-used-home"},
|
|
})
|
|
|
|
// Before the manifest "loads", workingDir is empty; sources
|
|
// under wd must be rejected.
|
|
_, err := m.AddSource(agentcontext.Source{Path: wd})
|
|
require.Error(t, err)
|
|
|
|
// After the manifest "loads", workingDir resolves and the
|
|
// same path validates without restarting the Manager.
|
|
resolved.Store(&wd)
|
|
_, err = m.AddSource(agentcontext.Source{Path: wd})
|
|
require.NoError(t, err)
|
|
}
|
|
|
|
func TestManager_AddSourceIsIdempotent(t *testing.T) {
|
|
t.Parallel()
|
|
wd := t.TempDir()
|
|
src := t.TempDir()
|
|
|
|
m := newTestManager(t, agentcontext.ManagerOptions{
|
|
WorkingDir: func() string { return wd },
|
|
AllowedRoots: []string{wd, src},
|
|
})
|
|
|
|
added1, err := m.AddSource(agentcontext.Source{Path: src})
|
|
require.NoError(t, err)
|
|
added2, err := m.AddSource(agentcontext.Source{Path: src})
|
|
require.NoError(t, err)
|
|
require.Equal(t, added1.Path, added2.Path)
|
|
|
|
sources := m.Sources()
|
|
require.Len(t, sources, 1)
|
|
}
|
|
|
|
// TestManager_SourceIdentityIsLexicalAndStable verifies the same configured
|
|
// source added before and after its symlink target exists collapses to one
|
|
// source keyed by the lexical (configured) path, not the resolved target.
|
|
func TestManager_SourceIdentityIsLexicalAndStable(t *testing.T) {
|
|
t.Parallel()
|
|
if runtime.GOOS == "windows" {
|
|
t.Skip("symlinks require admin privileges on Windows runners")
|
|
}
|
|
root := testutil.TempDirResolved(t)
|
|
target := filepath.Join(root, "target")
|
|
link := filepath.Join(root, "link")
|
|
require.NoError(t, os.Symlink(target, link))
|
|
|
|
m := newTestManager(t, agentcontext.ManagerOptions{
|
|
WorkingDir: func() string { return root },
|
|
AllowedRoots: []string{root},
|
|
})
|
|
|
|
// Target missing: identity is the lexical link path.
|
|
added1, err := m.AddSource(agentcontext.Source{Path: link})
|
|
require.NoError(t, err)
|
|
require.Equal(t, link, added1.Path)
|
|
|
|
// Once the target exists the link resolves, but the same configured
|
|
// path must still dedupe to one source.
|
|
require.NoError(t, os.MkdirAll(target, 0o755))
|
|
added2, err := m.AddSource(agentcontext.Source{Path: link})
|
|
require.NoError(t, err)
|
|
require.Equal(t, link, added2.Path,
|
|
"expected the source identity to stay the lexical link path")
|
|
|
|
require.Len(t, m.Sources(), 1)
|
|
}
|
|
|
|
func TestManager_RemoveSource(t *testing.T) {
|
|
t.Parallel()
|
|
wd := t.TempDir()
|
|
src := t.TempDir()
|
|
|
|
m := newTestManager(t, agentcontext.ManagerOptions{
|
|
WorkingDir: func() string { return wd },
|
|
AllowedRoots: []string{wd, src},
|
|
})
|
|
|
|
_, err := m.AddSource(agentcontext.Source{Path: src})
|
|
require.NoError(t, err)
|
|
require.NoError(t, m.RemoveSource(src))
|
|
require.Empty(t, m.Sources())
|
|
|
|
err = m.RemoveSource(src)
|
|
require.ErrorIs(t, err, agentcontext.ErrSourceNotFound)
|
|
}
|
|
|
|
func TestManager_HasSource(t *testing.T) {
|
|
t.Parallel()
|
|
wd := t.TempDir()
|
|
src := testutil.TempDirResolved(t)
|
|
|
|
m := newTestManager(t, agentcontext.ManagerOptions{
|
|
WorkingDir: func() string { return wd },
|
|
AllowedRoots: []string{wd, src},
|
|
})
|
|
|
|
canonical, ok := m.HasSource(src)
|
|
require.False(t, ok)
|
|
require.Equal(t, src, canonical)
|
|
|
|
_, err := m.AddSource(agentcontext.Source{Path: src})
|
|
require.NoError(t, err)
|
|
|
|
canonical, ok = m.HasSource(src)
|
|
require.True(t, ok)
|
|
require.Equal(t, src, canonical)
|
|
}
|
|
|
|
func TestManager_ResyncReturnsLatestSnapshot(t *testing.T) {
|
|
t.Parallel()
|
|
wd := t.TempDir()
|
|
mustWriteFile(t, filepath.Join(wd, "AGENTS.md"), "first")
|
|
|
|
m := newTestManager(t, agentcontext.ManagerOptions{
|
|
WorkingDir: func() string { return wd },
|
|
})
|
|
|
|
ctx := testutil.Context(t, testutil.WaitLong)
|
|
runDone := make(chan struct{})
|
|
go func() {
|
|
defer close(runDone)
|
|
_ = m.Run(ctx)
|
|
}()
|
|
t.Cleanup(func() {
|
|
_ = m.Close()
|
|
<-runDone
|
|
})
|
|
|
|
// Mutate AGENTS.md and call Resync. The returned
|
|
// snapshot must reflect the new content.
|
|
require.NoError(t, os.WriteFile(filepath.Join(wd, "AGENTS.md"), []byte("second content edit"), 0o600))
|
|
|
|
snap, err := m.Resync(ctx)
|
|
require.NoError(t, err)
|
|
|
|
require.Len(t, snap.Resources, 1)
|
|
require.Equal(t, "second content edit", string(snap.Resources[0].Payload))
|
|
}
|
|
|
|
// TestManager_ResyncCanceledKeepsLiveSnapshot guards CRF-44:
|
|
// a context cancellation mid-walk must not replace the live
|
|
// Snapshot with an empty one. Resync returns the existing
|
|
// Snapshot and ctx.Err() instead of publishing a stub.
|
|
func TestManager_ResyncCanceledKeepsLiveSnapshot(t *testing.T) {
|
|
t.Parallel()
|
|
wd := t.TempDir()
|
|
mustWriteFile(t, filepath.Join(wd, "AGENTS.md"), "live content")
|
|
|
|
m := newTestManager(t, agentcontext.ManagerOptions{
|
|
WorkingDir: func() string { return wd },
|
|
})
|
|
|
|
// Capture the live snapshot the Manager populated at
|
|
// construction time.
|
|
live := m.Snapshot()
|
|
require.Len(t, live.Resources, 1)
|
|
require.Equal(t, "live content", string(live.Resources[0].Payload))
|
|
|
|
// Cancel the context before calling Resync so
|
|
// ResolveContext observes the cancellation.
|
|
ctx, cancel := context.WithCancel(context.Background())
|
|
cancel()
|
|
|
|
snap, err := m.Resync(ctx)
|
|
require.ErrorIs(t, err, context.Canceled)
|
|
// The returned snapshot must still expose the live
|
|
// resources, not an empty result from the canceled walk.
|
|
require.Len(t, snap.Resources, 1)
|
|
require.Equal(t, "live content", string(snap.Resources[0].Payload))
|
|
|
|
// The next Snapshot call must also return live content;
|
|
// no stub was published.
|
|
after := m.Snapshot()
|
|
require.Equal(t, live.Version, after.Version)
|
|
require.Len(t, after.Resources, 1)
|
|
}
|
|
|
|
func TestManager_InitialSourcesSeeded(t *testing.T) {
|
|
t.Parallel()
|
|
wd := t.TempDir()
|
|
src := testutil.TempDirResolved(t)
|
|
mustWriteFile(t, filepath.Join(src, "AGENTS.md"), "from initial")
|
|
|
|
m := newTestManager(t, agentcontext.ManagerOptions{
|
|
WorkingDir: func() string { return wd },
|
|
AllowedRoots: []string{wd, src},
|
|
InitialSources: []agentcontext.Source{{Path: src}},
|
|
})
|
|
|
|
sources := m.Sources()
|
|
require.Len(t, sources, 1)
|
|
require.Equal(t, src, sources[0].Path)
|
|
|
|
snap := m.Snapshot()
|
|
require.Len(t, snap.Resources, 1)
|
|
require.Equal(t, src, snap.Resources[0].SourcePath)
|
|
}
|
|
|
|
// TestManager_SeedSourcesLateBindsAfterManifest models the
|
|
// agent's behavior when CODER_AGENT_EXP_*_DIRS contains a
|
|
// relative path that cannot resolve until the manifest's
|
|
// working directory lands. SeedSources must adopt the
|
|
// previously-unresolvable path, bypass AllowedRoots
|
|
// validation, and trigger a re-resolve.
|
|
func TestManager_SeedSourcesLateBindsAfterManifest(t *testing.T) {
|
|
t.Parallel()
|
|
wd := t.TempDir()
|
|
late := testutil.TempDirResolved(t)
|
|
mustWriteFile(t, filepath.Join(late, "AGENTS.md"), "late binding")
|
|
|
|
// AllowedRoots intentionally omits `late` so AddSource
|
|
// would reject it. SeedSources must accept it anyway,
|
|
// since the path comes from the trusted template config.
|
|
m := newTestManager(t, agentcontext.ManagerOptions{
|
|
WorkingDir: func() string { return wd },
|
|
AllowedRoots: []string{wd},
|
|
})
|
|
|
|
require.Empty(t, m.Sources())
|
|
|
|
m.SeedSources([]agentcontext.Source{{Path: late}})
|
|
|
|
sources := m.Sources()
|
|
require.Len(t, sources, 1)
|
|
require.Equal(t, late, sources[0].Path)
|
|
|
|
snap, err := m.Resync(testutil.Context(t, testutil.WaitShort))
|
|
require.NoError(t, err)
|
|
require.Len(t, snap.Resources, 1)
|
|
require.Equal(t, late, snap.Resources[0].SourcePath)
|
|
}
|
|
|
|
func TestManager_CloseIsIdempotent(t *testing.T) {
|
|
t.Parallel()
|
|
m := newTestManager(t, agentcontext.ManagerOptions{
|
|
WorkingDir: func() string { return t.TempDir() },
|
|
})
|
|
require.NoError(t, m.Close())
|
|
require.NoError(t, m.Close())
|
|
}
|
|
|
|
func TestManager_RunOnce(t *testing.T) {
|
|
t.Parallel()
|
|
m := newTestManager(t, agentcontext.ManagerOptions{
|
|
WorkingDir: func() string { return t.TempDir() },
|
|
})
|
|
ctx, cancel := context.WithCancel(testutil.Context(t, testutil.WaitShort))
|
|
defer cancel()
|
|
go func() { _ = m.Run(ctx) }()
|
|
|
|
// Wait for Run to claim the running flag, then verify the
|
|
// second call rejects with a deterministic error rather than
|
|
// racing the scheduler.
|
|
select {
|
|
case <-agentcontext.ManagerStarted(m):
|
|
case <-ctx.Done():
|
|
t.Fatalf("manager never started: %v", ctx.Err())
|
|
}
|
|
|
|
err := m.Run(ctx)
|
|
require.Error(t, err)
|
|
require.Contains(t, err.Error(), "more than once")
|
|
cancel()
|
|
_ = m.Close()
|
|
}
|
|
|
|
func TestManager_SubscribeBroadcastOnChange(t *testing.T) {
|
|
t.Parallel()
|
|
wd := t.TempDir()
|
|
src := t.TempDir()
|
|
|
|
m := newTestManager(t, agentcontext.ManagerOptions{
|
|
WorkingDir: func() string { return wd },
|
|
AllowedRoots: []string{wd, src},
|
|
})
|
|
|
|
ctx := testutil.Context(t, testutil.WaitLong)
|
|
go func() { _ = m.Run(ctx) }()
|
|
|
|
ch, unsub := m.SubscribeChanges()
|
|
defer unsub()
|
|
|
|
_, err := m.AddSource(agentcontext.Source{Path: src})
|
|
require.NoError(t, err)
|
|
|
|
select {
|
|
case <-ch:
|
|
case <-time.After(testutil.WaitShort):
|
|
t.Fatal("expected subscriber to be notified")
|
|
}
|
|
}
|
|
|
|
// TestManager_MCPResourcesAppliesToSnapshot verifies that MCP resources
|
|
// supplied via the resolver contribute KindMCPServer resources (with
|
|
// their tools) to the resolved snapshot.
|
|
func TestManager_MCPResourcesAppliesToSnapshot(t *testing.T) {
|
|
t.Parallel()
|
|
dir := t.TempDir()
|
|
|
|
m := newTestManager(t, agentcontext.ManagerOptions{
|
|
WorkingDir: func() string { return dir },
|
|
Resolver: &agentcontext.Resolver{
|
|
MCPResources: func() []agentcontext.Resource {
|
|
return []agentcontext.Resource{{
|
|
ID: "mcp_server:fs",
|
|
Kind: agentcontext.KindMCPServer,
|
|
Source: "fs",
|
|
Name: "fs",
|
|
Status: agentcontext.StatusOK,
|
|
Tools: []agentcontext.MCPTool{{Name: "read", Description: "Read"}},
|
|
}}
|
|
},
|
|
},
|
|
})
|
|
|
|
snap := m.Snapshot()
|
|
var found bool
|
|
for _, r := range snap.Resources {
|
|
if r.Kind == agentcontext.KindMCPServer && r.Source == "fs" {
|
|
found = true
|
|
require.Len(t, r.Tools, 1)
|
|
require.Equal(t, "read", r.Tools[0].Name)
|
|
}
|
|
}
|
|
require.True(t, found, "expected MCP server resource in snapshot")
|
|
}
|
|
|
|
// TestManager_WorkingDirScannedShallow confirms the working
|
|
// directory is a single scan root: its top-level instruction files
|
|
// are read, but the resolver neither climbs to an ancestor (no
|
|
// walk-up to a .git project root) nor descends into subdirectories.
|
|
func TestManager_WorkingDirScannedShallow(t *testing.T) {
|
|
t.Parallel()
|
|
root := testutil.TempDirResolved(t)
|
|
require.NoError(t, os.MkdirAll(filepath.Join(root, ".git"), 0o755))
|
|
mustWriteFile(t, filepath.Join(root, "AGENTS.md"), "root rules")
|
|
cwd := filepath.Join(root, "service")
|
|
require.NoError(t, os.MkdirAll(cwd, 0o755))
|
|
mustWriteFile(t, filepath.Join(cwd, "AGENTS.md"), "service rules")
|
|
// A subdirectory below the working dir must not be descended.
|
|
mustWriteFile(t, filepath.Join(cwd, "nested", "AGENTS.md"), "nested rules")
|
|
|
|
m := newTestManager(t, agentcontext.ManagerOptions{
|
|
WorkingDir: func() string { return cwd },
|
|
})
|
|
|
|
snap := m.Snapshot()
|
|
var sources []string
|
|
for _, r := range snap.Resources {
|
|
if r.Kind == agentcontext.KindInstructionFile {
|
|
sources = append(sources, r.Source)
|
|
}
|
|
}
|
|
// Only the working directory's own AGENTS.md is present: the
|
|
// ancestor root and the nested subdirectory are both excluded.
|
|
require.Equal(t, []string{filepath.Join(cwd, "AGENTS.md")}, sources)
|
|
}
|