docs(rfc): RFC-010 — restructure the CLI around explicit planes - #214
Conversation
The CLI silently spans three planes (data / storage-maintenance / control) and forces the operator to name a graph differently per plane: the graph you query as `--server prod --graph knowledge` you must maintain as `s3://bucket/knowledge.omni`. Plane restrictions (graphs list is server-only, optimize is storage-only) are accidental — discovered by hitting a cryptic error, not declared. RFC-010 proposes: one graph-addressing model across every verb, a declared per-subcommand capability surface (expanding RFC-009 Phase 4), and plane-grouped --help. Storage maintenance stays off the wire deliberately (no HTTP routes for optimize/cleanup/repair). CLI-internal only — no engine, server, or wire change. Incorporates the Codex review thread (kept verbatim with per-point Resolution notes): sharpened resolver authority rule (operator/legacy target must be direct storage; cluster-managed graphs via explicit --cluster --graph), per-subcommand capability table (schema plan vs show/apply, queries validate vs list, session/tooling classified), graphs list aligned to RFC-009's both-later target, init promoted to an explicit cluster-apply signpost, and a Test plan that extends the existing CLI suites and pins the new wrong-plane error strings. Linked from docs/dev/index.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
| ## Current state of affairs | ||
|
|
||
| The CLI has 23 top-level commands. They divide into three planes, addressed | ||
| three different ways: |
There was a problem hiding this comment.
query vs read — command name inconsistency with live codebase
AGENTS.md (Version surveyed: 0.7.0, the canonical current state) shows the query verb as omnigraph read (omnigraph read --query ./queries.gq), but this RFC uses query throughout — in the current-state plane table, all Target CLI ergonomics examples (omnigraph query --target knowledge --query q.gq), and the grouped --help block. If the actual top-level verb is read, this inconsistency will silently misdirect the implementation PR (#213) when authors pick the verb name to refactor around. Either confirm that query is already the correct verb name (perhaps introduced alongside RFC-009 Phase 3c), or align the RFC examples with read. If a rename is part of this proposal, it should be stated explicitly as a change rather than assumed.
|
|
||
| ### Maintenance: `--target` must resolve to direct storage (loud if not) | ||
|
|
||
| ```bash | ||
| $ omnigraph optimize --target prod | ||
| error: `--target prod` resolves to a remote server (https://prod…). | ||
| `optimize` is a storage-plane command and needs direct storage access. | ||
| Pass the graph's s3://… URI, or use --cluster <dir> --graph <id>. | ||
| ``` |
There was a problem hiding this comment.
graphs list plane classification conflicts between help block and capability table
The --help grouping example places graphs list under "INSPECT / SESSION" alongside queries list, login, logout, and other config/session commands. The capability table directly below classifies it as data-plane (Data (remote) ✅, with embedded-cluster coming later per RFC-009). These two representations within the same RFC contradict each other: the help block implies it's session-scoped tooling, while the table correctly reflects it as a data-plane command that requires a multi-graph server today. The help block should move graphs list into the DATA PLANE group (perhaps with a parenthetical noting the embedded-cluster future state), or the RFC needs prose explaining why it's grouped differently from its capability classification.
|
|
||
| **`graphs list` reconciliation:** RFC-009's answered open question (pinned in | ||
| `parity_matrix.rs`'s exclusions comment) targets `graphs list` becoming | ||
| Both-capability once the embedded arm enumerates the cluster catalog. This RFC | ||
| **aligns** with that rather than superseding it: the capability table shows | ||
| `graphs list` as remote today, embedded-cluster later. | ||
|
|
||
| ## Open questions | ||
|
|
||
| 1. **Capability-table location** — a CLI-internal const, or surfaced (e.g. in | ||
| `--help` and a machine-readable `omnigraph capabilities` for tooling)? | ||
| 2. **`--cluster <dir> --graph <id>` for maintenance** — does the maintenance | ||
| command resolve the storage URI from the applied cluster state, or from the | ||
| declared `cluster.yaml`? (Applied state is the truth the server serves; | ||
| declared config may be ahead of it.) | ||
|
|
||
| ## Review comments (Codex, 2026-06-13) | ||
|
|
||
| Overall take: the direction is right. The planes already exist; making them | ||
| declared in code, help text, and error messages should reduce operator surprise. | ||
| Keeping storage maintenance off HTTP is also the right boundary: `optimize`, | ||
| `repair`, and `cleanup` are direct-storage operator actions, not a multi-tenant | ||
| serving surface. | ||
|
|
||
| Before implementation, tighten these points: | ||
|
|
||
| 1. **Resolver authority needs a sharper rule.** The proposal says maintenance | ||
| resolves storage URIs "from `cluster.yaml` / operator config", but those are | ||
| different authority surfaces. Today `--target` is an operator/legacy | ||
| graph-target lookup; cluster config is read by `cluster` commands and by | ||
| `--cluster` server boot. Do not make a maintenance command silently consult | ||
| both and pick a precedence. Either: | ||
| - `--target` on maintenance means an operator/legacy target whose URI is | ||
| already direct storage, with remote targets failing loudly; or | ||
| - add an explicit cluster-root/config resolver for this case, so reading | ||
| cluster state is an intentional mode. | ||
|
|
||
| **Resolution (accepted):** both — `--target` resolves through operator/legacy | ||
| config and must be direct storage (remote → loud fail); cluster-managed graphs | ||
| use the explicit `--cluster <dir> --graph <id>` resolver. See *Authority | ||
| rule* under Proposed shape. | ||
|
|
||
| 2. **`graphs list` conflicts with RFC-009's target shape.** This RFC classifies | ||
| `graphs list` as remote-only, while RFC-009's answered open question says it | ||
| becomes Both-capability once the embedded arm enumerates the cluster catalog. | ||
| Pick one direction here: either this RFC explicitly supersedes that target, | ||
| or the capability table should show `graphs list` as remote today and | ||
| embedded-cluster later. | ||
|
|
||
| **Resolution (accepted):** align, don't supersede. The table shows `graphs | ||
| list` remote-today / embedded-cluster-later. See *Relationship to RFC-009*. | ||
|
|
||
| 3. **The capability table should be per subcommand, not per family.** The | ||
| family-level rows hide the exact cases the table is supposed to make | ||
| non-accidental. At minimum, call out: | ||
| - `schema plan` as local/storage-backed today, while `schema show` and | ||
| `schema apply` route through the graph client; | ||
| - `queries validate` versus `queries list`, which do not have the same | ||
| plane shape; | ||
| - `lint`, `policy`, `embed`, `login`, `logout`, `config`, and `version`, so | ||
| enumeration/session/tooling commands are intentionally classified instead | ||
| of falling outside the model. | ||
|
|
||
| **Resolution (accepted):** the capability table is now per-subcommand and | ||
| classifies every command, including the session/tooling group. | ||
|
|
||
| 4. **`init` should be an explicit exception.** Direct-storage `init` is fine. | ||
| A cluster-managed graph should be created by `cluster apply`, with ledger, | ||
| recovery, and approval semantics. If a named target resolves to a | ||
| cluster-managed graph root, `init` should signpost `cluster apply` rather | ||
| than quietly initializing that root out of band. | ||
|
|
||
| **Resolution (accepted):** promoted from open question to a decision. See | ||
| *`init` is an explicit exception*. | ||
|
|
||
| Testing notes for the implementation slice: | ||
|
|
||
| - Extend the existing CLI suites rather than adding a new duplicate harness: | ||
| `parity_matrix.rs` for capability exclusions, `cli_data.rs` for maintenance | ||
| wrong-plane errors, `cli_schema_config.rs` for `graphs list` / help behavior, | ||
| and `system_local.rs` for `--server` / operator-targeting edge cases. | ||
| - Pin the new wrong-plane error strings deliberately. This RFC is intentionally | ||
| replacing accidental `Omnigraph::open` strings with stable capability errors, | ||
| and those strings become observable behavior. | ||
|
|
||
| **Resolution (accepted):** captured as the *Test plan* section. |
There was a problem hiding this comment.
Review transcript embedded in the RFC body is non-standard for this project
RFC-009 (the document this RFC builds on) has no embedded review section — proposals end after "Open questions." Keeping the Codex review verbatim as a permanent ## Review comments (Codex, 2026-06-13) section means future readers navigating docs/dev/ will encounter what reads as a design review transcript as part of the canonical proposal. Since all four resolutions have already been incorporated into the body ("Proposed shape", "Compatibility", "Test plan", "Relationship to RFC-009"), the review section is now entirely redundant with the proposal text. Consider either removing the section (the incorporated resolutions stand on their own) or collapsing it to a single-sentence acknowledgment that the Codex review pass was incorporated.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Proposes making the CLI's three planes — data, storage/maintenance, control — explicit and coherent. Docs-only; separate from the Phase 3c code PR (#213).
Problem
The CLI forces you to name a graph differently per plane (query it as
--server prod --graph knowledge, maintain it ass3://bucket/knowledge.omni), and plane restrictions (graphs listserver-only,optimizestorage-only) are accidental — you find them by hitting a cryptic error.Proposal (CLI-internal only — no engine/server/wire change)
--targetworks forqueryandoptimize.--serverdoesn't apply" errors from one table.--help.Includes a Target CLI ergonomics section with concrete before/after examples.
Review thread folded in
Keeps the Codex review verbatim with per-point Resolution notes:
--targetmust be direct storage; cluster-managed graphs via explicit--cluster --graph— no silent precedence between operator config andcluster.yaml).schema planvsshow/apply;queries validatevslist; session/tooling commands classified).graphs listaligned to RFC-009's both-later target (not superseded).initpromoted to an explicitcluster applysignpost for cluster-managed roots.Linked from
docs/dev/index.md.🤖 Generated with Claude Code
Greptile Summary
This docs-only PR adds RFC-010 (
docs/dev/rfc-010-cli-planes-restructure.md), a proposal to make OmniGraph's three CLI planes (data, storage/maintenance, control) explicit through a unified--targetaddressing model, a per-subcommand capability table, and plane-grouped--help. A one-line update todocs/dev/index.mdregisters the new RFC in the working-documents table.graphs listRFC-009 alignment, per-subcommand capability table,initsignpost tocluster apply) and adds a concrete test plan pinning new wrong-plane error strings.omnigraph querythroughout (current-state table, examples, help block), butAGENTS.md(v0.7.0 baseline) consistently usesomnigraph read; ifreadis the actual verb, this mismatch propagates into the companion implementation PR (refactor(cli): collapse export + graphs-list onto GraphClient (RFC-009 Phase 3c) #213).--helpexample placesgraphs listin "INSPECT / SESSION" while the capability table classifies it as data-plane (remote today), creating an internal inconsistency within the RFC.Confidence Score: 4/5
Safe to merge as a docs-only RFC — no code, no wire changes, no migration. The verb naming question should be resolved before the companion implementation PR (#213) begins.
The RFC is well-reasoned and internally consistent on its main design decisions, but it systematically uses
queryas the top-level query verb while the live codebase reference (AGENTS.md) showsread. If the verb is actuallyread, every example and the plane classification table in this RFC will mislead the implementation PR.docs/dev/rfc-010-cli-planes-restructure.md — verb name consistency and
graphs listhelp-group placement.Important Files Changed
query/readverb name mismatch with AGENTS.md, a within-document inconsistency in wheregraphs listis grouped, and an unusual embedded AI review transcript that duplicates already-incorporated resolutions.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["graph-addressing input\n(positional URI | --target | --graph | --server)"] --> B{Resolver} B --> C{Command plane?} C -->|Data plane\nquery/mutate/load/branch…| D{Target type} D -->|file:// or s3://| E[EmbeddedClient\n→ engine directly] D -->|http/https or named server| F[RemoteClient\n→ HTTP GraphClient] D -->|--target alias| G{Config lookup} G -->|resolves to storage URI| E G -->|resolves to server URL| F C -->|Storage / maintenance plane\noptimize/repair/cleanup/init…| H{Target type} H -->|file:// or s3://| I[Embedded engine\non storage directly] H -->|http/https → FAIL| J["error: storage-plane command\nneeds direct storage access"] H -->|--target alias| K{Config lookup} K -->|resolves to storage URI| I K -->|resolves to remote URL| J H -->|--cluster dir --graph id| L[Resolve URI\nfrom cluster state] L --> I C -->|Control plane\ncluster validate/plan/apply…| M["--config dir\n→ cluster directory"] style J fill:#f88,stroke:#c00 style E fill:#8f8,stroke:#080 style F fill:#8f8,stroke:#080 style I fill:#8f8,stroke:#080 style M fill:#88f,stroke:#008Reviews (1): Last reviewed commit: "docs(rfc): RFC-010 — restructure the CLI..." | Re-trigger Greptile