Skip to content

fix(channels): preserve external policy authority - #10118

Closed
apurvvkumaria wants to merge 20 commits into
codex/9833-snapshot-authorityfrom
codex/9833-channel-authority
Closed

fix(channels): preserve external policy authority#10118
apurvvkumaria wants to merge 20 commits into
codex/9833-snapshot-authorityfrom
codex/9833-channel-authority

Conversation

@apurvvkumaria

Copy link
Copy Markdown
Collaborator

Summary

Standalone messaging channel operations now verify the recorded OpenShell policy authority before policy-sensitive changes. External policy remains owned by its external authority while NemoClaw manages channel state and recovery.

Related Issue

Part of #9833

Changes

  • Qualify channel policy requirements before add, remove, configure, and recovery work.
  • Revalidate authority before policy, credential, forwarding, registry, and success boundaries.
  • Preserve external policy and withhold NemoClaw preset attribution.
  • Cover OpenClaw and Hermes channel lifecycle, drift, rollback, and interrupted recovery.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification:
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — focused channel authority and recovery tests passed
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result:
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Apurv Kumaria akumaria@nvidia.com

Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
@coderabbitai

coderabbitai Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: e72d4d7b-a357-4a05-af6d-9307816a873b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

github-code-quality Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall line coverage in commit c7574ee in the codex/9833-channel-a... branch remains at 96%, unchanged from commit 83fd95b in the codex/9833-snapshot-... branch.


Updated August 28, 2026 07:21 UTC

@wscurran wscurran added area: messaging Messaging channels, bridges, manifests, or channel lifecycle area: policy Network policy, egress rules, presets, or sandbox policy bug-fix PR fixes a bug or regression integration: hermes Hermes integration behavior integration: openclaw OpenClaw integration behavior area: security Security controls, permissions, secrets, or hardening labels Aug 24, 2026
apurvvkumaria and others added 14 commits August 24, 2026 11:53
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
…nto codex/9833-channel-authority

# Conflicts:
#	test/channels/channels-add-preset.test.ts
…authority

Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
…authority

Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>

@cv cv 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.

I found two unresolved state-recovery defects on commit 1fe92d4139e82845fab1537568b235030e79c166:

  1. src/lib/actions/sandbox/policy-channel.ts:1719-1733 can detect an authority refusal after the rebuild, host-forward creation, or bridge-health probe. rollbackChannelAdd removes providers, credentials, policy attribution, and the registry plan, but it does not restore or stop the rebuilt sandbox. The running sandbox can retain the newly enabled bridge process and injected credential after the command reports refusal. Restore the prior runtime state, or prevent this runtime mutation when rollback cannot restore it.

  2. src/lib/actions/sandbox/policy-channel.ts:2324-2346 persists the enabled channel plan before the next authority validation. If authority changes after the persistence callback and before line 2346, the refusal exits without restoring the prior disabled plan. A later rebuild can then enable a channel whose start operation refused. Restore the prior plan after post-persistence refusal, or make the validation and persistence one guarded operation.

Issue #9833 contains the accepted product decision, and the complete review-thread snapshot has no existing threads. DCO passes, and GitHub reports every PR commit as Verified. CodeRabbit skipped its review because this PR targets a non-default branch. PR Advisor did not publish a synthesis. These unavailable automated reviews and the failing checks are not the reason for this decision.

Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>

@senthilr-nv senthilr-nv 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.

Reviewed latest PR commit 1f0106f. Product scope is accepted in #9833. The containment after rebuilt-runtime authority refusal, prior-plan restoration, and live-policy preservation repairs close the state-safety findings. Focused tests and repository checks pass; DCO and every PR commit signature are valid. No blocking findings remain.

@github-actions

Copy link
Copy Markdown
Contributor

PR Review Advisor finished for commit e8694a8. Include the Advisor findings in the complete PR feedback collection. Verify and group valid findings before repair.

All previous runs

@sandl99 sandl99 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.

Requesting changes on reviewed head c7574ee.

Blocking findings:

  1. The candidate does not compile. policy-channel.ts still calls the removed revalidateMessagingProviderAttachmentTarget helper, and gatewayName is referenced outside its declaration scope. Current static-checks and build/type-check CI confirm these undefined symbols.

  2. Provider attachment uses a mutable sandbox name after removing the fail-closed target revalidation. A name rebind can direct messaging credentials to a replacement sandbox instead of the identity that was authorized.

  3. Provider removal uses unscoped runOpenshell rather than the gateway-scoped deleteMessagingProviderWithRecovery path. On a named gateway, NotFound from the default gateway can be treated as successful cleanup, durable channel state can be removed, and the credential provider can remain active on the intended gateway.

  4. The durable external-policy authority model conflicts with accepted issue #10514, which makes OpenShell the sole durable policy authority and explicitly supersedes work preserving external-policy authority. Please retarget the implementation to that accepted direction before further review.

  5. The moved upsertMessagingProviders test asserts a two-argument call, while the production call supplies definitions, gateway name, and options.

Manual PR E2E cannot be requested for this revision because the PR targets codex/9833-snapshot-authority rather than main. After the design and correctness blockers are resolved and the PR targets main, exact-head evidence should be requested.

@apurvvkumaria

Copy link
Copy Markdown
Collaborator Author

Closing this PR because the accepted policy decision in #10514 supersedes #9833. This PR preserves external policy authority across messaging-channel operations, but #10514 removes that durable authority model and makes current OpenShell policy authoritative. Replacement work is in #10515, which delivers the accepted cutover and states that no intermediate authority-stack PR is intended to merge. This closure records supersession. It does not assess the contributor or the implementation quality.

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

Labels

area: messaging Messaging channels, bridges, manifests, or channel lifecycle area: policy Network policy, egress rules, presets, or sandbox policy area: security Security controls, permissions, secrets, or hardening bug-fix PR fixes a bug or regression integration: hermes Hermes integration behavior integration: openclaw OpenClaw integration behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants