Repository navigation
Conversation
Rewrite RFC-002 around a typed GraphLocator (storage: XOR server:+graph_id:), servers:+graphs: with three-tier addressing, serve: vs servers: de-collision, global-first layered config, a method x source auth model, and an omnigraph-config crate extraction. Verified against code, not ticket status. Incorporates the implementation-readiness review (10 points): 1. current flag is --target, not --graph; --graph canonical + --target alias 2. credential-redirection fix: endpoint-bound creds + layer identity rule + AX threat model 3. no-arg resolution: defaults.graph for bare commands; defaults.server only namespaces unknown ids 4. route unification spec: canonical single-mode graph_id; GET /graphs lists served set 5. serve.graphs replaces server.graph (preserves serve-a-subset) 6. restore query.roots (ad-hoc --query path resolution) 7. soften 'structurally unreachable'; move mTLS key off the repo tree 8. legacy bearer_token_env -> synthesized-server migration 9. enumeration caveat: known-id addressing vs graph_list-gated discovery 10. mark oauth/mtls reserved; full impl deferred to V6 Also realigns the docs/dev/index.md entry.
Address the second implementation-readiness review (7 points):
1. env-token endpoint-binding was not enforceable as written -> replace with a
trusted-origin credential model: ambient creds (env/keychain/profile) apply
only to servers whose identity came from a trusted layer; login-written creds
additionally bind to their issued-for endpoint.
2. project-layer auth: a lower-trust layer may define endpoint-only servers but
may not carry an auth: block at all (command = repo-authored RCE) - now a
validation rule, not just prose.
3. legacy remote-URI migration: split https://host/graphs/{gid} into
endpoint+graph_id so V2's always-/graphs/{id}/ client can't double the prefix.
4. summary realigned with body: enumeration is graph_list-gated, oauth reserved
(not first-class), secrets out-of-repo (not 'structurally unreachable').
5. disambiguate higher-precedence (project wins merges) vs higher-trust (global
owns identity) - they run opposite for the project layer.
6. drop top-level 'queries' from the named-resource merge map (per-graph only).
7. mark OMNIGRAPH_BIND proposed, not current; binary honors --bind/server.bind
only (lib.rs:899).
Address the third review (4 points):
1. serve.auth conflict: the lower-trust auth ban applied too broadly. Scope it
to servers.<name>.auth (client credential sourcing); serve.auth (secret-free
server-side accept config) stays valid in a committed deployment manifest.
Tightened the same wording in section 6 and 7 for consistency.
2. legacy URI split preserves the path prefix: strip only the trailing
/graphs/{gid}; endpoint keeps host + any reverse-proxy path.
3. define the explicit-credential surface: a project-only server is
unauthenticated/local-dev by default; authenticated use needs promotion to a
trusted layer or an operator-supplied --token-from flag (future, listed in 10).
4. OAuth no longer leaks into the V3 CLI surface: login OAuth device flow marked
V6 in the CLI bullet and the N11 breadboard row.
Synthesized from a six-agent code+upstream validation pass. Added: - V0 'Foundations' phase (crate + api-types extraction, version gate, layered-config fixture harness + keychain seam, test relocation). - Phase detail (sizing/gates/exit, file touchpoints) for V0-V6, a critical-path + parallelization graph, and a validation-findings list. - '## Divergence & single source of truth': states what the design prevents structurally (config<->CLI via the typed locator; config<->HTTP capability surface) vs only reduces (CLI<->HTTP route paths), and records the shared-route-table/generated-client move as the residual structural fix. Inline corrections from the audit: - storage.profile is env-only in Lance + omnigraph -> scoped out of v1; region/endpoint feasible now (Lance per-dataset storage_options). - single-mode GET /graphs is 405->403-by-default (not ->200) without a serve.policy; recorded the wire vs Cedar graph_id reconciliation decision. - omnigraph-config extraction alone does not shed Axum (CLI imports api::*) -> note the omnigraph-api-types companion crate.
Folds in the validated parts of the cloud-deployment workstream briefing.
Code claims verified to the line: GraphKey { tenant_id: Option<TenantId>,
graph_id } and ResolvedActor.tenant_id already ship (MR-668, identity.rs:116,189),
and tenant is server-resolved (MR-731, identity.rs:180) -- so these are cheap
reservations, not new machinery.
Added (reserve only, parse-but-reject; tenant never in locator/path/body):
- Non-Goals: cloud-mode multi-tenancy out of scope; shapes reserved so it is additive.
- 6: serve.auth.oauth.issuers as a LIST + tenant_claim (the one-way door);
field schema deferred to MR-956 RFC 0001 to avoid a second OIDC config.
Server-side OIDC reframed as Federated-Auth-owned (may precede V6), not 'my V6'.
- 6: serve.policy is a tagged source at the policy level (file today;
directory/manifest reserved) -- NOT a source: wrapper (pushback on the briefing's
prescription; the wrapper is the only actually-breaking part and is inconsistent
with storage:/auth:).
- 7: credential identity unit becomes (server, organization) for multi-org on one
cloud endpoint -- endpoint-binding alone can't disambiguate; reserve
omnigraph:<server>[/<org>] keying.
- 9: unified registry preserves GraphKey { tenant_id, graph_id }; don't flatten to
graph_id-only; GET /graphs tenant-scoped in Cloud.
- Open questions: OIDC ownership/timeline reconciliation.
Held the speculation line: organization selector, omnigraph:// URI sugar, and
--organization flag are additive-later, so they stay notes (Non-Goals), not new
fields/flags shipped now. Nit corrected: AuthSource::Oidc / graph:* scopes are
reserved via #[non_exhaustive], not present draft variants.
Move config.rs (schema + loader + resolvers) into a new clean-leaf crate
(serde/serde_yaml/clap/color-eyre). The server aliases it via
`pub use omnigraph_config as config;` so every internal `config::` path and
the `pub use config::{...}` re-exports resolve unchanged. No behavior change;
the 12 config tests move verbatim and pass. First step of RFC-002 V0.
Move queries.rs (stored-query registry + validation) into omnigraph-queries (deps: omnigraph-config, omnigraph-compiler; no serde). Repoint crate::config -> omnigraph_config. The two catalog-projection tests (which exercise api::query_catalog_entry) are removed here and relocated to omnigraph-api-types in the next commit, where that function lives. Server aliases via 'pub use omnigraph_queries as queries;'. 16 queries tests pass; server+cli compile. No behavior change.
Move api.rs (HTTP request/response DTOs + OpenAPI schemas) into omnigraph-api-types (deps: engine, compiler, queries, serde, serde_json, utoipa). Repoint crate::queries::StoredQuery -> omnigraph_queries. Relocate the two catalog-projection tests here, where query_catalog_entry lives, eliminating the queries<->api dev-dep cycle (no cycle, tests at their boundary). Server aliases via 'pub use omnigraph_api_types as api;'. openapi.json byte-identical (drift test passes; no regen). 2 api-types tests pass; server+cli compile. No behavior change.
Repoint CLI imports to the extracted crates: api DTOs -> omnigraph-api-types, QueryRegistry/check -> omnigraph-queries, config types -> omnigraph-config, and Policy* -> omnigraph-policy directly (no longer via the server re-export shim). Remove omnigraph-server from the CLI manifest. The CLI no longer pulls Axum/tower/utoipa-axum: 'cargo tree -p omnigraph-cli -i omnigraph-server' and '-i axum' both report not-in-graph. No behavior change (CLI compiles; no test churn — CLI tests import none of the moved symbols).
Workspace-crates line now reflects the omnigraph-config, omnigraph-queries, and omnigraph-api-types crates extracted from omnigraph-server (maintenance rule 1).
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
load_config reads the optional top-level 'version:' discriminator and rejects unsupported versions (>1) before parsing; no-version (legacy) and 'version: 1' both still parse via the existing lenient struct. Forward-compat gate (RFC-002 §3) added as a no-op so the typed GraphLocator schema can tighten in following commits without breaking legacy files. 14 config tests pass (incl. version:1 parses, version:2 rejected).
… §1/§2) Add the typed graph schema behind the version gate: per-graph storage: (string or block) XOR server:+graph_id:, a servers: map, and resolve_graph -> typed GraphLocator (local alias -> srv/gid qualified -> cli.graph default). uri stays a defaulted String filled by normalize_graphs from storage/server, so all existing .uri readers are unchanged and this commit is config-only (server reject-Remote is L5). deny_unknown_fields rejects typos; Storage has a hand-rolled string-or-block Deserialize for precise unknown-field errors. region/endpoint parsed but not yet engine-threaded (V2); GraphLocator.graph_id carried but unused on the wire until V2. 26 config tests pass; cargo test --workspace --locked green (per review).
Addresses a review finding: cargo fmt --check was failing on the V0/L1/L2 crates (and some pre-existing engine drift). cargo fmt --all --check is now clean. No behavior change.
A config without a `version:` is the legacy/lenient schema (RFC-002): an unrecognized top-level key must be tolerated, not rejected. This test pins that contract and fails today because `deny_unknown_fields` is applied unconditionally (struct-wide), so the `version` discriminator does not actually gate strictness. Intentionally red — goes green in the next commit, which gates unknown-field strictness on `version`.
…ored `#[serde(deny_unknown_fields)]` was applied unconditionally as a struct attribute, so the `version` discriminator did not actually gate strictness: a legacy (no-`version:`) config was wrongly rejected for any unrecognized key, contradicting the documented contract (RFC-002 §3: no version = legacy-lenient, `version: 1` = strict) and the code's own comments. Make strictness a function of `version`, decided at one point in `load_config_in`: parse once via `serde_ignored` (collecting ignored fields at all depths), then reject unknowns only under `version: 1`; legacy tolerates them (restoring the pre-existing behavior). Drop `deny_unknown_fields` from the two legacy-spanning structs (`OmnigraphConfig`, `TargetConfig`) and keep it on the v1-only typed blocks (`StorageBlock`, `ServerEntry`), which have no legacy form. This removes the double-parse (`check_config_version` is gone — the version is read from the parsed struct) and makes v1 strictness comprehensive: a misspelled nested key (e.g. under `cli:`) now errors instead of being silently dropped. Turns the previous commit's regression test green and pins v1 comprehensive strictness.
Make embedded-vs-remote a typed discriminant: `ResolvedCliGraph` now carries the `GraphLocator` from `config.resolve_graph` (replacing the `is_remote: bool` scheme-sniff field), exposed via an `is_remote()` accessor. The 9 dispatch reads switch from `graph.is_remote` to `graph.is_remote()`. Behavior-identical: the engine `uri`, Cedar `graph_id`, `selected`, and `policy_file` keep their exact current computation (`resolve_target_uri` / engine-normalized `normalize_policy_graph_uri` / `resolve_graph_selection` coherence check) — the locator's `uri`/`graph_id`/`endpoint` DIVERGE from those (base_dir-join, `file://` retention) and are V2 fields, so only `is_remote()` is read here. A doc comment records that trap. `is_remote_uri`/`resolve_uri` and the 11 raw-uri dispatch sites are untouched (their conversion + deletion is L4). No HTTP/API change; openapi.json unchanged.
Add an equivalence test asserting `ResolvedCliGraph::is_remote()` (the typed
locator discriminant) matches the old `is_remote_uri(graph.uri)` for every
address shape that flows through `resolve_cli_graph`: a `server:` graph, a legacy
remote `uri:` (with the `/graphs/{gid}` suffix), an embedded `uri:`, and positional
http(s)/s3 URIs. Extend the two existing graph_identity tests with `is_remote()`
assertions. `srv/gid` is omitted — it bails at `resolve_graph_selection` and is
unwired until V2.
Convert the 6 commands that still scheme-sniffed a raw URI — commit list, commit show, schema show, snapshot, export, graphs list — from `resolve_uri` + `is_remote_uri(&uri)` to `resolve_cli_graph` + `graph.is_remote()`, matching the pattern the other 8 commands already use (`let uri = graph.uri.clone()`). The scheme sniff is no longer a dispatch decision; `is_remote_uri` now survives only inside `normalize_policy_graph_uri` (Cedar id), and `resolve_uri` only inside `resolve_cli_graph` and the local-only optimize/cleanup paths. Behavior-preserving on the open path: local branches keep plain `Omnigraph::open` (these are reads — unlike writes, they don't attach policy). One intentional consistency delta: routing through `resolve_cli_graph` means these commands now apply the same top-level-block coherence check every other command applies — but only for a named-graph invocation with a populated top-level `policy`/`queries` block; positional invocations are byte-identical. No HTTP/API change.
omnigraph-server serves embedded graphs only (RFC-002 §3); a graph with
`server:` or a remote `http(s)://` `uri:` must be refused at config load. These
four boot tests (multi `server:`, multi remote `uri:`, single positional URI,
single named remote via server.graph) are RED today: load_server_settings
returns Ok for a remote entry — it builds a `Single`/`Multi { uri: "https://…" }`
and only fails later at `Omnigraph::open`. The next commit gates this.
omnigraph-server serves embedded graphs only (RFC-002 §3). Classify each served graph with the typed `config.resolve_graph(...).is_remote()` — the same locator classifier the CLI dispatches on — and `bail!` early with a clear "embedded graphs only" error if a graph sets `server:` or a remote `http(s)://` `uri:`, instead of accepting it and failing confusingly later at `Omnigraph::open`. Both modes covered: Single (hoist `selected` above the URI resolution and check before `cli_uri` is moved) and Multi (per entry, after the `GraphId` check). A new `ensure_embedded` helper keeps the message in one place. No config-crate change (reuses existing public `resolve_graph`/`is_remote`); embedded `storage:`/`uri:` entries pass through unchanged (positive guard test added). Updates the `server_settings_can_resolve_named_target` lib test, which asserted the old behavior (server resolves a remote named target); it now uses an embedded target — the remote case is rejected and covered by `single_mode_rejects_named_remote_graph`. Turns the previous commit's four red tests green.
The graph-selection flag is now `--graph` (the canonical noun, matching `graphs:`/`cli.graph`) with NO `--target` compat alias — a clean break, to avoid two-names-for-one-thing confusion. Applied across the 21 `omnigraph` subcommands and the `omnigraph-server` binary (the field stays `target` internally; `value_name = "GRAPH"` keeps help text clean). Updates the user-facing "no graph to serve" and stored-query selection-hint error messages, plus the two cli.rs tests that drove `--target`. Breaking: `--target` is removed (now an unknown-flag error); use `--graph`. `uri` is unchanged (positional or `--uri` per command — deliberately not folded into a shared arg, since the two uri patterns can't share one flattened definition without a breaking uri-form change). `--target-branch` (policy explain) is unrelated and untouched. Code comments and docs still referencing `--target` are swept in L8 with the docs.
Add a pure `OmnigraphConfig::deprecation_warnings() -> Vec<String>` so the CLI and server can surface load-time migration notices while the config crate stays stdio-agnostic. It flags a config with no `version:` (the legacy schema — gated on a new `loaded_from_file` so there's nothing to warn about when no `omnigraph.yaml` exists) and any graph still using the legacy `uri:` key (vs `storage:`/`server:`). Unused until the next commit wires the emit sites. 4 config tests added.
Surface the config deprecation notices at load: the CLI prints them via `eprintln!` in `load_cli_config`; the server logs them via `tracing::warn!` in `load_server_settings` (a no-op under tests with no subscriber). `omnigraph init` now scaffolds a `version: 1` config using `storage:` instead of the legacy `uri:`. Migrate the shared test fixtures (`support::local_yaml_config` → `version: 1` + `storage:`; `remote_yaml_config` → `version: 1` + `servers:`/`server:`) to the current schema so they don't trip the new warnings; the resolved `.uri` is unchanged, so the tests behave identically. The no-`version:` notice is gated on a loaded config file, so commands with a bare URI and no `omnigraph.yaml` stay quiet (verified by the existing command-deprecation tests, which still pass). Extends the `init` test for the scaffold shape and adds a legacy-config-warns test.
L6 renamed the flag but left stragglers. Two were functional: the `resolve_target_uri` "URI must be provided …" error still named `--target`, and `docker/entrypoint.sh` passed `--target` to omnigraph-server (which now only accepts `--graph`) — so the container failed to boot. Both fixed (plus the entrypoint smoke test's expected args). The rest are code comments across the config/server/cli crates and tests, and the cheat sheet, swept `--target` → `--graph`. `--target-branch` (policy explain) is a distinct flag and untouched; past release notes keep `--target` (accurate for those versions).
… RFC landed Sweep `--target` → `--graph` across the user docs (cli-reference, cli, server, policy) and update `omnigraph.example.yaml` to the v1 schema (`version: 1`, `storage:`, `servers:`/`server:`). Add the typed-locator schema (version/storage/ servers/graph_id + strictness) to cli-reference, and a server "embedded graphs only" note. The RFC gets an Implementation-status banner recording V1a as landed and its divergences (`--target` removed outright — no alias; `uri:` deprecation- warned) and corrects the stale `QueryRegistry`/config-location claims (it's in `omnigraph-queries` / `omnigraph-config` now). testing.md gains a Config & CLI note. Past release notes keep `--target` (accurate for those versions); `--target-branch` untouched.
ragnorc
changed the title
Add a raw top-level key-presence scan (reject_legacy_top_level_keys_under_v1) so the v1 schema can reject known-but-removed legacy keys that serde_ignored cannot surface (they stay struct fields for legacy parsing). The first such key is `project:` — it has no consumer; it is rejected under `version: 1` (naming the key) and stays honored-but-warned under the legacy schema. Drop `project:` from the `omnigraph init` scaffold so generated configs load clean under v1.
…nder v1 `defaults:` is the canonical CLI/client-defaults block. The legacy spelling `cli:` is accepted as a serde alias and honored under the legacy schema, but rejected under `version: 1` (pointing at the new spelling) and flagged by a deprecation warning. Generalizes the version-gated key scan into `legacy_top_level_keys`, which now drives both the v1 rejection and the legacy warnings via a shared migration-hint table. Renames the config accessors (cli_* -> default_*) and repoints the CLI call sites; migrates the init scaffold, the example config, and the shared test helpers to `defaults:`.
Rewrites the `omnigraph.yaml` schema reference and the CLI/policy/server config examples for `version: 1`: `cli:` -> `defaults:`, `server:` -> `serve:` (with the `graphs:` list), top-level `policy:`/`queries:` -> per-graph, `project:` removed, `uri:` -> `storage:`. Adds a legacy-spelling migration table and notes the legacy fallbacks. Updates testing.md's config-test inventory.
Split the body of load_config_in into a load_single_layer(cwd, path) helper that loads and fully processes one config file (version-gating, legacy scan, normalize) — the seam the upcoming layered loader needs — while load_config_in keeps its exact single-layer semantics (an explicit --config still errors on a missing file). Add the precedence-ordered Layer enum (Default < Global < State < Project) and a Provenance side-table newtype for the merge engine, plus the dirs dependency (already in the lock tree via lance, so zero new crates). No behavior change.
Add global_config_dir()/global_config_file() — RFC-002 §5 global-first resolution: OMNIGRAPH_CONFIG (explicit file) > OMNIGRAPH_HOME (dir) > $XDG_CONFIG_HOME/omnigraph > ~/.omnigraph, with config.yaml as the file. The resolution is factored through an injected-env form so it is hermetically testable without mutating process env. Not yet wired into loading — that is the layered loader.
Add resolve_all_paths_in_place, run at the end of load_single_layer (after normalize_graphs fills entry.uri), rewriting every relative path/URI field — graph uris, per-graph + top-level policy/queries files, serve.policy, auth.env_file, query.roots — to absolute against that layer's base_dir. A scheme URI passes through. This makes a layer self-contained so the upcoming merge composes absolute-only configs and never mis-resolves a path against the wrong layer's dir. Factor the join logic into resolve_uri_against/resolve_path_against, reused by the existing resolve_config_* methods. Behavior-preserving: the per-field resolvers pass already-absolute values through to the same result.
Add merge_layers(Vec<LoadedLayer>) -> (OmnigraphConfig, Provenance): folds parsed config layers low->high into one merged config plus a dotted-path origin map. Settings-objects deep-merge per leaf (a higher layer's Some/non-empty wins, None inherits); named-resource maps (servers/graphs/aliases/queries) union by key with a higher layer's entry replacing the lower wholesale (no intra-entry bleed); lists and scalars replace. Provenance is a sorted BTreeMap side-table, so the merged OmnigraphConfig shape and all its accessors stay unchanged — only config view reads it. Pure structure: every layer is already version-gated and path-resolved before merge. Not yet wired into loading.
Add load_layered_config: load the global ~/.omnigraph/config.yaml layer under the project ./omnigraph.yaml (or --config) layer and merge them, returning the merged config, its provenance, and per-layer deprecation warnings. The CLI's load_cli_config now uses it — so a graph/server/defaults defined only in the global config is usable from any directory with no project file. The server stays single-layer (a deployment manifest must not pick up ambient $HOME state). Global and cwd-default files are optional (absent = no layer); an explicit project --config still errors if missing. base_dir stays the highest loaded layer's config dir so relative ad-hoc --query paths resolve as before. The CLI test harness pins OMNIGRAPH_HOME to an empty temp dir so tests never read the developer's real global config.
Add config view [--resolved] [--show-origin] [--json] [<graph>]: prints the merged layered config (null/empty values pruned for readability), or with --show-origin the layer each field came from (sorted, deterministic), or with --resolved <graph> the typed GraphLocator (embedded vs remote). Suppress the always-empty legacy project:/server: blocks on serialize.
Add omnigraph use <graph>: validate the graph resolves in the merged config (loud
fail otherwise), then write ~/.omnigraph/state/active.yaml ({graph}). The layered
loader reads it as the State layer between Global and Project, so a bare command
targets the active graph — Project still overrides State, State overrides Global.
The State layer is synthetic (sets defaults.graph only) and raises no version/legacy
warnings.
Wrap the global/project layer loads with file context (in global config <path> / in project config <path>) so a strict v1 error names which file is at fault — a loud, attributable failure across layers (invariant 13). The single-layer load_config_in path keeps its raw message (one obvious file). Pairs with the layer-prefixed deprecation warnings the layered loader already surfaces.
Add a Layered config section to cli-reference.md (precedence chain, OMNIGRAPH_HOME/ CONFIG/XDG resolution, ~/.omnigraph layout, merge semantics) and the config view / use command rows; note global-first in cli.md; record the layered-config test surface in testing.md; update the RFC-002 implementation-status banner (V1-remainder landed).
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1a6371d. Configure here.
| config, | ||
| provenance, | ||
| warnings, | ||
| }) |
There was a problem hiding this comment.
Per-layer graph validation breaks merge
Medium Severity
load_layered_config_in runs normalize_graphs on each layer before merge, so a project omnigraph.yaml that defines a remote graph via server: fails if the matching servers: entry lives only in the global layer. Global-first setups cannot split server endpoints and per-repo graph entries across layers.
Reviewed by Cursor Bugbot for commit 1a6371d. Configure here.
| if let Some(parent) = path.parent() { | ||
| config.base_dir = parent.to_path_buf(); | ||
| } | ||
| Ok(Some(config)) |
There was a problem hiding this comment.
Active context server field ignored
Medium Severity
The state layer loader documents that active.yaml sets the default graph and optional server, but only graph is copied into defaults.graph. A persisted server value is never merged, so multi-server active context from omnigraph use or hand-edited state does not affect default targeting.
Reviewed by Cursor Bugbot for commit 1a6371d. Configure here.
…tly unmerged merge_layers destructured OmnigraphConfig with a `..` rest-pattern, so a config field added later would be silently dropped from the cross-layer fold (a drift trap). Bind every field explicitly (intentionally-ignored ones as `_`) so a new field fails to compile here until it is dispositioned — the compiler is the guard.
…omits no field config view --resolved hand-listed locator fields and dropped an embedded graph's region/endpoint/policy_file/selected — exactly the fields one debugs an S3 endpoint or per-graph policy with. Derive Serialize on GraphLocator (internally tagged `kind:`) and serialize+prune in print_resolved_locator, so every field is reported and a new field can't be forgotten by hand.
…ntry.uri resolve_all_paths_in_place absolutized the derived entry.uri but left the raw Storage::Bare/Block uri relative, so config view showed `uri: /abs` beside `storage: ./rel` and a future reader of storage.uri (V2 region/endpoint threading) would mis-resolve. Resolve the storage uri too. The eager test now exercises the storage block form — the coverage gap that let this slip.
An explicitly-set OMNIGRAPH_CONFIG pointing at a missing path was silently dropped (no global layer, confusing downstream 'graph not found'). Bail naming the file, so an explicitly-named global config is required like --config; the default ~/.omnigraph path stays optional.
…eContext.server config view --resolved --show-origin silently dropped --show-origin; add clap conflicts_with so the parser rejects the combo loudly. Remove the unused `server` field from ActiveContext (parsed, always written None, never read) — server-qualified active context is a V2/V3 concern; don't ship an unfulfilled field.
cli()/cli_process() set OMNIGRAPH_HOME but inherited OMNIGRAPH_CONFIG (which wins over HOME) and XDG_CONFIG_HOME, so a developer/CI with those exported got non-hermetic tests. env_remove both so the harness fully owns the config-resolution env; tests that exercise global-first still set OMNIGRAPH_CONFIG explicitly per-invocation.
… review fixes Merge `version` now follows the highest loaded-from-file layer (project over global), like every other scalar, instead of 'any layer that set it' — so a legacy project under a v1 global reports the project's (legacy) version. Document the config view --resolved output, the --resolved/--show-origin exclusivity, and the OMNIGRAPH_CONFIG-missing error in cli-reference.md.
ragnorc
changed the title
Contributor
|
Closing for now: this requires several architecture decisions to be settled before actual implementation. We'll revisit once those are resolved. |
This was referenced Jun 9, 2026
Closed
Merged
mrkorchun
pushed a commit
to mrkorchun/omnigraph
that referenced
this pull request
Aug 9, 2026
Terraform-style operator/project split: ~/.omnigraph/config.yaml for identity (operator.actor in the --as cascade), credentials keyed by server name (env -> 0600 credentials file; no inline secrets), and operator-owned named servers that project configs reference but cannot redefine. Explicitly a staged subset of RFC-002: adopts its settled decisions (one dir, keyed credentials, env precedence), defers GraphLocator/use/state-layer, and encodes the ten confirmed PR ModernRelay#139 findings as design rules (compat shims, key-level merges, atomic writes, the project-layer trust boundary). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mrkorchun
pushed a commit
to mrkorchun/omnigraph
that referenced
this pull request
Aug 9, 2026
… (RFC-007 PR 1) ~/.omnigraph/config.yaml joins the resolution chains as the operator surface: operator.actor becomes the last hop of THE actor chain (--as > legacy cli.actor during the RFC-008 window > operator.actor > none, one implementation for direct-engine and cluster commands alike) and defaults.output joins the read-format cascade below every more-specific source. Discovery honors $OMNIGRAPH_HOME (tilde-expanded, ModernRelay#139 finding 9); an absent file is an empty layer; unknown keys WARN and load (a file written for later slices must not break this CLI); malformed YAML is a loud error. The module is CLI-only — the server never reads operator config (invariant 11 by construction). $OMNIGRAPH_CONFIG becomes a first-class stand-in for --config in load_config (flag > env > ./omnigraph.yaml), one meaning in both binaries. The test harness pins hermeticity: spawned binaries get a nonexistent OMNIGRAPH_HOME by default so no test ever reads the developer's real operator config. New coverage: loader unit tests, the env-precedence matrix on load_config_in, and spawned-binary e2es for the actor chain (operator wins with no flag/legacy key; legacy outranks it; --as wins) and the format cascade. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mrkorchun
pushed a commit
to mrkorchun/omnigraph
that referenced
this pull request
Aug 9, 2026
…t (RFC-007 PR 2) The operator config gains servers: (name -> url; never a token). A remote command whose URL prefix-matches an operator server resolves its bearer token through the keyed chain first — OMNIGRAPH_TOKEN_<NAME> env, then the [<name>] section of ~/.omnigraph/credentials (created 0600 via temp+rename, ModernRelay#139 finding 7; group/world-readable files refused loudly) — falling through to the legacy chain unchanged. URL keying makes §D5 rule 3 structural: a token is only ever sent to the server it is keyed to. Longest-prefix matching with a path-boundary check (http://h:8080 never matches http://h:8080-evil). Inserting the keyed hop above the legacy chain is safe by construction — no existing setup can have servers: defined. omnigraph login <name> stores/rotates one section (token from --token or one stdin line — the pipe flow keeps secrets out of shell history); omnigraph logout removes it, idempotently; logging in before declaring the server warns instead of failing (the gh model). Coverage: URL-match/no-substring-trap, credentials round-trip preserving sibling sections, 0600 write + over-permissive refusal, env-name mapping; the legacy resolve test is now hermetic against a real ~/.omnigraph and asserts byte-identical legacy behavior with no servers defined; one spawned-binary e2e walks the whole lifecycle against an authed server: refusal -> wrong-token login (stdin) -> rotate (--token) -> authorized read -> env-beats-file -> non-matching-URL negative -> logout revokes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mrkorchun
pushed a commit
to mrkorchun/omnigraph
that referenced
this pull request
Aug 9, 2026
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 ModernRelay#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 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
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.
Rewrites
docs/dev/rfc-002-config-cli-architecture.md(config & CLI architecture) around a typedGraphLocator, multi-server client targeting, a method×source auth model, global-first layered config, a phased implementation plan, and reserved cloud-multi-tenancy shapes — all validated against the code. It then lands that plan's first phase: extractingomnigraph-config,omnigraph-queries, andomnigraph-api-typesout ofomnigraph-serverand repointingomnigraph-cliat them, so the CLI no longer depends on the HTTP server crate (Axum/tower leave its dependency tree). The extraction is a pure mechanical move — files relocated viagit mv, withomnigraph-serverre-exporting them under their original module names — so the generatedopenapi.jsonis byte-identical and there is no behavior change. Verified: per-crate tests pass (config 12, queries 16, api-types 2), the OpenAPI drift check is clean,cargo tree -p omnigraph-clishows neitheromnigraph-servernoraxum, and the--features awsbuild plus thecheck-agents-md.shdoc cross-link check pass.Note
Medium Risk
Touches central config resolution and CLI graph targeting across local and remote paths; behavior is heavily tested but mis-merged config or locator drift could affect every command and server boot.
Overview
Introduces
omnigraph-config,omnigraph-queries, andomnigraph-api-typesas workspace crates and removesomnigraph-serverfrom the CLI dependency tree.omnigraph-serverre-exports the three crates asconfig,queries, andapiso existing import paths stay stable.omnigraph-configowns YAML loading and resolution:version: 1strictness vs legacy lenience,storage/server/servers/serve/defaults(with legacycli/server/urifolded or warned),GraphLocator(embedded vs remote), global → state → project merging viaload_layered_config, andomnigraph useactive context in~/.omnigraph/state/active.yaml.The CLI loads layered config (warnings on stderr), renames
--targetto--graph, addsomnigraph config view(merged YAML,--show-origin,--resolved) andomnigraph use, and scaffoldsversion: 1configs oninit. Remote vs local dispatch still keys off the locator while preserving prioris_remote_uribehavior (tests pinned).Integration tests isolate
OMNIGRAPH_HOMEso global config does not leak from the developer machine; new tests cover global-only resolution, config introspection, and active-graph selection.Reviewed by Cursor Bugbot for commit d5a0913. Bugbot is set up for automated code reviews on this repo. Configure here.