You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
check() returns CoherenceFinding, so an agent can branch instead of regex (0.3.0) - #5
check() returned list[str], so the structure a caller needs was encoded in prose and
recovered with startswith("NOT CHECKED") — load-bearing control flow in three call sites here
and in both downstream repos.
CoherenceFinding is a str subclass, which is the load-bearing decision:
field
value
check
the producing function's name, e.g. "check_variables"
about
a node/join/decision name; "source->target" for an edge; the strategy name; "" for a whole-design finding
blocking
derived — False only for a NOT CHECKED — … stated gap
Plus blocking(findings), the filter render() / eval_battle / the devserver now share instead
of each spelling the prefix match out.
Additive — measured, not asserted
221 existing tests pass untouched, and 14 new ones bring it to 235.
Every finding the test designs produce — 64 of them — is byte-for-byte identical to origin/main.__repr__ is deliberately not overridden; the examples print whole lists of findings.
scripts/prove_workflow_workbench.py → ALL CHECKS PASSED
libs/case_build/tests/test_battle.py → 15 passed
The new tests, and why they are shaped this way
The 221 are an oracle for the messages. They say nothing about check and about, which is
what #4 asked for.
A structural oracle: every about is "" or a name resolvable against the design the caller handed in. Not a hand-written table of 27 expected strings — that is as likely to be wrong as the code it checks.
about follows the subject of the sentence, not the loop variable: an undeclared node cannot be looked up, so that finding is about the edge; a branch missing when= is about the branch, not the decision.
The compatibility oracle — the five string operations that must not move.
A source-scan ratchet, so an append site on a branch nothing provokes cannot ship returning a plain str.
All nine were mutation-tested: eight deliberate defects introduced one at a time, each caught
by exactly the assertion meant to catch it.
⚠️ One correction to the issue's premise, found by running it
The 221 tests assert finding message substrings, so a wrong message fails loudly.
True of some messages, not others. Rewording the tail of check_reachable's "unreachable from
START" finding leaves the whole suite green — the test asserts only "orphan" in f and "unreachable" in f. The byte-for-byte diff is what actually covers that claim;
the suite alone does not.
Propagation
Neither consumer picks this up automatically — both pin 016e0ab2 in uv.lock. Bumped to 0.3.0 (new public API, backward compatible). No coordinated migration: they bump on their own
schedule with uv lock --upgrade-package workflow-workbench, or not at all.
…egex (0.3.0)
Closes#4.
`check()` returned `list[str]`. The findings are sentences, so the structure a
caller needs was encoded in the prose:
hard = [f for f in findings if not f.startswith("NOT CHECKED")]
That prefix match was load-bearing control flow in three call sites here and in
both downstream repos — two kinds of finding wearing one type, told apart by
text. `.claude/rules/checks.md`: NOT CHECKED and 0 FOUND must never render the
same.
`CoherenceFinding` is a `str` SUBCLASS, which is the load-bearing decision:
check the producing function's name, e.g. "check_variables"
about a node/join/decision name; "source->target" for an edge; the
strategy name; "" for a finding about the whole design
blocking derived — False only for a `NOT CHECKED — …` stated gap
A bool, not a severity enum: two states, no third observed. A frozen dataclass
is tidier and costs a second breaking migration one release after StepSpec;
that is why it loses.
ADDITIVE, and measured rather than asserted:
· all 221 existing tests pass untouched
· every finding the test designs produce — 64 of them — is byte-for-byte
identical to origin/main. `__repr__` is deliberately NOT overridden,
because the examples print whole lists of findings
· the five downstream lines #4's comment names as the acceptance bar were
RUN, unmodified, against this build: nobsmed-v2's
scripts/prove_workflow_workbench.py (ALL CHECKS PASSED) and
libs/case_build/tests/test_battle.py (15 passed)
27 findings.append sites in checks.py tagged; message text unchanged at every
one. `GraphSpec._check`'s recursion finding is the 28th and the only producer
outside checks.py — it carries that qualified name because cycle detection
needs the `ancestry` only that method holds.
New tests, because the 221 are an oracle for the MESSAGES and say nothing about
`check` and `about`:
· a structural oracle — every `about` is "" or a name resolvable against the
design the caller handed in. Not a hand-written table of 27 expected
strings, which is as likely to be wrong as the code it checks
· `about` follows the SUBJECT, not the loop variable: an undeclared node
cannot be looked up, so that finding is about the EDGE; a branch missing
`when=` is about the branch, not the decision
· the compatibility oracle — the five string operations that must not move
· a source-scan ratchet, so an append site on a branch nothing provokes
cannot ship returning a plain `str`
All nine were mutation-tested: eight deliberate defects introduced one at a
time, each caught by exactly the assertion meant to catch it.
⚠️ One correction to #4's premise, found by running it. "The 221 tests assert
finding message substrings, so a wrong message fails loudly" is true of some
messages and not others — rewording the tail of check_reachable's "unreachable
from START" finding keeps the whole suite green, because the test asserts only
`"orphan" in f and "unreachable" in f`. The byte-for-byte diff above is the
check that actually covers the claim; the suite alone does not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
did not say what it checks, and the type it returns says — a
word that appeared NOWHERE else in the API. One concept wearing two names is the
thing the naming rule exists to prevent, and the orphan was the half I added.
spec.check(strategy) 0.2.0
spec.coherence_check(strategy) 0.3.0
No alias: a missed call site is an AttributeError at the call. Same choice 0.2.0
made when NodeSpec(...) became a TypeError.
was considered and rejected — IS the design, so
restates its own receiver.
UNCHANGED, deliberately: the eleven functions and .
Both name an individual check, which is what they still are.
103 replacements across 25 files, applied by counted exact-match rather than a blind
sed — every in the repo was first enumerated and confirmed to be this method.
⚠️ was deliberately REVERTED after the sweep touched it. It
documents what the API was AT 0.2.0, where the method genuinely was .
Renaming it there would make a historical upgrade guide assert something false — a
doc that lies is worse than no doc.
Done inside this PR rather than after it, so 0.3.0 carries ONE migration instead of
0.3.0 and 0.4.0 carrying one each.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follow-up to 0863362, whose commit message is truncated: it was passed through a
double-quoted shell string and every `backticked` term was eaten as command
substitution. The rationale it should have carried is below. No code in that commit
was affected — only its message.
WHY THE RENAME. `check()` did not say what it checks, and the type it returns says
`Coherence` — a word that appeared NOWHERE else in the API. One concept wearing two
names is what the naming rule exists to prevent, and the orphan half was the one I
added in this PR. `design_check()` was considered and rejected: `spec` IS the design,
so `spec.design_check()` restates its own receiver. There is no alias — a missed call
site is an `AttributeError` at the call, the same choice 0.2.0 made when
`NodeSpec(...)` became a `TypeError`.
Unchanged, deliberately: the eleven `check_*` functions and `CoherenceFinding.check`.
Both name an individual check, which is what they still are.
THE THREE LOOSE ENDS, and none was found by reading the diff:
1. The README's four `spec.coherence_check(...)` samples were MISSED by the sweep.
Its regex required no `.` before `check()`, which excluded exactly the form the
README uses. Caught by `test_the_docs_use_the_current_api` — the lint that exists
because the README's own opening example stopped running once before and nothing
said so. It went red on the first full run after the rename.
2. `docs/migration-0.3.md` cites the 0.2.0 precedent by name, so it contains
`NodeSpec(` and that same lint flags it. Added to `_NAMES_THE_OLD_API` rather than
reworded around: a migration doc necessarily names the API it migrates FROM, and
`test_the_retirement_exemptions_all_exist` keeps the exemption from going stale.
3. `docs/migration-0.2.md` was REVERTED after the sweep touched it (in 0863362). It
documents what the API was AT 0.2.0, where the method genuinely was `check()`.
Renaming it there would make a historical upgrade guide assert something false.
`CHANGELOG.md` keeps three bare `check()` mentions on purpose — they are the breaking
change narrating itself, and one sample is explicitly labelled `# 0.2.0`.
235 tests pass. `parity --check` green, `examples.greeting` and
`examples.ladder.stage9_decision` both run.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Extra binding finding uses an unresolvable subject
workflow_workbench/checks.py:290
For an extra binding, n is explicitly not declared by this design, so using its name as about violates the new invariant that about is a handle the caller can resolve against the supplied design/strategy. Tag this finding with the strategy name instead; the existing stranger test exercises this branch but never checks its metadata.
Child findings lose their parent subgraph location
workflow_workbench/checks.py:441
Forwarding child findings unchanged loses the subgraph location. A parent check can now return about="child_node", although that name is absent from the parent design, and two child bindings may use the same internal name; an agent therefore cannot resolve which subgraph the finding belongs to. Preserve both the parent binding path and the child's local subject (or explicitly retag it) and add a failing-child test.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
The breaking API rename contradicts the stated compatibility requirement, and structured metadata has unresolved typing and nested-subgraph contract issues.
blocking is documented as derived and unable to drift from blocking(), but storing it in a public slot makes f.blocking = False legal while blocking([f]) still recomputes True from the message. Make it a read-only computed property so both APIs always return the same verdict.
Child findings violate resolvable about invariant
workflow_workbench/checks.py:441
Propagating child findings unchanged violates the new about invariant that the value is resolvable against the design passed by the caller. For example, the existing incomplete-child fixture produces about="second" from Parent().coherence_check(...), but Parent only exposes the binding node transform; with multiple child bindings the caller cannot even identify which child owns that name. Retag these findings to the parent binding or add explicit design/ancestry metadata and cover nested findings in the structural test.
Fix test documentation to reference _coherence_check
tests/test_subgraph.py:605
This new test documentation refers to the removed _check method, while the implementation and assertion below use _coherence_check. Update the method name so the ownership explanation points to the actual code.
… docstrings
The README said what the library is FOR and never what it checks. A reader could not
tell whether `coherence_check()` was three trivial rules or eleven real ones, and the
data language — the whole reason the design is inspectable — was never written down.
TWO GENERATED TABLES, in the README where they were asked for:
the data language every word you declare a design with, grouped
well-formedness every rule coherence_check() enforces, split by whether it
needs a strategy
python3 -m workflow_workbench.reference --write
python3 -m workflow_workbench.reference --check
NO DESCRIPTION IS AUTHORED. Every cell is the first line of the thing's own docstring,
or — for the two unions, which have none of their own — the names of their members.
A table cannot claim what the code does not say.
What IS authored is the GROUPING: which word sits under "boxes" and which under
"wires". That is curation, it is source, and it is a literal in `reference.py`. The
source/derived split is the point, so there is a test asserting GROUPS carries names
only.
THE README OPENING NOW NAMES THE FAILURE, NOT THE AUDIENCE. Boris's framing: an agent
left to run free produces work that RUNS AND IS INCOHERENT — each piece locally fine,
the whole not adding up. That is a reason to care; "for developing workflows with an
AI coding agent" was only a statement of who it is for. Four problems, one declaration
answering all four: incoherence, strategies that can only be compared and not asserted,
reuse, and visibility.
⚠️ The visualization claim is narrowed to what is true. pydantic-graph DOES emit
mermaid — `build_mermaid_graph` in `graph_builder.py` — so "they have none" would have
been false. Measured: it is not exported from their `__init__`, and it takes a BUILT
graph's internals, so every implementation must exist first. Two honest differences
then, not a feature list: ours reads the declaration, and ours can draw two strategies
at once.
Rebased onto the coherence-finding branch, because these tables name the method and
that branch renames it. #6 is stacked on #5.
ALSO FIXED AT SOURCE, not worked around:
· `_Start`'s first docstring line ended mid-sentence on "so `mypy` can narrow a", and
`_End`'s was "See `_Start`." Both are now standalone sentences, because the table
lifts that line verbatim. `test_every_first_docstring_line_stands_alone` makes a
wrapping summary a test failure rather than a fragment in the README.
· the union rows needed their pipes ESCAPED — a bare `|` is a column separator, so
`StepSpec | JoinSpec | DecisionSpec` rendered as three empty cells. Invisible in
the source string; the test asserts the rendered row.
· the generated prose said "`NodeSpec(...)` is a `TypeError`", which
`test_the_docs_use_the_current_api` greps for and flags. Reworded at the source
rather than exempting the README — that lint exists because the README's own
example once stopped running and nothing said so, and the README is the last file
that should be exempt from it. It has now caught me twice in this change, both
times correctly.
`docs/well-formedness.md` is deleted: the tables live in the README now, and two homes
for one generated table is the duplication this repo keeps a drift test to avoid.
Six ways this can rot were introduced one at a time, and each went red: hand-editing a
README row, a docstring changing without regeneration, a new export left out of the
table, a stale exclusion naming something no longer exported, GROUPS growing a
hand-written description, and the unions losing their pipe escaping.
245 tests pass. `parity --check` and `reference --check` both green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…— and five rounds of review (#5#6#7#8)
Contains #5, #6 and #7 — the stack is linear, so this one merge brings all four.
- #5 `coherence_check()` returns `CoherenceFinding`, so an agent can branch instead of regex
- #6 both reference tables in the README, generated from the package's own docstrings
- #7 the glossary, linked from the prose, with a test that kills dead links
- #8 the thesis, `examples/contestable.py`, `StepSpec.problem`, the implement-a-workflow skill
Merged as a MERGE COMMIT, not a squash, so the eight commits stay in history.
## Five rounds of Copilot review, worked
20 inline comments were on record unread when consolidation started. Across five rounds:
14 live / 2 stale on round one, 5 on round two (three of them my own regression), pickle on
round three, a stale docstring on round four, and two previously-missed on round five.
⚠️ **Every single finding was a CHECK narrower than its claim, not code that was wrong.** The
dead-link regex skipped malformed links; the "every rule" table enumerated 11 of 12; the retired
-token map had no `.check(`; the compatibility oracle's five string operations omitted pickle;
the rule-count guard scanned one phrasing in one file; a UI test drove one of three render
surfaces. The code was mostly right. The things asserting it was right were the defects.
266 -> 278 tests. Both generators green. Bundle reproduces byte-for-byte from `npm run build`.
## One finding deliberately open
Nested findings propagate an `about` relative to the child design, so it is not resolvable by a
caller holding only the parent. Both readings are right about different things, and reconciling
them means `about` carrying a path across a boundary — a semantic change to the field. Tracked in
#9 with the limit documented in-code at both sites rather than left overstating.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4.
What changed
check()returnedlist[str], so the structure a caller needs was encoded in prose andrecovered with
startswith("NOT CHECKED")— load-bearing control flow in three call sites hereand in both downstream repos.
CoherenceFindingis astrsubclass, which is the load-bearing decision:check"check_variables"aboutname;"source->target"for an edge; the strategyname;""for a whole-design findingblockingFalseonly for aNOT CHECKED — …stated gapPlus
blocking(findings), the filterrender()/eval_battle/ the devserver now share insteadof each spelling the prefix match out.
Additive — measured, not asserted
origin/main.__repr__is deliberately not overridden; the examples print whole lists of findings.scripts/prove_workflow_workbench.py→ALL CHECKS PASSEDlibs/case_build/tests/test_battle.py→15 passedThe new tests, and why they are shaped this way
The 221 are an oracle for the messages. They say nothing about
checkandabout, which iswhat #4 asked for.
aboutis""or a name resolvable against the design the caller handed in. Not a hand-written table of 27 expected strings — that is as likely to be wrong as the code it checks.aboutfollows the subject of the sentence, not the loop variable: an undeclared node cannot be looked up, so that finding is about the edge; a branch missingwhen=is about the branch, not the decision.str.All nine were mutation-tested: eight deliberate defects introduced one at a time, each caught
by exactly the assertion meant to catch it.
True of some messages, not others. Rewording the tail of
check_reachable's "unreachable fromSTART" finding leaves the whole suite green — the test asserts only
"orphan" in f and "unreachable" in f. The byte-for-byte diff is what actually covers that claim;the suite alone does not.
Propagation
Neither consumer picks this up automatically — both pin
016e0ab2inuv.lock. Bumped to0.3.0 (new public API, backward compatible). No coordinated migration: they bump on their own
schedule with
uv lock --upgrade-package workflow-workbench, or not at all.🤖 Generated with Claude Code