fix: address Copilot review round 13 on #2251

- SkillsScreen: put the SESSION in the invalidation key. This screen stays
  mounted across a disconnect, so content alone cannot tell server A's entry
  from an identical-looking one on server B — A's in-flight verification
  could land afterwards and report `verified` for a file never read from B,
  and a retained batch entry could leave B's Verify all disabled.
  `useInspectorStores` now exposes a `sessionNonce`, bumped on both create
  and destroy so it never repeats across a reconnect, and App keys on
  `${activeServerId}:${sessionNonce}` — the server id alone would repeat,
  which is one of the crossings this exists to prevent.
- SkillsScreen: match the selection by `skillUriIdentity`, in both the
  lookup and the NavLink active check. A refresh that canonicalizes
  `skill://demo/%53KILL.md` names the same skill, and the detail pane was
  emptying out because the server changed its spelling — the last raw-string
  URI comparison left after round 12.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>

clients/web/src/App.tsx

@@ -459,6 +459,7 @@ function App() {
     tasks,
     refreshTasks,
     clearCompletedTasks,
+    sessionNonce,
     skills,
     skillsPageCount,
     skillsLoadError,
@@ -1826,6 +1827,10 @@ function App() {
   };
 
   const skillsPanelProps: SkillsPanelProps = {
+    // Server id AND per-connect nonce: the id alone would repeat on a
+    // reconnect to the same server, which is one of the crossings this key
+    // exists to prevent.
+    skillsSessionKey: `${activeServerId ?? ""}:${sessionNonce}`,
     skills,
     skillsPageCount,
     skillsLoadError,

clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx

@@ -101,6 +101,7 @@ const readFixtureFile = vi.fn(async (uri: string) => {
 });
 
 const baseProps: SkillsScreenProps = {
+  sessionKey: "session-1",
   skills: ALL_SKILLS,
   pageCount: 2,
   ui: EMPTY_SKILLS_UI,
@@ -718,6 +719,67 @@ describe("SkillsScreen", () => {
     expect(verifyAll()).toBeDisabled();
   });
 
+  it("discards a verification that lands after the session changed", async () => {
+    // This screen stays mounted across a disconnect, so content alone does not
+    // tell server A's entry from an identical-looking one on server B. Without
+    // the session in the key, A's in-flight read would land and report
+    // `verified` for a file that was never read from B.
+    const user = userEvent.setup();
+    let release: ((value: { text: string }) => void) | undefined;
+    const onReadSkillFile = vi.fn(
+      () =>
+        new Promise<{ text: string }>((resolve) => {
+          release = resolve;
+        }),
+    );
+    const { rerender } = renderWithMantine(
+      <SkillsScreen
+        {...baseProps}
+        sessionKey="server-a:1"
+        onReadSkillFile={onReadSkillFile}
+        skills={[CLEAN_SKILL]}
+        ui={{ ...EMPTY_SKILLS_UI, selectedSkillUri: CLEAN_SKILL.uri }}
+      />,
+    );
+    await user.click(screen.getByRole("button", { name: /Verify all/ }));
+
+    // Same entry, different session.
+    rerender(
+      <SkillsScreen
+        {...baseProps}
+        sessionKey="server-b:2"
+        onReadSkillFile={onReadSkillFile}
+        skills={[CLEAN_SKILL]}
+        ui={{ ...EMPTY_SKILLS_UI, selectedSkillUri: CLEAN_SKILL.uri }}
+      />,
+    );
+    release?.({ text: SELF_TEXT });
+    expect(screen.queryByText("verified")).not.toBeInTheDocument();
+    // ...and the batch guard did not carry over either.
+    expect(
+      screen.getByRole("button", { name: /Verify all/ }),
+    ).not.toBeDisabled();
+  });
+
+  it("keeps the selection when a refresh canonicalizes the skill's URI", async () => {
+    // The selection is stored as the URI the list gave us, so a server that
+    // re-spells it must not empty the detail pane for the same skill.
+    renderWithMantine(
+      <SkillsScreen
+        {...baseProps}
+        skills={[CLEAN_SKILL]}
+        ui={{
+          ...EMPTY_SKILLS_UI,
+          selectedSkillUri: "skill://data-analysis/%53KILL.md",
+        }}
+      />,
+    );
+    expect(screen.getByTestId("skill-detail")).toBeInTheDocument();
+    expect(
+      screen.queryByText("Select a skill to view details"),
+    ).not.toBeInTheDocument();
+  });
+
   it("shows the SKILL.md preview on demand", async () => {
     const user = userEvent.setup();
     renderWithMantine(<ControlledSkillsScreen />);

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx

@@ -108,6 +108,14 @@ interface FetchedEntryState {
 }
 
 export interface SkillsScreenProps {
+  /**
+   * Identity of the connected session. Part of the invalidation key below, so
+   * a verification still in flight when the user switches servers cannot land
+   * afterwards and report a verdict for an identical-looking entry on the new
+   * one — this screen stays mounted across a disconnect, so content alone does
+   * not distinguish the two.
+   */
+  sessionKey: string;
   skills: SkillEntry[];
   /** Pages the last `skills/list` walk took; shown so pagination is visible. */
   pageCount: number;
@@ -320,6 +328,7 @@ function shortDigest(digest: string | undefined): string {
  * the Inspector fetches only what the user asks it to.
  */
 export function SkillsScreen({
+  sessionKey,
   skills,
   pageCount,
   loadError,
@@ -378,10 +387,15 @@ export function SkillsScreen({
     );
   }, [skills, search]);
 
-  const selected = useMemo(
-    () => skills.find((skill) => skill.uri === selectedSkillUri),
-    [skills, selectedSkillUri],
-  );
+  // Matched by IDENTITY, like every other URI comparison here: a refresh that
+  // canonicalizes `skill://demo/%53KILL.md` to `skill://demo/SKILL.md` names
+  // the same skill, and the detail pane must not empty out because the server
+  // changed its spelling.
+  const selected = useMemo(() => {
+    if (selectedSkillUri === undefined) return undefined;
+    const wanted = skillUriIdentity(selectedSkillUri);
+    return skills.find((skill) => skillUriIdentity(skill.uri) === wanted);
+  }, [skills, selectedSkillUri]);
 
   const issues = useMemo(
     () => (selected ? checkSkillConformance(selected) : []),
@@ -406,8 +420,9 @@ export function SkillsScreen({
   // not. A primitive string, because `useValueChange` compares with `Object.is`
   // and a fresh object every render would loop.
   const manifestKey = useMemo(
-    () => (selected ? JSON.stringify(selected) : (selectedSkillUri ?? "")),
-    [selected, selectedSkillUri],
+    () =>
+      `${sessionKey}\n${selected ? JSON.stringify(selected) : (selectedSkillUri ?? "")}`,
+    [selected, selectedSkillUri, sessionKey],
   );
 
   // Adjusted DURING RENDER via `useValueChange` rather than in an effect, so a
@@ -623,7 +638,11 @@ export function SkillsScreen({
                 return (
                   <NavLink
                     key={skill.uri}
-                    active={skill.uri === selectedSkillUri}
+                    active={
+                      selectedSkillUri !== undefined &&
+                      skillUriIdentity(skill.uri) ===
+                        skillUriIdentity(selectedSkillUri)
+                    }
                     label={skillDisplayName(skill)}
                     description={skill.uri}
                     onClick={() =>

clients/web/src/components/views/InspectorView/InspectorView.stories.tsx

@@ -427,6 +427,7 @@ const appsArgs: AppsPanelProps = {
 };
 
 const skillsArgs: SkillsPanelProps = {
+  skillsSessionKey: "story-session",
   skills: [],
   skillsPageCount: 0,
   skillsUi: EMPTY_SKILLS_UI,

clients/web/src/components/views/InspectorView/InspectorView.test.tsx

@@ -172,6 +172,7 @@ function makeProps(...overrides: PropOverrides[]): InspectorViewProps {
       ...mergeBundle("apps", overrides),
     },
     skills: {
+      skillsSessionKey: "test-session",
       skills: [],
       skillsPageCount: 0,
       skillsUi: EMPTY_SKILLS_UI,

clients/web/src/components/views/InspectorView/InspectorView.tsx

@@ -456,6 +456,7 @@ export function InspectorView({
     onRefreshApps,
   } = appsPanel;
   const {
+    skillsSessionKey,
     skills,
     skillsPageCount,
     skillsLoadError,
@@ -1055,6 +1056,7 @@ export function InspectorView({
     onSortChange: setConsoleSort,
   };
   const skillsScreenProps = {
+    sessionKey: skillsSessionKey,
     skills,
     pageCount: skillsPageCount,
     loadError: skillsLoadError,

clients/web/src/components/views/InspectorView/types.ts

@@ -313,6 +313,12 @@ export interface AppsPanelProps {
 
 /** The Skills screen (SEP-2640): the enumerated skills and their verification. */
 export interface SkillsPanelProps {
+  /**
+   * Identity of the connected session. Part of the screen's invalidation key,
+   * so async verification state can never cross a connection — see
+   * `UseInspectorStoresResult.sessionNonce`.
+   */
+  skillsSessionKey: string;
   skills: SkillEntry[];
   /** Pages the last `skills/list` walk took. */
   skillsPageCount: number;

clients/web/src/hooks/useInspectorStores.test.tsx

@@ -257,6 +257,21 @@ describe("useInspectorStores", () => {
       expect(h.api().stores).not.toBe(first);
     });
 
+    it("advances the session nonce on both create and destroy", () => {
+      // It names one connected session and must never repeat across a
+      // reconnect — `SkillsScreen` keys async verification state on it, and a
+      // repeated value would let one session's result land in another.
+      const h = harness();
+      const seen = new Set<number>([h.api().sessionNonce]);
+      h.run((api) => api.createStores(client(), fetchLogOptions));
+      seen.add(h.api().sessionNonce);
+      h.run((api) => api.destroyStores());
+      seen.add(h.api().sessionNonce);
+      h.run((api) => api.createStores(client(), fetchLogOptions));
+      seen.add(h.api().sessionNonce);
+      expect(seen.size).toBe(4);
+    });
+
     it("destroys and clears on destroyStores", () => {
       const h = harness();
       h.run((api) => api.createStores(client(), fetchLogOptions));

clients/web/src/hooks/useInspectorStores.ts

@@ -86,6 +86,17 @@ export type FetchLogOptions = Pick<
 export interface UseInspectorStoresResult {
   /** The live stores, or `null` before the first connect / after teardown. */
   stores: InspectorStores | null;
+  /**
+   * Bumped on every `createStores` **and** every `destroyStores`, so it names
+   * one connected session and never repeats across a reconnect.
+   *
+   * Screens that hold async state keyed by *content* need this in the key:
+   * `SkillsScreen` stays mounted across a disconnect, so a verification still
+   * in flight for server A could otherwise land after a switch to server B and
+   * report a verdict for an identical-looking entry that was never read from B
+   * (#2234).
+   */
+  sessionNonce: number;
   /**
    * Build a fresh set of stores for `client`, tearing down whatever set is
    * live first. Stable, so callers need no dependency on the current stores.
@@ -162,6 +173,9 @@ export function useInspectorStores({
   paginatedLists,
 }: UseInspectorStoresParams): UseInspectorStoresResult {
   const [stores, setStores] = useState<InspectorStores | null>(null);
+  // See `sessionNonce` above. A counter rather than the store object's identity
+  // because it has to be usable as part of a string key.
+  const [sessionNonce, setSessionNonce] = useState(0);
   // Mirrors `stores` so `destroyStores` can read the live set without taking a
   // dependency on it — which is what keeps every caller's callback stable.
   const storesRef = useRef<InspectorStores | null>(null);
@@ -177,6 +191,7 @@ export function useInspectorStores({
     storesRef.current = null;
     fetchLogRef.current = null;
     setStores(null);
+    setSessionNonce((n) => n + 1);
   }, []);
 
   const createStores = useCallback(
@@ -214,6 +229,7 @@ export function useInspectorStores({
       storesRef.current = next;
       fetchLogRef.current = fetchRequestLogState;
       setStores(next);
+      setSessionNonce((n) => n + 1);
     },
     [destroyStores],
   );
@@ -340,6 +356,7 @@ export function useInspectorStores({
 
   return {
     stores,
+    sessionNonce,
     createStores,
     destroyStores,
     fetchLogRef,