feat(onboard): apply startup profiles in managed images - #7961
Merged
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThis change adds managed startup environment normalization, application runtime planning, secure image-runtime execution, shared messaging selectors, and managed post-agent-install behavior. It also forwards OpenClaw fast auto-pair controls and expands validation coverage. ChangesManaged startup environment and entrypoint
Messaging build behavior
OpenClaw runtime forwarding
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant EntrypointWrapper
participant ManagedStartupImageRuntime
participant AgentEnvironmentMapper
participant MessagingBuildApplier
EntrypointWrapper->>ManagedStartupImageRuntime: Pass normalized startup command and environment
ManagedStartupImageRuntime->>AgentEnvironmentMapper: Map profile and application runtime inputs
AgentEnvironmentMapper-->>ManagedStartupImageRuntime: Return export and cleanup plan
ManagedStartupImageRuntime->>MessagingBuildApplier: Apply post-agent-install managed runtime
MessagingBuildApplier-->>ManagedStartupImageRuntime: Restore messaging configuration
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
ericksoa
added
area: onboarding
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 13932b9 in the TypeScript / code-coverage/cliThe overall coverage in commit 13932b9 in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 3 optional E2E recommendations
Blockers
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Preserve the exact reviewed tree while moving the stacked base to merged PR3.3. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Preserve the exact reviewed tree while moving the stacked base to restacked PR3.4a. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Carry the reviewed PR3.4b slice unchanged onto the CodeRabbit feedback fix for PR3.4a. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![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: 4
🧹 Nitpick comments (3)
test/messaging-build-applier.test.ts (1)
1276-1298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the managed-runtime rejection paths.
The managed happy path is covered. The two new guards are not. Add one test that calls
applyMessagingBuildPhasewithmanagedStartupRuntime: trueand a phase other thanpost-agent-install, and one test that runs the CLI with--managed-startup-runtimeand--phase runtime-setup. Assert the specific error text in each case. These tests keep managed mode restricted topost-agent-install.🤖 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/messaging-build-applier.test.ts` around lines 1276 - 1298, Add coverage for both managed-runtime rejection paths: directly call applyMessagingBuildPhase with managedStartupRuntime: true and a non-post-agent-install phase, asserting the specific rejection error text, and run the CLI with --managed-startup-runtime and --phase runtime-setup, asserting its specific error output. Keep the existing managed post-agent-install success test unchanged.test/entrypoint-env-wrapper.test.ts (1)
118-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
HELPERfor the normalizer path.sliceBlockalready throws when either marker is missing.🤖 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/entrypoint-env-wrapper.test.ts` around lines 118 - 133, Update the test snippet construction to reuse the existing HELPER symbol for the normalizer path instead of independently reading entrypoint-env-wrapper.sh with fs.readFileSync and path.join. Keep sliceBlock responsible for extracting the block and preserving its existing missing-marker validation.src/lib/onboard/managed-startup/image-runtime.ts (1)
1024-1037: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse one comparator for both sorted blocks.
unsetLinessorts with the defaultArray.prototype.sort(UTF-16 code-unit order).exportLinessorts withlocaleCompare.localeComparedepends on the active ICU locale, so the export order can differ between build hosts even for the same input. The file is a deterministic artifact, so use the same code-unit comparator in both places.♻️ Proposed comparator alignment
const exportLines = Object.entries(output) - .sort(([left], [right]) => left.localeCompare(right)) + .sort(([left], [right]) => (left < right ? -1 : left > right ? 1 : 0)) .map(([name, value]) => {The same comparator appears in
validateManagedStartupApplicationRuntimePlanat Line 192; align it too if you take this change.🤖 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/onboard/managed-startup/image-runtime.ts` around lines 1024 - 1037, Use a shared deterministic code-unit comparator for sorting both unsetNames in unsetLines and Object.entries(output) in exportLines, replacing localeCompare so artifact ordering is host-independent. Also update validateManagedStartupApplicationRuntimePlan to use the same comparator, reusing the existing comparator symbol if available rather than defining inconsistent sorting logic.
🤖 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/messaging/post-agent-install-selection.ts`:
- Around line 24-77: Extract a shared channel-id normalizer from
selectActiveMessagingChannelIds and use it consistently in
selectEnabledMessagingAgentRender and selectEnabledPostAgentInstallBuildFiles.
Normalize render.channelId and step.channelId before checking membership, and
normalize both channel.channelId and step.channelId when locating the matching
channel and hook phase, while preserving the existing filtering behavior.
- Around line 4-38: Update selectActiveMessagingChannelIds to return
enabledPlanChannels(plan).map(({ channelId }) => channelId), ensuring
plan.disabledChannels is respected. Remove the duplicate local interfaces and
channel-selection logic, while preserving the helper’s canonical enabled-channel
ordering.
In `@src/lib/onboard/managed-startup/image-runtime.ts`:
- Around line 1278-1283: Update the CLI entry check around main so it does not
access require.main when require is unavailable under Vitest’s ESM runner; guard
require before evaluating the entry condition, while preserving the existing
CommonJS behavior and error handling.
In `@test/entrypoint-env-wrapper.test.ts`:
- Around line 156-161: Update runScenario to remove NEMOCLAW_DASHBOARD_PORT and
CHAT_UI_URL from the inherited process environment before spreading extraEnv.
Preserve all other ambient variables and ensure extraEnv is applied last so each
port scenario controls these values deterministically.
---
Nitpick comments:
In `@src/lib/onboard/managed-startup/image-runtime.ts`:
- Around line 1024-1037: Use a shared deterministic code-unit comparator for
sorting both unsetNames in unsetLines and Object.entries(output) in exportLines,
replacing localeCompare so artifact ordering is host-independent. Also update
validateManagedStartupApplicationRuntimePlan to use the same comparator, reusing
the existing comparator symbol if available rather than defining inconsistent
sorting logic.
In `@test/entrypoint-env-wrapper.test.ts`:
- Around line 118-133: Update the test snippet construction to reuse the
existing HELPER symbol for the normalizer path instead of independently reading
entrypoint-env-wrapper.sh with fs.readFileSync and path.join. Keep sliceBlock
responsible for extracting the block and preserving its existing missing-marker
validation.
In `@test/messaging-build-applier.test.ts`:
- Around line 1276-1298: Add coverage for both managed-runtime rejection paths:
directly call applyMessagingBuildPhase with managedStartupRuntime: true and a
non-post-agent-install phase, asserting the specific rejection error text, and
run the CLI with --managed-startup-runtime and --phase runtime-setup, asserting
its specific error output. Keep the existing managed post-agent-install success
test unchanged.
🪄 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: b58ebc9a-7701-42fa-844c-464466c27095
📒 Files selected for processing (14)
ci/env-var-doc-allowlist.jsonscripts/lib/entrypoint-env-wrapper.shsrc/lib/messaging/applier/build/messaging-build-applier.mtssrc/lib/messaging/post-agent-install-selection.tssrc/lib/onboard/managed-startup-agent-environment.test.tssrc/lib/onboard/managed-startup-image-runtime.test.tssrc/lib/onboard/managed-startup-profile.test.tssrc/lib/onboard/managed-startup/agent-environment.tssrc/lib/onboard/managed-startup/image-runtime.tssrc/lib/onboard/managed-startup/profile.tssrc/lib/onboard/sandbox-create-launch.test.tssrc/lib/onboard/sandbox-create-launch.tstest/entrypoint-env-wrapper.test.tstest/messaging-build-applier.test.ts
Comment on lines
+4
to
+38
| interface SelectionHook { | ||
| readonly id: string; | ||
| readonly phase: string; | ||
| } | ||
|
|
||
| interface SelectionChannel { | ||
| readonly channelId: string; | ||
| readonly active?: boolean; | ||
| readonly disabled?: boolean; | ||
| readonly hooks?: readonly SelectionHook[]; | ||
| } | ||
|
|
||
| interface SelectionPlanBase { | ||
| readonly channels: readonly SelectionChannel[]; | ||
| } | ||
|
|
||
| /** | ||
| * Canonical active-channel selection for the image applier. Each selection | ||
| * consumer must resolve the same active channels and mutable outputs. | ||
| */ | ||
| export function selectActiveMessagingChannelIds(plan: SelectionPlanBase): string[] { | ||
| const seen = new Set<string>(); | ||
| const channels: string[] = []; | ||
| for (const item of plan.channels) { | ||
| const channel = String(item.channelId || "") | ||
| .trim() | ||
| .toLowerCase(); | ||
| if (!channel || seen.has(channel)) continue; | ||
| if (item.active === true && item.disabled !== true) { | ||
| seen.add(channel); | ||
| channels.push(channel); | ||
| } | ||
| } | ||
| return channels; | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the existing enabled-channel helpers and compare their filter semantics.
set -uo pipefail
rg -nP --type=ts -C 12 '\b(enabledPlanChannels|filterEnabledPlanEntries)\s*(<|\()' src/lib/messaging
rg -nP --type=ts -C 4 '\b(enabledPlanChannels|filterEnabledPlanEntries)\s*\(' src | rg -v '\.test\.'Repository: NVIDIA/NemoClaw
Length of output: 23824
🏁 Script executed:
#!/bin/bash
set -uo pipefail
printf '%s\n' '--- target file ---'
cat -n src/lib/messaging/post-agent-install-selection.ts
printf '%s\n' '--- usages and imports ---'
rg -n -C 8 'selectActiveMessagingChannelIds|post-agent-install-selection|getActiveChannelIdsFromPlan|disabledChannels' src/lib/messaging
printf '%s\n' '--- manifest declarations ---'
rg -n -C 8 'interface SandboxMessaging(ChannelPlan|Plan)|type SandboxMessaging(ChannelPlan|Plan)|disabledChannels' src/lib/messaging/manifest*Repository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -uo pipefail
printf '%s\n' '--- target imports/usages outside its declaration ---'
rg -n 'post-agent-install-selection|selectActiveMessagingChannelIds|selectEnabledMessagingAgentRender|selectEnabledPostAgentInstallBuildFiles' --glob '!src/lib/messaging/post-agent-install-selection.ts' .
printf '%s\n' '--- shared filter and plan types ---'
cat -n src/lib/messaging/applier/plan-filter.ts
rg -n -C 4 'export interface SandboxMessaging(ChannelPlan|Plan)|export type MessagingChannelId|disabledChannels:' src/lib/messaging/manifest.ts
printf '%s\n' '--- all active-channel helper definitions ---'
rg -n -C 6 'function (getActiveChannelIdsFromPlan|enabledPlanChannels|selectActiveMessagingChannelIds)|export function (getActiveChannelIdsFromPlan|enabledPlanChannels|selectActiveMessagingChannelIds)' src/lib/messagingRepository: NVIDIA/NemoClaw
Length of output: 6881
🏁 Script executed:
#!/bin/bash
set -uo pipefail
printf '%s\n' '--- build-applier call sites ---'
sed -n '330,360p' src/lib/messaging/applier/build/messaging-build-applier.mts
sed -n '950,990p' src/lib/messaging/applier/build/messaging-build-applier.mts
printf '%s\n' '--- behavioral comparison ---'
python3 - <<'PY'
def local_selector(plan):
seen, out = set(), []
for item in plan["channels"]:
channel = str(item.get("channelId") or "").strip().lower()
if not channel or channel in seen:
continue
if item.get("active") is True and item.get("disabled") is not True:
seen.add(channel)
out.append(channel)
return out
def shared_selector(plan):
disabled = set(plan["disabledChannels"])
return [
item["channelId"]
for item in plan["channels"]
if item.get("active") and not item.get("disabled") and item["channelId"] not in disabled
]
cases = [
{
"name": "disabledChannels-only exclusion",
"plan": {
"channels": [{"channelId": "telegram", "active": True, "disabled": False}],
"disabledChannels": ["telegram"],
},
},
{
"name": "normal active channel",
"plan": {
"channels": [{"channelId": "telegram", "active": True, "disabled": False}],
"disabledChannels": [],
},
},
]
for case in cases:
local = local_selector(case["plan"])
shared = shared_selector(case["plan"])
print(case["name"], "local=", local, "shared=", shared, "different=", local != shared)
PYRepository: NVIDIA/NemoClaw
Length of output: 2639
Use the shared enabled-channel helper.
selectActiveMessagingChannelIds() ignores plan.disabledChannels, so build rendering can apply effects for disabled channels. Use enabledPlanChannels(plan).map(({ channelId }) => channelId) and remove the duplicate local shapes.
🤖 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/post-agent-install-selection.ts` around lines 4 - 38,
Update selectActiveMessagingChannelIds to return
enabledPlanChannels(plan).map(({ channelId }) => channelId), ensuring
plan.disabledChannels is respected. Remove the duplicate local interfaces and
channel-selection logic, while preserving the helper’s canonical enabled-channel
ordering.
Source: Coding guidelines
Contributor
Author
There was a problem hiding this comment.
Contributor
There was a problem hiding this comment.
@ericksoa, acknowledged. The fix is in stacked follow-up #7976. I will keep this thread open until the exact-head GitHub qualification completes successfully.
You are interacting with an AI system.
Comment on lines
+24
to
+77
| export function selectActiveMessagingChannelIds(plan: SelectionPlanBase): string[] { | ||
| const seen = new Set<string>(); | ||
| const channels: string[] = []; | ||
| for (const item of plan.channels) { | ||
| const channel = String(item.channelId || "") | ||
| .trim() | ||
| .toLowerCase(); | ||
| if (!channel || seen.has(channel)) continue; | ||
| if (item.active === true && item.disabled !== true) { | ||
| seen.add(channel); | ||
| channels.push(channel); | ||
| } | ||
| } | ||
| return channels; | ||
| } | ||
|
|
||
| export function selectEnabledMessagingAgentRender< | ||
| Render extends { | ||
| readonly agent: string; | ||
| readonly channelId: string; | ||
| }, | ||
| >( | ||
| plan: SelectionPlanBase & { | ||
| readonly agent: string; | ||
| readonly agentRender: readonly Render[]; | ||
| }, | ||
| ): Render[] { | ||
| const active = new Set(selectActiveMessagingChannelIds(plan)); | ||
| return plan.agentRender.filter( | ||
| (render) => render.agent === plan.agent && active.has(render.channelId), | ||
| ); | ||
| } | ||
|
|
||
| export function selectEnabledPostAgentInstallBuildFiles< | ||
| Step extends { | ||
| readonly channelId: string; | ||
| readonly kind: string; | ||
| readonly hookId?: string; | ||
| }, | ||
| >( | ||
| plan: SelectionPlanBase & { | ||
| readonly buildSteps: readonly Step[]; | ||
| }, | ||
| ): Step[] { | ||
| const active = new Set(selectActiveMessagingChannelIds(plan)); | ||
| return plan.buildSteps.filter((step) => { | ||
| if (!active.has(step.channelId) || step.kind !== "build-file") return false; | ||
| if (!step.hookId) return true; | ||
| const hookPhase = plan.channels | ||
| .find((channel) => channel.channelId === step.channelId) | ||
| ?.hooks?.find((hook) => hook.id === step.hookId)?.phase; | ||
| return hookPhase === undefined || hookPhase === "post-agent-install"; | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Normalize channel ids on both sides of every comparison.
selectActiveMessagingChannelIds stores trimmed, lowercased ids at lines 28-30. The consumers compare raw ids: line 53 (active.has(render.channelId)), line 70 (active.has(step.channelId)), and line 73 (channel.channelId === step.channelId). If a plan carries a channel id with different case or surrounding whitespace in agentRender or buildSteps, the selection returns empty and the render and build-file effects are dropped without an error. Extract one normalizer and apply it to both sides.
🐛 Proposed fix for asymmetric normalization
+function normalizeChannelId(channelId: string): string {
+ return channelId.trim().toLowerCase();
+}
+
export function selectActiveMessagingChannelIds(plan: SelectionPlanBase): string[] {
const seen = new Set<string>();
const channels: string[] = [];
for (const item of plan.channels) {
- const channel = String(item.channelId || "")
- .trim()
- .toLowerCase();
+ const channel = normalizeChannelId(item.channelId ?? ""); const active = new Set(selectActiveMessagingChannelIds(plan));
return plan.agentRender.filter(
- (render) => render.agent === plan.agent && active.has(render.channelId),
+ (render) => render.agent === plan.agent && active.has(normalizeChannelId(render.channelId)),
); const active = new Set(selectActiveMessagingChannelIds(plan));
return plan.buildSteps.filter((step) => {
- if (!active.has(step.channelId) || step.kind !== "build-file") return false;
+ const channelId = normalizeChannelId(step.channelId);
+ if (!active.has(channelId) || step.kind !== "build-file") return false;
if (!step.hookId) return true;
const hookPhase = plan.channels
- .find((channel) => channel.channelId === step.channelId)
+ .find((channel) => normalizeChannelId(channel.channelId) === channelId)
?.hooks?.find((hook) => hook.id === step.hookId)?.phase;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function selectActiveMessagingChannelIds(plan: SelectionPlanBase): string[] { | |
| const seen = new Set<string>(); | |
| const channels: string[] = []; | |
| for (const item of plan.channels) { | |
| const channel = String(item.channelId || "") | |
| .trim() | |
| .toLowerCase(); | |
| if (!channel || seen.has(channel)) continue; | |
| if (item.active === true && item.disabled !== true) { | |
| seen.add(channel); | |
| channels.push(channel); | |
| } | |
| } | |
| return channels; | |
| } | |
| export function selectEnabledMessagingAgentRender< | |
| Render extends { | |
| readonly agent: string; | |
| readonly channelId: string; | |
| }, | |
| >( | |
| plan: SelectionPlanBase & { | |
| readonly agent: string; | |
| readonly agentRender: readonly Render[]; | |
| }, | |
| ): Render[] { | |
| const active = new Set(selectActiveMessagingChannelIds(plan)); | |
| return plan.agentRender.filter( | |
| (render) => render.agent === plan.agent && active.has(render.channelId), | |
| ); | |
| } | |
| export function selectEnabledPostAgentInstallBuildFiles< | |
| Step extends { | |
| readonly channelId: string; | |
| readonly kind: string; | |
| readonly hookId?: string; | |
| }, | |
| >( | |
| plan: SelectionPlanBase & { | |
| readonly buildSteps: readonly Step[]; | |
| }, | |
| ): Step[] { | |
| const active = new Set(selectActiveMessagingChannelIds(plan)); | |
| return plan.buildSteps.filter((step) => { | |
| if (!active.has(step.channelId) || step.kind !== "build-file") return false; | |
| if (!step.hookId) return true; | |
| const hookPhase = plan.channels | |
| .find((channel) => channel.channelId === step.channelId) | |
| ?.hooks?.find((hook) => hook.id === step.hookId)?.phase; | |
| return hookPhase === undefined || hookPhase === "post-agent-install"; | |
| }); | |
| } | |
| function normalizeChannelId(channelId: string): string { | |
| return channelId.trim().toLowerCase(); | |
| } | |
| export function selectActiveMessagingChannelIds(plan: SelectionPlanBase): string[] { | |
| const seen = new Set<string>(); | |
| const channels: string[] = []; | |
| for (const item of plan.channels) { | |
| const channel = normalizeChannelId(item.channelId ?? ""); | |
| if (!channel || seen.has(channel)) continue; | |
| if (item.active === true && item.disabled !== true) { | |
| seen.add(channel); | |
| channels.push(channel); | |
| } | |
| } | |
| return channels; | |
| } | |
| export function selectEnabledMessagingAgentRender< | |
| Render extends { | |
| readonly agent: string; | |
| readonly channelId: string; | |
| }, | |
| >( | |
| plan: SelectionPlanBase & { | |
| readonly agent: string; | |
| readonly agentRender: readonly Render[]; | |
| }, | |
| ): Render[] { | |
| const active = new Set(selectActiveMessagingChannelIds(plan)); | |
| return plan.agentRender.filter( | |
| (render) => render.agent === plan.agent && active.has(normalizeChannelId(render.channelId)), | |
| ); | |
| } | |
| export function selectEnabledPostAgentInstallBuildFiles< | |
| Step extends { | |
| readonly channelId: string; | |
| readonly kind: string; | |
| readonly hookId?: string; | |
| }, | |
| >( | |
| plan: SelectionPlanBase & { | |
| readonly buildSteps: readonly Step[]; | |
| }, | |
| ): Step[] { | |
| const active = new Set(selectActiveMessagingChannelIds(plan)); | |
| return plan.buildSteps.filter((step) => { | |
| const channelId = normalizeChannelId(step.channelId); | |
| if (!active.has(channelId) || step.kind !== "build-file") return false; | |
| if (!step.hookId) return true; | |
| const hookPhase = plan.channels | |
| .find((channel) => normalizeChannelId(channel.channelId) === channelId) | |
| ?.hooks?.find((hook) => hook.id === step.hookId)?.phase; | |
| return hookPhase === undefined || hookPhase === "post-agent-install"; | |
| }); | |
| } |
🤖 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/post-agent-install-selection.ts` around lines 24 - 77,
Extract a shared channel-id normalizer from selectActiveMessagingChannelIds and
use it consistently in selectEnabledMessagingAgentRender and
selectEnabledPostAgentInstallBuildFiles. Normalize render.channelId and
step.channelId before checking membership, and normalize both channel.channelId
and step.channelId when locating the matching channel and hook phase, while
preserving the existing filtering behavior.
Contributor
Author
There was a problem hiding this comment.
Contributor
There was a problem hiding this comment.
@ericksoa, acknowledged. Keep this thread open until #7976 completes GitHub qualification. The finding remains pending on that follow-up.
You are interacting with an AI system.
Comment on lines
+156
to
+161
| return spawnSync("bash", [scriptPath], { | ||
| encoding: "utf-8", | ||
| timeout: 5000, | ||
| env: { ...process.env, PATH: `${fakeBin}:${process.env.PATH || ""}`, ...extraEnv }, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the port scenarios hermetic.
runScenario spreads the whole ambient environment into the child. The scenario at lines 196-206 asserts the 18789 default, and the scenario at lines 184-194 asserts a specific port. If a developer shell or a CI job exports NEMOCLAW_DASHBOARD_PORT or CHAT_UI_URL, these assertions change result without any code change. Remove the two variables that the test controls, then apply extraEnv.
Based on learnings: hermetic tests in this repo must not inherit CI or local environment values that can activate or override behavior under assertion.
🧪 Proposed fix for environment inheritance
function runScenario(setArgs: string, extraEnv: Record<string, string> = {}) {
+ const baseEnv = { ...process.env };
+ delete baseEnv.NEMOCLAW_DASHBOARD_PORT;
+ delete baseEnv.CHAT_UI_URL; return spawnSync("bash", [scriptPath], {
encoding: "utf-8",
timeout: 5000,
- env: { ...process.env, PATH: `${fakeBin}:${process.env.PATH || ""}`, ...extraEnv },
+ env: { ...baseEnv, PATH: `${fakeBin}:${process.env.PATH || ""}`, ...extraEnv },
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return spawnSync("bash", [scriptPath], { | |
| encoding: "utf-8", | |
| timeout: 5000, | |
| env: { ...process.env, PATH: `${fakeBin}:${process.env.PATH || ""}`, ...extraEnv }, | |
| }); | |
| } | |
| function runScenario(setArgs: string, extraEnv: Record<string, string> = {}) { | |
| const baseEnv = { ...process.env }; | |
| delete baseEnv.NEMOCLAW_DASHBOARD_PORT; | |
| delete baseEnv.CHAT_UI_URL; | |
| return spawnSync("bash", [scriptPath], { | |
| encoding: "utf-8", | |
| timeout: 5000, | |
| env: { ...baseEnv, PATH: `${fakeBin}:${process.env.PATH || ""}`, ...extraEnv }, | |
| }); | |
| } |
🤖 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/entrypoint-env-wrapper.test.ts` around lines 156 - 161, Update
runScenario to remove NEMOCLAW_DASHBOARD_PORT and CHAT_UI_URL from the inherited
process environment before spreading extraEnv. Preserve all other ambient
variables and ensure extraEnv is applied last so each port scenario controls
these values deterministically.
Source: Learnings
Contributor
Author
There was a problem hiding this comment.
Contributor
There was a problem hiding this comment.
@ericksoa, acknowledged. The fix is tracked in stacked follow-up PR #7976.
I will leave this thread open until the exact-head GitHub qualification is green.
🧠 Learnings used
Learnt from: laitingsheng
Repo: NVIDIA/NemoClaw PR: 2511
File: test/onboard-brave-validation.test.ts:45-48
Timestamp: 2026-04-27T09:53:20.358Z
Learning: In this repo’s test suite, prefer the established POSIX PATH separator `:` when constructing `process.env.PATH` in tests (e.g., `PATH: `${fakeBin}:${process.env.PATH || ""}``). Do not replace it with `path.delimiter` in these unit/integration tests, because they only run on Linux runners in CI here; `windows-latest` is limited to WSL e2e tests in `.github/workflows/wsl-e2e.yaml` and is already POSIX-compliant.
Learnt from: ericksoa
Repo: NVIDIA/NemoClaw PR: 3205
File: test/onboard.test.ts:4236-4250
Timestamp: 2026-05-07T23:34:28.652Z
Learning: For hermetic messaging-channel tests in this repo (e.g., files like test/onboard.test.ts), ensure the spawned helper script does not inherit CI/local messaging credentials/config that could enable extra channels. Before calling createSandbox() inside the helper script, delete/remove unrelated messaging env vars such as DISCORD_* and TELEGRAM_* (tokens/config). This prevents inherited environment from activating additional channels and breaking/destabilizing Slack-only (or other single-channel) assertions.
You are interacting with an AI system.
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Carry the reviewed PR3.4b slice unchanged onto the serialized PR3.4a transaction contract. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Restack PR3.4b without changing its review patch. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
23 tasks
Base automatically changed from
feat/buildless-profile-application-coordinator
to
main
July 31, 2026 05:10
Trigger exact-head CI after the canonical GitHub bot restack without changing the reviewed tree. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Author
|
Advisor PRA-1 disposition for exact head The named consumer is PR3.13, which installs the image-owned runtime and wrapper and wires every applicable OpenClaw, Hermes, and DCode entrypoint together. PR3.14b/3.14c qualify all-agent multiarch, GPU/local-inference, and recovery behavior; PR3.15 is the only production activation slice. Those obligations are explicit in #7744 and in this PR description. Wiring only this precursor now would violate the maintainer-approved atomic activation boundary, so no production-caller change belongs in #7961. |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
23 tasks
2 tasks
jyaunches
pushed a commit
that referenced
this pull request
Jul 31, 2026
<!-- markdownlint-disable MD041 --> ## Summary Adds root-owned, transactional managed shared-state application and the first Docker adapter. The slice applies and rolls back validated OpenClaw, Hermes, and LangChain Deep Agents Code state atomically while keeping production buildless activation and durable crash recovery in later review units. ## Related Issue Part of #7744 Supersedes #7962; the implementation tree and review patch are identical, but this branch corrects an immutable restack commit-message line that violated `commitlint`. ## Changes - Define the managed shared-state transaction contract, ownership/mode checks, commit receipt, and idempotent rollback. - Add Docker staging and root-apply adapters that execute with `env -i`. - Forward only the six allowlisted OpenClaw scheduler controls through the clean root path. - Validate application controls before completion-file inspection or filesystem/transaction mutation. - Refresh a verified same-profile runtime and completion digest without starting a duplicate shared-state transaction. - Cover OpenClaw, Hermes, and DCode transaction, replay, ownership, mode, cleanup, and failure behavior. - Intentionally expose no production cutover caller in this slice. PR3.10 owns the transactional bootstrap/cutover integration after the driver-neutral lifecycle exists; PR3.12 owns restart-spanning persistence; PR3.15 owns production activation. Wiring these primitives directly into current onboarding here would create the partial runtime activation prohibited by #7744. ## Type of Change - [x] 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 - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This transaction layer remains dormant and does not change a supported CLI, configuration, runtime selection, workflow, or support statement. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Exact-diff and independent P1/P2 reviews covered clean-exec forwarding, pre-mutation validation, same-profile replay, ownership/mode enforcement, atomic commit, and rollback. The absence of a production caller is required by this review boundary: PR3.10 integrates cutover only after PR3.6–3.9 establish lifecycle parity, and PR3.15 activates the complete all-agent path. No P1/P2 remains inside this slice. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: Reviewed the exact 10-file, `+3,251/-37` patch from `7eb0369e7a0bc91f6ae99fc1eaad3cd274a34c8a` through `83f29daf129adf51bbffa43106633ee748b7eefc` against the NemoClaw Writing Guide and controlled terms. The stable patch ID is unchanged from `13932b9b…511141002`: `9a4ead01527f33245de5ed62412dca1395e33a13`. The patch adds dormant internal managed-startup image-runtime, root-apply, shared-state transaction, and Docker-adapter primitives with co-located source tests. It changes no Markdown, `docs/`, CLI command or action, public configuration, default, output, workflow, or live-E2E path. Production-import scans found no activation caller outside the new internal module graph, so no user-facing documentation change is needed for this restack. - Agent: Codex Desktop <!-- docs-review-head-sha: 83f29da --> <!-- docs-review-agents-blob-sha: c052d60 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - Exact locally validated head/base: `83f29daf129adf51bbffa43106633ee748b7eefc` / `7eb0369e7a0bc91f6ae99fc1eaad3cd274a34c8a` (exact-tree local validation passed; refreshed remote qualification will run) - Review budget: 10 files, `+3,251/-37`; no documentation paths. Stable patch ID: `9a4ead01527f33245de5ed62412dca1395e33a13`. - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] 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 - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: 87 slice-focused tests and 290 cross-slice regression tests passed; CLI and plugin builds passed; `npm run validate:pr` passed; `git diff --check` is clean. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Exact-head required CI is the broad gate for this dormant transaction slice. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] 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) ## Stack - Base: merged PR3.4b #7961 on `main` at `7eb0369e7a0bc91f6ae99fc1eaad3cd274a34c8a` - This slice: PR3.5 branch `feat/buildless-shared-state-transactions-v2` at `83f29daf129adf51bbffa43106633ee748b7eefc` - Next: PR3.6 introduces the driver-neutral lifecycle and sandbox-action parity. It is not part of this review diff. - Buildless support remains disabled until every supported agent and required qualification gate passes. Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added managed startup support for applying configuration through Docker. * Added completion verification and waiting so startup progress can be confirmed. * Added transaction-based handling for shared application state, including commit and rollback. * **Reliability Improvements** * Added safeguards for invalid, oversized, tampered, or incomplete startup data. * Improved recovery after interrupted or failed startup operations. * Preserved file ownership, permissions, and contents during state restoration. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
23 tasks
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
1 participant
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
Applies validated all-agent startup plans inside managed images and defines exact runtime-input cleanup. It preserves a sanitized child environment and deterministic replay for OpenClaw, Hermes, and LangChain Deep Agents Code without enabling production buildless onboarding.
The managed-startup runtime and OCI argv normalizer are deliberately not installed or invoked by a production image entrypoint in this slice. PR3.13 is the named consumer that installs the image-owned assets, wires every applicable OpenClaw, Hermes, and DCode entrypoint together, and qualifies their exact image layouts. Wiring a production entrypoint here would make an incomplete, unqualified subset reachable before the all-agent publication and protected-E2E gates, violating the epic activation invariant. This PR is internally complete as the dormant application/runtime contract; production reachability and entrypoint integration travel with their all-agent image tests in PR3.13.
Related Issue
Part of #7744
Changes
process.envdefaults.Type of Change
Quality Gates
Documentation Writer Review
no-docs-needed+2,308/-66) changes dormant application/runtime internals and focused tests only. No user-visible buildless support is advertised or enabled.DGX Station Hardware Evidence
Verification
13932b9b2486cd42b6fc47645e2f3936e5308d1d/f8fb820159c4843a19759efc9b0e28d4aa122440(exact-head CI and protected E2E running)+2,308/-66; no documentation paths.Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm run validate:prpassed;git diff --checkis clean.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Exact-head required CI is the broad gate for this dormant behavior slice.npm run docsbuilds without warnings (doc changes only)Stack
mainatf8fb820159c4843a19759efc9b0e28d4aa122440(PR3.4a feat(onboard): map and coordinate startup profiles #7960 remains merged)feat/buildless-managed-image-applicationat13932b9b2486cd42b6fc47645e2f3936e5308d1dSigned-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes