Skip to content

refactor(proto): isolate gateway storage messages - #3169

Open
varshaprasad96 wants to merge 2 commits into
NVIDIA:mainfrom
varshaprasad96:refactor/3053-separate-storage-proto/varshaprasad96
Open

refactor(proto): isolate gateway storage messages#3169
varshaprasad96 wants to merge 2 commits into
NVIDIA:mainfrom
varshaprasad96:refactor/3053-separate-storage-proto/varshaprasad96

Conversation

@varshaprasad96

@varshaprasad96 varshaprasad96 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

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

  • Move seven storage-only messages into the server-private openshell.storage.v1 package and compile them only for the gateway.
  • Remove storage-only symbols from public descriptors and regenerated Go SDK bindings; Python and TypeScript generation remain public-only.
  • Add descriptor-derived gates for all 77 public RPCs, 13 durable roots, their transitive schemas, and their exact overlap.
  • Add v0.0.116 payload fixtures that survive an on-disk SQLite reopen, current migrations, and current persistence loaders byte-for-byte.
  • Document every dual-purpose persisted root and the separate public-wire/storage-migration compatibility policies.

Deviations from Plan

None — implemented as planned.

Testing

  • mise run pre-commit passes
  • mise run test passes
  • mise run ci passes
  • Unit tests added/updated
  • E2E tests added/updated (not applicable; no runtime, sandbox, policy, or infrastructure path changed)

Tests added:

  • Unit: private storage schema freeze, public SDK/descriptor exclusion, public/durable closure inventories, and v0.0.116 semantic decode coverage.
  • Integration: on-disk SQLite fixture reopen through current migrations and persistence loaders with exact payload preservation.
  • E2E: N/A; no files under e2e/ changed.

Checklist

Documentation updated:

  • architecture/gateway.md records storage classifications, per-root twin decisions, schema inventories, and compatibility policy.

@copy-pr-bot

copy-pr-bot Bot commented Sep 3, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@politerealism politerealism left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.rs decodes 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 in architecture/gateway.md, not an oversight — and that overlap is exactly what the fingerprint test guards going forward.
  • storage_types_are_absent_from_public_descriptor is a good explicit negative check, not just an implicit "it compiles" assumption.

No concerns that warrant a change. Solid, well-tested refactor.

LGTM.

@varshaprasad96
varshaprasad96 force-pushed the refactor/3053-separate-storage-proto/varshaprasad96 branch from 5f3daa1 to 507c3fc Compare September 8, 2026 19:51
@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Blocked

Gator is blocked because GitHub reports this PR has merge conflicts with main. Code review and current-head test dispatch cannot proceed reliably until the branch is mergeable.

Next action: @varshaprasad96, rebase or merge the latest main into this branch and resolve the conflicts, then push the updated branch.

Gator metadata
  • Head SHA: 507c3fc57c34023fcf6617be7a27ff129619176a
  • Base SHA: bb210ee9dc6823d3e0a25774c2ef5060d430fa79
  • Merge base SHA: e1084e19db622fd1134454d3b589aab2022aaddf
  • Patch ID: e05e2bb10b1010d0ae01fc7802eee00060cc2982
  • Gator payload: 8
  • Next state: gator:blocked
  • Blocked reason: merge_conflict

@johntmyers johntmyers added the gator:blocked Gator is blocked by process or repository gates label Sep 9, 2026
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>
@varshaprasad96
varshaprasad96 force-pushed the refactor/3053-separate-storage-proto/varshaprasad96 branch from 507c3fc to 58f63b2 Compare September 9, 2026 23:50
@varshaprasad96

Copy link
Copy Markdown
Contributor Author

PR has been rebased with main, and should be good for testing.

@johntmyers johntmyers left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread architecture/gateway.md Outdated
@johntmyers johntmyers added gator:in-review Gator is reviewing or awaiting PR review feedback and removed gator:blocked Gator is blocked by process or repository gates labels Sep 10, 2026
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
@johntmyers johntmyers added the test:e2e Requires end-to-end coverage label Sep 10, 2026
@github-actions

Copy link
Copy Markdown

Label test:e2e applied, but pull-request/3169 does not exist yet. A maintainer needs to comment /ok to test 469b8efc312174394cc669e52c01711d3d434633 to mirror this PR. Once the mirror exists, re-apply the label or re-run Branch E2E Checks from the Actions tab.

@johntmyers

Copy link
Copy Markdown
Collaborator

/ok to test 469b8ef

@johntmyers johntmyers added test:e2e Requires end-to-end coverage and removed test:e2e Requires end-to-end coverage labels Sep 10, 2026
@github-actions

Copy link
Copy Markdown

Label test:e2e applied for 469b8ef. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

@johntmyers johntmyers left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.md now 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:e2e applied; mirror authorized for the current head; run 34421167187 must 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

@johntmyers johntmyers added gator:blocked Gator is blocked by process or repository gates gator:approval-needed Gator completed review; maintainer approval needed and removed gator:in-review Gator is reviewing or awaiting PR review feedback gator:blocked Gator is blocked by process or repository gates labels Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:approval-needed Gator completed review; maintainer approval needed test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(proto): separate persistence-only messages from public API packages

3 participants