From 9a1d33294659dcc6110d66fa183f8c2678f680de Mon Sep 17 00:00:00 2001 From: Boris Dev Date: Tue, 6 Oct 2026 21:24:19 +0000 Subject: [PATCH 1/3] README: draw the specification diagram, not only the two-strategy diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The before/after is the pitch — the same graph with nothing implemented, then with two implementations bound — and only the second half was ever drawn. `diagram()` was described in prose and never shown. The check that should have caught this could not. `test_the_readme_mermaid_block_is_ the_diagram_the_code_emits` asserted ONE block by name, at a time when the README had exactly one: a check narrower than its claim, and blind to a missing picture because the blocks that are present all pass. Replaced with a set match over every mermaid block in every markdown file, in both directions — a block with no generator fails, and a registered generator with no block fails too. Verified both go red. 282 tests green locally. No CI in this repo, so that is one local run. --- README.md | 25 +++++++++++++-- tests/test_greeting.py | 69 +++++++++++++++++++++++++++++++++++++----- 2 files changed, 83 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index ede6510..f3f2689 100644 --- a/README.md +++ b/README.md @@ -116,8 +116,27 @@ One workflow — normalize a name, then compose a greeting from it: Desired behaviour: preserve the name's words, trim surrounding whitespace, collapse repeated internal whitespace, return `Hello, {name}!`. -Two strategies disagree about how much of that `normalize` does. `compose` is the same function -in both, so the comparison diagram highlights the one node that varies: +That table is the whole declaration, and it draws itself. **Nothing is implemented at this +point** — no `normalize` body, no `compose` body, no strategy, nothing an agent has written: + +```mermaid +flowchart TD + START([START]) + normalize["normalize"] + compose["compose"] + END([END]) + START -- raw_name --> normalize + normalize -- clean_name --> compose + compose -- greeting --> END +``` + +Bare boxes, because nothing is bound to them yet. This picture and `coherence_check()` are what +you review *before* asking an agent for a line of code — which is the one thing a drawing taken +from a built graph cannot do, since building it requires the code to already exist. + +Two strategies disagree about how much of that `normalize` does. Same graph, two implementations +bound: `compose` is the same function in both, so the comparison greys it and highlights the one +node that varies: ```mermaid flowchart TD @@ -194,7 +213,7 @@ Everything it produces goes to the terminal; no files are written. Excerpt: normalize_spaces 1.00 ``` -Two mermaid blocks go past on the way: the specification, and the comparison above. A browser +Both mermaid blocks above go past on the way — the specification, then the comparison. A browser viewer is available as a separate process — `uv run python3 -m workflow_workbench.cli serve`, see [`serve.py`](workflow_workbench/serve.py) — and nothing in the quickstart needs it. diff --git a/tests/test_greeting.py b/tests/test_greeting.py index 2661bea..dbee41f 100644 --- a/tests/test_greeting.py +++ b/tests/test_greeting.py @@ -104,17 +104,70 @@ def _score(report) -> float: return float(getattr(v, "value", v)) -def test_the_readme_mermaid_block_is_the_diagram_the_code_emits() -> None: - """⛔ THE ONE THAT MATTERS MOST. A hand-pasted diagram is a claim about the declaration that - stops being true the moment a node is renamed, and nothing else would notice. +# ── every drawn diagram, in BOTH directions ────────────────────────────────────────────────── +# +# ⛔ The check this replaced asserted ONE block — `diff_diagram()` — by name, at a time when the +# README contained exactly one. That is the repo's recurring defect: a check narrower than its +# claim, which goes quiet the moment a second block is pasted in by hand. So the registry below +# is matched against the docs as a SET, and a block with no entry fails just as loudly as an +# entry with no block. + +def _drawn() -> dict[str, str]: + """Every mermaid body the docs are expected to show, keyed by how to regenerate it. The `%%` title line is dropped because a fenced mermaid block on GitHub does not need it. """ - drawn = Greeting().diff_diagram(trim_only, normalize_spaces) - body = "\n".join(ln for ln in drawn.splitlines() if not ln.startswith("%%")).strip() - assert f"```mermaid\n{body}\n```" in README, ( - "the README's mermaid block is not what diff_diagram() emits. Regenerate it:\n" - " uv run python3 -m examples.greeting") + spec = Greeting() + return { + "Greeting().diagram()": spec.diagram(), + "Greeting().diff_diagram(trim_only, normalize_spaces)": + spec.diff_diagram(trim_only, normalize_spaces), + } + + +def _fenced(body: str) -> str: + return "```mermaid\n" + "\n".join( + ln for ln in body.splitlines() if not ln.startswith("%%")).strip() + "\n```" + + +def _blocks_in_markdown() -> dict[str, list[str]]: + """⚠️ Every markdown file, not just the README. A hand-pasted diagram in `docs/` is the same + claim about the declaration and rots the same way.""" + found: dict[str, list[str]] = {} + for path in sorted(ROOT.rglob("*.md")): + if any(part in {".venv", "node_modules", ".git"} for part in path.parts): + continue + text = path.read_text() + hits = [f"```mermaid\n{chunk.split('```')[0].strip()}\n```" + for chunk in text.split("```mermaid\n")[1:]] + if hits: + found[str(path.relative_to(ROOT))] = hits + return found + + +def test_every_mermaid_block_in_the_docs_is_one_the_code_emits() -> None: + """⛔ THE ONE THAT MATTERS MOST. A hand-pasted diagram is a claim about the declaration that + stops being true the moment a node is renamed, and nothing else would notice.""" + expected = {_fenced(body): how for how, body in _drawn().items()} + for path, blocks in _blocks_in_markdown().items(): + for block in blocks: + assert block in expected, ( + f"{path} contains a mermaid block no generator emits. Either regenerate it " + f"(uv run python3 -m examples.greeting) or register its source in _drawn().\n" + f"{block}") + + +def test_every_diagram_the_code_emits_is_actually_drawn_in_the_docs() -> None: + """The other direction, and the reason the plain `diagram()` block exists at all. + + `diagram()` — the picture with NOTHING implemented — was described in prose for weeks and + never drawn, while only the two-strategy diff was shown. A one-directional check cannot see + a missing picture: the blocks that are present all pass. + """ + drawn_anywhere = {b for blocks in _blocks_in_markdown().values() for b in blocks} + for how, body in _drawn().items(): + assert _fenced(body) in drawn_anywhere, ( + f"{how} is registered as a diagram the docs show, and no markdown file shows it.") @pytest.mark.parametrize("case,text,expected", CASES) From e03353ede8a0b1ac7b46fefb02ea9c12db0ba07c Mon Sep 17 00:00:00 2001 From: Boris Dev Date: Tue, 6 Oct 2026 22:27:27 +0000 Subject: [PATCH 2/3] check_boundary_types: the declared boundary was never compared to the design (#21) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A GraphSpec's `input_type` / `output_type` go straight to `GraphBuilder`, and nothing checked them against the variables the START/END edges carry: class Lying(Greeting): # every edge in Greeting carries str input_type, output_type = int, int coherence_check() -> [] run_sync(' a b ') -> 'Hello, a b!' A dead field would have been the small version. `_port_type` reads those two fields as the ORACLE for `check_subgraphs`, so a check that DOES run — whose whole job is "the child fits the node" — compared a parent node's contract against the child's unverified claim about itself. Measured: a child declaring `output_type=int` with every edge carrying `str` passed against a parent node declaring an int output, and the graph returned 'HELLO'. Assignability, not identity, and the direction differs per side: widening is legal on the way IN (parallel.py's `list[int]` into `numbers: list`), narrowing on the way OUT (stage10's `report: str` into `output_type=object`). The reverse of each is a finding, and test_the_reverse_of_each_is_NOT_legal is why the two legal cases prove anything. Two helper fixes fell out, both real: _produces called `list[int]` vs `list[int]` undecidable (`is` is False on aliases) and never compared a parameterised alias to a bare type. Both now decide — which can only turn an undecidable into a verdict, never manufacture a finding. This is why the new check is clean on all 14 designs in examples/ rather than printing a permanent NOT CHECKED on parallel.py. _type_name rendered `list[int]` and `list` IDENTICALLY. Its docstring said generic aliases have no `__name__`; since 3.10 they do, and it is the bare origin — a wrong name, not a missing one. `about=""`, not `about="input_type"`: a port name would be a fifth kind of value in a field `test_every_finding_names_something_the_caller_can_look_up` resolves. That is a vocabulary change — proposed on the issue, not slipped in here. Against the handoff's 7-for-7 pattern (every Copilot finding was a check narrower than its claim, twice shipped in the same commit as the thing it guards): the broad test runs over every GraphSpec in examples/, and one test reproduces #21 verbatim on a SHIPPED example rather than a fixture this module authored. 13 rules now, 8 needing nothing implemented. 294 tests green locally — no CI in this repo, so that is one local run. --- CHANGELOG.md | 48 ++++++ README.md | 9 +- tests/test_boundary_types.py | 273 +++++++++++++++++++++++++++++++ workflow_workbench/__init__.py | 3 +- workflow_workbench/checks.py | 125 +++++++++++++- workflow_workbench/graph_spec.py | 1 + 6 files changed, 448 insertions(+), 11 deletions(-) create mode 100644 tests/test_boundary_types.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 66d70e6..0c8e370 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,54 @@ Versioning: [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Added — `check_boundary_types`, the 13th rule + +A `GraphSpec`'s declared `input_type` / `output_type` are now compared against the variables the +edges at START and END actually carry. They were never checked, and they go straight to +`GraphBuilder`: + +```python +class Lying(Greeting): # every edge in Greeting carries str + input_type, output_type = int, int + +Lying().coherence_check() # [] — before +Lying().render(...).run_sync(inputs=" a b ") # 'Hello, a b!' — a str +``` + +**A dead field would have been the small version.** `_port_type` reads those two fields as the +ORACLE for `check_subgraphs`, so a check that does run — whose whole job is *"the child fits the +node"* — was comparing a parent node's contract against the child's unverified claim about +itself. Measured: a child declaring `output_type=int` while every edge in it carries `str` passed +against a parent node declaring an int output, and the graph returned `'HELLO'`. Issue #21. + +Assignability, not identity, and the direction differs per side: the input may be **widened** on +the way in (`input_type=list[int]` into an edge carrying `numbers: list` — `examples/parallel.py` +does this), the output **narrowed** on the way out (`report: str` reaching `output_type=object` — +`examples/ladder/stage10_no_basenode.py` does this). + +⚠️ **The default `type(None)` is a claim, not an absence.** A design that never declares a +boundary and then wires a `str` across it is now reported, because that default reaches the +engine as the graph's real signature. + +### Fixed — `_produces` called two spellings of one type undecidable + +`list[int] is list[int]` is `False`, so identity alone reported *not decidable* for literally the +same type; and a parameterised alias was never compared against a bare declared type, although +`list[int]` plainly IS a `list`. Both now decide. This can only turn an undecidable into a +verdict — it cannot manufacture a finding where there was none — and it is why +`check_boundary_types` is clean on all 28 boundary crossings in `examples/` rather than printing +a permanent `NOT CHECKED` line on `parallel.py`. + +`check_variable_types` reads the same helper and gains the same decidability. + +### Fixed — `_type_name` rendered `list[int]` and `list` identically + +Its docstring said generic aliases have no `__name__`. Since 3.10 they do, and it is the bare +origin — so the bug was a WRONG name rather than a missing one, and a finding comparing those two +types read as a complaint that `list` is not `list`. Found while writing the message for the +check above, which compares exactly that pair. + + ## [0.3.0] — 2026-10-01 ### ⛔ Breaking — `check()` is renamed to `coherence_check()` diff --git a/README.md b/README.md index f3f2689..7827349 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,7 @@ shape and its data contracts as **data**, before any step exists — which is wh of defect findable: ```python -spec.coherence_check() # 12 well-formedness rules, 7 of them with nothing implemented +spec.coherence_check() # 13 well-formedness rules, 8 of them with nothing implemented spec.diagram() # a picture of the same declaration spec.render(strategy) # refuses outright if anything blocks ``` @@ -39,7 +39,7 @@ Four problems, and the same declaration answers all four: | | | |---|---| -| **An agent's output works and is incoherent.** Each piece is locally fine; the whole does not add up. | `coherence_check()` — 12 [well-formedness rules](docs/glossary.md#well-formedness-rule), 7 needing nothing implemented | +| **An agent's output works and is incoherent.** Each piece is locally fine; the whole does not add up. | `coherence_check()` — 13 [well-formedness rules](docs/glossary.md#well-formedness-rule), 8 needing nothing implemented | | **A reasoning strategy cannot be asserted correct — only compared.** There is no right answer to diff against, so "better" is an empirical question. | [`eval_battle()`](docs/glossary.md#battle) — same cases, same evaluators, plus a replicate arm as the [noise floor](docs/glossary.md#noise-floor) | | **Complexity grows unless pieces are reused.** Two arms that differ in one stage should say so, not be two files. | the [data language](docs/glossary.md#deep-embedding): declare a role once, bind it many ways; `SubgraphBinding` reuses a whole child design as one node | | **You cannot see what you built.** | `diagram()` and `diff_diagram()`, from the declaration alone | @@ -238,9 +238,9 @@ missed. Every rule — generated from each check's own docstring -**12 rules.** `coherence_check()` returns one finding per violation and an empty list for a clean design; `render()` refuses on any finding that blocks. +**13 rules.** `coherence_check()` returns one finding per violation and an empty list for a clean design; `render()` refuses on any finding that blocks. -**7 need no implementations at all** — runnable the moment `nodes` and `edges` are written. +**8 need no implementations at all** — runnable the moment `nodes` and `edges` are written. | check | rule | |---|---| @@ -250,6 +250,7 @@ missed. | `check_step_arity` | A step body receives exactly ONE value, so a node cannot consume two inputs at once. | | `check_decisions` | `when` appears exactly on the edges leaving a decision, and nowhere else. | | `check_transform_edges` | A transform edge is fixed (`apply=`) or a variation point (bound) — exactly one. | +| `check_boundary_types` | The graph's declared `input_type` / `output_type` match what crosses START and END. | | `check_fan_out_rejoins` | Everything a fan-out produces must reach a join before it reaches END. | **5 more once a strategy exists**, checking the implementations against the roles they fill. diff --git a/tests/test_boundary_types.py b/tests/test_boundary_types.py new file mode 100644 index 0000000..9b04201 --- /dev/null +++ b/tests/test_boundary_types.py @@ -0,0 +1,273 @@ +"""`check_boundary_types` — the graph's declared boundary against the edges that cross it. + +⛔ WHY THIS FILE EXISTS. `input_type` / `output_type` go straight to `GraphBuilder`, and until +2026-10-06 nothing compared them to the design they belong to. A dead field would have been the +small version: `_port_type` reads them as the ORACLE for `check_subgraphs`, so a check that does +run, whose whole job is "the child fits the node", was comparing a node's contract against the +child's unverified claim about itself. Issue #21. + +⚠️ The failure mode this file is written against is the repo's own recurring one, 7 for 7 across +#8, #18 and #19: **a check narrower than its claim.** A guard written beside the thing it guards +inherits the author's assumption about where the fact lives. So the broad test here is +`test_no_design_in_the_repo_is_flagged_or_silently_skipped`, which runs over every `GraphSpec` in +`examples/` rather than over the four this module authors. +""" +from __future__ import annotations + +import importlib +import pkgutil + +from workflow_workbench import ( + END, START, EdgeSpec, GraphSpec, StepSpec, StrategySpec, SubgraphBinding, + TransformEdgeSpec, VariableSpec, check_boundary_types) +from workflow_workbench.checks import NOT_CHECKED, blocking + +text = VariableSpec("text", str) +num = VariableSpec("num", int) +items = VariableSpec("items", list) + +step = StepSpec("step", inputs=(text,), outputs=(text,)) + + +def _design(**attrs) -> GraphSpec: + base = {"name": "d", "nodes": (step,), + "edges": (EdgeSpec(source=START, target=step, carries=text), + EdgeSpec(source=step, target=END, carries=text)), + "input_type": str, "output_type": str} + return type("D", (GraphSpec,), {**base, **attrs})() + + +listy = StepSpec("listy", inputs=(items,), outputs=(items,)) + + +def _listy(**attrs) -> GraphSpec: + """A design whose single step consumes and produces `items: list`, so only the declared + boundary varies between cases.""" + base = {"name": "l", "nodes": (listy,), + "edges": (EdgeSpec(source=START, target=listy, carries=items), + EdgeSpec(source=listy, target=END, carries=items))} + return type("L", (GraphSpec,), {**base, **attrs})() + + +def _port(finding: str) -> str: + """Which side a finding is about. Read out of the SENTENCE, because `about` deliberately does + not carry a port name — see `check_boundary_types`.""" + return "input_type" if "input_type" in finding else "output_type" + + +async def same(ctx) -> str: + return ctx.inputs + + +STRATEGY = StrategySpec("s", {step: same}) + + +# ── the two failures, one per side ─────────────────────────────────────────────────────────── + +def test_a_lying_input_type_is_a_blocking_finding() -> None: + findings = _design(input_type=int).coherence_check() + assert len(findings) == 1, findings + assert "input_type int" in findings[0] and "'text' (str)" in findings[0] + assert findings[0].check == "check_boundary_types" + assert findings[0].about == "", ( + "a boundary belongs to the design, not to a node — and `about` holds a node / edge / " + "strategy name or nothing. A port name would be a fifth kind of value in a field " + "test_every_finding_names_something_the_caller_can_look_up resolves.") + assert blocking(findings) + + +def test_a_lying_output_type_is_a_blocking_finding() -> None: + findings = _design(output_type=int).coherence_check() + assert len(findings) == 1, findings + assert "output_type int" in findings[0] and "'text' (str)" in findings[0] + assert blocking(findings) + + +def test_render_refuses_a_design_whose_boundary_is_wrong() -> None: + """A blocking finding must stop the build, not merely be reported. Otherwise the engine gets + a signature the design contradicts and the mismatch surfaces at the caller.""" + import pytest + + from workflow_workbench import SpecError + + with pytest.raises(SpecError): + _design(output_type=int).render(STRATEGY) + + +def test_the_default_boundary_is_a_claim_and_is_reported() -> None: + """`type(None)` is not an absence — `render()` hands it to `GraphBuilder` as the graph's real + signature. A design that never declares a boundary and then wires a `str` across it is making + a false claim, and there is no third state to tell it apart from a deliberate None design.""" + findings = _design(input_type=type(None), output_type=type(None)).coherence_check() + assert len(findings) == 2, findings + assert {_port(f) for f in findings} == {"input_type", "output_type"}, findings + + +# ── the direction of assignability differs per side, and both are legal shapes ──────────────── + +def test_the_input_may_be_WIDENED_on_the_way_in() -> None: + """The graph receives `input_type` and the edge carries it onward, so a wider carried + variable is correct. `examples/parallel.py` really does this: `input_type=list[int]` crossing + an edge that carries `numbers: list`.""" + assert _listy(input_type=list[int], output_type=list).coherence_check() == [] + + +def test_the_output_may_be_NARROWED_on_the_way_out() -> None: + """The edge delivers and the graph promises, so a narrower delivered type is correct. + `examples/ladder/stage10_no_basenode.py` really does this: `report: str` reaching an + `output_type=object`.""" + assert _design(output_type=object).coherence_check() == [] + + +def test_the_reverse_of_each_is_NOT_legal() -> None: + """⛔ The half that makes the two tests above mean something. A check that accepted both + directions on both sides would pass all four of these designs and prove nothing. + + ⚠️ `check_boundary_types` directly, not `coherence_check()`: narrowing on the way IN means + declaring a wider boundary than the edge carries, and the only way to write that without + also tripping `check_variables` is to isolate the rule under test.""" + narrowed_in = _listy(input_type=list, output_type=list) + narrowed_in.__class__.input_type = object # wider in than the edge carries + assert [_port(f) for f in check_boundary_types(narrowed_in)] == ["input_type"] + + widened_out = _design(output_type=object) + widened_out.__class__.output_type = int # narrower out than the edge delivers + assert [_port(f) for f in check_boundary_types(widened_out)] == ["output_type"] + + +# ── what actually arrives at END ───────────────────────────────────────────────────────────── + +def test_a_transform_edge_is_compared_on_what_it_DELIVERS() -> None: + """A `TransformEdgeSpec` reshapes ON THE WIRE, so the carried type is not what the caller + gets. Comparing `carries` here would flag the correct design and pass the wrong one.""" + def length(v: str) -> int: + return len(v) + + correct = _design(output_type=int, + edges=(EdgeSpec(source=START, target=step, carries=text), + TransformEdgeSpec(source=step, target=END, carries=text, + delivers=num, apply=length))) + assert correct.coherence_check() == [] + + wrong = _design(output_type=str, + edges=(EdgeSpec(source=START, target=step, carries=text), + TransformEdgeSpec(source=step, target=END, carries=text, + delivers=num, apply=length))) + assert [_port(f) for f in wrong.coherence_check()] == ["output_type"] + + +# ── undecidable is reported, never passed ──────────────────────────────────────────────────── + +def test_an_undecidable_pair_is_stated_and_does_not_block() -> None: + """`list[int]` against a declared `list[str]` cannot be settled by `issubclass`. A guess + either way is worse than saying so — and NOT CHECKED must not stop a render.""" + nums = VariableSpec("nums", list[int]) + through = StepSpec("through", inputs=(nums,), outputs=(nums,)) + d = type("P", (GraphSpec,), { + "name": "p", "nodes": (through,), "input_type": list[int], "output_type": list[str], + "edges": (EdgeSpec(source=START, target=through, carries=nums), + EdgeSpec(source=through, target=END, carries=nums))})() + findings = check_boundary_types(d) + assert len(findings) == 1 and findings[0].startswith(NOT_CHECKED), findings + assert not blocking(findings) + assert "list[str]" in findings[0] and "list[int]" in findings[0], ( + "a generic alias must print its parameter — `__name__` is 'list' for both, which would " + "render this finding as a complaint that list is not list") + + +def test_the_exact_repro_from_the_issue_on_a_REAL_example() -> None: + """⚠️ Not a synthetic `_design`. Every other case here is a fixture this module authored, and + a guard written beside the thing it guards inherits its author's assumptions — the repo's + recurring defect, 7 for 7 across #8, #18 and #19. So this one subclasses a shipped example + and reproduces #21's measurement verbatim: clean before, and it ran returning a `str`. + """ + from examples.greeting import Greeting, trim_only + + class Lying(Greeting): + name = "lying" + input_type, output_type = int, int + + findings = Lying().coherence_check() + assert len(findings) == 2, findings + assert all(f.check == "check_boundary_types" for f in findings) + assert blocking(findings) + + import pytest + + from workflow_workbench import SpecError + + with pytest.raises(SpecError): + Lying().render(trim_only) + + assert Greeting().coherence_check(trim_only) == [], ( + "the unmodified example must stay clean — otherwise this proves nothing about the lie") + + +# ── the consequence that made this worth fixing ────────────────────────────────────────────── + +def test_a_subgraph_can_no_longer_pass_on_a_claim_nothing_verified() -> None: + """⛔ THE ONE THAT MATTERS MOST. `check_subgraphs` compares a parent node's contract against + `child.output_type`. Measured before the fix: a child declaring `output_type=int` while every + edge in it carries `str` passed against a parent node declaring an int output, and the graph + returned `'HELLO'`.""" + inner = StepSpec("inner", inputs=(text,), outputs=(text,)) + + class Child(GraphSpec): + name = "child" + input_type, output_type = str, int # the lie + nodes = (inner,) + edges = (EdgeSpec(source=START, target=inner, carries=text), + EdgeSpec(source=inner, target=END, carries=text)) + + async def shout(ctx) -> str: + return ctx.inputs.upper() + + child_strategy = StrategySpec("child_s", {inner: shout}) + produce = StepSpec("produce", inputs=(text,), outputs=(num,)) + + class Parent(GraphSpec): + name = "parent" + input_type, output_type = str, int + nodes = (produce,) + edges = (EdgeSpec(source=START, target=produce, carries=text), + EdgeSpec(source=produce, target=END, carries=num)) + + parent_strategy = StrategySpec( + "parent_s", {produce: SubgraphBinding(graph=Child(), strategy=child_strategy)}) + + assert blocking(Child().coherence_check(child_strategy)), "the child's own lie must be caught" + assert blocking(Parent().coherence_check(parent_strategy)), ( + "the parent walks into the child, so the lie must surface there too") + + +# ── the broad one: calibration against the repo, not against this module ───────────────────── + +def _designs_in_examples() -> dict[str, type]: + import examples + + found: dict[str, type] = {} + for mod in pkgutil.walk_packages(examples.__path__, "examples."): + try: + m = importlib.import_module(mod.name) + except Exception: + continue + for obj in vars(m).values(): + if isinstance(obj, type) and issubclass(obj, GraphSpec) and obj is not GraphSpec: + found.setdefault(f"{obj.__module__}.{obj.__name__}", obj) + return found + + +def test_no_design_in_the_repo_is_flagged_or_silently_skipped() -> None: + """⚠️ THE CALIBRATION, and the reason this is not a check nobody reads. + + A new blocking rule that fires on the repo's own examples is a rule that gets routed around. + Measured when this landed: 28 boundary crossings across every example, 0 findings — and the + one that was undecidable (`parallel.py`'s `list[int]` into `numbers: list`) is decidable + because `_produces` compares a parameterised alias against its origin, not because the + example was edited to suit the check. + """ + designs = _designs_in_examples() + assert len(designs) >= 8, f"only found {len(designs)} designs — this would pass vacuously" + for qual, cls in sorted(designs.items()): + findings = check_boundary_types(cls()) + assert findings == [], f"{qual}: {findings}" diff --git a/workflow_workbench/__init__.py b/workflow_workbench/__init__.py index 4f99e9d..3d55071 100644 --- a/workflow_workbench/__init__.py +++ b/workflow_workbench/__init__.py @@ -17,6 +17,7 @@ NOT_CHECKED, CoherenceFinding, blocking, + check_boundary_types, check_bindings, check_fan_out_rejoins, check_recursion, @@ -61,6 +62,6 @@ "check_names", "check_reachable", "check_variables", "check_bindings", "check_implementations", "check_subgraphs", "check_step_arity", "check_decisions", "check_variable_types", "check_transform_edges", "check_fan_out_rejoins", - "check_recursion", + "check_recursion", "check_boundary_types", "diagram", "diff_diagram", ] diff --git a/workflow_workbench/checks.py b/workflow_workbench/checks.py index 1960693..8eb418c 100644 --- a/workflow_workbench/checks.py +++ b/workflow_workbench/checks.py @@ -33,7 +33,7 @@ __all__ = ["CoherenceFinding", "blocking", "NOT_CHECKED", "check_names", "check_reachable", "check_variables", "check_bindings", "check_implementations", "check_subgraphs", "check_step_arity", "check_decisions", - "check_variable_types", "check_transform_edges", + "check_variable_types", "check_transform_edges", "check_boundary_types", "check_fan_out_rejoins", "check_recursion"] @@ -172,9 +172,19 @@ def _about(spec: Any) -> str: def _type_name(t: Any) -> str: - """A stable name for a type in a finding. `__name__` misses generic aliases like - `list[Fact]`, which have none — and printing `` for one and a bare name for the - other makes two findings about the same mistake look like two different mistakes.""" + """A stable name for a type in a finding. Printing `` for one kind and a bare name + for another makes two findings about the same mistake look like two different mistakes. + + ⛔ Corrected: this said generic aliases "have none". Since 3.10 `list[Fact].__name__` is + `'list'` — so the bug was not a missing name, it was a WRONG one, and `__name__` alone + rendered `list[int]` and `list` identically. A finding comparing those two then read as a + complaint that `list` is not `list`. Measured on 3.13 while adding `check_boundary_types`, + whose message compares exactly that pair. + """ + import typing + + if typing.get_origin(t) is not None: + return repr(t) return getattr(t, "__name__", None) or repr(t) @@ -736,9 +746,14 @@ def _produces(annotation: Any, declared: Any) -> bool | None: """ import typing - if declared is object or annotation is declared: + if declared is object or annotation is declared or annotation == declared: return True # `object` accepts anything; identity is identity + # EQUALITY above, not just identity. `list[int] is list[int]` is False — each expression + # builds a new alias object — so identity alone reported "not decidable" for two spellings + # of literally the same type. Equality can only turn an undecidable into True, never into a + # finding, so it cannot manufacture a false alarm. + origin = typing.get_origin(annotation) if origin is typing.Union or type(annotation).__name__ == "UnionType": members = typing.get_args(annotation) @@ -750,7 +765,105 @@ def _produces(annotation: Any, declared: Any) -> bool | None: if isinstance(annotation, type) and isinstance(declared, type): return issubclass(annotation, declared) - return None # generic aliases, TypeVars, exotic forms + # A parameterised alias satisfies a BARE declared type: `list[int]` IS a `list`, which is + # `examples/parallel.py`'s real shape — `input_type=list[int]` crossing an edge that carries + # `numbers: list`. Only the ORIGIN is compared, so nothing is claimed about the parameter; + # `list[int]` against a declared `list[str]` stays undecidable, which is honest. + if origin is not None and isinstance(declared, type): + return issubclass(origin, declared) + + return None # TypeVars, parameterised-vs-parameterised, exotica + + +def check_boundary_types(parent: Any) -> list[CoherenceFinding]: + """The graph's declared `input_type` / `output_type` match what crosses START and END. + + ⛔ WHY THIS EXISTS, measured before it was written. These two fields go STRAIGHT to + `GraphBuilder`, and nothing compared them to the design they belong to: + + class Lying(Greeting): # every edge in Greeting carries str + input_type, output_type = int, int + + coherence_check() -> [] + run_sync(inputs=' a b ') -> 'Hello, a b!' <- a str + + ⚠️ And a dead field would have been the small version. `_port_type` compares a parent node's + contract against `child.input_type` / `child.output_type`, so `check_subgraphs` — a check + that DOES run, whose whole job is "the child fits the node" — was reading an oracle nothing + verified. Measured: a child declaring `output_type=int` while every edge in it carries `str` + passes against a parent node declaring an int output, and the graph returns `'HELLO'`. + + The defect is not that a rule was missing. The rule existed and was carefully worded; what + was missing is that **nobody checked the thing being compared against.** + + ⚠️ Assignability, not identity, and the direction differs per side: + + input the graph RECEIVES `input_type` and the edge carries it onward, so the carried + variable may be WIDER. `input_type=list[int]` into `numbers: list` is correct, + and `examples/parallel.py` really does that. + output the edge DELIVERS and the graph promises, so the delivered type may be + NARROWER. `report: str` reaching an `output_type=object` is correct, and + `examples/ladder/stage10_no_basenode.py` really does that. + + ⚠️ Reuses `_produces` rather than deciding assignability itself. A second, narrower copy of a + rule written beside the thing it guards is this repo's recurring defect — see the Copilot + findings on #8, #18 and #19 — and `_produces` already handles `object`, unions, `issubclass` + and the undecidable generic alias that would otherwise be a false alarm on `parallel.py`. + + ⚠️ The default is `type(None)`, and that is a CLAIM, not an absence. It reaches `GraphBuilder` + as the graph's real signature, so a design that never declares a boundary and then wires a + `str` across it is reported. There is no third state to tell apart from a deliberate + `None`-in / `None`-out design, and inventing one would be a declaration for something nobody + has needed. + + ⚠️ What arrives at END is `delivers` when an edge sets it, `carries` otherwise — a + `TransformEdgeSpec` reshapes ON THE WIRE, so the carried type is not what the caller gets. + + ⚠️ `about=""` — a finding about the WHOLE DESIGN, which is what a boundary declaration is. + `about="input_type"` was written first and reverted: `about` is documented to hold a node / + join / decision name, `source->target` for an edge, a strategy name, or `""`, and + `test_every_finding_names_something_the_caller_can_look_up` enforces exactly that set. A port + name is a FIFTH kind, and quietly adding one to a field other code resolves is a vocabulary + change — propose it, do not slip it in. The side is in the sentence, where a reader needs it. + """ + findings: list[CoherenceFinding] = [] + unchecked: list[str] = [] + + for side in ("input", "output"): + if side == "input": + declared = parent.input_type + crossing = [(e, e.carries) for e in parent.edges if isinstance(e.source, _Start)] + port = "input_type" + else: + declared = parent.output_type + crossing = [(e, getattr(e, "delivers", None) or e.carries) + for e in parent.edges if isinstance(e.target, _End)] + port = "output_type" + + for _edge, var in crossing: + # input: the declared type is handed to the edge, so IT must satisfy the variable. + # output: the edge hands its value back, so the VARIABLE must satisfy the declared. + verdict = (_produces(declared, var.type) if side == "input" + else _produces(var.type, declared)) + if verdict is None: + unchecked.append( + f"{port} {_type_name(declared)} vs {var.name}: {_type_name(var.type)}") + elif verdict is False: + findings.append(CoherenceFinding( + f"this design declares {port} {_type_name(declared)}, but the edge at " + f"{'START' if side == 'input' else 'END'} {'carries' if side == 'input' else 'delivers'} " + f"{var.name!r} ({_type_name(var.type)}). {port} is what reaches " + f"`GraphBuilder` and what a parent design is checked against, so one of the " + f"two is wrong — and until now neither was checked.", + check="check_boundary_types")) + + if unchecked: + findings.append(CoherenceFinding( + "NOT CHECKED — boundary types were not compared for: " + "; ".join(sorted(unchecked)) + + ". A generic alias has no class to test against, and guessing either way would " + "land a false alarm on correct code.", + check="check_boundary_types")) + return findings def check_variable_types(parent: Any, strategy: StrategySpec) -> list[CoherenceFinding]: diff --git a/workflow_workbench/graph_spec.py b/workflow_workbench/graph_spec.py index 81703cf..5d942dc 100644 --- a/workflow_workbench/graph_spec.py +++ b/workflow_workbench/graph_spec.py @@ -137,6 +137,7 @@ def _coherence_check(self, strategy: StrategySpec | None, findings += checks.check_reachable(declared, self.edges) findings += checks.check_transform_edges(self.edges, strategy) findings += checks.check_fan_out_rejoins(declared, self.edges) + findings += checks.check_boundary_types(self) if strategy is not None: findings += checks.check_bindings(self._bindables(), strategy) findings += checks.check_implementations(strategy) From 7a0d18b4cde12d93234bb25f5493d670078b8ffa Mon Sep 17 00:00:00 2001 From: Boris Dev Date: Tue, 6 Oct 2026 22:36:15 +0000 Subject: [PATCH 3/3] Copilot on #22: both findings real, and one was a RAISE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **1. `_produces` crashed on a typing wrapper, through `coherence_check()`.** Taken, and it is worse than the review said — it reported "returns False"; it actually raises. `get_origin` is not always a runtime class: `typing.Literal` for `Literal['ok']`, `typing.Annotated` for `Annotated[int, 'tag']`, and `issubclass` on either gives `TypeError: issubclass() arg 1 must be a class`. Measured end to end before the fix: async def decide(ctx) -> Literal["ok", "no"]: ... coherence_check(strategy) -> ⛔ RAISED TypeError `coherence_check()` is documented "Never raises", and a crash is not a conservative failure — every other finding in that sweep is lost with it. The alias branch now requires `isinstance(origin, type)`. Those wrappers stay UNDECIDABLE rather than being unwrapped: unwrapping `Annotated` is a real improvement that nothing has needed, and project.md says add the guard when you have the failing case. **2. The calibration test covered 14 of 15 designs and could have covered 0.** Taken, and this is the eighth instance of the pattern the test's own docstring is written against — a check narrower than its claim, in the check written to avoid that. Two independent holes: examples/local/ no __init__.py, so pkgutil.walk_packages never descended into it. examples.local.extraction.Extraction was never checked. except: continue a module that failed to import was silently dropped, so the sweep could go green having checked nothing. The `>= 8` floor let either pass. Discovery is now by FILE, recursive, with no try — a broken example is a failure. 15 designs found, all clean, and `test_discovery_reaches_every_example_FILE_not_every_example_package` names `examples.local.extraction` so the hole cannot reopen quietly. `.claude/rules/checks.md`: a missing input must never read as a pass. Regression tests added for `Literal` and `Annotated`, including that `coherence_check()` reports NOT CHECKED rather than raising. 296 tests green locally — no CI in this repo, so that is one local run. --- CHANGELOG.md | 6 +++ tests/test_boundary_types.py | 74 ++++++++++++++++++++++++++++++------ workflow_workbench/checks.py | 14 ++++++- 3 files changed, 82 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c8e370..a30fecc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,12 @@ a permanent `NOT CHECKED` line on `parallel.py`. `check_variable_types` reads the same helper and gains the same decidability. +⚠️ Only a **runtime class** origin is compared. `get_origin` is `typing.Literal` for +`Literal['ok']` and `typing.Annotated` for `Annotated[int, 'tag']`, and `issubclass` on either +raises — which the first cut of this did, through `coherence_check()`, a method documented +*"Never raises."* Those wrappers stay undecidable rather than being unwrapped; nothing has needed +unwrapping yet. + ### Fixed — `_type_name` rendered `list[int]` and `list` identically Its docstring said generic aliases have no `__name__`. Since 3.10 they do, and it is the bare diff --git a/tests/test_boundary_types.py b/tests/test_boundary_types.py index 9b04201..0bf6156 100644 --- a/tests/test_boundary_types.py +++ b/tests/test_boundary_types.py @@ -15,7 +15,7 @@ from __future__ import annotations import importlib -import pkgutil +from pathlib import Path from workflow_workbench import ( END, START, EdgeSpec, GraphSpec, StepSpec, StrategySpec, SubgraphBinding, @@ -242,32 +242,84 @@ class Parent(GraphSpec): # ── the broad one: calibration against the repo, not against this module ───────────────────── +EXAMPLES = Path(__file__).resolve().parent.parent / "examples" + + def _designs_in_examples() -> dict[str, type]: - import examples + """Every `GraphSpec` under `examples/`, discovered by FILE and imported without a net. + + ⛔ This was `pkgutil.walk_packages`, wrapped in `except Exception: continue`, and it was + wrong in both halves — caught by Copilot on #22: + examples/local/ has no `__init__.py`, so walk_packages never descended into it and + `examples.local.extraction.Extraction` was never checked + except: continue a module that failed to import was silently dropped, so the + calibration below could go green having checked nothing + + The `>= 8` floor let either omission pass. A missing input must never read as a pass — + `.claude/rules/checks.md` — so discovery is by path and an import error is an error. + """ found: dict[str, type] = {} - for mod in pkgutil.walk_packages(examples.__path__, "examples."): - try: - m = importlib.import_module(mod.name) - except Exception: + for path in sorted(EXAMPLES.rglob("*.py")): + if "__pycache__" in path.parts: continue + rel = path.relative_to(EXAMPLES.parent).with_suffix("") + name = ".".join(rel.parts) + m = importlib.import_module(name) # ⛔ no try — a broken example is a failure for obj in vars(m).values(): if isinstance(obj, type) and issubclass(obj, GraphSpec) and obj is not GraphSpec: found.setdefault(f"{obj.__module__}.{obj.__name__}", obj) return found +def test_discovery_reaches_every_example_FILE_not_every_example_package() -> None: + """⚠️ The guard on the guard. `test_no_design_in_the_repo_is_flagged` is only as broad as + this, and the first version of it quietly covered 14 of 15 designs.""" + modules = {q.rsplit(".", 1)[0] for q in _designs_in_examples()} + assert "examples.local.extraction" in modules, ( + "examples/local has no __init__.py — a package-based walk skips it entirely") + files = {p for p in EXAMPLES.rglob("*.py") if "__pycache__" not in p.parts} + assert len(files) >= 14, f"only {len(files)} example files — discovery is looking in the wrong place" + + def test_no_design_in_the_repo_is_flagged_or_silently_skipped() -> None: """⚠️ THE CALIBRATION, and the reason this is not a check nobody reads. A new blocking rule that fires on the repo's own examples is a rule that gets routed around. - Measured when this landed: 28 boundary crossings across every example, 0 findings — and the - one that was undecidable (`parallel.py`'s `list[int]` into `numbers: list`) is decidable - because `_produces` compares a parameterised alias against its origin, not because the - example was edited to suit the check. + Measured when this landed: every boundary crossing in every example, 0 findings — and the one + that was undecidable (`parallel.py`'s `list[int]` into `numbers: list`) is decidable because + `_produces` compares a parameterised alias against its origin, not because the example was + edited to suit the check. """ designs = _designs_in_examples() - assert len(designs) >= 8, f"only found {len(designs)} designs — this would pass vacuously" + assert len(designs) >= 15, f"only found {len(designs)} designs — this would pass vacuously" for qual, cls in sorted(designs.items()): findings = check_boundary_types(cls()) assert findings == [], f"{qual}: {findings}" + + +# ── the regression Copilot found, and it was a raise rather than a wrong answer ─────────────── + +def test_a_typing_wrapper_whose_origin_is_not_a_class_does_not_CRASH_the_check() -> None: + """⛔ `get_origin` does not always return a runtime class — it is `typing.Literal` for + `Literal['ok']` and `typing.Annotated` for `Annotated[int, 'tag']`, and `issubclass` on + either raises `TypeError: issubclass() arg 1 must be a class`. + + The first cut of the alias branch in `_produces` had no class guard, so a step annotated + `-> Literal['ok', 'no']` made `coherence_check()` RAISE — and that method is documented + "Never raises." A crash is not a conservative failure: every other finding in the sweep is + lost with it. + """ + from typing import Annotated, Literal + + from workflow_workbench.checks import _produces + + assert _produces(Literal["ok", "no"], str) is None + assert _produces(Annotated[int, "tag"], int) is None + + async def decide(ctx) -> Literal["ok", "no"]: + return "ok" + + findings = _design().coherence_check(StrategySpec("literal_arm", {step: decide})) + assert len(findings) == 1 and findings[0].startswith(NOT_CHECKED), findings + assert not blocking(findings), "undecidable must not stop a render" diff --git a/workflow_workbench/checks.py b/workflow_workbench/checks.py index 8eb418c..416dca5 100644 --- a/workflow_workbench/checks.py +++ b/workflow_workbench/checks.py @@ -769,7 +769,19 @@ def _produces(annotation: Any, declared: Any) -> bool | None: # `examples/parallel.py`'s real shape — `input_type=list[int]` crossing an edge that carries # `numbers: list`. Only the ORIGIN is compared, so nothing is claimed about the parameter; # `list[int]` against a declared `list[str]` stays undecidable, which is honest. - if origin is not None and isinstance(declared, type): + # + # ⛔ `isinstance(origin, type)` IS THE GUARD, and the first cut of this branch did not have + # it. `get_origin` does not always return a runtime class: it is `typing.Literal` for + # `Literal['ok']` and `typing.Annotated` for `Annotated[int, 'tag']`, and `issubclass` on + # either RAISES `TypeError: issubclass() arg 1 must be a class`. That reached + # `coherence_check()`, which is documented "Never raises" — so a step annotated + # `-> Literal['ok', 'no']` crashed the check rather than being reported. Caught by Copilot on + # #22; measured before and after. + # + # Those wrappers stay UNDECIDABLE rather than being normalized to their argument. Unwrapping + # `Annotated` is a real improvement and nothing has needed it — `.claude/rules/project.md`: + # add the guard when you have the failing case, not when you foresee one. + if isinstance(origin, type) and isinstance(declared, type): return issubclass(origin, declared) return None # TypeVars, parameterised-vs-parameterised, exotica