Skip to content

Commit 533001f

Browse files
Admission refuses a sentinel pointing the wrong way (#109)
`START` is the graph's entry and `END` its exit, but `_check_endpoints` accepted both in either role — the sentinel test did not look at which side of the edge it was on. So a proposal carrying `END -> x` or `x -> START` was admitted, and then died in `Materializer` with `StateGraph`'s own "END cannot be a start node" / "START cannot be an end node". The run does not proceed either way. What was wrong is where the failure was charged and what the planner was told. `GovernedLoop` counts a `MaterializationError` as an execution failure, against `max_consecutive_execution_failures` (2), rather than as a rejection against `max_consecutive_rejections` (3) — so a planner got fewer retries for a mistake admission is supposed to catch than for one it does catch, and two in a row ended the run as `EXECUTION_FAILED`, a stop reason claiming the graph ran when nothing had. And a rejection is meant to be data. `feedback()` hands the planner codes and remedies; what it got here was prose assembled from an exception, with no code, no remedy, and nothing on the `admission` event's failed-check list — because admission had not failed. The prompt in proposal.py already tells models the rule; the gate is what did not hold when a model ignored it. Both endpoints are still reported rather than the first, matching every other check. The rejection rides `Check.REGISTRY` with code `sentinel_wrong_direction` and a remedy naming the side the sentinel belongs on. Two of the three new tests go red without the fix; the third is the guard that the normal shape still admits, which must stay green either way. Closes #108 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 00e0c7d commit 533001f

3 files changed

Lines changed: 112 additions & 1 deletion

File tree

docs/deep-dive.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -254,7 +254,7 @@ A stable system is not one that claims to have no edges — it is one whose edge
254254
- **`.env` and `grapharc.toml` follow the same discovery rule: the working directory, and nowhere else.** Neither searches parent directories — a run must not be governed by a file you did not know about, and must not be *billed* to one either. **This is a behaviour change:** the credential loader used to walk up to `/`, so a `.env` in an ancestor directory (a `$HOME` one on a shared box, a client project one above a demo checkout) was picked up silently. If you relied on that, move the file into the directory you run from, `export` the variable, or pass `env_file=` to name it explicitly. A real environment variable still beats any file.
255255
- **`grapharc run` has no budget unless you give it one.** Set any of `--max-tokens`, `--max-iterations`, `--max-seconds`, or `--max-concurrency`; without them each dimension is unlimited and the gate admits a topology of any worst-case cost.
256256

257-
**Verified this pass:** `pytest` → green, 2,145 selected and 13 deselected (the live ones); `ruff check .` clean; all eight `grapharc demo` stages green, plus the `trace` / `metrics` / `viz` / `replay` tour against a freshly recorded demo trace; the wheel builds and imports all submodules in a clean virtualenv with `[all]`, and `0.1.6` on PyPI is that wheel. The counts are a snapshot, not a property of the project — `pytest` re-derives them in one command, which is the only reason they are quoted, and `tests/test_deep_dive.py` fails this line rather than letting it drift.
257+
**Verified this pass:** `pytest` → green, 2,148 selected and 13 deselected (the live ones); `ruff check .` clean; all eight `grapharc demo` stages green, plus the `trace` / `metrics` / `viz` / `replay` tour against a freshly recorded demo trace; the wheel builds and imports all submodules in a clean virtualenv with `[all]`, and `0.1.6` on PyPI is that wheel. The counts are a snapshot, not a property of the project — `pytest` re-derives them in one command, which is the only reason they are quoted, and `tests/test_deep_dive.py` fails this line rather than letting it drift.
258258

259259
[ROADMAP.md](../ROADMAP.md) tracks what is built and what is not, item by item.
260260

grapharc/planner/admission.py

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -672,8 +672,50 @@ def _check_registry(self, proposal: Subgraph) -> list[Rejection]:
672672
def _check_endpoints(
673673
self, path: str, edge: ProposedEdge, names: frozenset[str]
674674
) -> list[Rejection]:
675+
"""Both endpoints must name something, and the sentinels must face the
676+
right way.
677+
678+
The sentinels are directional and the kernel enforces it: `START` is the
679+
graph's entry, so it can only be a source, and `END` is its exit, so it
680+
can only be a target. Accepting them in either role let a proposal
681+
carrying `END -> x` or `x -> START` through admission and into
682+
materialisation, where `StateGraph` raises "END cannot be a start node"
683+
/ "START cannot be an end node" — a shape defect surfacing as a
684+
`MaterializationError`.
685+
686+
That is the wrong failure in two ways. It is charged to
687+
`max_consecutive_execution_failures` (2) rather than
688+
`max_consecutive_rejections` (3), so a planner gets fewer tries at a
689+
mistake admission is supposed to catch. And the planner is handed prose
690+
— "The subgraph you proposed did not run: could not be built: ..." —
691+
instead of a `Rejection` with a code and a remedy, which is the whole
692+
contract of `feedback()`: a rejection is data the next round can act on.
693+
"""
675694
out: list[Rejection] = []
676695
for role, endpoint in (("source", edge.source), ("target", edge.target)):
696+
wrong_way = (endpoint == END and role == "source") or (
697+
endpoint == START and role == "target"
698+
)
699+
if wrong_way:
700+
other = END if endpoint == START else START
701+
out.append(
702+
Rejection(
703+
check=Check.REGISTRY,
704+
code="sentinel_wrong_direction",
705+
subject=_scoped(path, edge.render()),
706+
detail=(
707+
f"{endpoint!r} is the graph's "
708+
f"{'entry' if endpoint == START else 'exit'}, so it cannot "
709+
f"be an edge's {role}"
710+
),
711+
remedy=(
712+
f"use {endpoint!r} as the edge's "
713+
f"{'source' if endpoint == START else 'target'}, "
714+
f"or {other!r} here"
715+
),
716+
)
717+
)
718+
continue
677719
if endpoint in _SENTINELS or endpoint in names or endpoint in self.known_nodes:
678720
continue
679721
out.append(

tests/test_admission.py

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,75 @@ def test_an_edge_to_a_node_that_does_not_exist_is_rejected():
161161
assert "target" in reason.detail
162162

163163

164+
def test_end_cannot_be_an_edge_source_and_start_cannot_be_a_target():
165+
"""The sentinels are directional, and the gate has to say so.
166+
167+
`START` is the graph's entry and `END` its exit. Both used to be accepted
168+
in either role, so a proposal carrying `END -> x` or `x -> START` was
169+
admitted and then failed in `Materializer` with `StateGraph`'s own
170+
"END cannot be a start node" — a shape defect surfacing as a
171+
`MaterializationError` rather than a rejection. That is charged to the
172+
execution-failure allowance rather than the rejection allowance, and it
173+
reaches the planner as prose instead of a code and a remedy.
174+
"""
175+
backwards_end = Subgraph(
176+
nodes=(ProposedNode(name="fetch"),),
177+
edges=(
178+
ProposedEdge(source=START, target="fetch"),
179+
ProposedEdge(source=END, target="fetch"),
180+
),
181+
)
182+
result = checker(registry("fetch")).check(backwards_end)
183+
184+
assert not result.admitted
185+
(reason,) = result.reasons(Check.REGISTRY)
186+
assert reason.code == "sentinel_wrong_direction"
187+
assert "source" in reason.detail
188+
assert reason.remedy
189+
190+
backwards_start = Subgraph(
191+
nodes=(ProposedNode(name="fetch"),),
192+
edges=(
193+
ProposedEdge(source=START, target="fetch"),
194+
ProposedEdge(source="fetch", target=START),
195+
),
196+
)
197+
result = checker(registry("fetch")).check(backwards_start)
198+
199+
assert not result.admitted
200+
(reason,) = result.reasons(Check.REGISTRY)
201+
assert reason.code == "sentinel_wrong_direction"
202+
assert "target" in reason.detail
203+
204+
205+
def test_the_sentinels_still_work_the_way_round_they_are_meant_to():
206+
"""A guard on the guard: the fix must not refuse the normal shape."""
207+
result = checker(
208+
registry("fetch"), limits=AdmissionLimits(require_entry=True)
209+
).check(linear("fetch"))
210+
211+
assert result.admitted, result.rejections
212+
213+
214+
def test_a_refused_sentinel_edge_never_reaches_materialisation():
215+
"""The point of catching it here: `Materializer` raises on these, and the
216+
loop counts that against a different, smaller allowance."""
217+
proposal = Subgraph(
218+
nodes=(ProposedNode(name="fetch"),),
219+
edges=(
220+
ProposedEdge(source=START, target="fetch"),
221+
ProposedEdge(source=END, target="fetch"),
222+
),
223+
)
224+
gate = checker(registry("fetch"))
225+
result = gate.check(proposal)
226+
227+
assert not result.admitted
228+
# `_explode` is every registered kind's factory, so anything that built the
229+
# graph anyway would raise AssertionError rather than fail this quietly.
230+
assert result.failed_checks() == (Check.REGISTRY,)
231+
232+
164233
def test_an_edge_may_reference_a_node_already_in_the_graph():
165234
proposal = Subgraph(
166235
nodes=(ProposedNode(name="fetch"),),

0 commit comments

Comments
 (0)