packages/typescript/ai-code-mode/src/validate-bindings.ts (1)

91-136: Optional: shared seen across siblings suppresses reports for duplicated $ref/subschema references.

seen is threaded through the entire traversal, so once a subschema object (e.g. a $defs/Creds target) has been visited via one reference site, any other reference site to the same object is skipped entirely. In practice this means: if creds1 and creds2 both $ref the same definition that contains accessToken, only creds1.accessToken will be reported; creds2.accessToken won't appear in found at all. With the per-instance dedupCache in create-code-mode-tool.ts, the user still gets a warning for the tool, so impact is low — but the reported paramPath list under-represents the real exposure surface.

If you want full coverage, track cycle-breaking per-path (e.g. a Set of object seeded only with ancestors on the current DFS stack, removed after recursion) rather than a single global seen.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/typescript/ai-code-mode/src/validate-bindings.ts` around lines 91 -
136, The traversal currently uses a global `seen` that permanently marks visited
subschema objects, which suppresses reporting when the same schema object is
referenced from different paths; in function findSecretParams change `seen` to
act as an ancestor/stack-local cycle detector by removing the schema from `seen`
before returning (i.e., after all recursive calls complete, call
seen.delete(schema)), so the object is only treated as visited for the current
DFS path and other sibling/reference sites will be explored; keep using the same
`seen` Set parameter but ensure you delete the schema at the end of
findSecretParams.

packages/typescript/ai-code-mode/tests/validate-bindings.test.ts (4)

333-348: Also assert no console.warn on the 'throw' path.

This test doesn’t spy on console.warn, so two things are left unverified/unsafe:

1. If the implementation happens to call console.warn before throwing, that output leaks into the test runner’s stderr.
2. The intended contract — handler: 'throw' throws instead of warning — isn’t actually asserted.

Adding a spy and a not.toHaveBeenCalled() assertion tightens the contract:

♻️ Suggested change

   it('handler: "throw" throws on the first match', () => {
+    const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {})
     expect(() =>
       warnIfBindingsExposeSecrets(
         [
           {
             name: 't',
             inputSchema: {
               type: 'object',
               properties: { apiKey: { type: 'string' } },
             },
           },
         ],
         { handler: 'throw' },
       ),
     ).toThrow(/apiKey/)
+    expect(warnSpy).not.toHaveBeenCalled()
+    warnSpy.mockRestore()
   })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/typescript/ai-code-mode/tests/validate-bindings.test.ts` around
lines 333 - 348, Update the test for handler: "throw" to also spy on
console.warn and assert it was not called: before invoking
warnIfBindingsExposeSecrets create a jest.spyOn(console,
'warn').mockImplementation(() => {}) (or equivalent test spy), run the function
expecting toThrow(/apiKey/), then assert the spy.not.toHaveBeenCalled() and
finally restore the spy (spy.mockRestore()) to avoid cross-test pollution;
reference the test case and the warnIfBindingsExposeSecrets invocation to locate
where to add the spy and assertions.

---

350-383: Order-dependent assertion on matches[0]/matches[1].

toHaveLength(2) plus indexed toMatchObject assumes the traversal emits apiKey strictly before nested.token. That happens to match insertion order today, but it couples the test to the implementation’s DFS order rather than the observable contract ("each match is reported exactly once"). Consider asserting set-membership instead so a later traversal refactor (e.g., BFS, anyOf reordering) doesn't break the test:

♻️ Suggested change

-    expect(matches).toHaveLength(2)
-    expect(matches[0]).toMatchObject({
-      toolName: 'callApi',
-      paramName: 'apiKey',
-      paramPath: ['apiKey'],
-    })
-    expect(matches[1]).toMatchObject({
-      toolName: 'callApi',
-      paramName: 'token',
-      paramPath: ['nested', 'token'],
-    })
+    expect(matches).toHaveLength(2)
+    expect(matches).toEqual(
+      expect.arrayContaining([
+        expect.objectContaining({
+          toolName: 'callApi',
+          paramName: 'apiKey',
+          paramPath: ['apiKey'],
+        }),
+        expect.objectContaining({
+          toolName: 'callApi',
+          paramName: 'token',
+          paramPath: ['nested', 'token'],
+        }),
+      ]),
+    )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/typescript/ai-code-mode/tests/validate-bindings.test.ts` around
lines 350 - 383, The test currently assumes traversal order by checking
matches[0] and matches[1]; update the assertions in the 'handler: function
receives each match' spec to be order-independent: collect the pushed
SecretParameterInfo objects from warnIfBindingsExposeSecrets (the matches array)
and assert that it has length 2 and contains both expected objects using
set-membership style assertions (e.g., expect.arrayContaining or multiple
expect(matches).toContainEqual calls) referencing the same toolName 'callApi'
and paramName/paramPath shapes rather than relying on index-based checks.

---

80-161: Nice coverage of naming variants — consider one more edge case.

The secretNames / safeNames matrix is thorough. One gap worth adding given the PR’s goal of catching "the common antipattern of passing API keys as tool parameters": a case-insensitive variant of a compound name like OPENAI_API_KEY or SLACK_WEBHOOK_SECRET (SCREAMING_SNAKE_CASE env-style names often copy-pasted into schemas). If the detector already normalizes case, it’s a free test; if it doesn’t, this surfaces a real gap.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/typescript/ai-code-mode/tests/validate-bindings.test.ts` around
lines 80 - 161, Add tests covering SCREAMING_SNAKE_CASE secret-like names so the
detector handles case-insensitive compound env-style names: extend the
secretNames array used by the warnIfBindingsExposeSecrets tests to include
examples like 'OPENAI_API_KEY' and 'SLACK_WEBHOOK_SECRET' (or other UPPER_SNAKE
variants), then ensure the existing test that expects a warning for each
secretName still passes; reference the warnIfBindingsExposeSecrets call and the
secretNames array in validate-bindings.test.ts to locate where to add these
entries.

---

7-7: Consider centralizing console.warn spy restoration via afterEach.

Every test manually creates a vi.spyOn(console, 'warn') and calls warnSpy.mockRestore() at the end. If any expect in a test fails, mockRestore() is skipped and the spy leaks into subsequent tests, which can mask or alter assertions (e.g., toHaveBeenCalledTimes in the dedup suite). A single afterEach(() => vi.restoreAllMocks()) (plus removing the per-test mockRestore() calls) makes the suite leak-safe and a bit less repetitive.

♻️ Sketch

 import { describe, expect, it, vi } from 'vitest'
+import { afterEach } from 'vitest'
 ...
+afterEach(() => {
+  vi.restoreAllMocks()
+})

Also applies to: 27-27, 47-47, 71-71, 110-110, 146-146, 165-165, 189-189, 216-216, 248-248, 275-275, 297-297, 314-314, 388-388, 407-407

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/typescript/ai-code-mode/tests/validate-bindings.test.ts` at line 7,
Centralize console.warn spy cleanup by adding an afterEach(() =>
vi.restoreAllMocks()) in the test suite and remove all per-test
warnSpy.mockRestore() calls; keep the existing vi.spyOn(console, 'warn') usages
(e.g., the const warnSpy = vi.spyOn(console, 'warn') lines) but rely on global
restoration so tests won't leak spies and assertions like toHaveBeenCalledTimes
remain reliable.