feat: v0.7 shared experts + multi-pool foundations#18
Merged
Conversation
Returns ~/.agent-pool/experts/{name}/ with filename validation.
Foundation for v0.7 shared experts — pure addition, no existing behavior changed.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Three new functions for shared expert directories:
- ResolveSharedExpertDir → ~/.agent-pool/experts/{name}/
- ResolveSharedInbox → {poolDir}/shared-state/{name}/inbox/
- ResolveSharedLogDir → {poolDir}/shared-state/{name}/logs/
Pure additions — existing ResolveExpertDir/ResolveInbox unchanged.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Validates shared.include entries don't conflict with builtin roles or pool-scoped experts, and are filename-safe. Called at end of LoadPool. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ensureDirs now creates {poolDir}/shared-state/{name}/{inbox,logs} for each
shared.include entry. isSharedExpert checks membership in the list.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
SpawnConfig gains OverlayDir for pool-scoped project state overlay. AssemblePrompt reads overlay state.md and emits "## Project State" section between user-level state and errors. Empty OverlayDir is a no-op, preserving backward compatibility for pool-scoped experts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Major changes:
- processInboxMessage detects shared experts, sets ExpertDir to user-level
and OverlayDir to pool-scoped shared-state dir
- Logs written to pool-scoped shared-state/{name}/ for shared experts
- Route gains sharedNames param to route to shared-state inboxes
- drainInbox, drainAllInboxes, handleCancel resolve shared inbox paths
- resolveExpertName matches shared inbox paths
- Run() watches shared expert inboxes alongside pool-scoped ones
Merges Phase 2.2 and 2.3 since Route + spawn wiring are tightly coupled.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…gShared ServerConfig gains IsShared and SharedOverlayDir for shared expert MCP. WriteTempConfigShared passes --shared true in MCP args. cmdMCP parses the flag. Daemon uses WriteTempConfigShared for shared experts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
update_state gains optional scope param (default "project"): - Shared expert + scope=project → writes to SharedOverlayDir/state.md - Shared expert + scope=user → writes to user-level expertDir/state.md - Pool-scoped expert → scope ignored, writes to expertDir as before read_state for shared experts returns project_state field from overlay. RegisterExpertTools resolves expertDir via ResolveSharedExpertDir when IsShared is true. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
FlushConfig gains IsShared — resolves user-level state dir when set. cmdFlush parses --shared flag. readExpertResult falls back to shared-state dir when pool-scoped log lookup fails. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Non-fatal warning during ensureDirs when a shared.include entry has
no identity.md at ~/.agent-pool/experts/{name}/. Helps catch
misconfigured shared expert references early.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Approved plan covering 4 phases: shared expert resolution, layered prompt assembly, scoped state writes, and startup validation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR implements the "Shared Experts" feature (v0.7), enabling experts to share identity, state, logs, and prompts across multiple pools. Changes span shared expert path resolution utilities, pool config validation, daemon mailbox and spawn logic updates, MCP server/tool extensions with scope-aware state semantics, a new Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI / cmdMCP
participant Daemon as Daemon
participant MailRouter as Mail Router
participant Expert as Expert Process
participant MCP as MCP Server
participant FS as File System
CLI->>Daemon: Task for shared expert<br/>(via postoffice)
Daemon->>MailRouter: Route(sharedNames={expert:true})
MailRouter->>FS: Write to shared inbox<br/>(poolDir/shared-state/...)
Daemon->>Daemon: Poll shared inbox
Daemon->>MailRouter: ResolveSharedExpertDir(name)
MailRouter->>FS: Return user-level dir<br/>(~/.agent-pool/experts/...)
Daemon->>FS: Read identity.md from user dir
Daemon->>Expert: Spawn with OverlayDir<br/>(poolDir/shared-state/...)
Expert->>MCP: Initialize with shared config
MCP->>FS: read_state → user identity +<br/>overlay project_state
MCP->>FS: update_state(scope=project)<br/>→ poolDir/shared-state/.../state.md
Expert->>FS: Write logs to pool-scoped<br/>shared-state/logs/
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/mcp/server.go (1)
28-37: 🧹 Nitpick | 🔵 TrivialConsider validating
SharedOverlayDirwhenIsSharedis true.The
Validate()method doesn't enforce thatSharedOverlayDiris set whenIsShared=true. IfIsSharedis true butSharedOverlayDiris empty, downstream tool handlers may behave unexpectedly.♻️ Suggested validation enhancement
func (c *ServerConfig) Validate() error { if c.PoolDir == "" { return fmt.Errorf("pool directory is required") } if c.ExpertName == "" { return fmt.Errorf("expert name is required") } + if c.IsShared && c.SharedOverlayDir == "" { + return fmt.Errorf("shared overlay directory is required for shared experts") + } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/server.go` around lines 28 - 37, Update ServerConfig.Validate to also require SharedOverlayDir when IsShared is true: inside the Validate method (ServerConfig.Validate) after the existing PoolDir/ExpertName checks, add a conditional that if c.IsShared && c.SharedOverlayDir == "" then return an error (e.g., fmt.Errorf("shared overlay directory is required when IsShared is true")). This ensures SharedOverlayDir is validated alongside PoolDir and ExpertName.internal/mail/router_test.go (1)
39-87: 🧹 Nitpick | 🔵 TrivialConsider adding a test for
Routewith non-nilsharedNames.The existing tests all pass
nilforsharedNames. A test verifying that messages to shared experts are routed toshared-state/{name}/inbox/would provide coverage for the new routing branch.🧪 Suggested test case
func TestRoute_SharedExpert(t *testing.T) { poolDir := t.TempDir() postoffice := filepath.Join(poolDir, "postoffice") sharedInbox := filepath.Join(poolDir, "shared-state", "security-standards", "inbox") for _, dir := range []string{postoffice, sharedInbox} { if err := os.MkdirAll(dir, 0o755); err != nil { t.Fatal(err) } } msgContent := `--- id: task-shared-001 from: architect to: security-standards type: task timestamp: 2026-04-01T14:32:00Z --- Shared expert task. ` srcPath := filepath.Join(postoffice, "task-shared-001.md") if err := os.WriteFile(srcPath, []byte(msgContent), 0o644); err != nil { t.Fatal(err) } logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelWarn})) sharedNames := map[string]bool{"security-standards": true} msg, err := mail.Route(logger, poolDir, srcPath, sharedNames) if err != nil { t.Fatalf("Route failed: %v", err) } if msg.ID != "task-shared-001" { t.Errorf("msg.ID = %q, want %q", msg.ID, "task-shared-001") } // Verify file appeared in shared inbox destPath := filepath.Join(sharedInbox, "task-shared-001.md") if _, err := os.Stat(destPath); os.IsNotExist(err) { t.Error("routed file not found in shared inbox") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mail/router_test.go` around lines 39 - 87, Add a new test that exercises the Route function's shared-expert branch by calling mail.Route with a non-nil sharedNames map containing the target expert name (e.g., "security-standards"); create the poolDir/postoffice and poolDir/shared-state/{name}/inbox directories, write a message addressed to that shared expert, call mail.Route(logger, poolDir, srcPath, sharedNames) and assert msg.ID and that the file was moved into shared-state/{name}/inbox/{id}.md (and original removed). Reference the existing TestRoute_EndToEnd pattern and use the same logger and file perms while passing a non-nil sharedNames map to trigger the shared-state routing path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plans/2026-04-05-shared-experts.md`:
- Around line 1-239: In the docs/plans "v0.7 Shared Experts Implementation Plan"
update: change "built in" to hyphenated "built-in" (near the Design decisions /
Config validation discussion) and reword the two consecutive sentences that
start with "Also" (around the Watcher + inbox routing section) to vary sentence
openings and improve flow; apply these small copy edits in that markdown so
wording is consistent and the repeated "Also" isn't used twice in succession.
In `@internal/config/config.go`:
- Around line 82-88: builtinRoleNames duplicates mail.builtinRoles leading to
potential drift; change to use a single source by removing the local
builtinRoleNames and referencing the exported check from the mail package (e.g.,
use mail.IsBuiltinRole or mail.BuiltinRoles) inside Validate(), or alternatively
export the canonical list from config and import it into mail so there's one
definition; if importing mail into config would create a circular dependency,
add a compile-time assertion or unit test that compares config.builtinRoleNames
with mail.builtinRoles at build/test time and update the Validate() function to
call the single canonical helper (Validate() and
builtinRoleNames/mail.IsBuiltinRole are the key symbols to adjust).
In `@internal/daemon/daemon_test.go`:
- Around line 301-303: The test setup currently ignores errors from os.MkdirAll
and os.WriteFile when creating sharedExpertDir and identity.md; update the calls
around sharedExpertDir (constructed with filepath.Join) to check their returned
errors and fail the test on error (e.g., use t.Fatalf or t.Fatal) so failures in
os.MkdirAll or os.WriteFile surface instead of being discarded.
- Around line 296-298: Replace the manual HOME env restore (origHome :=
os.Getenv("HOME"); os.Setenv("HOME", fakeHome); t.Cleanup(func() {
os.Setenv("HOME", origHome) })) with the testing helper t.Setenv("HOME",
fakeHome); remove origHome and the t.Cleanup block so the test uses t.Setenv to
set and automatically restore HOME (locate usages around origHome, fakeHome and
the os.Setenv/os.Getenv calls in the test).
In `@internal/daemon/daemon.go`:
- Around line 1248-1256: isSharedExpert currently does a linear scan over
d.cfg.Shared.Include; change it to call d.sharedNamesMap() and perform a map
lookup for O(1) existence checks. Specifically, in Daemon.isSharedExpert use the
map returned by sharedNamesMap() (which may be nil) and return m[name] instead
of iterating; this leverages the existing helper and avoids repeated linear
scans over cfg.Shared.Include.
In `@internal/mcp/tools_test.go`:
- Around line 356-388: Replace the manual HOME env manipulation in
TestUpdateState_SharedExpert_UserScope with t.Setenv: instead of capturing
origHome, calling os.Setenv and t.Cleanup, call t.Setenv("HOME", fakeHome) at
the start of the test (keeping creation of userExpertDir and the rest
unchanged); update references in the test surrounding setupSharedExpertPool and
buildSharedMCPTestServer as needed so the environment is set before those calls
and remove the manual os.Setenv cleanup code.
- Around line 364-365: The call to os.MkdirAll for userExpertDir discards the
returned error; update the test in tools_test.go to check the error and fail the
test on failure (e.g., capture the error := os.MkdirAll(userExpertDir, 0o755)
and call t.Fatalf or use require.NoError(t, err)) so test setup failures are not
masked; locate the os.MkdirAll(userExpertDir, 0o755) invocation and add the
error check using the test helper in the file.
In `@internal/mcp/tools.go`:
- Around line 157-161: When handling the "project" scope, add a defensive guard
that verifies cfg.SharedOverlayDir is non-empty when cfg.IsShared (or when scope
== "project") before calling expert.WriteState; if it's empty return a clear
mcp.NewToolResultError (e.g., "shared overlay dir not configured") instead of
calling expert.WriteState with an empty path. Update the branch that currently
calls expert.WriteState(cfg.SharedOverlayDir, content) to first check
cfg.SharedOverlayDir == "" and return an error via mcp.NewToolResultError to
avoid writing to an unexpected location.
- Around line 19-31: The function resolveExpertDirForMCP silently falls back to
pool-scoped path when mail.ResolveSharedExpertDir returns an error; update
resolveExpertDirForMCP to surface that failure by either (A) adding a logger
parameter and logging a warning (including the error and the fallback path)
before returning mail.ResolveExpertDir(cfg.PoolDir, cfg.ExpertName), or (B)
change the signature to return (string, error) and propagate the error from
mail.ResolveSharedExpertDir so callers can decide; reference
resolveExpertDirForMCP, mail.ResolveSharedExpertDir and mail.ResolveExpertDir
when making the change and ensure any call sites are updated accordingly.
---
Outside diff comments:
In `@internal/mail/router_test.go`:
- Around line 39-87: Add a new test that exercises the Route function's
shared-expert branch by calling mail.Route with a non-nil sharedNames map
containing the target expert name (e.g., "security-standards"); create the
poolDir/postoffice and poolDir/shared-state/{name}/inbox directories, write a
message addressed to that shared expert, call mail.Route(logger, poolDir,
srcPath, sharedNames) and assert msg.ID and that the file was moved into
shared-state/{name}/inbox/{id}.md (and original removed). Reference the existing
TestRoute_EndToEnd pattern and use the same logger and file perms while passing
a non-nil sharedNames map to trigger the shared-state routing path.
In `@internal/mcp/server.go`:
- Around line 28-37: Update ServerConfig.Validate to also require
SharedOverlayDir when IsShared is true: inside the Validate method
(ServerConfig.Validate) after the existing PoolDir/ExpertName checks, add a
conditional that if c.IsShared && c.SharedOverlayDir == "" then return an error
(e.g., fmt.Errorf("shared overlay directory is required when IsShared is
true")). This ensures SharedOverlayDir is validated alongside PoolDir and
ExpertName.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e8103871-2022-44ec-99c0-58fd6cbf83f8
📒 Files selected for processing (18)
cmd/agent-pool/main.godocs/plans/2026-04-05-shared-experts.mdinternal/config/config.gointernal/config/config_test.gointernal/daemon/daemon.gointernal/daemon/daemon_test.gointernal/expert/expert.gointernal/expert/expert_test.gointernal/hooks/flush.gointernal/mail/router.gointernal/mail/router_test.gointernal/mcp/concierge_tools.gointernal/mcp/config.gointernal/mcp/config_test.gointernal/mcp/server.gointernal/mcp/testhelp_test.gointernal/mcp/tools.gointernal/mcp/tools_test.go
Comment on lines
+1
to
+239
| # v0.7 Shared Experts Implementation Plan | ||
|
|
||
| ## Context | ||
|
|
||
| Experts are currently pool-scoped — identity, state, logs all live under `{poolDir}/experts/{name}/`. This means a `security-standards` expert bootstrapped in one pool can't be reused in another without duplicating everything. v0.7 lifts experts to user scope (`~/.agent-pool/experts/`), with per-pool project overlays for project-specific state. Pools opt in via `shared.include` in pool.toml. | ||
|
|
||
| **Design decisions (resolved per v0.7 prompt):** | ||
| - Inbox: pool-scoped at `{poolDir}/shared-state/{name}/inbox/` | ||
| - Logs: pool-scoped at `{poolDir}/shared-state/{name}/logs/` | ||
| - Errors: user-level only at `~/.agent-pool/experts/{name}/errors.md` | ||
| - Multi-pool: deferred — separate `agent-pool start` processes per pool | ||
|
|
||
| ## Phase 1: Shared Expert Resolution | ||
|
|
||
| ### 1.1 — `config.SharedExpertDir` helper | ||
|
|
||
| **File:** `internal/config/config.go` | ||
|
|
||
| ```go | ||
| func SharedExpertDir(name string) (string, error) | ||
| ``` | ||
|
|
||
| Returns `~/.agent-pool/experts/{name}/`. Validates name (no path separators, not empty, not `.`/`..`). | ||
|
|
||
| **Tests** (`internal/config/config_test.go`): happy path, path traversal rejection, empty name. | ||
|
|
||
| ### 1.2 — Shared resolution functions in mail package | ||
|
|
||
| **File:** `internal/mail/router.go` | ||
|
|
||
| Add three new functions (existing `ResolveExpertDir`/`ResolveInbox` unchanged): | ||
|
|
||
| ```go | ||
| func ResolveSharedExpertDir(name string) (string, error) // → ~/.agent-pool/experts/{name}/ | ||
| func ResolveSharedInbox(poolDir, name string) string // → {poolDir}/shared-state/{name}/inbox/ | ||
| func ResolveSharedLogDir(poolDir, name string) string // → {poolDir}/shared-state/{name}/logs/ | ||
| ``` | ||
|
|
||
| `ResolveSharedExpertDir` delegates to `config.SharedExpertDir`. | ||
|
|
||
| **Tests** (`internal/mail/router_test.go`): verify paths for each function. | ||
|
|
||
| ### 1.3 — Config validation | ||
|
|
||
| **File:** `internal/config/config.go` | ||
|
|
||
| Add `Validate()` method on `PoolConfig`, called at end of `LoadPool`: | ||
| - `shared.include` names must not be builtin roles | ||
| - `shared.include` names must not overlap with `[experts.*]` keys | ||
| - Names must be filename-safe (no path separators) | ||
|
|
||
| **Tests** (`internal/config/config_test.go`): conflict with builtin, conflict with pool-scoped, path traversal, happy path. | ||
|
|
||
| ### 1.4 — `ensureDirs` + `isSharedExpert` helper | ||
|
|
||
| **File:** `internal/daemon/daemon.go` | ||
|
|
||
| ```go | ||
| func (d *Daemon) isSharedExpert(name string) bool | ||
| ``` | ||
|
|
||
| In `ensureDirs()`, iterate `d.cfg.Shared.Include` and create: | ||
| - `{poolDir}/shared-state/{name}/inbox/` | ||
| - `{poolDir}/shared-state/{name}/logs/` | ||
|
|
||
| **Test** (`internal/daemon/daemon_test.go`): verify dirs created for shared.include entries. | ||
|
|
||
| --- | ||
|
|
||
| ## Phase 2: Layered Prompt Assembly | ||
|
|
||
| ### 2.1 — `SpawnConfig.OverlayDir` + `AssemblePrompt` update | ||
|
|
||
| **File:** `internal/expert/expert.go` | ||
|
|
||
| Add `OverlayDir string` to `SpawnConfig`. | ||
|
|
||
| Update `AssemblePrompt`: after the "## Current State" section (user-level), if `OverlayDir` is set, read `{OverlayDir}/state.md` and emit "## Project State" section. Missing overlay file is silently skipped. | ||
|
|
||
| **Tests** (`internal/expert/expert_test.go`): | ||
| - Both user-level and overlay state present → two sections | ||
| - Overlay only (no user state.md) → just project state section | ||
| - OverlayDir set but no state.md file → graceful skip | ||
| - OverlayDir empty string → no overlay section (backward compat) | ||
|
|
||
| ### 2.2 — Daemon shared expert spawn wiring | ||
|
|
||
| **File:** `internal/daemon/daemon.go` | ||
|
|
||
| Update `processInboxMessage` (~line 559-595): | ||
|
|
||
| ```go | ||
| var expertDir, overlayDir, logDir string | ||
| if d.isSharedExpert(expertName) { | ||
| userDir, err := mail.ResolveSharedExpertDir(expertName) | ||
| // ... error handling | ||
| expertDir = userDir | ||
| overlayDir = filepath.Join(d.poolDir, "shared-state", expertName) | ||
| logDir = mail.ResolveSharedLogDir(d.poolDir, expertName) | ||
| } else { | ||
| expertDir = d.resolveExpertDir(expertName) | ||
| logDir = expertDir | ||
| } | ||
| ``` | ||
|
|
||
| Set `OverlayDir` on `SpawnConfig`. Write logs to `logDir` instead of `expertDir` for shared experts. | ||
|
|
||
| Update MCP config creation to use `WriteTempConfigShared` for shared experts. | ||
|
|
||
| **Tests** (`internal/daemon/daemon_test.go`): | ||
| - `TestSharedExpert_SpawnConfig` — verify ExpertDir is user-level, OverlayDir is pool-scoped | ||
| - `TestSharedExpert_LogsInPoolScope` — logs written to shared-state/{name}/logs/ | ||
|
|
||
| ### 2.3 — Watcher + inbox routing for shared experts | ||
|
|
||
| **File:** `internal/daemon/daemon.go` | ||
|
|
||
| Three changes in `Run()` (~line 156-162): | ||
| 1. Watch shared expert inboxes alongside pool-scoped ones | ||
| 2. Update `resolveExpertName` to match shared inbox paths | ||
| 3. Update `drainAllInboxes` to drain shared expert inboxes | ||
| 4. Update `drainInbox` to resolve shared inbox paths | ||
|
|
||
| Also update `handleCancel` (line 369) to resolve shared inbox paths. | ||
|
|
||
| Also update `Route` in `internal/mail/router.go` — add `sharedNames map[string]bool` parameter so it routes to `ResolveSharedInbox` for shared experts. The daemon passes `d.sharedNamesMap()`. | ||
|
|
||
| **Call sites for `mail.Route`:** Only `daemon.handlePostoffice` (daemon.go:259). | ||
|
|
||
| **Call sites for `mail.ResolveInbox`:** | ||
| - daemon.go:145 (architect inbox watch) — unchanged | ||
| - daemon.go:158 (expert inbox watch) — unchanged, shared handled separately | ||
| - daemon.go:369 (handleCancel) — needs shared check | ||
| - daemon.go:834 (drainInbox) — needs shared check | ||
| - daemon.go:1109-1115 (resolveExpertName) — needs shared check | ||
|
|
||
| **Tests** (`internal/daemon/daemon_test.go`): | ||
| - `TestSharedExpert_InboxDrainedOnStartup` — pre-existing message in shared inbox processed | ||
| - `TestSharedExpert_RouteToSharedInbox` — message addressed to shared expert lands in shared-state inbox | ||
| - `TestSharedAndPoolScoped_Coexist` — both types work in same pool | ||
|
|
||
| --- | ||
|
|
||
| ## Phase 3: Scoped State Writes | ||
|
|
||
| ### 3.1 — `ServerConfig.IsShared` + `--shared` CLI flag | ||
|
|
||
| **File:** `internal/mcp/server.go` — add `IsShared bool` and `SharedOverlayDir string` to `ServerConfig`. | ||
|
|
||
| **File:** `internal/mcp/config.go` — add `WriteTempConfigShared(poolDir, expertName string)` that passes `--shared true` in MCP args. | ||
|
|
||
| **File:** `cmd/agent-pool/main.go` — update `cmdMCP` to parse `--shared` flag, set `ServerConfig.IsShared` and compute `SharedOverlayDir`. | ||
|
|
||
| ### 3.2 — Scope-aware `update_state` and `read_state` | ||
|
|
||
| **File:** `internal/mcp/tools.go` | ||
|
|
||
| Update `RegisterExpertTools` to resolve expertDir via shared-aware helper and pass `cfg` to handlers. | ||
|
|
||
| `update_state` gains `scope` parameter (optional, default `"project"`): | ||
| - Pool-scoped expert: `scope` ignored, writes to `expertDir/state.md` as today | ||
| - Shared expert + `scope=project`: writes to `{SharedOverlayDir}/state.md` | ||
| - Shared expert + `scope=user`: writes to `{expertDir}/state.md` (user-level) | ||
|
|
||
| `read_state` for shared experts: returns `project_state` field alongside existing `state` field. | ||
|
|
||
| `append_error`: unchanged — `expertDir` is already user-level for shared experts. | ||
|
|
||
| **Tests** (`internal/mcp/tools_test.go`): | ||
| - Shared expert: project scope write, user scope write, default scope | ||
| - Pool-scoped: scope parameter ignored | ||
| - read_state with project_state for shared expert | ||
|
|
||
| ### 3.3 — Update flush hook + concierge tools | ||
|
|
||
| **File:** `internal/hooks/flush.go` (line 45) — needs to resolve shared expert dir correctly. Add `IsShared bool` to `FlushConfig`, use `mail.ResolveSharedExpertDir` when true. | ||
|
|
||
| **File:** `cmd/agent-pool/main.go` — update `cmdFlush` to parse `--shared`. | ||
|
|
||
| **File:** `internal/mcp/concierge_tools.go` (line 358) — `readExpertResult` reads logs from expertDir. For shared experts, logs are in `{poolDir}/shared-state/{name}/logs/`. Need to check shared.include list. Since concierge has pool config access, pass shared names through. | ||
|
|
||
| --- | ||
|
|
||
| ## Phase 4: Startup Validation | ||
|
|
||
| At the end of `ensureDirs`, warn (don't error) if a shared expert's user-level directory is missing `identity.md`: | ||
|
|
||
| ```go | ||
| for _, name := range d.cfg.Shared.Include { | ||
| userDir, _ := mail.ResolveSharedExpertDir(name) | ||
| if _, err := os.Stat(filepath.Join(userDir, "identity.md")); os.IsNotExist(err) { | ||
| d.logger.Warn("Shared expert missing identity.md", "expert", name, "path", userDir) | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Commit Sequence | ||
|
|
||
| | # | Scope | Files | Risk | | ||
| |---|-------|-------|------| | ||
| | 1 | `config.SharedExpertDir` | config.go, config_test.go | None — pure addition | | ||
| | 2 | `mail.ResolveShared*` functions | router.go, router_test.go | None — pure addition | | ||
| | 3 | Config validation | config.go, config_test.go | Low — existing valid configs unchanged | | ||
| | 4 | `ensureDirs` + `isSharedExpert` | daemon.go, daemon_test.go | Low — new dirs only | | ||
| | 5 | `SpawnConfig.OverlayDir` + `AssemblePrompt` | expert.go, expert_test.go | Low — empty OverlayDir is no-op | | ||
| | 6 | Daemon shared expert spawn | daemon.go, daemon_test.go | Medium — spawn path diverges | | ||
| | 7 | Watcher + Route + inbox routing | daemon.go, router.go, *_test.go | Medium — Route signature changes | | ||
| | 8 | `ServerConfig.IsShared` + `--shared` + `WriteTempConfigShared` | server.go, config.go, main.go | Low — additive | | ||
| | 9 | Scope-aware MCP tools | tools.go, tools_test.go | Medium — handler logic branches | | ||
| | 10 | Flush hook + concierge tools | flush.go, concierge_tools.go, main.go | Low — path resolution | | ||
| | 11 | Startup validation | daemon.go | None — warn only | | ||
|
|
||
| --- | ||
|
|
||
| ## Verification | ||
|
|
||
| 1. `make test` — full suite passes after each commit | ||
| 2. **Manual integration test:** | ||
| - Create `~/.agent-pool/experts/test-shared/identity.md` with content | ||
| - Create pool with `shared.include = ["test-shared"]` and a pool-scoped expert | ||
| - Send task to shared expert → verify prompt includes identity from user-level | ||
| - Call `update_state` with scope=project → verify file at `shared-state/test-shared/state.md` | ||
| - Call `update_state` with scope=user → verify file at `~/.agent-pool/experts/test-shared/state.md` | ||
| - Send task to pool-scoped expert → verify unchanged behavior | ||
| 3. `make test-cover` — new code has tests | ||
|
|
||
| ## Critical Files | ||
|
|
||
| - `internal/config/config.go` — SharedExpertDir, Validate | ||
| - `internal/mail/router.go` — ResolveShared*, Route signature | ||
| - `internal/expert/expert.go` — SpawnConfig.OverlayDir, AssemblePrompt | ||
| - `internal/daemon/daemon.go` — processInboxMessage, ensureDirs, resolveExpertName, drainInbox, handleCancel, Run | ||
| - `internal/mcp/tools.go` — RegisterExpertTools, handleUpdateState, handleReadState | ||
| - `internal/mcp/config.go` — WriteTempConfigShared | ||
| - `internal/mcp/server.go` — ServerConfig.IsShared | ||
| - `internal/hooks/flush.go` — FlushConfig.IsShared | ||
| - `internal/mcp/concierge_tools.go` — readExpertResult |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Well-structured implementation plan.
The phased approach with risk assessments and comprehensive test coverage is excellent. The plan aligns with the implemented code changes. A couple of minor style suggestions from static analysis:
- Line 48: Consider hyphenating "built-in" for consistency with common usage.
- Lines 124-126: Two successive sentences begin with "Also" — consider varying the sentence structure.
🧰 Tools
🪛 LanguageTool
[grammar] ~48-~48: Ensure spelling is correct
Context: ...: - shared.includenames must not be builtin roles -shared.include` names must not...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~125-~125: Two successive sentences begin with the same adverb. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ne 369) to resolve shared inbox paths. Also update Route in `internal/mail/router...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-04-05-shared-experts.md` around lines 1 - 239, In the
docs/plans "v0.7 Shared Experts Implementation Plan" update: change "built in"
to hyphenated "built-in" (near the Design decisions / Config validation
discussion) and reword the two consecutive sentences that start with "Also"
(around the Watcher + inbox routing section) to vary sentence openings and
improve flow; apply these small copy edits in that markdown so wording is
consistent and the repeated "Also" isn't used twice in succession.
Comment on lines
+82
to
+88
| // builtinRoleNames are the built-in role names that shared experts must not | ||
| // conflict with. Kept in sync with mail.builtinRoles. | ||
| var builtinRoleNames = map[string]bool{ | ||
| "architect": true, | ||
| "researcher": true, | ||
| "concierge": true, | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
builtinRoleNames duplicates mail.builtinRoles - consider using a single source of truth.
Both config.builtinRoleNames and mail.builtinRoles define the same three roles (architect, researcher, concierge). If these drift out of sync, validation could allow conflicts. Consider exporting one and importing it, or adding a compile-time check.
♻️ Option 1: Import from mail package
If mail.IsBuiltinRole is already exported, use it in Validate():
-var builtinRoleNames = map[string]bool{
- "architect": true,
- "researcher": true,
- "concierge": true,
-}
func (c *PoolConfig) Validate() error {
for _, name := range c.Shared.Include {
if name == "" || name != filepath.Base(name) || name == "." || name == ".." {
return fmt.Errorf("invalid shared expert name %q: must be a simple filename", name)
}
- if builtinRoleNames[name] {
+ if mail.IsBuiltinRole(name) {
return fmt.Errorf("shared expert %q conflicts with built-in role", name)
}Note: This would require importing internal/mail into internal/config, which may create a circular dependency. If so, the current duplication with a comment to keep in sync is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/config/config.go` around lines 82 - 88, builtinRoleNames duplicates
mail.builtinRoles leading to potential drift; change to use a single source by
removing the local builtinRoleNames and referencing the exported check from the
mail package (e.g., use mail.IsBuiltinRole or mail.BuiltinRoles) inside
Validate(), or alternatively export the canonical list from config and import it
into mail so there's one definition; if importing mail into config would create
a circular dependency, add a compile-time assertion or unit test that compares
config.builtinRoleNames with mail.builtinRoles at build/test time and update the
Validate() function to call the single canonical helper (Validate() and
builtinRoleNames/mail.IsBuiltinRole are the key symbols to adjust).
Comment on lines
+296
to
+298
| origHome := os.Getenv("HOME") | ||
| os.Setenv("HOME", fakeHome) | ||
| t.Cleanup(func() { os.Setenv("HOME", origHome) }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use t.Setenv for HOME manipulation in test.
Same pattern improvement as in tools_test.go.
♻️ Suggested refactor
func TestDaemon_SharedExpertSpawnConfig(t *testing.T) {
// Set HOME to a temp dir so SharedExpertDir resolves to a controlled location
fakeHome := t.TempDir()
- origHome := os.Getenv("HOME")
- os.Setenv("HOME", fakeHome)
- t.Cleanup(func() { os.Setenv("HOME", origHome) })
+ t.Setenv("HOME", fakeHome)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/daemon/daemon_test.go` around lines 296 - 298, Replace the manual
HOME env restore (origHome := os.Getenv("HOME"); os.Setenv("HOME", fakeHome);
t.Cleanup(func() { os.Setenv("HOME", origHome) })) with the testing helper
t.Setenv("HOME", fakeHome); remove origHome and the t.Cleanup block so the test
uses t.Setenv to set and automatically restore HOME (locate usages around
origHome, fakeHome and the os.Setenv/os.Getenv calls in the test).
Comment on lines
+301
to
+303
| sharedExpertDir := filepath.Join(fakeHome, ".agent-pool", "experts", "security-standards") | ||
| os.MkdirAll(sharedExpertDir, 0o755) | ||
| os.WriteFile(filepath.Join(sharedExpertDir, "identity.md"), []byte("I am the security expert."), 0o644) |
There was a problem hiding this comment.
Check errors from os.MkdirAll and os.WriteFile in test setup.
Discarded errors could mask test failures.
🛡️ Proposed fix
// Create the user-level shared expert directory
sharedExpertDir := filepath.Join(fakeHome, ".agent-pool", "experts", "security-standards")
- os.MkdirAll(sharedExpertDir, 0o755)
- os.WriteFile(filepath.Join(sharedExpertDir, "identity.md"), []byte("I am the security expert."), 0o644)
+ if err := os.MkdirAll(sharedExpertDir, 0o755); err != nil {
+ t.Fatalf("creating shared expert dir: %v", err)
+ }
+ if err := os.WriteFile(filepath.Join(sharedExpertDir, "identity.md"), []byte("I am the security expert."), 0o644); err != nil {
+ t.Fatalf("writing identity.md: %v", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| sharedExpertDir := filepath.Join(fakeHome, ".agent-pool", "experts", "security-standards") | |
| os.MkdirAll(sharedExpertDir, 0o755) | |
| os.WriteFile(filepath.Join(sharedExpertDir, "identity.md"), []byte("I am the security expert."), 0o644) | |
| // Create the user-level shared expert directory | |
| sharedExpertDir := filepath.Join(fakeHome, ".agent-pool", "experts", "security-standards") | |
| if err := os.MkdirAll(sharedExpertDir, 0o755); err != nil { | |
| t.Fatalf("creating shared expert dir: %v", err) | |
| } | |
| if err := os.WriteFile(filepath.Join(sharedExpertDir, "identity.md"), []byte("I am the security expert."), 0o644); err != nil { | |
| t.Fatalf("writing identity.md: %v", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/daemon/daemon_test.go` around lines 301 - 303, The test setup
currently ignores errors from os.MkdirAll and os.WriteFile when creating
sharedExpertDir and identity.md; update the calls around sharedExpertDir
(constructed with filepath.Join) to check their returned errors and fail the
test on error (e.g., use t.Fatalf or t.Fatal) so failures in os.MkdirAll or
os.WriteFile surface instead of being discarded.
Comment on lines
+1248
to
+1256
| // isSharedExpert reports whether the named expert is in the pool's shared.include list. | ||
| func (d *Daemon) isSharedExpert(name string) bool { | ||
| for _, shared := range d.cfg.Shared.Include { | ||
| if shared == name { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
isSharedExpert performs O(n) linear search; consider reusing sharedNamesMap.
The helper iterates through cfg.Shared.Include on each call. For pools with many shared experts, this could be optimized by using the already-computed sharedNamesMap() result or caching it.
♻️ Option: Reuse sharedNamesMap for O(1) lookup
// isSharedExpert reports whether the named expert is in the pool's shared.include list.
func (d *Daemon) isSharedExpert(name string) bool {
- for _, shared := range d.cfg.Shared.Include {
- if shared == name {
- return true
- }
- }
- return false
+ return d.sharedNamesMap()[name]
}Note: sharedNamesMap() returns nil for empty include, and nil[key] returns false in Go, so this is safe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/daemon/daemon.go` around lines 1248 - 1256, isSharedExpert currently
does a linear scan over d.cfg.Shared.Include; change it to call
d.sharedNamesMap() and perform a map lookup for O(1) existence checks.
Specifically, in Daemon.isSharedExpert use the map returned by sharedNamesMap()
(which may be nil) and return m[name] instead of iterating; this leverages the
existing helper and avoids repeated linear scans over cfg.Shared.Include.
Comment on lines
+356
to
+388
| func TestUpdateState_SharedExpert_UserScope(t *testing.T) { | ||
| // Set HOME to a temp dir so ResolveSharedExpertDir resolves to a controlled path | ||
| fakeHome := t.TempDir() | ||
| origHome := os.Getenv("HOME") | ||
| os.Setenv("HOME", fakeHome) | ||
| t.Cleanup(func() { os.Setenv("HOME", origHome) }) | ||
|
|
||
| // Create the user-level shared expert directory | ||
| userExpertDir := filepath.Join(fakeHome, ".agent-pool", "experts", "security-standards") | ||
| os.MkdirAll(userExpertDir, 0o755) | ||
|
|
||
| poolDir, _, overlayDir := setupSharedExpertPool(t) | ||
| srv := buildSharedMCPTestServer(t, poolDir, "security-standards", overlayDir) | ||
|
|
||
| result := callTool(t, srv, "update_state", map[string]any{ | ||
| "content": "user-level knowledge", | ||
| "scope": "user", | ||
| }) | ||
|
|
||
| text := resultText(t, result) | ||
| if !strings.Contains(text, "user") { | ||
| t.Errorf("result = %q, want mention of user", text) | ||
| } | ||
|
|
||
| // Verify file written to user-level dir | ||
| data, err := os.ReadFile(filepath.Join(userExpertDir, "state.md")) | ||
| if err != nil { | ||
| t.Fatalf("reading user-level state.md: %v", err) | ||
| } | ||
| if !strings.Contains(string(data), "user-level knowledge") { | ||
| t.Errorf("user-level state.md = %q, want 'user-level knowledge'", string(data)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using t.Setenv for cleaner HOME environment manipulation.
The test correctly manipulates HOME to control shared expert directory resolution. However, Go 1.17+ provides t.Setenv which automatically restores the original value and is cleaner than manual cleanup.
♻️ Suggested refactor using t.Setenv
func TestUpdateState_SharedExpert_UserScope(t *testing.T) {
// Set HOME to a temp dir so ResolveSharedExpertDir resolves to a controlled path
fakeHome := t.TempDir()
- origHome := os.Getenv("HOME")
- os.Setenv("HOME", fakeHome)
- t.Cleanup(func() { os.Setenv("HOME", origHome) })
+ t.Setenv("HOME", fakeHome)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestUpdateState_SharedExpert_UserScope(t *testing.T) { | |
| // Set HOME to a temp dir so ResolveSharedExpertDir resolves to a controlled path | |
| fakeHome := t.TempDir() | |
| origHome := os.Getenv("HOME") | |
| os.Setenv("HOME", fakeHome) | |
| t.Cleanup(func() { os.Setenv("HOME", origHome) }) | |
| // Create the user-level shared expert directory | |
| userExpertDir := filepath.Join(fakeHome, ".agent-pool", "experts", "security-standards") | |
| os.MkdirAll(userExpertDir, 0o755) | |
| poolDir, _, overlayDir := setupSharedExpertPool(t) | |
| srv := buildSharedMCPTestServer(t, poolDir, "security-standards", overlayDir) | |
| result := callTool(t, srv, "update_state", map[string]any{ | |
| "content": "user-level knowledge", | |
| "scope": "user", | |
| }) | |
| text := resultText(t, result) | |
| if !strings.Contains(text, "user") { | |
| t.Errorf("result = %q, want mention of user", text) | |
| } | |
| // Verify file written to user-level dir | |
| data, err := os.ReadFile(filepath.Join(userExpertDir, "state.md")) | |
| if err != nil { | |
| t.Fatalf("reading user-level state.md: %v", err) | |
| } | |
| if !strings.Contains(string(data), "user-level knowledge") { | |
| t.Errorf("user-level state.md = %q, want 'user-level knowledge'", string(data)) | |
| } | |
| } | |
| func TestUpdateState_SharedExpert_UserScope(t *testing.T) { | |
| // Set HOME to a temp dir so ResolveSharedExpertDir resolves to a controlled path | |
| fakeHome := t.TempDir() | |
| t.Setenv("HOME", fakeHome) | |
| // Create the user-level shared expert directory | |
| userExpertDir := filepath.Join(fakeHome, ".agent-pool", "experts", "security-standards") | |
| os.MkdirAll(userExpertDir, 0o755) | |
| poolDir, _, overlayDir := setupSharedExpertPool(t) | |
| srv := buildSharedMCPTestServer(t, poolDir, "security-standards", overlayDir) | |
| result := callTool(t, srv, "update_state", map[string]any{ | |
| "content": "user-level knowledge", | |
| "scope": "user", | |
| }) | |
| text := resultText(t, result) | |
| if !strings.Contains(text, "user") { | |
| t.Errorf("result = %q, want mention of user", text) | |
| } | |
| // Verify file written to user-level dir | |
| data, err := os.ReadFile(filepath.Join(userExpertDir, "state.md")) | |
| if err != nil { | |
| t.Fatalf("reading user-level state.md: %v", err) | |
| } | |
| if !strings.Contains(string(data), "user-level knowledge") { | |
| t.Errorf("user-level state.md = %q, want 'user-level knowledge'", string(data)) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/mcp/tools_test.go` around lines 356 - 388, Replace the manual HOME
env manipulation in TestUpdateState_SharedExpert_UserScope with t.Setenv:
instead of capturing origHome, calling os.Setenv and t.Cleanup, call
t.Setenv("HOME", fakeHome) at the start of the test (keeping creation of
userExpertDir and the rest unchanged); update references in the test surrounding
setupSharedExpertPool and buildSharedMCPTestServer as needed so the environment
is set before those calls and remove the manual os.Setenv cleanup code.
Comment on lines
+364
to
+365
| userExpertDir := filepath.Join(fakeHome, ".agent-pool", "experts", "security-standards") | ||
| os.MkdirAll(userExpertDir, 0o755) |
There was a problem hiding this comment.
Check error return from os.MkdirAll.
The error is discarded which could mask test setup failures.
🛡️ Proposed fix
// Create the user-level shared expert directory
userExpertDir := filepath.Join(fakeHome, ".agent-pool", "experts", "security-standards")
- os.MkdirAll(userExpertDir, 0o755)
+ if err := os.MkdirAll(userExpertDir, 0o755); err != nil {
+ t.Fatalf("creating user expert dir: %v", err)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/mcp/tools_test.go` around lines 364 - 365, The call to os.MkdirAll
for userExpertDir discards the returned error; update the test in tools_test.go
to check the error and fail the test on failure (e.g., capture the error :=
os.MkdirAll(userExpertDir, 0o755) and call t.Fatalf or use require.NoError(t,
err)) so test setup failures are not masked; locate the
os.MkdirAll(userExpertDir, 0o755) invocation and add the error check using the
test helper in the file.
Comment on lines
+19
to
+31
| // resolveExpertDirForMCP returns the state directory for the expert. | ||
| // Shared experts resolve to the user-level dir; pool-scoped experts use the pool dir. | ||
| func resolveExpertDirForMCP(cfg *ServerConfig) string { | ||
| if cfg.IsShared { | ||
| dir, err := mail.ResolveSharedExpertDir(cfg.ExpertName) | ||
| if err != nil { | ||
| // Fallback to pool-scoped path if user-level resolution fails | ||
| return mail.ResolveExpertDir(cfg.PoolDir, cfg.ExpertName) | ||
| } | ||
| return dir | ||
| } | ||
| return mail.ResolveExpertDir(cfg.PoolDir, cfg.ExpertName) | ||
| } |
There was a problem hiding this comment.
Silent fallback on error may cause state to be written to unexpected location.
When ResolveSharedExpertDir fails for a shared expert, silently falling back to the pool-scoped path could lead to confusing behavior—state intended for user-level could end up in the pool directory without any indication. Consider logging a warning or propagating the error.
Proposed fix to add warning on fallback
This would require passing a logger or returning an error. At minimum, consider documenting this fallback behavior or logging when it occurs:
func resolveExpertDirForMCP(cfg *ServerConfig) string {
if cfg.IsShared {
dir, err := mail.ResolveSharedExpertDir(cfg.ExpertName)
if err != nil {
- // Fallback to pool-scoped path if user-level resolution fails
+ // Fallback to pool-scoped path if user-level resolution fails.
+ // This can happen if the expert name is invalid. Callers should
+ // ensure shared experts have valid names via config validation.
return mail.ResolveExpertDir(cfg.PoolDir, cfg.ExpertName)
}
return dir
}
return mail.ResolveExpertDir(cfg.PoolDir, cfg.ExpertName)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/mcp/tools.go` around lines 19 - 31, The function
resolveExpertDirForMCP silently falls back to pool-scoped path when
mail.ResolveSharedExpertDir returns an error; update resolveExpertDirForMCP to
surface that failure by either (A) adding a logger parameter and logging a
warning (including the error and the fallback path) before returning
mail.ResolveExpertDir(cfg.PoolDir, cfg.ExpertName), or (B) change the signature
to return (string, error) and propagate the error from
mail.ResolveSharedExpertDir so callers can decide; reference
resolveExpertDirForMCP, mail.ResolveSharedExpertDir and mail.ResolveExpertDir
when making the change and ensure any call sites are updated accordingly.
Comment on lines
+157
to
+161
| case "project": | ||
| if err := expert.WriteState(cfg.SharedOverlayDir, content); err != nil { | ||
| return mcp.NewToolResultError(fmt.Sprintf("writing state: %v", err)), nil | ||
| } | ||
| return mcp.NewToolResultText("project-level state.md updated"), nil |
There was a problem hiding this comment.
Missing guard for empty SharedOverlayDir when scope is "project".
If cfg.IsShared is true but cfg.SharedOverlayDir is empty (e.g., misconfiguration), expert.WriteState("", content) could write to an unexpected location. Add a defensive check.
Proposed fix
case "project":
+ if cfg.SharedOverlayDir == "" {
+ return mcp.NewToolResultError("shared overlay directory not configured"), nil
+ }
if err := expert.WriteState(cfg.SharedOverlayDir, content); err != nil {
return mcp.NewToolResultError(fmt.Sprintf("writing state: %v", err)), nil
}
return mcp.NewToolResultText("project-level state.md updated"), nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/mcp/tools.go` around lines 157 - 161, When handling the "project"
scope, add a defensive guard that verifies cfg.SharedOverlayDir is non-empty
when cfg.IsShared (or when scope == "project") before calling expert.WriteState;
if it's empty return a clear mcp.NewToolResultError (e.g., "shared overlay dir
not configured") instead of calling expert.WriteState with an empty path. Update
the branch that currently calls expert.WriteState(cfg.SharedOverlayDir, content)
to first check cfg.SharedOverlayDir == "" and return an error via
mcp.NewToolResultError to avoid writing to an unexpected location.
- Extract BuiltinRoleNames to config package as single source of truth, mail.IsBuiltinRole now delegates to config.BuiltinRoleNames - Guard empty SharedOverlayDir before project-scope write (Major finding) - Return error from resolveExpertDirForMCP instead of silent fallback - Cache sharedSet in Daemon struct, isSharedExpert is now O(1) - Use t.Setenv instead of manual HOME manipulation in tests - Check os.MkdirAll/os.WriteFile errors in test setup Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
~/.agent-pool/experts/{name}/) and are reusable across poolsshared.includein pool.toml; config validation prevents conflicts with builtin roles and pool-scoped expertsshared-state/{name}/state.md)update_stateMCP tool:scope=project(default) writes to pool overlay,scope=userwrites to user-level dirread_statereturnsproject_statefield for shared experts alongside existingstatefield{poolDir}/shared-state/{name}/identity.mdat user levelRoutesignature gainssharedNamesparam for shared inbox routingTest plan
config.SharedExpertDir— happy path, path traversal, empty namemail.ResolveShared*— path verification for all three functionsPoolConfig.Validate— builtin conflict, pool-scoped conflict, path traversal, happy pathensureDirs— shared-state dirs created alongside pool-scoped dirsAssemblePromptwith overlay — both layers, overlay only, missing overlay, empty OverlayDirWriteTempConfigShared—--shared truein MCP argsupdate_state— project scope, user scope, default scope, pool-scoped ignores scoperead_statewithproject_statefor shared expertsRoutewith nil sharedNames — all existing tests pass (backward compat)make testpasses🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
--sharedCLI flag for shared expert operations