refactor(cli): collapse write/query forks onto GraphClient (RFC-009 Phase 3b) - #211
Merged
Conversation
…hase 3b) Phase 3a put the GraphClient enum in place and collapsed the five uniform read forks. 3b folds the remaining data-plane forks onto the same enum: load, ingest, mutate, query, branch create/delete/merge, and schema apply. The wrinkle 3a deferred was the local policy attachment. Reads and query open the local engine without a policy; writes open through open_local_db_with_policy and attribute a resolved actor. So the Embedded variant grows an optional policy context (graph/actor) filled by a second factory, resolve_with_policy; resolve() leaves it empty. open_embedded picks the open path from whether the context is present, preserving both of today's behaviors exactly. query still uses resolve() (no policy), as the read path did. apply_schema takes the catalog-validator closure as impl FnOnce(&Catalog) — the embedded arm runs it inside apply_schema_as_with_catalog_check, the remote arm ignores it (the server runs its own check). That non-object-safe closure is why GraphClient is an enum, not a trait. The stored-query registry is still built caller-side and only for the local path. load and ingest stay separate methods: same operation, but load surfaces the CLI LoadOutput (two distinct per-arm mappings preserved) while ingest surfaces the wire IngestOutput. The now-fully-dead execute_read/ execute_read_remote and execute_change/execute_change_remote pairs are retired (legacy_change_request_body stays — client.rs uses it); the export pair remains for 3c. The Phase-1 parity matrix is unchanged and green; full workspace tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
aaltshuler has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Contributor
|
@greptile review |
There was a problem hiding this comment.
aaltshuler has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
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
2 participants
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.
Phase 3a put the
GraphClientenum in place and collapsed the five uniform read forks. 3b folds the remaining data-plane forks onto the same enum:load,ingest,mutate,query,branch create/delete/merge, andschema apply. The 15 per-commandif graph.is_remote {…} else {…}forks are now gone.Design
Embeddedgrew an optional policy context —{ uri, graph: Option<ResolvedCliGraph>, actor }.resolve()(reads +query) leaves it empty → bareOmnigraph::open;resolve_with_policy()(writes) fills it →open_local_db_with_policy+ resolved actor. Oneopen_embeddedhelper picks the path, preserving today's two behaviors exactly.apply_schematakes the catalog validator asimpl FnOnce(&Catalog)— embedded runs it insideapply_schema_as_with_catalog_check, remote ignores it (server runs its own check). That non-object-safe closure is whyGraphClientis an enum, not a trait. The stored-query registry is still built caller-side, and only for the local path.loadandingeststay separate methods — same operation, different surfaced shape (CLILoadOutputwith two distinct per-arm mappings vs the wireIngestOutput), so no drift risk.execute_read/execute_read_remoteandexecute_change/execute_change_remotepairs are retired (legacy_change_request_bodystays —client.rsuses it). The export pair remains for 3c.Verification
parity_matrix.rstextually unchanged (not in the diff) — all 11 rows green.cli_data/cli_schema_config/cli_queriesgreen.cargo test --workspace --locked→ exit 0, zero failures.Out of scope (→ 3c): export (streaming), graphs-list, export helper retirement.
🤖 Generated with Claude Code