refactor(messaging): compact persisted messaging plans#5328
Merged
Conversation
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 (3)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR separates runtime messaging plans (including derived ChangesMessaging Plan Persistence: Separation of Runtime and Disk Representations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorFailed: Could not parse JSON from advisor output; see /home/runner/work/NemoClaw/NemoClaw/artifacts/e2e-advisor/e2e-advisor-raw-output.txt |
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: None Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorFailed: Could not parse JSON from advisor output; see /home/runner/work/NemoClaw/NemoClaw/artifacts/e2e-advisor/e2e-scenario-advisor-raw-output.txt |
Contributor
PR Review AdvisorFindings: 0 needs attention, 1 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
This is an automated advisory review. A human maintainer must make the final merge decision. |
sandl99
added
refactor
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/messaging/compiler/workflow-planner.ts (1)
178-202:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep credential availability parsing aligned with the validated sandbox plan.
readSandboxEntryPlan()rejects persisted plans whosesandboxNameoragentno longer match the current sandbox, butcredentialAvailabilityFromSandboxEntry()ignores those selectors and still merges credential flags from the same raw payload. A stale or mis-associated registry plan can therefore suppress credential prompts for add/rebuild even when the planner refused to trust the plan itself. Pass the same selectors here, or derive availability fromreadSandboxEntryPlan(), so both paths trust the same persisted state.🤖 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 `@src/lib/messaging/compiler/workflow-planner.ts` around lines 178 - 202, credentialAvailabilityFromSandboxEntry currently parses the raw sandboxEntry with parseSandboxMessagingPlan and can return availability from stale plans; change it to use the validated plan path (readSandboxEntryPlan) or call the same selector logic used by readSandboxEntryPlan so only plans whose sandboxName/agent match the current sandbox are trusted — locate credentialAvailabilityFromSandboxEntry, replace the parseSandboxMessagingPlan(sandboxEntry?.messaging?.plan) usage with the validated plan object returned by readSandboxEntryPlan(sandboxEntry, /* provide same selectors/context used elsewhere */) or add the same sandbox/agent checks before merging availability for each credential in registry.get(channelId); ensure you still iterate channelIds and manifest.credentials but derive bindings from the validated plan so availability is only set when readSandboxEntryPlan would accept the plan.
🧹 Nitpick comments (1)
src/lib/state/onboard-session.test.ts (1)
620-624: ⚡ Quick winAssert
hooksnormalization after reload as well.This test proves
hooksare stripped on disk, but the post-loadSession()assertion only checksagentRender. Adding achannels[0].hooks === []assertion would cover the second half of the compact/normalize contract too.✅ Suggested assertion update
const raw = JSON.parse(fs.readFileSync(session.SESSION_FILE, "utf-8")); expect(raw.messagingPlan.agentRender).toBeUndefined(); expect(raw.messagingPlan.channels[0].hooks).toBeUndefined(); - expect(requireLoadedSession(session.loadSession()).messagingPlan?.agentRender).toEqual([]); + const reloadedPlan = requireLoadedSession(session.loadSession()).messagingPlan; + expect(reloadedPlan?.agentRender).toEqual([]); + expect(reloadedPlan?.channels[0]?.hooks).toEqual([]); });🤖 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 `@src/lib/state/onboard-session.test.ts` around lines 620 - 624, The test parses the saved session and already asserts that on-disk messagingPlan.agentRender and channels[0].hooks are undefined but only checks agentRender after reload; update the post-load assertions to also verify hooks normalization by adding an assertion that requireLoadedSession(session.loadSession()).messagingPlan?.channels?.[0]?.hooks equals [] so the reload contract for hooks is covered alongside agentRender (referencing loadSession, requireLoadedSession, messagingPlan, and channels[0].hooks).
🤖 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/state/registry.ts`:
- Around line 319-349: normalizeRegistry/normalizeSandboxEntry dereference
malformed sandbox entries (e.g., null) causing TypeError during registry reads;
update normalizeRegistry (and serializeRegistryForDisk if symmetric behavior is
needed) to skip or sanitize non-object sandbox values before calling
normalizeSandboxEntry/serializeSandboxEntryForDisk by filtering
Object.entries(data.sandboxes ?? {}) to only include entries where typeof entry
=== "object" && entry !== null (or replace broken entries with a safe
default/empty entry), and ensure normalizeSandboxEntry still handles missing
messaging via cloneSandboxMessagingState(entry.messaging) without assuming entry
is an object.
In `@test/registry.test.ts`:
- Around line 245-246: The test callbacks use implicitly-typed parameters;
explicitly annotate them to satisfy TypeScript by typing the iterator parameters
for the arrays. Change the first callback to something like (entry: typeof
hydrated.agentRender[number]) => entry.channelId === "telegram" and the second
to (hook: typeof hydrated.channels[0].hooks[number]) => hook.channelId ===
"telegram", or import the concrete types (e.g., AgentRenderEntry / ChannelHook)
and use (entry: AgentRenderEntry) and (hook: ChannelHook) in the .some callbacks
for hydrated.agentRender and hydrated.channels[0].hooks respectively.
---
Outside diff comments:
In `@src/lib/messaging/compiler/workflow-planner.ts`:
- Around line 178-202: credentialAvailabilityFromSandboxEntry currently parses
the raw sandboxEntry with parseSandboxMessagingPlan and can return availability
from stale plans; change it to use the validated plan path
(readSandboxEntryPlan) or call the same selector logic used by
readSandboxEntryPlan so only plans whose sandboxName/agent match the current
sandbox are trusted — locate credentialAvailabilityFromSandboxEntry, replace the
parseSandboxMessagingPlan(sandboxEntry?.messaging?.plan) usage with the
validated plan object returned by readSandboxEntryPlan(sandboxEntry, /* provide
same selectors/context used elsewhere */) or add the same sandbox/agent checks
before merging availability for each credential in registry.get(channelId);
ensure you still iterate channelIds and manifest.credentials but derive bindings
from the validated plan so availability is only set when readSandboxEntryPlan
would accept the plan.
---
Nitpick comments:
In `@src/lib/state/onboard-session.test.ts`:
- Around line 620-624: The test parses the saved session and already asserts
that on-disk messagingPlan.agentRender and channels[0].hooks are undefined but
only checks agentRender after reload; update the post-load assertions to also
verify hooks normalization by adding an assertion that
requireLoadedSession(session.loadSession()).messagingPlan?.channels?.[0]?.hooks
equals [] so the reload contract for hooks is covered alongside agentRender
(referencing loadSession, requireLoadedSession, messagingPlan, and
channels[0].hooks).
🪄 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: 164fdd91-c4b9-4483-9b6e-ca6e5f1b5c07
📒 Files selected for processing (14)
src/lib/messaging/applier/host-state-applier.tssrc/lib/messaging/compiler/workflow-planner.tssrc/lib/messaging/index.tssrc/lib/messaging/persistence.tssrc/lib/messaging/plan-validation.test.tssrc/lib/messaging/plan-validation.tssrc/lib/onboard/messaging-channel-setup.tssrc/lib/state/onboard-session.test.tssrc/lib/state/onboard-session.tssrc/lib/state/registry-messaging.tssrc/lib/state/registry.tstest/e2e/test-channels-add-remove.shtest/e2e/test-channels-stop-start.shtest/registry.test.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27420277847
|
cv
approved these changes
Jun 13, 2026
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
This PR compacts persisted messaging plans so onboard sessions and the sandbox registry no longer store manifest-derived render and hook metadata. Runtime paths hydrate those fields from built-in messaging manifests before rebuild and resume planning.
Changes
src/lib/messaging/persistence.tsto stripagentRenderandchannels[].hooksfor disk writes and hydrate them for runtime consumers.onboard-session.jsonandsandboxes.jsonwhile keeping registry and session readers compatible with compact persisted plans.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit
Bug Fixes
New Features
Tests