feat: v0.8 researcher role + curation#19
Merged
Conversation
Watch researcher/inbox/ for incoming tasks. Resolve researcher config (model, session_timeout) from pool.toml [researcher] section. Drain researcher inbox at startup. Register researcher tools stub in MCP server. Role-aware MCP tool name selection via ToolNamesForRole() replaces hardcoded ExpertToolNames in --allowedTools construction. Adds researcher/logs/ to ensureDirs for log archival. Also adds v0.8 implementation plan to docs/plans/. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Six tools for the researcher role: - list_experts: all experts with state sizes, log counts, last task - read_expert_state: read another expert's identity/state/errors - read_expert_logs: last N log index entries with optional query - enrich_state: full context assembly (state + recent logs + summaries) - write_expert_state: write curated state or errors back to an expert - promote_pattern: append graduated pattern to identity.md The researcher reads any expert's state and logs, reasons about what to keep/prune/promote, then writes curated results back. Two-step pattern gives the LLM control over curation decisions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move postMessage logic from internal/mcp to mail.Post() so the CLI (seed command) and daemon (curation scheduler) can post messages without importing the mcp package. mcp.postMessage becomes a thin delegate to mail.Post. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Triggers researcher curation after interval_tasks completions (default 10) or interval_hours elapsed (default 168h). Generates a structured task message with per-expert metadata (state sizes, log counts) to guide the researcher's curation decisions. The scheduler integrates at two points: - markTaskCompleted: increments counter, triggers on threshold - Run: starts a time-based ticker goroutine Adds EventCurationTriggered event type to the event bus. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Composes a seed task for the researcher that includes the target expert name, project directory, and existing identity context. Validates the expert exists in pool config before posting. Usage: agent-pool seed --pool <dir> --expert <name> The seed task instructs the researcher to explore the project codebase and create initial state.md for the named expert. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Archives log files beyond defaults.log_retention (default 50) into tar.gz bundles during curation triggers. Matching .stderr companion files are included. index.md remains untouched for searchability. Uses archive/tar + compress/gzip from stdlib. The daemon's rotateAllLogs runs for all experts (pool-scoped and shared) before generating the researcher's curation task. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Researcher tools now detect shared experts via pool config and
handle both user-level and project overlay state:
- read_expert_state: returns project_state field for shared experts
- enrich_state: includes project overlay in assembled context
- write_expert_state: layer param ("user"/"project") for shared experts
- promote_pattern: targets user-level identity.md (cross-pool knowledge)
- list_experts: shared experts reported with type "shared"
Detection uses config.LoadPool to check shared.include, then
config.SharedExpertDir for user-level paths and poolDir/shared-state
for project overlay paths.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
v0.8 documents the researcher + curation implementation. v0.9 covers formulas (TOML workflow templates), config hot-reload, and partial-write hardening. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a researcher role with curation scheduling, expert log rotation, six researcher MCP tools, a CLI Changes
Sequence Diagram(s)sequenceDiagram
participant Daemon
participant Taskboard
participant CurationScheduler
participant LogRot as LogRotation
participant Mail as mail.Post
participant Researcher
Daemon->>Taskboard: markTaskCompleted(taskID)
Taskboard->>CurationScheduler: RecordTaskCompletion()
alt threshold reached
CurationScheduler-->>Daemon: trigger
Daemon->>LogRot: rotateAllLogs()
LogRot-->>Daemon: rotation results
Daemon->>Mail: buildCurationTaskBody() + Post(poolDir,msg)
Mail-->>Researcher: curation task posted
Daemon->>Daemon: emit EventCurationTriggered
end
sequenceDiagram
participant CLI as seed CLI
participant FS as Pool Discovery
participant Config
participant ExpertDir
participant Mail as mail.Post
participant Postoffice
CLI->>FS: resolve poolDir
CLI->>Config: LoadPool(poolDir)
CLI->>ExpertDir: validate expert in cfg.Experts or cfg.Shared.Include
CLI->>ExpertDir: read identity.md (optional)
CLI->>CLI: compose Cold-Start Seed Task
CLI->>Mail: Post(poolDir, msg)
Mail->>Postoffice: ensure dir + atomic write
Postoffice-->>CLI: success (task ID)
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: 14
🤖 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-06-researcher-curation.md`:
- Line 5: Replace the misspelled word "builtin" with "built-in" in the sentence
that reads "researcher role has been scaffolded across prior versions — config
parsing, mail routing, directory creation, builtin role registration" so it
becomes "built-in role registration"; update that exact phrase in the docs/plans
text to use the hyphenated form.
In `@docs/prompts/v09-formulas-polish.md`:
- Line 23: The markdown has markdownlint violations around headings and fenced
code blocks (e.g., the heading "Scope:" and the fenced blocks at the listed
locations); fix by ensuring there is a blank line before and after every heading
and before/after each fenced code block and by adding a language tag to every
triple-backtick fence (e.g., ```python, ```bash, ```text) for the fenced blocks
referenced (lines around 23, 39, 77, 130-131, 149, 153, 174, 180, 198, 203, 210)
so the file docs/prompts/v09-formulas-polish.md passes MD022/MD031/MD040.
In `@internal/config/config.go`:
- Around line 219-221: The config loader currently defaults
cfg.Defaults.LogRetention only when it's exactly 0; update the normalization so
any non-positive value is replaced with the default (e.g. treat <= 0 as unset).
Locate the code that sets cfg.Defaults.LogRetention (the block using
cfg.Defaults.LogRetention) and change the condition from == 0 to <= 0 (or
otherwise check for non-positive) so negative values are normalized to the
default retention of 50 at load time.
In `@internal/daemon/curation_test.go`:
- Around line 61-67: The test currently only detects that a "researcher" task
with ID prefix "curation-" was spawned via fake.getCalls() and then proceeds to
shutdown, causing in-flight writes and TempDir race flakes; update the test to
call the waitForTaskStatus helper for the detected curation task ID (matching
"curation-...") and wait for its completion status before initiating daemon
shutdown (also apply the same change where similar spawn-only checks occur
around the other occurrence noted), ensuring the test blocks until the curation
task finishes.
In `@internal/daemon/curation.go`:
- Around line 43-48: The RecordTaskCompletion logic in curationScheduler allows
intervalTasks <= 0 to behave as "always" and can double-fire because the
threshold check and Reset() occur in separate calls; fix by making
RecordTaskCompletion (and the analogous method at lines 61-66) handle the full
check-and-reset under the same cs.mu lock: if cs.intervalTasks <= 0 return
false; increment cs.taskCount, and if cs.taskCount >= cs.intervalTasks then set
cs.taskCount = 0 and return true (no separate Reset call); update or remove any
external Reset() usage so no other goroutine can observe the threshold between
check and reset.
- Around line 133-141: The curation code is using config.SharedExpertDir(name)
to compute paths for state/logs (used by fileSize and countLogFiles) but shared
experts' logs live in the overlay directory (shared-state/<expert>/logs) as
written in internal/daemon/daemon.go; update the loops that call
config.SharedExpertDir(name) (both occurrences) to point at the
shared-overlay/overlay state path used by the daemon — either call the existing
helper that returns the shared expert overlay path (e.g., SharedExpertOverlayDir
or equivalent) or build the path with filepath.Join(sharedStateBase, name) so
fileSize(filepath.Join(overlayDir, "state.md")) and
countLogFiles(filepath.Join(overlayDir, "logs")) use the same overlay directory
the daemon writes to.
In `@internal/daemon/daemon_test.go`:
- Around line 2348-2349: Tests call shutdownDaemon(t, cancel, errCh) before
ensuring spawned tasks have reached a terminal state; replace those shutdowns
with a call to waitForTaskStatus to wait for the task(s) to reach a terminal
status and then call shutdownDaemon. Locate the test cases that call
shutdownDaemon (e.g., the occurrences around shutdownDaemon(t, cancel, errCh))
and before each shutdown invoke waitForTaskStatus(t, client, taskID) (or the
appropriate helper signature used in these tests) for the researcher/spawned
task(s) so cleanup waits for completion, then call shutdownDaemon(t, cancel,
errCh).
In `@internal/expert/rotate_test.go`:
- Line 189: TestRotateLogs_IndexUntouched currently calls RotateLogs(dir, 3) and
ignores its error; update the test to check the returned error from RotateLogs
and fail the test if non-nil (e.g., call t.Fatalf or use your test assertion
helper to require no error). Locate the call inside
TestRotateLogs_IndexUntouched, capture the error (err := RotateLogs(dir, 3)) and
assert err == nil before proceeding with the rest of the test so rotation
failures cannot silently pass.
In `@internal/expert/rotate.go`:
- Around line 103-111: Check and handle errors from tw.Close(), gw.Close(), and
f.Close() before deleting sources: call each Close (tw.Close, then gw.Close,
then f.Close), capture their returned errors, and if any is non-nil return that
error (or wrap/aggregate it) instead of proceeding to the os.Remove loop over
toDelete; only run the os.Remove(...) best-effort deletions when all closes
succeed. Ensure you reference the tar.Writer tw, gzip.Writer gw, and file f for
locating the close calls and the toDelete/os.Remove loop to change control flow.
In `@internal/mail/post_test.go`:
- Around line 35-37: The test currently calls os.Stat(path) and only fails when
os.IsNotExist(err) is true, which hides other filesystem errors; update both
checks (the one using path and the other at the later occurrence) to first call
fi, err := os.Stat(path) and then: if os.IsNotExist(err) { t.Fatal("message file
not created") } else if err != nil { t.Fatalf("stat %q failed: %v", path, err) }
(or assert err == nil) so non-ENOENT errors are reported; keep the original
success branch using fi as before.
In `@internal/mail/post.go`:
- Around line 14-31: The Post function currently allows an empty poolDir which
makes filepath.Join(poolDir, "postoffice") resolve to a relative "postoffice"
and risk writing to the wrong working directory; update Post to validate poolDir
(e.g., check if poolDir == "" or strings.TrimSpace(poolDir) == "") at the start
and return a clear error before calling Compose, MkdirAll, or
atomicfile.WriteFile. Reference the Post function and the variables poolDir,
postoffice and path when adding this guard so the check occurs before computing
postoffice/path or performing file writes.
In `@internal/mcp/researcher_tools_test.go`:
- Around line 517-629: Add a test that seeds an overlay log file under the
shared overlay logs path and asserts the tools read from it: create a new test
(e.g. TestResearcherReadExpertLogs_SharedOverlay) that calls
setupSharedResearcherPool to get poolDir, then write a log file into
filepath.Join(poolDir, "shared-state", "security-standards", "logs",
"2026-01-01.md") with unique content, build the server with buildMCPTestServer
and call callTool(..., "read_expert_logs", {"expert":"security-standards"}) to
assert the returned text contains the seeded content; also call callTool(...,
"list_experts", nil) and/or callTool(..., "enrich_state", ...) as appropriate to
ensure overlay log presence is respected by
list_experts/read_expert_logs/enrich_state, using the existing test helpers
(resultText, json.Unmarshal) and the same expert name "security-standards" so
this covers regressions in overlay log handling.
In `@internal/mcp/researcher_tools.go`:
- Around line 243-267: The code uses resolveTargetExpertDir (used by
read_expert_logs/enrich_state) then appends "logs" which is wrong for shared
experts because their logs live in the pool overlay; update the logic so when
building indexPath or when calling expert.SearchIndex you check for the pool
logs location and prefer it for shared experts (e.g., if filepath.Join(dir,
"logs", "index.md") does not exist or the expert is a shared/pool expert, use
filepath.Join(cfg.PoolDir, expertName, "logs", "index.md") or
filepath.Join(cfg.PoolDir, expertName, "logs") for SearchIndex). Apply the same
fix in the other occurrences noted (the blocks around the other ranges you
referenced) so read_expert_logs, enrich_state and any SearchIndex/indexPath
construction use the pool overlay logs for shared experts instead of
dir+"/logs".
- Around line 120-127: list_experts is currently resolving shared expert
metadata with config.SharedExpertDir(name) which points at the global shared
expert dir, but the daemon writes shared logs under the pool overlay
(shared-state/<expert>/logs). Update the code that handles
poolCfg.Shared.Include (and the similar block at 146-169) to compute the
shared-expert directory from the pool overlay rather than the global shared dir:
use the pool overlay path (e.g., construct or call the config/overlay helper
that yields the pool-specific "shared-state/<expert>/logs" directory using the
current pool identifier) and pass that directory into gatherExpertInfo(name,
"shared", dir) so log_count and last_task reflect the pool-shared logs.
🪄 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: c5373e6c-e834-43f6-9647-c6cbbe73e0f1
📒 Files selected for processing (21)
cmd/agent-pool/main.godocs/plans/2026-04-06-researcher-curation.mddocs/prompts/v08-researcher-curation.mddocs/prompts/v09-formulas-polish.mdinternal/config/config.gointernal/daemon/curation.gointernal/daemon/curation_test.gointernal/daemon/daemon.gointernal/daemon/daemon_test.gointernal/daemon/events.gointernal/expert/rotate.gointernal/expert/rotate_test.gointernal/mail/post.gointernal/mail/post_test.gointernal/mcp/config.gointernal/mcp/config_test.gointernal/mcp/postoffice.gointernal/mcp/researcher_tools.gointernal/mcp/researcher_tools_test.gointernal/mcp/server.gointernal/mcp/testhelp_test.go
|
|
||
| ▎ Workflow templates and operational hardening. | ||
|
|
||
| Scope: |
There was a problem hiding this comment.
Resolve markdownlint violations (MD022/MD031/MD040).
Please add required blank lines around headings/fenced blocks and specify languages on all fenced code blocks to keep docs lint-clean.
Also applies to: 39-39, 77-77, 130-131, 149-149, 153-153, 174-174, 180-180, 198-198, 203-203, 210-210
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 23-23: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/prompts/v09-formulas-polish.md` at line 23, The markdown has
markdownlint violations around headings and fenced code blocks (e.g., the
heading "Scope:" and the fenced blocks at the listed locations); fix by ensuring
there is a blank line before and after every heading and before/after each
fenced code block and by adding a language tag to every triple-backtick fence
(e.g., ```python, ```bash, ```text) for the fenced blocks referenced (lines
around 23, 39, 77, 130-131, 149, 153, 174, 180, 198, 203, 210) so the file
docs/prompts/v09-formulas-polish.md passes MD022/MD031/MD040.
Comment on lines
+35
to
+37
| if _, err := os.Stat(path); os.IsNotExist(err) { | ||
| t.Fatal("message file not created") | ||
| } |
There was a problem hiding this comment.
Handle non-ENOENT os.Stat errors explicitly in assertions.
Current checks only fail on missing files/dirs; other filesystem errors can be masked.
Proposed fix
- if _, err := os.Stat(path); os.IsNotExist(err) {
- t.Fatal("message file not created")
- }
+ if _, err := os.Stat(path); err != nil {
+ if os.IsNotExist(err) {
+ t.Fatal("message file not created")
+ }
+ t.Fatalf("stat message file: %v", err)
+ }
@@
- if _, err := os.Stat(filepath.Join(poolDir, "postoffice")); os.IsNotExist(err) {
- t.Error("postoffice dir not created")
- }
+ if _, err := os.Stat(filepath.Join(poolDir, "postoffice")); err != nil {
+ if os.IsNotExist(err) {
+ t.Error("postoffice dir not created")
+ } else {
+ t.Fatalf("stat postoffice dir: %v", err)
+ }
+ }Also applies to: 73-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/mail/post_test.go` around lines 35 - 37, The test currently calls
os.Stat(path) and only fails when os.IsNotExist(err) is true, which hides other
filesystem errors; update both checks (the one using path and the other at the
later occurrence) to first call fi, err := os.Stat(path) and then: if
os.IsNotExist(err) { t.Fatal("message file not created") } else if err != nil {
t.Fatalf("stat %q failed: %v", path, err) } (or assert err == nil) so non-ENOENT
errors are reported; keep the original success branch using fi as before.
Critical: - Check tar/gzip/file Close errors before deleting source logs Major: - Atomic check-and-reset in curation scheduler prevents double-fire - Guard intervalTasks <= 0 (disabled, not "always fire") - Shared expert log rotation now targets pool overlay, not user dir - Shared expert list_experts/read_expert_logs/enrich_state now read logs from pool overlay (shared-state/<name>/logs) Minor: - Normalize non-positive log_retention at config load time - waitForTaskStatus before shutdown in researcher/curation tests - Handle non-ENOENT os.Stat errors in post_test assertions - Check RotateLogs error in TestRotateLogs_IndexUntouched - Fix "builtin" typo in plan doc - Add language tag to fenced code block in v0.9 prompt Nitpick: - Guard empty poolDir in mail.Post - Add shared overlay log integration test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
docs/prompts/v09-formulas-polish.md (1)
1-258:⚠️ Potential issue | 🟡 MinorMarkdown formatting violations remain unaddressed.
The markdown violations (MD022/MD031/MD040) flagged in the previous review are still present. Please add blank lines around headings and fenced code blocks, and specify language tags for all fenced blocks as previously requested.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/prompts/v09-formulas-polish.md` around lines 1 - 258, The markdown in docs/prompts/v09-formulas-polish.md still violates MD022/MD031/MD040: add a blank line above and below every heading and before/after each fenced code block, and ensure every fenced code block includes a language tag (e.g., replace ``` with ```toml, ```text, or ```bash as appropriate for the examples such as the TOML formula and the task/structure snippets). Scan the file for all fenced blocks and headings (the visible examples using ``` and the blocks showing TOML and text), insert the missing blank lines, and tag each fenced block with the correct language to satisfy the linter.internal/mail/post_test.go (1)
73-75:⚠️ Potential issue | 🟡 MinorHandle non-
ENOENTerrors in postoffice directory check.The check at line 73 only fails when
os.IsNotExist(err)is true, but silently ignores other filesystem errors (e.g., permission denied). This can mask test failures.Proposed fix
- if _, err := os.Stat(filepath.Join(poolDir, "postoffice")); os.IsNotExist(err) { - t.Error("postoffice dir not created") - } + if _, err := os.Stat(filepath.Join(poolDir, "postoffice")); err != nil { + if os.IsNotExist(err) { + t.Error("postoffice dir not created") + } else { + t.Fatalf("stat postoffice dir: %v", err) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mail/post_test.go` around lines 73 - 75, The current test only treats "not exists" as failure and ignores other os.Stat errors; update the postoffice directory check around os.Stat(filepath.Join(poolDir, "postoffice")) to handle all errors: if err == nil OK, else if os.IsNotExist(err) call t.Error("postoffice dir not created"), otherwise call t.Fatalf or t.Errorf with the actual err (e.g., "stat postoffice: %v") so permission or other filesystem errors fail the test and include the error details.
🤖 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-06-researcher-curation.md`:
- Around line 210-222: The ASCII dependency graph block (the fenced code block
containing lines like "Phase 1 (daemon wiring) ──┐", "Phase 2 (tools)
──────────┬──> Phase 6 (shared enrichment)", etc.) is missing a language tag;
update the opening fence to include a language specifier (e.g., ```text) so the
block becomes a fenced code block with a language tag to improve rendering and
syntax highlighting.
- Around line 131-134: Update the fenced code block that contains the commit
message example ("refactor(mail): extract Post() for CLI and daemon reuse" /
"feat(daemon): add curation scheduler with task and time triggers") to include a
language specifier (e.g., replace ``` with ```text) so the block renders with
proper syntax highlighting; ensure only the opening fence is changed and the
closing fence remains ``` to keep the block valid.
In `@internal/daemon/curation.go`:
- Around line 65-75: In ShouldTriggerByTime (on type curationScheduler) reset
the task-count state when a time-trigger fires: currently the method only
updates cs.lastCuration and returns true; update it to also set cs.taskCount = 0
(or the zero-value used elsewhere) before returning true so that the task-based
threshold (taskCount / interval_tasks) reflects work done since the last
time-based curation; keep the existing locking (cs.mu.Lock()/defer
cs.mu.Unlock()) and only modify cs.taskCount in the branch that sets
cs.lastCuration and returns true.
In `@internal/daemon/daemon.go`:
- Around line 1216-1222: The built-in routing now reserves the name
"researcher", making any user-supplied experts.researcher unreachable; fix by
validating and rejecting that reserved name at startup: in the daemon
constructor New (or the existing config validation function) inspect
d.cfg.Experts and if a key equals "researcher" return an error (or remove it) so
the process fails fast instead of silently preferring the built-in; ensure the
check references d.cfg.Experts and returns a clear error from New.
- Around line 874-878: When marking tasks completed in markTaskCompleted,
prevent daemon-generated curation tasks from incrementing the curation scheduler
by computing a boolean (e.g., countForCuration) from the taskboard entry (t)
before releasing d.mu and only calling d.curation.RecordTaskCompletion() when
that boolean is true; specifically exclude tasks where t.Expert == "researcher"
&& t.From == "daemon" so the existing block that does d.wg.Add(1); go func() {
defer d.wg.Done(); d.triggerCuration("task_threshold") }() is only executed when
RecordTaskCompletion() should actually count the completion.
- Around line 212-229: The current goroutine uses a fixed time.Ticker tied to
daemon start (checking every d.curation.intervalHours) which misaligns with the
last curation time; change the loop in the anonymous goroutine that references
d.curation.intervalHours, d.curation.ShouldTriggerByTime(), and
d.triggerCuration("time_interval") to use a resettable time.Timer (or a more
frequent poll interval) instead of a fixed Ticker: create a time.Timer seeded
with the remaining duration until the next allowed curation (or a small poll
interval), on timer.C check ShouldTriggerByTime() and call triggerCuration when
true, and after handling either reset the timer based on d.curation
interval/next-allowed time so the schedule anchors to the last curation rather
than daemon start; ensure timer.Stop() is called when cancelling via
childCtx.Done() to avoid leaks.
In `@internal/expert/rotate.go`:
- Around line 47-51: The code silently skips entries when e.Info() returns an
error, which can hide permission or filesystem issues; update the logic around
e.Info() (the call that currently precedes appending to jsonFiles and the
creation of logFile{name: e.Name(), mtime: info.ModTime()}) to either return or
propagate the error up or at minimum log the failure with the entry name and
error details (e.g., include e.Name() and err in the log) so problems are
visible; if skipping is intentional for robustness, add a short comment
explaining why skipping is safe and desirable.
In `@internal/mcp/researcher_tools.go`:
- Around line 462-475: The current insertion after a section heading in
researcher_tools.go (using variables content, section, insertAt and pattern)
places the new pattern at the top of the section; change the logic to append the
pattern at the end of the section by searching from insertAt for the next
Markdown heading (e.g., look for "\n#" or another section delimiter) or EOF to
compute sectionEnd, then insert the trimmed pattern followed by a newline at
sectionEnd instead of immediately after the heading; keep use of existing
symbols (content, insertAt, pattern) and ensure you preserve surrounding
newlines when composing the updated content.
---
Duplicate comments:
In `@docs/prompts/v09-formulas-polish.md`:
- Around line 1-258: The markdown in docs/prompts/v09-formulas-polish.md still
violates MD022/MD031/MD040: add a blank line above and below every heading and
before/after each fenced code block, and ensure every fenced code block includes
a language tag (e.g., replace ``` with ```toml, ```text, or ```bash as
appropriate for the examples such as the TOML formula and the task/structure
snippets). Scan the file for all fenced blocks and headings (the visible
examples using ``` and the blocks showing TOML and text), insert the missing
blank lines, and tag each fenced block with the correct language to satisfy the
linter.
In `@internal/mail/post_test.go`:
- Around line 73-75: The current test only treats "not exists" as failure and
ignores other os.Stat errors; update the postoffice directory check around
os.Stat(filepath.Join(poolDir, "postoffice")) to handle all errors: if err ==
nil OK, else if os.IsNotExist(err) call t.Error("postoffice dir not created"),
otherwise call t.Fatalf or t.Errorf with the actual err (e.g., "stat postoffice:
%v") so permission or other filesystem errors fail the test and include the
error details.
🪄 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: d6292b54-1ba4-456a-af45-b07c64f80c35
📒 Files selected for processing (13)
docs/plans/2026-04-06-researcher-curation.mddocs/prompts/v09-formulas-polish.mdinternal/config/config.gointernal/daemon/curation.gointernal/daemon/curation_test.gointernal/daemon/daemon.gointernal/daemon/daemon_test.gointernal/expert/rotate.gointernal/expert/rotate_test.gointernal/mail/post.gointernal/mail/post_test.gointernal/mcp/researcher_tools.gointernal/mcp/researcher_tools_test.go
Comment on lines
+131
to
+134
| ``` | ||
| refactor(mail): extract Post() for CLI and daemon reuse | ||
| feat(daemon): add curation scheduler with task and time triggers | ||
| ``` |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add language specifier to fenced code block.
The commit message example block lacks a language tag, which affects rendering and syntax highlighting.
Proposed fix
### Commits
-```
+```text
refactor(mail): extract Post() for CLI and daemon reuse
feat(daemon): add curation scheduler with task and time triggers
```🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 131-131: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 131-131: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-04-06-researcher-curation.md` around lines 131 - 134, Update
the fenced code block that contains the commit message example ("refactor(mail):
extract Post() for CLI and daemon reuse" / "feat(daemon): add curation scheduler
with task and time triggers") to include a language specifier (e.g., replace ```
with ```text) so the block renders with proper syntax highlighting; ensure only
the opening fence is changed and the closing fence remains ``` to keep the block
valid.
Comment on lines
+210
to
+222
| ``` | ||
| Phase 1 (daemon wiring) ──┐ | ||
| v | ||
| Phase 2 (tools) ──────────┬──> Phase 6 (shared enrichment) | ||
| │ | ||
| Phase 3a (mail.Post) ─────┤ | ||
| v | ||
| Phase 3b (scheduler) ─────┤ | ||
| v | ||
| Phase 4 (seed CLI) ───────┘ | ||
|
|
||
| Phase 5 (log rotation) ── independent, can parallel with 3-4-6 | ||
| ``` |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add language specifier to dependency graph block.
The dependency graph ASCII art lacks a language tag.
Proposed fix
-```
+```text
Phase 1 (daemon wiring) ──┐
v
...
```🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 210-210: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-04-06-researcher-curation.md` around lines 210 - 222, The
ASCII dependency graph block (the fenced code block containing lines like "Phase
1 (daemon wiring) ──┐", "Phase 2 (tools) ──────────┬──> Phase 6 (shared
enrichment)", etc.) is missing a language tag; update the opening fence to
include a language specifier (e.g., ```text) so the block becomes a fenced code
block with a language tag to improve rendering and syntax highlighting.
Comment on lines
+65
to
+75
| func (cs *curationScheduler) ShouldTriggerByTime() bool { | ||
| cs.mu.Lock() | ||
| defer cs.mu.Unlock() | ||
|
|
||
| if cs.intervalHours <= 0 { | ||
| return false | ||
| } | ||
|
|
||
| if time.Since(cs.lastCuration) >= time.Duration(cs.intervalHours)*time.Hour { | ||
| cs.lastCuration = time.Now() | ||
| return true |
There was a problem hiding this comment.
Reset taskCount when the time-based trigger fires.
ShouldTriggerByTime only advances lastCuration. If the scheduler is already sitting at interval_tasks-1, a time-based curation will be followed by another curation after just one more completed task. Clear taskCount here so both thresholds measure work since the last curation.
🛠️ Suggested fix
if time.Since(cs.lastCuration) >= time.Duration(cs.intervalHours)*time.Hour {
+ cs.taskCount = 0
cs.lastCuration = time.Now()
return true
}📝 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 (cs *curationScheduler) ShouldTriggerByTime() bool { | |
| cs.mu.Lock() | |
| defer cs.mu.Unlock() | |
| if cs.intervalHours <= 0 { | |
| return false | |
| } | |
| if time.Since(cs.lastCuration) >= time.Duration(cs.intervalHours)*time.Hour { | |
| cs.lastCuration = time.Now() | |
| return true | |
| func (cs *curationScheduler) ShouldTriggerByTime() bool { | |
| cs.mu.Lock() | |
| defer cs.mu.Unlock() | |
| if cs.intervalHours <= 0 { | |
| return false | |
| } | |
| if time.Since(cs.lastCuration) >= time.Duration(cs.intervalHours)*time.Hour { | |
| cs.taskCount = 0 | |
| cs.lastCuration = time.Now() | |
| return true | |
| } | |
| return false | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/daemon/curation.go` around lines 65 - 75, In ShouldTriggerByTime (on
type curationScheduler) reset the task-count state when a time-trigger fires:
currently the method only updates cs.lastCuration and returns true; update it to
also set cs.taskCount = 0 (or the zero-value used elsewhere) before returning
true so that the task-based threshold (taskCount / interval_tasks) reflects work
done since the last time-based curation; keep the existing locking
(cs.mu.Lock()/defer cs.mu.Unlock()) and only modify cs.taskCount in the branch
that sets cs.lastCuration and returns true.
Comment on lines
+212
to
+229
| // Start time-based curation ticker | ||
| if d.curation.intervalHours > 0 { | ||
| d.wg.Add(1) | ||
| go func() { | ||
| defer d.wg.Done() | ||
| ticker := time.NewTicker(time.Duration(d.curation.intervalHours) * time.Hour) | ||
| defer ticker.Stop() | ||
| for { | ||
| select { | ||
| case <-childCtx.Done(): | ||
| return | ||
| case <-ticker.C: | ||
| if d.curation.ShouldTriggerByTime() { | ||
| d.triggerCuration("time_interval") | ||
| } | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
The time-based scheduler is anchored to daemon start, not to the last curation.
This goroutine only checks once per full interval_hours tick. For example, with a 24-hour interval, a task-threshold curation at 24h+1m will be checked again at 48h (too early, so it skips) and then not fire until 72h. Use a resettable timer, or poll more frequently and keep ShouldTriggerByTime() as the gate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/daemon/daemon.go` around lines 212 - 229, The current goroutine uses
a fixed time.Ticker tied to daemon start (checking every
d.curation.intervalHours) which misaligns with the last curation time; change
the loop in the anonymous goroutine that references d.curation.intervalHours,
d.curation.ShouldTriggerByTime(), and d.triggerCuration("time_interval") to use
a resettable time.Timer (or a more frequent poll interval) instead of a fixed
Ticker: create a time.Timer seeded with the remaining duration until the next
allowed curation (or a small poll interval), on timer.C check
ShouldTriggerByTime() and call triggerCuration when true, and after handling
either reset the timer based on d.curation interval/next-allowed time so the
schedule anchors to the last curation rather than daemon start; ensure
timer.Stop() is called when cancelling via childCtx.Done() to avoid leaks.
Comment on lines
+874
to
+878
| // Check curation threshold after task completion | ||
| if d.curation.RecordTaskCompletion() { | ||
| d.wg.Add(1) | ||
| go func() { defer d.wg.Done(); d.triggerCuration("task_threshold") }() | ||
| } |
There was a problem hiding this comment.
Do not feed daemon-generated curation jobs back into interval_tasks.
markTaskCompleted runs for the researcher's own curation-* tasks too, so those completions increment the same scheduler. With interval_tasks = 1 this self-schedules forever; with larger thresholds the effective interval drops after the first curation. Gate RecordTaskCompletion() on task metadata and exclude daemon→researcher curation tasks.
🛠️ Suggested fix
- if d.curation.RecordTaskCompletion() {
+ if countForCuration && d.curation.RecordTaskCompletion() {
d.wg.Add(1)
go func() { defer d.wg.Done(); d.triggerCuration("task_threshold") }()
}// Derive this from the taskboard entry before releasing d.mu.
countForCuration := !(t.Expert == "researcher" && t.From == "daemon")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/daemon/daemon.go` around lines 874 - 878, When marking tasks
completed in markTaskCompleted, prevent daemon-generated curation tasks from
incrementing the curation scheduler by computing a boolean (e.g.,
countForCuration) from the taskboard entry (t) before releasing d.mu and only
calling d.curation.RecordTaskCompletion() when that boolean is true;
specifically exclude tasks where t.Expert == "researcher" && t.From == "daemon"
so the existing block that does d.wg.Add(1); go func() { defer d.wg.Done();
d.triggerCuration("task_threshold") }() is only executed when
RecordTaskCompletion() should actually count the completion.
Comment on lines
+1216
to
1222
| // Check built-in roles first | ||
| for _, role := range []string{"architect", "researcher"} { | ||
| roleInbox := mail.ResolveInbox(d.poolDir, role) | ||
| if absEqual(inboxDir, roleInbox) { | ||
| return role | ||
| } | ||
| } |
There was a problem hiding this comment.
Reserve researcher before routing built-ins first.
This new branch makes an existing [experts.researcher] config silently unreachable: built-in routing sends that name to /researcher/inbox, and the new built-in config path also prefers cfg.Researcher. Please reject researcher in d.cfg.Experts during config validation, or fail fast in New.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/daemon/daemon.go` around lines 1216 - 1222, The built-in routing now
reserves the name "researcher", making any user-supplied experts.researcher
unreachable; fix by validating and rejecting that reserved name at startup: in
the daemon constructor New (or the existing config validation function) inspect
d.cfg.Experts and if a key equals "researcher" return an error (or remove it) so
the process fails fast instead of silently preferring the built-in; ensure the
check references d.cfg.Experts and returns a clear error from New.
Comment on lines
+47
to
+51
| info, err := e.Info() | ||
| if err != nil { | ||
| continue | ||
| } | ||
| jsonFiles = append(jsonFiles, logFile{name: e.Name(), mtime: info.ModTime()}) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Silently skipping files with Info() errors may hide issues.
When e.Info() fails for a .json file, it's silently skipped. This could hide permission issues or filesystem problems affecting specific log files.
Consider logging or returning an error when Info() fails, rather than silently skipping:
info, err := e.Info()
if err != nil {
- continue
+ return 0, fmt.Errorf("getting info for %s: %w", e.Name(), err)
}Alternatively, if skipping is intentional (e.g., for robustness), add a comment explaining why.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/expert/rotate.go` around lines 47 - 51, The code silently skips
entries when e.Info() returns an error, which can hide permission or filesystem
issues; update the logic around e.Info() (the call that currently precedes
appending to jsonFiles and the creation of logFile{name: e.Name(), mtime:
info.ModTime()}) to either return or propagate the error up or at minimum log
the failure with the entry name and error details (e.g., include e.Name() and
err in the log) so problems are visible; if skipping is intentional for
robustness, add a short comment explaining why skipping is safe and desirable.
Comment on lines
+462
to
+475
| content := string(existing) | ||
|
|
||
| // Find or create the target section | ||
| if strings.Contains(content, section) { | ||
| // Append after the section heading | ||
| idx := strings.Index(content, section) | ||
| insertAt := idx + len(section) | ||
| // Skip to end of heading line | ||
| if nl := strings.Index(content[insertAt:], "\n"); nl >= 0 { | ||
| insertAt += nl | ||
| } else { | ||
| insertAt = len(content) | ||
| } | ||
| content = content[:insertAt] + "\n\n" + strings.TrimSpace(pattern) + "\n" + content[insertAt:] |
There was a problem hiding this comment.
Pattern insertion may produce inconsistent formatting.
When inserting after an existing section heading (lines 467-475), the code inserts "\n\n" + pattern + "\n" immediately after the heading line. If the section already has content, this inserts the new pattern at the top of the section rather than appending at the end.
For example, with existing content:
## Graduated Patterns
- Existing patternAdding a new pattern produces:
## Graduated Patterns
- New pattern
- Existing patternIf the intent is to append patterns chronologically (newest at bottom), consider finding the next heading or end-of-file instead:
// Find end of section (next heading or EOF)
sectionEnd := len(content)
if nextHeading := strings.Index(content[insertAt:], "\n#"); nextHeading >= 0 {
sectionEnd = insertAt + nextHeading
}
content = content[:sectionEnd] + "\n" + strings.TrimSpace(pattern) + "\n" + content[sectionEnd:]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/mcp/researcher_tools.go` around lines 462 - 475, The current
insertion after a section heading in researcher_tools.go (using variables
content, section, insertAt and pattern) places the new pattern at the top of the
section; change the logic to append the pattern at the end of the section by
searching from insertAt for the next Markdown heading (e.g., look for "\n#" or
another section delimiter) or EOF to compute sectionEnd, then insert the trimmed
pattern followed by a newline at sectionEnd instead of immediately after the
heading; keep use of existing symbols (content, insertAt, pattern) and ensure
you preserve surrounding newlines when composing the updated content.
5 tasks
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 seedCLI command for cold-start expert bootstrappingTest plan
make testpasses (11/11 packages, 33 new tests)make buildcompiles cleanlyagent-pool seed --expert authposts seed task to postoffice🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
agent-pool seedCLI for cold-start expert bootstrapping.Configuration
log_retentionsetting to control log archival thresholds.