Add runtime topology and exchange schema foundation#30
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a YAML-backed runtime-exchange model and filesystem store with locking, legacy import utilities and tests, modernizes the runtime topology schema (removing queue) to structured entities and exchangeRefs, updates session pruning to use exchangeRefs, and updates docs to reflect the authority split. ChangesRuntime Exchange Implementation
Sequence Diagram(s)sequenceDiagram
participant LegacyFS as Legacy FS Artifacts
participant Importer as exchange-import
participant Mapper as Exchange Mappers
participant Store as RuntimeExchangeStore
participant YAML as runtime-exchange.yaml
LegacyFS->>Importer: listFiles(threads, messages, tasks, attachments)
Importer->>Mapper: parse & map legacy entities
Mapper->>Importer: assembled RuntimeExchange
Importer->>Store: write(exchange)
Store->>Store: coerceRuntimeExchange(), normalizeRuntimeExchange()
Store->>YAML: atomic write (temp + rename)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime-core/topology-store.ts (1)
255-303:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce rig ownership and node/session consistency during normalization.
These filters only prove that the referenced IDs exist somewhere in the topology. A
service,worktreeGraveyardentry,teamRole,remoteClient,lifecycleOperation, orexchangeRefcan still survive while pointing at a node/session/service/worktree owned by another rig, and anexchangeRefcan also keep a mismatchednodeId+sessionIdpair. That leaves impossible cross-rig state in the persisted topology.🔧 Sketch of the normalization fix
const nodes = topology.nodes.filter((node) => rigIds.has(node.rigId)); + const nodeById = new Map(nodes.map((node) => [node.id, node] as const)); const nodeIds = new Set(nodes.map((node) => node.id)); const sessions = topology.sessions.filter((session) => nodeIds.has(session.nodeId)); + const sessionById = new Map(sessions.map((session) => [session.id, session] as const)); const services = topology.services.filter( - (service) => rigIds.has(service.rigId) && (!service.nodeId || nodeIds.has(service.nodeId)), + (service) => + rigIds.has(service.rigId) && + (!service.nodeId || nodeById.get(service.nodeId)?.rigId === service.rigId), ); + const serviceById = new Map(services.map((service) => [service.id, service] as const)); const worktrees = topology.worktrees.filter((worktree) => rigIds.has(worktree.rigId)); + const worktreeById = new Map(worktrees.map((worktree) => [worktree.id, worktree] as const)); const hasLifecycleTarget = (operation: RuntimeTopologyLifecycleOperation): boolean => { - if (operation.targetKind === "rig") return rigIds.has(operation.targetId); - if (operation.targetKind === "node") return nodeIds.has(operation.targetId); - if (operation.targetKind === "session") return sessionIds.has(operation.targetId); - if (operation.targetKind === "service") return serviceIds.has(operation.targetId); - return worktreeIds.has(operation.targetId); + if (operation.targetKind === "rig") return rigIds.has(operation.targetId); + if (operation.targetKind === "node") { + return nodeById.get(operation.targetId)?.rigId === operation.rigId; + } + if (operation.targetKind === "session") { + const session = sessionById.get(operation.targetId); + return session ? nodeById.get(session.nodeId)?.rigId === operation.rigId : false; + } + if (operation.targetKind === "service") { + return serviceById.get(operation.targetId)?.rigId === operation.rigId; + } + return worktreeById.get(operation.targetId)?.rigId === operation.rigId; }; worktreeGraveyard: topology.worktreeGraveyard.filter( - (entry) => rigIds.has(entry.rigId) && (!entry.worktreeId || worktreeIds.has(entry.worktreeId)), + (entry) => + rigIds.has(entry.rigId) && + (!entry.worktreeId || worktreeById.get(entry.worktreeId)?.rigId === entry.rigId), ), teamRoles: topology.teamRoles.filter( (role) => rigIds.has(role.rigId) && - (!role.nodeId || nodeIds.has(role.nodeId)) && - (!role.parentNodeId || nodeIds.has(role.parentNodeId)), + (!role.nodeId || nodeById.get(role.nodeId)?.rigId === role.rigId) && + (!role.parentNodeId || nodeById.get(role.parentNodeId)?.rigId === role.rigId), ), remoteClients: topology.remoteClients .filter((client) => rigIds.has(client.rigId)) .map((client) => ({ ...client, - ownsSessionIds: client.ownsSessionIds?.filter((sessionId) => sessionIds.has(sessionId)), + ownsSessionIds: client.ownsSessionIds?.filter((sessionId) => { + const session = sessionById.get(sessionId); + return session ? nodeById.get(session.nodeId)?.rigId === client.rigId : false; + }), })), exchangeRefs: topology.exchangeRefs.filter( (ref) => rigIds.has(ref.rigId) && - (!ref.nodeId || nodeIds.has(ref.nodeId)) && - (!ref.sessionId || sessionIds.has(ref.sessionId)), + (!ref.nodeId || nodeById.get(ref.nodeId)?.rigId === ref.rigId) && + (!ref.sessionId || nodeById.get(sessionById.get(ref.sessionId)?.nodeId ?? "")?.rigId === ref.rigId) && + (!ref.nodeId || !ref.sessionId || sessionById.get(ref.sessionId)?.nodeId === ref.nodeId), ),🤖 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/topology-store.ts` around lines 255 - 303, The filters currently only check that referenced IDs exist, allowing cross-rig references; tighten normalization by enforcing that every referenced entity belongs to the same rig and (where applicable) the same node/session: update the services filter (and serviceIds) to drop services whose nodeId or sessionId exists but belongs to a different rig; update worktreeGraveyard and worktrees/worktreeIds to ensure worktrees' rigId matches rigIds; tighten teamRoles to ensure role.nodeId and role.parentNodeId (if present) are owned by rigIds; update remoteClients mapping to remove ownsSessionIds that aren't in sessionIds and ensure those sessions belong to rigIds; make hasLifecycleTarget/lifecycleOperations enforce that targetId’s entity (rig/node/session/service/worktree) is owned by the same rig; and change exchangeRefs filter to require that if both nodeId and sessionId are present they refer to a session owned by that node and the same rig (and otherwise each referenced id must be owned by rigIds). Ensure you use the existing variables (services, serviceIds, worktrees, worktreeIds, hasLifecycleTarget, lifecycleOperations, exchangeRefs, remoteClients, worktreeGraveyard, teamRoles) when implementing these checks.
🤖 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/runtime-core/exchange-import.ts`:
- Around line 278-283: continuityKindForPath fails on Windows because it checks
for hardcoded "/" segments while produced paths may contain "\\"; update
continuityKindForPath to normalize the incoming path (e.g. replace backslashes
with forward slashes or use path.normalize then convert separators) before
testing, then run the same contains checks for "/recordings/", "/status/", and
"/history/"; modify the function continuityKindForPath (and any callers if
needed) so it operates on the normalized string and still returns the same
RuntimeExchangeContinuityRef["kind"] values.
In `@src/runtime-core/exchange-store.ts`:
- Around line 563-583: The acquireUpdateLock loop can deadlock on orphaned
`${this.path}.lock`; before retrying in the catch, detect and remove stale locks
by (1) checking the lock's owner file (join(lockPath, "owner")) and if present
parsing the PID and verifying liveness via process.kill(pid, 0) (if kill throws
ESRCH treat owner as dead) and (2) checking lock directory mtime/age against a
max-stale threshold (e.g. compare stat.mtimeMs to Date.now() and remove if older
than a configured MAX_LOCK_AGE_MS). If either condition indicates a
stale/orphaned lock, rmSync(lockPath, { recursive: true, force: true }) and
continue the loop; otherwise keep sleeping and retrying as before. Ensure errors
reading owner or stat are handled and do not mask the original error.
In `@src/runtime-core/topology-store.ts`:
- Around line 560-583: The functions asLifecycleOperationTargetKind and
asExchangeRefKind currently remap unknown values to a default valid kind ("rig"
and "message"), which silently mutates malformed input; change both to preserve
the original string when it doesn't match an accepted kind: convert the incoming
value to a string (as done now), return the matching kind when it equals one of
the allowed literals, otherwise return that original string instead of a default
so invalid/malformed kinds are preserved for validation or round-tripping rather
than being silently rewritten.
---
Outside diff comments:
In `@src/runtime-core/topology-store.ts`:
- Around line 255-303: The filters currently only check that referenced IDs
exist, allowing cross-rig references; tighten normalization by enforcing that
every referenced entity belongs to the same rig and (where applicable) the same
node/session: update the services filter (and serviceIds) to drop services whose
nodeId or sessionId exists but belongs to a different rig; update
worktreeGraveyard and worktrees/worktreeIds to ensure worktrees' rigId matches
rigIds; tighten teamRoles to ensure role.nodeId and role.parentNodeId (if
present) are owned by rigIds; update remoteClients mapping to remove
ownsSessionIds that aren't in sessionIds and ensure those sessions belong to
rigIds; make hasLifecycleTarget/lifecycleOperations enforce that targetId’s
entity (rig/node/session/service/worktree) is owned by the same rig; and change
exchangeRefs filter to require that if both nodeId and sessionId are present
they refer to a session owned by that node and the same rig (and otherwise each
referenced id must be owned by rigIds). Ensure you use the existing variables
(services, serviceIds, worktrees, worktreeIds, hasLifecycleTarget,
lifecycleOperations, exchangeRefs, remoteClients, worktreeGraveyard, teamRoles)
when implementing these checks.
🪄 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: 624111be-f766-4fd0-a3de-6372c6389165
📒 Files selected for processing (11)
docs/runtime-authority-dead-paths.mddocs/runtime-authority-inventory.mdsrc/paths.tssrc/runtime-core/exchange-import.test.tssrc/runtime-core/exchange-import.tssrc/runtime-core/exchange-store.test.tssrc/runtime-core/exchange-store.tssrc/runtime-core/topology-sessions.test.tssrc/runtime-core/topology-sessions.tssrc/runtime-core/topology-store.test.tssrc/runtime-core/topology-store.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/runtime-core/exchange-store.ts`:
- Line 9: The stale-lock recovery threshold UPDATE_LOCK_STALE_MS is longer than
the lock acquisition timeout UPDATE_LOCK_TIMEOUT_MS so mtime-based recovery can
never trigger before update() gives up; change the logic so UPDATE_LOCK_STALE_MS
< UPDATE_LOCK_TIMEOUT_MS (or compute stale threshold relative to
UPDATE_LOCK_TIMEOUT_MS) and adjust the constants or the recovery check in
exchange-store's update() / lock-acquisition code to detect and reclaim locks
whose mtime is older than the new shorter stale threshold (reference
UPDATE_LOCK_STALE_MS, UPDATE_LOCK_TIMEOUT_MS and the update() path that creates
the lock dir and checks owner/mtime).
🪄 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: c05b443c-1c54-4bf1-a5bc-b6de75abc263
📒 Files selected for processing (6)
src/runtime-core/exchange-import.test.tssrc/runtime-core/exchange-import.tssrc/runtime-core/exchange-store.test.tssrc/runtime-core/exchange-store.tssrc/runtime-core/topology-store.test.tssrc/runtime-core/topology-store.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/runtime-core/exchange-store.ts`:
- Line 9: The stale-lock recovery is deleting update locks solely based on
UPDATE_LOCK_STALE_MS (1_000) which can cause live-locks when the owner PID is
still alive; modify recoverStaleUpdateLock (and the related logic used by
update()) to (1) raise the stale threshold to a safer value and (2) before
deleting a lock record whose timestamp is older than the threshold, verify the
owner process is not alive (e.g., check the owner PID with process existence
check) and only then remove the row; ensure the deletion still uses the lock’s
owner PID and timestamp as part of the conditional delete to avoid races.
🪄 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: 5dea81dc-f04f-42b7-9a5c-8186d153d6ca
📒 Files selected for processing (2)
src/runtime-core/exchange-store.test.tssrc/runtime-core/exchange-store.ts
This was referenced May 26, 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
Documentation
Tests