Skip to content

Commit d0fee06

Browse files
committed
fix(onboard): accept schema-owned messaging plan fields (#9374)
<!-- markdownlint-disable MD041 --> ## Summary The managed startup profile validator rejected hydrated messaging package pins and credential placeholder lines before sandbox startup. This change accepts only the two schema-owned forms while continuing to reject raw credentials, malformed assignments, mismatched keys, wrong paths, and wrong types. ## Related Issue Fixes #9355 ## Changes - Accept a boolean `pin` only at `messaging.plan.buildSteps[*].value.pin`. - Accept a single canonical environment assignment only at `messaging.plan.agentRender[*].lines[*]` when its approved credential placeholder key matches the left-hand environment key. - Add focused positive and negative regression coverage for both accepted forms and the nearby rejection cases. - Close the detection gap where the generic credential-shape scanner had tests for standalone placeholders and raw secrets, but not for the hydrated messaging plan shapes that own these values. ## 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 <!-- Check one tests line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [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: Maintainer Aaron Erickson authorized admin merge on 2026-08-17 after exact-head CI, CodeRabbit, all feedback, and regression evidence were reviewed. CodeRabbit reports minimal merge risk and no actionable comments; the exact-head advisor recommends `merge_as_is` with no canonical findings. - [x] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: Maintainer Aaron Erickson accepted `CI / Pull Request / cli-test-shards (6)` and its `cli-tests`/`checks` aggregates. The unchanged current-main test references the renamed `Authorize Launchable image publication` step; PR #9369 corrects that one-line contract, and merged PR #9370 records the same accepted non-success. The failure does not overlap this PR's files or behavior. ## DGX Station Hardware Evidence <!-- Required only when scripts/prepare-dgx-station-host.sh changes. Maintainers must review the linked evidence before approving or merging. This is human-reviewed evidence, not authenticated hardware provenance. Exceptional bypasses use existing repository governance and must be documented on the PR. --> - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [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 — `npx vitest run --project cli src/lib/onboard/managed-startup-profile.test.ts` (120 passed) - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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) Exact-head qualification: [unfiltered PR E2E run 32080556047](https://github.com/NVIDIA/NemoClaw/actions/runs/32080556047) tested `512a2fc0942516fb2533252fe2bedc931992444c`. The current-main `messaging-providers` issue gate passed with every phase green, and protected all-agent GPU/local-inference/rollback/cleanup qualification passed. Eight unrelated non-successes were classified: six old-head OpenClaw ownership-handoff/cascade failures fixed on current `main` by #9370, one GPU runner driver/library mismatch that failed closed, and one Deep Agents evidence-publication failure after its behavior phases passed. The four additional #9355 targets live only in the still-unmerged #9323 matrix and must rerun there after #9323 consumes this prerequisite. --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> 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 support for credential placeholders in messaging startup configurations. * Added support for boolean package-install pins. * Valid credential references and correctly placed package pins are now accepted. * **Bug Fixes** * Improved validation of credential placeholders and package pins. * Continued rejecting raw credentials, malformed or mismatched assignments, misplaced pins, invalid pin types, and unsupported placeholder locations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit 14cde08)
1 parent f0326fc commit d0fee06

2 files changed

Lines changed: 184 additions & 5 deletions

File tree

src/lib/onboard/managed-startup-profile.test.ts

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -725,6 +725,119 @@ describe("managed startup profile", () => {
725725
).toThrow(/credential-shaped field name/);
726726
});
727727

728+
it("accepts schema-owned messaging package pins and credential placeholder lines (#9355)", () => {
729+
expect(() =>
730+
validateManagedStartupProfile({
731+
...OPENCLAW_PROFILE,
732+
messaging: {
733+
plan: {
734+
...OPENCLAW_PROFILE.messaging.plan,
735+
buildSteps: [
736+
{
737+
channelId: "slack",
738+
kind: "package-install",
739+
outputId: "slack-openclaw-plugin",
740+
required: true,
741+
value: {
742+
manager: "npm",
743+
spec: "@slack/web-api@7.9.3",
744+
pin: true,
745+
},
746+
},
747+
],
748+
agentRender: [
749+
...OPENCLAW_PROFILE.messaging.plan.agentRender,
750+
{
751+
channelId: "slack",
752+
agent: "hermes",
753+
target: "~/.hermes/.env",
754+
kind: "env-lines",
755+
lines: [
756+
"SLACK_BOT_TOKEN=xoxb-OPENSHELL-RESOLVE-ENV-SLACK_BOT_TOKEN",
757+
"DISCORD_BOT_TOKEN=openshell:resolve:env:DISCORD_BOT_TOKEN",
758+
"TELEGRAM_BOT_TOKEN=openshell:resolve:env:v1_TELEGRAM_BOT_TOKEN",
759+
],
760+
templateRefs: ["credential.slackBotToken.placeholder"],
761+
},
762+
],
763+
},
764+
},
765+
}),
766+
).not.toThrow();
767+
});
768+
769+
it.each([
770+
["a raw credential", `SLACK_BOT_TOKEN=xoxb-${"a".repeat(32)}`],
771+
["a malformed assignment", "SLACK_BOT_TOKEN =openshell:resolve:env:SLACK_BOT_TOKEN"],
772+
[
773+
"a placeholder for a different environment key",
774+
"SLACK_BOT_TOKEN=openshell:resolve:env:DISCORD_BOT_TOKEN",
775+
],
776+
[
777+
"a versioned placeholder for a different environment key",
778+
"SLACK_BOT_TOKEN=openshell:resolve:env:v1_DISCORD_BOT_TOKEN",
779+
],
780+
])("rejects %s in messaging environment lines (#9355)", (_label, line) => {
781+
expect(() =>
782+
validateManagedStartupProfile({
783+
...OPENCLAW_PROFILE,
784+
messaging: {
785+
plan: {
786+
...OPENCLAW_PROFILE.messaging.plan,
787+
agentRender: [
788+
{
789+
channelId: "slack",
790+
agent: "hermes",
791+
target: "~/.hermes/.env",
792+
kind: "env-lines",
793+
lines: [line],
794+
templateRefs: ["credential.slackBotToken.placeholder"],
795+
},
796+
],
797+
},
798+
},
799+
}),
800+
).toThrow(/credential-shaped string data/);
801+
});
802+
803+
it.each([
804+
[
805+
"a package pin outside buildSteps[*].value",
806+
{
807+
...OPENCLAW_PROFILE.messaging.plan,
808+
buildSteps: [{ pin: true }],
809+
},
810+
],
811+
[
812+
"a non-boolean package pin",
813+
{
814+
...OPENCLAW_PROFILE.messaging.plan,
815+
buildSteps: [{ value: { pin: "true" } }],
816+
},
817+
],
818+
[
819+
"a credential placeholder assignment outside agentRender[*].lines[*]",
820+
{
821+
...OPENCLAW_PROFILE.messaging.plan,
822+
note: "SLACK_BOT_TOKEN=openshell:resolve:env:SLACK_BOT_TOKEN",
823+
},
824+
],
825+
[
826+
"a direct credential placeholder outside schema-owned fields",
827+
{
828+
...OPENCLAW_PROFILE.messaging.plan,
829+
note: "openshell:resolve:env:SLACK_BOT_TOKEN",
830+
},
831+
],
832+
])("rejects %s (#9355)", (_label, plan) => {
833+
expect(() =>
834+
validateManagedStartupProfile({
835+
...OPENCLAW_PROFILE,
836+
messaging: { plan },
837+
}),
838+
).toThrow(/credential-shaped/);
839+
});
840+
728841
it.each([
729842
["routed inference", "inference", "routedBaseUrl"],
730843
["upstream inference", "inference", "upstreamEndpointUrl"],

src/lib/onboard/managed-startup/profile.ts

Lines changed: 71 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ const NON_SECRET_KEY_METADATA_NAMES = new Set([
5959
]);
6060
const MESSAGING_CREDENTIAL_PLACEHOLDER_RE =
6161
/^(?:openshell:resolve:env:|[A-Za-z0-9]+-OPENSHELL-RESOLVE-ENV-)(?:v[0-9]+_)?[A-Z][A-Z0-9_]*$/u;
62+
const JSON_ARRAY_INDEX_SEGMENT_RE = /^\[(?:0|[1-9][0-9]*)\]$/u;
6263
const SECRET_VALUE_PATTERNS: readonly RegExp[] = [
6364
/nvapi-[A-Za-z0-9_-]{10,}/u,
6465
/nvcf-[A-Za-z0-9_-]{10,}/u,
@@ -993,12 +994,71 @@ function valueLooksLikeSecret(value: string): boolean {
993994
}
994995

995996
function isMessagingCredentialPlaceholder(path: readonly string[], value: unknown): boolean {
997+
if (typeof value !== "string" || !MESSAGING_CREDENTIAL_PLACEHOLDER_RE.test(value)) {
998+
return false;
999+
}
1000+
const isCredentialBindingPlaceholder =
1001+
path.length === 5 &&
1002+
path[0] === "messaging" &&
1003+
path[1] === "plan" &&
1004+
path[2] === "credentialBindings" &&
1005+
JSON_ARRAY_INDEX_SEGMENT_RE.test(path[3] ?? "") &&
1006+
path[4] === "placeholder";
1007+
const isAgentRenderValuePlaceholder =
1008+
path.length >= 5 &&
1009+
path[0] === "messaging" &&
1010+
path[1] === "plan" &&
1011+
path[2] === "agentRender" &&
1012+
JSON_ARRAY_INDEX_SEGMENT_RE.test(path[3] ?? "") &&
1013+
path[4] === "value";
1014+
return isCredentialBindingPlaceholder || isAgentRenderValuePlaceholder;
1015+
}
1016+
1017+
function messagingCredentialPlaceholderEnvKey(value: string): string | null {
1018+
if (!MESSAGING_CREDENTIAL_PLACEHOLDER_RE.test(value)) return null;
1019+
const marker = value.startsWith("openshell:resolve:env:")
1020+
? "openshell:resolve:env:"
1021+
: "-OPENSHELL-RESOLVE-ENV-";
1022+
const key = value.slice(value.indexOf(marker) + marker.length);
1023+
return key.replace(/^v[0-9]+_/u, "");
1024+
}
1025+
1026+
function containsMessagingCredentialPlaceholder(value: string): boolean {
1027+
return value.includes("openshell:resolve:env:") || value.includes("-OPENSHELL-RESOLVE-ENV-");
1028+
}
1029+
1030+
function isMessagingCredentialPlaceholderAssignment(
1031+
path: readonly string[],
1032+
value: string,
1033+
): boolean {
1034+
if (
1035+
path.length !== 6 ||
1036+
path[0] !== "messaging" ||
1037+
path[1] !== "plan" ||
1038+
path[2] !== "agentRender" ||
1039+
!JSON_ARRAY_INDEX_SEGMENT_RE.test(path[3] ?? "") ||
1040+
path[4] !== "lines" ||
1041+
!JSON_ARRAY_INDEX_SEGMENT_RE.test(path[5] ?? "")
1042+
) {
1043+
return false;
1044+
}
1045+
const separator = value.indexOf("=");
1046+
if (separator <= 0 || value.indexOf("=", separator + 1) !== -1) return false;
1047+
const envKey = value.slice(0, separator);
1048+
const placeholderEnvKey = messagingCredentialPlaceholderEnvKey(value.slice(separator + 1));
1049+
return CREDENTIAL_ENV_NAME_PATTERN.test(envKey) && envKey === placeholderEnvKey;
1050+
}
1051+
1052+
function isMessagingPackagePin(path: readonly string[], value: unknown): boolean {
9961053
return (
997-
path.length >= 2 &&
1054+
path.length === 6 &&
9981055
path[0] === "messaging" &&
9991056
path[1] === "plan" &&
1000-
typeof value === "string" &&
1001-
MESSAGING_CREDENTIAL_PLACEHOLDER_RE.test(value)
1057+
path[2] === "buildSteps" &&
1058+
JSON_ARRAY_INDEX_SEGMENT_RE.test(path[3] ?? "") &&
1059+
path[4] === "value" &&
1060+
path[5] === "pin" &&
1061+
typeof value === "boolean"
10021062
);
10031063
}
10041064

@@ -1367,7 +1427,9 @@ function assertPayloadStructureAndCredentialShapes(root: unknown): void {
13671427
observeText(current.value);
13681428
if (
13691429
!isMessagingCredentialPlaceholder(current.path, current.value) &&
1370-
valueLooksLikeSecret(current.value)
1430+
!isMessagingCredentialPlaceholderAssignment(current.path, current.value) &&
1431+
(valueLooksLikeSecret(current.value) ||
1432+
containsMessagingCredentialPlaceholder(current.value))
13711433
) {
13721434
invalid(
13731435
`payload field ${payloadPath(current.path)} contains credential-shaped string data`,
@@ -1450,7 +1512,11 @@ function assertPayloadStructureAndCredentialShapes(root: unknown): void {
14501512
invalid("payload must contain only JSON data properties");
14511513
}
14521514
const child = descriptor.value;
1453-
if (isCredentialShapedName(key) && !isMessagingCredentialPlaceholder(current.path, child)) {
1515+
if (
1516+
isCredentialShapedName(key) &&
1517+
!isMessagingCredentialPlaceholder([...current.path, key], child) &&
1518+
!isMessagingPackagePin([...current.path, key], child)
1519+
) {
14541520
invalid(
14551521
`payload field ${payloadPath([...current.path, key])} has a credential-shaped field name`,
14561522
);

0 commit comments

Comments
 (0)