A routing typo waited for a run to fail, and a reused run id merged two of them - #86
Merged
Merged
Conversation
…wo of them Two declaration-time gaps, one in the kernel and one in the CLI, both of the same shape: something knowable before a run started was left for the run to discover, or not to. `add_conditional_edge` handed the router and its mapping to LangGraph untouched, so a mapping pointing at a node nobody added was accepted and the first branch to take it died on `self.ends[key]` — a bare `KeyError` from inside LangGraph's branch machinery, naming neither the graph, the source node nor the router. The mapping is topology and was checkable all along: an empty mapping is refused now, every unreachable target is named alongside the key that leads to it, and a router annotated with what it returns (a `Literal`, an `Enum`) has those members held against the mapping's keys with the same hash lookup LangGraph will use. A router that annotates nothing is still not second-guessed — but the key it returns is checked when it returns one, and raises `GraphRoutingError` naming the node, the key and the keys declared. The wrapper keeps the router's name and annotations: LangGraph branches by the one and infers the branch's input schema from the other. Every executing command appends to its `--trace`, which is right — `diff` reads two runs out of one file — but nothing checked whether the `--run-id` the operator passed was already in there. Two runs then merged under one name, and `metrics` summed both runs' tokens, `viz` welded the second path onto the first, `replay` reconstructed a chimera, with no signal at any point. The appendable file was never the defect; the reused id was. `plan`, `run` and `agent` (both executors) refuse an explicit `--run-id` that already has events in the target trace, exit 2, before a single event is written. Fail closed rather than auto-rename: the id is the name an operator looks the run up under later. Generated ids pay for no scan, and different ids in one file are untouched. Fixes #4 Fixes #29 Co-Authored-By: Claude Fable 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Two declaration-time gaps of the same shape — something knowable before a run started was left for the run to discover, or not to. Different files (runtime kernel vs CLI), one commit.
Fixes #4
Fixes #29
#4 — a conditional edge's mapping is checked where it is declared
add_conditional_edgepassed the router and its mapping to LangGraph untouched. A mapping target naming a node nobody added was accepted, an empty mapping was accepted, and the first branch to take one died onself.ends[key]— a bareKeyErrorfrom inside LangGraph's_branch.py, naming neither the graph, the source node, nor the router that produced the key.What is checked now, at
add_conditional_edge:END. All offending pairs are named at once ('go' -> 'sipn'), alongside the valid destinations.Literal[...]or anEnumreturn annotation — has those members held against the mapping's keys. The membership test is the same hash lookup LangGraph will perform, so the check predicts the run-time failure rather than approximating it (this matters forEnum, which hashes by member name whileStrEnumcompares by value).Beyond the issue's letter, and asked for in the task: (4) is no longer a bare
KeyError. The router is wrapped so an unmapped key raisesGraphRoutingErrornaming the node, the key and the keys that were declared — the error the kernel already raises for every other transition it cannot make. The wrapper usesfunctools.wrapsand a*args/**kwargssignature deliberately: LangGraph names the branch after the callable's__name__and infers the branch's input schema from its annotations, and it passesconfigto a router that asks for one. Anasync defrouter gets an async wrapper, or LangGraph would await the check's return value rather than the router's. ASendreturned by a router still passes through to LangGraph's own dispatch untouched, as does a list of keys.Error shape and ordering are unchanged elsewhere:
dag=Truestill refuses a conditional edge before the mapping is looked at, so the cookbook'sGraphCycleErrorexample is byte-identical.Before
After
#29 — a reused
--run-idis refused, exit 2Every executing command appends to its
--trace(TraceRecorderopens"a"), which is right —grapharc diffreads two runs out of one file. Nothing checked whether the id the operator passed was already in that file, so two runs merged under one name:metricssummed both runs' tokens and node counts,vizwelded the second path onto the end of the first,replayreconstructed a chimera, with no signal at any point.The issue left the remedy shape open on one axis, so, as directed: fail closed rather than auto-renaming or warning. A run id is the name an operator will look the run up under later; silently picking a different one is the same class of surprise as silently merging, and a warning does not stop the file from being corrupted.
plan,runandagent(both the sandboxed and theclaude-cliexecutor) refuse an explicit--run-idthat already has events in the target trace, with exit 2 naming the id, the event count and the file, before a single event is written — ahead ofrun's admission check, which writes the first event, so--check-onlyis guarded too.Out of scope per the issue and untouched: repairing already-interleaved files, and the multi-run-per-file pattern
diffdepends on.demohas no--run-idflag (it deriveslive-<example>), so nothing to guard there.grapharc/cli/runid.py: it reads one field per line rather than validating whole event models, and skips a line it cannot parse. The guard's job is to spot a collision, not to be the file's validator — the readers still report a torn trace in their own words.--run-id's help text on all three commands now says it is refused if--tracealready holds it.Before
After
A different id in the same file, and a generated id, both still exit 0.
Tests
Each new test was confirmed red before its fix (by stashing the source change and re-running):
tests/test_runtime_discipline.py— 11 new: unknown target (single and several at once), empty mapping,LiteralandEnumdeclarations checked, a correct mapping and an undeclared router unaffected, an unmapped return raisingGraphRoutingError, an unmapped key inside a returned list, and the branch name surviving the wrapper. 7 fail without the fix.tests/test_async_kernel.py— 2 new: anasync defrouter's unmapped return, and one that maps.tests/test_cli.py— 7 new:planrefused on reuse (text and JSON),runrefused before the admission event,agentrefused before the model is built, different ids in one file still supported, generated ids never guarded, and a unit test of the scan over a file with a torn line. 4 fail without the fix.Full suite green,
ruff check grapharc testsclean.Docs
The README paragraph naming this limitation ("a typo surfaces as a
KeyErrorat run time") and both cookbook sentences indocs/cookbook/01-basics.mdnow describe what is checked and what still is not.CHANGELOG.mdgets an entry per issue under the existing## Unreleased.🤖 Generated with Claude Code