mirror of
https://github.com/coder/coder.git
synced 2026-06-02 20:48:20 +00:00
fix(agent/agentcontext): address coder-agents-review round 5
CRF-44: bail without publishing when ctx is canceled mid-walk in both Resync and resolveAndBroadcast. ResolveContext returns an empty Snapshot when its ctx is canceled; publishing it would replace the live Snapshot with empty resources until the next trigger. Resync now returns the existing Snapshot and ctx.Err() instead. Added regression test TestManager_ResyncCanceledKeepsLiveSnapshot. CRF-43: unexport Manager.Started to started; the method has no production callers. Tests reach it through ManagerStarted in export_test.go. CRF-45: drop the inaccurate Unix newline conversion clause from readMCPConfig's doc comment. CRF-46: AllowedRoots doc clarified that an empty slice falls back to the home directory rather than disabling validation.
This commit is contained in:
@@ -0,0 +1,7 @@
|
||||
package agentcontext
|
||||
|
||||
// ManagerStarted exposes the unexported started() channel for
|
||||
// use by external _test packages. Production code does not need
|
||||
// this signal; the agent calls Run synchronously after wiring
|
||||
// the Manager. Tests use it to coordinate without polling.
|
||||
func ManagerStarted(m *Manager) <-chan struct{} { return m.started() }
|
||||
@@ -40,7 +40,7 @@ type ManagerOptions struct {
|
||||
InitialSources []Source
|
||||
// AllowedRoots restricts which paths may be added as
|
||||
// sources at runtime. Defaults to [~, ~/.coder, ~/.claude,
|
||||
// workingDir]. Empty disables validation.
|
||||
// workingDir]. Empty falls back to the home directory.
|
||||
AllowedRoots []string
|
||||
// Resolver, when non-nil, replaces the default resolver.
|
||||
// Tests use this to inject MCP providers and tighten
|
||||
@@ -232,11 +232,11 @@ func (m *Manager) Run(ctx context.Context) error {
|
||||
}
|
||||
}
|
||||
|
||||
// Started returns a channel that is closed once Run has
|
||||
// claimed the running flag. Callers waiting to coordinate with
|
||||
// the watcher loop can select on it; a closed channel never
|
||||
// started returns a channel that is closed once Run has
|
||||
// claimed the running flag. Tests use it to coordinate with
|
||||
// the watcher loop without polling; a closed channel never
|
||||
// blocks, so this is safe to call repeatedly.
|
||||
func (m *Manager) Started() <-chan struct{} {
|
||||
func (m *Manager) started() <-chan struct{} {
|
||||
return m.runStartedCh
|
||||
}
|
||||
|
||||
@@ -405,6 +405,14 @@ func (m *Manager) Resync(ctx context.Context) (Snapshot, error) {
|
||||
return m.Snapshot(), ctxErr
|
||||
}
|
||||
snap := resolver.ResolveContext(ctx, roots)
|
||||
if ctxErr := ctx.Err(); ctxErr != nil {
|
||||
// Cancellation mid-walk yields a partial or empty
|
||||
// Snapshot whose SnapshotError is set to
|
||||
// "context canceled". Publishing it would replace
|
||||
// the live Snapshot with empty resources until the
|
||||
// next trigger, so bail without touching state.
|
||||
return m.Snapshot(), ctxErr
|
||||
}
|
||||
if snap.SnapshotError == "" && watcher != nil {
|
||||
if d := watcher.Degraded(); d != "" {
|
||||
snap.SnapshotError = d
|
||||
@@ -545,6 +553,15 @@ func (m *Manager) resolveAndBroadcast(ctx context.Context) {
|
||||
return
|
||||
}
|
||||
snap := resolver.ResolveContext(ctx, roots)
|
||||
if err := ctx.Err(); err != nil {
|
||||
// Cancellation mid-walk yields a partial or empty
|
||||
// Snapshot. Publishing it would replace the live
|
||||
// Snapshot with empty resources, so bail without
|
||||
// touching state. The Run loop's gracefulCtx is
|
||||
// canceled only at shutdown, but defensive checks
|
||||
// keep the publish contract uniform with Resync.
|
||||
return
|
||||
}
|
||||
// Surface watcher degradation as a snapshot-level error
|
||||
// when the resolver did not already emit one.
|
||||
if snap.SnapshotError == "" && watcher != nil {
|
||||
|
||||
@@ -217,6 +217,44 @@ func TestManager_ResyncReturnsLatestSnapshot(t *testing.T) {
|
||||
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()
|
||||
@@ -260,7 +298,7 @@ func TestManager_RunOnce(t *testing.T) {
|
||||
// second call rejects with a deterministic error rather than
|
||||
// racing the scheduler.
|
||||
select {
|
||||
case <-m.Started():
|
||||
case <-agentcontext.ManagerStarted(m):
|
||||
case <-ctx.Done():
|
||||
t.Fatalf("manager never started: %v", ctx.Err())
|
||||
}
|
||||
|
||||
@@ -385,9 +385,8 @@ func (r *Resolver) readInstructionFile(path string, info fs.FileInfo, userSource
|
||||
|
||||
// readMCPConfig reads a .mcp.json file and produces a
|
||||
// KindMCPConfig resource. Parsing is left to consumers; the
|
||||
// resolver only enforces JSON shape lightly via size and Unix
|
||||
// newline conversion. Future work: detect malformed JSON and
|
||||
// surface StatusInvalid.
|
||||
// resolver only enforces the per-resource size cap. Future
|
||||
// work: detect malformed JSON and surface StatusInvalid.
|
||||
func (r *Resolver) readMCPConfig(path string, info fs.FileInfo, userSource string) Resource {
|
||||
return r.readFileResource(KindMCPConfig, path, info, userSource)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user