ITADN

apply always fails with "No plan found" — Planner is reconstructed per call, last_plan never survives between plan() and apply()

#91OpenInmocalc 创建于 17 天前
I
Inmocalccommented
## Summary `apply` (`safe`/`force`/`migrate`) always fails with `"No plan found. Run plan() first."`, even immediately after a `plan()` call that computed a correct diff, and even within a single process (`batch`) or a single long-lived MCP server. This happens because `Planner::last_plan` lives on the `Planner` *instance*, and every call site — CLI `batch`, and the MCP handlers `onto_plan`/`onto_apply` — constructs a **new** `Planner::new(...)` per invocation. There is no code path anywhere in the crate where `plan()` and `apply()` are called on the same `Planner` instance, so `apply()` can never see a plan that was just computed. Tested against v1.1.1 (`aarch64-apple-darwin`), the latest release as of 2026-08-11. ## Root cause `src/plan.rs`: ```rust pub struct Planner { db: StateDb, graph: Arc<GraphStore>, last_plan: RefCell<Option<PlanState>>, // <-- lives on the instance } ``` `apply()`: ```rust pub fn apply(&self, mode: &str) -> anyhow::Result<String> { ... let plan = self.last_plan.borrow(); let plan = match plan.as_ref() { Some(p) => p, None => anyhow::bail!("No plan found. Run plan() first."), }; ... } ``` Every construction site (`rg 'Planner::new'`) makes a fresh instance, so `last_plan` is always `None` by the time `apply()`/`onto_apply` runs: - `src/batch.rs:281` (`exec_plan`) and `src/batch.rs:292`/`:422` (`exec_apply`) — two **separate** functions, each with its own `Planner::new(self.db.clone(), self.graph.clone())`. - `src/server.rs` — `onto_plan` and `onto_apply` are independent `#[tool]` async handlers, each building its own `Planner::new(...)`. Since this pattern is identical to the CLI's, the bug reproduces over MCP too — no need to actually stand up `serve` to confirm it, though I did trace the code path to be sure there's no session-level `Planner` cached anywhere on the server struct. - `src/civex.rs:226` (`certify_action`) — the only other call site that uses `.plan()` — never calls `.apply()` on that same instance either (it only uses `.plan()` for blast-radius cost estimation and `.is_locked()` for the lock check). So there is no code path in the current crate where `plan()` and `apply()` share a `Planner`. This isn't a usage error (wrong flags, wrong order, wrong process boundary) — it's structural. ## Reproduction (single process, `batch`) `base.ttl`: ```turtle @prefix ex: <https://example.org/> . @prefix owl: <http://www.w3.org/2002/07/owl#> . @prefix rdfs: <http://www.w3.org/2000/01/rdf-schema#> . ex:Persona a owl:Class ; rdfs:label "Persona" . ``` `proposed.ttl` (same plus one class): ```turtle @prefix ex: <https://example.org/> . @prefix owl: <http://www.w3.org/2002/07/owl#> . @prefix rdfs: <http://www.w3.org/2000/01/rdf-schema#> . ex:Persona a owl:Class ; rdfs:label "Persona" . ex:Organizacion a owl:Class ; rdfs:label "Organizacion" . ``` ``` $ oo --data-dir ./data init $ cat batch.txt load base.ttl plan proposed.ttl apply safe stats $ oo --data-dir ./data batch --pretty batch.txt ``` ```json {"command":"load","result":{"ok":true,"path":"base.ttl","triples_loaded":2},"seq":0} {"command":"plan","result":{"added_classes":["https://example.org/Organizacion"],"added_properties":[],"blast_radius":{"triples_affected":0},"locked_violations":[],"removed_classes":[],"removed_properties":[],"risk_score":"low"},"seq":1} {"command":"apply","result":{"error":"No plan found. Run plan() first."},"seq":2} {"command":"stats","result":{"classes":1,"data_properties":0,"individuals":0,"object_properties":0,"properties":0,"triples":2},"seq":3} ``` `plan` correctly computes the diff (one class added, `Organizacion`). The very next command in the *same batch invocation, same process* — `apply` — fails. (I originally hit this against a ~1300-triple real-world ontology; the two-class example above reproduces it identically and needs no external files.) ## Secondary finding (separate from the state bug, worth tracking even after the above is fixed) Independent of the state bug: `plan()`/`apply()` currently operate purely on the TBox. `extract_classes_from_store`/`extract_properties_from_store` query only `?c a owl:Class` and `?p a owl:{Object,Datatype}Property` — they never look at individuals, so `plan()` is blind to ABox changes (the real-world ontology I originally tested this against has 194 individuals, and the diff never mentioned any of them). And `apply(mode="safe"|"force")` doesn't apply a diff at all — it does: ```rust self.graph.clear()?; let count = self.graph.load_turtle(&plan.new_turtle, None)?; ``` i.e. it clears the *entire* store and reloads the full proposed Turtle wholesale. For a "Terraform for knowledge graphs" plan/apply cycle, this means `apply` can't be used for incremental, governed ABox mutations (add one instance, update one triple) without accepting a full-store clear+reload each time — which is both a performance concern at scale and a correctness concern (anything not present in `new_turtle` silently disappears, including instance data `plan()` never told you about). There's also a smaller issue in `apply_migrate` (`mode="migrate"`): it pairs *every* removed class/property with `plan.added_classes.first()`/`plan.added_properties.first()` — a rename-detection heuristic that only makes sense for a single rename and produces incorrect `owl:equivalentClass`/`owl:equivalentProperty` bridges whenever more than one class/property is added in the same plan. ## Suggested fix (for the primary bug) Move `last_plan` off the `Planner` struct and onto something that outlives a single call — e.g. persisted in `StateDb` (the way `iri_locks` already is), keyed by a plan id that `plan()` returns and `apply(plan_id)` takes as a parameter, or held on the long-lived server/session struct rather than reconstructed by each `#[tool]` handler. Given that `apply` currently only supports "reload the entire proposed graph anyway" semantics, an alternative (and IMO cleaner) design would be to make `apply()` take the full delta as an explicit argument instead of implicit prior state, removing the cross-call state requirement entirely. Happy to share the small repro harness (a `batch` file + two Turtle files) if useful.
0 条评论