security/correctness fixes from automated review (High) by sroussey · Pull Request #506 · workglow-dev/libs

sroussey · 2026-05-15T08:43:29Z

Summary

Four High-severity security/correctness fixes from an automated review. Each fix is a single self-contained commit with a focused vitest suite.

Fixes applied

1. `fix(ai): replace runWithIterable Proxy with shallow clone`

File: packages/ai/src/task/base/runWithIterable.ts
The Proxy(context, { get }) wrapper hid the wrapper's true identity from callers that key on reference (e.g. WeakMap caches). Repeated reads through the proxy returned new transparent views rather than a single stable wrapper.
Switched to const wrappedContext: IExecuteContext = { ...context, signal: localAbort.signal };. Behaviour is otherwise unchanged: every non-signal field still aliases the original context by reference. Tests assert WeakMap-keyed identity stability and that localAbort propagates via wrappedContext.signal.

2. `fix(ai): classify provider-error vs no-finish in AiTask.execute`

Files: packages/ai/src/task/base/AiTask.ts, packages/ai/src/capability/StreamEventAccumulator.ts
result() now runs in its own try/catch. The original provider error is preserved on the thrown TaskError's .cause (and rethrown verbatim when materialise succeeded against a partial accumulation, so existing error-shape expectations stay stable).
StreamEventAccumulator.materialize tags no-finish failures with code: "ACCUMULATOR_NO_FINISH" and a diagnostic lastEventType field. AiTask.execute propagates both fields onto the wrapped TaskError. Exported ACCUMULATOR_NO_FINISH constant for consumers that branch on it.

3. `fix(util): snapshot-then-delete eviction in WorkerServerBase Set caps`

File: packages/util/src/worker/WorkerServerBase.ts
The cap-eviction loops in recordPendingAbort and scheduleCompletedRequestCleanup iterated the live Set via set.values() and called set.delete() from inside the loop. Permitted by spec but a perennial bug magnet.
Switched to const evict = Array.from(set).slice(0, EVICT_BATCH); for (const item of evict) set.delete(item);. Extracted HARD_CAP (1000) and EVICT_BATCH (500) as named module-level constants. Tests assert FIFO eviction order and that concurrent insert+evict workloads do not throw.

4. `fix(ai): escape regex metacharacters in RerankerTask.simpleRerank`

File: packages/ai/src/task/RerankerTask.ts
simpleRerank constructed new RegExp(word, "gi") directly from query tokens. Tokens containing regex metacharacters (foo(, \\, *abc, [, …) caused new RegExp to throw SyntaxError: Invalid regular expression.
Added a local escapeRegExp helper (canonical TC39 RegExp.escape pattern) and apply it before building the regex; defensively skip empty / whitespace-only tokens. Parametrised tests cover unbalanced-paren, lone-backslash, leading-quantifier, bracket-open, mixed, unicode, whitespace-only, and empty-after-split inputs.

Deferred fixes

Each item below was checked against the current main head and deferred because the target file/symbol does not exist there yet (and the plan instructed us to defer if the target only exists in an unmerged PR branch):

fix(util/worker, ai/task): TTL-based pendingAborts eviction; clarify runWithIterable bond #500 H1 (TTL expiry for unmatched markers) — WorkerServerBase has pendingAborts and consumePendingAbort, but no PENDING_ABORT_TTL_MS constant or time-based expiry path on main. Apply when fix(util/worker, ai/task): TTL-based pendingAborts eviction; clarify runWithIterable bond #500 lands.
fix(util/worker, ai/task): TTL-based pendingAborts eviction; clarify runWithIterable bond #500 H2 (monotonicNowMs helper) — depends on fix(util/worker, ai/task): TTL-based pendingAborts eviction; clarify runWithIterable bond #500 H1; nothing to scope down on main.
fix(ai): rerank candidate floor + KbSearchTask scoreThreshold forwarding #496 H1 (scoreThreshold schema minimum: 0) — KbSearchTask.ts exists but its input schema has no scoreThreshold property. Apply when fix(ai): rerank candidate floor + KbSearchTask scoreThreshold forwarding #496 lands.
fix(ai): rerank candidate floor + KbSearchTask scoreThreshold forwarding #496 H2 (rerank-mode docs / strategy reconciliation) — createStandardKbStrategy.ts does not exist on main.
Refactor KB RAG to strategy pattern, remove callback-based API #484 H2 (chunk_id field-name mismatch in toInsertChunkEntities) — toInsertChunkEntities does not exist anywhere in packages/ on main.
Refactor KB RAG to strategy pattern, remove callback-based API #484 H3 (deprecated callback type re-exports) — OnSearchCallback (and friends) still exist on main; no deprecation needed yet.
Refactor KB RAG to strategy pattern, remove callback-based API #484 H4 (thread firstStageTopK / vectorWeight through strategy.search) — no strategy abstraction on main (createStandardKbStrategy.ts missing); ISearchOptions already exposes the fields directly on KnowledgeBase, but the strategy plumbing this fix targets is not yet present.
fix(kb,hft): document strategy trust model; runtime shape guard on HFT_TextReranker #501 H1 / H2 (TSDoc on IKbAiStrategy) — packages/knowledge-base/src/knowledge-base/IKbAiStrategy.ts does not exist on main.

Test plan

bun install --frozen-lockfile succeeded.
turbo run build-types --filter=@workglow/ai --filter=@workglow/util --filter=@workglow/knowledge-base passed.
bun run build:packages (full Turbo build) succeeded.
All new test files pass under vitest run:
- packages/test/src/test/ai/streaming-abort.test.ts (5 tests, including 2 new runWithIterable identity / abort tests)
- packages/test/src/test/ai/StreamEventAccumulator.test.ts (10 tests, including 2 new ACCUMULATOR_NO_FINISH tag tests)
- packages/test/src/test/ai/AiTask.providerError.test.ts (NEW, 2 tests)
- packages/test/src/test/util/WorkerServerBase.race.test.ts (3 existing tests still pass)
- packages/test/src/test/util/WorkerServerBase.eviction.test.ts (NEW, 2 tests)
- packages/test/src/test/ai/RerankerTask.simpleRerank.test.ts (NEW, 9 parametrised tests)
Other already-affected suites (AiTaskPhases, AiTask.requires, AiTaskSchemas, AiChatTask, accumulatingEmit, collectStream, AiJob_runFn) all pass — 57/57 green.

Notes on reviewer-visible deltas

StreamEventAccumulator now tracks a lastEventType field; this is purely additive and is included in the no-finish error message ("… (lastEventType=phase)."). Existing tests that match /finish/i continue to pass.
The wrapped TaskError from AiTask.execute carries cause, code, and lastEventType as own properties (the project's BaseError does not declare a cause slot in its constructor signature, so they are attached after construction); this is a documented pattern used elsewhere in the codebase for diagnostic enrichment.
WorkerServerBase introduces two module-level constants (HARD_CAP = 1000, EVICT_BATCH = 500); the 1000/500 values match the previous inline magic numbers exactly, so behaviour is unchanged at the cap.

https://claude.ai/code/session_01VMmVCK1hBYWtq6Up8cxiAh

Generated by Claude Code

The Proxy-based context wrapper hid the wrapped object's true identity from callers that key on reference (e.g. WeakMap caches): repeated reads from the proxy returned new transparent views rather than a single stable wrapper. Switching to a plain shallow spread gives the strategy a single, identity-stable wrapper while still substituting `signal` with the local AbortController. Behaviour is otherwise unchanged: every non-`signal` field still aliases the original context by reference.

Previously, if a provider's strategy.execute() rejected after the stream had begun, the rejection bubbled out before result() ran, so callers saw the original provider error. If the provider resolved cleanly but the stream never produced a finish event, result() threw a generic Error with no way to distinguish that case from any other failure. This commit: - Wraps strategy.execute() and result() in separate try/catch blocks in AiTask.execute. The original provider error is preserved on the thrown TaskError's `cause` when materialisation also fails; if materialisation succeeds against a partial accumulation, the original provider error is rethrown verbatim so error-shape expectations stay stable. - Tags the materialise error with `code: "ACCUMULATOR_NO_FINISH"` and a diagnostic `lastEventType` field on StreamEventAccumulator. AiTask forwards both fields through to the wrapped TaskError so callers can branch on the shape.

Previously, the cap-eviction loops in `recordPendingAbort` and `scheduleCompletedRequestCleanup` iterated the live `Set` via `set.values()` and called `set.delete(entry.value)` from inside the loop. This is technically permitted by the Map/Set spec, but couples the iterator's internal cursor to mutation and is a perennial bug magnet — easy to misread, easy to break in a future refactor. Switch to snapshot-then-delete: `Array.from(set).slice(0, EVICT_BATCH)` captures the eviction list FIFO (Set preserves insertion order), then a plain `for ... of` loop deletes each. Extract `HARD_CAP` (1000) and `EVICT_BATCH` (500) as named module-level constants so the policy parameters are reviewable in one place.

`simpleRerank` constructed `new RegExp(word, "gi")` directly from user-supplied query tokens. Tokens containing regex metacharacters (`foo(`, `\\`, `*abc`, `[`, …) caused `new RegExp` to throw `SyntaxError: Invalid regular expression`, surfacing a 500-shaped failure in what is supposed to be a literal-keyword count. Add a local `escapeRegExp` helper (matches the canonical TC39 `RegExp.escape` pattern) and apply it before building the regex. Also defensively skip empty / whitespace-only tokens so the contract is robust to upstream changes.

pkg-pr-new · 2026-05-15T08:45:18Z

Open in StackBlitz

@workglow/cli

npm i https://pkg.pr.new/@workglow/cli@506

@workglow/ai

npm i https://pkg.pr.new/@workglow/ai@506

@workglow/browser-control

npm i https://pkg.pr.new/@workglow/browser-control@506

@workglow/indexeddb

npm i https://pkg.pr.new/@workglow/indexeddb@506

@workglow/javascript

npm i https://pkg.pr.new/@workglow/javascript@506

@workglow/job-queue

npm i https://pkg.pr.new/@workglow/job-queue@506

@workglow/knowledge-base

npm i https://pkg.pr.new/@workglow/knowledge-base@506

@workglow/mcp

npm i https://pkg.pr.new/@workglow/mcp@506

@workglow/storage

npm i https://pkg.pr.new/@workglow/storage@506

@workglow/task-graph

npm i https://pkg.pr.new/@workglow/task-graph@506

@workglow/tasks

npm i https://pkg.pr.new/@workglow/tasks@506

@workglow/util

npm i https://pkg.pr.new/@workglow/util@506

workglow

npm i https://pkg.pr.new/workglow@506

@workglow/anthropic

npm i https://pkg.pr.new/@workglow/anthropic@506

@workglow/bun-webview

npm i https://pkg.pr.new/@workglow/bun-webview@506

@workglow/chrome-ai

npm i https://pkg.pr.new/@workglow/chrome-ai@506

@workglow/electron

npm i https://pkg.pr.new/@workglow/electron@506

@workglow/google-gemini

npm i https://pkg.pr.new/@workglow/google-gemini@506

@workglow/huggingface-inference

npm i https://pkg.pr.new/@workglow/huggingface-inference@506

@workglow/huggingface-transformers

npm i https://pkg.pr.new/@workglow/huggingface-transformers@506

@workglow/node-llama-cpp

npm i https://pkg.pr.new/@workglow/node-llama-cpp@506

@workglow/ollama

npm i https://pkg.pr.new/@workglow/ollama@506

@workglow/openai

npm i https://pkg.pr.new/@workglow/openai@506

@workglow/playwright

npm i https://pkg.pr.new/@workglow/playwright@506

@workglow/postgres

npm i https://pkg.pr.new/@workglow/postgres@506

@workglow/sqlite

npm i https://pkg.pr.new/@workglow/sqlite@506

@workglow/supabase

npm i https://pkg.pr.new/@workglow/supabase@506

@workglow/tf-mediapipe

npm i https://pkg.pr.new/@workglow/tf-mediapipe@506

commit: d567cfc

github-actions · 2026-05-15T08:47:26Z

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	62.28%	21875 / 35120
🔵	Statements	62.15%	22642 / 36429
🔵	Functions	64.02%	4133 / 6455
🔵	Branches	50.86%	10494 / 20630

File Coverage

No changed files found.

Generated in workflow #2258 for commit d567cfc by the Vitest Coverage Report Action

Copilot

Pull request overview

This PR applies focused security/correctness fixes across AI streaming/task execution, reranking, and worker request bookkeeping, with targeted Vitest coverage for the regressions.

Changes:

Replaces proxy-based streaming context wrapping with a shallow clone for stable identity.
Adds no-finish error tagging/classification for AI stream accumulation and provider failures.
Escapes reranker query tokens before regex construction and snapshots worker eviction batches before deletion.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`packages/ai/src/task/base/runWithIterable.ts`	Uses a shallow-cloned execution context with a substituted local abort signal.
`packages/ai/src/task/base/AiTask.ts`	Separates provider execution errors from stream materialization errors and wraps no-finish failures.
`packages/ai/src/capability/StreamEventAccumulator.ts`	Adds `ACCUMULATOR_NO_FINISH` diagnostics and tracks the last observed event type.
`packages/ai/src/task/RerankerTask.ts`	Escapes query tokens before constructing keyword-match regexes.
`packages/util/src/worker/WorkerServerBase.ts`	Extracts eviction constants and snapshots Set entries before deleting capped bookkeeping entries.
`packages/test/src/test/ai/streaming-abort.test.ts`	Adds coverage for stable wrapped context identity and local abort propagation.
`packages/test/src/test/ai/StreamEventAccumulator.test.ts`	Adds coverage for no-finish error code and `lastEventType`.
`packages/test/src/test/ai/AiTask.providerError.test.ts`	Adds provider-error vs no-finish classification coverage.
`packages/test/src/test/ai/RerankerTask.simpleRerank.test.ts`	Adds regression tests for regex metacharacter queries.
`packages/test/src/test/util/WorkerServerBase.eviction.test.ts`	Adds FIFO eviction behavior coverage for pending abort markers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+        // Skip pure-whitespace / empty tokens (defensive: split(/\s+/) +
+        // .filter(w => w.length > 0) above should already exclude them, but
+        // make the contract local).
+        if (word.length === 0 || /^\s+$/.test(word)) continue;


sroussey · 2026-05-15T15:57:55Z

@copilot apply changes based on the comments in this thread

Agent-Logs-Url: https://github.com/workglow-dev/libs/sessions/4e18de91-3eec-49ac-b62f-ce4d286fe0a3 Co-authored-by: sroussey <127349+sroussey@users.noreply.github.com>

Copilot · 2026-05-15T16:05:30Z

@copilot apply changes based on the comments in this thread

Applied the review-thread change in 381bb46: simpleRerank now handles zero-token queries without NaN scores, and tests now assert finite scores for empty/whitespace query cases.

claude added 4 commits May 15, 2026 08:29

sroussey requested a review from Copilot May 15, 2026 15:53

Copilot started reviewing on behalf of sroussey May 15, 2026 15:53 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

Comment thread packages/ai/src/task/RerankerTask.ts

// Skip pure-whitespace / empty tokens (defensive: split(/\s+/) +

// .filter(w => w.length > 0) above should already exclude them, but

// make the contract local).

if (word.length === 0 || /^\s+$/.test(word)) continue;

Copilot started work on behalf of sroussey May 15, 2026 15:58 View session

fix(ai): avoid NaN reranker scores for empty queries

381bb46

Agent-Logs-Url: https://github.com/workglow-dev/libs/sessions/4e18de91-3eec-49ac-b62f-ce4d286fe0a3 Co-authored-by: sroussey <127349+sroussey@users.noreply.github.com>

Copilot finished work on behalf of sroussey May 15, 2026 16:06

sroussey merged commit 9cbdab4 into main May 15, 2026
4 checks passed

sroussey deleted the claude/loving-mendel-fKOax branch May 15, 2026 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

security/correctness fixes from automated review (High)#506

security/correctness fixes from automated review (High)#506
sroussey merged 5 commits into
mainfrom
claude/loving-mendel-fKOax

sroussey commented May 15, 2026

Uh oh!

pkg-pr-new Bot commented May 15, 2026

Uh oh!

github-actions Bot commented May 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

sroussey commented May 15, 2026

Uh oh!

Copilot AI commented May 15, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

sroussey commented May 15, 2026

Summary

Fixes applied

1. fix(ai): replace runWithIterable Proxy with shallow clone

2. fix(ai): classify provider-error vs no-finish in AiTask.execute

3. fix(util): snapshot-then-delete eviction in WorkerServerBase Set caps

4. fix(ai): escape regex metacharacters in RerankerTask.simpleRerank

Deferred fixes

Test plan

Notes on reviewer-visible deltas

Uh oh!

pkg-pr-new Bot commented May 15, 2026

Uh oh!

github-actions Bot commented May 15, 2026

Coverage Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

sroussey commented May 15, 2026

Uh oh!

Copilot AI commented May 15, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

1. `fix(ai): replace runWithIterable Proxy with shallow clone`

2. `fix(ai): classify provider-error vs no-finish in AiTask.execute`

3. `fix(util): snapshot-then-delete eviction in WorkerServerBase Set caps`

4. `fix(ai): escape regex metacharacters in RerankerTask.simpleRerank`