Skip to content

refactor: shared GraphPort architecture + GraphContext + graph.patch() (1.0.0-alpha.9)#18

Merged
flyingrobots merged 4 commits intomainfrom
refactor/shared-graph-port
Feb 27, 2026
Merged

refactor: shared GraphPort architecture + GraphContext + graph.patch() (1.0.0-alpha.9)#18
flyingrobots merged 4 commits intomainfrom
refactor/shared-graph-port

Conversation

@flyingrobots
Copy link
Copy Markdown
Owner

@flyingrobots flyingrobots commented Feb 26, 2026

Summary

  • Shared graph singleton: Introduced GraphPort / WarpGraphAdapter — one WarpGraph instance per process, injected via DI. Eliminates WRITER_CAS_CONFLICT errors from multiple WarpGraphHolder instances sharing the same writerId.
  • GraphContext replaces DashboardService + WarpDashboardAdapter: Single gateway using graph.query() for typed node fetching and graph.traverse for graph algorithms. Extracted DepAnalysis.ts for frontier/critical-path computation.
  • Atomic graph.patch() for all writes: All adapters, actuator, and coordinator daemon converted from manual syncCoverage → materialize → createPatchSession → commit to graph.patch(p => { … }). Redundant syncCoverage()/materialize() calls eliminated.
  • Cache invalidation fix: GraphContext now uses frontier key comparison (serialized writer:tick pairs) instead of hasFrontierChanged(), which missed in-process writes.
  • Deleted dead code: WarpGraphHolder, WarpDashboardAdapter, DashboardService, DashboardPort, WeaverService, WeaverPort, WarpWeaverAdapter and their tests.
  • Net -239 lines across 30 files. Submission lifecycle test dropped from 15s → ~1.2s.

Test plan

  • npm run build — clean
  • npm run test:local — 327/327 tests passing (32 files)
  • CI quality gates (build, lint, test)
  • Verify npx tsx xyph-actuator.ts status --view roadmap renders correctly
  • Verify npx tsx xyph-actuator.ts status --view deps renders correctly

Summary by CodeRabbit

  • New Features

    • Unified graph gateway and port abstraction for a single shared graph instance; writes are now atomic and visible immediately across the app.
    • New domain utilities for dependency analysis (frontier & critical path).
  • Bug Fixes

    • Improved cache invalidation to detect in-process and external mutations; reduced unnecessary materialization.
  • Architecture

    • Centralized graph access, removed legacy adapter indirections, and optimized write/read flows.
  • Removed

    • Deprecated several legacy graph adapters and related services; migration notes provided.
  • Documentation

    • Expanded architecture docs describing the new ports-and-adapters design.

…text

Delete WarpDashboardAdapter, DashboardService, WeaverService, WeaverPort,
and WarpWeaverAdapter — replaced by GraphContext (query-based snapshot)
and DepAnalysis (pure frontier/critical-path functions).

Convert all WARP adapters and xyph-actuator from manual
syncCoverage+materialize+createPatchSession+commit to atomic graph.patch().
Drop redundant materialize() calls (autoMaterialize handles reads).

Submission lifecycle test drops from 15s timeout to default 5s (~1.2s actual).
327 tests, all passing.
…1.0.0-alpha.9)

Replace per-adapter WarpGraphHolder with a shared GraphPort/WarpGraphAdapter
singleton injected via DI. All adapters, actuator, dashboard, and coordinator
daemon share one graph instance, eliminating CAS conflicts and redundant
syncCoverage()/materialize() calls.

Fix GraphContext cache invalidation: frontier key comparison replaces
hasFrontierChanged() to detect both in-process and external mutations.

Delete dead WarpGraphHolder. Net -259 lines.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Feb 26, 2026

📝 Walkthrough

Walkthrough

Introduces a GraphPort + WarpGraphAdapter DI pattern and a GraphContext gateway, refactors adapters to use graph.patch(...) for atomic writes, removes DashboardService/WarpDashboardAdapter and WeaverService/WarpWeaverAdapter (replacing Weaver logic with DepAnalysis utilities), updates TUI to use GraphContext, and bumps version to 1.0.0-alpha.9.

Changes

Cohort / File(s) Summary
Changelog & Package
CHANGELOG.md, package.json
Release bumped to 1.0.0-alpha.9 and changelog updated with architecture, performance, Removed/Fixed notes.
GraphPort & Context
src/ports/GraphPort.ts, src/infrastructure/GraphContext.ts, src/tui/GraphProvider.tsx
Adds GraphPort interface, creates GraphContext factory with snapshot caching/filtering/invalidate, and a React GraphProvider + hook for TUI consumption.
Graph Adapter
src/infrastructure/adapters/WarpGraphAdapter.ts
Renames/rewrites WarpGraphHolder → WarpGraphAdapter implementing GraphPort; exposes getGraph() and reset() with DI-friendly lifecycle.
Adapter DI Refactor
src/infrastructure/adapters/WarpRoadmapAdapter.ts, src/infrastructure/adapters/WarpIntakeAdapter.ts, src/infrastructure/adapters/WarpSubmissionAdapter.ts, src/coordinator-daemon.ts, xyph-actuator.ts
Adapters now accept a GraphPort (WarpGraphAdapter) instead of repo path/graph identifiers; write flows use graph.patch(...) for atomic mutations and remove createPatchSession/explicit materialize calls.
Removed Adapters & Services
src/infrastructure/adapters/WarpDashboardAdapter.ts, src/domain/services/DashboardService.ts, src/infrastructure/adapters/WarpWeaverAdapter.ts, src/ports/DashboardPort.ts, src/ports/WeaverPort.ts, src/domain/models/dashboard.ts
Deletes WarpDashboardAdapter, DashboardService, WarpWeaverAdapter, DashboardPort, WeaverPort, and LineageTree model interface.
Domain: DepAnalysis
src/domain/services/DepAnalysis.ts
Adds pure-domain utilities: TaskSummary/DepEdge types, computeFrontier(), computeCriticalPath() (DP longest-path) replacing WeaverService logic.
TUI changes
src/tui/Dashboard.tsx, xyph-dashboard.tsx
Dashboard component prop switched from DashboardService → GraphContext; wiring updated to createGraphContext(graphPort) and inject ctx into UI.
Tests: removals & refactors
test/integration/WarpDashboardAdapter.test.ts, test/unit/DashboardService.test.ts, test/integration/WarpWeaverAdapter.test.ts, test/unit/WeaverService.test.ts, test/unit/DepAnalysis.test.ts, test/integration/*
Removes tests tied to removed services/adapters; adds unit tests for DepAnalysis; many integration tests refactored to use WarpGraphAdapter + createGraphContext and graph.patch-based seeding.
Docs
docs/canonical/ARCHITECTURE.md
Documentation updated to describe Hexagonal architecture, Shared Graph Architecture, DI, and GraphContext behavior.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant GraphContext as GraphContext\n(createGraphContext)
  participant Adapter as WarpIntakeAdapter\n(or WarpSubmissionAdapter)
  participant GraphPort as WarpGraphAdapter\n(GraphPort)
  participant WarpGraph as WarpGraph\n(internal)

  Client->>Adapter: perform write (promote/submit/patch)
  Adapter->>GraphPort: getGraph()
  GraphPort->>WarpGraph: return shared graph
  Adapter->>WarpGraph: patch(fn)  -- apply atomic mutation
  WarpGraph-->>GraphPort: commit & autoMaterialize
  GraphPort-->>Adapter: patch result (sha)

  Client->>GraphContext: fetchSnapshot()
  GraphContext->>GraphPort: getGraph()
  GraphPort->>WarpGraph: return up-to-date graph
  GraphContext->>WarpGraph: materialize/query nodes & neighbors
  WarpGraph-->>GraphContext: node data
  GraphContext-->>Client: GraphSnapshot (cached)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~65 minutes

Possibly related PRs

Poem

🐰
I hopped to a port where the graph now sleeps,
A single shared tunnel where each patch leaps,
GraphContext hums, snapshots tidy and bright,
Patches apply atomically—what a delight!
The weaver's yarn rewoven into pure insight.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main architectural changes: introducing a shared GraphPort architecture, GraphContext, and graph.patch() pattern as part of version 1.0.0-alpha.9.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor/shared-graph-port

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ebe4b344ed

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread xyph-actuator.ts
Comment thread xyph-actuator.ts
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
CHANGELOG.md (1)

552-559: ⚠️ Potential issue | 🟡 Minor

Add comparison links for alpha.8 and alpha.9.

The link references at the bottom are missing entries for 1.0.0-alpha.8 and 1.0.0-alpha.9. The [Unreleased] link also points to alpha.7 instead of the latest release.

📝 Proposed fix
-[Unreleased]: https://github.com/flyingrobots/xyph/compare/v1.0.0-alpha.7...HEAD
+[Unreleased]: https://github.com/flyingrobots/xyph/compare/v1.0.0-alpha.9...HEAD
+[1.0.0-alpha.9]: https://github.com/flyingrobots/xyph/compare/v1.0.0-alpha.8...v1.0.0-alpha.9
+[1.0.0-alpha.8]: https://github.com/flyingrobots/xyph/compare/v1.0.0-alpha.7...v1.0.0-alpha.8
 [1.0.0-alpha.7]: https://github.com/flyingrobots/xyph/compare/v1.0.0-alpha.6...v1.0.0-alpha.7
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CHANGELOG.md` around lines 552 - 559, Update the bottom link references to
include the missing versions and point Unreleased to the latest release: change
the [Unreleased] link to compare v1.0.0-alpha.9...HEAD and add entries for
[1.0.0-alpha.9] (compare v1.0.0-alpha.8...v1.0.0-alpha.9) and [1.0.0-alpha.8]
(compare v1.0.0-alpha.7...v1.0.0-alpha.8) so the symbols [Unreleased],
[1.0.0-alpha.9], and [1.0.0-alpha.8] all exist and link to the correct GitHub
compare URLs.
🧹 Nitpick comments (3)
src/infrastructure/GraphContext.ts (1)

105-116: Don’t silently drop neighbor-query failures.

Ignoring rejected neighbor fetches can produce partial snapshots without surfacing data integrity problems. Prefer fail-fast or at least structured warning + degraded-mode signaling.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/infrastructure/GraphContext.ts` around lines 105 - 116, The current
Promise.allSettled loop silently drops rejected neighbor fetches (ids.map ->
graph.neighbors, toNeighborEntries) which can hide data integrity issues; update
the handling so that after Promise.allSettled(results) you iterate results and
for any result.status === 'rejected' capture the associated id (from the
original ids array at the same index), log/emit a structured warning or error
including that id and result.reason, and then either fail-fast by throwing a
descriptive error or set a clearly named degraded flag (e.g., neighborsDegraded
or neighborFetchFailed) so callers can detect partial snapshots; still add
fulfilled entries to map via map.set(id, neighbors) for successful ones.
test/integration/WarpIntakeAdapter.test.ts (1)

26-80: Consider consolidating seed patches for efficiency.

The seven sequential graph.patch() calls could be merged into a single atomic patch. This would reduce overhead and better reflect real-world batch seeding. However, the current approach is functionally correct and may improve test readability.

♻️ Optional: Consolidated seed patch
-    await graph.patch((p) => {
-      p.addNode('intent:sovereign-test')
-        .setProperty('intent:sovereign-test', 'title', 'Sovereign Test Intent')
-        .setProperty('intent:sovereign-test', 'requested_by', 'human.tester')
-        .setProperty('intent:sovereign-test', 'created_at', 1700000000000)
-        .setProperty('intent:sovereign-test', 'type', 'intent');
-    });
-
-    await graph.patch((p) => {
-      p.addNode('task:INTAKE-001')
-        ...
-    });
-    // ... remaining patches
+    await graph.patch((p) => {
+      // Intent
+      p.addNode('intent:sovereign-test')
+        .setProperty('intent:sovereign-test', 'title', 'Sovereign Test Intent')
+        .setProperty('intent:sovereign-test', 'requested_by', 'human.tester')
+        .setProperty('intent:sovereign-test', 'created_at', 1700000000000)
+        .setProperty('intent:sovereign-test', 'type', 'intent');
+
+      // Test tasks
+      p.addNode('task:INTAKE-001')
+        .setProperty('task:INTAKE-001', 'title', 'Intake promote target task')
+        .setProperty('task:INTAKE-001', 'status', 'INBOX')
+        .setProperty('task:INTAKE-001', 'hours', 2)
+        .setProperty('task:INTAKE-001', 'type', 'task');
+
+      // ... add remaining nodes in same patch
+    });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/integration/WarpIntakeAdapter.test.ts` around lines 26 - 80, Multiple
sequential graph.patch(...) calls should be consolidated into a single atomic
patch to reduce overhead and mimic batch seeding; replace the seven separate
graph.patch invocations with one graph.patch(p => { ... }) that calls
p.addNode('intent:sovereign-test') and all p.addNode('task:...') entries and
their corresponding p.setProperty(...) calls inside the same callback
(preserving node ids like 'intent:sovereign-test', 'task:INTAKE-001',
'task:INTAKE-002', 'task:INTAKE-003', 'task:INTAKE-004',
'task:INTAKE-FORBIDDEN', 'task:INTAKE-ALREADY-PROMOTED') so the changes are
applied atomically.
xyph-actuator.ts (1)

140-151: Potential inconsistency between graphPort writerId and command agentId.

The graphPort is initialized at module load time with agentId (line 15), but the claim command re-reads XYPH_AGENT_ID at line 142. If the environment variable changes between process start and command execution (unlikely in normal use but possible in tests), the assigned_to property will differ from the graph's writerId.

This doesn't break functionality since the adapter accepts any agentId for data, but could cause confusion if debugging writer attribution.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@xyph-actuator.ts` around lines 140 - 151, The claim command reads
process.env['XYPH_AGENT_ID'] again, which can diverge from the
module-initialized graphPort writerId; to fix, stop re-reading the env var
inside the .action handler and instead use the same agent identifier established
at module load (e.g., the module-level DEFAULT_AGENT_ID/AGENT_ID or
graphPort.writerId) so that the assigned_to property matches the graphPort
writer attribution; update the .action(async (id: string) => { ... }) block to
reference that shared agentId symbol rather than process.env.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/infrastructure/GraphContext.ts`:
- Around line 138-143: invalidateCache() is clearing local
cachedSnapshot/cachedFrontierKey/_graph but calling this.graphPort.reset()
breaks the “single WarpGraph instance per process” invariant by allowing a new
WarpGraph to be created while old references may still exist; remove the
this.graphPort.reset() call from invalidateCache() and only clear
this.cachedSnapshot, this.cachedFrontierKey, and this._graph in
invalidateCache(), and move any graphPort.reset() use to a controlled
shutdown/close path (e.g. a dedicated close/destroy method) that ensures no
active WarpGraph references remain before calling graphPort.reset().

---

Outside diff comments:
In `@CHANGELOG.md`:
- Around line 552-559: Update the bottom link references to include the missing
versions and point Unreleased to the latest release: change the [Unreleased]
link to compare v1.0.0-alpha.9...HEAD and add entries for [1.0.0-alpha.9]
(compare v1.0.0-alpha.8...v1.0.0-alpha.9) and [1.0.0-alpha.8] (compare
v1.0.0-alpha.7...v1.0.0-alpha.8) so the symbols [Unreleased], [1.0.0-alpha.9],
and [1.0.0-alpha.8] all exist and link to the correct GitHub compare URLs.

---

Nitpick comments:
In `@src/infrastructure/GraphContext.ts`:
- Around line 105-116: The current Promise.allSettled loop silently drops
rejected neighbor fetches (ids.map -> graph.neighbors, toNeighborEntries) which
can hide data integrity issues; update the handling so that after
Promise.allSettled(results) you iterate results and for any result.status ===
'rejected' capture the associated id (from the original ids array at the same
index), log/emit a structured warning or error including that id and
result.reason, and then either fail-fast by throwing a descriptive error or set
a clearly named degraded flag (e.g., neighborsDegraded or neighborFetchFailed)
so callers can detect partial snapshots; still add fulfilled entries to map via
map.set(id, neighbors) for successful ones.

In `@test/integration/WarpIntakeAdapter.test.ts`:
- Around line 26-80: Multiple sequential graph.patch(...) calls should be
consolidated into a single atomic patch to reduce overhead and mimic batch
seeding; replace the seven separate graph.patch invocations with one
graph.patch(p => { ... }) that calls p.addNode('intent:sovereign-test') and all
p.addNode('task:...') entries and their corresponding p.setProperty(...) calls
inside the same callback (preserving node ids like 'intent:sovereign-test',
'task:INTAKE-001', 'task:INTAKE-002', 'task:INTAKE-003', 'task:INTAKE-004',
'task:INTAKE-FORBIDDEN', 'task:INTAKE-ALREADY-PROMOTED') so the changes are
applied atomically.

In `@xyph-actuator.ts`:
- Around line 140-151: The claim command reads process.env['XYPH_AGENT_ID']
again, which can diverge from the module-initialized graphPort writerId; to fix,
stop re-reading the env var inside the .action handler and instead use the same
agent identifier established at module load (e.g., the module-level
DEFAULT_AGENT_ID/AGENT_ID or graphPort.writerId) so that the assigned_to
property matches the graphPort writer attribution; update the .action(async (id:
string) => { ... }) block to reference that shared agentId symbol rather than
process.env.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0bb7e28 and ebe4b34.

📒 Files selected for processing (30)
  • CHANGELOG.md
  • package.json
  • src/coordinator-daemon.ts
  • src/domain/models/dashboard.ts
  • src/domain/services/DashboardService.ts
  • src/domain/services/DepAnalysis.ts
  • src/domain/services/WeaverService.ts
  • src/infrastructure/GraphContext.ts
  • src/infrastructure/adapters/WarpDashboardAdapter.ts
  • src/infrastructure/adapters/WarpGraphAdapter.ts
  • src/infrastructure/adapters/WarpIntakeAdapter.ts
  • src/infrastructure/adapters/WarpRoadmapAdapter.ts
  • src/infrastructure/adapters/WarpSubmissionAdapter.ts
  • src/infrastructure/adapters/WarpWeaverAdapter.ts
  • src/ports/DashboardPort.ts
  • src/ports/GraphPort.ts
  • src/ports/WeaverPort.ts
  • src/tui/Dashboard.tsx
  • src/tui/GraphProvider.tsx
  • test/integration/CrossAdapterVisibility.test.ts
  • test/integration/WarpDashboardAdapter.test.ts
  • test/integration/WarpIntakeAdapter.test.ts
  • test/integration/WarpRoadmapAdapter.test.ts
  • test/integration/WarpSubmissionAdapter.test.ts
  • test/integration/WarpWeaverAdapter.test.ts
  • test/unit/DashboardService.test.ts
  • test/unit/DepAnalysis.test.ts
  • test/unit/WeaverService.test.ts
  • xyph-actuator.ts
  • xyph-dashboard.tsx
💤 Files with no reviewable changes (11)
  • src/domain/models/dashboard.ts
  • src/infrastructure/adapters/WarpDashboardAdapter.ts
  • src/infrastructure/adapters/WarpWeaverAdapter.ts
  • src/ports/DashboardPort.ts
  • src/domain/services/WeaverService.ts
  • src/ports/WeaverPort.ts
  • test/integration/WarpWeaverAdapter.test.ts
  • test/unit/DashboardService.test.ts
  • test/integration/WarpDashboardAdapter.test.ts
  • test/unit/WeaverService.test.ts
  • src/domain/services/DashboardService.ts

Comment thread src/infrastructure/GraphContext.ts
Critical: invalidateCache() no longer orphans shared graph via reset().
High: remove stale syncCoverage() from depend, inline dead getGraph().
Medium: reduce getStateSnapshot() calls, switch to Promise.all.
Low: batch test seeds, fix JSDoc reference.
Update ARCHITECTURE.md to reflect current hexagonal architecture.
…its)

- Fix topologicalSort traversal direction (dir: 'in') so deps view
  produces prerequisite-first execution order instead of reversed
- Add node type validation in depend command (prefix + type property)
- Use module-level agentId in claim command instead of re-reading env
- Add missing CHANGELOG comparison links for alpha.8 and alpha.9
@flyingrobots
Copy link
Copy Markdown
Owner Author

Addressing review feedback — round 3 (cf570d1)

Fixes applied

Issue Source Fix
P1: topologicalSort missing dir: 'in' Codex Added dir: 'in' so deps view produces prerequisite-first execution order. Without it, Kahn's algorithm returns dependents before prerequisites (reversed).
P2: Missing node type check in depend Codex Added getNodeProps + type validation after existence check. Prefix convention alone is insufficient as a type guarantee.
Nitpick: claim re-reads env var CodeRabbit Removed redundant process.env['XYPH_AGENT_ID'] read; now uses module-level agentId (line 13).
CHANGELOG links CodeRabbit Added comparison links for alpha.8/alpha.9 and updated [Unreleased] to point to alpha.9.

False positives (2)

1. GraphContext Promise.allSettled silent drop (nitpick on GraphContext.ts:105-116)
The review says "Ignoring rejected neighbor fetches can produce partial snapshots" — but the code uses Promise.all, not Promise.allSettled. Promise.all already fails fast on any rejection, so there is no silent drop. No change needed.

2. Consolidate WarpIntakeAdapter test seed patches (nitpick on WarpIntakeAdapter.test.ts:26-80)
The review asks to merge seven sequential graph.patch() calls into one — but this was already done in the prior commit (99c0116). Lines 26-62 are a single graph.patch() call. No change needed.


Build ✅ | Lint ✅ | 327/327 tests passing ✅

@flyingrobots
Copy link
Copy Markdown
Owner Author

@coderabbitai review please 🥕

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Feb 26, 2026

@flyingrobots: Thank you for the detailed summary of the fixes applied in cf570d1! I'll trigger a fresh review of the PR to verify the changes and provide any additional feedback.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@CHANGELOG.md`:
- Line 561: The CHANGELOG.md contains an unused link reference "[Unreleased]:
https://github.com/flyingrobots/xyph/compare/v1.0.0-alpha.9...HEAD" with no
corresponding section header; fix by either adding a top-level section header
"## [Unreleased]" (and place any pending notes under it) or remove the link
reference line entirely if you don't intend to publish an Unreleased section
yet—update the file so the "[Unreleased]" link reference and the "##
[Unreleased]" header are consistent.

In `@docs/canonical/ARCHITECTURE.md`:
- Around line 5-17: The fenced ASCII diagram blocks (for example the block
containing "Driving Adapters / xyph-actuator / xyph-dashboard / coordinator" and
the corresponding "Domain Core / domain/entities / domain/services" and "Driven
Adapters / WarpGraphAdapter / WarpIntakeAdapter" diagram) lack a language
specifier; update each such fenced code block to start with ```text (or
```plaintext) instead of ``` so the ASCII diagrams are treated as plain text
(apply the same change to the other ASCII diagram blocks in the file).

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ebe4b34 and cf570d1.

📒 Files selected for processing (7)
  • CHANGELOG.md
  • docs/canonical/ARCHITECTURE.md
  • src/infrastructure/GraphContext.ts
  • src/ports/GraphPort.ts
  • test/integration/WarpIntakeAdapter.test.ts
  • test/integration/WarpSubmissionAdapter.test.ts
  • xyph-actuator.ts

Comment thread CHANGELOG.md
- Refined Actuator `syncWith` logic to use `syncCoverage()` for reliable multi-writer convergence.

[Unreleased]: https://github.com/flyingrobots/xyph/compare/v1.0.0-alpha.7...HEAD
[Unreleased]: https://github.com/flyingrobots/xyph/compare/v1.0.0-alpha.9...HEAD
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Unused [Unreleased] link reference.

The [Unreleased] link definition exists but there's no corresponding ## [Unreleased] section header in the document. Either add an ## [Unreleased] section header for future changes, or remove the unused link reference if you plan to add it only when there are unreleased changes.

🔧 Option 1: Add section header
 # CHANGELOG

 All notable changes to XYPH will be documented in this file.

+## [Unreleased]
+
 ## [1.0.0-alpha.9] - 2026-02-25
🔧 Option 2: Remove unused link
-[Unreleased]: https://github.com/flyingrobots/xyph/compare/v1.0.0-alpha.9...HEAD
 [1.0.0-alpha.9]: https://github.com/flyingrobots/xyph/compare/v1.0.0-alpha.8...v1.0.0-alpha.9
📝 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
[Unreleased]: https://github.com/flyingrobots/xyph/compare/v1.0.0-alpha.9...HEAD
[1.0.0-alpha.9]: https://github.com/flyingrobots/xyph/compare/v1.0.0-alpha.8...v1.0.0-alpha.9
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)

[warning] 561-561: Link and image reference definitions should be needed
Unused link or image reference definition: "unreleased"

(MD053, link-image-reference-definitions)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CHANGELOG.md` at line 561, The CHANGELOG.md contains an unused link reference
"[Unreleased]:
https://github.com/flyingrobots/xyph/compare/v1.0.0-alpha.9...HEAD" with no
corresponding section header; fix by either adding a top-level section header
"## [Unreleased]" (and place any pending notes under it) or remove the link
reference line entirely if you don't intend to publish an Unreleased section
yet—update the file so the "[Unreleased]" link reference and the "##
[Unreleased]" header are consistent.

Comment on lines +5 to +17
```
Driving Adapters Domain Core Driven Adapters
┌──────────────┐ ┌───────────────────┐ ┌──────────────────┐
│ xyph-actuator│───────▶│ domain/entities │ │ WarpGraphAdapter │
│ (CLI) │ │ domain/services │◀──────▶│ WarpIntakeAdapter│
├──────────────┤ │ domain/models │ ports │ WarpSubmission..│
│xyph-dashboard│───────▶│ │◀──────▶│ WarpRoadmap... │
│ (TUI/Ink) │ └───────────────────┘ │ GitWorkspace... │
├──────────────┤ └──────────────────┘
│ coordinator │
│ (daemon) │
└──────────────┘
```
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add language specifier to fenced code blocks.

The ASCII diagrams lack language specifiers, triggering MD040 warnings. Use text or plaintext for ASCII art diagrams.

🔧 Proposed fix
-```
+```text
   Driving Adapters              Domain Core               Driven Adapters

Apply the same change to the other diagram blocks at lines 34, 53, 58, and 66.

🧰 Tools
🪛 markdownlint-cli2 (0.21.0)

[warning] 5-5: 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/canonical/ARCHITECTURE.md` around lines 5 - 17, The fenced ASCII diagram
blocks (for example the block containing "Driving Adapters / xyph-actuator /
xyph-dashboard / coordinator" and the corresponding "Domain Core /
domain/entities / domain/services" and "Driven Adapters / WarpGraphAdapter /
WarpIntakeAdapter" diagram) lack a language specifier; update each such fenced
code block to start with ```text (or ```plaintext) instead of ``` so the ASCII
diagrams are treated as plain text (apply the same change to the other ASCII
diagram blocks in the file).

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
xyph-actuator.ts (1)

517-518: Consider clarifying the comment about patch visibility.

The comment references _onPatchCommitted which is an internal detail of WarpGraph. Consider rephrasing to focus on the user-visible behavior:

🔧 Suggested clarification
-          // Same graph instance — decide() patch is already visible via _onPatchCommitted
+          // Same graph instance — decide() patch is immediately visible due to autoMaterialize: true
           const sealGraph = await graphPort.getGraph();
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@xyph-actuator.ts` around lines 517 - 518, Update the inline comment above the
call to graphPort.getGraph() (near sealGraph declaration) to remove the internal
method reference _onPatchCommitted and instead describe the observable behavior:
that the graph instance is the same and any decide() patch is already
applied/visible on that instance (so no additional patch handling is required).
Keep the comment concise and reference sealGraph and graphPort.getGraph() so
future readers understand the reason for retrieving the same graph instance
without exposing internal WarpGraph internals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@xyph-actuator.ts`:
- Around line 517-518: Update the inline comment above the call to
graphPort.getGraph() (near sealGraph declaration) to remove the internal method
reference _onPatchCommitted and instead describe the observable behavior: that
the graph instance is the same and any decide() patch is already applied/visible
on that instance (so no additional patch handling is required). Keep the comment
concise and reference sealGraph and graphPort.getGraph() so future readers
understand the reason for retrieving the same graph instance without exposing
internal WarpGraph internals.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ebe4b34 and cf570d1.

📒 Files selected for processing (7)
  • CHANGELOG.md
  • docs/canonical/ARCHITECTURE.md
  • src/infrastructure/GraphContext.ts
  • src/ports/GraphPort.ts
  • test/integration/WarpIntakeAdapter.test.ts
  • test/integration/WarpSubmissionAdapter.test.ts
  • xyph-actuator.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/ports/GraphPort.ts

@flyingrobots flyingrobots merged commit 2481a92 into main Feb 27, 2026
5 checks passed
@flyingrobots flyingrobots deleted the refactor/shared-graph-port branch February 28, 2026 16:24
flyingrobots added a commit that referenced this pull request Mar 8, 2026
Replace filesystem-specific terms (file, path, rename) with
storage-agnostic phrasing in the port interface documentation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant