Skip to content

refactor(policy)!: remove NetworkBinary harness field - #3222

Open
varshaprasad96 wants to merge 2 commits into
NVIDIA:mainfrom
varshaprasad96:refactor/3054-remove-networkbinary-harness/varshaprasad96
Open

refactor(policy)!: remove NetworkBinary harness field#3222
varshaprasad96 wants to merge 2 commits into
NVIDIA:mainfrom
varshaprasad96:refactor/3054-remove-networkbinary-harness/varshaprasad96

Conversation

@varshaprasad96

Copy link
Copy Markdown
Contributor

Summary

Remove the deprecated NetworkBinary.harness field before 0.1.0 while preserving advisor-origin provenance at the endpoint/rule boundary.

Related Issue

Closes #3054

Changes

  • Reserve protobuf field number 2 and name harness, update generated Go bindings, and remove runtime, SDK, and profile uses.
  • Reject legacy YAML/profile harness input with migration guidance while retaining protobuf wire compatibility.
  • Move advisor provenance enforcement to gateway-stamped endpoints and keep advisor binaries separated from explicit rules, including rule-name collisions.
  • Migrate legacy stored policy-history provenance without dropping unknown nested wire fields.
  • Update policy architecture, provider profile docs, and 0.1.0 migration notes.

Testing

  • mise run pre-commit
  • cargo test --workspace --exclude openshell-server -q
  • cargo test -p openshell-server --lib -q
  • cargo test -p openshell-supervisor-network --lib -q
  • cargo test -p openshell-sandbox --lib -q
  • mise run go:ci
  • mise run sdk:ts:ci
  • mise run test:python
  • Focused policy/profile/protobuf migration and provenance regressions.
  • mise run e2e:docker — blocked before execution because the local Docker daemon was unreachable.

Checklist

Closes NVIDIA#3054

Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Sep 8, 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.

@gmenher

gmenher commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

@varshaprasad96 I did a manual validation and it works as explained, scalar provider profile lint/import/export/delete works against a local Docker gateway; legacy harness profiles are rejected with the intended migration diagnostic; sandbox creation works.
I also ran the Docker E2E suiteand the other tests. The ambiguous-policy behavior was correct: the update was rejected and the active policy was preserved. One existing assertion outside this PR's diff was sensitive to diagnostic line wrapping, so I treated that as an unrelated test-harness issue during testing.
Only one non blocking question, would it make sense to add a regression test to confirm that migrating legacy payloads preserves unrelated unknown nested protobuf fields? I think it could help protect the forward-compatibility path.

@johntmyers johntmyers added gator:in-review Gator is reviewing or awaiting PR review feedback test:e2e Requires end-to-end coverage labels Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

Label test:e2e applied, but pull-request/3222 does not exist yet. A maintainer needs to comment /ok to test 28495d8803b4380252435ea0d72fed565bbb5611 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 28495d8

@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

This implementation matches the linked removal and migration scope, preserves advisor-origin provenance at the endpoint boundary, and has no blocking findings from the initial code review. Required Branch Checks and E2E workflows are running for the current head, so Gator will monitor them before requesting maintainer approval.

Blocking findings:

  • No blocking findings remain

Carried findings:

  • None

Non-blocking suggestions:

  • None
Gator metadata
  • Validation: Project-valid implementation of linked issue #3054
  • Docs: Architecture, provider profile, and 0.1.0 migration documentation updated
  • Checks: Current-head Branch Checks and E2E are in progress; Helm Lint is green
  • E2E: test:e2e applied, mirror authorization posted, and Branch E2E Checks running
  • Head SHA: 28495d8803b4380252435ea0d72fed565bbb5611
  • Base SHA: e1084e19db622fd1134454d3b589aab2022aaddf
  • Merge base SHA: 2ad86c1b2e3e81a26e0fd38ec860398acb31f2fc
  • Patch ID: 7e8e9eb0678cc383090be852ce40f199d97608ca
  • Gator payload: 8
  • Review mode: initial
  • Previous reviewed SHA: none
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:watch-pipeline

@johntmyers johntmyers added gator:watch-pipeline Gator is monitoring PR CI/CD status gator:in-review Gator is reviewing or awaiting PR review feedback and removed gator:in-review Gator is reviewing or awaiting PR review feedback gator:watch-pipeline Gator is monitoring PR CI/CD status labels Sep 9, 2026
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
@varshaprasad96

varshaprasad96 commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @gmenher, have added in 758f64309.

The stored-policy compatibility regression adds an unrelated future field (tag 99) alongside the former harness tag, round-trips the payload through policy_record_from_partspolicy_payload_from_recordpolicy_record_from_parts, and verifies that both fields survive before the legacy provenance migration runs. It then confirms the migrated policy still splits explicit and advisor binaries correctly.

Validation steps:

  • cargo test -p openshell-server --lib stored_policy_decode_migrates_legacy_provenance_and_preserves_unknown_fields
  • mise run pre-commit

@johntmyers

Copy link
Copy Markdown
Collaborator

/ok to test 758f643

@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

Re-check After Author Update

Thanks @varshaprasad96. I checked the regression test added in 758f64309 after your September 9 response: it exercises an unrelated future nested protobuf field through the stored-policy record/payload round trip and then confirms the legacy provenance migration still separates explicit and advisor binaries correctly. The focused follow-up review found no blocking issues. Required current-head Branch E2E Checks are now queued and running.

Disposition: resolved.

Remaining items:

  • No blocking items remain
Gator metadata
  • Validation: Project-valid implementation of linked issue #3054
  • Docs: Architecture, provider profile, and 0.1.0 migration documentation were updated in the reviewed patch; this follow-up is test-only
  • Checks: Current-head required Branch Checks, Helm Lint, and E2E status contexts are pending
  • E2E: test:e2e is applied; current-head mirror authorization was posted and Branch E2E Checks run 34410134126 is queued/running
  • Head SHA: 758f64309b497cbbc06779dc695d73b6b0cae28f
  • Base SHA: e1084e19db622fd1134454d3b589aab2022aaddf
  • Merge base SHA: 2ad86c1b2e3e81a26e0fd38ec860398acb31f2fc
  • Patch ID: b5d4531b4160b71369e101b89a00d980584b4346
  • Gator payload: 8
  • Review mode: follow_up
  • Previous reviewed SHA: 28495d8803b4380252435ea0d72fed565bbb5611
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:watch-pipeline

@johntmyers johntmyers added gator:watch-pipeline Gator is monitoring PR CI/CD status gator:in-review Gator is reviewing or awaiting PR review feedback and removed gator:in-review Gator is reviewing or awaiting PR review feedback gator:watch-pipeline Gator is monitoring PR CI/CD status labels Sep 9, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator

@varshaprasad96 looks like some tests need to be fixed, if you could take a look then we should be able to prep to merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:in-review Gator is reviewing or awaiting PR review feedback test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(policy)!: remove deprecated NetworkBinary.harness

3 participants