alkdev/flowgraph
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: architecture review - address 5 critical issues, 6 warnings, 3 suggestions

Browse Source 
Critical fixes:
- C1: Create standalone ADR-006 file (edge type consistency),
  extract from open-questions.md inline content
- C2: Convert CallResult from plain interface to TypeBox schema,
  aligning with 'TypeBox as single source of truth' constraint
- C3: Add fromJSON() cycle detection specification - enforce
  ADR-002 DAG invariant even on deserialized input
- C4: Rewrite consumer-integration.md Phase 4 to use ADR-005
  event-append pattern instead of direct signal mutation
- C5: Fix operator precedence bug in consumer-integration.md
  (missing parentheses around OR condition)

Warnings addressed:
- W1: Fix immutability claim - operation graph is 'conventionally
  immutable', not prevented by API
- W2: Add EventLogProjection to reactive exports map
- W3: Add CallResult/CallResultSchema to schema exports map
- W4: Fix reactive-execution.md Level 1 error handling to use
  event-append pattern instead of direct signal mutation
- W5: Remove duplicate dataFlow inference description in schema.md
- W6: Clarify ADR-006 project context (flowgraph vs taskgraph)

Suggestions implemented:
- S1: Add 'reviewed' document lifecycle status between draft/stable,
  update all docs to reviewed status
- S2: Add carve-out note for analysis result types in schema.md
  constraints (they are ephemeral, not serialized)
- S3: Add isComplete() and getAggregateStatus() convenience methods
  to WorkflowReactiveRoot specification
This commit is contained in:
glm-5.1
2026-05-21 19:40:45 +00:00
parent f3e084d02f
commit 907c33650f
14 changed files with 189 additions and 85 deletions
Download Patch File Download Diff File Expand all files Collapse all files

65
docs/architecture/decisions/006-edge-type-consistency.md Normal file
View File

@@ -0,0 +1,65 @@
# ADR-006: Edge Type Consistency and Single-Graph Architecture
## Status
Accepted
## Context
Two related questions affect how edge types are represented in flowgraph:
1. **Should `edgeType` be a required attribute on all edges, or only on edges where it varies?** Operation graphs only have `typed` edges, call graphs have `triggered` and `depends_on`, and template DAGs have `sequential` and `conditional`. Making `edgeType` required on every edge adds redundancy for operation graphs (where it's always `"typed"`) but ensures consistent serialization.
2. **Should `GraphologyHostConfig` produce separate graphs per edge type, or a single shared graph?** Separate graphs per edge type would enable type-specific queries and avoid attribute namespace collisions, but add complexity for cross-graph traversal and cache coherence.
These decisions affect the schema (schema.md), the serialized format, the call graph mutation API, and the host config implementations.
## Decision
1. **`edgeType` is a required universal attribute on every edge**, stored alongside (not inside) the mode-specific attribute schemas. This means the stored edge attributes are always `{ edgeType, ...modeSpecificAttrs }` at the graphology level.
2. **All edge types share a single graphology `DirectedGraph` instance per `FlowGraph`.** No separate graphs per edge type.
3. **Mode-specific attribute schemas (`OperationEdgeAttrs`, `TriggeredEdgeAttrs`, `DependencyEdgeAttrs`) do NOT include `edgeType`.** It's stored separately at the graphology level alongside the mode-specific attributes.
4. **`TemplateEdgeAttrs` includes `edgeType` as a constrained union (`"sequential" | "conditional"`)** because template edges need to distinguish their type for rendering.
## Rationale
### Why `edgeType` on every edge
- **Consistent serialization/deserialization** — graphology's native JSON format requires edge attributes. Having `edgeType` always present simplifies the format: every edge object has an `edgeType` field. No conditional logic to determine whether `edgeType` is present.
- **Uniform graphology queries** — filtering edges by type is always `edge.attributes.edgeType === "triggered"`, regardless of graph mode. No need to check whether the attribute exists first.
- **The redundancy for operation graphs is a minor cost** — `edgeType: "typed"` on every operation edge is two extra bytes per edge. The consistency benefit far outweighs the storage cost.
### Why a single shared graph
- **Cross-graph traversal is unnecessary at current scale** — the expected graph sizes (tens to hundreds of nodes) make separate graphs per edge type an over-engineering. Filtering by `edgeType` is O(n) on edges and negligible.
- **Single graph simplifies the API** — `addEdge`, `removeEdge`, `getEdgeAttributes` work on a single graph. No need for `addTriggeredEdge`, `addDependencyEdge`, `getTypedEdge` etc.
- **Separate graphs add complexity** — maintaining multiple graphology instances, keeping node sets synchronized across graphs, and handling cross-graph queries would add significant implementation and testing burden.
- **Future optimization is possible without API changes** — if a concrete performance issue arises with very large graphs, an internal `Map<EdgeType, Set<string>>` edge index can be added as an optimization without changing the public API.
## Consequences
- **All `FlowGraph` instances store edges with `{ edgeType, ...modeSpecificAttrs }` at the graphology level.** The TypeBox schemas for mode-specific attributes (`OperationEdgeAttrs`, `TriggeredEdgeAttrs`, `DependencyEdgeAttrs`) define only what's unique to each mode; `edgeType` is added separately during edge creation.
- **Edge-type filtering is done via standard graphology attribute queries.** Example: `graph.filterEdges((edge, attrs) => attrs.edgeType === "triggered")`.
- **The `CallEdgeAttrs` union type is discriminated by `edgeType` at runtime** (not by TypeBox schema validation, since `TriggeredEdgeAttrs` and `DependencyEdgeAttrs` are both empty objects). When validating serialized call graph edges, the two-step process applies: (1) check that `edgeType` is present and matches the expected value for the graph mode, (2) validate remaining attributes against the mode-specific schema.
- **Edge key format uses composite keys for multi-type scenarios.** The `triggered` edge type gets the simple `${source}->${target}` key format; `depends_on` always gets the composite `${source}->${target}:depends_on` format. This ensures deterministic keys even when multiple edge types connect the same node pair.
- **Serialization validation is a two-step process:** (1) check that `edgeType` is present and matches the expected value for the graph mode, (2) validate remaining attributes against the mode-specific schema.
## References
- Schema: [schema.md](../schema.md) — Edge attribute schemas, Edge Key Convention
- Call graph: [call-graph.md](../call-graph.md) — `triggered` and `depends_on` edges
- Operation graph: [operation-graph.md](../operation-graph.md) — `typed` edges
- Host configs: [host-configs.md](../host-configs.md) — Single vs. separate graph decision
- Open questions: [open-questions.md](../open-questions.md) — OQ-004, OQ-029