From 4d7dd180d9d1886e256985ae0380c68ba62cc44f Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 01:40:28 -0400 Subject: [PATCH] fix: address Copilot review round 13 on #2251 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY Signed-off-by: cliffhall --- clients/web/src/App.tsx | 5 ++ .../SkillsScreen/SkillsScreen.test.tsx | 62 +++++++++++++++++++ .../screens/SkillsScreen/SkillsScreen.tsx | 33 +++++++--- .../InspectorView/InspectorView.stories.tsx | 1 + .../InspectorView/InspectorView.test.tsx | 1 + .../views/InspectorView/InspectorView.tsx | 2 + .../components/views/InspectorView/types.ts | 6 ++ .../web/src/hooks/useInspectorStores.test.tsx | 15 +++++ clients/web/src/hooks/useInspectorStores.ts | 17 +++++ 9 files changed, 135 insertions(+), 7 deletions(-) diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 617ce438..ad853413 100644 --- a/clients/web/src/App.tsx +++ b/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, diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index b97608cc..b70067f5 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/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( + , + ); + await user.click(screen.getByRole("button", { name: /Verify all/ })); + + // Same entry, different session. + rerender( + , + ); + 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( + , + ); + 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(); diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index b882942b..35a39f1f 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/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 ( diff --git a/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx b/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx index 3ca51998..f8b2445a 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx +++ b/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, diff --git a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx index 4f784cb5..d717dcfb 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx +++ b/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, diff --git a/clients/web/src/components/views/InspectorView/InspectorView.tsx b/clients/web/src/components/views/InspectorView/InspectorView.tsx index d620333e..ff7d8a0c 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.tsx +++ b/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, diff --git a/clients/web/src/components/views/InspectorView/types.ts b/clients/web/src/components/views/InspectorView/types.ts index 15e2a249..1eff1468 100644 --- a/clients/web/src/components/views/InspectorView/types.ts +++ b/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; diff --git a/clients/web/src/hooks/useInspectorStores.test.tsx b/clients/web/src/hooks/useInspectorStores.test.tsx index 2ec027e2..d13713cc 100644 --- a/clients/web/src/hooks/useInspectorStores.test.tsx +++ b/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([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)); diff --git a/clients/web/src/hooks/useInspectorStores.ts b/clients/web/src/hooks/useInspectorStores.ts index 3ad32fad..bff13c9f 100644 --- a/clients/web/src/hooks/useInspectorStores.ts +++ b/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(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(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,