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
The thesis, a contestable example, and a skill — greeting becomes the foil - #8
Contains #5, #6 and #7 (based on main, so this diff is everything). PLACEHOLDER noted below.
The problem with our own examples
You said: an agent left to run free produces work that runs and is incoherent, and a node should be swappable hairy stuff, not trivial fixed logic.
Our examples contradicted that. trim vs trim_and_collapse is a question with a right answer you could look up — so the README taught the mechanism on a case that proves the mechanism unnecessary.
Greeting stays, and says so
⚠️And you would never need this library for this. Nothing in the greeting example is contestable… It is here because the whole mechanism fits in sixty seconds at this size, not because it earns its keep.
One paragraph, keeps a tested walkthrough, and a reader already thinking it stops arguing the moment we say it first.
examples/contestable.py — the shape that does earn it
Four stages each a judgement call, two strategies differing in one, and — because a design is data — a diagram, a coherence check and a diff with nothing implemented. The example is cheap to produce for the same reason the library is useful.
⛔ It prints no score, and a test asserts it never will. A number out of stubs would be a fabricated result inside the library built to catch those.
StepSpec.problem
Your item 5. Named problem, not description:
description invites "normalizes the name" — restates name, tells an implementer nothing. problem can't be filled that way without the emptiness showing.
The problem, never the solution."Two reasonable rankings can disagree" belongs there; "sort by score descending" is one arm's answer, and putting it in the design makes every other arm wrong by definition.
problem_resolved rejected — the node doesn't resolve anything.
Its consumer is the browser payload, so it isn't a field nothing reads.
⚠️ Empty means nobody wrote one, never "this stage is easy."
.claude/skills/implement-a-workflow/
Your item 1 — a skill rather than a CLAUDE.md, so repo users get it, not just contributors. Teaches the division of labour, and its first rule is "do not edit the declaration to make a check pass."
⚠️ The skill is now covered by test_the_docs_use_the_current_api, and it's the file where a retired API matters most: a skill is prose an agent acts on, so a dead method there isn't a confused human — it's generated code that won't run. The lint caught me on first write.
README
The thesis as the frame — making AI-agent development with an SWE agent interpretable — the division-of-labour table, and a "when not to use this" section.
Decisions
Made: no rename. graph-layout would have been actively wrong (layout = visual placement; in EDA, the physical mask). workflow stays.
Open, deliberately not built: a rubric field, and advisory findings for near-duplicate names. Both need a third blocking state, which contradicts #4's "a bool, two states, no third observed." A third has now been observed — your call.
PLACEHOLDER
The example's domain is a stand-in; the shape is the point and the nouns are marked for swapping.
…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>
… 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>
The README has to stay short, and this change keeps adding words that carry real
meaning a reader cannot guess — coherence, well-formedness, stated gap, noise floor,
declaration layer. Explaining each inline wears the reader down; dropping them loses
the knowledge. So: define them once, link them on first use.
16 terms. Each is three lines at most, and the third is the one worth reading — what
the word does NOT mean here:
coherence the parts CONNECT. Not that they support a conclusion — that is
only measurable, which is what a battle is for
consistency weaker than coherence, and the gap is the whole problem. An
inconsistency usually crashes; an incoherence RUNS
design rule chk chip design's name for this activity. ERC's floating net,
dangling output and two-drivers-on-one-net map onto three of our
rules almost exactly. LVS has no equivalent here, deliberately
soundness van der Aalst's workflow nets. Our checks are STRUCTURAL
approximations of it, not a proof — a smaller claim, stated
deep embedding why the design is data, and why every capability follows from it
well-formedness UML's own section heading. NOT an "invariant" in the strict sense
⛔ THE TEST IS THE POINT. A glossary with dead links is worse than none: a reader
clicks a term they do not know, lands nowhere, and learns not to click the next one.
The mechanism dies quietly, and nothing else in this repo would notice — a markdown
link is not code.
Five tests, and all six ways this rots were introduced one at a time and each went
red: renaming a term so README links break, breaking an internal cross-reference,
the README quietly dropping a load-bearing link, the glossary truncated to a stub,
two headings collapsing to one anchor, and the file deleted outright.
The anchor test replicates GitHub's slug algorithm rather than trusting that headings
and links happen to agree.
⚠️ `test_the_readme_actually_links_the_terms_it_leans_on` is the unusual one, and it
guards the failure this whole idea invites: the glossary exists, nobody links to it,
and the knowledge is parked where no reader will find it. Five terms are required by
name. Dropping one is allowed — dropping it on purpose, by editing that list.
250 tests pass. Both generators green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Audited by diffing README.md against origin/main and reading every REMOVED line,
rather than trusting that a rewrite kept what mattered. Two were real losses, not
rewordings:
1. "Specify the workflow and its data contracts, inspect its diagram, then have the
agent implement the steps." The development-sequence table still covers this, but
the one-line version is what tells a reader the VERB ORDER before they commit to
reading a table. Back, as "What you do:".
2. "The specification keeps the workflow understandable." Dropped entirely, and it is
the half of Boris's framing about agent output being hard to READ — a disservice to
holding the thing in your memory, separate from it being wrong.
Restored as a claim with a mechanism instead of an adjective: the declaration's size
is set by the SHAPE of the workflow, not by the complexity of the steps. A step body
can grow to 500 lines; its StepSpec stays one. Measured across this repo's examples:
11 to 43 declaration lines, whatever is bound into them.
⚠️ Stated as absolute lines, not as a ratio. The ratio over these files is 13-32%,
but the files are mostly `__main__` demo printing, so the percentage measures the
demos rather than the claim. The bound is the real property.
250 tests pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… foil
Boris: an agent left to run free produces work that RUNS AND IS INCOHERENT, and a
node should be swappable HAIRY stuff, not trivial fixed logic. Our own examples
contradicted that — `trim` vs `trim_and_collapse` is a question with a right answer
you could look up, so the README taught the mechanism on a case proving the mechanism
unnecessary.
GREETING STAYS, AND SAYS SO. Turning the weakness into the credibility move costs one
paragraph and keeps a tested working walkthrough: "you would never need this library
for this". A reader already thinking it stops arguing the moment we say it first.
NEW: examples/contestable.py — the shape that does earn it. Four stages that are each
a judgement call, two strategies differing in exactly ONE, and — because a design is
data — a diagram, a coherence check and a diff with NOTHING IMPLEMENTED. That is the
pitch demonstrating itself: the example is cheap to produce for the same reason the
library is useful.
⛔ IT PRINTS NO SCORE, and there is a test asserting it never does. A number out of
stubs would be a fabricated result inside the library built to catch those. The
example says so in prose; `test_the_example_never_prints_a_score` makes it true in
fact and goes red the day someone wires a battle to stubs.
NEW: `StepSpec.problem` — the colocated brief for whoever implements the role.
· named `problem`, NOT `description`. A field called `description` invites
"normalizes the name", which restates `name` and tells an implementer nothing. A
field called `problem` cannot be filled that way without the emptiness showing.
· the PROBLEM, never the solution. "Two reasonable rankings can disagree" belongs
there; "sort by score descending" is one arm's answer, and writing it in the
design makes every other arm wrong by definition.
· `problem_resolved` was rejected — the node does not resolve anything, an
implementation attempts it.
· its consumer is the browser payload, so it is not a field nothing reads. ⚠️ Empty
means NOBODY WROTE ONE, never "this stage is easy" — checks.md's NOT CHECKED vs
0 FOUND in another costume, and there is a test for it.
NEW: `.claude/skills/implement-a-workflow/` — Boris's idea, a skill rather than a
CLAUDE.md, so repo USERS get it and not just contributors. It teaches the division of
labour: the human owns the declaration, the agent owns the bodies, coherence_check()
is the contract. Its first rule is "do not edit the declaration to make a check pass".
⚠️ And the skill is now covered by `test_the_docs_use_the_current_api`. That is the
file where a retired API matters MOST: a skill is prose an AGENT reads and acts on, so
a dead method named there is not a confused human, it is generated code that will not
run. Caught by the lint on first write.
README: the thesis as the frame ("making AI-agent development with an SWE agent
interpretable"), the division-of-labour table, and a "when NOT to use this" section —
if your stages are deterministic and you would never swap one, use Pydantic Graph
directly.
⚠️ PLACEHOLDER: the example's domain is a stand-in. The SHAPE is the point and the
nouns are marked for swapping.
DECIDED THIS ROUND: no repo rename (`graph-layout` would have been actively wrong —
layout means visual placement, and in EDA the physical mask). `workflow` stays.
STILL OPEN, deliberately not built: a `rubric` field, and advisory/non-blocking
findings for near-duplicate names. Both need a third `blocking` state, which
contradicts issue #4's "a bool, two states, no third observed" — a third has now been
observed, and that is Boris's call to make.
Five ways this rots were introduced one at a time, each went red: the stub example
printing a score, both arms binding the same stage so nothing varies, a contested
stage losing its brief, `problem` no longer reaching the payload, and the SKILL
naming a retired API.
259 tests pass. Both generators green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Boris, designing the React Flow exploration: "i want to see which nodes hide
complexity ... click and see full subgraph ... like google maps". First step of that.
⛔ IT IS NOT A NODE TYPE, and that is the whole design. "Hides a subgraph" is a
property of the BINDING: the same role is a function in one arm and a child design in
another, and comparing those two — is decomposing this stage actually better — is one
of the most interesting battles the library can run. A `StrategyStepSpec` would make
that undeclarable. `test_a_callable_arm_and_a_subgraph_arm_are_one_design` is the
existing test that would have had to go.
So it is derived render state, and it costs nothing on the declaration side.
TWO INDEPENDENT CHANNELS, which is the existing convention in diagram.py:
shape what the node IS join [/…/] decision {{…}} composed [[…]]
colour what is AT STAKE :::varies :::shared
A node can be composed in both arms and not vary, or vary between a function and a
child graph. One channel could not carry both.
· `[[…]]` is mermaid's subroutine shape — the right one for a node whose
implementation is itself a design
· `diff_diagram` judges composed-ness on EITHER arm. Judging on arm `a` alone would
drop the shape for exactly the comparison worth looking at
· `payload.Binding.subgraph` is an EXPLICIT field, never inferred from `impl`
containing "::". A viewer that string-matches the label is one rename away from
silently losing every drill-down
FRONTEND: a `⤵ subgraph` badge and a distinct outline, derived from the payload field.
⛔ AND THE PANEL NOW SAYS "no per-stage result". Scores are per STRATEGY, end to end —
nothing measures one stage alone. A panel that shows a stage's code and silently shows
no number invites the reader to supply one, and an empty space is the most convincing
0 there is. (Per-stage scores would need node rubrics, which is still Boris's open
decision.)
⚠️ `workflow_workbench/static/workflow-workbench.js` IS A BUILT ARTIFACT COMMITTED TO
THE REPO. Editing `nodes.tsx` changes nothing until `npm run build` regenerates it, so
a test reading the .tsx — or grepping the served HTML — would pass against a bundle
that never learned any of this. The bundle is rebuilt here, and the new assertions run
in real Chromium against the DOM React Flow produced. Proven, not assumed: restoring
the PRE-rebuild bundle turns both of them red.
Mutations, each introduced alone:
devserver stops setting the flag RED
diff_diagram judges composed-ness on arm A only RED
a composed node loses its distinct shape RED
the stale bundle is restored RED
the viewer parses `impl` for "::" instead of the field RED
⚠️ That last one was a FALSE PASS at first and the fix is in the fixture. My initial
mutation also dropped the arm-B check, so it went red for the previous reason rather
than for label-parsing — and a true label-parsing viewer would have PASSED, because
the fixture's `impl` was `literature::thorough`. The composed fixture's label is now
`thorough_child`, containing no "::", which is the only way that substitution is
detectable at all.
266 tests pass; both generators green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The four PRs carried 20 inline Copilot comments and none had been read — the
review was on record before #8's last push and the turn closed out over it.
`.claude/rules/code-review.md` exists because of exactly that, so: all 20
verified against this tip, since GitHub re-anchors stale comments onto
plausible-looking lines.
TAKEN (14)
Four were the repo's own signature failure — a check that passed because it did
not touch what breaks:
- `test_glossary.py`'s link fragment was `#([a-z0-9-]+)`, so every MALFORMED
link was SKIPPED rather than reported. `#noise_floor`, `#Noise-Floor` and a
bare `glossary.md#` — the only shapes a human actually types wrong — were
invisible to the test whose entire job is killing dead links. Both regexes
widened to `[^)]*`; mutation-tested, and each was silent before.
- the README's rules table claims to be every rule and enumerated 11 of 12. The
recursive-subgraph rule was produced in `graph_spec.py`, outside
`checks.__all__`, so neither the generator nor the completeness test could see
it. The RULE moves to `checks.check_recursion`; the CALL SITE does not, which
keeps the single-owner argument intact (a cycle must stop the walk, so the
control flow stays in `GraphSpec`). Plus a guard that goes red on the NEXT
one: no finding may be constructed outside `checks.py`.
- `retired` in `test_parity.py` had `NodeSpec(` and no `.check(`, so the lint
that is supposed to stop a SKILL naming a dead method covered the 0.2.0 rename
and not the 0.3.0 one that shipped in the same release.
- `test_the_example_never_prints_a_score` matched only decimals starting `0.`,
so a fabricated 1.0 — the most flattering number a stub can print — passed the
one test carrying that honesty property.
Two were real defects:
- `problem` was inserted BEFORE `streams` as a positional field, so
`StepSpec("x", (), (), True)` silently assigned `True` to `problem` and left
streaming off. No type error anywhere. Now keyword-only, with the regression
test.
- `problem` reached the payload and stopped. App.tsx dropped it building
StageData and nothing rendered it, so the field justified as "its consumer is
the browser payload" was read by nobody while the payload test stayed green. It
renders in the Panel now, non-empty only, with two REAL browser tests —
reverting the render goes red.
And: `check_bindings` tagged a foreign binding's `about` with a name the checked
design does not declare, violating the invariant; `blocking()` was annotated
`-> list[str]` and erased `CoherenceFinding` on the way out of the public filter
meant to make its fields reachable; the 0.3.0 changelog said "Upgrading: nothing
to do" above a breaking rename; the glossary claimed battles are impossible over
a shallow embedding when `compare_graphs()` exists precisely to battle built
graphs; the tag oracle proved only that a tag named SOME check, so a copy/paste
passed it; a comment claimed the 221 tests are a byte-for-byte message oracle,
which the PR body's own counterexample disproves; `examples/parallel.py` printed
the removed name in its label; and the quickstart's hand-written rule count had
already gone stale against the generated table, so that now has a test too.
DECLINED, with the reason
- nested findings propagate with `about` relative to the CHILD, so it is not
resolvable by a caller holding only the parent. Copilot is right, and so is the
code comment defending the precision. Reconciling them means `about` carrying a
PATH across a boundary — a semantic change to the field, which lands with the
nested-graph rename. The overstated invariant is narrowed at both sites instead
of left claiming more than it checks.
STALE (2): `workflow_workbench/rules.py` and `docs/well-formedness.md` no longer
exist.
⚠️ `blocking()`'s generic return has NO automated oracle — no Python type checker
is configured in this repo. Stated rather than counted as covered.
266 -> 271 tests, both generators green, bundle rebuilt from source.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot's review was never read — 20 inline comments across #5–#8
Worked now. All 20 re-verified against f84816f rather than trusted, since GitHub re-anchors stale comments onto plausible-looking lines.
14 live, 2 stale, 1 declined with a reason. Full triage is in the commit message. The four worth naming here are all the same failure this repo has a rule about — a check that passed because it did not touch what breaks:
the check
what it was blind to
test_glossary.py dead-link test
its fragment class was #([a-z0-9-]+), so #noise_floor, #Noise-Floor and a bare glossary.md# were skipped, not reported — the only shapes a human types wrong
the README's "every rule" table
enumerated 11 of 12; the recursive-subgraph rule was produced outside checks.__all__, where neither the generator nor the completeness test could see it
retired in test_parity.py
had NodeSpec( and no .check( — so the lint guarding a SKILL against a dead method covered the 0.2.0 rename and not the 0.3.0 one shipped in the same release
"the example never prints a score"
matched only decimals starting 0., so a fabricated 1.0 sailed through the one test carrying that honesty property
Two real defects: problem was positional beforestreams, so StepSpec("x", (), (), True) silently set problem=True and left streaming off — now keyword-only. And problem reached the payload and stopped: App.tsx dropped it and nothing rendered it, so the field justified here as "its consumer is the browser payload" was read by nobody while the payload test stayed green. It renders in the Panel now, non-empty only, with two real browser tests.
The rules table is 12 now, and there is a guard that goes red on the next off-book rule rather than a patch for the last one.
Declined, with the reason: nested findings propagate with about relative to the child, so it is not resolvable by a caller holding only the parent. Copilot is right, and so is the code comment defending the precision. Reconciling them means about carrying a path across a boundary — a semantic change to the field, which lands with the nested-graph rename. The overstated invariant is narrowed at both sites rather than left claiming more than it checks.
⚠️blocking()'s generic return has no automated oracle — no Python type checker is configured in this repo. Stated rather than counted as covered.
266 → 271 tests, both generators green, bundle rebuilt from source. Review on record predates this commit; a fresh one is requested.
TAKEN, all five.
MINE (3 sites, and the guard I shipped in the same commit missed every one)
Moving the recursive-subgraph rule into `checks.py` took the count 11 -> 12 and left stale
claims in the README overview, the CHANGELOG and `docs/migration-0.3.md` (twice).
`test_the_hand_written_count...` was supposed to catch exactly this and did not, because it
scanned ONE phrasing in ONE file:
`11 [well-formedness rules](url)` a markdown link sits between number and noun
"the eleven `check_*` functions" backticks sit inside the noun
So it is now every prose file, digits AND number-words, with links and backticks normalised
away first — and all four spellings are mutation-tested, each one RED. Deliberately NOT keyed on
a bare "rules": `docs/ladder.md` says "eleven rungs" and that is a different eleven.
Where a count in prose added nothing beside a generated table, it is DELETED rather than
maintained in two places. `docs/migration-0.3.md` now names `check_recursion` as new instead of
listing it under "unchanged", which it was not.
⚠️ The first version of the broadened test also had a bug of its own — one capture group, and
`for n, in findall(...)` unpacking a string. Found by running it, which is the only reason the
backtick gap turned up at all: the first mutation run reported CHANGELOG and migration-0.3 as
GREEN, i.e. not caught.
NEW (2, both on code this PR touched)
- `CoherenceFinding.blocking` was documented as derived and guaranteed to agree with
`blocking()`, and was a WRITABLE slot: `finding.blocking = False` made the field disagree with
the helper on the same message. Two readings of one fact is what this type exists to remove, so
it is a read-only property now, derived through the same `_is_blocking`. Test asserts the
assignment raises AND that the two readings still agree.
- `payload.Binding` accepted `subgraph=True` beside `unbound=True` or `skipped=True`, so the
viewer could draw a composed badge on a stage nobody wired or that this arm declined to run —
opposite claims. The validator rejects both combinations. `.claude/rules/case-build.md` §3: a
check that was skipped must never render as a value.
271 -> 273 tests, both generators green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Restrict coherence checks to declared design bindables
workflow_workbench/checks.py:358
coherence_check() passes every strategy entry here, including bindings that check_bindings has already identified as foreign. If such an entry is also non-callable (or has bad arity), this emits about=<foreign node>, which is not resolvable against the design and contradicts the new about contract. Filter implementation checks to the design's declared bindables and let check_bindings exclusively report extras.
…e did not look
ONE comment, and it falsifies this PR's headline claim.
`CoherenceFinding` is sold as additive — "~30 string call sites here and in two downstream repos
need no edit". `str`'s inherited reducer rebuilds a subclass as `cls(message)`, and `check` is
keyword-only and REQUIRED, so:
pickle TypeError: CoherenceFinding.__new__() missing 1 required keyword-only argument
copy same
deepcopy same
A plain string did all three fine one release ago. That breaks process-pool and cache transport
for any consumer that moves findings between processes.
⚠️ **The defect is the ORACLE, not the missing method.** `test_a_finding_is_still_a_string_
everywhere_it_was_one` is explicitly "THE COMPATIBILITY ORACLE" and it enumerates five string
operations. Pickle, copy and deepcopy were not among the five, so the suite stayed green through
a real regression in exactly the property the test exists to defend. A list of what must not move
is only as good as the list — `.claude/rules/checks.md`.
Fixed with an explicit `__reduce__` and a module-level rebuild callable. The new test round-trips
all three, asserts type, byte-for-byte equality, both fields, AND that the derived `blocking`
survives — plus a whole LIST through pickle, which is how a process pool would actually move
them. Mutation-tested: removing `__reduce__` goes red.
273 -> 274 tests, both generators green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
It combines a breaking public API migration with extensive checker, documentation-generation, example, and frontend changes requiring final human review.
Update docstring reference to renamed _coherence_check method
workflow_workbench/checks.py:445
The docstring below still says GraphSpec._check owns cycle detection, but this PR renames that method to _coherence_check. Update the reference so the check's documentation points to the method that now exists.
⚠️ **And I nearly reported this round as clean.** My poll watched
`/pulls/8/comments` — the INLINE comments — and the review BODY carried the findings. Zero
inline, three findings. Third incomplete check in this sequence and the second one mine: a
query that answers "nothing found" when it was looking in one of two places.
TAKEN (1)
`checks.py` said "`GraphSpec._check` owns that" for cycle detection. The method became
`_coherence_check` in 0.3.0 AND the rule moved to `check_recursion` in this branch, so the
sentence was wrong twice. Both corrected.
`test_the_docs_use_the_current_api` could not see it: it guards PUBLIC tokens in markdown, and a
backticked `Class.attr` inside a Python docstring is invisible to it. So there is now a guard that
resolves every `` `Exported.attr` `` reference in the package's own docstrings against the real
attribute. Narrow on purpose — that is the shape that has actually gone stale. Mutation-tested.
`CHANGELOG.md:121` also names `GraphSpec._check` and is LEFT ALONE: it sits inside the 0.2.0
section describing what changed at that time, when the method really was `_check`. A changelog is
a historical record, and "fixing" it would make it a lie about the past.
ALREADY DONE, flagged as open (1)
"Node problem is not propagated or rendered in the UI" links the original comment. The substance
is fixed in f84816f — it renders in the Panel, non-empty only, with two browser tests and a
mutation that goes red. What is NOT done is the literal suggestion, threading it through
`StageData` onto the canvas node, and that is deliberate: the Panel is the selected-stage detail
view and the brief is for whoever implements that stage. A second surface for one value is the
duplication this repo keeps drift tests to avoid.
DEFERRED to #9 (1)
"Nested findings lose design ancestry and lookup context" — still correct, still the semantic
change to `about` that belongs with the nested-graph rename. The overstated invariant is already
narrowed at both sites rather than left claiming more than it checks.
274 -> 275 tests, both generators green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Direct GraphSpec page fails to render node problem briefs
workflow_workbench/devserver.py:83
The direct GraphSpec browser still drops this field. build_app() serves the inline PAGE, whose renderer reads node IDs and bindings but never n.problem (devserver.py:220-267). The new browser test uses a hand-authored report through the separate report server, while the producer test stops at schema validation, so devserver.serve(...) cannot display the brief. Route this page through the shared island or render non-empty briefs here and cover the real spec_payload → page path.
This issue also appears on line 121 of the same file.
--write reports success after partial table update failure
workflow_workbench/reference.py:221
When either marker pair is missing, the loop sets rc = 1 and skips that block, but --write still prints that both tables were rewritten. This produces a contradictory success message for a partial write. Only print it when rc == 0 (or report exactly which blocks were updated).
…lied on a partial
Both findings are "previously missed" — in code this branch had already touched — and the first
is my own fix reported as done while covering a third of the paths.
`problem` reaches THREE renderers' payloads and rendered on ONE:
frontend/.../Panel.tsx the React island FIXED in f84816f
workflow_workbench/report.py the no-JS fallback dropped it
workflow_workbench/devserver.py the direct GraphSpec page dropped it
And the test could not have noticed: it drove a hand-authored report through the REPORT server,
so it never crossed the real `spec_payload` -> page path at all. Both renderers now print a
non-empty brief only, both mutation-tested RED.
⚠️ The devserver test needs a REAL SERVER, not `set_content`: `PAGE` is a shell that fetches its
payload from `/spec/<name>/data`, so pasting the HTML into a blank page renders nothing and would
have passed either way. That is the whole reason this gap survived.
Second: `reference.py --write` printed "both tables rewritten from reference.py" even when a
marker pair was missing and that block had been skipped with rc=1 — a success line over a file
that is now part new and part stale. It names what it actually wrote now ("PARTIAL WRITE — 1 of 2
tables rewritten (rules)") and the test drives the real failure by removing a marker.
`.claude/rules/checks.md`: degraded is not a pass.
⚠️ And the pattern worth naming, because it is now five for five: every finding in this review
sequence has been a CHECK that was narrower than its claim, not code that was wrong. The dead-link
regex, the "every rule" table, the retired-token map, the compatibility oracle's five operations,
the rule-count guard, and now a UI test that drove one of three surfaces. The code was mostly
right; the things asserting it was right were the defects.
STILL OPEN, tracked in #9: nested findings propagate an `about` relative to the child, so it is
not resolvable by a caller holding only the parent. The limit is documented at both sites.
275 -> 278 tests, both generators green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The skill’s exhaustive-looking description omits strategy-level findings. check_recursion and the extra-binding branch of check_bindings set about to the strategy name, so an agent instructed to branch on this field can mistake that value for a node id.
Narrow the claim about rules preventing execution failures
README.md:215
This universal claim is false for rules in the generated table: check_names documents that Pydantic Graph refuses duplicate ids, and check_recursion prevents stack exhaustion rather than a plausible result. Narrow the statement to say that the rules catch structural defects before execution and that the relevant docstrings include measured runnable cases.
Document strategy-level findings in the description
README.md:349
This description omits strategy-level findings. check_recursion and the extra-binding branch of check_bindings both set about=strategy.name (checks.py:157,354), so consumers following this example may incorrectly treat every non-edge value as a node id. Document the strategy-name case here too.
Document the strategy-name form of about
docs/migration-0.3.md:59
This migration guide omits the strategy-name form of about, even though check_recursion and extra-binding findings use it. Consumers migrating specifically to branch on structured findings need that case documented to avoid resolving a strategy name as a node.
…ot tell
Both findings are mine, both are the shape this branch is about, and the first is the worst kind
— a confidently wrong doc added in the commit that added a test to protect it.
## 1. The table said `their_hello.py` contains `step_a`/`step_b`. It does not.
That file is ALREADY a greeting adaptation of upstream: its steps are `pick` (returns `"Hello"`)
and `compose` (returns `f"{ctx.inputs}, {ctx.state.name}!"`). `step_a`/`step_b` appear only in its
docstring, describing the upstream program it was adapted FROM. So the row I added pointed at a
file and described something else.
Three accurate rows now, and they make the point better than two wrong ones did:
upstream, unchanged their visualize_graph.py step_a -> 10
the control their_hello.py pick -> "Hello" no workbench
ours greeting.py normalize -> clean declared
⚠️ **`test_the_readme_shows_the_pydantic_graph_lineage` passed the whole time**, because it
asserted the WORDS were present, not that they were TRUE. That is the same defect as every
finding on #8, now committed by me in the test written to prevent it. It reads the control's
source and requires the row to name that file's real steps, and refuses `step_a` in that row
specifically.
## 2. "All 12 rules" sat OUTSIDE the generated markers
So `reference --write` could not update it, and `test_no_prose_anywhere_states_a_rule_count...`
matched "well-formedness rules" and "check functions" and not "12 rules" — a thirteenth check
would have left the summary stale with every test green.
The numeral is gone; the count lives only in the generated block, which is where it is derived.
The regex covers a bare `rules` now, so re-introducing one goes red rather than passing quietly.
Both mutation-tested: restoring the false table -> RED, putting a stale numeral back -> RED.
280 tests, both generators green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Contains #5, #6 and #7 (based on
main, so this diff is everything). PLACEHOLDER noted below.The problem with our own examples
You said: an agent left to run free produces work that runs and is incoherent, and a node should be swappable hairy stuff, not trivial fixed logic.
Our examples contradicted that.
trimvstrim_and_collapseis a question with a right answer you could look up — so the README taught the mechanism on a case that proves the mechanism unnecessary.Greeting stays, and says so
One paragraph, keeps a tested walkthrough, and a reader already thinking it stops arguing the moment we say it first.
examples/contestable.py— the shape that does earn itFour stages each a judgement call, two strategies differing in one, and — because a design is data — a diagram, a coherence check and a diff with nothing implemented. The example is cheap to produce for the same reason the library is useful.
⛔ It prints no score, and a test asserts it never will. A number out of stubs would be a fabricated result inside the library built to catch those.
StepSpec.problemYour item 5. Named
problem, notdescription:descriptioninvites "normalizes the name" — restatesname, tells an implementer nothing.problemcan't be filled that way without the emptiness showing.problem_resolvedrejected — the node doesn't resolve anything..claude/skills/implement-a-workflow/Your item 1 — a skill rather than a
CLAUDE.md, so repo users get it, not just contributors. Teaches the division of labour, and its first rule is "do not edit the declaration to make a check pass."test_the_docs_use_the_current_api, and it's the file where a retired API matters most: a skill is prose an agent acts on, so a dead method there isn't a confused human — it's generated code that won't run. The lint caught me on first write.README
The thesis as the frame — making AI-agent development with an SWE agent interpretable — the division-of-labour table, and a "when not to use this" section.
Decisions
Made: no rename.
graph-layoutwould have been actively wrong (layout = visual placement; in EDA, the physical mask).workflowstays.Open, deliberately not built: a
rubricfield, and advisory findings for near-duplicate names. Both need a thirdblockingstate, which contradicts #4's "a bool, two states, no third observed." A third has now been observed — your call.PLACEHOLDER
The example's domain is a stand-in; the shape is the point and the nouns are marked for swapping.
Mutation-tested
problemstops reaching the payload259 tests pass. Both generators green.
🤖 Generated with Claude Code