feat: v0.9 formulas + polish#20
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a TOML-driven formulas package, an architect MCP tool Changes
Sequence Diagram(s)sequenceDiagram
participant Architect
participant MCP_Server as "MCP Server"
participant Formula as "Formula Loader"
participant Postoffice
Architect->>MCP_Server: invoke instantiate_formula(formula, prefix, overrides, experts)
MCP_Server->>Formula: Load and Validate `formulas/<formula>.toml`
Note over Formula: parse steps, validate IDs, detect cycles
MCP_Server->>MCP_Server: build messages (prefix IDs, apply overrides/experts, rewrite DependsOn)
MCP_Server->>Postoffice: post messages (task files)
Postoffice-->>MCP_Server: ack / error
MCP_Server-->>Architect: return summary or error (with rollback info if failed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 10
🤖 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/prompts/v09-formulas-polish.md`:
- Around line 98-104: The fenced code block showing the directory tree (the
block starting with ``` and the listing of {poolDir}/formulas/) needs a Markdown
language tag to satisfy markdownlint; update that fenced block to use ```text
rather than plain ``` so the directory tree is treated as plain text (i.e.,
change the opening fence in the block that contains {poolDir}/formulas/ and its
three toml entries and index.md to ```text).
In `@internal/daemon/daemon.go`:
- Around line 1306-1340: The reload currently commits the new config and emits
EventConfigReloaded even if post-load steps (ensureDirs and watcher.Add) fail;
change the flow so you perform and verify filesystem/watch setup for all added
experts before committing the new config or emitting d.events.emit(Event{Type:
EventConfigReloaded ...}); specifically, run d.ensureDirs() and loop over added
calling watcher.Add(inboxDir) first, collect any errors, and if any setup step
fails abort the swap (do not replace the active config or emit
EventConfigReloaded), log the failure and optionally emit a failure event;
alternatively stage the new config and only assign it to the daemon state after
all ensureDirs/watcher.Add succeed (use the same symbols: ensureDirs,
watcher.Add, d.events.emit, EventConfigReloaded, added/removed).
- Around line 1273-1315: reloadConfig currently unlocks before performing
directory creation and watcher.Add calls which causes data races on
d.cfg/d.sharedSet (accessed by resolveExpertConfig, isSharedExpert,
sharedNamesMap) and concurrent map access on watcher.w.dirs (watcher.Add vs
resolveDir), and it logs success even if those ops fail; fix by keeping the
daemon lock (d.mu) held while rebuilding d.sharedSet and while performing
ensureDirs() and all watcher.Add() calls (or alternatively add proper
synchronization inside watcher: a watcher.mu protecting w.dirs and make
Add/resolveDir use it), and ensure any error from ensureDirs() or watcher.Add()
is returned (or causes reloadConfig to fail) instead of only logging so the
caller does not get a false success; update reloadConfig to return on first
error from ensureDirs()/watcher.Add() and/or propagate combined error to caller.
In `@internal/daemon/watcher.go`:
- Around line 123-140: The config-change branch currently only checks for
fsnotify.Write so atomic saves (temp-file + rename) are missed; update the
conditional in the watcher loop to treat fsnotify.Create and fsnotify.Rename the
same as Write for .toml files (e.g., change the check around
event.Has(fsnotify.Write) to include event.Has(fsnotify.Create) ||
event.Has(fsnotify.Rename)), then keep the existing stability check via
waitForStable(path) and the subsequent resolveDir + send WatcherEvent{Path:
path, Dir: dir, Kind: EventKindConfig} to w.events (respecting ctx.Done()) so
renames/creates trigger the same hot-reload path.
In `@internal/formula/formula.go`:
- Around line 85-88: The exported Validate function currently dereferences
f.Steps without guarding for a nil receiver, which causes a panic when called as
Validate(nil); update Validate (in the function named Validate and referencing
type Formula and field Steps) to first check whether f is nil and return a
descriptive error (e.g., "nil formula") before accessing f.Steps, then keep the
existing check for len(f.Steps) == 0 and return the existing error if empty.
- Around line 91-104: The validator currently only checks emptiness/uniqueness
of step IDs (in the Validate loop over f.Steps) but must also reject IDs that
cannot become valid task IDs during instantiate_formula/postMessage; add a check
in the same loop (before ids[s.ID] = true) that s.ID conforms to the simple-ID
constraints used by task creation (e.g. match a conservative regex such as only
alphanumerics, dot, underscore and hyphen: ^[A-Za-z0-9._-]+$ and reject any ID
containing '/' or whitespace), and return a clear error like "step %q has
invalid id" when it fails. Ensure this check is in the same function that
iterates f.Steps so invalid IDs are caught during Validate rather than during
instantiation.
In `@internal/mcp/architect_tools_test.go`:
- Around line 354-364: The new helper function writeFormula in
internal/mcp/architect_tools_test.go should be moved into the shared MCP test
helpers file testhelp_test.go to avoid duplication; remove writeFormula from
architect_tools_test.go and add it to testhelp_test.go alongside existing
helpers (makePoolDirs, buildMCPTestServer, callTool, resultText), keeping the
same signature and t.Helper() behavior so other tests can reuse it.
In `@internal/mcp/architect_tools.go`:
- Around line 300-389: The instantiation currently posts tasks as it builds them
in handleInstantiateFormula, risking partial publishes on failure; change it to
first validate and compose all messages and taskIDs into in-memory mail.Message
objects (and apply prefix/deps/overrides/experts) before any call to
postMessage, then call shouldRequireApproval (the same approval gate used by
send_task) and require approval if needed, and only after approval iterate the
composed messages to call postMessage; if any postMessage fails, implement
cleanup to remove any already-posted tasks or fail the whole operation
atomically (i.e., delete posted messages) so retries are safe.
- Around line 340-362: Only allow the experts map to affect steps whose
step.Role == "experts" and reject empty assignments: in the validation loop over
f.Steps (the block that checks step.Role == "experts"), require that
experts[step.ID] both exists and is non-empty and return an error if it is
blank; then in the message-building loop (where taskID, to, expertName are
computed), only override to = expertName when step.Role == "experts" and ok is
true (do not apply experts[] to other roles). This ensures experts only fills
placeholder expert steps and prevents blank or cross-role overrides.
- Around line 307-321: The code currently constructs formulaPath by joining
cfg.PoolDir + "formulas" with the unvalidated formulaName which allows path
traversal; validate formulaName the same way as prefix before using it: check
formulaName != "" and that formulaName == filepath.Base(formulaName) and
formulaName is not "." or "..", returning mcp.NewToolResultError on failure,
then build formulaPath and call formula.Load(formulaPath) as before to prevent
escaping the formulas directory.
🪄 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: 34542633-040f-465f-be96-18ee5a6a32da
📒 Files selected for processing (10)
docs/plans/2026-04-06-v09-formulas-polish.mddocs/prompts/v09-formulas-polish.mdinternal/daemon/daemon.gointernal/daemon/daemon_test.gointernal/daemon/events.gointernal/daemon/watcher.gointernal/formula/formula.gointernal/formula/formula_test.gointernal/mcp/architect_tools.gointernal/mcp/architect_tools_test.go
Comment on lines
123
to
140
| // Config events: Write on .toml files | ||
| if event.Has(fsnotify.Write) && strings.HasSuffix(base, ".toml") { | ||
| if err := waitForStable(path); err != nil { | ||
| w.logger.Warn("Skipping config file. Reason: not stable after polling", | ||
| "path", path, | ||
| "error", err, | ||
| ) | ||
| continue | ||
| } | ||
|
|
||
| dir := w.resolveDir(path) | ||
| select { | ||
| case w.events <- WatcherEvent{Path: path, Dir: dir, Kind: EventKindConfig}: | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| continue | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For fsnotify v1.9.0, when a file inside a watched directory is updated via temp-file + rename, which events are emitted for the final destination path? Are Write-only handlers sufficient to detect updates to pool.toml?
💡 Result:
What events does fsnotify v1.9.0 emit for the final destination path with temp-file + rename?
It depends on the platform backend, but on Linux/inotify (fsnotify’s most common backend) the key detail is:
- The destination of a rename into a watched directory (inotify
IN_MOVED_TO) is mapped tofsnotify.Create, notfsnotify.Rename. In v1.9.0,newEvent()setsCreatewhen it seesIN_MOVED_TO. [1] - The source side (
IN_MOVED_FROM) is what maps tofsnotify.Rename. [1]
So if an updater does the usual atomic replace:
- write
pool.toml.tmp(you’ll typically seeWriteevents for the temp file), then rename(pool.toml.tmp, pool.toml)
…then the final path pool.toml commonly shows up as Create (and sometimes additional attribute events), not Write—because the bytes were written to the temp file, not to pool.toml. This is exactly why fsnotify recommends watching the directory and not relying on watching a single file for atomic saves. [2]
Are Write-only handlers sufficient to detect updates to pool.toml?
No. If updates happen via temp-file + rename, a Write-only handler can miss updates to pool.toml because the final step may only produce Create and/or Rename-related activity (platform-dependent) rather than Write on pool.toml. [1][2][3]
Practical rule: treat any of these on pool.toml as “changed”: Create, Write, Rename, (often also Remove). [3]
Sources:
[1] fsnotify v1.9.0 inotify backend (IN_MOVED_TO ⇒ Create, IN_MOVED_FROM ⇒ Rename) https://github.com/fsnotify/fsnotify/blob/v1.9.0/backend_inotify.go
[2] fsnotify docs: atomic saves via temp file + move break file-watches; watch the parent dir instead https://pkg.go.dev/github.com/fsnotify/fsnotify
[3] fsnotify issue discussion: atomic-save patterns require watching more than Write fsnotify/fsnotify#372
🏁 Script executed:
# Examine the watcher.go file around lines 123-140
cat -n internal/daemon/watcher.go | sed -n '115,150p'Repository: cameronsjo/agent-pool
Length of output: 1088
🏁 Script executed:
# Check for pool.toml references in daemon files
rg 'pool\.toml' internal/daemon/Repository: cameronsjo/agent-pool
Length of output: 4131
🏁 Script executed:
# Check what event types are being handled overall in the watcher
rg 'event\.Has\(' internal/daemon/watcher.go -A 2 -B 2Repository: cameronsjo/agent-pool
Length of output: 510
🏁 Script executed:
# Check the daemon.go event handling for pool.toml
cat -n internal/daemon/daemon.go | grep -A 10 -B 5 "pool.toml"Repository: cameronsjo/agent-pool
Length of output: 1794
🏁 Script executed:
# Check how watcher.Add is called and what directories are watched
rg 'watcher\.Add|watcher\.Watch' internal/daemon/daemon.go -B 3 -A 3Repository: cameronsjo/agent-pool
Length of output: 1618
Detect config changes via Create and Rename events, not just Write.
The matcher at line 124 only reacts to fsnotify.Write. When pool.toml is updated via atomic save (temp-file + rename), the destination file appears as fsnotify.Create on Linux/inotify, not Write. This causes hot-reload to silently miss configuration changes.
Suggested fix
- // Config events: Write on .toml files
- if event.Has(fsnotify.Write) && strings.HasSuffix(base, ".toml") {
+ // Config events: updates to pool.toml
+ if base == "pool.toml" &&
+ (event.Has(fsnotify.Write) || event.Has(fsnotify.Create) || event.Has(fsnotify.Rename)) {📝 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
| // Config events: Write on .toml files | |
| if event.Has(fsnotify.Write) && strings.HasSuffix(base, ".toml") { | |
| if err := waitForStable(path); err != nil { | |
| w.logger.Warn("Skipping config file. Reason: not stable after polling", | |
| "path", path, | |
| "error", err, | |
| ) | |
| continue | |
| } | |
| dir := w.resolveDir(path) | |
| select { | |
| case w.events <- WatcherEvent{Path: path, Dir: dir, Kind: EventKindConfig}: | |
| case <-ctx.Done(): | |
| return | |
| } | |
| continue | |
| } | |
| // Config events: updates to pool.toml | |
| if base == "pool.toml" && | |
| (event.Has(fsnotify.Write) || event.Has(fsnotify.Create) || event.Has(fsnotify.Rename)) { | |
| if err := waitForStable(path); err != nil { | |
| w.logger.Warn("Skipping config file. Reason: not stable after polling", | |
| "path", path, | |
| "error", err, | |
| ) | |
| continue | |
| } | |
| dir := w.resolveDir(path) | |
| select { | |
| case w.events <- WatcherEvent{Path: path, Dir: dir, Kind: EventKindConfig}: | |
| case <-ctx.Done(): | |
| return | |
| } | |
| continue | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/daemon/watcher.go` around lines 123 - 140, The config-change branch
currently only checks for fsnotify.Write so atomic saves (temp-file + rename)
are missed; update the conditional in the watcher loop to treat fsnotify.Create
and fsnotify.Rename the same as Write for .toml files (e.g., change the check
around event.Has(fsnotify.Write) to include event.Has(fsnotify.Create) ||
event.Has(fsnotify.Rename)), then keep the existing stability check via
waitForStable(path) and the subsequent resolveDir + send WatcherEvent{Path:
path, Dir: dir, Kind: EventKindConfig} to w.events (respecting ctx.Done()) so
renames/creates trigger the same hot-reload path.
Comment on lines
+354
to
+364
| // writeFormula writes a TOML formula file into the pool's formulas/ directory. | ||
| func writeFormula(t *testing.T, poolDir, name, content string) { | ||
| t.Helper() | ||
| dir := filepath.Join(poolDir, "formulas") | ||
| if err := os.MkdirAll(dir, 0o755); err != nil { | ||
| t.Fatalf("creating formulas dir: %v", err) | ||
| } | ||
| if err := os.WriteFile(filepath.Join(dir, name+".toml"), []byte(content), 0o644); err != nil { | ||
| t.Fatalf("writing formula: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move writeFormula into testhelp_test.go.
This is a new MCP test helper, so keeping it with the existing shared helpers avoids helper drift across internal/mcp/*_test.go. As per coding guidelines "Store MCP test helpers in testhelp_test.go (makePoolDirs, buildMCPTestServer, callTool, resultText); do not duplicate in new test files".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/mcp/architect_tools_test.go` around lines 354 - 364, The new helper
function writeFormula in internal/mcp/architect_tools_test.go should be moved
into the shared MCP test helpers file testhelp_test.go to avoid duplication;
remove writeFormula from architect_tools_test.go and add it to testhelp_test.go
alongside existing helpers (makePoolDirs, buildMCPTestServer, callTool,
resultText), keeping the same signature and t.Helper() behavior so other tests
can reuse it.
New internal/formula/ package for TOML workflow template parsing. Formulas are reusable DAG templates that the architect instantiates to bulk-create tasks with correct dependency edges. - Formula and Step structs with TOML tags - Load() and LoadAll() for single/directory parsing - Validate() checks: empty fields, duplicate IDs, unknown deps, cycles - Cycle detection via Kahn's algorithm (mirrors taskboard approach) - 19 tests covering happy paths, error cases, and DAG validation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
New MCP tool for the architect to bulk-create tasks from formula templates. Loads a TOML formula, applies a prefix to task IDs and dependency edges, resolves expert assignments and body overrides, then posts all messages to the postoffice. - Validates prefix is filename-safe - Requires explicit expert assignment for role="experts" steps - Supports JSON overrides map for custom task bodies - 5 new tests covering happy paths and error cases Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Watch poolDir for .toml Write events. On pool.toml change: waitForStable, re-parse with config.LoadPool, validate, swap under lock. Parse failures keep the old config (log warning). - Watcher now handles two event kinds: EventKindMail (.md Create) and EventKindConfig (.toml Write) - reloadConfig() diffs old/new experts, creates dirs and watches for added experts, preserves dirs for removed ones - New EventConfigReloaded event type with added/removed expert lists - 2 integration tests: add expert via reload, invalid TOML keeps old Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add formulas/ directory to daemon's ensureDirs for auto-creation - Audited all daemon-consumed file writes: postoffice, taskboard, and mail routing already use atomicfile.WriteFile. Approval and log files use os.WriteFile (acceptable — approval files are tiny and polled, logs are write-once and not watched). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cameronsjo
force-pushed
the
feat/v0.9-formulas-polish
branch
from
813874b to
eef01bd
Compare
CodeRabbit findings addressed: - Guard Validate() against nil formula pointer - Reject step IDs with path separators (filename-safe check) - Validate formula name against path traversal (filepath.Base check) - Restrict experts map to role="experts" steps only - Add approval gate to instantiate_formula (matches send_task pattern) - Compose all messages before posting, roll back on partial failure - Hold daemon mutex for full reloadConfig duration (prevent races) - Only emit EventConfigReloaded on full success, not partial failure - Handle both Write and Create events for .toml (covers atomic rename) - Move writeFormula helper to testhelp_test.go per project conventions - Add language tag to fenced code block in dev prompt Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (9)
docs/prompts/v09-formulas-polish.md (1)
98-104:⚠️ Potential issue | 🟡 MinorRestore the fence language on the directory tree.
This untyped fence will re-trigger the docs lint noise;
textis enough here.Suggested fix
-``` +```text {poolDir}/formulas/ ├── feature-impl.toml # Standard feature flow ├── bug-triage.toml # Bug investigation + fix ├── code-review.toml # Review + feedback cycle └── index.md # Auto-generated summary</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/prompts/v09-formulas-polish.mdaround lines 98 - 104, The fenced code
block showing the directory tree for {poolDir}/formulas/ is missing a language
tag and causing lint noise; update the opening fence fromtotext so the
block reads as a text fence (affecting the block containing
"{poolDir}/formulas/" and the tree lines like "├── feature-impl.toml") to
silence the linter.</details> </blockquote></details> <details> <summary>internal/formula/formula.go (2)</summary><blockquote> `85-87`: _⚠️ Potential issue_ | _🟠 Major_ **Guard `Validate` against `nil`.** `Validate(nil)` panics here because `f` is dereferenced before any nil check. Return a regular error instead so callers can handle invalid input. <details> <summary>Suggested fix</summary> ```diff func Validate(f *Formula) error { + if f == nil { + return fmt.Errorf("formula is nil") + } if len(f.Steps) == 0 { return fmt.Errorf("formula has no steps") } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@internal/formula/formula.go` around lines 85 - 87, The Validate function currently dereferences f (accessing f.Steps) without checking for nil; add an initial guard in Validate to return a clear error when f == nil (e.g., "nil formula") before any use of f or f.Steps so callers get an error instead of a panic; update the function Validate and any related tests to expect an error for a nil *Formula. ``` </details> --- `91-104`: _⚠️ Potential issue_ | _🟠 Major_ **Reject step IDs that cannot become task/message IDs.** A step like `foo/bar` or `two words` passes validation here, but `instantiate_formula` later prefixes `step.ID` and uses it as a task/message ID. That fails late, potentially after earlier tasks were already created. <details> <summary>Suggested fix</summary> ```diff for _, s := range f.Steps { if s.ID == "" { return fmt.Errorf("step has empty id") } + if s.ID != filepath.Base(s.ID) || strings.ContainsAny(s.ID, " \t\r\n") || s.ID == "." || s.ID == ".." { + return fmt.Errorf("step %q has invalid id", s.ID) + } if s.Role == "" { return fmt.Errorf("step %q has empty role", s.ID) } ``` </details> Based on learnings "Ensure message IDs are filename-safe as they are used as filenames in routing and logging". <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@internal/formula/formula.go` around lines 91 - 104, The loop validating steps (for _, s := range f.Steps) currently allows IDs like "foo/bar" or "two words" which later break when instantiate_formula prefixes step.ID for task/message IDs; update this validation to reject any s.ID that is not filename-safe (e.g., only allow alphanumeric, underscore, hyphen, dot) by adding a check in the same validation path (before marking ids[s.ID]=true) and return a clear error like "invalid step id %q: must be filename-safe"; reference the step.ID check in the for _, s := range f.Steps loop and ensure compatibility with instantiate_formula usage. ``` </details> </blockquote></details> <details> <summary>internal/mcp/architect_tools_test.go (1)</summary><blockquote> `354-364`: _🛠️ Refactor suggestion_ | _🟠 Major_ **Move `writeFormula` into `testhelp_test.go`.** This is a shared MCP filesystem helper, so keeping it here breaks the repo’s test-helper convention and invites helper drift across future MCP tests. As per coding guidelines "Store MCP test helpers in testhelp_test.go (makePoolDirs, buildMCPTestServer, callTool, resultText); do not duplicate in new test files". <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@internal/mcp/architect_tools_test.go` around lines 354 - 364, The helper function writeFormula is duplicated in this test file; move its implementation into the shared MCP test helper file testhelp_test.go (alongside makePoolDirs, buildMCPTestServer, callTool, resultText), remove the duplicate from internal/mcp/architect_tools_test.go, and update any references to continue calling writeFormula; ensure the function remains package-test-scoped (keep signature func writeFormula(t *testing.T, poolDir, name, content string)) and that testhelp_test.go imports filepath and os so the helper compiles. ``` </details> </blockquote></details> <details> <summary>internal/daemon/daemon.go (1)</summary><blockquote> `1325-1392`: _⚠️ Potential issue_ | _🔴 Critical_ **Stage reload state before swapping `d.cfg`.** This assigns the new config before `ensureDirs()` / watch updates finish and drops the lock while other goroutines still read `d.cfg` and `d.sharedSet`. That creates a real race, and it can also report a successful reload after a partial setup failure. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@internal/daemon/daemon.go` around lines 1325 - 1392, The code swaps d.cfg and rebuilds d.sharedSet while releasing d.mu before finishing filesystem/watch setup, which can race with readers; change the sequence to stage and validate the new config under the lock: compute oldCfg and newCfg, build the new sharedSet and determine added/removed experts into local variables while holding d.mu, but do NOT assign d.cfg or d.sharedSet yet; then perform ensureDirs() and watcher.Add(...) for each added expert outside the lock and collect any errors; only after those setup steps succeed (or after deciding how to handle partial failures) acquire d.mu again and atomically assign d.cfg = newCfg and d.sharedSet = stagedSharedSet, then unlock and emit the Event and final log using the computed added/removed lists (functions/methods: d.mu.Lock()/Unlock(), d.cfg, d.sharedSet, ensureDirs(), watcher.Add()). ``` </details> </blockquote></details> <details> <summary>internal/daemon/watcher.go (1)</summary><blockquote> `123-139`: _⚠️ Potential issue_ | _🟠 Major_ **Handle atomic-save updates for `pool.toml`.** This branch only reacts to `fsnotify.Write`. For directory watches, atomic save/rename flows surface the destination path as `Create`, while `Rename` is reported on the old path, so a temp-file + rename update to `pool.toml` can bypass hot-reload entirely. The fsnotify docs also recommend watching the parent directory and filtering on `Event.Name`, which this code already does. ([pkg.go.dev](https://pkg.go.dev/github.com/fsnotify/fsnotify)) ```web For github.com/fsnotify/fsnotify, when a watched directory receives an atomic save (write temp file then rename to pool.toml), which events are emitted for the final path, and should handlers watch Create in addition to Write? ``` <details> <summary>Suggested fix</summary> ```diff - // Config events: Write on .toml files - if event.Has(fsnotify.Write) && strings.HasSuffix(base, ".toml") { + // Config events: updates to pool.toml + if base == "pool.toml" && + (event.Has(fsnotify.Write) || event.Has(fsnotify.Create) || event.Has(fsnotify.Rename)) { if err := waitForStable(path); err != nil { ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@internal/daemon/watcher.go` around lines 123 - 139, The code only reacts to fsnotify.Write for .toml files which misses atomic-save flows; update the branch around waitForStable/resolveDir/WatcherEvent to also handle fsnotify.Create (and optionally fsnotify.Rename if you want to catch rename-to-target events) so temp-file+rename updates to pool.toml trigger the same hot-reload path: change the condition to include event.Has(fsnotify.Create) (and/or event.Has(fsnotify.Rename)), keep the same waitForStable(path) check and the existing send to w.events (WatcherEvent{Path: path, Dir: dir, Kind: EventKindConfig}) and the ctx.Done() select so behavior is identical for Create/Rename as for Write. ``` </details> </blockquote></details> <details> <summary>internal/mcp/architect_tools.go (3)</summary><blockquote> `358-362`: _⚠️ Potential issue_ | _🟠 Major_ **Experts map can override recipients for non-`experts` roles.** The validation at Lines 341-348 correctly enforces that `role="experts"` steps must have an expert assignment. However, the recipient resolution at Lines 358-362 applies the `experts` map to **any** step, allowing callers to reroute `architect` or `concierge` steps. Restrict the override to only `role == "experts"` steps. <details> <summary>🔒 Proposed fix</summary> ```diff // Resolve recipient to := step.Role - if expertName, ok := experts[step.ID]; ok { - to = expertName + if step.Role == "experts" { + if expertName, ok := experts[step.ID]; ok { + to = expertName + } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@internal/mcp/architect_tools.go` around lines 358 - 362, The recipient override currently applies the experts map to every step; change the logic so the experts map is used only when the step's role is "experts" — i.e., when resolving recipient (currently using variables to := step.Role and experts[step.ID]) guard the override with a check like step.Role == "experts" before setting to = expertName so that architect/concierge steps cannot be rerouted. ``` </details> --- `307-321`: _⚠️ Potential issue_ | _🔴 Critical_ **Path traversal vulnerability on `formulaName` still present.** The `prefix` parameter is validated with `filepath.Base` check (Line 313-315), but `formulaName` lacks the same validation. An attacker can supply `../pool` or `../../etc/passwd` to read arbitrary TOML files outside the `formulas/` directory. <details> <summary>🔒 Proposed fix</summary> ```diff if formulaName == "" { return mcp.NewToolResultError("formula parameter is required"), nil } + if formulaName != filepath.Base(formulaName) || formulaName == "." || formulaName == ".." { + return mcp.NewToolResultError(fmt.Sprintf("invalid formula %q: must be a simple filename-safe string", formulaName)), nil + } if prefix == "" { ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@internal/mcp/architect_tools.go` around lines 307 - 321, The code accepts unvalidated formulaName and constructs formulaPath before calling formula.Load, allowing path traversal; mirror the prefix validation for formulaName by ensuring filepath.Base(formulaName) == formulaName and formulaName != "." && formulaName != ".." and reject any string containing path separators or absolute paths, then build formulaPath from cfg.PoolDir + "formulas" and formulaName+".toml" as before; place this check near the existing prefix validation (same function) so formula.Load is only called with a safe, filename-only formulaName. ``` </details> --- `350-390`: _⚠️ Potential issue_ | _🟠 Major_ **Partial publish on failure leaves orphaned tasks with dangling dependencies.** If `postMessage` fails at step N (Line 387-389), steps 1…N-1 are already persisted. The caller cannot safely retry because: 1. Some tasks are already queued with `DependsOn` references to tasks that may never be created. 2. The daemon's `ValidateAdd` (per context snippet 4) doesn't validate that referenced task IDs exist. 3. Unlike `send_task`, there's no `shouldRequireApproval` gate. Consider pre-validating and composing all messages before any writes, then publishing atomically or implementing cleanup on failure. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@internal/mcp/architect_tools.go` around lines 350 - 390, The loop builds and posts mail.Message objects one-by-one via postMessage which can leave persisted earlier tasks with dangling DependsOn if a subsequent post fails; change to fully compose and validate all messages first (iterate f.Steps to build a slice of *mail.Message, apply prefix to DependsOn, run the same validation used by ValidateAdd and any shouldRequireApproval checks) and only after successful validation atomically persist them (single batch write) or, if batching isn't possible, attempt to post them and on any postMessage error perform cleanup by deleting/rolling back already-posted taskIDs (use the taskIDs slice and mcp.NewToolResultError to return the failure), ensuring no orphaned DependsOn references remain. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@docs/plans/2026-04-06-v09-formulas-polish.md:
- Around line 156-159: The document currently cites functions with fragile line
numbers (e.g.,splitCSVat:301andRegisterArchitectToolsat:23);
update these references to use symbol-only references instead of line
numbers—e.g., mentionsplitCSV,RegisterArchitectTools,
atomicfile.WriteFile, andinternal/mcp/postoffice.go:postMessage(or just
postMessage) so the plan points to identifiers rather than file line offsets,
ensuring links remain accurate as code changes.- Around line 97-104: The fenced code block under "Watcher Changes" is missing a
language identifier; update that fenced block to include a language specifier
(e.g.,text ordiff) so syntax/highlighting is correct — locate the block
that documents Run(), WatcherEvent.Dir, waitForStable, pool.toml, poolDir and
postoffice and add the language tag to the opening ``` line.In
@internal/daemon/daemon.go:
- Around line 1325-1334: The reload path updates d.cfg and d.sharedSet but
doesn't rebuild d.curation, so update the reload logic (the block that sets
d.cfg and d.sharedSet) to also recreate d.curation from newCfg: either invoke
the same constructor logic used in New() or extract a helper like
buildCurationFromConfig(newCfg) and assign its result to d.curation while
holding d.mu; preserve any validation/error handling the New() path performs and
ensure the assignment is done under the same lock so the daemon uses the new
thresholds/intervals immediately after reload.- Around line 1336-1380: The reload logic only diffs
oldCfg.Experts/newCfg.Experts so shared experts are missed; update the
added/removed computation to build oldExperts and newExperts by iterating both
Experts and SharedExperts (e.g., loop over oldCfg.Experts and
oldCfg.SharedExperts into oldExperts, and likewise for newCfg) so newly added
shared experts get watched inboxes; then when handling removed entries, call
watcher.Remove(mail.ResolveInbox(d.poolDir, name)) (while still preserving
on-disk dirs) and when handling added entries call
watcher.Add(mail.ResolveInbox(d.poolDir, name)); keep using d.ensureDirs() to
create dirs but ensure watcher.Add/Remove are applied for both regular and
shared expert names.
Duplicate comments:
In@docs/prompts/v09-formulas-polish.md:
- Around line 98-104: The fenced code block showing the directory tree for
{poolDir}/formulas/ is missing a language tag and causing lint noise; update the
opening fence fromtotext so the block reads as a text fence (affecting
the block containing "{poolDir}/formulas/" and the tree lines like "├──
feature-impl.toml") to silence the linter.In
@internal/daemon/daemon.go:
- Around line 1325-1392: The code swaps d.cfg and rebuilds d.sharedSet while
releasing d.mu before finishing filesystem/watch setup, which can race with
readers; change the sequence to stage and validate the new config under the
lock: compute oldCfg and newCfg, build the new sharedSet and determine
added/removed experts into local variables while holding d.mu, but do NOT assign
d.cfg or d.sharedSet yet; then perform ensureDirs() and watcher.Add(...) for
each added expert outside the lock and collect any errors; only after those
setup steps succeed (or after deciding how to handle partial failures) acquire
d.mu again and atomically assign d.cfg = newCfg and d.sharedSet =
stagedSharedSet, then unlock and emit the Event and final log using the computed
added/removed lists (functions/methods: d.mu.Lock()/Unlock(), d.cfg,
d.sharedSet, ensureDirs(), watcher.Add()).In
@internal/daemon/watcher.go:
- Around line 123-139: The code only reacts to fsnotify.Write for .toml files
which misses atomic-save flows; update the branch around
waitForStable/resolveDir/WatcherEvent to also handle fsnotify.Create (and
optionally fsnotify.Rename if you want to catch rename-to-target events) so
temp-file+rename updates to pool.toml trigger the same hot-reload path: change
the condition to include event.Has(fsnotify.Create) (and/or
event.Has(fsnotify.Rename)), keep the same waitForStable(path) check and the
existing send to w.events (WatcherEvent{Path: path, Dir: dir, Kind:
EventKindConfig}) and the ctx.Done() select so behavior is identical for
Create/Rename as for Write.In
@internal/formula/formula.go:
- Around line 85-87: The Validate function currently dereferences f (accessing
f.Steps) without checking for nil; add an initial guard in Validate to return a
clear error when f == nil (e.g., "nil formula") before any use of f or f.Steps
so callers get an error instead of a panic; update the function Validate and any
related tests to expect an error for a nil *Formula.- Around line 91-104: The loop validating steps (for _, s := range f.Steps)
currently allows IDs like "foo/bar" or "two words" which later break when
instantiate_formula prefixes step.ID for task/message IDs; update this
validation to reject any s.ID that is not filename-safe (e.g., only allow
alphanumeric, underscore, hyphen, dot) by adding a check in the same validation
path (before marking ids[s.ID]=true) and return a clear error like "invalid step
id %q: must be filename-safe"; reference the step.ID check in the for _, s :=
range f.Steps loop and ensure compatibility with instantiate_formula usage.In
@internal/mcp/architect_tools_test.go:
- Around line 354-364: The helper function writeFormula is duplicated in this
test file; move its implementation into the shared MCP test helper file
testhelp_test.go (alongside makePoolDirs, buildMCPTestServer, callTool,
resultText), remove the duplicate from internal/mcp/architect_tools_test.go, and
update any references to continue calling writeFormula; ensure the function
remains package-test-scoped (keep signature func writeFormula(t *testing.T,
poolDir, name, content string)) and that testhelp_test.go imports filepath and
os so the helper compiles.In
@internal/mcp/architect_tools.go:
- Around line 358-362: The recipient override currently applies the experts map
to every step; change the logic so the experts map is used only when the step's
role is "experts" — i.e., when resolving recipient (currently using variables to
:= step.Role and experts[step.ID]) guard the override with a check like
step.Role == "experts" before setting to = expertName so that
architect/concierge steps cannot be rerouted.- Around line 307-321: The code accepts unvalidated formulaName and constructs
formulaPath before calling formula.Load, allowing path traversal; mirror the
prefix validation for formulaName by ensuring filepath.Base(formulaName) ==
formulaName and formulaName != "." && formulaName != ".." and reject any string
containing path separators or absolute paths, then build formulaPath from
cfg.PoolDir + "formulas" and formulaName+".toml" as before; place this check
near the existing prefix validation (same function) so formula.Load is only
called with a safe, filename-only formulaName.- Around line 350-390: The loop builds and posts mail.Message objects one-by-one
via postMessage which can leave persisted earlier tasks with dangling DependsOn
if a subsequent post fails; change to fully compose and validate all messages
first (iterate f.Steps to build a slice of *mail.Message, apply prefix to
DependsOn, run the same validation used by ValidateAdd and any
shouldRequireApproval checks) and only after successful validation atomically
persist them (single batch write) or, if batching isn't possible, attempt to
post them and on any postMessage error perform cleanup by deleting/rolling back
already-posted taskIDs (use the taskIDs slice and mcp.NewToolResultError to
return the failure), ensuring no orphaned DependsOn references remain.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: ASSERTIVE **Plan**: Pro **Run ID**: `af7c5ae5-d22b-442d-9def-40e2bacdc152` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 813874b97519562f6a004b548cefcb3a9c1dd693 and eef01bde3f0c474ab1f6bdf9ff4e5e79fa4f86d4. </details> <details> <summary>📒 Files selected for processing (10)</summary> * `docs/plans/2026-04-06-v09-formulas-polish.md` * `docs/prompts/v09-formulas-polish.md` * `internal/daemon/daemon.go` * `internal/daemon/daemon_test.go` * `internal/daemon/events.go` * `internal/daemon/watcher.go` * `internal/formula/formula.go` * `internal/formula/formula_test.go` * `internal/mcp/architect_tools.go` * `internal/mcp/architect_tools_test.go` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+1336
to
+1380
| // Diff experts: find added and removed | ||
| oldExperts := make(map[string]bool, len(oldCfg.Experts)) | ||
| for name := range oldCfg.Experts { | ||
| oldExperts[name] = true | ||
| } | ||
| newExperts := make(map[string]bool, len(newCfg.Experts)) | ||
| for name := range newCfg.Experts { | ||
| newExperts[name] = true | ||
| } | ||
|
|
||
| var added, removed []string | ||
| for name := range newExperts { | ||
| if !oldExperts[name] { | ||
| added = append(added, name) | ||
| } | ||
| } | ||
| for name := range oldExperts { | ||
| if !newExperts[name] { | ||
| removed = append(removed, name) | ||
| } | ||
| } | ||
|
|
||
| // Create dirs and watch new expert inboxes | ||
| if len(added) > 0 { | ||
| if err := d.ensureDirs(); err != nil { | ||
| d.logger.Error("Failed to create directories for new experts", | ||
| "error", err, | ||
| ) | ||
| } | ||
| for _, name := range added { | ||
| inboxDir := mail.ResolveInbox(d.poolDir, name) | ||
| if err := watcher.Add(inboxDir); err != nil { | ||
| d.logger.Error("Failed to watch new expert inbox", | ||
| "expert", name, | ||
| "error", err, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Log removed experts (don't delete dirs — data preservation) | ||
| for _, name := range removed { | ||
| d.logger.Info("Expert removed from config (inbox preserved)", | ||
| "expert", name, | ||
| ) |
There was a problem hiding this comment.
Diff the full watched-inbox set, not just cfg.Experts.
Reload only computes added/removed regular experts. Newly added shared experts get their directories from ensureDirs(), but their inboxes never get watcher.Add, so routed mail will just sit there; removed inboxes also stay subscribed until restart.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/daemon/daemon.go` around lines 1336 - 1380, The reload logic only
diffs oldCfg.Experts/newCfg.Experts so shared experts are missed; update the
added/removed computation to build oldExperts and newExperts by iterating both
Experts and SharedExperts (e.g., loop over oldCfg.Experts and
oldCfg.SharedExperts into oldExperts, and likewise for newCfg) so newly added
shared experts get watched inboxes; then when handling removed entries, call
watcher.Remove(mail.ResolveInbox(d.poolDir, name)) (while still preserving
on-disk dirs) and when handling added entries call
watcher.Add(mail.ResolveInbox(d.poolDir, name)); keep using d.ensureDirs() to
create dirs but ensure watcher.Add/Remove are applied for both regular and
shared expert names.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
internal/daemon/daemon.go (2)
1337-1374:⚠️ Potential issue | 🟠 MajorShared expert changes still do not participate in reload diffing.
Lines 1338-1352 only compare
oldCfg.ExpertsandnewCfg.Experts. Changes undershared.includenever reachensureDirs()orwatcher.Add(), so newly added shared experts remain unwatched until restart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/daemon/daemon.go` around lines 1337 - 1374, The diff only compares oldCfg.Experts vs newCfg.Experts so experts referenced via shared.include are ignored; update the diffing logic that builds oldExperts, added, and removed (the block using oldCfg.Experts, newCfg.Experts, added/removed) to merge in shared includes from both configurations (e.g., collect names from newCfg.Shared.include and oldCfg.Shared.include into the respective sets) before computing added/removed, so ensureDirs() and watcher.Add(mail.ResolveInbox(...)) are invoked for newly added shared experts as well.
1327-1376:⚠️ Potential issue | 🔴 Critical
reloadConfigstill exposes half-applied state to live handlers.Line 1329 publishes
d.cfgbeforeensureDirs()/watcher.Add(), but readers likeresolveExpertConfig,resolveSessionTimeout,sharedNamesMap, andisSharedExpertnever taked.mu.watcher.Add()also mutatesWatcher.dirswhileWatcher.Run()callsresolveDir()without synchronization. A hot-reload can therefore race live routing and, if setup later fails, leave the new config active without fully created/watched inboxes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/daemon/daemon.go` around lines 1327 - 1376, The reload publishes d.cfg too early and races live readers and Watcher.Run; fix by not assigning d.cfg or d.sharedSet until after creating dirs and successfully adding inboxes: compute the added/removed lists from oldCfg and newCfg without mutating d, call d.ensureDirs() and call watcher.Add(...) for each inbox (hold d.mu while calling watcher.Add or make watcher.Add thread-safe) and if all setup succeeds then take d.mu and assign d.cfg = newCfg and rebuild d.sharedSet; if setup fails leave the old d.cfg in place and log the failure. Ensure to reference/modify the reload logic around d.cfg, d.sharedSet, ensureDirs, watcher.Add, and the added/removed computation so the swap is atomic from readers like resolveExpertConfig/resolveSessionTimeout/sharedNamesMap/isSharedExpert and avoids races with Watcher.Run/resolveDir.internal/mcp/architect_tools.go (1)
344-367:⚠️ Potential issue | 🟠 MajorReject blank expert assignments during preflight.
Line 347 only checks key presence.
{"implement": ""}passes validation, and Line 366 then produces an empty recipient after the batch has already cleared preflight.Suggested fix
for _, step := range f.Steps { if step.Role == "experts" { - if _, ok := experts[step.ID]; !ok { + assigned := strings.TrimSpace(experts[step.ID]) + if assigned == "" { return mcp.NewToolResultError(fmt.Sprintf( "step %q has role 'experts' but no expert assigned in experts map", step.ID)), nil } + experts[step.ID] = assigned } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/architect_tools.go` around lines 344 - 367, The preflight validation only checks experts map membership but not whether the assigned value is non-empty; update the validation loop over f.Steps (the block that checks if step.Role == "experts") to fetch the value from experts[step.ID], trim whitespace, and return an mcp.NewToolResultError if the value is missing or empty (e.g., empty string or only whitespace) so blank assignments like {"implement": ""} are rejected before message composition; this ensures the later use of experts[step.ID] for the recipient (the to variable) is always a non-empty string.internal/daemon/watcher.go (1)
123-126:⚠️ Potential issue | 🟠 MajorHandle
Renamefor atomicpool.tomlsaves.This still ignores editors/backends that surface the final replace as
fsnotify.Rename, so hot-reload can silently miss valid config updates.Suggested fix
- if (event.Has(fsnotify.Write) || event.Has(fsnotify.Create)) && strings.HasSuffix(base, ".toml") { + if (event.Has(fsnotify.Write) || event.Has(fsnotify.Create) || event.Has(fsnotify.Rename)) && strings.HasSuffix(base, ".toml") {For fsnotify v1.9.0, when a watched file like pool.toml is updated via temp-file + rename on macOS or Windows, can the destination change be reported as Rename rather than Create or Write? Should hot-reload handlers include Rename alongside Create and Write?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/daemon/watcher.go` around lines 123 - 126, The watcher ignores fsnotify.Rename so atomic saves that emit Rename (not Create/Write) are missed; update the condition that checks the event (the line using event.Has(fsnotify.Write) || event.Has(fsnotify.Create) && strings.HasSuffix(base, ".toml")) to also include event.Has(fsnotify.Rename) so Rename is treated the same as Create/Write for ".toml" files (keep the strings.HasSuffix(base, ".toml") check and the same hot-reload handling codepath that follows).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/formula/formula_test.go`:
- Around line 42-57: The test fixture writes call to os.WriteFile currently
ignores the returned error, which can hide setup failures; update each
os.WriteFile call in formula_test.go (the ones at the shown snippet and the
additional occurrences noted) to capture the error (err := os.WriteFile(...))
and immediately fail the test on error (e.g., t.Fatalf("failed to write fixture:
%v", err) or use require.NoError(t, err)) so the test stops with a clear setup
error instead of a downstream parse/validation failure.
In `@internal/mcp/architect_tools.go`:
- Around line 404-413: The rollback is unsafe because postMessage writes
directly into the watched postoffice (cfg.PoolDir + "/postoffice") so earlier
messages can be routed before a later post fails; fix by making publishing
atomic: change the Phase 2 loop and postMessage usage so messages are first
written into an unobserved staging location (e.g., cfg.PoolDir + "/staging" or a
temp dir) and only after all writes succeed, atomically rename/move them into
postoffice (or write a single batch marker file and then move all files). Update
postMessage (or wrap it) to support writing to staging, and implement the final
atomic move into postoffice after the loop; on failure, delete staging files
(not postoffice) so no partial routing occurs. Ensure you reference postMessage,
posted, postofficeDir and cfg.PoolDir in the changes.
---
Duplicate comments:
In `@internal/daemon/daemon.go`:
- Around line 1337-1374: The diff only compares oldCfg.Experts vs newCfg.Experts
so experts referenced via shared.include are ignored; update the diffing logic
that builds oldExperts, added, and removed (the block using oldCfg.Experts,
newCfg.Experts, added/removed) to merge in shared includes from both
configurations (e.g., collect names from newCfg.Shared.include and
oldCfg.Shared.include into the respective sets) before computing added/removed,
so ensureDirs() and watcher.Add(mail.ResolveInbox(...)) are invoked for newly
added shared experts as well.
- Around line 1327-1376: The reload publishes d.cfg too early and races live
readers and Watcher.Run; fix by not assigning d.cfg or d.sharedSet until after
creating dirs and successfully adding inboxes: compute the added/removed lists
from oldCfg and newCfg without mutating d, call d.ensureDirs() and call
watcher.Add(...) for each inbox (hold d.mu while calling watcher.Add or make
watcher.Add thread-safe) and if all setup succeeds then take d.mu and assign
d.cfg = newCfg and rebuild d.sharedSet; if setup fails leave the old d.cfg in
place and log the failure. Ensure to reference/modify the reload logic around
d.cfg, d.sharedSet, ensureDirs, watcher.Add, and the added/removed computation
so the swap is atomic from readers like
resolveExpertConfig/resolveSessionTimeout/sharedNamesMap/isSharedExpert and
avoids races with Watcher.Run/resolveDir.
In `@internal/daemon/watcher.go`:
- Around line 123-126: The watcher ignores fsnotify.Rename so atomic saves that
emit Rename (not Create/Write) are missed; update the condition that checks the
event (the line using event.Has(fsnotify.Write) || event.Has(fsnotify.Create) &&
strings.HasSuffix(base, ".toml")) to also include event.Has(fsnotify.Rename) so
Rename is treated the same as Create/Write for ".toml" files (keep the
strings.HasSuffix(base, ".toml") check and the same hot-reload handling codepath
that follows).
In `@internal/mcp/architect_tools.go`:
- Around line 344-367: The preflight validation only checks experts map
membership but not whether the assigned value is non-empty; update the
validation loop over f.Steps (the block that checks if step.Role == "experts")
to fetch the value from experts[step.ID], trim whitespace, and return an
mcp.NewToolResultError if the value is missing or empty (e.g., empty string or
only whitespace) so blank assignments like {"implement": ""} are rejected before
message composition; this ensures the later use of experts[step.ID] for the
recipient (the to variable) is always a non-empty string.
🪄 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: d4e2af38-26f6-4215-8eac-0223ff89e84c
📒 Files selected for processing (7)
internal/daemon/daemon.gointernal/daemon/watcher.gointernal/formula/formula.gointernal/formula/formula_test.gointernal/mcp/architect_tools.gointernal/mcp/architect_tools_test.gointernal/mcp/testhelp_test.go
Comment on lines
+404
to
+413
| // Phase 2: Post all messages (all-or-nothing best effort) | ||
| var posted []string | ||
| for _, msg := range messages { | ||
| if err := postMessage(cfg.PoolDir, msg); err != nil { | ||
| // Clean up already-posted files on failure | ||
| postofficeDir := filepath.Join(cfg.PoolDir, "postoffice") | ||
| for _, id := range posted { | ||
| os.Remove(filepath.Join(postofficeDir, id+".md")) | ||
| } | ||
| return mcp.NewToolResultError(fmt.Sprintf("posting task %s: %v (rolled back %d previously posted)", msg.ID, err, len(posted))), nil |
There was a problem hiding this comment.
This rollback is not atomic against a live daemon.
Line 410 only deletes the postoffice copy. The daemon watches postoffice and routes each new .md immediately, so by the time a later postMessage fails, earlier tasks may already be in inboxes or on the taskboard. Returning “rolled back” here makes retries unsafe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/mcp/architect_tools.go` around lines 404 - 413, The rollback is
unsafe because postMessage writes directly into the watched postoffice
(cfg.PoolDir + "/postoffice") so earlier messages can be routed before a later
post fails; fix by making publishing atomic: change the Phase 2 loop and
postMessage usage so messages are first written into an unobserved staging
location (e.g., cfg.PoolDir + "/staging" or a temp dir) and only after all
writes succeed, atomically rename/move them into postoffice (or write a single
batch marker file and then move all files). Update postMessage (or wrap it) to
support writing to staging, and implement the final atomic move into postoffice
after the loop; on failure, delete staging files (not postoffice) so no partial
routing occurs. Ensure you reference postMessage, posted, postofficeDir and
cfg.PoolDir in the changes.
Round 2 CodeRabbit findings: - Rebuild d.curation during config reload (stale scheduler) - Diff shared experts in addition to pool-scoped experts during reload — create dirs and watch inboxes for newly added shared experts - Add language tags to plan doc code fences, remove stale line refs Simplify pass (from /simplify review): - Fix data race on Watcher.dirs: add sync.Mutex, guard Add() and resolveDir() — map written from main goroutine, read from watcher - Move filesystem I/O outside d.mu in reloadConfig: lock held only for config swap and diff, dir creation and watcher setup unlocked - Create only new expert dirs during reload instead of full ensureDirs - Remove dead defensive guard in detectCycle (deps already validated) - Add missing blank line before EventBufSize const in events.go Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Check os.WriteFile errors in formula test fixtures (fail fast on setup failure instead of producing misleading parse errors) - Document rollback limitation in instantiate_formula: daemon routes postoffice files immediately, so cleanup cannot recall already-routed messages. Dependency evaluation prevents premature downstream execution 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
internal/formula/package with TOML workflow template parsing, DAG validation (cycle detection, duplicate IDs, missing refs), andLoad/LoadAll/ValidateAPIinstantiate_formulaarchitect MCP tool that bulk-creates tasks from formula templates with prefixed IDs, dependency edges, expert assignments, and body overridespool.tomlfor changes, re-parses and validates on write, swaps config under lock. New experts get inbox dirs created and watched automatically. Invalid TOML keeps the old configformulas/added toensureDirs, audited all daemon-consumed file writes for atomic write usageTest plan
go vet ./...clean🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Summary by CodeRabbit
New Features
Documentation
Tests