test(frontend): cover the build-version script, user config and agent registration

frontend/src/app/common/service/user/config/user-config.service.spec.ts

@@ -197,6 +197,34 @@ describe("UserConfigService", () => {
     });
   });
 
+  // updateEntry is private and validates its own arguments rather than trusting
+  // its callers. Both guards below are unreachable through the public API --
+  // fetchKey/set/delete reject a blank key before they ever tap into
+  // updateEntry, and delete() returns early when the key is absent -- so the
+  // contract updateEntry keeps for itself is pinned here by calling it directly.
+  describe("updateEntry (private, called directly)", () => {
+    it("rejects a blank key even though no public caller can supply one", () => {
+      expect(() => (service as any).updateEntry("   ", "v")).toThrowError(/key cannot be empty/);
+      // The guard runs before any mutation, so nothing was written under a
+      // trimmed or raw form of the blank key.
+      expect(service.getDict()).toEqual({});
+    });
+
+    it("does not fire dictionaryChanged when told to delete a key that is absent", () => {
+      const next = vi.fn();
+      const sub = (service as any).dictionaryChangedSubject.subscribe(next);
+
+      (service as any).updateEntry("absent", undefined);
+
+      // Asserting on the subject, not just on getDict(): a version that deleted
+      // and notified unconditionally would leave the dictionary looking identical
+      // while still waking every subscriber.
+      expect(next).not.toHaveBeenCalled();
+      expect(service.getDict()).toEqual({});
+      sub.unsubscribe();
+    });
+  });
+
   describe("user-change reactions", () => {
     it("re-fetches the dictionary when a logged-in user is emitted on userChanged", () => {
       stubUserService.userChangeSubject.next(MOCK_USER);

frontend/src/app/workspace/component/agent/agent-panel/agent-registration/agent-registration.component.spec.ts

@@ -28,6 +28,7 @@ import { NotificationService } from "../../../../../common/service/notification/
 import { WorkflowActionService } from "../../../../service/workflow-graph/model/workflow-action.service";
 import { ComputingUnitStatusService } from "../../../../../common/service/computing-unit/computing-unit-status/computing-unit-status.service";
 import { ComputingUnitState } from "../../../../../common/type/computing-unit-connection.interface";
+import { NzTooltipDirective } from "ng-zorro-antd/tooltip";
 import { commonTestProviders } from "../../../../../common/testing/test-utils";
 
 const MODEL: ModelType = { id: "gpt", name: "GPT", description: "desc", icon: "robot" };
@@ -100,6 +101,17 @@ describe("AgentRegistrationComponent", () => {
       expect(notifyError).toHaveBeenCalledWith("Failed to fetch models: boom");
     });
 
+    it("stringifies a rejection that is not an Error into the same message", () => {
+      // The test above covers `error.message`; an rxjs source is free to reject
+      // with anything, and a bare string has no `.message`, so the component
+      // falls back to String(error) instead of interpolating `undefined`.
+      fetchModelTypes.mockReturnValue(throwError(() => "backend unreachable"));
+      fixture.detectChanges();
+
+      expect(component.hasLoadingError).toBe(true);
+      expect(notifyError).toHaveBeenCalledWith("Failed to fetch models: backend unreachable");
+    });
+
     it("marks the computing unit connected only when the status is Running", () => {
       getStatus.mockReturnValue(of(ComputingUnitState.Running));
       fixture.detectChanges();
@@ -214,5 +226,85 @@ describe("AgentRegistrationComponent", () => {
       expect((cards[1].nativeElement as HTMLElement).classList).toContain("selected");
       expect((cards[0].nativeElement as HTMLElement).classList).not.toContain("selected");
     });
+
+    it("writes what the user types in the name box back into customAgentName", async () => {
+      // The name box is `[(ngModel)]`-bound. Every other test in this file only
+      // assigns `customAgentName` on the instance, which is the direction the box
+      // is never driven in; typing is what feeds the name createAgent() sends.
+      fetchModelTypes.mockReturnValue(of([MODEL]));
+      // The input is `[disabled]="!selectedModelType"`, so pick a model before the
+      // first render; ngModel refuses to push a value into a disabled control.
+      component.selectModelType(MODEL.id);
+      fixture.detectChanges();
+      // ngModel pushes the field into the DOM on a microtask rather than
+      // synchronously inside detectChanges, so drain it before reading the box.
+      await new Promise(resolve => setTimeout(resolve, 0));
+
+      const input = fixture.debugElement.query(By.css("input[nz-input]")).nativeElement as HTMLInputElement;
+      expect(input.disabled).toBe(false);
+      expect(input.value).toBe("Texera Agent");
+
+      // Typed rather than assigned -- the write-back is the untested direction.
+      input.value = "My Analyst";
+      input.dispatchEvent(new Event("input"));
+      fixture.detectChanges();
+
+      expect(component.customAgentName).toBe("My Analyst");
+    });
+
+    it("labels the submit button Creating... while a creation is in flight", () => {
+      fetchModelTypes.mockReturnValue(of([MODEL]));
+      fixture.detectChanges();
+
+      const button = fixture.debugElement.query(By.css("button[nz-button]")).nativeElement as HTMLElement;
+      expect(button.textContent).toContain("Create Agent");
+      // The spinner is bound off the same flag as the label. Asserting only the
+      // label leaves `[nzLoading]` free to be bound inverted, which would spin
+      // the button whenever nothing is happening and stop spinning during the
+      // one request it exists to cover.
+      expect(button.classList).not.toContain("ant-btn-loading");
+
+      component.isCreating = true;
+      fixture.detectChanges();
+
+      expect(button.textContent).toContain("Creating...");
+      expect(button.textContent).not.toContain("Create Agent");
+      expect(button.classList).toContain("ant-btn-loading");
+    });
+
+    it("keeps the submit button shut, and says why, until a model and a computing unit are both there", () => {
+      // canCreate() is unit-tested above; what is pinned here is the template
+      // actually honouring it. Nothing else in this file reads the button's
+      // gate, so an inverted `[disabled]` binding -- Create enabled exactly
+      // when creation is impossible -- passes every other test: createAgent()
+      // re-checks selectedModelType and isCreating but never
+      // computingUnitConnected, so the click would reach the backend.
+      fetchModelTypes.mockReturnValue(of([MODEL]));
+      fixture.detectChanges();
+
+      const buttonEl = fixture.debugElement.query(By.css("button[nz-button]"));
+      const button = buttonEl.nativeElement as HTMLButtonElement;
+      const tooltip = buttonEl.injector.get(NzTooltipDirective);
+
+      // getStatus defaults to Pending, so the gate legitimately starts shut.
+      expect(component.canCreate()).toBe(false);
+      expect(button.disabled).toBe(true);
+      // Both arms of the tooltip ternary are already executed by this suite, so
+      // only comparing the strings notices them being handed to the wrong side.
+      expect(tooltip.title).toBe("Connect to a computing unit first");
+      // The banner carries the same explanation. Its *ngIf arms are likewise
+      // both executed by the suite, so only checking WHICH state renders it
+      // notices the condition being inverted.
+      expect(fixture.debugElement.query(By.css("nz-alert"))).toBeTruthy();
+
+      component.computingUnitConnected = true;
+      component.selectedModelType = MODEL.id;
+      fixture.detectChanges();
+
+      expect(component.canCreate()).toBe(true);
+      expect(button.disabled).toBe(false);
+      expect(tooltip.title).toBe("");
+      expect(fixture.debugElement.query(By.css("nz-alert"))).toBeNull();
+    });
   });
 });

frontend/src/app/workspace/service/code-editor/code-editor.service.spec.ts

@@ -44,6 +44,23 @@ describe("CodeEditorService", () => {
     expect(value).toBe(false);
   });
 
+  it("seeds the state when setEditorState runs before any getEditorState", () => {
+    // Nothing has asked for "fresh" yet, so setEditorState has no subject to push
+    // onto and must create one already carrying the requested value. Every other
+    // test in this file calls getEditorState first, which pre-creates the subject
+    // seeded to false, so this is the only path that exercises the seeding.
+    service.setEditorState("fresh", true);
+
+    let value: boolean | undefined;
+    service.getEditorState("fresh").subscribe(v => (value = v));
+    expect(value).toBe(true);
+
+    // The seeded subject is the same one getEditorState handed out -- a later
+    // write reaches the existing subscriber rather than a replacement.
+    service.setEditorState("fresh", false);
+    expect(value).toBe(false);
+  });
+
   it("should track state independently for different operator IDs", () => {
     let valueA: boolean | undefined;
     let valueB: boolean | undefined;

frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts

@@ -24,6 +24,7 @@ import { HttpClientTestingModule, HttpTestingController } from "@angular/common/
 import { NotificationService } from "src/app/common/service/notification/notification.service";
 import { GuiConfigService } from "src/app/common/service/gui-config.service";
 import { WorkflowUtilService } from "../workflow-graph/util/workflow-util.service";
+import { NotebookMigrationLLM } from "./migration-llm";
 import { firstValueFrom, throwError } from "rxjs";
 
 describe("NotebookMigrationService", () => {
@@ -297,6 +298,28 @@ describe("NotebookMigrationService", () => {
     expect(result).toEqual({ success: true, deleted: 1 });
   });
 
+  // Every other test that touches the LLM replaces createMigrationLLM() with a
+  // fake, so the seam's own body -- the one place that decides which collaborators
+  // the real client is wired to, and in which order -- is never run. This calls it
+  // directly. It stays safe for the module graph: the service already imports
+  // NotebookMigrationLLM at its own top, so "ai" is loaded here either way, and
+  // the constructor only assigns its two arguments. Deliberately no vi.mock("ai")
+  // in this file -- migration-llm.spec.ts owns that, and module mocks leak between
+  // files under `isolate: false`.
+  it("wires a real client to the injected config and workflow-util service", () => {
+    const llm = (service as any).createMigrationLLM();
+
+    expect(llm).toBeInstanceOf(NotebookMigrationLLM);
+    // Identity, not shape: the two constructor parameters are both plain objects
+    // here, so only `toBe` notices if they are ever passed in the wrong order.
+    expect((llm as any).config).toBe(mockGuiConfigService);
+    expect((llm as any).workflowUtilService).toBe(TestBed.inject(WorkflowUtilService));
+  });
+
+  it("hands out a fresh client per call so one conversion cannot see another's state", () => {
+    expect((service as any).createMigrationLLM()).not.toBe((service as any).createMigrationLLM());
+  });
+
   // sendToAIGenerateWorkflow (enabled) — drives the NotebookMigrationLLM lifecycle.
   // The service builds the client through its createMigrationLLM() seam, so stub
   // that with a plain fake. This keeps the real NotebookMigrationLLM (and its "ai"

frontend/src/build-version.spec.ts

@@ -17,6 +17,8 @@
  * under the License.
  */
 
+import { createRequire } from "node:module";
+import { dirname, join } from "node:path";
 import { renderVersionArtifacts } from "../build-version";
 
 describe("build-version: renderVersionArtifacts", () => {
@@ -77,3 +79,142 @@ describe("build-version: renderVersionArtifacts", () => {
     });
   });
 });
+
+// The suite above covers the pure renderer. What actually runs in CI is the
+// script's entry point: `node build-version.js`, whose main() decides WHICH of
+// the two rendered artifacts goes to WHICH path. Angular's production
+// fileReplacements reads src/environments/version.prod.ts and the running app
+// fetches src/assets/version.json, so crossing those two writes would ship a
+// bundle whose version banner never matches its manifest -- and nothing else in
+// the repo would notice, because both files are gitignored build outputs.
+//
+// main() is deliberately not exported (module.exports exposes only
+// renderVersionArtifacts), so the only way in without editing the script is to
+// load it the way Node does when it is the program: build a Module, install it
+// as process.mainModule, and let the `require.main === module` guard fire.
+describe("build-version: main()", () => {
+  // A CJS require. It resolves to the same module instances build-version.js
+  // itself requires, which is what makes the fs stub below land on the object
+  // the script destructures `writeFileSync` out of. (An `import * as fs` would
+  // not: esbuild refuses assignment to an import binding, and it would be a
+  // different namespace object anyway.)
+  //
+  // Anchored on the Vitest root -- frontend/, where build-version.js sits --
+  // rather than on import.meta.url, which is NOT stable here: the unit-test
+  // builder rewrites this spec to a synthetic frontend/spec-build-version.js
+  // under `--coverage` and leaves it at src/build-version.spec.ts without, so a
+  // "../" relative to it lands in a different directory in each mode.
+  const requireCjs = createRequire(join(process.cwd(), "build-version.spec.anchor.cjs"));
+  const NodeModule = requireCjs("node:module") as any;
+  const fs = requireCjs("fs") as { writeFileSync: unknown };
+  const scriptPath: string = requireCjs.resolve("./build-version.js");
+
+  type Write = { path: string; data: string };
+
+  // Runs build-version.js exactly as `node build-version.js` would, with
+  // writeFileSync and console.log captured instead of hitting the disk.
+  function loadScript(asMain: boolean): { writes: Write[]; logs: string[] } {
+    const writes: Write[] = [];
+    const logs: string[] = [];
+
+    const realWriteFileSync = fs.writeFileSync;
+    const realMainModule = process.mainModule;
+    const cachedEntry = NodeModule._cache[scriptPath];
+    const logSpy = vi.spyOn(console, "log").mockImplementation((...args: unknown[]) => {
+      logs.push(args.join(" "));
+    });
+
+    // Stubbed before the load, because the script destructures writeFileSync at
+    // module-evaluation time -- a later swap would be invisible to it.
+    fs.writeFileSync = (target: unknown, data: unknown) =>
+      void writes.push({ path: String(target), data: String(data) });
+
+    try {
+      // A fresh Module: the guard compares object identity, so the script must
+      // be evaluated again rather than served from the require cache.
+      delete NodeModule._cache[scriptPath];
+      const scriptModule = new NodeModule(scriptPath, null);
+      scriptModule.filename = scriptPath;
+      scriptModule.paths = NodeModule._nodeModulePaths(dirname(scriptPath));
+      // `require.main` is whatever process.mainModule holds. When the script is
+      // NOT the program, some OTHER module is -- that is the situation the guard
+      // exists for -- so stand a different module up rather than leaving
+      // process.mainModule unset, which would let a guard as loose as
+      // `require.main != null` pass for the wrong reason.
+      process.mainModule = asMain ? scriptModule : new NodeModule(scriptPath + ".importer.js", null);
+      NodeModule._cache[scriptPath] = scriptModule;
+      scriptModule.load(scriptPath);
+      return { writes, logs };
+    } finally {
+      fs.writeFileSync = realWriteFileSync;
+      process.mainModule = realMainModule;
+      if (cachedEntry) {
+        NodeModule._cache[scriptPath] = cachedEntry;
+      } else {
+        delete NodeModule._cache[scriptPath];
+      }
+      logSpy.mockRestore();
+    }
+  }
+
+  it("sends the bundle constant and the manifest to the two paths the prod build reads", () => {
+    const { writes } = loadScript(true);
+
+    expect(writes.length).toBe(2);
+
+    // Each artifact is matched to its own destination, not merely counted: a
+    // version that wrote the right number of files to the right two paths with
+    // the contents swapped would still be broken.
+    const prodTsWrite = writes.find(w => w.path.endsWith("version.prod.ts"));
+    const manifestWrite = writes.find(w => w.path.endsWith("version.json"));
+    expect(prodTsWrite).toBeDefined();
+    expect(manifestWrite).toBeDefined();
+
+    // The whole path, not a tail of it. A tail match cannot tell
+    // frontend/src/environments/version.prod.ts apart from the same suffix one
+    // directory up, and only the first is where Angular's fileReplacements
+    // looks and where the app fetches the manifest from -- a base directory the
+    // script resolved wrongly would ship the static "dev" version silently.
+    // Anchored on the script's own directory rather than process.cwd() so it
+    // holds in both runner modes.
+    expect(prodTsWrite!.path).toBe(join(dirname(scriptPath), "src", "environments", "version.prod.ts"));
+    expect(prodTsWrite!.data).toContain("export const Version");
+    expect(prodTsWrite!.data).toContain("AUTO-GENERATED");
+
+    expect(manifestWrite!.path).toBe(join(dirname(scriptPath), "src", "assets", "version.json"));
+    const manifest = JSON.parse(manifestWrite!.data);
+
+    // The version comes from package.json, not from a literal in the script.
+    const pkgVersion: string = requireCjs("./package.json").version;
+    expect(manifest.version).toBe(pkgVersion);
+
+    // An anchor that does NOT come out of main()'s own output. Every other
+    // assertion here reads the build number back from what main() wrote, so
+    // they agree with each other no matter what it was; only this one says what
+    // the value has to BE. The generator's contract is `<version>.<digits>`, so
+    // a main() that passed a constant, or passed the version as its own build
+    // number, fails here and nowhere else.
+    expect(manifest.buildNumber.startsWith(`${pkgVersion}.`)).toBe(true);
+    expect(manifest.buildNumber.slice(pkgVersion.length + 1)).toMatch(/^\d+$/);
+
+    // The comparison the running app makes: the number baked into the bundle
+    // and the number served in the manifest have to be the same one.
+    expect(prodTsWrite!.data).toContain(JSON.stringify(manifest.buildNumber));
+    expect(prodTsWrite!.data).toContain(JSON.stringify(manifest.version));
+  });
+
+  it("announces the build number it produced", () => {
+    const { writes, logs } = loadScript(true);
+    const manifest = JSON.parse(writes.find(w => w.path.endsWith("version.json"))!.data);
+    expect(logs).toEqual([`build-version: ${manifest.buildNumber}`]);
+  });
+
+  it("writes nothing when the script is merely required rather than run", () => {
+    // The other half of the `require.main === module` guard: importing the
+    // module for its renderer -- which this spec's own top-level import does --
+    // must not touch the working tree.
+    const { writes, logs } = loadScript(false);
+    expect(writes).toEqual([]);
+    expect(logs).toEqual([]);
+  });
+});