mirror of
https://github.com/ModernRelay/omnigraph.git
synced 2026-06-15 01:55:13 +02:00
docs(rfc): RFC-009 — unify CLI access paths; align the RFC corpus
Adopts the unify-embedded/remote draft as RFC-009 with three alignment amendments: (1) the promised 'companion config-authority RFC' is RFC-008, already landed through stage 4 — referenced, not re-proposed; (2) open question 3 is answered by the two-surface architecture (embedded graphs list enumerates the cluster catalog via read_serving_snapshot, never omnigraph.yaml); (3) Phase 2 salvages PR #139's reviewed-clean omnigraph-api-types extraction instead of rebuilding. Adds the cycle's two no-referee bugs (alias positional, write-if-absent flush) as concrete parity-matrix motivation, and RFC-007's addressing/credential chains as RemoteClient constructor inputs. Corpus alignment: RFC-002's header now maps each of its pieces to the successor that landed or superseded it (007/008/009) with a do-not- implement-from-here-unchecked warning; RFC-007 gains the RFC-009 relationship; RFC-008 stage 5 notes the Phases-4/5 easing; dev index row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
c94ee2572f
commit
9002cfd5b9
5 changed files with 222 additions and 2 deletions
|
|
@ -78,6 +78,7 @@ Working documents for in-flight feature work. Removed when the work lands.
|
|||
| Server boots from cluster state — Phase 5 mode switch, applied-revision serving | [rfc-005-server-cluster-boot.md](rfc-005-server-cluster-boot.md) |
|
||||
| Per-operator config — `~/.omnigraph/` identity, keyed credentials, named servers (the operator slice of RFC-002) | [rfc-007-operator-config.md](rfc-007-operator-config.md) |
|
||||
| Deprecate `omnigraph.yaml` — one concern per config surface; key-by-key migration map and staged retirement | [rfc-008-deprecate-omnigraph-yaml.md](rfc-008-deprecate-omnigraph-yaml.md) |
|
||||
| Unify CLI embedded/remote access paths — parity referee, shared wire-DTO crate, `GraphClient` trait, declared plane capabilities | [rfc-009-unify-access-paths.md](rfc-009-unify-access-paths.md) |
|
||||
|
||||
## Boundary
|
||||
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
# RFC: Config & CLI Architecture — Layered Config, Client Targeting, File Naming
|
||||
|
||||
**Status:** Proposed
|
||||
**Status:** Proposed (umbrella; implementation parked — PRs #139/#162). Its pieces have since landed or been superseded piecemeal: layered config/file-naming/credentials → [rfc-007-operator-config.md](rfc-007-operator-config.md) (landed); the project-manifest role of `omnigraph.yaml` → deprecated by [rfc-008-deprecate-omnigraph-yaml.md](rfc-008-deprecate-omnigraph-yaml.md) (stages 1–4 landed); the `omnigraph-api-types` extraction and client unification → [rfc-009-unify-access-paths.md](rfc-009-unify-access-paths.md) (proposed; salvages #139's clean extraction). Still exclusively here: `GraphLocator`/multi-homing (§1), roles (§3), the State layer / `omnigraph use`. Do not implement from this document without checking those successors first.
|
||||
**Date:** 2026-05-30
|
||||
**Tickets:** MR-668 (multi-graph server, shipped — the dependency this builds on), MR-969 (stored queries + MCP — supplies the in-repo agent tool surface), MR-973 (quickstart / onboarding), MR-974 (agent setup surface), MR-981 (agent-friendly CLI hardening)
|
||||
**Target release:** v0.8.x (tentative; phased — see Rollout)
|
||||
|
|
|
|||
|
|
@ -319,6 +319,12 @@ Every mention of `omnigraph.yaml` in this RFC describes the deprecation
|
|||
window only. Sequencing couples them: RFC-007 PRs 1–2 land first, then
|
||||
RFC-008's migration stages run against them.
|
||||
|
||||
[rfc-009-unify-access-paths.md](rfc-009-unify-access-paths.md) consumes
|
||||
this RFC's surfaces: the actor chain and keyed-credential chain become
|
||||
constructor-time inputs of its `RemoteClient`/`EmbeddedClient`, and
|
||||
`--server`/operator aliases resolve to the same (base URL, credential)
|
||||
pair before its `GraphClient` trait is touched.
|
||||
|
||||
RFC-002 remains the umbrella architecture. This RFC implements its §2
|
||||
(layered config, global-first), §4 (file naming / one dir), and §5
|
||||
(credentials) in their minimal load-bearing form, and explicitly defers §1
|
||||
|
|
|
|||
|
|
@ -134,7 +134,7 @@ contract), retirement is staged, loud, and tooled:
|
|||
`cluster.yaml` from `config migrate`.
|
||||
4. **Opt-in strict** *(landed — the release gap to stages 1–3 collapsed: no version boundary was crossed between them, so all four ship in the same release)*. `OMNIGRAPH_NO_LEGACY_CONFIG=1` turns the warning into
|
||||
an error — for teams that finished migrating and want regressions caught.
|
||||
5. **Remove at the next major.** Loading the file becomes an error pointing
|
||||
5. **Remove at the next major** *(eased by [rfc-009-unify-access-paths.md](rfc-009-unify-access-paths.md) Phases 4–5: declared plane capabilities and route alignment shrink the yaml-boot removal diff)*. Loading the file becomes an error pointing
|
||||
at `config migrate`. The `OmnigraphConfig` code path, the dual
|
||||
query-registry loaders, and the yaml-mode server boot source are deleted
|
||||
— the payoff that makes the whole exercise worth it.
|
||||
|
|
|
|||
213
docs/dev/rfc-009-unify-access-paths.md
Normal file
213
docs/dev/rfc-009-unify-access-paths.md
Normal file
|
|
@ -0,0 +1,213 @@
|
|||
# RFC: Unify the CLI's Embedded and Remote Access Paths
|
||||
|
||||
**Status:** Proposed
|
||||
**Date:** 2026-06-12
|
||||
**Audience:** engine/CLI/server maintainers
|
||||
**Builds on:** [rfc-007-operator-config.md](rfc-007-operator-config.md)
|
||||
(landed — `--server` targeting and operator aliases are remote-addressing
|
||||
surfaces the unified client must treat as first-class),
|
||||
[rfc-008-deprecate-omnigraph-yaml.md](rfc-008-deprecate-omnigraph-yaml.md)
|
||||
(stages 1–4 landed — the config-authority demotion this RFC's earlier
|
||||
drafts promised as a "companion" already happened; the remaining sliver,
|
||||
removing the yaml-mode server boot source, is RFC-008 stage 5 and is
|
||||
*eased* by Phases 4–5 here), [rfc-002-config-cli-architecture.md](rfc-002-config-cli-architecture.md)
|
||||
(umbrella; see Prior Art for what is salvageable from its parked
|
||||
implementation).
|
||||
**Sequencing:** post-v0.7.0 (the release cut comes first).
|
||||
|
||||
## Summary
|
||||
|
||||
Collapse the CLI's per-command `is_remote` forks into one execution path coded
|
||||
against a `GraphClient` trait with two implementations (embedded engine, HTTP),
|
||||
sharing one wire-DTO crate with the server. Establish an executable parity
|
||||
referee *before* the refactor. This is the same cure, in the same order, that
|
||||
fixed the storage layer: one contract, one implementation where semantics are
|
||||
one thing, an executable contract where two implementations must exist.
|
||||
|
||||
## Motivation — validated facts
|
||||
|
||||
Graph **semantics** cannot drift between paths today: both converge on the same
|
||||
engine `_as` entry points (verified: `omnigraph-server/src/handlers.rs` calls
|
||||
`mutate_as`, `apply_schema_as_with_catalog_check`, `load_as`,
|
||||
`branch_create_from_as`, `branch_delete_as`, `branch_merge_as` — the same
|
||||
functions the CLI's embedded arm calls), and Cedar enforcement lives inside
|
||||
those writers. What *can* drift is everything around them:
|
||||
|
||||
1. **15 `is_remote` forks** in `crates/omnigraph-cli/src/main.rs`, each
|
||||
duplicating request shaping and output mapping per command. (RFC-007 PR 3
|
||||
threaded `apply_server_flag` through exactly these sites — the duplication
|
||||
is measured, not estimated.)
|
||||
2. **Triple DTO construction.** "The result of a load" is built in three
|
||||
places: the server handler (engine result → HTTP response), the CLI remote
|
||||
arm (HTTP response → `LoadOutput` via `load_output_from_tables`), and the
|
||||
CLI local arm (engine result → hand-built `LoadOutput`). Three mappings
|
||||
that agree only by discipline — the exact shape of the storage-adapter bug
|
||||
class (one prose contract, N implementations, no referee).
|
||||
3. **The remote `load` arm rides the deprecated `/ingest` route.** A
|
||||
non-deprecated CLI verb coupled to a deprecated endpoint turns the
|
||||
endpoint's eventual removal into a surprise CLI breakage.
|
||||
4. **Plane restrictions are accidental, not declared.** `init` / `optimize` /
|
||||
`repair` / `cleanup` / `cluster *` are storage-only and `graphs list` is
|
||||
server-only by code shape; pointing `optimize` at an `https://` target
|
||||
fails with whatever `Omnigraph::open` says about an https URI. Per Hyrum,
|
||||
that accidental error text is already someone's dependency.
|
||||
5. **Parity pinning is thin.** One explicit parity test
|
||||
(`cli_schema_config.rs::schema_plan_parity_cli_and_sdk`), flow coverage in
|
||||
`system_local.rs`, and the OpenAPI drift test. No systematic
|
||||
per-verb embedded-vs-remote comparison exists. Two bugs from the current
|
||||
cycle argue the referee's value concretely: the operator-alias positional
|
||||
bug (the hidden `legacy_uri` positional swallowed the first arg — local
|
||||
and remote disagreed until a live smoke caught it) and the
|
||||
`write_text_if_absent` flush bug (one of N implementations of an
|
||||
unwritten contract) would both have failed a parity matrix.
|
||||
|
||||
## Design
|
||||
|
||||
Ordered so each phase is independently shippable and the referee exists before
|
||||
anything moves — mirroring the storage collapse, where the pinned contract
|
||||
tests gated the swap, and the test-monolith modularization (#192/#193), which
|
||||
makes Phase 3 tractable: the CLI dispatch is 1,184 lines today, not 4,200.
|
||||
|
||||
### Phase 1 — Parity matrix (the referee; do first, no refactor)
|
||||
|
||||
A CLI integration test (extend the `system_local.rs` harness, which already
|
||||
spawns both binaries): one fixture graph; for every forked verb, run the
|
||||
command once against the local URI and once against a spawned server with
|
||||
identical inputs; diff the `--json` outputs against an explicit allowlist of
|
||||
transport-only fields (e.g. resolved URI). Assert identical exit codes for the
|
||||
shared error cases.
|
||||
|
||||
This pins today's behavior so Phase 3 can't silently change it, and catches
|
||||
every future fork drift. It also incidentally covers utoipa annotation↔route
|
||||
mismatches (a lying `#[utoipa::path]` makes the remote leg 404).
|
||||
|
||||
### Phase 2 — One wire-DTO crate
|
||||
|
||||
Move the HTTP request/response types and the single `engine result → DTO`
|
||||
mapping per verb into a shared crate (working name `omnigraph-api-types`),
|
||||
carrying serde + utoipa `ToSchema` derives:
|
||||
|
||||
- `omnigraph-server` handlers serialize these types; utoipa derives
|
||||
`openapi.json` from them (the existing `openapi.rs` regeneration test stays
|
||||
the spec referee).
|
||||
- The CLI embedded path constructs them via the shared mapping.
|
||||
- The CLI remote path deserializes the literal same types.
|
||||
|
||||
The mapping then exists once, next to the type — it cannot fork. Spec codegen
|
||||
remains exactly where it belongs: foreign-language clients (the TS SDK
|
||||
pipeline). Generating a Rust client from the spec is explicitly rejected — it
|
||||
would round-trip Rust types through a lossy intermediate when compile-time
|
||||
type sharing is available.
|
||||
|
||||
**Prior art to salvage:** PR #139's review explicitly found the
|
||||
`omnigraph-api-types` extraction *clean* ("the crate extraction itself is
|
||||
clean and the openapi.json byte-identical claim holds" —
|
||||
[pr-139 findings](rfc-002-config-cli-architecture.md)); it was the behavior
|
||||
changes bundled alongside that killed the PR. Seed this phase from the
|
||||
extraction commits on `ragnorc/scrutinize-rfc-002` rather than rebuilding —
|
||||
cherry-picked narrowly, never relanded wholesale.
|
||||
|
||||
Boundary note: this does NOT violate "transport/auth stay at the boundary"
|
||||
(invariants §11). The shared crate holds plain serde DTOs; it depends on
|
||||
neither axum nor the engine's internals. The engine crate does not depend on
|
||||
it — the `engine result → DTO` mapping lives in the shared crate (or the CLI/
|
||||
server side), taking engine result types as input.
|
||||
|
||||
### Phase 3 — `GraphClient` trait, two implementations
|
||||
|
||||
```text
|
||||
trait GraphClient // verb-level: load, mutate, query, branch_*, schema_*, export, commit_*
|
||||
├── EmbeddedClient // wraps Omnigraph + the shared mapping; actor: explicit (--as cascade, RFC-007)
|
||||
└── RemoteClient // reqwest + bearer; actor: resolved server-side from the token
|
||||
```
|
||||
|
||||
Each CLI command body is written once against the trait; the 15 forks become
|
||||
2 impls × 1 contract. Actor resolution is a constructor-time difference of the
|
||||
impls, never a per-verb branch — the trust model (storage credentials =
|
||||
self-declared actor via the RFC-007 actor chain; server = token-resolved
|
||||
actor via the RFC-007 keyed-credential chain) is a feature, not drift.
|
||||
`RemoteClient` construction is where RFC-007's addressing lands once:
|
||||
positional URI, `--target`, `--server <name>`, and operator aliases all
|
||||
resolve to the same (base URL, credential) pair before the trait is touched.
|
||||
The Phase 1 matrix becomes the trait's conformance suite, run against both
|
||||
impls.
|
||||
|
||||
### Phase 4 — Declared plane capabilities
|
||||
|
||||
Each CLI command declares `Storage | Server | Both`. Dispatch checks the
|
||||
resolved target against the declaration and fails with one deliberate message
|
||||
("maintenance commands operate on storage directly; use a storage URI, not a
|
||||
server target") instead of today's incidental errors. The declaration table is
|
||||
also documentation: it makes the control-plane/data-plane split (maintenance
|
||||
and cluster commands must work with the server down) explicit in code.
|
||||
"Server" targets include operator-config named servers (RFC-007), not only
|
||||
literal `http(s)://` URIs.
|
||||
|
||||
### Phase 5 — Route alignment
|
||||
|
||||
Add a canonical `/load` endpoint (the handler already exists behind the
|
||||
`/ingest` shim); point `RemoteClient` at it; keep `/ingest` on its existing
|
||||
deprecation path. While here, check whether the server uses `utoipa-axum`'s
|
||||
router-coupled registration (`OpenApiRouter`/`routes!`); if it hand-mounts
|
||||
routes beside `#[utoipa::path]` annotations, prefer migrating registration so
|
||||
path annotations and mount points are the same declaration (the modularization
|
||||
already hit one orphaned-attribute incident of exactly this class).
|
||||
|
||||
## Non-goals
|
||||
|
||||
- **No localhost-server funnel for the embedded path.** Routing embedded use
|
||||
through a daemon would destroy the embedded/CLI/test story to buy parity the
|
||||
trait + matrix already provide.
|
||||
- **No trust-model unification.** `--as` vs bearer-resolved actors stay.
|
||||
- **No spec-codegen for the Rust client** (see Phase 2).
|
||||
- **No change to plane-restricted command availability** — maintenance stays
|
||||
storage-direct by design; Phase 4 only makes the restriction explicit.
|
||||
- **No config-authority work** — that was RFC-008, already landed through
|
||||
stage 4; this RFC neither accelerates nor blocks stage 5, though Phases 4–5
|
||||
make the eventual yaml-boot removal a smaller diff.
|
||||
|
||||
## Compatibility
|
||||
|
||||
- CLI `--json` output is observable contract; Phase 1 freezes it before
|
||||
Phase 3 moves code. Any field-level unification that *changes* output is a
|
||||
deliberate, release-noted decision, not a refactor side effect.
|
||||
- Error-message text for mis-planed commands changes in Phase 4 — release-note
|
||||
it (Hyrum).
|
||||
- `openapi.json` should be byte-stable through Phase 2 if the DTO move is
|
||||
faithful; the regeneration test enforces this.
|
||||
|
||||
## Testing
|
||||
|
||||
- Phase 1 matrix is the spine; it must stay green, textually unchanged,
|
||||
through Phases 2–3 (the storage-collapse playbook).
|
||||
- Phase 2: `openapi.rs` byte-stability + existing server tests.
|
||||
- Phase 4: one test per capability class asserting the deliberate error.
|
||||
- Phase 5: parity matrix leg for `load` flips to `/load`; an `/ingest` shim
|
||||
test stays until removal.
|
||||
|
||||
## Open questions
|
||||
|
||||
1. Crate granularity: one `omnigraph-api-types` crate vs folding into an
|
||||
existing one. (Leaning separate: server and CLI both depend on it; the
|
||||
engine must not. The #139 extraction already answered this with a separate
|
||||
crate that reviewed clean.)
|
||||
2. Does the `query`/`read` streaming path (NDJSON export) fit the trait, or is
|
||||
export a documented per-impl method? (Streaming over HTTP vs an iterator
|
||||
over the embedded engine differ in shape, not content.)
|
||||
3. ~~Whether `graphs list` belongs on the trait.~~ **Answered by the
|
||||
two-surface architecture**: the embedded impl enumerates the **cluster
|
||||
catalog** (`read_serving_snapshot` exists for exactly this), never
|
||||
`omnigraph.yaml` (deprecated, RFC-008). `graphs list` becomes
|
||||
`Both`-capability: remote = `GET /graphs` (policy-gated), embedded =
|
||||
catalog enumeration from a cluster storage root.
|
||||
|
||||
## Relationship to prior work
|
||||
|
||||
The third application of the same principle in this lineage: storage adapters
|
||||
(collapsed to one implementation + an executable contract — and its CAS/flush
|
||||
bugs were exactly the no-referee class), recovery liveness, and now access
|
||||
paths. RFC-007 supplied the addressing and credential surfaces `RemoteClient`
|
||||
consumes; RFC-008 removed the competing config authority; RFC-002 remains the
|
||||
umbrella whose remaining unimplemented pieces (`GraphLocator`, the State
|
||||
layer) would build on the trait introduced here rather than on per-command
|
||||
forks.
|
||||
Loading…
Add table
Add a link
Reference in a new issue