[elasticdotventures]

Summary

• Extends ProcessChart.from_sysml to handle all four KerML Activity control-flow node kinds: DecisionNode, ForkNode, JoinNode, MergeNode
• Adds decide(node_id, guards) and fork(node_id) drive methods on ProcessChart
• Bumps ufo-types pin from 5f8d018 → 6c8e9cc (feat/control-flow-nodes-rebased — awaiting upstream PR feat(sysml_model): add KerML Activity control-flow nodes (DecisionNode/ForkNode/JoinNode/MergeNode) ufo-types#35 merge; will update to squash SHA after merge)

Control-flow node semantics

SysML kind	Event fired	Notes
DecisionNode	"guard:<id>_true/false"	decide(id, guards) reads <id>_cond guard
ForkNode	"fork:<id>"	advances to first outgoing; caller handles concurrency
JoinNode	"next"	pass-through atomic state
MergeNode	"next"	same pass-through semantics

Test plan

• 7 kr0ki-core tests pass (3 existing + 4 new): decision true/false, fork, merge
• ufo-types pin updates to merged SHA after feat(sysml_model): add KerML Activity control-flow nodes (DecisionNode/ForkNode/JoinNode/MergeNode) ufo-types#35 lands

Dependencies

Stacked on PR #265 (task/259-p4-kr0ki-statechart).
Requires PromptExecution/ufo-types#35 to merge (or can be rebased to main once merged).

Closes #268

🤖 Generated with Claude Code

[elasticdotventures]

Adversarial Review: feat(kr0ki-core): Phase 5 — DecisionNode/ForkNode/JoinNode/MergeNode in ProcessChart (closes #268)

Provider: sm3lly/neo-coder | Generated: 2026-10-13T13:48Z

---

🟢 Advocate

Approach Assessment

This PR correctly extends ProcessChart to handle SysML-v2 control-flow nodes (DecisionNode, ForkNode, JoinNode, MergeNode) on top of the existing ActionDefinition/ActionUsage mapping. The approach is sound because:

1. Minimal API surface: Adds only two new drive methods (decide, fork) that map directly to the control-flow semantics. Join/Merge reuse the existing advance("next") pattern, keeping the interface consistent.

2. Guard naming convention is explicit and documented: The <id>_cond guard naming is clearly specified in both the module docs and the decide() method docstring. This convention is enforced at build time via the decision_guards map, preventing typos from silently failing.

3. Fork semantics are correctly delegated: The PR acknowledges that a single-threaded state chart cannot represent true parallelism, so it fires fork:<id> and advances to the first branch — leaving concurrency management to the caller. This is the right architectural choice for this layer.

4. Backward compatibility preserved: Existing advance() behavior for linear flows is unchanged. The new node kinds are additive; charts without control-flow nodes work identically.

Strong Points

• Clean refactoring of build_chart_def: The original code filtered elements into separate vectors (action_defs, action_usages) and processed them in two passes. The new code processes all elements in a single pass with a match on ElementKind. This is simpler, more extensible, and eliminates redundant iteration.

• Decision guard map is built at chart construction time: The decision_guards HashMap is populated during from_sysml, not at runtime. This means guard name validation happens once at build time, and decide() can fail fast if called with an unknown node ID.

• Tests cover all four new node kinds: The PR includes dedicated tests for DecisionNode (true branch), DecisionNode (false branch), ForkNode, and MergeNode. Each test is self-contained and validates the specific routing behavior.

• Documentation is thorough: The module-level docstring now includes a complete mapping table for all SysML v2 kinds, making the chart's behavior discoverable without reading the implementation.

• Error handling is consistent: Duplicate ID detection, chart build errors, and unknown decision node IDs all use the existing error patterns (Kr0kiError::DuplicateId, Kr0kiError::ChartBuild, return false).

Evidence

• 4 new focused tests covering each control-flow node kind:

  • decision_node_routes_true_branch — validates guard=true routing
  • decision_node_routes_false_branch — validates guard=false routing
  • fork_node_fires_and_advances — validates fork event and first-branch advancement
  • merge_node_passes_through — validates Join/Merge pass-through semantics

• Existing tests still pass: The PR notes that all 7 kr0ki-core tests (3 existing + 4 new) pass, confirming backward compatibility.

• Minimal diff footprint: Only 282 additions / 104 deletions, focused entirely on the control-flow node implementation and its tests. No unrelated changes.

Recommended to Merge If...

• The ufo-types dependency bump (5f8d018 → 6c8e9cc) is verified to build and pass tests in CI
• The upstream PR feat(sysml_model): add KerML Activity control-flow nodes (DecisionNode/ForkNode/JoinNode/MergeNode) ufo-types#35 is either merged or the team accepts the temporary pin to the feature branch commit
• Code review confirms the guard naming convention (<id>_cond) aligns with the broader SysML-v2 tooling ecosystem being used

The implementation is clean, well-tested, and follows established patterns in the codebase. No architectural concerns.

---

🔴 Challenger

Concerns

DecisionNode routing is positional, not guard-driven. The PR maps the first outgoing Succession edge to the "true" branch and the second to "false" — but SysML v2 DecisionNode guards are attached to edges, not nodes. If the diagram author reorders edges in the tool, routing silently swaps. The PR's own table says decide(id, guards) "reads <id>_cond guard" but the implementation only reads one boolean and never inspects edge-level guard expressions. This is a semantic mismatch that will bite when real SysML models with edge guards are imported.

ForkNode only advances one branch. fork(node_id) fires fork:<id> and transitions to the first outgoing successor. The PR acknowledges "caller handles concurrency" but provides no mechanism to track which branches have been taken or to join them later. For a state machine that's supposed to model SysML activity semantics, this is a significant simplification that isn't documented as such. A caller who doesn't know about this limitation will think both branches are active.

JoinNode and MergeNode are conflated. The PR treats both as pass-through on "next", but in SysML v2 they have different semantics: JoinNode requires all incoming branches to arrive (synchronization), while MergeNode passes through on any incoming branch (choice merge). The PR's own table says JoinNode "waits for 'next' from any incoming" which is actually MergeNode semantics. This will produce incorrect behavior for true join scenarios.

Guard naming convention is undocumented magic. The guard name <id>_cond is hardcoded in two places (build_chart_def and decide) with no validation that the caller supplies it correctly. If a caller passes guards without the exact <id>_cond key, decide silently returns false (no transition) rather than erroring.

Missing Coverage

• No test for DecisionNode with more than two outgoing edges. The implementation assumes exactly two; what happens with three? It silently routes the third edge nowhere.
• No test for JoinNode synchronization semantics (since they're not actually implemented).
• No error handling for malformed charts: DecisionNode with zero or one outgoing edge, ForkNode with no outgoing edges.
• No documentation of the guard naming convention in the public API docs — callers have to read the source to know they must use <id>_cond.
• No integration test showing a complete flow through decision → fork → join → merge.

Alternative Approaches

For DecisionNode: Instead of positional edge mapping, require guard expressions on edges and evaluate them at runtime. If that's too complex for this phase, at least validate that exactly two outgoing edges exist and document the positional assumption prominently.

For ForkNode/JoinNode: Consider whether a single-threaded state machine is the right model for SysML activity diagrams at all. Activity diagrams are inherently concurrent. If concurrency is out of scope, rename the API to make that explicit (e.g., fork_first_branch) and document the limitation.

For JoinNode vs MergeNode: Either implement the correct semantics (track incoming branch count for JoinNode) or pick one and document why the other is deferred. Don't silently conflate them.

Blocking Issues

The DecisionNode positional routing is a correctness issue that should be fixed before merge. The JoinNode/MergeNode conflation is also a correctness issue. These aren't just "nice to have" improvements — they mean the implementation doesn't actually match the SysML v2 semantics it claims to support.

[elasticdotventures]

Adversarial Review: feat(kr0ki-core): Phase 5 — DecisionNode/ForkNode/JoinNode/MergeNode in ProcessChart (closes #268)

Provider: sm3lly/neo-coder | Generated: 2026-10-15T17:48Z

---

🟢 Advocate

Approach Assessment

The PR correctly extends the existing ProcessChart SysML→lazily mapping to cover the four KerML Activity control-flow node kinds that were previously unhandled. The approach is sound because:

• It reuses the established ChartBuilder/StateBuilder API rather than introducing a new representation.
• It maps each SysML node to a well-defined lazily event/transition pattern, keeping the chart deterministic and driveable.
• It preserves backward compatibility: linear advance() flows continue to work unchanged for charts without control-flow nodes.

The PR also correctly identifies its dependency on the upstream ufo-types change (PromptExecution/ufo-types#35) and pins to the feature branch SHA, which is the right practice for stacked development.

Strong Points

• Clear node semantics table. The module doc and PR summary both document exactly which event each node kind fires. This is valuable for future maintainers and for the SysML→lazily spec.
• Guard naming convention. Using "<id>_cond" as the guard name and guard:<id>_true/guard:<id>_false as events is systematic, predictable, and testable.
• ForkNode design decision. Rather than trying to model parallelism inside the chart (which lazily doesn't support), the PR fires fork:<id> and documents that the caller handles concurrency. This is the correct pragmatic choice for a single-threaded state machine library.
• JoinNode/MergeNode pass-through. Treating both as atomic pass-through on "next" is simple and correct for the common case. The complexity of join synchronization is deferred to the caller, which is appropriate.
• Good test coverage. Four new tests cover: decision true branch, decision false branch, fork advancement, and merge pass-through. Each test is focused and verifies the specific behavior it's named for.
• Refactored build_chart_def. The single-pass loop over elements with a match on ElementKind is cleaner than the previous multi-pass approach (separate loops for defs, usages, etc.). This will be easier to extend for future node kinds.

Evidence

• Tests: 4 new tests added, each targeting a specific control-flow node behavior. The tests use realistic SysML element/relation structures (FeatureMembership, Succession) and verify both the transition firing and the resulting active state.
• Documentation: The module doc includes a complete mapping table that will serve as the reference for future extensions.
• Dependency tracking: The PR explicitly documents the ufo-types dependency and the plan to update the pin after the upstream merge.

Recommended to Merge If...

• The ufo-types pin update is acceptable as a temporary state (or the upstream PR is merged and the pin is updated before this PR lands).
• The test suite passes in CI (the PR notes "7 kr0ki-core tests pass" but this should be verified in the CI run).
• No other stacked PRs in the chain have blocking issues.

The implementation is clean, well-documented, and correctly scoped. The design decisions (especially around ForkNode concurrency) are pragmatic and well-justified.

---

🔴 Challenger

Concerns

DecisionNode guard naming convention is fragile. The PR hardcodes <id>_cond as the guard name, but SysML v2 doesn't mandate this. If a modeler names their guard differently, decide() silently returns false without erroring. This is a silent failure mode that will be hard to debug.

ForkNode semantics are incomplete. The PR advances only to the first outgoing branch and says "caller handles concurrency." This pushes the hard part of fork semantics (parallel execution) onto the caller while the chart itself only tracks one branch. If the caller forgets to fire events for other branches, the chart state becomes inconsistent. The fork() method name suggests it handles forking, but it really just advances to one branch.

JoinNode vs MergeNode distinction is lost. Both map to identical pass-through behavior on "next". In SysML v2, JoinNode waits for all incoming tokens (AND join) while MergeNode accepts any incoming token (OR join). The PR's docstring claims JoinNode "waits for 'next' from any incoming" which is actually MergeNode semantics. This is a semantic error.

Succession edge ordering assumption. For DecisionNode, the PR assumes the first outgoing edge is the true branch and the second is the false branch. SysML doesn't guarantee edge ordering — this is an arbitrary convention that will break when edges are defined in different order.

No validation of DecisionNode arity. The PR expects exactly two outgoing edges from a DecisionNode but doesn't validate this. A decision with one or three edges will build successfully but behave incorrectly.

Missing Coverage

• JoinNode AND semantics test. No test verifies that JoinNode waits for multiple incoming transitions (it can't with the current implementation, but the test would expose the gap).
• DecisionNode with non-standard guard names. No test for guards that don't follow the <id>_cond convention.
• ForkNode with multiple parallel branches. No test verifies behavior when caller needs to advance multiple branches.
• Error cases. No tests for: DecisionNode with 0, 1, or 3+ outgoing edges; ForkNode with no outgoing edges; chart with cycles involving control-flow nodes.
• Documentation of guard naming convention. The <id>_cond convention is only mentioned in code comments, not in user-facing docs.

Alternative Approaches

For DecisionNode guards: Instead of hardcoding <id>_cond, accept the guard name as an explicit parameter to decide(), or expose a method to query the guard name for a given decision node. This removes the naming convention assumption.

For JoinNode vs MergeNode: Implement actual JoinNode semantics using a counter or set of arrived branches. This requires tracking which incoming edges have fired, which is more complex but correct. Alternatively, rename the methods to be explicit about the simplified semantics.

For edge ordering: Require explicit edge labels or a mapping from decision node + branch value to target, rather than relying on insertion order.

Blocking Issues

JoinNode semantics are wrong. The PR claims to implement JoinNode but implements MergeNode semantics instead. This is a correctness bug, not just a missing feature. Either implement proper AND-join semantics or rename the node kind to avoid misleading users. This should block merge until resolved or documented as a known limitation with a clear distinction from true JoinNode behavior.