Repository navigation
AgentTask harness: agent-eval benchmarks, loop fixes, reasoning replay, and provider parity - #1015
Merged
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Collaborator
Author
|
CodeQL status after 6c8ea70: the medium alert, the mock echoing error text, is fixed. 11 alerts remain (1 critical, 10 high), all in
The threads give the path traced for each. They stay open until the author decides between two options:
Every other check on this head is for the author to judge once it completes. Generated by Claude Code |
![mergestorm-vortex[bot]](https://avatars.githubusercontent.com/in/3924858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review progress ████░░░░░░ 54/145 files
Comment — found 6 issue(s) at 6c8ea70.
Actionable comments posted: 6
Coverage is incomplete: reviewed 54/145 files; 2 skipped; 89 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/harbor/workglow_agent.py` (line 257, Important):
`node ... 2>&1 </dev/null | tee <file>` has no `set -o pipefail`, so the command's exit status is `tee`'s (0). If the agent exits non-zero (node missing from PATH, uncaught exception, bundle load failure), Harbor sees a successful run, so no `NonZeroAgentExitCodeError` is recorded. The trial then goes to the verifier and, if no summary was written, `classifyTrial` labels it `wrong-answer` instead of `agent-crash`. Fix by prefixing the command with `set -o pipefail;` (or `set -euo pipefail;`, as `install` does), so the agent's status propagates.
2. In `@examples/agent-eval/src/agent/tools/read.ts` (line 81, Important):
`const buffer = await readFile(path);` reads the whole file, then sniffs for binary and splits it into lines, even when the model asked for a 2000-line window. Reading a multi-hundred-MB log or dataset in a benchmark container costs that much RSS and time for a response capped at 50KB. Files over the Node buffer limit fail with a generic "read failed" error instead of a size message. A `stat` size check, or reading only the first few KB for the binary sniff and the requested line range, would bound this. Non-blocking.
3. In `@examples/agent-eval/src/run/runHarbor.ts` (line 46, Important):
The precheck tests `existsSync(bundle)` where `bundle` is always `<root>/dist/workglow-agent.mjs`. But the env it passes to Harbor uses `process.env.WORKGLOW_AGENT_BUNDLE ?? bundle` (line 60), and the Python `_bundle()` honours that env var too. If a user points WORKGLOW_AGENT_BUNDLE at a bundle elsewhere and has no dist build, this throws "run `bun run build-agent` first" even though the run would succeed. Check the same resolved path that is later exported (`process.env.WORKGLOW_AGENT_BUNDLE ?? bundle`).
4. In `@examples/cli/src/run-events/projectRunEvents.ts` (line 125, Important):
Previously every non-empty text-delta was emitted as a `text` event regardless of port. classifyTextDelta (and foldTextDelta in TaskRunApp/ChatTaskRow/StreamingTextRow, and the `port === "text"` check in web TaskNode) now returns nothing for any port other than `text` or `reasoning`. StreamProcessor accumulates append-mode output per port, so a task whose streamed output lives on a port such as `code` or `summary` now shows an empty panel and no scrollback in the CLI and web console, with no error. The unit test `ignores ports other than text and reasoning` pins this on purpose, so it is a deliberate choice, but it is a visible regression for non-chat streaming tasks. Either fall back to text for any port other than `reasoning`, or confirm such tasks are out of scope.
5. In `@packages/ai/src/task/ChatHistory.ts` (line 74, Important):
`messageChars` drops `payload` before measuring, so `trimHistoryForModel` never counts provider-native reasoning. But `normalizeHistoryForModel(trimHistoryForModel(...))` is what gets sent, and the converters replay each earlier turn's payload verbatim (Anthropic `parseOwnTurn`, OpenAI Responses `native_items`) whenever the provider and call ids still match. The request therefore grows by the payload size on every earlier turn without being counted, so `maxHistoryChars` no longer caps what reaches the model. The docstring says it caps history at `max` characters. A long multi-round turn with large encrypted reasoning can exceed the window the budget is meant to protect. If the exclusion is intentional, the trimming docs should say payloads are not capped.
6. In `@providers/openai/src/ai/common/OpenAI_ToolCalling.ts` (line 71, Important):
`replay ? { nativeTurnProvider: nativeKey } : {}` gates replay only on `replay_reasoning`. The request only gets `include: ["reasoning.encrypted_content"]` when `wantsEncryptedReasoning` is true (line ~89: `replay && reasoning !== undefined && reasoning.effort !== "none"`). So when a run's config has reasoning off (`effort: "none"`, or a non-reasoning model) but the history carries an earlier `openai:<model>` native turn whose reasoning items hold `encrypted_content`, `toOpenAIMessages`/`buildResponsesInput` replays those reasoning items verbatim into a request that did not ask for `reasoning.encrypted_content`. The header comment of this function and the comment in `accumulateOpenAIResponsesStream` say a replayed reasoning item without that include is looked up by id and fails where the response is not stored. Example: a persisted chat history written with effort high is resumed with effort none under the same model_name. Fix: use the same `wantsEncryptedReasoning` condition for the replay option as for emit and include. The test "asks for nothing extra when reasoning is off" only checks that `include` is absent and does not assert on `input`, so it does not catch this.
There was a problem hiding this comment.
Review progress ████████░░ 109/145 files
Request changes — found 6 issue(s) at 5ab86f4.
Actionable comments posted: 6
Coverage is incomplete: reviewed 109/145 files; 0 skipped; 36 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/bin/workglow-agent.ts` (line 97, Blocker):
`Number` converts an invalid value such as `--max-round-retries nope` to `NaN` without rejecting it. AgentTask then evaluates `attempts > maxRoundRetries`; every comparison with `NaN` is false, so retryable provider failures keep retrying indefinitely (notably in direct runs without a duration budget). Validate this as a finite, non-negative integer before passing it to the task.
2. In `@examples/agent-eval/package.json` (line 25, Important):
`"workglow-agent-eval": "./dist/bin/workglow-agent-eval.js"`
The workspace entry in `bun.lock` still declares this bin as `./dist/workglow-agent-eval.js`, while `build-cli` emits it under `dist/bin/`. After installing the workspace, the generated `workglow-agent-eval` executable therefore targets a nonexistent file; update the lockfile bin path to match this manifest/build output.
3. In `@examples/agent-eval/src/agent/atif.ts` (line 118, Important):
This per-round total omits `usage.imageInput` and `usage.imageCached`, and the `cached_tokens` field below omits `imageCached` too. `summarizeUsage` includes both image-token counters in the trajectory's `final_metrics`, so image-using rounds (supported by this harness) report per-step metrics that do not reconcile with the totals.
4. In `@providers/cactus/src/ai/common/Cactus_ToolCallingCore.ts` (line 49, Important):
`AgentTask` appends `reviewPrompt` as a user message, then can start another model round if the model requests a tool. In that sequence this returns the review prompt as the entire query; because Needle has no conversation state, the next round no longer gets the original user request (and this query also omits the tool result). The agent can therefore attempt the review without knowing what task it was reviewing. Preserve the original request/context when forming subsequent-round queries.
5. In `@providers/huggingface-transformers/src/ai/common/HFT_ToolCalling.ts` (line 216, Important):
`reasoning_content` is only added by `buildHFTMessages`, but `buildPromptAndPrefix` uses that builder only when the history contains a `tool` message; otherwise it uses `toTextFlatMessages`, which drops assistant reasoning blocks. For example, when `AgentTask` gets a no-tool answer and appends `reviewPrompt`, the next HFT round receives the prior answer and review request but not the reasoning this code captured, so the advertised reasoning replay does not work for that follow-up path. Preserve reasoning in the no-tool history path as well.
6. In `@providers/ollama/src/ai/common/Ollama_JobRunFns.ts` (line 35, Important):
`toOllamaMessages` attaches result images to the `role: "tool"` message. When a tool returns a screenshot or other image, Ollama's next round does not receive it as vision input; tool-result images need to follow the tool message in a user message. This converter is also wired into the browser run-fn, so the same case fails there.
Collaborator
Author
|
The analysis itself ran. GitHub then refused the upload: "CodeQL analyses from advanced configurations cannot be processed when the default setup is enabled." The new To fix (needs a repo admin): Settings → Code security → CodeQL analysis → switch to Advanced, or disable default setup. Then re-run this workflow; the No code change in this PR can clear it, so I haven't re-run the job. Generated by Claude Code |
There was a problem hiding this comment.
Review progress ██████████ 148/148 files
Comment — found 3 issue(s) at 64cb980.
Actionable comments posted: 3
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@.github/codeql/codeql-config.yml` (line 8, Important):
`examples/agent-eval` is matched against source-file paths as a glob, so this literal directory path does not match files beneath it. CodeQL will continue scanning the shell-capable harness and report the alerts this config is intended to suppress; use a recursive pattern such as `examples/agent-eval/**`.
2. In `@examples/agent-eval/src/bin/workglow-agent.ts` (line 212, Important):
`process.exit()` terminates immediately, without waiting for pending `process.stdout.write()` / `process.stderr.write()` data. Harbor runs this agent with output piped through `tee`; when the final answer or accumulated log output exceeds the pipe's writable buffer, the write is queued and this line can truncate it. Set `process.exitCode` and allow the event loop to drain (or explicitly wait for the streams) before exiting.
3. In `@providers/cactus/src/ai/common/Cactus_ToolCallingCore.ts` (line 53, Important):
`users[0]` is the first user message in the entire `input.messages` history, not the first message of this AgentTask turn. In `runAgentChat`, a second independent request is passed alongside the prior transcript, so Cactus receives `old request\n\nnew request` and can answer or call tools for the stale request. Use the current `input.prompt` as the active turn’s first question while retaining a later review/reminder user message when present.
There was a problem hiding this comment.
Review progress ██████████ 148/148 files
Comment — found 1 issue(s) at 82ea1c4.
Actionable comment posted: 1
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@providers/cactus/src/ai/common/Cactus_ToolCallingCore.ts` (line 60, Important):
`AgentTask` appends the model's assistant answer before adding `reviewPrompt`, but this query includes only the original prompt and latest user message. Since Cactus passes only this string to stateless Needle, the review round never receives the answer (or tool results) it is supposed to check and instead generates a fresh response to the question. Include the prior assistant/tool context needed for the review in the query.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 11, 2026 22:35
82ea1c4 to
ec97452
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/148 files
Request changes — found 7 issue(s) at ec97452.
Actionable comments posted: 7
Coverage is incomplete: reviewed 59/148 files; 1 skipped; 88 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
2. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
3. In `@examples/agent-eval/harbor/workglow_agent.py` (line 165, Important):
`return min(base, config.agent.max_timeout_sec or float("inf")) * multiplier`
This applies `agent.max_timeout_sec` before scaling, so the computed allowance can exceed Harbor's maximum. For example, with `base=600`, `max_timeout_sec=300`, and `multiplier=2`, this reports 600 seconds; `_max_duration_sec()` then gives the agent 480 seconds, although Harbor will cancel at 300 seconds. That can terminate a still-running trial before the agent writes its summary. Apply the multiplier before capping the resolved timeout.
4. In `@examples/agent-eval/src/bin/workglow-agent-eval.ts` (line 117, Important):
`Number` accepts values such as `nope` as `NaN`. `buildHarborJob` includes that value, and `runHarbor`'s `JSON.stringify` serializes it as `null`, so Harbor receives a changed timeout setting instead of a clear CLI error. Validate that the value is finite and in the supported range before building the job.
5. In `@examples/agent-eval/src/compare/report.ts` (line 210, Important):
With multiple attempts per task (for example, `-k 2`), equal nonzero pass fractions reach this branch. If both arms pass 1 of 2 attempts, the report counts the task as “Both ✓,” even though neither arm passed every attempt. Distinguish partial-rate ties from tasks both arms fully passed.
6. In `@packages/ai/src/task/MessageConversion.ts` (line 208, Important):
`OpenAI_ToolCalling_Stream` calls `toOpenAIMessages` with only `nativeTurnProvider`, so this option stays false for OpenAI. Tool-result images are then left inline in the `tool` message, and `buildResponsesInput` converts that content with `contentToString`, which drops every image part. When an OpenAI tool returns an image (for example, a screenshot), the next model round never receives it; enable the user-message image path for the Responses caller.
7. In `@providers/openrouter/src/ai/common/OpenRouter_ToolCalling.ts` (line 93, Important):
`filterValidToolCalls` can remove an unknown-name call while retaining another call from the same assistant response. This set then records only the surviving IDs, so `encodeReasoningTurn` makes the response's reasoning details look bound to that shortened call list; on the next round `withReasoningDetails` accepts and replays them beside the filtered transcript. For signed reasoning (for example, Anthropic details through OpenRouter), the details were produced with the original call set and may be rejected or replayed against the wrong turn. Suppress native-turn replay whenever any response call was filtered, rather than treating only forwarded IDs as the original turn's IDs.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 11, 2026 22:46
ec97452 to
5315c52
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/148 files
Request changes — found 9 issue(s) at 5315c52.
Actionable comments posted: 9
Coverage is incomplete: reviewed 60/148 files; 0 skipped; 88 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/agent/runCodingAgent.ts` (line 198, Important):
`finalText = steps.at(-1)?.text ?? ""`
When the last allowed round (for example, `--max-rounds 1`) returns text alongside a tool call, AgentTask can stop as `max-rounds` after running that tool without ever receiving a final answer. This still records the narration from the tool-call round as `finalText`, and the CLI prints it on stdout as if it were the answer. Only expose text from a terminal no-tool round, or otherwise avoid presenting an incomplete run's narration as the final answer.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 45, Important):
`spawn()` runs before the abort listener is installed below. If the task signal is already aborted when `runCommand` is entered, `addEventListener` will not replay the abort event, so the shell still runs until its timeout and can perform side effects after cancellation. This can occur when cancellation happens while AgentTask is awaiting progress before invoking the tool; reject immediately or handle the already-aborted state before starting the process.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 55, Important):
Once captured output reaches 16 MiB, this drops all later chunks while retaining only the prefix. `createBashTool` then calls `truncateTail` on that prefix, so for a command producing more than 16 MiB the model sees the end of the first 16 MiB rather than the command’s final output (where test failures and summaries commonly appear), despite the tool promising to show the tail.
4. In `@examples/agent-eval/src/agent/tools/read.ts` (line 98, Important):
`split("\n")` leaves a trailing empty element for the ordinary one-line file `hello\n`. Consequently `offset: 2` returns an empty result instead of reporting that the requested line is past EOF, and page counts/continuation offsets are off by one for newline-terminated files.
5. In `@examples/agent-eval/src/agent/tools/read.ts` (line 109, Important):
This slices by UTF-16 characters rather than UTF-8 bytes. A 50 KiB line of four-byte characters can therefore return about 200 KiB here, bypassing `MAX_OUTPUT_BYTES` and sending several times the intended tool output to the model. Bound this fallback by encoded byte length while preserving character boundaries.
6. In `@examples/agent-eval/src/bin/workglow-agent-eval.ts` (line 209, Important):
`buildReport(loadTrials([jobDir]))`
If Harbor creates `jobDir` but produces no parseable trial results (for example, it fails before scheduling trials), this builds and writes an empty comparison and the CLI exits successfully. The `compare` command already rejects this case; apply the same check here so a failed/empty run is not reported as a successful benchmark.
7. In `@packages/ai/src/task/AgentTask.ts` (line 838, Important):
`AgentToolRecord.chars` is calculated in `runCalls` before this notice is appended, while the updated result is what the model receives. With `announceTimeLeft`, each round's final tool record therefore undercounts the visible result by the notice length, making `steps[].tools[].chars` disagree with the transcript.
Additional findings outside changed lines:
1. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
2. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (2) — the lines they point at have not changed:
- 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 11, 2026 22:55
5315c52 to
abd0f4f
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 61/148 files
Request changes — found 6 issue(s) at abd0f4f.
Actionable comments posted: 6
Coverage is incomplete: reviewed 60/148 files; 1 skipped; 87 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/agent/tools/read.ts` (line 91, Important):
`return `${path} is a directory:\n${names.join("\n")}``
Directory results bypass `truncateHead`, so a directory with thousands of entries returns more than the documented 2000 lines/50 KB. The later AgentTask clamp is character-based and configurable, so it does not enforce either limit; large listings can consume excessive context. Apply the same output bounds to directory listings.
2. In `@packages/ai/src/task/AgentToolExecution.ts` (line 273, Important):
`approveToolCall` is awaited before this invocation, but the deadline is not checked again afterward. If a human approves a gated call only after `maxDurationMs` has elapsed, the tool still starts; task-backed tools also receive no deadline in `invokeTool`, so they cannot honor the expired deadline themselves. This lets a budget-expired turn perform the approved side effect after the deadline.
3. In `@providers/llamacpp-server/src/ai/common/LlamaCppServer_ToolCalling.ts` (line 72, Important):
`toOpenAIMessages(input, ...)` represents text from `input.messages` as content-part arrays, but the estimator below only counts messages whose `content` is a string. `AgentTask` supplies its transcript in `input.messages`, so the user prompt is omitted from the in-flight input-token snapshots (often showing 0 until terminal server usage arrives, or remaining wrong if the server omits usage). Flatten text parts when building the prompt estimate.
4. In `@providers/ollama/src/ai/common/Ollama_TextGeneration.ts` (line 84, Important):
`toOllamaChatMessages(...)` is used by the text-generation run function, but its subsequent `client.chat` request does not include `ollamaThinkParam(model)` (unlike the tool-calling request). A model configured with `effort: "high"` can therefore be used by `TextGenerationTask` or `AiChatTask` while Ollama receives no `think` setting, silently ignoring the effort policy exposed by the provider.
Additional findings outside changed lines:
1. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
2. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (2) — the lines they point at have not changed:
- 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 11, 2026 23:03
abd0f4f to
7995382
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/149 files
Request changes — found 6 issue(s) at 7995382.
Actionable comments posted: 6
Coverage is incomplete: reviewed 60/149 files; 0 skipped; 89 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 257, Important):
`writeFile(path, out, "utf8")`
When `toolConcurrency` is greater than 1, two `edit` calls targeting different regions of the same file can both read the original contents, compute separate results, and then overwrite the whole file; whichever write lands last silently discards the other edit, even though both calls report success. This is reachable through `runCodingAgent`'s `toolConcurrency` setting and `AgentTask` runs calls concurrently at that width.
2. In `@packages/ai/src/task/AgentTask.ts` (line 959, Important):
`normalizeHistoryForModel` merges adjacent user messages by extracting only their text blocks. Since AgentTask appends `prompt` after `input.messages`, a resumed conversation whose history ends in a user message will merge that message with the new prompt; if either contains an image, the image is dropped before the provider sees it. For example, a trailing user image in `messages` followed by a new text prompt becomes text-only, so the next model round cannot inspect the image.
3. In `@packages/ai/src/task/ToolCallingTask.ts` (line 346, Important):
When `emitCheckpoint` is set, `executeStream` captures only `text` and `toolCalls` before calling `finalizeCheckpoint`; that finalizer stores an assistant message without this `nativeTurn` payload (or reasoning). A later request consuming that checkpoint therefore replays an incomplete assistant turn. For providers that require signed/encrypted reasoning to be echoed back, such as Anthropic, a follow-up after a checkpointed tool call can be rejected because the required native turn was discarded.
4. In `@providers/huggingface-inference/src/ai/HfInferenceProvider.ts` (line 33, Important):
`effortPolicy` exposes the effort dial for this provider, but only `HFI_ToolCalling_Stream` currently adds `hfiReasoningParams(model)` to its request. The registered `HFI_TextGeneration_Stream` also handles `TextGenerationTask` and `AiChatTask` and omits that field, so selecting an effort on either of those common paths silently has no effect.
Additional findings outside changed lines:
1. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
2. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (2) — the lines they point at have not changed:
- 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 11, 2026 23:12
7995382 to
c2882f8
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/152 files
Request changes — found 7 issue(s) at c2882f8.
Actionable comments posted: 7
Coverage is incomplete: reviewed 60/152 files; 0 skipped; 92 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 258, Important):
`crlf ? result.content.replace(/\n/g, "\r\n") : result.content`
If a file contains both CRLF and LF separators, any successful edit converts every LF separator to CRLF, including untouched sections. This contradicts the adjacent claim that the tool keeps the file's own line endings and can create a whole-file diff from a small edit; preserve the original separators rather than choosing one ending for the entire file.
2. In `@examples/agent-eval/src/bin/workglow-agent-eval.ts` (line 207, Important):
`runHarbor` returns Harbor's nonzero exit code, but when at least one trial result exists this branch only prints a warning; the action then completes and the outer handler leaves `process.exitCode` at 0. Thus a failed or incomplete Harbor run can produce a partial comparison while shell scripts and CI treat the benchmark as successful. Keep writing the partial report if desired, but preserve the nonzero status.
3. In `@examples/agent-eval/src/compare/report.ts` (line 166, Important):
When a run has an unpriced passing trial, `costUsd` sums only priced trials while `passed` includes every pass. For example, one priced pass costing $1 plus one unpriced pass reports $0.50/pass, understating the cost; the Markdown table also presents this value without the asterisk used for the partial cost total. Avoid reporting this as an exact cost per pass when pass costs are incomplete.
4. In `@packages/ai/src/task/MessageConversion.ts` (line 243, Important):
`deferredImages` is flushed at the end of each individual `role: "tool"` message. If history represents parallel calls as separate tool messages and an earlier result contains an image, this inserts a user turn between the assistant's function calls and the later call's result (for example, `assistant(c1,c2) → tool(c1,image) → user(image) → tool(c2)`). OpenAI's Responses conversion preserves that order, so the model sees a new user turn before the outstanding tool result and the replayed tool-call exchange is malformed. Defer these images until the contiguous tool-result messages for that assistant turn have all been emitted.
5. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_EffortPolicy.ts` (line 16, Important):
`llamaCppEffortPolicy` advertises these effort levels for the whole provider, but `llamaCppThoughtBudget` is only consumed by `LlamaCpp_ToolCalling_Stream`. The registered `LlamaCpp_TextGeneration_Stream` and `LlamaCpp_Chat_Stream` both call `session.prompt` without a `budgets.thoughtTokens` option, so selecting an effort on those common paths is silently ignored despite the provider reporting it as supported.
Additional findings outside changed lines:
1. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
2. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (2) — the lines they point at have not changed:
- 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 11, 2026 23:23
c2882f8 to
c3dec79
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 57/154 files
Request changes — found 4 issue(s) at c3dec79.
Actionable comments posted: 4
Coverage is incomplete: reviewed 57/154 files; 0 skipped; 97 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@providers/anthropic/src/ai/common/Anthropic_CacheCheckpoint.ts` (line 143, Important):
Passing `undefined` makes `buildAnthropicMessages` discard every reasoning block, including Anthropic native payloads. A checkpoint produced by `ToolCallingTask.finalizeCheckpoint` can contain an assistant turn with signed thinking; when a consumer prepends this prefix, the signature is omitted and the prior turn is not replayed. Use the matching Anthropic model key for checkpoint-prefix conversion (the warm-up conversion at line 93 has the same omission).
2. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 64, Important):
`parseOwnTurn` accepts a payload whenever the tool-call IDs match, even if the current message has a different tool name or input. If a host edits an assistant history entry while preserving its call ID, this replays the old `tool_use` from `payload` and silently ignores the current transcript. Compare the complete tool-use blocks before replaying; otherwise rebuild the assistant message.
Additional findings outside changed lines:
1. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
2. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (2) — the lines they point at have not changed:
- 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 11, 2026 23:30
c3dec79 to
1ca5533
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 59/155 files
Request changes — found 9 issue(s) at 1ca5533.
Actionable comments posted: 9
Coverage is incomplete: reviewed 58/155 files; 1 skipped; 96 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
2. In `@examples/agent-eval/harbor/workglow_agent.py` (line 203, Important):
`command -v node` skips installation whenever any Node binary is present, and the later `node --version` only prints its version. On a musl/Alpine task image with a preinstalled Node below the bundle's stated 22+ requirement, setup proceeds and the agent is then run on an unsupported runtime, causing those trials to fail. Verify the installed major version and install a supported runtime or fail setup clearly.
3. In `@examples/agent-eval/src/agent/tools/read.ts` (line 81, Important):
`optionalInteger` only checks that the value is finite and then truncates it; it does not enforce the schema’s `minimum: 1`, and AgentTask passes function-tool arguments directly to `execute`. A call with `offset: 0` therefore reaches `slice(-1)` and returns the file’s last line as though it were the requested page; `limit: 0` can return an empty page with a misleading continuation notice. Validate these bounds at runtime and return a tool error for invalid values.
4. In `@examples/agent-eval/src/compare/report.ts` (line 161, Important):
`ran.length > 0 ? passed / ran.length : 0`
When every trial for an arm is blocked, this reports a 0% pass rate; `wilsonInterval(0, 0)` also renders a 0–0% interval, and `passAt1` falls back to 0. These are missing measurements, not zero-success measurements, so an entirely blocked arm is presented and ranked like an arm that ran and failed every task.
5. In `@examples/agent-eval/src/compare/report.ts` (line 173, Important):
`sum(ran.map((trial) => trial.inputTokens))`
`sum` turns each missing token count into zero, but the report displays these values as complete totals with no indication that some trials lacked usage data. For example, if one trial reports 100 input tokens and another has `inputTokens === undefined`, the table reports 100 as the arm total, understating usage. Distinguish unavailable counts from measured zero or mark these totals as partial.
6. In `@examples/agent-eval/src/run/jobConfig.ts` (line 74, Important):
`spec.slice(colon + 1).split(",")`
This splits before parsing each value as JSON, so valid object/array kwargs such as `opencode_config={"agent":{"build":{"steps":20}}}` fail at the embedded commas; string kwargs such as `append_system_prompt=Use X, then Y` fail too. The parser documents JSON-valued kwargs, but this delimiter handling makes those values impossible to pass through `--arm`.
7. In `@providers/openrouter/src/ai/common/OpenRouter_ToolCalling.ts` (line 39, Important):
`mergeReasoningDetailDelta` only stores these details for the native-turn payload; `accumulateOpenAIChatStream` forwards only `delta.reasoning` and `delta.reasoning_content`. When an OpenRouter response provides readable thoughts only as `reasoning_details` (for example, a `reasoning.text` detail without either legacy field), the reasoning is saved for replay but never reaches `AgentTask`'s reasoning output port or UI. Forward the text-bearing details as reasoning as well, avoiding duplication when the legacy field is also present.
Additional findings outside changed lines:
1. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
2. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (2) — the lines they point at have not changed:
- 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 11, 2026 23:39
1ca5533 to
63a04e1
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/155 files
Request changes — found 14 issue(s) at 63a04e1.
Actionable comments posted: 14
Coverage is incomplete: reviewed 60/155 files; 0 skipped; 95 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 140, Important):
The timeout stays armed while `finish` waits up to `DRAIN_MS` for pipe output. If the shell exits just before the timeout while a background child holds the pipes (for example, `sleep 0.9; sleep 30 & echo done` with a 1-second timeout), the timer fires during that drain window, marks the successful command as timed out, and SIGTERMs its process group. Clear the timeout when the shell emits `exit`, while retaining the drain delay.
2. In `@examples/agent-eval/src/agent/tools/read.ts` (line 104, Important):
`readFile(path)` loads the entire path before any size or file-type check. Because `resolvePath` accepts absolute paths, a call on `/dev/zero` never reaches EOF and can grow the agent process until it runs out of memory; even an ordinary huge file bypasses the advertised 50 KB output bound during reading. Reject non-regular files and cap or stream the read before allocating the full contents.
3. In `@examples/agent-eval/src/agent/tools/truncate.ts` (line 79, Important):
`text.split("\n")` leaves an empty final element for ordinary newline-terminated output. For example, `seq 1 5000` is reported by `createBashTool` as 5001 lines, and the 2000-line tail includes that empty element, so it retains only 1999 actual output lines. This also makes the new `seq` test assert the inflated count; treat a final newline as a terminator, not another line.
4. In `@examples/agent-eval/src/bin/workglow-agent-eval.ts` (line 117, Important):
`--effort low` (or `off`, etc.) still leaves this option at its default `high`; `parityKwargs` then sends OpenCode `variant: "high"` while Workglow and pi receive the requested level. The arms are no longer running at the same reasoning level, so the comparison is not measuring the advertised parity.
5. In `@examples/agent-eval/src/bin/workglow-agent-eval.ts` (line 172, Important):
This replaces any `task_names` already set by the suite. For example, `run -s polyglot-lite --tasks polyglot_go_*` drops the suite's Python/JavaScript restriction and runs Go tasks instead, broadening a command described as narrowing the suite. Preserve the suite's allowed task set when applying this filter.
6. In `@examples/agent-eval/src/bin/workglow-agent-eval.ts` (line 173, Important):
This overwrites a dataset's built-in `n_tasks` cap even when the CLI value is larger. For example, `-s swebench-50 -l 100` changes the suite from its documented first 50 tasks to 100, despite `--n-tasks` being described as an upper bound.
7. In `@examples/agent-eval/src/compare/report.ts` (line 163, Important):
When every trial is blocked, this still stores `passRate: 0` (and the summary also stores a zero Wilson interval and `passAt1: 0`). `renderMarkdown` hides these values via `ranAny`, but the CLI also writes the `ArmSummary` directly to `comparison.json`, where consumers see an all-blocked arm as measured zero success rather than no measurement.
8. In `@examples/agent-eval/src/compare/report.ts` (line 176, Important):
A trial with input and output counts but `cacheTokens === undefined` is not counted as missing usage. Its cache count is then added as zero at line 179, and the Markdown table shows the cache total without the `†` marker. This presents an incomplete cache-token total as complete.
9. In `@packages/ai/src/provider-utils/OpenAIShapedResponses.ts` (line 152, Important):
`sameCallIds` only checks `call_id`, so if a host changes a tool call's name or arguments but preserves its ID, this branch still sends the old native `function_call` and skips the edited assistant transcript. The following `function_call_output` may then contain a result for the edited call while Responses sees the old call; compare the names and arguments too before replaying these items.
10. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 123, Important):
`parseOwnTurn` checks only the tool-use blocks, so a host can edit or remove an assistant text block while keeping the calls unchanged and this still replays the original payload. The request then silently sends the stale text from `payload` instead of the transcript in `msg.content` (also occurs for text-only turns, where both call lists are empty); validate the current text blocks against the payload before replaying, or rebuild the assistant message.
11. In `@providers/ollama/src/ai/common/Ollama_Messages.ts` (line 88, Important):
`for (const msg of history)`
When a `ToolCallingTask` caller puts its system instructions in `messages` and leaves `input.systemPrompt` unset, this loop has no `system` branch, so those instructions are omitted from the Ollama request. `ToolCallingTaskInput.messages` accepts full `ChatMessage` history (including `role: "system"`), and the text-generation converter already lifts such entries; preserve them here as well, while retaining the explicit system prompt behavior.
Additional findings outside changed lines:
1. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
2. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
3. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (3) — the lines they point at have not changed:
- 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 11, 2026 23:51
63a04e1 to
1f15283
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/155 files
Request changes — found 7 issue(s) at 1f15283.
Actionable comments posted: 7
Coverage is incomplete: reviewed 60/155 files; 0 skipped; 95 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/run/suites.ts` (line 146, Important):
`globWithin(wanted, allowed)` only recognizes containment for literals and simple trailing-prefix globs. For example, `--suite polyglot-lite --tasks '*_001'` overlaps `polyglot_python_*` on Python tasks, but neither pattern contains the other, so `kept` stays empty and the CLI throws “matches none” instead of running the matching tasks. Handle valid overlaps (or only reject when disjointness is established).
2. In `@packages/ai/src/provider-utils/OpenAIShapedResponses.ts` (line 152, Important):
`if (native && sameCalls(native, message.tool_calls ?? [])) {`
This checks only tool calls, then replays every native output item and skips the current assistant content. If a caller edits or removes the assistant text while leaving its calls unchanged (including a text-only turn, where both call lists are empty), the old message item is still sent to the next Responses request and the edited transcript is ignored. Include the current assistant text in the replay check, or rebuild the turn when it differs.
3. In `@packages/ai/src/task/MessageConversion.ts` (line 199, Important):
`toOpenAIMessages` attaches the complete native assistant output whenever this provider key matches. Downstream, `buildResponsesInput` accepts those items based only on `sameCalls`; if a host edits or removes the assistant text while leaving its tool calls unchanged (and for a text-only turn, where both call lists are empty), it replays the old native message and ignores the current transcript. Check the text blocks against the native output too, or rebuild the assistant message when they differ.
4. In `@providers/ollama/src/ai/common/Ollama_Messages.ts` (line 134, Important):
`toolImages` is flushed at the end of each individual `role: "tool"` message. For a valid history with parallel calls represented as separate tool messages, if the first result contains an image, this inserts a synthetic user turn before the next call's result (assistant calls → tool result → user image → tool result). The later result is then no longer adjacent to the assistant's outstanding calls, so Ollama can treat the image as a new turn before receiving all results. Defer images across consecutive tool messages and flush them before the next non-tool message.
Additional findings outside changed lines:
1. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
2. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
3. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (3) — the lines they point at have not changed:
- 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 11, 2026 23:59
1f15283 to
4197146
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 68/155 files
Request changes — found 8 issue(s) at 4197146.
Actionable comments posted: 8
Coverage is incomplete: reviewed 60/155 files; 8 skipped; 87 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 188, Important):
`optionalInteger` truncates but does not enforce the schema's `minimum: 1`, and `AgentTask` passes function-tool arguments directly to `execute` without validating that schema. With `timeout: 0` (or a negative value), this starts `runCommand` with an immediate timer; the shell can still run a short command such as `touch marker` before the timer kills it, so the invalid timeout does not reliably prevent the command from executing. Reject values below 1 before spawning.
2. In `@examples/agent-eval/src/run/jobConfig.ts` (line 76, Important):
`split(/,(?=\s*[A-Za-z_][\w-]*=)/)` also splits inside a kwarg value whenever text after a comma looks like `key=`. For example, `workglow:append_system_prompt="When x=a, set y=b"` is split into separate pairs, corrupting the prompt; JSON-valued kwargs with such a string are affected too. Only recognize separators outside the value (or otherwise provide an escaping/quoting rule), so valid values containing this pattern survive parsing.
3. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 453, Important):
`thinking_delta` is only appended to the native replay payload here; unlike text deltas, it is never emitted as a `text-delta` on the `reasoning` port or accumulated into `ToolCallingTaskOutput.reasoning`. Consequently, Anthropic turns retain thoughts for replay but `AgentTask`'s `onReasoningDelta` and reasoning output/UI never receive them. Emit the thinking delta on the reasoning port as well as retaining it for replay.
4. In `@providers/google-gemini/src/ai/common/Gemini_ToolCalling.ts` (line 140, Important):
`if (separateImageParts.length > 0) {`
This flushes the image turn after each individual `role: "tool"` message. With an assistant turn containing parallel calls represented by separate tool messages, an image in the first result produces `assistant(c1,c2) → functionResponse(c1) → user(image) → functionResponse(c2)`. Gemini receives the image before the response for the other outstanding call, so it may treat the image as the next user turn before all call results are supplied. Accumulate these images across consecutive tool messages and emit them after their function responses.
5. In `@providers/llamacpp-server/src/ai/common/LlamaCppServer_ToolCalling.ts` (line 83, Important):
`toOpenAIMessages` only emits the separate `input.systemPrompt` and skips `role: "system"` entries while iterating `input.messages`. When a caller supplies its instructions as a system `ChatMessage` and leaves `systemPrompt` unset, this request omits those instructions entirely, changing the model's behavior. Lift system messages from the history (as the text-generation path does) before conversion.
Additional findings outside changed lines:
1. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
2. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
3. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (3) — the lines they point at have not changed:
- 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 12, 2026 00:08
4197146 to
03d69da
Compare
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 12, 2026 05:15
acad604 to
22f7425
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/161 files
Request changes — found 15 issue(s) at 22f7425.
Actionable comments posted: 15
Coverage is incomplete: reviewed 60/161 files; 0 skipped; 101 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 254, Important):
`const tail = truncateTail(result.output)`
The capture cap limits `result.output` to 16 MiB, but `truncateTail` splits the entire captured string into an array before keeping only 2,000 lines. A command that emits millions of short lines can therefore multiply the capped output into a very large array and exhaust the agent; the byte cap does not bound this allocation.
2. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 72, Important):
`const lines = content.split("\n")`
When an exact match is absent, `fuzzyRanges` splits the entire file and then `normalized` maps every line into a second array. A newline-dense file near the allowed 64 MiB limit can create millions of entries and exhaust agent memory even though it passes the size check. Bound the fuzzy search's line materialization.
3. In `@examples/agent-eval/src/agent/tools/read.ts` (line 154, Important):
`(text.endsWith("\n") ? text.slice(0, -1) : text).split("\n")`
`MAX_READ_BYTES` limits the file to 64 MiB, but this still allocates one array entry per line before the output cap is applied; `window` then copies the array, and `truncateHead` splits the joined text again. A newline-dense file within the allowed size can therefore expand to hundreds of megabytes and OOM the agent. Keep only the requested/output-bounded lines while scanning.
4. In `@examples/agent-eval/src/compare/harborJobs.ts` (line 161, Important):
`models` includes `undefined`, so when one trial lacks `agent_info.model_info` while another has it, the known trial gets a model suffix but the unknown one does not. For example, a blocked setup trial for `workglow` is labeled `workglow` while successful trials are `workglow@anthropic/m`; `buildReport` then treats them as separate arms, leaving the blocked trial out of the actual arm’s totals. Resolve the model from available configuration or keep unknown-model trials grouped with their arm.
5. In `@examples/agent-eval/src/compare/harborJobs.ts` (line 204, Important):
This API-error branch returns before the later `OVERFLOW.test(text)` fallback. A non-Workglow trial with `exceptionType: "ApiError"` and a message such as `maximum context length exceeded` is therefore reported as `provider-error`, not `context-overflow`, making the failure-mode table misleading for API-reported context limits. Check the overflow message before classifying the error as a generic provider failure.
6. In `@packages/ai/src/task/AgentTask.ts` (line 450, Important):
`orderedToolBody` inserts a newline when it merges adjacent text blocks, but `runCalls` later computes `AgentToolRecord.chars` with `toolResultText`, which concatenates text blocks without separators. For a `ToolResultContent` containing `"a"` and `"b"`, the model receives `"a\nb"` while the step records 2 characters instead of 3, undercounting the tool-result metric.
Additional findings outside changed lines:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 54, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 69, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
4. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 249, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
5. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
6. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 334, Blocker):
Avoid replaying signed thinking when forced choice disables it
`buildAnthropicMessages` replays the prior signed thinking blocks unconditionally for this model key, but on a legacy model with `toolChoice: "required"` (or a named forced choice), `applyAnthropicThinkingParams` deliberately omits `thinking` because Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run.
7. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
8. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
9. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (9) — the lines they point at have not changed:
- 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 54 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 69 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 249 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Avoid replaying signed thinking when forced choice disables it ·
providers/anthropic/src/ai/common/Anthropic_ToolCalling.tsline 334 · from0a9a5fb
buildAnthropicMessagesreplays the prior signed thinking blocks unconditionally for this model key, but on a legacy model withtoolChoice: "required"(or a named forced choice),applyAnthropicThinkingParamsdeliberately omitsthinkingbecause Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 12, 2026 05:25
22f7425 to
95d996b
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/161 files
Request changes — found 13 issue(s) at 95d996b.
Actionable comments posted: 13
Coverage is incomplete: reviewed 59/161 files; 1 skipped; 101 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/harbor/workglow_agent.py` (line 290, Important):
`stop` is passed as a relative `--stop-after-sec` only after `run()` has uploaded the instruction and credentials; the Node timer does not start until the agent process launches. Harbor's timeout, however, covers `run()` from its entry (as `_agent_timeout_sec` documents), so if those uploads take longer than the reserved margin (10 seconds for typical limits), the hard-stop fires after Harbor has already cancelled the run, and the summary/trajectory are lost. Calculate this delay from a timestamp taken at `run()` entry, or otherwise account for setup time when setting the relative stop.
2. In `@examples/agent-eval/src/agent/runSummary.ts` (line 99, Important):
`Usage.cached`/`imageCached` being `undefined` means the provider did not report those counts, not that they were zero. A round with reported input/output but no cache count passes `hasTokenCounts`, so this adds zero here and leaves `unreportedRounds` at zero; `workglow_agent.py` then publishes `n_cache_tokens: 0`, and the comparison marks the trial fully priced in tokens instead of flagging cache usage as unknown. Preserve per-category completeness (or otherwise avoid publishing an invented zero) so providers that omit cache accounting are not reported as having no cache tokens.
3. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 66, Important):
`fuzzyRanges` retries the entire `wanted` sequence from every line start. On a permitted newline-dense file with repeated `a` lines, an `oldText` containing hundreds or thousands of `a` lines followed by a non-matching final line makes each candidate scan nearly the whole needle, yielding O(file lines × needle lines) normalization work and effectively hanging the agent. Use a search that does not restart the full comparison at every line (or otherwise bound this work).
4. In `@examples/agent-eval/src/agent/tools/truncate.ts` (line 126, Important):
This adds one byte for a newline even when `text` has no final newline. For example, `truncateTail("abc", 100, 3)` treats the three-byte input as too large and returns only `"bc"` with `truncated: true`; at the default limit, an unterminated final line exactly 50 KiB loses its first character. Count the separator only when it is actually present.
Additional findings outside changed lines:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 54, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 69, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
4. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 249, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
5. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
6. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 334, Blocker):
Avoid replaying signed thinking when forced choice disables it
`buildAnthropicMessages` replays the prior signed thinking blocks unconditionally for this model key, but on a legacy model with `toolChoice: "required"` (or a named forced choice), `applyAnthropicThinkingParams` deliberately omits `thinking` because Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run.
7. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
8. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
9. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (9) — the lines they point at have not changed:
- 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 54 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 69 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 249 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Avoid replaying signed thinking when forced choice disables it ·
providers/anthropic/src/ai/common/Anthropic_ToolCalling.tsline 334 · from0a9a5fb
buildAnthropicMessagesreplays the prior signed thinking blocks unconditionally for this model key, but on a legacy model withtoolChoice: "required"(or a named forced choice),applyAnthropicThinkingParamsdeliberately omitsthinkingbecause Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 12, 2026 05:34
95d996b to
259dff5
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/161 files
Request changes — found 13 issue(s) at 259dff5.
Actionable comments posted: 13
Coverage is incomplete: reviewed 60/161 files; 0 skipped; 101 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/harbor/workglow_agent.py` (line 300, Important):
`max(1, stop - spent)` can move the stop past Harbor's deadline when setup has consumed nearly all of a short trial. For a 2-second limit, `_stop_after_sec()` returns 1 second; if the uploads take 1.1 seconds, `spent` is 1 and this still gives the agent a 1-second timer, so it stops at about 2.1 seconds—after Harbor cancels `run()` at 2 seconds, losing the summary and trajectory. The same minimum clamp on the derived loop budget can likewise leave it running beyond the outer deadline.
2. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 45, Important):
`found.push(at)`
`locate()` only needs to distinguish zero, one, or multiple matches, but `exactRanges` retains every hit (and `fuzzyRanges` similarly accumulates every matching range). On an allowed 64 MiB file containing repeated `a` characters, editing `oldText: "a"` builds tens of millions of indices before reporting ambiguity; a fuzzy match repeated across many lines can build millions of range objects. This can exhaust the benchmark agent instead of returning the ambiguity error; stop collecting once uniqueness is disproven.
3. In `@examples/agent-eval/src/run/suites.ts` (line 113, Important):
`splitStar` rejects every pattern containing `?`, so `narrowDataset` throws for valid, directly expressible filters such as `--suite polyglot-lite --tasks 'polyglot_python_?ank'`: that filter is already a subset of the suite's `polyglot_python_*` pattern, but `globIntersection` returns `undefined` instead of retaining it. This makes the advertised glob filter unusable with `?` on restricted suites.
4. In `@examples/agent-eval/src/test/tools.test.ts` (line 580, Important):
`loadCredentialsFile` adds `WORKGLOW_TEST_API_KEY` after the Vitest process has started, but Linux `/proc/<pid>/environ` exposes the process's exec-time environment; variables added later are not reflected there. This assertion can therefore pass even if a provider key inherited when the agent starts remains readable to model-controlled commands via `/proc/$PPID/environ`. Exercise the check in a subprocess launched with the credential already in its environment.
Additional findings outside changed lines:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 54, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 69, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
4. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 249, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
5. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
6. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 334, Blocker):
Avoid replaying signed thinking when forced choice disables it
`buildAnthropicMessages` replays the prior signed thinking blocks unconditionally for this model key, but on a legacy model with `toolChoice: "required"` (or a named forced choice), `applyAnthropicThinkingParams` deliberately omits `thinking` because Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run.
7. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
8. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
9. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (9) — the lines they point at have not changed:
- 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 54 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 69 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 249 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Avoid replaying signed thinking when forced choice disables it ·
providers/anthropic/src/ai/common/Anthropic_ToolCalling.tsline 334 · from0a9a5fb
buildAnthropicMessagesreplays the prior signed thinking blocks unconditionally for this model key, but on a legacy model withtoolChoice: "required"(or a named forced choice),applyAnthropicThinkingParamsdeliberately omitsthinkingbecause Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
| flags = f"{flags} --max-duration-sec {max(1, budget - spent)}".strip() | ||
| stop = self._stop_after_sec() | ||
| if stop is not None: | ||
| flags = f"{flags} --stop-after-sec {max(1, stop - spent)}".strip() |
There was a problem hiding this comment.
Keep the hard stop inside the remaining trial budget
max(1, stop - spent) can move the stop past Harbor's deadline when setup has consumed nearly all of a short trial. For a 2-second limit, _stop_after_sec() returns 1 second; if the uploads take 1.1 seconds, spent is 1 and this still gives the agent a 1-second timer, so it stops at about 2.1 seconds—after Harbor cancels run() at 2 seconds, losing the summary and trajectory. The same minimum clamp on the derived loop budget can likewise leave it running beyond the outer deadline.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@examples/agent-eval/harbor/workglow_agent.py` (line 300, Important):
`max(1, stop - spent)` can move the stop past Harbor's deadline when setup has consumed nearly all of a short trial. For a 2-second limit, `_stop_after_sec()` returns 1 second; if the uploads take 1.1 seconds, `spent` is 1 and this still gives the agent a 1-second timer, so it stops at about 2.1 seconds—after Harbor cancels `run()` at 2 seconds, losing the summary and trajectory. The same minimum clamp on the derived loop budget can likewise leave it running beyond the outer deadline.
| function exactRanges(content: string, needle: string): number[] { | ||
| const found: number[] = []; | ||
| let at = content.indexOf(needle); | ||
| while (at !== -1) { |
There was a problem hiding this comment.
Bound match collection to the uniqueness check
found.push(at)
locate() only needs to distinguish zero, one, or multiple matches, but exactRanges retains every hit (and fuzzyRanges similarly accumulates every matching range). On an allowed 64 MiB file containing repeated a characters, editing oldText: "a" builds tens of millions of indices before reporting ambiguity; a fuzzy match repeated across many lines can build millions of range objects. This can exhaust the benchmark agent instead of returning the ambiguity error; stop collecting once uniqueness is disproven.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@examples/agent-eval/src/agent/tools/edit.ts` (line 45, Important):
`found.push(at)`
`locate()` only needs to distinguish zero, one, or multiple matches, but `exactRanges` retains every hit (and `fuzzyRanges` similarly accumulates every matching range). On an allowed 64 MiB file containing repeated `a` characters, editing `oldText: "a"` builds tens of millions of indices before reporting ambiguity; a fuzzy match repeated across many lines can build millions of range objects. This can exhaust the benchmark agent instead of returning the ambiguity error; stop collecting once uniqueness is disproven.
| /** A glob with exactly one `*` and no `?`, split at the star. */ | ||
| function splitStar(glob: string): { readonly head: string; readonly tail: string } | undefined { | ||
| const star = glob.indexOf("*"); | ||
| if (star < 0 || glob.indexOf("*", star + 1) >= 0 || glob.includes("?")) return undefined; |
There was a problem hiding this comment.
Allow expressible ? glob intersections
splitStar rejects every pattern containing ?, so narrowDataset throws for valid, directly expressible filters such as --suite polyglot-lite --tasks 'polyglot_python_?ank': that filter is already a subset of the suite's polyglot_python_* pattern, but globIntersection returns undefined instead of retaining it. This makes the advertised glob filter unusable with ? on restricted suites.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@examples/agent-eval/src/run/suites.ts` (line 113, Important):
`splitStar` rejects every pattern containing `?`, so `narrowDataset` throws for valid, directly expressible filters such as `--suite polyglot-lite --tasks 'polyglot_python_?ank'`: that filter is already a subset of the suite's `polyglot_python_*` pattern, but `globIntersection` returns `undefined` instead of retaining it. This makes the advertised glob filter unusable with `?` on restricted suites.
| // a filter on the shell's environment cannot reach. | ||
| const out = String( | ||
| await createBashTool(context).execute!( | ||
| { command: "env; tr '\\0' '\\n' < /proc/$PPID/environ" }, |
There was a problem hiding this comment.
Test an exec-time parent credential
loadCredentialsFile adds WORKGLOW_TEST_API_KEY after the Vitest process has started, but Linux /proc/<pid>/environ exposes the process's exec-time environment; variables added later are not reflected there. This assertion can therefore pass even if a provider key inherited when the agent starts remains readable to model-controlled commands via /proc/$PPID/environ. Exercise the check in a subprocess launched with the credential already in its environment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@examples/agent-eval/src/test/tools.test.ts` (line 580, Important):
`loadCredentialsFile` adds `WORKGLOW_TEST_API_KEY` after the Vitest process has started, but Linux `/proc/<pid>/environ` exposes the process's exec-time environment; variables added later are not reflected there. This assertion can therefore pass even if a provider key inherited when the agent starts remains readable to model-controlled commands via `/proc/$PPID/environ`. Exercise the check in a subprocess launched with the credential already in its environment.
There was a problem hiding this comment.
Review progress ███████░░░ 105/161 files
Request changes — found 11 issue(s) at d82a24d.
Actionable comments posted: 11
Coverage is incomplete: reviewed 105/161 files; 0 skipped; 56 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@providers/anthropic/src/ai/common/Anthropic_CacheCheckpoint.ts` (line 93, Blocker):
`buildAnthropicCheckpointParams` now replays the prefix's signed thinking blocks, but the resulting request only sets `model` and `max_tokens: 1` and never applies the model's thinking parameters. When a prefix contains a thinking turn (for example, one produced with legacy extended thinking), `messages.create` sends those blocks with thinking disabled and Anthropic rejects the warm-up, so the `CacheCheckpointTask` fails instead of warming the prefix.
2. In `@examples/agent-eval/src/run/suites.ts` (line 166, Important):
`splitStar()` rejects any pattern containing `?`, and the fallback `subsetOfPrefix()` also refuses patterns containing `*`. Thus `--suite polyglot-lite --tasks 'polyglot_python_?*'` throws “cannot be narrowed” even though this is a valid fnmatch glob and its matches are entirely within the suite's `polyglot_python_*` restriction. This prevents narrowing a restricted suite with a common one-star/`?` filter.
Additional findings outside changed lines:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 89, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 104, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
4. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 255, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
5. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
6. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 334, Blocker):
Avoid replaying signed thinking when forced choice disables it
`buildAnthropicMessages` replays the prior signed thinking blocks unconditionally for this model key, but on a legacy model with `toolChoice: "required"` (or a named forced choice), `applyAnthropicThinkingParams` deliberately omits `thinking` because Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run.
7. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
8. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
9. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (9) — the lines they point at have not changed:
- 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 89 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 104 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 255 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Avoid replaying signed thinking when forced choice disables it ·
providers/anthropic/src/ai/common/Anthropic_ToolCalling.tsline 334 · from0a9a5fb
buildAnthropicMessagesreplays the prior signed thinking blocks unconditionally for this model key, but on a legacy model withtoolChoice: "required"(or a named forced choice),applyAnthropicThinkingParamsdeliberately omitsthinkingbecause Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
| const messages = | ||
| prefix.messages && prefix.messages.length > 0 | ||
| ? buildAnthropicMessages(prefix.messages, "") | ||
| ? buildAnthropicMessages(prefix.messages, "", `anthropic:${modelName}`) |
There was a problem hiding this comment.
🔴 Blocker · Blocker
Do not warm signed turns without thinking enabled
buildAnthropicCheckpointParams now replays the prefix's signed thinking blocks, but the resulting request only sets model and max_tokens: 1 and never applies the model's thinking parameters. When a prefix contains a thinking turn (for example, one produced with legacy extended thinking), messages.create sends those blocks with thinking disabled and Anthropic rejects the warm-up, so the CacheCheckpointTask fails instead of warming the prefix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@providers/anthropic/src/ai/common/Anthropic_CacheCheckpoint.ts` (line 93, Blocker):
`buildAnthropicCheckpointParams` now replays the prefix's signed thinking blocks, but the resulting request only sets `model` and `max_tokens: 1` and never applies the model's thinking parameters. When a prefix contains a thinking turn (for example, one produced with legacy extended thinking), `messages.create` sends those blocks with thinking disabled and Anthropic rejects the warm-up, so the `CacheCheckpointTask` fails instead of warming the prefix.
| if (!/[*?]/.test(b)) return globRegExp(a).test(b) ? [b] : []; | ||
| const sa = splitStar(a); | ||
| const sb = splitStar(b); | ||
| if (!sa || !sb) { |
There was a problem hiding this comment.
Allow question-mark filters with one star
splitStar() rejects any pattern containing ?, and the fallback subsetOfPrefix() also refuses patterns containing *. Thus --suite polyglot-lite --tasks 'polyglot_python_?*' throws “cannot be narrowed” even though this is a valid fnmatch glob and its matches are entirely within the suite's polyglot_python_* restriction. This prevents narrowing a restricted suite with a common one-star/? filter.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@examples/agent-eval/src/run/suites.ts` (line 166, Important):
`splitStar()` rejects any pattern containing `?`, and the fallback `subsetOfPrefix()` also refuses patterns containing `*`. Thus `--suite polyglot-lite --tasks 'polyglot_python_?*'` throws “cannot be narrowed” even though this is a valid fnmatch glob and its matches are entirely within the suite's `polyglot_python_*` restriction. This prevents narrowing a restricted suite with a common one-star/`?` filter.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 12, 2026 23:33
d82a24d to
849475b
Compare
There was a problem hiding this comment.
Review progress ██████████ 157/161 files
Request changes — found 12 issue(s) at 849475b.
Actionable comments posted: 12
Coverage is incomplete: reviewed 157/161 files; 0 skipped; 4 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/run/suites.ts` (line 164, Important):
`return head[i] === "?" ? undefined : [];`
For `--suite polyglot-lite --tasks 'polyglot_pytho?_*'`, the `?` can only match the suite prefix's `n`, so the intersection with `polyglot_python_*` is exactly that allowed prefix glob. This returns `undefined`, causing `narrowDataset` to throw “cannot be narrowed” and the CLI rejects a valid filter; emit the representable intersection instead.
2. In `@providers/anthropic/src/ai/common/Anthropic_CacheCheckpoint.ts` (line 133, Important):
`params.thinking === undefined` misses an explicit `{ type: "disabled" }` from `provider_config.thinking`. If a prefix contains signed thinking from an earlier enabled round and the checkpoint uses that disabled configuration, this leaves the signed blocks in `messages`; Anthropic rejects the request instead of warming the checkpoint. Treat disabled thinking as unavailable for replay too.
Additional findings outside changed lines:
1. In `@providers/anthropic/src/ai/common/Anthropic_CacheCheckpoint.ts` (line 93, Blocker):
Do not warm signed turns without thinking enabled
`buildAnthropicCheckpointParams` now replays the prefix's signed thinking blocks, but the resulting request only sets `model` and `max_tokens: 1` and never applies the model's thinking parameters. When a prefix contains a thinking turn (for example, one produced with legacy extended thinking), `messages.create` sends those blocks with thinking disabled and Anthropic rejects the warm-up, so the `CacheCheckpointTask` fails instead of warming the prefix.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 104, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
4. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 89, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
5. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 255, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
6. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
7. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 334, Blocker):
Avoid replaying signed thinking when forced choice disables it
`buildAnthropicMessages` replays the prior signed thinking blocks unconditionally for this model key, but on a legacy model with `toolChoice: "required"` (or a named forced choice), `applyAnthropicThinkingParams` deliberately omits `thinking` because Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run.
8. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
9. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
10. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (10) — the lines they point at have not changed:
- 🔴 Blocker · Do not warm signed turns without thinking enabled ·
providers/anthropic/src/ai/common/Anthropic_CacheCheckpoint.tsline 93 · fromd82a24d
buildAnthropicCheckpointParamsnow replays the prefix's signed thinking blocks, but the resulting request only setsmodelandmax_tokens: 1and never applies the model's thinking parameters. When a prefix contains a thinking turn (for example, one produced with legacy extended thinking),messages.createsends those blocks with thinking disabled and Anthropic rejects the warm-up, so theCacheCheckpointTaskfails instead of warming the prefix. - 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 104 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 89 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 255 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Avoid replaying signed thinking when forced choice disables it ·
providers/anthropic/src/ai/common/Anthropic_ToolCalling.tsline 334 · from0a9a5fb
buildAnthropicMessagesreplays the prior signed thinking blocks unconditionally for this model key, but on a legacy model withtoolChoice: "required"(or a named forced choice),applyAnthropicThinkingParamsdeliberately omitsthinkingbecause Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
| const compared = Math.min(head.length, prefix.length); | ||
| for (let i = 0; i < compared; i++) { | ||
| if (head[i] === prefix[i] || prefix[i] === "?") continue; | ||
| return head[i] === "?" ? undefined : []; |
There was a problem hiding this comment.
Accept ? wildcards pinned by the suite prefix
return head[i] === "?" ? undefined : [];
For --suite polyglot-lite --tasks 'polyglot_pytho?_*', the ? can only match the suite prefix's n, so the intersection with polyglot_python_* is exactly that allowed prefix glob. This returns undefined, causing narrowDataset to throw “cannot be narrowed” and the CLI rejects a valid filter; emit the representable intersection instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@examples/agent-eval/src/run/suites.ts` (line 164, Important):
`return head[i] === "?" ? undefined : [];`
For `--suite polyglot-lite --tasks 'polyglot_pytho?_*'`, the `?` can only match the suite prefix's `n`, so the intersection with `polyglot_python_*` is exactly that allowed prefix glob. This returns `undefined`, causing `narrowDataset` to throw “cannot be narrowed” and the CLI rejects a valid filter; emit the representable intersection instead.
| // unknown and the replay is preserved. | ||
| if ( | ||
| model !== undefined && | ||
| messages.length > 0 && |
There was a problem hiding this comment.
Strip signed turns when thinking is explicitly disabled
params.thinking === undefined misses an explicit { type: "disabled" } from provider_config.thinking. If a prefix contains signed thinking from an earlier enabled round and the checkpoint uses that disabled configuration, this leaves the signed blocks in messages; Anthropic rejects the request instead of warming the checkpoint. Treat disabled thinking as unavailable for replay too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@providers/anthropic/src/ai/common/Anthropic_CacheCheckpoint.ts` (line 133, Important):
`params.thinking === undefined` misses an explicit `{ type: "disabled" }` from `provider_config.thinking`. If a prefix contains signed thinking from an earlier enabled round and the checkpoint uses that disabled configuration, this leaves the signed blocks in `messages`; Anthropic rejects the request instead of warming the checkpoint. Treat disabled thinking as unavailable for replay too.
Collaborator
Author
|
@mergestorm-vortex review the rest. as for earlier comments: No changes made — every finding is already fixed in current code (HEAD 849475b and earlier). Verified by reading each file; validation: tools.test.ts 72 passed / 1 unrelated macOS /private symlink failure, jobConfig.test.ts 17 passed, py_compile OK.
|
There was a problem hiding this comment.
Review progress ██████████ 161/161 files
Request changes — found 10 issue(s) outside changed lines at 849475b.
Actionable comments posted: 10
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 104, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 89, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
4. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 255, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
5. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
6. In `@providers/anthropic/src/ai/common/Anthropic_CacheCheckpoint.ts` (line 93, Blocker):
Do not warm signed turns without thinking enabled
`buildAnthropicCheckpointParams` now replays the prefix's signed thinking blocks, but the resulting request only sets `model` and `max_tokens: 1` and never applies the model's thinking parameters. When a prefix contains a thinking turn (for example, one produced with legacy extended thinking), `messages.create` sends those blocks with thinking disabled and Anthropic rejects the warm-up, so the `CacheCheckpointTask` fails instead of warming the prefix.
7. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 334, Blocker):
Avoid replaying signed thinking when forced choice disables it
`buildAnthropicMessages` replays the prior signed thinking blocks unconditionally for this model key, but on a legacy model with `toolChoice: "required"` (or a named forced choice), `applyAnthropicThinkingParams` deliberately omits `thinking` because Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run.
8. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
9. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
10. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (10) — the lines they point at have not changed:
- 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 104 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 89 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 255 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Do not warm signed turns without thinking enabled ·
providers/anthropic/src/ai/common/Anthropic_CacheCheckpoint.tsline 93 · fromd82a24d
buildAnthropicCheckpointParamsnow replays the prefix's signed thinking blocks, but the resulting request only setsmodelandmax_tokens: 1and never applies the model's thinking parameters. When a prefix contains a thinking turn (for example, one produced with legacy extended thinking),messages.createsends those blocks with thinking disabled and Anthropic rejects the warm-up, so theCacheCheckpointTaskfails instead of warming the prefix. - 🔴 Blocker · Avoid replaying signed thinking when forced choice disables it ·
providers/anthropic/src/ai/common/Anthropic_ToolCalling.tsline 334 · from0a9a5fb
buildAnthropicMessagesreplays the prior signed thinking blocks unconditionally for this model key, but on a legacy model withtoolChoice: "required"(or a named forced choice),applyAnthropicThinkingParamsdeliberately omitsthinkingbecause Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
There was a problem hiding this comment.
Review progress ██████████ 168/168 files
Request changes — found 10 issue(s) outside changed lines at 92d7ea3.
Actionable comments posted: 10
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 104, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 89, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
4. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 255, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
5. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
6. In `@providers/anthropic/src/ai/common/Anthropic_CacheCheckpoint.ts` (line 93, Blocker):
Do not warm signed turns without thinking enabled
`buildAnthropicCheckpointParams` now replays the prefix's signed thinking blocks, but the resulting request only sets `model` and `max_tokens: 1` and never applies the model's thinking parameters. When a prefix contains a thinking turn (for example, one produced with legacy extended thinking), `messages.create` sends those blocks with thinking disabled and Anthropic rejects the warm-up, so the `CacheCheckpointTask` fails instead of warming the prefix.
7. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 334, Blocker):
Avoid replaying signed thinking when forced choice disables it
`buildAnthropicMessages` replays the prior signed thinking blocks unconditionally for this model key, but on a legacy model with `toolChoice: "required"` (or a named forced choice), `applyAnthropicThinkingParams` deliberately omits `thinking` because Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run.
8. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
9. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
10. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (10) — the lines they point at have not changed:
- 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 104 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 89 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 255 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Do not warm signed turns without thinking enabled ·
providers/anthropic/src/ai/common/Anthropic_CacheCheckpoint.tsline 93 · fromd82a24d
buildAnthropicCheckpointParamsnow replays the prefix's signed thinking blocks, but the resulting request only setsmodelandmax_tokens: 1and never applies the model's thinking parameters. When a prefix contains a thinking turn (for example, one produced with legacy extended thinking),messages.createsends those blocks with thinking disabled and Anthropic rejects the warm-up, so theCacheCheckpointTaskfails instead of warming the prefix. - 🔴 Blocker · Avoid replaying signed thinking when forced choice disables it ·
providers/anthropic/src/ai/common/Anthropic_ToolCalling.tsline 334 · from0a9a5fb
buildAnthropicMessagesreplays the prior signed thinking blocks unconditionally for this model key, but on a legacy model withtoolChoice: "required"(or a named forced choice),applyAnthropicThinkingParamsdeliberately omitsthinkingbecause Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
…nd native-turn storage
- Each tool's ToolExecuteContext carries the turn's deadline. announceTimeLeft
appends the time left to each round's last tool result. A retry wait that
would pass the deadline ends the turn "budget".
- reviewPrompt asks the model for one check of its answer before the turn ends.
- Assistant turns keep their reasoning, and a provider's own turn
({ provider, payload }) rides on the reasoning block so the provider can
replay it; history trimming keeps the current turn whole and measures earlier
ones without their payloads.
- Tool results may carry images. Empty tool results and reasoning-only turns
are sent as strings, and Ollama and llama.cpp server histories are converted
instead of forwarded as content blocks.
- Each round's reasoning streams on the task's own reasoning port, held with
the text until the attempt settles; AgentRoundContext gains onReasoningDelta.
OpenAI replays the Responses items with their encrypted reasoning, and Anthropic its content blocks with their signed thinking, keyed by provider and model so another model gets the rebuilt turn. A turn is replayed only while its call ids still match, and emitted only when it is truly replayable: OpenAI requests encrypted reasoning only for a reasoning model and rebuilds earlier turns when it does not ask for it; Anthropic retries once with thinking stripped when a request is rejected as edited. Both send tool-result images on a user turn after the results (OpenAI) or beside them (Anthropic).
…s on cloud APIs - DeepSeek sends its reasoning effort and replays reasoning_content. - OpenRouter replays reasoning_details beside the same tool-call ids, never thinking truncated before its signature, and never a turn with a call it filtered out. - xAI, OpenRouter and HF Inference send tool-result images in a user message; HF Inference gains an effort setting. - Gemini sends tool results as objects with errors under error, emits thought summaries as reasoning on Gemini 2.5 and later, and puts tool-result images in functionResponse.parts on Gemini 3+, or in a labelled turn of their own on older models, which reject that field.
…s on local runtimes - Ollama and llama.cpp server send the whole conversation with their own tool calls and replay thinking / reasoning_content; Ollama gains a think effort setting and sends tool-result images on a user turn after the results. - node-llama-cpp emits and replays thought segments, maps effort to a thought budget and never inlines image data. - HF Transformers routes <think> to reasoning (ThinkTagFilter), hands it back to the chat template whenever the history carries it, and names tool results. - Cactus pairs the latest user turn with the turn's prompt and emits its reasoning; Chrome AI's finish has the same shape as the other providers.
Ink rows, the web console and examples/web's TaskNode show the reasoning port separately and drop it once the answer starts; every other port is still the task's text. workglow agent chat and the run-event projection no longer print reasoning as answer text, and StreamingTextRow also draws while a task is streaming.
…nchmarks Runs AgentTask, opencode and pi on Terminal-Bench 2.0, Aider Polyglot and SWE-bench Verified through Harbor, with the same model, turn cap and reasoning level for every arm, and compares them task by task. The workglow arm is AgentTask plus read, bash, edit and write tools matching pi's: bash defaults to 120 s and is capped at the trial deadline, edit applies the edits that match and names the rest, and the system prompt lists the commands installed and asks for one verification once the change is complete. Includes ATIF trajectories, retries for trials the infrastructure lost, --tasks-dir for local task copies and a scripted mock model for offline runs.
The harness's tools read, write and run what a model hands them inside a throwaway container by design, which scanning reports as injection; default setup cannot exclude a path.
- Updated WorkglowAgent to handle setup time more accurately by using float values for spent time, ensuring proper budget calculations for max duration and stop after flags. - Introduced credential value scrubbing in bash tool outputs to prevent sensitive information from being logged, ensuring that only non-trivial credential values are redacted. - Improved error messages in edit tools to clarify ambiguous matches and occurrences, enhancing user feedback during operations.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 14, 2026 17:12
92d7ea3 to
a6d3fc8
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 61/168 files
Request changes — found 14 issue(s) at a6d3fc8.
Actionable comments posted: 14
Coverage is incomplete: reviewed 61/168 files; 0 skipped; 107 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 291, Blocker):
` scrubCredentialValues(result.output) ` only replaces the literal credential values. On Linux, a model-controlled command can read `/proc/$PPID/environ` and encode it (for example, `tr '\0' '\n' </proc/$PPID/environ | base64`); the encoded output contains none of the literal secrets, so it is returned to the model and may be persisted in the transcript/spill file, where the credential is trivially recoverable.
2. In `@examples/agent-eval/harbor/workglow_agent.py` (line 287, Important):
`await self._upload_credentials(environment)` runs before the `try/finally` below. If the credential file has already been uploaded but the method then fails (for example, its final `chmod 600` command fails), `run()` exits without executing the remote `rm`, leaving the API keys in `/installed-agent/secrets/credentials.json` in the task container. Include credential setup in the cleanup scope or remove the remote file on every failure after upload.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 286, Important):
When exactly 1,000 ms remain, the `< 1000` guard above permits the command and `commandTimeoutSec` rounds the budget to 1 second. `runCommand` starts that timer only after spawning the shell and setting up listeners, so a long command can run past the deadline by the spawn/setup and timer-scheduling delay. `AgentTask` only checks `maxDurationMs` between rounds; it does not abort this tool at the deadline.
4. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 384, Important):
`params.thinking === undefined`
When `provider_config.thinking` is explicitly `{ type: "disabled" }`, `applyAnthropicThinkingParams` sets `params.thinking` to that object, so this condition leaves a prior same-model native turn's signed thinking blocks in `messages`. Anthropic rejects the continuation when history came from a thinking-enabled round (for example, persisted history after changing the setting), so the tool-calling run fails instead of rebuilding the turn. Treat explicit disabled thinking as ineligible for replay as well.
Additional findings outside changed lines:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 104, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 89, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
4. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 255, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
5. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
6. In `@providers/anthropic/src/ai/common/Anthropic_CacheCheckpoint.ts` (line 93, Blocker):
Do not warm signed turns without thinking enabled
`buildAnthropicCheckpointParams` now replays the prefix's signed thinking blocks, but the resulting request only sets `model` and `max_tokens: 1` and never applies the model's thinking parameters. When a prefix contains a thinking turn (for example, one produced with legacy extended thinking), `messages.create` sends those blocks with thinking disabled and Anthropic rejects the warm-up, so the `CacheCheckpointTask` fails instead of warming the prefix.
7. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 334, Blocker):
Avoid replaying signed thinking when forced choice disables it
`buildAnthropicMessages` replays the prior signed thinking blocks unconditionally for this model key, but on a legacy model with `toolChoice: "required"` (or a named forced choice), `applyAnthropicThinkingParams` deliberately omits `thinking` because Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run.
8. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
9. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
10. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (10) — the lines they point at have not changed:
- 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 104 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 89 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 255 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Do not warm signed turns without thinking enabled ·
providers/anthropic/src/ai/common/Anthropic_CacheCheckpoint.tsline 93 · fromd82a24d
buildAnthropicCheckpointParamsnow replays the prefix's signed thinking blocks, but the resulting request only setsmodelandmax_tokens: 1and never applies the model's thinking parameters. When a prefix contains a thinking turn (for example, one produced with legacy extended thinking),messages.createsends those blocks with thinking disabled and Anthropic rejects the warm-up, so theCacheCheckpointTaskfails instead of warming the prefix. - 🔴 Blocker · Avoid replaying signed thinking when forced choice disables it ·
providers/anthropic/src/ai/common/Anthropic_ToolCalling.tsline 334 · from0a9a5fb
buildAnthropicMessagesreplays the prior signed thinking blocks unconditionally for this model key, but on a legacy model withtoolChoice: "required"(or a named forced choice),applyAnthropicThinkingParamsdeliberately omitsthinkingbecause Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
|
|
||
| // Scrubbed before anything persists it: the transcript, the event log, | ||
| // and the spill file below all derive from this string. | ||
| const output = scrubCredentialValues(result.output); |
There was a problem hiding this comment.
🔴 Blocker · Blocker
Redaction misses encoded credentials
scrubCredentialValues(result.output) only replaces the literal credential values. On Linux, a model-controlled command can read /proc/$PPID/environ and encode it (for example, tr '\0' '\n' </proc/$PPID/environ | base64); the encoded output contains none of the literal secrets, so it is returned to the model and may be persisted in the transcript/spill file, where the credential is trivially recoverable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@examples/agent-eval/src/agent/tools/bash.ts` (line 291, Blocker):
` scrubCredentialValues(result.output) ` only replaces the literal credential values. On Linux, a model-controlled command can read `/proc/$PPID/environ` and encode it (for example, `tr '\0' '\n' </proc/$PPID/environ | base64`); the encoded output contains none of the literal secrets, so it is returned to the model and may be persisted in the transcript/spill file, where the credential is trivially recoverable.
| environment, local_instruction, _REMOTE_INSTRUCTION.as_posix() | ||
| ) | ||
|
|
||
| await self._upload_credentials(environment) |
There was a problem hiding this comment.
Clean up credentials when upload setup fails
await self._upload_credentials(environment) runs before the try/finally below. If the credential file has already been uploaded but the method then fails (for example, its final chmod 600 command fails), run() exits without executing the remote rm, leaving the API keys in /installed-agent/secrets/credentials.json in the task container. Include credential setup in the cleanup scope or remove the remote file on every failure after upload.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@examples/agent-eval/harbor/workglow_agent.py` (line 287, Important):
`await self._upload_credentials(environment)` runs before the `try/finally` below. If the credential file has already been uploaded but the method then fails (for example, its final `chmod 600` command fails), `run()` exits without executing the remote `rm`, leaving the API keys in `/installed-agent/secrets/credentials.json` in the task container. Include credential setup in the cleanup scope or remove the remote file on every failure after upload.
| "No time is left for this task; the command was not run. Finish with what you have." | ||
| ); | ||
| } | ||
| const { sec: timeoutSec, capped } = commandTimeoutSec(requested, deadline, Date.now()); |
There was a problem hiding this comment.
Timeout can exceed the turn deadline
When exactly 1,000 ms remain, the < 1000 guard above permits the command and commandTimeoutSec rounds the budget to 1 second. runCommand starts that timer only after spawning the shell and setting up listeners, so a long command can run past the deadline by the spawn/setup and timer-scheduling delay. AgentTask only checks maxDurationMs between rounds; it does not abort this tool at the deadline.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@examples/agent-eval/src/agent/tools/bash.ts` (line 286, Important):
When exactly 1,000 ms remain, the `< 1000` guard above permits the command and `commandTimeoutSec` rounds the budget to 1 second. `runCommand` starts that timer only after spawning the shell and setting up listeners, so a long command can run past the deadline by the spawn/setup and timer-scheduling delay. `AgentTask` only checks `maxDurationMs` between rounds; it does not abort this tool at the deadline.
| // output_config.effort present) keeps replaying its turn verbatim. | ||
| if ( | ||
| (toolChoice?.type === "any" || toolChoice?.type === "tool") && | ||
| params.thinking === undefined && |
There was a problem hiding this comment.
Do not replay signed blocks when thinking is disabled
params.thinking === undefined
When provider_config.thinking is explicitly { type: "disabled" }, applyAnthropicThinkingParams sets params.thinking to that object, so this condition leaves a prior same-model native turn's signed thinking blocks in messages. Anthropic rejects the continuation when history came from a thinking-enabled round (for example, persisted history after changing the setting), so the tool-calling run fails instead of rebuilding the turn. Treat explicit disabled thinking as ineligible for replay as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 384, Important):
`params.thinking === undefined`
When `provider_config.thinking` is explicitly `{ type: "disabled" }`, `applyAnthropicThinkingParams` sets `params.thinking` to that object, so this condition leaves a prior same-model native turn's signed thinking blocks in `messages`. Anthropic rejects the continuation when history came from a thinking-enabled round (for example, persisted history after changing the setting), so the tool-calling run fails instead of rebuilding the turn. Treat explicit disabled thinking as ineligible for replay as well.
There was a problem hiding this comment.
Review progress ███████░░░ 118/168 files
Request changes — found 14 issue(s) at 9dcb6d6.
Actionable comments posted: 14
Coverage is incomplete: reviewed 118/168 files; 0 skipped; 50 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@providers/cactus/src/ai/common/Cactus_ToolCallingCore.ts` (line 59, Blocker):
`extractMessageText(messages[i].content)` only extracts the latest user turn; it never includes the preceding `role: "tool"` result. In an AgentTask round after a tool runs, `input.prompt` and that latest user turn are still the original question, so this stateless Needle request receives no tool output and can repeat the same tool call instead of answering from its result. Include the relevant tool-result history in the query sent to the engine.
2. In `@examples/agent-eval/src/agent/tools/truncate.ts` (line 132, Important):
`truncateTail("\n\n\n", 3, 2)` keeps two blank lines, then this branch treats the third empty line as an oversized partial line and appends it. The returned text is `"\n\n\n"` (3 bytes), exceeding `maxBytes` of 2. Avoid adding a partial line when there is no content to trim, or otherwise ensure the fallback accounts for the newline bytes in the final output.
3. In `@providers/cactus/src/ai/common/Cactus_ParseToolCalls.ts` (line 94, Important):
This now routes the body of an unclosed `<think>` block to reasoning, but `Cactus_ToolCallingCore` still calls `parseNeedleToolCalls(raw)`, whose `stripNeedleReasoning` removes only closed think fences. For example, an unterminated `<think>` containing a complete `<tool_call>[{"name":"lookup_weather","arguments":{"city":"Paris"}}]</tool_call>` is hidden from the text stream yet still becomes an emitted tool call and can execute. Make the parse path ignore tool-call markup inside an unclosed thought as well.
Additional findings outside changed lines:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 291, Blocker):
Redaction misses encoded credentials
` scrubCredentialValues(result.output) ` only replaces the literal credential values. On Linux, a model-controlled command can read `/proc/$PPID/environ` and encode it (for example, `tr '\0' '\n' </proc/$PPID/environ | base64`); the encoded output contains none of the literal secrets, so it is returned to the model and may be persisted in the transcript/spill file, where the credential is trivially recoverable.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 104, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
4. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 89, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
5. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 255, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
6. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
7. In `@providers/anthropic/src/ai/common/Anthropic_CacheCheckpoint.ts` (line 93, Blocker):
Do not warm signed turns without thinking enabled
`buildAnthropicCheckpointParams` now replays the prefix's signed thinking blocks, but the resulting request only sets `model` and `max_tokens: 1` and never applies the model's thinking parameters. When a prefix contains a thinking turn (for example, one produced with legacy extended thinking), `messages.create` sends those blocks with thinking disabled and Anthropic rejects the warm-up, so the `CacheCheckpointTask` fails instead of warming the prefix.
8. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 334, Blocker):
Avoid replaying signed thinking when forced choice disables it
`buildAnthropicMessages` replays the prior signed thinking blocks unconditionally for this model key, but on a legacy model with `toolChoice: "required"` (or a named forced choice), `applyAnthropicThinkingParams` deliberately omits `thinking` because Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run.
9. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
10. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
11. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (11) — the lines they point at have not changed:
- 🔴 Blocker · Redaction misses encoded credentials ·
examples/agent-eval/src/agent/tools/bash.tsline 291 · froma6d3fc8
scrubCredentialValues(result.output)only replaces the literal credential values. On Linux, a model-controlled command can read/proc/$PPID/environand encode it (for example,tr '\0' '\n' </proc/$PPID/environ | base64); the encoded output contains none of the literal secrets, so it is returned to the model and may be persisted in the transcript/spill file, where the credential is trivially recoverable. - 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 104 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 89 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 255 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Do not warm signed turns without thinking enabled ·
providers/anthropic/src/ai/common/Anthropic_CacheCheckpoint.tsline 93 · fromd82a24d
buildAnthropicCheckpointParamsnow replays the prefix's signed thinking blocks, but the resulting request only setsmodelandmax_tokens: 1and never applies the model's thinking parameters. When a prefix contains a thinking turn (for example, one produced with legacy extended thinking),messages.createsends those blocks with thinking disabled and Anthropic rejects the warm-up, so theCacheCheckpointTaskfails instead of warming the prefix. - 🔴 Blocker · Avoid replaying signed thinking when forced choice disables it ·
providers/anthropic/src/ai/common/Anthropic_ToolCalling.tsline 334 · from0a9a5fb
buildAnthropicMessagesreplays the prior signed thinking blocks unconditionally for this model key, but on a legacy model withtoolChoice: "required"(or a named forced choice),applyAnthropicThinkingParamsdeliberately omitsthinkingbecause Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
| if (messages && messages.length > 0) { | ||
| for (let i = messages.length - 1; i >= 0; i--) { | ||
| if (messages[i].role !== "user") continue; | ||
| const latest = extractMessageText(messages[i].content); |
There was a problem hiding this comment.
🔴 Blocker · Blocker
Include tool results in subsequent-round queries
extractMessageText(messages[i].content) only extracts the latest user turn; it never includes the preceding role: "tool" result. In an AgentTask round after a tool runs, input.prompt and that latest user turn are still the original question, so this stateless Needle request receives no tool output and can repeat the same tool call instead of answering from its result. Include the relevant tool-result history in the query sent to the engine.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@providers/cactus/src/ai/common/Cactus_ToolCallingCore.ts` (line 59, Blocker):
`extractMessageText(messages[i].content)` only extracts the latest user turn; it never includes the preceding `role: "tool"` result. In an AgentTask round after a tool runs, `input.prompt` and that latest user turn are still the original question, so this stateless Needle request receives no tool output and can repeat the same tool call instead of answering from its result. Include the relevant tool-result history in the query sent to the engine.
| const size = byteLength(line) + newline; | ||
| if (bytes + size > maxBytes) { | ||
| if (kept.every((shown) => shown.length === 0)) { | ||
| kept.push(tailWithinBytes(line, maxBytes - bytes - newline)); |
There was a problem hiding this comment.
Keep blank-line fallback within the byte limit
truncateTail("\n\n\n", 3, 2) keeps two blank lines, then this branch treats the third empty line as an oversized partial line and appends it. The returned text is "\n\n\n" (3 bytes), exceeding maxBytes of 2. Avoid adding a partial line when there is no content to trim, or otherwise ensure the fallback accounts for the newline bytes in the final output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@examples/agent-eval/src/agent/tools/truncate.ts` (line 132, Important):
`truncateTail("\n\n\n", 3, 2)` keeps two blank lines, then this branch treats the third empty line as an oversized partial line and appends it. The returned text is `"\n\n\n"` (3 bytes), exceeding `maxBytes` of 2. Avoid adding a partial line when there is no content to trim, or otherwise ensure the fallback accounts for the newline bytes in the final output.
| if (close === -1) return visible; | ||
| if (close === -1) { | ||
| const dangling = danglingPrefixLength(held, CLOSE); | ||
| think(held.slice(0, held.length - dangling)); |
There was a problem hiding this comment.
Do not parse tool calls inside an unterminated thought
This now routes the body of an unclosed <think> block to reasoning, but Cactus_ToolCallingCore still calls parseNeedleToolCalls(raw), whose stripNeedleReasoning removes only closed think fences. For example, an unterminated <think> containing a complete <tool_call>[{"name":"lookup_weather","arguments":{"city":"Paris"}}]</tool_call> is hidden from the text stream yet still becomes an emitted tool call and can execute. Make the parse path ignore tool-call markup inside an unclosed thought as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@providers/cactus/src/ai/common/Cactus_ParseToolCalls.ts` (line 94, Important):
This now routes the body of an unclosed `<think>` block to reasoning, but `Cactus_ToolCallingCore` still calls `parseNeedleToolCalls(raw)`, whose `stripNeedleReasoning` removes only closed think fences. For example, an unterminated `<think>` containing a complete `<tool_call>[{"name":"lookup_weather","arguments":{"city":"Paris"}}]</tool_call>` is hidden from the text stream yet still becomes an emitted tool call and can execute. Make the parse path ignore tool-call markup inside an unclosed thought as well.
There was a problem hiding this comment.
Review progress ██████████ 167/168 files
Request changes — found 14 issue(s) at 4a348cf.
Actionable comments posted: 14
Coverage is incomplete: reviewed 167/168 files; 0 skipped; 1 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@.github/codeql/codeql-config.yml` (line 8, Important):
`examples/agent-eval` matches the directory path itself, not the files beneath it, so CodeQL will still scan the harness source and report the intended tool/shell findings. Use a recursive pattern such as `examples/agent-eval/**` to exclude the tree.
2. In `@examples/agent-eval/src/agent/tools/fileLock.ts` (line 19, Important):
`join(await realpath(dirname(path)), basename(path))`
If `path` is a dangling symlink (for example, `alias -> target` before `target` exists), `realpath(path)` fails and this fallback keys the lock by `alias`, while a concurrent operation on `target` gets a different key. With `toolConcurrency > 1`, a write through the alias can create the target while an edit/write through the target proceeds without waiting; the operations can race on the same file and an update can be lost. Resolve the dangling symlink target when deriving the fallback key, or otherwise make both names share a lock.
3. In `@providers/cactus/src/ai/common/Cactus_ToolCallingCore.ts` (line 79, Important):
`extractMessageText` drops the assistant message's `tool_use` blocks, while the following tool message contributes only unlabeled result text. When `AgentTask` issues multiple calls in one round (for example, weather lookups for Paris and Rome), the next query sees the original question and values such as `15°C` and `20°C`, but no mapping from either value to its city; the stateless engine can therefore give a swapped or otherwise incorrect comparison. Include the corresponding call name/arguments when passing each result into the query.
Additional findings outside changed lines:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 104, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 291, Blocker):
Redaction misses encoded credentials
` scrubCredentialValues(result.output) ` only replaces the literal credential values. On Linux, a model-controlled command can read `/proc/$PPID/environ` and encode it (for example, `tr '\0' '\n' </proc/$PPID/environ | base64`); the encoded output contains none of the literal secrets, so it is returned to the model and may be persisted in the transcript/spill file, where the credential is trivially recoverable.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
4. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 89, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
5. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 255, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
6. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
7. In `@providers/anthropic/src/ai/common/Anthropic_CacheCheckpoint.ts` (line 93, Blocker):
Do not warm signed turns without thinking enabled
`buildAnthropicCheckpointParams` now replays the prefix's signed thinking blocks, but the resulting request only sets `model` and `max_tokens: 1` and never applies the model's thinking parameters. When a prefix contains a thinking turn (for example, one produced with legacy extended thinking), `messages.create` sends those blocks with thinking disabled and Anthropic rejects the warm-up, so the `CacheCheckpointTask` fails instead of warming the prefix.
8. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 334, Blocker):
Avoid replaying signed thinking when forced choice disables it
`buildAnthropicMessages` replays the prior signed thinking blocks unconditionally for this model key, but on a legacy model with `toolChoice: "required"` (or a named forced choice), `applyAnthropicThinkingParams` deliberately omits `thinking` because Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run.
9. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
10. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
11. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (11) — the lines they point at have not changed:
- 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 104 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Redaction misses encoded credentials ·
examples/agent-eval/src/agent/tools/bash.tsline 291 · froma6d3fc8
scrubCredentialValues(result.output)only replaces the literal credential values. On Linux, a model-controlled command can read/proc/$PPID/environand encode it (for example,tr '\0' '\n' </proc/$PPID/environ | base64); the encoded output contains none of the literal secrets, so it is returned to the model and may be persisted in the transcript/spill file, where the credential is trivially recoverable. - 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 89 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 255 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Do not warm signed turns without thinking enabled ·
providers/anthropic/src/ai/common/Anthropic_CacheCheckpoint.tsline 93 · fromd82a24d
buildAnthropicCheckpointParamsnow replays the prefix's signed thinking blocks, but the resulting request only setsmodelandmax_tokens: 1and never applies the model's thinking parameters. When a prefix contains a thinking turn (for example, one produced with legacy extended thinking),messages.createsends those blocks with thinking disabled and Anthropic rejects the warm-up, so theCacheCheckpointTaskfails instead of warming the prefix. - 🔴 Blocker · Avoid replaying signed thinking when forced choice disables it ·
providers/anthropic/src/ai/common/Anthropic_ToolCalling.tsline 334 · from0a9a5fb
buildAnthropicMessagesreplays the prior signed thinking blocks unconditionally for this model key, but on a legacy model withtoolChoice: "required"(or a named forced choice),applyAnthropicThinkingParamsdeliberately omitsthinkingbecause Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
| # ships. | ||
| name: workglow CodeQL config | ||
| paths-ignore: | ||
| - examples/agent-eval |
There was a problem hiding this comment.
Make the exclusion recursive
examples/agent-eval matches the directory path itself, not the files beneath it, so CodeQL will still scan the harness source and report the intended tool/shell findings. Use a recursive pattern such as examples/agent-eval/** to exclude the tree.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@.github/codeql/codeql-config.yml` (line 8, Important):
`examples/agent-eval` matches the directory path itself, not the files beneath it, so CodeQL will still scan the harness source and report the intended tool/shell findings. Use a recursive pattern such as `examples/agent-eval/**` to exclude the tree.
| } catch { | ||
| // Not there yet (a write creating it): the file it will be, in its real directory. | ||
| try { | ||
| return join(await realpath(dirname(path)), basename(path)); |
There was a problem hiding this comment.
Dangling symlink paths get different lock keys
join(await realpath(dirname(path)), basename(path))
If path is a dangling symlink (for example, alias -> target before target exists), realpath(path) fails and this fallback keys the lock by alias, while a concurrent operation on target gets a different key. With toolConcurrency > 1, a write through the alias can create the target while an edit/write through the target proceeds without waiting; the operations can race on the same file and an update can be lost. Resolve the dangling symlink target when deriving the fallback key, or otherwise make both names share a lock.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@examples/agent-eval/src/agent/tools/fileLock.ts` (line 19, Important):
`join(await realpath(dirname(path)), basename(path))`
If `path` is a dangling symlink (for example, `alias -> target` before `target` exists), `realpath(path)` fails and this fallback keys the lock by `alias`, while a concurrent operation on `target` gets a different key. With `toolConcurrency > 1`, a write through the alias can create the target while an edit/write through the target proceeds without waiting; the operations can race on the same file and an update can be lost. Resolve the dangling symlink target when deriving the fallback key, or otherwise make both names share a lock.
| /** Text the engine should see from one history message after the latest user turn. */ | ||
| function tailMessageText(message: NonNullable<ToolCallingTaskInput["messages"]>[number]): string { | ||
| if (message.role === "tool") return toolResultText(message.content); | ||
| if (message.role === "assistant") return extractMessageText(message.content); |
There was a problem hiding this comment.
Keep tool-call context with multiple results
extractMessageText drops the assistant message's tool_use blocks, while the following tool message contributes only unlabeled result text. When AgentTask issues multiple calls in one round (for example, weather lookups for Paris and Rome), the next query sees the original question and values such as 15°C and 20°C, but no mapping from either value to its city; the stateless engine can therefore give a swapped or otherwise incorrect comparison. Include the corresponding call name/arguments when passing each result into the query.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@providers/cactus/src/ai/common/Cactus_ToolCallingCore.ts` (line 79, Important):
`extractMessageText` drops the assistant message's `tool_use` blocks, while the following tool message contributes only unlabeled result text. When `AgentTask` issues multiple calls in one round (for example, weather lookups for Paris and Rome), the next query sees the original question and values such as `15°C` and `20°C`, but no mapping from either value to its city; the stateless engine can therefore give a swapped or otherwise incorrect comparison. Include the corresponding call name/arguments when passing each result into the query.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
This branch takes
AgentTaskfrom a basic loop to one that competes with opencode and pi on public agent benchmarks. It also brings every tool-calling provider up to what that loop needs.Agent-eval harness (
examples/agent-eval)AgentTask, opencode and pi on Terminal-Bench 2.0, Aider Polyglot and SWE-bench Verified through Harbor. All three arms use the same model, turn cap and reasoning level, and the harness compares them task by task.mock-modelfor offline end-to-end runs, retries for trials the infrastructure lost, and a--tasks-diroption for local task copies.AgentTaskplus four coding tools (read,bash,edit,write):bashdefaults to a 120 s timeout and is capped at the trial deadline.editapplies the edits that match and names the ones that don't.AgentTask loop
ToolExecuteContext.announceTimeLeftshows the model how much time is left, and a retry wait that would pass the deadline ends the turn with"budget".reviewPromptasks the model for one check of its answer before the turn ends.reasoningoutput port. It is held back with the text until each attempt settles, so a failed attempt contributes neither.AgentRoundContextgainsonReasoningDelta.Provider parity
thinking/reasoning_content). Ollama's browser build now reads the conversation, and Ollama also gets athinkeffort setting.error.functionResponse.parts. Older models get them in a separate labelled turn: they reject that field with HTTP 400, and an image placed beside the response leaked their thinking into the answer.reasoning_detailsis replayed between rounds, guarded by call ids. Thinking cut off before its signature is never replayed.<think>text goes to reasoning instead of the answer, and is sent back to the chat template. Tool-result messages carry the tool's name.CLI and web examples
examples/web'sTaskNodeshow reasoning apart from the answer.workglow agent chatand the run-event projection no longer print reasoning as answer text.StreamingTextRowwas only drawn while a task was processing, but the first streamed chunk moves a task into the streaming state, so the panel rarely appeared. It now also draws while streaming, so streamed answers now show in Ink rows.Results
docs/superpowers/evals/andplans/.Downstream
Builder and embarc-data each have a PR on the same branch name that needs this one released first: workglow-dev/builder and sroussey/embarc-data. Both pin
@workglow/*at 0.6.12, so the pins need bumping to the release that carries this.Test plan
ai,task,ai-provider*directories pass: 3,545 tests in the last whole-branch run.tscis clean onpackages/aiand every touched provider; oxlint and oxfmt are clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01Gkqic45a2wtQiXKsKD96x7
Generated by Claude Code