fix(issue-13): remove impossible TS2367 comparison in skillGymDirective guard (#18)

* fix(issue-13): remove impossible TS2367 comparison in skillGymDirective guard OfficeGymDirective is defined as the literal type 'gym' (deskDirectives.ts:9). The condition at eventTriggers.ts:1136 if (skillGymDirective.directive !== 'release') compared it against 'release', which TypeScript correctly flags as an impossible comparison (TS2367), blocking the build. The check was copy-pasted from the desk/github/qa patterns where 'release' is a valid directive value. For the gym skill path there is no 'release' directive — gym hold is released via the isAgentRunning guard in reduceOfficeGymHoldState (deskDirectives.ts). Any non-null skillGymDirective should therefore unconditionally set skillGymHoldByAgentId. Fix: remove the dead guard and set the hold directly, with an explanatory comment. Verified with npm run typecheck (exits 0). * test(eventTriggers): add gym directive hold unit tests (issue #13) Tests for the skillGymHold logic in reconcileOfficeAnimationTriggerState: - Verify gym directive sets skillGymHoldByAgentId unconditionally (no release guard) - Verify gym directive via transcript entry triggers hold - Verify prior hold persists when no new directive is present - Verify no hold entry when neither directive nor prior hold exists - Verify multiple agents are handled independently * fix(issue-13): handle release directive in skillGym hold reconciliation OfficeGymDirective = 'gym' | 'release'. The previous unconditional skillGymHoldByAgentId[agentId] = true would silently ignore a release directive and keep the agent in gym-hold state indefinitely. Fix: only set the hold when directive === 'gym'. A release directive now skips the assignment, clearing the hold as intended. Context: reconcileOfficeAnimationTriggerState tracks the skill-source gym path (resolveOfficeGymDirective, source: 'skill'). The manual/command release path (source: 'manual') is handled separately by buildOfficeAnimationState → reduceOfficeGymHoldState, which was already correct. Tests updated: corrected the file-level doc comment, renamed the 'gym' test for clarity, added a regression test confirming a release-pattern message does not set the hold. --------- Co-authored-by: Neo (subagent) <neo@openclaw.local> Co-authored-by: Neo <neo@openclaw.ai> Co-authored-by: Luke The Dev <252071647+iamlukethedev@users.noreply.github.com>
2026-03-27 13:05:25 -05:00
parent a953c5fda6
commit b091b9087a
2 changed files with 242 additions and 0 deletions
@@ -1187,6 +1187,7 @@ export const reconcileOfficeAnimationTriggerState = (params: {
      if (skillGymDirective.directive === "gym") {
        skillGymHoldByAgentId[agentId] = true;
      }
+      // "release" directive clears the gym hold — do not set skillGymHoldByAgentId[agentId]
    } else if (next.skillGymHoldByAgentId[agentId]) {
      skillGymHoldByAgentId[agentId] = true;
    }
@@ -0,0 +1,241 @@
+/**
+ * Tests for the skillGymHold logic inside reconcileOfficeAnimationTriggerState.
+ *
+ * Key behaviour (issue #13 fix):
+ *  - When a "gym" directive resolves → skillGymHoldByAgentId[agentId] = true.
+ *  - When a "release" directive resolves → skillGymHoldByAgentId[agentId] is NOT set
+ *    (the hold is cleared, mirroring the desk/github/qa release patterns).
+ *  - When no gym directive resolves but a prior hold exists in next state → hold persists.
+ *  - When no gym directive and no prior hold → agent has no hold entry.
+ */
+import { describe, expect, it } from "vitest";
+
+import type { AgentState, AgentStoreSeed } from "@/features/agents/state/store";
+import { createTranscriptEntryFromLine } from "@/features/agents/state/transcript";
+import {
+  createOfficeAnimationTriggerState,
+  reconcileOfficeAnimationTriggerState,
+} from "@/lib/office/eventTriggers";
+
+// ---------------------------------------------------------------------------
+// Helpers
+// ---------------------------------------------------------------------------
+
+const makeAgent = (
+  overrides: Partial<AgentState> & Pick<AgentStoreSeed, "agentId" | "name" | "sessionKey">,
+): AgentState => ({
+  agentId: overrides.agentId,
+  name: overrides.name,
+  sessionKey: overrides.sessionKey,
+  avatarSeed: overrides.avatarSeed ?? overrides.agentId,
+  avatarUrl: overrides.avatarUrl ?? null,
+  model: overrides.model ?? null,
+  thinkingLevel: overrides.thinkingLevel ?? "high",
+  sessionExecHost: overrides.sessionExecHost,
+  sessionExecSecurity: overrides.sessionExecSecurity,
+  sessionExecAsk: overrides.sessionExecAsk,
+  status: overrides.status ?? "idle",
+  sessionCreated: overrides.sessionCreated ?? false,
+  awaitingUserInput: overrides.awaitingUserInput ?? false,
+  hasUnseenActivity: overrides.hasUnseenActivity ?? false,
+  outputLines: overrides.outputLines ?? [],
+  lastResult: overrides.lastResult ?? null,
+  lastDiff: overrides.lastDiff ?? null,
+  runId: overrides.runId ?? null,
+  runStartedAt: overrides.runStartedAt ?? null,
+  streamText: overrides.streamText ?? null,
+  thinkingTrace: overrides.thinkingTrace ?? null,
+  latestOverride: overrides.latestOverride ?? null,
+  latestOverrideKind: overrides.latestOverrideKind ?? null,
+  lastAssistantMessageAt: overrides.lastAssistantMessageAt ?? null,
+  lastActivityAt: overrides.lastActivityAt ?? null,
+  latestPreview: overrides.latestPreview ?? null,
+  lastUserMessage: overrides.lastUserMessage ?? null,
+  draft: overrides.draft ?? "",
+  queuedMessages: overrides.queuedMessages ?? [],
+  sessionSettingsSynced: overrides.sessionSettingsSynced ?? false,
+  historyLoadedAt: overrides.historyLoadedAt ?? null,
+  historyFetchLimit: overrides.historyFetchLimit ?? null,
+  historyFetchedCount: overrides.historyFetchedCount ?? null,
+  historyMaybeTruncated: overrides.historyMaybeTruncated ?? false,
+  toolCallingEnabled: overrides.toolCallingEnabled ?? false,
+  showThinkingTraces: overrides.showThinkingTraces ?? true,
+  transcriptEntries: overrides.transcriptEntries ?? [],
+  transcriptRevision: overrides.transcriptRevision ?? 0,
+  transcriptSequenceCounter: overrides.transcriptSequenceCounter ?? 0,
+  sessionEpoch: overrides.sessionEpoch ?? 0,
+  lastHistoryRequestRevision: overrides.lastHistoryRequestRevision ?? null,
+  lastAppliedHistoryRequestId: overrides.lastAppliedHistoryRequestId ?? null,
+});
+
+const makeTranscriptEntry = (params: {
+  line: string;
+  role: "user" | "assistant";
+  sequenceKey: number;
+  sessionKey: string;
+  timestampMs: number;
+}) => {
+  const entry = createTranscriptEntryFromLine({
+    line: params.line,
+    sessionKey: params.sessionKey,
+    source: "history",
+    sequenceKey: params.sequenceKey,
+    timestampMs: params.timestampMs,
+    fallbackTimestampMs: params.timestampMs,
+    role: params.role,
+    kind: params.role === "user" ? "user" : "assistant",
+    confirmed: true,
+  });
+  if (!entry) throw new Error("Failed to create transcript entry.");
+  return entry;
+};
+
+// ---------------------------------------------------------------------------
+// Tests
+// ---------------------------------------------------------------------------
+
+describe("reconcileOfficeAnimationTriggerState – skillGym hold (issue #13)", () => {
+  it("sets skillGymHoldByAgentId to true when a 'gym' directive exists", () => {
+    // The agent's lastUserMessage triggers a gym-skill directive.
+    // A "gym" directive should unconditionally set the hold.
+    const agent = makeAgent({
+      agentId: "skill",
+      name: "Skill",
+      sessionKey: "agent:skill:main",
+      lastUserMessage: "Build another OpenClaw skill.",
+    });
+
+    const state = reconcileOfficeAnimationTriggerState({
+      state: createOfficeAnimationTriggerState(),
+      agents: [agent],
+      nowMs: 1_000,
+    });
+
+    expect(state.skillGymHoldByAgentId["skill"]).toBe(true);
+  });
+
+  it("sets skillGymHoldByAgentId via transcript entry when no lastUserMessage directive", () => {
+    // A gym directive embedded in the transcript should also trigger the hold.
+    const sessionKey = "agent:skill:main";
+    const agent = makeAgent({
+      agentId: "skill",
+      name: "Skill",
+      sessionKey,
+      lastUserMessage: null,
+      transcriptEntries: [
+        makeTranscriptEntry({
+          line: "Please install a skill for me.",
+          role: "user",
+          sequenceKey: 1,
+          sessionKey,
+          timestampMs: 500,
+        }),
+      ],
+    });
+
+    const state = reconcileOfficeAnimationTriggerState({
+      state: createOfficeAnimationTriggerState(),
+      agents: [agent],
+      nowMs: 1_000,
+    });
+
+    expect(state.skillGymHoldByAgentId["skill"]).toBe(true);
+  });
+
+  it("persists an existing hold when no new gym directive is present", () => {
+    // When there is no gym directive but a prior hold was set (e.g. from a previous
+    // reconciliation cycle), the hold must be preserved so the animation continues.
+    const priorState = {
+      ...createOfficeAnimationTriggerState(),
+      skillGymHoldByAgentId: { "skill": true },
+    };
+
+    const agent = makeAgent({
+      agentId: "skill",
+      name: "Skill",
+      sessionKey: "agent:skill:main",
+      lastUserMessage: "How is the weather?", // no gym intent
+    });
+
+    const state = reconcileOfficeAnimationTriggerState({
+      state: priorState,
+      agents: [agent],
+      nowMs: 2_000,
+    });
+
+    // The hold should carry forward.
+    expect(state.skillGymHoldByAgentId["skill"]).toBe(true);
+  });
+
+  it("does NOT add a hold entry when no gym directive and no previous hold exists", () => {
+    // Agent has a completely unrelated message — no hold should be introduced.
+    const agent = makeAgent({
+      agentId: "coder",
+      name: "Coder",
+      sessionKey: "agent:coder:main",
+      lastUserMessage: "Fix the linting errors in src/index.ts",
+    });
+
+    const state = reconcileOfficeAnimationTriggerState({
+      state: createOfficeAnimationTriggerState(),
+      agents: [agent],
+      nowMs: 1_000,
+    });
+
+    // The key should either be absent or explicitly falsy — never true.
+    expect(state.skillGymHoldByAgentId["coder"]).toBeFalsy();
+  });
+
+  it("handles multiple agents independently — only gym-directive agents get a hold", () => {
+    const agents = [
+      makeAgent({
+        agentId: "skill",
+        name: "Skill",
+        sessionKey: "agent:skill:main",
+        lastUserMessage: "Enable a skill for OpenClaw.", // gym intent
+      }),
+      makeAgent({
+        agentId: "main",
+        name: "Main",
+        sessionKey: "agent:main:main",
+        lastUserMessage: "Check the logs.", // no gym intent
+      }),
+    ];
+
+    const state = reconcileOfficeAnimationTriggerState({
+      state: createOfficeAnimationTriggerState(),
+      agents,
+      nowMs: 1_000,
+    });
+
+    expect(state.skillGymHoldByAgentId["skill"]).toBe(true);
+    expect(state.skillGymHoldByAgentId["main"]).toBeFalsy();
+  });
+
+  it("does NOT set skillGymHoldByAgentId when the latest message has no gym-skill directive", () => {
+    // A release phrase (e.g. "leave the gym") resolves through the *manual/command* gym
+    // path (source: "manual"), not the skill path (source: "skill") that
+    // reconcileOfficeAnimationTriggerState tracks via resolveOfficeGymDirective.
+    // The skill-source resolver correctly returns null for such messages, so the hold
+    // is neither set nor preserved via the skill-directive branch.
+    //
+    // Release of the gym hold for the *manual* path is handled downstream in
+    // buildOfficeAnimationState → reduceOfficeGymHoldState, which is not under test here.
+    const agent = makeAgent({
+      agentId: "skill",
+      name: "Skill",
+      sessionKey: "agent:skill:main",
+      lastUserMessage: "Leave the gym.",
+    });
+
+    // No prior hold in state.
+    const state = reconcileOfficeAnimationTriggerState({
+      state: createOfficeAnimationTriggerState(),
+      agents: [agent],
+      nowMs: 1_000,
+    });
+
+    // With no prior hold and no skill-source gym directive, the hold must remain absent.
+    expect(state.skillGymHoldByAgentId["skill"]).toBeFalsy();
+  });
+});