fix(policy): compose advisor proposals with provider endpoints - #2935
Conversation
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
|
🌿 Preview your docs: https://nvidia-preview-pr-2935.docs.buildwithfern.com/openshell |
E2E Test AttestationLocal E2E tests passed. CI does not currently run E2E tests, so this comment serves as the verification record.
|
|
Label |
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
This focused fix implements accepted issue #2921, updates the relevant architecture and Fern documentation, and the independent full-diff review found no blocking findings. Required E2E dispatch is blocked because the E2E Label Help bot requires Re-run all jobs on current-head run 32878657519, while this gator sandbox cannot call that rerun endpoint.
Action required: A maintainer must open run 32878657519 and click Re-run all jobs, or the operator must allow this sandbox to POST the exact rerun endpoint.
Blocking findings:
- No blocking findings remain
Carried findings:
- None
Non-blocking suggestions:
- None
Gator metadata
- Validation: Project-valid focused policy fix linked to accepted issue #2921
- Docs: Architecture and Fern policy documentation updated; navigation change not needed
- Checks: Current-head Branch Checks and required Helm gate are green
- E2E:
test:e2eapplied; current-head run 32878657519 requires rerun, but dispatch is blocked by sandbox L7 policy. The narrow policy proposal was rejected because the deployed gateway still treats advisor provenance as conflicting endpoint metadata. - Head SHA:
4de0192abf5b530ad48e15fa231b9ddc5f715598 - Base SHA:
38a94931ffa52f85b493094bcd46ab988016293b - Merge base SHA:
fb6610df3926e5f86ae40e1306bbc5847775e786 - Patch ID:
a13d47d65895c9764297433054a2b2d2fbc5f6e9 - Gator payload:
7 - Review mode:
initial - Previous reviewed SHA: none
- Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:blocked - Blocked reason:
test_dispatch_required
Maintainer Review NudgeThis PR has been in @NVIDIA/openshell-maintainers @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr, can someone review and either approve, request changes, or close this out? |
zredlined
left a comment
There was a problem hiding this comment.
tested against harness, fixes the before seen bug. code looks good
Monitoring CompleteMonitoring is complete because this PR has merged. Final status: The PR reached I removed the active |
| let mut proposed = explicit.clone(); | ||
| proposed.advisor_proposed = true; | ||
|
|
||
| assert!(find_endpoint_ambiguities(&policy_with(explicit, proposed)).is_empty()); |
There was a problem hiding this comment.
This newly valid state exposes a follow-up failure. canonicalize_advisor_add_rule deduplicates effective endpoint contracts using full NetworkEndpoint equality after clearing only provider_credentialed; it leaves advisor_proposed intact. An explicit endpoint and an otherwise identical advisor endpoint therefore count as two contracts, and the next mechanistic proposal for that host/port fails with cannot infer one existing endpoint contract. The server stores that proposal with application_error, so it cannot be auto-approved.
I added a focused reproducer in f966aa33. It fails on this PR head. Contract inference and the later sandbox-owner comparison should ignore provenance, while a newly mirrored overlay should retain its incoming advisor marker.
There was a problem hiding this comment.
i'll follow up w/ a separate fix pr
Summary
Allow policy-advisor proposals to extend provider-covered endpoints when the only difference is internal advisor provenance. Preserve explicit endpoint and binary identity during merges so the change does not weaken exact-host SSRF treatment or leave proposal reload waits stuck.
Related Issue
Fixes #2921
Changes
advisor_proposedas provenance rather than endpoint connection metadata during ambiguity validationpolicy.localproposal shape through pending review and approval, while verifying provider rules remain immutableTesting
mise run pre-commitpassesCommands run:
cargo test -p openshell-policy— 235 passedcargo test -p openshell-supervisor-network proposal_chunks_from_body— 2 passedcargo test -p openshell-server --features test-support— 1,421 passed, 7 ignoredmise run e2e:mechanistic-existing-endpoint— passed against a Docker-backed gateway and sandboxmise run pre-commit— passedChecklist