Skip to content

refactor(cypher): make graphforge-cypher a pure parser crate #1007

Description

@DecisionNerd

Found during the architecture audit (sub-agent review).

Finding (HIGH)

graphforge-cypher is described as "Cypher lexer and Pratt parser" but is a covert orchestrator reaching two stages downstream:

  • crates/graphforge-cypher/Cargo.toml:14 — production dep on graphforge-rel (and graphforge-ir)
  • cypher/src/lib.rs:16 — re-exports Binder, OntologyMode, RuntimeCatalog
  • cypher/src/lib.rs:79 — calls graphforge_rel::explain_logical for ExplainStage::LogicalPlan
  • cypher/src/lib.rs:96-120explain_stage drives binding itself
  • core/src/lib.rs:950-953 — documents this was done "to avoid circular crate dependencies"

This creates a second front-door that bypasses graphforge-api (the declared public API) and drags DataFusion/rel into the parser's dep tree.

Related inconsistencies:

  • Same BindError maps to GfError::Bind via api (api:3646-3665) but GfError::Plan via cypher::explain_stage (cypher/lib.rs:99-102) — identical binder rejection yields different public error codes depending on entry point
  • cypher/src/lib.rs:5-7 — crate is exempt from workspace lints (allow(clippy::all), allow(clippy::pedantic)) while every other pipeline crate inherits lints.workspace = true

Acceptance criteria

  • graphforge-cypher production deps limited to graphforge-ast (+ graphforge-core)
  • ExplainStage::LogicalPlan orchestration moves to graphforge-api (already depends on both stages)
  • Binder error codes identical across entry points (test proves it)
  • Workspace lints apply to graphforge-cypher, clippy clean

Architecture review decision

The finding is confirmed, with one wording correction: Cargo.toml already declares [lints] workspace = true; the crate-level #![allow(clippy::all)] and #![allow(clippy::pedantic)] directives neutralize those checks.

Dependency: define the facade error conversion in #1018 before declaring binder-code parity complete.

Additional acceptance criteria:

  • Remove the crate-level blanket Clippy allowances
  • Parser-only APIs do not depend on IR, rel, DataFusion, project paths, or runtime catalogs
  • Explain orchestration and stage error mapping have one owner in graphforge-api

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    coreCore source code changesparserChanges to Cypher parserrelease:noneNo release note or version impact

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions