-
-
Notifications
You must be signed in to change notification settings - Fork 187
Audit remediation: tests, isolate hardening, framework fixes, error hygiene #465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
5004ca1
test(ai-code-mode-skills): add unit test coverage for skill library
AlemTuzlak fee6465
feat(ai-isolate-cloudflare): support production deployments and harde…
AlemTuzlak 332901d
refactor(ai-ollama): extract tool-converter with test coverage
AlemTuzlak 9431209
fix(frameworks): propagate useChat callback changes after re-render
AlemTuzlak 22135e0
refactor(ai, ai-openai): narrow error handling and stop logging raw e…
AlemTuzlak 4f16917
test(isolates): add sandbox escape-attempt tests for Node and QuickJS…
AlemTuzlak bfa399a
ci: apply automated fixes
autofix-ci[bot] e65e2a8
fix: address PR review feedback
AlemTuzlak 1c196da
ci: apply automated fixes
autofix-ci[bot] 00d7705
Merge remote-tracking branch 'origin/main' into worktree-golden-hummi…
AlemTuzlak 145c026
fix(ai, ai-ollama): merge-driven regressions from CR
AlemTuzlak File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| --- | ||
| '@tanstack/ai': patch | ||
| '@tanstack/ai-openai': patch | ||
| --- | ||
|
|
||
| refactor(ai, ai-openai): narrow error handling and stop logging raw errors | ||
|
|
||
| `catch (error: any)` sites in `stream-to-response.ts`, `activities/stream-generation-result.ts`, and `activities/generateVideo/index.ts` are now narrowed to `unknown` and funnel through a shared `toRunErrorPayload(error, fallback)` helper that extracts `message` / `code` without leaking the original error object (which can carry request state from an SDK). | ||
|
|
||
| Removed four `console.error` calls in the OpenAI text adapter's `chatStream` catch block that dumped the full error object to stdout. SDK errors can carry the original request including auth headers, so the library now re-throws without logging; upstream callers should convert errors into structured events themselves. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| '@tanstack/ai-isolate-cloudflare': patch | ||
| --- | ||
|
|
||
| feat(ai-isolate-cloudflare): support production deployments and close tool-name injection vector | ||
|
|
||
| The Worker now documents production-capable `unsafe_eval` usage (previously the code, wrangler.toml, and README all described it as dev-only). Tool names are validated against a strict identifier regex before being interpolated into the generated wrapper code, so a malicious tool name like `foo'); process.exit(1); (function bar() {` is rejected at generation time rather than breaking out of the wrapping function. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| '@tanstack/ai-ollama': patch | ||
| --- | ||
|
|
||
| refactor(ai-ollama): extract tool conversion into `src/tools/` matching peer adapters | ||
|
|
||
| Tool handling lived inline inside the text adapter with raw type casts. It is now split into a dedicated `tool-converter.ts` / `function-tool.ts` pair (mirroring the structure used by `ai-openai`, `ai-anthropic`, `ai-grok`, and `ai-groq`) and re-exported from the package index as `convertFunctionToolToAdapterFormat` and `convertToolsToProviderFormat`. Runtime behavior is unchanged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| --- | ||
| '@tanstack/ai-react': patch | ||
| '@tanstack/ai-preact': patch | ||
| '@tanstack/ai-vue': patch | ||
| '@tanstack/ai-solid': patch | ||
| --- | ||
|
|
||
| fix(ai-react, ai-preact, ai-vue, ai-solid): propagate `useChat` callback changes | ||
|
|
||
| `onResponse`, `onChunk`, and `onCustomEvent` were captured by reference at client creation time. When a parent component re-rendered with fresh closures, the `ChatClient` kept calling the originals. Every framework now wraps these callbacks so the latest `options.xxx` is read at call time (via `optionsRef.current` in React/Preact, and direct option access in Vue/Solid, matching the pattern already used for `onFinish` / `onError`). Clearing a callback (setting it to `undefined`) now correctly no-ops instead of continuing to invoke the stale handler. | ||
302 changes: 302 additions & 0 deletions
302
packages/typescript/ai-code-mode-skills/tests/create-skill-management-tools.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,302 @@ | ||
| import { describe, expect, it, vi } from 'vitest' | ||
| import { createSkillManagementTools } from '../src/create-skill-management-tools' | ||
| import { createMemorySkillStorage } from '../src/storage/memory-storage' | ||
| import { | ||
| createAlwaysTrustedStrategy, | ||
| createDefaultTrustStrategy, | ||
| } from '../src/trust-strategies' | ||
|
|
||
| const mockContext = () => ({ emitCustomEvent: vi.fn() }) | ||
|
|
||
| function getTool( | ||
| tools: ReturnType<typeof createSkillManagementTools>, | ||
| name: string, | ||
| ) { | ||
| const tool = tools.find((t) => t.name === name) | ||
| if (!tool) throw new Error(`Tool ${name} not found`) | ||
| return tool | ||
| } | ||
|
|
||
| function validRegisterInput( | ||
| overrides: Partial<{ | ||
| name: string | ||
| description: string | ||
| code: string | ||
| inputSchema: string | ||
| outputSchema: string | ||
| usageHints: Array<string> | ||
| dependsOn: Array<string> | ||
| }> = {}, | ||
| ) { | ||
| return { | ||
| name: 'fetch_data', | ||
| description: 'A skill', | ||
| code: 'return input;', | ||
| inputSchema: '{"type":"object","properties":{}}', | ||
| outputSchema: '{"type":"object","properties":{}}', | ||
| usageHints: ['Use for fetching'], | ||
| dependsOn: [], | ||
| ...overrides, | ||
| } | ||
| } | ||
|
|
||
| describe('createSkillManagementTools', () => { | ||
| it('exposes search_skills, get_skill, and register_skill', () => { | ||
| const storage = createMemorySkillStorage([]) | ||
| const tools = createSkillManagementTools({ storage }) | ||
| expect(tools.map((t) => t.name).sort()).toEqual([ | ||
| 'get_skill', | ||
| 'register_skill', | ||
| 'search_skills', | ||
| ]) | ||
| }) | ||
|
|
||
| describe('search_skills', () => { | ||
| it('returns lightweight matching entries', async () => { | ||
| const storage = createMemorySkillStorage([ | ||
| { | ||
| id: '1', | ||
| name: 'github_stats', | ||
| description: 'GitHub stats', | ||
| code: 'secret', | ||
| inputSchema: {}, | ||
| outputSchema: {}, | ||
| usageHints: ['for github'], | ||
| dependsOn: [], | ||
| trustLevel: 'untrusted', | ||
| stats: { executions: 0, successRate: 0 }, | ||
| createdAt: '', | ||
| updatedAt: '', | ||
| }, | ||
| ]) | ||
| const tools = createSkillManagementTools({ storage }) | ||
| const tool = getTool(tools, 'search_skills') | ||
| const results = (await tool.execute!( | ||
| { query: 'github', limit: 5 }, | ||
| mockContext() as any, | ||
| )) as Array<Record<string, unknown>> | ||
| expect(results).toHaveLength(1) | ||
| expect(results[0]).not.toHaveProperty('code') | ||
| expect(results[0]!.name).toBe('github_stats') | ||
| }) | ||
|
|
||
| it('respects the limit parameter', async () => { | ||
| const storage = createMemorySkillStorage([ | ||
| { | ||
| id: 'a', | ||
| name: 'data_one', | ||
| description: '', | ||
| code: '', | ||
| inputSchema: {}, | ||
| outputSchema: {}, | ||
| usageHints: [], | ||
| dependsOn: [], | ||
| trustLevel: 'untrusted', | ||
| stats: { executions: 0, successRate: 0 }, | ||
| createdAt: '', | ||
| updatedAt: '', | ||
| }, | ||
| { | ||
| id: 'b', | ||
| name: 'data_two', | ||
| description: '', | ||
| code: '', | ||
| inputSchema: {}, | ||
| outputSchema: {}, | ||
| usageHints: [], | ||
| dependsOn: [], | ||
| trustLevel: 'untrusted', | ||
| stats: { executions: 0, successRate: 0 }, | ||
| createdAt: '', | ||
| updatedAt: '', | ||
| }, | ||
| ]) | ||
| const tools = createSkillManagementTools({ storage }) | ||
| const tool = getTool(tools, 'search_skills') | ||
| const results = (await tool.execute!( | ||
| { query: 'data', limit: 1 }, | ||
| mockContext() as any, | ||
| )) as Array<unknown> | ||
| expect(results).toHaveLength(1) | ||
| }) | ||
| }) | ||
|
|
||
| describe('get_skill', () => { | ||
| it('returns an error object for a missing skill', async () => { | ||
| const storage = createMemorySkillStorage([]) | ||
| const tools = createSkillManagementTools({ storage }) | ||
| const tool = getTool(tools, 'get_skill') | ||
| const result = (await tool.execute!( | ||
| { name: 'missing' }, | ||
| mockContext() as any, | ||
| )) as { error?: string } | ||
| expect(result.error).toContain('not found') | ||
| }) | ||
|
|
||
| it('returns the full skill including code when found', async () => { | ||
| const storage = createMemorySkillStorage([ | ||
| { | ||
| id: '1', | ||
| name: 'alpha', | ||
| description: 'Alpha', | ||
| code: 'return 1;', | ||
| inputSchema: { type: 'object' }, | ||
| outputSchema: { type: 'number' }, | ||
| usageHints: ['hint'], | ||
| dependsOn: [], | ||
| trustLevel: 'untrusted', | ||
| stats: { executions: 0, successRate: 0 }, | ||
| createdAt: '', | ||
| updatedAt: '', | ||
| }, | ||
| ]) | ||
| const tools = createSkillManagementTools({ storage }) | ||
| const tool = getTool(tools, 'get_skill') | ||
| const result = (await tool.execute!( | ||
| { name: 'alpha' }, | ||
| mockContext() as any, | ||
| )) as { | ||
| name?: string | ||
| code?: string | ||
| inputSchema?: string | ||
| } | ||
| expect(result.name).toBe('alpha') | ||
| expect(result.code).toBe('return 1;') | ||
| expect(result.inputSchema).toBe('{"type":"object"}') | ||
| }) | ||
| }) | ||
|
|
||
| describe('register_skill', () => { | ||
| it('rejects names starting with external_', async () => { | ||
| const storage = createMemorySkillStorage([]) | ||
| const tools = createSkillManagementTools({ storage }) | ||
| const tool = getTool(tools, 'register_skill') | ||
| const result = (await tool.execute!( | ||
| validRegisterInput({ name: 'external_evil' }), | ||
| mockContext() as any, | ||
| )) as { error?: string } | ||
| expect(result.error).toContain("cannot start with 'external_'") | ||
| }) | ||
|
|
||
| it('rejects names starting with skill_ (redundant prefix)', async () => { | ||
| const storage = createMemorySkillStorage([]) | ||
| const tools = createSkillManagementTools({ storage }) | ||
| const tool = getTool(tools, 'register_skill') | ||
| const result = (await tool.execute!( | ||
| validRegisterInput({ name: 'skill_duplicate' }), | ||
| mockContext() as any, | ||
| )) as { error?: string } | ||
| expect(result.error).toContain("should not include the 'skill_' prefix") | ||
| }) | ||
|
|
||
| it('rejects malformed JSON inputSchema', async () => { | ||
| const storage = createMemorySkillStorage([]) | ||
| const tools = createSkillManagementTools({ storage }) | ||
| const tool = getTool(tools, 'register_skill') | ||
| const result = (await tool.execute!( | ||
| validRegisterInput({ inputSchema: 'not valid json' }), | ||
| mockContext() as any, | ||
| )) as { error?: string } | ||
| expect(result.error).toContain('inputSchema must be a valid JSON string') | ||
| }) | ||
|
|
||
| it('rejects malformed JSON outputSchema', async () => { | ||
| const storage = createMemorySkillStorage([]) | ||
| const tools = createSkillManagementTools({ storage }) | ||
| const tool = getTool(tools, 'register_skill') | ||
| const result = (await tool.execute!( | ||
| validRegisterInput({ outputSchema: '{' }), | ||
| mockContext() as any, | ||
| )) as { error?: string } | ||
| expect(result.error).toContain('outputSchema must be a valid JSON string') | ||
| }) | ||
|
|
||
| it('rejects a duplicate name', async () => { | ||
| const storage = createMemorySkillStorage([ | ||
| { | ||
| id: '1', | ||
| name: 'existing', | ||
| description: '', | ||
| code: '', | ||
| inputSchema: {}, | ||
| outputSchema: {}, | ||
| usageHints: [], | ||
| dependsOn: [], | ||
| trustLevel: 'untrusted', | ||
| stats: { executions: 0, successRate: 0 }, | ||
| createdAt: '', | ||
| updatedAt: '', | ||
| }, | ||
| ]) | ||
| const tools = createSkillManagementTools({ storage }) | ||
| const tool = getTool(tools, 'register_skill') | ||
| const result = (await tool.execute!( | ||
| validRegisterInput({ name: 'existing' }), | ||
| mockContext() as any, | ||
| )) as { error?: string } | ||
| expect(result.error).toContain('already exists') | ||
| }) | ||
|
|
||
| it('persists a valid skill with defaults', async () => { | ||
| const storage = createMemorySkillStorage([]) | ||
| const tools = createSkillManagementTools({ storage }) | ||
| const tool = getTool(tools, 'register_skill') | ||
| const result = (await tool.execute!( | ||
| validRegisterInput({ name: 'valid_skill' }), | ||
| mockContext() as any, | ||
| )) as { success?: boolean; skillId?: string } | ||
| expect(result.success).toBe(true) | ||
| expect(result.skillId).toMatch(/^[0-9a-f-]{36}$/) | ||
|
|
||
| const saved = await storage.get('valid_skill') | ||
| expect(saved).not.toBeNull() | ||
| expect(saved!.stats).toEqual({ executions: 0, successRate: 0 }) | ||
| }) | ||
|
|
||
| it('applies the trust strategy to set initial trust level', async () => { | ||
| const storage = createMemorySkillStorage([]) | ||
| const tools = createSkillManagementTools({ | ||
| storage, | ||
| trustStrategy: createAlwaysTrustedStrategy(), | ||
| }) | ||
| const tool = getTool(tools, 'register_skill') | ||
| await tool.execute!( | ||
| validRegisterInput({ name: 's1' }), | ||
| mockContext() as any, | ||
| ) | ||
| const saved = await storage.get('s1') | ||
| expect(saved!.trustLevel).toBe('trusted') | ||
| }) | ||
|
|
||
| it('prefers explicit trustStrategy over storage.trustStrategy', async () => { | ||
| const storage = createMemorySkillStorage({ | ||
| trustStrategy: createAlwaysTrustedStrategy(), | ||
| }) | ||
| const tools = createSkillManagementTools({ | ||
| storage, | ||
| trustStrategy: createDefaultTrustStrategy(), | ||
| }) | ||
| const tool = getTool(tools, 'register_skill') | ||
| await tool.execute!( | ||
| validRegisterInput({ name: 's1' }), | ||
| mockContext() as any, | ||
| ) | ||
| const saved = await storage.get('s1') | ||
| expect(saved!.trustLevel).toBe('untrusted') | ||
| }) | ||
|
|
||
| it('falls back to storage.trustStrategy when none provided', async () => { | ||
| const storage = createMemorySkillStorage({ | ||
| trustStrategy: createAlwaysTrustedStrategy(), | ||
| }) | ||
| const tools = createSkillManagementTools({ storage }) | ||
| const tool = getTool(tools, 'register_skill') | ||
| await tool.execute!( | ||
| validRegisterInput({ name: 's1' }), | ||
| mockContext() as any, | ||
| ) | ||
| const saved = await storage.get('s1') | ||
| expect(saved!.trustLevel).toBe('trusted') | ||
| }) | ||
| }) | ||
| }) |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.