Hard cut coordination exchange paths#33
Merged
Merged
Conversation
added 6 commits
May 26, 2026 12:23
📝 WalkthroughWalkthroughThis PR migrates AIMUX state (notifications, threads, tasks) from file-backed JSON into a runtime-exchange store, implements deriveRuntimeExchangeIndexes, adds inbox/read/clear HTTP endpoints, updates server/client/UI/watcher/CLI code to use the runtime exchange, and updates agent documentation to prefer runtime-exchange-backed handoff/task/thread flows. ChangesRuntime Exchange State Storage and API Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/paths.ts (1)
6-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the comment to reflect that tasks and threads are no longer actively managed.
Line 6 still lists "tasks" and "threads" as agent-facing shared artifacts, but lines 339 and 360-363 show they are no longer created or migrated. According to the PR objectives, these are now legacy import-only paths.
📝 Proposed fix
- * (config, team, plans, context, history, tasks, status, threads) + * (config, team, plans, context, history, status)Add a note about legacy paths if needed:
+ * Legacy paths (import-only): tasks, threads🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/paths.ts` at line 6, Update the top-of-file comment in src/paths.ts that lists agent-facing shared artifacts to remove or mark "tasks" and "threads" as legacy/import-only (they are no longer created or migrated) so the comment matches current behavior; locate the comment block that enumerates "(config, team, plans, context, history, tasks, status, threads)" and either remove "tasks" and "threads" from the active list or append a short note like "tasks, threads — legacy/import-only" to make intent explicit.
🧹 Nitpick comments (4)
src/tasks.ts (1)
66-77: 💤 Low valueMutating the input
taskobject is a side effect.Lines 75-76 mutate the passed-in
taskobject. This could surprise callers who don't expect their reference to be modified. Consider returning a new object or documenting this behavior explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tasks.ts` around lines 66 - 77, The writeTask function currently mutates the input Task (task.updatedAt and task.reviewStatus); instead, create a new Task object (e.g., newTask) based on the incoming task, set updatedAt and normalized reviewStatus on that copy, and use newTask when calling toExchangeTask and when persisting; leave the original task untouched. Update references to task in writeTask to use the new copy and keep calls to nowIso(), normalizeReviewStatus(), toExchangeTask(), and createRuntimeExchangeStore().update(...) unchanged except for passing the newTask.src/tasks.test.ts (1)
137-158: 💤 Low valueTest setup is convoluted due to
writeTaskalways overwritingupdatedAt.The test writes a task, then immediately overwrites the exchange store to set a past
updatedAt. This works but is fragile. Consider adding a test helper or exposing a way to write tasks with explicit timestamps for testing scenarios.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tasks.test.ts` around lines 137 - 158, The test is brittle because writeTask always sets/overwrites updatedAt so the test hacks the exchange store to backdate a task; change writeTask (or add a new exported helper e.g., writeTaskWithTimestamp) to accept an optional updatedAt timestamp (or full Task object) so tests can persist tasks with explicit timestamps, update the test to call the new API instead of mutating createRuntimeExchangeStore, and ensure cleanupTasks, readTask and readAllTasks behavior remains unchanged.src/notifications.ts (1)
227-254: ⚡ Quick winRepeated
store.read()calls inside mapping loop.Lines 238-245 call
store.read()for each record in the loop. This reads and parses the exchange file repeatedly, which is inefficient. Read the exchange once before the loop.♻️ Proposed fix
export function markNotificationsRead(opts?: { id?: string; sessionId?: string }): number { const store = createRuntimeExchangeStore(); - const records = notificationRecords(store.read()).filter((record) => { + const exchange = store.read(); + const records = notificationRecords(exchange).filter((record) => { if (record.cleared || !record.unread) return false; if (opts?.id && record.id !== opts.id) return false; if (opts?.sessionId && record.sessionId !== opts.sessionId) return false; return true; }); if (records.length === 0) return 0; const threadIds = new Set( records .map((record) => { - const exchange = store.read(); return exchange.threads.find((thread) => { const message = threadLatestMessage(exchange, thread.id); return message && notificationRecord(exchange, thread, message).id === record.id; })?.id; }) .filter((id): id is string => Boolean(id)), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/notifications.ts` around lines 227 - 254, In markNotificationsRead, avoid calling store.read() inside the records.map loop; instead call const exchange = store.read() once before building threadIds and reuse that exchange when calling threadLatestMessage(exchange, thread.id) and notificationRecord(exchange, thread, message) so the mapping that produces threadIds uses the single cached exchange value; update the code where threadIds is computed to reference this cached exchange and keep the rest of the function behavior unchanged.src/runtime-core/exchange-derived.ts (1)
12-14: 💤 Low value
unique()helper duplicated across files.This same
unique()function is also defined insrc/threads.ts(lines 28-30). Consider extracting to a shared utility module to avoid duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime-core/exchange-derived.ts` around lines 12 - 14, The helper function unique is duplicated (the function named unique exists in this module and the threads module); extract it into a single exported utility (e.g., create a shared util function unique(values: Array<string | undefined>): string[]) in a common utilities module, export it, update this file to import and use unique instead of the local definition, and remove the duplicate definition from the other module so both consumers import the shared unique implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/builtin-metadata-watchers.ts`:
- Line 100: The DirectoryWatcher is created over
dirname(getRuntimeExchangePath()) so its callback fires for unrelated .aimux
changes and metadata.log(...) (in the taskWatcher callback) appends duplicate
task lines; change the watcher to either watch the specific runtime-exchange
file (use getRuntimeExchangePath() instead of dirname(...)) or, inside the
taskWatcher callback, inspect the reported changed path and only call
metadata.log when it equals the runtime exchange file (or when the task content
actually changed), and additionally guard metadata.log to avoid appending an
identical last entry (compare new task line to the last logged line) so
duplicates are not recorded.
In `@src/multiplexer/notifications.ts`:
- Around line 88-90: The current notificationTargetSessionId function returns
undefined when hasNotificationTarget(host, participantId) is false, which drops
the sessionId and prevents the missing-target flow; change
notificationTargetSessionId to always return the participantId (preserve the
sessionId) even if hasNotificationTarget(...) is false so the missing-target
handling can run, and apply the same change to the other analogous helper blocks
mentioned (the similar checks at lines 113-116 and 131-134) so they also
preserve sessionId rather than returning undefined.
---
Outside diff comments:
In `@src/paths.ts`:
- Line 6: Update the top-of-file comment in src/paths.ts that lists agent-facing
shared artifacts to remove or mark "tasks" and "threads" as legacy/import-only
(they are no longer created or migrated) so the comment matches current
behavior; locate the comment block that enumerates "(config, team, plans,
context, history, tasks, status, threads)" and either remove "tasks" and
"threads" from the active list or append a short note like "tasks, threads —
legacy/import-only" to make intent explicit.
---
Nitpick comments:
In `@src/notifications.ts`:
- Around line 227-254: In markNotificationsRead, avoid calling store.read()
inside the records.map loop; instead call const exchange = store.read() once
before building threadIds and reuse that exchange when calling
threadLatestMessage(exchange, thread.id) and notificationRecord(exchange,
thread, message) so the mapping that produces threadIds uses the single cached
exchange value; update the code where threadIds is computed to reference this
cached exchange and keep the rest of the function behavior unchanged.
In `@src/runtime-core/exchange-derived.ts`:
- Around line 12-14: The helper function unique is duplicated (the function
named unique exists in this module and the threads module); extract it into a
single exported utility (e.g., create a shared util function unique(values:
Array<string | undefined>): string[]) in a common utilities module, export it,
update this file to import and use unique instead of the local definition, and
remove the duplicate definition from the other module so both consumers import
the shared unique implementation.
In `@src/tasks.test.ts`:
- Around line 137-158: The test is brittle because writeTask always
sets/overwrites updatedAt so the test hacks the exchange store to backdate a
task; change writeTask (or add a new exported helper e.g.,
writeTaskWithTimestamp) to accept an optional updatedAt timestamp (or full Task
object) so tests can persist tasks with explicit timestamps, update the test to
call the new API instead of mutating createRuntimeExchangeStore, and ensure
cleanupTasks, readTask and readAllTasks behavior remains unchanged.
In `@src/tasks.ts`:
- Around line 66-77: The writeTask function currently mutates the input Task
(task.updatedAt and task.reviewStatus); instead, create a new Task object (e.g.,
newTask) based on the incoming task, set updatedAt and normalized reviewStatus
on that copy, and use newTask when calling toExchangeTask and when persisting;
leave the original task untouched. Update references to task in writeTask to use
the new copy and keep calls to nowIso(), normalizeReviewStatus(),
toExchangeTask(), and createRuntimeExchangeStore().update(...) unchanged except
for passing the newTask.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 89d6c2c6-950e-4f73-ba6b-40536f1159db
📒 Files selected for processing (26)
AGENTS.mdapp/lib/api.tsdocs/runtime-authority-dead-paths.mddocs/runtime-core-hard-cut-roadmap.mdsrc/builtin-metadata-watchers.test.tssrc/builtin-metadata-watchers.tssrc/debug-state.test.tssrc/debug-state.tssrc/main.tssrc/metadata-server.test.tssrc/metadata-server.tssrc/multiplexer/dashboard-interaction.test.tssrc/multiplexer/dashboard-interaction.tssrc/multiplexer/notifications.test.tssrc/multiplexer/notifications.tssrc/notifications.tssrc/orchestration-actions.test.tssrc/paths.tssrc/runtime-core/exchange-derived.tssrc/session-bootstrap.test.tssrc/session-bootstrap.tssrc/tasks.test.tssrc/tasks.tssrc/threads.test.tssrc/threads.tssrc/tmux/inbox-popup.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/builtin-metadata-watchers.ts (1)
100-100: 💤 Low valueConsider watching the runtime exchange file directly instead of its parent directory.
The current implementation watches
dirname(getRuntimeExchangePath()), which triggers the callback for any file change in that directory. While the fix at line 119 prevents duplicate logs, each trigger still callsreadAllTasks(), rebuilds thelatestBySessionmap, and compares all sessions—even when unrelated files in the directory change.Watching
getRuntimeExchangePath()directly would avoid unnecessary processing.⚡ Proposed optimization
- const taskWatcher = new DirectoryWatcher(dirname(getRuntimeExchangePath()), () => { + const taskWatcher = new DirectoryWatcher(getRuntimeExchangePath(), () => {Note: Node.js
fs.watch()supports watching individual files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/builtin-metadata-watchers.ts` at line 100, The DirectoryWatcher is currently constructed with dirname(getRuntimeExchangePath()), causing callbacks for any file in that directory; change the watcher to target the runtime exchange file itself by passing getRuntimeExchangePath() (e.g., update the taskWatcher creation to use getRuntimeExchangePath()), so the callback only fires for changes to that file; keep the existing callback body (readAllTasks(), latestBySession comparison) but verify DirectoryWatcher supports file paths and handle potential file-deletion/rename edge cases inside the same callback if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/builtin-metadata-watchers.ts`:
- Line 100: The DirectoryWatcher is currently constructed with
dirname(getRuntimeExchangePath()), causing callbacks for any file in that
directory; change the watcher to target the runtime exchange file itself by
passing getRuntimeExchangePath() (e.g., update the taskWatcher creation to use
getRuntimeExchangePath()), so the callback only fires for changes to that file;
keep the existing callback body (readAllTasks(), latestBySession comparison) but
verify DirectoryWatcher supports file paths and handle potential
file-deletion/rename edge cases inside the same callback if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f292360e-00bc-444f-b183-06b005ca4e54
📒 Files selected for processing (1)
src/builtin-metadata-watchers.ts
This was referenced May 26, 2026
This was referenced Jun 14, 2026
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
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Documentation