From ee664521eb251925d045b3d55f83796a7e629f4d Mon Sep 17 00:00:00 2001 From: Shashank Shekhar Singh Date: Fri, 14 Aug 2026 00:32:14 +0530 Subject: [PATCH] Admission refuses a sentinel pointing the wrong way MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- docs/deep-dive.md | 2 +- grapharc/planner/admission.py | 42 +++++++++++++++++++++ tests/test_admission.py | 69 +++++++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 1 deletion(-) diff --git a/docs/deep-dive.md b/docs/deep-dive.md index b767264..1c93ae9 100644 --- a/docs/deep-dive.md +++ b/docs/deep-dive.md @@ -254,7 +254,7 @@ A stable system is not one that claims to have no edges — it is one whose edge - **`.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. - **`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. -**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. +**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. [ROADMAP.md](../ROADMAP.md) tracks what is built and what is not, item by item. diff --git a/grapharc/planner/admission.py b/grapharc/planner/admission.py index 1becb1d..75f15b5 100644 --- a/grapharc/planner/admission.py +++ b/grapharc/planner/admission.py @@ -672,8 +672,50 @@ def _check_registry(self, proposal: Subgraph) -> list[Rejection]: def _check_endpoints( self, path: str, edge: ProposedEdge, names: frozenset[str] ) -> list[Rejection]: + """Both endpoints must name something, and the sentinels must face the + right way. + + The sentinels are directional and the kernel enforces it: `START` is the + graph's entry, so it can only be a source, and `END` is its exit, so it + can only be a target. Accepting them in either role let a proposal + carrying `END -> x` or `x -> START` through admission and into + materialisation, where `StateGraph` raises "END cannot be a start node" + / "START cannot be an end node" — a shape defect surfacing as a + `MaterializationError`. + + That is the wrong failure in two ways. It is charged to + `max_consecutive_execution_failures` (2) rather than + `max_consecutive_rejections` (3), so a planner gets fewer tries at a + mistake admission is supposed to catch. And the planner is handed prose + — "The subgraph you proposed did not run: could not be built: ..." — + instead of a `Rejection` with a code and a remedy, which is the whole + contract of `feedback()`: a rejection is data the next round can act on. + """ out: list[Rejection] = [] for role, endpoint in (("source", edge.source), ("target", edge.target)): + wrong_way = (endpoint == END and role == "source") or ( + endpoint == START and role == "target" + ) + if wrong_way: + other = END if endpoint == START else START + out.append( + Rejection( + check=Check.REGISTRY, + code="sentinel_wrong_direction", + subject=_scoped(path, edge.render()), + detail=( + f"{endpoint!r} is the graph's " + f"{'entry' if endpoint == START else 'exit'}, so it cannot " + f"be an edge's {role}" + ), + remedy=( + f"use {endpoint!r} as the edge's " + f"{'source' if endpoint == START else 'target'}, " + f"or {other!r} here" + ), + ) + ) + continue if endpoint in _SENTINELS or endpoint in names or endpoint in self.known_nodes: continue out.append( diff --git a/tests/test_admission.py b/tests/test_admission.py index 5a02372..271feeb 100644 --- a/tests/test_admission.py +++ b/tests/test_admission.py @@ -161,6 +161,75 @@ def test_an_edge_to_a_node_that_does_not_exist_is_rejected(): assert "target" in reason.detail +def test_end_cannot_be_an_edge_source_and_start_cannot_be_a_target(): + """The sentinels are directional, and the gate has to say so. + + `START` is the graph's entry and `END` its exit. Both used to be accepted + in either role, so a proposal carrying `END -> x` or `x -> START` was + admitted and then failed in `Materializer` with `StateGraph`'s own + "END cannot be a start node" — a shape defect surfacing as a + `MaterializationError` rather than a rejection. That is charged to the + execution-failure allowance rather than the rejection allowance, and it + reaches the planner as prose instead of a code and a remedy. + """ + backwards_end = Subgraph( + nodes=(ProposedNode(name="fetch"),), + edges=( + ProposedEdge(source=START, target="fetch"), + ProposedEdge(source=END, target="fetch"), + ), + ) + result = checker(registry("fetch")).check(backwards_end) + + assert not result.admitted + (reason,) = result.reasons(Check.REGISTRY) + assert reason.code == "sentinel_wrong_direction" + assert "source" in reason.detail + assert reason.remedy + + backwards_start = Subgraph( + nodes=(ProposedNode(name="fetch"),), + edges=( + ProposedEdge(source=START, target="fetch"), + ProposedEdge(source="fetch", target=START), + ), + ) + result = checker(registry("fetch")).check(backwards_start) + + assert not result.admitted + (reason,) = result.reasons(Check.REGISTRY) + assert reason.code == "sentinel_wrong_direction" + assert "target" in reason.detail + + +def test_the_sentinels_still_work_the_way_round_they_are_meant_to(): + """A guard on the guard: the fix must not refuse the normal shape.""" + result = checker( + registry("fetch"), limits=AdmissionLimits(require_entry=True) + ).check(linear("fetch")) + + assert result.admitted, result.rejections + + +def test_a_refused_sentinel_edge_never_reaches_materialisation(): + """The point of catching it here: `Materializer` raises on these, and the + loop counts that against a different, smaller allowance.""" + proposal = Subgraph( + nodes=(ProposedNode(name="fetch"),), + edges=( + ProposedEdge(source=START, target="fetch"), + ProposedEdge(source=END, target="fetch"), + ), + ) + gate = checker(registry("fetch")) + result = gate.check(proposal) + + assert not result.admitted + # `_explode` is every registered kind's factory, so anything that built the + # graph anyway would raise AssertionError rather than fail this quietly. + assert result.failed_checks() == (Check.REGISTRY,) + + def test_an_edge_may_reference_a_node_already_in_the_graph(): proposal = Subgraph( nodes=(ProposedNode(name="fetch"),),