Fix owner-only auth and overlapping skill env regressions (#38548)

src/agents/skills.test.ts

@@ -266,6 +266,38 @@ describe("applySkillEnvOverrides", () => {
     });
   });
 
+  it("keeps env keys tracked until all overlapping overrides restore", async () => {
+    const workspaceDir = await makeWorkspace();
+    const skillDir = path.join(workspaceDir, "skills", "env-skill");
+    await writeSkill({
+      dir: skillDir,
+      name: "env-skill",
+      description: "Needs env",
+      metadata: '{"openclaw":{"requires":{"env":["ENV_KEY"]},"primaryEnv":"ENV_KEY"}}',
+    });
+
+    const entries = loadWorkspaceSkillEntries(workspaceDir, resolveTestSkillDirs(workspaceDir));
+
+    withClearedEnv(["ENV_KEY"], () => {
+      const config = { skills: { entries: { "env-skill": { apiKey: "injected" } } } };
+      const restoreFirst = applySkillEnvOverrides({ skills: entries, config });
+      const restoreSecond = applySkillEnvOverrides({ skills: entries, config });
+
+      try {
+        expect(process.env.ENV_KEY).toBe("injected");
+        expect(getActiveSkillEnvKeys().has("ENV_KEY")).toBe(true);
+
+        restoreFirst();
+        expect(process.env.ENV_KEY).toBe("injected");
+        expect(getActiveSkillEnvKeys().has("ENV_KEY")).toBe(true);
+      } finally {
+        restoreSecond();
+        expect(process.env.ENV_KEY).toBeUndefined();
+        expect(getActiveSkillEnvKeys().has("ENV_KEY")).toBe(false);
+      }
+    });
+  });
+
   it("applies env overrides from snapshots", async () => {
     const workspaceDir = await makeWorkspace();
     const skillDir = path.join(workspaceDir, "skills", "env-skill");

src/agents/skills/env-overrides.ts

@@ -9,8 +9,13 @@ import type { SkillEntry, SkillSnapshot } from "./types.js";
 
 const log = createSubsystemLogger("env-overrides");
 
-type EnvUpdate = { key: string; prev: string | undefined };
+type EnvUpdate = { key: string };
 type SkillConfig = NonNullable<ReturnType<typeof resolveSkillConfig>>;
+type ActiveSkillEnvEntry = {
+  baseline: string | undefined;
+  value: string;
+  count: number;
+};
 
 /**
  * Tracks env var keys that are currently injected by skill overrides.
@@ -18,11 +23,51 @@ type SkillConfig = NonNullable<ReturnType<typeof resolveSkillConfig>>;
  * leak to child processes (e.g., OPENAI_API_KEY leaking to Codex CLI).
  * @see https://github.com/openclaw/openclaw/issues/36280
  */
-const activeSkillEnvKeys = new Set<string>();
+const activeSkillEnvEntries = new Map<string, ActiveSkillEnvEntry>();
 
 /** Returns a snapshot of env var keys currently injected by skill overrides. */
 export function getActiveSkillEnvKeys(): ReadonlySet<string> {
-  return activeSkillEnvKeys;
+  return new Set(activeSkillEnvEntries.keys());
+}
+
+function acquireActiveSkillEnvKey(key: string, value: string): boolean {
+  const active = activeSkillEnvEntries.get(key);
+  if (active) {
+    active.count += 1;
+    if (process.env[key] === undefined) {
+      process.env[key] = active.value;
+    }
+    return true;
+  }
+  if (process.env[key] !== undefined) {
+    return false;
+  }
+  activeSkillEnvEntries.set(key, {
+    baseline: process.env[key],
+    value,
+    count: 1,
+  });
+  return true;
+}
+
+function releaseActiveSkillEnvKey(key: string) {
+  const active = activeSkillEnvEntries.get(key);
+  if (!active) {
+    return;
+  }
+  active.count -= 1;
+  if (active.count > 0) {
+    if (process.env[key] === undefined) {
+      process.env[key] = active.value;
+    }
+    return;
+  }
+  activeSkillEnvEntries.delete(key);
+  if (active.baseline === undefined) {
+    delete process.env[key];
+  } else {
+    process.env[key] = active.baseline;
+  }
 }
 
 type SanitizedSkillEnvOverrides = {
@@ -112,7 +157,9 @@ function applySkillConfigEnvOverrides(params: {
   if (skillConfig.env) {
     for (const [rawKey, envValue] of Object.entries(skillConfig.env)) {
       const envKey = rawKey.trim();
-      if (!envKey || !envValue || process.env[envKey]) {
+      const hasExternallyManagedValue =
+        process.env[envKey] !== undefined && !activeSkillEnvEntries.has(envKey);
+      if (!envKey || !envValue || hasExternallyManagedValue) {
         continue;
       }
       pendingOverrides[envKey] = envValue;
@@ -124,7 +171,11 @@ function applySkillConfigEnvOverrides(params: {
       value: skillConfig.apiKey,
       path: `skills.entries.${skillKey}.apiKey`,
     }) ?? "";
-  if (normalizedPrimaryEnv && resolvedApiKey && !process.env[normalizedPrimaryEnv]) {
+  const canInjectPrimaryEnv =
+    normalizedPrimaryEnv &&
+    (process.env[normalizedPrimaryEnv] === undefined ||
+      activeSkillEnvEntries.has(normalizedPrimaryEnv));
+  if (canInjectPrimaryEnv && resolvedApiKey) {
     if (!pendingOverrides[normalizedPrimaryEnv]) {
       pendingOverrides[normalizedPrimaryEnv] = resolvedApiKey;
     }
@@ -143,24 +194,18 @@ function applySkillConfigEnvOverrides(params: {
   }
 
   for (const [envKey, envValue] of Object.entries(sanitized.allowed)) {
-    if (process.env[envKey]) {
+    if (!acquireActiveSkillEnvKey(envKey, envValue)) {
       continue;
     }
-    updates.push({ key: envKey, prev: process.env[envKey] });
-    process.env[envKey] = envValue;
-    activeSkillEnvKeys.add(envKey);
+    updates.push({ key: envKey });
+    process.env[envKey] = activeSkillEnvEntries.get(envKey)?.value ?? envValue;
   }
 }
 
 function createEnvReverter(updates: EnvUpdate[]) {
   return () => {
     for (const update of updates) {
-      activeSkillEnvKeys.delete(update.key);
-      if (update.prev === undefined) {
-        delete process.env[update.key];
-      } else {
-        process.env[update.key] = update.prev;
-      }
+      releaseActiveSkillEnvKey(update.key);
     }
   };
 }

src/auto-reply/command-auth.owner-default.test.ts

@@ -22,8 +22,8 @@ afterEach(() => {
   setActivePluginRegistry(createRegistry());
 });
 
-describe("senderIsOwner defaults to true when no owner allowlist configured (#26319)", () => {
-  it("senderIsOwner is true when no ownerAllowFrom is configured (single-user default)", () => {
+describe("senderIsOwner only reflects explicit owner authorization", () => {
+  it("does not treat direct-message senders as owners when no ownerAllowFrom is configured", () => {
     const cfg = {
       channels: { discord: {} },
     } as OpenClawConfig;
@@ -42,12 +42,11 @@ describe("senderIsOwner defaults to true when no owner allowlist configured (#26
       commandAuthorized: true,
     });
 
-    // Without an explicit ownerAllowFrom list, the sole authorized user should
-    // be treated as owner so ownerOnly tools (cron, gateway) are available.
-    expect(auth.senderIsOwner).toBe(true);
+    expect(auth.senderIsOwner).toBe(false);
+    expect(auth.isAuthorizedSender).toBe(true);
   });
 
-  it("senderIsOwner is false when no ownerAllowFrom is configured in a group chat", () => {
+  it("does not treat group-chat senders as owners when no ownerAllowFrom is configured", () => {
     const cfg = {
       channels: { discord: {} },
     } as OpenClawConfig;
@@ -67,6 +66,7 @@ describe("senderIsOwner defaults to true when no owner allowlist configured (#26
     });
 
     expect(auth.senderIsOwner).toBe(false);
+    expect(auth.isAuthorizedSender).toBe(true);
   });
 
   it("senderIsOwner is false when ownerAllowFrom is configured and sender does not match", () => {

src/auto-reply/command-auth.ts

@@ -351,14 +351,7 @@ export function resolveCommandAuthorization(params: {
     Array.isArray(ctx.GatewayClientScopes) &&
     ctx.GatewayClientScopes.includes("operator.admin");
   const ownerAllowlistConfigured = ownerAllowAll || explicitOwners.length > 0;
-  const isDirectChat = (ctx.ChatType ?? "").trim().toLowerCase() === "direct";
-  // In the default single-user direct-chat setup, allow an identified sender to
-  // keep ownerOnly tools even without an explicit owner allowlist.
-  const senderIsOwner =
-    senderIsOwnerByIdentity ||
-    senderIsOwnerByScope ||
-    ownerAllowAll ||
-    (!ownerAllowlistConfigured && isDirectChat && Boolean(senderId));
+  const senderIsOwner = senderIsOwnerByIdentity || senderIsOwnerByScope || ownerAllowAll;
   const requireOwner = enforceOwner || ownerAllowlistConfigured;
   const isOwnerForCommands = !requireOwner
     ? true