refactor(cli): GraphClient enum + read verbs (RFC-009 Phase 3a) - #210
Merged
Conversation
The embedded-vs-remote split gets one home: a GraphClient enum
(Embedded { uri } | Remote { http, base_url, token }) with a resolve()
factory that absorbs the shared preamble (apply_server_flag -> token ->
URI/remoteness) and a verb method per command. The five uniform read
forks — branch list, commit list, commit show, schema show, snapshot —
collapse from per-command if-graph-is-remote else to one line each
(main.rs: -113/+47). Behavior identical per verb (local reads still open
WITHOUT policy, as today); the Phase-1 parity matrix is the referee and
passes textually unchanged.
Enum, not the RFC trait: only two variants ever, and inherent async
methods avoid async_trait boxing and the apply_schema closure that is not
object-safe (3b) — same one-body-two-impls collapse, less ceremony.
Scope: the uniform reads only. The query verb (policy-open + operator-
alias early-return + param merge) joins the write verbs in 3b;
export/streaming and graphs-list in 3c, where the now-shared
execute_*_remote/execute_* pairs get retired.
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.
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
1 participant
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.
First slice of RFC-009 Phase 3 — the fork collapse the matrix (Phase 1) and the shared DTO crate (Phase 2) were built to make safe.
What lands
A
GraphClientenum in a newclient.rsis the single home for the embedded-vs-remote split:resolve()does the shared preamble (apply_server_flag→ token → URI/remoteness) once, and each verb is one method routing to the engine (local) or HTTP (remote). The five uniform read forks — branch list, commit list, commit show, schema show, snapshot — collapse from per-commandif graph.is_remote { … } else { … }to one line each.main.rsnet −66 lines.Two referees, both green
parity_matrix.rs— Phase 1) and passing: behavior is identical per verb, including that local reads still open the engine without policy (today's behavior, preserved exactly).cargo test --workspace --lockedgreen (60 suites).Design note
Enum, not the RFC's literal "trait": there are only ever two variants, and inherent
asyncmethods avoidasync_traitboxing and theapply_schemacatalog-validator closure that isn't object-safe (arrives in 3b). Same one-body-two-impls collapse, less ceremony — flagged here so the deviation is on the record.Scope (this is 3a of three)
query— the Embedded variant grows a policy context.execute_*_remote/execute_*helper pairs.Noted, not bundled
Two pre-existing dead-code warnings (
inferred_config_path,yaml_string) orphaned by #199's scaffold removal surfaced during the build — left untouched here (the plan scoped their removal as a separate cleanup); will clear in a small follow-up.🤖 Generated with Claude Code