fix(channels): sync session policy preset and registry policy preset#4013
Merged
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a best-effort helper that mirrors registry preset add/remove into the onboard session's ChangesSession Synchronization for Channel and Policy Operations
Sequence Diagram(s)sequenceDiagram
participant CLI
participant PolicyModule as policy-channel
participant PolicyEngine as policies
participant SessionSync as syncSessionPolicyPresetsWithRegistry
participant OnboardSession as onboardSession
CLI->>PolicyModule: add/remove preset
PolicyModule->>PolicyEngine: applyPreset / applyPresetContent / removePreset
PolicyEngine-->>PolicyModule: success
PolicyModule->>SessionSync: sync add/remove
SessionSync->>OnboardSession: loadSession()
OnboardSession-->>SessionSync: session data (or error)
SessionSync->>OnboardSession: updateSession(mutator) if required
OnboardSession-->>SessionSync: saved (or error logged)
SessionSync-->>PolicyModule: complete
PolicyModule-->>CLI: operation complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
PR Review AdvisorRecommendation: blocked This is an automated advisory review. A human maintainer must make the final merge decision. Limitations: This review used the provided trusted deterministic context and the supplied diff; no tests, package-manager commands, workflows, or PR scripts were executed.; PR title/body/comments/issue text were treated as untrusted evidence and mapped only to diff/test evidence.; The git diff in the prompt was truncated for large test files; the review relied on the provided deterministic context and visible diff hunks for production line evidence.; Required E2E jobs were recommended and auto-dispatched for the current head SHA, but no passed result for those job names at 9fcdcc9 was present in the provided context.; Open PR overlap exists on test/channels-add-preset.test.ts and test/policies.test.ts; a maintainer should reconcile drift before merging.; Human maintainer review remains required; this advisory result must not be treated as approval or a merge decision. Full advisor summaryPR Review AdvisorBase: The fix direction and unit coverage look plausible, but merge should wait on current-head required E2E evidence and the enforced sandbox policy-channel monolith growth blocker. Gate status
🔴 Blockers
🟡 Warnings
🔵 Suggestions
Acceptance coverage
Security review
Test / E2E status
✅ What looks good
Review completeness
|
hunglp6d
added
bug
12 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/channels-add-preset.test.ts (1)
384-530: ⚡ Quick winAdd a
channels removesync regression in this suite.This block validates only add-path sync, but the suite contract and fix scope include remove-path session synchronization too. Please add a remove test that asserts successful preset removal updates
session.policyPresetsand keeps best-effort behavior on missing/foreign session.Suggested test shape
describe("channels add/remove keeps session.policyPresets in sync with registry", () => { + it("removes the channel preset from session.policyPresets after a successful remove", () => { + // arrange session with ["npm", "slack"], invoke removeSandboxChannel("slack") + // assert sessionUpdates length === 1 and finalSession.policyPresets === ["npm"] + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/channels-add-preset.test.ts` around lines 384 - 530, Add a new test in the same "channels add/remove keeps session.policyPresets in sync with registry" suite that exercises the remove-path: call ctx.channelModule.removeSandboxChannel("test-sb", { channel: "slack" }) in a script built with buildPreamble and assert that (1) when the onboard session belongs to "test-sb" the remove produces exactly one session update (ctx.sessionUpdates.length === 1) and finalSession.policyPresets no longer contains "slack", (2) when the session belongs to a different sandbox the registry still sees the applied call (ctx.appliedCalls includes { sandboxName: "test-sb", presetName: "slack" }) but ctx.sessionUpdates remains [], and (3) mirror the existing error best-effort cases: when sessionMissing is true the call still applies to registry and when sessionUpdateThrows is true the flow still completes and prompts rebuild (ctx.callOrder includes "promptAndRebuild"); place checks analogous to the add tests using ctx.getSessionState, ctx.sessionUpdates, ctx.appliedCalls, ctx.callOrder, sessionMissing, and sessionUpdateThrows to locate behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/actions/sandbox/policy-channel.ts`:
- Around line 163-165: policies.applyPreset(sandboxName, answer) may fail but
the code always calls syncSessionPolicyPresetsWithRegistry(sandboxName, answer,
"add"); change the flow so you capture the boolean/returned result of
policies.applyPreset(...) and only call
syncSessionPolicyPresetsWithRegistry(...) when that result is truthy/successful;
if applyPreset returns false or throws, skip the sync (and optionally
surface/log the failure) to avoid diverging the session from the registry.
---
Nitpick comments:
In `@test/channels-add-preset.test.ts`:
- Around line 384-530: Add a new test in the same "channels add/remove keeps
session.policyPresets in sync with registry" suite that exercises the
remove-path: call ctx.channelModule.removeSandboxChannel("test-sb", { channel:
"slack" }) in a script built with buildPreamble and assert that (1) when the
onboard session belongs to "test-sb" the remove produces exactly one session
update (ctx.sessionUpdates.length === 1) and finalSession.policyPresets no
longer contains "slack", (2) when the session belongs to a different sandbox the
registry still sees the applied call (ctx.appliedCalls includes { sandboxName:
"test-sb", presetName: "slack" }) but ctx.sessionUpdates remains [], and (3)
mirror the existing error best-effort cases: when sessionMissing is true the
call still applies to registry and when sessionUpdateThrows is true the flow
still completes and prompts rebuild (ctx.callOrder includes "promptAndRebuild");
place checks analogous to the add tests using ctx.getSessionState,
ctx.sessionUpdates, ctx.appliedCalls, ctx.callOrder, sessionMissing, and
sessionUpdateThrows to locate behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5ad07f06-6721-42a5-ae19-e35a62a7b79b
📒 Files selected for processing (3)
src/lib/actions/sandbox/policy-channel.tstest/channels-add-preset.test.tstest/policy-add-remove-session-sync.test.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26282580925
|
…icy-preset-kinds # Conflicts: # src/lib/actions/sandbox/policy-channel.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26296041350
|
ericksoa
approved these changes
May 22, 2026
Contributor
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Approved after topper. The add path now syncs session policy presets only after successful applyPreset and remove-path sync coverage is in place; the follow-up test mock mirrors the production applyPreset success contract. Local focused validations passed and live checks are green at 9fcdcc9.
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26296440535
|
12 tasks
miyoungc
added a commit
that referenced
this pull request
May 22, 2026
Audit found the v0.0.49 release notes promised behaviors that did not ship or were never implemented. Realign to the actual code on main. - Drop the EXDEV runtime-deps claim: #3820 was reverted by #4051 in this release window, so the behavior is not present. - Drop the "skip broad permission repair" claim: no corresponding commit in v0.0.48..v0.0.49. - Rewrite the gateway probe classifier list in release-notes.mdx and commands.mdx to match the real states emitted by src/lib/status-command-deps.ts (named gateway unreachable / present but not Connected / pointing at a different name / not configured). The previous "non-JSON health response" example did not exist in code. - Expand the channel-removal bullet to describe #4001's user-visible teardown (durable QR-paired state wipe, abort-on-failure, config.json re-sync) in addition to the existing #4013 sync. - Add bullets for user-visible PRs that were merged in the release window but missing from the notes: #3854 (restricted dmesg in debug output), #3866 (shields status and logs --tail UX), #3984 (Hermes messaging policy scoping), and #4011 (Docker group security note). Regenerated nemoclaw-user-overview and nemoclaw-user-reference skills from the updated docs via scripts/docs-to-skills.py. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cv
pushed a commit
that referenced
this pull request
May 22, 2026
## Summary Refreshes the NemoClaw docs for the v0.0.49 hardening release, including release notes, command reference updates, troubleshooting guidance, version metadata, and regenerated user skills. ## Changes - #3796, #3854, #3863, #3866, #3984, #4001, #4011, #4013, #4020, #4022, #4023, #4060, #4062 -> `docs/about/release-notes.mdx`: Adds the v0.0.49 hardening release summary covering gateway reliability, status/doctor/shields and debug UX, OpenClaw compatibility, messaging channel teardown, Hermes policy scoping, snapshots, source installs and Docker group security note, GPU preflight, CLI usage, E2E, and CI improvements. - #3796 -> `docs/manage-sandboxes/backup-restore.mdx` and `docs/reference/commands.mdx`: Documents `snapshot restore --to` overwrite protection and the `--force` opt-in. - #3863, #4013, #4020, #4023 -> `docs/reference/commands.mdx`: Documents missing channel argument usage, sandbox-scoped custom preset matching, session policy preset sync, and gateway failure classification (uses the real probe states from `src/lib/status-command-deps.ts`). - #4022, #4060, #4062 -> `docs/reference/troubleshooting.mdx`: Adds guidance for gateway-down `connect`, source checkout OpenShell bootstrapping, WDDM placeholder GPU names, and Jetson sandbox GPU passthrough. - Release prep -> `docs/project.json`, `docs/versions1.json`, `.agents/skills/nemoclaw-user-*`: Bumps docs metadata to 0.0.49 and refreshes generated user skills from the Fern docs. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [x] Doc only (includes code sample changes) ## Verification - [x] `npx prek run --all-files` passes - [ ] `npm test` passes - [ ] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [x] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) \`make docs\` was attempted locally but did not complete because \`npm\` returned \`403 Forbidden\` while fetching \`fern-api\` from \`registry.npmjs.org\` in the sandboxed environment. --- Signed-off-by: Miyoung Choi <miyoungc@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Released v0.0.49 with reliability and compatibility improvements including faster gateway failure diagnostics and safer snapshot restore behavior * Enhanced snapshot restore documentation with `--to` cloning and `--force` overwrite requirements * Expanded troubleshooting guides for source installs, GPU setup, and gateway recovery * Clarified Docker group access requirements and improved CLI command reference * **Chores** * Version bumped to 0.0.49 <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/4078?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/4078?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
channels add <channel>updates the registry's applied preset list but not the onboard session's recorded preset selection. On a subsequentrebuild, the resume-mode onboard reads the stale session, diffs it against the registry, and narrows the channel's preset away just before the new sandbox boots — so the channel bridge starts up against a policy that briefly denies its upstream API, and downstream SDK state (notably Slack Bolt's per-event authorize path) stays wedged even after Step 5.5 of rebuild reapplies the preset from the backup manifest. This PR keepssession.policyPresetsin sync with the registry wheneverchannels add/channels removemutates a built-in preset, so rebuild's resume step no longer sees a divergence to "correct."Related Issue
Fixes #4012
Changes
src/lib/actions/sandbox/policy-channel.tssyncSessionPolicyPresetsForChannel(sandboxName, channelName, action)that mirrorspolicies.applyPreset/policies.removePresetintosession.policyPresetsviaonboardSession.updateSession.applyChannelPresetIfAvailableafter a successful apply (action"add"), and fromremoveChannelPresetIfPresentafter a successful remove (action"remove"). Failure paths leave the session untouched so it cannot diverge from the registry on a half-applied state.test/channels-add-preset.test.tsbuildPreambleto stubonboardSession.loadSession/updateSessionwith configurable session state and configurable load/save failure injection. Existing tests are unchanged because the new options all default to a happy-path session.channels add/remove keeps session.policyPresets in sync with registrywith four cases pinning down the invariants:session.policyPresetsexactly once;channels add;channels addstill completes and prompts rebuild.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Hung Le hple@nvidia.com
Summary by CodeRabbit