refactor(proto): isolate gateway storage messages - #3169
Conversation
politerealism
left a comment
There was a problem hiding this comment.
Reviewed the full diff (excluding the generated Go SDK bindings, which mechanically follow the .proto changes).
Summary: Moves seven server-internal, storage-only protobuf messages out of the public openshell.proto into a new private openshell.storage.v1 package compiled only into openshell-server, so they no longer leak into the public API descriptor set or generated SDK bindings.
What stood out positively:
- The core safety claim — that relocating a message's package is compile-time-only and requires no data migration, since protobuf wire bytes don't encode package names — is proven with real byte fixtures, not just asserted.
storage_proto.rsdecodes checked-in hex payloads produced by the old public-package types using the new private types, and separately round-trips them through an actual on-disk SQLite database via current migrations. - Schema drift is caught by fingerprint tests (SHA-256 over the storage schema, public RPC closure, durable-root closure, and their overlap), each failing with an actionable message telling the developer exactly what to review before updating the hash.
- The nine types that remain deliberately dual-purpose (
Sandbox,Provider,Workspace, etc.) are an explicit, reviewed accept documented inarchitecture/gateway.md, not an oversight — and that overlap is exactly what the fingerprint test guards going forward. storage_types_are_absent_from_public_descriptoris a good explicit negative check, not just an implicit "it compiles" assumption.
No concerns that warrant a change. Solid, well-tested refactor.
LGTM.
5f3daa1 to
507c3fc
Compare
BlockedGator is blocked because GitHub reports this PR has merge conflicts with Next action: @varshaprasad96, rebase or merge the latest Gator metadata
|
Move persistence-only protobufs into a server-private versioned package, remove them from generated public SDKs, and gate durable/public schema compatibility with legacy database fixtures. Closes NVIDIA#3053 Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
507c3fc to
58f63b2
Compare
|
PR has been rebased with main, and should be good for testing. |
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @varshaprasad96—the latest head resolves the earlier merge conflict. The implementation is project-valid and the storage boundary is well scoped, but the new canonical architecture inventory does not match the descriptor-derived guard added by this same PR.
Action required: update architecture/gateway.md to match the reviewed head’s actual public and durable schema inventory, then push a new commit.
Blocking findings:
GATOR-58f63b21-01: the documented RPC and durable-root inventory contradicts the executable compatibility guard.
Carried findings:
- None
Gator metadata
- Validation: Implements the focused, previously investigated API/storage separation in linked issue #3053.
- Docs: Architecture documentation is present but has a blocking inventory mismatch; Fern docs are not needed because this does not change a direct user workflow.
- Checks: Current-head required checks are waiting for test dispatch; DCO is green.
- E2E: Decision deferred until blocking review feedback is resolved.
- Head SHA:
58f63b2166d0fcdb8cd506373a350d529f558a6d - Base SHA:
a0814443f19c07102b19ff09d6ead3d3ba59f9c5 - Merge base SHA:
ea8eda6d5b2eba410ed3ce0d1b272b4f17b927eb - Patch ID:
d12923f16ba83a832219c41ae3e0f46c96eff020 - Gator payload:
8 - Review mode:
initial - Previous reviewed SHA: none
- Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
|
Label |
|
/ok to test 469b8ef |
|
Label |
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @varshaprasad96—I checked current head 469b8efc312174394cc669e52c01711d3d434633 after the architecture inventory update. The documented RPC counts, durable roots, and fingerprints now match the descriptor-derived guard, so GATOR-58f63b21-01 is resolved and no code-review blockers remain.
Test dispatch is temporarily blocked: E2E Label Help requires Re-run all jobs for current-head Branch E2E run 34421167187, but GitHub will not accept the authorized rerun while its first attempt is still queued. No author action is needed; Gator will retry the rerun after this attempt finishes.
Blocking findings:
- No blocking findings remain
Carried findings:
- None
Gator metadata
- Validation: Implements the focused API/storage separation tracked by linked issue #3053.
- Docs:
architecture/gateway.mdnow matches the executable schema inventory; Fern docs are not needed because this does not change a direct user workflow. - Checks: Current-head Branch Checks and E2E are pending; the bot-required rerun has not yet been accepted.
- E2E:
test:e2eapplied; mirror authorized for the current head; run34421167187must be rerun when GitHub permits it. - Head SHA:
469b8efc312174394cc669e52c01711d3d434633 - Base SHA:
a0814443f19c07102b19ff09d6ead3d3ba59f9c5 - Merge base SHA:
ea8eda6d5b2eba410ed3ce0d1b272b4f17b927eb - Patch ID:
15450cd742c0951dab013d4e0ccf1e56430f4d84 - Gator payload:
8 - Review mode:
follow_up - Previous reviewed SHA:
58f63b2166d0fcdb8cd506373a350d529f558a6d - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:blocked - Blocked reason:
test_dispatch_required
Summary
Separates persistence-only protobuf messages from OpenShell's public API packages. The gateway now owns a private, versioned storage package while preserving existing payload bytes and stable database object routing.
Related Issue
Closes #3053
Changes
openshell.storage.v1package and compile them only for the gateway.Deviations from Plan
None — implemented as planned.
Testing
mise run pre-commitpassesmise run testpassesmise run cipassesTests added:
e2e/changed.Checklist
Documentation updated:
architecture/gateway.mdrecords storage classifications, per-root twin decisions, schema inventories, and compatibility policy.