Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 36 additions & 2 deletions .github/workflows/nightly-e2e.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
# skip/05-network-policy.sh, then cleanup.sh --verify with if: always()).
# messaging-providers-e2e Validates messaging credential provider/placeholder/L7-proxy chain
# for Telegram + Discord. Uses fake tokens. See PR #1081.
# token-rotation-e2e Validates that rotating a messaging token and re-running onboard
# propagates the new credential to the sandbox. See issue #1903.
# sandbox-survival-e2e Sandbox survival across gateway restarts (onboard, inference,
# gateway stop/start, verify sandbox + workspace + inference).
# hermes-e2e Hermes Agent E2E — install → onboard --agent hermes → health
Expand Down Expand Up @@ -203,6 +205,38 @@ jobs:
path: /tmp/nemoclaw-e2e-install.log
if-no-files-found: ignore

# ── Token rotation (credential propagation to L7 proxy) ─────
# Validates that rotating a messaging token and re-running onboard
# propagates the new credential to the sandbox. Uses two fake tokens
# to prove the sandbox is rebuilt on rotation and reused when unchanged.
# See: issue #1903
token-rotation-e2e:
if: github.repository == 'NVIDIA/NemoClaw'
runs-on: ubuntu-latest
timeout-minutes: 45
steps:
- name: Checkout
uses: actions/checkout@v4

- name: Run token rotation E2E test
env:
NVIDIA_API_KEY: ${{ secrets.NVIDIA_API_KEY }}
NEMOCLAW_NON_INTERACTIVE: "1"
NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE: "1"
NEMOCLAW_POLICY_TIER: "open"
GITHUB_TOKEN: ${{ github.token }}
TELEGRAM_BOT_TOKEN_A: "test-fake-token-A-rotation-e2e"
TELEGRAM_BOT_TOKEN_B: "test-fake-token-B-rotation-e2e"
run: bash test/e2e/test-token-rotation.sh

- name: Upload install log on failure
if: failure()
uses: actions/upload-artifact@v4
with:
name: install-log-token-rotation
path: /tmp/nemoclaw-e2e-install.log
if-no-files-found: ignore

# ── Sandbox survival (gateway restart recovery) ──────────────
sandbox-survival-e2e:
if: github.repository == 'NVIDIA/NemoClaw'
Expand Down Expand Up @@ -550,8 +584,8 @@ jobs:

notify-on-failure:
runs-on: ubuntu-latest
needs: [cloud-e2e, cloud-experimental-e2e, messaging-providers-e2e, sandbox-survival-e2e, hermes-e2e, skip-permissions-e2e, sandbox-operations-e2e, inference-routing-e2e, snapshot-commands-e2e, rebuild-openclaw-e2e, upgrade-stale-sandbox-e2e, rebuild-hermes-e2e, gpu-e2e]
if: ${{ always() && (needs.cloud-e2e.result == 'failure' || needs.cloud-experimental-e2e.result == 'failure' || needs.messaging-providers-e2e.result == 'failure' || needs.sandbox-survival-e2e.result == 'failure' || needs.hermes-e2e.result == 'failure' || needs.skip-permissions-e2e.result == 'failure' || needs.sandbox-operations-e2e.result == 'failure' || needs.inference-routing-e2e.result == 'failure' || needs.snapshot-commands-e2e.result == 'failure' || needs.rebuild-openclaw-e2e.result == 'failure' || needs.upgrade-stale-sandbox-e2e.result == 'failure' || needs.rebuild-hermes-e2e.result == 'failure' || needs.gpu-e2e.result == 'failure') }}
needs: [cloud-e2e, cloud-experimental-e2e, messaging-providers-e2e, token-rotation-e2e, sandbox-survival-e2e, hermes-e2e, skip-permissions-e2e, sandbox-operations-e2e, inference-routing-e2e, snapshot-commands-e2e, rebuild-openclaw-e2e, upgrade-stale-sandbox-e2e, rebuild-hermes-e2e, gpu-e2e]
if: ${{ always() && (needs.cloud-e2e.result == 'failure' || needs.cloud-experimental-e2e.result == 'failure' || needs.messaging-providers-e2e.result == 'failure' || needs.token-rotation-e2e.result == 'failure' || needs.sandbox-survival-e2e.result == 'failure' || needs.hermes-e2e.result == 'failure' || needs.skip-permissions-e2e.result == 'failure' || needs.sandbox-operations-e2e.result == 'failure' || needs.inference-routing-e2e.result == 'failure' || needs.snapshot-commands-e2e.result == 'failure' || needs.rebuild-openclaw-e2e.result == 'failure' || needs.upgrade-stale-sandbox-e2e.result == 'failure' || needs.rebuild-hermes-e2e.result == 'failure' || needs.gpu-e2e.result == 'failure') }}
Comment on lines +587 to +588

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — shields-config-e2e was added in a separate PR and isn't part of this change, but I'll include it in the notify-on-failure aggregation since we're already touching that line.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ericksoa, sounds good! The patch in the review comment has the exact lines ready — just drop shields-config-e2e into the needs: list and add needs.shields-config-e2e.result == 'failure' || to the if: expression at the same position (after snapshot-commands-e2e), and you'll be all set.


🧠 Learnings used
Learnt from: ericksoa
Repo: NVIDIA/NemoClaw PR: 1967
File: test/e2e/test-token-rotation.sh:0-0
Timestamp: 2026-04-16T18:34:27.427Z
Learning: In `test/e2e/test-token-rotation.sh` (NemoClaw project), the E2E test intentionally focuses only on messaging credential rotation detection and sandbox rebuild triggering. Workspace/state preservation across rotation is not tested here; it is covered by the `sandboxRebuild` unit tests and `test/e2e/test-sandbox-rebuild.sh`. Do not flag the absence of a workspace-preservation assertion in this script as a test gap.

Learnt from: ericksoa
Repo: NVIDIA/NemoClaw PR: 1849
File: src/lib/sandbox-config.ts:354-361
Timestamp: 2026-04-16T17:09:43.616Z
Learning: In `src/lib/sandbox-config.ts` (TypeScript, NemoClaw CLI), the `appendAuditEntry` call inside `configSet` intentionally uses `action: "shields_down"` rather than a dedicated `"config_set"` action. This is by design: `config set` is only permitted while shields are down, so the audit entry records the shields-down authorization context rather than the config mutation itself. Do not flag this as a wrong action type in future reviews. A dedicated `config_set` audit action type is deferred to a follow-up.

Learnt from: ericksoa
Repo: NVIDIA/NemoClaw PR: 1849
File: test/shields-audit.test.ts:29-31
Timestamp: 2026-04-16T17:09:26.055Z
Learning: In `test/shields-audit.test.ts` (NemoClaw project), the local `appendAuditEntry()` reimplementation is intentional: (1) the test's purpose is to validate JSONL format and field structure only, (2) redaction is covered separately in `test/secret-redaction.test.ts`, and (3) the CJS/ESM boundary in `src/lib/shields-audit.ts` prevents calling the real logger directly from Vitest. Do not flag the local reimplementation as a test-coverage gap in future reviews.

Learnt from: jayavenkatesh19
Repo: NVIDIA/NemoClaw PR: 839
File: .github/workflows/nightly-e2e.yaml:97-101
Timestamp: 2026-03-25T17:41:58.209Z
Learning: In this repo’s GitHub Actions workflows, it is intentional and correct to set `NEMOCLAW_RECREATE_SANDBOX=1` in a job `env:` block when the job later runs `install.sh` in `--non-interactive` mode. `install.sh --non-interactive` orchestrates `nemoclaw onboard --non-interactive`, which reads `NEMOCLAW_RECREATE_SANDBOX` from the environment; the variable is documented in `install.sh` usage/help output, so it should not be flagged as an unknown or unused env var in workflows that call `install.sh`.

permissions:
issues: write
steps:
Expand Down
124 changes: 123 additions & 1 deletion src/lib/onboard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
// Supports non-interactive mode via --non-interactive flag or
// NEMOCLAW_NON_INTERACTIVE=1 env var for CI/CD pipelines.

const crypto = require("node:crypto");
const fs = require("fs");
const os = require("os");
const path = require("path");
Expand Down Expand Up @@ -71,6 +72,7 @@ const agentOnboard = require("./agent-onboard");
const agentDefs = require("./agent-defs");

const gatewayState = require("./gateway-state");
const sandboxState = require("./sandbox-state");
const validation = require("./validation");
const urlUtils = require("./url-utils");
const buildContext = require("./build-context");
Expand Down Expand Up @@ -792,6 +794,45 @@ function providerExistsInGateway(name) {
return result.status === 0;
}

/**
* Compute a SHA-256 hash of a credential value for change detection.
* Stored in the sandbox registry so we can detect rotation on reuse
* without needing to read the credential back from OpenShell.
* @param {string} value - Credential value to hash.
* @returns {string|null} Hex-encoded SHA-256 hash, or null if value is falsy.
*/
function hashCredential(value) {
if (!value) return null;
return crypto.createHash("sha256").update(String(value).trim()).digest("hex");
}

/**
* Detect whether any messaging provider credential has been rotated since
* the sandbox was created, by comparing SHA-256 hashes of the current
* token values against hashes stored in the sandbox registry.
*
* Returns `changed: false` for legacy sandboxes that have no stored hashes
* (conservative — avoids unnecessary rebuilds after upgrade).
*
* @param {string} sandboxName - Name of the sandbox to check.
* @param {Array<{name: string, envKey: string, token: string|null}>} tokenDefs
* @returns {{ changed: boolean, changedProviders: string[] }}
*/
function detectMessagingCredentialRotation(sandboxName, tokenDefs) {
const sb = registry.getSandbox(sandboxName);
const storedHashes = sb?.providerCredentialHashes || {};
const changedProviders = [];
for (const { name, envKey, token } of tokenDefs) {
if (!token) continue;
const storedHash = storedHashes[envKey];
if (!storedHash) continue;
if (storedHash !== hashCredential(token)) {
changedProviders.push(name);
}
}
return { changed: changedProviders.length > 0, changedProviders };
}

// Tri-state probe factory for messaging-conflict backfill. An upfront liveness
// check is necessary because `openshell provider get` exits non-zero for both
// "provider not attached" and "gateway unreachable"; without the liveness
Expand Down Expand Up @@ -2821,6 +2862,10 @@ async function createSandbox(
// Reconcile local registry state with the live OpenShell gateway state.
const liveExists = pruneStaleSandboxEntry(sandboxName);

// Declared outside the liveExists block so it is accessible during
// post-creation restore (the sandbox create path runs after the block).
let pendingStateRestore = null;

if (liveExists) {
const existingSandboxState = getSandboxReuseState(sandboxName);

Expand All @@ -2831,7 +2876,14 @@ async function createSandbox(
hasMessagingTokens &&
messagingTokenDefs.some(({ name, token }) => token && !providerExistsInGateway(name));

if (!isRecreateSandbox() && !needsProviderMigration) {
// Detect whether any messaging credential has been rotated since the
// sandbox was created. Provider credentials are resolved once at sandbox
// startup, so a rotated token requires a rebuild to take effect.
const credentialRotation = hasMessagingTokens
? detectMessagingCredentialRotation(sandboxName, messagingTokenDefs)
: { changed: false, changedProviders: [] };

if (!isRecreateSandbox() && !needsProviderMigration && !credentialRotation.changed) {
if (isNonInteractive()) {
if (existingSandboxState === "ready") {
// Upsert messaging providers even on reuse so credential changes take
Expand Down Expand Up @@ -2873,9 +2925,53 @@ async function createSandbox(
}
}

// Back up workspace state before destroying the sandbox when triggered
// by credential rotation, so files can be restored after recreation.
if (credentialRotation.changed && existingSandboxState === "ready") {
const rotatedNames = credentialRotation.changedProviders.join(", ");
console.log(` Messaging credential(s) rotated: ${rotatedNames}`);
console.log(" Rebuilding sandbox to propagate new credentials to the L7 proxy...");
try {
const backup = sandboxState.backupSandboxState(sandboxName);
if (backup.success) {
note(` ✓ State backed up (${backup.backedUpDirs.length} directories)`);
pendingStateRestore = backup;
} else {
console.error(" State backup failed — aborting rebuild to prevent data loss.");
console.error(" Pass --recreate-sandbox to force recreation without backup.");
upsertMessagingProviders(messagingTokenDefs);
// Update stored hashes so the next onboard doesn't re-detect rotation.
const abortHashes = {};
for (const { envKey, token } of messagingTokenDefs) {
if (token) abortHashes[envKey] = hashCredential(token);
}
if (Object.keys(abortHashes).length > 0) {
registry.updateSandbox(sandboxName, { providerCredentialHashes: abortHashes });
}
ensureDashboardForward(sandboxName, chatUiUrl);
return sandboxName;
}
} catch (err) {
console.error(` State backup threw: ${err.message} — aborting rebuild.`);
console.error(" Pass --recreate-sandbox to force recreation without backup.");
upsertMessagingProviders(messagingTokenDefs);
const abortHashes = {};
for (const { envKey, token } of messagingTokenDefs) {
if (token) abortHashes[envKey] = hashCredential(token);
}
if (Object.keys(abortHashes).length > 0) {
registry.updateSandbox(sandboxName, { providerCredentialHashes: abortHashes });
}
ensureDashboardForward(sandboxName, chatUiUrl);
return sandboxName;
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

if (needsProviderMigration) {
console.log(` Sandbox '${sandboxName}' exists but messaging providers are not attached.`);
console.log(" Recreating to ensure credentials flow through the provider pipeline.");
} else if (credentialRotation.changed) {
// Message already printed above during backup.
} else if (existingSandboxState === "ready") {
note(` Sandbox '${sandboxName}' exists and is ready — recreating by explicit request.`);
} else {
Expand Down Expand Up @@ -3231,6 +3327,12 @@ async function createSandbox(

// Register only after confirmed ready — prevents phantom entries
const effectiveAgent = agent || agentDefs.loadAgent("openclaw");
const providerCredentialHashes = {};
for (const { envKey, token } of messagingTokenDefs) {
if (token) {
providerCredentialHashes[envKey] = hashCredential(token);
}
}
registry.registerSandbox({
name: sandboxName,
model: model || null,
Expand All @@ -3239,9 +3341,27 @@ async function createSandbox(
agent: agent ? agent.name : null,
agentVersion: fromDockerfile ? null : effectiveAgent.expectedVersion || null,
dangerouslySkipPermissions: dangerouslySkipPermissions || undefined,
providerCredentialHashes:
Object.keys(providerCredentialHashes).length > 0 ? providerCredentialHashes : undefined,
messagingChannels: activeMessagingChannels,
});

// Restore workspace state if we backed it up during credential rotation.
if (pendingStateRestore?.success) {
note(" Restoring workspace state after credential rotation...");
const restore = sandboxState.restoreSandboxState(
sandboxName,
pendingStateRestore.manifest.backupPath,
);
if (restore.success) {
note(` ✓ State restored (${restore.restoredDirs.length} directories)`);
} else {
console.error(
` Warning: partial restore. Manual recovery: ${pendingStateRestore.manifest.backupPath}`,
);
}
}

// DNS proxy — run a forwarder in the sandbox pod so the isolated
// sandbox namespace can resolve hostnames (fixes #626).
console.log(" Setting up sandbox DNS proxy...");
Expand Down Expand Up @@ -5931,6 +6051,8 @@ module.exports = {
summarizeProbeFailure,
hasResponsesToolCall,
upsertProvider,
hashCredential,
detectMessagingCredentialRotation,
hydrateCredentialEnv,
pruneKnownHostsEntries,
shouldIncludeBuildContextPath,
Expand Down
2 changes: 2 additions & 0 deletions src/lib/registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export interface SandboxEntry {
agent?: string | null;
dangerouslySkipPermissions?: boolean;
agentVersion?: string | null;
providerCredentialHashes?: Record<string, string>;
messagingChannels?: string[];
}

Expand Down Expand Up @@ -166,6 +167,7 @@ export function registerSandbox(entry: SandboxEntry): void {
dangerouslySkipPermissions:
entry.dangerouslySkipPermissions === true ? true : undefined,
agentVersion: entry.agentVersion || null,
providerCredentialHashes: entry.providerCredentialHashes || undefined,
messagingChannels: entry.messagingChannels || [],
};
if (!data.defaultSandbox) {
Expand Down
Loading
Loading