fix: consolidate mermaid ref resolution (fixes #283, #263)

[spinje]

Summary

Consolidates reference resolution in the mermaid visualizer package (src/pflow/core/workflow/mermaid/) behind a single Scope primitive. Both open fidelity bugs (#283, #263) are fixed as structural byproducts of the consolidation.

Task

Follow-up task filed: .taskmaster/tasks/task_155/task-155.md (GraphModel extraction, pre-step to planned web UI — builds on the Scope seam established here).

Changes

New primitive — _scope.py

• Scope dataclass holding a live MermaidContext reference (not a snapshot — sibling outgoing_routes grow incrementally during rendering).
• Scope.resolve(root, field) — three resolution cases: batch item / sibling node (with optional output-field routing) / declared input.
• Scope.refs_in(value) — extract (root, field) pairs from data-flow bindings.
• Scope.source_refs_in(source) — two-stage scan (find ${...} blocks, then extract refs inside) to handle coalesce (${a.x ?? b.y}) and bare input refs (${data}) without false-positive matches on literal text.

Bug fixes

• Mermaid visualizer: data-flow edges lose fidelity when child inputs use inputs: dict #283: _generate_data_flow_edges + _generate_batch_item_data_flow now iterate params["inputs"] (canonical post-task-153) instead of top-level params.items(). Opaque inputs: ${item.inputs} templates skipped gracefully.
• Mermaid output wiring ignores input-root output sources #263: _connect_sources_to_output takes input_ids: dict[str, str] from the caller's scope; input-root refs (including bare ${data}) now emit edges from the input parallelogram.

Dead code deleted

• _RESERVED_PARAMS (blocklist for the pre-task-153 dual-input-passing world)
• _SOURCE_NODE_FIELD_RE (alternate regex, superseded by Scope.source_refs_in)
• _collect_param_refs (orphan — zero callers)
• _refs_input (superseded by Scope.refs_in)
• _resolve_ref_source (inlined into sole call site)

Goldens regenerated

• tests/test_core/golden_mermaid/deep-research-TD.mmd
• tests/test_core/golden_mermaid/deep-research-LR.mmd

Restored per-input edges (e.g. prepare --> analyze-sources__in_content, combine --> reviews__accuracy__in_summary) that were lost during the task-153 migration. document-processor golden unchanged (its bindings all reference top-level inputs, handled by _connect_top_level_inputs's nearest-consumer heuristic).

KNOWN REGRESSION (GH #283) docstring at test_mermaid_golden.py:59-66 removed.

Tests

• 3 outdated tests migrated from pre-task-153 top-level-params IR shape to canonical inputs: dict shape.
• 5 new regression tests:
  • test_opaque_template_inputs_fall_through_gracefully — inputs: ${item.inputs} doesn't crash
  • test_batch_data_flow_with_inputs_dict — Mermaid visualizer: data-flow edges lose fidelity when child inputs use inputs: dict #283 batch variant
  • test_output_source_from_declared_input — Mermaid output wiring ignores input-root output sources #263 with field
  • test_output_source_from_bare_input_ref — Mermaid output wiring ignores input-root output sources #263 bare ref (no dot)
  • test_output_source_from_input_in_subworkflow — Mermaid output wiring ignores input-root output sources #263 at sub-workflow scope ({prefix}in_{name} convention)
• Counter-assertion added to test_data_flow_edges_from_params to guard the depth-0 double-emit skip (caught by mutation testing — the skip was previously load-bearing but untested).

Explanation

Both bugs share one cause: the visualizer answered the question "given a ${x.y}, what mermaid ID does it resolve to?" in seven different places with seven different levels of completeness. Fixing each function independently would preserve that fragmentation and keep the door open for the next similar bug.

The refactor collapses all seven call sites to route through Scope.resolve. Any ref-consuming site now knows about nodes AND declared inputs AND batch items — no "silently drops inputs" path remains.

Depth-0 double-emit concern: _connect_top_level_inputs emits input_X → consumer__in_Y at the top level using a layout-preserving nearest-consumer heuristic (from task 146 — long-range edges destroy dagre layout). Scope.resolve is pure (always resolves), so the two data-flow generators add a two-line caller-side skip for depth-0 top-level input refs to avoid duplicate emission.

Created docs

• .taskmaster/tasks/task_155/task-155.md — follow-up task for GraphModel extraction (pre-step to planned React Flow or equivalent web UI). Builds on the Scope primitive.

Testing

• 78 mermaid-specific tests pass (tests/test_core/test_mermaid.py, test_mermaid_golden.py, tests/test_cli/test_visualize.py).
• 4864 full suite pass (make test).
• make check clean (ruff, ruff-format, mypy, deptry).
• Mutation-tested: breaking the Mermaid visualizer: data-flow edges lose fidelity when child inputs use inputs: dict #283 fix, Mermaid output wiring ignores input-root output sources #263 fix, or depth-0 skip now fails dedicated assertions (the depth-0 skip was previously unguarded — added counter-assertion).
• Adversarial regex tested: empty / literal / coalesce / triple-coalesce / bare refs / hyphens / adjacent templates / malformed ${}.

Manual visual verification in mermaid.live is still recommended for the regenerated deep-research-TD.mmd / deep-research-LR.mmd — string-level diffs miss visual breakage per mermaid/CLAUDE.md.

[gemini-code-assist]

Code Review

This pull request implements the Scope consolidation for the mermaid visualizer, a prerequisite for the planned graph model extraction. It introduces a centralized Scope class in _scope.py to unify template reference resolution, replacing scattered logic in _edges.py and _io.py. These changes fix fidelity bugs where data-flow edges were omitted for nested inputs: dictionaries (GH #283) and workflow-level input references in output sources (GH #263). Feedback focuses on improving the robustness of the new reference extraction logic to handle coalesce expressions and correcting a regex pattern.

[gemini-code-assist]

The refs_in method currently misses references in coalesce expressions (e.g., ${a.x ?? b.y}) because _DATA_FLOW_REF_PATTERN only matches identifiers preceded by ${. To maintain consistency with source_refs_in and correctly identify all dependencies in a binding, this method should leverage the same two-stage scanning logic.

Suggested change

	def refs_in(value: str) -> list[tuple[str, Optional[str]]]:
	"""Extract ``(root, field)`` pairs from template refs in a binding.
	Use for data-flow bindings where each template expression is one ref
	(e.g. ``"${producer.response}"``). Returns ``field=None`` for bare
	refs like ``"${data}"``.
	"""
	return [(m.group(1), m.group(2)) for m in _DATA_FLOW_REF_PATTERN.finditer(value)]
	@staticmethod
	def refs_in(value: str) -> list[tuple[str, Optional[str]]]:
	"""Extract ``(root, field)`` pairs from template refs in a binding.
	Handles multiple templates and coalesce expressions (e.g. ``${a.x ?? b.y}``).
	Returns ``field=None`` for bare refs like ``"${data}"``.
	"""
	return Scope.source_refs_in(value)

[gemini-code-assist]

This regex is missing a closing brace \}. While it works for extraction in simple cases, it is less robust than a complete template pattern. If the refactoring of refs_in to use source_refs_in is adopted, this constant can be removed entirely.

[claude]

Review — PR #299 (fixes #283 + #263)

Solid refactor. The Scope primitive is the right abstraction — one function replaces seven inconsistent call sites, and both fidelity bugs become structural byproducts rather than point fixes. Regression coverage is strong (including the mutation-testing-driven counter-assertion on the depth-0 skip), and the golden-file diffs cleanly restore per-input edges lost during task 153.

No blockers. A few minor issues worth addressing before merge.

Warnings — should be addressed

1. Stale comment on _PARAM_REF_RE (_context.py:42)

# Regex patterns (used outside `_scope.py` — e.g. `_extract_batch_source`)
_PARAM_REF_RE = re.compile(r"\$\{([a-zA-Z0-9_-]+)(?:\.|\})")

_extract_batch_source no longer uses this regex (now routes through Scope.refs_in). The only remaining consumer is _dynamic_batch_label at line 256. Update the comment, or better — collapse _dynamic_batch_label onto Scope.refs_in and delete _PARAM_REF_RE entirely. Keeping two near-identical regexes for the same question in the same package is what this PR is trying to eliminate.

Suggested:

# Used only by `_dynamic_batch_label` below (subtly stricter than Scope.refs_in —
# requires a `}` or `.` terminator so it rejects partial refs in labels).

…or drop it and use Scope.refs_in in _dynamic_batch_label.

2. Input/node name precedence change in _connect_sources_to_output

for root, field in Scope.source_refs_in(source):
    if root in input_ids:
        ctx.lines.append(...)
        continue
    if root not in node_ids:
        continue

Input IDs are now checked before node IDs. If an input and a sibling node share a name, the input wins — old behavior had the node win (input refs were silently dropped, which was the bug). This is almost certainly fine in practice (validator forbids same-name collisions), but the precedence is now load-bearing and undocumented. Either:

• Add a one-liner comment: # Inputs take precedence over nodes when names collide — validator should reject same-name IR, but be explicit.
• Or add an assert root not in node_ids guard in the input branch to make the invariant explicit.

Suggestions — optional improvements

3. Weak assertion in test_opaque_template_inputs_fall_through_gracefully

assert "consumer__in_x -->" in out or "consumer__in_x[/" in out  # input node still rendered

The or makes this test pass if either an outgoing edge OR a rendered input node exists — either one alone would not confirm the "no crash and input is still drawn" invariant the test is asserting. A mutation that drops the input-node rendering but keeps an edge (or vice versa) would pass. Consider splitting:

assert "consumer__in_x[/" in out, "input node must be rendered"
# The meaningful edge-absence check is already covered below

4. Scope.resolve — root == "item" short-circuits silently

if root == "item":
    if not self.batch_source:
        return None
    ...

If root == "item" and we're not in a batch, this returns None without falling through to check sibling nodes. A sibling node literally named item would be unresolvable outside batch context. This matches old behavior (not a regression), but worth a line in the docstring:

# NOTE: "item" is reserved for batch item refs — a sibling node named
# "item" is not reachable outside batch context by design.

5. Regex permissiveness shift (informational)

Scope.refs_in uses _DATA_FLOW_REF_PATTERN which (unlike the removed _refs_input / _PARAM_REF_RE) does not require a } or . terminator after the root. Malformed templates like ${data abc} would now match data as a root, while the old _refs_input would reject it. For well-formed workflows (validator-gated) this is a non-issue, but if the validator ever stops catching these you'd get silently-emitted edges for truncated templates. Not something to change in this PR — just worth a line in the Scope module docstring.

Strengths worth calling out

• Root-cause refactor over point fixes. Both bugs were symptoms of the same fragmentation; collapsing to one primitive closes the door on the next _refs_in-shaped bug.
• _BRACE_BLOCK_RE two-stage scan is a cleaner model than the old _SOURCE_NODE_FIELD_RE. Requiring refs to live inside ${…} blocks eliminates the false-positive-from-literal-text category entirely.
• Counter-assertion on the depth-0 skip in test_data_flow_edges_from_params. Explicitly verifying exactly-once emission (with a diagnostic message that names the bug) is the kind of test that catches mutations rather than accumulating coverage.
• Dead-code deletion is complete — no orphaned imports, all five removed symbols verified absent outside of .taskmaster/ historical docs and one intentional docstring back-reference in test_mermaid.py:1446.
• Golden regeneration is justified and documented in the PR body. The restored per-input edges (prepare → analyze-sources__in_content, combine → reviews__accuracy__in_summary) match the structural intent.

Verification

• Cannot run pytest locally in this review sandbox. PR body reports 78 mermaid tests + 4864 full suite pass and make check clean.
• Manual visual verification in mermaid.live for deep-research-TD.mmd / deep-research-LR.mmd should happen before merge per mermaid/CLAUDE.md — string-level diffs miss visual breakage.

Recommendation: approve after addressing the stale comment (#1) and documenting the input/node precedence (#2). Everything else is optional.