fix: address Copilot review round 7

Two scanner bypasses and a wrong claim in the testing skill.

The call scan used `\b` before the binding name, and a word boundary succeeds
straight after a dot — so `other.configure({ asyncUtilTimeout: 1000 })` satisfied
the check while the imported binding was never called at all. A negative
lookbehind excludes a member access and any identifier the name is merely the
tail of; both shapes are now tested.

The retry scan required a colon, so JavaScript's shorthand property form
`it("x", { retry }, …)` — the same declaration with the value bound above — went
through. Both forms are recognized now, and `{ retryCount }` still does not
match.

The testing skill told developers the auto-settle "no-ops under fake timers".
It throws, deliberately, so a deadlock reports immediately instead of hanging
until the project's hookTimeout. Documenting it as a silent no-op is the more
dangerous direction to be wrong in: someone combining the two would expect
degraded cleanup and get a failed test. The skill now says which it is and why.

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

.claude/skills/testing/SKILL.md

@@ -259,8 +259,12 @@ renders. The rule stands on consistency, not on timer safety.
   the helper's shared slack** — `HEADER_ANIM_MS + RAF_SLACK_MS`, both imported,
   never a literal: the first term tracks the component and the second tracks how
   busy the machine is, and only the second should move when the machine gets
-  busier (#2323). Do **not** also use `vi.useFakeTimers()` in that test
+  busier (#2323). Do **not** also use `vi.useFakeTimers()` in that test:
-  (the auto-settle no-ops under fake timers), and if the test unmounts the tree
+  the auto-settle awaits a real `setTimeout`, so under fake timers it **throws**
+  with a message telling you to call `vi.useRealTimers()` first — it does not
+  silently skip. That is deliberate (a deadlock would otherwise hang until the
+  project's `hookTimeout`), but it means the combination fails the test rather
+  than degrading. If the test unmounts the tree
   itself use the `unmount()` the helper returns. The mechanism is documented at
   length on the helper — read there before changing it.
 

scripts/verify-test-timeouts.mjs

@@ -318,8 +318,12 @@ export function checkAsyncUtilSource(
   // is something else entirely (Copilot). Requiring all of them to agree is
   // stricter than checking the last and gives a clearer message than "the
   // effective value is X" would.
+  // ⚠️ `(?<![.\\w$])` and not `\\b`: a word boundary succeeds straight after a
+  // dot, so `other.configure({ … })` matched the imported name and satisfied the
+  // check while the imported binding was never called at all (Copilot). Excluded
+  // are a member access and any identifier this name is merely the tail of.
   const calls = [];
-  const nameRe = new RegExp(`\\b${name}\\s*\\(`, "g");
+  const nameRe = new RegExp(`(?<![.\\w$])${name}\\s*\\(`, "g");
   let m;
   while ((m = nameRe.exec(code)) !== null) {
     const open = m.index + m[0].length - 1;
@@ -524,8 +528,13 @@ function readRetryOption(code, i) {
   const end = matchBracket(code, k);
   if (end === -1) return null;
   const options = code.slice(k + 1, end);
-  const retry = /(?:^|[\s,{])retry\s*:\s*([^,}\s]+)/.exec(options);
+  // Two forms, because JavaScript has two. `retry: 2` and the shorthand
-  return retry ? `retry: ${retry[1]}` : null;
+  // `{ retry }`, which is the same declaration with the value bound above and
+  // which a colon-requiring pattern misses entirely (Copilot).
+  const explicit = /(?:^|[\s,{])retry\s*:\s*([^,}\s]+)/.exec(options);
+  if (explicit) return `retry: ${explicit[1]}`;
+  const shorthand = /(?:^|[\s,{])retry\s*(?:,|$)/.test(options);
+  return shorthand ? "retry (shorthand)" : null;
 }
 
 /**

scripts/verify-test-timeouts.test.mjs

@@ -345,6 +345,8 @@ test("a retry that is not a test option is not a false positive", () => {
     `vi.mock("x", () => ({ retry: 2 }));`,
     `it("has retry: 2 in the name", () => {});`,
     `const o = { noRetry: 2 };`,
+    // Shorthand for a DIFFERENT property must not match the shorthand form.
+    `it("x", { retryCount }, () => {});`,
   ]) {
     assert.deepEqual(findTestLevelRetries(src), [], src);
   }
@@ -411,3 +413,34 @@ configure({ testIdAttribute: "data-testid" });
 configure({ asyncUtilTimeout: 1000 });`;
   assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), []);
 });
+
+test("the shorthand property form is caught", () => {
+  // `{ retry }` is the same declaration as `{ retry: 2 }` with the value bound
+  // above, and a colon-requiring pattern misses it entirely (Copilot).
+  assert.deepEqual(
+    findTestLevelRetries('const retry = 2; it("x", { retry }, () => {});'),
+    ["retry (shorthand)"],
+  );
+  assert.deepEqual(
+    findTestLevelRetries('it("x", { timeout: 100, retry }, () => {});'),
+    ["retry (shorthand)"],
+  );
+});
+
+test("a member call is not mistaken for the imported binding", () => {
+  // `\\b` succeeds straight after a dot, so `other.configure(…)` satisfied the
+  // check while the imported binding was never called at all (Copilot).
+  const source = `import { configure } from "storybook/test";
+other.configure({ asyncUtilTimeout: 1000 });`;
+  assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), [
+    "does not call configure()",
+  ]);
+});
+
+test("a longer identifier ending in the binding name is not a call to it", () => {
+  const source = `import { configure } from "storybook/test";
+reconfigure({ asyncUtilTimeout: 1000 });`;
+  assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), [
+    "does not call configure()",
+  ]);
+});