docs(rfc): RFC-010 verification-comment current-state fixups - #215
Merged
Conversation
Folds in the Codex verification review (kept verbatim with per-point Resolution notes): - `graphs list` is marked remote-only today in the current-state table (the embedded arm bails; it rides GraphClient only to share the resolver). - `init` is noted as positional-URI-only today (no `--target`); adding `--target` to init is part of the proposal, entangled with the init→cluster apply signpost, not current state. - Validated-fact #1 now describes the post-collapse reality (`GraphClient::resolve*`; only the two factories call `apply_server_flag`), dropping the stale "16 call sites" count. - The Authority rule carries a flag-shape caveat: `--graph` is already a global flag requiring `--server`, so the cluster-managed resolver and its flag shape are deferred to a later slice; the illustrative `--cluster <dir> --graph <id>` spelling is marked not-final. Docs-only; no code change. 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.
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.
Applies the Verification comments (Codex) on RFC-010 — current-state accuracy fixups found against the now-merged 3a/3b/3c code. Kept verbatim with per-point Resolution (accepted) notes.
graphs listmarked remote-only today in the current-state table (embedded arm bails; ridesGraphClientonly to share the resolver).initnoted as positional-URI-only today (no--target) — adding--targetis part of the proposal, entangled with theinit→cluster applysignpost, not current state.GraphClient::resolve*; only the two factories callapply_server_flag) — drops the stale "16 call sites" count.--graphis already a global flag requiring--server, so the cluster-managed resolver + its flag shape are deferred to a later slice; the illustrative--cluster <dir> --graph <id>spelling is marked not-final.Docs-only. This lands before the RFC-010 Slice 1 implementation so the RFC's current-state is accurate when the code starts.
🤖 Generated with Claude Code
Greptile Summary
This docs-only PR applies four current-state accuracy fixups to RFC-010 against the now-merged RFC-009 Phases 3a–3c code, each paired with a "Resolution (accepted)" note appended as a Verification comments appendix section.
graphs list/inittable corrections: The current-state plane table now marksgraphs listas remote-only today (with an explanatory note on itsGraphClientrouting) and adds an inline caveat thatinitaccepts only a required positional URI today — neither--targetnor config aliases.apply_server_flag" description is replaced with the post-collapse reality (GraphClient::resolve*; only the two factories callapply_server_flag).--cluster <dir> --graph <id>addressing for cluster-managed maintenance is now explicitly marked "for illustration" and the whole cluster-managed resolver is deferred to a later slice, because--graphis already a global flag requiring--serverwith a different meaning that clap cannot disambiguate.Confidence Score: 4/5
Safe to merge — docs-only change with no code or behavior impact.
All four fixups are accurate and well-reasoned. The one issue is a cross-section inconsistency: the authority rule now marks
--cluster <dir> --graph <id>as not-final and deferred, but the target ergonomics code block still presents that same flag shape as a clean example with no matching qualification. An implementer reading the ergonomics section first would see what looks like a settled flag contract.The target ergonomics section of
docs/dev/rfc-010-cli-planes-restructure.md(the--cluster ./cluster --graph knowledgecode block, ~line 147) should carry a caveat consistent with the flag-shape deferral note added to the authority rule.Important Files Changed
graphs listas remote-only, calls outinit's positional-URI-only addressing today, replaces the stale "16 call sites" count with the post-collapseGraphClient::resolve*description, and adds a flag-shape deferral caveat to the Authority rule. One cross-section inconsistency: the new authority-rule caveat marks--cluster <dir> --graph <id>as not-final/deferred, but the target ergonomics code block still presents that same flag shape as a clean end-state example without qualification.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[CLI invocation] --> B{Which plane?} B -->|Data verb| C[GraphClient resolver] C --> D{--server/--graph or\n--target/positional URI?} D -->|--server prod --graph id| E[GraphClient::resolve*\napply_server_flag in factory] D -->|--target / URI| F[Embedded engine\nor HTTP per config] E --> G[Remote HTTP execution] F --> H[Embedded or Remote\nper target resolution] B -->|Storage/Maintenance verb| I[resolve_uri /\nresolve_local_uri] I --> J{Addressing form?} J -->|positional URI| K[Direct storage\nfile:// or s3://] J -->|--target| L{Resolves to?} L -->|direct storage| K L -->|remote http://| M[Loud error:\nstorage-plane needs\ndirect storage] J -->|init today| N[Required positional URI only\nno --target today] K --> O[Embedded engine\non storage] B -->|Control verb| P[cluster directory\nreader] P --> Q[--config dir\nfile:// or s3://] style M fill:#f88,stroke:#c00 style N fill:#ffd,stroke:#aa0Comments Outside Diff (1)
docs/dev/rfc-010-cli-planes-restructure.md, line 143-148 (link)The PR adds a flag-shape caveat in the Authority rule section (lines 213–220) that explicitly marks
--cluster <dir> --graph <id>as "for illustration" and defers the cluster-managed resolver to a later slice. The target ergonomics section below (this code block plus the error-message example on ~line 140) still presentsomnigraph optimize --cluster ./cluster --graph knowledgeand--cluster <dir> --graph <id>as committed end-state shapes without any matching qualification. An implementer of Slice 1 who reads the ergonomics section first will see what looks like a settled flag contract, then find the deferral signal only much later in the authority rule. Given AGENTS.md rule 6 ("don't lie — replace wrong text rather than leaving it silently incorrect"), a brief inline note on this block mirroring the authority-rule caveat would make the RFC self-consistent.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!
Reviews (1): Last reviewed commit: "docs(rfc): RFC-010 — apply verification-..." | Re-trigger Greptile